Re: [PATCH 09/20] ARM64 / ACPI: Implement core functions for parsing MADT table
On Friday 24 January 2014, Hanjun Guo wrote: > >> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h > >> index e108d9c..c335c6d 100644 > >> --- a/arch/arm64/include/asm/acpi.h > >> +++ b/arch/arm64/include/asm/acpi.h > >> @@ -83,6 +83,9 @@ void arch_fix_phys_package_id(int num, u32 slot); > >> extern int (*acpi_suspend_lowlevel)(void); > >> #define acpi_wakeup_address (0) > >> > >> +#define MAX_GIC_CPU_INTERFACE 256 > > I'll bite. Where on Earth is this value coming from? > > I just thought 256 is big enough for now :( > Yes, should be a larger number for GICv3. Could this just be set to NR_CPUS? That way it will be large enough for any system you can actually run on. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 09/20] ARM64 / ACPI: Implement core functions for parsing MADT table
Hi Marc,
On 2014年01月24日 01:54, Marc Zyngier wrote:
Hi Hanjun,
On 17/01/14 12:25, Hanjun Guo wrote:
Implement core functions for parsing MADT table to get the information
about GIC cpu interface and GIC distributor to prepare for SMP and GIC
initialization.
Signed-off-by: Hanjun Guo
---
arch/arm64/include/asm/acpi.h |3 +
drivers/acpi/plat/arm-core.c | 139 -
drivers/acpi/tables.c | 21 +++
3 files changed, 162 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index e108d9c..c335c6d 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -83,6 +83,9 @@ void arch_fix_phys_package_id(int num, u32 slot);
extern int (*acpi_suspend_lowlevel)(void);
#define acpi_wakeup_address (0)
+#define MAX_GIC_CPU_INTERFACE 256
I'll bite. Where on Earth is this value coming from?
I just thought 256 is big enough for now :(
Yes, should be a larger number for GICv3.
If that's for
GICv2, 8 is the maximum. For GICv3+, that's incredibly low, and should
be probed probed at runtime anyway.
I would prefer to do that, but this value is used to
probe CPUs in MADT :)
+#define MAX_GIC_DISTRIBUTOR 1/* should be the same as
MAX_GIC_NR */
No support for cascaded GICs?
Yes, no cascade GICs in ACPI at now.
+
#else /* !CONFIG_ACPI */
#define acpi_disabled 1 /* ACPI sometimes enabled on ARM */
#define acpi_noirq 1 /* ACPI sometimes enabled on ARM */
diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c
index 1835b21..8ba3e6f 100644
--- a/drivers/acpi/plat/arm-core.c
+++ b/drivers/acpi/plat/arm-core.c
@@ -46,6 +46,16 @@ EXPORT_SYMBOL(acpi_disabled);
int acpi_pci_disabled;/* skip ACPI PCI scan and IRQ
initialization */
EXPORT_SYMBOL(acpi_pci_disabled);
+/*
+ * Local interrupt controller address,
+ * GIC cpu interface base address on ARM/ARM64
+ */
+static u64 acpi_lapic_addr __initdata;
If that's a GIC address, why not call it as such?
thanks for the suggesting, I will update.
+#define BAD_MADT_ENTRY(entry, end) ( \
+ (!entry) || (unsigned long)entry + sizeof(*entry) > end || \
+ ((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
+
#define PREFIX"ACPI: "
Just do:
#define pr_fmt(fmt) "ACPI: " fmt
and remove all the occurrences of PREFIX.
/* FIXME: this function should be moved to topology.c when it is ready */
@@ -92,6 +102,115 @@ void __init __acpi_unmap_table(char *map, unsigned long
size)
return;
}
+static int __init acpi_parse_madt(struct acpi_table_header *table)
+{
+ struct acpi_table_madt *madt = NULL;
No need to initialize this to NULL, you're doing an assignment at the
next line...
+
+ madt = (struct acpi_table_madt *)table;
+ if (!madt) {
+ pr_warn(PREFIX "Unable to map MADT\n");
There is no mapping here, please fix the message accordingly.
Ok, I will address your comments above in next version.
+ return -ENODEV;
+ }
+
+ if (madt->address) {
+ acpi_lapic_addr = (u64) madt->address;
So you're updating this static variable, for the distributor and each
CPU interface? /me puzzled...
Good catch. So I have a question: do we really have some SoCs
without banked registers on ARM64? if not , I think we can use
a single static variable is ok.
+ pr_info(PREFIX "Local APIC address 0x%08x\n", madt->address);
Away with this APIC madness. GICC and GICD are the concepts we're all
familiar with here, and using the proper terminology would certainly
help reviewing these patches...
That make sense to me too, will update.
+ }
+
+ return 0;
+}
+
+/*
+ * GIC structures on ARM are somthing like Local APIC structures on x86,
+ * which means GIC cpu interfaces for GICv2/v3. Every GIC structure in
+ * MADT table represents a cpu in the system.
And what do you do when your GICv3 doesn't have a memory-mapped
interface, but only uses system registers?
+ * GIC distributor structures are somthing like IOAPIC on x86. GIC can
+ * be initialized with information in this structure.
+ *
+ * Please refer to chapter5.2.12.14/15 of ACPI 5.0
A pointer to that documentation?
Please refer to http://www.acpi.info/
+ */
+
+static int __init
+acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end)
+{
+ struct acpi_madt_generic_interrupt *processor = NULL;
+
+ processor = (struct acpi_madt_generic_interrupt *)header;
+
+ if (BAD_MADT_ENTRY(processor, end))
+ return -EINVAL;
+
+ acpi_table_print_madt_entry(header);
+
+ return 0;
+}
+
+static int __init
+acpi_parse_gic_distributor(struct acpi_subtable_header *header,
+ const unsigned long end)
+{
+ struct acpi_madt_generic_
Re: [PATCH 09/20] ARM64 / ACPI: Implement core functions for parsing MADT table
Hi Hanjun,
On 17/01/14 12:25, Hanjun Guo wrote:
> Implement core functions for parsing MADT table to get the information
> about GIC cpu interface and GIC distributor to prepare for SMP and GIC
> initialization.
>
> Signed-off-by: Hanjun Guo
> ---
> arch/arm64/include/asm/acpi.h |3 +
> drivers/acpi/plat/arm-core.c | 139
> -
> drivers/acpi/tables.c | 21 +++
> 3 files changed, 162 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index e108d9c..c335c6d 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -83,6 +83,9 @@ void arch_fix_phys_package_id(int num, u32 slot);
> extern int (*acpi_suspend_lowlevel)(void);
> #define acpi_wakeup_address (0)
>
> +#define MAX_GIC_CPU_INTERFACE 256
I'll bite. Where on Earth is this value coming from? If that's for
GICv2, 8 is the maximum. For GICv3+, that's incredibly low, and should
be probed probed at runtime anyway.
> +#define MAX_GIC_DISTRIBUTOR 1 /* should be the same as
> MAX_GIC_NR */
No support for cascaded GICs?
> +
> #else/* !CONFIG_ACPI */
> #define acpi_disabled 1 /* ACPI sometimes enabled on ARM */
> #define acpi_noirq 1 /* ACPI sometimes enabled on ARM */
> diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c
> index 1835b21..8ba3e6f 100644
> --- a/drivers/acpi/plat/arm-core.c
> +++ b/drivers/acpi/plat/arm-core.c
> @@ -46,6 +46,16 @@ EXPORT_SYMBOL(acpi_disabled);
> int acpi_pci_disabled; /* skip ACPI PCI scan and IRQ
> initialization */
> EXPORT_SYMBOL(acpi_pci_disabled);
>
> +/*
> + * Local interrupt controller address,
> + * GIC cpu interface base address on ARM/ARM64
> + */
> +static u64 acpi_lapic_addr __initdata;
If that's a GIC address, why not call it as such?
> +#define BAD_MADT_ENTRY(entry, end) ( \
> + (!entry) || (unsigned long)entry + sizeof(*entry) > end || \
> + ((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
> +
> #define PREFIX "ACPI: "
Just do:
#define pr_fmt(fmt) "ACPI: " fmt
and remove all the occurrences of PREFIX.
> /* FIXME: this function should be moved to topology.c when it is ready */
> @@ -92,6 +102,115 @@ void __init __acpi_unmap_table(char *map, unsigned long
> size)
> return;
> }
>
> +static int __init acpi_parse_madt(struct acpi_table_header *table)
> +{
> + struct acpi_table_madt *madt = NULL;
No need to initialize this to NULL, you're doing an assignment at the
next line...
> +
> + madt = (struct acpi_table_madt *)table;
> + if (!madt) {
> + pr_warn(PREFIX "Unable to map MADT\n");
There is no mapping here, please fix the message accordingly.
> + return -ENODEV;
> + }
> +
> + if (madt->address) {
> + acpi_lapic_addr = (u64) madt->address;
So you're updating this static variable, for the distributor and each
CPU interface? /me puzzled...
> + pr_info(PREFIX "Local APIC address 0x%08x\n", madt->address);
Away with this APIC madness. GICC and GICD are the concepts we're all
familiar with here, and using the proper terminology would certainly
help reviewing these patches...
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * GIC structures on ARM are somthing like Local APIC structures on x86,
> + * which means GIC cpu interfaces for GICv2/v3. Every GIC structure in
> + * MADT table represents a cpu in the system.
And what do you do when your GICv3 doesn't have a memory-mapped
interface, but only uses system registers?
> + * GIC distributor structures are somthing like IOAPIC on x86. GIC can
> + * be initialized with information in this structure.
> + *
> + * Please refer to chapter5.2.12.14/15 of ACPI 5.0
A pointer to that documentation?
> + */
> +
> +static int __init
> +acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end)
> +{
> + struct acpi_madt_generic_interrupt *processor = NULL;
> +
> + processor = (struct acpi_madt_generic_interrupt *)header;
> +
> + if (BAD_MADT_ENTRY(processor, end))
> + return -EINVAL;
> +
> + acpi_table_print_madt_entry(header);
> +
> + return 0;
> +}
> +
> +static int __init
> +acpi_parse_gic_distributor(struct acpi_subtable_header *header,
> + const unsigned long end)
> +{
> + struct acpi_madt_generic_distributor *distributor = NULL;
> +
> + distributor = (struct acpi_madt_generic_distributor *)header;
> +
> + if (BAD_MADT_ENTRY(distributor, end))
> + return -EINVAL;
> +
> + acpi_table_print_madt_entry(header);
> +
> + return 0;
> +}
> +
> +/*
> + * Parse GIC cpu interface related entries in MADT
> + * returns 0 on success, < 0 on error
> + */
> +static int __init acpi_parse_madt_gic_entries(void)
> +{
> + int count;
> +
> + /*
> + * do a
Re: [PATCH 09/20] ARM64 / ACPI: Implement core functions for parsing MADT table
On 2014-1-17 22:12, Arnd Bergmann wrote:
> On Friday 17 January 2014, Hanjun Guo wrote:
>>
>> +/*
>> + * Local interrupt controller address,
>> + * GIC cpu interface base address on ARM/ARM64
>> + */
>> +static u64 acpi_lapic_addr __initdata;
>
> If it's cpu local, don't you need more than one address to support SMP?
Good point, thanks for pointing this out. I have a question, do we really
have some SoCs without banked registers?
I ask this question because we can do something for GIC cpu interface
with per cpu offset, but ACPI do NOT support per cpu offset for GIC
distributor.
> Also, the variable appears to be write-only.
Actually not, it will be used for GIC initialization.
>
>> +#define BAD_MADT_ENTRY(entry, end) (
>> \
>> +(!entry) || (unsigned long)entry + sizeof(*entry) > end || \
>> +((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
>> +
>
> Better make this an inline function.
>
>> +static int __init
>> +acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end)
>> +{
>> +struct acpi_madt_generic_interrupt *processor = NULL;
>> +
>> +processor = (struct acpi_madt_generic_interrupt *)header;
>
> You don't need the initialization in the first line when you write to the
> variable before reading it. Same in the other functions.
Ok, I will update them all in next version.
Thanks
hanjun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 09/20] ARM64 / ACPI: Implement core functions for parsing MADT table
On Friday 17 January 2014, Hanjun Guo wrote:
>
> +/*
> + * Local interrupt controller address,
> + * GIC cpu interface base address on ARM/ARM64
> + */
> +static u64 acpi_lapic_addr __initdata;
If it's cpu local, don't you need more than one address to support SMP?
Also, the variable appears to be write-only.
> +#define BAD_MADT_ENTRY(entry, end) ( \
> + (!entry) || (unsigned long)entry + sizeof(*entry) > end || \
> + ((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
> +
Better make this an inline function.
> +static int __init
> +acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end)
> +{
> + struct acpi_madt_generic_interrupt *processor = NULL;
> +
> + processor = (struct acpi_madt_generic_interrupt *)header;
You don't need the initialization in the first line when you write to the
variable before reading it. Same in the other functions.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 09/20] ARM64 / ACPI: Implement core functions for parsing MADT table
Implement core functions for parsing MADT table to get the information
about GIC cpu interface and GIC distributor to prepare for SMP and GIC
initialization.
Signed-off-by: Hanjun Guo
---
arch/arm64/include/asm/acpi.h |3 +
drivers/acpi/plat/arm-core.c | 139 -
drivers/acpi/tables.c | 21 +++
3 files changed, 162 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index e108d9c..c335c6d 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -83,6 +83,9 @@ void arch_fix_phys_package_id(int num, u32 slot);
extern int (*acpi_suspend_lowlevel)(void);
#define acpi_wakeup_address (0)
+#define MAX_GIC_CPU_INTERFACE 256
+#define MAX_GIC_DISTRIBUTOR 1/* should be the same as
MAX_GIC_NR */
+
#else /* !CONFIG_ACPI */
#define acpi_disabled 1/* ACPI sometimes enabled on ARM */
#define acpi_noirq 1 /* ACPI sometimes enabled on ARM */
diff --git a/drivers/acpi/plat/arm-core.c b/drivers/acpi/plat/arm-core.c
index 1835b21..8ba3e6f 100644
--- a/drivers/acpi/plat/arm-core.c
+++ b/drivers/acpi/plat/arm-core.c
@@ -46,6 +46,16 @@ EXPORT_SYMBOL(acpi_disabled);
int acpi_pci_disabled; /* skip ACPI PCI scan and IRQ initialization */
EXPORT_SYMBOL(acpi_pci_disabled);
+/*
+ * Local interrupt controller address,
+ * GIC cpu interface base address on ARM/ARM64
+ */
+static u64 acpi_lapic_addr __initdata;
+
+#define BAD_MADT_ENTRY(entry, end) ( \
+ (!entry) || (unsigned long)entry + sizeof(*entry) > end || \
+ ((struct acpi_subtable_header *)entry)->length < sizeof(*entry))
+
#define PREFIX "ACPI: "
/* FIXME: this function should be moved to topology.c when it is ready */
@@ -92,6 +102,115 @@ void __init __acpi_unmap_table(char *map, unsigned long
size)
return;
}
+static int __init acpi_parse_madt(struct acpi_table_header *table)
+{
+ struct acpi_table_madt *madt = NULL;
+
+ madt = (struct acpi_table_madt *)table;
+ if (!madt) {
+ pr_warn(PREFIX "Unable to map MADT\n");
+ return -ENODEV;
+ }
+
+ if (madt->address) {
+ acpi_lapic_addr = (u64) madt->address;
+
+ pr_info(PREFIX "Local APIC address 0x%08x\n", madt->address);
+ }
+
+ return 0;
+}
+
+/*
+ * GIC structures on ARM are somthing like Local APIC structures on x86,
+ * which means GIC cpu interfaces for GICv2/v3. Every GIC structure in
+ * MADT table represents a cpu in the system.
+ *
+ * GIC distributor structures are somthing like IOAPIC on x86. GIC can
+ * be initialized with information in this structure.
+ *
+ * Please refer to chapter5.2.12.14/15 of ACPI 5.0
+ */
+
+static int __init
+acpi_parse_gic(struct acpi_subtable_header *header, const unsigned long end)
+{
+ struct acpi_madt_generic_interrupt *processor = NULL;
+
+ processor = (struct acpi_madt_generic_interrupt *)header;
+
+ if (BAD_MADT_ENTRY(processor, end))
+ return -EINVAL;
+
+ acpi_table_print_madt_entry(header);
+
+ return 0;
+}
+
+static int __init
+acpi_parse_gic_distributor(struct acpi_subtable_header *header,
+ const unsigned long end)
+{
+ struct acpi_madt_generic_distributor *distributor = NULL;
+
+ distributor = (struct acpi_madt_generic_distributor *)header;
+
+ if (BAD_MADT_ENTRY(distributor, end))
+ return -EINVAL;
+
+ acpi_table_print_madt_entry(header);
+
+ return 0;
+}
+
+/*
+ * Parse GIC cpu interface related entries in MADT
+ * returns 0 on success, < 0 on error
+ */
+static int __init acpi_parse_madt_gic_entries(void)
+{
+ int count;
+
+ /*
+* do a partial walk of MADT to determine how many CPUs
+* we have including disabled CPUs
+*/
+ count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_INTERRUPT,
+ acpi_parse_gic, MAX_GIC_CPU_INTERFACE);
+
+ if (!count) {
+ pr_err(PREFIX "No GIC entries present\n");
+ return -ENODEV;
+ } else if (count < 0) {
+ pr_err(PREFIX "Error parsing GIC entry\n");
+ return count;
+ }
+
+ return 0;
+}
+
+/*
+ * Parse GIC distributor related entries in MADT
+ * returns 0 on success, < 0 on error
+ */
+static int __init acpi_parse_madt_gic_distributor_entries(void)
+{
+ int count;
+
+ count = acpi_table_parse_madt(ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR,
+ acpi_parse_gic_distributor, MAX_GIC_DISTRIBUTOR);
+
+ if (!count) {
+ pr_err(PREFIX "No GIC distributor entries present\n");
+ return -ENODEV;
+ } else if (count < 0) {
+ pr_err(PREFIX "Error parsing GIC distributor entry\n");
+ return count;
+ }
+
+ return 0;
+}
+
i
