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

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

[Gregory CLEMENT]

Hi,

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

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

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

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

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

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

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

Thanks,

Gregory

Changelog:
v3 -> v4:

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

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

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

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

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

v2 -> v3:

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

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

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

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

v1 -> v2:

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

- Removed the superfluous parentheses

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

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

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

-- 
1.8.1.2

[PATCH v4 1/3] 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@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 | 120 +++++++++++++++++++++++++++++++++++++
 include/linux/mvebu-soc-id.h       |  32 ++++++++++
 3 files changed, 153 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/mach-mvebu/mvebu-soc-id.c
 create mode 100644 include/linux/mvebu-soc-id.h

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..7aaba3a2f096
--- /dev/null
+++ b/arch/arm/mach-mvebu/mvebu-soc-id.c
@@ -0,0 +1,120 @@
+/*
+ * Variant 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/mvebu-soc-id.h>
+#include <linux/of.h>
+#include <linux/of_address.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;
+}
+EXPORT_SYMBOL_GPL(mvebu_get_soc_id);
+
+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/include/linux/mvebu-soc-id.h b/include/linux/mvebu-soc-id.h
new file mode 100644
index 000000000000..31654252fe35
--- /dev/null
+++ b/include/linux/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 v4 2/3] ARM: mvebu: Add quirk for i2c

[Gregory CLEMENT]

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

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

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

diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
index e2acff98e750..c93ac68779e0 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/mvebu-soc-id.h>
 #include <asm/hardware/cache-l2x0.h>
 #include <asm/mach/arch.h>
 #include <asm/mach/map.h>
@@ -45,8 +46,31 @@ static void __init armada_370_xp_timer_and_clk_init(void)
 #endif
 }
 
+static struct property i2c_offload_broken = {
+	.name = "offload-broken",
+};
+
+static void __init i2c_quirk(void)
+{
+	struct device_node *np;
+	u32 dev, rev;
+
+	/*
+	 * Only revisons more recent than A0 support the offload
+	 * mechanism. We can exit only if we are sure that we can
+	 * get the SoC revision and it is more recent than A0.
+	 */
+	if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV)
+		return;
+
+	for_each_compatible_node(np, NULL, "marvell,mv78230-i2c")
+		of_add_property(np, &i2c_offload_broken);
+	return;
+}
+
 static void __init armada_370_xp_dt_init(void)
 {
+	i2c_quirk();
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 }
 
-- 
1.8.1.2

[PATCH v4 3/3] 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 driver now checks if offload the mechanism is tagged as broken
and enable it in the opposite case.

Cc: stable@vger.kernel.org
Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 drivers/i2c/busses/i2c-mv64xxx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

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

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

[Jason Gunthorpe]

On Tue, Jan 07, 2014 at 05:35:03PM +0100, Gregory CLEMENT wrote:
> +static struct property i2c_offload_broken = {
> +	.name = "offload-broken",
> +};
> +
> +static void __init i2c_quirk(void)
> +{
> +	struct device_node *np;
> +	u32 dev, rev;
> +
> +	/*
> +	 * Only revisons more recent than A0 support the offload
> +	 * mechanism. We can exit only if we are sure that we can
> +	 * get the SoC revision and it is more recent than A0.
> +	 */
> +	if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV)
> +		return;
> +
> +	for_each_compatible_node(np, NULL, "marvell,mv78230-i2c")
> +		of_add_property(np, &i2c_offload_broken);

I like this approach.

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

Jason

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

[Thomas Petazzoni]

Dear Gregory CLEMENT,

On Tue,  7 Jan 2014 17:35:04 +0100, Gregory CLEMENT wrote:

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

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

Something like:

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

Thanks!

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

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

[Wolfram Sang]

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

On Tue, Jan 07, 2014 at 11:38:53AM -0700, Jason Gunthorpe wrote:
> On Tue, Jan 07, 2014 at 05:35:03PM +0100, Gregory CLEMENT wrote:
> > +static struct property i2c_offload_broken = {
> > +	.name = "offload-broken",
> > +};
> > +
> > +static void __init i2c_quirk(void)
> > +{
> > +	struct device_node *np;
> > +	u32 dev, rev;
> > +
> > +	/*
> > +	 * Only revisons more recent than A0 support the offload
> > +	 * mechanism. We can exit only if we are sure that we can
> > +	 * get the SoC revision and it is more recent than A0.
> > +	 */
> > +	if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV)
> > +		return;
> > +
> > +	for_each_compatible_node(np, NULL, "marvell,mv78230-i2c")
> > +		of_add_property(np, &i2c_offload_broken);
> 
> I like this approach.

Sorry, but I don't.

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

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


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

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

[Gregory CLEMENT]

Hi Wolfram,

On 08/01/2014 00:06, Wolfram Sang wrote:
> On Tue, Jan 07, 2014 at 11:38:53AM -0700, Jason Gunthorpe wrote:
>> On Tue, Jan 07, 2014 at 05:35:03PM +0100, Gregory CLEMENT wrote:
>>> +static struct property i2c_offload_broken = {
>>> +	.name = "offload-broken",
>>> +};
>>> +
>>> +static void __init i2c_quirk(void)
>>> +{
>>> +	struct device_node *np;
>>> +	u32 dev, rev;
>>> +
>>> +	/*
>>> +	 * Only revisons more recent than A0 support the offload
>>> +	 * mechanism. We can exit only if we are sure that we can
>>> +	 * get the SoC revision and it is more recent than A0.
>>> +	 */
>>> +	if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV)
>>> +		return;
>>> +
>>> +	for_each_compatible_node(np, NULL, "marvell,mv78230-i2c")
>>> +		of_add_property(np, &i2c_offload_broken);
>>
>> I like this approach.
> 
> Sorry, but I don't.
> 
>> However, when I first read this I thought it should be a -a0 specific
>> compatible string, not a 'offload-broken' property - any idea what the
>> DT consensus is here? I've seen both approach in use ..
> 
> I prefer the replacement of the compatible string. If it should really
> be a seperate property, then it should be a vendor specific property. It
> is not generic, at all.

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

Thanks,

Gregory

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

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

[Wolfram Sang]

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

On Wed, Jan 08, 2014 at 11:15:05AM +0100, Gregory CLEMENT wrote:
> Hi Wolfram,
> 
> On 08/01/2014 00:06, Wolfram Sang wrote:
> > On Tue, Jan 07, 2014 at 11:38:53AM -0700, Jason Gunthorpe wrote:
> >> On Tue, Jan 07, 2014 at 05:35:03PM +0100, Gregory CLEMENT wrote:
> >>> +static struct property i2c_offload_broken = {
> >>> +	.name = "offload-broken",
> >>> +};
> >>> +
> >>> +static void __init i2c_quirk(void)
> >>> +{
> >>> +	struct device_node *np;
> >>> +	u32 dev, rev;
> >>> +
> >>> +	/*
> >>> +	 * Only revisons more recent than A0 support the offload
> >>> +	 * mechanism. We can exit only if we are sure that we can
> >>> +	 * get the SoC revision and it is more recent than A0.
> >>> +	 */
> >>> +	if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV)
> >>> +		return;
> >>> +
> >>> +	for_each_compatible_node(np, NULL, "marvell,mv78230-i2c")
> >>> +		of_add_property(np, &i2c_offload_broken);
> >>
> >> I like this approach.
> > 
> > Sorry, but I don't.
> > 
> >> However, when I first read this I thought it should be a -a0 specific
> >> compatible string, not a 'offload-broken' property - any idea what the
> >> DT consensus is here? I've seen both approach in use ..
> > 
> > I prefer the replacement of the compatible string. If it should really
> > be a seperate property, then it should be a vendor specific property. It
> > is not generic, at all.
> 
> Something like "marvell,offload-broken" would be acceptable?

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


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

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

[Arnd Bergmann]

On Tuesday 07 January 2014, Gregory CLEMENT wrote:

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

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

	Arnd

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

[Arnd Bergmann]

On Tuesday 07 January 2014, Gregory CLEMENT wrote:
> diff --git a/include/linux/mvebu-soc-id.h b/include/linux/mvebu-soc-id.h
> new file mode 100644
> index 000000000000..31654252fe35
> --- /dev/null
> +++ b/include/linux/mvebu-soc-id.h
> +#ifdef CONFIG_ARCH_MVEBU
> +int mvebu_get_soc_id(u32 *dev, u32 *rev);
> +#else
> +static inline int mvebu_get_soc_id(u32 *dev, u32 *rev)
> +{
> +	return -1;
> +}
> +#endif
> +
> +#endif /* __LINUX_MVEBU_SOC_ID_H */

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

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

	Arnd

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

[Gregory CLEMENT]

On 08/01/2014 12:29, Wolfram Sang wrote:
> On Wed, Jan 08, 2014 at 11:15:05AM +0100, Gregory CLEMENT wrote:
>> Hi Wolfram,
>>
>> On 08/01/2014 00:06, Wolfram Sang wrote:
>>> On Tue, Jan 07, 2014 at 11:38:53AM -0700, Jason Gunthorpe wrote:
>>>> On Tue, Jan 07, 2014 at 05:35:03PM +0100, Gregory CLEMENT wrote:
>>>>> +static struct property i2c_offload_broken = {
>>>>> +	.name = "offload-broken",
>>>>> +};
>>>>> +
>>>>> +static void __init i2c_quirk(void)
>>>>> +{
>>>>> +	struct device_node *np;
>>>>> +	u32 dev, rev;
>>>>> +
>>>>> +	/*
>>>>> +	 * Only revisons more recent than A0 support the offload
>>>>> +	 * mechanism. We can exit only if we are sure that we can
>>>>> +	 * get the SoC revision and it is more recent than A0.
>>>>> +	 */
>>>>> +	if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV)
>>>>> +		return;
>>>>> +
>>>>> +	for_each_compatible_node(np, NULL, "marvell,mv78230-i2c")
>>>>> +		of_add_property(np, &i2c_offload_broken);
>>>>
>>>> I like this approach.
>>>
>>> Sorry, but I don't.
>>>
>>>> However, when I first read this I thought it should be a -a0 specific
>>>> compatible string, not a 'offload-broken' property - any idea what the
>>>> DT consensus is here? I've seen both approach in use ..
>>>
>>> I prefer the replacement of the compatible string. If it should really
>>> be a seperate property, then it should be a vendor specific property. It
>>> is not generic, at all.
>>
>> Something like "marvell,offload-broken" would be acceptable?
> 
> A tad more, yes. Still, since this is a feature/quirk of the IP core
> revision, it should be deduced from the compatible property IMO. It
> cannot be configured anywhere, so it doesn't change on board level.

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

diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
index e2acff98e750..8c4ecf987e82 100644
--- a/arch/arm/mach-mvebu/armada-370-xp.c
+++ b/arch/arm/mach-mvebu/armada-370-xp.c
@@ -21,6 +21,8 @@
 #include <linux/clocksource.h>
 #include <linux/dma-mapping.h>
 #include <linux/mbus.h>
+#include <linux/mvebu-soc-id.h>
+#include <linux/slab.h>
 #include <asm/hardware/cache-l2x0.h>
 #include <asm/mach/arch.h>
 #include <asm/mach/map.h>
@@ -45,8 +47,38 @@ static void __init armada_370_xp_timer_and_clk_init(void)
 #endif
 }

+static void __init i2c_quirk(void)
+{
+       struct device_node *np;
+       u32 dev, rev;
+       int i = 0;
+
+       /*
+        * Only revisons more recent than A0 support the offload
+        * mechanism. We can exit only if we are sure that we can
+        * get the SoC revision and it is more recent than A0.
+        */
+       if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV)
+               return;
+
+       for_each_compatible_node(np, NULL, "marvell,mv78230-i2c") {
+               struct property *new_compat;
+
+               new_compat = kzalloc(sizeof(*new_compat), GFP_KERNEL);
+
+               new_compat->name =  kstrdup("compatible", GFP_KERNEL);
+               new_compat->length =  sizeof("marvell,mv78230-a0-i2c");
+               new_compat->value =  kstrdup("marvell,mv78230-a0-i2c",
+                                               GFP_KERNEL);
+
+               of_update_property(np, new_compat);
+       }
+       return;
+}
+
 static void __init armada_370_xp_dt_init(void)
 {
+       i2c_quirk();
        of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 }


> 
> 
> 
> _______________________________________________
> 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

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

[Wolfram Sang]

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


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

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


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

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

[Gregory CLEMENT]

On 08/01/2014 13:52, Arnd Bergmann wrote:
> On Tuesday 07 January 2014, Gregory CLEMENT wrote:
> 
>>  static void __init armada_370_xp_dt_init(void)
>>  {
>> +       i2c_quirk();
>>         of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
>>  }
> 
> I'd prefer to enable the quirk only on machines that we know may be affected, i.e.
> OpenBlocks AX3-4. That would make it easier in the future for everyone to figure
> out whether they need to include the quirk in their kernels or not, depending
> on whether they want to support these machines. Just a precaution in case we
> end up having lots of quirks in the long run.

You means something like the following code ?

static void __init armada_370_xp_dt_init(void)
 {
+       if (of_machine_is_compatible("plathome,openblocks-ax3-4"))
+               i2c_quirk();
        of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 }




> 
> 	Arnd
> --
> 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

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

[Arnd Bergmann]

On Wednesday 08 January 2014 14:42:45 Gregory CLEMENT wrote:
> You means something like the following code ?
> 
> static void __init armada_370_xp_dt_init(void)
>  {
> +       if (of_machine_is_compatible("plathome,openblocks-ax3-4"))
> +               i2c_quirk();
>         of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
>  }

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

	Arnd

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

[Arnd Bergmann]

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

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

	Arnd

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

[Gregory CLEMENT]

Hi Wolfram,

On 08/01/2014 14:39, Wolfram Sang wrote:
> 
>>>>>> However, when I first read this I thought it should be a -a0 specific
>>>>>> compatible string, not a 'offload-broken' property - any idea what the
>>>>>> DT consensus is here? I've seen both approach in use ..
>>>>>
>>>>> I prefer the replacement of the compatible string. If it should really
>>>>> be a seperate property, then it should be a vendor specific property. It
>>>>> is not generic, at all.
>>>>
>>>> Something like "marvell,offload-broken" would be acceptable?
>>>
>>> A tad more, yes. Still, since this is a feature/quirk of the IP core
>>> revision, it should be deduced from the compatible property IMO. It
>>> cannot be configured anywhere, so it doesn't change on board level.
>>
>> So you would prefer using the "marvell,mv78230-a0-i2c" comaptible string and
>> updating it with the follwing piece of code?
> 
> This is the approach I favour, yes. Can't say much about the
> implementation. Looks OK, but dunno if this is minimal...

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

> 


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

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

[Gregory CLEMENT]

On 08/01/2014 14:45, Arnd Bergmann wrote:
> On Wednesday 08 January 2014 14:39:59 Wolfram Sang wrote:
>>>>>>> However, when I first read this I thought it should be a -a0 specific
>>>>>>> compatible string, not a 'offload-broken' property - any idea what the
>>>>>>> DT consensus is here? I've seen both approach in use ..
>>>>>>
>>>>>> I prefer the replacement of the compatible string. If it should really
>>>>>> be a seperate property, then it should be a vendor specific property. It
>>>>>> is not generic, at all.
>>>>>
>>>>> Something like "marvell,offload-broken" would be acceptable?
>>>>
>>>> A tad more, yes. Still, since this is a feature/quirk of the IP core
>>>> revision, it should be deduced from the compatible property IMO. It
>>>> cannot be configured anywhere, so it doesn't change on board level.
>>>
>>> So you would prefer using the "marvell,mv78230-a0-i2c" comaptible string and
>>> updating it with the follwing piece of code?
>>
>> This is the approach I favour, yes. Can't say much about the
>> implementation. Looks OK, but dunno if this is minimal...
>>
> 
> I would prefer the separate property in this case, but only because it's
> easier to add in the quirk than to change the compatible string, but it's
> not a strong preference and I don't mind getting overruled if you all
> favor the alternative.

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


Thanks,

Gregory

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

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

[Gregory CLEMENT]

On 08/01/2014 13:55, Arnd Bergmann wrote:
> On Tuesday 07 January 2014, Gregory CLEMENT wrote:
>> diff --git a/include/linux/mvebu-soc-id.h b/include/linux/mvebu-soc-id.h
>> new file mode 100644
>> index 000000000000..31654252fe35
>> --- /dev/null
>> +++ b/include/linux/mvebu-soc-id.h
>> +#ifdef CONFIG_ARCH_MVEBU
>> +int mvebu_get_soc_id(u32 *dev, u32 *rev);
>> +#else
>> +static inline int mvebu_get_soc_id(u32 *dev, u32 *rev)
>> +{
>> +	return -1;
>> +}
>> +#endif
>> +
>> +#endif /* __LINUX_MVEBU_SOC_ID_H */
> 
> With the quirk handling in patch 3, I think we should remove the public interface
> and EXPORT_SYMBOL(), as well as move this header file into mach-mvebu.
> 
> That said, we may want to add support for the soc_bus later on (not as part of the
> stable bug fix) to make it possible to query the soc version from user space through
> the standard sysfs files.

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


Thanks,

Gregory


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

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

[Gregory CLEMENT]

On 08/01/2014 15:07, Gregory CLEMENT wrote:
> Hi Wolfram,
> 
> On 08/01/2014 14:39, Wolfram Sang wrote:
>>
>>>>>>> However, when I first read this I thought it should be a -a0 specific
>>>>>>> compatible string, not a 'offload-broken' property - any idea what the
>>>>>>> DT consensus is here? I've seen both approach in use ..
>>>>>>
>>>>>> I prefer the replacement of the compatible string. If it should really
>>>>>> be a seperate property, then it should be a vendor specific property. It
>>>>>> is not generic, at all.
>>>>>
>>>>> Something like "marvell,offload-broken" would be acceptable?
>>>>
>>>> A tad more, yes. Still, since this is a feature/quirk of the IP core
>>>> revision, it should be deduced from the compatible property IMO. It
>>>> cannot be configured anywhere, so it doesn't change on board level.
>>>
>>> So you would prefer using the "marvell,mv78230-a0-i2c" comaptible string and
>>> updating it with the follwing piece of code?
>>
>> This is the approach I favour, yes. Can't say much about the
>> implementation. Looks OK, but dunno if this is minimal...
> 
> Allocating memory in each loop could seem convoluted. In my first approach
> I just used a static struct but I got warning during boot about duplicated
> node. It seems we can use the same property struct for two different nodes.

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


> 
>>
> 
> 


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