[PATCH 0/3] clk: sunxi-ng: gate/ungate PLL CPU clk after rate change

[PATCH 0/3] clk: sunxi-ng: gate/ungate PLL CPU clk after rate change

[Chen-Yu Tsai]

Hi everyone,

This series adds a clk notifier for use on the PLL CPU clks found in
Allwinner SoCs. Some people have observed issues with the design and
implementation of the CPU PLL clock, starting from the A31. Changes
to the PLL clock need a few cycles to stabilize. If the changes are
too drastic, the dividers in particular, there is a good chance that
the system will hang.

Previously we thought that reparenting the CPU clock away from the PLL
while changes were made, and then reparenting it back once it was stable,
should have been enough to mitigate the issue. Unfortunately it was
not. With cpufreq support for A33 recently introduced in commit
03749eb88e63 ("ARM: dts: sun8i: add opp-v2 table for A33"), system hangs
were observed one out of two to three boots, right after userspace
configured cpufreq to switch to the ondemand governor. Other experiments
done by Ondrej Jirman [1] show that it is not enough to just reparent
the CPU clock, but the PLL clock's dividers must not be used.

We suspect the divider changes make the PLL unstable to the point that
it can not recover, possibly not providing any output afterwards. We
lack any hard evidence (oscilloscope readings or hardware implementation
details) to fully explain the behavior. However, if the hardware is
stuck in some undesired state, it is possible to "reset" it, by gating
the PLL, then ungating it.

This series adds a new clk notifier that does exactly that. The clk
notifier is registered on the PLL clock. Whenever its rate is changed,
the notifier comes in and toggles the gate. The notifier should always
be the first one registered. And all consumers of the clock must also
have notifiers on it to temporarily reparent away during the change.

Patches 2 and 3 register this new notifier for the CPU PLL clocks on
the A33 and H3, respectively. With the first 2 patches applied, the
cpufreq related system hangs on the A33 go away.

Given that commit 03749eb88e63 ("ARM: dts: sun8i: add opp-v2 table for
A33") is already in v4.11-rc, I suggest we either try to merge the
first 2 patches for a very late -rc fix, or drop A33 cpufreq support
from v4.11, and add it later once this series is merged.

Regards
ChenYu


[1] http://www.spinics.net/lists/arm-kernel/msg552501.html

Chen-Yu Tsai (3):
  clk: sunxi-ng: Add clk notifier to gate then ungate PLL clocks
  clk: sunxi-ng: a33: gate then ungate PLL CPU clk after rate change
  clk: sunxi-ng: h3: gate then ungate PLL CPU clk after rate change

 drivers/clk/sunxi-ng/ccu-sun8i-a33.c | 11 ++++++++
 drivers/clk/sunxi-ng/ccu-sun8i-h3.c  | 11 ++++++++
 drivers/clk/sunxi-ng/ccu_common.c    | 49 ++++++++++++++++++++++++++++++++++++
 drivers/clk/sunxi-ng/ccu_common.h    | 12 +++++++++
 4 files changed, 83 insertions(+)

-- 
2.11.0

[PATCH 1/3] clk: sunxi-ng: Add clk notifier to gate then ungate PLL clocks

[Chen-Yu Tsai]

In common PLL designs, changes to the dividers take effect almost
immediately, while changes to the multipliers (implemented as
dividers in the feedback loop) take a few cycles to work into
the feedback loop for the PLL to stablize.

Sometimes when the PLL clock rate is changed, the decrease in the
divider is too much for the decrease in the multiplier to catch up.
The PLL clock rate will spike, and in some cases, might lock up
completely. This is especially the case if the divider changed is
the pre-divider, which affects the reference frequency.

This patch introduces a clk notifier callback that will gate and
then ungate a clk after a rate change, effectively resetting it,
so it continues to work, despite any possible lockups. Care must
be taken to reparent any consumers to other temporary clocks during
the rate change, and that this notifier callback must be the first
to be registered.

This is intended to fix occasional lockups with cpufreq on newer
Allwinner SoCs, such as the A33 and the H3. Previously it was
thought that reparenting the cpu clock away from the PLL while
it stabilized was enough, as this worked quite well on the A31.

On the A33, hangs have been observed after cpufreq was recently
introduced. With the H3, a more thorough test [1] showed that
reparenting alone isn't enough. The system still locks up unless
the dividers are limited to 1.

A hunch was if the PLL was stuck in some unknown state, perhaps
gating then ungating it would bring it back to normal. Tests
done by Icenowy Zheng using Ondrej's test firmware shows this
to be a valid solution.

[1] http://www.spinics.net/lists/arm-kernel/msg552501.html

Reported-by: Ondrej Jirman <megous@megous.com>
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
Tested-by: Icenowy Zheng <icenowy@aosc.io>
Tested-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 drivers/clk/sunxi-ng/ccu_common.c | 49 +++++++++++++++++++++++++++++++++++++++
 drivers/clk/sunxi-ng/ccu_common.h | 12 ++++++++++
 2 files changed, 61 insertions(+)

diff --git a/drivers/clk/sunxi-ng/ccu_common.c b/drivers/clk/sunxi-ng/ccu_common.c
index 188fa50d0380..40aac316128f 100644
--- a/drivers/clk/sunxi-ng/ccu_common.c
+++ b/drivers/clk/sunxi-ng/ccu_common.c
@@ -14,11 +14,13 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/iopoll.h>
 #include <linux/slab.h>
 
 #include "ccu_common.h"
+#include "ccu_gate.h"
 #include "ccu_reset.h"
 
 static DEFINE_SPINLOCK(ccu_lock);
@@ -39,6 +41,53 @@ void ccu_helper_wait_for_lock(struct ccu_common *common, u32 lock)
 	WARN_ON(readl_relaxed_poll_timeout(addr, reg, reg & lock, 100, 70000));
 }
 
+/*
+ * This clock notifier is called when the frequency of a PLL clock is
+ * changed. In common PLL designs, changes to the dividers take effect
+ * almost immediately, while changes to the multipliers (implemented
+ * as dividers in the feedback loop) take a few cycles to work into
+ * the feedback loop for the PLL to stablize.
+ *
+ * Sometimes when the PLL clock rate is changed, the decrease in the
+ * divider is too much for the decrease in the multiplier to catch up.
+ * The PLL clock rate will spike, and in some cases, might lock up
+ * completely.
+ *
+ * This notifier callback will gate and then ungate the clock,
+ * effectively resetting it, so it proceeds to work. Care must be
+ * taken to reparent consumers to other temporary clocks during the
+ * rate change, and that this notifier callback must be the first
+ * to be registered.
+ */
+static int ccu_pll_notifier_cb(struct notifier_block *nb,
+			       unsigned long event, void *data)
+{
+	struct ccu_pll_nb *pll = to_ccu_pll_nb(nb);
+	int ret = 0;
+
+	if (event != POST_RATE_CHANGE)
+		goto out;
+
+	ccu_gate_helper_disable(pll->common, pll->enable);
+
+	ret = ccu_gate_helper_enable(pll->common, pll->enable);
+	if (ret)
+		goto out;
+
+	ccu_helper_wait_for_lock(pll->common, pll->lock);
+
+out:
+	return notifier_from_errno(ret);
+}
+
+int ccu_pll_notifier_register(struct ccu_pll_nb *pll_nb)
+{
+	pll_nb->clk_nb.notifier_call = ccu_pll_notifier_cb;
+
+	return clk_notifier_register(pll_nb->common->hw.clk,
+				     &pll_nb->clk_nb);
+}
+
 int sunxi_ccu_probe(struct device_node *node, void __iomem *reg,
 		    const struct sunxi_ccu_desc *desc)
 {
diff --git a/drivers/clk/sunxi-ng/ccu_common.h b/drivers/clk/sunxi-ng/ccu_common.h
index 73d81dc58fc5..d6fdd7a789aa 100644
--- a/drivers/clk/sunxi-ng/ccu_common.h
+++ b/drivers/clk/sunxi-ng/ccu_common.h
@@ -83,6 +83,18 @@ struct sunxi_ccu_desc {
 
 void ccu_helper_wait_for_lock(struct ccu_common *common, u32 lock);
 
+struct ccu_pll_nb {
+	struct notifier_block	clk_nb;
+	struct ccu_common	*common;
+
+	u32	enable;
+	u32	lock;
+};
+
+#define to_ccu_pll_nb(_nb) container_of(_nb, struct ccu_pll_nb, clk_nb)
+
+int ccu_pll_notifier_register(struct ccu_pll_nb *pll_nb);
+
 int sunxi_ccu_probe(struct device_node *node, void __iomem *reg,
 		    const struct sunxi_ccu_desc *desc);
 
-- 
2.11.0

[PATCH 2/3] clk: sunxi-ng: a33: gate then ungate PLL CPU clk after rate change

[Chen-Yu Tsai]

This patch utilizes the new PLL clk notifier to gate then ungate the
PLL CPU clock after rate changes. This should mitigate the system
hangs observed after the introduction of cpufreq for the A33.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
Tested-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---
 drivers/clk/sunxi-ng/ccu-sun8i-a33.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-a33.c b/drivers/clk/sunxi-ng/ccu-sun8i-a33.c
index 56370c2c7f98..8d38e6510e29 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-a33.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-a33.c
@@ -756,6 +756,13 @@ static const struct sunxi_ccu_desc sun8i_a33_ccu_desc = {
 	.num_resets	= ARRAY_SIZE(sun8i_a33_ccu_resets),
 };
 
+static struct ccu_pll_nb sun8i_a33_pll_cpu_nb = {
+	.common	= &pll_cpux_clk.common,
+	/* copy from pll_cpux_clk */
+	.enable	= BIT(31),
+	.lock	= BIT(28),
+};
+
 static struct ccu_mux_nb sun8i_a33_cpu_nb = {
 	.common		= &cpux_clk.common,
 	.cm		= &cpux_clk.mux,
@@ -787,6 +794,10 @@ static void __init sun8i_a33_ccu_setup(struct device_node *node)
 
 	sunxi_ccu_probe(node, reg, &sun8i_a33_ccu_desc);
 
+	/* Gate then ungate PLL CPU after any rate changes */
+	ccu_pll_notifier_register(&sun8i_a33_pll_cpu_nb);
+
+	/* Reparent CPU during PLL CPU rate changes */
 	ccu_mux_notifier_register(pll_cpux_clk.common.hw.clk,
 				  &sun8i_a33_cpu_nb);
 }
-- 
2.11.0

[PATCH 3/3] clk: sunxi-ng: h3: gate then ungate PLL CPU clk after rate change

[Chen-Yu Tsai]

This patch utilizes the new PLL clk notifier to gate then ungate the
PLL CPU clock after rate changes. This should prevent any system hangs
resulting from cpufreq changes to the clk.

Reported-by: Ondrej Jirman <megous@megous.com>
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
Tested-by: Icenowy Zheng <icenowy@aosc.io>
---
 drivers/clk/sunxi-ng/ccu-sun8i-h3.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
index 4cbc1b701b7c..fc04ef2af1ac 100644
--- a/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
+++ b/drivers/clk/sunxi-ng/ccu-sun8i-h3.c
@@ -1103,6 +1103,13 @@ static const struct sunxi_ccu_desc sun50i_h5_ccu_desc = {
 	.num_resets	= ARRAY_SIZE(sun50i_h5_ccu_resets),
 };
 
+static struct ccu_pll_nb sun8i_h3_pll_cpu_nb = {
+	.common	= &pll_cpux_clk.common,
+	/* copy from pll_cpux_clk */
+	.enable	= BIT(31),
+	.lock	= BIT(28),
+};
+
 static struct ccu_mux_nb sun8i_h3_cpu_nb = {
 	.common		= &cpux_clk.common,
 	.cm		= &cpux_clk.mux,
@@ -1130,6 +1137,10 @@ static void __init sunxi_h3_h5_ccu_init(struct device_node *node,
 
 	sunxi_ccu_probe(node, reg, desc);
 
+	/* Gate then ungate PLL CPU after any rate changes */
+	ccu_pll_notifier_register(&sun8i_h3_pll_cpu_nb);
+
+	/* Reparent CPU during PLL CPU rate changes */
 	ccu_mux_notifier_register(pll_cpux_clk.common.hw.clk,
 				  &sun8i_h3_cpu_nb);
 }
-- 
2.11.0

Re: [PATCH 1/3] clk: sunxi-ng: Add clk notifier to gate then ungate PLL clocks

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2347 bytes --]

Hi Chen-Yu,

On Thu, Apr 13, 2017 at 10:13:52AM +0800, Chen-Yu Tsai wrote:
> In common PLL designs, changes to the dividers take effect almost
> immediately, while changes to the multipliers (implemented as
> dividers in the feedback loop) take a few cycles to work into
> the feedback loop for the PLL to stablize.
> 
> Sometimes when the PLL clock rate is changed, the decrease in the
> divider is too much for the decrease in the multiplier to catch up.
> The PLL clock rate will spike, and in some cases, might lock up
> completely. This is especially the case if the divider changed is
> the pre-divider, which affects the reference frequency.
> 
> This patch introduces a clk notifier callback that will gate and
> then ungate a clk after a rate change, effectively resetting it,
> so it continues to work, despite any possible lockups. Care must
> be taken to reparent any consumers to other temporary clocks during
> the rate change, and that this notifier callback must be the first
> to be registered.
> 
> This is intended to fix occasional lockups with cpufreq on newer
> Allwinner SoCs, such as the A33 and the H3. Previously it was
> thought that reparenting the cpu clock away from the PLL while
> it stabilized was enough, as this worked quite well on the A31.
> 
> On the A33, hangs have been observed after cpufreq was recently
> introduced. With the H3, a more thorough test [1] showed that
> reparenting alone isn't enough. The system still locks up unless
> the dividers are limited to 1.
> 
> A hunch was if the PLL was stuck in some unknown state, perhaps
> gating then ungating it would bring it back to normal. Tests
> done by Icenowy Zheng using Ondrej's test firmware shows this
> to be a valid solution.
> 
> [1] http://www.spinics.net/lists/arm-kernel/msg552501.html
> 
> Reported-by: Ondrej Jirman <megous@megous.com>
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> Tested-by: Icenowy Zheng <icenowy@aosc.io>
> Tested-by: Quentin Schulz <quentin.schulz@free-electrons.com>

Thanks for looking into this, and coming up with a clean solution, and
a great commit log.

However, I wondering, isn't that notifier just a re-implementation of
CLK_SET_RATE_GATE?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 1/3] clk: sunxi-ng: Add clk notifier to gate then ungate PLL clocks

[Chen-Yu Tsai]

On Thu, Apr 13, 2017 at 3:02 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi Chen-Yu,
>
> On Thu, Apr 13, 2017 at 10:13:52AM +0800, Chen-Yu Tsai wrote:
>> In common PLL designs, changes to the dividers take effect almost
>> immediately, while changes to the multipliers (implemented as
>> dividers in the feedback loop) take a few cycles to work into
>> the feedback loop for the PLL to stablize.
>>
>> Sometimes when the PLL clock rate is changed, the decrease in the
>> divider is too much for the decrease in the multiplier to catch up.
>> The PLL clock rate will spike, and in some cases, might lock up
>> completely. This is especially the case if the divider changed is
>> the pre-divider, which affects the reference frequency.
>>
>> This patch introduces a clk notifier callback that will gate and
>> then ungate a clk after a rate change, effectively resetting it,
>> so it continues to work, despite any possible lockups. Care must
>> be taken to reparent any consumers to other temporary clocks during
>> the rate change, and that this notifier callback must be the first
>> to be registered.
>>
>> This is intended to fix occasional lockups with cpufreq on newer
>> Allwinner SoCs, such as the A33 and the H3. Previously it was
>> thought that reparenting the cpu clock away from the PLL while
>> it stabilized was enough, as this worked quite well on the A31.
>>
>> On the A33, hangs have been observed after cpufreq was recently
>> introduced. With the H3, a more thorough test [1] showed that
>> reparenting alone isn't enough. The system still locks up unless
>> the dividers are limited to 1.
>>
>> A hunch was if the PLL was stuck in some unknown state, perhaps
>> gating then ungating it would bring it back to normal. Tests
>> done by Icenowy Zheng using Ondrej's test firmware shows this
>> to be a valid solution.
>>
>> [1] http://www.spinics.net/lists/arm-kernel/msg552501.html
>>
>> Reported-by: Ondrej Jirman <megous@megous.com>
>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> Tested-by: Icenowy Zheng <icenowy@aosc.io>
>> Tested-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>
> Thanks for looking into this, and coming up with a clean solution, and
> a great commit log.
>
> However, I wondering, isn't that notifier just a re-implementation of
> CLK_SET_RATE_GATE?

They are not the same. AFAIK, CLK_SET_RATE_GATE tells the clk framework
that this clk's rate cannot be changed if it is enabled (which means
some one is using it). However the clk framework does nothing to
actually handle it. It just returns an error. Any consumers are
responsible for gating the clock before making changes. This is a nice
thing to have, as it can prevent unintended changes to dot clocks or
audio clocks used with active output streams. We could consider setting
this for the audio and video PLLs.

Here we are dealing with the CPU PLL, which, for practical reasons,
is always enabled as far as the clk framework is concerned. The
reason being the OPPs are never low enough for the CPU clock to
use any other parent. To have it disabled, we would have to kick
consumers (the CPU clock in this case) to use other clocks, so it's
safe, remember which ones we kicked, and then bring them back once
everything is done.

AFAIK, we, samsung, rockchip, meson, do the temporary reparenting
using clk_notifiers to access the mux registers directly. As far
as the clk framework is concerned, nothing has changed.

I'm not saying it's not possible to support this in the core, but
the core already has to do a lot of bookkeeping and recalculation
when anything changes. Adding something transient into the process
isn't helping. And the reparenting might temporarily violate any
downstream requirements.

For now, I think clk notifiers is the easier solution for these
one off requirements that are pretty much contained in a small
part of the system.

Regards
ChenYu

Re: [PATCH 1/3] clk: sunxi-ng: Add clk notifier to gate then ungate PLL clocks

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4734 bytes --]

On Thu, Apr 13, 2017 at 03:35:30PM +0800, Chen-Yu Tsai wrote:
> On Thu, Apr 13, 2017 at 3:02 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > Hi Chen-Yu,
> >
> > On Thu, Apr 13, 2017 at 10:13:52AM +0800, Chen-Yu Tsai wrote:
> >> In common PLL designs, changes to the dividers take effect almost
> >> immediately, while changes to the multipliers (implemented as
> >> dividers in the feedback loop) take a few cycles to work into
> >> the feedback loop for the PLL to stablize.
> >>
> >> Sometimes when the PLL clock rate is changed, the decrease in the
> >> divider is too much for the decrease in the multiplier to catch up.
> >> The PLL clock rate will spike, and in some cases, might lock up
> >> completely. This is especially the case if the divider changed is
> >> the pre-divider, which affects the reference frequency.
> >>
> >> This patch introduces a clk notifier callback that will gate and
> >> then ungate a clk after a rate change, effectively resetting it,
> >> so it continues to work, despite any possible lockups. Care must
> >> be taken to reparent any consumers to other temporary clocks during
> >> the rate change, and that this notifier callback must be the first
> >> to be registered.
> >>
> >> This is intended to fix occasional lockups with cpufreq on newer
> >> Allwinner SoCs, such as the A33 and the H3. Previously it was
> >> thought that reparenting the cpu clock away from the PLL while
> >> it stabilized was enough, as this worked quite well on the A31.
> >>
> >> On the A33, hangs have been observed after cpufreq was recently
> >> introduced. With the H3, a more thorough test [1] showed that
> >> reparenting alone isn't enough. The system still locks up unless
> >> the dividers are limited to 1.
> >>
> >> A hunch was if the PLL was stuck in some unknown state, perhaps
> >> gating then ungating it would bring it back to normal. Tests
> >> done by Icenowy Zheng using Ondrej's test firmware shows this
> >> to be a valid solution.
> >>
> >> [1] http://www.spinics.net/lists/arm-kernel/msg552501.html
> >>
> >> Reported-by: Ondrej Jirman <megous@megous.com>
> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> >> Tested-by: Icenowy Zheng <icenowy@aosc.io>
> >> Tested-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> >
> > Thanks for looking into this, and coming up with a clean solution, and
> > a great commit log.
> >
> > However, I wondering, isn't that notifier just a re-implementation of
> > CLK_SET_RATE_GATE?
> 
> They are not the same. AFAIK, CLK_SET_RATE_GATE tells the clk framework
> that this clk's rate cannot be changed if it is enabled (which means
> some one is using it). However the clk framework does nothing to
> actually handle it. It just returns an error. Any consumers are
> responsible for gating the clock before making changes. This is a nice
> thing to have, as it can prevent unintended changes to dot clocks or
> audio clocks used with active output streams. We could consider setting
> this for the audio and video PLLs.

Ah, you're right. I merged the two first patches and will send them
for 4.11.

> Here we are dealing with the CPU PLL, which, for practical reasons,
> is always enabled as far as the clk framework is concerned. The
> reason being the OPPs are never low enough for the CPU clock to
> use any other parent. To have it disabled, we would have to kick
> consumers (the CPU clock in this case) to use other clocks, so it's
> safe, remember which ones we kicked, and then bring them back once
> everything is done.
> 
> AFAIK, we, samsung, rockchip, meson, do the temporary reparenting
> using clk_notifiers to access the mux registers directly. As far
> as the clk framework is concerned, nothing has changed.
> 
> I'm not saying it's not possible to support this in the core, but
> the core already has to do a lot of bookkeeping and recalculation
> when anything changes. Adding something transient into the process
> isn't helping. And the reparenting might temporarily violate any
> downstream requirements.
> 
> For now, I think clk notifiers is the easier solution for these
> one off requirements that are pretty much contained in a small
> part of the system.

However, the third one is less urgent, since we don't have H3 cpufreq
support yet, so we won't hit that case, and I'd like to have first a
common function that register the notifiers since the order really
matters, we don't want to have someone getting it wrong.

Since this is 4.13 material, there's no rush on that one though.

Thanks again!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]