ARM: integrator: call common init function from machine

arch/arm/boot/dts/integratorcp.dts new file +6 -0

new file mode 100644
index 0000000..8fad5a1
@@ -0,0 +1,69 @@
+/*
+ * Device Tree for the ARM Integrator/CP platform
+ */
+
+/dts-v1/;
+/include/ "skeleton.dtsi"

arch/arm/boot/dts/integratorcp.dts new file +2 -0

+#ifdef CONFIG_OF
+

arch/arm/boot/dts/integratorcp.dts new file +15 -0

+DT_MACHINE_START(INTEGRATOR_AP_DT, "ARM Integrator/AP (Device Tree)")
+	.reserve	= integrator_reserve,
+	.map_io		= ap_map_io,
+	.nr_irqs	= NR_IRQS_INTEGRATOR_AP,
+	.init_early	= ap_init_early,
+	.init_irq	= ap_init_irq_of,
+	.handle_irq	= fpga_handle_irq,
+	.timer		= &ap_of_timer,
+	.init_machine	= ap_init,
+	.restart	= integrator_restart,
+	.dt_compat      = ap_dt_board_compat,
+MACHINE_END
+
+#else
+

arch/arm/boot/dts/integratorcp.dts line 16

Quoted 10 lines · Linus Walleij

arch/arm/boot/dts/integratorcp.dts +9 -0

+/ {
+     model = "ARM Integrator/CP";
+     compatible = "arm,integrator-cp";
+     ranges;
+
+     aliases {
+             arm,integrator-clocksource = &timer2;
+             arm,integrator-clockevent = &timer1;
+     };

It looks like this file is almost a direct superset of the integratorap.dts file. How about including the other file from here and just adding the extra nodes and overriding the few bits that are actually different?

Quoted 2 lines

arch/arm/boot/dts/integratorcp.dts +2 -0

+#ifdef CONFIG_OF
+

...

Quoted 15 lines

arch/arm/boot/dts/integratorcp.dts +15 -0

+DT_MACHINE_START(INTEGRATOR_AP_DT, "ARM Integrator/AP (Device Tree)")
+     .reserve        = integrator_reserve,
+     .map_io         = ap_map_io,
+     .nr_irqs        = NR_IRQS_INTEGRATOR_AP,
+     .init_early     = ap_init_early,
+     .init_irq       = ap_init_irq_of,
+     .handle_irq     = fpga_handle_irq,
+     .timer          = &ap_of_timer,
+     .init_machine   = ap_init,
+     .restart        = integrator_restart,
+     .dt_compat      = ap_dt_board_compat,
+MACHINE_END
+
+#else
+

...

arch/arm/boot/dts/integratorcp.dts MACHINE_START(INTEGRATOR, "ARM-Integrator") · line 575

Quoted 9 lines

MACHINE_START(INTEGRATOR, "ARM-Integrator")

      /* Maintainer: ARM Ltd/Deep Blue Solutions Ltd */
      .atag_offset    = 0x100,

arch/arm/boot/dts/integratorcp.dts +2 -0

@@ -486,3 +571,5 @@ MACHINE_START(INTEGRATOR, "ARM-Integrator")
      .init_machine   = ap_init,
      .restart        = integrator_restart,
 MACHINE_END
+
+#endif

I think we discussed this before. It would be nice to replace the #if/#else with

#ifdef CONFIG_OF
...
#endif
#ifdef CONFIG_ATAG
...
#endif

Quoted 9 lines · Arnd Bergmann

I think we discussed this before. It would be nice to replace the #if/#else with

#ifdef CONFIG_OF
...
#endif
#ifdef CONFIG_ATAG
...
#endif

Sure, have you applied Nico's patch adding CONFIG_ATAG to and ARM SoC branch so I can base my patches on it?

I was under the impression that his patch would go through Russell's tree and I would thus not be able to rely on it until it has landed there and you have pulled Russell's stuff into your tree.

Currently I would make it impossible to use ATAGs if I do this since the symbol does not exist, so I really need to base such an approach in a tree which has CONFIG_ATAG in the first place.

[Me]

Quoted 3 lines

Currently I would make it impossible to use ATAGs if I do this since the symbol does not exist, so I really need to base such an approach in a tree which has CONFIG_ATAG in the first place.

I've just merged that patch though.

Quoted 6 lines · Russell King - ARM Linux

[Me]

Quoted 3 lines

Currently I would make it impossible to use ATAGs if I do this since the symbol does not exist, so I really need to base such an approach in a tree which has CONFIG_ATAG in the first place.

I've just merged that patch though.

OK great, Arnd do you have a Russell baseline in ARM SoC or is it easier if I try to submit this series directly to Russell?

So, Linus, it's up to you (and Russell) what you prefer, but I'd be happy to take it through arm-soc if we get a stable base.

This is initial device tree support for the ARM Integrator family, we create a very basic device tree, #ifdef out the non-DT machines when compiling for device tree.

Signed-off-by Linus Walleij <[email protected]>

---
ChangeLog v1->v2:
- Also switch over clocksource and clockevent to device tree. Else the IRQ mapping goes bananas.
---

+++ b/arch/arm/boot/dts/integratorcp.dts

arch/arm/boot/dts/integratorcp.dts new file +5 -0

@@ -0,0 +1,69 @@
+/*
+ * Device Tree for the ARM Integrator/CP platform
+ */
+
+/dts-v1/;

arch/arm/boot/dts/integratorcp.dts line 15

Quoted 8 lines · Linus Walleij

arch/arm/boot/dts/integratorcp.dts new file +8 -0

+/ {
+     model = "ARM Integrator/CP";
+     compatible = "arm,integrator-cp";
+     ranges;
+
+     aliases {
+             arm,integrator-clocksource = &timer2;
+             arm,integrator-clockevent = &timer1;

This is linux specific and a common issue we need a solution for. Grant had done something for versatile and we discussed it some, but nothing ever got close to finalized. IIRC, Grant's solution was just add a "linux,clockevent" or "linux,clocksource" to the nodes that are to be used. But he wasn't really happy with it. Perhaps using these names as aliases would be better.

Quoted 17 lines · Rob Herring

arch/arm/boot/dts/integratorcp.dts line 15

Quoted 8 lines · Linus Walleij

arch/arm/boot/dts/integratorcp.dts new file +8 -0

+/ {
+     model = "ARM Integrator/CP";
+     compatible = "arm,integrator-cp";
+     ranges;
+
+     aliases {
+             arm,integrator-clocksource = &timer2;
+             arm,integrator-clockevent = &timer1;

This is linux specific and a common issue we need a solution for. Grant had done something for versatile and we discussed it some, but nothing ever got close to finalized. IIRC, Grant's solution was just add a "linux,clockevent" or "linux,clocksource" to the nodes that are to be used. But he wasn't really happy with it. Perhaps using these names as aliases would be better.

Atleast I'm honest about what this is, IMHO this is better than any of the solutions we already have in boot/dts.

Look at the versatile express
arch/arm/boot/dts/vexpress-v2m.dtsi:

        aliases {
                arm,v2m_timer = &v2m_timer01;
        };

It's quite hard to tell that this "vtm_timer" is actually the block serving as clock source and clockevent for the Vexpress, but if you inspect the code you can see that this is what it is.

I can of course skip the aliases and go for the timer nodes I want directly, but that isn't any helpful for people reading the device tree, is it?

Maybe primary-timer, secondary-timer is sufficiently good and neutral aliases?

[Me]

Quoted 3 lines

I can of course skip the aliases and go for the timer nodes I want directly, but that isn't any helpful for people reading the device tree, is it?

Well, the dts doesn't really need to describe how Linux is using things.

I'd like to get to common DT code for sp804 so all platforms just have a call to sp804_dt_init() which does all the initialization.

Quoted 2 lines

Maybe primary-timer, secondary-timer is sufficiently good and neutral aliases?

That doesn't distinguish clk-event vs. clk-source. Perhaps we can use presence of interrupt to distinguish that.

It shouldn't really matter
which timer Linux picks as long as the h/w has the necessary features.

We just need to make sure dts describes those features. Some of the things we have to deal with:

Highbank has interrupt for timer0, but not on timer1, so timer0 must be clk-event. It also users timer1 for sched clock, but ARM Ltd platforms use a different timer.

Realview and Versatile use timer0 for clk-event and timer3 for clk-src. Is there something broken with timer1 and timer2? It is certainly possible the h/w has the clock tied off for 1 of the 2 timers.

VExpress CA9 has broken timers on the core tile (at least they were ifdef'ed out before the last clean-up). We could just remove/disable them in the dts so they are not used.

Quoted 6 lines · Rob Herring

[Me]

Quoted 3 lines

I can of course skip the aliases and go for the timer nodes I want directly, but that isn't any helpful for people reading the device tree, is it?

Well, the dts doesn't really need to describe how Linux is using things.

The arm,integrator-clocksource and arm,integrator-clockevent are renamed arm,timer-primary and arm,timer-secondary in the last patch set.

Are you OK with this for now, going forward? I'd be happy to take a round refactoring it for the next cycle as outlined below.

Quoted 8 lines · Linus Walleij

Quoted 6 lines · Rob Herring

[Me]

Quoted 3 lines

I can of course skip the aliases and go for the timer nodes I want directly, but that isn't any helpful for people reading the device tree, is it?

Well, the dts doesn't really need to describe how Linux is using things.

Quoted 7 lines

The arm,integrator-clocksource and arm,integrator-clockevent are renamed arm,timer-primary and arm,timer-secondary in the last patch set.

Are you OK with this for now, going forward? I'd be happy to take a round refactoring it for the next cycle as outlined below.

If the timers are really free for use by anything and you want to encode how Linux uses them, I would suggest naming the aliases "linux.clocksource" and "linux,clockevent" to make it clear that you are not describing the hardware here but rather picking a configuration.

Quoted 6 lines · Rob Herring

[Me]

Quoted 3 lines

I can of course skip the aliases and go for the timer nodes I want directly, but that isn't any helpful for people reading the device tree, is it?

Well, the dts doesn't really need to describe how Linux is using things.

The arm,integrator-clocksource and arm,integrator-clockevent are renamed arm,timer-primary and arm,timer-secondary in the last patch set.

Are you OK with this for now, going forward? I'd be happy to take a round refactoring it for the next cycle as outlined below.

Quoted 2 lines

I'd like to get to common DT code for sp804 so all platforms just have a call to sp804_dt_init() which does all the initialization.

The Integrator/AP does not have the SP804 timer, it has a precursor with similar, yet different register setup. The driver is in the integrator_ab. core file.

(I could look into merging this SP804 ancestor into the SP804 driver with some flag or so, but that would be a different TODO item.)

The Integrator/CP has the SP804 however.

Quoted 3 lines

Quoted 2 lines

Maybe primary-timer, secondary-timer is sufficiently good and neutral aliases?

Quoted 2 lines

That doesn't distinguish clk-event vs. clk-source. Perhaps we can use presence of interrupt to distinguish that.

They all have interrupts in the Integrator case so all timers are equal. But I vaguely recall someone whisper bad things about timer0.

Quoted 2 lines

It shouldn't really matter
which timer Linux picks as long as the h/w has the necessary features.

That is true, what I've been doing so far is just a 1-to-1 mapping to current usage without much massaging of the abstract resource concept.

Quoted 6 lines

We just need to make sure dts describes those features. Some of the things we have to deal with:

Highbank has interrupt for timer0, but not on timer1, so timer0 must be clk-event. It also users timer1 for sched clock, but ARM Ltd platforms use a different timer.

So I guess this SP804? They just didn't route the IRQ line I guess... :-(

Quoted 3 lines

Realview and Versatile use timer0 for clk-event and timer3 for clk-src. Is there something broken with timer1 and timer2? It is certainly possible the h/w has the clock tied off for 1 of the 2 timers.

Integrator use timer2 for clockevent and timer1 for clocksource, and legends tell that timer0 should be avoided. But I guess I could try to find out by experience, I fear that it could work but be instable so that it appears to work but in reality does not...

Quoted 3 lines

VExpress CA9 has broken timers on the core tile (at least they were ifdef'ed out before the last clean-up). We could just remove/disable them in the dts so they are not used.

There is something about the DT ambition here (or for that matter the boardfile ambition). On the RealView PB1176 there is a broken DMA engine, we just deleted it from the resource list. Other people might want to have all available resources listed but marked "broken" in the DT for completeness.

The latter has the upside of providing hints to the next engineer who would otherwise come around and ask why the hardware in the refman is not listed in the DT and start to try to add it and find out it is broken. Knowing it's there and it's broken is encoding real knowledge at the expense of a few DT bytes.

Well, the dts doesn't really need to describe how Linux is using things. I'd like to get to common DT code for sp804 so all platforms just have a call to sp804_dt_init() which does all the initialization.

Realview and Versatile use timer0 for clk-event and timer3 for clk-src. Is there something broken with timer1 and timer2? It is certainly possible the h/w has the clock tied off for 1 of the 2 timers.

VExpress CA9 has broken timers on the core tile (at least they were ifdef'ed out before the last clean-up). We could just remove/disable them in the dts so they are not used.

In the PL010 UART callback a comparison against the base address is done to figure out which UART is doing the callback. This does not play well with device tree, so let's check the dev_name() of the device instead.

Signed-off-by Linus Walleij <[email protected]>

 arch/arm/mach-integrator/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/mach-integrator/core.c +0 -0

index 5ba4bc8..67ea181 100644

Quoted 6 lines · Linus Walleij

In the PL010 UART callback a comparison against the base address is done to figure out which UART is doing the callback. This does not play well with device tree, so let's check the dev_name() of the device instead.

Signed-off-by Linus Walleij <[email protected]>

Hmm, I think the dev_name would also not be that nice, because it changes after the conversion to DT is complete.

Quoted 5 lines

+++ b/arch/arm/mach-integrator/core.c

arch/arm/mach-integrator/core.c +0 -0

@@ -100,7 +100,7 @@ static void integrator_uart_set_mctrl(struct amba_device *dev, void __iomem *bas
 {
        unsigned int ctrls = 0, ctrlc = 0, rts_mask, dtr_mask;
 

arch/arm/mach-integrator/core.c static void integrator_uart_set_mctrl(struct amba_device *dev, void __iomem *bas · line 106

Quoted 5 lines

arch/arm/mach-integrator/core.c +1 -1

-       if (dev == &uart0_device) {
+       if (!strcmp(dev_name(&dev->dev), "uart0")) {
                rts_mask = 1 << 4;
                dtr_mask = 1 << 5;
        } else {

Maybe it's possible to compare the base pointer against INTEGRATOR_UART0_BASE instead?

Quoted 7 lines · Arnd Bergmann

Quoted 5 lines · Linus Walleij

arch/arm/mach-integrator/core.c +1 -1

-       if (dev == &uart0_device) {
+       if (!strcmp(dev_name(&dev->dev), "uart0")) {
                rts_mask = 1 << 4;
                dtr_mask = 1 << 5;
        } else {

Quoted 1 line

Maybe it's possible to compare the base pointer against INTEGRATOR_UART0_BASE instead?

Nope, that is an ioremapped() base, I have no clue what it may be.