Message-ID: <f2725090-e199-493d-9ae3-e807d65f647b@163.com>
Date: Tue, 25 Mar 2025 20:16:09 +0800
From: Hans Zhang <18255117159@....com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: lpieralisi@...nel.org, kw@...ux.com, manivannan.sadhasivam@...aro.org,
 robh@...nel.org, bhelgaas@...gle.com, jingoohan1@...il.com,
 thomas.richard@...tlin.com, linux-pci@...r.kernel.org,
 LKML <linux-kernel@...r.kernel.org>
Subject: Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for
 finding the capabilities



On 2025/3/25 19:15, Ilpo Järvinen wrote:
> On Tue, 25 Mar 2025, Hans Zhang wrote:
>> On 2025/3/24 23:02, Ilpo Järvinen wrote:
>>>>>>     +static u32 cdns_pcie_read_cfg(void *priv, int where, int size)
>>>>>> +{
>>>>>> +	struct cdns_pcie *pcie = priv;
>>>>>> +	u32 val;
>>>>>> +
>>>>>> +	if (size == 4)
>>>>>> +		val = readl(pcie->reg_base + where);
>>>>>
>>>>> Should this use cdns_pcie_readl() ?
>>>>
>>>> pci_host_bridge_find_*capability required to read two or four bytes.
>>>>
>>>> reg = read_cfg(priv, cap_ptr, 2);
>>>> or
>>>> header = read_cfg(priv, pos, 4);
>>>>
>>>> Here I mainly want to write it the same way as size == 2 and size == 1.
>>>> Or size == 4 should I write it as cdns_pcie_readl() ?
>>>
>>> As is, it seems two functions are added for the same thing for the case
>>> with size == 4 with different names which feels duplication. One could add
>>> cdns_pcie_readw() and cdns_pcie_readb() too but perhaps cdns_pcie_readl()
>>> should just call this new function instead?
>>
>> Hi Ilpo,
>>
>> Redefine a function with reference to DWC?
> 
> This patch was about cadence so my comment above what related to that.
> 

Hi Ilpo,

Thanks your for reply. Let's look at the main problem first.

>> u32 dw_pcie_read_dbi(struct dw_pcie *pci, u32 reg, size_t size)
>>    dw_pcie_read(pci->dbi_base + reg, size, &val);
>>      dw_pcie_read
>>
>> int dw_pcie_read(void __iomem *addr, int size, u32 *val)
>> {
>> 	if (!IS_ALIGNED((uintptr_t)addr, size)) {
>> 		*val = 0;
>> 		return PCIBIOS_BAD_REGISTER_NUMBER;
>> 	}
>>
>> 	if (size == 4) {
>> 		*val = readl(addr);
>> 	} else if (size == 2) {
>> 		*val = readw(addr);
>> 	} else if (size == 1) {
>> 		*val = readb(addr);
>> 	} else {
>> 		*val = 0;
>> 		return PCIBIOS_BAD_REGISTER_NUMBER;
>> 	}
>>
>> 	return PCIBIOS_SUCCESSFUL;
>> }
>> EXPORT_SYMBOL_GPL(dw_pcie_read);
>>
>>>
>>>>>> +	else if (size == 2)
>>>>>> +		val = readw(pcie->reg_base + where);
>>>>>> +	else if (size == 1)
>>>>>> +		val = readb(pcie->reg_base + where);
>>>>>> +
>>>>>> +	return val;
>>>>>> +}
>>>>>> +
>>>>>> +u8 cdns_pcie_find_capability(struct cdns_pcie *pcie, u8 cap)
>>>>>> +{
>>>>>> +	return pci_host_bridge_find_capability(pcie,
>>>>>> cdns_pcie_read_cfg, cap);
>>>>>> +}
>>>>>> +
>>>>>> +u16 cdns_pcie_find_ext_capability(struct cdns_pcie *pcie, u8 cap)
>>>>>> +{
>>>>>> +	return pci_host_bridge_find_ext_capability(pcie,
>>>>>> cdns_pcie_read_cfg,
>>>>>> cap);
>>>>>> +}
>>>>>
>>>>> I'm really wondering why the read config function is provided directly
>>>>> as
>>>>> an argument. Shouldn't struct pci_host_bridge have some ops that can
>>>>> read
>>>>> config so wouldn't it make much more sense to pass it and use the func
>>>>> from there? There seems to ops in pci_host_bridge that has read(), does
>>>>> that work? If not, why?
>>>>>
>>>>
>>>> No effect.
>>>
>>> I'm not sure what you meant?
>>>
>>>> Because we need to get the offset of the capability before PCIe
>>>> enumerates the device.
>>>
>>> Is this to say it is needed before the struct pci_host_bridge is created?
>>>
>>>> I originally added a separate find capability related
>>>> function for CDNS in the following patch. It's also copied directly from
>>>> DWC.
>>>> Mani felt there was too much duplicate code and also suggested passing a
>>>> callback function that could manipulate the registers of the root port of
>>>> DWC
>>>> or CDNS.
>>>
>>> I very much like the direction this patchset is moving (moving shared
>>> part of controllers code to core), I just feel this doesn't go far enough
>>> when it's passing function pointer to the read function.
>>>
>>> I admit I've never written a controller driver so perhaps there's
>>> something detail I lack knowledge of but I'd want to understand why
>>> struct pci_ops (which exists both in pci_host_bridge and pci_bus) cannot
>>> be used?
>>>
>>
>>
>> I don't know if the following code can make it clear to you.
>>
>> static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
>> 	.host_init	= qcom_pcie_host_init,
>>                    pcie->cfg->ops->post_init(pcie);
>>                      qcom_pcie_post_init_2_3_3
>>                        dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>> };
>>
>> int dw_pcie_host_init(struct dw_pcie_rp *pp)
>>    bridge = devm_pci_alloc_host_bridge(dev, 0);
> 
> It does this almost immediately:
> 
>      bridge->ops = &dw_pcie_ops;
> 
> Can we like add some function into those ops such that the necessary read
> can be performed? Like .early_root_config_read or something like that?
> 
> Then the host bridge capability finder can input struct pci_host_bridge
> *host_bridge and can do host_bridge->ops->early_root_cfg_read(host_bridge,
> ...). That would already be a big win over passing the read function
> itself as a pointer.
> 
> Hopefully having such a function in the ops would allow moving other
> common controller driver functionality into PCI core as well as it would
> abstract the per controller read function (for the time before everything
> is fully instanciated).
> 
> Is that a workable approach?
>

I'll try to add and test it in your way first.

Another problem here is that I've seen some drivers invoke 
dw_pcie_find_*capability before if (pp->ops->init) {. When I confirm it, 
or I'll see if I can cover all the issues.

If I pass the test, I will provide the temporary patch here, please 
check whether it is OK, and then submit the next version. If not, we'll 
discuss it.

Thank you very much for your advice.

>>    if (pp->ops->host_init)
>>      pp->ops = &qcom_pcie_dw_ops;  // qcom here needs to find capability
>>
>>    pci_host_probe(bridge); // pcie enumerate flow
>>      pci_scan_root_bus_bridge(bridge);
>>        pci_register_host_bridge(bridge);
>>          bus->ops = bridge->ops;   // Only pci bus ops can be used
>>
>>


Best regards,
Hans

Powered by blists - more mailing lists