[PATCH] ARM: mvebu: armada xp: Generalize use of i2c quirk

[PATCH] ARM: mvebu: armada xp: Generalize use of i2c quirk

[Andrew Lunn]

A second product has come to light which makes use of the A0 stepping
of the Armada XP SoC. A0 stepping has a hardware bug in the i2c core
meaning that hardware offload does not work, resulting in the kernel
failing to boot. The quirk detects that the kernel is running on an A0
stepping SoC and disables the use of hardware offload.

Currently the quirk is only enabled for PlatHome Openblocks AX3. The
AX3 has been produced with both A0 and B1 stepping SoCs. The second
product is the Lenovo Iomega IX4-300d. It seems likely that this
device will also swap from A0 to B1 SoC sometime during its life.

If there are two products using A0, it seems likely there are more
products with A0. Also, since the number of A0 SoCs is limited, these
products are also likely to transition to B1. Hence detecting at run
time is the safest option. So enable the quirk for all Armada XP
boards.

Tested on an AX3 with A0 stepping.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 arch/arm/mach-mvebu/board-v7.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c
index 8bb742fdf5ca..be080cff98d2 100644
--- a/arch/arm/mach-mvebu/board-v7.c
+++ b/arch/arm/mach-mvebu/board-v7.c
@@ -167,7 +167,7 @@ static void __init thermal_quirk(void)
 
 static void __init mvebu_dt_init(void)
 {
-	if (of_machine_is_compatible("plathome,openblocks-ax3-4"))
+	if (of_machine_is_compatible("marvell,armadaxp"))
 		i2c_quirk();
 	if (of_machine_is_compatible("marvell,a375-db"))
 		thermal_quirk();
-- 
2.0.1

[PATCH] ARM: mvebu: armada xp: Generalize use of i2c quirk

[Arnd Bergmann]

On Friday 25 July 2014 19:22:31 Andrew Lunn wrote:
> A second product has come to light which makes use of the A0 stepping
> of the Armada XP SoC. A0 stepping has a hardware bug in the i2c core
> meaning that hardware offload does not work, resulting in the kernel
> failing to boot. The quirk detects that the kernel is running on an A0
> stepping SoC and disables the use of hardware offload.
> 
> Currently the quirk is only enabled for PlatHome Openblocks AX3. The
> AX3 has been produced with both A0 and B1 stepping SoCs. The second
> product is the Lenovo Iomega IX4-300d. It seems likely that this
> device will also swap from A0 to B1 SoC sometime during its life.
> 
> If there are two products using A0, it seems likely there are more
> products with A0. Also, since the number of A0 SoCs is limited, these
> products are also likely to transition to B1. Hence detecting at run
> time is the safest option. So enable the quirk for all Armada XP
> boards.
> 
> Tested on an AX3 with A0 stepping.
> 

Based on the discussion on IRC, I think we can actually simplify the
existing quirk and avoid spreading it further. Below would be my
preferred approach.

8<------
[PATCH] ARM: mvebu: simplify openblocks ax3 quirk

We currently check the SoC revision in order to find out whether we
have an old or new openblocks ax3 machine and apply a quirk to
fix the compatible string for the old ones so the i2c driver does
not crash.

As it turns out, that is not necessary because the code for the a0
version also works on all following revisions, just slightly slower.
As only an RTC is connected to this bus and there are no performance
constraints in accessing that, we can easily apply that fixup
for all ax3 machines.

This also fixes the dtb file to contain the specific revision for a0,
so we can eventually run without the quirk. Other machines that may
be based on the a0 revision should do the same.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts b/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
index 4e5a59ee1501..2c6ac448b098 100644
--- a/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
+++ b/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
@@ -158,10 +158,16 @@
 				phy-mode = "sgmii";
 			};
 			i2c at 11000 {
+				compatible = "marvell,mv78230-a0-i2c",
+					     "marvell,mv78230-i2c",
+					     "marvell,mv64xxx-i2c";
 				status = "okay";
 				clock-frequency = <400000>;
 			};
 			i2c at 11100 {
+				compatible = "marvell,mv78230-a0-i2c",
+					     "marvell,mv78230-i2c",
+					     "marvell,mv64xxx-i2c";
 				status = "okay";
 				clock-frequency = <400000>;
 
diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c
index 6478626e3ff6..3f9cc8b58119 100644
--- a/arch/arm/mach-mvebu/board-v7.c
+++ b/arch/arm/mach-mvebu/board-v7.c
@@ -102,13 +102,13 @@ static void __init i2c_quirk(void)
 	u32 dev, rev;
 
 	/*
-	 * Only revisons more recent than A0 support the offload
-	 * mechanism. We can exit only if we are sure that we can
-	 * get the SoC revision and it is more recent than A0.
+	 * Some ax3 machines have an older SoC revision with a
+	 * slightly incompatible i2c controller. Since existing
+	 * device tree blobs may not reflect this correctly, let's
+	 * update the compatible string for that device.
+	 * New dtb files should list the a0 revision here, which
+	 * will work on all SoCs.
 	 */
-	if (mvebu_get_soc_id(&dev, &rev) == 0 && rev > MV78XX0_A0_REV)
-		return;
-
 	for_each_compatible_node(np, NULL, "marvell,mv78230-i2c") {
 		struct property *new_compat;
 

[PATCH] ARM: mvebu: armada xp: Generalize use of i2c quirk

[Andrew Lunn]

> Based on the discussion on IRC, I think we can actually simplify the
> existing quirk and avoid spreading it further. Below would be my
> preferred approach.
> 
> 8<------
> [PATCH] ARM: mvebu: simplify openblocks ax3 quirk
> 
> We currently check the SoC revision in order to find out whether we
> have an old or new openblocks ax3 machine and apply a quirk to
> fix the compatible string for the old ones so the i2c driver does
> not crash.
> 
> As it turns out, that is not necessary because the code for the a0
> version also works on all following revisions, just slightly slower.
> As only an RTC is connected to this bus and there are no performance
> constraints in accessing that, we can easily apply that fixup
> for all ax3 machines.
> 
> This also fixes the dtb file to contain the specific revision for a0,
> so we can eventually run without the quirk. Other machines that may
> be based on the a0 revision should do the same.

So lets discuss this.

What we already has works fine for AX3. Your version just simplifies
it some more, forcing all AX3 to never use hardware offloading, with
the aim to reduce maintenance burden in the long run. To remove the
quirk we either need to know the last A0 based AX3 has given out the
magic smoke, or we have a ABI break forcing all AX3 owners to upgrade
there DT blobs. The first we can never know, and we really don't want
the second, it is against the principles of DT.

Your patch does nothing for other products out in the wild with A0
SoCs. We now know there are some IX4-400d with A0. And Benoit Masson
had to figure out the i2c issue the hard way. Although it appears that
WRT1900AC is using B0, maybe there are some early devices with A0?
Can we ever know if there are?

By detecting this issue at run time, we avoid these issues. We avoid
some poor developer bringing up a board has to work out why their
kernel hangs during boot. We avoid some poor potential OpenWRT user
finding out that A0 WRT1900AC does exist, and their box no longer
boots after installing OpenWRT. And are the other products we don't
know about yet using A0?

Is the potential maintenance burden savings worth the pain we might be
causing others by not doing this at runtime?

	Andrew

[PATCH] ARM: mvebu: armada xp: Generalize use of i2c quirk

[Gregory CLEMENT]

On 25/07/2014 19:22, Andrew Lunn wrote:
> A second product has come to light which makes use of the A0 stepping
> of the Armada XP SoC. A0 stepping has a hardware bug in the i2c core
> meaning that hardware offload does not work, resulting in the kernel
> failing to boot. The quirk detects that the kernel is running on an A0
> stepping SoC and disables the use of hardware offload.
> 
> Currently the quirk is only enabled for PlatHome Openblocks AX3. The
> AX3 has been produced with both A0 and B1 stepping SoCs. The second
> product is the Lenovo Iomega IX4-300d. It seems likely that this
> device will also swap from A0 to B1 SoC sometime during its life.
> 
> If there are two products using A0, it seems likely there are more
> products with A0. Also, since the number of A0 SoCs is limited, these
> products are also likely to transition to B1. Hence detecting at run
> time is the safest option. So enable the quirk for all Armada XP
> boards.
> 
> Tested on an AX3 with A0 stepping.


Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>


Thanks,

Gregory


> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  arch/arm/mach-mvebu/board-v7.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c
> index 8bb742fdf5ca..be080cff98d2 100644
> --- a/arch/arm/mach-mvebu/board-v7.c
> +++ b/arch/arm/mach-mvebu/board-v7.c
> @@ -167,7 +167,7 @@ static void __init thermal_quirk(void)
>  
>  static void __init mvebu_dt_init(void)
>  {
> -	if (of_machine_is_compatible("plathome,openblocks-ax3-4"))
> +	if (of_machine_is_compatible("marvell,armadaxp"))
>  		i2c_quirk();
>  	if (of_machine_is_compatible("marvell,a375-db"))
>  		thermal_quirk();
> 


-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH] ARM: mvebu: armada xp: Generalize use of i2c quirk

[Thomas Petazzoni]

Dear Andrew Lunn,

On Fri, 25 Jul 2014 19:22:31 +0200, Andrew Lunn wrote:
> A second product has come to light which makes use of the A0 stepping
> of the Armada XP SoC. A0 stepping has a hardware bug in the i2c core
> meaning that hardware offload does not work, resulting in the kernel
> failing to boot. The quirk detects that the kernel is running on an A0
> stepping SoC and disables the use of hardware offload.
> 
> Currently the quirk is only enabled for PlatHome Openblocks AX3. The
> AX3 has been produced with both A0 and B1 stepping SoCs. The second
> product is the Lenovo Iomega IX4-300d. It seems likely that this
> device will also swap from A0 to B1 SoC sometime during its life.
> 
> If there are two products using A0, it seems likely there are more
> products with A0. Also, since the number of A0 SoCs is limited, these
> products are also likely to transition to B1. Hence detecting at run
> time is the safest option. So enable the quirk for all Armada XP
> boards.
> 
> Tested on an AX3 with A0 stepping.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  arch/arm/mach-mvebu/board-v7.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Arnd's solution doesn't work, because it doesn't fix at all the problem
for the new Lenovo platform or any other new platform.

Only minor nit: s/B1/B0/ in the commit log.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH] ARM: mvebu: armada xp: Generalize use of i2c quirk

[Arnd Bergmann]

On Friday 25 July 2014 21:03:22 Andrew Lunn wrote:
> > Based on the discussion on IRC, I think we can actually simplify the
> > existing quirk and avoid spreading it further. Below would be my
> > preferred approach.
> > 
> > 8<------
> > [PATCH] ARM: mvebu: simplify openblocks ax3 quirk
> > 
> > We currently check the SoC revision in order to find out whether we
> > have an old or new openblocks ax3 machine and apply a quirk to
> > fix the compatible string for the old ones so the i2c driver does
> > not crash.
> > 
> > As it turns out, that is not necessary because the code for the a0
> > version also works on all following revisions, just slightly slower.
> > As only an RTC is connected to this bus and there are no performance
> > constraints in accessing that, we can easily apply that fixup
> > for all ax3 machines.
> > 
> > This also fixes the dtb file to contain the specific revision for a0,
> > so we can eventually run without the quirk. Other machines that may
> > be based on the a0 revision should do the same.
> 
> So lets discuss this.
> 
> What we already has works fine for AX3. Your version just simplifies
> it some more, forcing all AX3 to never use hardware offloading, with
> the aim to reduce maintenance burden in the long run. To remove the
> quirk we either need to know the last A0 based AX3 has given out the
> magic smoke, or we have a ABI break forcing all AX3 owners to upgrade
> there DT blobs. The first we can never know, and we really don't want
> the second, it is against the principles of DT.

Right.

> Your patch does nothing for other products out in the wild with A0
> SoCs. We now know there are some IX4-400d with A0. And Benoit Masson
> had to figure out the i2c issue the hard way. Although it appears that
> WRT1900AC is using B0, maybe there are some early devices with A0?
> Can we ever know if there are?

The safe answer would be to make WRT1900AC and others that we suspect
may have A0 also claim compatibility with that. What we do know is
that neither IX4 nor WRT1900AC actually benefit from the fixed
revision, since they have only low-speed devices on their I2C
and can used polled mode.

> By detecting this issue at run time, we avoid these issues. We avoid
> some poor developer bringing up a board has to work out why their
> kernel hangs during boot. We avoid some poor potential OpenWRT user
> finding out that A0 WRT1900AC does exist, and their box no longer
> boots after installing OpenWRT. And are the other products we don't
> know about yet using A0?
> 
> Is the potential maintenance burden savings worth the pain we might be
> causing others by not doing this at runtime?

It seems everyone besides me is in favor of doing the runtime check,
so consider me overrule here. We might want to reflect that in the
binding though and no longer mention the a0 revision, since that is
not reliable any more with the new interpretation. Any operating
system will have to figure out for itself whether it can safely
use the offload mode on the i2c or not if we accept dtbs for
arbitrary a0 revision devices that list the compatible string for
the fixed chip.

	Arnd


diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
index 5c30026921ae..6eb6f6e40ba1 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
@@ -9,11 +9,6 @@ Required properties :
                      - "allwinner,sun6i-a31-i2c"
                      - "marvell,mv64xxx-i2c"
                      - "marvell,mv78230-i2c"
-                     - "marvell,mv78230-a0-i2c"
-                       * Note: Only use "marvell,mv78230-a0-i2c" for a
-                         very rare, initial version of the SoC which
-                         had broken offload support.  Linux
-                         auto-detects this and sets it appropriately.
  - interrupts      : The interrupt number
 
 Optional properties :