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

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

Quoted 5 lines

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

+		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 "="

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 <[email protected]>

Cc stable at vger.kernel.org

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

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

index e2acff98e750..0f61a4f22fb5 100644
@@ -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;

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

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

Signature · 7 lines

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

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'.

Quoted 4 lines · Jason Cooper

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.

Quoted 6 lines · Jason Gunthorpe

Quoted 4 lines · Jason Cooper

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./

Quoted 2 lines

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?

Quoted 2 lines

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?

Quoted 22 lines · Jason Cooper

Quoted 6 lines · Jason Gunthorpe

Quoted 4 lines · Jason Cooper

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./

Quoted 2 lines

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?

Quoted 5 lines

Quoted 2 lines

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?

Jason,

Quoted 26 lines · Jason Cooper

Quoted 25 lines · Jason Cooper

Quoted 6 lines · Jason Gunthorpe

Quoted 4 lines · Jason Cooper

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./

Quoted 2 lines

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?

Quoted 2 lines

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

Quoted 4 lines

Quoted 2 lines

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

Quoted 31 lines · Gregory CLEMENT

Jason,

Quoted 29 lines · Jason Cooper

Quoted 27 lines · Jason Cooper

Quoted 6 lines · Jason Gunthorpe

Quoted 4 lines · Jason Cooper

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./

Quoted 2 lines

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?

Quoted 2 lines

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?

Quoted 3 lines

Quoted 1 line

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.

Quoted 34 lines · Jason Cooper

Quoted 33 lines · Gregory CLEMENT

Jason,

Quoted 30 lines · Jason Cooper

Quoted 22 lines · Jason Cooper

Quoted 6 lines · Jason Gunthorpe

Quoted 4 lines · Jason Cooper

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./

Quoted 2 lines

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?

Quoted 5 lines

Quoted 2 lines

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?

Quoted 5 lines

Quoted 1 line

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)

Quoted 41 lines · Gregory CLEMENT

Quoted 39 lines · Jason Cooper

Quoted 28 lines · Gregory CLEMENT

Jason,

Quoted 26 lines · Jason Cooper

Quoted 25 lines · Jason Cooper

Quoted 6 lines · Jason Gunthorpe

Quoted 4 lines · Jason Cooper

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./

Quoted 2 lines

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?

Quoted 2 lines

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

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

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?

Quoted 2 lines

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.

Quoted 5 lines

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:

Quoted 1 line

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.

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

Quoted 42 lines · Jason Cooper

Quoted 41 lines · Gregory CLEMENT

Quoted 39 lines · Jason Cooper

Quoted 28 lines · Gregory CLEMENT

Jason,

Quoted 26 lines · Jason Cooper

Quoted 25 lines · Jason Cooper

Quoted 6 lines · Jason Gunthorpe

Quoted 4 lines · Jason Cooper

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./

Quoted 2 lines

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?

Quoted 2 lines

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

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

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?

Quoted 6 lines

Quoted 2 lines

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!

Quoted 5 lines

Quoted 5 lines

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:

Quoted 7 lines

Quoted 1 line

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

Quoted 10 lines

thx,

Jason.

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

Quoted 6 lines · Jason Gunthorpe

Quoted 4 lines · Jason Cooper

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./

Quoted 2 lines

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?

Quoted 2 lines

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.

Hi Jason,

Quoted 22 lines · Jason Cooper

Quoted 6 lines · Jason Gunthorpe

Quoted 4 lines · Jason Cooper

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./

Quoted 2 lines

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?

Quoted 5 lines

Quoted 2 lines

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

Quoted 5 lines

thx,

Jason.

Signature · 4 lines

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

Quoted 2 lines

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?

Quoted 2 lines

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?

Quoted 7 lines · Jason Cooper

Quoted 2 lines

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.

Quoted 3 lines

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.

Quoted 3 lines

Quoted 2 lines

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

Quoted 2 lines

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

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 <[email protected]>

Reported-by Andrew Lunn <[email protected]>

Cc stable at vger.kernel.org

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

drivers/i2c/busses/i2c-mv64xxx.c +0 -0

index 8be7e42aa4de..f424c0f89946 100644

Quoted 16 lines · 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 <[email protected]>

Reported-by Andrew Lunn <[email protected]>

Cc stable at vger.kernel.org

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

drivers/i2c/busses/i2c-mv64xxx.c static const struct of_device_id mv64xxx_i2c_of_match_table[] = { · line 698

Quoted 12 lines

drivers/i2c/busses/i2c-mv64xxx.c +4 -0

index 8be7e42aa4de..f424c0f89946 100644
@@ -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 <[email protected]>

Quoted 7 lines · Wolfram Sang

Quoted 7 lines

drivers/i2c/busses/i2c-mv64xxx.c +0 -0

index 8be7e42aa4de..f424c0f89946 100644
@@ -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},

Quoted 8 lines

drivers/i2c/busses/i2c-mv64xxx.c static const struct of_device_id mv64xxx_i2c_of_match_table[] = { · line 698

Quoted 5 lines

drivers/i2c/busses/i2c-mv64xxx.c +4 -0

     { .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.

Quoted 7 lines

drivers/i2c/busses/i2c-mv64xxx.c static const struct of_device_id mv64xxx_i2c_of_match_table[] = { · line 698

Quoted 4 lines

drivers/i2c/busses/i2c-mv64xxx.c +4 -0

+	{
+		.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.

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 <[email protected]>

cc devicetree at vger.kernel.org

---
 Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

index 82e8f6f17179..9410ed72ec45 100644

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

  - interrupts      : The interrupt number
 
 Optional properties :

Signature · 1 line

1.8.1.2

Gregory,

Quoted 3 lines · 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.

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

Quoted 15 lines

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

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

cc devicetree at vger.kernel.org

---
 Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

index 82e8f6f17179..9410ed72ec45 100644

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

Quoted 5 lines

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

@@ -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.

Quoted 7 lines

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

  - interrupts      : The interrupt number
 
 Optional properties :

Signature · 1 line

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

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?

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

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".

Gregory,

Quoted 11 lines · 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.

Quoted 6 lines

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.

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

Quoted 21 lines · Jason Cooper

Gregory,

Quoted 11 lines · 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.

Quoted 6 lines

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".

Quoted 8 lines

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

Quoted 10 lines

thx,

Jason.

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

Quoted 30 lines · Gregory CLEMENT

Quoted 21 lines · Jason Cooper

Gregory,

Quoted 11 lines · 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.

Quoted 6 lines

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".

Quoted 8 lines

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.

Quoted 8 lines

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:

(quoted diff) +0 -1

-o<---o<---o<---o<---o<---o<---o<---o<---o<---o<---o<--

arch/arm/mach-mvebu/mvebu-soc-id.c +1 -2

@@ -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?