Re: [PATCH v7 06/17] ARM64 / ACPI: Make PCI optional for ACPI on ARM64
From: Jon Masters
Date: Sun Jan 18 2015 - 01:33:11 EST
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.
--
Computer Architect | Sent from my #ARM Powered Mobile Device
On Jan 18, 2015 1:26 AM, Hanjun Guo <hanjun.guo@xxxxxxxxxx> wrote:
>
> On 2015å01æ16æ 17:49, Catalin Marinas wrote:
> > On Wed, Jan 14, 2015 at 03:04:54PM +0000, Hanjun Guo wrote:
> >> Since PCI iOn 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@xxxxxxx>
>> Tested-by: Yijing Wang <wangyijing@xxxxxxxxxx>
>> Signed-off-by: Hanjun Guo <hanjun.guo@xxxxxxxxxx>
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/