linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] PCI: generic: Assiging msi-controller to PCI hostbridge
@ 2014-11-10 19:24 suravee.suthikulpanit
  2014-11-10 19:24 ` [PATCH 1/2] PCI: Add new pci_ops for setting MSI parent for PCI bus suravee.suthikulpanit
  2014-11-10 19:24 ` [PATCH 2/2] PCI: generic: Add set_msi_parent callback suravee.suthikulpanit
  0 siblings, 2 replies; 10+ messages in thread
From: suravee.suthikulpanit @ 2014-11-10 19:24 UTC (permalink / raw)
  To: bhelgaas
  Cc: liviu.dudau, lorenzo.pieralisi, will.deacon, linux-pci,
	linux-arm-kernel, linux-kernel, Suravee Suthikulpanit

From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] PCI: Add new pci_ops for setting MSI parent for PCI bus
  2014-11-10 19:24 [PATCH 0/2] PCI: generic: Assiging msi-controller to PCI hostbridge suravee.suthikulpanit
@ 2014-11-10 19:24 ` suravee.suthikulpanit
  2014-11-10 19:24 ` [PATCH 2/2] PCI: generic: Add set_msi_parent callback suravee.suthikulpanit
  1 sibling, 0 replies; 10+ messages in thread
From: suravee.suthikulpanit @ 2014-11-10 19:24 UTC (permalink / raw)
  To: bhelgaas
  Cc: liviu.dudau, lorenzo.pieralisi, will.deacon, linux-pci,
	linux-arm-kernel, linux-kernel, Suravee Suthikulpanit

From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>

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 <bhelgaas@google.com>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] PCI: generic: Add set_msi_parent callback
  2014-11-10 19:24 [PATCH 0/2] PCI: generic: Assiging msi-controller to PCI hostbridge suravee.suthikulpanit
  2014-11-10 19:24 ` [PATCH 1/2] PCI: Add new pci_ops for setting MSI parent for PCI bus suravee.suthikulpanit
@ 2014-11-10 19:24 ` suravee.suthikulpanit
  2014-11-11 11:24   ` Will Deacon
  1 sibling, 1 reply; 10+ messages in thread
From: suravee.suthikulpanit @ 2014-11-10 19:24 UTC (permalink / raw)
  To: bhelgaas
  Cc: liviu.dudau, lorenzo.pieralisi, will.deacon, linux-pci,
	linux-arm-kernel, linux-kernel, Suravee Suthikulpanit

From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>

This patch implement set_msi_parent callback for PCI generic host controller.

Cc: Bjorn Helgass <bhelgaas@google.com>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] PCI: generic: Add set_msi_parent callback
  2014-11-10 19:24 ` [PATCH 2/2] PCI: generic: Add set_msi_parent callback suravee.suthikulpanit
@ 2014-11-11 11:24   ` Will Deacon
  2014-11-11 11:52     ` Liviu Dudau
  2014-11-11 15:33     ` Suravee Suthikulpanit
  0 siblings, 2 replies; 10+ messages in thread
From: Will Deacon @ 2014-11-11 11:24 UTC (permalink / raw)
  To: suravee.suthikulpanit@amd.com
  Cc: bhelgaas@google.com, Liviu Dudau, Lorenzo Pieralisi,
	linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org

Hi Suravee,

On Mon, Nov 10, 2014 at 07:24:40PM +0000, suravee.suthikulpanit@amd.com wrote:
> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> 
> This patch implement set_msi_parent callback for PCI generic host controller.
> 
> Cc: Bjorn Helgass <bhelgaas@google.com>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> ---
>  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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] PCI: generic: Add set_msi_parent callback
  2014-11-11 11:24   ` Will Deacon
@ 2014-11-11 11:52     ` Liviu Dudau
  2014-11-13 15:22       ` Lorenzo Pieralisi
  2014-11-11 15:33     ` Suravee Suthikulpanit
  1 sibling, 1 reply; 10+ messages in thread
From: Liviu Dudau @ 2014-11-11 11:52 UTC (permalink / raw)
  To: Will Deacon
  Cc: suravee.suthikulpanit@amd.com, bhelgaas@google.com,
	Lorenzo Pieralisi, linux-pci@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org

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, suravee.suthikulpanit@amd.com wrote:
> > From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> > 
> > This patch implement set_msi_parent callback for PCI generic host controller.
> > 
> > Cc: Bjorn Helgass <bhelgaas@google.com>
> > Cc: Liviu Dudau <liviu.dudau@arm.com>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> > ---
> >  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!  /
  ---------------
    ¯\_(ツ)_/¯


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] PCI: generic: Add set_msi_parent callback
  2014-11-11 11:24   ` Will Deacon
  2014-11-11 11:52     ` Liviu Dudau
@ 2014-11-11 15:33     ` Suravee Suthikulpanit
  2014-11-13 15:32       ` Lorenzo Pieralisi
  1 sibling, 1 reply; 10+ messages in thread
From: Suravee Suthikulpanit @ 2014-11-11 15:33 UTC (permalink / raw)
  To: Will Deacon
  Cc: bhelgaas@google.com, Liviu Dudau, Lorenzo Pieralisi,
	linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] PCI: generic: Add set_msi_parent callback
  2014-11-11 11:52     ` Liviu Dudau
@ 2014-11-13 15:22       ` Lorenzo Pieralisi
  2014-11-13 18:32         ` Liviu Dudau
  2014-11-13 23:19         ` Jiang Liu
  0 siblings, 2 replies; 10+ messages in thread
From: Lorenzo Pieralisi @ 2014-11-13 15:22 UTC (permalink / raw)
  To: Liviu Dudau
  Cc: Will Deacon, suravee.suthikulpanit@amd.com, bhelgaas@google.com,
	linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org

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, suravee.suthikulpanit@amd.com wrote:
> > > From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> > > 
> > > This patch implement set_msi_parent callback for PCI generic host controller.
> > > 
> > > Cc: Bjorn Helgass <bhelgaas@google.com>
> > > Cc: Liviu Dudau <liviu.dudau@arm.com>
> > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> > > ---
> > >  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!  /
>   ---------------
>     ¯\_(ツ)_/¯

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] PCI: generic: Add set_msi_parent callback
  2014-11-11 15:33     ` Suravee Suthikulpanit
@ 2014-11-13 15:32       ` Lorenzo Pieralisi
  0 siblings, 0 replies; 10+ messages in thread
From: Lorenzo Pieralisi @ 2014-11-13 15:32 UTC (permalink / raw)
  To: Suravee Suthikulpanit
  Cc: Will Deacon, bhelgaas@google.com, Liviu Dudau,
	linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, arnd, robh+dt

[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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] PCI: generic: Add set_msi_parent callback
  2014-11-13 15:22       ` Lorenzo Pieralisi
@ 2014-11-13 18:32         ` Liviu Dudau
  2014-11-13 23:19         ` Jiang Liu
  1 sibling, 0 replies; 10+ messages in thread
From: Liviu Dudau @ 2014-11-13 18:32 UTC (permalink / raw)
  To: Lorenzo Pieralisi
  Cc: Will Deacon, suravee.suthikulpanit@amd.com, bhelgaas@google.com,
	linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org

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, suravee.suthikulpanit@amd.com wrote:
> > > > From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> > > > 
> > > > This patch implement set_msi_parent callback for PCI generic host controller.
> > > > 
> > > > Cc: Bjorn Helgass <bhelgaas@google.com>
> > > > Cc: Liviu Dudau <liviu.dudau@arm.com>
> > > > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > > > Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> > > > ---
> > > >  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!  /
  ---------------
    ¯\_(ツ)_/¯


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] PCI: generic: Add set_msi_parent callback
  2014-11-13 15:22       ` Lorenzo Pieralisi
  2014-11-13 18:32         ` Liviu Dudau
@ 2014-11-13 23:19         ` Jiang Liu
  1 sibling, 0 replies; 10+ messages in thread
From: Jiang Liu @ 2014-11-13 23:19 UTC (permalink / raw)
  To: Lorenzo Pieralisi, Liviu Dudau
  Cc: Will Deacon, suravee.suthikulpanit@amd.com, bhelgaas@google.com,
	linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org

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, suravee.suthikulpanit@amd.com wrote:
>>>> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>>>
>>> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2014-11-13 23:19 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-11-10 19:24 [PATCH 0/2] PCI: generic: Assiging msi-controller to PCI hostbridge suravee.suthikulpanit
2014-11-10 19:24 ` [PATCH 1/2] PCI: Add new pci_ops for setting MSI parent for PCI bus suravee.suthikulpanit
2014-11-10 19:24 ` [PATCH 2/2] PCI: generic: Add set_msi_parent callback suravee.suthikulpanit
2014-11-11 11:24   ` Will Deacon
2014-11-11 11:52     ` Liviu Dudau
2014-11-13 15:22       ` Lorenzo Pieralisi
2014-11-13 18:32         ` Liviu Dudau
2014-11-13 23:19         ` Jiang Liu
2014-11-11 15:33     ` Suravee Suthikulpanit
2014-11-13 15:32       ` Lorenzo Pieralisi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).