| Message ID | 1389112504-9931-3-git-send-email-gregory.clement@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
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 -- 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 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.
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
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.
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 -- 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 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
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 -- 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
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); }
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@free-electrons.com> --- arch/arm/mach-mvebu/armada-370-xp.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)