ARM: shmobile: Add INTC-SYS clock to device tree

INTC-SYS is the module clock for the GIC. Accessing the GIC while it is disabled causes:

    Unhandled fault: asynchronous external abort (0x1211) at 0x00000000

Currently, the GIC driver cannot enable its module clock for several reasons:
- It does not use a platform device, so Runtime PM is not an option,
- gic_of_init() runs before any clocks are registered, so it cannot explicitly enable the clock,
- gic_of_init() cannot return -EPROBE_DEFER, as IRQCHIP_DECLARE() doesn't support deferred probing.

Hence we have to keep on relying on the boot loader for enabling the module clock.

To prevent the module clock from being disabled when the CCF core thinks it is unused, and thus causing a system lock-up, add a quirk to the MSTP clock driver to make sure the module clock is never disabled.

Signed-off-by Geert Uytterhoeven <[email protected]>

---
 drivers/clk/shmobile/clk-mstp.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/clk/shmobile/clk-mstp.c +0 -0

index 2d2fe773ac8168f9..742af84735a07450 100644

drivers/clk/shmobile/clk-mstp.c +1 -0

+
        spin_lock_irqsave(&group->lock, flags);
 
        value = clk_readl(group->smstpcr);

Signature · 1 line

1.9.1

Quoting Geert Uytterhoeven (2015-03-18 12:16:00)

Quoted 30 lines

INTC-SYS is the module clock for the GIC. Accessing the GIC while it is disabled causes:

    Unhandled fault: asynchronous external abort (0x1211) at 0x00000000

Currently, the GIC driver cannot enable its module clock for several reasons:
- It does not use a platform device, so Runtime PM is not an option,
- gic_of_init() runs before any clocks are registered, so it cannot explicitly enable the clock,
- gic_of_init() cannot return -EPROBE_DEFER, as IRQCHIP_DECLARE() doesn't support deferred probing.

Hence we have to keep on relying on the boot loader for enabling the module clock.

To prevent the module clock from being disabled when the CCF core thinks it is unused, and thus causing a system lock-up, add a quirk to the MSTP clock driver to make sure the module clock is never disabled.

Signed-off-by Geert Uytterhoeven <[email protected]>

---
 drivers/clk/shmobile/clk-mstp.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/clk/shmobile/clk-mstp.c +0 -0

index 2d2fe773ac8168f9..742af84735a07450 100644

drivers/clk/shmobile/clk-mstp.c static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable) · line 69

Quoted 8 lines

drivers/clk/shmobile/clk-mstp.c +5 -0

@@ -62,6 +62,12 @@ static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable)
        unsigned int i;
        u32 value;
 
+       /* INTC-SYS is the module clock of the GIC, and must not be disabled */
+       if (!enable && !strcmp(__clk_get_name(hw->clk), "intc-sys")) {
+               pr_debug("MSTP %pC skipping disable\n", hw->clk);
+               return 0;
+       }

Hello Geert,

This is a bit ugly for three reasons:

1) we hit this code for every MSTP clock {en,dis}able call
2) __clk_get_name is kind of gross

3) the enable_count will not be correct. It will be zero but the clock will actually be enabled

Have you considered Lee's series to express these always-on clocks in DT? See,

https://lkml.org/lkml/2015/2/24/495

Hi Mike,

Quoted 42 lines · Michael Turquette

Quoting Geert Uytterhoeven (2015-03-18 12:16:00)

Quoted 30 lines

INTC-SYS is the module clock for the GIC. Accessing the GIC while it is disabled causes:

    Unhandled fault: asynchronous external abort (0x1211) at 0x00000000

Currently, the GIC driver cannot enable its module clock for several reasons:
- It does not use a platform device, so Runtime PM is not an option,
- gic_of_init() runs before any clocks are registered, so it cannot explicitly enable the clock,
- gic_of_init() cannot return -EPROBE_DEFER, as IRQCHIP_DECLARE() doesn't support deferred probing.

Hence we have to keep on relying on the boot loader for enabling the module clock.

To prevent the module clock from being disabled when the CCF core thinks it is unused, and thus causing a system lock-up, add a quirk to the MSTP clock driver to make sure the module clock is never disabled.

Signed-off-by Geert Uytterhoeven <[email protected]>

---
 drivers/clk/shmobile/clk-mstp.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/clk/shmobile/clk-mstp.c +0 -0

index 2d2fe773ac8168f9..742af84735a07450 100644

drivers/clk/shmobile/clk-mstp.c static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable) · line 69

Quoted 8 lines

drivers/clk/shmobile/clk-mstp.c +5 -0

@@ -62,6 +62,12 @@ static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable)
        unsigned int i;
        u32 value;
 
+       /* INTC-SYS is the module clock of the GIC, and must not be disabled */
+       if (!enable && !strcmp(__clk_get_name(hw->clk), "intc-sys")) {
+               pr_debug("MSTP %pC skipping disable\n", hw->clk);
+               return 0;
+       }

Hello Geert,

Quoted 4 lines

This is a bit ugly for three reasons:

1) we hit this code for every MSTP clock {en,dis}able call
2) __clk_get_name is kind of gross

Sure, this is ugly. That's why this was an RFC.
I was mainly trying to trigger a reply from the GIC maintainers ;-)

Quoted 2 lines

3) the enable_count will not be correct. It will be zero but the clock will actually be enabled

That's indeed something I didn't take into account. Will change to just enabling the clock from the clock driver.

Quoted 4 lines

Have you considered Lee's series to express these always-on clocks in DT? See,

https://lkml.org/lkml/2015/2/24/495

That solution doesn't apply here, as we do have a correct description of the hardware
in DT (after the other patches in the series, like e.g. (courtesy for Lee) http://permalink.gmane.org/gmane.linux.power-management.general/58123).

Thanks!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Quoted 44 lines · Geert Uytterhoeven

Quoted 42 lines · Michael Turquette

Quoting Geert Uytterhoeven (2015-03-18 12:16:00)

Quoted 30 lines

INTC-SYS is the module clock for the GIC. Accessing the GIC while it is disabled causes:

    Unhandled fault: asynchronous external abort (0x1211) at 0x00000000

Currently, the GIC driver cannot enable its module clock for several reasons:
- It does not use a platform device, so Runtime PM is not an option,
- gic_of_init() runs before any clocks are registered, so it cannot explicitly enable the clock,
- gic_of_init() cannot return -EPROBE_DEFER, as IRQCHIP_DECLARE() doesn't support deferred probing.

Hence we have to keep on relying on the boot loader for enabling the module clock.

To prevent the module clock from being disabled when the CCF core thinks it is unused, and thus causing a system lock-up, add a quirk to the MSTP clock driver to make sure the module clock is never disabled.

Signed-off-by Geert Uytterhoeven <[email protected]>

---
 drivers/clk/shmobile/clk-mstp.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/clk/shmobile/clk-mstp.c +0 -0

index 2d2fe773ac8168f9..742af84735a07450 100644

drivers/clk/shmobile/clk-mstp.c static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable) · line 69

Quoted 8 lines

drivers/clk/shmobile/clk-mstp.c +5 -0

@@ -62,6 +62,12 @@ static int cpg_mstp_clock_endisable(struct clk_hw *hw, bool enable)
        unsigned int i;
        u32 value;
 
+       /* INTC-SYS is the module clock of the GIC, and must not be disabled */
+       if (!enable && !strcmp(__clk_get_name(hw->clk), "intc-sys")) {
+               pr_debug("MSTP %pC skipping disable\n", hw->clk);
+               return 0;
+       }

Hello Geert,

Quoted 7 lines

Quoted 4 lines

This is a bit ugly for three reasons:

1) we hit this code for every MSTP clock {en,dis}able call
2) __clk_get_name is kind of gross

Sure, this is ugly. That's why this was an RFC.
I was mainly trying to trigger a reply from the GIC maintainers ;-)

Given that I'm the only GIC-related person on the cc list, I suppose this is puts me on the spot.

adverse to it. My only gripe is with the undocumented clock property in the binding, and that leads to two questions:
This doesn't touch the GIC code at all, so I don't feel completely

adverse to it. My only gripe is with the undocumented clock property in the binding, and that leads to two questions:
- the GIC architecture doesn't mention a clock at all, so that's a Renesas special. Do we want to have a vendor-specific property for this? Or does it belong elsewhere?

- alternatively, do we want the core GIC code to deal with this? In which case, how do we express the policy?

Quoted 3 lines

adverse to it. My only gripe is with the undocumented clock property in the binding, and that leads to two questions:
This doesn't touch the GIC code at all, so I don't feel completely

The reason no GIC code is touched (for now), is that it's non-trivial to fix the GIC driver to handle this.

Quoted 5 lines

adverse to it. My only gripe is with the undocumented clock property in the binding, and that leads to two questions:
- the GIC architecture doesn't mention a clock at all, so that's a Renesas special. Do we want to have a vendor-specific property for this? Or does it belong elsewhere?

Apart from being implemented using synchronous logic and thus using a clock signal internally, the GIC and its driver don't care about this clock.

When an existing IP module that doesn't care about clocks becomes reused on a new SoC, and it now ends up being part of a "PM Domain" (e.g. a Power Domain or Clock Domain, or both), this "PM Domain" is a feature of the platform, not of the IP module itself (cfr. "(Existing) Device Driver" in [1]; GIC falls in the same category as the Thermal Module example). Hence I don't think it's necessary to mention this in the ARM GIC binding.

Quoted 2 lines

- alternatively, do we want the core GIC code to deal with this? In which case, how do we express the policy?

The proper way to handle this automatically is to add Runtime PM support to the driver. However, this requires using a platform device.

I would like to add the clock and GIC dependency on the clock in the DTS now, for reasons of DTS stability. But that means I need a temporary workaround to avoid the clock from being disabled, until the GIC driver handles this.

I don't expect a fix for the GIC code to just show up magically. I just wanted you to be aware of the problem. GIC is not the only problematic module here, there are others, cfr. the last slide of [2].