Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1398228 > unrolled thread

[PATCH V7 07/11] pci, acpi: Handle ACPI companion assignment.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH V7 07/11] pci, acpi: Handle ACPI companion assignment. Tomasz Nowicki <tn@semihalf.com> - 2016-05-10 17:30 +0200
    Re: [PATCH V7 07/11] pci, acpi: Handle ACPI companion assignment. "Rafael J. Wysocki" <rafael@kernel.org> - 2016-05-10 20:40 +0200
      Re: [PATCH V7 07/11] pci, acpi: Handle ACPI companion assignment. "Rafael J. Wysocki" <rafael@kernel.org> - 2016-05-10 20:50 +0200
      Re: [PATCH V7 07/11] pci, acpi: Handle ACPI companion assignment. Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-05-11 12:20 +0200
        Re: [PATCH V7 07/11] pci, acpi: Handle ACPI companion assignment. "Rafael J. Wysocki" <rafael@kernel.org> - 2016-05-11 22:40 +0200
          Re: [PATCH V7 07/11] pci, acpi: Handle ACPI companion assignment. Bjorn Helgaas <helgaas@kernel.org> - 2016-05-12 00:50 +0200
            Re: [PATCH V7 07/11] pci, acpi: Handle ACPI companion assignment. Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-05-12 12:10 +0200
            Re: [PATCH V7 07/11] pci, acpi: Handle ACPI companion assignment. Jayachandran C <jchandra@broadcom.com> - 2016-05-12 12:50 +0200
              Re: [PATCH V7 07/11] pci, acpi: Handle ACPI companion assignment. "Rafael J. Wysocki" <rafael@kernel.org> - 2016-05-12 13:30 +0200
                Re: [PATCH V7 07/11] pci, acpi: Handle ACPI companion assignment. Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-05-13 12:40 +0200
            Re: [PATCH V7 07/11] pci, acpi: Handle ACPI companion assignment. Tomasz Nowicki <tn@semihalf.com> - 2016-05-12 13:00 +0200
              Re: [PATCH V7 07/11] pci, acpi: Handle ACPI companion assignment. Bjorn Helgaas <helgaas@kernel.org> - 2016-05-12 14:10 +0200
    Re: [PATCH V7 07/11] pci, acpi: Handle ACPI companion assignment. Dongdong Liu <liudongdong3@huawei.com> - 2016-05-17 05:20 +0200
      Re: [PATCH V7 07/11] pci, acpi: Handle ACPI companion assignment. Tomasz Nowicki <tn@semihalf.com> - 2016-05-17 15:50 +0200

#1398228 — [PATCH V7 07/11] pci, acpi: Handle ACPI companion assignment.

This patch provides a way to set the ACPI companion in PCI code.
We define acpi_pci_set_companion() to set the ACPI companion pointer and
call it from PCI core code. The function is stub for now.

Signed-off-by: Jayachandran C <jchandra@broadcom.com>
Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
---
 drivers/pci/probe.c      | 2 ++
 include/linux/pci-acpi.h | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 8004f67..fb0b752 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -12,6 +12,7 @@
 #include <linux/slab.h>
 #include <linux/module.h>
 #include <linux/cpumask.h>
+#include <linux/pci-acpi.h>
 #include <linux/pci-aspm.h>
 #include <linux/aer.h>
 #include <linux/acpi.h>
@@ -2141,6 +2142,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 	bridge->dev.parent = parent;
 	bridge->dev.release = pci_release_host_bridge_dev;
 	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
+	acpi_pci_set_companion(bridge);
 	error = pcibios_root_bridge_prepare(bridge);
 	if (error) {
 		kfree(bridge);
diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
index 09f9f02..1baa515 100644
--- a/include/linux/pci-acpi.h
+++ b/include/linux/pci-acpi.h
@@ -111,6 +111,10 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
 static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
 #endif	/* CONFIG_ACPI */
 
+static inline void acpi_pci_set_companion(struct pci_host_bridge *bridge)
+{
+}
+
 static inline int acpi_pci_bus_domain_nr(struct pci_bus *bus)
 {
 	return 0;
-- 
1.9.1

[toc] | [next] | [standalone]

#1398386

On Tue, May 10, 2016 at 5:19 PM, Tomasz Nowicki <tn@semihalf.com> wrote:
> This patch provides a way to set the ACPI companion in PCI code.
> We define acpi_pci_set_companion() to set the ACPI companion pointer and
> call it from PCI core code. The function is stub for now.
>
> Signed-off-by: Jayachandran C <jchandra@broadcom.com>
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> ---
>  drivers/pci/probe.c      | 2 ++
>  include/linux/pci-acpi.h | 4 ++++
>  2 files changed, 6 insertions(+)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 8004f67..fb0b752 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -12,6 +12,7 @@
>  #include <linux/slab.h>
>  #include <linux/module.h>
>  #include <linux/cpumask.h>
> +#include <linux/pci-acpi.h>
>  #include <linux/pci-aspm.h>
>  #include <linux/aer.h>
>  #include <linux/acpi.h>
> @@ -2141,6 +2142,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>         bridge->dev.parent = parent;
>         bridge->dev.release = pci_release_host_bridge_dev;
>         dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
> +       acpi_pci_set_companion(bridge);

Yes, we'll probably add something similar here.

Do I think now is the right time to do that?  No.

>         error = pcibios_root_bridge_prepare(bridge);
>         if (error) {
>                 kfree(bridge);
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 09f9f02..1baa515 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -111,6 +111,10 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
>  #endif /* CONFIG_ACPI */
>
> +static inline void acpi_pci_set_companion(struct pci_host_bridge *bridge)
> +{
> +}
> +
>  static inline int acpi_pci_bus_domain_nr(struct pci_bus *bus)
>  {
>         return 0;
> --

Honestly, to me it looks like this series is trying very hard to avoid
doing any PCI host bridge configuration stuff from arch/arm64/
although (a) that might be simpler and (b) it would allow us to
identify the code that's common between *all* architectures using ACPI
support for host bridge configuration and to move *that* to a common
place later.  As done here it seems to be following the "ARM64 is
generic and the rest of the world is special" line which isn't really
helpful.

[toc] | [prev] | [next] | [standalone]

#1398398

On Tue, May 10, 2016 at 8:37 PM, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, May 10, 2016 at 5:19 PM, Tomasz Nowicki <tn@semihalf.com> wrote:
>> This patch provides a way to set the ACPI companion in PCI code.
>> We define acpi_pci_set_companion() to set the ACPI companion pointer and
>> call it from PCI core code. The function is stub for now.
>>
>> Signed-off-by: Jayachandran C <jchandra@broadcom.com>
>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>> ---
>>  drivers/pci/probe.c      | 2 ++
>>  include/linux/pci-acpi.h | 4 ++++
>>  2 files changed, 6 insertions(+)
>>
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 8004f67..fb0b752 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -12,6 +12,7 @@
>>  #include <linux/slab.h>
>>  #include <linux/module.h>
>>  #include <linux/cpumask.h>
>> +#include <linux/pci-acpi.h>
>>  #include <linux/pci-aspm.h>
>>  #include <linux/aer.h>
>>  #include <linux/acpi.h>
>> @@ -2141,6 +2142,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>>         bridge->dev.parent = parent;
>>         bridge->dev.release = pci_release_host_bridge_dev;
>>         dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
>> +       acpi_pci_set_companion(bridge);
>
> Yes, we'll probably add something similar here.
>
> Do I think now is the right time to do that?  No.
>
>>         error = pcibios_root_bridge_prepare(bridge);
>>         if (error) {
>>                 kfree(bridge);
>> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>> index 09f9f02..1baa515 100644
>> --- a/include/linux/pci-acpi.h
>> +++ b/include/linux/pci-acpi.h
>> @@ -111,6 +111,10 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>>  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
>>  #endif /* CONFIG_ACPI */
>>
>> +static inline void acpi_pci_set_companion(struct pci_host_bridge *bridge)
>> +{
>> +}
>> +
>>  static inline int acpi_pci_bus_domain_nr(struct pci_bus *bus)
>>  {
>>         return 0;
>> --
>
> Honestly, to me it looks like this series is trying very hard to avoid
> doing any PCI host bridge configuration stuff from arch/arm64/
> although (a) that might be simpler and (b) it would allow us to
> identify the code that's common between *all* architectures using ACPI
> support for host bridge configuration and to move *that* to a common
> place later.  As done here it seems to be following the "ARM64 is
> generic and the rest of the world is special" line which isn't really
> helpful.

Speaking of which, at least one of the reasons why the ACPI PCI host
bridge thing on x86 and ia64 went to the arch code was to avoid
explicit references to ACPI-specific data types and related #ifdeffery
in the generic PCI code and data structures.  If you are going to add
those references now anyway, that reason is not relevant any more and
all of that can just be reworked to refer to ACPI explicitly.

[toc] | [prev] | [next] | [standalone]

#1398905

On Tue, May 10, 2016 at 08:37:00PM +0200, Rafael J. Wysocki wrote:
> On Tue, May 10, 2016 at 5:19 PM, Tomasz Nowicki <tn@semihalf.com> wrote:
> > This patch provides a way to set the ACPI companion in PCI code.
> > We define acpi_pci_set_companion() to set the ACPI companion pointer and
> > call it from PCI core code. The function is stub for now.
> >
> > Signed-off-by: Jayachandran C <jchandra@broadcom.com>
> > Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> > ---
> >  drivers/pci/probe.c      | 2 ++
> >  include/linux/pci-acpi.h | 4 ++++
> >  2 files changed, 6 insertions(+)
> >
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 8004f67..fb0b752 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/module.h>
> >  #include <linux/cpumask.h>
> > +#include <linux/pci-acpi.h>
> >  #include <linux/pci-aspm.h>
> >  #include <linux/aer.h>
> >  #include <linux/acpi.h>
> > @@ -2141,6 +2142,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> >         bridge->dev.parent = parent;
> >         bridge->dev.release = pci_release_host_bridge_dev;
> >         dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
> > +       acpi_pci_set_companion(bridge);
> 
> Yes, we'll probably add something similar here.
> 
> Do I think now is the right time to do that?  No.
> 
> >         error = pcibios_root_bridge_prepare(bridge);
> >         if (error) {
> >                 kfree(bridge);
> > diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> > index 09f9f02..1baa515 100644
> > --- a/include/linux/pci-acpi.h
> > +++ b/include/linux/pci-acpi.h
> > @@ -111,6 +111,10 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
> >  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
> >  #endif /* CONFIG_ACPI */
> >
> > +static inline void acpi_pci_set_companion(struct pci_host_bridge *bridge)
> > +{
> > +}
> > +
> >  static inline int acpi_pci_bus_domain_nr(struct pci_bus *bus)
> >  {
> >         return 0;
> > --
> 
> Honestly, to me it looks like this series is trying very hard to avoid
> doing any PCI host bridge configuration stuff from arch/arm64/
> although (a) that might be simpler and (b) it would allow us to
> identify the code that's common between *all* architectures using ACPI
> support for host bridge configuration and to move *that* to a common
> place later.  As done here it seems to be following the "ARM64 is
> generic and the rest of the world is special" line which isn't really
> helpful.

I think patch [1-2] should be merged regardless (they may require minor
tweaks if we decide to move pci_acpi_scan_root() to arch/arm64 though,
for include files location). I guess you are referring to patch 8 in
your comments above, which boils down to deciding whether:

- pci_acpi_scan_root() (and unfortunately all the MCFG/ECAM handling that
  goes with it) should live in arch/arm64 or drivers/acpi

acpi_pci_bus_domain_nr() is a bit more problematic since it is meant
to be called from PCI core code (ARM64 selects PCI_DOMAINS_GENERIC for
DT and same kernel has to work with OF and ACPI selected) and it is
arch specific (because what we have in bus->sysdata is arch specific,
waiting for the domain number to be embedded in struct pci_host_bridge).

Your point is fair, I am not sure that moving the pci_acpi_scan_root()
to arch/arm64 would make things much simpler though, it is just a matter
of deciding where that code has to live.

How do you want us to proceed ?

Thanks,
Lorenzo

[toc] | [prev] | [next] | [standalone]

#1399496

On Wed, May 11, 2016 at 12:11 PM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Tue, May 10, 2016 at 08:37:00PM +0200, Rafael J. Wysocki wrote:
>> On Tue, May 10, 2016 at 5:19 PM, Tomasz Nowicki <tn@semihalf.com> wrote:
>> > This patch provides a way to set the ACPI companion in PCI code.
>> > We define acpi_pci_set_companion() to set the ACPI companion pointer and
>> > call it from PCI core code. The function is stub for now.
>> >
>> > Signed-off-by: Jayachandran C <jchandra@broadcom.com>
>> > Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>> > ---
>> >  drivers/pci/probe.c      | 2 ++
>> >  include/linux/pci-acpi.h | 4 ++++
>> >  2 files changed, 6 insertions(+)
>> >
>> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> > index 8004f67..fb0b752 100644
>> > --- a/drivers/pci/probe.c
>> > +++ b/drivers/pci/probe.c
>> > @@ -12,6 +12,7 @@
>> >  #include <linux/slab.h>
>> >  #include <linux/module.h>
>> >  #include <linux/cpumask.h>
>> > +#include <linux/pci-acpi.h>
>> >  #include <linux/pci-aspm.h>
>> >  #include <linux/aer.h>
>> >  #include <linux/acpi.h>
>> > @@ -2141,6 +2142,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>> >         bridge->dev.parent = parent;
>> >         bridge->dev.release = pci_release_host_bridge_dev;
>> >         dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
>> > +       acpi_pci_set_companion(bridge);
>>
>> Yes, we'll probably add something similar here.
>>
>> Do I think now is the right time to do that?  No.
>>
>> >         error = pcibios_root_bridge_prepare(bridge);
>> >         if (error) {
>> >                 kfree(bridge);
>> > diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>> > index 09f9f02..1baa515 100644
>> > --- a/include/linux/pci-acpi.h
>> > +++ b/include/linux/pci-acpi.h
>> > @@ -111,6 +111,10 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>> >  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
>> >  #endif /* CONFIG_ACPI */
>> >
>> > +static inline void acpi_pci_set_companion(struct pci_host_bridge *bridge)
>> > +{
>> > +}
>> > +
>> >  static inline int acpi_pci_bus_domain_nr(struct pci_bus *bus)
>> >  {
>> >         return 0;
>> > --
>>
>> Honestly, to me it looks like this series is trying very hard to avoid
>> doing any PCI host bridge configuration stuff from arch/arm64/
>> although (a) that might be simpler and (b) it would allow us to
>> identify the code that's common between *all* architectures using ACPI
>> support for host bridge configuration and to move *that* to a common
>> place later.  As done here it seems to be following the "ARM64 is
>> generic and the rest of the world is special" line which isn't really
>> helpful.
>
> I think patch [1-2] should be merged regardless (they may require minor
> tweaks if we decide to move pci_acpi_scan_root() to arch/arm64 though,
> for include files location). I guess you are referring to patch 8 in
> your comments above, which boils down to deciding whether:
>
> - pci_acpi_scan_root() (and unfortunately all the MCFG/ECAM handling that
>   goes with it) should live in arch/arm64 or drivers/acpi

To be precise, everything under #ifdef CONFIG_ACPI_PCI_HOST_GENERIC or
equivalent is de facto ARM64-specific, because (as it stands in the
patch series) ARM64 is the only architecture that will select that
option.  Unless you are aware of any more architectures planning to
use ACPI (and I'm not aware of any), it will stay the only
architecture selecting it in the foreseeable future.

Therefore you could replace CONFIG_ACPI_PCI_HOST_GENERIC with
CONFIG_ARM64 everywhere in that code which is why in my opinion the
code should live somewhere under arch/arm64/.

Going forward, it should be possible to identify common parts of the
PCI host bridge configuration code in arch/ and move it to
drivers/acpi/ or drivers/pci/, but I bet that won't be the entire code
this series puts under CONFIG_ACPI_PCI_HOST_GENERIC.

The above leads to a quite straightforward conclusion about the order
in which to do things: I'd add ACPI support for PCI host bridge on
ARM64 following what's been done on ia64 (as x86 is more quirky and
kludgy overall) as far as reasonably possible first and then think
about moving common stuff to a common place.

> acpi_pci_bus_domain_nr() is a bit more problematic since it is meant
> to be called from PCI core code (ARM64 selects PCI_DOMAINS_GENERIC for
> DT and same kernel has to work with OF and ACPI selected) and it is
> arch specific (because what we have in bus->sysdata is arch specific,
> waiting for the domain number to be embedded in struct pci_host_bridge).
>
> Your point is fair, I am not sure that moving the pci_acpi_scan_root()
> to arch/arm64 would make things much simpler though, it is just a matter
> of deciding where that code has to live.
>
> How do you want us to proceed ?

Pretty much as stated above. :-)

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1399558

On Wed, May 11, 2016 at 10:30:51PM +0200, Rafael J. Wysocki wrote:
> On Wed, May 11, 2016 at 12:11 PM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> > On Tue, May 10, 2016 at 08:37:00PM +0200, Rafael J. Wysocki wrote:
> >> On Tue, May 10, 2016 at 5:19 PM, Tomasz Nowicki <tn@semihalf.com> wrote:
> >> > This patch provides a way to set the ACPI companion in PCI code.
> >> > We define acpi_pci_set_companion() to set the ACPI companion pointer and
> >> > call it from PCI core code. The function is stub for now.
> >> >
> >> > Signed-off-by: Jayachandran C <jchandra@broadcom.com>
> >> > Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> >> > ---
> >> >  drivers/pci/probe.c      | 2 ++
> >> >  include/linux/pci-acpi.h | 4 ++++
> >> >  2 files changed, 6 insertions(+)
> >> >
> >> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> >> > index 8004f67..fb0b752 100644
> >> > --- a/drivers/pci/probe.c
> >> > +++ b/drivers/pci/probe.c
> >> > @@ -12,6 +12,7 @@
> >> >  #include <linux/slab.h>
> >> >  #include <linux/module.h>
> >> >  #include <linux/cpumask.h>
> >> > +#include <linux/pci-acpi.h>
> >> >  #include <linux/pci-aspm.h>
> >> >  #include <linux/aer.h>
> >> >  #include <linux/acpi.h>
> >> > @@ -2141,6 +2142,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> >> >         bridge->dev.parent = parent;
> >> >         bridge->dev.release = pci_release_host_bridge_dev;
> >> >         dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
> >> > +       acpi_pci_set_companion(bridge);
> >>
> >> Yes, we'll probably add something similar here.
> >>
> >> Do I think now is the right time to do that?  No.
> >>
> >> >         error = pcibios_root_bridge_prepare(bridge);
> >> >         if (error) {
> >> >                 kfree(bridge);
> >> > diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> >> > index 09f9f02..1baa515 100644
> >> > --- a/include/linux/pci-acpi.h
> >> > +++ b/include/linux/pci-acpi.h
> >> > @@ -111,6 +111,10 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
> >> >  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
> >> >  #endif /* CONFIG_ACPI */
> >> >
> >> > +static inline void acpi_pci_set_companion(struct pci_host_bridge *bridge)
> >> > +{
> >> > +}
> >> > +
> >> >  static inline int acpi_pci_bus_domain_nr(struct pci_bus *bus)
> >> >  {
> >> >         return 0;
> >> > --
> >>
> >> Honestly, to me it looks like this series is trying very hard to avoid
> >> doing any PCI host bridge configuration stuff from arch/arm64/
> >> although (a) that might be simpler and (b) it would allow us to
> >> identify the code that's common between *all* architectures using ACPI
> >> support for host bridge configuration and to move *that* to a common
> >> place later.  As done here it seems to be following the "ARM64 is
> >> generic and the rest of the world is special" line which isn't really
> >> helpful.
> >
> > I think patch [1-2] should be merged regardless (they may require minor
> > tweaks if we decide to move pci_acpi_scan_root() to arch/arm64 though,
> > for include files location). I guess you are referring to patch 8 in
> > your comments above, which boils down to deciding whether:
> >
> > - pci_acpi_scan_root() (and unfortunately all the MCFG/ECAM handling that
> >   goes with it) should live in arch/arm64 or drivers/acpi
> 
> To be precise, everything under #ifdef CONFIG_ACPI_PCI_HOST_GENERIC or
> equivalent is de facto ARM64-specific, because (as it stands in the
> patch series) ARM64 is the only architecture that will select that
> option.  Unless you are aware of any more architectures planning to
> use ACPI (and I'm not aware of any), it will stay the only
> architecture selecting it in the foreseeable future.
> 
> Therefore you could replace CONFIG_ACPI_PCI_HOST_GENERIC with
> CONFIG_ARM64 everywhere in that code which is why in my opinion the
> code should live somewhere under arch/arm64/.
> 
> Going forward, it should be possible to identify common parts of the
> PCI host bridge configuration code in arch/ and move it to
> drivers/acpi/ or drivers/pci/, but I bet that won't be the entire code
> this series puts under CONFIG_ACPI_PCI_HOST_GENERIC.
> 
> The above leads to a quite straightforward conclusion about the order
> in which to do things: I'd add ACPI support for PCI host bridge on
> ARM64 following what's been done on ia64 (as x86 is more quirky and
> kludgy overall) as far as reasonably possible first and then think
> about moving common stuff to a common place.

That does seem like a reasonable approach.  I had hoped to get more of
this in for v4.7, but we don't have much time left.  Maybe some of
Rafael's comments can be addressed by moving and slight restructuring
and we can still squeeze it in.

The first three patches:

  PCI: Provide common functions for ECAM mapping
  PCI: generic, thunder: Use generic ECAM API
  PCI, of: Move PCI I/O space management to PCI core code

seem relatively straightforward, and I applied them to pci/arm64 with
the intent of merging them unless there are objections.  I made the
following tweaks, mainly to try to improve some error messages:

diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c
index 3d52005..e1add01 100644
--- a/drivers/pci/ecam.c
+++ b/drivers/pci/ecam.c
@@ -24,9 +24,9 @@
 #include "ecam.h"
 
 /*
- * On 64 bit systems, we do a single ioremap for the whole config space
- * since we have enough virtual address range available. On 32 bit, do an
- * ioremap per bus.
+ * On 64-bit systems, we do a single ioremap for the whole config space
+ * since we have enough virtual address range available.  On 32-bit, we
+ * ioremap the config space for each bus individually.
  */
 static const bool per_bus_mapping = !config_enabled(CONFIG_64BIT);
 
@@ -42,6 +42,7 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
 {
 	struct pci_config_window *cfg;
 	unsigned int bus_range, bus_range_max, bsz;
+	struct resource *conflict;
 	int i, err;
 
 	if (busr->start > busr->end)
@@ -58,10 +59,10 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
 	bus_range = resource_size(&cfg->busr);
 	bus_range_max = resource_size(cfgres) >> ops->bus_shift;
 	if (bus_range > bus_range_max) {
-		dev_warn(dev, "bus max %#x reduced to %#x",
-					bus_range, bus_range_max);
 		bus_range = bus_range_max;
 		cfg->busr.end = busr->start + bus_range - 1;
+		dev_warn(dev, "ECAM area %pR can only accommodate %pR (reduced from %pR desired)\n",
+			 cfgres, &cfg->busr, busr);
 	}
 	bsz = 1 << ops->bus_shift;
 
@@ -70,9 +71,11 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
 	cfg->res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
 	cfg->res.name = "PCI ECAM";
 
-	err = request_resource(&iomem_resource, &cfg->res);
-	if (err) {
-		dev_err(dev, "request ECAM res %pR failed\n", &cfg->res);
+	conflict = request_resource(&iomem_resource, &cfg->res);
+	if (conflict) {
+		err = -EBUSY;
+		dev_err(dev, "can't claim ECAM area %pR: address conflict with %s %pR\n",
+			&cfg->res, conflict->name, conflict);
 		goto err_exit;
 	}
 
diff --git a/drivers/pci/ecam.h b/drivers/pci/ecam.h
index 1ad2176..9878beb 100644
--- a/drivers/pci/ecam.h
+++ b/drivers/pci/ecam.h
@@ -33,7 +33,7 @@ struct pci_ecam_ops {
 
 /*
  * struct to hold the mappings of a config space window. This
- * is expected to be used as sysdata for PCI controlllers which
+ * is expected to be used as sysdata for PCI controllers that
  * use ECAM.
  */
 struct pci_config_window {
@@ -43,11 +43,11 @@ struct pci_config_window {
 	struct pci_ecam_ops		*ops;
 	union {
 		void __iomem		*win;	/* 64-bit single mapping */
-		void __iomem		**winp; /* 32-bit per bus mapping */
+		void __iomem		**winp; /* 32-bit per-bus mapping */
 	};
 };
 
-/* create and free for pci_config_window */
+/* create and free pci_config_window */
 struct pci_config_window *pci_ecam_create(struct device *dev,
 		struct resource *cfgres, struct resource *busr,
 		struct pci_ecam_ops *ops);
@@ -56,11 +56,11 @@ void pci_ecam_free(struct pci_config_window *cfg);
 /* map_bus when ->sysdata is an instance of pci_config_window */
 void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
 			       int where);
-/* default ECAM ops, bus shift 20, generic read and write */
+/* default ECAM ops */
 extern struct pci_ecam_ops pci_generic_ecam_ops;
 
 #ifdef CONFIG_PCI_HOST_GENERIC
-/* for DT based pci controllers that support ECAM */
+/* for DT-based PCI controllers that support ECAM */
 int pci_host_common_probe(struct platform_device *pdev,
 			  struct pci_ecam_ops *ops);
 #endif

[toc] | [prev] | [next] | [standalone]

#1399851

On Wed, May 11, 2016 at 05:43:14PM -0500, Bjorn Helgaas wrote:
> On Wed, May 11, 2016 at 10:30:51PM +0200, Rafael J. Wysocki wrote:
> > On Wed, May 11, 2016 at 12:11 PM, Lorenzo Pieralisi
> > <lorenzo.pieralisi@arm.com> wrote:
> > > On Tue, May 10, 2016 at 08:37:00PM +0200, Rafael J. Wysocki wrote:
> > >> On Tue, May 10, 2016 at 5:19 PM, Tomasz Nowicki <tn@semihalf.com> wrote:
> > >> > This patch provides a way to set the ACPI companion in PCI code.
> > >> > We define acpi_pci_set_companion() to set the ACPI companion pointer and
> > >> > call it from PCI core code. The function is stub for now.
> > >> >
> > >> > Signed-off-by: Jayachandran C <jchandra@broadcom.com>
> > >> > Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> > >> > ---
> > >> >  drivers/pci/probe.c      | 2 ++
> > >> >  include/linux/pci-acpi.h | 4 ++++
> > >> >  2 files changed, 6 insertions(+)
> > >> >
> > >> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > >> > index 8004f67..fb0b752 100644
> > >> > --- a/drivers/pci/probe.c
> > >> > +++ b/drivers/pci/probe.c
> > >> > @@ -12,6 +12,7 @@
> > >> >  #include <linux/slab.h>
> > >> >  #include <linux/module.h>
> > >> >  #include <linux/cpumask.h>
> > >> > +#include <linux/pci-acpi.h>
> > >> >  #include <linux/pci-aspm.h>
> > >> >  #include <linux/aer.h>
> > >> >  #include <linux/acpi.h>
> > >> > @@ -2141,6 +2142,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> > >> >         bridge->dev.parent = parent;
> > >> >         bridge->dev.release = pci_release_host_bridge_dev;
> > >> >         dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
> > >> > +       acpi_pci_set_companion(bridge);
> > >>
> > >> Yes, we'll probably add something similar here.
> > >>
> > >> Do I think now is the right time to do that?  No.
> > >>
> > >> >         error = pcibios_root_bridge_prepare(bridge);
> > >> >         if (error) {
> > >> >                 kfree(bridge);
> > >> > diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> > >> > index 09f9f02..1baa515 100644
> > >> > --- a/include/linux/pci-acpi.h
> > >> > +++ b/include/linux/pci-acpi.h
> > >> > @@ -111,6 +111,10 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
> > >> >  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
> > >> >  #endif /* CONFIG_ACPI */
> > >> >
> > >> > +static inline void acpi_pci_set_companion(struct pci_host_bridge *bridge)
> > >> > +{
> > >> > +}
> > >> > +
> > >> >  static inline int acpi_pci_bus_domain_nr(struct pci_bus *bus)
> > >> >  {
> > >> >         return 0;
> > >> > --
> > >>
> > >> Honestly, to me it looks like this series is trying very hard to avoid
> > >> doing any PCI host bridge configuration stuff from arch/arm64/
> > >> although (a) that might be simpler and (b) it would allow us to
> > >> identify the code that's common between *all* architectures using ACPI
> > >> support for host bridge configuration and to move *that* to a common
> > >> place later.  As done here it seems to be following the "ARM64 is
> > >> generic and the rest of the world is special" line which isn't really
> > >> helpful.
> > >
> > > I think patch [1-2] should be merged regardless (they may require minor
> > > tweaks if we decide to move pci_acpi_scan_root() to arch/arm64 though,
> > > for include files location). I guess you are referring to patch 8 in
> > > your comments above, which boils down to deciding whether:
> > >
> > > - pci_acpi_scan_root() (and unfortunately all the MCFG/ECAM handling that
> > >   goes with it) should live in arch/arm64 or drivers/acpi
> > 
> > To be precise, everything under #ifdef CONFIG_ACPI_PCI_HOST_GENERIC or
> > equivalent is de facto ARM64-specific, because (as it stands in the
> > patch series) ARM64 is the only architecture that will select that
> > option.  Unless you are aware of any more architectures planning to
> > use ACPI (and I'm not aware of any), it will stay the only
> > architecture selecting it in the foreseeable future.
> > 
> > Therefore you could replace CONFIG_ACPI_PCI_HOST_GENERIC with
> > CONFIG_ARM64 everywhere in that code which is why in my opinion the
> > code should live somewhere under arch/arm64/.
> > 
> > Going forward, it should be possible to identify common parts of the
> > PCI host bridge configuration code in arch/ and move it to
> > drivers/acpi/ or drivers/pci/, but I bet that won't be the entire code
> > this series puts under CONFIG_ACPI_PCI_HOST_GENERIC.
> > 
> > The above leads to a quite straightforward conclusion about the order
> > in which to do things: I'd add ACPI support for PCI host bridge on
> > ARM64 following what's been done on ia64 (as x86 is more quirky and
> > kludgy overall) as far as reasonably possible first and then think
> > about moving common stuff to a common place.
> 
> That does seem like a reasonable approach.  I had hoped to get more of
> this in for v4.7, but we don't have much time left.  Maybe some of
> Rafael's comments can be addressed by moving and slight restructuring
> and we can still squeeze it in.

Yes, it seems like a reasonable approach, as long as we accept that
part of this series has to live in arch/arm64 otherwise we are going
round in circles (because that's the gist of this discussion, to
decide where this code has to live, I do not think there is any objection
to the code per-se anymore).

I suggest we post a v8 (with code move to arch/arm64) end of merge
window (or you prefer seeing patches now to prevent any additional
changes later ?), my aim is to get this into -next (whether via arm64 or
pci tree it has to be decided) as early as possible for next cycle (-rc1)
so that it can get exposure and testing, I do not think that missing the
merge window is a big issue if we agree that the code is ready to go.

> The first three patches:
> 
>   PCI: Provide common functions for ECAM mapping
>   PCI: generic, thunder: Use generic ECAM API
>   PCI, of: Move PCI I/O space management to PCI core code
> 
> seem relatively straightforward, and I applied them to pci/arm64 with
> the intent of merging them unless there are objections.  I made the
> following tweaks, mainly to try to improve some error messages:

Ok, thanks a lot !

Lorenzo

> diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c
> index 3d52005..e1add01 100644
> --- a/drivers/pci/ecam.c
> +++ b/drivers/pci/ecam.c
> @@ -24,9 +24,9 @@
>  #include "ecam.h"
>  
>  /*
> - * On 64 bit systems, we do a single ioremap for the whole config space
> - * since we have enough virtual address range available. On 32 bit, do an
> - * ioremap per bus.
> + * On 64-bit systems, we do a single ioremap for the whole config space
> + * since we have enough virtual address range available.  On 32-bit, we
> + * ioremap the config space for each bus individually.
>   */
>  static const bool per_bus_mapping = !config_enabled(CONFIG_64BIT);
>  
> @@ -42,6 +42,7 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
>  {
>  	struct pci_config_window *cfg;
>  	unsigned int bus_range, bus_range_max, bsz;
> +	struct resource *conflict;
>  	int i, err;
>  
>  	if (busr->start > busr->end)
> @@ -58,10 +59,10 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
>  	bus_range = resource_size(&cfg->busr);
>  	bus_range_max = resource_size(cfgres) >> ops->bus_shift;
>  	if (bus_range > bus_range_max) {
> -		dev_warn(dev, "bus max %#x reduced to %#x",
> -					bus_range, bus_range_max);
>  		bus_range = bus_range_max;
>  		cfg->busr.end = busr->start + bus_range - 1;
> +		dev_warn(dev, "ECAM area %pR can only accommodate %pR (reduced from %pR desired)\n",
> +			 cfgres, &cfg->busr, busr);
>  	}
>  	bsz = 1 << ops->bus_shift;
>  
> @@ -70,9 +71,11 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
>  	cfg->res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
>  	cfg->res.name = "PCI ECAM";
>  
> -	err = request_resource(&iomem_resource, &cfg->res);
> -	if (err) {
> -		dev_err(dev, "request ECAM res %pR failed\n", &cfg->res);
> +	conflict = request_resource(&iomem_resource, &cfg->res);
> +	if (conflict) {
> +		err = -EBUSY;
> +		dev_err(dev, "can't claim ECAM area %pR: address conflict with %s %pR\n",
> +			&cfg->res, conflict->name, conflict);
>  		goto err_exit;
>  	}
>  
> diff --git a/drivers/pci/ecam.h b/drivers/pci/ecam.h
> index 1ad2176..9878beb 100644
> --- a/drivers/pci/ecam.h
> +++ b/drivers/pci/ecam.h
> @@ -33,7 +33,7 @@ struct pci_ecam_ops {
>  
>  /*
>   * struct to hold the mappings of a config space window. This
> - * is expected to be used as sysdata for PCI controlllers which
> + * is expected to be used as sysdata for PCI controllers that
>   * use ECAM.
>   */
>  struct pci_config_window {
> @@ -43,11 +43,11 @@ struct pci_config_window {
>  	struct pci_ecam_ops		*ops;
>  	union {
>  		void __iomem		*win;	/* 64-bit single mapping */
> -		void __iomem		**winp; /* 32-bit per bus mapping */
> +		void __iomem		**winp; /* 32-bit per-bus mapping */
>  	};
>  };
>  
> -/* create and free for pci_config_window */
> +/* create and free pci_config_window */
>  struct pci_config_window *pci_ecam_create(struct device *dev,
>  		struct resource *cfgres, struct resource *busr,
>  		struct pci_ecam_ops *ops);
> @@ -56,11 +56,11 @@ void pci_ecam_free(struct pci_config_window *cfg);
>  /* map_bus when ->sysdata is an instance of pci_config_window */
>  void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
>  			       int where);
> -/* default ECAM ops, bus shift 20, generic read and write */
> +/* default ECAM ops */
>  extern struct pci_ecam_ops pci_generic_ecam_ops;
>  
>  #ifdef CONFIG_PCI_HOST_GENERIC
> -/* for DT based pci controllers that support ECAM */
> +/* for DT-based PCI controllers that support ECAM */
>  int pci_host_common_probe(struct platform_device *pdev,
>  			  struct pci_ecam_ops *ops);
>  #endif
> 

[toc] | [prev] | [next] | [standalone]

#1399910

On Thu, May 12, 2016 at 4:13 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> On Wed, May 11, 2016 at 10:30:51PM +0200, Rafael J. Wysocki wrote:
>> On Wed, May 11, 2016 at 12:11 PM, Lorenzo Pieralisi
>> <lorenzo.pieralisi@arm.com> wrote:
>> > On Tue, May 10, 2016 at 08:37:00PM +0200, Rafael J. Wysocki wrote:
>> >> On Tue, May 10, 2016 at 5:19 PM, Tomasz Nowicki <tn@semihalf.com> wrote:
>> >> > This patch provides a way to set the ACPI companion in PCI code.
>> >> > We define acpi_pci_set_companion() to set the ACPI companion pointer and
>> >> > call it from PCI core code. The function is stub for now.
>> >> >
>> >> > Signed-off-by: Jayachandran C <jchandra@broadcom.com>
>> >> > Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>> >> > ---
>> >> >  drivers/pci/probe.c      | 2 ++
>> >> >  include/linux/pci-acpi.h | 4 ++++
>> >> >  2 files changed, 6 insertions(+)
>> >> >
>> >> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> >> > index 8004f67..fb0b752 100644
>> >> > --- a/drivers/pci/probe.c
>> >> > +++ b/drivers/pci/probe.c
>> >> > @@ -12,6 +12,7 @@
>> >> >  #include <linux/slab.h>
>> >> >  #include <linux/module.h>
>> >> >  #include <linux/cpumask.h>
>> >> > +#include <linux/pci-acpi.h>
>> >> >  #include <linux/pci-aspm.h>
>> >> >  #include <linux/aer.h>
>> >> >  #include <linux/acpi.h>
>> >> > @@ -2141,6 +2142,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>> >> >         bridge->dev.parent = parent;
>> >> >         bridge->dev.release = pci_release_host_bridge_dev;
>> >> >         dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
>> >> > +       acpi_pci_set_companion(bridge);
>> >>
>> >> Yes, we'll probably add something similar here.
>> >>
>> >> Do I think now is the right time to do that?  No.
>> >>
>> >> >         error = pcibios_root_bridge_prepare(bridge);
>> >> >         if (error) {
>> >> >                 kfree(bridge);
>> >> > diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>> >> > index 09f9f02..1baa515 100644
>> >> > --- a/include/linux/pci-acpi.h
>> >> > +++ b/include/linux/pci-acpi.h
>> >> > @@ -111,6 +111,10 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>> >> >  static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
>> >> >  #endif /* CONFIG_ACPI */
>> >> >
>> >> > +static inline void acpi_pci_set_companion(struct pci_host_bridge *bridge)
>> >> > +{
>> >> > +}
>> >> > +
>> >> >  static inline int acpi_pci_bus_domain_nr(struct pci_bus *bus)
>> >> >  {
>> >> >         return 0;
>> >> > --
>> >>
>> >> Honestly, to me it looks like this series is trying very hard to avoid
>> >> doing any PCI host bridge configuration stuff from arch/arm64/
>> >> although (a) that might be simpler and (b) it would allow us to
>> >> identify the code that's common between *all* architectures using ACPI
>> >> support for host bridge configuration and to move *that* to a common
>> >> place later.  As done here it seems to be following the "ARM64 is
>> >> generic and the rest of the world is special" line which isn't really
>> >> helpful.
>> >
>> > I think patch [1-2] should be merged regardless (they may require minor
>> > tweaks if we decide to move pci_acpi_scan_root() to arch/arm64 though,
>> > for include files location). I guess you are referring to patch 8 in
>> > your comments above, which boils down to deciding whether:
>> >
>> > - pci_acpi_scan_root() (and unfortunately all the MCFG/ECAM handling that
>> >   goes with it) should live in arch/arm64 or drivers/acpi
>>
>> To be precise, everything under #ifdef CONFIG_ACPI_PCI_HOST_GENERIC or
>> equivalent is de facto ARM64-specific, because (as it stands in the
>> patch series) ARM64 is the only architecture that will select that
>> option.  Unless you are aware of any more architectures planning to
>> use ACPI (and I'm not aware of any), it will stay the only
>> architecture selecting it in the foreseeable future.
>>
>> Therefore you could replace CONFIG_ACPI_PCI_HOST_GENERIC with
>> CONFIG_ARM64 everywhere in that code which is why in my opinion the
>> code should live somewhere under arch/arm64/.
>>
>> Going forward, it should be possible to identify common parts of the
>> PCI host bridge configuration code in arch/ and move it to
>> drivers/acpi/ or drivers/pci/, but I bet that won't be the entire code
>> this series puts under CONFIG_ACPI_PCI_HOST_GENERIC.
>>
>> The above leads to a quite straightforward conclusion about the order
>> in which to do things: I'd add ACPI support for PCI host bridge on
>> ARM64 following what's been done on ia64 (as x86 is more quirky and
>> kludgy overall) as far as reasonably possible first and then think
>> about moving common stuff to a common place.
>
> That does seem like a reasonable approach.  I had hoped to get more of
> this in for v4.7, but we don't have much time left.  Maybe some of
> Rafael's comments can be addressed by moving and slight restructuring
> and we can still squeeze it in.
>
> The first three patches:
>
>   PCI: Provide common functions for ECAM mapping
>   PCI: generic, thunder: Use generic ECAM API
>   PCI, of: Move PCI I/O space management to PCI core code
>
> seem relatively straightforward, and I applied them to pci/arm64 with
> the intent of merging them unless there are objections.  I made the
> following tweaks, mainly to try to improve some error messages:
>
> diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c
> index 3d52005..e1add01 100644
> --- a/drivers/pci/ecam.c
> +++ b/drivers/pci/ecam.c
> @@ -24,9 +24,9 @@
>  #include "ecam.h"
>
>  /*
> - * On 64 bit systems, we do a single ioremap for the whole config space
> - * since we have enough virtual address range available. On 32 bit, do an
> - * ioremap per bus.
> + * On 64-bit systems, we do a single ioremap for the whole config space
> + * since we have enough virtual address range available.  On 32-bit, we
> + * ioremap the config space for each bus individually.
>   */
>  static const bool per_bus_mapping = !config_enabled(CONFIG_64BIT);
>
> @@ -42,6 +42,7 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
>  {
>         struct pci_config_window *cfg;
>         unsigned int bus_range, bus_range_max, bsz;
> +       struct resource *conflict;
>         int i, err;
>
>         if (busr->start > busr->end)
> @@ -58,10 +59,10 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
>         bus_range = resource_size(&cfg->busr);
>         bus_range_max = resource_size(cfgres) >> ops->bus_shift;
>         if (bus_range > bus_range_max) {
> -               dev_warn(dev, "bus max %#x reduced to %#x",
> -                                       bus_range, bus_range_max);
>                 bus_range = bus_range_max;
>                 cfg->busr.end = busr->start + bus_range - 1;
> +               dev_warn(dev, "ECAM area %pR can only accommodate %pR (reduced from %pR desired)\n",
> +                        cfgres, &cfg->busr, busr);
>         }
>         bsz = 1 << ops->bus_shift;
>
> @@ -70,9 +71,11 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
>         cfg->res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
>         cfg->res.name = "PCI ECAM";
>
> -       err = request_resource(&iomem_resource, &cfg->res);
> -       if (err) {
> -               dev_err(dev, "request ECAM res %pR failed\n", &cfg->res);
> +       conflict = request_resource(&iomem_resource, &cfg->res);
> +       if (conflict) {
> +               err = -EBUSY;
> +               dev_err(dev, "can't claim ECAM area %pR: address conflict with %s %pR\n",
> +                       &cfg->res, conflict->name, conflict);
>                 goto err_exit;
>         }
>
> diff --git a/drivers/pci/ecam.h b/drivers/pci/ecam.h
> index 1ad2176..9878beb 100644
> --- a/drivers/pci/ecam.h
> +++ b/drivers/pci/ecam.h
> @@ -33,7 +33,7 @@ struct pci_ecam_ops {
>
>  /*
>   * struct to hold the mappings of a config space window. This
> - * is expected to be used as sysdata for PCI controlllers which
> + * is expected to be used as sysdata for PCI controllers that
>   * use ECAM.
>   */
>  struct pci_config_window {
> @@ -43,11 +43,11 @@ struct pci_config_window {
>         struct pci_ecam_ops             *ops;
>         union {
>                 void __iomem            *win;   /* 64-bit single mapping */
> -               void __iomem            **winp; /* 32-bit per bus mapping */
> +               void __iomem            **winp; /* 32-bit per-bus mapping */
>         };
>  };
>
> -/* create and free for pci_config_window */
> +/* create and free pci_config_window */
>  struct pci_config_window *pci_ecam_create(struct device *dev,
>                 struct resource *cfgres, struct resource *busr,
>                 struct pci_ecam_ops *ops);
> @@ -56,11 +56,11 @@ void pci_ecam_free(struct pci_config_window *cfg);
>  /* map_bus when ->sysdata is an instance of pci_config_window */
>  void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn,
>                                int where);
> -/* default ECAM ops, bus shift 20, generic read and write */
> +/* default ECAM ops */
>  extern struct pci_ecam_ops pci_generic_ecam_ops;
>
>  #ifdef CONFIG_PCI_HOST_GENERIC
> -/* for DT based pci controllers that support ECAM */
> +/* for DT-based PCI controllers that support ECAM */
>  int pci_host_common_probe(struct platform_device *pdev,
>                           struct pci_ecam_ops *ops);
>  #endif

If we are moving the ACPI/PCI code from drivers/acpi to
arch/arm64/ , there is an issue in having the header file
ecam.h in drivers/pci

The current include of "../pci/ecam.h" is slightly ugly (Arnd
and David had already noted this), but including the driver
header from arch code would be even worse.

I can either merge ecam.h into include/linux/pci.h
or move it to a new file include/linux/pci-ecam.h, any
suggestion on which is preferable?

JC.

[toc] | [prev] | [next] | [standalone]

#1399940

On Thu, May 12, 2016 at 12:43 PM, Jayachandran C <jchandra@broadcom.com> wrote:
> On Thu, May 12, 2016 at 4:13 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
>> On Wed, May 11, 2016 at 10:30:51PM +0200, Rafael J. Wysocki wrote:
>>> On Wed, May 11, 2016 at 12:11 PM, Lorenzo Pieralisi
>>> <lorenzo.pieralisi@arm.com> wrote:
>>> > On Tue, May 10, 2016 at 08:37:00PM +0200, Rafael J. Wysocki wrote:
>>> >> On Tue, May 10, 2016 at 5:19 PM, Tomasz Nowicki <tn@semihalf.com> wrote:

[cut]

>
> If we are moving the ACPI/PCI code from drivers/acpi to
> arch/arm64/ , there is an issue in having the header file
> ecam.h in drivers/pci
>
> The current include of "../pci/ecam.h" is slightly ugly (Arnd
> and David had already noted this), but including the driver
> header from arch code would be even worse.
>
> I can either merge ecam.h into include/linux/pci.h
> or move it to a new file include/linux/pci-ecam.h, any
> suggestion on which is preferable?

My preference would be pci-ecam.h as we did a similar thing for
pci-dma.h, for example, but basically this is up to Bjorn.

[toc] | [prev] | [next] | [standalone]

#1400691

On Thu, May 12, 2016 at 01:27:23PM +0200, Rafael J. Wysocki wrote:
> On Thu, May 12, 2016 at 12:43 PM, Jayachandran C <jchandra@broadcom.com> wrote:
> > On Thu, May 12, 2016 at 4:13 AM, Bjorn Helgaas <helgaas@kernel.org> wrote:
> >> On Wed, May 11, 2016 at 10:30:51PM +0200, Rafael J. Wysocki wrote:
> >>> On Wed, May 11, 2016 at 12:11 PM, Lorenzo Pieralisi
> >>> <lorenzo.pieralisi@arm.com> wrote:
> >>> > On Tue, May 10, 2016 at 08:37:00PM +0200, Rafael J. Wysocki wrote:
> >>> >> On Tue, May 10, 2016 at 5:19 PM, Tomasz Nowicki <tn@semihalf.com> wrote:
> 
> [cut]
> 
> >
> > If we are moving the ACPI/PCI code from drivers/acpi to
> > arch/arm64/ , there is an issue in having the header file
> > ecam.h in drivers/pci
> >
> > The current include of "../pci/ecam.h" is slightly ugly (Arnd
> > and David had already noted this), but including the driver
> > header from arch code would be even worse.
> >
> > I can either merge ecam.h into include/linux/pci.h
> > or move it to a new file include/linux/pci-ecam.h, any
> > suggestion on which is preferable?
> 
> My preference would be pci-ecam.h as we did a similar thing for
> pci-dma.h, for example, but basically this is up to Bjorn.

A word of caution for all interested parties, what we may move
to arch/arm64 (if Catalin and Will are ok with that) here is content
of drivers/acpi/pci_root_generic.c, not drivers/acpi/pci_mcfg.c (and
definitely not the MCFG quirks handling that is coming up next on top
of this series).

I just wanted to make sure we understand that MCFG quirks handling
like eg:

https://lkml.org/lkml/2016/4/28/790

that is coming up following this series has no chance whatsoever to
be handled within arch/arm64, it is just not going to happen.

Maybe I am jumping the gun, I just want to make sure that everyone is
aware that moving part of this series to arch/arm64 has implications,
(and that's why I said that moving part of this code to arch/arm64 is
not as simple as it looks) it may be ok to have an ACPI PCI
implementation that is arch/arm64 specific (mostly for IO space and PCI
resources assignment handling that unfortunately is not uniform across
X86, IA64 and ARM64), but MCFG quirks and related platform code stay out
of arch/arm64 I guess we are all aware of that, just wanted to make
sure :)

Lorenzo

[toc] | [prev] | [next] | [standalone]

#1399916

On 12.05.2016 00:43, Bjorn Helgaas wrote:
> On Wed, May 11, 2016 at 10:30:51PM +0200, Rafael J. Wysocki wrote:
>> On Wed, May 11, 2016 at 12:11 PM, Lorenzo Pieralisi
>> <lorenzo.pieralisi@arm.com> wrote:
>>> On Tue, May 10, 2016 at 08:37:00PM +0200, Rafael J. Wysocki wrote:
>>>> On Tue, May 10, 2016 at 5:19 PM, Tomasz Nowicki <tn@semihalf.com> wrote:
>>>>> This patch provides a way to set the ACPI companion in PCI code.
>>>>> We define acpi_pci_set_companion() to set the ACPI companion pointer and
>>>>> call it from PCI core code. The function is stub for now.
>>>>>
>>>>> Signed-off-by: Jayachandran C <jchandra@broadcom.com>
>>>>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>>>>> ---
>>>>>   drivers/pci/probe.c      | 2 ++
>>>>>   include/linux/pci-acpi.h | 4 ++++
>>>>>   2 files changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>>>> index 8004f67..fb0b752 100644
>>>>> --- a/drivers/pci/probe.c
>>>>> +++ b/drivers/pci/probe.c
>>>>> @@ -12,6 +12,7 @@
>>>>>   #include <linux/slab.h>
>>>>>   #include <linux/module.h>
>>>>>   #include <linux/cpumask.h>
>>>>> +#include <linux/pci-acpi.h>
>>>>>   #include <linux/pci-aspm.h>
>>>>>   #include <linux/aer.h>
>>>>>   #include <linux/acpi.h>
>>>>> @@ -2141,6 +2142,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>>>>>          bridge->dev.parent = parent;
>>>>>          bridge->dev.release = pci_release_host_bridge_dev;
>>>>>          dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
>>>>> +       acpi_pci_set_companion(bridge);
>>>>
>>>> Yes, we'll probably add something similar here.
>>>>
>>>> Do I think now is the right time to do that?  No.
>>>>
>>>>>          error = pcibios_root_bridge_prepare(bridge);
>>>>>          if (error) {
>>>>>                  kfree(bridge);
>>>>> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>>>>> index 09f9f02..1baa515 100644
>>>>> --- a/include/linux/pci-acpi.h
>>>>> +++ b/include/linux/pci-acpi.h
>>>>> @@ -111,6 +111,10 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>>>>>   static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
>>>>>   #endif /* CONFIG_ACPI */
>>>>>
>>>>> +static inline void acpi_pci_set_companion(struct pci_host_bridge *bridge)
>>>>> +{
>>>>> +}
>>>>> +
>>>>>   static inline int acpi_pci_bus_domain_nr(struct pci_bus *bus)
>>>>>   {
>>>>>          return 0;
>>>>> --
>>>>
>>>> Honestly, to me it looks like this series is trying very hard to avoid
>>>> doing any PCI host bridge configuration stuff from arch/arm64/
>>>> although (a) that might be simpler and (b) it would allow us to
>>>> identify the code that's common between *all* architectures using ACPI
>>>> support for host bridge configuration and to move *that* to a common
>>>> place later.  As done here it seems to be following the "ARM64 is
>>>> generic and the rest of the world is special" line which isn't really
>>>> helpful.
>>>
>>> I think patch [1-2] should be merged regardless (they may require minor
>>> tweaks if we decide to move pci_acpi_scan_root() to arch/arm64 though,
>>> for include files location). I guess you are referring to patch 8 in
>>> your comments above, which boils down to deciding whether:
>>>
>>> - pci_acpi_scan_root() (and unfortunately all the MCFG/ECAM handling that
>>>    goes with it) should live in arch/arm64 or drivers/acpi
>>
>> To be precise, everything under #ifdef CONFIG_ACPI_PCI_HOST_GENERIC or
>> equivalent is de facto ARM64-specific, because (as it stands in the
>> patch series) ARM64 is the only architecture that will select that
>> option.  Unless you are aware of any more architectures planning to
>> use ACPI (and I'm not aware of any), it will stay the only
>> architecture selecting it in the foreseeable future.
>>
>> Therefore you could replace CONFIG_ACPI_PCI_HOST_GENERIC with
>> CONFIG_ARM64 everywhere in that code which is why in my opinion the
>> code should live somewhere under arch/arm64/.
>>
>> Going forward, it should be possible to identify common parts of the
>> PCI host bridge configuration code in arch/ and move it to
>> drivers/acpi/ or drivers/pci/, but I bet that won't be the entire code
>> this series puts under CONFIG_ACPI_PCI_HOST_GENERIC.
>>
>> The above leads to a quite straightforward conclusion about the order
>> in which to do things: I'd add ACPI support for PCI host bridge on
>> ARM64 following what's been done on ia64 (as x86 is more quirky and
>> kludgy overall) as far as reasonably possible first and then think
>> about moving common stuff to a common place.
>
> That does seem like a reasonable approach.  I had hoped to get more of
> this in for v4.7, but we don't have much time left.  Maybe some of
> Rafael's comments can be addressed by moving and slight restructuring
> and we can still squeeze it in.
>
> The first three patches:
>
>    PCI: Provide common functions for ECAM mapping
>    PCI: generic, thunder: Use generic ECAM API
>    PCI, of: Move PCI I/O space management to PCI core code
>
> seem relatively straightforward, and I applied them to pci/arm64 with
> the intent of merging them unless there are objections.  I made the
> following tweaks, mainly to try to improve some error messages:
>
> diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c
> index 3d52005..e1add01 100644
> --- a/drivers/pci/ecam.c
> +++ b/drivers/pci/ecam.c
> @@ -24,9 +24,9 @@
>   #include "ecam.h"
>
>   /*
> - * On 64 bit systems, we do a single ioremap for the whole config space
> - * since we have enough virtual address range available. On 32 bit, do an
> - * ioremap per bus.
> + * On 64-bit systems, we do a single ioremap for the whole config space
> + * since we have enough virtual address range available.  On 32-bit, we
> + * ioremap the config space for each bus individually.
>    */
>   static const bool per_bus_mapping = !config_enabled(CONFIG_64BIT);
>
> @@ -42,6 +42,7 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
>   {
>   	struct pci_config_window *cfg;
>   	unsigned int bus_range, bus_range_max, bsz;
> +	struct resource *conflict;
>   	int i, err;
>
>   	if (busr->start > busr->end)
> @@ -58,10 +59,10 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
>   	bus_range = resource_size(&cfg->busr);
>   	bus_range_max = resource_size(cfgres) >> ops->bus_shift;
>   	if (bus_range > bus_range_max) {
> -		dev_warn(dev, "bus max %#x reduced to %#x",
> -					bus_range, bus_range_max);
>   		bus_range = bus_range_max;
>   		cfg->busr.end = busr->start + bus_range - 1;
> +		dev_warn(dev, "ECAM area %pR can only accommodate %pR (reduced from %pR desired)\n",
> +			 cfgres, &cfg->busr, busr);
>   	}
>   	bsz = 1 << ops->bus_shift;
>
> @@ -70,9 +71,11 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
>   	cfg->res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
>   	cfg->res.name = "PCI ECAM";
>
> -	err = request_resource(&iomem_resource, &cfg->res);
> -	if (err) {
> -		dev_err(dev, "request ECAM res %pR failed\n", &cfg->res);
> +	conflict = request_resource(&iomem_resource, &cfg->res);

We need request_resource_conflict here then:
-	conflict = request_resource(&iomem_resource, &cfg->res);
+	conflict = request_resource_conflict(&iomem_resource, &cfg->res);

Thanks,
Tomasz

[toc] | [prev] | [next] | [standalone]

#1399974

On Thu, May 12, 2016 at 12:50:07PM +0200, Tomasz Nowicki wrote:
> On 12.05.2016 00:43, Bjorn Helgaas wrote:

> >@@ -70,9 +71,11 @@ struct pci_config_window *pci_ecam_create(struct device *dev,
> >  	cfg->res.flags = IORESOURCE_MEM | IORESOURCE_BUSY;
> >  	cfg->res.name = "PCI ECAM";
> >
> >-	err = request_resource(&iomem_resource, &cfg->res);
> >-	if (err) {
> >-		dev_err(dev, "request ECAM res %pR failed\n", &cfg->res);
> >+	conflict = request_resource(&iomem_resource, &cfg->res);
> 
> We need request_resource_conflict here then:
> -	conflict = request_resource(&iomem_resource, &cfg->res);
> +	conflict = request_resource_conflict(&iomem_resource, &cfg->res);

Whoops, fixed, thanks!

[toc] | [prev] | [next] | [standalone]

#1402057

Hi Tomasz

I used the patchset and added "PATCH V6 11/13 specic quirks", tested on HiSilicon D02 board but met the below problem.

[    2.614115] [<ffffff80083b13bc>] hisi_pcie_init+0x6c/0x1ec
[    2.619571] [<ffffff80083ab060>] pci_ecam_create+0x130/0x1ec
[    2.625209] [<ffffff80083f3764>] pci_acpi_scan_root+0x160/0x218
[    2.631096] [<ffffff80083d1f6c>] acpi_pci_root_add+0x36c/0x42c
[    2.636897] [<ffffff80083ce36c>] acpi_bus_attach+0xe4/0x1a8
[    2.642438] [<ffffff80083ce3d8>] acpi_bus_attach+0x150/0x1a8
[    2.648066] [<ffffff80083ce3d8>] acpi_bus_attach+0x150/0x1a8
[    2.653693] [<ffffff80083ce55c>] acpi_bus_scan+0x64/0x74
[    2.658975] [<ffffff8008ae665c>] acpi_scan_init+0x5c/0x19c
[    2.664429] [<ffffff8008ae6408>] acpi_init+0x280/0x2a4
[    2.669538] [<ffffff80080829e8>] do_one_initcall+0x8c/0x19c
[    2.675080] [<ffffff8008ac3af8>] kernel_init_freeable+0x14c/0x1ec
[    2.681139] [<ffffff80087a8438>] kernel_init+0x10/0xfc
[    2.686248] [<ffffff8008085e10>] ret_from_fork+0x10/0x40

In hisi_pcie_init, I used "struct acpi_device *device = ACPI_COMPANION(dev);".
I found the reason is V7 lack the below code. I added the below code, it worked ok.

[PATCH V6 01/13] pci, acpi, x86, ia64: Move ACPI host bridge device companion assignment to core code.
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -564,6 +564,11 @@ static int acpi_pci_root_add(struct acpi_device *device,
  		}
  	}

+	/*
+	 * pci_create_root_bus() needs to detect the parent device type,
+	 * so initialize its companion data accordingly.
+	 */
+	ACPI_COMPANION_SET(&device->dev, device);

This code will be upstreamed with the "PATCH V6 11/13 specic quirks" in next time after the patchset is accepted.
Right ?

在 2016/5/10 23:19, Tomasz Nowicki 写道:
> This patch provides a way to set the ACPI companion in PCI code.
> We define acpi_pci_set_companion() to set the ACPI companion pointer and
> call it from PCI core code. The function is stub for now.
>
> Signed-off-by: Jayachandran C <jchandra@broadcom.com>
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> ---
>   drivers/pci/probe.c      | 2 ++
>   include/linux/pci-acpi.h | 4 ++++
>   2 files changed, 6 insertions(+)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 8004f67..fb0b752 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -12,6 +12,7 @@
>   #include <linux/slab.h>
>   #include <linux/module.h>
>   #include <linux/cpumask.h>
> +#include <linux/pci-acpi.h>
>   #include <linux/pci-aspm.h>
>   #include <linux/aer.h>
>   #include <linux/acpi.h>
> @@ -2141,6 +2142,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>   	bridge->dev.parent = parent;
>   	bridge->dev.release = pci_release_host_bridge_dev;
>   	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
> +	acpi_pci_set_companion(bridge);
>   	error = pcibios_root_bridge_prepare(bridge);
>   	if (error) {
>   		kfree(bridge);
> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
> index 09f9f02..1baa515 100644
> --- a/include/linux/pci-acpi.h
> +++ b/include/linux/pci-acpi.h
> @@ -111,6 +111,10 @@ static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>   static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
>   #endif	/* CONFIG_ACPI */
>
> +static inline void acpi_pci_set_companion(struct pci_host_bridge *bridge)
> +{
> +}
> +
>   static inline int acpi_pci_bus_domain_nr(struct pci_bus *bus)
>   {
>   	return 0;
>

[toc] | [prev] | [next] | [standalone]

#1402348

On 17.05.2016 05:11, Dongdong Liu wrote:
> Hi Tomasz
>
> I used the patchset and added "PATCH V6 11/13 specic quirks", tested on
> HiSilicon D02 board but met the below problem.
>
> [    2.614115] [<ffffff80083b13bc>] hisi_pcie_init+0x6c/0x1ec
> [    2.619571] [<ffffff80083ab060>] pci_ecam_create+0x130/0x1ec
> [    2.625209] [<ffffff80083f3764>] pci_acpi_scan_root+0x160/0x218
> [    2.631096] [<ffffff80083d1f6c>] acpi_pci_root_add+0x36c/0x42c
> [    2.636897] [<ffffff80083ce36c>] acpi_bus_attach+0xe4/0x1a8
> [    2.642438] [<ffffff80083ce3d8>] acpi_bus_attach+0x150/0x1a8
> [    2.648066] [<ffffff80083ce3d8>] acpi_bus_attach+0x150/0x1a8
> [    2.653693] [<ffffff80083ce55c>] acpi_bus_scan+0x64/0x74
> [    2.658975] [<ffffff8008ae665c>] acpi_scan_init+0x5c/0x19c
> [    2.664429] [<ffffff8008ae6408>] acpi_init+0x280/0x2a4
> [    2.669538] [<ffffff80080829e8>] do_one_initcall+0x8c/0x19c
> [    2.675080] [<ffffff8008ac3af8>] kernel_init_freeable+0x14c/0x1ec
> [    2.681139] [<ffffff80087a8438>] kernel_init+0x10/0xfc
> [    2.686248] [<ffffff8008085e10>] ret_from_fork+0x10/0x40
>
> In hisi_pcie_init, I used "struct acpi_device *device =
> ACPI_COMPANION(dev);".
> I found the reason is V7 lack the below code. I added the below code, it
> worked ok.
>
> [PATCH V6 01/13] pci, acpi, x86, ia64: Move ACPI host bridge device
> companion assignment to core code.
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -564,6 +564,11 @@ static int acpi_pci_root_add(struct acpi_device
> *device,
>           }
>       }
>
> +    /*
> +     * pci_create_root_bus() needs to detect the parent device type,
> +     * so initialize its companion data accordingly.
> +     */
> +    ACPI_COMPANION_SET(&device->dev, device);
>
> This code will be upstreamed with the "PATCH V6 11/13 specic quirks" in
> next time after the patchset is accepted.
> Right ?

We had that patch in previous series to retrieve PCI domain nicely. But 
that has bad implication to userspace. See:
https://lkml.org/lkml/2016/5/9/918

I understand that:
[PATCH V6 01/13] pci, acpi, x86, ia64: Move ACPI host bridge device 
companion assignment to core code.
helps to get firmware specific info in hisi_pcie_init but we need to 
figure out something better for quirk handling too.

Tomasz

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web