Re: [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On Tue, 02 Sep 2014 14:02:31 +0100, Marc Zyngier wrote:
> On 02/09/14 12:48, Tomasz Nowicki wrote:
> > On 01.09.2014 19:35, Marc Zyngier wrote:
> >> On 01/09/14 15:57, Hanjun Guo wrote:
> >>> From: Tomasz Nowicki
> >>>
> >>> ACPI kernel uses MADT table for proper GIC initialization. It needs to
> >>> parse GIC related subtables, collect CPU interface and distributor
> >>> addresses and call driver initialization function (which is hardware
> >>> abstraction agnostic). In a similar way, FDT initialize GICv1/2.
> >>>
> >>> NOTE: This commit allow to initialize GICv1/2 only.
> >>
> >> I cannot help but notice that there is no support for KVM here. It'd be
> >> good to add a note to that effect, so that people do not expect
> >> virtualization support to be working when booting with ACPI.
> >
> > yes, it is worth mentioning!
> >
> >>
> >>> Signed-off-by: Tomasz Nowicki
> >>> Signed-off-by: Hanjun Guo
> >>> ---
> >>> arch/arm64/include/asm/acpi.h|2 -
> >>> arch/arm64/kernel/acpi.c | 23 +++
> >>> arch/arm64/kernel/irq.c |5 ++
> >>> drivers/irqchip/irq-gic.c| 114
> >>> ++
> >>> include/linux/irqchip/arm-gic-acpi.h | 33 ++
> >>> 5 files changed, 175 insertions(+), 2 deletions(-)
> >>> create mode 100644 include/linux/irqchip/arm-gic-acpi.h
> >>>
> >>> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> >>> index a867467..5d2ab63 100644
> >>> --- a/arch/arm64/include/asm/acpi.h
> >>> +++ b/arch/arm64/include/asm/acpi.h
> >>> @@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void);
> >>> extern int (*acpi_suspend_lowlevel)(void);
> >>> #define acpi_wakeup_address 0
> >>>
> >>> -#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535
> >>> -
> >>> #else
> >>>
> >>> static inline bool acpi_psci_present(void) { return false; }
> >>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> >>> index 354b912..b3b82b0 100644
> >>> --- a/arch/arm64/kernel/acpi.c
> >>> +++ b/arch/arm64/kernel/acpi.c
> >>> @@ -23,6 +23,7 @@
> >>> #include
> >>> #include
> >>> #include
> >>> +#include
> >>>
> >>> #include
> >>> #include
> >>> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
> >>> pr_err("Can't find FADT or error happened during parsing
> >>> FADT\n");
> >>> }
> >>>
> >>> +void __init acpi_gic_init(void)
> >>> +{
> >>> +struct acpi_table_header *table;
> >>> +acpi_status status;
> >>> +acpi_size tbl_size;
> >>> +int err;
> >>> +
> >>> +status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, ,
> >>> _size);
> >>> +if (ACPI_FAILURE(status)) {
> >>> +const char *msg = acpi_format_exception(status);
> >>> +
> >>> +pr_err("Failed to get MADT table, %s\n", msg);
> >>> +return;
> >>> +}
> >>> +
> >>> +err = gic_v2_acpi_init(table);
> >>> +if (err)
> >>> +pr_err("Failed to initialize GIC IRQ controller");
> >>
> >> What will happen when you get to implement GICv3 support? Another entry
> >> like this? Why isn't this entirely contained in the GIC driver? Do I
> >> sound like a stuck record?
> >
> > There will be another call to GICv3 init:
> > [...]
> > err = gic_v3_acpi_init(table);
> > if (err)
> > err = gic_v2_acpi_init(table);
> > if (err)
> > pr_err("Failed to initialize GIC IRQ controller");
> > [...]
> > This is the main reason I put common code here.
> >
> >>
> >>> +
> >>> +early_acpi_os_unmap_memory((char *)table, tbl_size);
> >>> +}
> >>> +
> >>> /*
> >>>* acpi_suspend_lowlevel() - save kernel state and suspend.
> >>>*
> >>> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
> >>> index 0f08dfd..c074d60 100644
> >>> --- a/arch/arm64/kernel/irq.c
> >>> +++ b/arch/arm64/kernel/irq.c
> >>> @@ -28,6 +28,7 @@
> >>> #include
> >>> #include
> >>> #include
> >>> +#include
> >>>
> >>> unsigned long irq_err_count;
> >>>
> >>> @@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct
> >>> pt_regs *))
> >>> void __init init_IRQ(void)
> >>> {
> >>> irqchip_init();
> >>> +
> >>> +if (!handle_arch_irq)
> >>> +acpi_gic_init();
> >>> +
> >>
> >> Why isn't this called from irqchip_init? It would seem like the logical
> >> spot to probe an interrupt controller.
> >
> > irqchip.c is OF dependent, I want to decouple these from the very
> > beginning.
>
> No. irqchip.c is not OF dependent, it is just that DT is the only thing
> we support so far. I don't think duplicating the kernel infrastructure
> "because we're different" is the right way.
>
> There is no reason for your probing structure to be artificially
> different (you're parsing the same information, at the same time). Just
> put in place a similar probing mechanism, and this will look a lot better.
>
> >>
> >>> if (!handle_arch_irq)
> >>> panic("No
Re: [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
Hi Grant,
On 11/09/14 12:48, Grant Likely wrote:
> On Tue, 02 Sep 2014 13:48:37 +0200, Tomasz Nowicki
> wrote:
>> On 01.09.2014 19:35, Marc Zyngier wrote:
>>> On 01/09/14 15:57, Hanjun Guo wrote:
From: Tomasz Nowicki
ACPI kernel uses MADT table for proper GIC initialization. It needs to
parse GIC related subtables, collect CPU interface and distributor
addresses and call driver initialization function (which is hardware
abstraction agnostic). In a similar way, FDT initialize GICv1/2.
NOTE: This commit allow to initialize GICv1/2 only.
>>>
>>> I cannot help but notice that there is no support for KVM here. It'd be
>>> good to add a note to that effect, so that people do not expect
>>> virtualization support to be working when booting with ACPI.
>>
>> yes, it is worth mentioning!
>>
>>>
Signed-off-by: Tomasz Nowicki
Signed-off-by: Hanjun Guo
---
arch/arm64/include/asm/acpi.h|2 -
arch/arm64/kernel/acpi.c | 23 +++
arch/arm64/kernel/irq.c |5 ++
drivers/irqchip/irq-gic.c| 114
++
include/linux/irqchip/arm-gic-acpi.h | 33 ++
5 files changed, 175 insertions(+), 2 deletions(-)
create mode 100644 include/linux/irqchip/arm-gic-acpi.h
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index a867467..5d2ab63 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void);
extern int (*acpi_suspend_lowlevel)(void);
#define acpi_wakeup_address 0
-#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535
-
#else
static inline bool acpi_psci_present(void) { return false; }
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 354b912..b3b82b0 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -23,6 +23,7 @@
#include
#include
#include
+#include
#include
#include
@@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
pr_err("Can't find FADT or error happened during parsing
FADT\n");
}
+void __init acpi_gic_init(void)
+{
+ struct acpi_table_header *table;
+ acpi_status status;
+ acpi_size tbl_size;
+ int err;
+
+ status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, , _size);
+ if (ACPI_FAILURE(status)) {
+ const char *msg = acpi_format_exception(status);
+
+ pr_err("Failed to get MADT table, %s\n", msg);
+ return;
+ }
+
+ err = gic_v2_acpi_init(table);
+ if (err)
+ pr_err("Failed to initialize GIC IRQ controller");
>>>
>>> What will happen when you get to implement GICv3 support? Another entry
>>> like this? Why isn't this entirely contained in the GIC driver? Do I
>>> sound like a stuck record?
>>
>> There will be another call to GICv3 init:
>> [...]
>> err = gic_v3_acpi_init(table);
>> if (err)
>> err = gic_v2_acpi_init(table);
>> if (err)
>> pr_err("Failed to initialize GIC IRQ controller");
>> [...]
>> This is the main reason I put common code here.
>>
>>>
+
+ early_acpi_os_unmap_memory((char *)table, tbl_size);
+}
+
/*
* acpi_suspend_lowlevel() - save kernel state and suspend.
*
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 0f08dfd..c074d60 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
unsigned long irq_err_count;
@@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct
pt_regs *))
void __init init_IRQ(void)
{
irqchip_init();
+
+ if (!handle_arch_irq)
+ acpi_gic_init();
+
>>>
>>> Why isn't this called from irqchip_init? It would seem like the logical
>>> spot to probe an interrupt controller.
>>
>> irqchip.c is OF dependent, I want to decouple these from the very
>> beginning.
>
> It doesn't have to be that way, but given that ARM64 is the only
> platform in the foreseeable future that will use ACPI irq setup, it
> doesn't make sense to put it in there.
I have a different perspective. There is no reason to pollute the arch
code with something that is essentially platform specific.
irqchip_init is the logical place to probe for an irqchip, and I fail to
see the point of sticking this code somewhere else. Why would ACPI be so
special that it requires additional hooks in the arch code?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the
Re: [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On Tue, 02 Sep 2014 13:48:37 +0200, Tomasz Nowicki
wrote:
> On 01.09.2014 19:35, Marc Zyngier wrote:
> > On 01/09/14 15:57, Hanjun Guo wrote:
> >> From: Tomasz Nowicki
> >>
> >> ACPI kernel uses MADT table for proper GIC initialization. It needs to
> >> parse GIC related subtables, collect CPU interface and distributor
> >> addresses and call driver initialization function (which is hardware
> >> abstraction agnostic). In a similar way, FDT initialize GICv1/2.
> >>
> >> NOTE: This commit allow to initialize GICv1/2 only.
> >
> > I cannot help but notice that there is no support for KVM here. It'd be
> > good to add a note to that effect, so that people do not expect
> > virtualization support to be working when booting with ACPI.
>
> yes, it is worth mentioning!
>
> >
> >> Signed-off-by: Tomasz Nowicki
> >> Signed-off-by: Hanjun Guo
> >> ---
> >> arch/arm64/include/asm/acpi.h|2 -
> >> arch/arm64/kernel/acpi.c | 23 +++
> >> arch/arm64/kernel/irq.c |5 ++
> >> drivers/irqchip/irq-gic.c| 114
> >> ++
> >> include/linux/irqchip/arm-gic-acpi.h | 33 ++
> >> 5 files changed, 175 insertions(+), 2 deletions(-)
> >> create mode 100644 include/linux/irqchip/arm-gic-acpi.h
> >>
> >> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> >> index a867467..5d2ab63 100644
> >> --- a/arch/arm64/include/asm/acpi.h
> >> +++ b/arch/arm64/include/asm/acpi.h
> >> @@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void);
> >> extern int (*acpi_suspend_lowlevel)(void);
> >> #define acpi_wakeup_address 0
> >>
> >> -#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535
> >> -
> >> #else
> >>
> >> static inline bool acpi_psci_present(void) { return false; }
> >> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> >> index 354b912..b3b82b0 100644
> >> --- a/arch/arm64/kernel/acpi.c
> >> +++ b/arch/arm64/kernel/acpi.c
> >> @@ -23,6 +23,7 @@
> >> #include
> >> #include
> >> #include
> >> +#include
> >>
> >> #include
> >> #include
> >> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
> >>pr_err("Can't find FADT or error happened during parsing
> >> FADT\n");
> >> }
> >>
> >> +void __init acpi_gic_init(void)
> >> +{
> >> + struct acpi_table_header *table;
> >> + acpi_status status;
> >> + acpi_size tbl_size;
> >> + int err;
> >> +
> >> + status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, , _size);
> >> + if (ACPI_FAILURE(status)) {
> >> + const char *msg = acpi_format_exception(status);
> >> +
> >> + pr_err("Failed to get MADT table, %s\n", msg);
> >> + return;
> >> + }
> >> +
> >> + err = gic_v2_acpi_init(table);
> >> + if (err)
> >> + pr_err("Failed to initialize GIC IRQ controller");
> >
> > What will happen when you get to implement GICv3 support? Another entry
> > like this? Why isn't this entirely contained in the GIC driver? Do I
> > sound like a stuck record?
>
> There will be another call to GICv3 init:
> [...]
> err = gic_v3_acpi_init(table);
> if (err)
> err = gic_v2_acpi_init(table);
> if (err)
> pr_err("Failed to initialize GIC IRQ controller");
> [...]
> This is the main reason I put common code here.
>
> >
> >> +
> >> + early_acpi_os_unmap_memory((char *)table, tbl_size);
> >> +}
> >> +
> >> /*
> >>* acpi_suspend_lowlevel() - save kernel state and suspend.
> >>*
> >> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
> >> index 0f08dfd..c074d60 100644
> >> --- a/arch/arm64/kernel/irq.c
> >> +++ b/arch/arm64/kernel/irq.c
> >> @@ -28,6 +28,7 @@
> >> #include
> >> #include
> >> #include
> >> +#include
> >>
> >> unsigned long irq_err_count;
> >>
> >> @@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct
> >> pt_regs *))
> >> void __init init_IRQ(void)
> >> {
> >>irqchip_init();
> >> +
> >> + if (!handle_arch_irq)
> >> + acpi_gic_init();
> >> +
> >
> > Why isn't this called from irqchip_init? It would seem like the logical
> > spot to probe an interrupt controller.
>
> irqchip.c is OF dependent, I want to decouple these from the very
> beginning.
It doesn't have to be that way, but given that ARM64 is the only
platform in the foreseeable future that will use ACPI irq setup, it
doesn't make sense to put it in there.
g.
--
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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On Tue, 02 Sep 2014 13:48:37 +0200, Tomasz Nowicki tomasz.nowi...@linaro.org
wrote:
On 01.09.2014 19:35, Marc Zyngier wrote:
On 01/09/14 15:57, Hanjun Guo wrote:
From: Tomasz Nowicki tomasz.nowi...@linaro.org
ACPI kernel uses MADT table for proper GIC initialization. It needs to
parse GIC related subtables, collect CPU interface and distributor
addresses and call driver initialization function (which is hardware
abstraction agnostic). In a similar way, FDT initialize GICv1/2.
NOTE: This commit allow to initialize GICv1/2 only.
I cannot help but notice that there is no support for KVM here. It'd be
good to add a note to that effect, so that people do not expect
virtualization support to be working when booting with ACPI.
yes, it is worth mentioning!
Signed-off-by: Tomasz Nowicki tomasz.nowi...@linaro.org
Signed-off-by: Hanjun Guo hanjun@linaro.org
---
arch/arm64/include/asm/acpi.h|2 -
arch/arm64/kernel/acpi.c | 23 +++
arch/arm64/kernel/irq.c |5 ++
drivers/irqchip/irq-gic.c| 114
++
include/linux/irqchip/arm-gic-acpi.h | 33 ++
5 files changed, 175 insertions(+), 2 deletions(-)
create mode 100644 include/linux/irqchip/arm-gic-acpi.h
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index a867467..5d2ab63 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void);
extern int (*acpi_suspend_lowlevel)(void);
#define acpi_wakeup_address 0
-#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535
-
#else
static inline bool acpi_psci_present(void) { return false; }
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 354b912..b3b82b0 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -23,6 +23,7 @@
#include linux/irqdomain.h
#include linux/bootmem.h
#include linux/smp.h
+#include linux/irqchip/arm-gic-acpi.h
#include asm/cputype.h
#include asm/cpu_ops.h
@@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
pr_err(Can't find FADT or error happened during parsing
FADT\n);
}
+void __init acpi_gic_init(void)
+{
+ struct acpi_table_header *table;
+ acpi_status status;
+ acpi_size tbl_size;
+ int err;
+
+ status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, table, tbl_size);
+ if (ACPI_FAILURE(status)) {
+ const char *msg = acpi_format_exception(status);
+
+ pr_err(Failed to get MADT table, %s\n, msg);
+ return;
+ }
+
+ err = gic_v2_acpi_init(table);
+ if (err)
+ pr_err(Failed to initialize GIC IRQ controller);
What will happen when you get to implement GICv3 support? Another entry
like this? Why isn't this entirely contained in the GIC driver? Do I
sound like a stuck record?
There will be another call to GICv3 init:
[...]
err = gic_v3_acpi_init(table);
if (err)
err = gic_v2_acpi_init(table);
if (err)
pr_err(Failed to initialize GIC IRQ controller);
[...]
This is the main reason I put common code here.
+
+ early_acpi_os_unmap_memory((char *)table, tbl_size);
+}
+
/*
* acpi_suspend_lowlevel() - save kernel state and suspend.
*
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 0f08dfd..c074d60 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -28,6 +28,7 @@
#include linux/irqchip.h
#include linux/seq_file.h
#include linux/ratelimit.h
+#include linux/irqchip/arm-gic-acpi.h
unsigned long irq_err_count;
@@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct
pt_regs *))
void __init init_IRQ(void)
{
irqchip_init();
+
+ if (!handle_arch_irq)
+ acpi_gic_init();
+
Why isn't this called from irqchip_init? It would seem like the logical
spot to probe an interrupt controller.
irqchip.c is OF dependent, I want to decouple these from the very
beginning.
It doesn't have to be that way, but given that ARM64 is the only
platform in the foreseeable future that will use ACPI irq setup, it
doesn't make sense to put it in there.
g.
--
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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
Hi Grant,
On 11/09/14 12:48, Grant Likely wrote:
On Tue, 02 Sep 2014 13:48:37 +0200, Tomasz Nowicki
tomasz.nowi...@linaro.org wrote:
On 01.09.2014 19:35, Marc Zyngier wrote:
On 01/09/14 15:57, Hanjun Guo wrote:
From: Tomasz Nowicki tomasz.nowi...@linaro.org
ACPI kernel uses MADT table for proper GIC initialization. It needs to
parse GIC related subtables, collect CPU interface and distributor
addresses and call driver initialization function (which is hardware
abstraction agnostic). In a similar way, FDT initialize GICv1/2.
NOTE: This commit allow to initialize GICv1/2 only.
I cannot help but notice that there is no support for KVM here. It'd be
good to add a note to that effect, so that people do not expect
virtualization support to be working when booting with ACPI.
yes, it is worth mentioning!
Signed-off-by: Tomasz Nowicki tomasz.nowi...@linaro.org
Signed-off-by: Hanjun Guo hanjun@linaro.org
---
arch/arm64/include/asm/acpi.h|2 -
arch/arm64/kernel/acpi.c | 23 +++
arch/arm64/kernel/irq.c |5 ++
drivers/irqchip/irq-gic.c| 114
++
include/linux/irqchip/arm-gic-acpi.h | 33 ++
5 files changed, 175 insertions(+), 2 deletions(-)
create mode 100644 include/linux/irqchip/arm-gic-acpi.h
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index a867467..5d2ab63 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void);
extern int (*acpi_suspend_lowlevel)(void);
#define acpi_wakeup_address 0
-#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535
-
#else
static inline bool acpi_psci_present(void) { return false; }
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 354b912..b3b82b0 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -23,6 +23,7 @@
#include linux/irqdomain.h
#include linux/bootmem.h
#include linux/smp.h
+#include linux/irqchip/arm-gic-acpi.h
#include asm/cputype.h
#include asm/cpu_ops.h
@@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
pr_err(Can't find FADT or error happened during parsing
FADT\n);
}
+void __init acpi_gic_init(void)
+{
+ struct acpi_table_header *table;
+ acpi_status status;
+ acpi_size tbl_size;
+ int err;
+
+ status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, table, tbl_size);
+ if (ACPI_FAILURE(status)) {
+ const char *msg = acpi_format_exception(status);
+
+ pr_err(Failed to get MADT table, %s\n, msg);
+ return;
+ }
+
+ err = gic_v2_acpi_init(table);
+ if (err)
+ pr_err(Failed to initialize GIC IRQ controller);
What will happen when you get to implement GICv3 support? Another entry
like this? Why isn't this entirely contained in the GIC driver? Do I
sound like a stuck record?
There will be another call to GICv3 init:
[...]
err = gic_v3_acpi_init(table);
if (err)
err = gic_v2_acpi_init(table);
if (err)
pr_err(Failed to initialize GIC IRQ controller);
[...]
This is the main reason I put common code here.
+
+ early_acpi_os_unmap_memory((char *)table, tbl_size);
+}
+
/*
* acpi_suspend_lowlevel() - save kernel state and suspend.
*
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 0f08dfd..c074d60 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -28,6 +28,7 @@
#include linux/irqchip.h
#include linux/seq_file.h
#include linux/ratelimit.h
+#include linux/irqchip/arm-gic-acpi.h
unsigned long irq_err_count;
@@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct
pt_regs *))
void __init init_IRQ(void)
{
irqchip_init();
+
+ if (!handle_arch_irq)
+ acpi_gic_init();
+
Why isn't this called from irqchip_init? It would seem like the logical
spot to probe an interrupt controller.
irqchip.c is OF dependent, I want to decouple these from the very
beginning.
It doesn't have to be that way, but given that ARM64 is the only
platform in the foreseeable future that will use ACPI irq setup, it
doesn't make sense to put it in there.
I have a different perspective. There is no reason to pollute the arch
code with something that is essentially platform specific.
irqchip_init is the logical place to probe for an irqchip, and I fail to
see the point of sticking this code somewhere else. Why would ACPI be so
special that it requires additional hooks in the arch code?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On Tue, 02 Sep 2014 14:02:31 +0100, Marc Zyngier marc.zyng...@arm.com wrote:
On 02/09/14 12:48, Tomasz Nowicki wrote:
On 01.09.2014 19:35, Marc Zyngier wrote:
On 01/09/14 15:57, Hanjun Guo wrote:
From: Tomasz Nowicki tomasz.nowi...@linaro.org
ACPI kernel uses MADT table for proper GIC initialization. It needs to
parse GIC related subtables, collect CPU interface and distributor
addresses and call driver initialization function (which is hardware
abstraction agnostic). In a similar way, FDT initialize GICv1/2.
NOTE: This commit allow to initialize GICv1/2 only.
I cannot help but notice that there is no support for KVM here. It'd be
good to add a note to that effect, so that people do not expect
virtualization support to be working when booting with ACPI.
yes, it is worth mentioning!
Signed-off-by: Tomasz Nowicki tomasz.nowi...@linaro.org
Signed-off-by: Hanjun Guo hanjun@linaro.org
---
arch/arm64/include/asm/acpi.h|2 -
arch/arm64/kernel/acpi.c | 23 +++
arch/arm64/kernel/irq.c |5 ++
drivers/irqchip/irq-gic.c| 114
++
include/linux/irqchip/arm-gic-acpi.h | 33 ++
5 files changed, 175 insertions(+), 2 deletions(-)
create mode 100644 include/linux/irqchip/arm-gic-acpi.h
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index a867467..5d2ab63 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void);
extern int (*acpi_suspend_lowlevel)(void);
#define acpi_wakeup_address 0
-#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535
-
#else
static inline bool acpi_psci_present(void) { return false; }
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 354b912..b3b82b0 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -23,6 +23,7 @@
#include linux/irqdomain.h
#include linux/bootmem.h
#include linux/smp.h
+#include linux/irqchip/arm-gic-acpi.h
#include asm/cputype.h
#include asm/cpu_ops.h
@@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
pr_err(Can't find FADT or error happened during parsing
FADT\n);
}
+void __init acpi_gic_init(void)
+{
+struct acpi_table_header *table;
+acpi_status status;
+acpi_size tbl_size;
+int err;
+
+status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, table,
tbl_size);
+if (ACPI_FAILURE(status)) {
+const char *msg = acpi_format_exception(status);
+
+pr_err(Failed to get MADT table, %s\n, msg);
+return;
+}
+
+err = gic_v2_acpi_init(table);
+if (err)
+pr_err(Failed to initialize GIC IRQ controller);
What will happen when you get to implement GICv3 support? Another entry
like this? Why isn't this entirely contained in the GIC driver? Do I
sound like a stuck record?
There will be another call to GICv3 init:
[...]
err = gic_v3_acpi_init(table);
if (err)
err = gic_v2_acpi_init(table);
if (err)
pr_err(Failed to initialize GIC IRQ controller);
[...]
This is the main reason I put common code here.
+
+early_acpi_os_unmap_memory((char *)table, tbl_size);
+}
+
/*
* acpi_suspend_lowlevel() - save kernel state and suspend.
*
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 0f08dfd..c074d60 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -28,6 +28,7 @@
#include linux/irqchip.h
#include linux/seq_file.h
#include linux/ratelimit.h
+#include linux/irqchip/arm-gic-acpi.h
unsigned long irq_err_count;
@@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct
pt_regs *))
void __init init_IRQ(void)
{
irqchip_init();
+
+if (!handle_arch_irq)
+acpi_gic_init();
+
Why isn't this called from irqchip_init? It would seem like the logical
spot to probe an interrupt controller.
irqchip.c is OF dependent, I want to decouple these from the very
beginning.
No. irqchip.c is not OF dependent, it is just that DT is the only thing
we support so far. I don't think duplicating the kernel infrastructure
because we're different is the right way.
There is no reason for your probing structure to be artificially
different (you're parsing the same information, at the same time). Just
put in place a similar probing mechanism, and this will look a lot better.
if (!handle_arch_irq)
panic(No interrupt controller found.);
}
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4b959e6..85cbf43 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -33,12 +33,14 @@
Re: [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 09/03/2014 02:42 PM, Arnd Bergmann wrote:
> On Monday 01 September 2014 22:57:51 Hanjun Guo wrote:
>> + /* Collect CPU base addresses */
>> + count = acpi_parse_entries(sizeof(struct acpi_table_madt),
>> + gic_acpi_parse_madt_cpu, table,
>> + ACPI_MADT_TYPE_GENERIC_INTERRUPT,
>> + ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
>> + if (count < 0) {
>> + pr_err("Error during GICC entries parsing\n");
>> + return -EFAULT;
>> + } else if (!count) {
>> + /* No GICC entries provided, use address from MADT header */
>> + struct acpi_table_madt *madt = (struct acpi_table_madt
>> *)table;
>> +
>> + if (!madt->address)
>> + return -EFAULT;
>> +
>> + cpu_phy_base = (u64)madt->address;
>> + }
>
> After I read through ACPI-5.1 section 5.2.12.14, I wonder if this is the
> best way to treat a missing ACPI_MADT_TYPE_GENERIC_INTERRUPT table.
>
> Do we expect to see those in practice? It seems like using the x86 local
> APIC address as a fallback for the GIC address is not something we
> should do unless we absolutely have to support a system that doesn't
> have the GIC table.
All GICv2 based ACPI systems should define GICCs for the CPU interfaces.
Jon.
--
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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 09/03/2014 10:57 AM, Arnd Bergmann wrote: > On Wednesday 03 September 2014 11:26:14 Tomasz Nowicki wrote: > In particular, the ACPI tables describing the irqchip have no way to > identify the GIC at all, if I read the spec correctly, you have to > parse the tables, ioremap the registers and then read the ID to know > if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any "device" > for the GIC that a hypothetical probe function would be based on. (aside) I have already noticed this and am separately raising a request to have this dealt with in the specification at a later time. I believe it's fairly contrived, since I don't think it'll be at all common to have a GICv3/v4 IP implementation that has a CPU interface defined. The problem (not now, but in later implementations that actually have GICv3/v4 hardware is making assumptions since even a GICv3 or GICv4 system could implement a legacy compatibility mode in which the memory register interface is defined and mappable so you would be valid in having defined memory addresses in the MADT linked structures then. Jon. -- 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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 09/03/2014 06:30 AM, Marc Zyngier wrote: > On 02/09/14 16:45, Hanjun Guo wrote: >> This value is for max processors entries in MADT, and we will use it to scan >> MADT >> for SMP/GIC Init, I just make it big enough for GICv3/4. since ACPI core >> will stop >> scan MADT if the real numbers of processors entries are reached no matter >> how big ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES is, I think we can just >> define a number big enough then it will work (x86 and ia64 did the same >> thing). > > Also, with GICv3++, there is no such thing as a memory-mapped CPU > interface anymore. What you get is a bunch of redistributors (one per > CPU). I assume what you have here actually describe the redistributors, > and its name should reflect that. (though you could have a GICv3/v4 system providing a legacy GICv2(m) compatibility mode having the CPU memory interfaces still defined) Jon. -- 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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 09/01/2014 01:35 PM, Marc Zyngier wrote: > On 01/09/14 15:57, Hanjun Guo wrote: >> From: Tomasz Nowicki >> >> ACPI kernel uses MADT table for proper GIC initialization. It needs to >> parse GIC related subtables, collect CPU interface and distributor >> addresses and call driver initialization function (which is hardware >> abstraction agnostic). In a similar way, FDT initialize GICv1/2. >> >> NOTE: This commit allow to initialize GICv1/2 only. > > I cannot help but notice that there is no support for KVM here. It'd be > good to add a note to that effect, so that people do not expect > virtualization support to be working when booting with ACPI. Wei Huang within Red Hat is currently beginning the process of bringing up KVM support under ACPI on real hardware. This will be fixed asap. Jon. -- 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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 09/01/2014 01:35 PM, Marc Zyngier wrote: On 01/09/14 15:57, Hanjun Guo wrote: From: Tomasz Nowicki tomasz.nowi...@linaro.org ACPI kernel uses MADT table for proper GIC initialization. It needs to parse GIC related subtables, collect CPU interface and distributor addresses and call driver initialization function (which is hardware abstraction agnostic). In a similar way, FDT initialize GICv1/2. NOTE: This commit allow to initialize GICv1/2 only. I cannot help but notice that there is no support for KVM here. It'd be good to add a note to that effect, so that people do not expect virtualization support to be working when booting with ACPI. Wei Huang within Red Hat is currently beginning the process of bringing up KVM support under ACPI on real hardware. This will be fixed asap. Jon. -- 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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 09/03/2014 06:30 AM, Marc Zyngier wrote: On 02/09/14 16:45, Hanjun Guo wrote: This value is for max processors entries in MADT, and we will use it to scan MADT for SMP/GIC Init, I just make it big enough for GICv3/4. since ACPI core will stop scan MADT if the real numbers of processors entries are reached no matter how big ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES is, I think we can just define a number big enough then it will work (x86 and ia64 did the same thing). Also, with GICv3++, there is no such thing as a memory-mapped CPU interface anymore. What you get is a bunch of redistributors (one per CPU). I assume what you have here actually describe the redistributors, and its name should reflect that. (though you could have a GICv3/v4 system providing a legacy GICv2(m) compatibility mode having the CPU memory interfaces still defined) Jon. -- 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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 09/03/2014 10:57 AM, Arnd Bergmann wrote: On Wednesday 03 September 2014 11:26:14 Tomasz Nowicki wrote: In particular, the ACPI tables describing the irqchip have no way to identify the GIC at all, if I read the spec correctly, you have to parse the tables, ioremap the registers and then read the ID to know if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any device for the GIC that a hypothetical probe function would be based on. (aside) I have already noticed this and am separately raising a request to have this dealt with in the specification at a later time. I believe it's fairly contrived, since I don't think it'll be at all common to have a GICv3/v4 IP implementation that has a CPU interface defined. The problem (not now, but in later implementations that actually have GICv3/v4 hardware is making assumptions since even a GICv3 or GICv4 system could implement a legacy compatibility mode in which the memory register interface is defined and mappable so you would be valid in having defined memory addresses in the MADT linked structures then. Jon. -- 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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 09/03/2014 02:42 PM, Arnd Bergmann wrote:
On Monday 01 September 2014 22:57:51 Hanjun Guo wrote:
+ /* Collect CPU base addresses */
+ count = acpi_parse_entries(sizeof(struct acpi_table_madt),
+ gic_acpi_parse_madt_cpu, table,
+ ACPI_MADT_TYPE_GENERIC_INTERRUPT,
+ ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
+ if (count 0) {
+ pr_err(Error during GICC entries parsing\n);
+ return -EFAULT;
+ } else if (!count) {
+ /* No GICC entries provided, use address from MADT header */
+ struct acpi_table_madt *madt = (struct acpi_table_madt
*)table;
+
+ if (!madt-address)
+ return -EFAULT;
+
+ cpu_phy_base = (u64)madt-address;
+ }
After I read through ACPI-5.1 section 5.2.12.14, I wonder if this is the
best way to treat a missing ACPI_MADT_TYPE_GENERIC_INTERRUPT table.
Do we expect to see those in practice? It seems like using the x86 local
APIC address as a fallback for the GIC address is not something we
should do unless we absolutely have to support a system that doesn't
have the GIC table.
All GICv2 based ACPI systems should define GICCs for the CPU interfaces.
Jon.
--
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: [Linaro-acpi] [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 05.09.2014 12:39, Marc Zyngier wrote:
On 05/09/14 11:13, Arnd Bergmann wrote:
On Friday 05 September 2014 10:47:30 Marc Zyngier wrote:
I still prefer being explicit here for the same reason I mentioned earlier:
I want it to be very clear that we don't support arbitrary irqchips other
than the ones in the APCI specification. The infrastructure exists on DT
because we have to support a large number of incompatible irqchips.
I'm not suggesting that we should support more than the ACPI spec says.
And that's certainly the whole point of a spec, isn't it? ACPI says what
we support, and we're not going any further. I'm just saying that we
shouldn't make the maintenance burden heavier, and the code nothing
short of disgusting. Using our current infrastructure doesn't mean we're
going to support GIcv2.37.
Ok
In particular, the ACPI tables describing the irqchip have no way to
identify the GIC at all, if I read the spec correctly, you have to
parse the tables, ioremap the registers and then read the ID to know
if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any "device"
for the GIC that a hypothetical probe function would be based on.
This is not the way I read the spec. Table 5-46 (Interrupt Controller
Structure) tells you if you have a CPU interface (GICv1/v2) or a
redistributor (GICv3/v4). That's enough to know whether or not you
should carry on probing a particular controller.
Ah, good. I missed that.
The various GIC versions don't really have a unified memory map anyway
(well, none that you can rely on), and you really have to rely on ACPI
to tell you what you have.
So we are back to needing to support two different irqchip drivers
(v1/v2/v2m and v3/v4), instead of five or more, right?
As long as we make sure that the variants are directly probed from the
"canonical" driver (v2m from v2, ITS and v4 from v3), then we're OK.
Yes, that is our intention.
It does seem wrong to parse the tables in the irq-gic.c file though:
that part can well be common across the various gic versions and then
call into either irq-gic.c or irq-gic-v3.c for the version specific
parts. Whether we put that common code into drivers/irqchip/irqchip.c,
drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or
drivers/acpi/irq-gic.c I don't care at all.
I don't think so you can make that common code very easily. The
information required by both drivers is organized differently.
If it was, I'd have done that for the DT binding.
I see, and that's also what Tomasz just explained. So can we just
have one an irqchip_init() function doing this:?
if (dt)
of_irq_init(__irqchip_of_table);
else if (acpi) {
read cpu-interface and redistributor address from acpi tables
if (cpu-interface)
gic_v2_acpi_init(table);
else if(redistributor)
gic_v3_acpi_init(table)
}
That'd be better, but that only considers the basic architecture. GICv2m
and GICv3's ITS bring additional complexity (and their own table
parsing). At that stage, I'm not sure how much commonality we're left with.
Right, my local ACPI GICv3 support assumes to probe ITS from
gic_v3_acpi_init() body.
Tomasz
--
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: [Linaro-acpi] [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 05/09/14 11:13, Arnd Bergmann wrote:
> On Friday 05 September 2014 10:47:30 Marc Zyngier wrote:
>>>
>>> I still prefer being explicit here for the same reason I mentioned earlier:
>>> I want it to be very clear that we don't support arbitrary irqchips other
>>> than the ones in the APCI specification. The infrastructure exists on DT
>>> because we have to support a large number of incompatible irqchips.
>>
>> I'm not suggesting that we should support more than the ACPI spec says.
>> And that's certainly the whole point of a spec, isn't it? ACPI says what
>> we support, and we're not going any further. I'm just saying that we
>> shouldn't make the maintenance burden heavier, and the code nothing
>> short of disgusting. Using our current infrastructure doesn't mean we're
>> going to support GIcv2.37.
>
> Ok
>
>>> In particular, the ACPI tables describing the irqchip have no way to
>>> identify the GIC at all, if I read the spec correctly, you have to
>>> parse the tables, ioremap the registers and then read the ID to know
>>> if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any "device"
>>> for the GIC that a hypothetical probe function would be based on.
>>
>> This is not the way I read the spec. Table 5-46 (Interrupt Controller
>> Structure) tells you if you have a CPU interface (GICv1/v2) or a
>> redistributor (GICv3/v4). That's enough to know whether or not you
>> should carry on probing a particular controller.
>
> Ah, good. I missed that.
>
>> The various GIC versions don't really have a unified memory map anyway
>> (well, none that you can rely on), and you really have to rely on ACPI
>> to tell you what you have.
>
> So we are back to needing to support two different irqchip drivers
> (v1/v2/v2m and v3/v4), instead of five or more, right?
As long as we make sure that the variants are directly probed from the
"canonical" driver (v2m from v2, ITS and v4 from v3), then we're OK.
>>> It does seem wrong to parse the tables in the irq-gic.c file though:
>>> that part can well be common across the various gic versions and then
>>> call into either irq-gic.c or irq-gic-v3.c for the version specific
>>> parts. Whether we put that common code into drivers/irqchip/irqchip.c,
>>> drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or
>>> drivers/acpi/irq-gic.c I don't care at all.
>>
>> I don't think so you can make that common code very easily. The
>> information required by both drivers is organized differently.
>> If it was, I'd have done that for the DT binding.
>
> I see, and that's also what Tomasz just explained. So can we just
> have one an irqchip_init() function doing this:?
>
> if (dt)
> of_irq_init(__irqchip_of_table);
> else if (acpi) {
> read cpu-interface and redistributor address from acpi tables
> if (cpu-interface)
> gic_v2_acpi_init(table);
> else if(redistributor)
> gic_v3_acpi_init(table)
> }
That'd be better, but that only considers the basic architecture. GICv2m
and GICv3's ITS bring additional complexity (and their own table
parsing). At that stage, I'm not sure how much commonality we're left with.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
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: [Linaro-acpi] [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 05.09.2014 12:13, Arnd Bergmann wrote:
On Friday 05 September 2014 10:47:30 Marc Zyngier wrote:
I still prefer being explicit here for the same reason I mentioned earlier:
I want it to be very clear that we don't support arbitrary irqchips other
than the ones in the APCI specification. The infrastructure exists on DT
because we have to support a large number of incompatible irqchips.
I'm not suggesting that we should support more than the ACPI spec says.
And that's certainly the whole point of a spec, isn't it? ACPI says what
we support, and we're not going any further. I'm just saying that we
shouldn't make the maintenance burden heavier, and the code nothing
short of disgusting. Using our current infrastructure doesn't mean we're
going to support GIcv2.37.
Ok
In particular, the ACPI tables describing the irqchip have no way to
identify the GIC at all, if I read the spec correctly, you have to
parse the tables, ioremap the registers and then read the ID to know
if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any "device"
for the GIC that a hypothetical probe function would be based on.
This is not the way I read the spec. Table 5-46 (Interrupt Controller
Structure) tells you if you have a CPU interface (GICv1/v2) or a
redistributor (GICv3/v4). That's enough to know whether or not you
should carry on probing a particular controller.
Ah, good. I missed that.
The various GIC versions don't really have a unified memory map anyway
(well, none that you can rely on), and you really have to rely on ACPI
to tell you what you have.
So we are back to needing to support two different irqchip drivers
(v1/v2/v2m and v3/v4), instead of five or more, right?
It does seem wrong to parse the tables in the irq-gic.c file though:
that part can well be common across the various gic versions and then
call into either irq-gic.c or irq-gic-v3.c for the version specific
parts. Whether we put that common code into drivers/irqchip/irqchip.c,
drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or
drivers/acpi/irq-gic.c I don't care at all.
I don't think so you can make that common code very easily. The
information required by both drivers is organized differently.
If it was, I'd have done that for the DT binding.
I see, and that's also what Tomasz just explained. So can we just
have one an irqchip_init() function doing this:?
if (dt)
of_irq_init(__irqchip_of_table);
else if (acpi) {
read cpu-interface and redistributor address from acpi tables
if (cpu-interface)
gic_v2_acpi_init(table);
else if(redistributor)
gic_v3_acpi_init(table)
}
It is pretty much the same as:
if (dt)
of_irq_init(__irqchip_of_table);
else if (acpi) {
err = gic_v3_acpi_init(table) {
read redistributor address from acpi tables
[...]
}
if (err) {
err = gic_v2_acpi_init(table) {
read cpu-interface address from acpi tables
[...]
}
}
}
What basically is my current implementation (well, without GICv3 support
but that will be next step).
Tomasz
--
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: [Linaro-acpi] [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On Friday 05 September 2014 10:47:30 Marc Zyngier wrote:
> >
> > I still prefer being explicit here for the same reason I mentioned earlier:
> > I want it to be very clear that we don't support arbitrary irqchips other
> > than the ones in the APCI specification. The infrastructure exists on DT
> > because we have to support a large number of incompatible irqchips.
>
> I'm not suggesting that we should support more than the ACPI spec says.
> And that's certainly the whole point of a spec, isn't it? ACPI says what
> we support, and we're not going any further. I'm just saying that we
> shouldn't make the maintenance burden heavier, and the code nothing
> short of disgusting. Using our current infrastructure doesn't mean we're
> going to support GIcv2.37.
Ok
> > In particular, the ACPI tables describing the irqchip have no way to
> > identify the GIC at all, if I read the spec correctly, you have to
> > parse the tables, ioremap the registers and then read the ID to know
> > if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any "device"
> > for the GIC that a hypothetical probe function would be based on.
>
> This is not the way I read the spec. Table 5-46 (Interrupt Controller
> Structure) tells you if you have a CPU interface (GICv1/v2) or a
> redistributor (GICv3/v4). That's enough to know whether or not you
> should carry on probing a particular controller.
Ah, good. I missed that.
> The various GIC versions don't really have a unified memory map anyway
> (well, none that you can rely on), and you really have to rely on ACPI
> to tell you what you have.
So we are back to needing to support two different irqchip drivers
(v1/v2/v2m and v3/v4), instead of five or more, right?
> > It does seem wrong to parse the tables in the irq-gic.c file though:
> > that part can well be common across the various gic versions and then
> > call into either irq-gic.c or irq-gic-v3.c for the version specific
> > parts. Whether we put that common code into drivers/irqchip/irqchip.c,
> > drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or
> > drivers/acpi/irq-gic.c I don't care at all.
>
> I don't think so you can make that common code very easily. The
> information required by both drivers is organized differently.
> If it was, I'd have done that for the DT binding.
I see, and that's also what Tomasz just explained. So can we just
have one an irqchip_init() function doing this:?
if (dt)
of_irq_init(__irqchip_of_table);
else if (acpi) {
read cpu-interface and redistributor address from acpi tables
if (cpu-interface)
gic_v2_acpi_init(table);
else if(redistributor)
gic_v3_acpi_init(table)
}
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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 03/09/14 15:57, Arnd Bergmann wrote:
> On Wednesday 03 September 2014 11:26:14 Tomasz Nowicki wrote:
>> On 02.09.2014 15:02, Marc Zyngier wrote:
>>> On 02/09/14 12:48, Tomasz Nowicki wrote:
On 01.09.2014 19:35, Marc Zyngier wrote:
>> @@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct
>> pt_regs *))
>>void __init init_IRQ(void)
>>{
>> irqchip_init();
>> +
>> +if (!handle_arch_irq)
>> +acpi_gic_init();
>> +
>
> Why isn't this called from irqchip_init? It would seem like the logical
> spot to probe an interrupt controller.
irqchip.c is OF dependent, I want to decouple these from the very
beginning.
>>>
>>> No. irqchip.c is not OF dependent, it is just that DT is the only thing
>>> we support so far. I don't think duplicating the kernel infrastructure
>>> "because we're different" is the right way.
>>>
>>> There is no reason for your probing structure to be artificially
>>> different (you're parsing the same information, at the same time). Just
>>> put in place a similar probing mechanism, and this will look a lot better.
>
>
Having only GICv2, it would work. Considering we would do the same for
GICv3 (arm-gic-v3.h) there will be register name conflicts for both
headers inclusion:
[...]
#include
#include
[...]
err = gic_v3_acpi_init(table);
if (err)
err = gic_v2_acpi_init(table);
if (err)
pr_err("Failed to initialize GIC IRQ controller");
[...]
So instead of changing register names prefix, I choose new header will
be less painfully.
>>>
>>> Yes, and this is exactly why I pushed back on that last time. I'll
>>> continue saying that interrupt controllers should be self-probing, with
>>> ACPI as they are with DT.
>>>
>>> Even with the restrictions of ACPI and SBSA, we end-up with at least 2
>>> main families of interrupt controllers (GICv2 and GICv3), both with a
>>> number of "interesting" variations (GICv2m and GICv4, to only mention
>>> those I'm directly involved with).
>>>
>>> I can safely predict that the above will become a tangled mess within 18
>>> months, and the idea of littering the arch code with a bunch of
>>> hardcoded "if (blah())" doesn't fill me with joy and confidence.
>>>
>>> In summary: we have the infrastructure already, just use it.
>>
>> We had that discussion but I see we still don't have consensus here. It
>> would be good to know our direction before we prepare next patch
>> version. Arnd any comments on this from you side?
>
> I still prefer being explicit here for the same reason I mentioned earlier:
> I want it to be very clear that we don't support arbitrary irqchips other
> than the ones in the APCI specification. The infrastructure exists on DT
> because we have to support a large number of incompatible irqchips.
I'm not suggesting that we should support more than the ACPI spec says.
And that's certainly the whole point of a spec, isn't it? ACPI says what
we support, and we're not going any further. I'm just saying that we
shouldn't make the maintenance burden heavier, and the code nothing
short of disgusting. Using our current infrastructure doesn't mean we're
going to support GIcv2.37.
> In particular, the ACPI tables describing the irqchip have no way to
> identify the GIC at all, if I read the spec correctly, you have to
> parse the tables, ioremap the registers and then read the ID to know
> if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any "device"
> for the GIC that a hypothetical probe function would be based on.
This is not the way I read the spec. Table 5-46 (Interrupt Controller
Structure) tells you if you have a CPU interface (GICv1/v2) or a
redistributor (GICv3/v4). That's enough to know whether or not you
should carry on probing a particular controller.
The various GIC versions don't really have a unified memory map anyway
(well, none that you can rely on), and you really have to rely on ACPI
to tell you what you have.
> It does seem wrong to parse the tables in the irq-gic.c file though:
> that part can well be common across the various gic versions and then
> call into either irq-gic.c or irq-gic-v3.c for the version specific
> parts. Whether we put that common code into drivers/irqchip/irqchip.c,
> drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or
> drivers/acpi/irq-gic.c I don't care at all.
I don't think so you can make that common code very easily. The
information required by both drivers is organized differently.
If it was, I'd have done that for the DT binding.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
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
Re: [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 03.09.2014 16:57, Arnd Bergmann wrote:
On Wednesday 03 September 2014 11:26:14 Tomasz Nowicki wrote:
On 02.09.2014 15:02, Marc Zyngier wrote:
On 02/09/14 12:48, Tomasz Nowicki wrote:
On 01.09.2014 19:35, Marc Zyngier wrote:
@@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs
*))
void __init init_IRQ(void)
{
irqchip_init();
+
+if (!handle_arch_irq)
+acpi_gic_init();
+
Why isn't this called from irqchip_init? It would seem like the logical
spot to probe an interrupt controller.
irqchip.c is OF dependent, I want to decouple these from the very
beginning.
No. irqchip.c is not OF dependent, it is just that DT is the only thing
we support so far. I don't think duplicating the kernel infrastructure
"because we're different" is the right way.
There is no reason for your probing structure to be artificially
different (you're parsing the same information, at the same time). Just
put in place a similar probing mechanism, and this will look a lot better.
Having only GICv2, it would work. Considering we would do the same for
GICv3 (arm-gic-v3.h) there will be register name conflicts for both
headers inclusion:
[...]
#include
#include
[...]
err = gic_v3_acpi_init(table);
if (err)
err = gic_v2_acpi_init(table);
if (err)
pr_err("Failed to initialize GIC IRQ controller");
[...]
So instead of changing register names prefix, I choose new header will
be less painfully.
Yes, and this is exactly why I pushed back on that last time. I'll
continue saying that interrupt controllers should be self-probing, with
ACPI as they are with DT.
Even with the restrictions of ACPI and SBSA, we end-up with at least 2
main families of interrupt controllers (GICv2 and GICv3), both with a
number of "interesting" variations (GICv2m and GICv4, to only mention
those I'm directly involved with).
I can safely predict that the above will become a tangled mess within 18
months, and the idea of littering the arch code with a bunch of
hardcoded "if (blah())" doesn't fill me with joy and confidence.
In summary: we have the infrastructure already, just use it.
We had that discussion but I see we still don't have consensus here. It
would be good to know our direction before we prepare next patch
version. Arnd any comments on this from you side?
I still prefer being explicit here for the same reason I mentioned earlier:
I want it to be very clear that we don't support arbitrary irqchips other
than the ones in the APCI specification. The infrastructure exists on DT
because we have to support a large number of incompatible irqchips.
In particular, the ACPI tables describing the irqchip have no way to
identify the GIC at all, if I read the spec correctly, you have to
parse the tables, ioremap the registers and then read the ID to know
if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any "device"
for the GIC that a hypothetical probe function would be based on.
Yes, it is just one table with bunch of different subtables types. Since
GIC identification register is at different offset across GICv1/2 and
GICv3, the probe logic seems to be slightly different. IMO, we should
look into our MADT and try GICv3 fist and then GICv2, that means
presence of redistributor entries suggests GICv3/4. Its absence means we
need to look for GICC subtables and then call GICv2 init.
It does seem wrong to parse the tables in the irq-gic.c file though:
that part can well be common across the various gic versions and then
call into either irq-gic.c or irq-gic-v3.c for the version specific
parts. Whether we put that common code into drivers/irqchip/irqchip.c,
drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or
drivers/acpi/irq-gic.c I don't care at all.
We already had tried making MADT parser common for all GICs in the
separate file (outside GIC driver), it did not end up well. In the light
of above comment, the logic will be complex and some GIC local strutures
need to be exported:
1. Check redistributor entries.
2. If none exist, as fallback, check GICC entries, there are
redistributor base address assigned to CPU. Unfortunately it has no
register set size so ioremap only two pages (RD + SGI) check if we
support VLPI and extend ioremap if true. We cannot operate on GIC
register outside GIC driver so that requires another exported GICv3
function.
3. Jump to redistributor part if we are OK so far, if not, then we start
playing with GICC enries and look for cpu base addresses.
Honestly, apart from distributor parsing logic, there is no common code.
As you can see, current gic_v2_acpi_init() logic is quite easy to
follow. And gic_v3_acpi_init() inside of GICv3 driver is easy too. I
might not see other solutions but I am open to other suggestions, so
please correct me in that case.
Regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body
Re: [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 03.09.2014 16:57, Arnd Bergmann wrote:
On Wednesday 03 September 2014 11:26:14 Tomasz Nowicki wrote:
On 02.09.2014 15:02, Marc Zyngier wrote:
On 02/09/14 12:48, Tomasz Nowicki wrote:
On 01.09.2014 19:35, Marc Zyngier wrote:
@@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs
*))
void __init init_IRQ(void)
{
irqchip_init();
+
+if (!handle_arch_irq)
+acpi_gic_init();
+
Why isn't this called from irqchip_init? It would seem like the logical
spot to probe an interrupt controller.
irqchip.c is OF dependent, I want to decouple these from the very
beginning.
No. irqchip.c is not OF dependent, it is just that DT is the only thing
we support so far. I don't think duplicating the kernel infrastructure
because we're different is the right way.
There is no reason for your probing structure to be artificially
different (you're parsing the same information, at the same time). Just
put in place a similar probing mechanism, and this will look a lot better.
Having only GICv2, it would work. Considering we would do the same for
GICv3 (arm-gic-v3.h) there will be register name conflicts for both
headers inclusion:
[...]
#include linux/irqchip/arm-gic.h
#include linux/irqchip/arm-gic-v3.h
[...]
err = gic_v3_acpi_init(table);
if (err)
err = gic_v2_acpi_init(table);
if (err)
pr_err(Failed to initialize GIC IRQ controller);
[...]
So instead of changing register names prefix, I choose new header will
be less painfully.
Yes, and this is exactly why I pushed back on that last time. I'll
continue saying that interrupt controllers should be self-probing, with
ACPI as they are with DT.
Even with the restrictions of ACPI and SBSA, we end-up with at least 2
main families of interrupt controllers (GICv2 and GICv3), both with a
number of interesting variations (GICv2m and GICv4, to only mention
those I'm directly involved with).
I can safely predict that the above will become a tangled mess within 18
months, and the idea of littering the arch code with a bunch of
hardcoded if (blah()) doesn't fill me with joy and confidence.
In summary: we have the infrastructure already, just use it.
We had that discussion but I see we still don't have consensus here. It
would be good to know our direction before we prepare next patch
version. Arnd any comments on this from you side?
I still prefer being explicit here for the same reason I mentioned earlier:
I want it to be very clear that we don't support arbitrary irqchips other
than the ones in the APCI specification. The infrastructure exists on DT
because we have to support a large number of incompatible irqchips.
In particular, the ACPI tables describing the irqchip have no way to
identify the GIC at all, if I read the spec correctly, you have to
parse the tables, ioremap the registers and then read the ID to know
if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any device
for the GIC that a hypothetical probe function would be based on.
Yes, it is just one table with bunch of different subtables types. Since
GIC identification register is at different offset across GICv1/2 and
GICv3, the probe logic seems to be slightly different. IMO, we should
look into our MADT and try GICv3 fist and then GICv2, that means
presence of redistributor entries suggests GICv3/4. Its absence means we
need to look for GICC subtables and then call GICv2 init.
It does seem wrong to parse the tables in the irq-gic.c file though:
that part can well be common across the various gic versions and then
call into either irq-gic.c or irq-gic-v3.c for the version specific
parts. Whether we put that common code into drivers/irqchip/irqchip.c,
drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or
drivers/acpi/irq-gic.c I don't care at all.
We already had tried making MADT parser common for all GICs in the
separate file (outside GIC driver), it did not end up well. In the light
of above comment, the logic will be complex and some GIC local strutures
need to be exported:
1. Check redistributor entries.
2. If none exist, as fallback, check GICC entries, there are
redistributor base address assigned to CPU. Unfortunately it has no
register set size so ioremap only two pages (RD + SGI) check if we
support VLPI and extend ioremap if true. We cannot operate on GIC
register outside GIC driver so that requires another exported GICv3
function.
3. Jump to redistributor part if we are OK so far, if not, then we start
playing with GICC enries and look for cpu base addresses.
Honestly, apart from distributor parsing logic, there is no common code.
As you can see, current gic_v2_acpi_init() logic is quite easy to
follow. And gic_v3_acpi_init() inside of GICv3 driver is easy too. I
might not see other solutions but I am open to other suggestions, so
please correct me in that case.
Regards,
Tomasz
--
To unsubscribe from this list: send the line
Re: [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 03/09/14 15:57, Arnd Bergmann wrote:
On Wednesday 03 September 2014 11:26:14 Tomasz Nowicki wrote:
On 02.09.2014 15:02, Marc Zyngier wrote:
On 02/09/14 12:48, Tomasz Nowicki wrote:
On 01.09.2014 19:35, Marc Zyngier wrote:
@@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct
pt_regs *))
void __init init_IRQ(void)
{
irqchip_init();
+
+if (!handle_arch_irq)
+acpi_gic_init();
+
Why isn't this called from irqchip_init? It would seem like the logical
spot to probe an interrupt controller.
irqchip.c is OF dependent, I want to decouple these from the very
beginning.
No. irqchip.c is not OF dependent, it is just that DT is the only thing
we support so far. I don't think duplicating the kernel infrastructure
because we're different is the right way.
There is no reason for your probing structure to be artificially
different (you're parsing the same information, at the same time). Just
put in place a similar probing mechanism, and this will look a lot better.
Having only GICv2, it would work. Considering we would do the same for
GICv3 (arm-gic-v3.h) there will be register name conflicts for both
headers inclusion:
[...]
#include linux/irqchip/arm-gic.h
#include linux/irqchip/arm-gic-v3.h
[...]
err = gic_v3_acpi_init(table);
if (err)
err = gic_v2_acpi_init(table);
if (err)
pr_err(Failed to initialize GIC IRQ controller);
[...]
So instead of changing register names prefix, I choose new header will
be less painfully.
Yes, and this is exactly why I pushed back on that last time. I'll
continue saying that interrupt controllers should be self-probing, with
ACPI as they are with DT.
Even with the restrictions of ACPI and SBSA, we end-up with at least 2
main families of interrupt controllers (GICv2 and GICv3), both with a
number of interesting variations (GICv2m and GICv4, to only mention
those I'm directly involved with).
I can safely predict that the above will become a tangled mess within 18
months, and the idea of littering the arch code with a bunch of
hardcoded if (blah()) doesn't fill me with joy and confidence.
In summary: we have the infrastructure already, just use it.
We had that discussion but I see we still don't have consensus here. It
would be good to know our direction before we prepare next patch
version. Arnd any comments on this from you side?
I still prefer being explicit here for the same reason I mentioned earlier:
I want it to be very clear that we don't support arbitrary irqchips other
than the ones in the APCI specification. The infrastructure exists on DT
because we have to support a large number of incompatible irqchips.
I'm not suggesting that we should support more than the ACPI spec says.
And that's certainly the whole point of a spec, isn't it? ACPI says what
we support, and we're not going any further. I'm just saying that we
shouldn't make the maintenance burden heavier, and the code nothing
short of disgusting. Using our current infrastructure doesn't mean we're
going to support GIcv2.37.
In particular, the ACPI tables describing the irqchip have no way to
identify the GIC at all, if I read the spec correctly, you have to
parse the tables, ioremap the registers and then read the ID to know
if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any device
for the GIC that a hypothetical probe function would be based on.
This is not the way I read the spec. Table 5-46 (Interrupt Controller
Structure) tells you if you have a CPU interface (GICv1/v2) or a
redistributor (GICv3/v4). That's enough to know whether or not you
should carry on probing a particular controller.
The various GIC versions don't really have a unified memory map anyway
(well, none that you can rely on), and you really have to rely on ACPI
to tell you what you have.
It does seem wrong to parse the tables in the irq-gic.c file though:
that part can well be common across the various gic versions and then
call into either irq-gic.c or irq-gic-v3.c for the version specific
parts. Whether we put that common code into drivers/irqchip/irqchip.c,
drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or
drivers/acpi/irq-gic.c I don't care at all.
I don't think so you can make that common code very easily. The
information required by both drivers is organized differently.
If it was, I'd have done that for the DT binding.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
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: [Linaro-acpi] [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On Friday 05 September 2014 10:47:30 Marc Zyngier wrote:
I still prefer being explicit here for the same reason I mentioned earlier:
I want it to be very clear that we don't support arbitrary irqchips other
than the ones in the APCI specification. The infrastructure exists on DT
because we have to support a large number of incompatible irqchips.
I'm not suggesting that we should support more than the ACPI spec says.
And that's certainly the whole point of a spec, isn't it? ACPI says what
we support, and we're not going any further. I'm just saying that we
shouldn't make the maintenance burden heavier, and the code nothing
short of disgusting. Using our current infrastructure doesn't mean we're
going to support GIcv2.37.
Ok
In particular, the ACPI tables describing the irqchip have no way to
identify the GIC at all, if I read the spec correctly, you have to
parse the tables, ioremap the registers and then read the ID to know
if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any device
for the GIC that a hypothetical probe function would be based on.
This is not the way I read the spec. Table 5-46 (Interrupt Controller
Structure) tells you if you have a CPU interface (GICv1/v2) or a
redistributor (GICv3/v4). That's enough to know whether or not you
should carry on probing a particular controller.
Ah, good. I missed that.
The various GIC versions don't really have a unified memory map anyway
(well, none that you can rely on), and you really have to rely on ACPI
to tell you what you have.
So we are back to needing to support two different irqchip drivers
(v1/v2/v2m and v3/v4), instead of five or more, right?
It does seem wrong to parse the tables in the irq-gic.c file though:
that part can well be common across the various gic versions and then
call into either irq-gic.c or irq-gic-v3.c for the version specific
parts. Whether we put that common code into drivers/irqchip/irqchip.c,
drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or
drivers/acpi/irq-gic.c I don't care at all.
I don't think so you can make that common code very easily. The
information required by both drivers is organized differently.
If it was, I'd have done that for the DT binding.
I see, and that's also what Tomasz just explained. So can we just
have one an irqchip_init() function doing this:?
if (dt)
of_irq_init(__irqchip_of_table);
else if (acpi) {
read cpu-interface and redistributor address from acpi tables
if (cpu-interface)
gic_v2_acpi_init(table);
else if(redistributor)
gic_v3_acpi_init(table)
}
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: [Linaro-acpi] [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 05.09.2014 12:13, Arnd Bergmann wrote:
On Friday 05 September 2014 10:47:30 Marc Zyngier wrote:
I still prefer being explicit here for the same reason I mentioned earlier:
I want it to be very clear that we don't support arbitrary irqchips other
than the ones in the APCI specification. The infrastructure exists on DT
because we have to support a large number of incompatible irqchips.
I'm not suggesting that we should support more than the ACPI spec says.
And that's certainly the whole point of a spec, isn't it? ACPI says what
we support, and we're not going any further. I'm just saying that we
shouldn't make the maintenance burden heavier, and the code nothing
short of disgusting. Using our current infrastructure doesn't mean we're
going to support GIcv2.37.
Ok
In particular, the ACPI tables describing the irqchip have no way to
identify the GIC at all, if I read the spec correctly, you have to
parse the tables, ioremap the registers and then read the ID to know
if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any device
for the GIC that a hypothetical probe function would be based on.
This is not the way I read the spec. Table 5-46 (Interrupt Controller
Structure) tells you if you have a CPU interface (GICv1/v2) or a
redistributor (GICv3/v4). That's enough to know whether or not you
should carry on probing a particular controller.
Ah, good. I missed that.
The various GIC versions don't really have a unified memory map anyway
(well, none that you can rely on), and you really have to rely on ACPI
to tell you what you have.
So we are back to needing to support two different irqchip drivers
(v1/v2/v2m and v3/v4), instead of five or more, right?
It does seem wrong to parse the tables in the irq-gic.c file though:
that part can well be common across the various gic versions and then
call into either irq-gic.c or irq-gic-v3.c for the version specific
parts. Whether we put that common code into drivers/irqchip/irqchip.c,
drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or
drivers/acpi/irq-gic.c I don't care at all.
I don't think so you can make that common code very easily. The
information required by both drivers is organized differently.
If it was, I'd have done that for the DT binding.
I see, and that's also what Tomasz just explained. So can we just
have one an irqchip_init() function doing this:?
if (dt)
of_irq_init(__irqchip_of_table);
else if (acpi) {
read cpu-interface and redistributor address from acpi tables
if (cpu-interface)
gic_v2_acpi_init(table);
else if(redistributor)
gic_v3_acpi_init(table)
}
It is pretty much the same as:
if (dt)
of_irq_init(__irqchip_of_table);
else if (acpi) {
err = gic_v3_acpi_init(table) {
read redistributor address from acpi tables
[...]
}
if (err) {
err = gic_v2_acpi_init(table) {
read cpu-interface address from acpi tables
[...]
}
}
}
What basically is my current implementation (well, without GICv3 support
but that will be next step).
Tomasz
--
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: [Linaro-acpi] [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 05/09/14 11:13, Arnd Bergmann wrote:
On Friday 05 September 2014 10:47:30 Marc Zyngier wrote:
I still prefer being explicit here for the same reason I mentioned earlier:
I want it to be very clear that we don't support arbitrary irqchips other
than the ones in the APCI specification. The infrastructure exists on DT
because we have to support a large number of incompatible irqchips.
I'm not suggesting that we should support more than the ACPI spec says.
And that's certainly the whole point of a spec, isn't it? ACPI says what
we support, and we're not going any further. I'm just saying that we
shouldn't make the maintenance burden heavier, and the code nothing
short of disgusting. Using our current infrastructure doesn't mean we're
going to support GIcv2.37.
Ok
In particular, the ACPI tables describing the irqchip have no way to
identify the GIC at all, if I read the spec correctly, you have to
parse the tables, ioremap the registers and then read the ID to know
if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any device
for the GIC that a hypothetical probe function would be based on.
This is not the way I read the spec. Table 5-46 (Interrupt Controller
Structure) tells you if you have a CPU interface (GICv1/v2) or a
redistributor (GICv3/v4). That's enough to know whether or not you
should carry on probing a particular controller.
Ah, good. I missed that.
The various GIC versions don't really have a unified memory map anyway
(well, none that you can rely on), and you really have to rely on ACPI
to tell you what you have.
So we are back to needing to support two different irqchip drivers
(v1/v2/v2m and v3/v4), instead of five or more, right?
As long as we make sure that the variants are directly probed from the
canonical driver (v2m from v2, ITS and v4 from v3), then we're OK.
It does seem wrong to parse the tables in the irq-gic.c file though:
that part can well be common across the various gic versions and then
call into either irq-gic.c or irq-gic-v3.c for the version specific
parts. Whether we put that common code into drivers/irqchip/irqchip.c,
drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or
drivers/acpi/irq-gic.c I don't care at all.
I don't think so you can make that common code very easily. The
information required by both drivers is organized differently.
If it was, I'd have done that for the DT binding.
I see, and that's also what Tomasz just explained. So can we just
have one an irqchip_init() function doing this:?
if (dt)
of_irq_init(__irqchip_of_table);
else if (acpi) {
read cpu-interface and redistributor address from acpi tables
if (cpu-interface)
gic_v2_acpi_init(table);
else if(redistributor)
gic_v3_acpi_init(table)
}
That'd be better, but that only considers the basic architecture. GICv2m
and GICv3's ITS bring additional complexity (and their own table
parsing). At that stage, I'm not sure how much commonality we're left with.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
--
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: [Linaro-acpi] [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 05.09.2014 12:39, Marc Zyngier wrote:
On 05/09/14 11:13, Arnd Bergmann wrote:
On Friday 05 September 2014 10:47:30 Marc Zyngier wrote:
I still prefer being explicit here for the same reason I mentioned earlier:
I want it to be very clear that we don't support arbitrary irqchips other
than the ones in the APCI specification. The infrastructure exists on DT
because we have to support a large number of incompatible irqchips.
I'm not suggesting that we should support more than the ACPI spec says.
And that's certainly the whole point of a spec, isn't it? ACPI says what
we support, and we're not going any further. I'm just saying that we
shouldn't make the maintenance burden heavier, and the code nothing
short of disgusting. Using our current infrastructure doesn't mean we're
going to support GIcv2.37.
Ok
In particular, the ACPI tables describing the irqchip have no way to
identify the GIC at all, if I read the spec correctly, you have to
parse the tables, ioremap the registers and then read the ID to know
if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any device
for the GIC that a hypothetical probe function would be based on.
This is not the way I read the spec. Table 5-46 (Interrupt Controller
Structure) tells you if you have a CPU interface (GICv1/v2) or a
redistributor (GICv3/v4). That's enough to know whether or not you
should carry on probing a particular controller.
Ah, good. I missed that.
The various GIC versions don't really have a unified memory map anyway
(well, none that you can rely on), and you really have to rely on ACPI
to tell you what you have.
So we are back to needing to support two different irqchip drivers
(v1/v2/v2m and v3/v4), instead of five or more, right?
As long as we make sure that the variants are directly probed from the
canonical driver (v2m from v2, ITS and v4 from v3), then we're OK.
Yes, that is our intention.
It does seem wrong to parse the tables in the irq-gic.c file though:
that part can well be common across the various gic versions and then
call into either irq-gic.c or irq-gic-v3.c for the version specific
parts. Whether we put that common code into drivers/irqchip/irqchip.c,
drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or
drivers/acpi/irq-gic.c I don't care at all.
I don't think so you can make that common code very easily. The
information required by both drivers is organized differently.
If it was, I'd have done that for the DT binding.
I see, and that's also what Tomasz just explained. So can we just
have one an irqchip_init() function doing this:?
if (dt)
of_irq_init(__irqchip_of_table);
else if (acpi) {
read cpu-interface and redistributor address from acpi tables
if (cpu-interface)
gic_v2_acpi_init(table);
else if(redistributor)
gic_v3_acpi_init(table)
}
That'd be better, but that only considers the basic architecture. GICv2m
and GICv3's ITS bring additional complexity (and their own table
parsing). At that stage, I'm not sure how much commonality we're left with.
Right, my local ACPI GICv3 support assumes to probe ITS from
gic_v3_acpi_init() body.
Tomasz
--
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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 2014年09月03日 19:17, Hanjun Guo wrote: >>> + >>> +#ifdef CONFIG_ACPI >>> +#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 >> With GICv2? I doubt it. > I will create macro for each GIC driver: > #define ACPI_MAX_GICV2_CPU_INTERFACE_ENTRIES8 > #define ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES65535 Where do you get this value (ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES) from? >>> This value is for max processors entries in MADT, and we will use it to >>> scan MADT >>> for SMP/GIC Init, I just make it big enough for GICv3/4. since ACPI core >>> will stop >>> scan MADT if the real numbers of processors entries are reached no matter >>> how big ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES is, I think we can just >>> define a number big enough then it will work (x86 and ia64 did the same >>> thing). >> Also, with GICv3++, there is no such thing as a memory-mapped CPU >> interface anymore. What you get is a bunch of redistributors (one per >> CPU). I assume what you have here actually describe the redistributors, >> and its name should reflect that. > As Sudeep said, it is not to link to GIC architecture, so I think we can keep > it stick with ACPI spec, in ACPI spec, it called "GICC structure" (section > 5.2.12.14 > in ACPI 5.1), so we can name it as ACPI_MAX_GICC_STRUCTURE_ENTRIES no matter > GICv2 or GICv3/4 (with GICv2, it may have more than 8 entries with some > disabled > ones, will no more than 8 enabled entries). > > What do you think? After more consideration on this, we think that we can remove those macros which introduce confusions, and just pass 0 to ACPI core for the max entries of GICC structure or GICR structure, ACPI core will continue scan all the entries in MADT with 0 passed. With that, we can avoid such name confusions for all GIC related structures in MADT no matter GICv2 or GICv3/4. 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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 04.09.2014 12:14, Arnd Bergmann wrote:
On Thursday 04 September 2014 12:10:28 Tomasz Nowicki wrote:
On 03.09.2014 20:42, Arnd Bergmann wrote:
On Monday 01 September 2014 22:57:51 Hanjun Guo wrote:
+ /* Collect CPU base addresses */
+ count = acpi_parse_entries(sizeof(struct acpi_table_madt),
+ gic_acpi_parse_madt_cpu, table,
+ ACPI_MADT_TYPE_GENERIC_INTERRUPT,
+ ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
+ if (count < 0) {
+ pr_err("Error during GICC entries parsing\n");
+ return -EFAULT;
+ } else if (!count) {
+ /* No GICC entries provided, use address from MADT header */
+ struct acpi_table_madt *madt = (struct acpi_table_madt *)table;
+
+ if (!madt->address)
+ return -EFAULT;
+
+ cpu_phy_base = (u64)madt->address;
+ }
After I read through ACPI-5.1 section 5.2.12.14, I wonder if this is the
best way to treat a missing ACPI_MADT_TYPE_GENERIC_INTERRUPT table.
Do we expect to see those in practice? It seems like using the x86 local
APIC address as a fallback for the GIC address is not something we
should do unless we absolutely have to support a system that doesn't
have the GIC table.
No, we do not expect and hopefully there will be no such
But, we are trying to be as much as possible inline with 5.1 spec,
5.2.12.14 says:
[...]
If provided here (CPU physical base address), the "Local Interrupt
Controller Address" field in the MADT must be ignored by the OSPM.
[...]
Yes, that's what I saw. So ignoring it all the time is fine, right?
Presumably the madt->address field is only referenced here because
some pre-5.1 implementations used to do that.
So this is very vague statement. On the one hand it would make sense to
take madt->address if we have no GICC entries. On the other hand we do
not support non-banked GIC cpu registers. So all of then need to have
the same cpu_base_address. What if one has null address? Should the rest
take madt->address? I think you are right, I will remove madt->address
fallback and simplify the code.
Thanks,
Tomasz
--
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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On Thursday 04 September 2014 12:10:28 Tomasz Nowicki wrote:
> On 03.09.2014 20:42, Arnd Bergmann wrote:
> > On Monday 01 September 2014 22:57:51 Hanjun Guo wrote:
> >> + /* Collect CPU base addresses */
> >> + count = acpi_parse_entries(sizeof(struct acpi_table_madt),
> >> + gic_acpi_parse_madt_cpu, table,
> >> + ACPI_MADT_TYPE_GENERIC_INTERRUPT,
> >> + ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
> >> + if (count < 0) {
> >> + pr_err("Error during GICC entries parsing\n");
> >> + return -EFAULT;
> >> + } else if (!count) {
> >> + /* No GICC entries provided, use address from MADT header
> >> */
> >> + struct acpi_table_madt *madt = (struct acpi_table_madt
> >> *)table;
> >> +
> >> + if (!madt->address)
> >> + return -EFAULT;
> >> +
> >> + cpu_phy_base = (u64)madt->address;
> >> + }
> >
> > After I read through ACPI-5.1 section 5.2.12.14, I wonder if this is the
> > best way to treat a missing ACPI_MADT_TYPE_GENERIC_INTERRUPT table.
> >
> > Do we expect to see those in practice? It seems like using the x86 local
> > APIC address as a fallback for the GIC address is not something we
> > should do unless we absolutely have to support a system that doesn't
> > have the GIC table.
>
> No, we do not expect and hopefully there will be no such
>
> But, we are trying to be as much as possible inline with 5.1 spec,
> 5.2.12.14 says:
> [...]
> If provided here (CPU physical base address), the "Local Interrupt
> Controller Address" field in the MADT must be ignored by the OSPM.
> [...]
>
Yes, that's what I saw. So ignoring it all the time is fine, right?
Presumably the madt->address field is only referenced here because
some pre-5.1 implementations used to do that.
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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 03.09.2014 20:42, Arnd Bergmann wrote:
On Monday 01 September 2014 22:57:51 Hanjun Guo wrote:
+ /* Collect CPU base addresses */
+ count = acpi_parse_entries(sizeof(struct acpi_table_madt),
+ gic_acpi_parse_madt_cpu, table,
+ ACPI_MADT_TYPE_GENERIC_INTERRUPT,
+ ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
+ if (count < 0) {
+ pr_err("Error during GICC entries parsing\n");
+ return -EFAULT;
+ } else if (!count) {
+ /* No GICC entries provided, use address from MADT header */
+ struct acpi_table_madt *madt = (struct acpi_table_madt *)table;
+
+ if (!madt->address)
+ return -EFAULT;
+
+ cpu_phy_base = (u64)madt->address;
+ }
After I read through ACPI-5.1 section 5.2.12.14, I wonder if this is the
best way to treat a missing ACPI_MADT_TYPE_GENERIC_INTERRUPT table.
Do we expect to see those in practice? It seems like using the x86 local
APIC address as a fallback for the GIC address is not something we
should do unless we absolutely have to support a system that doesn't
have the GIC table.
No, we do not expect and hopefully there will be no such :)
But, we are trying to be as much as possible inline with 5.1 spec,
5.2.12.14 says:
[...]
If provided here (CPU physical base address), the "Local Interrupt
Controller Address" field in the MADT must be ignored by the OSPM.
[...]
Regards,
Tomasz
--
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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 03.09.2014 20:42, Arnd Bergmann wrote:
On Monday 01 September 2014 22:57:51 Hanjun Guo wrote:
+ /* Collect CPU base addresses */
+ count = acpi_parse_entries(sizeof(struct acpi_table_madt),
+ gic_acpi_parse_madt_cpu, table,
+ ACPI_MADT_TYPE_GENERIC_INTERRUPT,
+ ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
+ if (count 0) {
+ pr_err(Error during GICC entries parsing\n);
+ return -EFAULT;
+ } else if (!count) {
+ /* No GICC entries provided, use address from MADT header */
+ struct acpi_table_madt *madt = (struct acpi_table_madt *)table;
+
+ if (!madt-address)
+ return -EFAULT;
+
+ cpu_phy_base = (u64)madt-address;
+ }
After I read through ACPI-5.1 section 5.2.12.14, I wonder if this is the
best way to treat a missing ACPI_MADT_TYPE_GENERIC_INTERRUPT table.
Do we expect to see those in practice? It seems like using the x86 local
APIC address as a fallback for the GIC address is not something we
should do unless we absolutely have to support a system that doesn't
have the GIC table.
No, we do not expect and hopefully there will be no such :)
But, we are trying to be as much as possible inline with 5.1 spec,
5.2.12.14 says:
[...]
If provided here (CPU physical base address), the Local Interrupt
Controller Address field in the MADT must be ignored by the OSPM.
[...]
Regards,
Tomasz
--
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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On Thursday 04 September 2014 12:10:28 Tomasz Nowicki wrote:
On 03.09.2014 20:42, Arnd Bergmann wrote:
On Monday 01 September 2014 22:57:51 Hanjun Guo wrote:
+ /* Collect CPU base addresses */
+ count = acpi_parse_entries(sizeof(struct acpi_table_madt),
+ gic_acpi_parse_madt_cpu, table,
+ ACPI_MADT_TYPE_GENERIC_INTERRUPT,
+ ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
+ if (count 0) {
+ pr_err(Error during GICC entries parsing\n);
+ return -EFAULT;
+ } else if (!count) {
+ /* No GICC entries provided, use address from MADT header
*/
+ struct acpi_table_madt *madt = (struct acpi_table_madt
*)table;
+
+ if (!madt-address)
+ return -EFAULT;
+
+ cpu_phy_base = (u64)madt-address;
+ }
After I read through ACPI-5.1 section 5.2.12.14, I wonder if this is the
best way to treat a missing ACPI_MADT_TYPE_GENERIC_INTERRUPT table.
Do we expect to see those in practice? It seems like using the x86 local
APIC address as a fallback for the GIC address is not something we
should do unless we absolutely have to support a system that doesn't
have the GIC table.
No, we do not expect and hopefully there will be no such
But, we are trying to be as much as possible inline with 5.1 spec,
5.2.12.14 says:
[...]
If provided here (CPU physical base address), the Local Interrupt
Controller Address field in the MADT must be ignored by the OSPM.
[...]
Yes, that's what I saw. So ignoring it all the time is fine, right?
Presumably the madt-address field is only referenced here because
some pre-5.1 implementations used to do that.
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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 04.09.2014 12:14, Arnd Bergmann wrote:
On Thursday 04 September 2014 12:10:28 Tomasz Nowicki wrote:
On 03.09.2014 20:42, Arnd Bergmann wrote:
On Monday 01 September 2014 22:57:51 Hanjun Guo wrote:
+ /* Collect CPU base addresses */
+ count = acpi_parse_entries(sizeof(struct acpi_table_madt),
+ gic_acpi_parse_madt_cpu, table,
+ ACPI_MADT_TYPE_GENERIC_INTERRUPT,
+ ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
+ if (count 0) {
+ pr_err(Error during GICC entries parsing\n);
+ return -EFAULT;
+ } else if (!count) {
+ /* No GICC entries provided, use address from MADT header */
+ struct acpi_table_madt *madt = (struct acpi_table_madt *)table;
+
+ if (!madt-address)
+ return -EFAULT;
+
+ cpu_phy_base = (u64)madt-address;
+ }
After I read through ACPI-5.1 section 5.2.12.14, I wonder if this is the
best way to treat a missing ACPI_MADT_TYPE_GENERIC_INTERRUPT table.
Do we expect to see those in practice? It seems like using the x86 local
APIC address as a fallback for the GIC address is not something we
should do unless we absolutely have to support a system that doesn't
have the GIC table.
No, we do not expect and hopefully there will be no such
But, we are trying to be as much as possible inline with 5.1 spec,
5.2.12.14 says:
[...]
If provided here (CPU physical base address), the Local Interrupt
Controller Address field in the MADT must be ignored by the OSPM.
[...]
Yes, that's what I saw. So ignoring it all the time is fine, right?
Presumably the madt-address field is only referenced here because
some pre-5.1 implementations used to do that.
So this is very vague statement. On the one hand it would make sense to
take madt-address if we have no GICC entries. On the other hand we do
not support non-banked GIC cpu registers. So all of then need to have
the same cpu_base_address. What if one has null address? Should the rest
take madt-address? I think you are right, I will remove madt-address
fallback and simplify the code.
Thanks,
Tomasz
--
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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 2014年09月03日 19:17, Hanjun Guo wrote: + +#ifdef CONFIG_ACPI +#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 With GICv2? I doubt it. I will create macro for each GIC driver: #define ACPI_MAX_GICV2_CPU_INTERFACE_ENTRIES8 #define ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES65535 Where do you get this value (ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES) from? This value is for max processors entries in MADT, and we will use it to scan MADT for SMP/GIC Init, I just make it big enough for GICv3/4. since ACPI core will stop scan MADT if the real numbers of processors entries are reached no matter how big ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES is, I think we can just define a number big enough then it will work (x86 and ia64 did the same thing). Also, with GICv3++, there is no such thing as a memory-mapped CPU interface anymore. What you get is a bunch of redistributors (one per CPU). I assume what you have here actually describe the redistributors, and its name should reflect that. As Sudeep said, it is not to link to GIC architecture, so I think we can keep it stick with ACPI spec, in ACPI spec, it called GICC structure (section 5.2.12.14 in ACPI 5.1), so we can name it as ACPI_MAX_GICC_STRUCTURE_ENTRIES no matter GICv2 or GICv3/4 (with GICv2, it may have more than 8 entries with some disabled ones, will no more than 8 enabled entries). What do you think? After more consideration on this, we think that we can remove those macros which introduce confusions, and just pass 0 to ACPI core for the max entries of GICC structure or GICR structure, ACPI core will continue scan all the entries in MADT with 0 passed. With that, we can avoid such name confusions for all GIC related structures in MADT no matter GICv2 or GICv3/4. 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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On Monday 01 September 2014 22:57:51 Hanjun Guo wrote:
> + /* Collect CPU base addresses */
> + count = acpi_parse_entries(sizeof(struct acpi_table_madt),
> + gic_acpi_parse_madt_cpu, table,
> + ACPI_MADT_TYPE_GENERIC_INTERRUPT,
> + ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
> + if (count < 0) {
> + pr_err("Error during GICC entries parsing\n");
> + return -EFAULT;
> + } else if (!count) {
> + /* No GICC entries provided, use address from MADT header */
> + struct acpi_table_madt *madt = (struct acpi_table_madt
> *)table;
> +
> + if (!madt->address)
> + return -EFAULT;
> +
> + cpu_phy_base = (u64)madt->address;
> + }
After I read through ACPI-5.1 section 5.2.12.14, I wonder if this is the
best way to treat a missing ACPI_MADT_TYPE_GENERIC_INTERRUPT table.
Do we expect to see those in practice? It seems like using the x86 local
APIC address as a fallback for the GIC address is not something we
should do unless we absolutely have to support a system that doesn't
have the GIC table.
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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On Wednesday 03 September 2014 11:26:14 Tomasz Nowicki wrote:
> On 02.09.2014 15:02, Marc Zyngier wrote:
> > On 02/09/14 12:48, Tomasz Nowicki wrote:
> >> On 01.09.2014 19:35, Marc Zyngier wrote:
> @@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct
> pt_regs *))
> void __init init_IRQ(void)
> {
> irqchip_init();
> +
> +if (!handle_arch_irq)
> +acpi_gic_init();
> +
> >>>
> >>> Why isn't this called from irqchip_init? It would seem like the logical
> >>> spot to probe an interrupt controller.
> >>
> >> irqchip.c is OF dependent, I want to decouple these from the very
> >> beginning.
> >
> > No. irqchip.c is not OF dependent, it is just that DT is the only thing
> > we support so far. I don't think duplicating the kernel infrastructure
> > "because we're different" is the right way.
> >
> > There is no reason for your probing structure to be artificially
> > different (you're parsing the same information, at the same time). Just
> > put in place a similar probing mechanism, and this will look a lot better.
> >> Having only GICv2, it would work. Considering we would do the same for
> >> GICv3 (arm-gic-v3.h) there will be register name conflicts for both
> >> headers inclusion:
> >>
> >> [...]
> >> #include
> >> #include
> >> [...]
> >> err = gic_v3_acpi_init(table);
> >> if (err)
> >> err = gic_v2_acpi_init(table);
> >> if (err)
> >> pr_err("Failed to initialize GIC IRQ controller");
> >> [...]
> >> So instead of changing register names prefix, I choose new header will
> >> be less painfully.
> >
> > Yes, and this is exactly why I pushed back on that last time. I'll
> > continue saying that interrupt controllers should be self-probing, with
> > ACPI as they are with DT.
> >
> > Even with the restrictions of ACPI and SBSA, we end-up with at least 2
> > main families of interrupt controllers (GICv2 and GICv3), both with a
> > number of "interesting" variations (GICv2m and GICv4, to only mention
> > those I'm directly involved with).
> >
> > I can safely predict that the above will become a tangled mess within 18
> > months, and the idea of littering the arch code with a bunch of
> > hardcoded "if (blah())" doesn't fill me with joy and confidence.
> >
> > In summary: we have the infrastructure already, just use it.
>
> We had that discussion but I see we still don't have consensus here. It
> would be good to know our direction before we prepare next patch
> version. Arnd any comments on this from you side?
I still prefer being explicit here for the same reason I mentioned earlier:
I want it to be very clear that we don't support arbitrary irqchips other
than the ones in the APCI specification. The infrastructure exists on DT
because we have to support a large number of incompatible irqchips.
In particular, the ACPI tables describing the irqchip have no way to
identify the GIC at all, if I read the spec correctly, you have to
parse the tables, ioremap the registers and then read the ID to know
if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any "device"
for the GIC that a hypothetical probe function would be based on.
It does seem wrong to parse the tables in the irq-gic.c file though:
that part can well be common across the various gic versions and then
call into either irq-gic.c or irq-gic-v3.c for the version specific
parts. Whether we put that common code into drivers/irqchip/irqchip.c,
drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or
drivers/acpi/irq-gic.c I don't care at all.
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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
>> + >> +#ifdef CONFIG_ACPI >> +#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 > With GICv2? I doubt it. I will create macro for each GIC driver: #define ACPI_MAX_GICV2_CPU_INTERFACE_ENTRIES8 #define ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES65535 >>> Where do you get this value (ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES) from? >> This value is for max processors entries in MADT, and we will use it to scan >> MADT >> for SMP/GIC Init, I just make it big enough for GICv3/4. since ACPI core >> will stop >> scan MADT if the real numbers of processors entries are reached no matter >> how big ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES is, I think we can just >> define a number big enough then it will work (x86 and ia64 did the same >> thing). > Also, with GICv3++, there is no such thing as a memory-mapped CPU > interface anymore. What you get is a bunch of redistributors (one per > CPU). I assume what you have here actually describe the redistributors, > and its name should reflect that. As Sudeep said, it is not to link to GIC architecture, so I think we can keep it stick with ACPI spec, in ACPI spec, it called "GICC structure" (section 5.2.12.14 in ACPI 5.1), so we can name it as ACPI_MAX_GICC_STRUCTURE_ENTRIES no matter GICv2 or GICv3/4 (with GICv2, it may have more than 8 entries with some disabled ones, will no more than 8 enabled entries). What do you think? 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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 02/09/14 16:45, Hanjun Guo wrote:
> On 2014年09月02日 21:02, Marc Zyngier wrote:
>> On 02/09/14 12:48, Tomasz Nowicki wrote:
>>> On 01.09.2014 19:35, Marc Zyngier wrote:
On 01/09/14 15:57, Hanjun Guo wrote:
> From: Tomasz Nowicki
>
> ACPI kernel uses MADT table for proper GIC initialization. It needs to
> parse GIC related subtables, collect CPU interface and distributor
> addresses and call driver initialization function (which is hardware
> abstraction agnostic). In a similar way, FDT initialize GICv1/2.
>
> NOTE: This commit allow to initialize GICv1/2 only.
I cannot help but notice that there is no support for KVM here. It'd be
good to add a note to that effect, so that people do not expect
virtualization support to be working when booting with ACPI.
>>> yes, it is worth mentioning!
>>>
> Signed-off-by: Tomasz Nowicki
> Signed-off-by: Hanjun Guo
> ---
> arch/arm64/include/asm/acpi.h|2 -
> arch/arm64/kernel/acpi.c | 23 +++
> arch/arm64/kernel/irq.c |5 ++
> drivers/irqchip/irq-gic.c| 114
> ++
> include/linux/irqchip/arm-gic-acpi.h | 33 ++
> 5 files changed, 175 insertions(+), 2 deletions(-)
> create mode 100644 include/linux/irqchip/arm-gic-acpi.h
>
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index a867467..5d2ab63 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void);
> extern int (*acpi_suspend_lowlevel)(void);
> #define acpi_wakeup_address 0
>
> -#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535
> -
> #else
>
> static inline bool acpi_psci_present(void) { return false; }
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index 354b912..b3b82b0 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -23,6 +23,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
> pr_err("Can't find FADT or error happened during parsing
> FADT\n");
> }
>
> +void __init acpi_gic_init(void)
> +{
> +struct acpi_table_header *table;
> +acpi_status status;
> +acpi_size tbl_size;
> +int err;
> +
> +status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, ,
> _size);
> +if (ACPI_FAILURE(status)) {
> +const char *msg = acpi_format_exception(status);
> +
> +pr_err("Failed to get MADT table, %s\n", msg);
> +return;
> +}
> +
> +err = gic_v2_acpi_init(table);
> +if (err)
> +pr_err("Failed to initialize GIC IRQ controller");
What will happen when you get to implement GICv3 support? Another entry
like this? Why isn't this entirely contained in the GIC driver? Do I
sound like a stuck record?
>>> There will be another call to GICv3 init:
>>> [...]
>>> err = gic_v3_acpi_init(table);
>>> if (err)
>>> err = gic_v2_acpi_init(table);
>>> if (err)
>>> pr_err("Failed to initialize GIC IRQ controller");
>>> [...]
>>> This is the main reason I put common code here.
>>>
> +
> +early_acpi_os_unmap_memory((char *)table, tbl_size);
> +}
> +
> /*
>* acpi_suspend_lowlevel() - save kernel state and suspend.
>*
> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
> index 0f08dfd..c074d60 100644
> --- a/arch/arm64/kernel/irq.c
> +++ b/arch/arm64/kernel/irq.c
> @@ -28,6 +28,7 @@
> #include
> #include
> #include
> +#include
>
> unsigned long irq_err_count;
>
> @@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct
> pt_regs *))
> void __init init_IRQ(void)
> {
> irqchip_init();
> +
> +if (!handle_arch_irq)
> +acpi_gic_init();
> +
Why isn't this called from irqchip_init? It would seem like the logical
spot to probe an interrupt controller.
>>> irqchip.c is OF dependent, I want to decouple these from the very
>>> beginning.
>> No. irqchip.c is not OF dependent, it is just that DT is the only thing
>> we support so far. I don't think duplicating the kernel infrastructure
>> "because we're different" is the right way.
>>
>> There is no reason for your probing structure to be artificially
>> different (you're parsing the same information, at the same time). Just
>> put in place a similar probing mechanism, and this will look a lot better.
>>
> if (!handle_arch_irq)
> panic("No interrupt controller
Re: [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 02.09.2014 15:02, Marc Zyngier wrote:
On 02/09/14 12:48, Tomasz Nowicki wrote:
On 01.09.2014 19:35, Marc Zyngier wrote:
On 01/09/14 15:57, Hanjun Guo wrote:
From: Tomasz Nowicki
ACPI kernel uses MADT table for proper GIC initialization. It needs to
parse GIC related subtables, collect CPU interface and distributor
addresses and call driver initialization function (which is hardware
abstraction agnostic). In a similar way, FDT initialize GICv1/2.
NOTE: This commit allow to initialize GICv1/2 only.
I cannot help but notice that there is no support for KVM here. It'd be
good to add a note to that effect, so that people do not expect
virtualization support to be working when booting with ACPI.
yes, it is worth mentioning!
Signed-off-by: Tomasz Nowicki
Signed-off-by: Hanjun Guo
---
arch/arm64/include/asm/acpi.h|2 -
arch/arm64/kernel/acpi.c | 23 +++
arch/arm64/kernel/irq.c |5 ++
drivers/irqchip/irq-gic.c| 114
++
include/linux/irqchip/arm-gic-acpi.h | 33 ++
5 files changed, 175 insertions(+), 2 deletions(-)
create mode 100644 include/linux/irqchip/arm-gic-acpi.h
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index a867467..5d2ab63 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void);
extern int (*acpi_suspend_lowlevel)(void);
#define acpi_wakeup_address 0
-#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535
-
#else
static inline bool acpi_psci_present(void) { return false; }
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 354b912..b3b82b0 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -23,6 +23,7 @@
#include
#include
#include
+#include
#include
#include
@@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
pr_err("Can't find FADT or error happened during parsing FADT\n");
}
+void __init acpi_gic_init(void)
+{
+struct acpi_table_header *table;
+acpi_status status;
+acpi_size tbl_size;
+int err;
+
+status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, , _size);
+if (ACPI_FAILURE(status)) {
+const char *msg = acpi_format_exception(status);
+
+pr_err("Failed to get MADT table, %s\n", msg);
+return;
+}
+
+err = gic_v2_acpi_init(table);
+if (err)
+pr_err("Failed to initialize GIC IRQ controller");
What will happen when you get to implement GICv3 support? Another entry
like this? Why isn't this entirely contained in the GIC driver? Do I
sound like a stuck record?
There will be another call to GICv3 init:
[...]
err = gic_v3_acpi_init(table);
if (err)
err = gic_v2_acpi_init(table);
if (err)
pr_err("Failed to initialize GIC IRQ controller");
[...]
This is the main reason I put common code here.
+
+early_acpi_os_unmap_memory((char *)table, tbl_size);
+}
+
/*
* acpi_suspend_lowlevel() - save kernel state and suspend.
*
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 0f08dfd..c074d60 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
unsigned long irq_err_count;
@@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs
*))
void __init init_IRQ(void)
{
irqchip_init();
+
+if (!handle_arch_irq)
+acpi_gic_init();
+
Why isn't this called from irqchip_init? It would seem like the logical
spot to probe an interrupt controller.
irqchip.c is OF dependent, I want to decouple these from the very
beginning.
No. irqchip.c is not OF dependent, it is just that DT is the only thing
we support so far. I don't think duplicating the kernel infrastructure
"because we're different" is the right way.
There is no reason for your probing structure to be artificially
different (you're parsing the same information, at the same time). Just
put in place a similar probing mechanism, and this will look a lot better.
if (!handle_arch_irq)
panic("No interrupt controller found.");
}
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4b959e6..85cbf43 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -33,12 +33,14 @@
#include
#include
#include
+#include
#include
#include
#include
#include
#include
#include
+#include
#include
#include
@@ -1029,3 +1031,115 @@ IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic",
gic_of_init);
IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init);
#endif
+
+#ifdef CONFIG_ACPI
+static u64 dist_phy_base, cpu_phy_base = ULONG_MAX;
Please use phys_addr_t for physical addresses. The use of ULONG_MAX
looks dodgy. Please
Re: [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 02.09.2014 15:02, Marc Zyngier wrote:
On 02/09/14 12:48, Tomasz Nowicki wrote:
On 01.09.2014 19:35, Marc Zyngier wrote:
On 01/09/14 15:57, Hanjun Guo wrote:
From: Tomasz Nowicki tomasz.nowi...@linaro.org
ACPI kernel uses MADT table for proper GIC initialization. It needs to
parse GIC related subtables, collect CPU interface and distributor
addresses and call driver initialization function (which is hardware
abstraction agnostic). In a similar way, FDT initialize GICv1/2.
NOTE: This commit allow to initialize GICv1/2 only.
I cannot help but notice that there is no support for KVM here. It'd be
good to add a note to that effect, so that people do not expect
virtualization support to be working when booting with ACPI.
yes, it is worth mentioning!
Signed-off-by: Tomasz Nowicki tomasz.nowi...@linaro.org
Signed-off-by: Hanjun Guo hanjun@linaro.org
---
arch/arm64/include/asm/acpi.h|2 -
arch/arm64/kernel/acpi.c | 23 +++
arch/arm64/kernel/irq.c |5 ++
drivers/irqchip/irq-gic.c| 114
++
include/linux/irqchip/arm-gic-acpi.h | 33 ++
5 files changed, 175 insertions(+), 2 deletions(-)
create mode 100644 include/linux/irqchip/arm-gic-acpi.h
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index a867467..5d2ab63 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void);
extern int (*acpi_suspend_lowlevel)(void);
#define acpi_wakeup_address 0
-#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535
-
#else
static inline bool acpi_psci_present(void) { return false; }
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 354b912..b3b82b0 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -23,6 +23,7 @@
#include linux/irqdomain.h
#include linux/bootmem.h
#include linux/smp.h
+#include linux/irqchip/arm-gic-acpi.h
#include asm/cputype.h
#include asm/cpu_ops.h
@@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
pr_err(Can't find FADT or error happened during parsing FADT\n);
}
+void __init acpi_gic_init(void)
+{
+struct acpi_table_header *table;
+acpi_status status;
+acpi_size tbl_size;
+int err;
+
+status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, table, tbl_size);
+if (ACPI_FAILURE(status)) {
+const char *msg = acpi_format_exception(status);
+
+pr_err(Failed to get MADT table, %s\n, msg);
+return;
+}
+
+err = gic_v2_acpi_init(table);
+if (err)
+pr_err(Failed to initialize GIC IRQ controller);
What will happen when you get to implement GICv3 support? Another entry
like this? Why isn't this entirely contained in the GIC driver? Do I
sound like a stuck record?
There will be another call to GICv3 init:
[...]
err = gic_v3_acpi_init(table);
if (err)
err = gic_v2_acpi_init(table);
if (err)
pr_err(Failed to initialize GIC IRQ controller);
[...]
This is the main reason I put common code here.
+
+early_acpi_os_unmap_memory((char *)table, tbl_size);
+}
+
/*
* acpi_suspend_lowlevel() - save kernel state and suspend.
*
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 0f08dfd..c074d60 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -28,6 +28,7 @@
#include linux/irqchip.h
#include linux/seq_file.h
#include linux/ratelimit.h
+#include linux/irqchip/arm-gic-acpi.h
unsigned long irq_err_count;
@@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs
*))
void __init init_IRQ(void)
{
irqchip_init();
+
+if (!handle_arch_irq)
+acpi_gic_init();
+
Why isn't this called from irqchip_init? It would seem like the logical
spot to probe an interrupt controller.
irqchip.c is OF dependent, I want to decouple these from the very
beginning.
No. irqchip.c is not OF dependent, it is just that DT is the only thing
we support so far. I don't think duplicating the kernel infrastructure
because we're different is the right way.
There is no reason for your probing structure to be artificially
different (you're parsing the same information, at the same time). Just
put in place a similar probing mechanism, and this will look a lot better.
if (!handle_arch_irq)
panic(No interrupt controller found.);
}
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4b959e6..85cbf43 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -33,12 +33,14 @@
#include linux/of.h
#include linux/of_address.h
#include linux/of_irq.h
+#include linux/acpi.h
#include linux/irqdomain.h
#include linux/interrupt.h
#include linux/percpu.h
#include linux/slab.h
#include linux/irqchip/chained_irq.h
Re: [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 02/09/14 16:45, Hanjun Guo wrote:
On 2014年09月02日 21:02, Marc Zyngier wrote:
On 02/09/14 12:48, Tomasz Nowicki wrote:
On 01.09.2014 19:35, Marc Zyngier wrote:
On 01/09/14 15:57, Hanjun Guo wrote:
From: Tomasz Nowicki tomasz.nowi...@linaro.org
ACPI kernel uses MADT table for proper GIC initialization. It needs to
parse GIC related subtables, collect CPU interface and distributor
addresses and call driver initialization function (which is hardware
abstraction agnostic). In a similar way, FDT initialize GICv1/2.
NOTE: This commit allow to initialize GICv1/2 only.
I cannot help but notice that there is no support for KVM here. It'd be
good to add a note to that effect, so that people do not expect
virtualization support to be working when booting with ACPI.
yes, it is worth mentioning!
Signed-off-by: Tomasz Nowicki tomasz.nowi...@linaro.org
Signed-off-by: Hanjun Guo hanjun@linaro.org
---
arch/arm64/include/asm/acpi.h|2 -
arch/arm64/kernel/acpi.c | 23 +++
arch/arm64/kernel/irq.c |5 ++
drivers/irqchip/irq-gic.c| 114
++
include/linux/irqchip/arm-gic-acpi.h | 33 ++
5 files changed, 175 insertions(+), 2 deletions(-)
create mode 100644 include/linux/irqchip/arm-gic-acpi.h
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index a867467..5d2ab63 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void);
extern int (*acpi_suspend_lowlevel)(void);
#define acpi_wakeup_address 0
-#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535
-
#else
static inline bool acpi_psci_present(void) { return false; }
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 354b912..b3b82b0 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -23,6 +23,7 @@
#include linux/irqdomain.h
#include linux/bootmem.h
#include linux/smp.h
+#include linux/irqchip/arm-gic-acpi.h
#include asm/cputype.h
#include asm/cpu_ops.h
@@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
pr_err(Can't find FADT or error happened during parsing
FADT\n);
}
+void __init acpi_gic_init(void)
+{
+struct acpi_table_header *table;
+acpi_status status;
+acpi_size tbl_size;
+int err;
+
+status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, table,
tbl_size);
+if (ACPI_FAILURE(status)) {
+const char *msg = acpi_format_exception(status);
+
+pr_err(Failed to get MADT table, %s\n, msg);
+return;
+}
+
+err = gic_v2_acpi_init(table);
+if (err)
+pr_err(Failed to initialize GIC IRQ controller);
What will happen when you get to implement GICv3 support? Another entry
like this? Why isn't this entirely contained in the GIC driver? Do I
sound like a stuck record?
There will be another call to GICv3 init:
[...]
err = gic_v3_acpi_init(table);
if (err)
err = gic_v2_acpi_init(table);
if (err)
pr_err(Failed to initialize GIC IRQ controller);
[...]
This is the main reason I put common code here.
+
+early_acpi_os_unmap_memory((char *)table, tbl_size);
+}
+
/*
* acpi_suspend_lowlevel() - save kernel state and suspend.
*
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 0f08dfd..c074d60 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -28,6 +28,7 @@
#include linux/irqchip.h
#include linux/seq_file.h
#include linux/ratelimit.h
+#include linux/irqchip/arm-gic-acpi.h
unsigned long irq_err_count;
@@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct
pt_regs *))
void __init init_IRQ(void)
{
irqchip_init();
+
+if (!handle_arch_irq)
+acpi_gic_init();
+
Why isn't this called from irqchip_init? It would seem like the logical
spot to probe an interrupt controller.
irqchip.c is OF dependent, I want to decouple these from the very
beginning.
No. irqchip.c is not OF dependent, it is just that DT is the only thing
we support so far. I don't think duplicating the kernel infrastructure
because we're different is the right way.
There is no reason for your probing structure to be artificially
different (you're parsing the same information, at the same time). Just
put in place a similar probing mechanism, and this will look a lot better.
if (!handle_arch_irq)
panic(No interrupt controller found.);
}
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4b959e6..85cbf43 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -33,12 +33,14 @@
#include linux/of.h
#include linux/of_address.h
#include linux/of_irq.h
+#include linux/acpi.h
#include linux/irqdomain.h
#include
Re: [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
+ +#ifdef CONFIG_ACPI +#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535 With GICv2? I doubt it. I will create macro for each GIC driver: #define ACPI_MAX_GICV2_CPU_INTERFACE_ENTRIES8 #define ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES65535 Where do you get this value (ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES) from? This value is for max processors entries in MADT, and we will use it to scan MADT for SMP/GIC Init, I just make it big enough for GICv3/4. since ACPI core will stop scan MADT if the real numbers of processors entries are reached no matter how big ACPI_MAX_GICV3_CPU_INTERFACE_ENTRIES is, I think we can just define a number big enough then it will work (x86 and ia64 did the same thing). Also, with GICv3++, there is no such thing as a memory-mapped CPU interface anymore. What you get is a bunch of redistributors (one per CPU). I assume what you have here actually describe the redistributors, and its name should reflect that. As Sudeep said, it is not to link to GIC architecture, so I think we can keep it stick with ACPI spec, in ACPI spec, it called GICC structure (section 5.2.12.14 in ACPI 5.1), so we can name it as ACPI_MAX_GICC_STRUCTURE_ENTRIES no matter GICv2 or GICv3/4 (with GICv2, it may have more than 8 entries with some disabled ones, will no more than 8 enabled entries). What do you think? 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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On Wednesday 03 September 2014 11:26:14 Tomasz Nowicki wrote:
On 02.09.2014 15:02, Marc Zyngier wrote:
On 02/09/14 12:48, Tomasz Nowicki wrote:
On 01.09.2014 19:35, Marc Zyngier wrote:
@@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct
pt_regs *))
void __init init_IRQ(void)
{
irqchip_init();
+
+if (!handle_arch_irq)
+acpi_gic_init();
+
Why isn't this called from irqchip_init? It would seem like the logical
spot to probe an interrupt controller.
irqchip.c is OF dependent, I want to decouple these from the very
beginning.
No. irqchip.c is not OF dependent, it is just that DT is the only thing
we support so far. I don't think duplicating the kernel infrastructure
because we're different is the right way.
There is no reason for your probing structure to be artificially
different (you're parsing the same information, at the same time). Just
put in place a similar probing mechanism, and this will look a lot better.
Having only GICv2, it would work. Considering we would do the same for
GICv3 (arm-gic-v3.h) there will be register name conflicts for both
headers inclusion:
[...]
#include linux/irqchip/arm-gic.h
#include linux/irqchip/arm-gic-v3.h
[...]
err = gic_v3_acpi_init(table);
if (err)
err = gic_v2_acpi_init(table);
if (err)
pr_err(Failed to initialize GIC IRQ controller);
[...]
So instead of changing register names prefix, I choose new header will
be less painfully.
Yes, and this is exactly why I pushed back on that last time. I'll
continue saying that interrupt controllers should be self-probing, with
ACPI as they are with DT.
Even with the restrictions of ACPI and SBSA, we end-up with at least 2
main families of interrupt controllers (GICv2 and GICv3), both with a
number of interesting variations (GICv2m and GICv4, to only mention
those I'm directly involved with).
I can safely predict that the above will become a tangled mess within 18
months, and the idea of littering the arch code with a bunch of
hardcoded if (blah()) doesn't fill me with joy and confidence.
In summary: we have the infrastructure already, just use it.
We had that discussion but I see we still don't have consensus here. It
would be good to know our direction before we prepare next patch
version. Arnd any comments on this from you side?
I still prefer being explicit here for the same reason I mentioned earlier:
I want it to be very clear that we don't support arbitrary irqchips other
than the ones in the APCI specification. The infrastructure exists on DT
because we have to support a large number of incompatible irqchips.
In particular, the ACPI tables describing the irqchip have no way to
identify the GIC at all, if I read the spec correctly, you have to
parse the tables, ioremap the registers and then read the ID to know
if you have GICv1/v2/v2m/v3/v4. There doesn't seem to be any device
for the GIC that a hypothetical probe function would be based on.
It does seem wrong to parse the tables in the irq-gic.c file though:
that part can well be common across the various gic versions and then
call into either irq-gic.c or irq-gic-v3.c for the version specific
parts. Whether we put that common code into drivers/irqchip/irqchip.c,
drivers/irqchip/gic-common.c, drivers/irqchip/irq-acpi-gic.c or
drivers/acpi/irq-gic.c I don't care at all.
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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On Monday 01 September 2014 22:57:51 Hanjun Guo wrote:
+ /* Collect CPU base addresses */
+ count = acpi_parse_entries(sizeof(struct acpi_table_madt),
+ gic_acpi_parse_madt_cpu, table,
+ ACPI_MADT_TYPE_GENERIC_INTERRUPT,
+ ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES);
+ if (count 0) {
+ pr_err(Error during GICC entries parsing\n);
+ return -EFAULT;
+ } else if (!count) {
+ /* No GICC entries provided, use address from MADT header */
+ struct acpi_table_madt *madt = (struct acpi_table_madt
*)table;
+
+ if (!madt-address)
+ return -EFAULT;
+
+ cpu_phy_base = (u64)madt-address;
+ }
After I read through ACPI-5.1 section 5.2.12.14, I wonder if this is the
best way to treat a missing ACPI_MADT_TYPE_GENERIC_INTERRUPT table.
Do we expect to see those in practice? It seems like using the x86 local
APIC address as a fallback for the GIC address is not something we
should do unless we absolutely have to support a system that doesn't
have the GIC table.
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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On Tue, Sep 02, 2014 at 12:48:37PM +0100, Tomasz Nowicki wrote: > On 01.09.2014 19:35, Marc Zyngier wrote: > > On 01/09/14 15:57, Hanjun Guo wrote: > >> From: Tomasz Nowicki > >> > >> ACPI kernel uses MADT table for proper GIC initialization. It needs to > >> parse GIC related subtables, collect CPU interface and distributor > >> addresses and call driver initialization function (which is hardware > >> abstraction agnostic). In a similar way, FDT initialize GICv1/2. > >> > >> NOTE: This commit allow to initialize GICv1/2 only. > > > > I cannot help but notice that there is no support for KVM here. It'd be > > good to add a note to that effect, so that people do not expect > > virtualization support to be working when booting with ACPI. > > yes, it is worth mentioning! It's also worth mentioning if there are any plans to fix this ;). -- Catalin -- 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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 02/09/14 16:45, Hanjun Guo wrote:
On 2014年09月02日 21:02, Marc Zyngier wrote:
On 02/09/14 12:48, Tomasz Nowicki wrote:
On 01.09.2014 19:35, Marc Zyngier wrote:
On 01/09/14 15:57, Hanjun Guo wrote:
From: Tomasz Nowicki
ACPI kernel uses MADT table for proper GIC initialization. It needs to
parse GIC related subtables, collect CPU interface and distributor
addresses and call driver initialization function (which is hardware
abstraction agnostic). In a similar way, FDT initialize GICv1/2.
NOTE: This commit allow to initialize GICv1/2 only.
I cannot help but notice that there is no support for KVM here. It'd be
good to add a note to that effect, so that people do not expect
virtualization support to be working when booting with ACPI.
yes, it is worth mentioning!
Signed-off-by: Tomasz Nowicki
Signed-off-by: Hanjun Guo
---
arch/arm64/include/asm/acpi.h|2 -
arch/arm64/kernel/acpi.c | 23 +++
arch/arm64/kernel/irq.c |5 ++
drivers/irqchip/irq-gic.c| 114
++
include/linux/irqchip/arm-gic-acpi.h | 33 ++
5 files changed, 175 insertions(+), 2 deletions(-)
create mode 100644 include/linux/irqchip/arm-gic-acpi.h
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index a867467..5d2ab63 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void);
extern int (*acpi_suspend_lowlevel)(void);
#define acpi_wakeup_address 0
-#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535
-
#else
static inline bool acpi_psci_present(void) { return false; }
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 354b912..b3b82b0 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -23,6 +23,7 @@
#include
#include
#include
+#include
#include
#include
@@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
pr_err("Can't find FADT or error happened during parsing FADT\n");
}
+void __init acpi_gic_init(void)
+{
+struct acpi_table_header *table;
+acpi_status status;
+acpi_size tbl_size;
+int err;
+
+status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, , _size);
+if (ACPI_FAILURE(status)) {
+const char *msg = acpi_format_exception(status);
+
+pr_err("Failed to get MADT table, %s\n", msg);
+return;
+}
+
+err = gic_v2_acpi_init(table);
+if (err)
+pr_err("Failed to initialize GIC IRQ controller");
What will happen when you get to implement GICv3 support? Another entry
like this? Why isn't this entirely contained in the GIC driver? Do I
sound like a stuck record?
There will be another call to GICv3 init:
[...]
err = gic_v3_acpi_init(table);
if (err)
err = gic_v2_acpi_init(table);
if (err)
pr_err("Failed to initialize GIC IRQ controller");
[...]
This is the main reason I put common code here.
+
+early_acpi_os_unmap_memory((char *)table, tbl_size);
+}
+
/*
* acpi_suspend_lowlevel() - save kernel state and suspend.
*
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 0f08dfd..c074d60 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
unsigned long irq_err_count;
@@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs
*))
void __init init_IRQ(void)
{
irqchip_init();
+
+if (!handle_arch_irq)
+acpi_gic_init();
+
Why isn't this called from irqchip_init? It would seem like the logical
spot to probe an interrupt controller.
irqchip.c is OF dependent, I want to decouple these from the very
beginning.
No. irqchip.c is not OF dependent, it is just that DT is the only thing
we support so far. I don't think duplicating the kernel infrastructure
"because we're different" is the right way.
There is no reason for your probing structure to be artificially
different (you're parsing the same information, at the same time). Just
put in place a similar probing mechanism, and this will look a lot better.
if (!handle_arch_irq)
panic("No interrupt controller found.");
}
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4b959e6..85cbf43 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -33,12 +33,14 @@
#include
#include
#include
+#include
#include
#include
#include
#include
#include
#include
+#include
#include
#include
@@ -1029,3 +1031,115 @@ IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic",
gic_of_init);
IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init);
#endif
+
+#ifdef CONFIG_ACPI
+static u64 dist_phy_base, cpu_phy_base = ULONG_MAX;
Please use phys_addr_t for physical addresses. The use of
Re: [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 02/09/14 16:45, Hanjun Guo wrote:
> On 2014年09月02日 21:02, Marc Zyngier wrote:
>> On 02/09/14 12:48, Tomasz Nowicki wrote:
>>> On 01.09.2014 19:35, Marc Zyngier wrote:
On 01/09/14 15:57, Hanjun Guo wrote:
> From: Tomasz Nowicki
>
> ACPI kernel uses MADT table for proper GIC initialization. It needs to
> parse GIC related subtables, collect CPU interface and distributor
> addresses and call driver initialization function (which is hardware
> abstraction agnostic). In a similar way, FDT initialize GICv1/2.
>
> NOTE: This commit allow to initialize GICv1/2 only.
I cannot help but notice that there is no support for KVM here. It'd be
good to add a note to that effect, so that people do not expect
virtualization support to be working when booting with ACPI.
>>> yes, it is worth mentioning!
>>>
> Signed-off-by: Tomasz Nowicki
> Signed-off-by: Hanjun Guo
> ---
> arch/arm64/include/asm/acpi.h|2 -
> arch/arm64/kernel/acpi.c | 23 +++
> arch/arm64/kernel/irq.c |5 ++
> drivers/irqchip/irq-gic.c| 114
> ++
> include/linux/irqchip/arm-gic-acpi.h | 33 ++
> 5 files changed, 175 insertions(+), 2 deletions(-)
> create mode 100644 include/linux/irqchip/arm-gic-acpi.h
>
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index a867467..5d2ab63 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void);
> extern int (*acpi_suspend_lowlevel)(void);
> #define acpi_wakeup_address 0
>
> -#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535
> -
> #else
>
> static inline bool acpi_psci_present(void) { return false; }
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index 354b912..b3b82b0 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -23,6 +23,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
> pr_err("Can't find FADT or error happened during parsing
> FADT\n");
> }
>
> +void __init acpi_gic_init(void)
> +{
> +struct acpi_table_header *table;
> +acpi_status status;
> +acpi_size tbl_size;
> +int err;
> +
> +status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, ,
> _size);
> +if (ACPI_FAILURE(status)) {
> +const char *msg = acpi_format_exception(status);
> +
> +pr_err("Failed to get MADT table, %s\n", msg);
> +return;
> +}
> +
> +err = gic_v2_acpi_init(table);
> +if (err)
> +pr_err("Failed to initialize GIC IRQ controller");
What will happen when you get to implement GICv3 support? Another entry
like this? Why isn't this entirely contained in the GIC driver? Do I
sound like a stuck record?
>>> There will be another call to GICv3 init:
>>> [...]
>>> err = gic_v3_acpi_init(table);
>>> if (err)
>>> err = gic_v2_acpi_init(table);
>>> if (err)
>>> pr_err("Failed to initialize GIC IRQ controller");
>>> [...]
>>> This is the main reason I put common code here.
>>>
> +
> +early_acpi_os_unmap_memory((char *)table, tbl_size);
> +}
> +
> /*
>* acpi_suspend_lowlevel() - save kernel state and suspend.
>*
> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
> index 0f08dfd..c074d60 100644
> --- a/arch/arm64/kernel/irq.c
> +++ b/arch/arm64/kernel/irq.c
> @@ -28,6 +28,7 @@
> #include
> #include
> #include
> +#include
>
> unsigned long irq_err_count;
>
> @@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct
> pt_regs *))
> void __init init_IRQ(void)
> {
> irqchip_init();
> +
> +if (!handle_arch_irq)
> +acpi_gic_init();
> +
Why isn't this called from irqchip_init? It would seem like the logical
spot to probe an interrupt controller.
>>> irqchip.c is OF dependent, I want to decouple these from the very
>>> beginning.
>> No. irqchip.c is not OF dependent, it is just that DT is the only thing
>> we support so far. I don't think duplicating the kernel infrastructure
>> "because we're different" is the right way.
>>
>> There is no reason for your probing structure to be artificially
>> different (you're parsing the same information, at the same time). Just
>> put in place a similar probing mechanism, and this will look a lot better.
>>
> if (!handle_arch_irq)
> panic("No interrupt controller
Re: [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 2014年09月02日 21:02, Marc Zyngier wrote:
> On 02/09/14 12:48, Tomasz Nowicki wrote:
>> On 01.09.2014 19:35, Marc Zyngier wrote:
>>> On 01/09/14 15:57, Hanjun Guo wrote:
From: Tomasz Nowicki
ACPI kernel uses MADT table for proper GIC initialization. It needs to
parse GIC related subtables, collect CPU interface and distributor
addresses and call driver initialization function (which is hardware
abstraction agnostic). In a similar way, FDT initialize GICv1/2.
NOTE: This commit allow to initialize GICv1/2 only.
>>> I cannot help but notice that there is no support for KVM here. It'd be
>>> good to add a note to that effect, so that people do not expect
>>> virtualization support to be working when booting with ACPI.
>> yes, it is worth mentioning!
>>
Signed-off-by: Tomasz Nowicki
Signed-off-by: Hanjun Guo
---
arch/arm64/include/asm/acpi.h|2 -
arch/arm64/kernel/acpi.c | 23 +++
arch/arm64/kernel/irq.c |5 ++
drivers/irqchip/irq-gic.c| 114
++
include/linux/irqchip/arm-gic-acpi.h | 33 ++
5 files changed, 175 insertions(+), 2 deletions(-)
create mode 100644 include/linux/irqchip/arm-gic-acpi.h
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index a867467..5d2ab63 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void);
extern int (*acpi_suspend_lowlevel)(void);
#define acpi_wakeup_address 0
-#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535
-
#else
static inline bool acpi_psci_present(void) { return false; }
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 354b912..b3b82b0 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -23,6 +23,7 @@
#include
#include
#include
+#include
#include
#include
@@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
pr_err("Can't find FADT or error happened during parsing
FADT\n");
}
+void __init acpi_gic_init(void)
+{
+struct acpi_table_header *table;
+acpi_status status;
+acpi_size tbl_size;
+int err;
+
+status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, ,
_size);
+if (ACPI_FAILURE(status)) {
+const char *msg = acpi_format_exception(status);
+
+pr_err("Failed to get MADT table, %s\n", msg);
+return;
+}
+
+err = gic_v2_acpi_init(table);
+if (err)
+pr_err("Failed to initialize GIC IRQ controller");
>>> What will happen when you get to implement GICv3 support? Another entry
>>> like this? Why isn't this entirely contained in the GIC driver? Do I
>>> sound like a stuck record?
>> There will be another call to GICv3 init:
>> [...]
>> err = gic_v3_acpi_init(table);
>> if (err)
>> err = gic_v2_acpi_init(table);
>> if (err)
>> pr_err("Failed to initialize GIC IRQ controller");
>> [...]
>> This is the main reason I put common code here.
>>
+
+early_acpi_os_unmap_memory((char *)table, tbl_size);
+}
+
/*
* acpi_suspend_lowlevel() - save kernel state and suspend.
*
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 0f08dfd..c074d60 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
unsigned long irq_err_count;
@@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct
pt_regs *))
void __init init_IRQ(void)
{
irqchip_init();
+
+if (!handle_arch_irq)
+acpi_gic_init();
+
>>> Why isn't this called from irqchip_init? It would seem like the logical
>>> spot to probe an interrupt controller.
>> irqchip.c is OF dependent, I want to decouple these from the very
>> beginning.
> No. irqchip.c is not OF dependent, it is just that DT is the only thing
> we support so far. I don't think duplicating the kernel infrastructure
> "because we're different" is the right way.
>
> There is no reason for your probing structure to be artificially
> different (you're parsing the same information, at the same time). Just
> put in place a similar probing mechanism, and this will look a lot better.
>
if (!handle_arch_irq)
panic("No interrupt controller found.");
}
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4b959e6..85cbf43 100644
--- a/drivers/irqchip/irq-gic.c
+++
Re: [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 02/09/14 12:48, Tomasz Nowicki wrote:
> On 01.09.2014 19:35, Marc Zyngier wrote:
>> On 01/09/14 15:57, Hanjun Guo wrote:
>>> From: Tomasz Nowicki
>>>
>>> ACPI kernel uses MADT table for proper GIC initialization. It needs to
>>> parse GIC related subtables, collect CPU interface and distributor
>>> addresses and call driver initialization function (which is hardware
>>> abstraction agnostic). In a similar way, FDT initialize GICv1/2.
>>>
>>> NOTE: This commit allow to initialize GICv1/2 only.
>>
>> I cannot help but notice that there is no support for KVM here. It'd be
>> good to add a note to that effect, so that people do not expect
>> virtualization support to be working when booting with ACPI.
>
> yes, it is worth mentioning!
>
>>
>>> Signed-off-by: Tomasz Nowicki
>>> Signed-off-by: Hanjun Guo
>>> ---
>>> arch/arm64/include/asm/acpi.h|2 -
>>> arch/arm64/kernel/acpi.c | 23 +++
>>> arch/arm64/kernel/irq.c |5 ++
>>> drivers/irqchip/irq-gic.c| 114
>>> ++
>>> include/linux/irqchip/arm-gic-acpi.h | 33 ++
>>> 5 files changed, 175 insertions(+), 2 deletions(-)
>>> create mode 100644 include/linux/irqchip/arm-gic-acpi.h
>>>
>>> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
>>> index a867467..5d2ab63 100644
>>> --- a/arch/arm64/include/asm/acpi.h
>>> +++ b/arch/arm64/include/asm/acpi.h
>>> @@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void);
>>> extern int (*acpi_suspend_lowlevel)(void);
>>> #define acpi_wakeup_address 0
>>>
>>> -#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535
>>> -
>>> #else
>>>
>>> static inline bool acpi_psci_present(void) { return false; }
>>> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
>>> index 354b912..b3b82b0 100644
>>> --- a/arch/arm64/kernel/acpi.c
>>> +++ b/arch/arm64/kernel/acpi.c
>>> @@ -23,6 +23,7 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>>
>>> #include
>>> #include
>>> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
>>> pr_err("Can't find FADT or error happened during parsing
>>> FADT\n");
>>> }
>>>
>>> +void __init acpi_gic_init(void)
>>> +{
>>> +struct acpi_table_header *table;
>>> +acpi_status status;
>>> +acpi_size tbl_size;
>>> +int err;
>>> +
>>> +status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, , _size);
>>> +if (ACPI_FAILURE(status)) {
>>> +const char *msg = acpi_format_exception(status);
>>> +
>>> +pr_err("Failed to get MADT table, %s\n", msg);
>>> +return;
>>> +}
>>> +
>>> +err = gic_v2_acpi_init(table);
>>> +if (err)
>>> +pr_err("Failed to initialize GIC IRQ controller");
>>
>> What will happen when you get to implement GICv3 support? Another entry
>> like this? Why isn't this entirely contained in the GIC driver? Do I
>> sound like a stuck record?
>
> There will be another call to GICv3 init:
> [...]
> err = gic_v3_acpi_init(table);
> if (err)
> err = gic_v2_acpi_init(table);
> if (err)
> pr_err("Failed to initialize GIC IRQ controller");
> [...]
> This is the main reason I put common code here.
>
>>
>>> +
>>> +early_acpi_os_unmap_memory((char *)table, tbl_size);
>>> +}
>>> +
>>> /*
>>>* acpi_suspend_lowlevel() - save kernel state and suspend.
>>>*
>>> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
>>> index 0f08dfd..c074d60 100644
>>> --- a/arch/arm64/kernel/irq.c
>>> +++ b/arch/arm64/kernel/irq.c
>>> @@ -28,6 +28,7 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>>
>>> unsigned long irq_err_count;
>>>
>>> @@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct
>>> pt_regs *))
>>> void __init init_IRQ(void)
>>> {
>>> irqchip_init();
>>> +
>>> +if (!handle_arch_irq)
>>> +acpi_gic_init();
>>> +
>>
>> Why isn't this called from irqchip_init? It would seem like the logical
>> spot to probe an interrupt controller.
>
> irqchip.c is OF dependent, I want to decouple these from the very
> beginning.
No. irqchip.c is not OF dependent, it is just that DT is the only thing
we support so far. I don't think duplicating the kernel infrastructure
"because we're different" is the right way.
There is no reason for your probing structure to be artificially
different (you're parsing the same information, at the same time). Just
put in place a similar probing mechanism, and this will look a lot better.
>>
>>> if (!handle_arch_irq)
>>> panic("No interrupt controller found.");
>>> }
>>> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
>>> index 4b959e6..85cbf43 100644
>>> --- a/drivers/irqchip/irq-gic.c
>>> +++ b/drivers/irqchip/irq-gic.c
>>> @@ -33,12 +33,14 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>> #include
>>> #include
>>> #include
Re: [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 01.09.2014 19:35, Marc Zyngier wrote:
On 01/09/14 15:57, Hanjun Guo wrote:
From: Tomasz Nowicki
ACPI kernel uses MADT table for proper GIC initialization. It needs to
parse GIC related subtables, collect CPU interface and distributor
addresses and call driver initialization function (which is hardware
abstraction agnostic). In a similar way, FDT initialize GICv1/2.
NOTE: This commit allow to initialize GICv1/2 only.
I cannot help but notice that there is no support for KVM here. It'd be
good to add a note to that effect, so that people do not expect
virtualization support to be working when booting with ACPI.
yes, it is worth mentioning!
Signed-off-by: Tomasz Nowicki
Signed-off-by: Hanjun Guo
---
arch/arm64/include/asm/acpi.h|2 -
arch/arm64/kernel/acpi.c | 23 +++
arch/arm64/kernel/irq.c |5 ++
drivers/irqchip/irq-gic.c| 114 ++
include/linux/irqchip/arm-gic-acpi.h | 33 ++
5 files changed, 175 insertions(+), 2 deletions(-)
create mode 100644 include/linux/irqchip/arm-gic-acpi.h
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index a867467..5d2ab63 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void);
extern int (*acpi_suspend_lowlevel)(void);
#define acpi_wakeup_address 0
-#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535
-
#else
static inline bool acpi_psci_present(void) { return false; }
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 354b912..b3b82b0 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -23,6 +23,7 @@
#include
#include
#include
+#include
#include
#include
@@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
pr_err("Can't find FADT or error happened during parsing
FADT\n");
}
+void __init acpi_gic_init(void)
+{
+ struct acpi_table_header *table;
+ acpi_status status;
+ acpi_size tbl_size;
+ int err;
+
+ status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, , _size);
+ if (ACPI_FAILURE(status)) {
+ const char *msg = acpi_format_exception(status);
+
+ pr_err("Failed to get MADT table, %s\n", msg);
+ return;
+ }
+
+ err = gic_v2_acpi_init(table);
+ if (err)
+ pr_err("Failed to initialize GIC IRQ controller");
What will happen when you get to implement GICv3 support? Another entry
like this? Why isn't this entirely contained in the GIC driver? Do I
sound like a stuck record?
There will be another call to GICv3 init:
[...]
err = gic_v3_acpi_init(table);
if (err)
err = gic_v2_acpi_init(table);
if (err)
pr_err("Failed to initialize GIC IRQ controller");
[...]
This is the main reason I put common code here.
+
+ early_acpi_os_unmap_memory((char *)table, tbl_size);
+}
+
/*
* acpi_suspend_lowlevel() - save kernel state and suspend.
*
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 0f08dfd..c074d60 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
unsigned long irq_err_count;
@@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs
*))
void __init init_IRQ(void)
{
irqchip_init();
+
+ if (!handle_arch_irq)
+ acpi_gic_init();
+
Why isn't this called from irqchip_init? It would seem like the logical
spot to probe an interrupt controller.
irqchip.c is OF dependent, I want to decouple these from the very
beginning.
if (!handle_arch_irq)
panic("No interrupt controller found.");
}
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4b959e6..85cbf43 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -33,12 +33,14 @@
#include
#include
#include
+#include
#include
#include
#include
#include
#include
#include
+#include
#include
#include
@@ -1029,3 +1031,115 @@ IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic",
gic_of_init);
IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init);
#endif
+
+#ifdef CONFIG_ACPI
+static u64 dist_phy_base, cpu_phy_base = ULONG_MAX;
Please use phys_addr_t for physical addresses. The use of ULONG_MAX
looks dodgy. Please have a proper symbol to flag the fact that it hasn't
been assigned yet.
Sure, will do.
+
+static int __init
+gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
+ const unsigned long end)
+{
+ struct acpi_madt_generic_interrupt *processor;
+ u64 gic_cpu_base;
phys_addr_t
+ processor = (struct acpi_madt_generic_interrupt *)header;
+
+ if (BAD_MADT_ENTRY(processor, end))
+ return -EINVAL;
+
+
Re: [Linaro-acpi] [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 1 September 2014 19:35, Marc Zyngier wrote: > > On 01/09/14 15:57, Hanjun Guo wrote: > > From: Tomasz Nowicki > > > > ACPI kernel uses MADT table for proper GIC initialization. It needs to > > parse GIC related subtables, collect CPU interface and distributor > > addresses and call driver initialization function (which is hardware > > abstraction agnostic). In a similar way, FDT initialize GICv1/2. > > > > NOTE: This commit allow to initialize GICv1/2 only. > > I cannot help but notice that there is no support for KVM here. It'd be > good to add a note to that effect, so that people do not expect > virtualization support to be working when booting with ACPI. Just a heads up for this, I run up to this problem too while testing and by forcing some values for vgic and arch_timers I could boot a QEMU guest without problems. I'll try and see if it will be easy to handle this more properly instead of hardcoded values. Regards. -- 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: [Linaro-acpi] [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 1 September 2014 19:35, Marc Zyngier marc.zyng...@arm.com wrote: On 01/09/14 15:57, Hanjun Guo wrote: From: Tomasz Nowicki tomasz.nowi...@linaro.org ACPI kernel uses MADT table for proper GIC initialization. It needs to parse GIC related subtables, collect CPU interface and distributor addresses and call driver initialization function (which is hardware abstraction agnostic). In a similar way, FDT initialize GICv1/2. NOTE: This commit allow to initialize GICv1/2 only. I cannot help but notice that there is no support for KVM here. It'd be good to add a note to that effect, so that people do not expect virtualization support to be working when booting with ACPI. Just a heads up for this, I run up to this problem too while testing and by forcing some values for vgic and arch_timers I could boot a QEMU guest without problems. I'll try and see if it will be easy to handle this more properly instead of hardcoded values. Regards. -- 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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 01.09.2014 19:35, Marc Zyngier wrote:
On 01/09/14 15:57, Hanjun Guo wrote:
From: Tomasz Nowicki tomasz.nowi...@linaro.org
ACPI kernel uses MADT table for proper GIC initialization. It needs to
parse GIC related subtables, collect CPU interface and distributor
addresses and call driver initialization function (which is hardware
abstraction agnostic). In a similar way, FDT initialize GICv1/2.
NOTE: This commit allow to initialize GICv1/2 only.
I cannot help but notice that there is no support for KVM here. It'd be
good to add a note to that effect, so that people do not expect
virtualization support to be working when booting with ACPI.
yes, it is worth mentioning!
Signed-off-by: Tomasz Nowicki tomasz.nowi...@linaro.org
Signed-off-by: Hanjun Guo hanjun@linaro.org
---
arch/arm64/include/asm/acpi.h|2 -
arch/arm64/kernel/acpi.c | 23 +++
arch/arm64/kernel/irq.c |5 ++
drivers/irqchip/irq-gic.c| 114 ++
include/linux/irqchip/arm-gic-acpi.h | 33 ++
5 files changed, 175 insertions(+), 2 deletions(-)
create mode 100644 include/linux/irqchip/arm-gic-acpi.h
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index a867467..5d2ab63 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void);
extern int (*acpi_suspend_lowlevel)(void);
#define acpi_wakeup_address 0
-#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535
-
#else
static inline bool acpi_psci_present(void) { return false; }
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 354b912..b3b82b0 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -23,6 +23,7 @@
#include linux/irqdomain.h
#include linux/bootmem.h
#include linux/smp.h
+#include linux/irqchip/arm-gic-acpi.h
#include asm/cputype.h
#include asm/cpu_ops.h
@@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
pr_err(Can't find FADT or error happened during parsing
FADT\n);
}
+void __init acpi_gic_init(void)
+{
+ struct acpi_table_header *table;
+ acpi_status status;
+ acpi_size tbl_size;
+ int err;
+
+ status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, table, tbl_size);
+ if (ACPI_FAILURE(status)) {
+ const char *msg = acpi_format_exception(status);
+
+ pr_err(Failed to get MADT table, %s\n, msg);
+ return;
+ }
+
+ err = gic_v2_acpi_init(table);
+ if (err)
+ pr_err(Failed to initialize GIC IRQ controller);
What will happen when you get to implement GICv3 support? Another entry
like this? Why isn't this entirely contained in the GIC driver? Do I
sound like a stuck record?
There will be another call to GICv3 init:
[...]
err = gic_v3_acpi_init(table);
if (err)
err = gic_v2_acpi_init(table);
if (err)
pr_err(Failed to initialize GIC IRQ controller);
[...]
This is the main reason I put common code here.
+
+ early_acpi_os_unmap_memory((char *)table, tbl_size);
+}
+
/*
* acpi_suspend_lowlevel() - save kernel state and suspend.
*
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 0f08dfd..c074d60 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -28,6 +28,7 @@
#include linux/irqchip.h
#include linux/seq_file.h
#include linux/ratelimit.h
+#include linux/irqchip/arm-gic-acpi.h
unsigned long irq_err_count;
@@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs
*))
void __init init_IRQ(void)
{
irqchip_init();
+
+ if (!handle_arch_irq)
+ acpi_gic_init();
+
Why isn't this called from irqchip_init? It would seem like the logical
spot to probe an interrupt controller.
irqchip.c is OF dependent, I want to decouple these from the very
beginning.
if (!handle_arch_irq)
panic(No interrupt controller found.);
}
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4b959e6..85cbf43 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -33,12 +33,14 @@
#include linux/of.h
#include linux/of_address.h
#include linux/of_irq.h
+#include linux/acpi.h
#include linux/irqdomain.h
#include linux/interrupt.h
#include linux/percpu.h
#include linux/slab.h
#include linux/irqchip/chained_irq.h
#include linux/irqchip/arm-gic.h
+#include linux/irqchip/arm-gic-acpi.h
#include asm/cputype.h
#include asm/irq.h
@@ -1029,3 +1031,115 @@ IRQCHIP_DECLARE(msm_8660_qgic, qcom,msm-8660-qgic,
gic_of_init);
IRQCHIP_DECLARE(msm_qgic2, qcom,msm-qgic2, gic_of_init);
#endif
+
+#ifdef CONFIG_ACPI
+static u64 dist_phy_base, cpu_phy_base = ULONG_MAX;
Please use phys_addr_t for physical addresses. The use of ULONG_MAX
looks dodgy. Please have a proper symbol to
Re: [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 02/09/14 12:48, Tomasz Nowicki wrote:
On 01.09.2014 19:35, Marc Zyngier wrote:
On 01/09/14 15:57, Hanjun Guo wrote:
From: Tomasz Nowicki tomasz.nowi...@linaro.org
ACPI kernel uses MADT table for proper GIC initialization. It needs to
parse GIC related subtables, collect CPU interface and distributor
addresses and call driver initialization function (which is hardware
abstraction agnostic). In a similar way, FDT initialize GICv1/2.
NOTE: This commit allow to initialize GICv1/2 only.
I cannot help but notice that there is no support for KVM here. It'd be
good to add a note to that effect, so that people do not expect
virtualization support to be working when booting with ACPI.
yes, it is worth mentioning!
Signed-off-by: Tomasz Nowicki tomasz.nowi...@linaro.org
Signed-off-by: Hanjun Guo hanjun@linaro.org
---
arch/arm64/include/asm/acpi.h|2 -
arch/arm64/kernel/acpi.c | 23 +++
arch/arm64/kernel/irq.c |5 ++
drivers/irqchip/irq-gic.c| 114
++
include/linux/irqchip/arm-gic-acpi.h | 33 ++
5 files changed, 175 insertions(+), 2 deletions(-)
create mode 100644 include/linux/irqchip/arm-gic-acpi.h
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index a867467..5d2ab63 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void);
extern int (*acpi_suspend_lowlevel)(void);
#define acpi_wakeup_address 0
-#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535
-
#else
static inline bool acpi_psci_present(void) { return false; }
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 354b912..b3b82b0 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -23,6 +23,7 @@
#include linux/irqdomain.h
#include linux/bootmem.h
#include linux/smp.h
+#include linux/irqchip/arm-gic-acpi.h
#include asm/cputype.h
#include asm/cpu_ops.h
@@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
pr_err(Can't find FADT or error happened during parsing
FADT\n);
}
+void __init acpi_gic_init(void)
+{
+struct acpi_table_header *table;
+acpi_status status;
+acpi_size tbl_size;
+int err;
+
+status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, table, tbl_size);
+if (ACPI_FAILURE(status)) {
+const char *msg = acpi_format_exception(status);
+
+pr_err(Failed to get MADT table, %s\n, msg);
+return;
+}
+
+err = gic_v2_acpi_init(table);
+if (err)
+pr_err(Failed to initialize GIC IRQ controller);
What will happen when you get to implement GICv3 support? Another entry
like this? Why isn't this entirely contained in the GIC driver? Do I
sound like a stuck record?
There will be another call to GICv3 init:
[...]
err = gic_v3_acpi_init(table);
if (err)
err = gic_v2_acpi_init(table);
if (err)
pr_err(Failed to initialize GIC IRQ controller);
[...]
This is the main reason I put common code here.
+
+early_acpi_os_unmap_memory((char *)table, tbl_size);
+}
+
/*
* acpi_suspend_lowlevel() - save kernel state and suspend.
*
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 0f08dfd..c074d60 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -28,6 +28,7 @@
#include linux/irqchip.h
#include linux/seq_file.h
#include linux/ratelimit.h
+#include linux/irqchip/arm-gic-acpi.h
unsigned long irq_err_count;
@@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct
pt_regs *))
void __init init_IRQ(void)
{
irqchip_init();
+
+if (!handle_arch_irq)
+acpi_gic_init();
+
Why isn't this called from irqchip_init? It would seem like the logical
spot to probe an interrupt controller.
irqchip.c is OF dependent, I want to decouple these from the very
beginning.
No. irqchip.c is not OF dependent, it is just that DT is the only thing
we support so far. I don't think duplicating the kernel infrastructure
because we're different is the right way.
There is no reason for your probing structure to be artificially
different (you're parsing the same information, at the same time). Just
put in place a similar probing mechanism, and this will look a lot better.
if (!handle_arch_irq)
panic(No interrupt controller found.);
}
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4b959e6..85cbf43 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -33,12 +33,14 @@
#include linux/of.h
#include linux/of_address.h
#include linux/of_irq.h
+#include linux/acpi.h
#include linux/irqdomain.h
#include linux/interrupt.h
#include linux/percpu.h
#include linux/slab.h
#include
Re: [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 2014年09月02日 21:02, Marc Zyngier wrote:
On 02/09/14 12:48, Tomasz Nowicki wrote:
On 01.09.2014 19:35, Marc Zyngier wrote:
On 01/09/14 15:57, Hanjun Guo wrote:
From: Tomasz Nowicki tomasz.nowi...@linaro.org
ACPI kernel uses MADT table for proper GIC initialization. It needs to
parse GIC related subtables, collect CPU interface and distributor
addresses and call driver initialization function (which is hardware
abstraction agnostic). In a similar way, FDT initialize GICv1/2.
NOTE: This commit allow to initialize GICv1/2 only.
I cannot help but notice that there is no support for KVM here. It'd be
good to add a note to that effect, so that people do not expect
virtualization support to be working when booting with ACPI.
yes, it is worth mentioning!
Signed-off-by: Tomasz Nowicki tomasz.nowi...@linaro.org
Signed-off-by: Hanjun Guo hanjun@linaro.org
---
arch/arm64/include/asm/acpi.h|2 -
arch/arm64/kernel/acpi.c | 23 +++
arch/arm64/kernel/irq.c |5 ++
drivers/irqchip/irq-gic.c| 114
++
include/linux/irqchip/arm-gic-acpi.h | 33 ++
5 files changed, 175 insertions(+), 2 deletions(-)
create mode 100644 include/linux/irqchip/arm-gic-acpi.h
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index a867467..5d2ab63 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void);
extern int (*acpi_suspend_lowlevel)(void);
#define acpi_wakeup_address 0
-#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535
-
#else
static inline bool acpi_psci_present(void) { return false; }
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 354b912..b3b82b0 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -23,6 +23,7 @@
#include linux/irqdomain.h
#include linux/bootmem.h
#include linux/smp.h
+#include linux/irqchip/arm-gic-acpi.h
#include asm/cputype.h
#include asm/cpu_ops.h
@@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
pr_err(Can't find FADT or error happened during parsing
FADT\n);
}
+void __init acpi_gic_init(void)
+{
+struct acpi_table_header *table;
+acpi_status status;
+acpi_size tbl_size;
+int err;
+
+status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, table,
tbl_size);
+if (ACPI_FAILURE(status)) {
+const char *msg = acpi_format_exception(status);
+
+pr_err(Failed to get MADT table, %s\n, msg);
+return;
+}
+
+err = gic_v2_acpi_init(table);
+if (err)
+pr_err(Failed to initialize GIC IRQ controller);
What will happen when you get to implement GICv3 support? Another entry
like this? Why isn't this entirely contained in the GIC driver? Do I
sound like a stuck record?
There will be another call to GICv3 init:
[...]
err = gic_v3_acpi_init(table);
if (err)
err = gic_v2_acpi_init(table);
if (err)
pr_err(Failed to initialize GIC IRQ controller);
[...]
This is the main reason I put common code here.
+
+early_acpi_os_unmap_memory((char *)table, tbl_size);
+}
+
/*
* acpi_suspend_lowlevel() - save kernel state and suspend.
*
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 0f08dfd..c074d60 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -28,6 +28,7 @@
#include linux/irqchip.h
#include linux/seq_file.h
#include linux/ratelimit.h
+#include linux/irqchip/arm-gic-acpi.h
unsigned long irq_err_count;
@@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct
pt_regs *))
void __init init_IRQ(void)
{
irqchip_init();
+
+if (!handle_arch_irq)
+acpi_gic_init();
+
Why isn't this called from irqchip_init? It would seem like the logical
spot to probe an interrupt controller.
irqchip.c is OF dependent, I want to decouple these from the very
beginning.
No. irqchip.c is not OF dependent, it is just that DT is the only thing
we support so far. I don't think duplicating the kernel infrastructure
because we're different is the right way.
There is no reason for your probing structure to be artificially
different (you're parsing the same information, at the same time). Just
put in place a similar probing mechanism, and this will look a lot better.
if (!handle_arch_irq)
panic(No interrupt controller found.);
}
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4b959e6..85cbf43 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -33,12 +33,14 @@
#include linux/of.h
#include linux/of_address.h
#include linux/of_irq.h
+#include linux/acpi.h
#include linux/irqdomain.h
#include linux/interrupt.h
#include linux/percpu.h
Re: [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 02/09/14 16:45, Hanjun Guo wrote:
On 2014年09月02日 21:02, Marc Zyngier wrote:
On 02/09/14 12:48, Tomasz Nowicki wrote:
On 01.09.2014 19:35, Marc Zyngier wrote:
On 01/09/14 15:57, Hanjun Guo wrote:
From: Tomasz Nowicki tomasz.nowi...@linaro.org
ACPI kernel uses MADT table for proper GIC initialization. It needs to
parse GIC related subtables, collect CPU interface and distributor
addresses and call driver initialization function (which is hardware
abstraction agnostic). In a similar way, FDT initialize GICv1/2.
NOTE: This commit allow to initialize GICv1/2 only.
I cannot help but notice that there is no support for KVM here. It'd be
good to add a note to that effect, so that people do not expect
virtualization support to be working when booting with ACPI.
yes, it is worth mentioning!
Signed-off-by: Tomasz Nowicki tomasz.nowi...@linaro.org
Signed-off-by: Hanjun Guo hanjun@linaro.org
---
arch/arm64/include/asm/acpi.h|2 -
arch/arm64/kernel/acpi.c | 23 +++
arch/arm64/kernel/irq.c |5 ++
drivers/irqchip/irq-gic.c| 114
++
include/linux/irqchip/arm-gic-acpi.h | 33 ++
5 files changed, 175 insertions(+), 2 deletions(-)
create mode 100644 include/linux/irqchip/arm-gic-acpi.h
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index a867467..5d2ab63 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void);
extern int (*acpi_suspend_lowlevel)(void);
#define acpi_wakeup_address 0
-#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535
-
#else
static inline bool acpi_psci_present(void) { return false; }
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 354b912..b3b82b0 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -23,6 +23,7 @@
#include linux/irqdomain.h
#include linux/bootmem.h
#include linux/smp.h
+#include linux/irqchip/arm-gic-acpi.h
#include asm/cputype.h
#include asm/cpu_ops.h
@@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
pr_err(Can't find FADT or error happened during parsing
FADT\n);
}
+void __init acpi_gic_init(void)
+{
+struct acpi_table_header *table;
+acpi_status status;
+acpi_size tbl_size;
+int err;
+
+status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, table,
tbl_size);
+if (ACPI_FAILURE(status)) {
+const char *msg = acpi_format_exception(status);
+
+pr_err(Failed to get MADT table, %s\n, msg);
+return;
+}
+
+err = gic_v2_acpi_init(table);
+if (err)
+pr_err(Failed to initialize GIC IRQ controller);
What will happen when you get to implement GICv3 support? Another entry
like this? Why isn't this entirely contained in the GIC driver? Do I
sound like a stuck record?
There will be another call to GICv3 init:
[...]
err = gic_v3_acpi_init(table);
if (err)
err = gic_v2_acpi_init(table);
if (err)
pr_err(Failed to initialize GIC IRQ controller);
[...]
This is the main reason I put common code here.
+
+early_acpi_os_unmap_memory((char *)table, tbl_size);
+}
+
/*
* acpi_suspend_lowlevel() - save kernel state and suspend.
*
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 0f08dfd..c074d60 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -28,6 +28,7 @@
#include linux/irqchip.h
#include linux/seq_file.h
#include linux/ratelimit.h
+#include linux/irqchip/arm-gic-acpi.h
unsigned long irq_err_count;
@@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct
pt_regs *))
void __init init_IRQ(void)
{
irqchip_init();
+
+if (!handle_arch_irq)
+acpi_gic_init();
+
Why isn't this called from irqchip_init? It would seem like the logical
spot to probe an interrupt controller.
irqchip.c is OF dependent, I want to decouple these from the very
beginning.
No. irqchip.c is not OF dependent, it is just that DT is the only thing
we support so far. I don't think duplicating the kernel infrastructure
because we're different is the right way.
There is no reason for your probing structure to be artificially
different (you're parsing the same information, at the same time). Just
put in place a similar probing mechanism, and this will look a lot better.
if (!handle_arch_irq)
panic(No interrupt controller found.);
}
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4b959e6..85cbf43 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -33,12 +33,14 @@
#include linux/of.h
#include linux/of_address.h
#include linux/of_irq.h
+#include linux/acpi.h
#include linux/irqdomain.h
#include
Re: [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 02/09/14 16:45, Hanjun Guo wrote:
On 2014年09月02日 21:02, Marc Zyngier wrote:
On 02/09/14 12:48, Tomasz Nowicki wrote:
On 01.09.2014 19:35, Marc Zyngier wrote:
On 01/09/14 15:57, Hanjun Guo wrote:
From: Tomasz Nowicki tomasz.nowi...@linaro.org
ACPI kernel uses MADT table for proper GIC initialization. It needs to
parse GIC related subtables, collect CPU interface and distributor
addresses and call driver initialization function (which is hardware
abstraction agnostic). In a similar way, FDT initialize GICv1/2.
NOTE: This commit allow to initialize GICv1/2 only.
I cannot help but notice that there is no support for KVM here. It'd be
good to add a note to that effect, so that people do not expect
virtualization support to be working when booting with ACPI.
yes, it is worth mentioning!
Signed-off-by: Tomasz Nowicki tomasz.nowi...@linaro.org
Signed-off-by: Hanjun Guo hanjun@linaro.org
---
arch/arm64/include/asm/acpi.h|2 -
arch/arm64/kernel/acpi.c | 23 +++
arch/arm64/kernel/irq.c |5 ++
drivers/irqchip/irq-gic.c| 114
++
include/linux/irqchip/arm-gic-acpi.h | 33 ++
5 files changed, 175 insertions(+), 2 deletions(-)
create mode 100644 include/linux/irqchip/arm-gic-acpi.h
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index a867467..5d2ab63 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void);
extern int (*acpi_suspend_lowlevel)(void);
#define acpi_wakeup_address 0
-#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535
-
#else
static inline bool acpi_psci_present(void) { return false; }
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 354b912..b3b82b0 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -23,6 +23,7 @@
#include linux/irqdomain.h
#include linux/bootmem.h
#include linux/smp.h
+#include linux/irqchip/arm-gic-acpi.h
#include asm/cputype.h
#include asm/cpu_ops.h
@@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
pr_err(Can't find FADT or error happened during parsing FADT\n);
}
+void __init acpi_gic_init(void)
+{
+struct acpi_table_header *table;
+acpi_status status;
+acpi_size tbl_size;
+int err;
+
+status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, table, tbl_size);
+if (ACPI_FAILURE(status)) {
+const char *msg = acpi_format_exception(status);
+
+pr_err(Failed to get MADT table, %s\n, msg);
+return;
+}
+
+err = gic_v2_acpi_init(table);
+if (err)
+pr_err(Failed to initialize GIC IRQ controller);
What will happen when you get to implement GICv3 support? Another entry
like this? Why isn't this entirely contained in the GIC driver? Do I
sound like a stuck record?
There will be another call to GICv3 init:
[...]
err = gic_v3_acpi_init(table);
if (err)
err = gic_v2_acpi_init(table);
if (err)
pr_err(Failed to initialize GIC IRQ controller);
[...]
This is the main reason I put common code here.
+
+early_acpi_os_unmap_memory((char *)table, tbl_size);
+}
+
/*
* acpi_suspend_lowlevel() - save kernel state and suspend.
*
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 0f08dfd..c074d60 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -28,6 +28,7 @@
#include linux/irqchip.h
#include linux/seq_file.h
#include linux/ratelimit.h
+#include linux/irqchip/arm-gic-acpi.h
unsigned long irq_err_count;
@@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs
*))
void __init init_IRQ(void)
{
irqchip_init();
+
+if (!handle_arch_irq)
+acpi_gic_init();
+
Why isn't this called from irqchip_init? It would seem like the logical
spot to probe an interrupt controller.
irqchip.c is OF dependent, I want to decouple these from the very
beginning.
No. irqchip.c is not OF dependent, it is just that DT is the only thing
we support so far. I don't think duplicating the kernel infrastructure
because we're different is the right way.
There is no reason for your probing structure to be artificially
different (you're parsing the same information, at the same time). Just
put in place a similar probing mechanism, and this will look a lot better.
if (!handle_arch_irq)
panic(No interrupt controller found.);
}
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4b959e6..85cbf43 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -33,12 +33,14 @@
#include linux/of.h
#include linux/of_address.h
#include linux/of_irq.h
+#include linux/acpi.h
#include linux/irqdomain.h
#include linux/interrupt.h
#include linux/percpu.h
#include linux/slab.h
#include
Re: [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On Tue, Sep 02, 2014 at 12:48:37PM +0100, Tomasz Nowicki wrote: On 01.09.2014 19:35, Marc Zyngier wrote: On 01/09/14 15:57, Hanjun Guo wrote: From: Tomasz Nowicki tomasz.nowi...@linaro.org ACPI kernel uses MADT table for proper GIC initialization. It needs to parse GIC related subtables, collect CPU interface and distributor addresses and call driver initialization function (which is hardware abstraction agnostic). In a similar way, FDT initialize GICv1/2. NOTE: This commit allow to initialize GICv1/2 only. I cannot help but notice that there is no support for KVM here. It'd be good to add a note to that effect, so that people do not expect virtualization support to be working when booting with ACPI. yes, it is worth mentioning! It's also worth mentioning if there are any plans to fix this ;). -- Catalin -- 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 v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 01/09/14 15:57, Hanjun Guo wrote:
> From: Tomasz Nowicki
>
> ACPI kernel uses MADT table for proper GIC initialization. It needs to
> parse GIC related subtables, collect CPU interface and distributor
> addresses and call driver initialization function (which is hardware
> abstraction agnostic). In a similar way, FDT initialize GICv1/2.
>
> NOTE: This commit allow to initialize GICv1/2 only.
I cannot help but notice that there is no support for KVM here. It'd be
good to add a note to that effect, so that people do not expect
virtualization support to be working when booting with ACPI.
> Signed-off-by: Tomasz Nowicki
> Signed-off-by: Hanjun Guo
> ---
> arch/arm64/include/asm/acpi.h|2 -
> arch/arm64/kernel/acpi.c | 23 +++
> arch/arm64/kernel/irq.c |5 ++
> drivers/irqchip/irq-gic.c| 114
> ++
> include/linux/irqchip/arm-gic-acpi.h | 33 ++
> 5 files changed, 175 insertions(+), 2 deletions(-)
> create mode 100644 include/linux/irqchip/arm-gic-acpi.h
>
> diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
> index a867467..5d2ab63 100644
> --- a/arch/arm64/include/asm/acpi.h
> +++ b/arch/arm64/include/asm/acpi.h
> @@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void);
> extern int (*acpi_suspend_lowlevel)(void);
> #define acpi_wakeup_address 0
>
> -#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535
> -
> #else
>
> static inline bool acpi_psci_present(void) { return false; }
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> index 354b912..b3b82b0 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -23,6 +23,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
> pr_err("Can't find FADT or error happened during parsing
> FADT\n");
> }
>
> +void __init acpi_gic_init(void)
> +{
> + struct acpi_table_header *table;
> + acpi_status status;
> + acpi_size tbl_size;
> + int err;
> +
> + status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, , _size);
> + if (ACPI_FAILURE(status)) {
> + const char *msg = acpi_format_exception(status);
> +
> + pr_err("Failed to get MADT table, %s\n", msg);
> + return;
> + }
> +
> + err = gic_v2_acpi_init(table);
> + if (err)
> + pr_err("Failed to initialize GIC IRQ controller");
What will happen when you get to implement GICv3 support? Another entry
like this? Why isn't this entirely contained in the GIC driver? Do I
sound like a stuck record?
> +
> + early_acpi_os_unmap_memory((char *)table, tbl_size);
> +}
> +
> /*
> * acpi_suspend_lowlevel() - save kernel state and suspend.
> *
> diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
> index 0f08dfd..c074d60 100644
> --- a/arch/arm64/kernel/irq.c
> +++ b/arch/arm64/kernel/irq.c
> @@ -28,6 +28,7 @@
> #include
> #include
> #include
> +#include
>
> unsigned long irq_err_count;
>
> @@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct
> pt_regs *))
> void __init init_IRQ(void)
> {
> irqchip_init();
> +
> + if (!handle_arch_irq)
> + acpi_gic_init();
> +
Why isn't this called from irqchip_init? It would seem like the logical
spot to probe an interrupt controller.
> if (!handle_arch_irq)
> panic("No interrupt controller found.");
> }
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 4b959e6..85cbf43 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -33,12 +33,14 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -1029,3 +1031,115 @@ IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic",
> gic_of_init);
> IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init);
>
> #endif
> +
> +#ifdef CONFIG_ACPI
> +static u64 dist_phy_base, cpu_phy_base = ULONG_MAX;
Please use phys_addr_t for physical addresses. The use of ULONG_MAX
looks dodgy. Please have a proper symbol to flag the fact that it hasn't
been assigned yet.
> +
> +static int __init
> +gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
> + const unsigned long end)
> +{
> + struct acpi_madt_generic_interrupt *processor;
> + u64 gic_cpu_base;
phys_addr_t
> + processor = (struct acpi_madt_generic_interrupt *)header;
> +
> + if (BAD_MADT_ENTRY(processor, end))
> + return -EINVAL;
> +
> + gic_cpu_base = processor->base_address;
> + if (!gic_cpu_base)
> + return -EFAULT;
Is zero an invalid address?
> +
> + /*
> + * There is no support for non-banked GICv1/2 register in ACPI spec.
> + * All CPU interface addresses have to be the same.
> +
[PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
From: Tomasz Nowicki
ACPI kernel uses MADT table for proper GIC initialization. It needs to
parse GIC related subtables, collect CPU interface and distributor
addresses and call driver initialization function (which is hardware
abstraction agnostic). In a similar way, FDT initialize GICv1/2.
NOTE: This commit allow to initialize GICv1/2 only.
Signed-off-by: Tomasz Nowicki
Signed-off-by: Hanjun Guo
---
arch/arm64/include/asm/acpi.h|2 -
arch/arm64/kernel/acpi.c | 23 +++
arch/arm64/kernel/irq.c |5 ++
drivers/irqchip/irq-gic.c| 114 ++
include/linux/irqchip/arm-gic-acpi.h | 33 ++
5 files changed, 175 insertions(+), 2 deletions(-)
create mode 100644 include/linux/irqchip/arm-gic-acpi.h
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index a867467..5d2ab63 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void);
extern int (*acpi_suspend_lowlevel)(void);
#define acpi_wakeup_address 0
-#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535
-
#else
static inline bool acpi_psci_present(void) { return false; }
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 354b912..b3b82b0 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -23,6 +23,7 @@
#include
#include
#include
+#include
#include
#include
@@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
pr_err("Can't find FADT or error happened during parsing
FADT\n");
}
+void __init acpi_gic_init(void)
+{
+ struct acpi_table_header *table;
+ acpi_status status;
+ acpi_size tbl_size;
+ int err;
+
+ status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, , _size);
+ if (ACPI_FAILURE(status)) {
+ const char *msg = acpi_format_exception(status);
+
+ pr_err("Failed to get MADT table, %s\n", msg);
+ return;
+ }
+
+ err = gic_v2_acpi_init(table);
+ if (err)
+ pr_err("Failed to initialize GIC IRQ controller");
+
+ early_acpi_os_unmap_memory((char *)table, tbl_size);
+}
+
/*
* acpi_suspend_lowlevel() - save kernel state and suspend.
*
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 0f08dfd..c074d60 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
unsigned long irq_err_count;
@@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs
*))
void __init init_IRQ(void)
{
irqchip_init();
+
+ if (!handle_arch_irq)
+ acpi_gic_init();
+
if (!handle_arch_irq)
panic("No interrupt controller found.");
}
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4b959e6..85cbf43 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -33,12 +33,14 @@
#include
#include
#include
+#include
#include
#include
#include
#include
#include
#include
+#include
#include
#include
@@ -1029,3 +1031,115 @@ IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic",
gic_of_init);
IRQCHIP_DECLARE(msm_qgic2, "qcom,msm-qgic2", gic_of_init);
#endif
+
+#ifdef CONFIG_ACPI
+static u64 dist_phy_base, cpu_phy_base = ULONG_MAX;
+
+static int __init
+gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
+ const unsigned long end)
+{
+ struct acpi_madt_generic_interrupt *processor;
+ u64 gic_cpu_base;
+
+ processor = (struct acpi_madt_generic_interrupt *)header;
+
+ if (BAD_MADT_ENTRY(processor, end))
+ return -EINVAL;
+
+ gic_cpu_base = processor->base_address;
+ if (!gic_cpu_base)
+ return -EFAULT;
+
+ /*
+* There is no support for non-banked GICv1/2 register in ACPI spec.
+* All CPU interface addresses have to be the same.
+*/
+ if (cpu_phy_base != ULONG_MAX && gic_cpu_base != cpu_phy_base)
+ return -EFAULT;
+
+ cpu_phy_base = gic_cpu_base;
+ return 0;
+}
+
+static int __init
+gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header,
+ const unsigned long end)
+{
+ struct acpi_madt_generic_distributor *dist;
+
+ dist = (struct acpi_madt_generic_distributor *)header;
+
+ if (BAD_MADT_ENTRY(dist, end))
+ return -EINVAL;
+
+ dist_phy_base = dist->base_address;
+ if (!dist_phy_base)
+ return -EFAULT;
+
+ return 0;
+}
+
+int __init
+gic_v2_acpi_init(struct acpi_table_header *table)
+{
+ void __iomem *cpu_base, *dist_base;
+ int count;
+
+ /* Collect CPU base addresses */
+ count = acpi_parse_entries(sizeof(struct acpi_table_madt),
+ gic_acpi_parse_madt_cpu, table,
+
[PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
From: Tomasz Nowicki tomasz.nowi...@linaro.org
ACPI kernel uses MADT table for proper GIC initialization. It needs to
parse GIC related subtables, collect CPU interface and distributor
addresses and call driver initialization function (which is hardware
abstraction agnostic). In a similar way, FDT initialize GICv1/2.
NOTE: This commit allow to initialize GICv1/2 only.
Signed-off-by: Tomasz Nowicki tomasz.nowi...@linaro.org
Signed-off-by: Hanjun Guo hanjun@linaro.org
---
arch/arm64/include/asm/acpi.h|2 -
arch/arm64/kernel/acpi.c | 23 +++
arch/arm64/kernel/irq.c |5 ++
drivers/irqchip/irq-gic.c| 114 ++
include/linux/irqchip/arm-gic-acpi.h | 33 ++
5 files changed, 175 insertions(+), 2 deletions(-)
create mode 100644 include/linux/irqchip/arm-gic-acpi.h
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index a867467..5d2ab63 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void);
extern int (*acpi_suspend_lowlevel)(void);
#define acpi_wakeup_address 0
-#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535
-
#else
static inline bool acpi_psci_present(void) { return false; }
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 354b912..b3b82b0 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -23,6 +23,7 @@
#include linux/irqdomain.h
#include linux/bootmem.h
#include linux/smp.h
+#include linux/irqchip/arm-gic-acpi.h
#include asm/cputype.h
#include asm/cpu_ops.h
@@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
pr_err(Can't find FADT or error happened during parsing
FADT\n);
}
+void __init acpi_gic_init(void)
+{
+ struct acpi_table_header *table;
+ acpi_status status;
+ acpi_size tbl_size;
+ int err;
+
+ status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, table, tbl_size);
+ if (ACPI_FAILURE(status)) {
+ const char *msg = acpi_format_exception(status);
+
+ pr_err(Failed to get MADT table, %s\n, msg);
+ return;
+ }
+
+ err = gic_v2_acpi_init(table);
+ if (err)
+ pr_err(Failed to initialize GIC IRQ controller);
+
+ early_acpi_os_unmap_memory((char *)table, tbl_size);
+}
+
/*
* acpi_suspend_lowlevel() - save kernel state and suspend.
*
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 0f08dfd..c074d60 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -28,6 +28,7 @@
#include linux/irqchip.h
#include linux/seq_file.h
#include linux/ratelimit.h
+#include linux/irqchip/arm-gic-acpi.h
unsigned long irq_err_count;
@@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct pt_regs
*))
void __init init_IRQ(void)
{
irqchip_init();
+
+ if (!handle_arch_irq)
+ acpi_gic_init();
+
if (!handle_arch_irq)
panic(No interrupt controller found.);
}
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4b959e6..85cbf43 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -33,12 +33,14 @@
#include linux/of.h
#include linux/of_address.h
#include linux/of_irq.h
+#include linux/acpi.h
#include linux/irqdomain.h
#include linux/interrupt.h
#include linux/percpu.h
#include linux/slab.h
#include linux/irqchip/chained_irq.h
#include linux/irqchip/arm-gic.h
+#include linux/irqchip/arm-gic-acpi.h
#include asm/cputype.h
#include asm/irq.h
@@ -1029,3 +1031,115 @@ IRQCHIP_DECLARE(msm_8660_qgic, qcom,msm-8660-qgic,
gic_of_init);
IRQCHIP_DECLARE(msm_qgic2, qcom,msm-qgic2, gic_of_init);
#endif
+
+#ifdef CONFIG_ACPI
+static u64 dist_phy_base, cpu_phy_base = ULONG_MAX;
+
+static int __init
+gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
+ const unsigned long end)
+{
+ struct acpi_madt_generic_interrupt *processor;
+ u64 gic_cpu_base;
+
+ processor = (struct acpi_madt_generic_interrupt *)header;
+
+ if (BAD_MADT_ENTRY(processor, end))
+ return -EINVAL;
+
+ gic_cpu_base = processor-base_address;
+ if (!gic_cpu_base)
+ return -EFAULT;
+
+ /*
+* There is no support for non-banked GICv1/2 register in ACPI spec.
+* All CPU interface addresses have to be the same.
+*/
+ if (cpu_phy_base != ULONG_MAX gic_cpu_base != cpu_phy_base)
+ return -EFAULT;
+
+ cpu_phy_base = gic_cpu_base;
+ return 0;
+}
+
+static int __init
+gic_acpi_parse_madt_distributor(struct acpi_subtable_header *header,
+ const unsigned long end)
+{
+ struct acpi_madt_generic_distributor *dist;
+
+ dist = (struct acpi_madt_generic_distributor *)header;
+
+ if (BAD_MADT_ENTRY(dist, end))
+
Re: [PATCH v3 13/17] ARM64 / ACPI: Add GICv2 specific ACPI boot support
On 01/09/14 15:57, Hanjun Guo wrote:
From: Tomasz Nowicki tomasz.nowi...@linaro.org
ACPI kernel uses MADT table for proper GIC initialization. It needs to
parse GIC related subtables, collect CPU interface and distributor
addresses and call driver initialization function (which is hardware
abstraction agnostic). In a similar way, FDT initialize GICv1/2.
NOTE: This commit allow to initialize GICv1/2 only.
I cannot help but notice that there is no support for KVM here. It'd be
good to add a note to that effect, so that people do not expect
virtualization support to be working when booting with ACPI.
Signed-off-by: Tomasz Nowicki tomasz.nowi...@linaro.org
Signed-off-by: Hanjun Guo hanjun@linaro.org
---
arch/arm64/include/asm/acpi.h|2 -
arch/arm64/kernel/acpi.c | 23 +++
arch/arm64/kernel/irq.c |5 ++
drivers/irqchip/irq-gic.c| 114
++
include/linux/irqchip/arm-gic-acpi.h | 33 ++
5 files changed, 175 insertions(+), 2 deletions(-)
create mode 100644 include/linux/irqchip/arm-gic-acpi.h
diff --git a/arch/arm64/include/asm/acpi.h b/arch/arm64/include/asm/acpi.h
index a867467..5d2ab63 100644
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -97,8 +97,6 @@ void __init acpi_smp_init_cpus(void);
extern int (*acpi_suspend_lowlevel)(void);
#define acpi_wakeup_address 0
-#define ACPI_MAX_GIC_CPU_INTERFACE_ENTRIES 65535
-
#else
static inline bool acpi_psci_present(void) { return false; }
diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index 354b912..b3b82b0 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -23,6 +23,7 @@
#include linux/irqdomain.h
#include linux/bootmem.h
#include linux/smp.h
+#include linux/irqchip/arm-gic-acpi.h
#include asm/cputype.h
#include asm/cpu_ops.h
@@ -313,6 +314,28 @@ void __init acpi_boot_table_init(void)
pr_err(Can't find FADT or error happened during parsing
FADT\n);
}
+void __init acpi_gic_init(void)
+{
+ struct acpi_table_header *table;
+ acpi_status status;
+ acpi_size tbl_size;
+ int err;
+
+ status = acpi_get_table_with_size(ACPI_SIG_MADT, 0, table, tbl_size);
+ if (ACPI_FAILURE(status)) {
+ const char *msg = acpi_format_exception(status);
+
+ pr_err(Failed to get MADT table, %s\n, msg);
+ return;
+ }
+
+ err = gic_v2_acpi_init(table);
+ if (err)
+ pr_err(Failed to initialize GIC IRQ controller);
What will happen when you get to implement GICv3 support? Another entry
like this? Why isn't this entirely contained in the GIC driver? Do I
sound like a stuck record?
+
+ early_acpi_os_unmap_memory((char *)table, tbl_size);
+}
+
/*
* acpi_suspend_lowlevel() - save kernel state and suspend.
*
diff --git a/arch/arm64/kernel/irq.c b/arch/arm64/kernel/irq.c
index 0f08dfd..c074d60 100644
--- a/arch/arm64/kernel/irq.c
+++ b/arch/arm64/kernel/irq.c
@@ -28,6 +28,7 @@
#include linux/irqchip.h
#include linux/seq_file.h
#include linux/ratelimit.h
+#include linux/irqchip/arm-gic-acpi.h
unsigned long irq_err_count;
@@ -78,6 +79,10 @@ void __init set_handle_irq(void (*handle_irq)(struct
pt_regs *))
void __init init_IRQ(void)
{
irqchip_init();
+
+ if (!handle_arch_irq)
+ acpi_gic_init();
+
Why isn't this called from irqchip_init? It would seem like the logical
spot to probe an interrupt controller.
if (!handle_arch_irq)
panic(No interrupt controller found.);
}
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 4b959e6..85cbf43 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -33,12 +33,14 @@
#include linux/of.h
#include linux/of_address.h
#include linux/of_irq.h
+#include linux/acpi.h
#include linux/irqdomain.h
#include linux/interrupt.h
#include linux/percpu.h
#include linux/slab.h
#include linux/irqchip/chained_irq.h
#include linux/irqchip/arm-gic.h
+#include linux/irqchip/arm-gic-acpi.h
#include asm/cputype.h
#include asm/irq.h
@@ -1029,3 +1031,115 @@ IRQCHIP_DECLARE(msm_8660_qgic, qcom,msm-8660-qgic,
gic_of_init);
IRQCHIP_DECLARE(msm_qgic2, qcom,msm-qgic2, gic_of_init);
#endif
+
+#ifdef CONFIG_ACPI
+static u64 dist_phy_base, cpu_phy_base = ULONG_MAX;
Please use phys_addr_t for physical addresses. The use of ULONG_MAX
looks dodgy. Please have a proper symbol to flag the fact that it hasn't
been assigned yet.
+
+static int __init
+gic_acpi_parse_madt_cpu(struct acpi_subtable_header *header,
+ const unsigned long end)
+{
+ struct acpi_madt_generic_interrupt *processor;
+ u64 gic_cpu_base;
phys_addr_t
+ processor = (struct acpi_madt_generic_interrupt *)header;
+
+ if (BAD_MADT_ENTRY(processor, end))
+
