Fix i2c bus hang on A0 version of the Armada XP SoCs

PATCH Superseded Gregory CLEMENT 5 participants 20 messages Jan 7, 2014 – Jan 9, 2014

Subsystems (5)

Arm (32-bit) Arm sub-architectures I2C subsystem host drivers Arm/Marvell Kirkwood and Armada 370, 375, 38x, 39x, XP, 3700, 7K/8K, CN9130 SoC support +1 more subsystems

lore

Share & tools

lore

Gregory CLEMENT

author

2014-01-07 16:35 UTC #0 lore raw

v4 cover Fix i2c bus hang on A0 version of the Armada XP SoCs

Hi,

Eventually the 3rd version was not the last of the series fixing the i2c bus hang on A0 version of the Armada XP SoCs. It occurred on the early release of the OpenBlocks AX3-4 boards. Indeed the first variants of Armada XP SoCs (A0 stepping) have issues related to the i2c controller which prevent to use the offload mechanism and lead to a kernel hang during boot.

I have also just realized that I didn't put i2c list and maintainer with my 3rd version, it was a mistake and not done in purpose.

On Arnd request I moved the arch specific code outside the i2c driver. This is now handle by the patch 2 and it create the appropriate flag if needed.

The first patch add a mean to detect the SoCs version at run-time and the second one use this feature in the driver.

Wolfram, I removed your acked-by on the 3rd patch as the code was changed, could you check that you agree with it and it is the case give your acked-by ?

These 3 patches should be applied on 3.13-rc and on stable kernel 3.12 as it fixes a regression introduce by the commit 930ab3d403ae "i2c:
mv64xxx: Add I2C Transaction Generator support".

The first patch could be latter be extend to also be used with dove, kirkwood, orion5x and mv78x00 when there will be merged in mvebu.

Thanks,

Gregory

Changelog:
v3 -> v4:

- checked the offload-broken property instead of calling the mvebu_get_soc_id() function in the mv64xxx_of_config() function.

- added the second patch to manage the quirk and update the device node with the offload-broken if needed.

- removed the acked-by from Wolfram as the code have change in the 3rd patch

In mvebu-soc-id.c:
- used EXPORT_SYMBOL_GPL instead of EXPORT_SYMBOL

- used core_initcall instead of arch_initcall to be called earlier enough.

v2 -> v3:

- fixed typo in the comments added in i2c-mv64xxx.c

- used pr_fmt instead of %s __func__ in all the pr_* functions

- added a check on the pointer returned by of_get_next_child()

- added a return immediately after the 1st check to be able to get rid of indenting the entire function code inside the if { ... } block.

v1 -> v2:

- Changed the way to test the return of the function mvebu_get_soc_id in order to make it clearer.

- Removed the superfluous parentheses

- Added Wolfram's acked-by on the 2nd patch

Gregory CLEMENT (3):
ARM: mvebu: Add support to get the ID and the revision of a SoC
ARM: mvebu: Add quirk for i2c
i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs

 arch/arm/mach-mvebu/Makefile        |   2 +-
 arch/arm/mach-mvebu/armada-370-xp.c |  24 ++++++++
 arch/arm/mach-mvebu/mvebu-soc-id.c  | 120 ++++++++++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-mv64xxx.c    |   4 +-
 include/linux/mvebu-soc-id.h        |  32 ++++++++++
 5 files changed, 180 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm/mach-mvebu/mvebu-soc-id.c
 create mode 100644 include/linux/mvebu-soc-id.h

Signature · 1 line

1.8.1.2

Gregory CLEMENT

author

2014-01-07 16:35 UTC #1 lore raw

PATCH v4 1/3 ARM: mvebu: Add support to get the ID and the revision of a SoC

All the mvebu SoCs have information related to their variant and revision that can be read from the PCI control register.

This patch adds support for Armada XP and Armada 370. This reading of the revision and the ID are done before the PCI initialization to avoid any conflicts. Once these data are retrieved, the resources are freed to let the PCI subsystem use it.

Cc stable at vger.kernel.org

Signed-off-by Gregory CLEMENT <[email protected]>

---
 arch/arm/mach-mvebu/Makefile       |   2 +-
 arch/arm/mach-mvebu/mvebu-soc-id.c | 120 +++++++++++++++++++++++++++++++++++++
 include/linux/mvebu-soc-id.h       |  32 ++++++++++
 3 files changed, 153 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/mach-mvebu/mvebu-soc-id.c
 create mode 100644 include/linux/mvebu-soc-id.h

arch/arm/mach-mvebu/Makefile +1 -1

index 2d04f0e21870..878aebe98dcc 100644
@@ -3,7 +3,7 @@ ccflags-$(CONFIG_ARCH_MULTIPLATFORM) := -I$(srctree)/$(src)/include \
 
 AFLAGS_coherency_ll.o		:= -Wa,-march=armv7-a
 
-obj-y				 += system-controller.o
+obj-y				 += system-controller.o mvebu-soc-id.o
 obj-$(CONFIG_MACH_ARMADA_370_XP) += armada-370-xp.o
 obj-$(CONFIG_ARCH_MVEBU)	 += coherency.o coherency_ll.o pmsu.o
 obj-$(CONFIG_SMP)                += platsmp.o headsmp.o

arch/arm/mach-mvebu/mvebu-soc-id.c new file +120 -0

120 lines not on this page · raw message

include/linux/mvebu-soc-id.h new file +32 -0

new file mode 100644
index 000000000000..31654252fe35
@@ -0,0 +1,32 @@
+/*
+ * Marvell EBU SoC ID and revision definitions.
+ *
+ * Copyright (C) 2014 Marvell Semiconductor
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef __LINUX_MVEBU_SOC_ID_H
+#define __LINUX_MVEBU_SOC_ID_H
+
+/* Armada XP ID */
+#define MV78230_DEV_ID	    0x7823
+#define MV78260_DEV_ID	    0x7826
+#define MV78460_DEV_ID	    0x7846
+
+/* Armada XP Revision */
+#define MV78XX0_A0_REV	    0x1
+#define MV78XX0_B0_REV	    0x2
+
+#ifdef CONFIG_ARCH_MVEBU
+int mvebu_get_soc_id(u32 *dev, u32 *rev);
+#else
+static inline int mvebu_get_soc_id(u32 *dev, u32 *rev)
+{
+	return -1;
+}
+#endif
+
+#endif /* __LINUX_MVEBU_SOC_ID_H */

Signature · 1 line

1.8.1.2

Arnd Bergmann

2014-01-08 12:55 UTC #2 lore raw

PATCH v4 1/3 ARM: mvebu: Add support to get the ID and the revision of a SoC

Quoted 5 lines · Gregory CLEMENT

include/linux/mvebu-soc-id.h new file +0 -0
new file mode 100644
index 000000000000..31654252fe35

include/linux/mvebu-soc-id.h line 32

Quoted 10 lines

include/linux/mvebu-soc-id.h new file +10 -0

+#ifdef CONFIG_ARCH_MVEBU
+int mvebu_get_soc_id(u32 *dev, u32 *rev);
+#else
+static inline int mvebu_get_soc_id(u32 *dev, u32 *rev)
+{
+	return -1;
+}
+#endif
+
+#endif /* __LINUX_MVEBU_SOC_ID_H */

With the quirk handling in patch 3, I think we should remove the public interface and EXPORT_SYMBOL(), as well as move this header file into mach-mvebu.

That said, we may want to add support for the soc_bus later on (not as part of the stable bug fix) to make it possible to query the soc version from user space through the standard sysfs files.

Arnd

Gregory CLEMENT

author

2014-01-08 14:16 UTC #3 lore raw

PATCH v4 1/3 ARM: mvebu: Add support to get the ID and the revision of a SoC

Quoted 20 lines · Arnd Bergmann

Quoted 5 lines · Gregory CLEMENT
include/linux/mvebu-soc-id.h new file +0 -0
new file mode 100644
index 000000000000..31654252fe35
include/linux/mvebu-soc-id.h line 32
Quoted 10 lines
include/linux/mvebu-soc-id.h new file +10 -0
+#ifdef CONFIG_ARCH_MVEBU
+int mvebu_get_soc_id(u32 *dev, u32 *rev);
+#else
+static inline int mvebu_get_soc_id(u32 *dev, u32 *rev)
+{
+	return -1;
+}
+#endif
+
+#endif /* __LINUX_MVEBU_SOC_ID_H */
With the quirk handling in patch 3, I think we should remove the public interface and EXPORT_SYMBOL(), as well as move this header file into mach-mvebu.

Quoted 3 lines

That said, we may want to add support for the soc_bus later on (not as part of the stable bug fix) to make it possible to query the soc version from user space through the standard sysfs files.

We could do it later indeed. For having a more acceptable patch for stable I will move this header file into mach-mvebu.

Thanks,

Gregory

Signature · 4 lines

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

Gregory CLEMENT

author

2014-01-07 16:35 UTC #4 lore raw

PATCH v4 2/3 ARM: mvebu: Add quirk for i2c

In reply to Gregory CLEMENT

The first variants of Armada XP SoCs (A0 stepping) have issues related to the i2c controller which prevent to use the offload mechanism and lead to a kernel hang during boot.

This commit add quirk in the mvebu platform code to check the SoC version and then add the "offload-broken" property for the i2c controller according to the revision of the SoC.

Signed-off-by Gregory CLEMENT <[email protected]>

---
 arch/arm/mach-mvebu/armada-370-xp.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

arch/arm/mach-mvebu/armada-370-xp.c +24 -0

index e2acff98e750..c93ac68779e0 100644
@@ -21,6 +21,7 @@
 #include <linux/clocksource.h>
 #include <linux/dma-mapping.h>
 #include <linux/mbus.h>
+#include <linux/mvebu-soc-id.h>
 #include <asm/hardware/cache-l2x0.h>
 #include <asm/mach/arch.h>
 #include <asm/mach/map.h>

@@ -45,8 +46,31 @@ static void __init armada_370_xp_timer_and_clk_init(void)
 #endif
 }
 
+static struct property i2c_offload_broken = {
+	.name = "offload-broken",
+};
+
+static void __init i2c_quirk(void)
+{
+	struct device_node *np;
+	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.
+	 */
+	if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV)
+		return;
+
+	for_each_compatible_node(np, NULL, "marvell,mv78230-i2c")
+		of_add_property(np, &i2c_offload_broken);
+	return;
+}
+
 static void __init armada_370_xp_dt_init(void)
 {
+	i2c_quirk();
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 }

Signature · 1 line

1.8.1.2

Jason Gunthorpe

2014-01-07 18:38 UTC #5 lore raw

PATCH v4 2/3 ARM: mvebu: Add quirk for i2c

Quoted 9 lines · Gregory CLEMENT

arch/arm/mach-mvebu/armada-370-xp.c +9 -0

+static struct property i2c_offload_broken = {
+	.name = "offload-broken",
+};
+
+static void __init i2c_quirk(void)
+{
+	struct device_node *np;
+	u32 dev, rev;
+

arch/arm/mach-mvebu/armada-370-xp.c static void __init armada_370_xp_timer_and_clk_init(void) · line 67

Quoted 10 lines

arch/arm/mach-mvebu/armada-370-xp.c +10 -0

+	/*
+	 * 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.
+	 */
+	if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV)
+		return;
+
+	for_each_compatible_node(np, NULL, "marvell,mv78230-i2c")
+		of_add_property(np, &i2c_offload_broken);

I like this approach.

However, when I first read this I thought it should be a -a0 specific compatible string, not a 'offload-broken' property - any idea what the DT consensus is here? I've seen both approach in use ..

Jason

Wolfram Sang

2014-01-07 23:06 UTC #6 lore raw

PATCH v4 2/3 ARM: mvebu: Add quirk for i2c

Quoted 14 lines · Jason Gunthorpe

Quoted 13 lines · Gregory CLEMENT

arch/arm/mach-mvebu/armada-370-xp.c +13 -0

+static struct property i2c_offload_broken = {
+	.name = "offload-broken",
+};
+
+static void __init i2c_quirk(void)
+{
+	struct device_node *np;
+	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.

Quoted 8 lines

arch/arm/mach-mvebu/armada-370-xp.c static void __init armada_370_xp_timer_and_clk_init(void) · line 67
Quoted 6 lines
arch/arm/mach-mvebu/armada-370-xp.c +6 -0
+	 */
+	if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV)
+		return;
+
+	for_each_compatible_node(np, NULL, "marvell,mv78230-i2c")
+		of_add_property(np, &i2c_offload_broken);
I like this approach.

Sorry, but I don't.

Quoted 3 lines

However, when I first read this I thought it should be a -a0 specific compatible string, not a 'offload-broken' property - any idea what the DT consensus is here? I've seen both approach in use ..

I prefer the replacement of the compatible string. If it should really be a seperate property, then it should be a vendor specific property. It is not generic, at all.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140108/cd7a5e56/attachment.sig>

Gregory CLEMENT

author

2014-01-08 10:15 UTC #7 lore raw

PATCH v4 2/3 ARM: mvebu: Add quirk for i2c

Hi Wolfram,

Quoted 26 lines · Wolfram Sang

Quoted 14 lines · Jason Gunthorpe
Quoted 13 lines · Gregory CLEMENT
arch/arm/mach-mvebu/armada-370-xp.c +13 -0
+static struct property i2c_offload_broken = {
+	.name = "offload-broken",
+};
+
+static void __init i2c_quirk(void)
+{
+	struct device_node *np;
+	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.
Quoted 8 lines
arch/arm/mach-mvebu/armada-370-xp.c static void __init armada_370_xp_timer_and_clk_init(void) · line 67
Quoted 6 lines
arch/arm/mach-mvebu/armada-370-xp.c +6 -0
+	 */
+	if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV)
+		return;
+
+	for_each_compatible_node(np, NULL, "marvell,mv78230-i2c")
+		of_add_property(np, &i2c_offload_broken);
I like this approach.
Sorry, but I don't.

Quoted 7 lines

Quoted 3 lines
However, when I first read this I thought it should be a -a0 specific compatible string, not a 'offload-broken' property - any idea what the DT consensus is here? I've seen both approach in use ..
I prefer the replacement of the compatible string. If it should really be a seperate property, then it should be a vendor specific property. It is not generic, at all.

Something like "marvell,offload-broken" would be acceptable?

Thanks,

Gregory

Signature · 4 lines

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

Wolfram Sang

2014-01-08 11:29 UTC #8 lore raw

PATCH v4 2/3 ARM: mvebu: Add quirk for i2c

Quoted 33 lines · Gregory CLEMENT

Hi Wolfram,
Quoted 30 lines · Wolfram Sang
Quoted 14 lines · Jason Gunthorpe
Quoted 13 lines · Gregory CLEMENT
arch/arm/mach-mvebu/armada-370-xp.c +13 -0
+static struct property i2c_offload_broken = {
+	.name = "offload-broken",
+};
+
+static void __init i2c_quirk(void)
+{
+	struct device_node *np;
+	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.
Quoted 8 lines
arch/arm/mach-mvebu/armada-370-xp.c static void __init armada_370_xp_timer_and_clk_init(void) · line 67
Quoted 6 lines
arch/arm/mach-mvebu/armada-370-xp.c +6 -0
+	 */
+	if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV)
+		return;
+
+	for_each_compatible_node(np, NULL, "marvell,mv78230-i2c")
+		of_add_property(np, &i2c_offload_broken);
I like this approach.
Sorry, but I don't.
Quoted 3 lines
However, when I first read this I thought it should be a -a0 specific compatible string, not a 'offload-broken' property - any idea what the DT consensus is here? I've seen both approach in use ..

Quoted 5 lines

Quoted 3 lines
I prefer the replacement of the compatible string. If it should really be a seperate property, then it should be a vendor specific property. It is not generic, at all.
Something like "marvell,offload-broken" would be acceptable?

A tad more, yes. Still, since this is a feature/quirk of the IP core revision, it should be deduced from the compatible property IMO. It cannot be configured anywhere, so it doesn't change on board level.

Gregory CLEMENT

author

2014-01-08 13:03 UTC #9 lore raw

PATCH v4 2/3 ARM: mvebu: Add quirk for i2c

Quoted 38 lines · Wolfram Sang

Quoted 37 lines · Gregory CLEMENT
Hi Wolfram,
Quoted 33 lines · Wolfram Sang
Quoted 14 lines · Jason Gunthorpe
Quoted 13 lines · Gregory CLEMENT
(quoted diff) +13 -0
+static struct property i2c_offload_broken = {
+	.name = "offload-broken",
+};
+
+static void __init i2c_quirk(void)
+{
+	struct device_node *np;
+	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.
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6 -0
+	 */
+	if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV)
+		return;
+
+	for_each_compatible_node(np, NULL, "marvell,mv78230-i2c")
+		of_add_property(np, &i2c_offload_broken);
I like this approach.
Sorry, but I don't.
Quoted 3 lines
However, when I first read this I thought it should be a -a0 specific compatible string, not a 'offload-broken' property - any idea what the DT consensus is here? I've seen both approach in use ..
I prefer the replacement of the compatible string. If it should really be a seperate property, then it should be a vendor specific property. It is not generic, at all.

Quoted 5 lines

Quoted 1 line
Something like "marvell,offload-broken" would be acceptable?
A tad more, yes. Still, since this is a feature/quirk of the IP core revision, it should be deduced from the compatible property IMO. It cannot be configured anywhere, so it doesn't change on board level.

So you would prefer using the "marvell,mv78230-a0-i2c" comaptible string and updating it with the follwing piece of code?

arch/arm/mach-mvebu/armada-370-xp.c +32 -0

index e2acff98e750..8c4ecf987e82 100644
@@ -21,6 +21,8 @@
 #include <linux/clocksource.h>
 #include <linux/dma-mapping.h>
 #include <linux/mbus.h>
+#include <linux/mvebu-soc-id.h>
+#include <linux/slab.h>
 #include <asm/hardware/cache-l2x0.h>
 #include <asm/mach/arch.h>
 #include <asm/mach/map.h>

@@ -45,8 +47,38 @@ static void __init armada_370_xp_timer_and_clk_init(void)
 #endif
 }
 
+static void __init i2c_quirk(void)
+{
+       struct device_node *np;
+       u32 dev, rev;
+       int i = 0;
+
+       /*
+        * 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.
+        */
+       if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV)
+               return;
+
+       for_each_compatible_node(np, NULL, "marvell,mv78230-i2c") {
+               struct property *new_compat;
+
+               new_compat = kzalloc(sizeof(*new_compat), GFP_KERNEL);
+
+               new_compat->name =  kstrdup("compatible", GFP_KERNEL);
+               new_compat->length =  sizeof("marvell,mv78230-a0-i2c");
+               new_compat->value =  kstrdup("marvell,mv78230-a0-i2c",
+                                               GFP_KERNEL);
+
+               of_update_property(np, new_compat);
+       }
+       return;
+}
+
 static void __init armada_370_xp_dt_init(void)
 {
+       i2c_quirk();
        of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 }

Quoted 8 lines

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Signature · 4 lines

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

Wolfram Sang

2014-01-08 13:39 UTC #10 lore raw

PATCH v4 2/3 ARM: mvebu: Add quirk for i2c

Quoted 9 lines

Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 4 lines
However, when I first read this I thought it should be a -a0 specific compatible string, not a 'offload-broken' property - any idea what the DT consensus is here? I've seen both approach in use ..
I prefer the replacement of the compatible string. If it should really be a seperate property, then it should be a vendor specific property. It is not generic, at all.

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
Something like "marvell,offload-broken" would be acceptable?
A tad more, yes. Still, since this is a feature/quirk of the IP core revision, it should be deduced from the compatible property IMO. It cannot be configured anywhere, so it doesn't change on board level.
So you would prefer using the "marvell,mv78230-a0-i2c" comaptible string and updating it with the follwing piece of code?

This is the approach I favour, yes. Can't say much about the implementation. Looks OK, but dunno if this is minimal...

Arnd Bergmann

2014-01-08 13:45 UTC #11 lore raw

PATCH v4 2/3 ARM: mvebu: Add quirk for i2c

Quoted 14 lines · On Wednesday 08 January 2014 14:39:59 Wolfram Sang wrote:

Quoted 14 lines
Quoted 13 lines
Quoted 9 lines
Quoted 7 lines
Quoted 3 lines
However, when I first read this I thought it should be a -a0 specific compatible string, not a 'offload-broken' property - any idea what the DT consensus is here? I've seen both approach in use ..
I prefer the replacement of the compatible string. If it should really be a seperate property, then it should be a vendor specific property. It is not generic, at all.
Something like "marvell,offload-broken" would be acceptable?
A tad more, yes. Still, since this is a feature/quirk of the IP core revision, it should be deduced from the compatible property IMO. It cannot be configured anywhere, so it doesn't change on board level.

Quoted 6 lines

Quoted 2 lines
So you would prefer using the "marvell,mv78230-a0-i2c" comaptible string and updating it with the follwing piece of code?
This is the approach I favour, yes. Can't say much about the implementation. Looks OK, but dunno if this is minimal...

I would prefer the separate property in this case, but only because it's easier to add in the quirk than to change the compatible string, but it's not a strong preference and I don't mind getting overruled if you all favor the alternative.

Arnd

Gregory CLEMENT

author

2014-01-08 14:10 UTC #12 lore raw

PATCH v4 2/3 ARM: mvebu: Add quirk for i2c

Quoted 18 lines · Arnd Bergmann

Quoted 17 lines · On Wednesday 08 January 2014 14:39:59 Wolfram Sang wrote:
Quoted 16 lines
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
However, when I first read this I thought it should be a -a0 specific compatible string, not a 'offload-broken' property - any idea what the DT consensus is here? I've seen both approach in use ..
I prefer the replacement of the compatible string. If it should really be a seperate property, then it should be a vendor specific property. It is not generic, at all.
Quoted 5 lines
Quoted 1 line
Something like "marvell,offload-broken" would be acceptable?
A tad more, yes. Still, since this is a feature/quirk of the IP core revision, it should be deduced from the compatible property IMO. It cannot be configured anywhere, so it doesn't change on board level.
So you would prefer using the "marvell,mv78230-a0-i2c" comaptible string and updating it with the follwing piece of code?

Quoted 8 lines

Quoted 3 lines
This is the approach I favour, yes. Can't say much about the implementation. Looks OK, but dunno if this is minimal...
I would prefer the separate property in this case, but only because it's easier to add in the quirk than to change the compatible string, but it's not a strong preference and I don't mind getting overruled if you all favor the alternative.

Great, all the different part seems to be validated, I can now send the last version.

Thanks,

Gregory

Signature · 4 lines

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

Gregory CLEMENT

author

2014-01-08 14:07 UTC #13 lore raw

PATCH v4 2/3 ARM: mvebu: Add quirk for i2c

In reply to Wolfram Sang

Hi Wolfram,

Quoted 15 lines · Wolfram Sang

Quoted 15 lines
Quoted 14 lines
Quoted 10 lines
Quoted 8 lines
Quoted 4 lines
However, when I first read this I thought it should be a -a0 specific compatible string, not a 'offload-broken' property - any idea what the DT consensus is here? I've seen both approach in use ..
I prefer the replacement of the compatible string. If it should really be a seperate property, then it should be a vendor specific property. It is not generic, at all.
Something like "marvell,offload-broken" would be acceptable?
A tad more, yes. Still, since this is a feature/quirk of the IP core revision, it should be deduced from the compatible property IMO. It cannot be configured anywhere, so it doesn't change on board level.

Quoted 5 lines

Quoted 2 lines
So you would prefer using the "marvell,mv78230-a0-i2c" comaptible string and updating it with the follwing piece of code?
This is the approach I favour, yes. Can't say much about the implementation. Looks OK, but dunno if this is minimal...

Allocating memory in each loop could seem convoluted. In my first approach I just used a static struct but I got warning during boot about duplicated node. It seems we can use the same property struct for two different nodes.

Quoted 1 line

Signature · 4 lines

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

Gregory CLEMENT

author

2014-01-09 14:39 UTC #14 lore raw

PATCH v4 2/3 ARM: mvebu: Add quirk for i2c

Quoted 21 lines · Gregory CLEMENT

Hi Wolfram,
Quoted 18 lines · Wolfram Sang
Quoted 17 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 4 lines
However, when I first read this I thought it should be a -a0 specific compatible string, not a 'offload-broken' property - any idea what the DT consensus is here? I've seen both approach in use ..
I prefer the replacement of the compatible string. If it should really be a seperate property, then it should be a vendor specific property. It is not generic, at all.
Quoted 5 lines
Quoted 1 line
Something like "marvell,offload-broken" would be acceptable?
A tad more, yes. Still, since this is a feature/quirk of the IP core revision, it should be deduced from the compatible property IMO. It cannot be configured anywhere, so it doesn't change on board level.
So you would prefer using the "marvell,mv78230-a0-i2c" comaptible string and updating it with the follwing piece of code?

Quoted 6 lines

Quoted 2 lines
This is the approach I favour, yes. Can't say much about the implementation. Looks OK, but dunno if this is minimal...
Allocating memory in each loop could seem convoluted. In my first approach I just used a static struct but I got warning during boot about duplicated node. It seems we can use the same property struct for two different nodes.

Oh! I just meant the opposite: " It seems we can NOT use the same property struct for two different nodes."

Quoted 4 lines

Quoted 2 lines

Signature · 4 lines

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

Arnd Bergmann

2014-01-08 12:52 UTC #15 lore raw

PATCH v4 2/3 ARM: mvebu: Add quirk for i2c

In reply to Gregory CLEMENT

Quoted 5 lines · Gregory CLEMENT

static void __init armada_370_xp_dt_init(void)
{
+       i2c_quirk();
        of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
}

I'd prefer to enable the quirk only on machines that we know may be affected, i.e. OpenBlocks AX3-4. That would make it easier in the future for everyone to figure out whether they need to include the quirk in their kernels or not, depending on whether they want to support these machines. Just a precaution in case we end up having lots of quirks in the long run.

Arnd

Gregory CLEMENT

author

2014-01-08 13:42 UTC #16 lore raw

PATCH v4 2/3 ARM: mvebu: Add quirk for i2c

Quoted 8 lines · Arnd Bergmann

Quoted 5 lines · Gregory CLEMENT
static void __init armada_370_xp_dt_init(void)
{
+       i2c_quirk();
        of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
}

Quoted 5 lines

I'd prefer to enable the quirk only on machines that we know may be affected, i.e. OpenBlocks AX3-4. That would make it easier in the future for everyone to figure out whether they need to include the quirk in their kernels or not, depending on whether they want to support these machines. Just a precaution in case we end up having lots of quirks in the long run.

You means something like the following code ?

static void __init armada_370_xp_dt_init(void)
{
+ if (of_machine_is_compatible("plathome,openblocks-ax3-4"))

+               i2c_quirk();
        of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);

}

Quoted 7 lines

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 4 lines

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

Arnd Bergmann

2014-01-08 13:44 UTC #17 lore raw

PATCH v4 2/3 ARM: mvebu: Add quirk for i2c

arch/arm/mach-mvebu/armada-370-xp.c static void __init armada_370_xp_timer_and_clk_init(void) · line 75

Quoted 8 lines · On Wednesday 08 January 2014 14:42:45 Gregory CLEMENT wrote:

You means something like the following code ?
static void __init armada_370_xp_dt_init(void)
{
arch/arm/mach-mvebu/armada-370-xp.c +2 -0
+       if (of_machine_is_compatible("plathome,openblocks-ax3-4"))
+               i2c_quirk();
        of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 }

Yes, that looks like a good way to do it.

Arnd

Gregory CLEMENT

author

2014-01-07 16:35 UTC #18 lore raw

PATCH v4 3/3 i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs

In reply to Gregory CLEMENT

The first variants of Armada XP SoCs (A0 stepping) have issues related to the i2c controller which prevent to use the offload mechanism and lead to a kernel hang during boot.

The driver now checks if offload the mechanism is tagged as broken and enable it in the opposite case.

Cc stable at vger.kernel.org

Signed-off-by Gregory CLEMENT <[email protected]>

---
 drivers/i2c/busses/i2c-mv64xxx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/i2c/busses/i2c-mv64xxx.c +3 -1

index 8be7e42aa4de..de819daa19e6 100644
@@ -779,8 +779,10 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
 	 * Transaction Generator support and the errata fix.
 	 */
 	if (of_device_is_compatible(np, "marvell,mv78230-i2c")) {
-		drv_data->offload_enabled = true;
 		drv_data->errata_delay = true;
+
+		if (!of_find_property(np, "offload-broken", NULL))
+			drv_data->offload_enabled = true;
 	}
 
 out:

Signature · 1 line

1.8.1.2

Thomas Petazzoni

2014-01-07 22:17 UTC #19 lore raw

PATCH v4 3/3 i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs

Dear Gregory CLEMENT,

drivers/i2c/busses/i2c-mv64xxx.c mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data, · line 786

Quoted 7 lines · Gregory CLEMENT

drivers/i2c/busses/i2c-mv64xxx.c +3 -1

 	if (of_device_is_compatible(np, "marvell,mv78230-i2c")) {
-		drv_data->offload_enabled = true;
 		drv_data->errata_delay = true;
+
+		if (!of_find_property(np, "offload-broken", NULL))
+			drv_data->offload_enabled = true;
 	}

I think of_property_read_bool() is more appropriate than of_find_property() in this situation, because offload-broken is indeed a boolean property.

Something like:

		drv_data->offload_enable = !of_property_read_bool(np, "offload-broken");

Thanks!

Thomas

Signature · 3 lines

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