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

Claudiu Beznea
2025-09-08T17:10:03Z

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