Hello Bjorn,
Could you please merge this patch?
On 19/10/23 15:35, Serge Semin wrote:
> On Thu, Oct 19, 2023 at 01:43:30PM +0530, Siddharth Vadapalli wrote:
>> In the process of converting .scan_bus() callbacks to .add_bus(), the
>> ks_pcie_v3_65_scan_bus() function was changed to ks_pcie_v3_65_add_bus().
>> The .scan_bus() method belonged to ks_pcie_host_ops which was specific
>> to controller version 3.65a, while the .add_bus() method had been added
>> to ks_pcie_ops which is shared between the controller versions 3.65a and
>> 4.90a. Neither the older ks_pcie_v3_65_scan_bus() method, nor the newer
>> ks_pcie_v3_65_add_bus() method are applicable to the controller version
>> 4.90a which is present in AM654x SoCs.
>>
>> Thus, fix this by creating ks_pcie_am6_ops for the AM654x SoC which uses DW
>> PCIe IP-core version 4.90a controller and omitting the .add_bus() method
>> which is not applicable to the 4.90a controller. Update ks_pcie_host_init()
>> accordingly in order to set the pci_ops to ks_pcie_am6_ops if the platform
>> is AM654x SoC and to ks_pcie_ops otherwise, by making use of the "is_am6"
>> flag.
>>
>> Fixes: 6ab15b5e7057 ("PCI: dwc: keystone: Convert .scan_bus() callback to use add_bus")
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@xxxxxx>
>
> LGTM. Thanks!
> Reviewed-by: Serge Semin <fancer.lancer@xxxxxxxxx>
>
> One more note is further just to draw attention to possible driver
> simplifications.
>
>> ---
>> Hello,
>>
>> This patch is based on linux-next tagged next-20231018.
>>
>> The v2 of this patch is at:
>> https://lore.kernel.org/r/20231018075038.2740534-1-s-vadapalli@xxxxxx/
>>
>> Changes since v2:
>> - Implemented Serge's suggestion of adding a new pci_ops structure for
>> AM654x SoC using DWC PCIe IP-core version 4.90a controller.
>> - Created struct pci_ops ks_pcie_am6_ops for AM654x SoC without the
>> .add_bus method while retaining other ops from ks_pcie_ops.
>> - Updated ks_pcie_host_init() to set pci_ops to ks_pcie_am6_ops if the
>> platform is AM654x and to ks_pcie_ops otherwise by making use of the
>> already existing "is_am6" flag.
>> - Combined the section:
>> if (!ks_pcie->is_am6)
>> pp->bridge->child_ops = &ks_child_pcie_ops;
>> into the newly added ELSE condition.
>>
>> The v1 of this patch is at:
>> https://lore.kernel.org/r/20231011123451.34827-1-s-vadapalli@xxxxxx/
>>
>> While there are a lot of changes since v1 and this patch could have been
>> posted as a v1 patch itself, I decided to post it as the v2 of the patch
>> mentioned above since it aims to address the issue described by the v1
>> patch and is similar in that sense. However, the solution to the issue
>> described in the v1 patch appears to be completely different from what
>> was implemented in the v1 patch. Thus, the commit message and subject of
>> this patch have been modified accordingly.
>>
>> Changes since v1:
>> - Updated patch subject and commit message.
>> - Determined that issue is not with the absence of Link as mentioned in
>> v1 patch. Even with Link up and endpoint device connected, if
>> ks_pcie_v3_65_add_bus() is invoked and executed, all reads to the
>> MSI-X offsets return 0xffffffff when pcieport driver attempts to setup
>> AER and PME services. The all Fs return value indicates that the MSI-X
>> configuration is failing even if Endpoint device is connected. This is
>> because the ks_pcie_v3_65_add_bus() function is not applicable to the
>> AM654x SoC which uses DW PCIe IP-core version 4.90a.
>>
>> Regards,
>> Siddharth.
>>
>> drivers/pci/controller/dwc/pci-keystone.c | 13 +++++++++++--
>> 1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
>> index 49aea6ce3e87..66341a0b6c6b 100644
>> --- a/drivers/pci/controller/dwc/pci-keystone.c
>> +++ b/drivers/pci/controller/dwc/pci-keystone.c
>> @@ -487,6 +487,12 @@ static struct pci_ops ks_pcie_ops = {
>> .add_bus = ks_pcie_v3_65_add_bus,
>> };
>>
>> +static struct pci_ops ks_pcie_am6_ops = {
>> + .map_bus = dw_pcie_own_conf_map_bus,
>> + .read = pci_generic_config_read,
>> + .write = pci_generic_config_write,
>> +};
>> +
>> /**
>> * ks_pcie_link_up() - Check if link up
>> * @pci: A pointer to the dw_pcie structure which holds the DesignWare PCIe host
>> @@ -804,9 +810,12 @@ static int __init ks_pcie_host_init(struct dw_pcie_rp *pp)
>> struct keystone_pcie *ks_pcie = to_keystone_pcie(pci);
>> int ret;
>>
>> - pp->bridge->ops = &ks_pcie_ops;
>> - if (!ks_pcie->is_am6)
>> + if (ks_pcie->is_am6) {
>> + pp->bridge->ops = &ks_pcie_am6_ops;
>> + } else {
>
>> + pp->bridge->ops = &ks_pcie_ops;
>> pp->bridge->child_ops = &ks_child_pcie_ops;
>
> Bjorn, could you please clarify the next suggestion? I'm not that
> fluent in the PCIe core details, but based on the
> pci_host_bridge.child_ops and pci_host_bridge.ops names, the first ops
> will be utilized for the child (non-root) PCIe buses, meanwhile the
> later ones - for the root bus only (see pci_alloc_child_bus()). Right?
>
> If so then either the pci_is_root_bus() check can be dropped from the
> ks_pcie_v3_65_add_bus() method since the ops it belong to will be
> utilized for the root bus anyway, or the entire ks_child_pcie_ops
> instance can be dropped since the ks_pcie_v3_65_add_bus() method will
> be no-op for the child buses anyway meanwhile ks_child_pcie_ops
> matches to ks_pcie_ops in the rest of the ops. After doing that I
> would have also changed the ks_pcie_v3_65_add_bus name to
> ks_pcie_v3_65_add_root_bus() in anyway. Am I right?
>
> -Serge(y)
>
>> + }
>>
>> ret = ks_pcie_config_legacy_irq(ks_pcie);
>> if (ret)
>> --
>> 2.34.1
>>
--
Regards,
Siddharth.
