[V8,5/9] pci, acpi: add acpi hook to assign domain number.

PCI core code provides a config option (CONFIG_PCI_DOMAINS_GENERIC)
that allows assigning the PCI bus domain number generically by
relying on device tree bindings, and falling back to a simple counter
when the respective DT properties (ie "linux,pci-domain") are not
specified in the host bridge device tree node.

In a similar way, when a system is booted through ACPI, architectures
that are selecting CONFIG_PCI_DOMAINS_GENERIC (ie ARM64) require kernel
hooks to retrieve the domain number so that the PCI bus domain number
set-up can be handled seamlessly with DT and ACPI in generic core code
when CONFIG_PCI_DOMAINS_GENERIC is selected.

Since currently it is not possible to retrieve a pointer to the PCI
host bridge ACPI device backing the host bridge from core PCI code
(which would allow retrieving the domain number in an arch agnostic
way through the ACPI _SEG method), an arch specific ACPI hook has to
be declared and implemented by all arches that rely on
CONFIG_PCI_DOMAINS_GENERIC to retrieve the domain number and set it
up in core PCI code.

For the aforementioned reasons, this patch introduces a dummy
acpi_pci_bus_domain_nr() hook in preparation for per-arch implementation
of the same to retrieve the domain number on a per-arch basis when
the system boots through ACPI.

For the sake of code clarity the current code implementing generic
domain number assignment (ie pci_bus_assign_domain_nr(), selected by
CONFIG_PCI_DOMAINS_GENERIC) is reshuffled so that the code implementing
the DT domain assignment function is stubbed out into a corresponding
helper, so that DT and ACPI functions are clearly separated in
preparation for arches acpi_pci_bus_domain_nr() implementations.

Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
---
 drivers/pci/pci.c   | 11 +++++++++--
 include/linux/pci.h |  1 +
 2 files changed, 10 insertions(+), 2 deletions(-)

Hi Tomasz,

On Mon, May 30, 2016 at 05:14:18PM +0200, Tomasz Nowicki wrote:
> PCI core code provides a config option (CONFIG_PCI_DOMAINS_GENERIC)
> that allows assigning the PCI bus domain number generically by
> relying on device tree bindings, and falling back to a simple counter
> when the respective DT properties (ie "linux,pci-domain") are not
> specified in the host bridge device tree node.
> 
> In a similar way, when a system is booted through ACPI, architectures
> that are selecting CONFIG_PCI_DOMAINS_GENERIC (ie ARM64) require kernel
> hooks to retrieve the domain number so that the PCI bus domain number
> set-up can be handled seamlessly with DT and ACPI in generic core code
> when CONFIG_PCI_DOMAINS_GENERIC is selected.
> 
> Since currently it is not possible to retrieve a pointer to the PCI
> host bridge ACPI device backing the host bridge from core PCI code
> (which would allow retrieving the domain number in an arch agnostic
> way through the ACPI _SEG method), an arch specific ACPI hook has to
> be declared and implemented by all arches that rely on
> CONFIG_PCI_DOMAINS_GENERIC to retrieve the domain number and set it
> up in core PCI code.
> 
> For the aforementioned reasons, this patch introduces a dummy
> acpi_pci_bus_domain_nr() hook in preparation for per-arch implementation
> of the same to retrieve the domain number on a per-arch basis when
> the system boots through ACPI.
> 
> For the sake of code clarity the current code implementing generic
> domain number assignment (ie pci_bus_assign_domain_nr(), selected by
> CONFIG_PCI_DOMAINS_GENERIC) is reshuffled so that the code implementing
> the DT domain assignment function is stubbed out into a corresponding
> helper, so that DT and ACPI functions are clearly separated in
> preparation for arches acpi_pci_bus_domain_nr() implementations.
> 
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> ---
>  drivers/pci/pci.c   | 11 +++++++++--
>  include/linux/pci.h |  1 +
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index eb431b5..2b52178 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -7,6 +7,7 @@
>   *	Copyright 1997 -- 2000 Martin Mares <mj@ucw.cz>
>   */
>  
> +#include <linux/acpi.h>
>  #include <linux/kernel.h>
>  #include <linux/delay.h>
>  #include <linux/init.h>
> @@ -4941,7 +4942,7 @@ int pci_get_new_domain_nr(void)
>  }
>  
>  #ifdef CONFIG_PCI_DOMAINS_GENERIC
> -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> +static int of_pci_bus_domain_nr(struct device *parent)

Can we do a little cleanup before this patch?  

  - pci_bus_assign_domain_nr() is only used inside drivers/pci, so
    maybe we move the prototype to drivers/pci/pci.h?

  - I don't really like the style of calling a function that
    internally assigns bus->domain_nr.  Could we do something like
    this instead?

    int pci_bus_domain_nr(...)
    {
      ...
      return domain;
    }

    ... pci_create_root_bus(...)
    {
      ...
      b->domain_nr = pci_bus_domain_nr(...);

That would be two new patches, if this makes sense.

And this patch would only rename pci_bus_assign_domain_nr() to
of_pci_bus_domain_nr() and add the pci_bus_domain_nr() wrapper.

>  {
>  	static int use_dt_domains = -1;
>  	int domain = -1;
> @@ -4985,7 +4986,13 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
>  		domain = -1;
>  	}
>  
> -	bus->domain_nr = domain;
> +	return domain;
> +}
> +
> +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> +{
> +	bus->domain_nr = acpi_disabled ? of_pci_bus_domain_nr(parent) :
> +					 acpi_pci_bus_domain_nr(bus);
>  }
>  #endif
>  #endif
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 12349de..bba4053 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1390,6 +1390,7 @@ static inline int pci_domain_nr(struct pci_bus *bus)
>  {
>  	return bus->domain_nr;
>  }
> +static inline int acpi_pci_bus_domain_nr(struct pci_bus *bus) { return -1; }

I would split the addition of acpi_pci_bus_domain_nr() to a separate
patch and include the ARM64 definition in that same patch.  That patch
would only add this stub definition, the ARM64 definition, and the new
call in pci_bus_domain_nr().

>  void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent);
>  #else
>  static inline void pci_bus_assign_domain_nr(struct pci_bus *bus,
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Bjorn,

On 08.06.2016 02:15, Bjorn Helgaas wrote:
> Hi Tomasz,
>
> On Mon, May 30, 2016 at 05:14:18PM +0200, Tomasz Nowicki wrote:
>> PCI core code provides a config option (CONFIG_PCI_DOMAINS_GENERIC)
>> that allows assigning the PCI bus domain number generically by
>> relying on device tree bindings, and falling back to a simple counter
>> when the respective DT properties (ie "linux,pci-domain") are not
>> specified in the host bridge device tree node.
>>
>> In a similar way, when a system is booted through ACPI, architectures
>> that are selecting CONFIG_PCI_DOMAINS_GENERIC (ie ARM64) require kernel
>> hooks to retrieve the domain number so that the PCI bus domain number
>> set-up can be handled seamlessly with DT and ACPI in generic core code
>> when CONFIG_PCI_DOMAINS_GENERIC is selected.
>>
>> Since currently it is not possible to retrieve a pointer to the PCI
>> host bridge ACPI device backing the host bridge from core PCI code
>> (which would allow retrieving the domain number in an arch agnostic
>> way through the ACPI _SEG method), an arch specific ACPI hook has to
>> be declared and implemented by all arches that rely on
>> CONFIG_PCI_DOMAINS_GENERIC to retrieve the domain number and set it
>> up in core PCI code.
>>
>> For the aforementioned reasons, this patch introduces a dummy
>> acpi_pci_bus_domain_nr() hook in preparation for per-arch implementation
>> of the same to retrieve the domain number on a per-arch basis when
>> the system boots through ACPI.
>>
>> For the sake of code clarity the current code implementing generic
>> domain number assignment (ie pci_bus_assign_domain_nr(), selected by
>> CONFIG_PCI_DOMAINS_GENERIC) is reshuffled so that the code implementing
>> the DT domain assignment function is stubbed out into a corresponding
>> helper, so that DT and ACPI functions are clearly separated in
>> preparation for arches acpi_pci_bus_domain_nr() implementations.
>>
>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> ---
>>   drivers/pci/pci.c   | 11 +++++++++--
>>   include/linux/pci.h |  1 +
>>   2 files changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index eb431b5..2b52178 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -7,6 +7,7 @@
>>    *	Copyright 1997 -- 2000 Martin Mares <mj@ucw.cz>
>>    */
>>
>> +#include <linux/acpi.h>
>>   #include <linux/kernel.h>
>>   #include <linux/delay.h>
>>   #include <linux/init.h>
>> @@ -4941,7 +4942,7 @@ int pci_get_new_domain_nr(void)
>>   }
>>
>>   #ifdef CONFIG_PCI_DOMAINS_GENERIC
>> -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
>> +static int of_pci_bus_domain_nr(struct device *parent)
>
> Can we do a little cleanup before this patch?
>
>    - pci_bus_assign_domain_nr() is only used inside drivers/pci, so
>      maybe we move the prototype to drivers/pci/pci.h?

pci_bus_assign_domain_nr() goes with pci_domain_nr() as an option for 
CONFIG_PCI_DOMAINS_GENERIC. pci_domain_nr() is used commonly outside 
drivers/pci so we would need to split these calls then, thus personally 
I think it would be better to keep both in inclue/linux/pci.h

>
>    - I don't really like the style of calling a function that
>      internally assigns bus->domain_nr.  Could we do something like
>      this instead?
>
>      int pci_bus_domain_nr(...)
>      {
>        ...
>        return domain;
>      }
>
>      ... pci_create_root_bus(...)
>      {
>        ...
>        b->domain_nr = pci_bus_domain_nr(...);


We can. I do not see much difference between pci_bus_domain_nr() and 
pci_domain_nr() which we already have so how about calling it 
pci_bus_find_domain_nr instead? Lorenzo, any strong preference for it?

>
> That would be two new patches, if this makes sense.
>
> And this patch would only rename pci_bus_assign_domain_nr() to
> of_pci_bus_domain_nr() and add the pci_bus_domain_nr() wrapper.

Giving that we would keep prototypes in inclue/linux/pci.h

1. First patch would rename pci_bus_assign_domain_nr() to 
pci_bus_find_domain_nr() and it would return domain number so that we 
could do:
      ... pci_create_root_bus(...)
      {
        ...
        b->domain_nr = pci_bus_domain_nr(...);
        ...
      }

2. Second patch would transform pci_bus_find_domain_nr() to be the 
wrapper for of_pci_bus_domain_nr()

3. Third patch would add stub definition, the ARM64 definition and the 
new call acpi_pci_bus_domain_nr() in pci_bus_find_domain_nr() wrapper.

Does this plan sound reasonable?

>
>>   {
>>   	static int use_dt_domains = -1;
>>   	int domain = -1;
>> @@ -4985,7 +4986,13 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
>>   		domain = -1;
>>   	}
>>
>> -	bus->domain_nr = domain;
>> +	return domain;
>> +}
>> +
>> +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
>> +{
>> +	bus->domain_nr = acpi_disabled ? of_pci_bus_domain_nr(parent) :
>> +					 acpi_pci_bus_domain_nr(bus);
>>   }
>>   #endif
>>   #endif
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index 12349de..bba4053 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1390,6 +1390,7 @@ static inline int pci_domain_nr(struct pci_bus *bus)
>>   {
>>   	return bus->domain_nr;
>>   }
>> +static inline int acpi_pci_bus_domain_nr(struct pci_bus *bus) { return -1; }
>
> I would split the addition of acpi_pci_bus_domain_nr() to a separate
> patch and include the ARM64 definition in that same patch.  That patch
> would only add this stub definition, the ARM64 definition, and the new
> call in pci_bus_domain_nr().

OK

Thanks,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Jun 08, 2016 at 12:21:19PM +0200, Tomasz Nowicki wrote:
> On 08.06.2016 02:15, Bjorn Helgaas wrote:
> >On Mon, May 30, 2016 at 05:14:18PM +0200, Tomasz Nowicki wrote:
> >>PCI core code provides a config option (CONFIG_PCI_DOMAINS_GENERIC)
> >>that allows assigning the PCI bus domain number generically by
> >>relying on device tree bindings, and falling back to a simple counter
> >>when the respective DT properties (ie "linux,pci-domain") are not
> >>specified in the host bridge device tree node.
> >>
> >>In a similar way, when a system is booted through ACPI, architectures
> >>that are selecting CONFIG_PCI_DOMAINS_GENERIC (ie ARM64) require kernel
> >>hooks to retrieve the domain number so that the PCI bus domain number
> >>set-up can be handled seamlessly with DT and ACPI in generic core code
> >>when CONFIG_PCI_DOMAINS_GENERIC is selected.
> >>
> >>Since currently it is not possible to retrieve a pointer to the PCI
> >>host bridge ACPI device backing the host bridge from core PCI code
> >>(which would allow retrieving the domain number in an arch agnostic
> >>way through the ACPI _SEG method), an arch specific ACPI hook has to
> >>be declared and implemented by all arches that rely on
> >>CONFIG_PCI_DOMAINS_GENERIC to retrieve the domain number and set it
> >>up in core PCI code.
> >>
> >>For the aforementioned reasons, this patch introduces a dummy
> >>acpi_pci_bus_domain_nr() hook in preparation for per-arch implementation
> >>of the same to retrieve the domain number on a per-arch basis when
> >>the system boots through ACPI.
> >>
> >>For the sake of code clarity the current code implementing generic
> >>domain number assignment (ie pci_bus_assign_domain_nr(), selected by
> >>CONFIG_PCI_DOMAINS_GENERIC) is reshuffled so that the code implementing
> >>the DT domain assignment function is stubbed out into a corresponding
> >>helper, so that DT and ACPI functions are clearly separated in
> >>preparation for arches acpi_pci_bus_domain_nr() implementations.
> >>
> >>Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> >>Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> >>---
> >>  drivers/pci/pci.c   | 11 +++++++++--
> >>  include/linux/pci.h |  1 +
> >>  2 files changed, 10 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> >>index eb431b5..2b52178 100644
> >>--- a/drivers/pci/pci.c
> >>+++ b/drivers/pci/pci.c
> >>@@ -7,6 +7,7 @@
> >>   *	Copyright 1997 -- 2000 Martin Mares <mj@ucw.cz>
> >>   */
> >>
> >>+#include <linux/acpi.h>
> >>  #include <linux/kernel.h>
> >>  #include <linux/delay.h>
> >>  #include <linux/init.h>
> >>@@ -4941,7 +4942,7 @@ int pci_get_new_domain_nr(void)
> >>  }
> >>
> >>  #ifdef CONFIG_PCI_DOMAINS_GENERIC
> >>-void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> >>+static int of_pci_bus_domain_nr(struct device *parent)
> >
> >Can we do a little cleanup before this patch?
> >
> >   - pci_bus_assign_domain_nr() is only used inside drivers/pci, so
> >     maybe we move the prototype to drivers/pci/pci.h?
> 
> pci_bus_assign_domain_nr() goes with pci_domain_nr() as an option
> for CONFIG_PCI_DOMAINS_GENERIC. pci_domain_nr() is used commonly
> outside drivers/pci so we would need to split these calls then, thus
> personally I think it would be better to keep both in
> inclue/linux/pci.h

OK.

> >   - I don't really like the style of calling a function that
> >     internally assigns bus->domain_nr.  Could we do something like
> >     this instead?
> >
> >     int pci_bus_domain_nr(...)
> >     {
> >       ...
> >       return domain;
> >     }
> >
> >     ... pci_create_root_bus(...)
> >     {
> >       ...
> >       b->domain_nr = pci_bus_domain_nr(...);
> 
> 
> We can. I do not see much difference between pci_bus_domain_nr() and
> pci_domain_nr() which we already have so how about calling it
> pci_bus_find_domain_nr instead? Lorenzo, any strong preference for
> it?

I suggested that to make them all parallel:

  pci_bus_domain_nr()
  of_pci_bus_domain_nr()
  acpi_pci_bus_domain_nr()

If you want to make them all *pci_bus_find_domain_nr() that would be
OK with me, but I think it is worth keeping them the same except for
the prefix (none/"of_"/"acpi_").  I think that's what you suggested
below.

> >That would be two new patches, if this makes sense.
> >
> >And this patch would only rename pci_bus_assign_domain_nr() to
> >of_pci_bus_domain_nr() and add the pci_bus_domain_nr() wrapper.
> 
> Giving that we would keep prototypes in inclue/linux/pci.h
> 
> 1. First patch would rename pci_bus_assign_domain_nr() to
> pci_bus_find_domain_nr() and it would return domain number so that
> we could do:
>      ... pci_create_root_bus(...)
>      {
>        ...
>        b->domain_nr = pci_bus_domain_nr(...);
>        ...
>      }
> 
> 2. Second patch would transform pci_bus_find_domain_nr() to be the
> wrapper for of_pci_bus_domain_nr()
> 
> 3. Third patch would add stub definition, the ARM64 definition and
> the new call acpi_pci_bus_domain_nr() in pci_bus_find_domain_nr()
> wrapper.
> 
> Does this plan sound reasonable?

Sounds perfect!
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Bjorn, Tomasz,

On Tue, Jun 07, 2016 at 07:15:59PM -0500, Bjorn Helgaas wrote:

[...]

> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index eb431b5..2b52178 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -7,6 +7,7 @@
> >   *	Copyright 1997 -- 2000 Martin Mares <mj@ucw.cz>
> >   */
> >  
> > +#include <linux/acpi.h>
> >  #include <linux/kernel.h>
> >  #include <linux/delay.h>
> >  #include <linux/init.h>
> > @@ -4941,7 +4942,7 @@ int pci_get_new_domain_nr(void)
> >  }
> >  
> >  #ifdef CONFIG_PCI_DOMAINS_GENERIC
> > -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > +static int of_pci_bus_domain_nr(struct device *parent)
> 
> Can we do a little cleanup before this patch?  
> 
>   - pci_bus_assign_domain_nr() is only used inside drivers/pci, so
>     maybe we move the prototype to drivers/pci/pci.h?
> 
>   - I don't really like the style of calling a function that
>     internally assigns bus->domain_nr.  Could we do something like
>     this instead?
> 
>     int pci_bus_domain_nr(...)
>     {
>       ...
>       return domain;
>     }
> 
>     ... pci_create_root_bus(...)
>     {
>       ...
>       b->domain_nr = pci_bus_domain_nr(...);

We noticed while preparing v9, that this would force us to
write an empty pci_bus_domain_nr() prototype for
!PCI_DOMAINS_GENERIC (ie every arch but ARM/ARM64) that should
return 0 to keep current behaviour unchanged.

That's why pci_bus_assign_domain_nr() was there, so that it
was compiled out on !PCI_DOMAINS_GENERIC.

I really would like v9 to be final so let's fix it before posting it
shortly please.

For the above we have three options:

1) Leave code as-is in v8

2) in pci_create_root_bus():
#ifdef CONFIG_PCI_DOMAINS_GENERIC
       b->domain_nr = pci_bus_domain_nr(...);
#endif

+ other changes requested above

3) in pci_create_root_bus()

b->domain_nr = pci_bus_domain_nr(...);

unguarded and a stub:

#ifndef CONFIG_PCI_DOMAINS_GENERIC
static inline int pci_bus_domain_nr() { return 0; }
#endif

+ other changes requested above

I think it is a matter of details and frankly I am not sure (2) or (3)
make things much cleaner than they are (given that as you know with
Arnd's changes we will rewrite this code anyway), so please let's
choose an option so that we can post v9 and hopefully this series is
ready to go.

Thanks !
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Jun 10, 2016 at 04:14:58PM +0100, Lorenzo Pieralisi wrote:
> Hi Bjorn, Tomasz,
> 
> On Tue, Jun 07, 2016 at 07:15:59PM -0500, Bjorn Helgaas wrote:
> 
> [...]
> 
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index eb431b5..2b52178 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -7,6 +7,7 @@
> > >   *	Copyright 1997 -- 2000 Martin Mares <mj@ucw.cz>
> > >   */
> > >  
> > > +#include <linux/acpi.h>
> > >  #include <linux/kernel.h>
> > >  #include <linux/delay.h>
> > >  #include <linux/init.h>
> > > @@ -4941,7 +4942,7 @@ int pci_get_new_domain_nr(void)
> > >  }
> > >  
> > >  #ifdef CONFIG_PCI_DOMAINS_GENERIC
> > > -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> > > +static int of_pci_bus_domain_nr(struct device *parent)
> > 
> > Can we do a little cleanup before this patch?  
> > 
> >   - pci_bus_assign_domain_nr() is only used inside drivers/pci, so
> >     maybe we move the prototype to drivers/pci/pci.h?
> > 
> >   - I don't really like the style of calling a function that
> >     internally assigns bus->domain_nr.  Could we do something like
> >     this instead?
> > 
> >     int pci_bus_domain_nr(...)
> >     {
> >       ...
> >       return domain;
> >     }
> > 
> >     ... pci_create_root_bus(...)
> >     {
> >       ...
> >       b->domain_nr = pci_bus_domain_nr(...);
> 
> We noticed while preparing v9, that this would force us to
> write an empty pci_bus_domain_nr() prototype for
> !PCI_DOMAINS_GENERIC (ie every arch but ARM/ARM64) that should
> return 0 to keep current behaviour unchanged.
> 
> That's why pci_bus_assign_domain_nr() was there, so that it
> was compiled out on !PCI_DOMAINS_GENERIC.
> 
> I really would like v9 to be final so let's fix it before posting it
> shortly please.
> 
> For the above we have three options:
> 
> 1) Leave code as-is in v8
> 
> 2) in pci_create_root_bus():
> #ifdef CONFIG_PCI_DOMAINS_GENERIC
>        b->domain_nr = pci_bus_domain_nr(...);
> #endif
> 
> + other changes requested above
> 
> 3) in pci_create_root_bus()
> 
> b->domain_nr = pci_bus_domain_nr(...);
> 
> unguarded and a stub:
> 
> #ifndef CONFIG_PCI_DOMAINS_GENERIC
> static inline int pci_bus_domain_nr() { return 0; }
> #endif
> 
> + other changes requested above

Actually, Tomasz made me notice that pci_bus.domain_nr is
only declared for CONFIG_PCI_DOMAINS_GENERIC so (3) is not
even an option and IMO (2) is not much nicer than code in
v8 as-is with an ifdef in the middle of pci_create_root_bus().

Thanks,
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 10.06.2016 17:49, Lorenzo Pieralisi wrote:
> On Fri, Jun 10, 2016 at 04:14:58PM +0100, Lorenzo Pieralisi wrote:
>> Hi Bjorn, Tomasz,
>>
>> On Tue, Jun 07, 2016 at 07:15:59PM -0500, Bjorn Helgaas wrote:
>>
>> [...]
>>
>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>> index eb431b5..2b52178 100644
>>>> --- a/drivers/pci/pci.c
>>>> +++ b/drivers/pci/pci.c
>>>> @@ -7,6 +7,7 @@
>>>>    *	Copyright 1997 -- 2000 Martin Mares <mj@ucw.cz>
>>>>    */
>>>>
>>>> +#include <linux/acpi.h>
>>>>   #include <linux/kernel.h>
>>>>   #include <linux/delay.h>
>>>>   #include <linux/init.h>
>>>> @@ -4941,7 +4942,7 @@ int pci_get_new_domain_nr(void)
>>>>   }
>>>>
>>>>   #ifdef CONFIG_PCI_DOMAINS_GENERIC
>>>> -void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
>>>> +static int of_pci_bus_domain_nr(struct device *parent)
>>>
>>> Can we do a little cleanup before this patch?
>>>
>>>    - pci_bus_assign_domain_nr() is only used inside drivers/pci, so
>>>      maybe we move the prototype to drivers/pci/pci.h?
>>>
>>>    - I don't really like the style of calling a function that
>>>      internally assigns bus->domain_nr.  Could we do something like
>>>      this instead?
>>>
>>>      int pci_bus_domain_nr(...)
>>>      {
>>>        ...
>>>        return domain;
>>>      }
>>>
>>>      ... pci_create_root_bus(...)
>>>      {
>>>        ...
>>>        b->domain_nr = pci_bus_domain_nr(...);
>>
>> We noticed while preparing v9, that this would force us to
>> write an empty pci_bus_domain_nr() prototype for
>> !PCI_DOMAINS_GENERIC (ie every arch but ARM/ARM64) that should
>> return 0 to keep current behaviour unchanged.
>>
>> That's why pci_bus_assign_domain_nr() was there, so that it
>> was compiled out on !PCI_DOMAINS_GENERIC.
>>
>> I really would like v9 to be final so let's fix it before posting it
>> shortly please.
>>
>> For the above we have three options:
>>
>> 1) Leave code as-is in v8
>>
>> 2) in pci_create_root_bus():
>> #ifdef CONFIG_PCI_DOMAINS_GENERIC
>>         b->domain_nr = pci_bus_domain_nr(...);
>> #endif
>>
>> + other changes requested above
>>
>> 3) in pci_create_root_bus()
>>
>> b->domain_nr = pci_bus_domain_nr(...);
>>
>> unguarded and a stub:
>>
>> #ifndef CONFIG_PCI_DOMAINS_GENERIC
>> static inline int pci_bus_domain_nr() { return 0; }
>> #endif
>>
>> + other changes requested above
>
> Actually, Tomasz made me notice that pci_bus.domain_nr is
> only declared for CONFIG_PCI_DOMAINS_GENERIC so (3) is not
> even an option and IMO (2) is not much nicer than code in
> v8 as-is with an ifdef in the middle of pci_create_root_bus().
>

To me (1) is nicer too. Bjorn what is your take on this? This is last 
bit before sending v9.

Thanks,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Fri, Jun 10, 2016 at 06:49:32PM +0200, Tomasz Nowicki wrote:
> On 10.06.2016 17:49, Lorenzo Pieralisi wrote:
> >On Fri, Jun 10, 2016 at 04:14:58PM +0100, Lorenzo Pieralisi wrote:
> >>Hi Bjorn, Tomasz,
> >>
> >>On Tue, Jun 07, 2016 at 07:15:59PM -0500, Bjorn Helgaas wrote:
> >>
> >>[...]
> >>
> >>>>diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> >>>>index eb431b5..2b52178 100644
> >>>>--- a/drivers/pci/pci.c
> >>>>+++ b/drivers/pci/pci.c
> >>>>@@ -7,6 +7,7 @@
> >>>>   *	Copyright 1997 -- 2000 Martin Mares <mj@ucw.cz>
> >>>>   */
> >>>>
> >>>>+#include <linux/acpi.h>
> >>>>  #include <linux/kernel.h>
> >>>>  #include <linux/delay.h>
> >>>>  #include <linux/init.h>
> >>>>@@ -4941,7 +4942,7 @@ int pci_get_new_domain_nr(void)
> >>>>  }
> >>>>
> >>>>  #ifdef CONFIG_PCI_DOMAINS_GENERIC
> >>>>-void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent)
> >>>>+static int of_pci_bus_domain_nr(struct device *parent)
> >>>
> >>>Can we do a little cleanup before this patch?
> >>>
> >>>   - pci_bus_assign_domain_nr() is only used inside drivers/pci, so
> >>>     maybe we move the prototype to drivers/pci/pci.h?
> >>>
> >>>   - I don't really like the style of calling a function that
> >>>     internally assigns bus->domain_nr.  Could we do something like
> >>>     this instead?
> >>>
> >>>     int pci_bus_domain_nr(...)
> >>>     {
> >>>       ...
> >>>       return domain;
> >>>     }
> >>>
> >>>     ... pci_create_root_bus(...)
> >>>     {
> >>>       ...
> >>>       b->domain_nr = pci_bus_domain_nr(...);
> >>
> >>We noticed while preparing v9, that this would force us to
> >>write an empty pci_bus_domain_nr() prototype for
> >>!PCI_DOMAINS_GENERIC (ie every arch but ARM/ARM64) that should
> >>return 0 to keep current behaviour unchanged.
> >>
> >>That's why pci_bus_assign_domain_nr() was there, so that it
> >>was compiled out on !PCI_DOMAINS_GENERIC.
> >>
> >>I really would like v9 to be final so let's fix it before posting it
> >>shortly please.
> >>
> >>For the above we have three options:
> >>
> >>1) Leave code as-is in v8
> >>
> >>2) in pci_create_root_bus():
> >>#ifdef CONFIG_PCI_DOMAINS_GENERIC
> >>        b->domain_nr = pci_bus_domain_nr(...);
> >>#endif
> >>
> >>+ other changes requested above
> >>
> >>3) in pci_create_root_bus()
> >>
> >>b->domain_nr = pci_bus_domain_nr(...);
> >>
> >>unguarded and a stub:
> >>
> >>#ifndef CONFIG_PCI_DOMAINS_GENERIC
> >>static inline int pci_bus_domain_nr() { return 0; }
> >>#endif
> >>
> >>+ other changes requested above
> >
> >Actually, Tomasz made me notice that pci_bus.domain_nr is
> >only declared for CONFIG_PCI_DOMAINS_GENERIC so (3) is not
> >even an option and IMO (2) is not much nicer than code in
> >v8 as-is with an ifdef in the middle of pci_create_root_bus().
> >
> 
> To me (1) is nicer too. Bjorn what is your take on this? This is
> last bit before sending v9.

My preference is (2).  The ifdef in pci_create_root_bus() is a little
ugly, but I like it better because it will fit nicely into Arnd's
idea of having the native drivers allocate and fill in a host bridge
structure before calling the PCI core.  The domain is one thing those
drivers could fill in.  I like that model much better than having the
PCI core make callbacks to get information that we should have passed
in to begin with.

The current code suggests that assigning the domain is the PCI core's
responsibility, and that's not really the case -- for ACPI it's
totally up to pci_root.c, for other drivers it comes from the DT, and
for others it might depend on the driver's knowledge of the hardware
(I'm thinking of parisc, where, I think we currently put all the
bridges in the same domain, but IIRC they *could* each be in their own
domain with a full [bus 00-ff] range for each bridge because each
bridge has its own config space access mechanism).

But it's not that big a deal either way -- we could do this bit of
restructuring later, too.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Bjorn,

Sorry for top posting while on the road. If this refactoring can happen later, is it possible to merge now (well, -next anyway) and explore other work as next steps?

Jon.