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

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Oct 07, 2025 at 04:36:53PM +0300, 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.

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

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

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

> +/* Timeouts experimentally determined. */

No need for period at end.

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

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

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

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