* [PATCH] clk: mmp2: avoid disabling the SP clock when unused
@ 2019-01-16 9:35 Lubomir Rintel
2019-01-16 16:37 ` Stephen Boyd
0 siblings, 1 reply; 6+ messages in thread
From: Lubomir Rintel @ 2019-01-16 9:35 UTC (permalink / raw)
To: Michael Turquette, Stephen Boyd
Cc: linux-clk, linux-kernel, Lubomir Rintel, stable
There could be vital functionality running on the SP PJ1 core it can not be
restarted just by turning the clock back on.
On the OLPC laptop, the keyboard controller code runs there. It
wouldn't be possible to load the driver for it as a module if the clock
is disabled on boot.
Cc: stable@vger.kernel.org # v4.18+
Fixes: commit fc27c2394d96 ("clk: mmp2: add SP clock")
Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
---
drivers/clk/mmp/clk-of-mmp2.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/mmp/clk-of-mmp2.c b/drivers/clk/mmp/clk-of-mmp2.c
index f2a1c9bbaa63..3e33f1295f59 100644
--- a/drivers/clk/mmp/clk-of-mmp2.c
+++ b/drivers/clk/mmp/clk-of-mmp2.c
@@ -246,7 +246,7 @@ static struct mmp_param_gate_clk apmu_gate_clks[] = {
{MMP2_CLK_CCIC1, "ccic1_clk", "ccic1_mix_clk", CLK_SET_RATE_PARENT, APMU_CCIC1, 0x1b, 0x1b, 0x0, 0, &ccic1_lock},
{MMP2_CLK_CCIC1_PHY, "ccic1_phy_clk", "ccic1_mix_clk", CLK_SET_RATE_PARENT, APMU_CCIC1, 0x24, 0x24, 0x0, 0, &ccic1_lock},
{MMP2_CLK_CCIC1_SPHY, "ccic1_sphy_clk", "ccic1_sphy_div", CLK_SET_RATE_PARENT, APMU_CCIC1, 0x300, 0x300, 0x0, 0, &ccic1_lock},
- {MMP2_CLK_SP, "sp_clk", NULL, CLK_SET_RATE_PARENT, APMU_SP, 0x1b, 0x1b, 0x0, 0, &sp_lock},
+ {MMP2_CLK_SP, "sp_clk", NULL, CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, APMU_SP, 0x1b, 0x1b, 0x0, 0, &sp_lock},
};
static void mmp2_axi_periph_clk_init(struct mmp2_clk_unit *pxa_unit)
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] clk: mmp2: avoid disabling the SP clock when unused
2019-01-16 9:35 [PATCH] clk: mmp2: avoid disabling the SP clock when unused Lubomir Rintel
@ 2019-01-16 16:37 ` Stephen Boyd
2019-01-16 17:26 ` Lubomir Rintel
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2019-01-16 16:37 UTC (permalink / raw)
To: Lubomir Rintel, Michael Turquette
Cc: linux-clk, linux-kernel, Lubomir Rintel, stable
Quoting Lubomir Rintel (2019-01-16 01:35:05)
> There could be vital functionality running on the SP PJ1 core it can not be
> restarted just by turning the clock back on.
>
> On the OLPC laptop, the keyboard controller code runs there. It
> wouldn't be possible to load the driver for it as a module if the clock
> is disabled on boot.
>
> Cc: stable@vger.kernel.org # v4.18+
> Fixes: commit fc27c2394d96 ("clk: mmp2: add SP clock")
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> ---
> drivers/clk/mmp/clk-of-mmp2.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/mmp/clk-of-mmp2.c b/drivers/clk/mmp/clk-of-mmp2.c
> index f2a1c9bbaa63..3e33f1295f59 100644
> --- a/drivers/clk/mmp/clk-of-mmp2.c
> +++ b/drivers/clk/mmp/clk-of-mmp2.c
> @@ -246,7 +246,7 @@ static struct mmp_param_gate_clk apmu_gate_clks[] = {
> {MMP2_CLK_CCIC1, "ccic1_clk", "ccic1_mix_clk", CLK_SET_RATE_PARENT, APMU_CCIC1, 0x1b, 0x1b, 0x0, 0, &ccic1_lock},
> {MMP2_CLK_CCIC1_PHY, "ccic1_phy_clk", "ccic1_mix_clk", CLK_SET_RATE_PARENT, APMU_CCIC1, 0x24, 0x24, 0x0, 0, &ccic1_lock},
> {MMP2_CLK_CCIC1_SPHY, "ccic1_sphy_clk", "ccic1_sphy_div", CLK_SET_RATE_PARENT, APMU_CCIC1, 0x300, 0x300, 0x0, 0, &ccic1_lock},
> - {MMP2_CLK_SP, "sp_clk", NULL, CLK_SET_RATE_PARENT, APMU_SP, 0x1b, 0x1b, 0x0, 0, &sp_lock},
> + {MMP2_CLK_SP, "sp_clk", NULL, CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, APMU_SP, 0x1b, 0x1b, 0x0, 0, &sp_lock},
Is it a critical clk that should never be turned off? If so, mark it as
CLK_IS_CRITICAL. Either way, please add a comment in the code about why
CLK_IGNORE_UNUSED or CLK_IS_CRITICAL is used.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] clk: mmp2: avoid disabling the SP clock when unused
2019-01-16 16:37 ` Stephen Boyd
@ 2019-01-16 17:26 ` Lubomir Rintel
2019-01-16 23:29 ` Stephen Boyd
0 siblings, 1 reply; 6+ messages in thread
From: Lubomir Rintel @ 2019-01-16 17:26 UTC (permalink / raw)
To: Stephen Boyd, Michael Turquette; +Cc: linux-clk, linux-kernel, stable
On Wed, 2019-01-16 at 08:37 -0800, Stephen Boyd wrote:
> Quoting Lubomir Rintel (2019-01-16 01:35:05)
> > There could be vital functionality running on the SP PJ1 core it can not be
> > restarted just by turning the clock back on.
> >
> > On the OLPC laptop, the keyboard controller code runs there. It
> > wouldn't be possible to load the driver for it as a module if the clock
> > is disabled on boot.
> >
> > Cc: stable@vger.kernel.org # v4.18+
> > Fixes: commit fc27c2394d96 ("clk: mmp2: add SP clock")
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > ---
> > drivers/clk/mmp/clk-of-mmp2.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/mmp/clk-of-mmp2.c b/drivers/clk/mmp/clk-of-mmp2.c
> > index f2a1c9bbaa63..3e33f1295f59 100644
> > --- a/drivers/clk/mmp/clk-of-mmp2.c
> > +++ b/drivers/clk/mmp/clk-of-mmp2.c
> > @@ -246,7 +246,7 @@ static struct mmp_param_gate_clk apmu_gate_clks[] = {
> > {MMP2_CLK_CCIC1, "ccic1_clk", "ccic1_mix_clk", CLK_SET_RATE_PARENT, APMU_CCIC1, 0x1b, 0x1b, 0x0, 0, &ccic1_lock},
> > {MMP2_CLK_CCIC1_PHY, "ccic1_phy_clk", "ccic1_mix_clk", CLK_SET_RATE_PARENT, APMU_CCIC1, 0x24, 0x24, 0x0, 0, &ccic1_lock},
> > {MMP2_CLK_CCIC1_SPHY, "ccic1_sphy_clk", "ccic1_sphy_div", CLK_SET_RATE_PARENT, APMU_CCIC1, 0x300, 0x300, 0x0, 0, &ccic1_lock},
> > - {MMP2_CLK_SP, "sp_clk", NULL, CLK_SET_RATE_PARENT, APMU_SP, 0x1b, 0x1b, 0x0, 0, &sp_lock},
> > + {MMP2_CLK_SP, "sp_clk", NULL, CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, APMU_SP, 0x1b, 0x1b, 0x0, 0, &sp_lock},
>
> Is it a critical clk that should never be turned off?
I don't think it is. It is entirely plausible to have no use for the
"security processor", and in that case it's just okay to keep the clock
disabled.
> If so, mark it as
> CLK_IS_CRITICAL. Either way, please add a comment in the code about why
> CLK_IGNORE_UNUSED or CLK_IS_CRITICAL is used.
Okay.
Thanks,
Lubo
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] clk: mmp2: avoid disabling the SP clock when unused
2019-01-16 17:26 ` Lubomir Rintel
@ 2019-01-16 23:29 ` Stephen Boyd
2019-01-17 9:47 ` Lubomir Rintel
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2019-01-16 23:29 UTC (permalink / raw)
To: Lubomir Rintel, Michael Turquette; +Cc: linux-clk, linux-kernel, stable
Quoting Lubomir Rintel (2019-01-16 09:26:31)
> On Wed, 2019-01-16 at 08:37 -0800, Stephen Boyd wrote:
> > Quoting Lubomir Rintel (2019-01-16 01:35:05)
> > > There could be vital functionality running on the SP PJ1 core it can not be
> > > restarted just by turning the clock back on.
> > >
> > > On the OLPC laptop, the keyboard controller code runs there. It
> > > wouldn't be possible to load the driver for it as a module if the clock
> > > is disabled on boot.
> > >
> > > Cc: stable@vger.kernel.org # v4.18+
> > > Fixes: commit fc27c2394d96 ("clk: mmp2: add SP clock")
> > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > > ---
> > > drivers/clk/mmp/clk-of-mmp2.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/clk/mmp/clk-of-mmp2.c b/drivers/clk/mmp/clk-of-mmp2.c
> > > index f2a1c9bbaa63..3e33f1295f59 100644
> > > --- a/drivers/clk/mmp/clk-of-mmp2.c
> > > +++ b/drivers/clk/mmp/clk-of-mmp2.c
> > > @@ -246,7 +246,7 @@ static struct mmp_param_gate_clk apmu_gate_clks[] = {
> > > {MMP2_CLK_CCIC1, "ccic1_clk", "ccic1_mix_clk", CLK_SET_RATE_PARENT, APMU_CCIC1, 0x1b, 0x1b, 0x0, 0, &ccic1_lock},
> > > {MMP2_CLK_CCIC1_PHY, "ccic1_phy_clk", "ccic1_mix_clk", CLK_SET_RATE_PARENT, APMU_CCIC1, 0x24, 0x24, 0x0, 0, &ccic1_lock},
> > > {MMP2_CLK_CCIC1_SPHY, "ccic1_sphy_clk", "ccic1_sphy_div", CLK_SET_RATE_PARENT, APMU_CCIC1, 0x300, 0x300, 0x0, 0, &ccic1_lock},
> > > - {MMP2_CLK_SP, "sp_clk", NULL, CLK_SET_RATE_PARENT, APMU_SP, 0x1b, 0x1b, 0x0, 0, &sp_lock},
> > > + {MMP2_CLK_SP, "sp_clk", NULL, CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, APMU_SP, 0x1b, 0x1b, 0x0, 0, &sp_lock},
> >
> > Is it a critical clk that should never be turned off?
>
> I don't think it is. It is entirely plausible to have no use for the
> "security processor", and in that case it's just okay to keep the clock
> disabled.
So does the firmware/bootloader leave the clk enabled out of boot and we
shouldn't really touch the on/off bits of the clk hardware from the
kernel? Or do we want to actively manage this clk from a driver
somewhere in the kernel?
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] clk: mmp2: avoid disabling the SP clock when unused
2019-01-16 23:29 ` Stephen Boyd
@ 2019-01-17 9:47 ` Lubomir Rintel
2019-01-18 17:31 ` Stephen Boyd
0 siblings, 1 reply; 6+ messages in thread
From: Lubomir Rintel @ 2019-01-17 9:47 UTC (permalink / raw)
To: Stephen Boyd, Michael Turquette; +Cc: linux-clk, linux-kernel, stable
On Wed, 2019-01-16 at 15:29 -0800, Stephen Boyd wrote:
> Quoting Lubomir Rintel (2019-01-16 09:26:31)
> > On Wed, 2019-01-16 at 08:37 -0800, Stephen Boyd wrote:
> > > Quoting Lubomir Rintel (2019-01-16 01:35:05)
> > > > There could be vital functionality running on the SP PJ1 core it can not be
> > > > restarted just by turning the clock back on.
> > > >
> > > > On the OLPC laptop, the keyboard controller code runs there. It
> > > > wouldn't be possible to load the driver for it as a module if the clock
> > > > is disabled on boot.
> > > >
> > > > Cc: stable@vger.kernel.org # v4.18+
> > > > Fixes: commit fc27c2394d96 ("clk: mmp2: add SP clock")
> > > > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > > > ---
> > > > drivers/clk/mmp/clk-of-mmp2.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/clk/mmp/clk-of-mmp2.c b/drivers/clk/mmp/clk-of-mmp2.c
> > > > index f2a1c9bbaa63..3e33f1295f59 100644
> > > > --- a/drivers/clk/mmp/clk-of-mmp2.c
> > > > +++ b/drivers/clk/mmp/clk-of-mmp2.c
> > > > @@ -246,7 +246,7 @@ static struct mmp_param_gate_clk apmu_gate_clks[] = {
> > > > {MMP2_CLK_CCIC1, "ccic1_clk", "ccic1_mix_clk", CLK_SET_RATE_PARENT, APMU_CCIC1, 0x1b, 0x1b, 0x0, 0, &ccic1_lock},
> > > > {MMP2_CLK_CCIC1_PHY, "ccic1_phy_clk", "ccic1_mix_clk", CLK_SET_RATE_PARENT, APMU_CCIC1, 0x24, 0x24, 0x0, 0, &ccic1_lock},
> > > > {MMP2_CLK_CCIC1_SPHY, "ccic1_sphy_clk", "ccic1_sphy_div", CLK_SET_RATE_PARENT, APMU_CCIC1, 0x300, 0x300, 0x0, 0, &ccic1_lock},
> > > > - {MMP2_CLK_SP, "sp_clk", NULL, CLK_SET_RATE_PARENT, APMU_SP, 0x1b, 0x1b, 0x0, 0, &sp_lock},
> > > > + {MMP2_CLK_SP, "sp_clk", NULL, CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, APMU_SP, 0x1b, 0x1b, 0x0, 0, &sp_lock},
> > >
> > > Is it a critical clk that should never be turned off?
> >
> > I don't think it is. It is entirely plausible to have no use for the
> > "security processor", and in that case it's just okay to keep the clock
> > disabled.
>
> So does the firmware/bootloader leave the clk enabled out of boot and we
> shouldn't really touch the on/off bits of the clk hardware from the
> kernel?
I think so.
> Or do we want to actively manage this clk from a driver
> somewhere in the kernel?
The olpc_apsp driver actually manages this clock, but that might turn
out to be the wrong thing to do. It currently only works if the driver
is built-in and thus the clock is always kept enabled.
I'm now somewhat more confused that I believed myself to be when
sending the patch. Perhaps you could help me understand things a bit
more:
1.) What's the principal difference between CLK_IGNORE_UNUSED and
CLK_IS_CRITICAL? Is it that the CLK_IGNORE_UNUSED clocks are permitted
to be disabled by a driver, but an attempt to disable CLK_IS_CRITICAL
is a bug?
2.) Perhaps it makes sense to disable the SP on the machines that don't
utilize it even if firmware keeps it enabled? Would it make sense to
make the clk driver use the "protected-clocks" DT property for this?
----8<----
Here's some more context for the SP on the MMP2:
The SP is a small PJ1 core. It starts when the platform is powered on
and eventually brings up the large PJ4 core.
On the OLPC machine, the first stage firmware (cforth) starts running
on the SP, configures the DRAM, starts the boot firmware (openfirmware)
on the main PJ4 core and then enters a loop that bit-bangs the PS/2
protocol on the attached keyboard/touchpad.
It is entirely possible that on some boards the SP is not used for
anything but the bringup of the bootloader on the main core.
SP is sometimes referred to as WTM, "wireless trusted module", but it's
not clear to me why is it the case. There's no documentation.
Lubo
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] clk: mmp2: avoid disabling the SP clock when unused
2019-01-17 9:47 ` Lubomir Rintel
@ 2019-01-18 17:31 ` Stephen Boyd
0 siblings, 0 replies; 6+ messages in thread
From: Stephen Boyd @ 2019-01-18 17:31 UTC (permalink / raw)
To: Lubomir Rintel, Michael Turquette; +Cc: linux-clk, linux-kernel, stable
Quoting Lubomir Rintel (2019-01-17 01:47:55)
> On Wed, 2019-01-16 at 15:29 -0800, Stephen Boyd wrote:
> > Quoting Lubomir Rintel (2019-01-16 09:26:31)
> > > On Wed, 2019-01-16 at 08:37 -0800, Stephen Boyd wrote:
> > > >
> > > > Is it a critical clk that should never be turned off?
> > >
> > > I don't think it is. It is entirely plausible to have no use for the
> > > "security processor", and in that case it's just okay to keep the clock
> > > disabled.
> >
> > So does the firmware/bootloader leave the clk enabled out of boot and we
> > shouldn't really touch the on/off bits of the clk hardware from the
> > kernel?
>
> I think so.
>
> > Or do we want to actively manage this clk from a driver
> > somewhere in the kernel?
>
> The olpc_apsp driver actually manages this clock, but that might turn
> out to be the wrong thing to do. It currently only works if the driver
> is built-in and thus the clock is always kept enabled.
>
> I'm now somewhat more confused that I believed myself to be when
> sending the patch. Perhaps you could help me understand things a bit
> more:
>
> 1.) What's the principal difference between CLK_IGNORE_UNUSED and
> CLK_IS_CRITICAL? Is it that the CLK_IGNORE_UNUSED clocks are permitted
> to be disabled by a driver, but an attempt to disable CLK_IS_CRITICAL
> is a bug?
Yes. CLK_IGNORE_UNUSED is a workaround for two opposing forces. The
first being the lack of a way for the clk framework to "hand off" the
state of a clk to clk consumers after late init and the second being the
desire to save power by disabling unused clocks.
>
> 2.) Perhaps it makes sense to disable the SP on the machines that don't
> utilize it even if firmware keeps it enabled? Would it make sense to
> make the clk driver use the "protected-clocks" DT property for this?
I don't think so. The protected-clocks property lists clks that
shouldn't be touched by the OS. Critical clks in theory are still
touched because the framework turns them on once, in case they're not
already enabled. That's sort of a weird implementation detail that some
drivers rely on. We haven't had a situation where drivers shouldn't
read/write clk registers but also mark them as critical. I would think
that we just wouldn't register those clks with the kernel because they
would never be used.
Does having this clk actually matter? Maybe it can just be removed
instead? Put another way, is it saving any power to manage the clk from
the kernel?
>
> ----8<----
>
> Here's some more context for the SP on the MMP2:
>
> The SP is a small PJ1 core. It starts when the platform is powered on
> and eventually brings up the large PJ4 core.
>
> On the OLPC machine, the first stage firmware (cforth) starts running
> on the SP, configures the DRAM, starts the boot firmware (openfirmware)
> on the main PJ4 core and then enters a loop that bit-bangs the PS/2
> protocol on the attached keyboard/touchpad.
>
> It is entirely possible that on some boards the SP is not used for
> anything but the bringup of the bootloader on the main core.
>
> SP is sometimes referred to as WTM, "wireless trusted module", but it's
> not clear to me why is it the case. There's no documentation.
>
Ok. Thanks for the background.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-01-18 17:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-16 9:35 [PATCH] clk: mmp2: avoid disabling the SP clock when unused Lubomir Rintel
2019-01-16 16:37 ` Stephen Boyd
2019-01-16 17:26 ` Lubomir Rintel
2019-01-16 23:29 ` Stephen Boyd
2019-01-17 9:47 ` Lubomir Rintel
2019-01-18 17:31 ` Stephen Boyd
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).