[PATCH] ARM: Kirkwood: Fix crash when neither NAND nor SPI

[PATCH] ARM: Kirkwood: Fix crash when neither NAND nor SPI

[Andrew Lunn]

Both NAND and SPI make use of the RUNIT clk. However, if neither NAND
nor SPI is used in the system, RUNIT clock gets turned off, and the
SoC hard locks. It appears something else in the SoC, which is not
documented, is also using RUNIT. So prepare and enable RUNIT clock in
kirkwood_clk_init().

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 arch/arm/mach-kirkwood/common.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/mach-kirkwood/common.c b/arch/arm/mach-kirkwood/common.c
index 25fb3fd..97f7d36 100644
--- a/arch/arm/mach-kirkwood/common.c
+++ b/arch/arm/mach-kirkwood/common.c
@@ -238,6 +238,10 @@ void __init kirkwood_clk_init(void)
 	orion_clkdev_add("0", "pcie", pex0);
 	orion_clkdev_add("1", "pcie", pex1);
 	orion_clkdev_add(NULL, "kirkwood-i2s", audio);
+
+	/* Something other than SPI and NAND needs runit, so make sure
+	   it never gets turned off. */
+	clk_prepare_enable(runit);
 }
 
 /*****************************************************************************
-- 
1.7.10

[PATCH] ARM: Kirkwood: Fix crash when neither NAND nor SPI

[Arnd Bergmann]

On Friday 08 June 2012, Andrew Lunn wrote:
> Both NAND and SPI make use of the RUNIT clk. However, if neither NAND
> nor SPI is used in the system, RUNIT clock gets turned off, and the
> SoC hard locks. It appears something else in the SoC, which is not
> documented, is also using RUNIT. So prepare and enable RUNIT clock in
> kirkwood_clk_init().
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>

This looks bogus, because if you really need to enable that clock all
the time, we don't even need a way to turn it off.

Do you need a similar change with the 3.4 kernel before the big
change to the clock framework? If not, I would guess that there
is some other problem.

	Arnd

[PATCH] ARM: Kirkwood: Fix crash when neither NAND nor SPI

[Andrew Lunn]

On Fri, Jun 08, 2012 at 03:34:13PM +0000, Arnd Bergmann wrote:
> On Friday 08 June 2012, Andrew Lunn wrote:
> > Both NAND and SPI make use of the RUNIT clk. However, if neither NAND
> > nor SPI is used in the system, RUNIT clock gets turned off, and the
> > SoC hard locks. It appears something else in the SoC, which is not
> > documented, is also using RUNIT. So prepare and enable RUNIT clock in
> > kirkwood_clk_init().
> > 
> > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> 
> This looks bogus, because if you really need to enable that clock all
> the time, we don't even need a way to turn it off.

This is a generic gated clock, so the ability to turn it off comes
from the framework. The framework also turns off all clk which are
unused in a late init, unless flagged otherwise.

> Do you need a similar change with the 3.4 kernel before the big
> change to the clock framework? If not, I would guess that there
> is some other problem.

3.4 did not have code to ensure it was not turned off. 

However, all current systems have either SPI FLASH or NAND, since that
is how these devices boot. So, under normal conditions, this clock is
always used, and so never gets turned off as being unused. However,
while porting boards to DT, a few of us have hit situations where we
get the DT wrong, the spi/nand does not match, and the clock is unused
and so turned off. This is particularly true as we implement DT
support in the SPI and NAND drivers. This is not so easy to debug,
since it just stops, rather than panic when the root file system is
missing.

I also tried to save a little bit of power when the SPI device was
inactive, turning the clock off between transactions. This resulted in
the same lockup.

By preparing and enabling the clock, we reflect that there is probably
something else inside RUNIT, and it makes it easier to debug DT
bugs. The two alternatives would be to set the flag CLK_IGNORE_UNUSED,
or just leave it the way it is, now that DT SPI and NAND support is
mostly developed, and we are less likely to hit this situation.

       Andrew

[PATCH] ARM: Kirkwood: Fix crash when neither NAND nor SPI

[Jon Medhurst (Tixy)]

On Fri, 2012-06-08 at 18:42 +0200, Andrew Lunn wrote:
> On Fri, Jun 08, 2012 at 03:34:13PM +0000, Arnd Bergmann wrote:
> > On Friday 08 June 2012, Andrew Lunn wrote:
> > > Both NAND and SPI make use of the RUNIT clk. However, if neither NAND
> > > nor SPI is used in the system, RUNIT clock gets turned off, and the
> > > SoC hard locks. It appears something else in the SoC, which is not
> > > documented, is also using RUNIT. So prepare and enable RUNIT clock in
> > > kirkwood_clk_init().
> > > 
> > > Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> > 
> > This looks bogus, because if you really need to enable that clock all
> > the time, we don't even need a way to turn it off.
> 
> This is a generic gated clock, so the ability to turn it off comes
> from the framework. The framework also turns off all clk which are
> unused in a late init, unless flagged otherwise.
> 
> > Do you need a similar change with the 3.4 kernel before the big
> > change to the clock framework? If not, I would guess that there
> > is some other problem.
> 
> 3.4 did not have code to ensure it was not turned off. 
> 
> However, all current systems have either SPI FLASH or NAND, since that
> is how these devices boot. So, under normal conditions, this clock is
> always used, and so never gets turned off as being unused.

What if your filesystems are on devices other than flash (I use an SD
card on my Sheevaplugs). In that case flash isn't used at all by the
Linux kernel and could be safely turned off. I'm not saying that this is
worth the hassle, just pointing out that flash may be unused by Linux on
some systems.

-- 
Tixy

[PATCH] ARM: Kirkwood: Fix crash when neither NAND nor SPI

[Andrew Lunn]

> > However, all current systems have either SPI FLASH or NAND, since that
> > is how these devices boot. So, under normal conditions, this clock is
> > always used, and so never gets turned off as being unused.
> 
> What if your filesystems are on devices other than flash (I use an SD
> card on my Sheevaplugs). In that case flash isn't used at all by the
> Linux kernel and could be safely turned off. I'm not saying that this is
> worth the hassle, just pointing out that flash may be unused by Linux on
> some systems.

The current sheevaplug code sets up the nand driver and mtd
partitions. That is sufficient to ensure the clock is kept on. It does
not matter if a partitions are used or not.

I in fact use a similar system to you. I typically use kirkwood
devices which are NAS boxes and i have the root file system on the
disk and i tftpboot the kernel. The SPI flash is unused, but its still
made available via mtd.

Problems will however come when somebody creates a box which really
does boot from disk and skips NAND and SPI-FLASH in the DT description
of the device. That is why i like claiming this clock in the core
code, just to say there is something in the CPU core using it. 

     Andrew

[PATCH] ARM: Kirkwood: Fix crash when neither NAND nor SPI

[Simon Baatz]

Hi Andrew,

Am 08.06.2012 16:09, schrieb Andrew Lunn:
> Both NAND and SPI make use of the RUNIT clk. However, if neither NAND
> nor SPI is used in the system, RUNIT clock gets turned off, and the
> SoC hard locks. It appears something else in the SoC, which is not
> documented, is also using RUNIT. So prepare and enable RUNIT clock in
> kirkwood_clk_init().
>
> +
> +	/* Something other than SPI and NAND needs runit, so make sure
> +	   it never gets turned off. */
> +	clk_prepare_enable(runit);
>

I am experiencing another type of crash caused by the GE0 clock (on a
IB-NAS 6210). I am compiling mv643xx_eth as a module. Thus, GE0 and GE1
get turned off first since they are unused.

When the module loads and it should enable GE0, the box hangs. I haven't
had time to find out why; my guess would be that the clock might already
be needed somewhere in mv643xx_eth_shared_probe(). The box boots again
after I add

clk_prepare_enable(ge0);

to the code above as a quick hack.

- Simon

[PATCH] ARM: Kirkwood: Fix crash when neither NAND nor SPI

[Andrew Lunn]

On Sun, Jun 10, 2012 at 08:09:34PM +0200, Simon Baatz wrote:
> Hi Andrew,
> 
> Am 08.06.2012 16:09, schrieb Andrew Lunn:
> > Both NAND and SPI make use of the RUNIT clk. However, if neither NAND
> > nor SPI is used in the system, RUNIT clock gets turned off, and the
> > SoC hard locks. It appears something else in the SoC, which is not
> > documented, is also using RUNIT. So prepare and enable RUNIT clock in
> > kirkwood_clk_init().
> >
> > +
> > +	/* Something other than SPI and NAND needs runit, so make sure
> > +	   it never gets turned off. */
> > +	clk_prepare_enable(runit);
> >
> 
> I am experiencing another type of crash caused by the GE0 clock (on a
> IB-NAS 6210). I am compiling mv643xx_eth as a module. Thus, GE0 and GE1
> get turned off first since they are unused.

O.K. never tried that. I only tested with kirkwood_defconfig.

> When the module loads and it should enable GE0, the box hangs. I haven't
> had time to find out why; my guess would be that the clock might already
> be needed somewhere in mv643xx_eth_shared_probe(). The box boots again
> after I add

It is probably infer_hw_params(). 

> 
> clk_prepare_enable(ge0);
> 
> to the code above as a quick hack.

The correct fix is going to be interesting. I assume that either ge0
or ge1 is sufficient to make the shared hardware work. So one option
would be to register a clock with name MV643XX_ETH_SHARED_NAME, which
is ge0. The shared probe code can turn it on/off during the shared
probe function.  When the specific interface is probed, the correct
clock for that interface will be enabled, and it will also driver the
shared hardware.

I will try this out tomorrow.

Do you have any other drivers built as modules? I was conservative and
turned the clock on in the probe function, so i expect that most work
as modules. However XOR has a similar sort of structure, so might also
be broken...

	Thanks
		Andrew

[PATCH] ARM: Kirkwood: Fix crash when neither NAND nor SPI

[Simon Baatz]

On Sun, Jun 10, 2012 at 10:13:10PM +0200, Andrew Lunn wrote:
> On Sun, Jun 10, 2012 at 08:09:34PM +0200, Simon Baatz wrote:
...
> Do you have any other drivers built as modules? I was conservative and
> turned the clock on in the probe function, so i expect that most work
> as modules. However XOR has a similar sort of structure, so might also
> be broken...

I don't use XOR as module. However, in addition to the Ethernet
one, I have the following modules that enable clocks:

mv_cesa
sata_mv 
ehci_hcd

I did not encounter any problem with these so far (with the exception
of data corruption in mv_cesa, which is completely unrelated (and is
no problem of mv_cesa, but a problem of the current implementation of
flush_kernel_dcache_page in my view)).

- Simon

[PATCH] ARM: Kirkwood: Fix crash when neither NAND nor SPI

[Andrew Lunn]

On Sun, Jun 10, 2012 at 08:09:34PM +0200, Simon Baatz wrote:
> Hi Andrew,
> 
> Am 08.06.2012 16:09, schrieb Andrew Lunn:
> > Both NAND and SPI make use of the RUNIT clk. However, if neither NAND
> > nor SPI is used in the system, RUNIT clock gets turned off, and the
> > SoC hard locks. It appears something else in the SoC, which is not
> > documented, is also using RUNIT. So prepare and enable RUNIT clock in
> > kirkwood_clk_init().
> >
> > +
> > +	/* Something other than SPI and NAND needs runit, so make sure
> > +	   it never gets turned off. */
> > +	clk_prepare_enable(runit);
> >
> 
> I am experiencing another type of crash caused by the GE0 clock (on a
> IB-NAS 6210). I am compiling mv643xx_eth as a module. Thus, GE0 and GE1
> get turned off first since they are unused.
> 
> When the module loads and it should enable GE0, the box hangs.

I debugged this further. As i guessed, it is interesting, but not how
i expected.

By enabling a clock while performing shared_probe, i stopped it
locking up. I also needed to add a short delay in the individual
interface probe, otherwise it locked up there.

However, this leads to another problem. It appears to forget its MAC
address, as set by u-boot, while the clock is off :-(

I'm not sure how to handle this. One option is simply to not allow the
two clocks to be turned off. The module can then be loaded/unloaded at
any time, and the MAC address set by u-boot will still be available.

Or i could enable_prepare() the clock in kirkwood_ge00_init() and
kirkwood_ge01_init() when the platform device structure is
created. These functions indicate at some time in the future, the
driver might be loaded to driver one or the other interface, so
keeping the clk ticking to preserve the MAC address is a good idea.
This will however not work for DT boards, since they won't in the
future call these functions. We would need additional code to look
into the DT during boot, see if there are nodes for the ethernet
interfaces, and if so, enable_prepare() the clocks.

I'm tempted to go with this last solution, since it is more in keeping
with the way it was done before i replaced the code with the generic
clock framework.

      Andrew