search

SpinalHDL / VexRiscv

A FPGA friendly 32 bit RISC-V CPU implementation
MIT License
2.52k stars 420 forks source link

CFU variants in pythondata-cpu-vexriscv are broken due to CSR clash at 0xBC0 #258

Open tcal-x opened 2 years ago

tcal-x commented 2 years ago

At the VexRiscv commit used by pythondata-cpu-vexriscv to build the verilogs, there is a clash in the use of CSR 0xBC0, used both by CfuPlugin for its state/index/enable register, and by the external interrupt mask: https://github.com/SpinalHDL/VexRiscv/search?q=BC0, https://github.com/enjoy-digital/litex/search?q=BC0.

The problem is that the external CFU cannot be used unless the external CFU enable bit is set, but this bit is often cleared by the external interrupt code.

There is a workaround on the VexRiscv dev branch, to optionally not implement the CFU enable bit (i.e. have the external CFU always implicitly enabled), but that commit is not on VexRiscv master branch and so can't be used by pythondata-cpu-vexriscv.

mithro commented 2 years ago

Can we just change the CSR address?

tcal-x commented 2 years ago

Can we just change the CSR address?

I'd discussed this a bit with Charles. He was wary of changing the address of the external interrupt mask, since although the address was originally chosen arbitrarily, changing it now would IIRC break some pre-built Linux SMP binaries and other assets. Changing the irq CSR would also require changes in LiteX, so changing it even locally in CFU Playground would be difficult since I don't want to fork the litex repo.

I'm wary of changing the CFU CSR address upstream since this is directly from the Composable CFU Spec.

Changing the CFU CSR address locally in CFU Playground is a possible way forward, and I can make that change immediately.

mithro commented 2 years ago

@tcal-x - I think if the CSR address is hard coded in the Composable CFU Spec, then that spec has an issue because of the above conflict?

Dolu1990 commented 2 years ago

There is a workaround on the VexRiscv dev branch, to optionally not implement the CFU enable bit (i.e. have the external CFU always implicitly enabled), but that commit is not on VexRiscv master branch and so can't be used by pythondata-cpu-vexriscv.

@tcal-x I merged very recently VexRiscv dev into master : ) https://github.com/SpinalHDL/VexRiscv/commit/24795ef09b88defe2ee1bb335e5caaf7e07e64ff

It has the feature in ^^

Dolu1990 commented 2 years ago

Long term i guess we can programaticaly change the interrupt CSR id when CFU is enabled, add a litex generated #define to specify that interrupt CSR id from the python, patch the bios to use that #define ?

mithro commented 2 years ago

@Dolu1990 - Sounds like a plan?

tcal-x commented 2 years ago

(previous discussion: https://github.com/google/CFU-Playground/issues/596)

Ok, here is the plan:

1) bump spinalHDL/vexriscv submodule in pythondata-cpu-vexriscv to get the workaround, and rebuild the CFU variants with the workaround (CFU.en is removed, i.e. the external CFU interface is always enabled). This restores previous functionality.

2) I can then safely bump all Litex dependencies in CFU Playground.

3) In LiteX, make the interrupt CSR address an SoC #define CONFIG value; update all CPU's csr-defs.h to use this variable.

4) Then, we can update the Vex build script so that when CFU is enabled, the interrupt CSR is moved to a different address (the user can explicitly set it to some value other than BC0, and if they don't, the script will choose a new arbitrary address). Then, again rebuild the CFU variants in pythondata-cpu-vexriscv, but now we can also keep CFU.enable so that we more closely match the composable CFU spec.

Edit: wait, I need to consider how this interacts with the VexRiscv pre-built Verilogs, and how we can make sure the Vex build script and the LiteX genereation agree on the interrupt CSR address. Does LiteX scan the VexRiscv .yaml file for the variant?

Dolu1990 commented 2 years ago

Edit: wait, I need to consider how this interacts with the VexRiscv pre-built Verilogs, and how we can make sure the Vex build script and the LiteX genereation agree on the interrupt CSR address. Does LiteX scan the VexRiscv .yaml file for the variant? the user can explicitly set it to some value other than BC0

That one reason to opt for a static CSR id instead of a configurable one :)

Also, i just thinked about it, but that would also impact the litex linux interrupt driver for VexRiscv in the case you also want CFU :( I guess for now we can just ignore that.

What's a mess XD

Note i will be AFK for the next 10 days (holidays)

mithro commented 2 years ago

Shouldn't the Linux interrupt driver be looking up the address in the provided devicetree?

Dolu1990 commented 2 years ago

Ahh by CSR we mean RISC-V CSR, not memory mapped CSR, so not a "reg" field in the device tree.

tcal-x commented 2 years ago

The first two steps above have been completed:

  1. https://github.com/litex-hub/pythondata-cpu-vexriscv/pull/18.

  2. https://github.com/google/CFU-Playground/pull/619