Message-ID: <20251126191940.GA2831320@bhelgaas>
Date: Wed, 26 Nov 2025 13:19:40 -0600
From: Bjorn Helgaas <helgaas@...nel.org>
To: Claudiu Beznea <claudiu.beznea@...on.dev>
Cc: bhelgaas@...gle.com, lpieralisi@...nel.org, kwilczynski@...nel.org,
	mani@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, geert+renesas@...der.be, magnus.damm@...il.com,
	p.zabel@...gutronix.de, linux-pci@...r.kernel.org,
	linux-renesas-soc@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Claudiu Beznea <claudiu.beznea.uj@...renesas.com>,
	Wolfram Sang <wsa+renesas@...g-engineering.com>
Subject: Re: [PATCH v8 2/6] PCI: rzg3s-host: Add Renesas RZ/G3S SoC host
 driver

On Wed, Nov 26, 2025 at 07:22:09PM +0200, Claudiu Beznea wrote:
> On 11/25/25 20:37, Bjorn Helgaas wrote:
> > On Wed, Nov 19, 2025 at 04:35:19PM +0200, Claudiu wrote:
> >> From: Claudiu Beznea <claudiu.beznea.uj@...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.
> > 
> >> +/* Serialization is provided by 'pci_lock' in drivers/pci/access.c */
> >> +static int rzg3s_pcie_root_write(struct pci_bus *bus, unsigned int devfn,
> >> +				 int where, int size, u32 val)
> >> +{
> >> +	struct rzg3s_pcie_host *host = bus->sysdata;
> >> +	int ret;
> >> +
> >> +	/* Enable access control to the CFGU */
> >> +	writel_relaxed(RZG3S_PCI_PERM_CFG_HWINIT_EN,
> >> +		       host->axi + RZG3S_PCI_PERM);
> > 
> > I suppose this has been asked and answered already, but it's curious
> > that you need this for config writes but not for reads.  Obviously it
> > must *work*, but it's unusual and might warrant a comment.  "Access
> > control" must be a hint, but only means something to experts.
> 
> After initialization, some PCI registers are read only. To enable write
> access to these registers after initialization, the access control need to
> be enabled.

rzg3s_pcie_root_write() is a config accessor.  All the users of this
expect the semantics documented by the PCI/PCIe spec.  Registers
documented as read-only *should* be read-only in those situations.

BIOS and native host bridge drivers like this one may need to
initialize registers that are read-only to the OS.  Some hardware
supports that initialization via device-specific addresses outside of
normal config space.

It looks like this hardware supports that initialization using the
PCI/PCIe-documented config addresses, but uses RZG3S_PCI_PERM to
control whether things are read-only or not.  It's used that way in
rzg3s_pcie_host_init_port(), rzg3s_pcie_config_init(), etc.

That's perfectly fine, as long as this all happens before the OS sees
the devices.  If writes to read-only registers happen *after* the OS
starts enumerating devices, then we have a problem because the OS may
cache the values of read-only registers.

Bottom line, I think:

  - rzg3s_pcie_root_write() should *not* use RZG3S_PCI_PERM at all.
    In fact, it should not exist, and rzg3s_pcie_root_ops.write should
    be pci_generic_config_write().

  - rzg3s_pcie_config_init() should use RZG3S_PCI_PERM to update
    RZG3S_PCI_CFG_BARMSK00, but should *not* need it to update the
    bus info because those registers are read/write per spec.

The other users look like they do need RZG3S_PCI_PERM to initialize
the Vendor and Device IDs (which are read-only per spec) and PHY
registers (which are not part of the programming model documented by
the PCIe spec).

rzg3s_pcie_config_init() looks like a problem because it's called from
rzg3s_pcie_resume_noirq(), which happens after the OS owns the PCI
hardware.  The OS may have updated the bus numbers, and
rzg3s_pcie_config_init() will overwrite them with whatever it got from
DT.  With a single Root Port, it's not *likely* that the OS would
change the secondary or subordinate bus numbers, but you can't assume
they are fixed.

I don't know if the hardware loses the bus numbers when it is
suspended.  If it does, I think you would need to capture them during
suspend, save them somewhere like rzg3s_pcie_port, and restore them
during resume.

You should be able to verify that this is a problem by booting the
kernel, decreasing the subordinate bus number via setpci, suspending
and resuming, and checking the subordinate bus number with lspci.  I
think you'll see that setpci update lost.

> >> +static irqreturn_t rzg3s_pcie_msi_irq(int irq, void *data)
> >> +{
> >> +	u8 regs = RZG3S_PCI_MSI_INT_NR / RZG3S_PCI_MSI_INT_PER_REG;
> >> +	DECLARE_BITMAP(bitmap, RZG3S_PCI_MSI_INT_NR);
> >> +	struct rzg3s_pcie_host *host = data;
> >> +	struct rzg3s_pcie_msi *msi = &host->msi;
> >> +	unsigned long bit;
> >> +	u32 status;
> >> +
> >> +	status = readl_relaxed(host->axi + RZG3S_PCI_PINTRCVIS);
> >> +	if (!(status & RZG3S_PCI_PINTRCVIS_MSI))
> >> +		return IRQ_NONE;
> >> +
> >> +	/* Clear the MSI */
> >> +	rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_PINTRCVIS,
> >> +			       RZG3S_PCI_PINTRCVIS_MSI,
> >> +			       RZG3S_PCI_PINTRCVIS_MSI);
> > 
> > Other writes to RZG3S_PCI_PINTRCVIS are guarded by host->hw_lock.  Is this
> > one safe without it?
> 
> It should be safe as RZG3S_PCI_PINTRCVIS is a R/W1C type of register.
> 
> HW manual describes R/W1C registers for PCIe as "Write-1-to-clear status
> . It can be cleared to 0b by writing 1b with a readable register.
>  Writing 0b does not change anything."
> 
> With this, it should be safe to drop the guard from rzg3s_pcie_intx_irq_ack().
> 
> > 
> >> +	rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_MSGRCVIS,
> >> +			       RZG3S_PCI_MSGRCVIS_MRI, RZG3S_PCI_MSGRCVIS_MRI);
> >> +
> >> +	for (u8 reg_id = 0; reg_id < regs; reg_id++) {
> >> +		status = readl_relaxed(host->axi + RZG3S_PCI_MSIRS(reg_id));
> >> +		bitmap_write(bitmap, status, reg_id * RZG3S_PCI_MSI_INT_PER_REG,
> >> +			     RZG3S_PCI_MSI_INT_PER_REG);
> >> +	}
> >> +
> >> +	for_each_set_bit(bit, bitmap, RZG3S_PCI_MSI_INT_NR) {
> >> +		int ret;
> >> +
> >> +		ret = generic_handle_domain_irq(msi->domain, bit);
> >> +		if (ret) {
> >> +			u8 reg_bit = bit % RZG3S_PCI_MSI_INT_PER_REG;
> >> +			u8 reg_id = bit / RZG3S_PCI_MSI_INT_PER_REG;
> >> +
> >> +			/* Unknown MSI, just clear it */
> >> +			writel_relaxed(BIT(reg_bit),
> >> +				       host->axi + RZG3S_PCI_MSIRS(reg_id));
> > 
> > Other writes to RZG3S_PCI_MSIRS are guarded by host->hw_lock.  Is this
> > one safe without it?
> 
> RZG3S_PCI_MSIRS is also a R/W1C type of register. With it, it should be
> safe to drop the guard from rzg3s_pcie_msi_irq_ack() as well.
> 
> I'm going to prepare a follow up patch to drop the guard on
> rzg3s_pcie_intx_irq_ack() and rzg3s_pcie_msi_irq_ack(). Please let me know
> if you have something against.

Sounds good.  Maybe add a comment at RZG3S_PCI_PINTRCVIS and
RZG3S_PCI_MSIRS about them being R/W1C as a hint that they don't need
locking.

> I can also prepare a patch to detail in a comment the "enable access
> control to the CFGU" operation in rzg3s_pcie_root_write(), if you prefer.

I think you should do the patch below.

And also investigate the question about resume and the bus numbers.
If it is an issue, you'll have to figure out how to fix that.

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index 667e6d629474..547cbe676a25 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -439,28 +439,9 @@ static void __iomem *rzg3s_pcie_root_map_bus(struct pci_bus *bus,
 	return host->pcie + where;
 }
 
-/* Serialized by 'pci_lock' */
-static int rzg3s_pcie_root_write(struct pci_bus *bus, unsigned int devfn,
-				 int where, int size, u32 val)
-{
-	struct rzg3s_pcie_host *host = bus->sysdata;
-	int ret;
-
-	/* Enable access control to the CFGU */
-	writel_relaxed(RZG3S_PCI_PERM_CFG_HWINIT_EN,
-		       host->axi + RZG3S_PCI_PERM);
-
-	ret = pci_generic_config_write(bus, devfn, where, size, val);
-
-	/* Disable access control to the CFGU */
-	writel_relaxed(0, host->axi + RZG3S_PCI_PERM);
-
-	return ret;
-}
-
 static struct pci_ops rzg3s_pcie_root_ops = {
 	.read		= pci_generic_config_read,
-	.write		= rzg3s_pcie_root_write,
+	.write		= pci_generic_config_write,
 	.map_bus	= rzg3s_pcie_root_map_bus,
 };
 
@@ -1065,14 +1046,14 @@ static int rzg3s_pcie_config_init(struct rzg3s_pcie_host *host)
 	writel_relaxed(0xffffffff, host->pcie + RZG3S_PCI_CFG_BARMSK00L);
 	writel_relaxed(0xffffffff, host->pcie + RZG3S_PCI_CFG_BARMSK00U);
 
+	/* Disable access control to the CFGU */
+	writel_relaxed(0, host->axi + RZG3S_PCI_PERM);
+
 	/* Update bus info */
 	writeb_relaxed(primary_bus, host->pcie + PCI_PRIMARY_BUS);
 	writeb_relaxed(secondary_bus, host->pcie + PCI_SECONDARY_BUS);
 	writeb_relaxed(subordinate_bus, host->pcie + PCI_SUBORDINATE_BUS);
 
-	/* Disable access control to the CFGU */
-	writel_relaxed(0, host->axi + RZG3S_PCI_PERM);
-
 	return 0;
 }
 

Powered by blists - more mailing lists