| Message ID | 20231019081330.2975470-1-s-vadapalli@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] PCI: keystone: Fix pci_ops for AM654x SoC | expand |
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@ti.com> LGTM. Thanks! Reviewed-by: Serge Semin <fancer.lancer@gmail.com> 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@ti.com/ > > 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@ti.com/ > > 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 >
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@ti.com> > > LGTM. Thanks! > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > > 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@ti.com/ > > > > 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@ti.com/ > > > > 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. Bjorn
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@ti.com> > > > > LGTM. Thanks! > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > > > > 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@ti.com/ > > > > > > 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@ti.com/ > > > > > > 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
Serge, On 23/10/23 16:12, Serge Semin wrote: ... > 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? * Unfortunately I don't have any SoC/Device with me that has the v3.65 PCIe controller, so I will not be able to test Bjorn's suggestion. > > * 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
On Mon, Oct 23, 2023 at 05:05:30PM +0530, Siddharth Vadapalli wrote: > On 23/10/23 16:12, Serge Semin wrote: > > ... > > > 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? > > Unfortunately I don't have any SoC/Device with me that has the v3.65 PCIe > controller, so I will not be able to test Bjorn's suggestion. Huh. 57e1d8206e48 ("MAINTAINERS: move Murali Karicheri to credits") removed the maintainer for pci-keystone.c, so the driver hasn't had a maintainer for over two years. Given the fact that there's no maintainer, I'm more than happy to take a patch to move this code to somewhere in the host_init() callback, even if you don't have the hardware to test it. Bjorn
Hello Bjorn, On 24/10/23 02:56, Bjorn Helgaas wrote: > On Mon, Oct 23, 2023 at 05:05:30PM +0530, Siddharth Vadapalli wrote: >> On 23/10/23 16:12, Serge Semin wrote: >> >> ... >> >>> 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? >> >> Unfortunately I don't have any SoC/Device with me that has the v3.65 PCIe >> controller, so I will not be able to test Bjorn's suggestion. > > Huh. 57e1d8206e48 ("MAINTAINERS: move Murali Karicheri to credits") > removed the maintainer for pci-keystone.c, so the driver hasn't had a > maintainer for over two years. > > Given the fact that there's no maintainer, I'm more than happy to take > a patch to move this code to somewhere in the host_init() callback, > even if you don't have the hardware to test it. Sure, I can work on a patch for it. The execution flow with the existing code is as follows: ks_pcie_probe() dw_pcie_host_init() pci_host_probe() ks_pcie_v3_65_add_bus() So I assume that as long as ks_pcie_v3_65_add_bus() is invoked after pci_host_probe(), it should be acceptable. With this understanding, I plan to move it immediately after the invocation to dw_pcie_host_init() within ks_pcie_probe() conditionally (based on the is_am6 flag). The new call trace with this change will look like: ks_pcie_probe() dw_pcie_host_init() pci_host_probe() ks_pcie_v3_65_add_bus() Since the .add_bus() method will be removed following this change, I suppose that the patch I post for it can be applied instead of this v3 patch which fixes pci_ops. The patch I plan to post as a replacement for the current patch which shall also remove the ks_pcie_v3_65_add_bus() and the .add_bus() method is: diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c index 0def919f89fa..3933e857ecaa 100644 --- a/drivers/pci/controller/dwc/pci-keystone.c +++ b/drivers/pci/controller/dwc/pci-keystone.c @@ -447,44 +447,10 @@ static struct pci_ops ks_child_pcie_ops = { .write = pci_generic_config_write, }; -/** - * ks_pcie_v3_65_add_bus() - keystone add_bus post initialization - * @bus: A pointer to the PCI bus structure. - * - * This sets BAR0 to enable inbound access for MSI_IRQ register - */ -static int ks_pcie_v3_65_add_bus(struct pci_bus *bus) -{ - struct dw_pcie_rp *pp = bus->sysdata; - struct dw_pcie *pci = to_dw_pcie_from_pp(pp); - struct keystone_pcie *ks_pcie = to_keystone_pcie(pci); - - if (!pci_is_root_bus(bus)) - return 0; - - /* Configure and set up BAR0 */ - ks_pcie_set_dbi_mode(ks_pcie); - - /* Enable BAR0 */ - dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 1); - dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, SZ_4K - 1); - - ks_pcie_clear_dbi_mode(ks_pcie); - - /* - * For BAR0, just setting bus address for inbound writes (MSI) should - * be sufficient. Use physical address to avoid any conflicts. - */ - dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, ks_pcie->app.start); - - return 0; -} - static struct pci_ops ks_pcie_ops = { .map_bus = dw_pcie_own_conf_map_bus, .read = pci_generic_config_read, .write = pci_generic_config_write, - .add_bus = ks_pcie_v3_65_add_bus, }; /** @@ -1270,6 +1236,29 @@ static int ks_pcie_probe(struct platform_device *pdev) ret = dw_pcie_host_init(&pci->pp); if (ret < 0) goto err_get_sync; + + if (!ks_pcie->is_am6) { + /* + * For v3.65 DWC PCIe Controllers, setup BAR0 to enable + * inbound access for MSI_IRQ register. + */ + + /* Configure and set up BAR0 */ + ks_pcie_set_dbi_mode(ks_pcie); + + /* Enable BAR0 */ + dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 1); + dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, SZ_4K - 1); + + ks_pcie_clear_dbi_mode(ks_pcie); + + /* + * For BAR0, just setting bus address for inbound writes (MSI) should + * be sufficient. Use physical address to avoid any conflicts. + */ + dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, ks_pcie->app.start); + } + break; case DW_PCIE_EP_TYPE: if (!IS_ENABLED(CONFIG_PCI_KEYSTONE_EP)) { Please review and let me know if this looks fine. If so, I will post the patch for it.
Hi Siddharth, Bjorn On Wed, Oct 25, 2023 at 10:32:50AM +0530, Siddharth Vadapalli wrote: > Hello Bjorn, > > On 24/10/23 02:56, Bjorn Helgaas wrote: > > On Mon, Oct 23, 2023 at 05:05:30PM +0530, Siddharth Vadapalli wrote: > >> On 23/10/23 16:12, Serge Semin wrote: > >> > >> ... > >> > >>> 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? > >> > >> Unfortunately I don't have any SoC/Device with me that has the v3.65 PCIe > >> controller, so I will not be able to test Bjorn's suggestion. > > > > Huh. 57e1d8206e48 ("MAINTAINERS: move Murali Karicheri to credits") > > removed the maintainer for pci-keystone.c, so the driver hasn't had a > > maintainer for over two years. > > > > Given the fact that there's no maintainer, I'm more than happy to take > > a patch to move this code to somewhere in the host_init() callback, > > even if you don't have the hardware to test it. > > Sure, I can work on a patch for it. The execution flow with the existing code is > as follows: > > ks_pcie_probe() > dw_pcie_host_init() > pci_host_probe() > ks_pcie_v3_65_add_bus() > > So I assume that as long as ks_pcie_v3_65_add_bus() is invoked after > pci_host_probe(), it should be acceptable. With this understanding, I plan to > move it immediately after the invocation to dw_pcie_host_init() within > ks_pcie_probe() conditionally (based on the is_am6 flag). The new call trace > with this change will look like: > > ks_pcie_probe() > dw_pcie_host_init() > pci_host_probe() > ks_pcie_v3_65_add_bus() I guess what Bjorn implied was to add the ks_pcie_v3_65_add_bus() invocation to the host_init() callback, i.e. in ks_pcie_host_init(). Moreover initializing the BARs/MSI after all the PCIe hierarchy has been probed will eventually cause problems since MSI's won't work until the ks_pcie_v3_65_add_bus() is called. > > Since the .add_bus() method will be removed following this change, I suppose > that the patch I post for it can be applied instead of this v3 patch which fixes > pci_ops. > > The patch I plan to post as a replacement for the current patch which shall also > remove the ks_pcie_v3_65_add_bus() and the .add_bus() method is: Just a note. Seeing the Bjorn's suggestion may cause a regression on the Keystone PCIe Host v3.65 I would suggest to merge in the original fix as is and post an additional cleanup patch, like below with proper modifications, on top of it. Thus we'll minimize a risk to break things at least on the stable kernels. -Serge(y) > > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c > index 0def919f89fa..3933e857ecaa 100644 > --- a/drivers/pci/controller/dwc/pci-keystone.c > +++ b/drivers/pci/controller/dwc/pci-keystone.c > @@ -447,44 +447,10 @@ static struct pci_ops ks_child_pcie_ops = { > .write = pci_generic_config_write, > }; > > -/** > - * ks_pcie_v3_65_add_bus() - keystone add_bus post initialization > - * @bus: A pointer to the PCI bus structure. > - * > - * This sets BAR0 to enable inbound access for MSI_IRQ register > - */ > -static int ks_pcie_v3_65_add_bus(struct pci_bus *bus) > -{ > - struct dw_pcie_rp *pp = bus->sysdata; > - struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > - struct keystone_pcie *ks_pcie = to_keystone_pcie(pci); > - > - if (!pci_is_root_bus(bus)) > - return 0; > - > - /* Configure and set up BAR0 */ > - ks_pcie_set_dbi_mode(ks_pcie); > - > - /* Enable BAR0 */ > - dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 1); > - dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, SZ_4K - 1); > - > - ks_pcie_clear_dbi_mode(ks_pcie); > - > - /* > - * For BAR0, just setting bus address for inbound writes (MSI) should > - * be sufficient. Use physical address to avoid any conflicts. > - */ > - dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, ks_pcie->app.start); > - > - return 0; > -} > - > static struct pci_ops ks_pcie_ops = { > .map_bus = dw_pcie_own_conf_map_bus, > .read = pci_generic_config_read, > .write = pci_generic_config_write, > - .add_bus = ks_pcie_v3_65_add_bus, > }; > > /** > @@ -1270,6 +1236,29 @@ static int ks_pcie_probe(struct platform_device *pdev) > ret = dw_pcie_host_init(&pci->pp); > if (ret < 0) > goto err_get_sync; > + > + if (!ks_pcie->is_am6) { > + /* > + * For v3.65 DWC PCIe Controllers, setup BAR0 to enable > + * inbound access for MSI_IRQ register. > + */ > + > + /* Configure and set up BAR0 */ > + ks_pcie_set_dbi_mode(ks_pcie); > + > + /* Enable BAR0 */ > + dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 1); > + dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, SZ_4K - 1); > + > + ks_pcie_clear_dbi_mode(ks_pcie); > + > + /* > + * For BAR0, just setting bus address for inbound writes (MSI) should > + * be sufficient. Use physical address to avoid any conflicts. > + */ > + dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, ks_pcie->app.start); > + } > + > break; > case DW_PCIE_EP_TYPE: > if (!IS_ENABLED(CONFIG_PCI_KEYSTONE_EP)) { > > Please review and let me know if this looks fine. If so, I will post the patch for it. > > -- > Regards, > Siddharth.
Serge, On 25/10/23 15:58, Serge Semin wrote: > Hi Siddharth, Bjorn > > On Wed, Oct 25, 2023 at 10:32:50AM +0530, Siddharth Vadapalli wrote: ... >> > >> ks_pcie_probe() >> dw_pcie_host_init() >> pci_host_probe() >> ks_pcie_v3_65_add_bus() > > I guess what Bjorn implied was to add the ks_pcie_v3_65_add_bus() > invocation to the host_init() callback, i.e. in ks_pcie_host_init(). I misunderstood the term "host_init()", and interpret it as "dw_pcie_host_init()", due to which I thought I will add it after dw_pcie_host_init() invocation in ks_pcie_probe. Thank you for clarifying. > Moreover initializing the BARs/MSI after all the PCIe hierarchy has > been probed will eventually cause problems since MSI's won't work > until the ks_pcie_v3_65_add_bus() is called. > >> >> Since the .add_bus() method will be removed following this change, I suppose >> that the patch I post for it can be applied instead of this v3 patch which fixes >> pci_ops. >> > >> The patch I plan to post as a replacement for the current patch which shall also >> remove the ks_pcie_v3_65_add_bus() and the .add_bus() method is: > > Just a note. Seeing the Bjorn's suggestion may cause a regression on > the Keystone PCIe Host v3.65 I would suggest to merge in the original > fix as is and post an additional cleanup patch, like below with proper > modifications, on top of it. Thus we'll minimize a risk to break > things at least on the stable kernels. I will post it as a separate cleanup patch in that case and this v3 patch can be merged independently as you suggested.
Bjorn, Serge, On 25/10/23 16:28, Siddharth Vadapalli wrote: > Serge, > > I will post it as a separate cleanup patch in that case and this v3 patch can be > merged independently as you suggested. I have posted the patch as an RFC patch at: https://lore.kernel.org/r/20231027084159.4166188-1-s-vadapalli@ti.com/
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@ti.com> > > LGTM. Thanks! > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > > 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@ti.com/ >> >> 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@ti.com/ >> >> 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 >>
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; + } ret = ks_pcie_config_legacy_irq(ks_pcie); if (ret)
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@ti.com> --- 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@ti.com/ 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@ti.com/ 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(-)