| Message ID | 1402342036-9168-1-git-send-email-ezequiel.garcia@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Monday 09 June 2014 16:27:16 Ezequiel Garcia wrote: > We currently skip the I2C and thermal quirks only if the SoC revision is > known to be one that does not need them. If the SoC revision cannot be > obtained, the current behavior is to apply the quirk assuming it's needed. > > This commit changes this, by requiring the SoC revision to be known in order > to peform a quirk. This clearly needs a better description if we want to apply it. We had a rather long discussion when the code was first added exactly this way and you should explain which of the assumptions we made back then are now incorrect. Is it ever wrong (as opposed to inefficient) to apply the quirk even on a newer SoC? IIRC, the reasoning was that almost all machines have the new SoC version, so they should have their DTs marked with the correct IDs already. Arnd > diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c > index 594262b..14bf590 100644 > --- a/arch/arm/mach-mvebu/board-v7.c > +++ b/arch/arm/mach-mvebu/board-v7.c > @@ -89,13 +89,15 @@ static void __init i2c_quirk(void) > { > struct device_node *np; > u32 dev, rev; > + int res; > > /* > * 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(&dev, &rev) == 0 && rev > MV78XX0_A0_REV) > + res = mvebu_get_soc_id(&dev, &rev); > + if (res < 0 || (res == 0 && rev > MV78XX0_A0_REV)) > return; > > for_each_compatible_node(np, NULL, "marvell,mv78230-i2c") { > @@ -119,8 +121,10 @@ static void __init thermal_quirk(void) > { > struct device_node *np; > u32 dev, rev; > + int res; > > - if (mvebu_get_soc_id(&dev, &rev) == 0 && rev > ARMADA_375_Z1_REV) > + res = mvebu_get_soc_id(&dev, &rev); > + if (res < 0 || (res == 0 && rev > ARMADA_375_Z1_REV)) > return; > > for_each_compatible_node(np, NULL, "marvell,armada375-thermal") { >
Hi Arnd, Thanks for taking a look. On 10 Jun 10:21 AM, Arnd Bergmann wrote: > On Monday 09 June 2014 16:27:16 Ezequiel Garcia wrote: > > We currently skip the I2C and thermal quirks only if the SoC revision is > > known to be one that does not need them. If the SoC revision cannot be > > obtained, the current behavior is to apply the quirk assuming it's needed. > > > > This commit changes this, by requiring the SoC revision to be known in order > > to peform a quirk. > > This clearly needs a better description if we want to apply it. We had > a rather long discussion when the code was first added exactly this > way and you should explain which of the assumptions we made back then > are now incorrect. > > Is it ever wrong (as opposed to inefficient) to apply the quirk even on a > newer SoC? > Yes, for the thermal quirk it is wrong as it consists in changing the compatible string and moving the registers around. So if you apply the quirk on a SoC that doesn't need it, thermal won't work. > IIRC, the reasoning was that almost all machines have the new SoC version, Agreed, and that's why I think it's better to *not* apply the quirk unless the SoC revision can be properly obtained and matched as needing it. > so they should have their DTs marked with the correct IDs already. > Hm.. I guess the I2C and thermal quirks don't work the same way. Here's how the latter works. The driver supports two different compatible strings: "armada375-thermal" and "armada375-z1-thermal". However, we don't expect the user to know the revision, so the quirk code detects the SoC revision and changes the compatible string if needed. In addition, given the register offsets are different in the Z1 revision, we workaround that by moving them in the quirk code, before the thermal driver probes. So, there's no "correct ID in the devicetree", as we assume the devicetree works for the new SoC and only apply changes if we detect the SoC revision needs the workaround. Under this scheme, this commit makes a requirement to know the SoC revision before applying a quirk, where the current code applies it blindly. However, I'm not sure if this is correct for the I2C quirk. Maybe I was too fast assuming it was.
On 10/06/2014 15:40, Ezequiel Garcia wrote: > Hi Arnd, > > Thanks for taking a look. > > On 10 Jun 10:21 AM, Arnd Bergmann wrote: >> On Monday 09 June 2014 16:27:16 Ezequiel Garcia wrote: >>> We currently skip the I2C and thermal quirks only if the SoC revision is >>> known to be one that does not need them. If the SoC revision cannot be >>> obtained, the current behavior is to apply the quirk assuming it's needed. >>> >>> This commit changes this, by requiring the SoC revision to be known in order >>> to peform a quirk. >> >> This clearly needs a better description if we want to apply it. We had >> a rather long discussion when the code was first added exactly this >> way and you should explain which of the assumptions we made back then >> are now incorrect. >> >> Is it ever wrong (as opposed to inefficient) to apply the quirk even on a >> newer SoC? >> > > Yes, for the thermal quirk it is wrong as it consists in changing the compatible > string and moving the registers around. > > So if you apply the quirk on a SoC that doesn't need it, thermal won't work. Actually it is the opposite for the I2C quirk. If you don't apply it on an SoC which needs it then the i2C won't work, whereas if you apply it on an SoC which don't need it, then you won't benefit of an optimization but the I2C will remain usable. So with your change we can have a situation where the i2c is no more usable. That's why I would prefer that you don't modify the i2c quirk. Thanks, Gregory
Hi Gregory, On 10 Jun 05:39 PM, Gregory CLEMENT wrote: > On 10/06/2014 15:40, Ezequiel Garcia wrote: > > On 10 Jun 10:21 AM, Arnd Bergmann wrote: > >> On Monday 09 June 2014 16:27:16 Ezequiel Garcia wrote: > >>> We currently skip the I2C and thermal quirks only if the SoC revision is > >>> known to be one that does not need them. If the SoC revision cannot be > >>> obtained, the current behavior is to apply the quirk assuming it's needed. > >>> > >>> This commit changes this, by requiring the SoC revision to be known in order > >>> to peform a quirk. > >> > >> This clearly needs a better description if we want to apply it. We had > >> a rather long discussion when the code was first added exactly this > >> way and you should explain which of the assumptions we made back then > >> are now incorrect. > >> > >> Is it ever wrong (as opposed to inefficient) to apply the quirk even on a > >> newer SoC? > >> > > > > Yes, for the thermal quirk it is wrong as it consists in changing the compatible > > string and moving the registers around. > > > > So if you apply the quirk on a SoC that doesn't need it, thermal won't work. > > Actually it is the opposite for the I2C quirk. If you don't apply it on an SoC > which needs it then the i2C won't work, whereas if you apply it on an SoC which > don't need it, then you won't benefit of an optimization but the I2C will remain > usable. > > So with your change we can have a situation where the i2c is no more usable. > That's why I would prefer that you don't modify the i2c quirk. > Thanks a lot for clarifying this point. I'll prepare a v2, changing only the thermal quirk, and explaining the difference in a comment and in the commit log.
diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c index 594262b..14bf590 100644 --- a/arch/arm/mach-mvebu/board-v7.c +++ b/arch/arm/mach-mvebu/board-v7.c @@ -89,13 +89,15 @@ static void __init i2c_quirk(void) { struct device_node *np; u32 dev, rev; + int res; /* * 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(&dev, &rev) == 0 && rev > MV78XX0_A0_REV) + res = mvebu_get_soc_id(&dev, &rev); + if (res < 0 || (res == 0 && rev > MV78XX0_A0_REV)) return; for_each_compatible_node(np, NULL, "marvell,mv78230-i2c") { @@ -119,8 +121,10 @@ static void __init thermal_quirk(void) { struct device_node *np; u32 dev, rev; + int res; - if (mvebu_get_soc_id(&dev, &rev) == 0 && rev > ARMADA_375_Z1_REV) + res = mvebu_get_soc_id(&dev, &rev); + if (res < 0 || (res == 0 && rev > ARMADA_375_Z1_REV)) return; for_each_compatible_node(np, NULL, "marvell,armada375-thermal") {
We currently skip the I2C and thermal quirks only if the SoC revision is known to be one that does not need them. If the SoC revision cannot be obtained, the current behavior is to apply the quirk assuming it's needed. This commit changes this, by requiring the SoC revision to be known in order to peform a quirk. Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> --- arch/arm/mach-mvebu/board-v7.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)