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,

On 8/30/25 14:22, 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.

Actually, when calling:

pci_host_probe() ->
  pci_scan_root_bus_bridge() ->
    pci_scan_child_bus() ->
      pci_scan_child_bus_extend() ->
        // ...
        for (devnr = 0; devnr < PCI_MAX_NR_DEVS; devnr++)
            pci_scan_slot(bus, PCI_DEVFN(devnr, 0));

The pci_scan_slot() calls only_one_child() at the beginning but that don't
return 1 on the root bus as it is called just after pci_host_probe() and
the bus->self is not set (as of my investigation it is set later in
pci_scan_child_bus_extend()) leading to rzg3s_pcie_root_map_bus() being
called with devfn != 0.

Similar drivers having ops and child_ops assigned use the same approach.
E.g. dw_pcie_own_conf_map_bus():

void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus, unsigned int
devfn, int where)
{
	struct dw_pcie_rp *pp = bus->sysdata;
	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);

	if (PCI_SLOT(devfn) > 0)
		return NULL;

	return pci->dbi_base + where;
}

Thank you,
Claudiu