ARM: mvebu: armada xp: Generalize use of i2c quirk
Subsystems (5)
Andrew Lunn
maintainerPATCH 1/1 ARM: mvebu: armada xp: Generalize use of i2c quirk
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.
--- arch/arm/mach-mvebu/board-v7.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/mach-mvebu/board-v7.c +1 -1
@@ -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 ();
Signature · 1 line
2.0.1
PATCH 1/1 ARM: mvebu: armada xp: Generalize use of i2c quirk
Quoted 11 lines · 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.
Quoted 8 lines
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.
arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts +6 -0
@@ -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>;
arch/arm/mach-mvebu/board-v7.c +6 -6
@@ -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;
Andrew Lunn
maintainerPATCH 1/1 ARM: mvebu: armada xp: Generalize use of i2c quirk
Quoted 18 lines
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 quirkWe 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.
Quoted 3 lines
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 1/1 ARM: mvebu: armada xp: Generalize use of i2c quirk
Quoted 24 lines · On Friday 25 July 2014 21:03:22 Andrew Lunn wrote:
Quoted 18 lines
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 quirkWe 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.
Quoted 3 lines
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.
Quoted 7 lines
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.
Quoted 5 lines
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.
Quoted 9 lines
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
Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +0 -5
@@ -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 :
PATCH 1/1 ARM: mvebu: armada xp: Generalize use of i2c quirk
In reply to Andrew LunnQuoted 11 lines · 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.
Quoted 7 lines
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.
Thanks,
Gregory
Quoted 20 lines
Signed-off-by Andrew Lunn <[email protected]>--- arch/arm/mach-mvebu/board-v7.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/arm/mach-mvebu/board-v7.c +1
-1@@ -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 ();
Signature · 4 lines
Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com
PATCH 1/1 ARM: mvebu: armada xp: Generalize use of i2c quirk
In reply to Andrew LunnDear Andrew Lunn,
Quoted 17 lines · 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.
Quoted 6 lines
Tested on an AX3 with A0 stepping.
Signed-off-by Andrew Lunn <[email protected]>--- arch/arm/mach-mvebu/board-v7.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
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
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com