Re: [PATCH v3 2/2] ARM: pci: kill pcibios_msi_controller

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2015. 7. 29., at AM 6:13, Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx> wrote:
> 
>> On Wed, Jul 29, 2015 at 09:32:01AM +0100, Jingoo Han wrote:
>> 
>>> On 2015. 7. 28., at PM 7:32, Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx> wrote:
>>> 
>>> On ARM PCI systems relying on the pcibios API to initialize PCI host
>>> controllers, the pcibios_msi_controller weak callback is used to look-up
>>> the msi_controller pointer, through pci_sys_data msi_ctrl pointer.
>>> 
>>> pci_sys_data is an ARM specific structure, which prevents using the
>>> same mechanism (so same PCI host controller drivers) on ARM64 systems.
>>> 
>>> Since the struct pci_bus already contains an msi_controller pointer and
>>> the kernel already uses it to look-up the msi controller,
>>> this patch converts ARM host controller and related pcibios/host bridges
>>> initialization routines so that the msi_controller pointer look-up can be
>>> carried out by PCI core code through the struct pci_bus msi pointer,
>>> removing the need for the arch specific pcibios_msi_controller callback
>>> and the related pci_sys_data msi_ctrl pointer.
>>> 
>>> To simplify the conversion, this patch adds a new function in PCI
>>> core code (pci_scan_root_bus_msi()) that takes the msi_controller
>>> pointer as an additional parameter wrt pci_scan_root_bus() so that
>>> the msi controller pointer can be effectively propagated at probe time
>>> through the augmented API.
>>> 
>>> The existing pci_scan_root_bus() API is made to rely on the newly
>>> introduced function, by passing a NULL msi pointer to it so
>>> that it can be used when no msi controller pointer passing is required
>>> without additional code (and API conversions) in the core PCI layer.
>>> 
>>> ARM is the only arch relying on the pcibios_msi_controller() weak
>>> function, hence this patch also removes its default weak implementation
>>> from PCI core code since it becomes of no use.
>>> 
>>> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
>>> Suggested-by: Russell King <linux@xxxxxxxxxxxxxxxx>
>>> Acked-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>>> Cc: Pratyush Anand <pratyush.anand@xxxxxxxxx>
>>> Cc: Arnd Bergmann <arnd@xxxxxxxx>
>>> Cc: Jingoo Han <jingoohan1@xxxxxxxxx>
>>> Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
>>> Cc: Simon Horman <horms@xxxxxxxxxxxx>
>>> Cc: Russell King <linux@xxxxxxxxxxxxxxxx>
>>> Cc: Thomas Petazzoni <thomas.petazzoni@xxxxxxxxxxxxxxxxxx>
>>> Cc: Thierry Reding <thierry.reding@xxxxxxxxx>
>>> Cc: Michal Simek <michal.simek@xxxxxxxxxx>
>>> Cc: Marc Zyngier <marc.zyngier@xxxxxxx>
>>> ---
>>> v2->v3
>>> 
>>> - Added pci_scan_root_bus_msi() in core PCI code and converted ARM bios32 to
>>> use it
>>> 
>>> v2: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/359183.html
>>> 
>>> v1->v2
>>> 
>>> - Added patch to replace panic statements with WARN
>>> - Removed unused pcibios_msi_controller() and pci_msi_controller() from
>>> core code
>>> - Dropped RFT status
>>> 
>>> v1: http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/356028.html
>>> 
>>> arch/arm/include/asm/mach/pci.h    |  5 -----
>>> arch/arm/kernel/bios32.c           | 17 +++--------------
>>> drivers/pci/host/pcie-designware.c |  9 +++++++--
>>> drivers/pci/host/pcie-xilinx.c     | 12 ++++++++++--
>>> drivers/pci/msi.c                  | 17 +----------------
>>> drivers/pci/probe.c                | 15 +++++++++++++--
>>> include/linux/pci.h                |  4 ++++
>>> 7 files changed, 38 insertions(+), 41 deletions(-)
>>> 
>>> diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h
>>> index 28b9bb3..8857d28 100644
>>> --- a/arch/arm/include/asm/mach/pci.h
>>> +++ b/arch/arm/include/asm/mach/pci.h
>>> @@ -19,9 +19,7 @@ struct pci_bus;
>>> struct device;
>>> 
>>> struct hw_pci {
>>> -#ifdef CONFIG_PCI_MSI
>>>   struct msi_controller *msi_ctrl;
>>> -#endif
>>>   struct pci_ops    *ops;
>>>   int        nr_controllers;
>>>   void        **private_data;
>>> @@ -42,9 +40,6 @@ struct hw_pci {
>>> * Per-controller structure
>>> */
>>> struct pci_sys_data {
>>> -#ifdef CONFIG_PCI_MSI
>>> -    struct msi_controller *msi_ctrl;
>>> -#endif
>>>   struct list_head node;
>>>   int        busnr;        /* primary bus number            */
>>>   u64        mem_offset;    /* bus->cpu memory mapping offset    */
>>> diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
>>> index a5c782c..1a20076 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 (WARN(!sys, "PCI: unable to allocate sys data!"))
>>>           break;
>>> 
>>> -#ifdef CONFIG_PCI_MSI
>>> -        sys->msi_ctrl = hw->msi_ctrl;
>>> -#endif
>>>       sys->busnr   = busnr;
>>>       sys->swizzle = hw->swizzle;
>>>       sys->map_irq = hw->map_irq;
>>> @@ -486,8 +474,9 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
>>>           if (hw->scan)
>>>               sys->bus = hw->scan(nr, sys);
>>>           else
>>> -                sys->bus = pci_scan_root_bus(parent, sys->busnr,
>>> -                        hw->ops, sys, &sys->resources);
>>> +                sys->bus = pci_scan_root_bus_msi(parent,
>>> +                    sys->busnr, hw->ops, sys,
>>> +                    &sys->resources, hw->msi_ctrl);
>>> 
>>>           if (WARN(!sys->bus, "PCI: unable to scan bus!")) {
>>>               kfree(sys);
>>> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c
>>> index 69486be..e584dfa 100644
>>> --- a/drivers/pci/host/pcie-designware.c
>>> +++ b/drivers/pci/host/pcie-designware.c
>>> @@ -526,7 +526,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
>>> 
>>> #ifdef CONFIG_PCI_MSI
>>>   dw_pcie_msi_chip.dev = pp->dev;
>>> -    dw_pci.msi_ctrl = &dw_pcie_msi_chip;
>>> #endif
>>> 
>>>   dw_pci.nr_controllers = 1;
>>> @@ -708,11 +707,17 @@ static struct pci_bus *dw_pcie_scan_bus(int nr, struct pci_sys_data *sys)
>>>   struct pcie_port *pp = sys_to_pcie(sys);
>>> 
>>>   pp->root_bus_nr = sys->busnr;
>>> -    bus = pci_scan_root_bus(pp->dev, sys->busnr,
>>> +    bus = pci_create_root_bus(pp->dev, sys->busnr,
>>>                 &dw_pcie_ops, sys, &sys->resources);
>> 
>> How about using pci_scan_root_bus_msi()?
>> This is because
>> pci_scan_root_bus_msi() adds the msi_controller to pci_bus.
>> Thus, adding msi_controller and calling pci_scan_child_bus() is not necessary
>> when pci_scan_root_bus_msi() is called here.
>> 
>> If I am wrong, please let me know kindly. Thank you.
> 
> No you are right, I will update this driver and xilinx too to use
> the new API, there is no reason to split the calls create/scan
> with the new pci_scan_root_bus_msi() API.
> 
> Did you manage to test it ?

Sorry, I cannot test it. I don't have
the Exynos boards to test MSI feature.
So, I can just review patches.

Best regards,
Jingoo Han

> Thanks !
> Lorenzo
> 
>> Best regards,
>> Jingoo Han
>> 
>>>   if (!bus)
>>>       return NULL;
>>> 
>>> +#ifdef CONFIG_PCI_MSI
>>> +    bus->msi = &dw_pcie_msi_chip;
>>> +#endif
>>> +
>>> +    pci_scan_child_bus(bus);
>>> +
>>>   if (bus && pp->ops->scan_bus)
>>>       pp->ops->scan_bus(pp);
>>> 
>>> diff --git a/drivers/pci/host/pcie-xilinx.c b/drivers/pci/host/pcie-xilinx.c
>>> index f1a06a0..b21eb7d 100644
>>> --- a/drivers/pci/host/pcie-xilinx.c
>>> +++ b/drivers/pci/host/pcie-xilinx.c
>>> @@ -647,9 +647,18 @@ static struct pci_bus *xilinx_pcie_scan_bus(int nr, struct pci_sys_data *sys)
>>>   struct pci_bus *bus;
>>> 
>>>   port->root_busno = sys->busnr;
>>> -    bus = pci_scan_root_bus(port->dev, sys->busnr, &xilinx_pcie_ops,
>>> +    bus = pci_create_root_bus(port->dev, sys->busnr, &xilinx_pcie_ops,
>>>               sys, &sys->resources);
>>> 
>>> +    if (!bus)
>>> +        return NULL;
>>> +
>>> +#ifdef CONFIG_PCI_MSI
>>> +    bus->msi = &xilinx_pcie_msi_chip;
>>> +#endif
>>> +
>>> +    pci_scan_child_bus(bus);
>>> +
>>>   return bus;
>>> }
>>> 
>>> @@ -847,7 +856,6 @@ static int xilinx_pcie_probe(struct platform_device *pdev)
>>> 
>>> #ifdef CONFIG_PCI_MSI
>>>   xilinx_pcie_msi_chip.dev = port->dev;
>>> -    hw.msi_ctrl = &xilinx_pcie_msi_chip;
>>> #endif
>>>   pci_common_init_dev(dev, &hw);
>>> 
>>> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>>> index f66be86..0d20142 100644
>>> --- a/drivers/pci/msi.c
>>> +++ b/drivers/pci/msi.c
>>> @@ -77,24 +77,9 @@ static void pci_msi_teardown_msi_irqs(struct pci_dev *dev)
>>> 
>>> /* Arch hooks */
>>> 
>>> -struct msi_controller * __weak pcibios_msi_controller(struct pci_dev *dev)
>>> -{
>>> -    return NULL;
>>> -}
>>> -
>>> -static struct msi_controller *pci_msi_controller(struct pci_dev *dev)
>>> -{
>>> -    struct msi_controller *msi_ctrl = dev->bus->msi;
>>> -
>>> -    if (msi_ctrl)
>>> -        return msi_ctrl;
>>> -
>>> -    return pcibios_msi_controller(dev);
>>> -}
>>> -
>>> int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
>>> {
>>> -    struct msi_controller *chip = pci_msi_controller(dev);
>>> +    struct msi_controller *chip = dev->bus->msi;
>>>   int err;
>>> 
>>>   if (!chip || !chip->setup_irq)
>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>> index cefd636..4915c6d 100644
>>> --- a/drivers/pci/probe.c
>>> +++ b/drivers/pci/probe.c
>>> @@ -2096,8 +2096,9 @@ void pci_bus_release_busn_res(struct pci_bus *b)
>>>           res, ret ? "can not be" : "is");
>>> }
>>> 
>>> -struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
>>> -        struct pci_ops *ops, void *sysdata, struct list_head *resources)
>>> +struct pci_bus *pci_scan_root_bus_msi(struct device *parent, int bus,
>>> +        struct pci_ops *ops, void *sysdata,
>>> +        struct list_head *resources, struct msi_controller *msi)
>>> {
>>>   struct resource_entry *window;
>>>   bool found = false;
>>> @@ -2114,6 +2115,8 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
>>>   if (!b)
>>>       return NULL;
>>> 
>>> +    b->msi = msi;
>>> +
>>>   if (!found) {
>>>       dev_info(&b->dev,
>>>        "No busn resource found for root bus, will use [bus %02x-ff]\n",
>>> @@ -2128,6 +2131,14 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
>>> 
>>>   return b;
>>> }
>>> +EXPORT_SYMBOL(pci_scan_root_bus_msi);
>>> +
>>> +struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
>>> +        struct pci_ops *ops, void *sysdata, struct list_head *resources)
>>> +{
>>> +    return pci_scan_root_bus_msi(parent, bus, ops, sysdata, resources,
>>> +                     NULL);
>>> +}
>>> EXPORT_SYMBOL(pci_scan_root_bus);
>>> 
>>> struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops,
>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>> index 8a0321a..4d4f9d2 100644
>>> --- a/include/linux/pci.h
>>> +++ b/include/linux/pci.h
>>> @@ -787,6 +787,10 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>>> int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax);
>>> int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax);
>>> void pci_bus_release_busn_res(struct pci_bus *b);
>>> +struct pci_bus *pci_scan_root_bus_msi(struct device *parent, int bus,
>>> +                      struct pci_ops *ops, void *sysdata,
>>> +                      struct list_head *resources,
>>> +                      struct msi_controller *msi);
>>> struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
>>>                        struct pci_ops *ops, void *sysdata,
>>>                        struct list_head *resources);
>>> --
>>> 2.2.1
>> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux