This series add support for mapping and routing GIC interrupts as well
as setting up the GIC timer through device-tree. Patches 1 adds the
"mti" vendor prefix, patch 2 adds the GIC binding document, and patches
3 and 4 add device-tree support for the GIC irqchip and clocksource drivers,
respectively.
Based on next-20141028, which includes part 1 [0] and part 2 [1] of my
GIC cleanup series.
Changes from v2:
- added back third cell to specifier to differentiate between shared and
local interrupts
- added timer sub-node and it's properties
- changed compatible string to include CPU version
- rebased on GIC cleanup series
Changes from v1:
- updated bindings to drop third interrupt cell and remove CPU interrupt
controller as the parent of the GIC
- moved GIC to drivers/irqchip/
- other minor fixes/cleanups
[0] https://lkml.org/lkml/2014/9/18/487
[1] https://lkml.org/lkml/2014/10/20/481
Andrew Bresticker (4):
of: Add vendor prefix for MIPS Technologies, Inc.
of: Add binding document for MIPS GIC
irqchip: mips-gic: Add device-tree support
clocksource: mips-gic: Add device-tree support
.../bindings/interrupt-controller/mips-gic.txt | 55 +++++++++++++++++
.../devicetree/bindings/vendor-prefixes.txt | 1 +
drivers/clocksource/Kconfig | 1 +
drivers/clocksource/mips-gic-timer.c | 37 ++++++++---
drivers/irqchip/irq-mips-gic.c | 71 ++++++++++++++++++++--
.../dt-bindings/interrupt-controller/mips-gic.h | 9 +++
6 files changed, 162 insertions(+), 12 deletions(-)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mips-gic.txt
create mode 100644 include/dt-bindings/interrupt-controller/mips-gic.h
--
2.1.0.rc2.206.gedb03e5
Parse the GIC timer frequency and interrupt from the device-tree.
Signed-off-by: Andrew Bresticker <[email protected]>
---
New for v3.
---
drivers/clocksource/Kconfig | 1 +
drivers/clocksource/mips-gic-timer.c | 37 +++++++++++++++++++++++++++++-------
2 files changed, 31 insertions(+), 7 deletions(-)
diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
index cb7e7f4..89836dc 100644
--- a/drivers/clocksource/Kconfig
+++ b/drivers/clocksource/Kconfig
@@ -226,5 +226,6 @@ config CLKSRC_VERSATILE
config CLKSRC_MIPS_GIC
bool
depends on MIPS_GIC
+ select CLKSRC_OF
endmenu
diff --git a/drivers/clocksource/mips-gic-timer.c b/drivers/clocksource/mips-gic-timer.c
index a749c81..fcdc534 100644
--- a/drivers/clocksource/mips-gic-timer.c
+++ b/drivers/clocksource/mips-gic-timer.c
@@ -11,6 +11,7 @@
#include <linux/interrupt.h>
#include <linux/irqchip/mips-gic.h>
#include <linux/notifier.h>
+#include <linux/of_irq.h>
#include <linux/percpu.h>
#include <linux/smp.h>
#include <linux/time.h>
@@ -101,8 +102,6 @@ static int gic_clockevent_init(void)
if (!cpu_has_counter || !gic_frequency)
return -ENXIO;
- gic_timer_irq = MIPS_GIC_IRQ_BASE +
- GIC_LOCAL_TO_HWIRQ(GIC_LOCAL_INT_COMPARE);
setup_percpu_irq(gic_timer_irq, &gic_compare_irqaction);
register_cpu_notifier(&gic_cpu_nb);
@@ -123,17 +122,41 @@ static struct clocksource gic_clocksource = {
.flags = CLOCK_SOURCE_IS_CONTINUOUS,
};
-void __init gic_clocksource_init(unsigned int frequency)
+static void __init __gic_clocksource_init(void)
{
- gic_frequency = frequency;
-
/* Set clocksource mask. */
gic_clocksource.mask = CLOCKSOURCE_MASK(gic_get_count_width());
/* Calculate a somewhat reasonable rating value. */
- gic_clocksource.rating = 200 + frequency / 10000000;
+ gic_clocksource.rating = 200 + gic_frequency / 10000000;
- clocksource_register_hz(&gic_clocksource, frequency);
+ clocksource_register_hz(&gic_clocksource, gic_frequency);
gic_clockevent_init();
}
+
+void __init gic_clocksource_init(unsigned int frequency)
+{
+ gic_frequency = frequency;
+ gic_timer_irq = MIPS_GIC_IRQ_BASE +
+ GIC_LOCAL_TO_HWIRQ(GIC_LOCAL_INT_COMPARE);
+
+ __gic_clocksource_init();
+}
+
+static void __init gic_clocksource_of_init(struct device_node *node)
+{
+ if (of_property_read_u32(node, "clock-frequency", &gic_frequency)) {
+ pr_err("GIC frequency not specified.\n");
+ return;
+ }
+ gic_timer_irq = irq_of_parse_and_map(node, 0);
+ if (!gic_timer_irq) {
+ pr_err("GIC timer IRQ not specified.\n");
+ return;
+ }
+
+ __gic_clocksource_init();
+}
+CLOCKSOURCE_OF_DECLARE(mips_gic_timer, "mti,interaptiv-gic-timer",
+ gic_clocksource_of_init);
--
2.1.0.rc2.206.gedb03e5
Add the vendor prefix "mti" for MIPS Technologies, Inc.
Signed-off-by: Andrew Bresticker <[email protected]>
---
I'll update the users of the "mips" prefix to use "mti" instead once
this lands.
No changes from v2.
New for v2.
---
Documentation/devicetree/bindings/vendor-prefixes.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 0979393..0221b49 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -98,6 +98,7 @@ mitsubishi Mitsubishi Electric Corporation
mosaixtech Mosaix Technologies, Inc.
moxa Moxa
mpl MPL AG
+mti MIPS Technologies, Inc.
mundoreader Mundo Reader S.L.
murata Murata Manufacturing Co., Ltd.
mxicy Macronix International Co., Ltd.
--
2.1.0.rc2.206.gedb03e5
The Global Interrupt Controller (GIC) present on certain MIPS systems
can be used to route external interrupts to individual VPEs and CPU
interrupt vectors. It also supports a timer and software-generated
interrupts.
Signed-off-by: Andrew Bresticker <[email protected]>
---
Changes from v2:
- added third cell to specify local vs. shared
- added documentation for timer sub-node
- changed compatible string to include CPU version
Changes from v1:
- moved from mips/ to interrupt-controller/
- removed interrupts and interrupt-parent properties
- added available-cpu-vectors property
- dropped third cell in interrupt specifier
---
.../bindings/interrupt-controller/mips-gic.txt | 55 ++++++++++++++++++++++
.../dt-bindings/interrupt-controller/mips-gic.h | 9 ++++
2 files changed, 64 insertions(+)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mips-gic.txt
create mode 100644 include/dt-bindings/interrupt-controller/mips-gic.h
diff --git a/Documentation/devicetree/bindings/interrupt-controller/mips-gic.txt b/Documentation/devicetree/bindings/interrupt-controller/mips-gic.txt
new file mode 100644
index 0000000..84cbbed
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/mips-gic.txt
@@ -0,0 +1,55 @@
+MIPS Global Interrupt Controller (GIC)
+
+The MIPS GIC routes external interrupts to individual VPEs and IRQ pins.
+It also supports local (per-processor) interrupts and software-generated
+interrupts which can be used as IPIs. The GIC also includes a free-running
+global timer, per-CPU count/compare timers, and a watchdog.
+
+Required properties:
+- compatible : Should be "mti,<cpu>-gic". Supported variants:
+ - "mti,interaptiv-gic"
+- reg : Base address and length of the GIC registers.
+- interrupt-controller : Identifies the node as an interrupt controller
+- #interrupt-cells : Specifies the number of cells needed to encode an
+ interrupt specifier. Should be 3.
+ - The first cell is the type of interrupt, local or shared.
+ See <include/dt-bindings/interrupt-controller/mips-gic.h>.
+ - The second cell is the GIC interrupt number.
+ - The third cell encodes the interrupt flags.
+ See <include/dt-bindings/interrupt-controller/irq.h> for a list of valid
+ flags.
+- mti,available-cpu-vectors : Specifies the list of CPU interrupt vectors
+ to which the GIC may route interrupts. May contain up to 6 entries, one
+ for each of the CPU's hardware interrupt vectors. Valid values are 2 - 7.
+ This property is ignored if the CPU is started in EIC mode.
+
+Required properties for timer sub-node:
+- compatible : Should be "mti,<cpu>-gic-timer". Supported variants:
+ - "mti,interaptiv-gic-timer"
+- interrupts : Interrupt for the GIC local timer.
+- clock-frequency : Clock frequency at which the GIC timers operate.
+
+Example:
+
+ gic: interrupt-controller@1bdc0000 {
+ compatible = "mti,interaptiv-gic";
+ reg = <0x1bdc0000 0x20000>;
+
+ interrupt-controller;
+ #interrupt-cells = <3>;
+
+ mti,available-cpu-vectors = <2>, <3>, <4>, <5>;
+
+ timer {
+ compatible = "mti,interaptiv-gic-timer";
+ interrupts = <GIC_LOCAL 1 IRQ_TYPE_NONE>;
+ clock-frequency = <50000000>;
+ };
+ };
+
+ uart@18101400 {
+ ...
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_SHARED 24 IRQ_TYPE_LEVEL_HIGH>;
+ ...
+ };
diff --git a/include/dt-bindings/interrupt-controller/mips-gic.h b/include/dt-bindings/interrupt-controller/mips-gic.h
new file mode 100644
index 0000000..cf35a57
--- /dev/null
+++ b/include/dt-bindings/interrupt-controller/mips-gic.h
@@ -0,0 +1,9 @@
+#ifndef _DT_BINDINGS_INTERRUPT_CONTROLLER_MIPS_GIC_H
+#define _DT_BINDINGS_INTERRUPT_CONTROLLER_MIPS_GIC_H
+
+#include <dt-bindings/interrupt-controller/irq.h>
+
+#define GIC_SHARED 0
+#define GIC_LOCAL 1
+
+#endif
--
2.1.0.rc2.206.gedb03e5
Add device-tree support for the MIPS GIC. Update the GIC irqdomain's
xlate() callback to handle the three-cell specifier described in the
MIPS GIC binding document.
Signed-off-by: Andrew Bresticker <[email protected]>
---
Changes from v2:
- rebased on GIC irqchip cleanups
- updated for change in bindings
- only parse first CPU vector
- allow platforms to use EIC mode
Changes from v1:
- updated for change in bindings
- set base address and enable bit in GCR_GIC_BASE
---
drivers/irqchip/irq-mips-gic.c | 71 +++++++++++++++++++++++++++++++++++++++---
1 file changed, 66 insertions(+), 5 deletions(-)
diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c
index 61ac482..914d73d 100644
--- a/drivers/irqchip/irq-mips-gic.c
+++ b/drivers/irqchip/irq-mips-gic.c
@@ -12,12 +12,18 @@
#include <linux/interrupt.h>
#include <linux/irq.h>
#include <linux/irqchip/mips-gic.h>
+#include <linux/of_address.h>
#include <linux/sched.h>
#include <linux/smp.h>
+#include <asm/mips-cm.h>
#include <asm/setup.h>
#include <asm/traps.h>
+#include <dt-bindings/interrupt-controller/mips-gic.h>
+
+#include "irqchip.h"
+
unsigned int gic_present;
struct gic_pcpu_mask {
@@ -662,14 +668,34 @@ static int gic_irq_domain_map(struct irq_domain *d, unsigned int virq,
return gic_shared_irq_domain_map(d, virq, hw);
}
+static int gic_irq_domain_xlate(struct irq_domain *d, struct device_node *ctrlr,
+ const u32 *intspec, unsigned int intsize,
+ irq_hw_number_t *out_hwirq,
+ unsigned int *out_type)
+{
+ if (intsize != 3)
+ return -EINVAL;
+
+ if (intspec[0] == GIC_SHARED)
+ *out_hwirq = GIC_SHARED_TO_HWIRQ(intspec[1]);
+ else if (intspec[0] == GIC_LOCAL)
+ *out_hwirq = GIC_LOCAL_TO_HWIRQ(intspec[1]);
+ else
+ return -EINVAL;
+ *out_type = intspec[2] & IRQ_TYPE_SENSE_MASK;
+
+ return 0;
+}
+
static struct irq_domain_ops gic_irq_domain_ops = {
.map = gic_irq_domain_map,
- .xlate = irq_domain_xlate_twocell,
+ .xlate = gic_irq_domain_xlate,
};
-void __init gic_init(unsigned long gic_base_addr,
- unsigned long gic_addrspace_size, unsigned int cpu_vec,
- unsigned int irqbase)
+static void __init __gic_init(unsigned long gic_base_addr,
+ unsigned long gic_addrspace_size,
+ unsigned int cpu_vec, unsigned int irqbase,
+ struct device_node *node)
{
unsigned int gicconfig;
@@ -695,7 +721,7 @@ void __init gic_init(unsigned long gic_base_addr,
gic_irq_dispatch);
}
- gic_irq_domain = irq_domain_add_simple(NULL, GIC_NUM_LOCAL_INTRS +
+ gic_irq_domain = irq_domain_add_simple(node, GIC_NUM_LOCAL_INTRS +
gic_shared_intrs, irqbase,
&gic_irq_domain_ops, NULL);
if (!gic_irq_domain)
@@ -705,3 +731,38 @@ void __init gic_init(unsigned long gic_base_addr,
gic_ipi_init();
}
+
+void __init gic_init(unsigned long gic_base_addr,
+ unsigned long gic_addrspace_size,
+ unsigned int cpu_vec, unsigned int irqbase)
+{
+ __gic_init(gic_base_addr, gic_addrspace_size, cpu_vec, irqbase, NULL);
+}
+
+static int __init gic_of_init(struct device_node *node,
+ struct device_node *parent)
+{
+ int cpu_vec;
+ struct resource res;
+
+ /* Use the first available CPU vector. */
+ if (of_property_read_u32_index(node, "mti,available-cpu-vectors", 0,
+ &cpu_vec)) {
+ pr_err("No CPU vectors available for GIC\n");
+ return -ENODEV;
+ }
+
+ if (of_address_to_resource(node, 0, &res)) {
+ pr_err("Failed to get GIC memory range\n");
+ return -ENODEV;
+ }
+
+ if (mips_cm_present())
+ write_gcr_gic_base(res.start | CM_GCR_GIC_BASE_GICEN_MSK);
+ gic_present = true;
+
+ __gic_init(res.start, resource_size(&res), cpu_vec, 0, node);
+
+ return 0;
+}
+IRQCHIP_DECLARE(mips_gic, "mti,interaptiv-gic", gic_of_init);
--
2.1.0.rc2.206.gedb03e5
On Tuesday 28 October 2014 17:12:38 Andrew Bresticker wrote:
> This series add support for mapping and routing GIC interrupts as well
> as setting up the GIC timer through device-tree. Patches 1 adds the
> "mti" vendor prefix, patch 2 adds the GIC binding document, and patches
> 3 and 4 add device-tree support for the GIC irqchip and clocksource drivers,
> respectively.
>
> Based on next-20141028, which includes part 1 [0] and part 2 [1] of my
> GIC cleanup series.
>
Looks all good to me,
Acked-by: Arnd Bergmann <[email protected]>
Hi Andrew,
On 29/10/14 00:12, Andrew Bresticker wrote:
> - changed compatible string to include CPU version
> +Required properties:
> +- compatible : Should be "mti,<cpu>-gic". Supported variants:
> + - "mti,interaptiv-gic"
> +Required properties for timer sub-node:
> +- compatible : Should be "mti,<cpu>-gic-timer". Supported variants:
> + - "mti,interaptiv-gic-timer"
Erm, I'm a bit confused...
Why do you include the core name in the compatible string?
You seem to be suggesting that:
1) The GIC/timer drivers need to know what core they're running on.
Is that really true?
2) It isn't possible to probe the core type.
But the kernel already knows this, so what's wrong with using
current_cpu_type() like everything else that needs to know?
3) Every new core should require a new compatible string to be added
before the GIC will work. You don't even have a generic compatible
string that DT can specify after the core specific one as a fallback.
Please lets not do this unless it's actually necessary (which AFAICT it
really isn't).
Thanks
James
On 10/29/2014 12:12 AM, Andrew Bresticker wrote:
> +- mti,available-cpu-vectors : Specifies the list of CPU interrupt vectors
> + to which the GIC may route interrupts. May contain up to 6 entries, one
> + for each of the CPU's hardware interrupt vectors. Valid values are 2 - 7.
> + This property is ignored if the CPU is started in EIC mode.
> +
Wouldn't it be better to have this in the reversed sense ie:
mti,nonavailable-cpu-vectors? I think the assumption that by default
they're all available unless something else is connected to them which
is unlikely in most cases. It can be made optional property then.
I don't have a strong opinion about it though.
Qais
On 10/29/2014 12:12 AM, Andrew Bresticker wrote:
> +- reg : Base address and length of the GIC registers.
>
Also except for sead3, the base address should be properly reported by
the hardware. The size is fixed (for a specific version of GIC at least
- which is also reported by the hardware). So it would be nice to make
this optional.
Qais
Hi James,
On Wed, Oct 29, 2014 at 2:21 AM, James Hogan <[email protected]> wrote:
> Hi Andrew,
>
> On 29/10/14 00:12, Andrew Bresticker wrote:
>> - changed compatible string to include CPU version
>
>> +Required properties:
>> +- compatible : Should be "mti,<cpu>-gic". Supported variants:
>> + - "mti,interaptiv-gic"
>
>> +Required properties for timer sub-node:
>> +- compatible : Should be "mti,<cpu>-gic-timer". Supported variants:
>> + - "mti,interaptiv-gic-timer"
>
> Erm, I'm a bit confused...
> Why do you include the core name in the compatible string?
>
> You seem to be suggesting that:
>
> 1) The GIC/timer drivers need to know what core they're running on.
>
> Is that really true?
They don't now, but it's possible that a future CPU has a newer
revision of the GIC which has some differences that need to be
accounted for in the driver.
> 2) It isn't possible to probe the core type.
>
> But the kernel already knows this, so what's wrong with using
> current_cpu_type() like everything else that needs to know?
>
> 3) Every new core should require a new compatible string to be added
> before the GIC will work. You don't even have a generic compatible
> string that DT can specify after the core specific one as a fallback.
Yes, adding a generic compatible string would be a good idea.
> Please lets not do this unless it's actually necessary (which AFAICT it
> really isn't).
The point of this was to future-proof these bindings and I though that
CPU type was the best way to indicate version in the compatible
string. This is also how it's done for the ARM GIC and arch timers.
Perhaps the best thing to do is to require both a core-specific
("mti,interaptiv-gic") and generic ("mti,gic") compatible string and
just match on the generic one for now until there's a need to use the
core-specific one. Thoughts?
On Wed, Oct 29, 2014 at 4:01 AM, Qais Yousef <[email protected]> wrote:
> On 10/29/2014 12:12 AM, Andrew Bresticker wrote:
>>
>> +- mti,available-cpu-vectors : Specifies the list of CPU interrupt vectors
>> + to which the GIC may route interrupts. May contain up to 6 entries,
>> one
>> + for each of the CPU's hardware interrupt vectors. Valid values are 2 -
>> 7.
>> + This property is ignored if the CPU is started in EIC mode.
>> +
>
>
> Wouldn't it be better to have this in the reversed sense ie:
> mti,nonavailable-cpu-vectors? I think the assumption that by default they're
> all available unless something else is connected to them which is unlikely
> in most cases. It can be made optional property then.
>
> I don't have a strong opinion about it though.
Actually, I think I like the reversed sense as well. Perhaps
"mti,reserved-cpu-vectors"?
On 10/29/2014 04:56 PM, Andrew Bresticker wrote:
> On Wed, Oct 29, 2014 at 4:01 AM, Qais Yousef <[email protected]> wrote:
>> On 10/29/2014 12:12 AM, Andrew Bresticker wrote:
>>> +- mti,available-cpu-vectors : Specifies the list of CPU interrupt vectors
>>> + to which the GIC may route interrupts. May contain up to 6 entries,
>>> one
>>> + for each of the CPU's hardware interrupt vectors. Valid values are 2 -
>>> 7.
>>> + This property is ignored if the CPU is started in EIC mode.
>>> +
>>
>> Wouldn't it be better to have this in the reversed sense ie:
>> mti,nonavailable-cpu-vectors? I think the assumption that by default they're
>> all available unless something else is connected to them which is unlikely
>> in most cases. It can be made optional property then.
>>
>> I don't have a strong opinion about it though.
> Actually, I think I like the reversed sense as well. Perhaps
> "mti,reserved-cpu-vectors"?
Yep that's a better wording for sure :)
On Wed, Oct 29, 2014 at 4:09 AM, Qais Yousef <[email protected]> wrote:
> On 10/29/2014 12:12 AM, Andrew Bresticker wrote:
>>
>> +- reg : Base address and length of the GIC registers.
>>
>
> Also except for sead3, the base address should be properly reported by the
> hardware. The size is fixed (for a specific version of GIC at least - which
> is also reported by the hardware). So it would be nice to make this
> optional.
Even though this is usually probable, I'd prefer to leave this as
required, or at least "optional, but recommended". I don't have a
very strong opinion on it though, but perhaps the device-tree folks
do?
On 29/10/14 16:55, Andrew Bresticker wrote:
> Hi James,
>
> On Wed, Oct 29, 2014 at 2:21 AM, James Hogan <[email protected]> wrote:
>> Hi Andrew,
>>
>> On 29/10/14 00:12, Andrew Bresticker wrote:
>>> - changed compatible string to include CPU version
>>
>>> +Required properties:
>>> +- compatible : Should be "mti,<cpu>-gic". Supported variants:
>>> + - "mti,interaptiv-gic"
>>
>>> +Required properties for timer sub-node:
>>> +- compatible : Should be "mti,<cpu>-gic-timer". Supported variants:
>>> + - "mti,interaptiv-gic-timer"
>>
>> Erm, I'm a bit confused...
>> Why do you include the core name in the compatible string?
>>
>> You seem to be suggesting that:
>>
>> 1) The GIC/timer drivers need to know what core they're running on.
>>
>> Is that really true?
>
> They don't now, but it's possible that a future CPU has a newer
> revision of the GIC which has some differences that need to be
> accounted for in the driver.
>
>> 2) It isn't possible to probe the core type.
>>
>> But the kernel already knows this, so what's wrong with using
>> current_cpu_type() like everything else that needs to know?
>>
>> 3) Every new core should require a new compatible string to be added
>> before the GIC will work. You don't even have a generic compatible
>> string that DT can specify after the core specific one as a fallback.
>
> Yes, adding a generic compatible string would be a good idea.
>
>> Please lets not do this unless it's actually necessary (which AFAICT it
>> really isn't).
>
> The point of this was to future-proof these bindings and I though that
> CPU type was the best way to indicate version in the compatible
> string. This is also how it's done for the ARM GIC and arch timers.
> Perhaps the best thing to do is to require both a core-specific
> ("mti,interaptiv-gic") and generic ("mti,gic") compatible string and
> just match on the generic one for now until there's a need to use the
> core-specific one. Thoughts?
FPGA boards like Malta are something else to consider (when it is
eventually converted to DT - Paul on CC knows more than me). You might
load an interAptiv, or a proAptiv, or a P5600 bitstream, and the gic
setup will be pretty much the same I think, since e.g. the address
depends on where it is convenient to put it in the address space of the
platform.
Any thoughts on the existence of current_cpu_type(), and the GIC
revision register? They pretty much make encoding of core in compatible
string redundant I think.
Cheers
James
On 10/29/2014 05:08 PM, Andrew Bresticker wrote:
> On Wed, Oct 29, 2014 at 4:09 AM, Qais Yousef <[email protected]> wrote:
>> On 10/29/2014 12:12 AM, Andrew Bresticker wrote:
>>> +- reg : Base address and length of the GIC registers.
>>>
>> Also except for sead3, the base address should be properly reported by the
>> hardware. The size is fixed (for a specific version of GIC at least - which
>> is also reported by the hardware). So it would be nice to make this
>> optional.
> Even though this is usually probable, I'd prefer to leave this as
> required, or at least "optional, but recommended". I don't have a
> very strong opinion on it though, but perhaps the device-tree folks
> do?
The biggest advantage I can think of is that it can potentially make GIC
DT definition more shareable across for instance multiple revisions of
an SoC that might have the GIC at different base addresses.
I won't insist too much though.
On Wed, Oct 29, 2014 at 10:13 AM, James Hogan <[email protected]> wrote:
> On 29/10/14 16:55, Andrew Bresticker wrote:
>> Hi James,
>>
>> On Wed, Oct 29, 2014 at 2:21 AM, James Hogan <[email protected]> wrote:
>>> Hi Andrew,
>>>
>>> On 29/10/14 00:12, Andrew Bresticker wrote:
>>>> - changed compatible string to include CPU version
>>>
>>>> +Required properties:
>>>> +- compatible : Should be "mti,<cpu>-gic". Supported variants:
>>>> + - "mti,interaptiv-gic"
>>>
>>>> +Required properties for timer sub-node:
>>>> +- compatible : Should be "mti,<cpu>-gic-timer". Supported variants:
>>>> + - "mti,interaptiv-gic-timer"
>>>
>>> Erm, I'm a bit confused...
>>> Why do you include the core name in the compatible string?
>>>
>>> You seem to be suggesting that:
>>>
>>> 1) The GIC/timer drivers need to know what core they're running on.
>>>
>>> Is that really true?
>>
>> They don't now, but it's possible that a future CPU has a newer
>> revision of the GIC which has some differences that need to be
>> accounted for in the driver.
>>
>>> 2) It isn't possible to probe the core type.
>>>
>>> But the kernel already knows this, so what's wrong with using
>>> current_cpu_type() like everything else that needs to know?
>>>
>>> 3) Every new core should require a new compatible string to be added
>>> before the GIC will work. You don't even have a generic compatible
>>> string that DT can specify after the core specific one as a fallback.
>>
>> Yes, adding a generic compatible string would be a good idea.
>>
>>> Please lets not do this unless it's actually necessary (which AFAICT it
>>> really isn't).
>>
>> The point of this was to future-proof these bindings and I though that
>> CPU type was the best way to indicate version in the compatible
>> string. This is also how it's done for the ARM GIC and arch timers.
>> Perhaps the best thing to do is to require both a core-specific
>> ("mti,interaptiv-gic") and generic ("mti,gic") compatible string and
>> just match on the generic one for now until there's a need to use the
>> core-specific one. Thoughts?
>
> FPGA boards like Malta are something else to consider (when it is
> eventually converted to DT - Paul on CC knows more than me). You might
> load an interAptiv, or a proAptiv, or a P5600 bitstream, and the gic
> setup will be pretty much the same I think, since e.g. the address
> depends on where it is convenient to put it in the address space of the
> platform.
Ah, I didn't realize that the CPU bitstream could be changed
independently of the GIC.
In that case, the CPU revision isn't that useful.
> Any thoughts on the existence of current_cpu_type(), and the GIC
> revision register? They pretty much make encoding of core in compatible
> string redundant I think.
Ok, I suppose using the revision register is fine then.
On Wed, Oct 29, 2014 at 05:23:59PM +0000, Qais Yousef wrote:
> On 10/29/2014 05:08 PM, Andrew Bresticker wrote:
> > On Wed, Oct 29, 2014 at 4:09 AM, Qais Yousef <[email protected]> wrote:
> >> On 10/29/2014 12:12 AM, Andrew Bresticker wrote:
> >>> +- reg : Base address and length of the GIC registers.
> >>>
> >> Also except for sead3, the base address should be properly reported by the
> >> hardware. The size is fixed (for a specific version of GIC at least - which
> >> is also reported by the hardware). So it would be nice to make this
> >> optional.
> > Even though this is usually probable, I'd prefer to leave this as
> > required, or at least "optional, but recommended". I don't have a
> > very strong opinion on it though, but perhaps the device-tree folks
> > do?
It boils down to how reliable the values you can read out of the HW are.
If it can always be probed reliably, then the property isn't strictly
necessary. However, the fact that you can probe it now doesn't mean you
can always probe it reliably (e.g. a future CPU's reporting mechanism
might be different, or values might be plain wrong). It depends on what
you expect in future in that regard.
As an example, for a while on ARM we thought we could probe the number
of CPUs present from the HW, but new CPUs and multi-cluster designs
broke our assumptions there. Now we just rely on the /cpus node
containing the appropriate cpu sub-nodes (regardless of whether this
could be probed for a particular CPU/SoC). Luckily we were able to
change that as we were still in the early days of DT conversion.
So consider the possible ways your current probing mechanism is
realistically likely to be broken. If there are clear ways of working
around that, you're probably fine with an optional property. If it looks
like things could change substantially, require the property for now --
you can always ignore it in future if things turn out to be reliably
probeable through other means.
> The biggest advantage I can think of is that it can potentially make GIC
> DT definition more shareable across for instance multiple revisions of
> an SoC that might have the GIC at different base addresses.
For the different revisions of an SoC, I would expect that if your
interrupt controller moved other elements would also? A layer or two of
dtsi files can keep all the common stuff common while allowing
per-revision changes.
> I won't insist too much though.
Similarly, I don't really have a strong opinion either way. There's no
single answer on this.
Thanks,
Mark.
On Wed, Oct 29, 2014 at 12:12:42AM +0000, Andrew Bresticker wrote:
> Parse the GIC timer frequency and interrupt from the device-tree.
>
> Signed-off-by: Andrew Bresticker <[email protected]>
> ---
> New for v3.
> ---
> drivers/clocksource/Kconfig | 1 +
> drivers/clocksource/mips-gic-timer.c | 37 +++++++++++++++++++++++++++++-------
> 2 files changed, 31 insertions(+), 7 deletions(-)
>
[...]
> +static void __init gic_clocksource_of_init(struct device_node *node)
> +{
> + if (of_property_read_u32(node, "clock-frequency", &gic_frequency)) {
> + pr_err("GIC frequency not specified.\n");
> + return;
> + }
> + gic_timer_irq = irq_of_parse_and_map(node, 0);
> + if (!gic_timer_irq) {
> + pr_err("GIC timer IRQ not specified.\n");
> + return;
> + }
> +
> + __gic_clocksource_init();
> +}
> +CLOCKSOURCE_OF_DECLARE(mips_gic_timer, "mti,interaptiv-gic-timer",
> + gic_clocksource_of_init);
Your binding document expected the timer node under the GIC node, and it
looks like this relies on GIC internals. Hwoever, this allows for people
to put the timer node anywhere (and it turns out people are _really_ bad
at putting things together as the binding author expected).
It might be better if the GIC driver detected the sub node and probed
the clocksource driver (or registered a platform device that the
clocksource driver gets registered from). that could prevent some
horrible issues with probe ordering and/or bad dts.
Mark.
On Wed, Oct 29, 2014 at 04:55:56PM +0000, Andrew Bresticker wrote:
> Hi James,
>
> On Wed, Oct 29, 2014 at 2:21 AM, James Hogan <[email protected]> wrote:
> > Hi Andrew,
> >
> > On 29/10/14 00:12, Andrew Bresticker wrote:
> >> - changed compatible string to include CPU version
> >
> >> +Required properties:
> >> +- compatible : Should be "mti,<cpu>-gic". Supported variants:
> >> + - "mti,interaptiv-gic"
> >
> >> +Required properties for timer sub-node:
> >> +- compatible : Should be "mti,<cpu>-gic-timer". Supported variants:
> >> + - "mti,interaptiv-gic-timer"
> >
> > Erm, I'm a bit confused...
> > Why do you include the core name in the compatible string?
> >
> > You seem to be suggesting that:
> >
> > 1) The GIC/timer drivers need to know what core they're running on.
> >
> > Is that really true?
>
> They don't now, but it's possible that a future CPU has a newer
> revision of the GIC which has some differences that need to be
> accounted for in the driver.
At that point you can allocate a new compatible string. Until then you
don't necessarily need to distinguish.
Is the timer defined by the architecture, or is it specific to this CPU
(and might get reused in future)?
> > 2) It isn't possible to probe the core type.
> >
> > But the kernel already knows this, so what's wrong with using
> > current_cpu_type() like everything else that needs to know?
> >
> > 3) Every new core should require a new compatible string to be added
> > before the GIC will work. You don't even have a generic compatible
> > string that DT can specify after the core specific one as a fallback.
>
> Yes, adding a generic compatible string would be a good idea.
>
> > Please lets not do this unless it's actually necessary (which AFAICT it
> > really isn't).
>
> The point of this was to future-proof these bindings and I though that
> CPU type was the best way to indicate version in the compatible
> string. This is also how it's done for the ARM GIC and arch timers.
> Perhaps the best thing to do is to require both a core-specific
> ("mti,interaptiv-gic") and generic ("mti,gic") compatible string and
> just match on the generic one for now until there's a need to use the
> core-specific one. Thoughts?
If this timer is architected you can have a generic string for now, with
each CPU having a more specific string just in case, e.g.
compatible = "mti,interaptiv-gic-timer", "mti,gic-timer".
The kernel driver can currently match just "mti,gic-timer", andd
everything should be fine if it turns out nothing changes with new CPUs:
compatible = "mti,newcpu-gic-timer", "mti,gic-timer";
If the new CPU's timer doesn't quite match, you add its comaptible
string to the driver, and drop "mti,gic-timer" from the node's
comaptible list:
compatible = "mti,newcpu-gic-timer";
Mark.
Hi Andrew,
On Wed, Oct 29, 2014 at 10:25:27AM -0700, Andrew Bresticker wrote:
> On Wed, Oct 29, 2014 at 10:13 AM, James Hogan <[email protected]> wrote:
> > On 29/10/14 16:55, Andrew Bresticker wrote:
> >> On Wed, Oct 29, 2014 at 2:21 AM, James Hogan <[email protected]> wrote:
> >>> Please lets not do this unless it's actually necessary (which AFAICT it
> >>> really isn't).
> >>
> >> The point of this was to future-proof these bindings and I though that
> >> CPU type was the best way to indicate version in the compatible
> >> string. This is also how it's done for the ARM GIC and arch timers.
> >> Perhaps the best thing to do is to require both a core-specific
> >> ("mti,interaptiv-gic") and generic ("mti,gic") compatible string and
> >> just match on the generic one for now until there's a need to use the
> >> core-specific one. Thoughts?
> >
> > FPGA boards like Malta are something else to consider (when it is
> > eventually converted to DT - Paul on CC knows more than me). You might
> > load an interAptiv, or a proAptiv, or a P5600 bitstream, and the gic
> > setup will be pretty much the same I think, since e.g. the address
> > depends on where it is convenient to put it in the address space of the
> > platform.
>
> Ah, I didn't realize that the CPU bitstream could be changed
> independently of the GIC.
To clarify, the GIC is still closely bound to the CPU and contained
within the FPGA bitstream. The register interface should I believe
always comply with some version of the GIC architecture specification,
and I don't think anybody wants per-bitstream DT files / kernels, so in
practice the way the GIC is set up for Malta (how interrupt lines are
connected up and where in address space GIC can go) is unlikely to
become incompatible.
Cheers
James
Hi Mark,
>> +static void __init gic_clocksource_of_init(struct device_node *node)
>> +{
>> + if (of_property_read_u32(node, "clock-frequency", &gic_frequency)) {
>> + pr_err("GIC frequency not specified.\n");
>> + return;
>> + }
>> + gic_timer_irq = irq_of_parse_and_map(node, 0);
>> + if (!gic_timer_irq) {
>> + pr_err("GIC timer IRQ not specified.\n");
>> + return;
>> + }
>> +
>> + __gic_clocksource_init();
>> +}
>> +CLOCKSOURCE_OF_DECLARE(mips_gic_timer, "mti,interaptiv-gic-timer",
>> + gic_clocksource_of_init);
>
> Your binding document expected the timer node under the GIC node, and it
> looks like this relies on GIC internals. Hwoever, this allows for people
> to put the timer node anywhere (and it turns out people are _really_ bad
> at putting things together as the binding author expected).
>
> It might be better if the GIC driver detected the sub node and probed
> the clocksource driver (or registered a platform device that the
> clocksource driver gets registered from). that could prevent some
> horrible issues with probe ordering and/or bad dts.
Probing the clocksource during probe of the irqchip doesn't work
because time-keeping isn't set up at the time. Registering a platform
device for the timer pushes back GIC timer registration to later in
the boot process (assuming it now becomes a module_platform_driver)
and makes it rather non-parallel to how it's done in the non-DT case
(this could be changed, though IMO it's a lot of churn just to thwart
bad DT authors).
Probe ordering shouldn't be an issue because irqchips are probed
before clocksources. Perhaps checking for the presence of GIC
(there's the global gic_present) and node->parent (to enforce the
parent-child relationship) would be sufficient?
Thanks,
Andrew