| Message ID | 1389193589-18485-3-git-send-email-gregory.clement@free-electrons.com |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
> + 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 "="
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, 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@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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. >
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. >
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. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
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 >> > >
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); }
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@vger.kernel.org --- arch/arm/mach-mvebu/armada-370-xp.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)