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

[Serge Semin]

On Thu, Oct 19, 2023 at 05:08:47PM -0500, Bjorn Helgaas wrote:
> On Thu, Oct 19, 2023 at 01:05:24PM +0300, 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?
> 
> I think so.  07e292950b93 ("PCI: Allow root and child buses to have
> different pci_ops") says:
> 
>   PCI host bridges often have different ways to access the root and child
>   bus config spaces. The host bridge drivers have invented their own
>   abstractions to handle this. Let's support having different root and
>   child bus pci_ops so these per driver abstractions can be removed.
> 
> https://git.kernel.org/linus/07e292950b93
> 
> > 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?
> 
> Probably so.
> 
> But I don't think this code should be in an .add_bus() method in the
> first place.  Ideally I think the content of ks_pcie_v3_65_add_bus()
> would move to the ks_pcie_host_init() path so we wouldn't need the
> .add_bus() hook at all.
> 
> I think it was added as ks_dw_pcie_v3_65_scan_bus() by 0c4ffcfe1fbc
> ("PCI: keystone: Add TI Keystone PCIe driver"), which doesn't explain
> why doing this after scanning the secondary bus is needed.
> 
> The .scan_bus() hook was added by b14a3d1784a9 ("PCI: designware: Add
> support for v3.65 hardware"), which says:
> 
>   3. MSI interrupt generation requires EP to write to the RC's
>      application register.  So enhance the driver to allow setup of
>      inbound access to MSI IRQ register as a post scan bus API callback.
> 
> That's not a convincing argument for why the BAR setup has to be done
> *after* enumerating the endpoints, but presumably there was some
> reason.

Ok. Thank you very much for clarification. From that perspective
indeed it would be better to move the ks_pcie_v3_65_add_bus() method
invocation to the host_init() callback.

Siddharth, if it won't be that much bother and you have an access to
the v3.65-based Keystone PCIe device, could you please have a look
whether it's possible to implement what Bjorn suggested? *

* it's irrespective to this patch. This fix looks good. If Bjorn
and/or Mani are ok with it, I guess it can be already merged in.

-Serge(y)

> 
> Bjorn