Re: [PATCH v5 6/6] soc: renesas: Add L2 cache management for RZ/Five SoC

Palmer Dabbelt <palmer@xxxxxxxxxxx> · Thu, 15 Dec 2022 13:40:30 -0800 (PST)

On Thu, 15 Dec 2022 12:17:38 PST (-0800), geert@xxxxxxxxxxxxxx wrote:
Hi Conor,

On Thu, Dec 15, 2022 at 8:54 PM Conor Dooley <conor@xxxxxxxxxx> wrote:
On Thu, Dec 15, 2022 at 05:46:42PM +0000, Lad, Prabhakar wrote:
> On Thu, Dec 15, 2022 at 11:10 AM Geert Uytterhoeven
> <geert@xxxxxxxxxxxxxx> wrote:
> > On Thu, Dec 15, 2022 at 12:06 PM Lad, Prabhakar
> > <prabhakar.csengg@xxxxxxxxx> wrote:
> > > On Thu, Dec 15, 2022 at 10:36 AM Geert Uytterhoeven
> > > <geert@xxxxxxxxxxxxxx> wrote:
> > > > On Mon, Dec 12, 2022 at 12:58 PM Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > > > >
> > > > > I/O Coherence Port (IOCP) provides an AXI interface for connecting
> > > > > external non-caching masters, such as DMA controllers. The accesses
> > > > > from IOCP are coherent with D-Caches and L2 Cache.
> > > > >
> > > > > IOCP is a specification option and is disabled on the Renesas RZ/Five
> > > > > SoC due to this reason IP blocks using DMA will fail.
> > > > >
> > > > > The Andes AX45MP core has a Programmable Physical Memory Attributes (PMA)
> > > > > block that allows dynamic adjustment of memory attributes in the runtime.
> > > > > It contains a configurable amount of PMA entries implemented as CSR
> > > > > registers to control the attributes of memory locations in interest.
> > > > > Below are the memory attributes supported:
> > > > > * Device, Non-bufferable
> > > > > * Device, bufferable
> > > > > * Memory, Non-cacheable, Non-bufferable
> > > > > * Memory, Non-cacheable, Bufferable
> > > > > * Memory, Write-back, No-allocate
> > > > > * Memory, Write-back, Read-allocate
> > > > > * Memory, Write-back, Write-allocate
> > > > > * Memory, Write-back, Read and Write-allocate
> > > > >
> > > > > More info about PMA (section 10.3):
> > > > > Link: http://www.andestech.com/wp-content/uploads/AX45MP-1C-Rev.-5.0.0-Datasheet.pdf
> > > > >
> > > > > As a workaround for SoCs with IOCP disabled CMO needs to be handled by
> > > > > software. Firstly OpenSBI configures the memory region as
> > > > > "Memory, Non-cacheable, Bufferable" and passes this region as a global
> > > > > shared dma pool as a DT node. With DMA_GLOBAL_POOL enabled all DMA
> > > > > allocations happen from this region and synchronization callbacks are
> > > > > implemented to synchronize when doing DMA transactions.
> > > > >
> > > > > Example PMA region passes as a DT node from OpenSBI:
> > > > >     reserved-memory {
> > > > >         #address-cells = <2>;
> > > > >         #size-cells = <2>;
> > > > >         ranges;
> > > > >
> > > > >         pma_resv0@58000000 {
> > > > >             compatible = "shared-dma-pool";
> > > > >             reg = <0x0 0x58000000 0x0 0x08000000>;
> > > > >             no-map;
> > > > >             linux,dma-default;
> > > > >         };
> > > > >     };
> > > > >
> > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > > >
> > > > Thanks for your patch!
> > > >
> > > > >  arch/riscv/include/asm/cacheflush.h       |   8 +
> > > > >  arch/riscv/include/asm/errata_list.h      |  28 ++-
> > > > >  drivers/soc/renesas/Kconfig               |   6 +
> > > > >  drivers/soc/renesas/Makefile              |   2 +
> > > > >  drivers/soc/renesas/rzfive/Kconfig        |   6 +
> > > > >  drivers/soc/renesas/rzfive/Makefile       |   3 +
> > > > >  drivers/soc/renesas/rzfive/ax45mp_cache.c | 256 ++++++++++++++++++++++
> > > >
> > > > Given this touches arch/riscv/include/asm/, I don't think the
> > > > code belongs under drivers/soc/renesas/.
> > > >
> > > Ok. Do you have any suggestions on where you want me to put this code?
> >
> > As it plugs into core riscv functionality, I think it should be under
> > arch/riscv/.
> > if the RISC-V maintainers object to that, another option is
> > drivers/soc/andestech/ or (new) drivers/cache/
> >
> RISC-V maintainers had already made it clear to not to include vendor
> specific stuff in the arch/riscv folder, so I'll consider putting this

We have vendor-specific behavior in arch/riscv now, and it's even in the 
policies (the behavior has been there for a while, the policy is new).  
There's probably a bunch of posts saying otherwise because we used to 
not take vendor-specific behavior, but that's not the case any more.

> into drivers/cache/ folder to sync with the bindings.
>
> Conor/Palmer - do you have any objections/suggestions?

I'm not its maintainer so sorta moot what I say, but having drivers in
arch/riscv makes little sense to me..

Some drivers are pretty tightly coupled to an ISA, I'm thinking of 
things like interrupts/timers where we're writing CSRs.  I could see how 
cache controllers would fall into that category, doubly so as they get 
integrated to the memory model.  That leaves us in sort of a grey area 
and there's certainly ports that have way more drivers in arch so it's 
not an uncommon way to do things.

I generally lean pretty heavily towards keeping things that could at all 
be considered out of arch/riscv and instead have them in some sort of 
driver folder.  That probably results in more total work to do, as we've 
got to have arch/riscv interfaces with one use case and sync up between 
multiple trees sometimes to get stuff merged.  I think it also results 
in better code: being closer to the other drivers makes us more likely 
to get reviewed by folks who understand the space, and it's generally 
easier to share code in the subsystems.

Putting stuff in drivers/cache does sound like a good idea since the
binding is going there too.

The SiFive ccache driver is in drivers/soc and it was suggested to me
this week that there's likely going to be a second SiFive cache driver
at some point in the near future. Plus Microchip are going to have to
add cache management stuff to the existing SiFive ccache driver.
Having them be their own thing makes sense in my mind - especially since
they're not tied to SoCs sold by Andes or SiFive.

I had a quick, and I mean *quick* look through other soc drivers to see
if there were any other cache controller drivers but nothing stood out
to me. Maybe someone else has more of a clue there. Ditto for misc, had
a look but nothing seemed obvious.

Usually they're under arch/:
$ git ls-files -- "arch/*cache*" | wc -l
148
$ git ls-files -- "drivers/*cache*" | wc -l
63

E.g. arch/arm/mm/cache-l2x0.c.

Above all I like to do things as normally as possible, so if most cache 
drivers are in arch then I'm OK with.  Looks like we originally had it 
arch/riscv, though, until 9209fb51896f ("riscv: move sifive_l2_cache.c 
to drivers/soc") moved it out.

I don't really remember this history/rationale here, at the time the 
SiFive cache driver wasn't really doing anything all that exciting: it 
was just way enable and some statistics, we hadn't really planned on 
using it for the non-coherent stuff that folks are now trying to 
retrofit it to handle.  That sort of thing makes it a lot more coupled 
to the ISA.

Given that we already moved the SiFive one out it seems sane to just 
start with the rest in drivers/soc/$VENDOR.  Looks like it was 
Christoph's idea to do the move, so I'm adding him in case he's got an 
opinion (and also the SOC alias, as that seems generally relevant).

Gr{oetje,eeting}s,

                        Geert