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

From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
To: Samuel Holland <samuel@sholland.org>
Cc: Paul Walmsley <paul.walmsley@sifive.com>,
	Palmer Dabbelt <palmer@dabbelt.com>,
	 Albert Ou <aou@eecs.berkeley.edu>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	 Magnus Damm <magnus.damm@gmail.com>,
	Heiko Stuebner <heiko@sntech.de>,
	 Rob Herring <robh+dt@kernel.org>,
	 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	 Conor Dooley <conor.dooley@microchip.com>,
	Guo Ren <guoren@kernel.org>,  Jisheng Zhang <jszhang@kernel.org>,
	Atish Patra <atishp@rivosinc.com>,
	 Anup Patel <apatel@ventanamicro.com>,
	Andrew Jones <ajones@ventanamicro.com>,
	 Nathan Chancellor <nathan@kernel.org>,
	Philipp Tomsich <philipp.tomsich@vrull.eu>,
	devicetree@vger.kernel.org,  linux-kernel@vger.kernel.org,
	linux-riscv@lists.infradead.org,
	 linux-renesas-soc@vger.kernel.org,
	Biju Das <biju.das.jz@bp.renesas.com>,
	 Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Subject: Re: [PATCH v4 7/7] soc: renesas: Add L2 cache management for RZ/Five SoC
Date: Thu, 1 Dec 2022 11:30:08 +0000	[thread overview]
Message-ID: <CA+V-a8uuAqrtVEB1GqFicQjRvkPGpG5788oNSzCp1LNyoUNmXQ@mail.gmail.com> (raw)
In-Reply-To: <565e1861-7052-9bd3-e7ba-e590bd91cf20@sholland.org>

Hi Samuel,

On Tue, Nov 29, 2022 at 5:58 AM Samuel Holland <samuel@sholland.org> wrote:
>
> On 11/26/22 15:09, Lad, Prabhakar wrote:
> >>> +     if (!ax45mp_priv->l2c_base) {
> >>> +             ret = -ENOMEM;
> >>> +             goto l2c_err;
> >>> +     }
> >>> +
> >>> +     ret = ax45mp_configure_l2_cache(np);
> >>> +     if (ret)
> >>> +             goto l2c_err;
> >>> +
> >>> +     ret = ax45mp_configure_pma_regions(np);
> >>> +     if (ret)
> >>> +             goto l2c_err;
> >>> +
> >>> +     static_branch_disable(&ax45mp_l2c_configured);
> >>
> >> Instead of enabling this before the probe function, and disabling it
> >> afterward, just enable it once here, in the success case. Then you can
> >> drop the !ax45mp_priv check in the functions above.
> >>
> > I think I had tried it but static_branch_unlikely() was always returning true.
>
> You use DEFINE_STATIC_KEY_FALSE above, so static_branch_unlikely()
> should return false until you call static_branch_enable().
>
OK, got that.

> >> And none of the functions would get called anyway if the alternative is
> >> not applied. I suppose it's not possible to do some of this probe logic
> >> in the alternative check function?
> >>
> > you mean to check in the vendor errata patch function to see if this
> > driver has probed?
>
> I meant to do the equivalent of:
>
> +     ax45mp_priv->ucctl_ok = ax45mp_cpu_cache_controlable();
> +     ax45mp_priv->l2cache_enabled = ax45mp_cpu_l2c_ctl_status() &
> AX45MP_L2_CACHE_CTL_CEN_MASK;
>
> in the errata function, since that decides if the cache maintenance
> functions actually do anything. But ax45mp_cpu_l2c_ctl_status() gets the
> MMIO address from the DT, and trying to do that from the errata function
> could get ugly, so maybe it is not a good suggestion.
>
Actually I did think about this and the best approach is to do it in
errata only as you suggested. So here's my approach for dropping the
above checks is to introduce vendor specific SBI EXT
(RZFIVE_SBI_EXT_IOCP_SW_WORKAROUND) which will check both the above
conditions and only apply the errata on success and hence avoid the
"if" checks every time in the sync operation.

Cheers,
Prabhakar

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv