[PATCH] clk: shmobile: div6: Avoid changing divisor in .disable()

[PATCH] clk: shmobile: div6: Avoid changing divisor in .disable()

[Geert Uytterhoeven]

While DIV6 clocks require the divisor field to be non-zero when stopping
the clock, some clocks (e.g. ZB on sh73a0) fail to be re-enabled later
if the divisor field is changed when stopping the clock.

To fix this, do not touch the divisor field if it's already non-zero.

On kzm9g, the smsc911x Ethernet controller is connected to the sh73a0
Bus State Controller, which is clocked by the ZB clock. Without this
fix, if the ZB clock is disabled during system suspend, and re-enabled
during resume, the kernel locks up when the smsc911x driver tries to
access the Ethernet registers.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/clk/shmobile/clk-div6.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/shmobile/clk-div6.c b/drivers/clk/shmobile/clk-div6.c
index 639241e31e03ec24..905bf98a74fcfb1e 100644
--- a/drivers/clk/shmobile/clk-div6.c
+++ b/drivers/clk/shmobile/clk-div6.c
@@ -54,12 +54,17 @@ static int cpg_div6_clock_enable(struct clk_hw *hw)
 static void cpg_div6_clock_disable(struct clk_hw *hw)
 {
 	struct div6_clock *clock = to_div6_clock(hw);
+	u32 val;
 
-	/* DIV6 clocks require the divisor field to be non-zero when stopping
+	val = clk_readl(clock->reg);
+	val |= CPG_DIV6_CKSTP;
+	/*
+	 * DIV6 clocks require the divisor field to be non-zero when stopping
 	 * the clock.
 	 */
-	clk_writel(clk_readl(clock->reg) | CPG_DIV6_CKSTP | CPG_DIV6_DIV_MASK,
-		   clock->reg);
+	if (!(val & CPG_DIV6_DIV_MASK))
+		val |= CPG_DIV6_DIV_MASK;
+	clk_writel(val, clock->reg);
 }
 
 static int cpg_div6_clock_is_enabled(struct clk_hw *hw)
-- 
1.9.1

Re: [PATCH] clk: shmobile: div6: Avoid changing divisor in .disable()

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Monday 24 November 2014 17:20:48 Geert Uytterhoeven wrote:
> While DIV6 clocks require the divisor field to be non-zero when stopping
> the clock, some clocks (e.g. ZB on sh73a0) fail to be re-enabled later
> if the divisor field is changed when stopping the clock.
> 
> To fix this, do not touch the divisor field if it's already non-zero.
> 
> On kzm9g, the smsc911x Ethernet controller is connected to the sh73a0
> Bus State Controller, which is clocked by the ZB clock. Without this
> fix, if the ZB clock is disabled during system suspend, and re-enabled
> during resume, the kernel locks up when the smsc911x driver tries to
> access the Ethernet registers.

Do we have any idea what happens at the hardware level ? Could it be that the 
divider set when stopping the clock has an invalid value ? Does the problem 
occur if you set the divider to a valid value instead of CPG_DIV6_DIV_MASK 
when stopping the clock ?

The problem could also be caused by the new divider not being taken into 
account fast enough if modified during the same write cycle as the CKSTP bit 
when restarting the clock. Does the system still lock if you set the divider 
and clear the CKSTP bit in two seperate write operations when restarting the 
clock ?

> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/clk/shmobile/clk-div6.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/shmobile/clk-div6.c
> b/drivers/clk/shmobile/clk-div6.c index 639241e31e03ec24..905bf98a74fcfb1e
> 100644
> --- a/drivers/clk/shmobile/clk-div6.c
> +++ b/drivers/clk/shmobile/clk-div6.c
> @@ -54,12 +54,17 @@ static int cpg_div6_clock_enable(struct clk_hw *hw)
>  static void cpg_div6_clock_disable(struct clk_hw *hw)
>  {
>  	struct div6_clock *clock = to_div6_clock(hw);
> +	u32 val;
> 
> -	/* DIV6 clocks require the divisor field to be non-zero when stopping
> +	val = clk_readl(clock->reg);
> +	val |= CPG_DIV6_CKSTP;
> +	/*
> +	 * DIV6 clocks require the divisor field to be non-zero when stopping
>  	 * the clock.
>  	 */
> -	clk_writel(clk_readl(clock->reg) | CPG_DIV6_CKSTP | CPG_DIV6_DIV_MASK,
> -		   clock->reg);
> +	if (!(val & CPG_DIV6_DIV_MASK))
> +		val |= CPG_DIV6_DIV_MASK;
> +	clk_writel(val, clock->reg);
>  }
> 
>  static int cpg_div6_clock_is_enabled(struct clk_hw *hw)

-- 
Regards,

Laurent Pinchart

Re: [PATCH] clk: shmobile: div6: Avoid changing divisor in .disable()

[Geert Uytterhoeven]

Hi Laurent,

On Wed, Nov 26, 2014 at 1:10 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Monday 24 November 2014 17:20:48 Geert Uytterhoeven wrote:
>> While DIV6 clocks require the divisor field to be non-zero when stopping
>> the clock, some clocks (e.g. ZB on sh73a0) fail to be re-enabled later
>> if the divisor field is changed when stopping the clock.
>>
>> To fix this, do not touch the divisor field if it's already non-zero.
>>
>> On kzm9g, the smsc911x Ethernet controller is connected to the sh73a0
>> Bus State Controller, which is clocked by the ZB clock. Without this
>> fix, if the ZB clock is disabled during system suspend, and re-enabled
>> during resume, the kernel locks up when the smsc911x driver tries to
>> access the Ethernet registers.
>
> Do we have any idea what happens at the hardware level ? Could it be that the
> divider set when stopping the clock has an invalid value ? Does the problem
> occur if you set the divider to a valid value instead of CPG_DIV6_DIV_MASK
> when stopping the clock ?

Yes, the problem also happened when using another value (I think I used 3 or 4,
while the default value is 2)  when stopping the clock, while it ran fine when
using the other divider value all the time (so the divider value was valid).

> The problem could also be caused by the new divider not being taken into
> account fast enough if modified during the same write cycle as the CKSTP bit
> when restarting the clock. Does the system still lock if you set the divider
> and clear the CKSTP bit in two seperate write operations when restarting the
> clock ?

I hadn't tried that. Let's see...

As "the divider must be non-zero when stopping the clock", I have to clear
the CKSTP bit first, and restore the divider in the second step, i.e.

#include <linux/clk-private.h>

        val = clk_readl(clock->reg);
        val &= ~CPG_DIV6_CKSTP;
        val2 = (val & ~CPG_DIV6_DIV_MASK) | CPG_DIV6_DIV(clock->div - 1);
        pr_info("DIV6 %s on: 0x%08x, 0x%08x", hw->clk->name, val, val2);
        clk_writel(val, clock->reg);
        pr_info("[delay]\n");
        clk_writel(val2, clock->reg);

This also works, but there must be a small delay in between the two writes.
If I remove the pr_info("[delay]\n"), it still fails about half of the time.
Sometimes even during bootup, when it enables the zb clock while it's already
running, and thus when writing the existing value (0x2) twice in a row.

Given we don't know the minimal required delay, I still prefer my solution
(until we find a DIV6 clock where the default divisor is zero, and it
fails again?).

What do you think?

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

Re: [PATCH] clk: shmobile: div6: Avoid changing divisor in .disable()

[Laurent Pinchart]

Hi Geert,

On Wednesday 26 November 2014 10:39:38 Geert Uytterhoeven wrote:
> On Wed, Nov 26, 2014 at 1:10 AM, Laurent Pinchart wrote:
> > On Monday 24 November 2014 17:20:48 Geert Uytterhoeven wrote:
> >> While DIV6 clocks require the divisor field to be non-zero when stopping
> >> the clock, some clocks (e.g. ZB on sh73a0) fail to be re-enabled later
> >> if the divisor field is changed when stopping the clock.
> >> 
> >> To fix this, do not touch the divisor field if it's already non-zero.
> >> 
> >> On kzm9g, the smsc911x Ethernet controller is connected to the sh73a0
> >> Bus State Controller, which is clocked by the ZB clock. Without this
> >> fix, if the ZB clock is disabled during system suspend, and re-enabled
> >> during resume, the kernel locks up when the smsc911x driver tries to
> >> access the Ethernet registers.
> > 
> > Do we have any idea what happens at the hardware level ? Could it be that
> > the divider set when stopping the clock has an invalid value ? Does the
> > problem occur if you set the divider to a valid value instead of
> > CPG_DIV6_DIV_MASK when stopping the clock ?
> 
> Yes, the problem also happened when using another value (I think I used 3 or
> 4, while the default value is 2)  when stopping the clock, while it ran
> fine when using the other divider value all the time (so the divider value
> was valid).

OK.

> > The problem could also be caused by the new divider not being taken into
> > account fast enough if modified during the same write cycle as the CKSTP
> > bit when restarting the clock. Does the system still lock if you set the
> > divider and clear the CKSTP bit in two seperate write operations when
> > restarting the clock ?
> 
> I hadn't tried that. Let's see...
> 
> As "the divider must be non-zero when stopping the clock", I have to clear
> the CKSTP bit first, and restore the divider in the second step, i.e.
> 
> #include <linux/clk-private.h>
> 
>         val = clk_readl(clock->reg);
>         val &= ~CPG_DIV6_CKSTP;
>         val2 = (val & ~CPG_DIV6_DIV_MASK) | CPG_DIV6_DIV(clock->div - 1);
>         pr_info("DIV6 %s on: 0x%08x, 0x%08x", hw->clk->name, val, val2);
>         clk_writel(val, clock->reg);
>         pr_info("[delay]\n");
>         clk_writel(val2, clock->reg);
> 
> This also works, but there must be a small delay in between the two writes.
> If I remove the pr_info("[delay]\n"), it still fails about half of the time.
> Sometimes even during bootup, when it enables the zb clock while it's
> already running, and thus when writing the existing value (0x2) twice in a
> row.
> 
> Given we don't know the minimal required delay, I still prefer my solution
> (until we find a DIV6 clock where the default divisor is zero, and it
> fails again?).
> 
> What do you think?

I'm fine with your fix as a (long-term) temporary solution. Could you please 
clearly state in the commit message (and possibly in a small comment in the 
code) that we don't know why the hardware fails without the patch ?

I think we should also try to understand the root cause, as I have an 
unpleasant feeling the problem will come back to bite us later. Time to light 
candles and invoke the Renesas hardware designer demon ? :-)

-- 
Regards,

Laurent Pinchart