| Message ID | 1389961514-13562-7-git-send-email-hanjun.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Friday 17 January 2014, Hanjun Guo wrote: > +++ b/arch/arm64/pci/Makefile > @@ -0,0 +1 @@ > +obj-y += pci.o > diff --git a/arch/arm64/pci/pci.c b/arch/arm64/pci/pci.c > new file mode 100644 > index 0000000..4e46790 > --- /dev/null > +++ b/arch/arm64/pci/pci.c > @@ -0,0 +1,33 @@ > +#include <linux/acpi.h> > +#include <linux/types.h> > +#include <linux/kernel.h> > +#include <linux/pci.h> > + > +/** > + * raw_pci_read - Platform-specific PCI config space access. > + * > + * Default empty implementation. Replace with an architecture-specific setup > + * routine, if necessary. > + */ > +int __weak raw_pci_read(unsigned int domain, unsigned int bus, > + unsigned int devfn, int reg, int len, u32 *val) > +{ > + return -EINVAL; > +} > + > +int __weak raw_pci_write(unsigned int domain, unsigned int bus, > + unsigned int devfn, int reg, int len, u32 val) > +{ > + return -EINVAL; > +} I'd rather not see __weak functions here. Just provide them unconditionally so that we can add a proper implementation when needed. You could also define these as 'static inline' in a header file to keep them from consuming space in the object code. > diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c > index 3c8521d..1835b21 100644 > --- a/drivers/acpi/plat/arm-core.c > +++ b/drivers/acpi/plat/arm-core.c > @@ -100,6 +100,25 @@ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) > } > EXPORT_SYMBOL_GPL(acpi_gsi_to_irq); > > +int acpi_isa_irq_to_gsi(unsigned isa_irq, u32 *gsi) > +{ > + return -1; > +} > + > +int acpi_register_ioapic(acpi_handle handle, u64 phys_addr, u32 gsi_base) > +{ > + /* TBD */ > + return -EINVAL; > +} > +EXPORT_SYMBOL(acpi_register_ioapic); > + > +int acpi_unregister_ioapic(acpi_handle handle, u32 gsi_base) > +{ > + /* TBD */ > + return -EINVAL; > +} > +EXPORT_SYMBOL(acpi_unregister_ioapic); > + My feeling is that these are better handled in the ACPI code by not calling them on architectures that have no ISA or no IOAPIC support. We have configuration symbols for both, so you don't have to make it depend on CONFIG_ARM64 or CONFIG_X86. Arnd
On 2014-1-17 22:04, Arnd Bergmann wrote: > On Friday 17 January 2014, Hanjun Guo wrote: >> +++ b/arch/arm64/pci/Makefile >> @@ -0,0 +1 @@ >> +obj-y += pci.o >> diff --git a/arch/arm64/pci/pci.c b/arch/arm64/pci/pci.c >> new file mode 100644 >> index 0000000..4e46790 >> --- /dev/null >> +++ b/arch/arm64/pci/pci.c >> @@ -0,0 +1,33 @@ >> +#include <linux/acpi.h> >> +#include <linux/types.h> >> +#include <linux/kernel.h> >> +#include <linux/pci.h> >> + >> +/** >> + * raw_pci_read - Platform-specific PCI config space access. >> + * >> + * Default empty implementation. Replace with an architecture-specific setup >> + * routine, if necessary. >> + */ >> +int __weak raw_pci_read(unsigned int domain, unsigned int bus, >> + unsigned int devfn, int reg, int len, u32 *val) >> +{ >> + return -EINVAL; >> +} >> + >> +int __weak raw_pci_write(unsigned int domain, unsigned int bus, >> + unsigned int devfn, int reg, int len, u32 val) >> +{ >> + return -EINVAL; >> +} > > I'd rather not see __weak functions here. Just provide them unconditionally > so that we can add a proper implementation when needed. You could also > define these as 'static inline' in a header file to keep them from consuming > space in the object code. Ok, I will remove __weak in next version. > >> diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c >> index 3c8521d..1835b21 100644 >> --- a/drivers/acpi/plat/arm-core.c >> +++ b/drivers/acpi/plat/arm-core.c >> @@ -100,6 +100,25 @@ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) >> } >> EXPORT_SYMBOL_GPL(acpi_gsi_to_irq); >> >> +int acpi_isa_irq_to_gsi(unsigned isa_irq, u32 *gsi) >> +{ >> + return -1; >> +} >> + >> +int acpi_register_ioapic(acpi_handle handle, u64 phys_addr, u32 gsi_base) >> +{ >> + /* TBD */ >> + return -EINVAL; >> +} >> +EXPORT_SYMBOL(acpi_register_ioapic); >> + >> +int acpi_unregister_ioapic(acpi_handle handle, u32 gsi_base) >> +{ >> + /* TBD */ >> + return -EINVAL; >> +} >> +EXPORT_SYMBOL(acpi_unregister_ioapic); >> + > > My feeling is that these are better handled in the ACPI code by not > calling them on architectures that have no ISA or no IOAPIC support. > > We have configuration symbols for both, so you don't have to make > it depend on CONFIG_ARM64 or CONFIG_X86. Do you mean introduce a stub function when there is no ISA support? acpi_register_ioapic()/acpi_unregister_ioapic() will be used for IOAPIC hotplug and GIC distributor is something like IOAPIC on x86, so I think these two functions can be reserved for future use. Thanks Hanjun
On Monday 20 January 2014 16:08:01 Hanjun Guo wrote: > >> diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c > >> index 3c8521d..1835b21 100644 > >> --- a/drivers/acpi/plat/arm-core.c > >> +++ b/drivers/acpi/plat/arm-core.c > >> @@ -100,6 +100,25 @@ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) > >> } > >> EXPORT_SYMBOL_GPL(acpi_gsi_to_irq); > >> > >> +int acpi_isa_irq_to_gsi(unsigned isa_irq, u32 *gsi) > >> +{ > >> + return -1; > >> +} > >> + > >> +int acpi_register_ioapic(acpi_handle handle, u64 phys_addr, u32 gsi_base) > >> +{ > >> + /* TBD */ > >> + return -EINVAL; > >> +} > >> +EXPORT_SYMBOL(acpi_register_ioapic); > >> + > >> +int acpi_unregister_ioapic(acpi_handle handle, u32 gsi_base) > >> +{ > >> + /* TBD */ > >> + return -EINVAL; > >> +} > >> +EXPORT_SYMBOL(acpi_unregister_ioapic); > >> + > > > > My feeling is that these are better handled in the ACPI code by not > > calling them on architectures that have no ISA or no IOAPIC support. > > > > We have configuration symbols for both, so you don't have to make > > it depend on CONFIG_ARM64 or CONFIG_X86. > > Do you mean introduce a stub function when there is no ISA support? Do you anticipate ISA devices on ARM64? I hope not ;-) My guess is that whatever code calls this function should be disabled in reduced hw mode. > acpi_register_ioapic()/acpi_unregister_ioapic() will be used for IOAPIC > hotplug and GIC distributor is something like IOAPIC on x86, so I think > these two functions can be reserved for future use. But GIC is not hotplugged, is it? It still sounds x86 specific to me. Arnd
On 2014?01?20? 16:20, Arnd Bergmann wrote: > On Monday 20 January 2014 16:08:01 Hanjun Guo wrote: >>>> diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c >>>> index 3c8521d..1835b21 100644 >>>> --- a/drivers/acpi/plat/arm-core.c >>>> +++ b/drivers/acpi/plat/arm-core.c >>>> @@ -100,6 +100,25 @@ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) >>>> } >>>> EXPORT_SYMBOL_GPL(acpi_gsi_to_irq); >>>> >>>> +int acpi_isa_irq_to_gsi(unsigned isa_irq, u32 *gsi) >>>> +{ >>>> + return -1; >>>> +} >>>> + >>>> +int acpi_register_ioapic(acpi_handle handle, u64 phys_addr, u32 gsi_base) >>>> +{ >>>> + /* TBD */ >>>> + return -EINVAL; >>>> +} >>>> +EXPORT_SYMBOL(acpi_register_ioapic); >>>> + >>>> +int acpi_unregister_ioapic(acpi_handle handle, u32 gsi_base) >>>> +{ >>>> + /* TBD */ >>>> + return -EINVAL; >>>> +} >>>> +EXPORT_SYMBOL(acpi_unregister_ioapic); >>>> + >>> My feeling is that these are better handled in the ACPI code by not >>> calling them on architectures that have no ISA or no IOAPIC support. >>> >>> We have configuration symbols for both, so you don't have to make >>> it depend on CONFIG_ARM64 or CONFIG_X86. >> Do you mean introduce a stub function when there is no ISA support? > Do you anticipate ISA devices on ARM64? I hope not ;-) me too :) > > My guess is that whatever code calls this function should be disabled > in reduced hw mode. ok, that would be make sense, will update it in next version. > >> acpi_register_ioapic()/acpi_unregister_ioapic() will be used for IOAPIC >> hotplug and GIC distributor is something like IOAPIC on x86, so I think >> these two functions can be reserved for future use. > But GIC is not hotplugged, is it? It still sounds x86 specific to me. Well, if we want to do physical CPU hotplug on ARM/ARM64 (maybe years later?), then GIC add/remove is needed because we have to remove GIC on the SoC too when we remove the physical CPU. Thanks Hanjun
On Monday 20 January 2014, Hanjun Guo wrote: > >> acpi_register_ioapic()/acpi_unregister_ioapic() will be used for IOAPIC > >> hotplug and GIC distributor is something like IOAPIC on x86, so I think > >> these two functions can be reserved for future use. > > But GIC is not hotplugged, is it? It still sounds x86 specific to me. > > Well, if we want to do physical CPU hotplug on ARM/ARM64 (maybe years > later?), > then GIC add/remove is needed because we have to remove GIC > on the SoC too when we remove the physical CPU. In general, I recommend not planning for the future in kernel code when you don't know what is going to happen. It's always easy enough to change things once you get there, as long as no stable ABI is involved. I just looked at the caller of these functions, and found a self-contained PCI driver in drivers/pci/ioapic.c, which uses two sepate PCI classes for ioapic and ioxapic. I think it's a safe assumption to say that even if we get ARM CPU+GIC hotplug, that would not use the same ioapic driver. This driver is currently marked X86-only, and that should probably stay this way, so you won't need the hooks. Arnd
On 2014-1-21 2:39, Arnd Bergmann wrote: > On Monday 20 January 2014, Hanjun Guo wrote: >>>> acpi_register_ioapic()/acpi_unregister_ioapic() will be used for IOAPIC >>>> hotplug and GIC distributor is something like IOAPIC on x86, so I think >>>> these two functions can be reserved for future use. >>> But GIC is not hotplugged, is it? It still sounds x86 specific to me. >> >> Well, if we want to do physical CPU hotplug on ARM/ARM64 (maybe years >> later?), >> then GIC add/remove is needed because we have to remove GIC >> on the SoC too when we remove the physical CPU. > > In general, I recommend not planning for the future in kernel code when you > don't know what is going to happen. It's always easy enough to change > things once you get there, as long as no stable ABI is involved. Ok, I agree with you. > > I just looked at the caller of these functions, and found a self-contained > PCI driver in drivers/pci/ioapic.c, which uses two sepate PCI classes for > ioapic and ioxapic. I think it's a safe assumption to say that even if we > get ARM CPU+GIC hotplug, that would not use the same ioapic driver. This > driver is currently marked X86-only, and that should probably stay this way, > so you won't need the hooks. Will find a suitable way to fix that in next version, thanks for you comments :) Hanjun
diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile index 2fceb71..fcaa58e 100644 --- a/arch/arm64/Makefile +++ b/arch/arm64/Makefile @@ -45,6 +45,7 @@ export TEXT_OFFSET GZFLAGS core-y += arch/arm64/kernel/ arch/arm64/mm/ core-$(CONFIG_KVM) += arch/arm64/kvm/ core-$(CONFIG_XEN) += arch/arm64/xen/ +drivers-$(CONFIG_PCI) += arch/arm64/pci/ libs-y := arch/arm64/lib/ $(libs-y) libs-y += $(LIBGCC) diff --git a/arch/arm64/pci/Makefile b/arch/arm64/pci/Makefile new file mode 100644 index 0000000..b8d5dbd --- /dev/null +++ b/arch/arm64/pci/Makefile @@ -0,0 +1 @@ +obj-y += pci.o diff --git a/arch/arm64/pci/pci.c b/arch/arm64/pci/pci.c new file mode 100644 index 0000000..4e46790 --- /dev/null +++ b/arch/arm64/pci/pci.c @@ -0,0 +1,33 @@ +#include <linux/acpi.h> +#include <linux/types.h> +#include <linux/kernel.h> +#include <linux/pci.h> + +/** + * raw_pci_read - Platform-specific PCI config space access. + * + * Default empty implementation. Replace with an architecture-specific setup + * routine, if necessary. + */ +int __weak raw_pci_read(unsigned int domain, unsigned int bus, + unsigned int devfn, int reg, int len, u32 *val) +{ + return -EINVAL; +} + +int __weak raw_pci_write(unsigned int domain, unsigned int bus, + unsigned int devfn, int reg, int len, u32 val) +{ + return -EINVAL; +} + +/* Root bridge scanning */ +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) +{ + return NULL; +} + +void __init pci_acpi_crs_quirks(void) +{ + return; +} diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c index 3c8521d..1835b21 100644 --- a/drivers/acpi/plat/arm-core.c +++ b/drivers/acpi/plat/arm-core.c @@ -100,6 +100,25 @@ int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) } EXPORT_SYMBOL_GPL(acpi_gsi_to_irq); +int acpi_isa_irq_to_gsi(unsigned isa_irq, u32 *gsi) +{ + return -1; +} + +int acpi_register_ioapic(acpi_handle handle, u64 phys_addr, u32 gsi_base) +{ + /* TBD */ + return -EINVAL; +} +EXPORT_SYMBOL(acpi_register_ioapic); + +int acpi_unregister_ioapic(acpi_handle handle, u32 gsi_base) +{ + /* TBD */ + return -EINVAL; +} +EXPORT_SYMBOL(acpi_unregister_ioapic); + /* * success: return IRQ number (>0) * failure: return =< 0
Introduce some PCI functions to make ACPI can be compiled when CONFIG_PCI is enabled, these functions should be revisited when implemented on ARM64. Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org> --- arch/arm64/Makefile | 1 + arch/arm64/pci/Makefile | 1 + arch/arm64/pci/pci.c | 33 +++++++++++++++++++++++++++++++++ drivers/acpi/plat/arm-core.c | 19 +++++++++++++++++++ 4 files changed, 54 insertions(+) create mode 100644 arch/arm64/pci/Makefile create mode 100644 arch/arm64/pci/pci.c