| Message ID | 1426706164-28309-2-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Geert Uytterhoeven (2015-03-18 12:16:00) > 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 <geert+renesas@glider.be> > --- > drivers/clk/shmobile/clk-mstp.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/clk/shmobile/clk-mstp.c b/drivers/clk/shmobile/clk-mstp.c > index 2d2fe773ac8168f9..742af84735a07450 100644 > --- a/drivers/clk/shmobile/clk-mstp.c > +++ b/drivers/clk/shmobile/clk-mstp.c > @@ -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 Regards, Mike > + > spin_lock_irqsave(&group->lock, flags); > > value = clk_readl(group->smstpcr); > -- > 1.9.1 >
Hi Mike, On Wed, Mar 25, 2015 at 12:25 AM, Michael Turquette <mturquette@linaro.org> wrote: > Quoting Geert Uytterhoeven (2015-03-18 12:16:00) >> 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 <geert+renesas@glider.be> >> --- >> drivers/clk/shmobile/clk-mstp.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/clk/shmobile/clk-mstp.c b/drivers/clk/shmobile/clk-mstp.c >> index 2d2fe773ac8168f9..742af84735a07450 100644 >> --- a/drivers/clk/shmobile/clk-mstp.c >> +++ b/drivers/clk/shmobile/clk-mstp.c >> @@ -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 Sure, this is ugly. That's why this was an RFC. I was mainly trying to trigger a reply from the GIC maintainers ;-) > 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. > 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@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
On 25/03/15 04:17, Geert Uytterhoeven wrote: > Hi Mike, > > On Wed, Mar 25, 2015 at 12:25 AM, Michael Turquette > <mturquette@linaro.org> wrote: >> Quoting Geert Uytterhoeven (2015-03-18 12:16:00) >>> 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 <geert+renesas@glider.be> >>> --- >>> drivers/clk/shmobile/clk-mstp.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/drivers/clk/shmobile/clk-mstp.c b/drivers/clk/shmobile/clk-mstp.c >>> index 2d2fe773ac8168f9..742af84735a07450 100644 >>> --- a/drivers/clk/shmobile/clk-mstp.c >>> +++ b/drivers/clk/shmobile/clk-mstp.c >>> @@ -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 > > 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. 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? Thanks, M. >> 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. > >> 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@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 >
Hi Marc, On Wed, Mar 25, 2015 at 10:21 AM, Marc Zyngier <marc.zyngier@arm.com> wrote: > On 25/03/15 04:17, Geert Uytterhoeven wrote: >> On Wed, Mar 25, 2015 at 12:25 AM, Michael Turquette >> <mturquette@linaro.org> wrote: >>> Quoting Geert Uytterhoeven (2015-03-18 12:16:00) >>>> 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 <geert+renesas@glider.be> >>>> --- >>>> drivers/clk/shmobile/clk-mstp.c | 6 ++++++ >>>> 1 file changed, 6 insertions(+) >>>> >>>> diff --git a/drivers/clk/shmobile/clk-mstp.c b/drivers/clk/shmobile/clk-mstp.c >>>> index 2d2fe773ac8168f9..742af84735a07450 100644 >>>> --- a/drivers/clk/shmobile/clk-mstp.c >>>> +++ b/drivers/clk/shmobile/clk-mstp.c >>>> @@ -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 >> >> 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 The reason no GIC code is touched (for now), is that it's non-trivial to fix the GIC driver to handle this. > 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. > - 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]. Thanks! References: Presentations at ELC2015: [1] "Last one out, turn off the lights", by Geert Uytterhoeven (http://events.linuxfoundation.org/sites/events/files/slides/Last_One_Out_Turn_Off_The_Lights_Handouts.pdf) [2] "Introduction to Kernel Power Managemen", by Kevin Hilman (still to appear at http://events.linuxfoundation.org/events/embedded-linux-conference/program/slides) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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
Hi Geert, On 25/03/15 21:19, Geert Uytterhoeven wrote: [...] >> 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. Indeed. The interrupt controller is basically the first device to be activated in the life of the system, and I can see a nasty chicken and egg problem creeping up if you're trying to do clock management in a generic way at that level. >> 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. > >> - 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]. As long as there is an agreement from the DT people on the presence of that extra property in the GIC node, I'm happy with that. I'd like it to be documented though. Thanks, M.
diff --git a/drivers/clk/shmobile/clk-mstp.c b/drivers/clk/shmobile/clk-mstp.c index 2d2fe773ac8168f9..742af84735a07450 100644 --- a/drivers/clk/shmobile/clk-mstp.c +++ b/drivers/clk/shmobile/clk-mstp.c @@ -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; + } + spin_lock_irqsave(&group->lock, flags); value = clk_readl(group->smstpcr);
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 <geert+renesas@glider.be> --- drivers/clk/shmobile/clk-mstp.c | 6 ++++++ 1 file changed, 6 insertions(+)