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, Manivannan,

Apologies for the late reply, I've been off for a while.

On 8/31/25 07:07, Manivannan Sadhasivam wrote:
> On Sat, Aug 30, 2025 at 02:22:45PM GMT, Claudiu Beznea wrote:
>>
>>
>> On 30.08.2025 09:59, Manivannan Sadhasivam wrote:
>>> On Fri, Jul 04, 2025 at 07:14:05PM GMT, 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);
>>>
>>> You don't need to do !!ret as the C11 standard guarantees that any scalar type
>>> stored as bool will have the value of 0 or 1.
>>
>> OK, will drop it anyway as suggested in another thread.
>>
>>>
>>>> +}
>>>> +
>>>
>>> [...]
>>>
>>>> +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;
>>>
>>> Is it really possible to have devfn as non-zero for a root bus?
>>
>> I will drop it.
>>
>>>
>>>> +
>>>> +	return host->pcie + where;
>>>> +}
>>>> +
>>>
>>> [...]
>>>
>>>> +static int rzg3s_pcie_msi_setup(struct rzg3s_pcie_host *host)
>>>> +{
>>>> +	size_t size = RZG3S_PCI_MSI_INT_NR * sizeof(u32);
>>>> +	struct rzg3s_pcie_msi *msi = &host->msi;
>>>> +	struct device *dev = host->dev;
>>>> +	int id, ret;
>>>> +
>>>> +	msi->pages = __get_free_pages(GFP_KERNEL | GFP_DMA, 0);
>>>> +	if (!msi->pages)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	msi->dma_addr = dma_map_single(dev, (void *)msi->pages, size * 2,
>>>> +				       DMA_BIDIRECTIONAL);
>>>> +	if (dma_mapping_error(dev, msi->dma_addr)) {
>>>> +		ret = -ENOMEM;
>>>> +		goto free_pages;
>>>> +	}
>>>> +
>>>> +	/*
>>>> +	 * According to the RZ/G3S HW manual (Rev.1.10, section 34.4.5.2 Setting
>>>> +	 * the MSI Window) the MSI window need to be within any AXI window. Find
>>>> +	 * an AXI window to setup the MSI window.
>>>
>>> Are you really finding the AXI window or just making sure that the MSI window
>>> falls into one of the AXI window?
>>
>> I'm making sure the MSI windows falls into one of the enabled AXI windows.
>>
> 
> Then you need to reword the comment as such. Currently, it is not clear.

OK

> 
>>>
>>> And I believe it is OK to have more than one MSI window within an AXI window.
>>
>> This IP supports a single MSI window that need to fit into one of the
>> enabled AXI windows.
>>
> 
> [...]
> 
>>>> +
>>>> +	/* Update vendor ID and device ID */
>>>
>>> Are you really updating it or setting it? If you are updating it, are the
>>> default IDs invalid?
>>
>> Default IDs are valid (at least on RZ/G3S) but Renesas specific. Renesas
>> wants to let individual users to set their own IDs.
>>
> 
> So they are optional then? But the binding treats them as required, which should
> be changed if the default IDs are valid.

On RZ/G3S the default IDs are valid. On other SoCs that will be using this
driver (e.g. RZ/G3E) the default IDs are not valid.

These were marked as required as Renesas wants them to be set by the
company that manufactures the end product itself.


> 
>>>
>>>> +	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. */
>>>> +	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));
>>>> +
>>>> +	if (probe) {
>>>> +		ret = devm_add_action_or_reset(host->dev,
>>>> +					       rzg3s_pcie_cfg_resets_action,
>>>> +					       host);
>>>
>>> Oh well, this gets ugly. Now the devm_add_action_or_reset() is sprinkled
>>> throughout the driver :/
>>>
>>> As I said earlier, there are concerns in unloading the driver if it implements
>>> an irqchip. So if you change the module_platform_driver() to
>>> builtin_platform_driver() for this driver, these devm_add_action_or_reset()
>>> calls become unused.
>>
>> They can still be useful in case the probe fails. As the initialization
>> path is complicated, having actions or resets looks to me that makes the
>> code cleaner as the rest of devm_* helpers.
>>
>> I can drop it and replace with gotos and dedicated functions but this will
>> complicate the code, AFAICT.
>>
>> Please let me know how would you like me to proceed.
>>
> 
> It is generally preferred to cleanup the resources in err path using goto
> labels.

Just to be sure I'll prepare the next version with something that was
requested: would you like to have goto lables on this driver?

I kept it like this as I considered the code is simpler.

Thank you,
Claudiu