Message-ID: <ca1da9cb-5003-49f2-ab8d-70b80a10d8cd@tuxon.dev>
Date: Fri, 19 Sep 2025 12:28:10 +0300
From: Claudiu Beznea <claudiu.beznea@...on.dev>
To: Manivannan Sadhasivam <mani@...nel.org>
Cc: bhelgaas@...gle.com, lpieralisi@...nel.org, kwilczynski@...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 v4 2/6] PCI: rzg3s-host: Add Renesas RZ/G3S SoC host
 driver



On 9/19/25 11:45, Manivannan Sadhasivam wrote:
> On Fri, Sep 12, 2025 at 03:24:40PM +0300, 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.
>>
>> Hardware manual can be downloaded from the address in the "Link" section.
>> The following steps should be followed to access the manual:
>> 1/ Click the "User Manual" button
>> 2/ Click "Confirm"; this will start downloading an archive
>> 3/ Open the downloaded archive
>> 4/ Navigate to r01uh1014ej*-rzg3s-users-manual-hardware -> Deliverables
>> 5/ Open the file r01uh1014ej*-rzg3s.pdf
>>
>> Link: https://www.renesas.com/en/products/rz-g3s?queryID=695cc067c2d89e3f271d43656ede4d12
>> Tested-by: Wolfram Sang <wsa+renesas@...g-engineering.com>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
>> ---
>>
> 
> [...]
> 
>> +static void rzg3s_pcie_update_bits(void __iomem *base, u32 offset, u32 mask,
>> +				   u32 val)
>> +{
>> +	u32 tmp;
>> +
>> +	tmp = readl(base + offset);
> 
> Unless there is an ordering requirement, you can safely use
> {readl/writel}_relaxed variants throughout the driver.

HW manual lists specific steps to follow when issuing requests. These steps
are listed in chapter "34.4.2.4 Issuing Special Requests" in the manual
pointed in patch description.

> 
>> +	tmp &= ~mask;
>> +	tmp |= val & mask;
>> +	writel(tmp, base + offset);
>> +}
>> +
> 
> [...]
> 
>> +static void __iomem *rzg3s_pcie_child_map_bus(struct pci_bus *bus,
>> +					      unsigned int devfn,
>> +					      int where)
>> +{
>> +	struct rzg3s_pcie_host *host = bus->sysdata;
>> +	unsigned int dev, func, reg;
>> +
>> +	dev = PCI_SLOT(devfn);
>> +	func = PCI_FUNC(devfn);
>> +	reg = where & ~0x3;
>> +
>> +	/* Set the destination */
>> +	writel(FIELD_PREP(RZG3S_PCI_REQADR1_BUS, bus->number) |
>> +	       FIELD_PREP(RZG3S_PCI_REQADR1_DEV, dev) |
>> +	       FIELD_PREP(RZG3S_PCI_REQADR1_FUNC, func) |
>> +	       FIELD_PREP(RZG3S_PCI_REQADR1_REG, reg),
>> +	       host->axi + RZG3S_PCI_REQADR1);
>> +
>> +	/* Set byte enable */
>> +	writel(RZG3S_PCI_REQBE_BYTE_EN, host->axi + RZG3S_PCI_REQBE);
>> +
>> +	/*
>> +	 * rzg3s_pcie_child_map_bus() is used to configure the controller before
>> +	 * executing requests. It is called only within this driver and not
>> +	 * through subsystem calls. Since it does not return an address that
>> +	 * needs to be used later, return NULL.
>> +	 */
> 
> What guarantees that the PCI core will not call this function through
> pci_ops::map_bus?

As of my code inspection the pci_ops::map_bus is currently called from:
pci_generic_config_read()
pci_generic_config_write()
pci_generic_config_read32()
pci_generic_config_write32()

As of my code inspection, these are currently called from vendor specific
drivers. I the core behavior will be changed, I can't guarantee the
statement from the comment. Please let me know if you want me to drop the
initialization of rzg3s_pcie_child_ops::map_bus and call
rzg3s_pcie_child_map_bus() explicitly instead of calling it though
rzg3s_pcie_child_ops::map_bus

As mentioned in the previous review rounds, this is implemented like this
as it was suggested in v1 review process.

> 
>> +	return NULL;
>> +}
>> +
>> +static struct pci_ops rzg3s_pcie_child_ops = {
>> +	.read		= rzg3s_pcie_child_read,
>> +	.write		= rzg3s_pcie_child_write,
>> +	.map_bus	= rzg3s_pcie_child_map_bus,
>> +};
>> +
>> +static void __iomem *rzg3s_pcie_root_map_bus(struct pci_bus *bus,
>> +					     unsigned int devfn,
>> +					     int where)
>> +{
>> +	struct rzg3s_pcie_host *host = bus->sysdata;
>> +
>> +	if (devfn)
>> +		return NULL;
>> +
>> +	return host->pcie + where;
>> +}
>> +
>> +/* 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;
>> +
>> +	/* Enable access control to the CFGU */
>> +	writel(RZG3S_PCI_PERM_CFG_HWINIT_EN, host->axi + RZG3S_PCI_PERM);
>> +
> 
> I'm not sure if 'host->axi' written above and the address written below are in
> the same domain or not. 

host->axi and host->pci are both part of the PCI controller address space.
I don't have more info on it than this. HW manual don't mention anything
about this.

> If they are, then the writes will be serialized and
> would be no issues. If they are in different domains, then you would need to do
> readl() to make sure the above write reaches the hardware before writing below.
> 
>> +	pci_generic_config_write(bus, devfn, where, size, val);
>> +
>> +	/* Disable access control to the CFGU */
>> +	writel(0, host->axi + RZG3S_PCI_PERM);
>> +
>> +	return PCIBIOS_SUCCESSFUL;
>> +}
>> +
>> +static struct pci_ops rzg3s_pcie_root_ops = {
>> +	.read		= pci_generic_config_read,
>> +	.write		= rzg3s_pcie_root_write,
>> +	.map_bus	= rzg3s_pcie_root_map_bus,
>> +};
>> +
> 
> [...]
> 
>> +static int rzg3s_pcie_intx_setup(struct rzg3s_pcie_host *host)
>> +{
>> +	struct device *dev = host->dev;
>> +
>> +	for (int i = 0; i < PCI_NUM_INTX; i++) {
>> +		struct platform_device *pdev = to_platform_device(dev);
>> +		char irq_name[5] = {0};
>> +		int irq;
>> +
>> +		scnprintf(irq_name, ARRAY_SIZE(irq_name), "int%c", 'a' + i);
>> +
>> +		irq = platform_get_irq_byname(pdev, irq_name);
>> +		if (irq < 0)
>> +			return dev_err_probe(dev, -EINVAL,
>> +					     "Failed to parse and map INT%c IRQ\n",
>> +					     'A' + i);
>> +
>> +		host->intx_irqs[i] = irq;
>> +		irq_set_chained_handler_and_data(irq,
>> +						 rzg3s_pcie_intx_irq_handler,
>> +						 host);
>> +	}
>> +
>> +	host->intx_domain = irq_domain_create_linear(of_fwnode_handle(dev->of_node),
>> +						     PCI_NUM_INTX,
>> +						     &rzg3s_pcie_intx_domain_ops,
>> +						     host);
>> +	if (!host->intx_domain)
>> +		return dev_err_probe(dev, -EINVAL,
>> +				     "Failed to add irq domain for INTx IRQs\n");
>> +	irq_domain_update_bus_token(host->intx_domain, DOMAIN_BUS_WIRED);
>> +
>> +	return devm_add_action_or_reset(dev, rzg3s_pcie_intx_teardown, host);
> 
> Didn't I suggest dropping these devm_add_action_or_reset() calls and use error
> labels as like other controller drivers?

It has been mentioned like "It is generally preferred to cleanup the
resources in err path using goto labels."; thorough "generally preferred" I
understood this as a non-strict rule, thus I asked back if you prefer it
for this driver as well, but got no further reply. Sorry for any confusion,
if any.

But before posting this version I also prepared a version that drops the
devm actions or resets and uses gotos instead. I'll send it in reply to
this patch for you to check it. I personally consider it complicates the
failure path. Please let me know your thoughts.

> 
>> +}
>> +
> 
> [...]
> 
>> +static struct platform_driver rzg3s_pcie_driver = {
>> +	.driver = {
>> +		.name = "rzg3s-pcie-host",
>> +		.of_match_table = rzg3s_pcie_of_match,
>> +		.pm = pm_ptr(&rzg3s_pcie_pm_ops),
>> +		.suppress_bind_attrs = true,
>> +	},
>> +	.probe = rzg3s_pcie_probe,
> 
> You could use '.probe_type = PROBE_PREFER_ASYNCHRONOUS' to allow async probing
> of the devices. This will have a big impact in boot time if you have multiple
> controllers.

Thank you for the hint. I'll look to it.

Thank you for your review,
Claudiu

> 
> - Mani
> 

Powered by blists - more mailing lists