2023-10-27 08:44:11

by Siddharth Vadapalli

[permalink] [raw]
Subject: [RFC PATCH] PCI: keystone: Refactor ks_pcie_v3_65_add_bus() functionality

The .add_bus() callback "ks_pcie_v3_65_add_bus()" exists to setup BAR0 for
MSI configuration. This method is expected to be invoked after the
enumeration of endpoints for the v3.65a DWC PCIe IP Controller.

Based on the discussion at [0], the contents of "ks_pcie_v3_65_add_bus()"
can be moved to the .host_init callback "ks_pcie_host_init()" and the
.add_bus callback within "struct pci_ops ks_pcie_ops" is no longer
required.

Hence, for the v3.65a DWC PCIe IP Controllers (!ks_pcie->is_am6), perform
the MSI specific configuration of BAR0 within "ks_pcie_host_init()"
itself.

[0] https://lore.kernel.org/r/20231019220847.GA1413474@bhelgaas/

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-20231027.
This patch depends on the patch at:
https://lore.kernel.org/r/[email protected]/

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 e9245b7632c5..369f5e556df3 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,
};

static struct pci_ops ks_pcie_am6_ops = {
@@ -834,6 +800,23 @@ static int __init ks_pcie_host_init(struct dw_pcie_rp *pp)
if (ret < 0)
return ret;

+ if (!ks_pcie->is_am6) {
+ /* 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.34.1


2023-10-27 12:42:43

by Serge Semin

[permalink] [raw]
Subject: Re: [RFC PATCH] PCI: keystone: Refactor ks_pcie_v3_65_add_bus() functionality

On Fri, Oct 27, 2023 at 02:11:59PM +0530, Siddharth Vadapalli wrote:
> The .add_bus() callback "ks_pcie_v3_65_add_bus()" exists to setup BAR0 for
> MSI configuration. This method is expected to be invoked after the
> enumeration of endpoints for the v3.65a DWC PCIe IP Controller.
>
> Based on the discussion at [0], the contents of "ks_pcie_v3_65_add_bus()"
> can be moved to the .host_init callback "ks_pcie_host_init()" and the
> .add_bus callback within "struct pci_ops ks_pcie_ops" is no longer
> required.
>
> Hence, for the v3.65a DWC PCIe IP Controllers (!ks_pcie->is_am6), perform
> the MSI specific configuration of BAR0 within "ks_pcie_host_init()"
> itself.
>
> [0] https://lore.kernel.org/r/20231019220847.GA1413474@bhelgaas/
>
> 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-20231027.
> This patch depends on the patch at:
> https://lore.kernel.org/r/[email protected]/
>
> 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 e9245b7632c5..369f5e556df3 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,
> };
>
> static struct pci_ops ks_pcie_am6_ops = {
> @@ -834,6 +800,23 @@ static int __init ks_pcie_host_init(struct dw_pcie_rp *pp)
> if (ret < 0)
> return ret;
>

> + if (!ks_pcie->is_am6) {
> + /* 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);
> + }
> +

Seeing this is required for MSI IRQs what about moving it to the
ks_pcie_config_msi_irq() together with the BARs zeroing out performed
in the ks_pcie_setup_rc_app_regs() function especially seeing the
later function is dedicated for the 'app' regs initialization only
based on the function name. Bjorn, what do you think?

-Serge(y)

> #ifdef CONFIG_ARM
> /*
> * PCIe access errors that result into OCP errors are caught by ARM as
> --
> 2.34.1
>