| Message ID | 1388743185-24822-3-git-send-email-gregory.clement@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jason, On 03/01/2014 10:59, Gregory CLEMENT wrote: > The first variants of Armada XP SoCs (A0 stepping) have issues related > to the i2c controller which prevent to use the offload mechanism and > lead to a kernel hang during boot. > > The driver now check the revision of the SoC. If the revision is not > more recent than the A0 or if the driver can't get the SoC revision > then it disables the offload mechanism. > > Cc: stable@vger.kernel.org > Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> > Acked-by: Wolfram Sang <wsa@the-dreams.de> I have just realized that it should be fair to add a Reported-by: Andrew Lunn <andrew@lunn.ch> Thanks, Gregory > --- > drivers/i2c/busses/i2c-mv64xxx.c | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c > index 8be7e42aa4de..634b04d241fb 100644 > --- a/drivers/i2c/busses/i2c-mv64xxx.c > +++ b/drivers/i2c/busses/i2c-mv64xxx.c > @@ -24,6 +24,7 @@ > #include <linux/clk.h> > #include <linux/err.h> > #include <linux/delay.h> > +#include <linux/mvebu-soc-id.h> > > #define MV64XXX_I2C_ADDR_ADDR(val) ((val & 0x7f) << 1) > #define MV64XXX_I2C_BAUD_DIV_N(val) (val & 0x7) > @@ -779,8 +780,16 @@ 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; > + u32 dev, rev; > + > drv_data->errata_delay = true; > + /* > + * Only revison more recent than A0 support offload > + * mechanism. In case we can't get the SoC revision > + * weplay safe and we don't enable it > + */ > + if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV) > + drv_data->offload_enabled = true; > } > > out: >
Dear Gregory CLEMENT, On Fri, 3 Jan 2014 10:59:45 +0100, Gregory CLEMENT wrote: > + /* > + * Only revison more recent than A0 support offload revison -> revisions offload mechanism -> the offload mechanism > + * mechanism. In case we can't get the SoC revision > + * weplay safe and we don't enable it weplay -> we play > + */ > + if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV) > + drv_data->offload_enabled = true; > } > > out: Thanks! Thomas
On 03/01/2014 19:49, Thomas Petazzoni wrote: > Dear Gregory CLEMENT, > > On Fri, 3 Jan 2014 10:59:45 +0100, Gregory CLEMENT wrote: > >> + /* >> + * Only revison more recent than A0 support offload > > revison -> revisions > > offload mechanism -> the offload mechanism > >> + * mechanism. In case we can't get the SoC revision >> + * weplay safe and we don't enable it > > weplay -> we play > >> + */ >> + if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV) >> + drv_data->offload_enabled = true; >> } >> >> out: > These typos will be fixed in the next version. > Thanks! > > Thomas >
On Friday 03 January 2014, Gregory CLEMENT wrote: > The first variants of Armada XP SoCs (A0 stepping) have issues related > to the i2c controller which prevent to use the offload mechanism and > lead to a kernel hang during boot. > > The driver now check the revision of the SoC. If the revision is not > more recent than the A0 or if the driver can't get the SoC revision > then it disables the offload mechanism. > > Cc: stable@vger.kernel.org > Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> > Acked-by: Wolfram Sang <wsa@the-dreams.de> Relying on the soc id patch for a "stable" bug fix seems a little far-reaching to me. I would be happier to first try to do a local detection based on the i2c bus device node itself. Do you know how common the A0 revision is? You mention "early release of the OpenBlocks AX3-4 boards". Any others that you suspect? If not, how about adding either a boolean property in the node or a new "compatible" value to distinguish the working version from the broken one? If A0 is very common, you might do the same thing in the opposite way and default to "broken" unless it is explicitly known to be the good version. In general, I'm much in favor of keeping "quirks" local to device drivers if possible and not rely on global system state. Arnd
Hi Arnd, On 05/01/2014 15:33, Arnd Bergmann wrote: > On Friday 03 January 2014, Gregory CLEMENT wrote: >> The first variants of Armada XP SoCs (A0 stepping) have issues related >> to the i2c controller which prevent to use the offload mechanism and >> lead to a kernel hang during boot. >> >> The driver now check the revision of the SoC. If the revision is not >> more recent than the A0 or if the driver can't get the SoC revision >> then it disables the offload mechanism. >> >> Cc: stable@vger.kernel.org >> Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> >> Acked-by: Wolfram Sang <wsa@the-dreams.de> > > Relying on the soc id patch for a "stable" bug fix seems a little > far-reaching to me. I would be happier to first try to do a local > detection based on the i2c bus device node itself. Do you know how It was my first proposal in case adding the soc id detection was a too big things. But it turned out that the amount of code is very low so I really think it worth adding it along the fix. Device tree is supposed to be stable so as soon as we add something in it we are supposed support it forever. Moreover using device tree for something we can probe is counter productive. > common the A0 revision is? You mention "early release of the > OpenBlocks AX3-4 boards". Any others that you suspect? If not, No, from the info I gathered I expected that only OpenBlocks AX3-4 would be the only product shipped with an A0 version and as I said it should be only a limit amount of them. > how about adding either a boolean property in the node or a > new "compatible" value to distinguish the working version from > the broken one? > > If A0 is very common, you might do the same thing in the opposite > way and default to "broken" unless it is explicitly known to be > the good version. In general, I'm much in favor of keeping "quirks" > local to device drivers if possible and not rely on global system > state. > > Arnd >
On Monday 06 January 2014, Gregory CLEMENT wrote: > > Relying on the soc id patch for a "stable" bug fix seems a little > > far-reaching to me. I would be happier to first try to do a local > > detection based on the i2c bus device node itself. Do you know how > > It was my first proposal in case adding the soc id detection was > a too big things. But it turned out that the amount of code is very > low so I really think it worth adding it along the fix. Device tree > is supposed to be stable so as soon as we add something in it we are > supposed support it forever. Moreover using device tree for something > we can probe is counter productive. I would still be happier if we did both and only need to check the SoC version if the device is in the "possibly broken" category but default to the existing behavior. My main concern is that this patch is adding platform code that we'd otherwise have to carry in the kernel indefinitely. I agree that it's best if we can probe stuff automatically, but that doesn't normally mean looking at an unrelated piece of information. If the i2c controller registers themselves tell us whether this device is broken or not, we should use that information. Looking at a global SoC version register however is more like checking a board ID in the pre-DT days where the board number is the only information we have and everything is derived from that. > > common the A0 revision is? You mention "early release of the > > OpenBlocks AX3-4 boards". Any others that you suspect? If not, > > No, from the info I gathered I expected that only OpenBlocks AX3-4 > would be the only product shipped with an A0 version and as I said > it should be only a limit amount of them. Ok, good. So we really only need to worry about this one board for now and can make all the others default to normal operation without checking the SoC version. Another idea: Could we have a quirk in the mvebu platform code for the AX3-4 to check the SoC version and then change the property for the i2c controller based on whether we expect it to work or not? This way, we wouldn't even need an interface between the platform code and the driver code. Arnd
On 07/01/2014 10:03, Arnd Bergmann wrote: > On Monday 06 January 2014, Gregory CLEMENT wrote: >>> Relying on the soc id patch for a "stable" bug fix seems a little >>> far-reaching to me. I would be happier to first try to do a local >>> detection based on the i2c bus device node itself. Do you know how >> >> It was my first proposal in case adding the soc id detection was >> a too big things. But it turned out that the amount of code is very >> low so I really think it worth adding it along the fix. Device tree >> is supposed to be stable so as soon as we add something in it we are >> supposed support it forever. Moreover using device tree for something >> we can probe is counter productive. > > I would still be happier if we did both and only need to check the > SoC version if the device is in the "possibly broken" category > but default to the existing behavior. > > My main concern is that this patch is adding platform code that we'd > otherwise have to carry in the kernel indefinitely. I agree that > it's best if we can probe stuff automatically, but that doesn't normally > mean looking at an unrelated piece of information. If the i2c controller > registers themselves tell us whether this device is broken or not, > we should use that information. Looking at a global SoC version register > however is more like checking a board ID in the pre-DT days where the > board number is the only information we have and everything is > derived from that. Well the way the hardware is designed is exactly like this: between two revision of a SoC you can have slightly differences between various IP and most of the time this IP don't have a specific register for it. Moreover from my experience a change done in a IP of a given revision of a SoC will be for this revision and not necessary reported in future generation of a SoC. Most of the time the IP are not really under a VCS. That means that the SoC ID is the only reliable information to know the version of most of the IP inside this SoC. > >>> common the A0 revision is? You mention "early release of the >>> OpenBlocks AX3-4 boards". Any others that you suspect? If not, >> >> No, from the info I gathered I expected that only OpenBlocks AX3-4 >> would be the only product shipped with an A0 version and as I said >> it should be only a limit amount of them. > > Ok, good. So we really only need to worry about this one board for > now and can make all the others default to normal operation without > checking the SoC version. > > Another idea: Could we have a quirk in the mvebu platform code for > the AX3-4 to check the SoC version and then change the property for > the i2c controller based on whether we expect it to work or not? > This way, we wouldn't even need an interface between the platform > code and the driver code. I can try this last approach: using the device tree to pass platform parameter from the arch part to the driver. > > 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 Tuesday 07 January 2014, Gregory CLEMENT wrote: > On 07/01/2014 10:03, Arnd Bergmann wrote: > > On Monday 06 January 2014, Gregory CLEMENT wrote: > > My main concern is that this patch is adding platform code that we'd > > otherwise have to carry in the kernel indefinitely. I agree that > > it's best if we can probe stuff automatically, but that doesn't normally > > mean looking at an unrelated piece of information. If the i2c controller > > registers themselves tell us whether this device is broken or not, > > we should use that information. Looking at a global SoC version register > > however is more like checking a board ID in the pre-DT days where the > > board number is the only information we have and everything is > > derived from that. > > Well the way the hardware is designed is exactly like this: between > two revision of a SoC you can have slightly differences between various > IP and most of the time this IP don't have a specific register for it. > Moreover from my experience a change done in a IP of a given revision > of a SoC will be for this revision and not necessary reported in future > generation of a SoC. Most of the time the IP are not really under a VCS. > That means that the SoC ID is the only reliable information to know > the version of most of the IP inside this SoC. Right. This is not exactly what I'd call "discoverable" though: ideally we would have a way to ask the hardware for the availability of specific features, but that clearly doesn't work here, which leaves the default way to handle it by using DT properties to describe it in software. In case of the AX3-4 board, that would unfortunately imply that we would either need two separate dtb files or fall back to the workaround for all of them. Neither approach seems particularly user-friendly, so some form of autodetection seems appropriate. > > Another idea: Could we have a quirk in the mvebu platform code for > > the AX3-4 to check the SoC version and then change the property for > > the i2c controller based on whether we expect it to work or not? > > This way, we wouldn't even need an interface between the platform > > code and the driver code. > > I can try this last approach: using the device tree to pass platform > parameter from the arch part to the driver. Ok, thanks! Arnd
diff --git a/drivers/i2c/busses/i2c-mv64xxx.c b/drivers/i2c/busses/i2c-mv64xxx.c index 8be7e42aa4de..634b04d241fb 100644 --- a/drivers/i2c/busses/i2c-mv64xxx.c +++ b/drivers/i2c/busses/i2c-mv64xxx.c @@ -24,6 +24,7 @@ #include <linux/clk.h> #include <linux/err.h> #include <linux/delay.h> +#include <linux/mvebu-soc-id.h> #define MV64XXX_I2C_ADDR_ADDR(val) ((val & 0x7f) << 1) #define MV64XXX_I2C_BAUD_DIV_N(val) (val & 0x7) @@ -779,8 +780,16 @@ 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; + u32 dev, rev; + drv_data->errata_delay = true; + /* + * Only revison more recent than A0 support offload + * mechanism. In case we can't get the SoC revision + * weplay safe and we don't enable it + */ + if (mvebu_get_soc_id(&rev, &dev) == 0 && dev > MV78XX0_A0_REV) + drv_data->offload_enabled = true; } out: