On Mon, Mar 25, 2024 at 11:07:22AM +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, as a fix, move the contents of "ks_pcie_v3_65_add_bus()" to the
> .host_init callback "ks_pcie_host_init()" and execute it only for non
> AM654x SoC devices which have the v3.65a DWC PCIe IP Controllers.
>
> Fixes: 6ab15b5e7057 ("PCI: dwc: keystone: Convert .scan_bus() callback to use add_bus")
> Suggested-by: Serge Semin <[email protected]>
> Suggested-by: Bjorn Helgaas <[email protected]>
> Signed-off-by: Siddharth Vadapalli <[email protected]>
> ---
>
> Hello,
>
> This patch is based on linux-next tagged next-20240325.
> This patch is technically the next version for the v3 patch at:
> https://patchwork.kernel.org/project/linux-pci/patch/[email protected]/
> but the implementation is based on the RFC patch at:
> https://patchwork.kernel.org/project/linux-pci/patch/[email protected]/
> Since the RFC patch mentioned above fixes the same issue being
> fixed by the v3 patch, I have dropped the v3 patch and am using
> the RFC patch since it is a cleaner implementation and was discussed at:
> https://lore.kernel.org/r/20231019220847.GA1413474@bhelgaas/
>
> Regards,
> Siddharth.
>
> drivers/pci/controller/dwc/pci-keystone.c | 51 ++++++++---------------
> 1 file changed, 17 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index 844de4418724..f45bdeac520a 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -445,44 +445,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,
> };
>
> /**
> @@ -822,6 +788,23 @@ static int __init ks_pcie_host_init(struct dw_pcie_rp *pp)
> if (ret < 0)
> return ret;
>
> + if (!ks_pcie->is_am6) {
Perhaps add a comment here stating WHY this is needed for v3.65a (!is_am6).
From reading the old threads, it appears that v3.65a:
-Has no support for iATUs. iATU-specific resource handling code is to be
bypassed for v3.65 h/w. Thus v3.65a has it's own .child_ops implementation,
so that pcie-designware-host.c does not configure the iATUs.
-v3.65a has it's own .msi_init implementation, so that pcie-designware-host.c
does not call dw_pcie_msi_host_init() to configure the MSI controller.
While 4.90a:
-Does have iATU support.
-Does use the generic dw_pcie_msi_host_init().
Considering the major differences (with v3.65a being the outlier) here,
I think it would have been a much wiser idea to have two different glue
drivers for these two compatibles (ti,keystone-pcie and ti,am654-pcie-rc).
Right now the driver is quite hard to read, most of the functions in this
driver exist because v3.65a does not have an iATU and does not use the
generic DWC way to handle MSIs. Additionally, you have "if (!ks_pcie->is_am6)"
spread out all over the driver, to control quite major things, like if you
should overload .child_ops, or if you should set up inbound translation without
an iATU. This makes is even harder to see which code is actually used for
am654... like the fact that it actually uses the generic way to handle MSIs...
The driver for am654 would be much nicer since many of the functions in
this driver would not be needed (and the fact that you have only implemented
EP support for am654 and not for v3.65a). All EP related stuff would be in
the am654 file/driver.
You could keep the quirky stuff for v3.65a in the existing pci-keystone.c
driver.
(I guess if there is a function that is identical between the twos, you could
have a pci-keystone-common.{c,h} that can be used by both drivers, but from
the looks of it, they seem to share very little code.
Kind regards,
Niklas
> + /* 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);
> + }
> +
> #ifdef CONFIG_ARM
> /*
> * PCIe access errors that result into OCP errors are caught by ARM as
> --
> 2.40.1
>
On Mon, Mar 25, 2024 at 12:23:05PM +0100, Niklas Cassel wrote:
> On Mon, Mar 25, 2024 at 11:07:22AM +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, as a fix, move the contents of "ks_pcie_v3_65_add_bus()" to the
> > .host_init callback "ks_pcie_host_init()" and execute it only for non
> > AM654x SoC devices which have the v3.65a DWC PCIe IP Controllers.
> >
> > Fixes: 6ab15b5e7057 ("PCI: dwc: keystone: Convert .scan_bus() callback to use add_bus")
> > Suggested-by: Serge Semin <[email protected]>
> > Suggested-by: Bjorn Helgaas <[email protected]>
> > Signed-off-by: Siddharth Vadapalli <[email protected]>
> > ---
> >
> > Hello,
> >
> > This patch is based on linux-next tagged next-20240325.
> > This patch is technically the next version for the v3 patch at:
> > https://patchwork.kernel.org/project/linux-pci/patch/[email protected]/
> > but the implementation is based on the RFC patch at:
> > https://patchwork.kernel.org/project/linux-pci/patch/[email protected]/
> > Since the RFC patch mentioned above fixes the same issue being
> > fixed by the v3 patch, I have dropped the v3 patch and am using
> > the RFC patch since it is a cleaner implementation and was discussed at:
> > https://lore.kernel.org/r/20231019220847.GA1413474@bhelgaas/
> >
> > Regards,
> > Siddharth.
> >
> > drivers/pci/controller/dwc/pci-keystone.c | 51 ++++++++---------------
> > 1 file changed, 17 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> > index 844de4418724..f45bdeac520a 100644
> > --- a/drivers/pci/controller/dwc/pci-keystone.c
> > +++ b/drivers/pci/controller/dwc/pci-keystone.c
> > @@ -445,44 +445,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,
> > };
> >
> > /**
> > @@ -822,6 +788,23 @@ static int __init ks_pcie_host_init(struct dw_pcie_rp *pp)
> > if (ret < 0)
> > return ret;
> >
>
> > + if (!ks_pcie->is_am6) {
>
> Perhaps add a comment here stating WHY this is needed for v3.65a (!is_am6).
>
> From reading the old threads, it appears that v3.65a:
> -Has no support for iATUs. iATU-specific resource handling code is to be
> bypassed for v3.65 h/w. Thus v3.65a has it's own .child_ops implementation,
> so that pcie-designware-host.c does not configure the iATUs.
> -v3.65a has it's own .msi_init implementation, so that pcie-designware-host.c
> does not call dw_pcie_msi_host_init() to configure the MSI controller.
>
> While 4.90a:
> -Does have iATU support.
> -Does use the generic dw_pcie_msi_host_init().
>
> Considering the major differences (with v3.65a being the outlier) here,
> I think it would have been a much wiser idea to have two different glue
> drivers for these two compatibles (ti,keystone-pcie and ti,am654-pcie-rc).
>
> Right now the driver is quite hard to read, most of the functions in this
> driver exist because v3.65a does not have an iATU and does not use the
> generic DWC way to handle MSIs. Additionally, you have "if (!ks_pcie->is_am6)"
> spread out all over the driver, to control quite major things, like if you
> should overload .child_ops, or if you should set up inbound translation without
> an iATU. This makes is even harder to see which code is actually used for
> am654... like the fact that it actually uses the generic way to handle MSIs...
>
> The driver for am654 would be much nicer since many of the functions in
> this driver would not be needed (and the fact that you have only implemented
> EP support for am654 and not for v3.65a). All EP related stuff would be in
> the am654 file/driver.
> You could keep the quirky stuff for v3.65a in the existing pci-keystone.c
> driver.
>
> (I guess if there is a function that is identical between the twos, you could
> have a pci-keystone-common.{c,h} that can be used by both drivers, but from
> the looks of it, they seem to share very little code.
Thank you for reviewing the patch. I agree that two drivers will be
better considering the !ks_pcie->is_am6 present throughout the driver.
However, I hope you notice the fact that commit:
6ab15b5e7057 PCI: dwc: keystone: Convert .scan_bus() callback to use add_bus
introduced a regression in a driver which was working prior to that
commit for AM654. While there are flaws in the driver and it needs to be
split to handle v3.65a and other versions in a cleaner manner, I am
unable to understand why that is a precursor to fixing the regression.
If splitting the driver is the only way to fix this regression, please
let me know and I will work on that instead, though it will take up more
time.
Regards,
Siddharth.
Hello Siddharth,
On Mon, Mar 25, 2024 at 05:52:28PM +0530, Siddharth Vadapalli wrote:
> On Mon, Mar 25, 2024 at 12:23:05PM +0100, Niklas Cassel wrote:
> > On Mon, Mar 25, 2024 at 11:07:22AM +0530, Siddharth Vadapalli wrote:
> > > @@ -822,6 +788,23 @@ static int __init ks_pcie_host_init(struct dw_pcie_rp *pp)
> > > if (ret < 0)
> > > return ret;
> > >
> >
> > > + if (!ks_pcie->is_am6) {
> >
> > Perhaps add a comment here stating WHY this is needed for v3.65a (!is_am6).
> >
> > From reading the old threads, it appears that v3.65a:
> > -Has no support for iATUs. iATU-specific resource handling code is to be
> > bypassed for v3.65 h/w. Thus v3.65a has it's own .child_ops implementation,
> > so that pcie-designware-host.c does not configure the iATUs.
> > -v3.65a has it's own .msi_init implementation, so that pcie-designware-host.c
> > does not call dw_pcie_msi_host_init() to configure the MSI controller.
> >
> > While 4.90a:
> > -Does have iATU support.
> > -Does use the generic dw_pcie_msi_host_init().
> >
> > Considering the major differences (with v3.65a being the outlier) here,
> > I think it would have been a much wiser idea to have two different glue
> > drivers for these two compatibles (ti,keystone-pcie and ti,am654-pcie-rc).
> >
> > Right now the driver is quite hard to read, most of the functions in this
> > driver exist because v3.65a does not have an iATU and does not use the
> > generic DWC way to handle MSIs. Additionally, you have "if (!ks_pcie->is_am6)"
> > spread out all over the driver, to control quite major things, like if you
> > should overload .child_ops, or if you should set up inbound translation without
> > an iATU. This makes is even harder to see which code is actually used for
> > am654... like the fact that it actually uses the generic way to handle MSIs...
> >
> > The driver for am654 would be much nicer since many of the functions in
> > this driver would not be needed (and the fact that you have only implemented
> > EP support for am654 and not for v3.65a). All EP related stuff would be in
> > the am654 file/driver.
> > You could keep the quirky stuff for v3.65a in the existing pci-keystone.c
> > driver.
> >
> > (I guess if there is a function that is identical between the twos, you could
> > have a pci-keystone-common.{c,h} that can be used by both drivers, but from
> > the looks of it, they seem to share very little code.
>
> Thank you for reviewing the patch. I agree that two drivers will be
> better considering the !ks_pcie->is_am6 present throughout the driver.
> However, I hope you notice the fact that commit:
> 6ab15b5e7057 PCI: dwc: keystone: Convert .scan_bus() callback to use add_bus
> introduced a regression in a driver which was working prior to that
> commit for AM654. While there are flaws in the driver and it needs to be
> split to handle v3.65a and other versions in a cleaner manner, I am
> unable to understand why that is a precursor to fixing the regression.
>
> If splitting the driver is the only way to fix this regression, please
> let me know and I will work on that instead, though it will take up more
> time.
I think you are misunderstanding me.
I think this patch is fine, except for the comment that I gave:
"Perhaps add a comment here stating WHY this is needed for v3.65a (!is_am6)."
Like:
/*
* This is only needed for !am654 since it has its own msi_irq_chip
* implementation. (am654 uses the generic msi_irq_chip implementation.)
*/
if (!ks_pcie->is_am6) {
...
}
In fact, if you move this code to ks_pcie_msi_host_init(), instead of
ks_pcie_host_init(), you would not need a comment (or a if (!ks_pcie->is_am6)),
since ks_pcie_msi_host_init() is only executed by !am654.
My suggestion to split this driver to two different drivers is just because
I noticed how different they are (am654 has iATUs, uses generic msi_irq_chip
implementation and has EP-mode support. !am654 has no iATUs, its own MSI
implementation and no EP-mode support.)
So the am654 driver would look like most other DWC glue drivers.
The non-am654 driver would look mostly like it looks today, except you would
remove the EP-mode support.
However, this suggestion can of course be implemented sometime in the future
and should not be a blocker for the patch in $subject.
Kind regards,
Niklas
On Mon, Mar 25, 2024 at 02:45:09PM +0100, Niklas Cassel wrote:
> Hello Siddharth,
>
> On Mon, Mar 25, 2024 at 05:52:28PM +0530, Siddharth Vadapalli wrote:
> > On Mon, Mar 25, 2024 at 12:23:05PM +0100, Niklas Cassel wrote:
> > > On Mon, Mar 25, 2024 at 11:07:22AM +0530, Siddharth Vadapalli wrote:
> > > > @@ -822,6 +788,23 @@ static int __init ks_pcie_host_init(struct dw_pcie_rp *pp)
> > > > if (ret < 0)
> > > > return ret;
> > > >
> > >
> > > > + if (!ks_pcie->is_am6) {
> > >
> > > Perhaps add a comment here stating WHY this is needed for v3.65a (!is_am6).
> > >
> > > From reading the old threads, it appears that v3.65a:
> > > -Has no support for iATUs. iATU-specific resource handling code is to be
> > > bypassed for v3.65 h/w. Thus v3.65a has it's own .child_ops implementation,
> > > so that pcie-designware-host.c does not configure the iATUs.
> > > -v3.65a has it's own .msi_init implementation, so that pcie-designware-host.c
> > > does not call dw_pcie_msi_host_init() to configure the MSI controller.
> > >
> > > While 4.90a:
> > > -Does have iATU support.
> > > -Does use the generic dw_pcie_msi_host_init().
> > >
> > > Considering the major differences (with v3.65a being the outlier) here,
> > > I think it would have been a much wiser idea to have two different glue
> > > drivers for these two compatibles (ti,keystone-pcie and ti,am654-pcie-rc).
> > >
> > > Right now the driver is quite hard to read, most of the functions in this
> > > driver exist because v3.65a does not have an iATU and does not use the
> > > generic DWC way to handle MSIs. Additionally, you have "if (!ks_pcie->is_am6)"
> > > spread out all over the driver, to control quite major things, like if you
> > > should overload .child_ops, or if you should set up inbound translation without
> > > an iATU. This makes is even harder to see which code is actually used for
> > > am654... like the fact that it actually uses the generic way to handle MSIs...
> > >
> > > The driver for am654 would be much nicer since many of the functions in
> > > this driver would not be needed (and the fact that you have only implemented
> > > EP support for am654 and not for v3.65a). All EP related stuff would be in
> > > the am654 file/driver.
> > > You could keep the quirky stuff for v3.65a in the existing pci-keystone.c
> > > driver.
> > >
> > > (I guess if there is a function that is identical between the twos, you could
> > > have a pci-keystone-common.{c,h} that can be used by both drivers, but from
> > > the looks of it, they seem to share very little code.
> >
> > Thank you for reviewing the patch. I agree that two drivers will be
> > better considering the !ks_pcie->is_am6 present throughout the driver.
> > However, I hope you notice the fact that commit:
> > 6ab15b5e7057 PCI: dwc: keystone: Convert .scan_bus() callback to use add_bus
> > introduced a regression in a driver which was working prior to that
> > commit for AM654. While there are flaws in the driver and it needs to be
> > split to handle v3.65a and other versions in a cleaner manner, I am
> > unable to understand why that is a precursor to fixing the regression.
> >
> > If splitting the driver is the only way to fix this regression, please
> > let me know and I will work on that instead, though it will take up more
> > time.
>
> I think you are misunderstanding me.
>
> I think this patch is fine, except for the comment that I gave:
> "Perhaps add a comment here stating WHY this is needed for v3.65a (!is_am6)."
>
> Like:
>
> /*
> * This is only needed for !am654 since it has its own msi_irq_chip
> * implementation. (am654 uses the generic msi_irq_chip implementation.)
> */
> if (!ks_pcie->is_am6) {
> ...
> }
>
>
> In fact, if you move this code to ks_pcie_msi_host_init(), instead of
> ks_pcie_host_init(), you would not need a comment (or a if (!ks_pcie->is_am6)),
> since ks_pcie_msi_host_init() is only executed by !am654.
This seems much better :)
In the current code, the execution is as follows:
ks_pcie_probe()
dw_pcie_host_init()
pci_host_probe()
ks_pcie_v3_65_add_bus()
Moving the contents of ks_pcie_v3_65_add_bus() to ks_pcie_msi_host_init()
will result in:
ks_pcie_probe()
dw_pcie_host_init()
if (pci_msi_enabled())
if (pp->ops->msi_init) {
ret = pp->ops->msi_init(pp);
ks_pcie_msi_host_init()
pci_host_probe()
I will update this patch based on your suggestion. If it's alright, may I
also add your "Suggested-by" tag for the v5 patch? Please let me know.
>
>
>
>
> My suggestion to split this driver to two different drivers is just because
> I noticed how different they are (am654 has iATUs, uses generic msi_irq_chip
> implementation and has EP-mode support. !am654 has no iATUs, its own MSI
> implementation and no EP-mode support.)
>
> So the am654 driver would look like most other DWC glue drivers.
> The non-am654 driver would look mostly like it looks today, except you would
> remove the EP-mode support.
>
> However, this suggestion can of course be implemented sometime in the future
> and should not be a blocker for the patch in $subject.
Thank you for clarifying.
Regards,
Siddharth.
On Tue, Mar 26, 2024 at 10:29:10AM +0530, Siddharth Vadapalli wrote:
> On Mon, Mar 25, 2024 at 02:45:09PM +0100, Niklas Cassel wrote:
> >
> > In fact, if you move this code to ks_pcie_msi_host_init(), instead of
> > ks_pcie_host_init(), you would not need a comment (or a if (!ks_pcie->is_am6)),
> > since ks_pcie_msi_host_init() is only executed by !am654.
>
> This seems much better :)
>
> In the current code, the execution is as follows:
>
> ks_pcie_probe()
> dw_pcie_host_init()
> pci_host_probe()
> ks_pcie_v3_65_add_bus()
>
> Moving the contents of ks_pcie_v3_65_add_bus() to ks_pcie_msi_host_init()
> will result in:
>
> ks_pcie_probe()
> dw_pcie_host_init()
> if (pci_msi_enabled())
> if (pp->ops->msi_init) {
> ret = pp->ops->msi_init(pp);
> ks_pcie_msi_host_init()
> pci_host_probe()
>
> I will update this patch based on your suggestion. If it's alright, may I
> also add your "Suggested-by" tag for the v5 patch? Please let me know.
Fine by me :)
Kind regards,
Niklas