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 <fancer.lancer@xxxxxxxxx> > Suggested-by: Bjorn Helgaas <helgaas@xxxxxxxxxx> > Signed-off-by: Siddharth Vadapalli <s-vadapalli@xxxxxx> > --- > Hello, > > This patch is based on linux-next tagged next-20231027. > This patch depends on the patch at: > https://lore.kernel.org/r/20231019081330.2975470-1-s-vadapalli@xxxxxx/ > > 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 >
