Re: [PATCH v3] PCI: keystone: Fix pci_ops for AM654x SoC

[Serge Semin]

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
>