Re: [PATCH v8 2/6] PCI: rzg3s-host: Add Renesas RZ/G3S SoC host driver

[Claudiu Beznea <claudiu.beznea@tuxon.dev>]

Hi, Bjorn,

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@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.
> 
>> +/* 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.

This is the quote from the HW manual: "Some registers with the RO attribute
stated in the PCI Express Base Specification are writable at the time of
initialization.
 When writing to these registers, CFG_HWINIT_EN (Permission Register
(offset: H’300) bit[2]) must be set to 1b."

> 
>> +	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 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.

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.

Thank you for your review,
Claudiu