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
>
