| Message ID | 1464621262-26770-6-git-send-email-tn@semihalf.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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.
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) { 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; } 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,