Generalize use of i2c quirk for Armada XP

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 B0 stepping SoCs. The second product is the Lenovo Iomega IX4-300d. It seems likely that this device will also swap from A0 to B0 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 B0. 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 <[email protected]>

Acked-by Gregory CLEMENT <[email protected]>

Acked-by Thomas Petazzoni <[email protected]>

---

V2:
Added two Acked-by:
s/B1/B0

---
 arch/arm/mach-mvebu/board-v7.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

The compatible string "marvell,mv78230-a0-i2c" should only be used internally when at run time we detect an Armada XP with broken hardware offload. This string should never appear in DT blobs passed to the kernel. Hence remove it from the binding Documentation.

Patch proposed by Arnd Bergmann <[email protected]>

Signed-off-by Andrew Lunn <[email protected]>

---
 Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 5 -----
 1 file changed, 5 deletions(-)

Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +0 -0

index 5c30026921ae..6eb6f6e40ba1 100644

Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt Required properties : · line 16

Quoted 13 lines

Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +0 -5

index 5c30026921ae..6eb6f6e40ba1 100644
@@ -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.

I think we're losing knowledge here. *We* know that attempting to enable transaction offload on A0 SoCs is bad news. Other OS's would now need to dig through the Linux kernel code for clues as to what's happening.

Perhaps we should retain the info in the form of a note at the bottom of this file?

Quoted 19 lines

Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt Required properties : · line 16

Quoted 13 lines

Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +0 -5

index 5c30026921ae..6eb6f6e40ba1 100644
@@ -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.

I think we're losing knowledge here. *We* know that attempting to enable transaction offload on A0 SoCs is bad news. Other OS's would now need to dig through the Linux kernel code for clues as to what's happening.

Quoted 2 lines

Perhaps we should retain the info in the form of a note at the bottom of this file?

Hi Jason

I did wounder about this a bit. I've not looked, but now that XP datasheets are public, i assume there is an errata for this, so it at least should be documented by Marvell. But anybody looking in /proc/device-tree on an A0 is going to see this undocumented string which might raise questions.

So i'm happy to document it at the end of the binding.

Arnd, what do you say?

Quoted 30 lines · On Monday 28 July 2014 15:27:16 Andrew Lunn wrote:

Quoted 19 lines

Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt Required properties : · line 16

Quoted 13 lines

Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +0 -5

index 5c30026921ae..6eb6f6e40ba1 100644
@@ -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.

I think we're losing knowledge here. *We* know that attempting to enable transaction offload on A0 SoCs is bad news. Other OS's would now need to dig through the Linux kernel code for clues as to what's happening.

Quoted 2 lines

Perhaps we should retain the info in the form of a note at the bottom of this file?

Hi Jason

I did wounder about this a bit. I've not looked, but now that XP datasheets are public, i assume there is an errata for this, so it at least should be documented by Marvell. But anybody looking in /proc/device-tree on an A0 is going to see this undocumented string which might raise questions.

Quoted 3 lines

So i'm happy to document it at the end of the binding.

Arnd, what do you say?

The final consequence of the API change would be to no longer change the compatible string in the fixup, but instead to call an API from the driver to find out the SoC revision when it encounters an mv78230-i2c.

I remember this being discussed when the quirk was initially added, but it seemed cleaner to handle this in the platform code at the time when it was just for one particular board. Now that it's basically an accepted feature of the i2c device that you have to know the SoC version, that should probably become a proper API.

Also, we now have drivers/soc/ and can move the soc-id code there with a publically documented API.

Quoted 9 lines

I remember this being discussed when the quirk was initially added, but it seemed cleaner to handle this in the platform code at the time when it was just for one particular board. Now that it's basically an accepted feature of the i2c device that you have to know the SoC version, that should probably become a proper API.

Also, we now have drivers/soc/ and can move the soc-id code there with a publically documented API.

Getting the SoC ID and revision seems like something that should be generic. Would it be better to make this part of drivers/base/soc.c? mvebu already does a soc_device_register() with the relevant information.

Add a call something like:

/*
* Return the soc device attributes for a given soc_dev. If soc_dev is NULL,
* the first device on the soc bus is returned.
*/
struct soc_device_attribute *soc_attribute_get(struct soc_device * soc_dev);

Quoted 7 lines

Add a call something like:

/*
* Return the soc device attributes for a given soc_dev. If soc_dev is NULL,
* the first device on the soc bus is returned.
*/
struct soc_device_attribute *soc_attribute_get(struct soc_device * soc_dev);

Interesting idea, yes.

There could also be a higher-level function that does a strcmp() in addition, so that a driver can do some variation of

	if (soc_name_is("Armada XP") && soc_revision_is("A0"))

Quoted 9 lines

Quoted 7 lines

Add a call something like:

/*
* Return the soc device attributes for a given soc_dev. If soc_dev is NULL,
* the first device on the soc bus is returned.
*/
struct soc_device_attribute *soc_attribute_get(struct soc_device * soc_dev);

Interesting idea, yes.

O.K, i will take a shot of implementing this sometime in the next few weeks. Bit busy at the moment.

Quoted 3 lines

There could also be a higher-level function that does a strcmp() in addition, so that a driver can do some variation of

Quoted 1 line

      if (soc_name_is("Armada XP") && soc_revision_is("A0"))

We don't translate into 'Marketing names'. We just turn the register values into a stringified number. So it would have to be something like:

        if (soc_name_is("7826") && soc_revision_is("1"))

Quoted 5 lines

We don't translate into 'Marketing names'. We just turn the register values into a stringified number. So it would have to be something like:

        if (soc_name_is("7826") && soc_revision_is("1"))

Hmm, I think that is inconsistent with how the strings are used on other platforms, where you want something that uniquely identifies the device in user-readable strings.

Quoted 9 lines

Quoted 5 lines

We don't translate into 'Marketing names'. We just turn the register values into a stringified number. So it would have to be something like:

        if (soc_name_is("7826") && soc_revision_is("1"))

Hmm, I think that is inconsistent with how the strings are used on other platforms, where you want something that uniquely identifies the device in user-readable strings.

Unfortunately, it is not so simple. There are a few different Armada XP SoCs. E.g. dual core and quad core. And this is pretty normal for Marvell. There are maybe 5 different Kirkwoods, a few different Orion5x etc. So at minimum it would of had to be e.g. "Armada XP 7826".

However, it is too late now. This is part of the ABI, and Debian's flash-kernel is making use of this in order to get the right DT blob. So we cannot change it.

I remember this being discussed when the quirk was initially added, but it seemed cleaner to handle this in the platform code at the time when it was just for one particular board. Now that it's basically an accepted feature of the i2c device that you have to know the SoC version, that should probably become a proper API.

Quoted 5 lines · Arnd Bergmann

I remember this being discussed when the quirk was initially added, but it seemed cleaner to handle this in the platform code at the time when it was just for one particular board. Now that it's basically an accepted feature of the i2c device that you have to know the SoC version, that should probably become a proper API.

Reading the SOC ID (which has to come from a PCI-E controller) is already known to mess up PCI-E. IMHO, reading it should be minimized, unless we can figure out how to fix that.

At some point we need to push back on the bootloader situation and require the kernel to be booted with a correct DT.

All the infrastructure was already provided for this, I'm not sure how we got into the situation where it was OK for bootloaders to ignore the DT requirements and it is the kernel's job to fix it

Quoted 30 lines · On Monday 28 July 2014 15:27:16 Andrew Lunn wrote:

Quoted 19 lines

Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt Required properties : · line 16

Quoted 13 lines

Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt +0 -5

index 5c30026921ae..6eb6f6e40ba1 100644
@@ -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.

I think we're losing knowledge here. *We* know that attempting to enable transaction offload on A0 SoCs is bad news. Other OS's would now need to dig through the Linux kernel code for clues as to what's happening.

Quoted 2 lines

Perhaps we should retain the info in the form of a note at the bottom of this file?

Hi Jason

I did wounder about this a bit. I've not looked, but now that XP datasheets are public, i assume there is an errata for this, so it at least should be documented by Marvell. But anybody looking in /proc/device-tree on an A0 is going to see this undocumented string which might raise questions.

Quoted 3 lines

So i'm happy to document it at the end of the binding.

Arnd, what do you say?

The final consequence of the API change would be to no longer change the compatible string in the fixup, but instead to call an API from the driver to find out the SoC revision when it encounters an mv78230-i2c.

I remember this being discussed when the quirk was initially added, but it seemed cleaner to handle this in the platform code at the time when it was just for one particular board. Now that it's basically an accepted feature of the i2c device that you have to know the SoC version, that should probably become a proper API.

Also, we now have drivers/soc/ and can move the soc-id code there with a publically documented API.

I agree it's sad to lose the knowledge about the revision, but we already have that as we now accept the generic string even on the a0 machines other than ax3.