[PATCH 0/2] PCI: rzg3s-host: Cleanups

[PATCH 0/2] PCI: rzg3s-host: Cleanups

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Hi,

Series adds cleanups for the Renesas RZ/G3S host controller driver
as discussed in [1].

Thank you,
Claudiu

[1] https://lore.kernel.org/all/20251125183754.GA2755815@bhelgaas/

Claudiu Beznea (2):
  PCI: rzg3s-host: Use pci_generic_config_write() for the root bus
  PCI: rzg3s-host: Drop the lock on RZG3S_PCI_MSIRS and
    RZG3S_PCI_PINTRCVIS

 drivers/pci/controller/pcie-rzg3s-host.c | 34 +++++-------------------
 1 file changed, 7 insertions(+), 27 deletions(-)

-- 
2.43.0

[PATCH 1/2] PCI: rzg3s-host: Use pci_generic_config_write() for the root bus

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The Renesas RZ/G3S host controller allows writing to read-only PCIe
configuration registers when the RZG3S_PCI_PERM_CFG_HWINIT_EN bit is set in
the RZG3S_PCI_PERM register. However, callers of struct pci_ops::write
expect the semantics defined by the PCIe specification, meaning that writes
to read-only registers must not be allowed.

The previous custom struct pci_ops::write implementation for the root bus
temporarily enabled write access before calling pci_generic_config_write().
This breaks the expected semantics.

Remove the custom implementation and simply use pci_generic_config_write().

Along with this change, the updates of the PCI_PRIMARY_BUS,
PCI_SECONDARY_BUS, and PCI_SUBORDINATE_BUS registers were moved so that
they no longer depends on the RZG3S_PCI_PERM_CFG_HWINIT_EN bit in the
RZG3S_PCI_PERM_CFG register, since these registers are R/W.

Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/pci/controller/pcie-rzg3s-host.c | 27 ++++--------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

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;
 }
 
-- 
2.43.0

Re: [PATCH 1/2] PCI: rzg3s-host: Use pci_generic_config_write() for the root bus

[Krishna Chaitanya Chundru]

On 12/5/2025 4:54 PM, Claudiu wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> The Renesas RZ/G3S host controller allows writing to read-only PCIe
> configuration registers when the RZG3S_PCI_PERM_CFG_HWINIT_EN bit is set in
> the RZG3S_PCI_PERM register. However, callers of struct pci_ops::write
> expect the semantics defined by the PCIe specification, meaning that writes
> to read-only registers must not be allowed.
>
> The previous custom struct pci_ops::write implementation for the root bus
> temporarily enabled write access before calling pci_generic_config_write().
> This breaks the expected semantics.
>
> Remove the custom implementation and simply use pci_generic_config_write().
>
> Along with this change, the updates of the PCI_PRIMARY_BUS,
> PCI_SECONDARY_BUS, and PCI_SUBORDINATE_BUS registers were moved so that
> they no longer depends on the RZG3S_PCI_PERM_CFG_HWINIT_EN bit in the
> RZG3S_PCI_PERM_CFG register, since these registers are R/W.
>
Don't you need fixes tag and back port to stable kernels, this patch 
looks like a bug fix.

- Krishna Chaitanya.
> Suggested-by: Bjorn Helgaas <helgaas@kernel.org>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> ---
>   drivers/pci/controller/pcie-rzg3s-host.c | 27 ++++--------------------
>   1 file changed, 4 insertions(+), 23 deletions(-)
>
> 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;
>   }
>   

Re: [PATCH 1/2] PCI: rzg3s-host: Use pci_generic_config_write() for the root bus

[Claudiu Beznea]

Hi, Krishna,

On 12/6/25 03:29, Krishna Chaitanya Chundru wrote:
> 
> On 12/5/2025 4:54 PM, Claudiu wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> The Renesas RZ/G3S host controller allows writing to read-only PCIe
>> configuration registers when the RZG3S_PCI_PERM_CFG_HWINIT_EN bit is set in
>> the RZG3S_PCI_PERM register. However, callers of struct pci_ops::write
>> expect the semantics defined by the PCIe specification, meaning that writes
>> to read-only registers must not be allowed.
>>
>> The previous custom struct pci_ops::write implementation for the root bus
>> temporarily enabled write access before calling pci_generic_config_write().
>> This breaks the expected semantics.
>>
>> Remove the custom implementation and simply use pci_generic_config_write().
>>
>> Along with this change, the updates of the PCI_PRIMARY_BUS,
>> PCI_SECONDARY_BUS, and PCI_SUBORDINATE_BUS registers were moved so that
>> they no longer depends on the RZG3S_PCI_PERM_CFG_HWINIT_EN bit in the
>> RZG3S_PCI_PERM_CFG register, since these registers are R/W.
>>
> Don't you need fixes tag and back port to stable kernels, this patch looks
> like a bug fix.

I consider it was not a functional fix. The driver was just accepted for
v6.19 and though it will be included before the pull request is issued. Due
to this I haven't added it. If any, I'm adding the tag here.

Fixes: 7ef502fb35b2 ("PCI: Add Renesas RZ/G3S host controller driver")

Thank you,
Claudiu

[PATCH 2/2] PCI: rzg3s-host: Drop the lock on RZG3S_PCI_MSIRS and RZG3S_PCI_PINTRCVIS

[Claudiu]

From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The RZG3S_PCI_MSIRS and RZG3S_PCI_PINTRCVIS registers are of the R/W1C
type. According to the RZ/G3S HW Manual, Rev. 1.10, chapter 34.2.1
Register Type, R/W1C register bits are cleared to 0b by writing 1b, while
writing 0b has no effect. Therefore, there is no need to take a lock
around writes to these registers.

Drop the locking.

Along with this, add a note about the R/W1C register type to the register
offset definitions.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/pci/controller/pcie-rzg3s-host.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/pcie-rzg3s-host.c b/drivers/pci/controller/pcie-rzg3s-host.c
index 547cbe676a25..d08a63d89452 100644
--- a/drivers/pci/controller/pcie-rzg3s-host.c
+++ b/drivers/pci/controller/pcie-rzg3s-host.c
@@ -73,6 +73,7 @@
 #define RZG3S_PCI_PINTRCVIE_INTX(i)		BIT(i)
 #define RZG3S_PCI_PINTRCVIE_MSI			BIT(4)
 
+/* Register is R/W1C, it doesn't require locking. */
 #define RZG3S_PCI_PINTRCVIS			0x114
 #define RZG3S_PCI_PINTRCVIS_INTX(i)		BIT(i)
 #define RZG3S_PCI_PINTRCVIS_MSI			BIT(4)
@@ -114,6 +115,8 @@
 #define RZG3S_PCI_MSIRE_ENA			BIT(0)
 
 #define RZG3S_PCI_MSIRM(id)			(0x608 + (id) * 0x10)
+
+/* Register is R/W1C, it doesn't require locking. */
 #define RZG3S_PCI_MSIRS(id)			(0x60c + (id) * 0x10)
 
 #define RZG3S_PCI_AWBASEL(id)			(0x1000 + (id) * 0x20)
@@ -507,8 +510,6 @@ static void rzg3s_pcie_msi_irq_ack(struct irq_data *d)
 	u8 reg_bit = d->hwirq % RZG3S_PCI_MSI_INT_PER_REG;
 	u8 reg_id = d->hwirq / RZG3S_PCI_MSI_INT_PER_REG;
 
-	guard(raw_spinlock_irqsave)(&host->hw_lock);
-
 	writel_relaxed(BIT(reg_bit), host->axi + RZG3S_PCI_MSIRS(reg_id));
 }
 
@@ -840,8 +841,6 @@ static void rzg3s_pcie_intx_irq_ack(struct irq_data *d)
 {
 	struct rzg3s_pcie_host *host = irq_data_get_irq_chip_data(d);
 
-	guard(raw_spinlock_irqsave)(&host->hw_lock);
-
 	rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_PINTRCVIS,
 			       RZG3S_PCI_PINTRCVIS_INTX(d->hwirq),
 			       RZG3S_PCI_PINTRCVIS_INTX(d->hwirq));
-- 
2.43.0