[PATCH v5 0/4] Fix i2c bus hang on A0 version of the Armada XP SoCs

[PATCH v5 0/4] Fix i2c bus hang on A0 version of the Armada XP SoCs

[Gregory CLEMENT]

Hi,

Here come the 5th version 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.

The main change are the use of marvell,mv78230-a0-i2c and that the
function mvebu_get_soc_id() is now local to mach-mvebu.

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

The 3 first 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 and
even expose the SoC ID and revision to userspace.

Jason, do you still agree to take the series once Wolfram have given
his acked-by?

Thanks,

Gregory

Changelog:
v4 -> v5:

- use the marvell,mv78230-a0-i2c compatible string instead of the
  offload-broken property.

- move the mvebu-soc-id.h file into mach-mvebu

- no more export the mvebu_get_soc_id() function

- enable the quirk only on machines that we know may be affected, i.e.
OpenBlocks AX3-4.

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 (4):
  ARM: mvebu: Add support to get the ID and the revision of a SoC
  ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board
  i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs
  i2c: mv64xxx: Document the newly introduced Armada XP A0 compatible

 .../devicetree/bindings/i2c/i2c-mv64xxx.txt        |   2 +-
 arch/arm/mach-mvebu/Makefile                       |   2 +-
 arch/arm/mach-mvebu/armada-370-xp.c                |  32 ++++++
 arch/arm/mach-mvebu/mvebu-soc-id.c                 | 119 +++++++++++++++++++++
 arch/arm/mach-mvebu/mvebu-soc-id.h                 |  32 ++++++
 drivers/i2c/busses/i2c-mv64xxx.c                   |   8 ++
 6 files changed, 193 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm/mach-mvebu/mvebu-soc-id.c
 create mode 100644 arch/arm/mach-mvebu/mvebu-soc-id.h

-- 
1.8.1.2

[PATCH v5 0/4] Fix i2c bus hang on A0 version of the Armada XP SoCs

[Gregory CLEMENT]

Hi,

Here come the 5th version 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.

The main change are the use of marvell,mv78230-a0-i2c and that the
function mvebu_get_soc_id() is now local to mach-mvebu.

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

The 3 first 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 and
even expose the SoC ID and revision to userspace.

Jason, do you still agree to take the series once Wolfram have given
his acked-by?

Thanks,

Gregory

Changelog:
v4 -> v5:

- use the marvell,mv78230-a0-i2c compatible string instead of the
  offload-broken property.

- move the mvebu-soc-id.h file into mach-mvebu

- no more export the mvebu_get_soc_id() function

- enable the quirk only on machines that we know may be affected, i.e.
OpenBlocks AX3-4.

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 (4):
  ARM: mvebu: Add support to get the ID and the revision of a SoC
  ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board
  i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs
  i2c: mv64xxx: Document the newly introduced Armada XP A0 compatible

 .../devicetree/bindings/i2c/i2c-mv64xxx.txt        |   2 +-
 arch/arm/mach-mvebu/Makefile                       |   2 +-
 arch/arm/mach-mvebu/armada-370-xp.c                |  32 ++++++
 arch/arm/mach-mvebu/mvebu-soc-id.c                 | 119 +++++++++++++++++++++
 arch/arm/mach-mvebu/mvebu-soc-id.h                 |  32 ++++++
 drivers/i2c/busses/i2c-mv64xxx.c                   |   8 ++
 6 files changed, 193 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm/mach-mvebu/mvebu-soc-id.c
 create mode 100644 arch/arm/mach-mvebu/mvebu-soc-id.h

-- 
1.8.1.2

[PATCH v5 1/4] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Gregory CLEMENT]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 arch/arm/mach-mvebu/Makefile       |   2 +-
 arch/arm/mach-mvebu/mvebu-soc-id.c | 119 +++++++++++++++++++++++++++++++++++++
 arch/arm/mach-mvebu/mvebu-soc-id.h |  32 ++++++++++
 3 files changed, 152 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/mach-mvebu/mvebu-soc-id.c
 create mode 100644 arch/arm/mach-mvebu/mvebu-soc-id.h

diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
index 2d04f0e21870..878aebe98dcc 100644
--- a/arch/arm/mach-mvebu/Makefile
+++ b/arch/arm/mach-mvebu/Makefile
@@ -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
diff --git a/arch/arm/mach-mvebu/mvebu-soc-id.c b/arch/arm/mach-mvebu/mvebu-soc-id.c
new file mode 100644
index 000000000000..fe4fc1cbdfaf
--- /dev/null
+++ b/arch/arm/mach-mvebu/mvebu-soc-id.c
@@ -0,0 +1,119 @@
+/*
+ * ID and revision information for mvebu SoCs
+ *
+ * Copyright (C) 2014 Marvell
+ *
+ * Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
+ *
+ * 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.
+ *
+ * All the mvebu SoCs have information related to their variant and
+ * revision that can be read from the PCI control register. This is
+ * done before the PCI initialization to avoid any conflict. Once the
+ * ID and revision are retrieved, the mapping is freed.
+ */
+
+#define pr_fmt(fmt) "mvebu-soc-id: " fmt
+
+#include <linux/clk.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include "mvebu-soc-id.h"
+
+#define PCIE_DEV_ID_OFF		0x0
+#define PCIE_DEV_REV_OFF	0x8
+
+#define SOC_ID_MASK	    0xFFFF0000
+#define SOC_REV_MASK	    0xFF
+
+static u32 soc_dev_id;
+static u32 soc_rev;
+static bool is_id_valid;
+
+static const struct of_device_id mvebu_pcie_of_match_table[] = {
+	{ .compatible = "marvell,armada-xp-pcie", },
+	{ .compatible = "marvell,armada-370-pcie", },
+	{},
+};
+
+int mvebu_get_soc_id(u32 *dev, u32 *rev)
+{
+	if (is_id_valid) {
+		*dev = soc_dev_id;
+		*rev = soc_rev;
+		return 0;
+	} else
+		return -1;
+}
+
+static int __init mvebu_soc_id_init(void)
+{
+	struct device_node *np;
+	int ret = 0;
+	void __iomem *pci_base;
+	struct clk *clk;
+	struct device_node *child;
+
+	np = of_find_matching_node(NULL, mvebu_pcie_of_match_table);
+	if (!np)
+		return ret;
+
+	/*
+	 * ID and revision are available from any port, so we
+	 * just pick the first one
+	 */
+	child = of_get_next_child(np, NULL);
+	if (child == NULL) {
+		pr_err("cannot get pci node\n");
+		ret = -ENOMEM;
+		goto clk_err;
+	}
+
+	clk = of_clk_get_by_name(child, NULL);
+	if (IS_ERR(clk)) {
+		pr_err("cannot get clock\n");
+		ret = -ENOMEM;
+		goto clk_err;
+	}
+
+	ret = clk_prepare_enable(clk);
+	if (ret) {
+		pr_err("cannot enable clock\n");
+		goto clk_err;
+	}
+
+	pci_base = of_iomap(child, 0);
+	if (IS_ERR(pci_base)) {
+		pr_err("cannot map registers\n");
+		ret = -ENOMEM;
+		goto res_ioremap;
+	}
+
+	/* SoC ID */
+	soc_dev_id = readl(pci_base + PCIE_DEV_ID_OFF) >> 16;
+
+	/* SoC revision */
+	soc_rev = readl(pci_base + PCIE_DEV_REV_OFF) & SOC_REV_MASK;
+
+	is_id_valid = true;
+
+	pr_info("MVEBU SoC ID=0x%X, Rev=0x%X\n", soc_dev_id, soc_rev);
+
+	iounmap(pci_base);
+
+res_ioremap:
+	clk_disable_unprepare(clk);
+
+clk_err:
+	of_node_put(child);
+	of_node_put(np);
+
+	return ret;
+}
+core_initcall(mvebu_soc_id_init);
+
diff --git a/arch/arm/mach-mvebu/mvebu-soc-id.h b/arch/arm/mach-mvebu/mvebu-soc-id.h
new file mode 100644
index 000000000000..31654252fe35
--- /dev/null
+++ b/arch/arm/mach-mvebu/mvebu-soc-id.h
@@ -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 */
-- 
1.8.1.2

[PATCH v5 1/4] ARM: mvebu: Add support to get the ID and the revision of a SoC

[Gregory CLEMENT]

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 <gregory.clement@free-electrons.com>
---
 arch/arm/mach-mvebu/Makefile       |   2 +-
 arch/arm/mach-mvebu/mvebu-soc-id.c | 119 +++++++++++++++++++++++++++++++++++++
 arch/arm/mach-mvebu/mvebu-soc-id.h |  32 ++++++++++
 3 files changed, 152 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/mach-mvebu/mvebu-soc-id.c
 create mode 100644 arch/arm/mach-mvebu/mvebu-soc-id.h

diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
index 2d04f0e21870..878aebe98dcc 100644
--- a/arch/arm/mach-mvebu/Makefile
+++ b/arch/arm/mach-mvebu/Makefile
@@ -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
diff --git a/arch/arm/mach-mvebu/mvebu-soc-id.c b/arch/arm/mach-mvebu/mvebu-soc-id.c
new file mode 100644
index 000000000000..fe4fc1cbdfaf
--- /dev/null
+++ b/arch/arm/mach-mvebu/mvebu-soc-id.c
@@ -0,0 +1,119 @@
+/*
+ * ID and revision information for mvebu SoCs
+ *
+ * Copyright (C) 2014 Marvell
+ *
+ * Gregory CLEMENT <gregory.clement@free-electrons.com>
+ *
+ * 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.
+ *
+ * All the mvebu SoCs have information related to their variant and
+ * revision that can be read from the PCI control register. This is
+ * done before the PCI initialization to avoid any conflict. Once the
+ * ID and revision are retrieved, the mapping is freed.
+ */
+
+#define pr_fmt(fmt) "mvebu-soc-id: " fmt
+
+#include <linux/clk.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include "mvebu-soc-id.h"
+
+#define PCIE_DEV_ID_OFF		0x0
+#define PCIE_DEV_REV_OFF	0x8
+
+#define SOC_ID_MASK	    0xFFFF0000
+#define SOC_REV_MASK	    0xFF
+
+static u32 soc_dev_id;
+static u32 soc_rev;
+static bool is_id_valid;
+
+static const struct of_device_id mvebu_pcie_of_match_table[] = {
+	{ .compatible = "marvell,armada-xp-pcie", },
+	{ .compatible = "marvell,armada-370-pcie", },
+	{},
+};
+
+int mvebu_get_soc_id(u32 *dev, u32 *rev)
+{
+	if (is_id_valid) {
+		*dev = soc_dev_id;
+		*rev = soc_rev;
+		return 0;
+	} else
+		return -1;
+}
+
+static int __init mvebu_soc_id_init(void)
+{
+	struct device_node *np;
+	int ret = 0;
+	void __iomem *pci_base;
+	struct clk *clk;
+	struct device_node *child;
+
+	np = of_find_matching_node(NULL, mvebu_pcie_of_match_table);
+	if (!np)
+		return ret;
+
+	/*
+	 * ID and revision are available from any port, so we
+	 * just pick the first one
+	 */
+	child = of_get_next_child(np, NULL);
+	if (child == NULL) {
+		pr_err("cannot get pci node\n");
+		ret = -ENOMEM;
+		goto clk_err;
+	}
+
+	clk = of_clk_get_by_name(child, NULL);
+	if (IS_ERR(clk)) {
+		pr_err("cannot get clock\n");
+		ret = -ENOMEM;
+		goto clk_err;
+	}
+
+	ret = clk_prepare_enable(clk);
+	if (ret) {
+		pr_err("cannot enable clock\n");
+		goto clk_err;
+	}
+
+	pci_base = of_iomap(child, 0);
+	if (IS_ERR(pci_base)) {
+		pr_err("cannot map registers\n");
+		ret = -ENOMEM;
+		goto res_ioremap;
+	}
+
+	/* SoC ID */
+	soc_dev_id = readl(pci_base + PCIE_DEV_ID_OFF) >> 16;
+
+	/* SoC revision */
+	soc_rev = readl(pci_base + PCIE_DEV_REV_OFF) & SOC_REV_MASK;
+
+	is_id_valid = true;
+
+	pr_info("MVEBU SoC ID=0x%X, Rev=0x%X\n", soc_dev_id, soc_rev);
+
+	iounmap(pci_base);
+
+res_ioremap:
+	clk_disable_unprepare(clk);
+
+clk_err:
+	of_node_put(child);
+	of_node_put(np);
+
+	return ret;
+}
+core_initcall(mvebu_soc_id_init);
+
diff --git a/arch/arm/mach-mvebu/mvebu-soc-id.h b/arch/arm/mach-mvebu/mvebu-soc-id.h
new file mode 100644
index 000000000000..31654252fe35
--- /dev/null
+++ b/arch/arm/mach-mvebu/mvebu-soc-id.h
@@ -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 */
-- 
1.8.1.2

[PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[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 update the compatible string for the i2c controller
according to the revision of the SoC. Currently only some OpenBlocks
AX3-4 boards are known to use an A0 revision so the check is done only
for these boards.

Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
---
 arch/arm/mach-mvebu/armada-370-xp.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
index e2acff98e750..0f61a4f22fb5 100644
--- a/arch/arm/mach-mvebu/armada-370-xp.c
+++ b/arch/arm/mach-mvebu/armada-370-xp.c
@@ -21,6 +21,7 @@
 #include <linux/clocksource.h>
 #include <linux/dma-mapping.h>
 #include <linux/mbus.h>
+#include <linux/slab.h>
 #include <asm/hardware/cache-l2x0.h>
 #include <asm/mach/arch.h>
 #include <asm/mach/map.h>
@@ -28,6 +29,7 @@
 #include "armada-370-xp.h"
 #include "common.h"
 #include "coherency.h"
+#include "mvebu-soc-id.h"
 
 static void __init armada_370_xp_map_io(void)
 {
@@ -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;
+
+	/*
+	 * 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)
 {
+	if (of_machine_is_compatible("plathome,openblocks-ax3-4"))
+		i2c_quirk();
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 }
 
-- 
1.8.1.2

[PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[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 update the compatible string for the i2c controller
according to the revision of the SoC. Currently only some OpenBlocks
AX3-4 boards are known to use an A0 revision so the check is done only
for these boards.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Cc: stable at vger.kernel.org
---
 arch/arm/mach-mvebu/armada-370-xp.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
index e2acff98e750..0f61a4f22fb5 100644
--- a/arch/arm/mach-mvebu/armada-370-xp.c
+++ b/arch/arm/mach-mvebu/armada-370-xp.c
@@ -21,6 +21,7 @@
 #include <linux/clocksource.h>
 #include <linux/dma-mapping.h>
 #include <linux/mbus.h>
+#include <linux/slab.h>
 #include <asm/hardware/cache-l2x0.h>
 #include <asm/mach/arch.h>
 #include <asm/mach/map.h>
@@ -28,6 +29,7 @@
 #include "armada-370-xp.h"
 #include "common.h"
 #include "coherency.h"
+#include "mvebu-soc-id.h"
 
 static void __init armada_370_xp_map_io(void)
 {
@@ -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;
+
+	/*
+	 * 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)
 {
+	if (of_machine_is_compatible("plathome,openblocks-ax3-4"))
+		i2c_quirk();
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 }
 
-- 
1.8.1.2

[PATCH v5 3/4] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs

[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 commit introduces a new the compatible string
marvell,mv78230-a0-i2c for the i2c controller. When this compatible
string is used the driver disables the offload mechanism and the
kernel no more hangs on these SoCs.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Reported-by: Andrew Lunn <andrew@lunn.ch>
Cc: stable@vger.kernel.org
---
 drivers/i2c/busses/i2c-mv64xxx.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 8be7e42aa4de..f424c0f89946 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -692,6 +692,10 @@ static const struct of_device_id mv64xxx_i2c_of_match_table[] = {
 	{ .compatible = "allwinner,sun4i-i2c", .data = &mv64xxx_i2c_regs_sun4i},
 	{ .compatible = "marvell,mv64xxx-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
 	{ .compatible = "marvell,mv78230-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
+	{
+		.compatible = "marvell,mv78230-a0-i2c",
+		.data = &mv64xxx_i2c_regs_mv64xxx
+	},
 	{}
 };
 MODULE_DEVICE_TABLE(of, mv64xxx_i2c_of_match_table);
@@ -783,6 +787,10 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
 		drv_data->errata_delay = true;
 	}
 
+	if (of_device_is_compatible(np, "marvell,mv78230-a0-i2c")) {
+		drv_data->offload_enabled = false;
+		drv_data->errata_delay = true;
+	}
 out:
 	return rc;
 #endif
-- 
1.8.1.2

[PATCH v5 3/4] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs

[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 commit introduces a new the compatible string
marvell,mv78230-a0-i2c for the i2c controller. When this compatible
string is used the driver disables the offload mechanism and the
kernel no more hangs on these SoCs.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Reported-by: Andrew Lunn <andrew@lunn.ch>
Cc: stable at vger.kernel.org
---
 drivers/i2c/busses/i2c-mv64xxx.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
index 8be7e42aa4de..f424c0f89946 100644
--- a/drivers/i2c/busses/i2c-mv64xxx.c
+++ b/drivers/i2c/busses/i2c-mv64xxx.c
@@ -692,6 +692,10 @@ static const struct of_device_id mv64xxx_i2c_of_match_table[] = {
 	{ .compatible = "allwinner,sun4i-i2c", .data = &mv64xxx_i2c_regs_sun4i},
 	{ .compatible = "marvell,mv64xxx-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
 	{ .compatible = "marvell,mv78230-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
+	{
+		.compatible = "marvell,mv78230-a0-i2c",
+		.data = &mv64xxx_i2c_regs_mv64xxx
+	},
 	{}
 };
 MODULE_DEVICE_TABLE(of, mv64xxx_i2c_of_match_table);
@@ -783,6 +787,10 @@ mv64xxx_of_config(struct mv64xxx_i2c_data *drv_data,
 		drv_data->errata_delay = true;
 	}
 
+	if (of_device_is_compatible(np, "marvell,mv78230-a0-i2c")) {
+		drv_data->offload_enabled = false;
+		drv_data->errata_delay = true;
+	}
 out:
 	return rc;
 #endif
-- 
1.8.1.2

[PATCH v5 4/4] i2c: mv64xxx: Document the newly introduced Armada XP A0 compatible

[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
ead to a kernel hang during boot.

The commit introduces a new the compatible string
marvell,mv78230-a0-i2c for the i2c controller.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
cc: devicetree@vger.kernel.org
---
 Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
index 82e8f6f17179..9410ed72ec45 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
@@ -5,7 +5,7 @@ Required properties :
 
  - reg             : Offset and length of the register set for the device
  - compatible      : Should be "marvell,mv64xxx-i2c" or "allwinner,sun4i-i2c"
-                     or "marvell,mv78230-i2c"
+                     or "marvell,mv78230-i2c" or "marvell,mv78230-a0-i2c"
  - interrupts      : The interrupt number
 
 Optional properties :
-- 
1.8.1.2

[PATCH v5 4/4] i2c: mv64xxx: Document the newly introduced Armada XP A0 compatible

[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
ead to a kernel hang during boot.

The commit introduces a new the compatible string
marvell,mv78230-a0-i2c for the i2c controller.

Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
cc: devicetree at vger.kernel.org
---
 Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
index 82e8f6f17179..9410ed72ec45 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
@@ -5,7 +5,7 @@ Required properties :
 
  - reg             : Offset and length of the register set for the device
  - compatible      : Should be "marvell,mv64xxx-i2c" or "allwinner,sun4i-i2c"
-                     or "marvell,mv78230-i2c"
+                     or "marvell,mv78230-i2c" or "marvell,mv78230-a0-i2c"
  - interrupts      : The interrupt number
 
 Optional properties :
-- 
1.8.1.2

Re: [PATCH v5 0/4] Fix i2c bus hang on A0 version of the Armada XP SoCs

[Arnd Bergmann]

On Wednesday 08 January 2014 16:06:25 Gregory CLEMENT wrote:
> Hi,
> 
> Here come the 5th version 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.
> 
> The main change are the use of marvell,mv78230-a0-i2c and that the
> function mvebu_get_soc_id() is now local to mach-mvebu.
> 
> The first patch add a mean to detect the SoCs version at run-time and
> the second one use this feature in the driver.
> 
> The 3 first 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 and
> even expose the SoC ID and revision to userspace.
> 
> Jason, do you still agree to take the series once Wolfram have given
> his acked-by?

Whole series

Acked-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

[PATCH v5 0/4] Fix i2c bus hang on A0 version of the Armada XP SoCs

[Arnd Bergmann]

On Wednesday 08 January 2014 16:06:25 Gregory CLEMENT wrote:
> Hi,
> 
> Here come the 5th version 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.
> 
> The main change are the use of marvell,mv78230-a0-i2c and that the
> function mvebu_get_soc_id() is now local to mach-mvebu.
> 
> The first patch add a mean to detect the SoCs version at run-time and
> the second one use this feature in the driver.
> 
> The 3 first 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 and
> even expose the SoC ID and revision to userspace.
> 
> Jason, do you still agree to take the series once Wolfram have given
> his acked-by?

Whole series

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

Re: [PATCH v5 3/4] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1517 bytes --]

On Wed, Jan 08, 2014 at 04:06:28PM +0100, Gregory CLEMENT wrote:
> 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 commit introduces a new the compatible string
> marvell,mv78230-a0-i2c for the i2c controller. When this compatible
> string is used the driver disables the offload mechanism and the
> kernel no more hangs on these SoCs.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> Reported-by: Andrew Lunn <andrew@lunn.ch>
> Cc: stable@vger.kernel.org
> ---
>  drivers/i2c/busses/i2c-mv64xxx.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index 8be7e42aa4de..f424c0f89946 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -692,6 +692,10 @@ static const struct of_device_id mv64xxx_i2c_of_match_table[] = {
>  	{ .compatible = "allwinner,sun4i-i2c", .data = &mv64xxx_i2c_regs_sun4i},
>  	{ .compatible = "marvell,mv64xxx-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
>  	{ .compatible = "marvell,mv78230-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
> +	{
> +		.compatible = "marvell,mv78230-a0-i2c",
> +		.data = &mv64xxx_i2c_regs_mv64xxx
> +	},

I think a oneliner entry like the entries above is easier to read, but
that is very minor...

Acked-by: Wolfram Sang <wsa@the-dreams.de>


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v5 3/4] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs

[Wolfram Sang]

On Wed, Jan 08, 2014 at 04:06:28PM +0100, Gregory CLEMENT wrote:
> 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 commit introduces a new the compatible string
> marvell,mv78230-a0-i2c for the i2c controller. When this compatible
> string is used the driver disables the offload mechanism and the
> kernel no more hangs on these SoCs.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> Reported-by: Andrew Lunn <andrew@lunn.ch>
> Cc: stable at vger.kernel.org
> ---
>  drivers/i2c/busses/i2c-mv64xxx.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> index 8be7e42aa4de..f424c0f89946 100644
> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> @@ -692,6 +692,10 @@ static const struct of_device_id mv64xxx_i2c_of_match_table[] = {
>  	{ .compatible = "allwinner,sun4i-i2c", .data = &mv64xxx_i2c_regs_sun4i},
>  	{ .compatible = "marvell,mv64xxx-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
>  	{ .compatible = "marvell,mv78230-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
> +	{
> +		.compatible = "marvell,mv78230-a0-i2c",
> +		.data = &mv64xxx_i2c_regs_mv64xxx
> +	},

I think a oneliner entry like the entries above is easier to read, but
that is very minor...

Acked-by: Wolfram Sang <wsa@the-dreams.de>

-------------- 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/5d478cfa/attachment.sig>

Re: [PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 258 bytes --]

> +		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);
> +

Very minor again: Strange whitespacing after "="


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Wolfram Sang]

> +		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);
> +

Very minor again: Strange whitespacing after "="

-------------- 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/e4f01007/attachment.sig>

Re: [PATCH v5 3/4] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs

[Gregory CLEMENT]

On 08/01/2014 16:21, Wolfram Sang wrote:
> On Wed, Jan 08, 2014 at 04:06:28PM +0100, Gregory CLEMENT wrote:
>> 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 commit introduces a new the compatible string
>> marvell,mv78230-a0-i2c for the i2c controller. When this compatible
>> string is used the driver disables the offload mechanism and the
>> kernel no more hangs on these SoCs.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> Reported-by: Andrew Lunn <andrew@lunn.ch>
>> Cc: stable@vger.kernel.org
>> ---
>>  drivers/i2c/busses/i2c-mv64xxx.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
>> index 8be7e42aa4de..f424c0f89946 100644
>> --- a/drivers/i2c/busses/i2c-mv64xxx.c
>> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
>> @@ -692,6 +692,10 @@ static const struct of_device_id mv64xxx_i2c_of_match_table[] = {
>>  	{ .compatible = "allwinner,sun4i-i2c", .data = &mv64xxx_i2c_regs_sun4i},
>>  	{ .compatible = "marvell,mv64xxx-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
>>  	{ .compatible = "marvell,mv78230-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
>> +	{
>> +		.compatible = "marvell,mv78230-a0-i2c",
>> +		.data = &mv64xxx_i2c_regs_mv64xxx
>> +	},
> 
> I think a oneliner entry like the entries above is easier to read, but
> that is very minor...

By using one line we would break the 80 character rule,
hat why I did in this way.

> 
> Acked-by: Wolfram Sang <wsa@the-dreams.de>
> 

Thanks!

Gregory


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

[PATCH v5 3/4] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs

[Gregory CLEMENT]

On 08/01/2014 16:21, Wolfram Sang wrote:
> On Wed, Jan 08, 2014 at 04:06:28PM +0100, Gregory CLEMENT wrote:
>> 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 commit introduces a new the compatible string
>> marvell,mv78230-a0-i2c for the i2c controller. When this compatible
>> string is used the driver disables the offload mechanism and the
>> kernel no more hangs on these SoCs.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> Reported-by: Andrew Lunn <andrew@lunn.ch>
>> Cc: stable at vger.kernel.org
>> ---
>>  drivers/i2c/busses/i2c-mv64xxx.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
>> index 8be7e42aa4de..f424c0f89946 100644
>> --- a/drivers/i2c/busses/i2c-mv64xxx.c
>> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
>> @@ -692,6 +692,10 @@ static const struct of_device_id mv64xxx_i2c_of_match_table[] = {
>>  	{ .compatible = "allwinner,sun4i-i2c", .data = &mv64xxx_i2c_regs_sun4i},
>>  	{ .compatible = "marvell,mv64xxx-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
>>  	{ .compatible = "marvell,mv78230-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
>> +	{
>> +		.compatible = "marvell,mv78230-a0-i2c",
>> +		.data = &mv64xxx_i2c_regs_mv64xxx
>> +	},
> 
> I think a oneliner entry like the entries above is easier to read, but
> that is very minor...

By using one line we would break the 80 character rule,
hat why I did in this way.

> 
> Acked-by: Wolfram Sang <wsa@the-dreams.de>
> 

Thanks!

Gregory


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

Re: [PATCH v5 3/4] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs

[Arnd Bergmann]

On Wednesday 08 January 2014, Gregory CLEMENT wrote:
> On 08/01/2014 16:21, Wolfram Sang wrote:

> >> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> >> index 8be7e42aa4de..f424c0f89946 100644
> >> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> >> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> >> @@ -692,6 +692,10 @@ static const struct of_device_id mv64xxx_i2c_of_match_table[] = {
> >>      { .compatible = "allwinner,sun4i-i2c", .data = &mv64xxx_i2c_regs_sun4i},
> >>      { .compatible = "marvell,mv64xxx-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
> >>      { .compatible = "marvell,mv78230-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
> >> +    {
> >> +            .compatible = "marvell,mv78230-a0-i2c",
> >> +            .data = &mv64xxx_i2c_regs_mv64xxx
> >> +    },
> > 
> > I think a oneliner entry like the entries above is easier to read, but
> > that is very minor...
> 
> By using one line we would break the 80 character rule,
> hat why I did in this way.
> 

It's more a guideline than a strict rule. I agree that one line would
be better here, but it's not important.

	Arnd

[PATCH v5 3/4] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs

[Arnd Bergmann]

On Wednesday 08 January 2014, Gregory CLEMENT wrote:
> On 08/01/2014 16:21, Wolfram Sang wrote:

> >> diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c
> >> index 8be7e42aa4de..f424c0f89946 100644
> >> --- a/drivers/i2c/busses/i2c-mv64xxx.c
> >> +++ b/drivers/i2c/busses/i2c-mv64xxx.c
> >> @@ -692,6 +692,10 @@ static const struct of_device_id mv64xxx_i2c_of_match_table[] = {
> >>      { .compatible = "allwinner,sun4i-i2c", .data = &mv64xxx_i2c_regs_sun4i},
> >>      { .compatible = "marvell,mv64xxx-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
> >>      { .compatible = "marvell,mv78230-i2c", .data = &mv64xxx_i2c_regs_mv64xxx},
> >> +    {
> >> +            .compatible = "marvell,mv78230-a0-i2c",
> >> +            .data = &mv64xxx_i2c_regs_mv64xxx
> >> +    },
> > 
> > I think a oneliner entry like the entries above is easier to read, but
> > that is very minor...
> 
> By using one line we would break the 80 character rule,
> hat why I did in this way.
> 

It's more a guideline than a strict rule. I agree that one line would
be better here, but it's not important.

	Arnd

Re: [PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Gregory CLEMENT]

On 08/01/2014 16:22, Wolfram Sang wrote:
>> +		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);
>> +
> 
> Very minor again: Strange whitespacing after "="
> 
arg I missed it :(

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

[PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Gregory CLEMENT]

On 08/01/2014 16:22, Wolfram Sang wrote:
>> +		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);
>> +
> 
> Very minor again: Strange whitespacing after "="
> 
arg I missed it :(

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

Re: [PATCH v5 3/4] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 452 bytes --]

> >> +	{
> >> +		.compatible = "marvell,mv78230-a0-i2c",
> >> +		.data = &mv64xxx_i2c_regs_mv64xxx
> >> +	},
> > 
> > I think a oneliner entry like the entries above is easier to read, but
> > that is very minor...
> 
> By using one line we would break the 80 character rule,
> hat why I did in this way.

This rule is there to keep code readable. If it doesn't help
readability, it may/can/should be broken IMO. It's a mileage, though.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH v5 3/4] i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs

[Wolfram Sang]

> >> +	{
> >> +		.compatible = "marvell,mv78230-a0-i2c",
> >> +		.data = &mv64xxx_i2c_regs_mv64xxx
> >> +	},
> > 
> > I think a oneliner entry like the entries above is easier to read, but
> > that is very minor...
> 
> By using one line we would break the 80 character rule,
> hat why I did in this way.

This rule is there to keep code readable. If it doesn't help
readability, it may/can/should be broken IMO. It's a mileage, though.

-------------- 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/dc7ee46a/attachment.sig>

Re: [PATCH v5 0/4] Fix i2c bus hang on A0 version of the Armada XP SoCs

[Gregory CLEMENT]

On 08/01/2014 16:06, Gregory CLEMENT wrote:
> Hi,
> 
> Here come the 5th version 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.
> 
> The main change are the use of marvell,mv78230-a0-i2c and that the
> function mvebu_get_soc_id() is now local to mach-mvebu.
> 
> The first patch add a mean to detect the SoCs version at run-time and
> the second one use this feature in the driver.
> 
> The 3 first 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 and
> even expose the SoC ID and revision to userspace.
> 
> Jason, do you still agree to take the series once Wolfram have given
> his acked-by?

Jason,

if it can help you I have just pull my branch at:
https://github.com/MISL-EBU-System-SW/mainline-public/tree/i2c-mv64xxx-3.13-rc6-fixes-v6

git@github.com:MISL-EBU-System-SW/mainline-public.git

I also taken into account the two comments and added the acked-by from Wolfram and
Arnd on the appropriate commit.


Thanks,

Gregory

> 
> Thanks,
> 
> Gregory
> 
> Changelog:
> v4 -> v5:
> 
> - use the marvell,mv78230-a0-i2c compatible string instead of the
>   offload-broken property.
> 
> - move the mvebu-soc-id.h file into mach-mvebu
> 
> - no more export the mvebu_get_soc_id() function
> 
> - enable the quirk only on machines that we know may be affected, i.e.
> OpenBlocks AX3-4.
> 
> 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 (4):
>   ARM: mvebu: Add support to get the ID and the revision of a SoC
>   ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board
>   i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs
>   i2c: mv64xxx: Document the newly introduced Armada XP A0 compatible
> 
>  .../devicetree/bindings/i2c/i2c-mv64xxx.txt        |   2 +-
>  arch/arm/mach-mvebu/Makefile                       |   2 +-
>  arch/arm/mach-mvebu/armada-370-xp.c                |  32 ++++++
>  arch/arm/mach-mvebu/mvebu-soc-id.c                 | 119 +++++++++++++++++++++
>  arch/arm/mach-mvebu/mvebu-soc-id.h                 |  32 ++++++
>  drivers/i2c/busses/i2c-mv64xxx.c                   |   8 ++
>  6 files changed, 193 insertions(+), 2 deletions(-)
>  create mode 100644 arch/arm/mach-mvebu/mvebu-soc-id.c
>  create mode 100644 arch/arm/mach-mvebu/mvebu-soc-id.h
> 


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

[PATCH v5 0/4] Fix i2c bus hang on A0 version of the Armada XP SoCs

[Gregory CLEMENT]

On 08/01/2014 16:06, Gregory CLEMENT wrote:
> Hi,
> 
> Here come the 5th version 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.
> 
> The main change are the use of marvell,mv78230-a0-i2c and that the
> function mvebu_get_soc_id() is now local to mach-mvebu.
> 
> The first patch add a mean to detect the SoCs version at run-time and
> the second one use this feature in the driver.
> 
> The 3 first 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 and
> even expose the SoC ID and revision to userspace.
> 
> Jason, do you still agree to take the series once Wolfram have given
> his acked-by?

Jason,

if it can help you I have just pull my branch at:
https://github.com/MISL-EBU-System-SW/mainline-public/tree/i2c-mv64xxx-3.13-rc6-fixes-v6

git at github.com:MISL-EBU-System-SW/mainline-public.git

I also taken into account the two comments and added the acked-by from Wolfram and
Arnd on the appropriate commit.


Thanks,

Gregory

> 
> Thanks,
> 
> Gregory
> 
> Changelog:
> v4 -> v5:
> 
> - use the marvell,mv78230-a0-i2c compatible string instead of the
>   offload-broken property.
> 
> - move the mvebu-soc-id.h file into mach-mvebu
> 
> - no more export the mvebu_get_soc_id() function
> 
> - enable the quirk only on machines that we know may be affected, i.e.
> OpenBlocks AX3-4.
> 
> 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 (4):
>   ARM: mvebu: Add support to get the ID and the revision of a SoC
>   ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board
>   i2c: mv64xxx: Fix bus hang on A0 version of the Armada XP SoCs
>   i2c: mv64xxx: Document the newly introduced Armada XP A0 compatible
> 
>  .../devicetree/bindings/i2c/i2c-mv64xxx.txt        |   2 +-
>  arch/arm/mach-mvebu/Makefile                       |   2 +-
>  arch/arm/mach-mvebu/armada-370-xp.c                |  32 ++++++
>  arch/arm/mach-mvebu/mvebu-soc-id.c                 | 119 +++++++++++++++++++++
>  arch/arm/mach-mvebu/mvebu-soc-id.h                 |  32 ++++++
>  drivers/i2c/busses/i2c-mv64xxx.c                   |   8 ++
>  6 files changed, 193 insertions(+), 2 deletions(-)
>  create mode 100644 arch/arm/mach-mvebu/mvebu-soc-id.c
>  create mode 100644 arch/arm/mach-mvebu/mvebu-soc-id.h
> 


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

Re: [PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Jason Cooper]

Gregory, Arnd,

On Wed, Jan 08, 2014 at 04:06:27PM +0100, Gregory CLEMENT wrote:
> 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 update the compatible string for the i2c controller
> according to the revision of the SoC. Currently only some OpenBlocks
> AX3-4 boards are known to use an A0 revision so the check is done only
> for these boards.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  arch/arm/mach-mvebu/armada-370-xp.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
> index e2acff98e750..0f61a4f22fb5 100644
> --- a/arch/arm/mach-mvebu/armada-370-xp.c
> +++ b/arch/arm/mach-mvebu/armada-370-xp.c
> @@ -21,6 +21,7 @@
>  #include <linux/clocksource.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/mbus.h>
> +#include <linux/slab.h>
>  #include <asm/hardware/cache-l2x0.h>
>  #include <asm/mach/arch.h>
>  #include <asm/mach/map.h>
> @@ -28,6 +29,7 @@
>  #include "armada-370-xp.h"
>  #include "common.h"
>  #include "coherency.h"
> +#include "mvebu-soc-id.h"
>  
>  static void __init armada_370_xp_map_io(void)
>  {
> @@ -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;
> +
> +	/*
> +	 * 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);

I'm still having some trouble with this compatible string choice...  But
I have refined the problem :)

Do we create new compatible strings to indicate errata, or to indicate
'from this version forward there are new features'?  The former would
indicate as Gregory has written '...-a0-i2c', the latter would warrant
'...-b0-i2c' and disabling offloading if we don't see '...-b0-i2c'.

I can see Gregory's approach if he were still using the property
'offload-broken', but I suspect the compatible strings should be handled
differently.

thx,

Jason.

> +
> +		of_update_property(np, new_compat);
> +	}
> +	return;
> +}
> +
>  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);
>  }
>  
> -- 
> 1.8.1.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Jason Cooper]

Gregory, Arnd,

On Wed, Jan 08, 2014 at 04:06:27PM +0100, Gregory CLEMENT wrote:
> 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 update the compatible string for the i2c controller
> according to the revision of the SoC. Currently only some OpenBlocks
> AX3-4 boards are known to use an A0 revision so the check is done only
> for these boards.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> Cc: stable at vger.kernel.org
> ---
>  arch/arm/mach-mvebu/armada-370-xp.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
> index e2acff98e750..0f61a4f22fb5 100644
> --- a/arch/arm/mach-mvebu/armada-370-xp.c
> +++ b/arch/arm/mach-mvebu/armada-370-xp.c
> @@ -21,6 +21,7 @@
>  #include <linux/clocksource.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/mbus.h>
> +#include <linux/slab.h>
>  #include <asm/hardware/cache-l2x0.h>
>  #include <asm/mach/arch.h>
>  #include <asm/mach/map.h>
> @@ -28,6 +29,7 @@
>  #include "armada-370-xp.h"
>  #include "common.h"
>  #include "coherency.h"
> +#include "mvebu-soc-id.h"
>  
>  static void __init armada_370_xp_map_io(void)
>  {
> @@ -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;
> +
> +	/*
> +	 * 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);

I'm still having some trouble with this compatible string choice...  But
I have refined the problem :)

Do we create new compatible strings to indicate errata, or to indicate
'from this version forward there are new features'?  The former would
indicate as Gregory has written '...-a0-i2c', the latter would warrant
'...-b0-i2c' and disabling offloading if we don't see '...-b0-i2c'.

I can see Gregory's approach if he were still using the property
'offload-broken', but I suspect the compatible strings should be handled
differently.

thx,

Jason.

> +
> +		of_update_property(np, new_compat);
> +	}
> +	return;
> +}
> +
>  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);
>  }
>  
> -- 
> 1.8.1.2
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Jason Gunthorpe]

On Fri, Jan 10, 2014 at 01:22:40PM -0500, Jason Cooper wrote:

> Do we create new compatible strings to indicate errata, or to indicate
> 'from this version forward there are new features'?  The former would
> indicate as Gregory has written '...-a0-i2c', the latter would warrant
> '...-b0-i2c' and disabling offloading if we don't see '...-b0-i2c'.

IMHO the compatible string should represent a specific HW/SW ABI. So
you need a unique compatible string for every variation of that ABI.

We already have a compatible string defined for the ABI that B0
presents.

Jason

[PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Jason Gunthorpe]

On Fri, Jan 10, 2014 at 01:22:40PM -0500, Jason Cooper wrote:

> Do we create new compatible strings to indicate errata, or to indicate
> 'from this version forward there are new features'?  The former would
> indicate as Gregory has written '...-a0-i2c', the latter would warrant
> '...-b0-i2c' and disabling offloading if we don't see '...-b0-i2c'.

IMHO the compatible string should represent a specific HW/SW ABI. So
you need a unique compatible string for every variation of that ABI.

We already have a compatible string defined for the ABI that B0
presents.

Jason

Re: [PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Jason Cooper]

On Fri, Jan 10, 2014 at 12:05:21PM -0700, Jason Gunthorpe wrote:
> On Fri, Jan 10, 2014 at 01:22:40PM -0500, Jason Cooper wrote:
> 
> > Do we create new compatible strings to indicate errata, or to indicate
> > 'from this version forward there are new features'?  The former would
> > indicate as Gregory has written '...-a0-i2c', the latter would warrant
> > '...-b0-i2c' and disabling offloading if we don't see '...-b0-i2c'.

s/-b0-i2c'./-b0-i2c' or newer./

> IMHO the compatible string should represent a specific HW/SW ABI. So
> you need a unique compatible string for every variation of that ABI.

My concern is that we tend to do things like "marvell,orion-sata" for
the first version of the IP block we can work with.  orion5x, kirkwood,
dove, and armada 370/xp all use that compatible string to refer to that
IP block.

Given that we look at it as 'and newer', '...-a0-i2c' would mean no
offloading until we introduce '-b0-i2c'.  Or am I mis-understanding what
you're saying?

> We already have a compatible string defined for the ABI that B0
> presents.

So 'mv78230-i2c' is newer than 'mv78230-a0-i2c', or are you referring to
something else?

thx,

Jason.

[PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Jason Cooper]

On Fri, Jan 10, 2014 at 12:05:21PM -0700, Jason Gunthorpe wrote:
> On Fri, Jan 10, 2014 at 01:22:40PM -0500, Jason Cooper wrote:
> 
> > Do we create new compatible strings to indicate errata, or to indicate
> > 'from this version forward there are new features'?  The former would
> > indicate as Gregory has written '...-a0-i2c', the latter would warrant
> > '...-b0-i2c' and disabling offloading if we don't see '...-b0-i2c'.

s/-b0-i2c'./-b0-i2c' or newer./

> IMHO the compatible string should represent a specific HW/SW ABI. So
> you need a unique compatible string for every variation of that ABI.

My concern is that we tend to do things like "marvell,orion-sata" for
the first version of the IP block we can work with.  orion5x, kirkwood,
dove, and armada 370/xp all use that compatible string to refer to that
IP block.

Given that we look at it as 'and newer', '...-a0-i2c' would mean no
offloading until we introduce '-b0-i2c'.  Or am I mis-understanding what
you're saying?

> We already have a compatible string defined for the ABI that B0
> presents.

So 'mv78230-i2c' is newer than 'mv78230-a0-i2c', or are you referring to
something else?

thx,

Jason.

Re: [PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Jason Cooper]

On Fri, Jan 10, 2014 at 02:45:50PM -0500, Jason Cooper wrote:
> On Fri, Jan 10, 2014 at 12:05:21PM -0700, Jason Gunthorpe wrote:
> > On Fri, Jan 10, 2014 at 01:22:40PM -0500, Jason Cooper wrote:
> > 
> > > Do we create new compatible strings to indicate errata, or to indicate
> > > 'from this version forward there are new features'?  The former would
> > > indicate as Gregory has written '...-a0-i2c', the latter would warrant
> > > '...-b0-i2c' and disabling offloading if we don't see '...-b0-i2c'.
> 
> s/-b0-i2c'./-b0-i2c' or newer./
> 
> > IMHO the compatible string should represent a specific HW/SW ABI. So
> > you need a unique compatible string for every variation of that ABI.
> 
> My concern is that we tend to do things like "marvell,orion-sata" for
> the first version of the IP block we can work with.  orion5x, kirkwood,
> dove, and armada 370/xp all use that compatible string to refer to that
> IP block.
> 
> Given that we look at it as 'and newer', '...-a0-i2c' would mean no
> offloading until we introduce '-b0-i2c'.  Or am I mis-understanding what
> you're saying?
> 
> > We already have a compatible string defined for the ABI that B0
> > presents.
> 
> So 'mv78230-i2c' is newer than 'mv78230-a0-i2c', or are you referring to
> something else?

I think the crux of it is:  Is mv78230-i2c the first, or the default?

thx,

Jason.

[PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Jason Cooper]

On Fri, Jan 10, 2014 at 02:45:50PM -0500, Jason Cooper wrote:
> On Fri, Jan 10, 2014 at 12:05:21PM -0700, Jason Gunthorpe wrote:
> > On Fri, Jan 10, 2014 at 01:22:40PM -0500, Jason Cooper wrote:
> > 
> > > Do we create new compatible strings to indicate errata, or to indicate
> > > 'from this version forward there are new features'?  The former would
> > > indicate as Gregory has written '...-a0-i2c', the latter would warrant
> > > '...-b0-i2c' and disabling offloading if we don't see '...-b0-i2c'.
> 
> s/-b0-i2c'./-b0-i2c' or newer./
> 
> > IMHO the compatible string should represent a specific HW/SW ABI. So
> > you need a unique compatible string for every variation of that ABI.
> 
> My concern is that we tend to do things like "marvell,orion-sata" for
> the first version of the IP block we can work with.  orion5x, kirkwood,
> dove, and armada 370/xp all use that compatible string to refer to that
> IP block.
> 
> Given that we look at it as 'and newer', '...-a0-i2c' would mean no
> offloading until we introduce '-b0-i2c'.  Or am I mis-understanding what
> you're saying?
> 
> > We already have a compatible string defined for the ABI that B0
> > presents.
> 
> So 'mv78230-i2c' is newer than 'mv78230-a0-i2c', or are you referring to
> something else?

I think the crux of it is:  Is mv78230-i2c the first, or the default?

thx,

Jason.

Re: [PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Gregory CLEMENT]

Hi Jason,

On 10/01/2014 20:45, Jason Cooper wrote:
> On Fri, Jan 10, 2014 at 12:05:21PM -0700, Jason Gunthorpe wrote:
>> On Fri, Jan 10, 2014 at 01:22:40PM -0500, Jason Cooper wrote:
>>
>>> Do we create new compatible strings to indicate errata, or to indicate
>>> 'from this version forward there are new features'?  The former would
>>> indicate as Gregory has written '...-a0-i2c', the latter would warrant
>>> '...-b0-i2c' and disabling offloading if we don't see '...-b0-i2c'.
> 
> s/-b0-i2c'./-b0-i2c' or newer./
> 
>> IMHO the compatible string should represent a specific HW/SW ABI. So
>> you need a unique compatible string for every variation of that ABI.
> 
> My concern is that we tend to do things like "marvell,orion-sata" for
> the first version of the IP block we can work with.  orion5x, kirkwood,
> dove, and armada 370/xp all use that compatible string to refer to that
> IP block.
> 
> Given that we look at it as 'and newer', '...-a0-i2c' would mean no
> offloading until we introduce '-b0-i2c'.  Or am I mis-understanding what
> you're saying?
> 
>> We already have a compatible string defined for the ABI that B0
>> presents.
> 
> So 'mv78230-i2c' is newer than 'mv78230-a0-i2c', or are you referring to
> something else?

I think you put too much attention in the name.

There are just name referring a specific hardware. I don't think
there is a consideration of order. For instance this driver also
work with allwinner,sun4i-i2c, here we can clearly see that this
compatible don't describe a newer or an older version of the device
it just describe an "other" version.


About this whole series how do you plan to handle it?
It was acked by Wolfram and even by Arnd.

This series is for fixing a bug so it should be part of the stable
kernels including the 3.13. However we are so close to the release
of the 3.13, that it seems to be too late.

At least I hope it can be pushed to the arm-soc-next and be part of the
3.14-rc1. What do you think about it?


Thanks,

Gregory


> 
> thx,
> 
> Jason.
> 


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

[PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Gregory CLEMENT]

Hi Jason,

On 10/01/2014 20:45, Jason Cooper wrote:
> On Fri, Jan 10, 2014 at 12:05:21PM -0700, Jason Gunthorpe wrote:
>> On Fri, Jan 10, 2014 at 01:22:40PM -0500, Jason Cooper wrote:
>>
>>> Do we create new compatible strings to indicate errata, or to indicate
>>> 'from this version forward there are new features'?  The former would
>>> indicate as Gregory has written '...-a0-i2c', the latter would warrant
>>> '...-b0-i2c' and disabling offloading if we don't see '...-b0-i2c'.
> 
> s/-b0-i2c'./-b0-i2c' or newer./
> 
>> IMHO the compatible string should represent a specific HW/SW ABI. So
>> you need a unique compatible string for every variation of that ABI.
> 
> My concern is that we tend to do things like "marvell,orion-sata" for
> the first version of the IP block we can work with.  orion5x, kirkwood,
> dove, and armada 370/xp all use that compatible string to refer to that
> IP block.
> 
> Given that we look at it as 'and newer', '...-a0-i2c' would mean no
> offloading until we introduce '-b0-i2c'.  Or am I mis-understanding what
> you're saying?
> 
>> We already have a compatible string defined for the ABI that B0
>> presents.
> 
> So 'mv78230-i2c' is newer than 'mv78230-a0-i2c', or are you referring to
> something else?

I think you put too much attention in the name.

There are just name referring a specific hardware. I don't think
there is a consideration of order. For instance this driver also
work with allwinner,sun4i-i2c, here we can clearly see that this
compatible don't describe a newer or an older version of the device
it just describe an "other" version.


About this whole series how do you plan to handle it?
It was acked by Wolfram and even by Arnd.

This series is for fixing a bug so it should be part of the stable
kernels including the 3.13. However we are so close to the release
of the 3.13, that it seems to be too late.

At least I hope it can be pushed to the arm-soc-next and be part of the
3.14-rc1. What do you think about it?


Thanks,

Gregory


> 
> thx,
> 
> Jason.
> 


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

Re: [PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Gregory CLEMENT]

Jason,
On 10/01/2014 21:08, Jason Cooper wrote:
> On Fri, Jan 10, 2014 at 02:45:50PM -0500, Jason Cooper wrote:
>> On Fri, Jan 10, 2014 at 12:05:21PM -0700, Jason Gunthorpe wrote:
>>> On Fri, Jan 10, 2014 at 01:22:40PM -0500, Jason Cooper wrote:
>>>
>>>> Do we create new compatible strings to indicate errata, or to indicate
>>>> 'from this version forward there are new features'?  The former would
>>>> indicate as Gregory has written '...-a0-i2c', the latter would warrant
>>>> '...-b0-i2c' and disabling offloading if we don't see '...-b0-i2c'.
>>
>> s/-b0-i2c'./-b0-i2c' or newer./
>>
>>> IMHO the compatible string should represent a specific HW/SW ABI. So
>>> you need a unique compatible string for every variation of that ABI.
>>
>> My concern is that we tend to do things like "marvell,orion-sata" for
>> the first version of the IP block we can work with.  orion5x, kirkwood,
>> dove, and armada 370/xp all use that compatible string to refer to that
>> IP block.
>>
>> Given that we look at it as 'and newer', '...-a0-i2c' would mean no
>> offloading until we introduce '-b0-i2c'.  Or am I mis-understanding what
>> you're saying?
>>
>>> We already have a compatible string defined for the ABI that B0
>>> presents.
>>
>> So 'mv78230-i2c' is newer than 'mv78230-a0-i2c', or are you referring to
>> something else?
> 
> I think the crux of it is:  Is mv78230-i2c the first, or the default?

Here it's clearly the default

Gregory


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

[PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Gregory CLEMENT]

Jason,
On 10/01/2014 21:08, Jason Cooper wrote:
> On Fri, Jan 10, 2014 at 02:45:50PM -0500, Jason Cooper wrote:
>> On Fri, Jan 10, 2014 at 12:05:21PM -0700, Jason Gunthorpe wrote:
>>> On Fri, Jan 10, 2014 at 01:22:40PM -0500, Jason Cooper wrote:
>>>
>>>> Do we create new compatible strings to indicate errata, or to indicate
>>>> 'from this version forward there are new features'?  The former would
>>>> indicate as Gregory has written '...-a0-i2c', the latter would warrant
>>>> '...-b0-i2c' and disabling offloading if we don't see '...-b0-i2c'.
>>
>> s/-b0-i2c'./-b0-i2c' or newer./
>>
>>> IMHO the compatible string should represent a specific HW/SW ABI. So
>>> you need a unique compatible string for every variation of that ABI.
>>
>> My concern is that we tend to do things like "marvell,orion-sata" for
>> the first version of the IP block we can work with.  orion5x, kirkwood,
>> dove, and armada 370/xp all use that compatible string to refer to that
>> IP block.
>>
>> Given that we look at it as 'and newer', '...-a0-i2c' would mean no
>> offloading until we introduce '-b0-i2c'.  Or am I mis-understanding what
>> you're saying?
>>
>>> We already have a compatible string defined for the ABI that B0
>>> presents.
>>
>> So 'mv78230-i2c' is newer than 'mv78230-a0-i2c', or are you referring to
>> something else?
> 
> I think the crux of it is:  Is mv78230-i2c the first, or the default?

Here it's clearly the default

Gregory


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

Re: [PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Jason Gunthorpe]

On Fri, Jan 10, 2014 at 02:45:50PM -0500, Jason Cooper wrote:

> > IMHO the compatible string should represent a specific HW/SW ABI. So
> > you need a unique compatible string for every variation of that ABI.
> 
> My concern is that we tend to do things like "marvell,orion-sata" for
> the first version of the IP block we can work with.  orion5x, kirkwood,
> dove, and armada 370/xp all use that compatible string to refer to that
> IP block.

Right, absent guidance from the originators of the IP block that is
sort of all we are left with. But something like 'marvell,orion-sata'
is just a label to describe the ABI implemented by the HW on that
particular chip.

> Given that we look at it as 'and newer', '...-a0-i2c' would mean no
> offloading until we introduce '-b0-i2c'.  Or am I mis-understanding what
> you're saying?

I would stop thinking of this in terms of 'is newer' / 'is older'.

marvell,orion-sata means any HW that implements the same ABI as the
orion chip. 

When we get a different chip that has a compatible, but extended ABI
we introduce a new label:

 compatible = "marvell,foobar-sata", "marvell,orion-sata";

And if we get one that has a very similar, but incompatible ABI, we
still introduce a new label:

 compatible = "marvell,foobar2-sata";

And everything works properly.

> > We already have a compatible string defined for the ABI that B0
> > presents.
> 
> So 'mv78230-i2c' is newer than 'mv78230-a0-i2c', or are you referring to
> something else?

'mv78230-i2c' is the name that was picked to describe the ABI that
works as-documented in the manual
'mv78230-a0-i2c' is the name that was picked to describe the ABI that
works as-implemented in the A0 chip :)

There is no newer/older, they are just two different ABIs.

I guess it turns out that 'mv78230-a0-i2c' is a strict compatible
subset of 'mv78230-i2c' - but that doesn't really make a difference.

The 'mv78230-i2c' driver is guarenteed avaialble in all places where
the 'mv78230-a0-i2c' driver would be available.

Jason

[PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Jason Gunthorpe]

On Fri, Jan 10, 2014 at 02:45:50PM -0500, Jason Cooper wrote:

> > IMHO the compatible string should represent a specific HW/SW ABI. So
> > you need a unique compatible string for every variation of that ABI.
> 
> My concern is that we tend to do things like "marvell,orion-sata" for
> the first version of the IP block we can work with.  orion5x, kirkwood,
> dove, and armada 370/xp all use that compatible string to refer to that
> IP block.

Right, absent guidance from the originators of the IP block that is
sort of all we are left with. But something like 'marvell,orion-sata'
is just a label to describe the ABI implemented by the HW on that
particular chip.

> Given that we look at it as 'and newer', '...-a0-i2c' would mean no
> offloading until we introduce '-b0-i2c'.  Or am I mis-understanding what
> you're saying?

I would stop thinking of this in terms of 'is newer' / 'is older'.

marvell,orion-sata means any HW that implements the same ABI as the
orion chip. 

When we get a different chip that has a compatible, but extended ABI
we introduce a new label:

 compatible = "marvell,foobar-sata", "marvell,orion-sata";

And if we get one that has a very similar, but incompatible ABI, we
still introduce a new label:

 compatible = "marvell,foobar2-sata";

And everything works properly.

> > We already have a compatible string defined for the ABI that B0
> > presents.
> 
> So 'mv78230-i2c' is newer than 'mv78230-a0-i2c', or are you referring to
> something else?

'mv78230-i2c' is the name that was picked to describe the ABI that
works as-documented in the manual
'mv78230-a0-i2c' is the name that was picked to describe the ABI that
works as-implemented in the A0 chip :)

There is no newer/older, they are just two different ABIs.

I guess it turns out that 'mv78230-a0-i2c' is a strict compatible
subset of 'mv78230-i2c' - but that doesn't really make a difference.

The 'mv78230-i2c' driver is guarenteed avaialble in all places where
the 'mv78230-a0-i2c' driver would be available.

Jason

Re: [PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Jason Cooper]

On Fri, Jan 10, 2014 at 09:09:09PM +0100, Gregory CLEMENT wrote:
> Hi Jason,
> 
> On 10/01/2014 20:45, Jason Cooper wrote:
> > On Fri, Jan 10, 2014 at 12:05:21PM -0700, Jason Gunthorpe wrote:
> >> On Fri, Jan 10, 2014 at 01:22:40PM -0500, Jason Cooper wrote:
> >>
> >>> Do we create new compatible strings to indicate errata, or to indicate
> >>> 'from this version forward there are new features'?  The former would
> >>> indicate as Gregory has written '...-a0-i2c', the latter would warrant
> >>> '...-b0-i2c' and disabling offloading if we don't see '...-b0-i2c'.
> > 
> > s/-b0-i2c'./-b0-i2c' or newer./
> > 
> >> IMHO the compatible string should represent a specific HW/SW ABI. So
> >> you need a unique compatible string for every variation of that ABI.
> > 
> > My concern is that we tend to do things like "marvell,orion-sata" for
> > the first version of the IP block we can work with.  orion5x, kirkwood,
> > dove, and armada 370/xp all use that compatible string to refer to that
> > IP block.
> > 
> > Given that we look at it as 'and newer', '...-a0-i2c' would mean no
> > offloading until we introduce '-b0-i2c'.  Or am I mis-understanding what
> > you're saying?
> > 
> >> We already have a compatible string defined for the ABI that B0
> >> presents.
> > 
> > So 'mv78230-i2c' is newer than 'mv78230-a0-i2c', or are you referring to
> > something else?
> 
> I think you put too much attention in the name.

Sure, I might be bikeshedding, but...

> There are just name referring a specific hardware. I don't think
> there is a consideration of order. For instance this driver also
> work with allwinner,sun4i-i2c, here we can clearly see that this
> compatible don't describe a newer or an older version of the device
> it just describe an "other" version.

No one has said "EPAPR requires compatible string heirarchies to be
handled as such."  Or, "EPAPR doesn't specify, so we choose to treat
them in such-and-such manner."

The point I'm trying to highlight in this case is that we aren't clear
on how we code compatible strings.  Yes, we use the most specific
compatible in the list, which is typically the first one.  What we
haven't cleared up is how to handle a newer IP with an older compatible
string.

eg: we boot an -a0 SoC, the dtb has the i2c node with most specific
compatible string 'mv78230-a0-i2c'.  Obviously, we disable offload.
What do we do with an older DTB which only has 'mv78230-i2c'? [see 1]

now we're running on a -b0 SoC.  We boot with a DTB that has a node
'mv78230-i2c'.  Is this an old DTB?  Or is this a new DTB written with
the understanding that -a0-i2c is an exception?  You see?  We aren't
being definitive.  The kernel really doesn't know for certain whether it
should enable offloading in this case or not.

[1] Yes, for Armada i2c, we can retrieve the CPU revision to wade out of
this grey area.  I'm just looking for clarification regarding the
general DT handling of this scenario.

I'm reluctant to push this series until I have an answer because the
answer will change the meaning of 'mv78230-i2c'.  Which *is* relevant to
this series.

Personally, I feel that since 'mv78230-i2c' has been used to refer to A0
revision of the IP, we should treat any nodes using it (as it's most
specific string) as having broken offloading.  Which implies that
-b0-i2c should be used to indicate IPs with working offloading.

I swear, I'm starting to feel like Russell...  Maybe I'm just grumpy
after crawling out from under my email backlog...

> About this whole series how do you plan to handle it?
> It was acked by Wolfram and even by Arnd.
> 
> This series is for fixing a bug so it should be part of the stable
> kernels including the 3.13. However we are so close to the release
> of the 3.13, that it seems to be too late.

Yes, it's a fix, and it'll get in.  It might be too late for v3.13, but
it'll get in to v3.13.1 (and the other appropriate stable kernels).  I
don't like to rush fixes just because of a pending merge window.

thx,

Jason.

> At least I hope it can be pushed to the arm-soc-next and be part of the
> 3.14-rc1. What do you think about it?
> 
> 
> Thanks,
> 
> Gregory
> 
> 
> > 
> > thx,
> > 
> > Jason.
> > 
> 
> 
> -- 
> Gregory Clement, Free Electrons
> Kernel, drivers, real-time and embedded Linux
> development, consulting, training and support.
> http://free-electrons.com

[PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Jason Cooper]

On Fri, Jan 10, 2014 at 09:09:09PM +0100, Gregory CLEMENT wrote:
> Hi Jason,
> 
> On 10/01/2014 20:45, Jason Cooper wrote:
> > On Fri, Jan 10, 2014 at 12:05:21PM -0700, Jason Gunthorpe wrote:
> >> On Fri, Jan 10, 2014 at 01:22:40PM -0500, Jason Cooper wrote:
> >>
> >>> Do we create new compatible strings to indicate errata, or to indicate
> >>> 'from this version forward there are new features'?  The former would
> >>> indicate as Gregory has written '...-a0-i2c', the latter would warrant
> >>> '...-b0-i2c' and disabling offloading if we don't see '...-b0-i2c'.
> > 
> > s/-b0-i2c'./-b0-i2c' or newer./
> > 
> >> IMHO the compatible string should represent a specific HW/SW ABI. So
> >> you need a unique compatible string for every variation of that ABI.
> > 
> > My concern is that we tend to do things like "marvell,orion-sata" for
> > the first version of the IP block we can work with.  orion5x, kirkwood,
> > dove, and armada 370/xp all use that compatible string to refer to that
> > IP block.
> > 
> > Given that we look at it as 'and newer', '...-a0-i2c' would mean no
> > offloading until we introduce '-b0-i2c'.  Or am I mis-understanding what
> > you're saying?
> > 
> >> We already have a compatible string defined for the ABI that B0
> >> presents.
> > 
> > So 'mv78230-i2c' is newer than 'mv78230-a0-i2c', or are you referring to
> > something else?
> 
> I think you put too much attention in the name.

Sure, I might be bikeshedding, but...

> There are just name referring a specific hardware. I don't think
> there is a consideration of order. For instance this driver also
> work with allwinner,sun4i-i2c, here we can clearly see that this
> compatible don't describe a newer or an older version of the device
> it just describe an "other" version.

No one has said "EPAPR requires compatible string heirarchies to be
handled as such."  Or, "EPAPR doesn't specify, so we choose to treat
them in such-and-such manner."

The point I'm trying to highlight in this case is that we aren't clear
on how we code compatible strings.  Yes, we use the most specific
compatible in the list, which is typically the first one.  What we
haven't cleared up is how to handle a newer IP with an older compatible
string.

eg: we boot an -a0 SoC, the dtb has the i2c node with most specific
compatible string 'mv78230-a0-i2c'.  Obviously, we disable offload.
What do we do with an older DTB which only has 'mv78230-i2c'? [see 1]

now we're running on a -b0 SoC.  We boot with a DTB that has a node
'mv78230-i2c'.  Is this an old DTB?  Or is this a new DTB written with
the understanding that -a0-i2c is an exception?  You see?  We aren't
being definitive.  The kernel really doesn't know for certain whether it
should enable offloading in this case or not.

[1] Yes, for Armada i2c, we can retrieve the CPU revision to wade out of
this grey area.  I'm just looking for clarification regarding the
general DT handling of this scenario.

I'm reluctant to push this series until I have an answer because the
answer will change the meaning of 'mv78230-i2c'.  Which *is* relevant to
this series.

Personally, I feel that since 'mv78230-i2c' has been used to refer to A0
revision of the IP, we should treat any nodes using it (as it's most
specific string) as having broken offloading.  Which implies that
-b0-i2c should be used to indicate IPs with working offloading.

I swear, I'm starting to feel like Russell...  Maybe I'm just grumpy
after crawling out from under my email backlog...

> About this whole series how do you plan to handle it?
> It was acked by Wolfram and even by Arnd.
> 
> This series is for fixing a bug so it should be part of the stable
> kernels including the 3.13. However we are so close to the release
> of the 3.13, that it seems to be too late.

Yes, it's a fix, and it'll get in.  It might be too late for v3.13, but
it'll get in to v3.13.1 (and the other appropriate stable kernels).  I
don't like to rush fixes just because of a pending merge window.

thx,

Jason.

> At least I hope it can be pushed to the arm-soc-next and be part of the
> 3.14-rc1. What do you think about it?
> 
> 
> Thanks,
> 
> Gregory
> 
> 
> > 
> > thx,
> > 
> > Jason.
> > 
> 
> 
> -- 
> Gregory Clement, Free Electrons
> Kernel, drivers, real-time and embedded Linux
> development, consulting, training and support.
> http://free-electrons.com

Re: [PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Jason Cooper]

On Fri, Jan 10, 2014 at 09:12:41PM +0100, Gregory CLEMENT wrote:
> Jason,
> On 10/01/2014 21:08, Jason Cooper wrote:
> > On Fri, Jan 10, 2014 at 02:45:50PM -0500, Jason Cooper wrote:
> >> On Fri, Jan 10, 2014 at 12:05:21PM -0700, Jason Gunthorpe wrote:
> >>> On Fri, Jan 10, 2014 at 01:22:40PM -0500, Jason Cooper wrote:
> >>>
> >>>> Do we create new compatible strings to indicate errata, or to indicate
> >>>> 'from this version forward there are new features'?  The former would
> >>>> indicate as Gregory has written '...-a0-i2c', the latter would warrant
> >>>> '...-b0-i2c' and disabling offloading if we don't see '...-b0-i2c'.
> >>
> >> s/-b0-i2c'./-b0-i2c' or newer./
> >>
> >>> IMHO the compatible string should represent a specific HW/SW ABI. So
> >>> you need a unique compatible string for every variation of that ABI.
> >>
> >> My concern is that we tend to do things like "marvell,orion-sata" for
> >> the first version of the IP block we can work with.  orion5x, kirkwood,
> >> dove, and armada 370/xp all use that compatible string to refer to that
> >> IP block.
> >>
> >> Given that we look at it as 'and newer', '...-a0-i2c' would mean no
> >> offloading until we introduce '-b0-i2c'.  Or am I mis-understanding what
> >> you're saying?
> >>
> >>> We already have a compatible string defined for the ABI that B0
> >>> presents.
> >>
> >> So 'mv78230-i2c' is newer than 'mv78230-a0-i2c', or are you referring to
> >> something else?
> > 
> > I think the crux of it is:  Is mv78230-i2c the first, or the default?
> 
> Here it's clearly the default

So we should default to no offloading when we see it?  Since it has been
deployed referring to -a0 revision i2c IP blocks?

thx,

Jason.

[PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Jason Cooper]

On Fri, Jan 10, 2014 at 09:12:41PM +0100, Gregory CLEMENT wrote:
> Jason,
> On 10/01/2014 21:08, Jason Cooper wrote:
> > On Fri, Jan 10, 2014 at 02:45:50PM -0500, Jason Cooper wrote:
> >> On Fri, Jan 10, 2014 at 12:05:21PM -0700, Jason Gunthorpe wrote:
> >>> On Fri, Jan 10, 2014 at 01:22:40PM -0500, Jason Cooper wrote:
> >>>
> >>>> Do we create new compatible strings to indicate errata, or to indicate
> >>>> 'from this version forward there are new features'?  The former would
> >>>> indicate as Gregory has written '...-a0-i2c', the latter would warrant
> >>>> '...-b0-i2c' and disabling offloading if we don't see '...-b0-i2c'.
> >>
> >> s/-b0-i2c'./-b0-i2c' or newer./
> >>
> >>> IMHO the compatible string should represent a specific HW/SW ABI. So
> >>> you need a unique compatible string for every variation of that ABI.
> >>
> >> My concern is that we tend to do things like "marvell,orion-sata" for
> >> the first version of the IP block we can work with.  orion5x, kirkwood,
> >> dove, and armada 370/xp all use that compatible string to refer to that
> >> IP block.
> >>
> >> Given that we look at it as 'and newer', '...-a0-i2c' would mean no
> >> offloading until we introduce '-b0-i2c'.  Or am I mis-understanding what
> >> you're saying?
> >>
> >>> We already have a compatible string defined for the ABI that B0
> >>> presents.
> >>
> >> So 'mv78230-i2c' is newer than 'mv78230-a0-i2c', or are you referring to
> >> something else?
> > 
> > I think the crux of it is:  Is mv78230-i2c the first, or the default?
> 
> Here it's clearly the default

So we should default to no offloading when we see it?  Since it has been
deployed referring to -a0 revision i2c IP blocks?

thx,

Jason.

Re: [PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Jason Cooper]

On Fri, Jan 10, 2014 at 02:06:34PM -0700, Jason Gunthorpe wrote:
> On Fri, Jan 10, 2014 at 02:45:50PM -0500, Jason Cooper wrote:
> 
> > > IMHO the compatible string should represent a specific HW/SW ABI. So
> > > you need a unique compatible string for every variation of that ABI.
> > 
> > My concern is that we tend to do things like "marvell,orion-sata" for
> > the first version of the IP block we can work with.  orion5x, kirkwood,
> > dove, and armada 370/xp all use that compatible string to refer to that
> > IP block.
> 
> Right, absent guidance from the originators of the IP block that is
> sort of all we are left with. But something like 'marvell,orion-sata'
> is just a label to describe the ABI implemented by the HW on that
> particular chip.
> 
> > Given that we look at it as 'and newer', '...-a0-i2c' would mean no
> > offloading until we introduce '-b0-i2c'.  Or am I mis-understanding what
> > you're saying?
> 
> I would stop thinking of this in terms of 'is newer' / 'is older'.
> 
> marvell,orion-sata means any HW that implements the same ABI as the
> orion chip. 
> 
> When we get a different chip that has a compatible, but extended ABI
> we introduce a new label:
> 
>  compatible = "marvell,foobar-sata", "marvell,orion-sata";
> 
> And if we get one that has a very similar, but incompatible ABI, we
> still introduce a new label:
> 
>  compatible = "marvell,foobar2-sata";
> 
> And everything works properly.
> 
> > > We already have a compatible string defined for the ABI that B0
> > > presents.
> > 
> > So 'mv78230-i2c' is newer than 'mv78230-a0-i2c', or are you referring to
> > something else?
> 
> 'mv78230-i2c' is the name that was picked to describe the ABI that
> works as-documented in the manual
> 'mv78230-a0-i2c' is the name that was picked to describe the ABI that
> works as-implemented in the A0 chip :)

Ok, so my only outstanding concern is that 'mv78230-i2c' has been used
to refer to the A0 revision of the i2c IP block (v3.12).  If we now
change the meaning to be "as documented" then we get broken systems.

thx,

Jason.

> There is no newer/older, they are just two different ABIs.
> 
> I guess it turns out that 'mv78230-a0-i2c' is a strict compatible
> subset of 'mv78230-i2c' - but that doesn't really make a difference.
> 
> The 'mv78230-i2c' driver is guarenteed avaialble in all places where
> the 'mv78230-a0-i2c' driver would be available.
> 
> Jason

[PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Jason Cooper]

On Fri, Jan 10, 2014 at 02:06:34PM -0700, Jason Gunthorpe wrote:
> On Fri, Jan 10, 2014 at 02:45:50PM -0500, Jason Cooper wrote:
> 
> > > IMHO the compatible string should represent a specific HW/SW ABI. So
> > > you need a unique compatible string for every variation of that ABI.
> > 
> > My concern is that we tend to do things like "marvell,orion-sata" for
> > the first version of the IP block we can work with.  orion5x, kirkwood,
> > dove, and armada 370/xp all use that compatible string to refer to that
> > IP block.
> 
> Right, absent guidance from the originators of the IP block that is
> sort of all we are left with. But something like 'marvell,orion-sata'
> is just a label to describe the ABI implemented by the HW on that
> particular chip.
> 
> > Given that we look at it as 'and newer', '...-a0-i2c' would mean no
> > offloading until we introduce '-b0-i2c'.  Or am I mis-understanding what
> > you're saying?
> 
> I would stop thinking of this in terms of 'is newer' / 'is older'.
> 
> marvell,orion-sata means any HW that implements the same ABI as the
> orion chip. 
> 
> When we get a different chip that has a compatible, but extended ABI
> we introduce a new label:
> 
>  compatible = "marvell,foobar-sata", "marvell,orion-sata";
> 
> And if we get one that has a very similar, but incompatible ABI, we
> still introduce a new label:
> 
>  compatible = "marvell,foobar2-sata";
> 
> And everything works properly.
> 
> > > We already have a compatible string defined for the ABI that B0
> > > presents.
> > 
> > So 'mv78230-i2c' is newer than 'mv78230-a0-i2c', or are you referring to
> > something else?
> 
> 'mv78230-i2c' is the name that was picked to describe the ABI that
> works as-documented in the manual
> 'mv78230-a0-i2c' is the name that was picked to describe the ABI that
> works as-implemented in the A0 chip :)

Ok, so my only outstanding concern is that 'mv78230-i2c' has been used
to refer to the A0 revision of the i2c IP block (v3.12).  If we now
change the meaning to be "as documented" then we get broken systems.

thx,

Jason.

> There is no newer/older, they are just two different ABIs.
> 
> I guess it turns out that 'mv78230-a0-i2c' is a strict compatible
> subset of 'mv78230-i2c' - but that doesn't really make a difference.
> 
> The 'mv78230-i2c' driver is guarenteed avaialble in all places where
> the 'mv78230-a0-i2c' driver would be available.
> 
> Jason

Re: [PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Gregory CLEMENT]

On 10/01/2014 22:14, Jason Cooper wrote:
> On Fri, Jan 10, 2014 at 09:12:41PM +0100, Gregory CLEMENT wrote:
>> Jason,
>> On 10/01/2014 21:08, Jason Cooper wrote:
>>> On Fri, Jan 10, 2014 at 02:45:50PM -0500, Jason Cooper wrote:
>>>> On Fri, Jan 10, 2014 at 12:05:21PM -0700, Jason Gunthorpe wrote:
>>>>> On Fri, Jan 10, 2014 at 01:22:40PM -0500, Jason Cooper wrote:
>>>>>
>>>>>> Do we create new compatible strings to indicate errata, or to indicate
>>>>>> 'from this version forward there are new features'?  The former would
>>>>>> indicate as Gregory has written '...-a0-i2c', the latter would warrant
>>>>>> '...-b0-i2c' and disabling offloading if we don't see '...-b0-i2c'.
>>>>
>>>> s/-b0-i2c'./-b0-i2c' or newer./
>>>>
>>>>> IMHO the compatible string should represent a specific HW/SW ABI. So
>>>>> you need a unique compatible string for every variation of that ABI.
>>>>
>>>> My concern is that we tend to do things like "marvell,orion-sata" for
>>>> the first version of the IP block we can work with.  orion5x, kirkwood,
>>>> dove, and armada 370/xp all use that compatible string to refer to that
>>>> IP block.
>>>>
>>>> Given that we look at it as 'and newer', '...-a0-i2c' would mean no
>>>> offloading until we introduce '-b0-i2c'.  Or am I mis-understanding what
>>>> you're saying?
>>>>
>>>>> We already have a compatible string defined for the ABI that B0
>>>>> presents.
>>>>
>>>> So 'mv78230-i2c' is newer than 'mv78230-a0-i2c', or are you referring to
>>>> something else?
>>>
>>> I think the crux of it is:  Is mv78230-i2c the first, or the default?
>>
>> Here it's clearly the default
> 
> So we should default to no offloading when we see it?  Since it has been
> deployed referring to -a0 revision i2c IP blocks?
> 

But this assumption is wrong as I already wrote few days ago, mv78230-i2c
has been deployed referring to -b0 revision i2c IP blocks since the begining.

It was developed on and for B0 version, and this compatible was created for
this specific version. It was latter that we realized that it was not fully
compatible with A0. But for sure:

mv78230-i2c == I2C IP running on Armada XP B0 (or latter)

> thx,
> 
> Jason.
> 


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

[PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Gregory CLEMENT]

On 10/01/2014 22:14, Jason Cooper wrote:
> On Fri, Jan 10, 2014 at 09:12:41PM +0100, Gregory CLEMENT wrote:
>> Jason,
>> On 10/01/2014 21:08, Jason Cooper wrote:
>>> On Fri, Jan 10, 2014 at 02:45:50PM -0500, Jason Cooper wrote:
>>>> On Fri, Jan 10, 2014 at 12:05:21PM -0700, Jason Gunthorpe wrote:
>>>>> On Fri, Jan 10, 2014 at 01:22:40PM -0500, Jason Cooper wrote:
>>>>>
>>>>>> Do we create new compatible strings to indicate errata, or to indicate
>>>>>> 'from this version forward there are new features'?  The former would
>>>>>> indicate as Gregory has written '...-a0-i2c', the latter would warrant
>>>>>> '...-b0-i2c' and disabling offloading if we don't see '...-b0-i2c'.
>>>>
>>>> s/-b0-i2c'./-b0-i2c' or newer./
>>>>
>>>>> IMHO the compatible string should represent a specific HW/SW ABI. So
>>>>> you need a unique compatible string for every variation of that ABI.
>>>>
>>>> My concern is that we tend to do things like "marvell,orion-sata" for
>>>> the first version of the IP block we can work with.  orion5x, kirkwood,
>>>> dove, and armada 370/xp all use that compatible string to refer to that
>>>> IP block.
>>>>
>>>> Given that we look at it as 'and newer', '...-a0-i2c' would mean no
>>>> offloading until we introduce '-b0-i2c'.  Or am I mis-understanding what
>>>> you're saying?
>>>>
>>>>> We already have a compatible string defined for the ABI that B0
>>>>> presents.
>>>>
>>>> So 'mv78230-i2c' is newer than 'mv78230-a0-i2c', or are you referring to
>>>> something else?
>>>
>>> I think the crux of it is:  Is mv78230-i2c the first, or the default?
>>
>> Here it's clearly the default
> 
> So we should default to no offloading when we see it?  Since it has been
> deployed referring to -a0 revision i2c IP blocks?
> 

But this assumption is wrong as I already wrote few days ago, mv78230-i2c
has been deployed referring to -b0 revision i2c IP blocks since the begining.

It was developed on and for B0 version, and this compatible was created for
this specific version. It was latter that we realized that it was not fully
compatible with A0. But for sure:

mv78230-i2c == I2C IP running on Armada XP B0 (or latter)

> thx,
> 
> Jason.
> 


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

Re: [PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Jason Cooper]

On Fri, Jan 10, 2014 at 10:21:29PM +0100, Gregory CLEMENT wrote:
> On 10/01/2014 22:14, Jason Cooper wrote:
> > On Fri, Jan 10, 2014 at 09:12:41PM +0100, Gregory CLEMENT wrote:
> >> Jason,
> >> On 10/01/2014 21:08, Jason Cooper wrote:
> >>> On Fri, Jan 10, 2014 at 02:45:50PM -0500, Jason Cooper wrote:
> >>>> On Fri, Jan 10, 2014 at 12:05:21PM -0700, Jason Gunthorpe wrote:
> >>>>> On Fri, Jan 10, 2014 at 01:22:40PM -0500, Jason Cooper wrote:
> >>>>>
> >>>>>> Do we create new compatible strings to indicate errata, or to indicate
> >>>>>> 'from this version forward there are new features'?  The former would
> >>>>>> indicate as Gregory has written '...-a0-i2c', the latter would warrant
> >>>>>> '...-b0-i2c' and disabling offloading if we don't see '...-b0-i2c'.
> >>>>
> >>>> s/-b0-i2c'./-b0-i2c' or newer./
> >>>>
> >>>>> IMHO the compatible string should represent a specific HW/SW ABI. So
> >>>>> you need a unique compatible string for every variation of that ABI.
> >>>>
> >>>> My concern is that we tend to do things like "marvell,orion-sata" for
> >>>> the first version of the IP block we can work with.  orion5x, kirkwood,
> >>>> dove, and armada 370/xp all use that compatible string to refer to that
> >>>> IP block.
> >>>>
> >>>> Given that we look at it as 'and newer', '...-a0-i2c' would mean no
> >>>> offloading until we introduce '-b0-i2c'.  Or am I mis-understanding what
> >>>> you're saying?
> >>>>
> >>>>> We already have a compatible string defined for the ABI that B0
> >>>>> presents.
> >>>>
> >>>> So 'mv78230-i2c' is newer than 'mv78230-a0-i2c', or are you referring to
> >>>> something else?
> >>>
> >>> I think the crux of it is:  Is mv78230-i2c the first, or the default?
> >>
> >> Here it's clearly the default
> > 
> > So we should default to no offloading when we see it?  Since it has been
> > deployed referring to -a0 revision i2c IP blocks?
> > 
> 
> But this assumption is wrong as I already wrote few days ago, mv78230-i2c
> has been deployed referring to -b0 revision i2c IP blocks since the begining.

Ok, sorry.  As I wrote on irc last week, I've been on travel and haven't
been able to fully digest everything coming in.  My re-read of all the
threads regarding this this morning didn't catch it.

> It was developed on and for B0 version, and this compatible was created for
> this specific version. It was latter that we realized that it was not fully
> compatible with A0. But for sure:
> 
> mv78230-i2c == I2C IP running on Armada XP B0 (or latter)

Ok, this still feels counter-intuitive, and folks not familiar with the
development process might assume the opposite.  So I'll reply to 4/4
with a reword to make your above statement an explicit part of the
binding documentation.  No need to do another patch version.  I'll fix
it up when I pull it in if you're ok with it.

thx,

Jason.

[PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Jason Cooper]

On Fri, Jan 10, 2014 at 10:21:29PM +0100, Gregory CLEMENT wrote:
> On 10/01/2014 22:14, Jason Cooper wrote:
> > On Fri, Jan 10, 2014 at 09:12:41PM +0100, Gregory CLEMENT wrote:
> >> Jason,
> >> On 10/01/2014 21:08, Jason Cooper wrote:
> >>> On Fri, Jan 10, 2014 at 02:45:50PM -0500, Jason Cooper wrote:
> >>>> On Fri, Jan 10, 2014 at 12:05:21PM -0700, Jason Gunthorpe wrote:
> >>>>> On Fri, Jan 10, 2014 at 01:22:40PM -0500, Jason Cooper wrote:
> >>>>>
> >>>>>> Do we create new compatible strings to indicate errata, or to indicate
> >>>>>> 'from this version forward there are new features'?  The former would
> >>>>>> indicate as Gregory has written '...-a0-i2c', the latter would warrant
> >>>>>> '...-b0-i2c' and disabling offloading if we don't see '...-b0-i2c'.
> >>>>
> >>>> s/-b0-i2c'./-b0-i2c' or newer./
> >>>>
> >>>>> IMHO the compatible string should represent a specific HW/SW ABI. So
> >>>>> you need a unique compatible string for every variation of that ABI.
> >>>>
> >>>> My concern is that we tend to do things like "marvell,orion-sata" for
> >>>> the first version of the IP block we can work with.  orion5x, kirkwood,
> >>>> dove, and armada 370/xp all use that compatible string to refer to that
> >>>> IP block.
> >>>>
> >>>> Given that we look at it as 'and newer', '...-a0-i2c' would mean no
> >>>> offloading until we introduce '-b0-i2c'.  Or am I mis-understanding what
> >>>> you're saying?
> >>>>
> >>>>> We already have a compatible string defined for the ABI that B0
> >>>>> presents.
> >>>>
> >>>> So 'mv78230-i2c' is newer than 'mv78230-a0-i2c', or are you referring to
> >>>> something else?
> >>>
> >>> I think the crux of it is:  Is mv78230-i2c the first, or the default?
> >>
> >> Here it's clearly the default
> > 
> > So we should default to no offloading when we see it?  Since it has been
> > deployed referring to -a0 revision i2c IP blocks?
> > 
> 
> But this assumption is wrong as I already wrote few days ago, mv78230-i2c
> has been deployed referring to -b0 revision i2c IP blocks since the begining.

Ok, sorry.  As I wrote on irc last week, I've been on travel and haven't
been able to fully digest everything coming in.  My re-read of all the
threads regarding this this morning didn't catch it.

> It was developed on and for B0 version, and this compatible was created for
> this specific version. It was latter that we realized that it was not fully
> compatible with A0. But for sure:
> 
> mv78230-i2c == I2C IP running on Armada XP B0 (or latter)

Ok, this still feels counter-intuitive, and folks not familiar with the
development process might assume the opposite.  So I'll reply to 4/4
with a reword to make your above statement an explicit part of the
binding documentation.  No need to do another patch version.  I'll fix
it up when I pull it in if you're ok with it.

thx,

Jason.

Re: [PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Gregory CLEMENT]

On 10/01/2014 22:37, Jason Cooper wrote:
> On Fri, Jan 10, 2014 at 10:21:29PM +0100, Gregory CLEMENT wrote:
>> On 10/01/2014 22:14, Jason Cooper wrote:
>>> On Fri, Jan 10, 2014 at 09:12:41PM +0100, Gregory CLEMENT wrote:
>>>> Jason,
>>>> On 10/01/2014 21:08, Jason Cooper wrote:
>>>>> On Fri, Jan 10, 2014 at 02:45:50PM -0500, Jason Cooper wrote:
>>>>>> On Fri, Jan 10, 2014 at 12:05:21PM -0700, Jason Gunthorpe wrote:
>>>>>>> On Fri, Jan 10, 2014 at 01:22:40PM -0500, Jason Cooper wrote:
>>>>>>>
>>>>>>>> Do we create new compatible strings to indicate errata, or to indicate
>>>>>>>> 'from this version forward there are new features'?  The former would
>>>>>>>> indicate as Gregory has written '...-a0-i2c', the latter would warrant
>>>>>>>> '...-b0-i2c' and disabling offloading if we don't see '...-b0-i2c'.
>>>>>>
>>>>>> s/-b0-i2c'./-b0-i2c' or newer./
>>>>>>
>>>>>>> IMHO the compatible string should represent a specific HW/SW ABI. So
>>>>>>> you need a unique compatible string for every variation of that ABI.
>>>>>>
>>>>>> My concern is that we tend to do things like "marvell,orion-sata" for
>>>>>> the first version of the IP block we can work with.  orion5x, kirkwood,
>>>>>> dove, and armada 370/xp all use that compatible string to refer to that
>>>>>> IP block.
>>>>>>
>>>>>> Given that we look at it as 'and newer', '...-a0-i2c' would mean no
>>>>>> offloading until we introduce '-b0-i2c'.  Or am I mis-understanding what
>>>>>> you're saying?
>>>>>>
>>>>>>> We already have a compatible string defined for the ABI that B0
>>>>>>> presents.
>>>>>>
>>>>>> So 'mv78230-i2c' is newer than 'mv78230-a0-i2c', or are you referring to
>>>>>> something else?
>>>>>
>>>>> I think the crux of it is:  Is mv78230-i2c the first, or the default?
>>>>
>>>> Here it's clearly the default
>>>
>>> So we should default to no offloading when we see it?  Since it has been
>>> deployed referring to -a0 revision i2c IP blocks?
>>>
>>
>> But this assumption is wrong as I already wrote few days ago, mv78230-i2c
>> has been deployed referring to -b0 revision i2c IP blocks since the begining.
> 
> Ok, sorry.  As I wrote on irc last week, I've been on travel and haven't
> been able to fully digest everything coming in.  My re-read of all the
> threads regarding this this morning didn't catch it.

No problem there was a lot of email just for this simple fix!

> 
>> It was developed on and for B0 version, and this compatible was created for
>> this specific version. It was latter that we realized that it was not fully
>> compatible with A0. But for sure:
>>
>> mv78230-i2c == I2C IP running on Armada XP B0 (or latter)
> 
> Ok, this still feels counter-intuitive, and folks not familiar with the
> development process might assume the opposite.  So I'll reply to 4/4
> with a reword to make your above statement an explicit part of the
> binding documentation.  No need to do another patch version.  I'll fix
> it up when I pull it in if you're ok with it.

Please use my branch because as I wrote you:
https://github.com/MISL-EBU-System-SW/mainline-public/tree/i2c-mv64xxx-3.13-rc6-fixes-v6

I took into account the last minor review from Arnd and Wolfram, and I
also updated all the flags acked-by and reported-by. But if you prefer
I can just sent a 6th version on the mailing list.

Thanks,

Gregory


> 
> thx,
> 
> Jason.
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 


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

[PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Gregory CLEMENT]

On 10/01/2014 22:37, Jason Cooper wrote:
> On Fri, Jan 10, 2014 at 10:21:29PM +0100, Gregory CLEMENT wrote:
>> On 10/01/2014 22:14, Jason Cooper wrote:
>>> On Fri, Jan 10, 2014 at 09:12:41PM +0100, Gregory CLEMENT wrote:
>>>> Jason,
>>>> On 10/01/2014 21:08, Jason Cooper wrote:
>>>>> On Fri, Jan 10, 2014 at 02:45:50PM -0500, Jason Cooper wrote:
>>>>>> On Fri, Jan 10, 2014 at 12:05:21PM -0700, Jason Gunthorpe wrote:
>>>>>>> On Fri, Jan 10, 2014 at 01:22:40PM -0500, Jason Cooper wrote:
>>>>>>>
>>>>>>>> Do we create new compatible strings to indicate errata, or to indicate
>>>>>>>> 'from this version forward there are new features'?  The former would
>>>>>>>> indicate as Gregory has written '...-a0-i2c', the latter would warrant
>>>>>>>> '...-b0-i2c' and disabling offloading if we don't see '...-b0-i2c'.
>>>>>>
>>>>>> s/-b0-i2c'./-b0-i2c' or newer./
>>>>>>
>>>>>>> IMHO the compatible string should represent a specific HW/SW ABI. So
>>>>>>> you need a unique compatible string for every variation of that ABI.
>>>>>>
>>>>>> My concern is that we tend to do things like "marvell,orion-sata" for
>>>>>> the first version of the IP block we can work with.  orion5x, kirkwood,
>>>>>> dove, and armada 370/xp all use that compatible string to refer to that
>>>>>> IP block.
>>>>>>
>>>>>> Given that we look at it as 'and newer', '...-a0-i2c' would mean no
>>>>>> offloading until we introduce '-b0-i2c'.  Or am I mis-understanding what
>>>>>> you're saying?
>>>>>>
>>>>>>> We already have a compatible string defined for the ABI that B0
>>>>>>> presents.
>>>>>>
>>>>>> So 'mv78230-i2c' is newer than 'mv78230-a0-i2c', or are you referring to
>>>>>> something else?
>>>>>
>>>>> I think the crux of it is:  Is mv78230-i2c the first, or the default?
>>>>
>>>> Here it's clearly the default
>>>
>>> So we should default to no offloading when we see it?  Since it has been
>>> deployed referring to -a0 revision i2c IP blocks?
>>>
>>
>> But this assumption is wrong as I already wrote few days ago, mv78230-i2c
>> has been deployed referring to -b0 revision i2c IP blocks since the begining.
> 
> Ok, sorry.  As I wrote on irc last week, I've been on travel and haven't
> been able to fully digest everything coming in.  My re-read of all the
> threads regarding this this morning didn't catch it.

No problem there was a lot of email just for this simple fix!

> 
>> It was developed on and for B0 version, and this compatible was created for
>> this specific version. It was latter that we realized that it was not fully
>> compatible with A0. But for sure:
>>
>> mv78230-i2c == I2C IP running on Armada XP B0 (or latter)
> 
> Ok, this still feels counter-intuitive, and folks not familiar with the
> development process might assume the opposite.  So I'll reply to 4/4
> with a reword to make your above statement an explicit part of the
> binding documentation.  No need to do another patch version.  I'll fix
> it up when I pull it in if you're ok with it.

Please use my branch because as I wrote you:
https://github.com/MISL-EBU-System-SW/mainline-public/tree/i2c-mv64xxx-3.13-rc6-fixes-v6

I took into account the last minor review from Arnd and Wolfram, and I
also updated all the flags acked-by and reported-by. But if you prefer
I can just sent a 6th version on the mailing list.

Thanks,

Gregory


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


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

Re: [PATCH v5 4/4] i2c: mv64xxx: Document the newly introduced Armada XP A0 compatible

[Jason Cooper]

Gregory,

On Wed, Jan 08, 2014 at 04:06:29PM +0100, Gregory CLEMENT wrote:
> The first variants of Armada XP SoCs (A0 stepping) have issues related
> to the i2c controller which prevent to use the offload mechanism and
> ead to a kernel hang during boot.

I'll fixup s/ead/lead/ here.

> 
> The commit introduces a new the compatible string
> marvell,mv78230-a0-i2c for the i2c controller.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>  Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> index 82e8f6f17179..9410ed72ec45 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> @@ -5,7 +5,7 @@ Required properties :
>  
>   - reg             : Offset and length of the register set for the device
>   - compatible      : Should be "marvell,mv64xxx-i2c" or "allwinner,sun4i-i2c"
> -                     or "marvell,mv78230-i2c"
> +                     or "marvell,mv78230-i2c" or "marvell,mv78230-a0-i2c"

If it's ok with you Gregory, I'll amend this hunk as follows:

			or "marvell,mv78230-i2c" or "marvell,mv78230-a0-i2c"
			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.

thx,

Jason.

>   - interrupts      : The interrupt number
>  
>  Optional properties :
> -- 
> 1.8.1.2
> 

[PATCH v5 4/4] i2c: mv64xxx: Document the newly introduced Armada XP A0 compatible

[Jason Cooper]

Gregory,

On Wed, Jan 08, 2014 at 04:06:29PM +0100, Gregory CLEMENT wrote:
> The first variants of Armada XP SoCs (A0 stepping) have issues related
> to the i2c controller which prevent to use the offload mechanism and
> ead to a kernel hang during boot.

I'll fixup s/ead/lead/ here.

> 
> The commit introduces a new the compatible string
> marvell,mv78230-a0-i2c for the i2c controller.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> cc: devicetree at vger.kernel.org
> ---
>  Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> index 82e8f6f17179..9410ed72ec45 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> @@ -5,7 +5,7 @@ Required properties :
>  
>   - reg             : Offset and length of the register set for the device
>   - compatible      : Should be "marvell,mv64xxx-i2c" or "allwinner,sun4i-i2c"
> -                     or "marvell,mv78230-i2c"
> +                     or "marvell,mv78230-i2c" or "marvell,mv78230-a0-i2c"

If it's ok with you Gregory, I'll amend this hunk as follows:

			or "marvell,mv78230-i2c" or "marvell,mv78230-a0-i2c"
			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.

thx,

Jason.

>   - interrupts      : The interrupt number
>  
>  Optional properties :
> -- 
> 1.8.1.2
> 

Re: [PATCH v5 4/4] i2c: mv64xxx: Document the newly introduced Armada XP A0 compatible

[Gregory CLEMENT]

On 10/01/2014 22:55, Jason Cooper wrote:
> Gregory,
> 
> On Wed, Jan 08, 2014 at 04:06:29PM +0100, Gregory CLEMENT wrote:
>> The first variants of Armada XP SoCs (A0 stepping) have issues related
>> to the i2c controller which prevent to use the offload mechanism and
>> ead to a kernel hang during boot.
> 
> I'll fixup s/ead/lead/ here.
> 
>>
>> The commit introduces a new the compatible string
>> marvell,mv78230-a0-i2c for the i2c controller.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>> cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> ---
>>  Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
>> index 82e8f6f17179..9410ed72ec45 100644
>> --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
>> @@ -5,7 +5,7 @@ Required properties :
>>  
>>   - reg             : Offset and length of the register set for the device
>>   - compatible      : Should be "marvell,mv64xxx-i2c" or "allwinner,sun4i-i2c"
>> -                     or "marvell,mv78230-i2c"
>> +                     or "marvell,mv78230-i2c" or "marvell,mv78230-a0-i2c"
> 
> If it's ok with you Gregory, I'll amend this hunk as follows:
> 
> 			or "marvell,mv78230-i2c" or "marvell,mv78230-a0-i2c"
> 			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.
> 
Hi Jason,

I agree with your changes

Thanks,

Gregory

> 
>>   - interrupts      : The interrupt number
>>  
>>  Optional properties :
>> -- 
>> 1.8.1.2
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


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

[PATCH v5 4/4] i2c: mv64xxx: Document the newly introduced Armada XP A0 compatible

[Gregory CLEMENT]

On 10/01/2014 22:55, Jason Cooper wrote:
> Gregory,
> 
> On Wed, Jan 08, 2014 at 04:06:29PM +0100, Gregory CLEMENT wrote:
>> The first variants of Armada XP SoCs (A0 stepping) have issues related
>> to the i2c controller which prevent to use the offload mechanism and
>> ead to a kernel hang during boot.
> 
> I'll fixup s/ead/lead/ here.
> 
>>
>> The commit introduces a new the compatible string
>> marvell,mv78230-a0-i2c for the i2c controller.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> cc: devicetree at vger.kernel.org
>> ---
>>  Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
>> index 82e8f6f17179..9410ed72ec45 100644
>> --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
>> @@ -5,7 +5,7 @@ Required properties :
>>  
>>   - reg             : Offset and length of the register set for the device
>>   - compatible      : Should be "marvell,mv64xxx-i2c" or "allwinner,sun4i-i2c"
>> -                     or "marvell,mv78230-i2c"
>> +                     or "marvell,mv78230-i2c" or "marvell,mv78230-a0-i2c"
> 
> If it's ok with you Gregory, I'll amend this hunk as follows:
> 
> 			or "marvell,mv78230-i2c" or "marvell,mv78230-a0-i2c"
> 			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.
> 
Hi Jason,

I agree with your changes

Thanks,

Gregory

> 
>>   - interrupts      : The interrupt number
>>  
>>  Optional properties :
>> -- 
>> 1.8.1.2
>>
> --
> 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
> 


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

Re: [PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Gregory CLEMENT]

On 10/01/2014 22:52, Gregory CLEMENT wrote:
> On 10/01/2014 22:37, Jason Cooper wrote:
>> On Fri, Jan 10, 2014 at 10:21:29PM +0100, Gregory CLEMENT wrote:
>>> On 10/01/2014 22:14, Jason Cooper wrote:
>>>> On Fri, Jan 10, 2014 at 09:12:41PM +0100, Gregory CLEMENT wrote:
>>>>> Jason,
>>>>> On 10/01/2014 21:08, Jason Cooper wrote:
>>>>>> On Fri, Jan 10, 2014 at 02:45:50PM -0500, Jason Cooper wrote:
>>>>>>> On Fri, Jan 10, 2014 at 12:05:21PM -0700, Jason Gunthorpe wrote:
>>>>>>>> On Fri, Jan 10, 2014 at 01:22:40PM -0500, Jason Cooper wrote:
>>>>>>>>
>>>>>>>>> Do we create new compatible strings to indicate errata, or to indicate
>>>>>>>>> 'from this version forward there are new features'?  The former would
>>>>>>>>> indicate as Gregory has written '...-a0-i2c', the latter would warrant
>>>>>>>>> '...-b0-i2c' and disabling offloading if we don't see '...-b0-i2c'.
>>>>>>>
>>>>>>> s/-b0-i2c'./-b0-i2c' or newer./
>>>>>>>
>>>>>>>> IMHO the compatible string should represent a specific HW/SW ABI. So
>>>>>>>> you need a unique compatible string for every variation of that ABI.
>>>>>>>
>>>>>>> My concern is that we tend to do things like "marvell,orion-sata" for
>>>>>>> the first version of the IP block we can work with.  orion5x, kirkwood,
>>>>>>> dove, and armada 370/xp all use that compatible string to refer to that
>>>>>>> IP block.
>>>>>>>
>>>>>>> Given that we look at it as 'and newer', '...-a0-i2c' would mean no
>>>>>>> offloading until we introduce '-b0-i2c'.  Or am I mis-understanding what
>>>>>>> you're saying?
>>>>>>>
>>>>>>>> We already have a compatible string defined for the ABI that B0
>>>>>>>> presents.
>>>>>>>
>>>>>>> So 'mv78230-i2c' is newer than 'mv78230-a0-i2c', or are you referring to
>>>>>>> something else?
>>>>>>
>>>>>> I think the crux of it is:  Is mv78230-i2c the first, or the default?
>>>>>
>>>>> Here it's clearly the default
>>>>
>>>> So we should default to no offloading when we see it?  Since it has been
>>>> deployed referring to -a0 revision i2c IP blocks?
>>>>
>>>
>>> But this assumption is wrong as I already wrote few days ago, mv78230-i2c
>>> has been deployed referring to -b0 revision i2c IP blocks since the begining.
>>
>> Ok, sorry.  As I wrote on irc last week, I've been on travel and haven't
>> been able to fully digest everything coming in.  My re-read of all the
>> threads regarding this this morning didn't catch it.
> 
> No problem there was a lot of email just for this simple fix!
> 
>>
>>> It was developed on and for B0 version, and this compatible was created for
>>> this specific version. It was latter that we realized that it was not fully
>>> compatible with A0. But for sure:
>>>
>>> mv78230-i2c == I2C IP running on Armada XP B0 (or latter)
>>
>> Ok, this still feels counter-intuitive, and folks not familiar with the
>> development process might assume the opposite.  So I'll reply to 4/4
>> with a reword to make your above statement an explicit part of the
>> binding documentation.  No need to do another patch version.  I'll fix
>> it up when I pull it in if you're ok with it.
> 
> Please use my branch because as I wrote you:
> https://github.com/MISL-EBU-System-SW/mainline-public/tree/i2c-mv64xxx-3.13-rc6-fixes-v6
> 
> I took into account the last minor review from Arnd and Wolfram, and I
> also updated all the flags acked-by and reported-by. But if you prefer
> I can just sent a 6th version on the mailing list.

Hi Jason,

Finally 3.13 was not released but the 3.13-rc8 instead.
Do you consider to pull this series and then push it this week,
before the final release of 3.13?

Thanks,

Gregory


> 
> Thanks,
> 
> Gregory
> 
> 
>>
>> thx,
>>
>> Jason.
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
> 
> 


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

[PATCH v5 2/4] ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board

[Gregory CLEMENT]

On 10/01/2014 22:52, Gregory CLEMENT wrote:
> On 10/01/2014 22:37, Jason Cooper wrote:
>> On Fri, Jan 10, 2014 at 10:21:29PM +0100, Gregory CLEMENT wrote:
>>> On 10/01/2014 22:14, Jason Cooper wrote:
>>>> On Fri, Jan 10, 2014 at 09:12:41PM +0100, Gregory CLEMENT wrote:
>>>>> Jason,
>>>>> On 10/01/2014 21:08, Jason Cooper wrote:
>>>>>> On Fri, Jan 10, 2014 at 02:45:50PM -0500, Jason Cooper wrote:
>>>>>>> On Fri, Jan 10, 2014 at 12:05:21PM -0700, Jason Gunthorpe wrote:
>>>>>>>> On Fri, Jan 10, 2014 at 01:22:40PM -0500, Jason Cooper wrote:
>>>>>>>>
>>>>>>>>> Do we create new compatible strings to indicate errata, or to indicate
>>>>>>>>> 'from this version forward there are new features'?  The former would
>>>>>>>>> indicate as Gregory has written '...-a0-i2c', the latter would warrant
>>>>>>>>> '...-b0-i2c' and disabling offloading if we don't see '...-b0-i2c'.
>>>>>>>
>>>>>>> s/-b0-i2c'./-b0-i2c' or newer./
>>>>>>>
>>>>>>>> IMHO the compatible string should represent a specific HW/SW ABI. So
>>>>>>>> you need a unique compatible string for every variation of that ABI.
>>>>>>>
>>>>>>> My concern is that we tend to do things like "marvell,orion-sata" for
>>>>>>> the first version of the IP block we can work with.  orion5x, kirkwood,
>>>>>>> dove, and armada 370/xp all use that compatible string to refer to that
>>>>>>> IP block.
>>>>>>>
>>>>>>> Given that we look at it as 'and newer', '...-a0-i2c' would mean no
>>>>>>> offloading until we introduce '-b0-i2c'.  Or am I mis-understanding what
>>>>>>> you're saying?
>>>>>>>
>>>>>>>> We already have a compatible string defined for the ABI that B0
>>>>>>>> presents.
>>>>>>>
>>>>>>> So 'mv78230-i2c' is newer than 'mv78230-a0-i2c', or are you referring to
>>>>>>> something else?
>>>>>>
>>>>>> I think the crux of it is:  Is mv78230-i2c the first, or the default?
>>>>>
>>>>> Here it's clearly the default
>>>>
>>>> So we should default to no offloading when we see it?  Since it has been
>>>> deployed referring to -a0 revision i2c IP blocks?
>>>>
>>>
>>> But this assumption is wrong as I already wrote few days ago, mv78230-i2c
>>> has been deployed referring to -b0 revision i2c IP blocks since the begining.
>>
>> Ok, sorry.  As I wrote on irc last week, I've been on travel and haven't
>> been able to fully digest everything coming in.  My re-read of all the
>> threads regarding this this morning didn't catch it.
> 
> No problem there was a lot of email just for this simple fix!
> 
>>
>>> It was developed on and for B0 version, and this compatible was created for
>>> this specific version. It was latter that we realized that it was not fully
>>> compatible with A0. But for sure:
>>>
>>> mv78230-i2c == I2C IP running on Armada XP B0 (or latter)
>>
>> Ok, this still feels counter-intuitive, and folks not familiar with the
>> development process might assume the opposite.  So I'll reply to 4/4
>> with a reword to make your above statement an explicit part of the
>> binding documentation.  No need to do another patch version.  I'll fix
>> it up when I pull it in if you're ok with it.
> 
> Please use my branch because as I wrote you:
> https://github.com/MISL-EBU-System-SW/mainline-public/tree/i2c-mv64xxx-3.13-rc6-fixes-v6
> 
> I took into account the last minor review from Arnd and Wolfram, and I
> also updated all the flags acked-by and reported-by. But if you prefer
> I can just sent a 6th version on the mailing list.

Hi Jason,

Finally 3.13 was not released but the 3.13-rc8 instead.
Do you consider to pull this series and then push it this week,
before the final release of 3.13?

Thanks,

Gregory


> 
> Thanks,
> 
> Gregory
> 
> 
>>
>> thx,
>>
>> Jason.
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
> 
> 


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

Re: [PATCH v5 0/4] Fix i2c bus hang on A0 version of the Armada XP SoCs

[Jason Cooper]

Gregory,

On Wed, Jan 08, 2014 at 04:06:25PM +0100, Gregory CLEMENT wrote:
> Hi,
> 
> Here come the 5th version 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.
> 
> The main change are the use of marvell,mv78230-a0-i2c and that the
> function mvebu_get_soc_id() is now local to mach-mvebu.
> 
> The first patch add a mean to detect the SoCs version at run-time and
> the second one use this feature in the driver.
> 
> The 3 first 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".

Ok, I've pulled this in by cherrypicking the commits.  I needed to add
the 'Fixes: ...' and 'Cc: stable ...' language, so the commit ids were
going to change anyhow.  I also added the note to the binding as we
discussed.  I've also based this against v3.13-rc1 as there doesn't
appear to be any need to drag in everything up to -rc6.

I've pushed this to mvebu/fixes.  Please take a look.  If it all looks
good I'll send the pull request off tomorrow.

thx,

Jason.

[PATCH v5 0/4] Fix i2c bus hang on A0 version of the Armada XP SoCs

[Jason Cooper]

Gregory,

On Wed, Jan 08, 2014 at 04:06:25PM +0100, Gregory CLEMENT wrote:
> Hi,
> 
> Here come the 5th version 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.
> 
> The main change are the use of marvell,mv78230-a0-i2c and that the
> function mvebu_get_soc_id() is now local to mach-mvebu.
> 
> The first patch add a mean to detect the SoCs version at run-time and
> the second one use this feature in the driver.
> 
> The 3 first 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".

Ok, I've pulled this in by cherrypicking the commits.  I needed to add
the 'Fixes: ...' and 'Cc: stable ...' language, so the commit ids were
going to change anyhow.  I also added the note to the binding as we
discussed.  I've also based this against v3.13-rc1 as there doesn't
appear to be any need to drag in everything up to -rc6.

I've pushed this to mvebu/fixes.  Please take a look.  If it all looks
good I'll send the pull request off tomorrow.

thx,

Jason.

Re: [PATCH v5 0/4] Fix i2c bus hang on A0 version of the Armada XP SoCs

[Gregory CLEMENT]

On 14/01/2014 03:14, Jason Cooper wrote:
> Gregory,
> 
> On Wed, Jan 08, 2014 at 04:06:25PM +0100, Gregory CLEMENT wrote:
>> Hi,
>>
>> Here come the 5th version 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.
>>
>> The main change are the use of marvell,mv78230-a0-i2c and that the
>> function mvebu_get_soc_id() is now local to mach-mvebu.
>>
>> The first patch add a mean to detect the SoCs version at run-time and
>> the second one use this feature in the driver.
>>
>> The 3 first 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".
> 
> Ok, I've pulled this in by cherrypicking the commits.  I needed to add
> the 'Fixes: ...' and 'Cc: stable ...' language, so the commit ids were
> going to change anyhow.  I also added the note to the binding as we
> discussed.  I've also based this against v3.13-rc1 as there doesn't
> appear to be any need to drag in everything up to -rc6.
> 
> I've pushed this to mvebu/fixes.  Please take a look.  If it all looks
> good I'll send the pull request off tomorrow.
Hi Jason,

Everything looks perfect!

Thanks for having improved it,

Gregory

> 
> thx,
> 
> Jason.
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 


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

[PATCH v5 0/4] Fix i2c bus hang on A0 version of the Armada XP SoCs

[Gregory CLEMENT]

On 14/01/2014 03:14, Jason Cooper wrote:
> Gregory,
> 
> On Wed, Jan 08, 2014 at 04:06:25PM +0100, Gregory CLEMENT wrote:
>> Hi,
>>
>> Here come the 5th version 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.
>>
>> The main change are the use of marvell,mv78230-a0-i2c and that the
>> function mvebu_get_soc_id() is now local to mach-mvebu.
>>
>> The first patch add a mean to detect the SoCs version at run-time and
>> the second one use this feature in the driver.
>>
>> The 3 first 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".
> 
> Ok, I've pulled this in by cherrypicking the commits.  I needed to add
> the 'Fixes: ...' and 'Cc: stable ...' language, so the commit ids were
> going to change anyhow.  I also added the note to the binding as we
> discussed.  I've also based this against v3.13-rc1 as there doesn't
> appear to be any need to drag in everything up to -rc6.
> 
> I've pushed this to mvebu/fixes.  Please take a look.  If it all looks
> good I'll send the pull request off tomorrow.
Hi Jason,

Everything looks perfect!

Thanks for having improved it,

Gregory

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


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

Re: [PATCH v5 0/4] Fix i2c bus hang on A0 version of the Armada XP SoCs

[Gregory CLEMENT]

On 14/01/2014 09:46, Gregory CLEMENT wrote:
> On 14/01/2014 03:14, Jason Cooper wrote:
>> Gregory,
>>
>> On Wed, Jan 08, 2014 at 04:06:25PM +0100, Gregory CLEMENT wrote:
>>> Hi,
>>>
>>> Here come the 5th version 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.
>>>
>>> The main change are the use of marvell,mv78230-a0-i2c and that the
>>> function mvebu_get_soc_id() is now local to mach-mvebu.
>>>
>>> The first patch add a mean to detect the SoCs version at run-time and
>>> the second one use this feature in the driver.
>>>
>>> The 3 first 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".
>>
>> Ok, I've pulled this in by cherrypicking the commits.  I needed to add
>> the 'Fixes: ...' and 'Cc: stable ...' language, so the commit ids were
>> going to change anyhow.  I also added the note to the binding as we
>> discussed.  I've also based this against v3.13-rc1 as there doesn't
>> appear to be any need to drag in everything up to -rc6.
>>
>> I've pushed this to mvebu/fixes.  Please take a look.  If it all looks
>> good I'll send the pull request off tomorrow.
> Hi Jason,
> 
> Everything looks perfect!
> 
> Thanks for having improved it,
> 
> Gregory
> 

Hi Jason,

Eventually we didn't finish with it!
Ezequiel found an issue when we try to access pci_base if it have
not been properly mapped, in this case the kernel hang. Indeed the check
of  the return of of_iomap was wrong.

The fix would be something like:

-o<---o<---o<---o<---o<---o<---o<---o<---o<---o<---o<--
--- a/arch/arm/mach-mvebu/mvebu-soc-id.c
+++ b/arch/arm/mach-mvebu/mvebu-soc-id.c
@@ -88,7 +88,7 @@ static int __init mvebu_soc_id_init(void)
        }

        pci_base = of_iomap(child, 0);
-       if (IS_ERR(pci_base)) {
+       if (pci_base == NULL) {
                pr_err("cannot map registers\n");
                ret = -ENOMEM;
                goto res_ioremap;
-o<---o<---o<---o<---o<---o<---o<---o<---o<---o<---o<--

How do you want we handle it?

Do you want a proper patch to amend?
A new series?
Something else?

Thanks,

Gregory


>>
>> thx,
>>
>> Jason.
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
> 
> 


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

[PATCH v5 0/4] Fix i2c bus hang on A0 version of the Armada XP SoCs

[Gregory CLEMENT]

On 14/01/2014 09:46, Gregory CLEMENT wrote:
> On 14/01/2014 03:14, Jason Cooper wrote:
>> Gregory,
>>
>> On Wed, Jan 08, 2014 at 04:06:25PM +0100, Gregory CLEMENT wrote:
>>> Hi,
>>>
>>> Here come the 5th version 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.
>>>
>>> The main change are the use of marvell,mv78230-a0-i2c and that the
>>> function mvebu_get_soc_id() is now local to mach-mvebu.
>>>
>>> The first patch add a mean to detect the SoCs version at run-time and
>>> the second one use this feature in the driver.
>>>
>>> The 3 first 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".
>>
>> Ok, I've pulled this in by cherrypicking the commits.  I needed to add
>> the 'Fixes: ...' and 'Cc: stable ...' language, so the commit ids were
>> going to change anyhow.  I also added the note to the binding as we
>> discussed.  I've also based this against v3.13-rc1 as there doesn't
>> appear to be any need to drag in everything up to -rc6.
>>
>> I've pushed this to mvebu/fixes.  Please take a look.  If it all looks
>> good I'll send the pull request off tomorrow.
> Hi Jason,
> 
> Everything looks perfect!
> 
> Thanks for having improved it,
> 
> Gregory
> 

Hi Jason,

Eventually we didn't finish with it!
Ezequiel found an issue when we try to access pci_base if it have
not been properly mapped, in this case the kernel hang. Indeed the check
of  the return of of_iomap was wrong.

The fix would be something like:

-o<---o<---o<---o<---o<---o<---o<---o<---o<---o<---o<--
--- a/arch/arm/mach-mvebu/mvebu-soc-id.c
+++ b/arch/arm/mach-mvebu/mvebu-soc-id.c
@@ -88,7 +88,7 @@ static int __init mvebu_soc_id_init(void)
        }

        pci_base = of_iomap(child, 0);
-       if (IS_ERR(pci_base)) {
+       if (pci_base == NULL) {
                pr_err("cannot map registers\n");
                ret = -ENOMEM;
                goto res_ioremap;
-o<---o<---o<---o<---o<---o<---o<---o<---o<---o<---o<--

How do you want we handle it?

Do you want a proper patch to amend?
A new series?
Something else?

Thanks,

Gregory


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


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

Re: [PATCH v5 0/4] Fix i2c bus hang on A0 version of the Armada XP SoCs

[Jason Cooper]

On Mon, Jan 20, 2014 at 12:20:40PM +0100, Gregory CLEMENT wrote:
> On 14/01/2014 09:46, Gregory CLEMENT wrote:
> > On 14/01/2014 03:14, Jason Cooper wrote:
> >> Gregory,
> >>
> >> On Wed, Jan 08, 2014 at 04:06:25PM +0100, Gregory CLEMENT wrote:
> >>> Hi,
> >>>
> >>> Here come the 5th version 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.
> >>>
> >>> The main change are the use of marvell,mv78230-a0-i2c and that the
> >>> function mvebu_get_soc_id() is now local to mach-mvebu.
> >>>
> >>> The first patch add a mean to detect the SoCs version at run-time and
> >>> the second one use this feature in the driver.
> >>>
> >>> The 3 first 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".
> >>
> >> Ok, I've pulled this in by cherrypicking the commits.  I needed to add
> >> the 'Fixes: ...' and 'Cc: stable ...' language, so the commit ids were
> >> going to change anyhow.  I also added the note to the binding as we
> >> discussed.  I've also based this against v3.13-rc1 as there doesn't
> >> appear to be any need to drag in everything up to -rc6.
> >>
> >> I've pushed this to mvebu/fixes.  Please take a look.  If it all looks
> >> good I'll send the pull request off tomorrow.
> > Hi Jason,
> > 
> > Everything looks perfect!
> > 
> > Thanks for having improved it,
> > 
> > Gregory
> > 
> 
> Hi Jason,
> 
> Eventually we didn't finish with it!
> Ezequiel found an issue when we try to access pci_base if it have
> not been properly mapped, in this case the kernel hang. Indeed the check
> of  the return of of_iomap was wrong.
> 
> The fix would be something like:
> 
> -o<---o<---o<---o<---o<---o<---o<---o<---o<---o<---o<--
> --- a/arch/arm/mach-mvebu/mvebu-soc-id.c
> +++ b/arch/arm/mach-mvebu/mvebu-soc-id.c
> @@ -88,7 +88,7 @@ static int __init mvebu_soc_id_init(void)
>         }
> 
>         pci_base = of_iomap(child, 0);
> -       if (IS_ERR(pci_base)) {
> +       if (pci_base == NULL) {
>                 pr_err("cannot map registers\n");
>                 ret = -ENOMEM;
>                 goto res_ioremap;
> -o<---o<---o<---o<---o<---o<---o<---o<---o<---o<---o<--
> 
> How do you want we handle it?
> 
> Do you want a proper patch to amend?
> A new series?
> Something else?
> 

submit a patch, please.  We'll push it as a fix.

thx,

Jason.

[PATCH v5 0/4] Fix i2c bus hang on A0 version of the Armada XP SoCs

[Jason Cooper]

On Mon, Jan 20, 2014 at 12:20:40PM +0100, Gregory CLEMENT wrote:
> On 14/01/2014 09:46, Gregory CLEMENT wrote:
> > On 14/01/2014 03:14, Jason Cooper wrote:
> >> Gregory,
> >>
> >> On Wed, Jan 08, 2014 at 04:06:25PM +0100, Gregory CLEMENT wrote:
> >>> Hi,
> >>>
> >>> Here come the 5th version 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.
> >>>
> >>> The main change are the use of marvell,mv78230-a0-i2c and that the
> >>> function mvebu_get_soc_id() is now local to mach-mvebu.
> >>>
> >>> The first patch add a mean to detect the SoCs version at run-time and
> >>> the second one use this feature in the driver.
> >>>
> >>> The 3 first 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".
> >>
> >> Ok, I've pulled this in by cherrypicking the commits.  I needed to add
> >> the 'Fixes: ...' and 'Cc: stable ...' language, so the commit ids were
> >> going to change anyhow.  I also added the note to the binding as we
> >> discussed.  I've also based this against v3.13-rc1 as there doesn't
> >> appear to be any need to drag in everything up to -rc6.
> >>
> >> I've pushed this to mvebu/fixes.  Please take a look.  If it all looks
> >> good I'll send the pull request off tomorrow.
> > Hi Jason,
> > 
> > Everything looks perfect!
> > 
> > Thanks for having improved it,
> > 
> > Gregory
> > 
> 
> Hi Jason,
> 
> Eventually we didn't finish with it!
> Ezequiel found an issue when we try to access pci_base if it have
> not been properly mapped, in this case the kernel hang. Indeed the check
> of  the return of of_iomap was wrong.
> 
> The fix would be something like:
> 
> -o<---o<---o<---o<---o<---o<---o<---o<---o<---o<---o<--
> --- a/arch/arm/mach-mvebu/mvebu-soc-id.c
> +++ b/arch/arm/mach-mvebu/mvebu-soc-id.c
> @@ -88,7 +88,7 @@ static int __init mvebu_soc_id_init(void)
>         }
> 
>         pci_base = of_iomap(child, 0);
> -       if (IS_ERR(pci_base)) {
> +       if (pci_base == NULL) {
>                 pr_err("cannot map registers\n");
>                 ret = -ENOMEM;
>                 goto res_ioremap;
> -o<---o<---o<---o<---o<---o<---o<---o<---o<---o<---o<--
> 
> How do you want we handle it?
> 
> Do you want a proper patch to amend?
> A new series?
> Something else?
> 

submit a patch, please.  We'll push it as a fix.

thx,

Jason.