Re: [PATCH 5/8] PCI: rzg3s-host: Add Initial PCIe Host Driver for Renesas RZ/G3S SoC

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, May 09, 2025 at 01:29:40PM +0300, Claudiu Beznea wrote:
> On 05.05.2025 14:26, Claudiu Beznea wrote:
> > On 01.05.2025 23:12, Bjorn Helgaas wrote:
> >> On Wed, Apr 30, 2025 at 01:32:33PM +0300, Claudiu wrote:
> >>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>
> >>> The Renesas RZ/G3S features a PCIe IP that complies with the PCI Express
> >>> Base Specification 4.0 and supports speeds of up to 5 GT/s. It functions
> >>> only as a root complex, with a single-lane (x1) configuration. The
> >>> controller includes Type 1 configuration registers, as well as IP
> >>> specific registers (called AXI registers) required for various adjustments.
> >>>
> >>> Other Renesas RZ SoCs (e.g., RZ/G3E, RZ/V2H) share the same AXI registers
> >>> but have both Root Complex and Endpoint capabilities. As a result, the PCIe
> >>> host driver can be reused for these variants with minimal adjustments.
> ...

> >>> +static void rzg3s_pcie_update_bits(void __iomem *base, u32 offset, u32 mask, u32 val)
> >>> +{
> >>> +	u32 tmp;
> >>> +
> >>> +	tmp = readl(base + offset);
> >>> +	tmp &= ~mask;
> >>> +	tmp |= val & mask;
> >>> +	writel(tmp, base + offset);
> >>> +}
> >>
> >> Nothing rzg3s-specific here.
> >>
> >> I think u32p_replace_bits() (include/linux/bitfield.h) is basically this.
> > 
> > I wasn't aware of it. I'll use it in the next version. Thank for pointing it.
> 
> I look into changing to u32p_replace_bits() but this one needs a mask that
> can be verified at build time. It cannot be used directly in this function.
> Would you prefer me to replace all the calls to rzg3s_pcie_update_bits() with:
> 
> tmp = readl();
> u32p_replace_bits(&tmp, ...)
> writel(tmp);

It seems like this is the prevailing way it's used.

You have ~20 calls, so it seems like it might be excessive to replace
each with readl/u32p_replace_bits/writel.

But maybe you could use u32p_replace_bits() inside
rzg3s_pcie_update_bits().

Bjorn