[RFC/PATCH] ARM: mvebu: Don't apply the quirks if the SoC revision is unknown

[RFC/PATCH] ARM: mvebu: Don't apply the quirks if the SoC revision is unknown

[Ezequiel Garcia]

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(-)

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") {
-- 
1.9.1

[RFC/PATCH] ARM: mvebu: Don't apply the quirks if the SoC revision is unknown

[Arnd Bergmann]

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") {
> 

[RFC/PATCH] ARM: mvebu: Don't apply the quirks if the SoC revision is unknown

[Ezequiel Garcia]

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.
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[RFC/PATCH] ARM: mvebu: Don't apply the quirks if the SoC revision is unknown

[Gregory CLEMENT]

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



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

[RFC/PATCH] ARM: mvebu: Don't apply the quirks if the SoC revision is unknown

[Ezequiel Garcia]

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.
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com