Re: [RFC v5.1 9/9] [DON'T APPLY] cache: sifive-ccache: add cache flushing capability

From: Conor Dooley <conor@kernel.org>
To: Arnd Bergmann <arnd@arndb.de>, palmer@dabbelt.com
Cc: "Palmer Dabbelt" <palmer@dabbelt.com>,
	Prabhakar <prabhakar.csengg@gmail.com>,
	"Conor.Dooley" <conor.dooley@microchip.com>,
	"Andrew Jones" <ajones@ventanamicro.com>,
	"Albert Ou" <aou@eecs.berkeley.edu>,
	"Anup Patel" <apatel@ventanamicro.com>,
	"Atish Patra" <atishp@rivosinc.com>,
	"Biju Das" <biju.das.jz@bp.renesas.com>,
	devicetree@vger.kernel.org,
	"Geert Uytterhoeven" <geert@linux-m68k.org>,
	guoren <guoren@kernel.org>,
	"Christoph Hellwig" <hch@infradead.org>,
	"Heiko Stübner" <heiko@sntech.de>,
	"Jisheng Zhang" <jszhang@kernel.org>,
	krzysztof.kozlowski+dt@linaro.org, linux-kernel@vger.kernel.org,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	linux-riscv@lists.infradead.org,
	"Magnus Damm" <magnus.damm@gmail.com>,
	"Nathan Chancellor" <nathan@kernel.org>,
	"Paul Walmsley" <paul.walmsley@sifive.com>,
	"Philipp Tomsich" <philipp.tomsich@vrull.eu>,
	"Lad, Prabhakar" <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Samuel Holland" <samuel@sholland.org>,
	soc@kernel.org, "Daire McNamara" <daire.mcnamara@microchip.com>
Subject: Re: [RFC v5.1 9/9] [DON'T APPLY] cache: sifive-ccache: add cache flushing capability
Date: Wed, 4 Jan 2023 13:20:13 +0000	[thread overview]
Message-ID: <Y7V9Debcc9lqWBmT@spud> (raw)
In-Reply-To: <1b7d4caa-2c9c-4aef-81ac-47288d3a652c@app.fastmail.com>

[-- Attachment #1: Type: text/plain, Size: 3018 bytes --]

On Wed, Jan 04, 2023 at 01:18:45PM +0100, Arnd Bergmann wrote:
> On Wed, Jan 4, 2023, at 12:56, Conor Dooley wrote:
> > On Wed, Jan 04, 2023 at 11:19:44AM +0100, Arnd Bergmann wrote:
> >> On Wed, Jan 4, 2023, at 10:23, Conor Dooley wrote:
> >> I would try to replace both of these indirections and instead
> >> handle it all from C code in arch_sync_dma_for_device() directly,
> >> for the purpose of readability and maintainability.
> >> static inline void dma_cache_clean(void *vaddr, size_t size)
> >> {
> >>         if (!cache_maint_ops.clean)
> >>                zicbom_cache_clean(vaddr, size, riscv_cbom_block_size);
> >
> > And I figure that this function is effectively a wrapper around ALT_CMO_OP()?
> >
> >>         else
> >>                cache_maint_ops.clean(vaddr, size, riscv_cbom_block_size);
> >
> > And this one gets registered by the driver using an interface like the
> > one I already proposed, just with the cache_maint_ops struct expanded?
> 
> Yes, exactly.
> 
> > Extrapolating, with these changes having an errata would not even be
> > needed in order to do cache maintenance.
> > Since the ALT_CMO_OP() version would only be used inside
> > zicbom_cache_clean(), assuming I understood correctly, a driver could
> > just register cache_maint_ops for a given platform without having to
> > muck around with errata.
> 
> That is the idea, and ALT_CMO_OP() itself can just go away
> as by just putting the inline asm without the alternative into
> the zicbom_cache_clean() version, making the THEAD branch yet
> another cache_maint_ops instance.

Perhaps more of a question for Palmer than you, but how about leaving
ALT_CMO_OP as-is in riscv/for-next at the moment, wrapping it in
zicbom_cache_foo() & leaving that extraction for a follow-on work?
There's another conversation going on about expanding the THEAD stuff,
so that could be done on top of Prabhakar's v6.

That series is here:
https://lore.kernel.org/linux-riscv/CAJF2gTQp1bOp9kfoOkbvNnSXQhzrCpG3rn8C+LPPoJtMCCDOdA@mail.gmail.com/T/#t
Although unfortunately Icenowy is having issues getting their patches to
the lists so I assume it'll get let through at some point today.

> >> which then makes it very clear what the actual code path
> >> is, while leaving the zicbom case free of indirect function
> >> calls. You can still use a static_branch() to optimize the
> >> conditional, but I would try to avoid any extra indirection
> >> levels or errata checks.
> >
> > The other thing that I like about this is we can then remove the various
> > calls to ALT_CMO_OP() that are scattered around arch/riscv now & replace
> > them with functions that have more understandable names.
> 
> I only see them in arch/riscv/mm/dma-noncoherent.c and arch/riscv/mm/pmem.c,
> but yes, both of these should just call the new functions, whatever the
> calling conventions end up being.

Dunno why I had it in my head there was a third place. Seeing ghosts
maybe!

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]