Message-ID: <51af454f-6322-47c3-9e93-4fc07efc2b8d@tuxon.dev>
Date: Thu, 23 Oct 2025 08:11:17 +0300
From: Claudiu Beznea <claudiu.beznea@...on.dev>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: lpieralisi@...nel.org, kwilczynski@...nel.org, mani@...nel.org,
 robh@...nel.org, bhelgaas@...gle.com, 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 v5 2/6] PCI: rzg3s-host: Add Renesas RZ/G3S SoC host
 driver

Hi, Bjorn,

On 10/22/25 22:49, Bjorn Helgaas wrote:
> On Tue, Oct 07, 2025 at 04:36:53PM +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.
> 
>> +++ b/drivers/pci/controller/Kconfig
>> @@ -266,6 +266,14 @@ config PCI_RCAR_GEN2
>>  	  There are 3 internal PCI controllers available with a single
>>  	  built-in EHCI/OHCI host controller present on each one.
>>  
>> +config PCIE_RENESAS_RZG3S_HOST
>> +	bool "Renesas RZ/G3S PCIe host controller"
>> +	depends on ARCH_RENESAS || COMPILE_TEST
>> +	select MFD_SYSCON
>> +	select IRQ_MSI_LIB
>> +	help
>> +	  Say Y here if you want PCIe host controller support on Renesas RZ/G3S SoC.
> 
> Wrap to fit in 80 columns like the rest of the file.

OK

> 
>> +++ b/drivers/pci/controller/pcie-rzg3s-host.c
> 
>> +#define RZG3S_PCI_MSIRCVWMSKL			0x108
>> +#define RZG3S_PCI_MSIRCVWMSKL_MASK		GENMASK(31, 2)
> 
> Unfortunate to have to add _MASK here when none of the other GENMASKs
> need it.  Can't think of a better name though.

Most of the register offsets and fields defines tried to use the naming
from the HW manual. Register at offset 0x108 have bits 31..2 read/writable
and is where we should be writing through driver, and bits 1..0 are read
only and have fixed value. These fields are named in HW manual as:

- MSI Receive Window Mask (Lower) [31:2]
- MSI Receive Window Mask (Lower) [1:0]

As bits 31..2 are read/writable, would you prefer something like:

#define RZG3S_PCI_MSIRCVWMSKL_RW		GENMASK(31, 2)

?

> 
>> +#define RZG3S_PCI_MSIRCVWMSKU			0x10c
> 
> Unused.
> 
>> +#define RZG3S_PCI_AMEIE				0x210
> 
> Unused.
> 
>> +#define RZG3S_PCI_ASEIE1			0x220
> 
> Unused.
> 
>> +#define RZG3S_PCI_PCSTAT2_STATE_RX_DETECT	GENMASK(15, 8)
> 
> Unused.

I agree with all the unused defines pointed. Will be dropped in the next
version.

> 
>> +/* Timeouts experimentally determined. */
> 
> No need for period at end.

Missed this one. I'll update it.

> 
>> +static int rzg3s_pcie_child_read_conf(struct rzg3s_pcie_host *host,
>> +				      struct pci_bus *bus,
>> +				      unsigned int devfn, int where,
>> +				      u32 *data)
> 
> Would fit in three lines if you want.
> 
>> +static int rzg3s_pcie_child_write_conf(struct rzg3s_pcie_host *host,
>> +				       struct pci_bus *bus,
>> +				       unsigned int devfn, int where,
>> +				       u32 data)
> 
> Ditto.

Will update both of these along with:

rzg3s_pcie_child_prepare_bus()
rzg3s_pcie_root_map_bus()
rzg3s_pcie_set_outbound_window()

that have the same symptom.

> 
>> +static int rzg3s_pcie_msi_enable(struct rzg3s_pcie_host *host)
>> +{
>> +	struct platform_device *pdev = to_platform_device(host->dev);
>> +	struct rzg3s_pcie_msi *msi = &host->msi;
>> +	struct device *dev = host->dev;
>> +	const char *devname;
>> +	int irq, ret;
>> +
>> +	ret = devm_mutex_init(dev, &msi->map_lock);
>> +	if (ret)
>> +		return ret;
>> +
>> +	msi->irq = platform_get_irq_byname(pdev, "msi");
>> +	if (msi->irq < 0)
>> +		return dev_err_probe(dev, irq ? irq : -EINVAL,
>> +				     "Failed to get MSI IRQ!\n");
>> +
>> +	devname = devm_kasprintf(dev, GFP_KERNEL, "%s-msi", dev_name(dev));
>> +	if (!devname)
>> +		return -ENOMEM;
>> +
>> +	ret = rzg3s_pcie_msi_allocate_domains(msi);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = request_irq(msi->irq, rzg3s_pcie_msi_irq, 0, devname, host);
> 
> Should this be devm_request_irq()?  Most drivers use it, although
> pci-tegra.c and pcie-apple.c do not.  Maybe there's some special rule
> about using request_irq() even though the driver uses devm in general?
> I dunno.

In general is not good to mix devm cleanups with driver specific one.

As it was requested to drop the devm cleanups from this driver (especially
devm_pm_runtime_enable() which enables the also the clocks) I switched the
initial devm_request_irq() to request_irq() to avoid keeping the interrupt
requested on error path, after driver's probed was executed, until devm
cleanups are called, and potentially having it firing w/o hardware
resourced being enabled (e.g. clocks), and potentially reading HW registers.

E.g., accessing the HW registers while clocks are disabled on the SoC I'm
working with leads to synchronous aborts.

So, I only kept the devm helpers for memory allocations, resets
assert/de-assert and the mutex initialization.

> 
>> +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);
> 
> Looks like this should be outside the loop.

OK, I kept it here as it is used only inside this block.

> 
>> +		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);
>> +	}
> 
>> +static int rzg3s_pcie_power_resets_deassert(struct rzg3s_pcie_host *host)
>> +{
>> +	const struct rzg3s_pcie_soc_data *data = host->data;
>> +
>> +	/*
>> +	 * According to the RZ/G3S HW manual (Rev.1.10, section
>> +	 * 34.5.1.2 De-asserting the Reset) the PCIe IP needs to wait 5ms from
>> +	 * power on to the de-assertion of reset.
>> +	 */
>> +	usleep_range(5000, 5100);
> 
> Consider fsleep() so we don't have to make up the 100us interval.

OK

Thank you for your review,
Claudiu

Powered by blists - more mailing lists