| Message ID | 1421247905-3749-7-git-send-email-hanjun.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/14/2015 09:04 AM, Hanjun Guo wrote: > Since PCI is not required in ACPI spec and ARM can run without > it, introduce some stub functions to make PCI optional for ACPI, > and make ACPI core run without CONFIG_PCI on ARM64. > > When PCI is enabled on ARM64, ACPI core will need some PCI functions > to make it functional, so introduce some empty functions here and > implement it later. > > Since ACPI on X86 and IA64 depends on PCI and this patch only makes > PCI optional for ARM64, it will not break anything on X86 and IA64. > > Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > Tested-by: Yijing Wang <wangyijing@huawei.com> > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > --- Tested-by: Mark Langsdorf <mlangsdo@redhat.com>
On Wed, Jan 14, 2015 at 03:04:54PM +0000, Hanjun Guo wrote: > Since PCI is not required in ACPI spec and ARM can run without > it, introduce some stub functions to make PCI optional for ACPI, > and make ACPI core run without CONFIG_PCI on ARM64. > > When PCI is enabled on ARM64, ACPI core will need some PCI functions > to make it functional, so introduce some empty functions here and > implement it later. > > Since ACPI on X86 and IA64 depends on PCI and this patch only makes > PCI optional for ARM64, it will not break anything on X86 and IA64. > > Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > Tested-by: Yijing Wang <wangyijing@huawei.com> > Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> Is this patch still required, now that we have PCI for arm64? I know the ACPI spec doesn't require PCI but do we expect any arm64 servers aimed at ACPI without PCIe? Anyway, that's not the main point, see more below. > diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h > index 872ba93..fded096 100644 > --- a/arch/arm64/include/asm/pci.h > +++ b/arch/arm64/include/asm/pci.h > @@ -24,6 +24,12 @@ > */ > #define PCI_DMA_BUS_IS_PHYS (0) > > +static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel) > +{ > + /* no legacy IRQ on arm64 */ > + return -ENODEV; > +} > + > extern int isa_dma_bridge_buggy; > > #ifdef CONFIG_PCI > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c > index ce5836c..42fb195 100644 > --- a/arch/arm64/kernel/pci.c > +++ b/arch/arm64/kernel/pci.c > @@ -10,6 +10,7 @@ > * > */ > > +#include <linux/acpi.h> > #include <linux/init.h> > #include <linux/io.h> > #include <linux/kernel.h> > @@ -68,3 +69,30 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) > bus->domain_nr = domain; > } > #endif > + > +/* > + * raw_pci_read/write - Platform-specific PCI config space access. > + * > + * Default empty implementation. Replace with an architecture-specific setup > + * routine, if necessary. > + */ > +int raw_pci_read(unsigned int domain, unsigned int bus, > + unsigned int devfn, int reg, int len, u32 *val) > +{ > + return -EINVAL; > +} > + > +int raw_pci_write(unsigned int domain, unsigned int bus, > + unsigned int devfn, int reg, int len, u32 val) > +{ > + return -EINVAL; > +} > + > +#ifdef CONFIG_ACPI > +/* Root bridge scanning */ > +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) > +{ > + /* TODO: Should be revisited when implementing PCI on ACPI */ > + return NULL; > +} > +#endif Do these functions have anything to do with the subject? You add them in arch/arm64/kernel/pci.c which is compiled only when CONFIG_PCI while the commit log implies that you add them to allow CONFIG_PCI to be off. When PCI is enabled and the above functions are compiled in, do they need to return any useful data or just -EINVAL. Are they ever called? > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile > index 39f3ec1..c346011 100644 > --- a/drivers/acpi/Makefile > +++ b/drivers/acpi/Makefile > @@ -43,7 +43,7 @@ acpi-y += processor_core.o > acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) += processor_pdc.o > acpi-y += ec.o > acpi-$(CONFIG_ACPI_DOCK) += dock.o > -acpi-y += pci_root.o pci_link.o pci_irq.o > +acpi-$(CONFIG_PCI) += pci_root.o pci_link.o pci_irq.o > acpi-y += acpi_lpss.o > acpi-y += acpi_platform.o > acpi-y += acpi_pnp.o > diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h > index 163e82f..c5ff8ba 100644 > --- a/drivers/acpi/internal.h > +++ b/drivers/acpi/internal.h > @@ -26,8 +26,13 @@ > acpi_status acpi_os_initialize1(void); > int init_acpi_device_notify(void); > int acpi_scan_init(void); > +#ifdef CONFIG_PCI > void acpi_pci_root_init(void); > void acpi_pci_link_init(void); > +#else > +static inline void acpi_pci_root_init(void) {} > +static inline void acpi_pci_link_init(void) {} > +#endif > void acpi_processor_init(void); > void acpi_platform_init(void); > void acpi_pnp_init(void); That's a good clean-up. > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 360a966..1476a66 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -564,15 +564,6 @@ struct pci_ops { > int (*write)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 val); > }; > > -/* > - * ACPI needs to be able to access PCI config space before we've done a > - * PCI bus scan and created pci_bus structures. > - */ > -int raw_pci_read(unsigned int domain, unsigned int bus, unsigned int devfn, > - int reg, int len, u32 *val); > -int raw_pci_write(unsigned int domain, unsigned int bus, unsigned int devfn, > - int reg, int len, u32 val); > - > struct pci_bus_region { > dma_addr_t start; > dma_addr_t end; > @@ -1329,6 +1320,16 @@ typedef int (*arch_set_vga_state_t)(struct pci_dev *pdev, bool decode, > unsigned int command_bits, u32 flags); > void pci_register_set_vga_state(arch_set_vga_state_t func); > > +/* > + * ACPI needs to be able to access PCI config space before we've done a > + * PCI bus scan and created pci_bus structures. > + */ > +int raw_pci_read(unsigned int domain, unsigned int bus, unsigned int devfn, > + int reg, int len, u32 *val); > +int raw_pci_write(unsigned int domain, unsigned int bus, unsigned int devfn, > + int reg, int len, u32 val); > +void pcibios_penalize_isa_irq(int irq, int active); > + > #else /* CONFIG_PCI is not enabled */ > > /* > @@ -1430,6 +1431,23 @@ static inline struct pci_dev *pci_get_bus_and_slot(unsigned int bus, > unsigned int devfn) > { return NULL; } > > +static inline struct pci_bus *pci_find_bus(int domain, int busnr) > +{ return NULL; } > + > +static inline int pci_bus_write_config_byte(struct pci_bus *bus, > + unsigned int devfn, int where, u8 val) > +{ return -ENOSYS; } > + > +static inline int raw_pci_read(unsigned int domain, unsigned int bus, > + unsigned int devfn, int reg, int len, u32 *val) > +{ return -ENOSYS; } > + > +static inline int raw_pci_write(unsigned int domain, unsigned int bus, > + unsigned int devfn, int reg, int len, u32 val) > +{ return -ENOSYS; } So you implement the !CONFIG_PCI functions here to return -ENOSYS while the arm64 CONFIG_PCI ones would return -EINVAL. I'm confused.
On 2015?01?16? 17:49, Catalin Marinas wrote: > On Wed, Jan 14, 2015 at 03:04:54PM +0000, Hanjun Guo wrote: >> Since PCI is not required in ACPI spec and ARM can run without >> it, introduce some stub functions to make PCI optional for ACPI, >> and make ACPI core run without CONFIG_PCI on ARM64. >> >> When PCI is enabled on ARM64, ACPI core will need some PCI functions >> to make it functional, so introduce some empty functions here and >> implement it later. >> >> Since ACPI on X86 and IA64 depends on PCI and this patch only makes >> PCI optional for ARM64, it will not break anything on X86 and IA64. >> >> Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> >> Tested-by: Yijing Wang <wangyijing@huawei.com> >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> > > Is this patch still required, now that we have PCI for arm64? I know the > ACPI spec doesn't require PCI but do we expect any arm64 servers aimed > at ACPI without PCIe? I think so, how about make ACPI depends on PCI on ARM64 too? > > Anyway, that's not the main point, see more below. > >> diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h >> index 872ba93..fded096 100644 >> --- a/arch/arm64/include/asm/pci.h >> +++ b/arch/arm64/include/asm/pci.h >> @@ -24,6 +24,12 @@ >> */ >> #define PCI_DMA_BUS_IS_PHYS (0) >> >> +static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel) >> +{ >> + /* no legacy IRQ on arm64 */ >> + return -ENODEV; >> +} >> + >> extern int isa_dma_bridge_buggy; >> >> #ifdef CONFIG_PCI >> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c >> index ce5836c..42fb195 100644 >> --- a/arch/arm64/kernel/pci.c >> +++ b/arch/arm64/kernel/pci.c >> @@ -10,6 +10,7 @@ >> * >> */ >> >> +#include <linux/acpi.h> >> #include <linux/init.h> >> #include <linux/io.h> >> #include <linux/kernel.h> >> @@ -68,3 +69,30 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) >> bus->domain_nr = domain; >> } >> #endif >> + >> +/* >> + * raw_pci_read/write - Platform-specific PCI config space access. >> + * >> + * Default empty implementation. Replace with an architecture-specific setup >> + * routine, if necessary. >> + */ >> +int raw_pci_read(unsigned int domain, unsigned int bus, >> + unsigned int devfn, int reg, int len, u32 *val) >> +{ >> + return -EINVAL; >> +} >> + >> +int raw_pci_write(unsigned int domain, unsigned int bus, >> + unsigned int devfn, int reg, int len, u32 val) >> +{ >> + return -EINVAL; >> +} >> + >> +#ifdef CONFIG_ACPI >> +/* Root bridge scanning */ >> +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) >> +{ >> + /* TODO: Should be revisited when implementing PCI on ACPI */ >> + return NULL; >> +} >> +#endif > > Do these functions have anything to do with the subject? You add them in > arch/arm64/kernel/pci.c which is compiled only when CONFIG_PCI while the > commit log implies that you add them to allow CONFIG_PCI to be off. My bad, I can update the change log to make it explicit it is needed when PCI is enabled. > > When PCI is enabled and the above functions are compiled in, do they > need to return any useful data or just -EINVAL. Are they ever called? They will be called if PCI root bridge is defined in DSDT, should I print some warning message before it is implemented? > >> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile >> index 39f3ec1..c346011 100644 >> --- a/drivers/acpi/Makefile >> +++ b/drivers/acpi/Makefile >> @@ -43,7 +43,7 @@ acpi-y += processor_core.o >> acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) += processor_pdc.o >> acpi-y += ec.o >> acpi-$(CONFIG_ACPI_DOCK) += dock.o >> -acpi-y += pci_root.o pci_link.o pci_irq.o >> +acpi-$(CONFIG_PCI) += pci_root.o pci_link.o pci_irq.o >> acpi-y += acpi_lpss.o >> acpi-y += acpi_platform.o >> acpi-y += acpi_pnp.o >> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h >> index 163e82f..c5ff8ba 100644 >> --- a/drivers/acpi/internal.h >> +++ b/drivers/acpi/internal.h >> @@ -26,8 +26,13 @@ >> acpi_status acpi_os_initialize1(void); >> int init_acpi_device_notify(void); >> int acpi_scan_init(void); >> +#ifdef CONFIG_PCI >> void acpi_pci_root_init(void); >> void acpi_pci_link_init(void); >> +#else >> +static inline void acpi_pci_root_init(void) {} >> +static inline void acpi_pci_link_init(void) {} >> +#endif >> void acpi_processor_init(void); >> void acpi_platform_init(void); >> void acpi_pnp_init(void); > > That's a good clean-up. If we make ACPI depends on PCI on ARM64, these two stub functions are not needed anymore. > >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index 360a966..1476a66 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -564,15 +564,6 @@ struct pci_ops { >> int (*write)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 val); >> }; >> >> -/* >> - * ACPI needs to be able to access PCI config space before we've done a >> - * PCI bus scan and created pci_bus structures. >> - */ >> -int raw_pci_read(unsigned int domain, unsigned int bus, unsigned int devfn, >> - int reg, int len, u32 *val); >> -int raw_pci_write(unsigned int domain, unsigned int bus, unsigned int devfn, >> - int reg, int len, u32 val); >> - >> struct pci_bus_region { >> dma_addr_t start; >> dma_addr_t end; >> @@ -1329,6 +1320,16 @@ typedef int (*arch_set_vga_state_t)(struct pci_dev *pdev, bool decode, >> unsigned int command_bits, u32 flags); >> void pci_register_set_vga_state(arch_set_vga_state_t func); >> >> +/* >> + * ACPI needs to be able to access PCI config space before we've done a >> + * PCI bus scan and created pci_bus structures. >> + */ >> +int raw_pci_read(unsigned int domain, unsigned int bus, unsigned int devfn, >> + int reg, int len, u32 *val); >> +int raw_pci_write(unsigned int domain, unsigned int bus, unsigned int devfn, >> + int reg, int len, u32 val); >> +void pcibios_penalize_isa_irq(int irq, int active); >> + >> #else /* CONFIG_PCI is not enabled */ >> >> /* >> @@ -1430,6 +1431,23 @@ static inline struct pci_dev *pci_get_bus_and_slot(unsigned int bus, >> unsigned int devfn) >> { return NULL; } >> >> +static inline struct pci_bus *pci_find_bus(int domain, int busnr) >> +{ return NULL; } >> + >> +static inline int pci_bus_write_config_byte(struct pci_bus *bus, >> + unsigned int devfn, int where, u8 val) >> +{ return -ENOSYS; } >> + >> +static inline int raw_pci_read(unsigned int domain, unsigned int bus, >> + unsigned int devfn, int reg, int len, u32 *val) >> +{ return -ENOSYS; } >> + >> +static inline int raw_pci_write(unsigned int domain, unsigned int bus, >> + unsigned int devfn, int reg, int len, u32 val) >> +{ return -ENOSYS; } > > So you implement the !CONFIG_PCI functions here to return -ENOSYS while > the arm64 CONFIG_PCI ones would return -EINVAL. I'm confused. return -ENOSYS in the arm64 CONFIG_PCI ones next version :) Thanks Hanjun
Hi Folks, Sorry for top posting from bed. The mainstream servers will all likely do PCIe but there are several that may not. They should not be excluded. That said, if we booted a previously built kernel on a system without an MCFG and got no ECAM/root then things would probably still work. I think it'll work out either way but for the record there is no requirement to do PCIe on ARM servers that conform to spec. Jon.
On 2015?01?18? 14:31, Jon Masters wrote: > Hi Folks, > > Sorry for top posting from bed. The mainstream servers will all likely do > PCIe but there are several that may not. They should not be excluded. That said, >if we booted a previously built kernel on a system without an MCFG and > got no ECAM/root then things would probably still work. > > I think it'll work out either way but for the record there is no requirement to do PCIe on ARM servers that conform to spec. OK, Catalin already said that was not the main point of the comments for this patch, I think the title and change log of the patch is inconsistent with the code makes Catalin confused, I will update them in next version. Thanks Hanjun
On Sun, Jan 18, 2015 at 02:46:35PM +0800, Hanjun Guo wrote: > On 2015?01?18? 14:31, Jon Masters wrote: > >Hi Folks, > > > >Sorry for top posting from bed. The mainstream servers will all likely do > > PCIe but there are several that may not. They should not be excluded. That > said, > >if we booted a previously built kernel on a system without an MCFG and > > got no ECAM/root then things would probably still work. > > > >I think it'll work out either way but for the record there is no requirement to do PCIe on ARM servers that conform to spec. > > OK, Catalin already said that was not the main point of the > comments for this patch, I think the title and change log > of the patch is inconsistent with the code makes Catalin confused, > I will update them in next version. > Well what we are talking about is the presence of CONFIG_PCI=y which even in Jons case will be true as he wants to run the same kernel on both sets of hardware. Now the architecture has PCI support I think its safe to remove the make PCI optional part of the patch as this should be handled runtime not compile time. Graeme
On 01/18/2015 04:29 AM, Graeme Gregory wrote: > On Sun, Jan 18, 2015 at 02:46:35PM +0800, Hanjun Guo wrote: >> On 2015?01?18? 14:31, Jon Masters wrote: >>> Hi Folks, >>> >>> Sorry for top posting from bed. The mainstream servers will all likely do >>> PCIe but there are several that may not. They should not be excluded. That >> said, >>> if we booted a previously built kernel on a system without an MCFG and >>> got no ECAM/root then things would probably still work. >>> >>> I think it'll work out either way but for the record there is no requirement to do PCIe on ARM servers that conform to spec. >> >> OK, Catalin already said that was not the main point of the >> comments for this patch, I think the title and change log >> of the patch is inconsistent with the code makes Catalin confused, >> I will update them in next version. >> > Well what we are talking about is the presence of CONFIG_PCI=y which even > in Jons case will be true as he wants to run the same kernel on both > sets of hardware. Yup. And btw, the ACPI+PCI use case works beautifully already today. I will followup to my other Tested-by with a bit more detail later, but these patches have successfully been used on a wide range of PCIe based hardware already (I personally have tried a number of 10G network cards, SATA, USB, and even a graphics card or two for giggles). Jon.
On 2015?01?18? 17:29, Graeme Gregory wrote: > On Sun, Jan 18, 2015 at 02:46:35PM +0800, Hanjun Guo wrote: >> On 2015?01?18? 14:31, Jon Masters wrote: >>> Hi Folks, >>> >>> Sorry for top posting from bed. The mainstream servers will all likely do >>> PCIe but there are several that may not. They should not be excluded. That >> said, >>> if we booted a previously built kernel on a system without an MCFG and >>> got no ECAM/root then things would probably still work. >>> >>> I think it'll work out either way but for the record there is no requirement to do PCIe on ARM servers that conform to spec. >> >> OK, Catalin already said that was not the main point of the >> comments for this patch, I think the title and change log >> of the patch is inconsistent with the code makes Catalin confused, >> I will update them in next version. >> > Well what we are talking about is the presence of CONFIG_PCI=y which even > in Jons case will be true as he wants to run the same kernel on both > sets of hardware. > > Now the architecture has PCI support I think its safe to remove the make > PCI optional part of the patch as this should be handled runtime not > compile time. I missed that part, must be something wrong work in Sunday :) I will update the patch with ACPI depends on PCI, which makes thing much simpler. Thanks Hanjun
On Sun, Jan 18, 2015 at 09:29:56AM +0000, Graeme Gregory wrote: > On Sun, Jan 18, 2015 at 02:46:35PM +0800, Hanjun Guo wrote: > > On 2015?01?18? 14:31, Jon Masters wrote: > > >Hi Folks, > > > > > >Sorry for top posting from bed. The mainstream servers will all likely do > > > PCIe but there are several that may not. They should not be excluded. That > > said, > > >if we booted a previously built kernel on a system without an MCFG and > > > got no ECAM/root then things would probably still work. > > > > > >I think it'll work out either way but for the record there is no requirement to do PCIe on ARM servers that conform to spec. > > > > OK, Catalin already said that was not the main point of the > > comments for this patch, I think the title and change log > > of the patch is inconsistent with the code makes Catalin confused, > > I will update them in next version. > > Well what we are talking about is the presence of CONFIG_PCI=y which even > in Jons case will be true as he wants to run the same kernel on both > sets of hardware. > > Now the architecture has PCI support I think its safe to remove the make > PCI optional part of the patch as this should be handled runtime not > compile time. I agree, if we never see a reason to build a kernel image with !PCI && ACPI, we can simplify this patch.
On Sun, Jan 18, 2015 at 06:25:53AM +0000, Hanjun Guo wrote: > On 2015?01?16? 17:49, Catalin Marinas wrote: > > On Wed, Jan 14, 2015 at 03:04:54PM +0000, Hanjun Guo wrote: > >> --- a/arch/arm64/kernel/pci.c > >> +++ b/arch/arm64/kernel/pci.c > >> @@ -10,6 +10,7 @@ > >> * > >> */ > >> > >> +#include <linux/acpi.h> > >> #include <linux/init.h> > >> #include <linux/io.h> > >> #include <linux/kernel.h> > >> @@ -68,3 +69,30 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) > >> bus->domain_nr = domain; > >> } > >> #endif > >> + > >> +/* > >> + * raw_pci_read/write - Platform-specific PCI config space access. > >> + * > >> + * Default empty implementation. Replace with an architecture-specific setup > >> + * routine, if necessary. > >> + */ > >> +int raw_pci_read(unsigned int domain, unsigned int bus, > >> + unsigned int devfn, int reg, int len, u32 *val) > >> +{ > >> + return -EINVAL; > >> +} > >> + > >> +int raw_pci_write(unsigned int domain, unsigned int bus, > >> + unsigned int devfn, int reg, int len, u32 val) > >> +{ > >> + return -EINVAL; > >> +} > >> + > >> +#ifdef CONFIG_ACPI > >> +/* Root bridge scanning */ > >> +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) > >> +{ > >> + /* TODO: Should be revisited when implementing PCI on ACPI */ > >> + return NULL; > >> +} > >> +#endif [...] > > When PCI is enabled and the above functions are compiled in, do they > > need to return any useful data or just -EINVAL. Are they ever called? > > They will be called if PCI root bridge is defined in DSDT, should I > print some warning message before it is implemented? My point: do they need to return real data when a PCI root bridge is defined in DSDT or you always expect them to always return some -E*? Can you explain why?
On 2015?01?19? 18:42, Catalin Marinas wrote: > On Sun, Jan 18, 2015 at 06:25:53AM +0000, Hanjun Guo wrote: >> On 2015?01?16? 17:49, Catalin Marinas wrote: >>> On Wed, Jan 14, 2015 at 03:04:54PM +0000, Hanjun Guo wrote: >>>> --- a/arch/arm64/kernel/pci.c >>>> +++ b/arch/arm64/kernel/pci.c >>>> @@ -10,6 +10,7 @@ >>>> * >>>> */ >>>> >>>> +#include <linux/acpi.h> >>>> #include <linux/init.h> >>>> #include <linux/io.h> >>>> #include <linux/kernel.h> >>>> @@ -68,3 +69,30 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) >>>> bus->domain_nr = domain; >>>> } >>>> #endif >>>> + >>>> +/* >>>> + * raw_pci_read/write - Platform-specific PCI config space access. >>>> + * >>>> + * Default empty implementation. Replace with an architecture-specific setup >>>> + * routine, if necessary. >>>> + */ >>>> +int raw_pci_read(unsigned int domain, unsigned int bus, >>>> + unsigned int devfn, int reg, int len, u32 *val) >>>> +{ >>>> + return -EINVAL; >>>> +} >>>> + >>>> +int raw_pci_write(unsigned int domain, unsigned int bus, >>>> + unsigned int devfn, int reg, int len, u32 val) >>>> +{ >>>> + return -EINVAL; >>>> +} >>>> + >>>> +#ifdef CONFIG_ACPI >>>> +/* Root bridge scanning */ >>>> +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) >>>> +{ >>>> + /* TODO: Should be revisited when implementing PCI on ACPI */ >>>> + return NULL; >>>> +} >>>> +#endif > [...] >>> When PCI is enabled and the above functions are compiled in, do they >>> need to return any useful data or just -EINVAL. Are they ever called? >> >> They will be called if PCI root bridge is defined in DSDT, should I >> print some warning message before it is implemented? > > My point: do they need to return real data when a PCI root bridge is > defined in DSDT or you always expect them to always return some -E*? Can > you explain why? Not always return -E* or NULL; For raw_pci_read/write(), they are needed to access the PCI config space before the PCI root bus is created. so they will return 0 if access to PCI config space is ok; pci_acpi_scan_root() will return root bus pointer if it is successfully created. Thanks Hanjun
On Tue, Jan 20, 2015 at 02:39:16AM +0000, Hanjun Guo wrote: > On 2015?01?19? 18:42, Catalin Marinas wrote: > > On Sun, Jan 18, 2015 at 06:25:53AM +0000, Hanjun Guo wrote: > >> On 2015?01?16? 17:49, Catalin Marinas wrote: > >>> On Wed, Jan 14, 2015 at 03:04:54PM +0000, Hanjun Guo wrote: > >>>> --- a/arch/arm64/kernel/pci.c > >>>> +++ b/arch/arm64/kernel/pci.c > >>>> @@ -10,6 +10,7 @@ > >>>> * > >>>> */ > >>>> > >>>> +#include <linux/acpi.h> > >>>> #include <linux/init.h> > >>>> #include <linux/io.h> > >>>> #include <linux/kernel.h> > >>>> @@ -68,3 +69,30 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) > >>>> bus->domain_nr = domain; > >>>> } > >>>> #endif > >>>> + > >>>> +/* > >>>> + * raw_pci_read/write - Platform-specific PCI config space access. > >>>> + * > >>>> + * Default empty implementation. Replace with an architecture-specific setup > >>>> + * routine, if necessary. > >>>> + */ > >>>> +int raw_pci_read(unsigned int domain, unsigned int bus, > >>>> + unsigned int devfn, int reg, int len, u32 *val) > >>>> +{ > >>>> + return -EINVAL; > >>>> +} > >>>> + > >>>> +int raw_pci_write(unsigned int domain, unsigned int bus, > >>>> + unsigned int devfn, int reg, int len, u32 val) > >>>> +{ > >>>> + return -EINVAL; > >>>> +} > >>>> + > >>>> +#ifdef CONFIG_ACPI > >>>> +/* Root bridge scanning */ > >>>> +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) > >>>> +{ > >>>> + /* TODO: Should be revisited when implementing PCI on ACPI */ > >>>> + return NULL; > >>>> +} > >>>> +#endif > > [...] > >>> When PCI is enabled and the above functions are compiled in, do they > >>> need to return any useful data or just -EINVAL. Are they ever called? > >> > >> They will be called if PCI root bridge is defined in DSDT, should I > >> print some warning message before it is implemented? > > > > My point: do they need to return real data when a PCI root bridge is > > defined in DSDT or you always expect them to always return some -E*? Can > > you explain why? > > Not always return -E* or NULL; > > For raw_pci_read/write(), they are needed to access the PCI config space > before the PCI root bus is created. so they will return 0 if access to > PCI config space is ok; pci_acpi_scan_root() will return root bus > pointer if it is successfully created. OK. So what's the plan for implementing these functions properly. For the raw_pci_read/write, the comment states "replace with an architecture-specific setup routine". What does this mean? For pci_acpi_scan_root(), at least the comment states a "TODO". Is there anyone working on this or we don't expect servers with PCIe soon?
On 2015?01?20? 19:00, Catalin Marinas wrote: > On Tue, Jan 20, 2015 at 02:39:16AM +0000, Hanjun Guo wrote: >> On 2015?01?19? 18:42, Catalin Marinas wrote: >>> On Sun, Jan 18, 2015 at 06:25:53AM +0000, Hanjun Guo wrote: >>>> On 2015?01?16? 17:49, Catalin Marinas wrote: >>>>> On Wed, Jan 14, 2015 at 03:04:54PM +0000, Hanjun Guo wrote: >>>>>> --- a/arch/arm64/kernel/pci.c >>>>>> +++ b/arch/arm64/kernel/pci.c >>>>>> @@ -10,6 +10,7 @@ >>>>>> * >>>>>> */ >>>>>> >>>>>> +#include <linux/acpi.h> >>>>>> #include <linux/init.h> >>>>>> #include <linux/io.h> >>>>>> #include <linux/kernel.h> >>>>>> @@ -68,3 +69,30 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) >>>>>> bus->domain_nr = domain; >>>>>> } >>>>>> #endif >>>>>> + >>>>>> +/* >>>>>> + * raw_pci_read/write - Platform-specific PCI config space access. >>>>>> + * >>>>>> + * Default empty implementation. Replace with an architecture-specific setup >>>>>> + * routine, if necessary. >>>>>> + */ >>>>>> +int raw_pci_read(unsigned int domain, unsigned int bus, >>>>>> + unsigned int devfn, int reg, int len, u32 *val) >>>>>> +{ >>>>>> + return -EINVAL; >>>>>> +} >>>>>> + >>>>>> +int raw_pci_write(unsigned int domain, unsigned int bus, >>>>>> + unsigned int devfn, int reg, int len, u32 val) >>>>>> +{ >>>>>> + return -EINVAL; >>>>>> +} >>>>>> + >>>>>> +#ifdef CONFIG_ACPI >>>>>> +/* Root bridge scanning */ >>>>>> +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) >>>>>> +{ >>>>>> + /* TODO: Should be revisited when implementing PCI on ACPI */ >>>>>> + return NULL; >>>>>> +} >>>>>> +#endif >>> [...] >>>>> When PCI is enabled and the above functions are compiled in, do they >>>>> need to return any useful data or just -EINVAL. Are they ever called? >>>> >>>> They will be called if PCI root bridge is defined in DSDT, should I >>>> print some warning message before it is implemented? >>> >>> My point: do they need to return real data when a PCI root bridge is >>> defined in DSDT or you always expect them to always return some -E*? Can >>> you explain why? >> >> Not always return -E* or NULL; >> >> For raw_pci_read/write(), they are needed to access the PCI config space >> before the PCI root bus is created. so they will return 0 if access to >> PCI config space is ok; pci_acpi_scan_root() will return root bus >> pointer if it is successfully created. > > OK. So what's the plan for implementing these functions properly. For We were planing to add the ACPI PCI support in later patch set, and actually Tomasz already prepared the RFC patch for this [1]. > the raw_pci_read/write, the comment states "replace with an > architecture-specific setup routine". What does this mean? Sorry, this comment should be removed for misleadings. > > For pci_acpi_scan_root(), at least the comment states a "TODO". Is there > anyone working on this or we don't expect servers with PCIe soon? Yes, we already have RFC patches [2], but need some clean up and update, we plan to post PCI patches again when ACPI core patches are merged. [1]: https://lkml.org/lkml/2014/11/19/437 [2]: http://www.spinics.net/lists/linux-acpi/msg54053.html Thanks Hanjun
On 20.01.2015 12:00, Catalin Marinas wrote: > On Tue, Jan 20, 2015 at 02:39:16AM +0000, Hanjun Guo wrote: >> On 2015?01?19? 18:42, Catalin Marinas wrote: >>> On Sun, Jan 18, 2015 at 06:25:53AM +0000, Hanjun Guo wrote: >>>> On 2015?01?16? 17:49, Catalin Marinas wrote: >>>>> On Wed, Jan 14, 2015 at 03:04:54PM +0000, Hanjun Guo wrote: >>>>>> --- a/arch/arm64/kernel/pci.c >>>>>> +++ b/arch/arm64/kernel/pci.c >>>>>> @@ -10,6 +10,7 @@ >>>>>> * >>>>>> */ >>>>>> >>>>>> +#include <linux/acpi.h> >>>>>> #include <linux/init.h> >>>>>> #include <linux/io.h> >>>>>> #include <linux/kernel.h> >>>>>> @@ -68,3 +69,30 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) >>>>>> bus->domain_nr = domain; >>>>>> } >>>>>> #endif >>>>>> + >>>>>> +/* >>>>>> + * raw_pci_read/write - Platform-specific PCI config space access. >>>>>> + * >>>>>> + * Default empty implementation. Replace with an architecture-specific setup >>>>>> + * routine, if necessary. >>>>>> + */ >>>>>> +int raw_pci_read(unsigned int domain, unsigned int bus, >>>>>> + unsigned int devfn, int reg, int len, u32 *val) >>>>>> +{ >>>>>> + return -EINVAL; >>>>>> +} >>>>>> + >>>>>> +int raw_pci_write(unsigned int domain, unsigned int bus, >>>>>> + unsigned int devfn, int reg, int len, u32 val) >>>>>> +{ >>>>>> + return -EINVAL; >>>>>> +} >>>>>> + >>>>>> +#ifdef CONFIG_ACPI >>>>>> +/* Root bridge scanning */ >>>>>> +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) >>>>>> +{ >>>>>> + /* TODO: Should be revisited when implementing PCI on ACPI */ >>>>>> + return NULL; >>>>>> +} >>>>>> +#endif >>> [...] >>>>> When PCI is enabled and the above functions are compiled in, do they >>>>> need to return any useful data or just -EINVAL. Are they ever called? >>>> >>>> They will be called if PCI root bridge is defined in DSDT, should I >>>> print some warning message before it is implemented? >>> >>> My point: do they need to return real data when a PCI root bridge is >>> defined in DSDT or you always expect them to always return some -E*? Can >>> you explain why? >> >> Not always return -E* or NULL; >> >> For raw_pci_read/write(), they are needed to access the PCI config space >> before the PCI root bus is created. so they will return 0 if access to >> PCI config space is ok; pci_acpi_scan_root() will return root bus >> pointer if it is successfully created. > > OK. So what's the plan for implementing these functions properly. For > the raw_pci_read/write, the comment states "replace with an > architecture-specific setup routine". What does this mean? raw_pci_read/write will use MMCONFIG code to access PCI config space. Please see my patch set: http://lkml.iu.edu/hypermail/linux/kernel/1411.2/02753.html which is going to refactor the x86 specific code so it would be usable for ARM64 too. > > For pci_acpi_scan_root(), at least the comment states a "TODO". Is there > anyone working on this or we don't expect servers with PCIe soon? > We do, Cavium, AMD and APM boards have PCIe. Me and Mark Salter have posted initial support for ACPI PCI probe: http://lkml.iu.edu/hypermail/linux/kernel/1411.0/05026.html http://lists.linaro.org/pipermail/linaro-acpi/2014-November/002970.html Regards, Tomasz
On Tue, Jan 20, 2015 at 12:26:57PM +0000, Tomasz Nowicki wrote: > On 20.01.2015 12:00, Catalin Marinas wrote: > > On Tue, Jan 20, 2015 at 02:39:16AM +0000, Hanjun Guo wrote: > >> On 2015?01?19? 18:42, Catalin Marinas wrote: > >>> On Sun, Jan 18, 2015 at 06:25:53AM +0000, Hanjun Guo wrote: > >>>> On 2015?01?16? 17:49, Catalin Marinas wrote: > >>>>> On Wed, Jan 14, 2015 at 03:04:54PM +0000, Hanjun Guo wrote: > >>>>>> --- a/arch/arm64/kernel/pci.c > >>>>>> +++ b/arch/arm64/kernel/pci.c > >>>>>> @@ -10,6 +10,7 @@ > >>>>>> * > >>>>>> */ > >>>>>> > >>>>>> +#include <linux/acpi.h> > >>>>>> #include <linux/init.h> > >>>>>> #include <linux/io.h> > >>>>>> #include <linux/kernel.h> > >>>>>> @@ -68,3 +69,30 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) > >>>>>> bus->domain_nr = domain; > >>>>>> } > >>>>>> #endif > >>>>>> + > >>>>>> +/* > >>>>>> + * raw_pci_read/write - Platform-specific PCI config space access. > >>>>>> + * > >>>>>> + * Default empty implementation. Replace with an architecture-specific setup > >>>>>> + * routine, if necessary. > >>>>>> + */ > >>>>>> +int raw_pci_read(unsigned int domain, unsigned int bus, > >>>>>> + unsigned int devfn, int reg, int len, u32 *val) > >>>>>> +{ > >>>>>> + return -EINVAL; > >>>>>> +} > >>>>>> + > >>>>>> +int raw_pci_write(unsigned int domain, unsigned int bus, > >>>>>> + unsigned int devfn, int reg, int len, u32 val) > >>>>>> +{ > >>>>>> + return -EINVAL; > >>>>>> +} > >>>>>> + > >>>>>> +#ifdef CONFIG_ACPI > >>>>>> +/* Root bridge scanning */ > >>>>>> +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) > >>>>>> +{ > >>>>>> + /* TODO: Should be revisited when implementing PCI on ACPI */ > >>>>>> + return NULL; > >>>>>> +} > >>>>>> +#endif > >>> [...] > >>>>> When PCI is enabled and the above functions are compiled in, do they > >>>>> need to return any useful data or just -EINVAL. Are they ever called? > >>>> > >>>> They will be called if PCI root bridge is defined in DSDT, should I > >>>> print some warning message before it is implemented? > >>> > >>> My point: do they need to return real data when a PCI root bridge is > >>> defined in DSDT or you always expect them to always return some -E*? Can > >>> you explain why? > >> > >> Not always return -E* or NULL; > >> > >> For raw_pci_read/write(), they are needed to access the PCI config space > >> before the PCI root bus is created. so they will return 0 if access to > >> PCI config space is ok; pci_acpi_scan_root() will return root bus > >> pointer if it is successfully created. > > > > OK. So what's the plan for implementing these functions properly. For > > the raw_pci_read/write, the comment states "replace with an > > architecture-specific setup routine". What does this mean? > raw_pci_read/write will use MMCONFIG code to access PCI config space. > Please see my patch set: > http://lkml.iu.edu/hypermail/linux/kernel/1411.2/02753.html > which is going to refactor the x86 specific code so it would be usable > for ARM64 too. OK. Thanks for the information.
diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h index 872ba93..fded096 100644 --- a/arch/arm64/include/asm/pci.h +++ b/arch/arm64/include/asm/pci.h @@ -24,6 +24,12 @@ */ #define PCI_DMA_BUS_IS_PHYS (0) +static inline int pci_get_legacy_ide_irq(struct pci_dev *dev, int channel) +{ + /* no legacy IRQ on arm64 */ + return -ENODEV; +} + extern int isa_dma_bridge_buggy; #ifdef CONFIG_PCI diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c index ce5836c..42fb195 100644 --- a/arch/arm64/kernel/pci.c +++ b/arch/arm64/kernel/pci.c @@ -10,6 +10,7 @@ * */ +#include <linux/acpi.h> #include <linux/init.h> #include <linux/io.h> #include <linux/kernel.h> @@ -68,3 +69,30 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) bus->domain_nr = domain; } #endif + +/* + * raw_pci_read/write - Platform-specific PCI config space access. + * + * Default empty implementation. Replace with an architecture-specific setup + * routine, if necessary. + */ +int raw_pci_read(unsigned int domain, unsigned int bus, + unsigned int devfn, int reg, int len, u32 *val) +{ + return -EINVAL; +} + +int raw_pci_write(unsigned int domain, unsigned int bus, + unsigned int devfn, int reg, int len, u32 val) +{ + return -EINVAL; +} + +#ifdef CONFIG_ACPI +/* Root bridge scanning */ +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) +{ + /* TODO: Should be revisited when implementing PCI on ACPI */ + return NULL; +} +#endif diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index 39f3ec1..c346011 100644 --- a/drivers/acpi/Makefile +++ b/drivers/acpi/Makefile @@ -43,7 +43,7 @@ acpi-y += processor_core.o acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) += processor_pdc.o acpi-y += ec.o acpi-$(CONFIG_ACPI_DOCK) += dock.o -acpi-y += pci_root.o pci_link.o pci_irq.o +acpi-$(CONFIG_PCI) += pci_root.o pci_link.o pci_irq.o acpi-y += acpi_lpss.o acpi-y += acpi_platform.o acpi-y += acpi_pnp.o diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h index 163e82f..c5ff8ba 100644 --- a/drivers/acpi/internal.h +++ b/drivers/acpi/internal.h @@ -26,8 +26,13 @@ acpi_status acpi_os_initialize1(void); int init_acpi_device_notify(void); int acpi_scan_init(void); +#ifdef CONFIG_PCI void acpi_pci_root_init(void); void acpi_pci_link_init(void); +#else +static inline void acpi_pci_root_init(void) {} +static inline void acpi_pci_link_init(void) {} +#endif void acpi_processor_init(void); void acpi_platform_init(void); void acpi_pnp_init(void); diff --git a/include/linux/pci.h b/include/linux/pci.h index 360a966..1476a66 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -564,15 +564,6 @@ struct pci_ops { int (*write)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 val); }; -/* - * ACPI needs to be able to access PCI config space before we've done a - * PCI bus scan and created pci_bus structures. - */ -int raw_pci_read(unsigned int domain, unsigned int bus, unsigned int devfn, - int reg, int len, u32 *val); -int raw_pci_write(unsigned int domain, unsigned int bus, unsigned int devfn, - int reg, int len, u32 val); - struct pci_bus_region { dma_addr_t start; dma_addr_t end; @@ -1329,6 +1320,16 @@ typedef int (*arch_set_vga_state_t)(struct pci_dev *pdev, bool decode, unsigned int command_bits, u32 flags); void pci_register_set_vga_state(arch_set_vga_state_t func); +/* + * ACPI needs to be able to access PCI config space before we've done a + * PCI bus scan and created pci_bus structures. + */ +int raw_pci_read(unsigned int domain, unsigned int bus, unsigned int devfn, + int reg, int len, u32 *val); +int raw_pci_write(unsigned int domain, unsigned int bus, unsigned int devfn, + int reg, int len, u32 val); +void pcibios_penalize_isa_irq(int irq, int active); + #else /* CONFIG_PCI is not enabled */ /* @@ -1430,6 +1431,23 @@ static inline struct pci_dev *pci_get_bus_and_slot(unsigned int bus, unsigned int devfn) { return NULL; } +static inline struct pci_bus *pci_find_bus(int domain, int busnr) +{ return NULL; } + +static inline int pci_bus_write_config_byte(struct pci_bus *bus, + unsigned int devfn, int where, u8 val) +{ return -ENOSYS; } + +static inline int raw_pci_read(unsigned int domain, unsigned int bus, + unsigned int devfn, int reg, int len, u32 *val) +{ return -ENOSYS; } + +static inline int raw_pci_write(unsigned int domain, unsigned int bus, + unsigned int devfn, int reg, int len, u32 val) +{ return -ENOSYS; } + +static inline void pcibios_penalize_isa_irq(int irq, int active) { } + static inline int pci_domain_nr(struct pci_bus *bus) { return 0; } static inline struct pci_dev *pci_dev_get(struct pci_dev *dev) { return NULL; } static inline int pci_get_new_domain_nr(void) { return -ENOSYS; } @@ -1639,7 +1657,6 @@ int pcibios_set_pcie_reset_state(struct pci_dev *dev, enum pcie_reset_state state); int pcibios_add_device(struct pci_dev *dev); void pcibios_release_device(struct pci_dev *dev); -void pcibios_penalize_isa_irq(int irq, int active); #ifdef CONFIG_HIBERNATE_CALLBACKS extern struct dev_pm_ops pcibios_pm_ops;