From: Suravee Suthikulpanit <[email protected]>
This patch set introduces a new callback function to allow PCI host drivers
to specify MSI controller to be used for the child buses / devices.
This is reabased from:
git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git pci/host-generic
Suravee Suthikulpanit (2):
PCI: Add new pci_ops for setting MSI parent for PCI bus
PCI: generic: Add set_msi_parent callback
drivers/pci/host/pci-host-generic.c | 14 ++++++++++++++
drivers/pci/probe.c | 3 +++
include/linux/pci.h | 1 +
3 files changed, 18 insertions(+)
--
1.9.3
From: Suravee Suthikulpanit <[email protected]>
In the pci_scan_root_bus, pci_bus is created and passed down to:
pci_scan_child_bus
pci_scan_bridge
pci_add_new_bus
pci_alloc_child_bus
In pci_alloc_child_bus, the parent's msi_chip is propagated to child,
and the referenced by PCI devices when calling arch_setup_msi_irq().
However, in the current implementation of pci_scan_root_bus, the msi_chip
of the root_bus is not set before handing off to pci_scan_child_bus.
This patch proposes a new callback function in the struct pci_ops
to allow host controller to provide a call back for specifying
msi_chip to be used.
Cc: Bjorn Helgass <[email protected]>
Cc: Liviu Dudau <[email protected]>
Cc: Lorenzo Pieralisi <[email protected]>
Signed-off-by: Suravee Suthikulpanit <[email protected]>
---
drivers/pci/probe.c | 3 +++
include/linux/pci.h | 1 +
2 files changed, 4 insertions(+)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 5ed9930..cf7114d 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2081,6 +2081,9 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,
if (!b)
return NULL;
+ if (ops->set_msi_parent)
+ ops->set_msi_parent(b);
+
if (!found) {
dev_info(&b->dev,
"No busn resource found for root bus, will use [bus %02x-ff]\n",
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 5be8db4..6093544 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -560,6 +560,7 @@ static inline int pcibios_err_to_errno(int err)
struct pci_ops {
int (*read)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *val);
int (*write)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 val);
+ int (*set_msi_parent)(struct pci_bus *bus);
};
/*
--
1.9.3
From: Suravee Suthikulpanit <[email protected]>
This patch implement set_msi_parent callback for PCI generic host controller.
Cc: Bjorn Helgass <[email protected]>
Cc: Liviu Dudau <[email protected]>
Cc: Lorenzo Pieralisi <[email protected]>
Signed-off-by: Suravee Suthikulpanit <[email protected]>
---
drivers/pci/host/pci-host-generic.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
index 036ab1b..f9bb97a 100644
--- a/drivers/pci/host/pci-host-generic.c
+++ b/drivers/pci/host/pci-host-generic.c
@@ -42,6 +42,7 @@ struct gen_pci {
struct pci_host_bridge host;
struct gen_pci_cfg_windows cfg;
struct list_head resources;
+ struct msi_chip *mchip;
};
#ifdef CONFIG_ARM
@@ -128,9 +129,19 @@ static int gen_pci_config_write(struct pci_bus *bus, unsigned int devfn,
return PCIBIOS_SUCCESSFUL;
}
+static int gen_pci_set_msi_parent(struct pci_bus *bus)
+{
+ struct gen_pci *pci = bus_to_gen_pci(bus);
+
+ bus->msi = pci->mchip;
+
+ return PCIBIOS_SUCCESSFUL;
+}
+
static struct pci_ops gen_pci_ops = {
.read = gen_pci_config_read,
.write = gen_pci_config_write,
+ .set_msi_parent = gen_pci_set_msi_parent,
};
static const struct of_device_id gen_pci_of_match[] = {
@@ -313,6 +324,9 @@ static int gen_pci_probe(struct platform_device *pdev)
return err;
}
+ pci->mchip = of_pci_find_msi_chip_by_node(of_parse_phandle(np,
+ "msi-parent", 0));
+
#ifdef CONFIG_ARM
pci_common_init_dev(dev, &hw);
#else
--
1.9.3
Hi Suravee,
On Mon, Nov 10, 2014 at 07:24:40PM +0000, [email protected] wrote:
> From: Suravee Suthikulpanit <[email protected]>
>
> This patch implement set_msi_parent callback for PCI generic host controller.
>
> Cc: Bjorn Helgass <[email protected]>
> Cc: Liviu Dudau <[email protected]>
> Cc: Lorenzo Pieralisi <[email protected]>
> Signed-off-by: Suravee Suthikulpanit <[email protected]>
> ---
> drivers/pci/host/pci-host-generic.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> index 036ab1b..f9bb97a 100644
> --- a/drivers/pci/host/pci-host-generic.c
> +++ b/drivers/pci/host/pci-host-generic.c
> @@ -42,6 +42,7 @@ struct gen_pci {
> struct pci_host_bridge host;
> struct gen_pci_cfg_windows cfg;
> struct list_head resources;
> + struct msi_chip *mchip;
> };
>
> #ifdef CONFIG_ARM
> @@ -128,9 +129,19 @@ static int gen_pci_config_write(struct pci_bus *bus, unsigned int devfn,
> return PCIBIOS_SUCCESSFUL;
> }
>
> +static int gen_pci_set_msi_parent(struct pci_bus *bus)
> +{
> + struct gen_pci *pci = bus_to_gen_pci(bus);
> +
> + bus->msi = pci->mchip;
> +
> + return PCIBIOS_SUCCESSFUL;
> +}
I don't think this makes much sense a host controller callback. Why can't
bus->msi be set in generic code?
> static struct pci_ops gen_pci_ops = {
> .read = gen_pci_config_read,
> .write = gen_pci_config_write,
> + .set_msi_parent = gen_pci_set_msi_parent,
> };
>
> static const struct of_device_id gen_pci_of_match[] = {
> @@ -313,6 +324,9 @@ static int gen_pci_probe(struct platform_device *pdev)
> return err;
> }
>
> + pci->mchip = of_pci_find_msi_chip_by_node(of_parse_phandle(np,
> + "msi-parent", 0));
This bit should be in the generic of_pci.c code and not duplicated for
each host controller.
Will
On Tue, Nov 11, 2014 at 11:24:24AM +0000, Will Deacon wrote:
> Hi Suravee,
>
> On Mon, Nov 10, 2014 at 07:24:40PM +0000, [email protected] wrote:
> > From: Suravee Suthikulpanit <[email protected]>
> >
> > This patch implement set_msi_parent callback for PCI generic host controller.
> >
> > Cc: Bjorn Helgass <[email protected]>
> > Cc: Liviu Dudau <[email protected]>
> > Cc: Lorenzo Pieralisi <[email protected]>
> > Signed-off-by: Suravee Suthikulpanit <[email protected]>
> > ---
> > drivers/pci/host/pci-host-generic.c | 14 ++++++++++++++
> > 1 file changed, 14 insertions(+)
> >
> > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> > index 036ab1b..f9bb97a 100644
> > --- a/drivers/pci/host/pci-host-generic.c
> > +++ b/drivers/pci/host/pci-host-generic.c
> > @@ -42,6 +42,7 @@ struct gen_pci {
> > struct pci_host_bridge host;
> > struct gen_pci_cfg_windows cfg;
> > struct list_head resources;
> > + struct msi_chip *mchip;
> > };
> >
> > #ifdef CONFIG_ARM
> > @@ -128,9 +129,19 @@ static int gen_pci_config_write(struct pci_bus *bus, unsigned int devfn,
> > return PCIBIOS_SUCCESSFUL;
> > }
> >
> > +static int gen_pci_set_msi_parent(struct pci_bus *bus)
> > +{
> > + struct gen_pci *pci = bus_to_gen_pci(bus);
> > +
> > + bus->msi = pci->mchip;
> > +
> > + return PCIBIOS_SUCCESSFUL;
> > +}
>
> I don't think this makes much sense a host controller callback. Why can't
> bus->msi be set in generic code?
Because of the current way in which a bus gets created and them immediately used for
scanning devices if you use pci_scan_root_bus(). Alternative is to use pci_create_root_bus()
and increase your code size with the body of pci_scan_root_bus().
Solution is to have pci_host_bridge holding the msi_chip pointer and that gets created
before root bus, with pci_scan_root_bus() now having all the info needed to do successful
setup of scanned devices.
Best regards,
Liviu
>
> > static struct pci_ops gen_pci_ops = {
> > .read = gen_pci_config_read,
> > .write = gen_pci_config_write,
> > + .set_msi_parent = gen_pci_set_msi_parent,
> > };
> >
> > static const struct of_device_id gen_pci_of_match[] = {
> > @@ -313,6 +324,9 @@ static int gen_pci_probe(struct platform_device *pdev)
> > return err;
> > }
> >
> > + pci->mchip = of_pci_find_msi_chip_by_node(of_parse_phandle(np,
> > + "msi-parent", 0));
>
> This bit should be in the generic of_pci.c code and not duplicated for
> each host controller.
>
> Will
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
On 11/11/14 18:24, Will Deacon wrote:
>> @@ -313,6 +324,9 @@ static int gen_pci_probe(struct platform_device *pdev)
>> > return err;
>> > }
>> >
>> >+ pci->mchip = of_pci_find_msi_chip_by_node(of_parse_phandle(np,
>> >+ "msi-parent", 0));
> This bit should be in the generic of_pci.c code and not duplicated for
> each host controller.
>
> Will
I forgot to mention and include in the patch that we are also
introducing "msi-parent" binding to the generic host controller. I'll do
that in v2.
Unless this is something that we would like to add to the generic OF
binding for PCI, I think it should be in the pci-host-generic.c.
Thanks,
Suravee
On Tue, Nov 11, 2014 at 11:52:27AM +0000, Liviu Dudau wrote:
> On Tue, Nov 11, 2014 at 11:24:24AM +0000, Will Deacon wrote:
> > Hi Suravee,
> >
> > On Mon, Nov 10, 2014 at 07:24:40PM +0000, [email protected] wrote:
> > > From: Suravee Suthikulpanit <[email protected]>
> > >
> > > This patch implement set_msi_parent callback for PCI generic host controller.
> > >
> > > Cc: Bjorn Helgass <[email protected]>
> > > Cc: Liviu Dudau <[email protected]>
> > > Cc: Lorenzo Pieralisi <[email protected]>
> > > Signed-off-by: Suravee Suthikulpanit <[email protected]>
> > > ---
> > > drivers/pci/host/pci-host-generic.c | 14 ++++++++++++++
> > > 1 file changed, 14 insertions(+)
> > >
> > > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> > > index 036ab1b..f9bb97a 100644
> > > --- a/drivers/pci/host/pci-host-generic.c
> > > +++ b/drivers/pci/host/pci-host-generic.c
> > > @@ -42,6 +42,7 @@ struct gen_pci {
> > > struct pci_host_bridge host;
> > > struct gen_pci_cfg_windows cfg;
> > > struct list_head resources;
> > > + struct msi_chip *mchip;
> > > };
> > >
> > > #ifdef CONFIG_ARM
> > > @@ -128,9 +129,19 @@ static int gen_pci_config_write(struct pci_bus *bus, unsigned int devfn,
> > > return PCIBIOS_SUCCESSFUL;
> > > }
> > >
> > > +static int gen_pci_set_msi_parent(struct pci_bus *bus)
> > > +{
> > > + struct gen_pci *pci = bus_to_gen_pci(bus);
> > > +
> > > + bus->msi = pci->mchip;
> > > +
> > > + return PCIBIOS_SUCCESSFUL;
> > > +}
> >
> > I don't think this makes much sense a host controller callback. Why can't
> > bus->msi be set in generic code?
>
> Because of the current way in which a bus gets created and them immediately used for
> scanning devices if you use pci_scan_root_bus(). Alternative is to use pci_create_root_bus()
> and increase your code size with the body of pci_scan_root_bus().
>
> Solution is to have pci_host_bridge holding the msi_chip pointer and that gets created
> before root bus, with pci_scan_root_bus() now having all the info needed to do successful
> setup of scanned devices.
Why can't we add a hook like pci_bus_assign_domain_nr(), say:
pci_bus_msi_init()
in pci_create_root_bus() that does what Suravee wants in a generic way ?
What am I missing ?
Lorenzo
>
> Best regards,
> Liviu
>
> >
> > > static struct pci_ops gen_pci_ops = {
> > > .read = gen_pci_config_read,
> > > .write = gen_pci_config_write,
> > > + .set_msi_parent = gen_pci_set_msi_parent,
> > > };
> > >
> > > static const struct of_device_id gen_pci_of_match[] = {
> > > @@ -313,6 +324,9 @@ static int gen_pci_probe(struct platform_device *pdev)
> > > return err;
> > > }
> > >
> > > + pci->mchip = of_pci_find_msi_chip_by_node(of_parse_phandle(np,
> > > + "msi-parent", 0));
> >
> > This bit should be in the generic of_pci.c code and not duplicated for
> > each host controller.
> >
> > Will
> >
>
> --
> ====================
> | I would like to |
> | fix the world, |
> | but they're not |
> | giving me the |
> \ source code! /
> ---------------
> ¯\_(ツ)_/¯
[CC'ing Arnd and RobH]
On Tue, Nov 11, 2014 at 03:33:44PM +0000, Suravee Suthikulpanit wrote:
> On 11/11/14 18:24, Will Deacon wrote:
> >> @@ -313,6 +324,9 @@ static int gen_pci_probe(struct platform_device *pdev)
> >> > return err;
> >> > }
> >> >
> >> >+ pci->mchip = of_pci_find_msi_chip_by_node(of_parse_phandle(np,
> >> >+ "msi-parent", 0));
> > This bit should be in the generic of_pci.c code and not duplicated for
> > each host controller.
> >
> > Will
>
> I forgot to mention and include in the patch that we are also
> introducing "msi-parent" binding to the generic host controller. I'll do
> that in v2.
>
> Unless this is something that we would like to add to the generic OF
> binding for PCI, I think it should be in the pci-host-generic.c.
Is there a reason why we do *not* want to add this property to generic OF
PCI binding ?
Thanks,
Lorenzo
On Thu, Nov 13, 2014 at 03:22:43PM +0000, Lorenzo Pieralisi wrote:
> On Tue, Nov 11, 2014 at 11:52:27AM +0000, Liviu Dudau wrote:
> > On Tue, Nov 11, 2014 at 11:24:24AM +0000, Will Deacon wrote:
> > > Hi Suravee,
> > >
> > > On Mon, Nov 10, 2014 at 07:24:40PM +0000, [email protected] wrote:
> > > > From: Suravee Suthikulpanit <[email protected]>
> > > >
> > > > This patch implement set_msi_parent callback for PCI generic host controller.
> > > >
> > > > Cc: Bjorn Helgass <[email protected]>
> > > > Cc: Liviu Dudau <[email protected]>
> > > > Cc: Lorenzo Pieralisi <[email protected]>
> > > > Signed-off-by: Suravee Suthikulpanit <[email protected]>
> > > > ---
> > > > drivers/pci/host/pci-host-generic.c | 14 ++++++++++++++
> > > > 1 file changed, 14 insertions(+)
> > > >
> > > > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c
> > > > index 036ab1b..f9bb97a 100644
> > > > --- a/drivers/pci/host/pci-host-generic.c
> > > > +++ b/drivers/pci/host/pci-host-generic.c
> > > > @@ -42,6 +42,7 @@ struct gen_pci {
> > > > struct pci_host_bridge host;
> > > > struct gen_pci_cfg_windows cfg;
> > > > struct list_head resources;
> > > > + struct msi_chip *mchip;
> > > > };
> > > >
> > > > #ifdef CONFIG_ARM
> > > > @@ -128,9 +129,19 @@ static int gen_pci_config_write(struct pci_bus *bus, unsigned int devfn,
> > > > return PCIBIOS_SUCCESSFUL;
> > > > }
> > > >
> > > > +static int gen_pci_set_msi_parent(struct pci_bus *bus)
> > > > +{
> > > > + struct gen_pci *pci = bus_to_gen_pci(bus);
> > > > +
> > > > + bus->msi = pci->mchip;
> > > > +
> > > > + return PCIBIOS_SUCCESSFUL;
> > > > +}
> > >
> > > I don't think this makes much sense a host controller callback. Why can't
> > > bus->msi be set in generic code?
> >
> > Because of the current way in which a bus gets created and them immediately used for
> > scanning devices if you use pci_scan_root_bus(). Alternative is to use pci_create_root_bus()
> > and increase your code size with the body of pci_scan_root_bus().
> >
> > Solution is to have pci_host_bridge holding the msi_chip pointer and that gets created
> > before root bus, with pci_scan_root_bus() now having all the info needed to do successful
> > setup of scanned devices.
>
> Why can't we add a hook like pci_bus_assign_domain_nr(), say:
>
> pci_bus_msi_init()
>
> in pci_create_root_bus() that does what Suravee wants in a generic way ?
Because even pci_bus_assign_domain_nr() was not really generic until your patches (small steps, etc).
We need a patch that proposes that and we can discuss it.
>
> What am I missing ?
Other than the fact that we need to sort out the initialisation order of all these components, nothing I guess.
Best regards,
Liviu
>
> Lorenzo
>
> >
> > Best regards,
> > Liviu
> >
> > >
> > > > static struct pci_ops gen_pci_ops = {
> > > > .read = gen_pci_config_read,
> > > > .write = gen_pci_config_write,
> > > > + .set_msi_parent = gen_pci_set_msi_parent,
> > > > };
> > > >
> > > > static const struct of_device_id gen_pci_of_match[] = {
> > > > @@ -313,6 +324,9 @@ static int gen_pci_probe(struct platform_device *pdev)
> > > > return err;
> > > > }
> > > >
> > > > + pci->mchip = of_pci_find_msi_chip_by_node(of_parse_phandle(np,
> > > > + "msi-parent", 0));
> > >
> > > This bit should be in the generic of_pci.c code and not duplicated for
> > > each host controller.
> > >
> > > Will
> > >
> >
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
On 2014/11/13 23:22, Lorenzo Pieralisi wrote:
> On Tue, Nov 11, 2014 at 11:52:27AM +0000, Liviu Dudau wrote:
>> On Tue, Nov 11, 2014 at 11:24:24AM +0000, Will Deacon wrote:
>>> Hi Suravee,
>>>
>>> On Mon, Nov 10, 2014 at 07:24:40PM +0000, [email protected] wrote:
>>>> From: Suravee Suthikulpanit <[email protected]>
>>>
>>> I don't think this makes much sense a host controller callback. Why can't
>>> bus->msi be set in generic code?
>>
>> Because of the current way in which a bus gets created and them immediately used for
>> scanning devices if you use pci_scan_root_bus(). Alternative is to use pci_create_root_bus()
>> and increase your code size with the body of pci_scan_root_bus().
>>
>> Solution is to have pci_host_bridge holding the msi_chip pointer and that gets created
>> before root bus, with pci_scan_root_bus() now having all the info needed to do successful
>> setup of scanned devices.
>
> Why can't we add a hook like pci_bus_assign_domain_nr(), say:
>
> pci_bus_msi_init()
>
> in pci_create_root_bus() that does what Suravee wants in a generic way ?
+1
BTW, x86 may have multiple MSI controllers/domains under a host bridge,
so prefer calling it for every bus instead of for root bus only.
Regards!
Gerry
>
> What am I missing ?
>
> Lorenzo
>
>>
>> Best regards,
>> Liviu
>>
>>>
>>>> static struct pci_ops gen_pci_ops = {
>>>> .read = gen_pci_config_read,
>>>> .write = gen_pci_config_write,
>>>> + .set_msi_parent = gen_pci_set_msi_parent,
>>>> };
>>>>
>>>> static const struct of_device_id gen_pci_of_match[] = {
>>>> @@ -313,6 +324,9 @@ static int gen_pci_probe(struct platform_device *pdev)
>>>> return err;
>>>> }
>>>>
>>>> + pci->mchip = of_pci_find_msi_chip_by_node(of_parse_phandle(np,
>>>> + "msi-parent", 0));
>>>
>>> This bit should be in the generic of_pci.c code and not duplicated for
>>> each host controller.
>>>
>>> Will
>>>
>>
>> --
>> ====================
>> | I would like to |
>> | fix the world, |
>> | but they're not |
>> | giving me the |
>> \ source code! /
>> ---------------
>> ¯\_(ツ)_/¯
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>