Page MenuHomeFreeBSD

sifive ccache driver
Needs ReviewPublic

Authored by br on Thu, Nov 28, 7:40 PM.

Details

Reviewers
mhorne
jrtc27
Summary

The EIC7700 has non-coherent DMAs but predate the standard RISC-V Zicbom extension, so we need to use the SiFive CCache controller for non-standard cache management operations.
Not sure about file location as this supports EIC7700 only, but could be adopted later to support earlier SiFive SoCs as well.

Test Plan

Tested on SiFive Premier P550
SATA, PCIe and SDHCI only

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

br requested review of this revision.Thu, Nov 28, 7:40 PM
br edited the test plan for this revision. (Show Details)
sys/riscv/sifive/sifive_ccache.c
56

Use a real pointer?

58

You're not really reading/writing pointers

58

Also do these not need to be volatile?

77

This needs to be done per-page; if you cross a page boundary the physical pages may not be contiguous like the virtual ones.

86

Does this really need to be done for every iteration?

126

This is impossible to hit?

extract paddr on each page assuming the range is not contiguous

sys/riscv/sifive/sifive_ccache.c
58

what do you mean?

86

probably not!

mhorne requested changes to this revision.Mon, Dec 2, 9:50 PM

I strongly think this should be a newbus device driver. Unless there is some urgent need that it should be set up at SI_SUB_CPU?

As for the file placement, it is certainly correct. The VisionFive v1/JH7100 used the same workaround for cache flushing, and locally I had something very similar to what is here. I may never finish that board support, but this driver could be used exactly.

As for FU540/FU740, there would be no harm in attaching the device but not installing the flush hooks.

sys/riscv/sifive/sifive_ccache.c
86

LGTM. I overlooked this possibility, and perhaps this could explain why I never got things working 100% on the JH7100. Perhaps I should try again...

This revision now requires changes to proceed.Mon, Dec 2, 9:50 PM

I strongly think this should be a newbus device driver. Unless there is some urgent need that it should be set up at SI_SUB_CPU?

This driver needed before threadinit() otherwise tid_alloc() won't work. It is needed even all cache ways in ccache driver are disabled (however the way 0 is always enabled). May be it flushes other L1/L2/L3 caches I am not sure, but from I saw the freebsd could not use memory that was just allocated in tid_alloc() without a flush. So this should go anywhere before SI_SUB_INTRINSIC.

In D47831#1091595, @br wrote:

I strongly think this should be a newbus device driver. Unless there is some urgent need that it should be set up at SI_SUB_CPU?

This driver needed before threadinit() otherwise tid_alloc() won't work. It is needed even all cache ways in ccache driver are disabled (however the way 0 is always enabled). May be it flushes other L1/L2/L3 caches I am not sure, but from I saw the freebsd could not use memory that was just allocated in tid_alloc() without a flush. So this should go anywhere before SI_SUB_INTRINSIC.

let me actually check this once again tomorrow as I had several issues at the same time and some of them resolved already

I managed to convert the driver to newbus but noticed slower performance of NVME when flushing cache using bus_write_8().

  • direct access to pointer: 97.5MB/s
  • using bus_write_8(): 93.3MB/s

So I provided another mapping for a faster access. WDYT?
another idea is to extract the mapping from sc->res and access it directly?

In D47831#1091983, @br wrote:

I managed to convert the driver to newbus but noticed slower performance of NVME when flushing cache using bus_write_8().

  • direct access to pointer: 97.5MB/s
  • using bus_write_8(): 93.3MB/s

So I provided another mapping for a faster access. WDYT?
another idea is to extract the mapping from sc->res and access it directly?

Wow, that is significant.

I think pulling the mapping from sc->res->r_bushandle is best. This is unusual but a valid use of the API.

sys/riscv/sifive/sifive_ccache.c
129

We expect/require only one instance of this device, so that should be checked here.