Re: [PATCH v3 5/9] PCI: rzg3s-host: Add Initial PCIe Host Driver for Renesas RZ/G3S SoC

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

Hi, Bjorn,

Apolgies for taking so long to reply to this.

On 08.07.2025 22:24, Bjorn Helgaas wrote:
> [+cc Nam for MSI parent domain conversions, head of this thread at
> https://lore.kernel.org/r/20250704161410.3931884-1-claudiu.beznea.uj@bp.renesas.com]
> 
> In subject:
> 
>   PCI: rzg3s-host: Add Renesas RZ/G3S SoC host driver
> 
> so the important stuff is up front instead of being wrapped at the
> end.

OK

> 
> On Fri, Jul 04, 2025 at 07:14:05PM +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.
>>
>> 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@sang-engineering.com>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
>> +static bool rzg3s_pcie_child_issue_request(struct rzg3s_pcie_host *host)
>> +{
>> +	u32 val;
>> +	int ret;
>> +
>> +	rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_REQISS,
>> +			       RZG3S_PCI_REQISS_REQ_ISSUE,
>> +			       RZG3S_PCI_REQISS_REQ_ISSUE);
>> +	ret = readl_poll_timeout_atomic(host->axi + RZG3S_PCI_REQISS, val,
>> +					!(val & RZG3S_PCI_REQISS_REQ_ISSUE),
>> +					5, RZG3S_REQ_ISSUE_TIMEOUT_US);
>> +
>> +	return !!ret || (val & RZG3S_PCI_REQISS_MOR_STATUS);
> 
> From the context in the caller, I guess returning "true" means we
> timed out or RZG3S_PCI_REQISS_MOR_STATUS contained some failure
> status, and "false" means success.  This is a little bit mind-bending,
> and it's a pain to have to deduce the bool meaning from the context.
> 
> Personally I would return 0 for success or a negative errno, e.g.,
> 
>   if (val & RZG3S_PCI_REQISS_MOR_STATUS)
>     return -E<something>;
>   return ret;
> 

I'll change it in the next version.

>> +}
>> +
>> +static int rzg3s_pcie_child_read_conf(struct rzg3s_pcie_host *host,
>> +				      struct pci_bus *bus,
>> +				      unsigned int devfn, int where,
>> +				      u32 *data)
>> +{
>> +	int ret;
>> +
>> +	bus->ops->map_bus(bus, devfn, where);
>> +
>> +	/* Set the type of request */
>> +	writel(RZG3S_PCI_REQISS_TR_TP0_RD, host->axi + RZG3S_PCI_REQISS);
>> +
>> +	/* Issue the request and wait to finish */
>> +	ret = rzg3s_pcie_child_issue_request(host);
>> +	if (ret)
>> +		return PCIBIOS_SET_FAILED;
>> +
>> +	/* Read the data */
>> +	*data = readl(host->axi + RZG3S_PCI_REQRCVDAT);
>> +
>> +	return PCIBIOS_SUCCESSFUL;
>> +}
> 
>> +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(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);
>> +	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(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->parent, 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(BIT(reg_bit),
>> +			       host->axi + RZG3S_PCI_MSIRS(reg_id));
>> +		}
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static void rzg3s_pcie_msi_top_irq_ack(struct irq_data *d)
>> +{
>> +	irq_chip_ack_parent(d);
>> +}
>> +
>> +static void rzg3s_pcie_msi_top_irq_mask(struct irq_data *d)
>> +{
>> +	pci_msi_mask_irq(d);
>> +	irq_chip_mask_parent(d);
>> +}
>> +
>> +static void rzg3s_pcie_msi_top_irq_unmask(struct irq_data *d)
>> +{
>> +	pci_msi_unmask_irq(d);
>> +	irq_chip_unmask_parent(d);
>> +}
>> +
>> +static struct irq_chip rzg3s_pcie_msi_top_chip = {
>> +	.name		= "PCIe MSI",
>> +	.irq_ack	= rzg3s_pcie_msi_top_irq_ack,
>> +	.irq_mask	= rzg3s_pcie_msi_top_irq_mask,
>> +	.irq_unmask	= rzg3s_pcie_msi_top_irq_unmask,
>> +};
>> +
>> +static void rzg3s_pcie_msi_irq_ack(struct irq_data *d)
>> +{
>> +	struct rzg3s_pcie_msi *msi = irq_data_get_irq_chip_data(d);
>> +	struct rzg3s_pcie_host *host = rzg3s_msi_to_host(msi);
>> +	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(BIT(reg_bit), host->axi + RZG3S_PCI_MSIRS(reg_id));
>> +}
>> +
>> +static void rzg3s_pcie_msi_irq_mask(struct irq_data *d)
>> +{
>> +	struct rzg3s_pcie_msi *msi = irq_data_get_irq_chip_data(d);
>> +	struct rzg3s_pcie_host *host = rzg3s_msi_to_host(msi);
>> +	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);
>> +
>> +	rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_MSIRM(reg_id), BIT(reg_bit),
>> +			       BIT(reg_bit));
>> +}
>> +
>> +static void rzg3s_pcie_msi_irq_unmask(struct irq_data *d)
>> +{
>> +	struct rzg3s_pcie_msi *msi = irq_data_get_irq_chip_data(d);
>> +	struct rzg3s_pcie_host *host = rzg3s_msi_to_host(msi);
>> +	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);
>> +
>> +	rzg3s_pcie_update_bits(host->axi, RZG3S_PCI_MSIRM(reg_id), BIT(reg_bit),
>> +			       0);
>> +}
>> +
>> +static void rzg3s_pcie_irq_compose_msi_msg(struct irq_data *data,
>> +					   struct msi_msg *msg)
>> +{
>> +	struct rzg3s_pcie_msi *msi = irq_data_get_irq_chip_data(data);
>> +	struct rzg3s_pcie_host *host = rzg3s_msi_to_host(msi);
>> +	u32 drop_mask = RZG3S_PCI_MSIRCVWADRL_ENA |
>> +			RZG3S_PCI_MSIRCVWADRL_MSG_DATA_ENA;
>> +	u32 lo, hi;
>> +
>> +	/*
>> +	 * Enable and msg data enable bits are part of the address lo. Drop
>> +	 * them.
>> +	 */
>> +	lo = readl(host->axi + RZG3S_PCI_MSIRCVWADRL) & ~drop_mask;
>> +	hi = readl(host->axi + RZG3S_PCI_MSIRCVWADRU);
>> +
>> +	msg->address_lo = lo;
>> +	msg->address_hi = hi;
>> +	msg->data = data->hwirq;
>> +}
>> +
>> +static struct irq_chip rzg3s_pcie_msi_bottom_chip = {
>> +	.name			= "rzg3s-pcie-msi",
>> +	.irq_ack		= rzg3s_pcie_msi_irq_ack,
>> +	.irq_mask		= rzg3s_pcie_msi_irq_mask,
>> +	.irq_unmask		= rzg3s_pcie_msi_irq_unmask,
>> +	.irq_compose_msi_msg	= rzg3s_pcie_irq_compose_msi_msg,
>> +};
>> +
>> +static int rzg3s_pcie_msi_domain_alloc(struct irq_domain *domain,
>> +				       unsigned int virq, unsigned int nr_irqs,
>> +				       void *args)
>> +{
>> +	struct rzg3s_pcie_msi *msi = domain->host_data;
>> +	int hwirq;
>> +
>> +	scoped_guard(mutex, &msi->map_lock) {
>> +		hwirq = bitmap_find_free_region(msi->map, RZG3S_PCI_MSI_INT_NR,
>> +						order_base_2(nr_irqs));
>> +	}
>> +
>> +	if (hwirq < 0)
>> +		return -ENOSPC;
>> +
>> +	for (unsigned int i = 0; i < nr_irqs; i++) {
>> +		irq_domain_set_info(domain, virq + i, hwirq + i,
>> +				    &rzg3s_pcie_msi_bottom_chip,
>> +				    domain->host_data, handle_edge_irq, NULL,
>> +				    NULL);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void rzg3s_pcie_msi_domain_free(struct irq_domain *domain,
>> +				       unsigned int virq, unsigned int nr_irqs)
>> +{
>> +	struct irq_data *d = irq_domain_get_irq_data(domain, virq);
>> +	struct rzg3s_pcie_msi *msi = domain->host_data;
>> +
>> +	guard(mutex)(&msi->map_lock);
>> +
>> +	bitmap_release_region(msi->map, d->hwirq, order_base_2(nr_irqs));
>> +}
>> +
>> +static const struct irq_domain_ops rzg3s_pcie_msi_domain_ops = {
>> +	.alloc	= rzg3s_pcie_msi_domain_alloc,
>> +	.free	= rzg3s_pcie_msi_domain_free,
>> +};
>> +
>> +static struct msi_domain_info rzg3s_pcie_msi_info = {
>> +	.flags	= MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
>> +		  MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_NO_AFFINITY,
>> +	.chip	= &rzg3s_pcie_msi_top_chip,
>> +};
>> +
>> +static int rzg3s_pcie_msi_allocate_domains(struct rzg3s_pcie_msi *msi)
>> +{
>> +	struct rzg3s_pcie_host *host = rzg3s_msi_to_host(msi);
>> +	struct device *dev = host->dev;
>> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
>> +	struct irq_domain *parent;
>> +
>> +	parent = irq_domain_create_linear(fwnode, RZG3S_PCI_MSI_INT_NR,
>> +					  &rzg3s_pcie_msi_domain_ops, msi);
>> +	if (!parent)
>> +		return dev_err_probe(dev, -ENOMEM,
>> +				     "failed to create IRQ domain\n");
>> +	irq_domain_update_bus_token(parent, DOMAIN_BUS_NEXUS);
>> +
>> +	msi->domain = pci_msi_create_irq_domain(fwnode, &rzg3s_pcie_msi_info,
>> +						parent);
>> +	if (!msi->domain) {
>> +		irq_domain_remove(parent);
>> +		return dev_err_probe(dev, -ENOMEM,
>> +				     "failed to create MSI domain\n");
>> +	}
> 
> Can you please rework this to follow what Nam Cao is doing for
> existing drivers:
> https://lore.kernel.org/r/cover.1750858083.git.namcao@linutronix.de

Sure! Thank you for pointing it!

> 
>> +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", 97 + i);
> 
> Can you use 'a' instead of 97?

Yes!

> 
>> +		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",
>> +					     65 + i);
> 
> And 'A' instead of 65?

Yes!

> 
>> +static int rzg3s_pcie_config_init(struct rzg3s_pcie_host *host)
>> +{
>> +	struct pci_host_bridge *bridge = pci_host_bridge_from_priv(host);
>> +	struct resource_entry *ft;
>> +	struct resource *bus;
>> +	u8 subordinate_bus;
>> +	u8 secondary_bus;
>> +	u8 primary_bus;
>> +
>> +	ft = resource_list_first_type(&bridge->windows, IORESOURCE_BUS);
>> +	if (!ft)
>> +		return -ENODEV;
>> +
>> +	bus = ft->res;
>> +	primary_bus = bus->start;
>> +	secondary_bus = bus->start + 1;
>> +	subordinate_bus = bus->end;
>> +
>> +	/* Enable access control to the CFGU */
>> +	writel(RZG3S_PCI_PERM_CFG_HWINIT_EN, host->axi + RZG3S_PCI_PERM);
>> +
>> +	/* Update vendor ID and device ID */
>> +	writew(host->vendor_id, host->pcie + PCI_VENDOR_ID);
>> +	writew(host->device_id, host->pcie + PCI_DEVICE_ID);
>> +
>> +	/* HW manual recommends to write 0xffffffff on initialization */
>> +	writel(0xffffffff, host->pcie + RZG3S_PCI_CFG_BARMSK00L);
>> +	writel(0xffffffff, host->pcie + RZG3S_PCI_CFG_BARMSK00U);
>> +
>> +	/* Update bus info. */
> 
> Drop period at end to match other one-line single-sentence comments.

Missed it. I'll drop it.

> 
>> +	writeb(primary_bus, host->pcie + PCI_PRIMARY_BUS);
>> +	writeb(secondary_bus, host->pcie + PCI_SECONDARY_BUS);
>> +	writeb(subordinate_bus, host->pcie + PCI_SUBORDINATE_BUS);
>> +
>> +	/* Disable access control to the CFGU */
>> +	writel(0, host->axi + RZG3S_PCI_PERM);
>> +
>> +	return 0;
>> +}
> 
>> +static int rzg3s_pcie_host_init(struct rzg3s_pcie_host *host, bool probe)
>> +{
>> +	u32 val;
>> +	int ret;
>> +
>> +	/* Initialize the PCIe related registers */
>> +	ret = rzg3s_pcie_config_init(host);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Initialize the interrupts */
>> +	rzg3s_pcie_irq_init(host);
>> +
>> +	ret = reset_control_bulk_deassert(host->data->num_cfg_resets,
>> +					  host->cfg_resets);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Wait for link up */
>> +	ret = readl_poll_timeout(host->axi + RZG3S_PCI_PCSTAT1, val,
>> +				 !(val & RZG3S_PCI_PCSTAT1_DL_DOWN_STS),
>> +				 PCIE_LINK_WAIT_SLEEP_MS,
>> +				 PCIE_LINK_WAIT_SLEEP_MS *
>> +				 PCIE_LINK_WAIT_MAX_RETRIES * MILLI);
>> +	if (ret) {
>> +		reset_control_bulk_assert(host->data->num_cfg_resets,
>> +					  host->cfg_resets);
>> +		return ret;
>> +	}
>> +
>> +	val = readl(host->axi + RZG3S_PCI_PCSTAT2);
>> +	dev_info(host->dev, "PCIe link status [0x%x]\n", val);
>> +
>> +	val = FIELD_GET(RZG3S_PCI_PCSTAT2_STATE_RX_DETECT, val);
>> +	dev_info(host->dev, "PCIe x%d: link up\n", hweight32(val));
> 
> Maybe a little verbose for production use?

OK, I'll drop it.

> 
>> +	if (probe) {
>> +		ret = devm_add_action_or_reset(host->dev,
>> +					       rzg3s_pcie_cfg_resets_action,
>> +					       host);
>> +	}
>> +
>> +	return ret;
>> +}
> 
>> +static int rzg3s_soc_pcie_init_phy(struct rzg3s_pcie_host *host)
>> +{
>> +	static const u32 xcfgd_settings[RZG3S_PCI_PHY_XCFGD_NUM] = {
>> +		[8]  = 0xe0006801, 0x007f7e30, 0x183e0000, 0x978ff500,
>> +		       0xec000000, 0x009f1400, 0x0000d009,
>> +		[17] = 0x78000000,
>> +		[19] = 0x00880000, 0x000005c0, 0x07000000, 0x00780920,
>> +		       0xc9400ce2, 0x90000c0c, 0x000c1414, 0x00005034,
>> +		       0x00006000, 0x00000001,
>> +	};
>> +	static const u32 xcfga_cmn_settings[RZG3S_PCI_PHY_XCFGA_CMN_NUM] = {
>> +		0x00000d10, 0x08310100, 0x00c21404, 0x013c0010, 0x01874440,
>> +		0x1a216082, 0x00103440, 0x00000080, 0x00000010, 0x0c1000c1,
>> +		0x1000c100, 0x0222000c, 0x00640019, 0x00a00028, 0x01d11228,
>> +		0x0201001d,
>> +	};
>> +	static const u32 xcfga_rx_settings[RZG3S_PCI_PHY_XCFGA_RX_NUM] = {
>> +		0x07d55000, 0x030e3f00, 0x00000288, 0x102c5880, 0x0000000b,
>> +		0x04141441, 0x00641641, 0x00d63d63, 0x00641641, 0x01970377,
>> +		0x00190287, 0x00190028, 0x00000028,
>> +	};
>> +
>> +	/*
>> +	 * Enable access permission for physical layer control and status
>> +	 * registers.
>> +	 */
>> +	writel(RZG3S_PCI_PERM_PIPE_PHY_REG_EN, host->axi + RZG3S_PCI_PERM);
>> +
>> +	for (u8 i = 0; i < RZG3S_PCI_PHY_XCFGD_NUM; i++)
>> +		writel(xcfgd_settings[i], host->axi + RZG3S_PCI_PHY_XCFGD(i));
>> +
>> +	for (u8 i = 0; i < RZG3S_PCI_PHY_XCFGA_CMN_NUM; i++) {
>> +		writel(xcfga_cmn_settings[i],
>> +		       host->axi + RZG3S_PCI_PHY_XCFGA_CMN(i));
>> +	}
>> +
>> +	for (u8 i = 0; i < RZG3S_PCI_PHY_XCFGA_RX_NUM; i++) {
>> +		writel(xcfga_rx_settings[i],
>> +		       host->axi + RZG3S_PCI_PHY_XCFGA_RX(i));
>> +	}
> 
> Why "for (unsigned int i = 0; ...)" above and "u8" here?  Seems like
> similar situation here and no benefit for using u8 vs unsigned int.

OK, I'll use unsigned int here as well.

> 
>> +	writel(0x107, host->axi + RZG3S_PCI_PHY_XCFGA_TX);
>> +
>> +	/* Select PHY settings values */
>> +	writel(RZG3S_PCI_PHY_XCFG_CTRL_PHYREG_SEL,
>> +	       host->axi + RZG3S_PCI_PHY_XCFG_CTRL);
>> +
>> +	/*
>> +	 * Disable access permission for physical layer control and status
>> +	 * registers.
>> +	 */
>> +	writel(0, host->axi + RZG3S_PCI_PERM);
>> +
>> +	return 0;
>> +}
> 
>> +static int
>> +rzg3s_pcie_host_setup(struct rzg3s_pcie_host *host,
>> +		      int (*intx_setup)(struct rzg3s_pcie_host *host),
>> +		      int (*msi_setup)(struct rzg3s_pcie_host *host),
>> +		      bool probe)
>> +{
>> +	struct device *dev = host->dev;
>> +	int ret;
>> +
>> +	/* Set inbound windows */
>> +	ret = rzg3s_pcie_parse_map_dma_ranges(host);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret,
>> +				     "Failed to set inbound windows!\n");
>> +
>> +	/* Set outbound windows */
>> +	ret = rzg3s_pcie_parse_map_ranges(host);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret,
>> +				     "Failed to set outbound windows!\n");
>> +
>> +	/* Set the PHY, if any */
>> +	if (host->data->init_phy) {
>> +		ret = host->data->init_phy(host);
>> +		if (ret)
>> +			return dev_err_probe(dev, ret,
>> +					     "Failed to set the PHY!\n");
>> +	}
>> +
>> +	if (intx_setup) {
>> +		ret = intx_setup(host);
>> +		if (ret)
>> +			return dev_err_probe(dev, ret,
>> +					     "Failed to setup INTx\n");
>> +	}
>> +
>> +	/* Set the MSIs */
>> +	if (IS_ENABLED(CONFIG_PCI_MSI)) {
>> +		ret = msi_setup(host);
>> +		if (ret)
>> +			return dev_err_probe(dev, ret,
>> +					     "Failed to setup MSIs\n");
>> +	}
>> +
>> +	/* Initialize the host */
> 
> Superfluous comment since you have a good function name.

Will drop it.

> 
>> +	ret = rzg3s_pcie_host_init(host, probe);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret,
>> +				     "Failed to initialize the HW!\n");
>> +
>> +	/* Try to set maximum supported link speed */
> 
> Ditto.

OK.

> 
>> +	ret = rzg3s_pcie_set_max_link_speed(host);
>> +	if (ret)
>> +		dev_info(dev, "Failed to set max link speed\n");
>> +
>> +	return 0;
>> +}
> 
>> +static int rzg3s_pcie_probe(struct platform_device *pdev)
>> +{
>> +	struct pci_host_bridge *bridge;
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *np = dev->of_node;
>> +	struct device_node *sysc_np __free(device_node) =
>> +		of_parse_phandle(np, "renesas,sysc", 0);
>> +	struct rzg3s_pcie_host *host;
>> +	int ret;
>> +
>> +	bridge = devm_pci_alloc_host_bridge(dev, sizeof(*host));
>> +	if (!bridge)
>> +		return -ENOMEM;
>> +
>> +	host = pci_host_bridge_priv(bridge);
>> +	host->dev = dev;
>> +	host->data = device_get_match_data(dev);
>> +	platform_set_drvdata(pdev, host);
>> +
>> +	host->axi = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(host->axi))
>> +		return PTR_ERR(host->axi);
>> +	host->pcie = host->axi + RZG3S_PCI_CFG_BASE;
>> +
>> +	ret = of_property_read_u32(np, "vendor-id", &host->vendor_id);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = of_property_read_u32(np, "device-id", &host->device_id);
>> +	if (ret)
>> +		return ret;
>> +
>> +	host->sysc = syscon_node_to_regmap(sysc_np);
>> +	if (IS_ERR(host->sysc))
>> +		return PTR_ERR(host->sysc);
>> +
>> +	ret = regmap_update_bits(host->sysc, RZG3S_SYS_PCIE_RST_RSM_B,
>> +				 RZG3S_SYS_PCIE_RST_RSM_B_MASK,
>> +				 FIELD_PREP(RZG3S_SYS_PCIE_RST_RSM_B_MASK, 1));
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = devm_add_action_or_reset(dev, rzg3s_pcie_sysc_signal_action,
>> +				       host->sysc);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = rzg3s_pcie_resets_prepare(host);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = devm_pm_runtime_enable(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = pm_runtime_resume_and_get(dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = devm_add_action_or_reset(dev, rzg3s_pcie_pm_runtime_put, dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	raw_spin_lock_init(&host->hw_lock);
>> +
>> +	ret = rzg3s_pcie_host_setup(host, rzg3s_pcie_intx_setup,
>> +				    rzg3s_pcie_msi_enable, true);
>> +	if (ret)
>> +		return ret;
>> +
>> +	msleep(PCIE_RESET_CONFIG_WAIT_MS);
> 
> We also call rzg3s_pcie_host_setup(), where rzg3s_pcie_host_init()
> deasserts all the resets and waits for the link to come up, from
> rzg3s_pcie_resume_noirq().
> 
> Seems like the rzg3s_pcie_resume_noirq() path needs similar delay?
> Perhaps the delay should be in rzg3s_pcie_host_init() where the event
> that defines the beginning of the required delay is?

You're right. I'll add the delay to rzg3s_pcie_host_setup().

> 
>> +	bridge->sysdata = host;
>> +	bridge->ops = &rzg3s_pcie_root_ops;
>> +	bridge->child_ops = &rzg3s_pcie_child_ops;
>> +	ret = pci_host_probe(bridge);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return devm_add_action_or_reset(dev, rzg3s_pcie_host_remove_action,
>> +					host);
>> +}
> 
>> +static int rzg3s_pcie_resume_noirq(struct device *dev)
>> +{
>> +	struct rzg3s_pcie_host *host = dev_get_drvdata(dev);
>> +	const struct rzg3s_pcie_soc_data *data = host->data;
>> +	struct regmap *sysc = host->sysc;
>> +	int ret;
>> +
>> +	ret = regmap_update_bits(sysc, RZG3S_SYS_PCIE_RST_RSM_B,
>> +				 RZG3S_SYS_PCIE_RST_RSM_B_MASK,
>> +				 FIELD_PREP(RZG3S_SYS_PCIE_RST_RSM_B_MASK, 1));
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = rzg3s_pcie_power_resets_deassert(host);
>> +	if (ret)
>> +		goto assert_rst_rsm_b;
>> +
>> +	ret = pm_runtime_resume_and_get(dev);
>> +	if (ret)
>> +		goto assert_power_resets;
>> +
>> +	ret = rzg3s_pcie_host_setup(host, NULL, rzg3s_pcie_msi_hw_setup, false);
>> +	if (ret)
>> +		goto rpm_put;
>> +
>> +	return 0;
>> +
>> +	/*
>> +	 * If any error happens there is no way to recover the IP. Put it in the
>> +	 * lowest possible power state.
>> +	 */
>> +rpm_put:
>> +	pm_runtime_put_sync(dev);
>> +assert_power_resets:
>> +	reset_control_bulk_assert(data->num_power_resets,
>> +				  host->power_resets);
>> +assert_rst_rsm_b:
>> +	regmap_update_bits(sysc, RZG3S_SYS_PCIE_RST_RSM_B,
>> +			   RZG3S_SYS_PCIE_RST_RSM_B_MASK,
>> +			   FIELD_PREP(RZG3S_SYS_PCIE_RST_RSM_B_MASK, 0));
>> +	return ret;
>> +}