| Message ID | 2095085.CuXXeGT0Wg@wuerfel (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On 05/27/2015 05:43 PM, Arnd Bergmann wrote: > On Wednesday 27 May 2015 17:31:46 Arnd Bergmann wrote: >> Yes, that works. However there are two problems with the approach: >> >> - we have to change all PCI host drivers on ARM to do this in order to remove >> the ARM-specific pcibios_msi_controller() function >> - it's possible that there are dw_pcie implementations that do not include >> an MSI controller, so that pointer would be NULL, which leads to the >> core code to still call the ARM-specific pcibios_msi_controller() function >> unless we remove it. > I wonder if this simple patch would be sufficient to kill off > pcibios_msi_controller(). > > Can one of you try it? > > Arnd Hi Arnd, I tested it quickly on my platform, with and without CONFIG_PCI_MSI. In both case it seems to run fine with dw pcie. > > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c > index fcbbbb1b9e95..7afb6d253162 100644 > --- a/arch/arm/kernel/bios32.c > +++ b/arch/arm/kernel/bios32.c > @@ -18,15 +18,6 @@ > > static int debug_pci; > > -#ifdef CONFIG_PCI_MSI > -struct msi_controller *pcibios_msi_controller(struct pci_dev *dev) > -{ > - struct pci_sys_data *sysdata = dev->bus->sysdata; > - > - return sysdata->msi_ctrl; > -} > -#endif > - > /* > * We can't use pci_get_device() here since we are > * called from interrupt context. > @@ -462,9 +453,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, > if (!sys) > panic("PCI: unable to allocate sys data!"); > > -#ifdef CONFIG_PCI_MSI > - sys->msi_ctrl = hw->msi_ctrl; > -#endif > sys->busnr = busnr; > sys->swizzle = hw->swizzle; > sys->map_irq = hw->map_irq; > @@ -493,6 +481,9 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, > panic("PCI: unable to scan bus!"); > > busnr = sys->bus->busn_res.end + 1; > +#ifdef CONFIG_PCI_MSI > + bus->msi = hw->msi_ctrl; (with small adaptation here: s/bus->msi/sys->bus->msi) BR, Fabrice > +#endif > > list_add(&sys->node, head); > } else { > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015/5/27 23:43, Arnd Bergmann wrote: > On Wednesday 27 May 2015 17:31:46 Arnd Bergmann wrote: >> Yes, that works. However there are two problems with the approach: >> >> - we have to change all PCI host drivers on ARM to do this in order to remove >> the ARM-specific pcibios_msi_controller() function >> - it's possible that there are dw_pcie implementations that do not include >> an MSI controller, so that pointer would be NULL, which leads to the >> core code to still call the ARM-specific pcibios_msi_controller() function >> unless we remove it. > > I wonder if this simple patch would be sufficient to kill off > pcibios_msi_controller(). > > Can one of you try it? > > Arnd > > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c > index fcbbbb1b9e95..7afb6d253162 100644 > --- a/arch/arm/kernel/bios32.c > +++ b/arch/arm/kernel/bios32.c > @@ -18,15 +18,6 @@ > > static int debug_pci; > > -#ifdef CONFIG_PCI_MSI > -struct msi_controller *pcibios_msi_controller(struct pci_dev *dev) > -{ > - struct pci_sys_data *sysdata = dev->bus->sysdata; > - > - return sysdata->msi_ctrl; > -} > -#endif > - > /* > * We can't use pci_get_device() here since we are > * called from interrupt context. > @@ -462,9 +453,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, > if (!sys) > panic("PCI: unable to allocate sys data!"); > > -#ifdef CONFIG_PCI_MSI > - sys->msi_ctrl = hw->msi_ctrl; > -#endif > sys->busnr = busnr; > sys->swizzle = hw->swizzle; > sys->map_irq = hw->map_irq; > @@ -493,6 +481,9 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, > panic("PCI: unable to scan bus!"); > > busnr = sys->bus->busn_res.end + 1; > +#ifdef CONFIG_PCI_MSI > + bus->msi = hw->msi_ctrl; > +#endif > > list_add(&sys->node, head); > } else { > Hi Arnd, I think it does not work in above way(with adaptation as Fabrice mentioned). As the msi controller is passed to secondary bus one by one during the process of enumeration. Here we just set msi controller for root bus. Best Regards, Zhou > > . > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015/5/28 0:19, Fabrice Gasnier wrote: > On 05/27/2015 05:43 PM, Arnd Bergmann wrote: >> On Wednesday 27 May 2015 17:31:46 Arnd Bergmann wrote: >>> Yes, that works. However there are two problems with the approach: >>> >>> - we have to change all PCI host drivers on ARM to do this in order to remove >>> the ARM-specific pcibios_msi_controller() function >>> - it's possible that there are dw_pcie implementations that do not include >>> an MSI controller, so that pointer would be NULL, which leads to the >>> core code to still call the ARM-specific pcibios_msi_controller() function >>> unless we remove it. >> I wonder if this simple patch would be sufficient to kill off >> pcibios_msi_controller(). >> >> Can one of you try it? >> >> Arnd > > Hi Arnd, > I tested it quickly on my platform, with and without CONFIG_PCI_MSI. > In both case it seems to run fine with dw pcie. > Hi Fabrice, Thanks for testing. But if you applied this patch, the code will not go into pcibios_init_hw(), so it did not touch what Arnd modified. Best Regards, Zhou >> >> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c >> index fcbbbb1b9e95..7afb6d253162 100644 >> --- a/arch/arm/kernel/bios32.c >> +++ b/arch/arm/kernel/bios32.c >> @@ -18,15 +18,6 @@ >> static int debug_pci; >> -#ifdef CONFIG_PCI_MSI >> -struct msi_controller *pcibios_msi_controller(struct pci_dev *dev) >> -{ >> - struct pci_sys_data *sysdata = dev->bus->sysdata; >> - >> - return sysdata->msi_ctrl; >> -} >> -#endif >> - >> /* >> * We can't use pci_get_device() here since we are >> * called from interrupt context. >> @@ -462,9 +453,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, >> if (!sys) >> panic("PCI: unable to allocate sys data!"); >> -#ifdef CONFIG_PCI_MSI >> - sys->msi_ctrl = hw->msi_ctrl; >> -#endif >> sys->busnr = busnr; >> sys->swizzle = hw->swizzle; >> sys->map_irq = hw->map_irq; >> @@ -493,6 +481,9 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, >> panic("PCI: unable to scan bus!"); >> busnr = sys->bus->busn_res.end + 1; >> +#ifdef CONFIG_PCI_MSI >> + bus->msi = hw->msi_ctrl; > > (with small adaptation here: s/bus->msi/sys->bus->msi) > > BR, > Fabrice >> +#endif >> list_add(&sys->node, head); >> } else { >> > > > . > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 28 May 2015 19:34:52 Zhou Wang wrote: > > @@ -462,9 +453,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, > > if (!sys) > > panic("PCI: unable to allocate sys data!"); > > > > -#ifdef CONFIG_PCI_MSI > > - sys->msi_ctrl = hw->msi_ctrl; > > -#endif > > sys->busnr = busnr; > > sys->swizzle = hw->swizzle; > > sys->map_irq = hw->map_irq; > > @@ -493,6 +481,9 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, > > panic("PCI: unable to scan bus!"); > > > > busnr = sys->bus->busn_res.end + 1; > > +#ifdef CONFIG_PCI_MSI > > + bus->msi = hw->msi_ctrl; > > +#endif > > > > list_add(&sys->node, head); > > } else { > > > > Hi Arnd, > > I think it does not work in above way(with adaptation as Fabrice mentioned). > As the msi controller is passed to secondary bus one by one during the process of enumeration. > Here we just set msi controller for root bus. Ah, too bad. I guess you are right, the bus->msi pointer here would really need to be set between pci_create_root_bus() and pci_scan_child_bus(), but we currently don't run any ARM specific code between the two. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c index fcbbbb1b9e95..7afb6d253162 100644 --- a/arch/arm/kernel/bios32.c +++ b/arch/arm/kernel/bios32.c @@ -18,15 +18,6 @@ static int debug_pci; -#ifdef CONFIG_PCI_MSI -struct msi_controller *pcibios_msi_controller(struct pci_dev *dev) -{ - struct pci_sys_data *sysdata = dev->bus->sysdata; - - return sysdata->msi_ctrl; -} -#endif - /* * We can't use pci_get_device() here since we are * called from interrupt context. @@ -462,9 +453,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, if (!sys) panic("PCI: unable to allocate sys data!"); -#ifdef CONFIG_PCI_MSI - sys->msi_ctrl = hw->msi_ctrl; -#endif sys->busnr = busnr; sys->swizzle = hw->swizzle; sys->map_irq = hw->map_irq; @@ -493,6 +481,9 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, panic("PCI: unable to scan bus!"); busnr = sys->bus->busn_res.end + 1; +#ifdef CONFIG_PCI_MSI + bus->msi = hw->msi_ctrl; +#endif list_add(&sys->node, head); } else {