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 hanjun.guo@linaro.org
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 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;
- }
So you do a lot of parsing to count stuff, and then discard the number of counted objects... You might as well check that there is at least one valid object and stop there.
- 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;
+}
int acpi_gsi_to_irq(u32 gsi, unsigned int *irq) { *irq = gsi_to_irq(gsi); @@ -141,11 +260,29 @@ static int __init acpi_parse_fadt(struct acpi_table_header *table) static void __init early_acpi_process_madt(void) {
- return;
- acpi_table_parse(ACPI_SIG_MADT, acpi_parse_madt);
} static void __init acpi_process_madt(void) {
- int error;
- if (!acpi_table_parse(ACPI_SIG_MADT, acpi_parse_madt)) {
How many times are you going to parse the same table? Surely you can stash whatever information you need and be done with it?
/*
* Parse MADT GIC cpu interface entries
*/
error = acpi_parse_madt_gic_entries();
if (!error) {
/*
* Parse MADT GIC distributor entries
*/
acpi_parse_madt_gic_distributor_entries();
}
- }
- pr_info("Using ACPI for processor (GIC) configuration information\n");
- return;
} diff --git a/drivers/acpi/tables.c b/drivers/acpi/tables.c index d67a1fe..b3e4615 100644 --- a/drivers/acpi/tables.c +++ b/drivers/acpi/tables.c @@ -191,6 +191,27 @@ void acpi_table_print_madt_entry(struct acpi_subtable_header *header) } break;
- case ACPI_MADT_TYPE_GENERIC_INTERRUPT:
{
struct acpi_madt_generic_interrupt *p =
(struct acpi_madt_generic_interrupt *)header;
printk(KERN_INFO PREFIX
Use pr_info
"GIC (acpi_id[0x%04x] gic_id[0x%04x] %s)\n",
p->uid, p->gic_id,
(p->flags & ACPI_MADT_ENABLED) ? "enabled" : "disabled");
}
break;
- case ACPI_MADT_TYPE_GENERIC_DISTRIBUTOR:
{
struct acpi_madt_generic_distributor *p =
(struct acpi_madt_generic_distributor *)header;
printk(KERN_INFO PREFIX
"GIC Distributor (id[0x%04x] address[0x%08llx] gsi_base[%d])\n",
p->gic_id, p->base_address, p->global_irq_base);
}
break;
- default: printk(KERN_WARNING PREFIX "Found unsupported MADT entry (type = 0x%x)\n",
Most of that code seems to be repeatedly parsing and printing stuff, and I fail to see what it actually does.
M.