All of lore.kernel.org
 help / color / mirror / Atom feed
* [Patch V2 0/2] Generalize use of i2c quirk for Armada XP
@ 2014-07-26 17:20 Andrew Lunn
  2014-07-26 17:20 ` [Patch V2 1/2] ARM: mvebu: armada xp: Generalize use of i2c quirk Andrew Lunn
  2014-07-26 17:20 ` [Patch V2 2/2] i2c: mv64xxx: Remove internal compatible string from Documentation Andrew Lunn
  0 siblings, 2 replies; 16+ messages in thread
From: Andrew Lunn @ 2014-07-26 17:20 UTC (permalink / raw)
  To: linux-arm-kernel

As requested by Arnd, a second patch now removes the documentation of
the -a0 compatibility string.

Acked-by's have been added to the first patch, and a minor nit fixed.

Andrew Lunn (2):
  ARM: mvebu: armada xp: Generalize use of i2c quirk
  i2c: mv64xxx: Remove internal compatible string from Documentation

 Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 5 -----
 arch/arm/mach-mvebu/board-v7.c                        | 2 +-
 2 files changed, 1 insertion(+), 6 deletions(-)

-- 
2.0.1

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Patch V2 1/2] ARM: mvebu: armada xp: Generalize use of i2c quirk
  2014-07-26 17:20 [Patch V2 0/2] Generalize use of i2c quirk for Armada XP Andrew Lunn
@ 2014-07-26 17:20 ` Andrew Lunn
  2014-10-15 15:25   ` Jason Cooper
  2014-07-26 17:20 ` [Patch V2 2/2] i2c: mv64xxx: Remove internal compatible string from Documentation Andrew Lunn
  1 sibling, 1 reply; 16+ messages in thread
From: Andrew Lunn @ 2014-07-26 17:20 UTC (permalink / raw)
  To: linux-arm-kernel

A second product has come to light which makes use of the A0 stepping
of the Armada XP SoC. A0 stepping has a hardware bug in the i2c core
meaning that hardware offload does not work, resulting in the kernel
failing to boot. The quirk detects that the kernel is running on an A0
stepping SoC and disables the use of hardware offload.

Currently the quirk is only enabled for PlatHome Openblocks AX3. The
AX3 has been produced with both A0 and B0 stepping SoCs. The second
product is the Lenovo Iomega IX4-300d. It seems likely that this
device will also swap from A0 to B0 SoC sometime during its life.

If there are two products using A0, it seems likely there are more
products with A0. Also, since the number of A0 SoCs is limited, these
products are also likely to transition to B0. Hence detecting at run
time is the safest option. So enable the quirk for all Armada XP
boards.

Tested on an AX3 with A0 stepping.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Acked-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---

V2:
Added two Acked-by:
s/B1/B0
---
 arch/arm/mach-mvebu/board-v7.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-mvebu/board-v7.c b/arch/arm/mach-mvebu/board-v7.c
index 8bb742fdf5ca..be080cff98d2 100644
--- a/arch/arm/mach-mvebu/board-v7.c
+++ b/arch/arm/mach-mvebu/board-v7.c
@@ -167,7 +167,7 @@ static void __init thermal_quirk(void)
 
 static void __init mvebu_dt_init(void)
 {
-	if (of_machine_is_compatible("plathome,openblocks-ax3-4"))
+	if (of_machine_is_compatible("marvell,armadaxp"))
 		i2c_quirk();
 	if (of_machine_is_compatible("marvell,a375-db"))
 		thermal_quirk();
-- 
2.0.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Patch V2 2/2] i2c: mv64xxx: Remove internal compatible string from Documentation
  2014-07-26 17:20 [Patch V2 0/2] Generalize use of i2c quirk for Armada XP Andrew Lunn
  2014-07-26 17:20 ` [Patch V2 1/2] ARM: mvebu: armada xp: Generalize use of i2c quirk Andrew Lunn
@ 2014-07-26 17:20 ` Andrew Lunn
  2014-07-28 13:22   ` Jason Cooper
  1 sibling, 1 reply; 16+ messages in thread
From: Andrew Lunn @ 2014-07-26 17:20 UTC (permalink / raw)
  To: linux-arm-kernel

The compatible string "marvell,mv78230-a0-i2c" should only be used
internally when at run time we detect an Armada XP with broken
hardware offload. This string should never appear in DT blobs passed
to the kernel. Hence remove it from the binding Documentation.

Patch proposed by Arnd Bergmann <arnd@arndb.de>

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
index 5c30026921ae..6eb6f6e40ba1 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
@@ -9,11 +9,6 @@ Required properties :
                      - "allwinner,sun6i-a31-i2c"
                      - "marvell,mv64xxx-i2c"
                      - "marvell,mv78230-i2c"
-                     - "marvell,mv78230-a0-i2c"
-                       * Note: Only use "marvell,mv78230-a0-i2c" for a
-                         very rare, initial version of the SoC which
-                         had broken offload support.  Linux
-                         auto-detects this and sets it appropriately.
  - interrupts      : The interrupt number
 
 Optional properties :
-- 
2.0.1

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Patch V2 2/2] i2c: mv64xxx: Remove internal compatible string from Documentation
  2014-07-26 17:20 ` [Patch V2 2/2] i2c: mv64xxx: Remove internal compatible string from Documentation Andrew Lunn
@ 2014-07-28 13:22   ` Jason Cooper
  2014-07-28 13:27     ` Andrew Lunn
  0 siblings, 1 reply; 16+ messages in thread
From: Jason Cooper @ 2014-07-28 13:22 UTC (permalink / raw)
  To: linux-arm-kernel

Andrew,

I'm fine with this series wrt to fixing Linux, however:

On Sat, Jul 26, 2014 at 07:20:38PM +0200, Andrew Lunn wrote:
> The compatible string "marvell,mv78230-a0-i2c" should only be used
> internally when at run time we detect an Armada XP with broken
> hardware offload. This string should never appear in DT blobs passed
> to the kernel. Hence remove it from the binding Documentation.
> 
> Patch proposed by Arnd Bergmann <arnd@arndb.de>
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> index 5c30026921ae..6eb6f6e40ba1 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> @@ -9,11 +9,6 @@ Required properties :
>                       - "allwinner,sun6i-a31-i2c"
>                       - "marvell,mv64xxx-i2c"
>                       - "marvell,mv78230-i2c"
> -                     - "marvell,mv78230-a0-i2c"
> -                       * Note: Only use "marvell,mv78230-a0-i2c" for a
> -                         very rare, initial version of the SoC which
> -                         had broken offload support.  Linux
> -                         auto-detects this and sets it appropriately.

I think we're losing knowledge here.  *We* know that attempting to
enable transaction offload on A0 SoCs is bad news.  Other OS's would now
need to dig through the Linux kernel code for clues as to what's
happening.

Perhaps we should retain the info in the form of a note at the bottom of
this file?

thx,

Jason.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Patch V2 2/2] i2c: mv64xxx: Remove internal compatible string from Documentation
  2014-07-28 13:22   ` Jason Cooper
@ 2014-07-28 13:27     ` Andrew Lunn
  2014-07-28 13:51       ` Arnd Bergmann
  0 siblings, 1 reply; 16+ messages in thread
From: Andrew Lunn @ 2014-07-28 13:27 UTC (permalink / raw)
  To: linux-arm-kernel

> > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> > index 5c30026921ae..6eb6f6e40ba1 100644
> > --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> > +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> > @@ -9,11 +9,6 @@ Required properties :
> >                       - "allwinner,sun6i-a31-i2c"
> >                       - "marvell,mv64xxx-i2c"
> >                       - "marvell,mv78230-i2c"
> > -                     - "marvell,mv78230-a0-i2c"
> > -                       * Note: Only use "marvell,mv78230-a0-i2c" for a
> > -                         very rare, initial version of the SoC which
> > -                         had broken offload support.  Linux
> > -                         auto-detects this and sets it appropriately.
> 
> I think we're losing knowledge here.  *We* know that attempting to
> enable transaction offload on A0 SoCs is bad news.  Other OS's would now
> need to dig through the Linux kernel code for clues as to what's
> happening.
> 
> Perhaps we should retain the info in the form of a note at the bottom of
> this file?

Hi Jason

I did wounder about this a bit. I've not looked, but now that XP
datasheets are public, i assume there is an errata for this, so it at
least should be documented by Marvell. But anybody looking in
/proc/device-tree on an A0 is going to see this undocumented string
which might raise questions.

So i'm happy to document it at the end of the binding.

Arnd, what do you say?

      Andrew

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Patch V2 2/2] i2c: mv64xxx: Remove internal compatible string from Documentation
  2014-07-28 13:27     ` Andrew Lunn
@ 2014-07-28 13:51       ` Arnd Bergmann
  2014-07-28 14:12         ` Andrew Lunn
                           ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Arnd Bergmann @ 2014-07-28 13:51 UTC (permalink / raw)
  To: linux-arm-kernel

On Monday 28 July 2014 15:27:16 Andrew Lunn wrote:
> > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> > > index 5c30026921ae..6eb6f6e40ba1 100644
> > > --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> > > +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> > > @@ -9,11 +9,6 @@ Required properties :
> > >                       - "allwinner,sun6i-a31-i2c"
> > >                       - "marvell,mv64xxx-i2c"
> > >                       - "marvell,mv78230-i2c"
> > > -                     - "marvell,mv78230-a0-i2c"
> > > -                       * Note: Only use "marvell,mv78230-a0-i2c" for a
> > > -                         very rare, initial version of the SoC which
> > > -                         had broken offload support.  Linux
> > > -                         auto-detects this and sets it appropriately.
> > 
> > I think we're losing knowledge here.  *We* know that attempting to
> > enable transaction offload on A0 SoCs is bad news.  Other OS's would now
> > need to dig through the Linux kernel code for clues as to what's
> > happening.
> > 
> > Perhaps we should retain the info in the form of a note at the bottom of
> > this file?
> 
> Hi Jason
> 
> I did wounder about this a bit. I've not looked, but now that XP
> datasheets are public, i assume there is an errata for this, so it at
> least should be documented by Marvell. But anybody looking in
> /proc/device-tree on an A0 is going to see this undocumented string
> which might raise questions.
> 
> So i'm happy to document it at the end of the binding.
> 
> Arnd, what do you say?

The final consequence of the API change would be to no longer
change the compatible string in the fixup, but instead to call an
API from the driver to find out the SoC revision when it encounters
an mv78230-i2c.

I remember this being discussed when the quirk was initially added,
but it seemed cleaner to handle this in the platform code at the time
when it was just for one particular board. Now that it's basically
an accepted feature of the i2c device that you have to know the
SoC version, that should probably become a proper API.

Also, we now have drivers/soc/ and can move the soc-id code there
with a publically documented API.

I agree it's sad to lose the knowledge about the revision, but we
already have that as we now accept the generic string even on the
a0 machines other than ax3.

	Arnd

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Patch V2 2/2] i2c: mv64xxx: Remove internal compatible string from Documentation
  2014-07-28 13:51       ` Arnd Bergmann
@ 2014-07-28 14:12         ` Andrew Lunn
  2014-07-28 14:35           ` Arnd Bergmann
  2014-07-28 18:29         ` Jason Gunthorpe
  2014-07-30  8:44         ` Maxime Ripard
  2 siblings, 1 reply; 16+ messages in thread
From: Andrew Lunn @ 2014-07-28 14:12 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 28, 2014 at 03:51:27PM +0200, Arnd Bergmann wrote:
> On Monday 28 July 2014 15:27:16 Andrew Lunn wrote:
> > > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> > > > index 5c30026921ae..6eb6f6e40ba1 100644
> > > > --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> > > > +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> > > > @@ -9,11 +9,6 @@ Required properties :
> > > >                       - "allwinner,sun6i-a31-i2c"
> > > >                       - "marvell,mv64xxx-i2c"
> > > >                       - "marvell,mv78230-i2c"
> > > > -                     - "marvell,mv78230-a0-i2c"
> > > > -                       * Note: Only use "marvell,mv78230-a0-i2c" for a
> > > > -                         very rare, initial version of the SoC which
> > > > -                         had broken offload support.  Linux
> > > > -                         auto-detects this and sets it appropriately.
> > > 
> > > I think we're losing knowledge here.  *We* know that attempting to
> > > enable transaction offload on A0 SoCs is bad news.  Other OS's would now
> > > need to dig through the Linux kernel code for clues as to what's
> > > happening.
> > > 
> > > Perhaps we should retain the info in the form of a note at the bottom of
> > > this file?
> > 
> > Hi Jason
> > 
> > I did wounder about this a bit. I've not looked, but now that XP
> > datasheets are public, i assume there is an errata for this, so it at
> > least should be documented by Marvell. But anybody looking in
> > /proc/device-tree on an A0 is going to see this undocumented string
> > which might raise questions.
> > 
> > So i'm happy to document it at the end of the binding.
> > 
> > Arnd, what do you say?
> 
> The final consequence of the API change would be to no longer
> change the compatible string in the fixup, but instead to call an
> API from the driver to find out the SoC revision when it encounters
> an mv78230-i2c.
> 
> I remember this being discussed when the quirk was initially added,
> but it seemed cleaner to handle this in the platform code at the time
> when it was just for one particular board. Now that it's basically
> an accepted feature of the i2c device that you have to know the
> SoC version, that should probably become a proper API.
> 
> Also, we now have drivers/soc/ and can move the soc-id code there
> with a publically documented API.

Getting the SoC ID and revision seems like something that should be
generic. Would it be better to make this part of drivers/base/soc.c?
mvebu already does a soc_device_register() with the relevant
information.

Add a call something like:

/*
 * Return the soc device attributes for a given soc_dev. If soc_dev is NULL,
 * the first device on the soc bus is returned.
 */
struct soc_device_attribute *soc_attribute_get(struct soc_device * soc_dev);

       Andrew

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Patch V2 2/2] i2c: mv64xxx: Remove internal compatible string from Documentation
  2014-07-28 14:12         ` Andrew Lunn
@ 2014-07-28 14:35           ` Arnd Bergmann
  2014-07-28 15:25             ` Andrew Lunn
  0 siblings, 1 reply; 16+ messages in thread
From: Arnd Bergmann @ 2014-07-28 14:35 UTC (permalink / raw)
  To: linux-arm-kernel

On Monday 28 July 2014 16:12:17 Andrew Lunn wrote:
> > 
> > I remember this being discussed when the quirk was initially added,
> > but it seemed cleaner to handle this in the platform code at the time
> > when it was just for one particular board. Now that it's basically
> > an accepted feature of the i2c device that you have to know the
> > SoC version, that should probably become a proper API.
> > 
> > Also, we now have drivers/soc/ and can move the soc-id code there
> > with a publically documented API.
> 
> Getting the SoC ID and revision seems like something that should be
> generic. Would it be better to make this part of drivers/base/soc.c?
> mvebu already does a soc_device_register() with the relevant
> information.
> 
> Add a call something like:
> 
> /*
>  * Return the soc device attributes for a given soc_dev. If soc_dev is NULL,
>  * the first device on the soc bus is returned.
>  */
> struct soc_device_attribute *soc_attribute_get(struct soc_device * soc_dev);

Interesting idea, yes.

There could also be a higher-level function that does a strcmp() in addition,
so that a driver can do some variation of

	if (soc_name_is("Armada XP") && soc_revision_is("A0"))
		...

	Arnd

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Patch V2 2/2] i2c: mv64xxx: Remove internal compatible string from Documentation
  2014-07-28 14:35           ` Arnd Bergmann
@ 2014-07-28 15:25             ` Andrew Lunn
  2014-07-28 15:47               ` Arnd Bergmann
  0 siblings, 1 reply; 16+ messages in thread
From: Andrew Lunn @ 2014-07-28 15:25 UTC (permalink / raw)
  To: linux-arm-kernel

> > Add a call something like:
> > 
> > /*
> >  * Return the soc device attributes for a given soc_dev. If soc_dev is NULL,
> >  * the first device on the soc bus is returned.
> >  */
> > struct soc_device_attribute *soc_attribute_get(struct soc_device * soc_dev);
> 
> Interesting idea, yes.

O.K, i will take a shot of implementing this sometime in the next few
weeks. Bit busy at the moment.
 
> There could also be a higher-level function that does a strcmp() in addition,
> so that a driver can do some variation of
> 
> 	if (soc_name_is("Armada XP") && soc_revision_is("A0"))

We don't translate into 'Marketing names'. We just turn the register
values into a stringified number. So it would have to be something
like:

	if (soc_name_is("7826") && soc_revision_is("1"))

	Andrew

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Patch V2 2/2] i2c: mv64xxx: Remove internal compatible string from Documentation
  2014-07-28 15:25             ` Andrew Lunn
@ 2014-07-28 15:47               ` Arnd Bergmann
  2014-07-28 15:52                 ` Andrew Lunn
  0 siblings, 1 reply; 16+ messages in thread
From: Arnd Bergmann @ 2014-07-28 15:47 UTC (permalink / raw)
  To: linux-arm-kernel

On Monday 28 July 2014 17:25:06 Andrew Lunn wrote:
> > > Add a call something like:
> > > 
> > > /*
> > >  * Return the soc device attributes for a given soc_dev. If soc_dev is NULL,
> > >  * the first device on the soc bus is returned.
> > >  */
> > > struct soc_device_attribute *soc_attribute_get(struct soc_device * soc_dev);
> > 
> > Interesting idea, yes.
> 
> O.K, i will take a shot of implementing this sometime in the next few
> weeks. Bit busy at the moment.
>  
> > There could also be a higher-level function that does a strcmp() in addition,
> > so that a driver can do some variation of
> > 
> >       if (soc_name_is("Armada XP") && soc_revision_is("A0"))
> 
> We don't translate into 'Marketing names'. We just turn the register
> values into a stringified number. So it would have to be something
> like:
> 
>         if (soc_name_is("7826") && soc_revision_is("1"))

Hmm, I think that is inconsistent with how the strings are used
on other platforms, where you want something that uniquely identifies
the device in user-readable strings.

I haven't checked how all the others are doing it though.

	Arnd

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Patch V2 2/2] i2c: mv64xxx: Remove internal compatible string from Documentation
  2014-07-28 15:47               ` Arnd Bergmann
@ 2014-07-28 15:52                 ` Andrew Lunn
  2014-07-28 16:03                   ` Arnd Bergmann
  0 siblings, 1 reply; 16+ messages in thread
From: Andrew Lunn @ 2014-07-28 15:52 UTC (permalink / raw)
  To: linux-arm-kernel

> > We don't translate into 'Marketing names'. We just turn the register
> > values into a stringified number. So it would have to be something
> > like:
> > 
> >         if (soc_name_is("7826") && soc_revision_is("1"))
> 
> Hmm, I think that is inconsistent with how the strings are used
> on other platforms, where you want something that uniquely identifies
> the device in user-readable strings.

Unfortunately, it is not so simple. There are a few different Armada
XP SoCs. E.g. dual core and quad core. And this is pretty normal for
Marvell. There are maybe 5 different Kirkwoods, a few different
Orion5x etc. So at minimum it would of had to be e.g. "Armada XP
7826".

However, it is too late now. This is part of the ABI, and Debian's
flash-kernel is making use of this in order to get the right DT blob.
So we cannot change it.

   Andrew

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Patch V2 2/2] i2c: mv64xxx: Remove internal compatible string from Documentation
  2014-07-28 15:52                 ` Andrew Lunn
@ 2014-07-28 16:03                   ` Arnd Bergmann
  0 siblings, 0 replies; 16+ messages in thread
From: Arnd Bergmann @ 2014-07-28 16:03 UTC (permalink / raw)
  To: linux-arm-kernel

On Monday 28 July 2014 17:52:56 Andrew Lunn wrote:
> > > We don't translate into 'Marketing names'. We just turn the register
> > > values into a stringified number. So it would have to be something
> > > like:
> > > 
> > >         if (soc_name_is("7826") && soc_revision_is("1"))
> > 
> > Hmm, I think that is inconsistent with how the strings are used
> > on other platforms, where you want something that uniquely identifies
> > the device in user-readable strings.
> 
> Unfortunately, it is not so simple. There are a few different Armada
> XP SoCs. E.g. dual core and quad core. And this is pretty normal for
> Marvell. There are maybe 5 different Kirkwoods, a few different
> Orion5x etc. So at minimum it would of had to be e.g. "Armada XP
> 7826".
> 
> However, it is too late now. This is part of the ABI, and Debian's
> flash-kernel is making use of this in order to get the right DT blob.
> So we cannot change it.
> 

Ok, I see. Let's hope then that "Marvell" as a family name in combination
with those numbers remains unique. A driver will probably have to
check all three, but it could be combined into a single function that
takes NULL as a don't care argument:

	if (soc_is("Marvell", "7826", NULL))
		...

	Arnd

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Patch V2 2/2] i2c: mv64xxx: Remove internal compatible string from Documentation
  2014-07-28 13:51       ` Arnd Bergmann
  2014-07-28 14:12         ` Andrew Lunn
@ 2014-07-28 18:29         ` Jason Gunthorpe
  2014-07-29 10:21           ` Arnd Bergmann
  2014-07-30  8:44         ` Maxime Ripard
  2 siblings, 1 reply; 16+ messages in thread
From: Jason Gunthorpe @ 2014-07-28 18:29 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 28, 2014 at 03:51:27PM +0200, Arnd Bergmann wrote:

> I remember this being discussed when the quirk was initially added,
> but it seemed cleaner to handle this in the platform code at the time
> when it was just for one particular board. Now that it's basically
> an accepted feature of the i2c device that you have to know the
> SoC version, that should probably become a proper API.

Reading the SOC ID (which has to come from a PCI-E controller) is
already known to mess up PCI-E. IMHO, reading it should be minimized,
unless we can figure out how to fix that.

At some point we need to push back on the bootloader situation and
require the kernel to be booted with a correct DT.

All the infrastructure was already provided for this, I'm not sure how
we got into the situation where it was OK for bootloaders to ignore
the DT requirements and it is the kernel's job to fix it :(

Jason

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Patch V2 2/2] i2c: mv64xxx: Remove internal compatible string from Documentation
  2014-07-28 18:29         ` Jason Gunthorpe
@ 2014-07-29 10:21           ` Arnd Bergmann
  0 siblings, 0 replies; 16+ messages in thread
From: Arnd Bergmann @ 2014-07-29 10:21 UTC (permalink / raw)
  To: linux-arm-kernel

On Monday 28 July 2014 12:29:13 Jason Gunthorpe wrote:
> On Mon, Jul 28, 2014 at 03:51:27PM +0200, Arnd Bergmann wrote:
> 
> > I remember this being discussed when the quirk was initially added,
> > but it seemed cleaner to handle this in the platform code at the time
> > when it was just for one particular board. Now that it's basically
> > an accepted feature of the i2c device that you have to know the
> > SoC version, that should probably become a proper API.
> 
> Reading the SOC ID (which has to come from a PCI-E controller) is
> already known to mess up PCI-E. IMHO, reading it should be minimized,
> unless we can figure out how to fix that.
> 
> At some point we need to push back on the bootloader situation and
> require the kernel to be booted with a correct DT.
> 
> All the infrastructure was already provided for this, I'm not sure how
> we got into the situation where it was OK for bootloaders to ignore
> the DT requirements and it is the kernel's job to fix it 

Thomas argued that it's always possible to identify the revision
and that we already need that anyway to work around broken dts file
for ax3, so it would be easier to generalize that hack.

Everybody else (except me) seemed happy with that idea, so I eventually
gave up arguing against it.

	Arnd

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Patch V2 2/2] i2c: mv64xxx: Remove internal compatible string from Documentation
  2014-07-28 13:51       ` Arnd Bergmann
  2014-07-28 14:12         ` Andrew Lunn
  2014-07-28 18:29         ` Jason Gunthorpe
@ 2014-07-30  8:44         ` Maxime Ripard
  2 siblings, 0 replies; 16+ messages in thread
From: Maxime Ripard @ 2014-07-30  8:44 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Arnd,

Sorry for jumping into the discussion like this, but we start having
pretty much the same issues on the Allwinner stuff.

On Mon, Jul 28, 2014 at 03:51:27PM +0200, Arnd Bergmann wrote:
> On Monday 28 July 2014 15:27:16 Andrew Lunn wrote:
> > > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> > > > index 5c30026921ae..6eb6f6e40ba1 100644
> > > > --- a/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> > > > +++ b/Documentation/devicetree/bindings/i2c/i2c-mv64xxx.txt
> > > > @@ -9,11 +9,6 @@ Required properties :
> > > >                       - "allwinner,sun6i-a31-i2c"
> > > >                       - "marvell,mv64xxx-i2c"
> > > >                       - "marvell,mv78230-i2c"
> > > > -                     - "marvell,mv78230-a0-i2c"
> > > > -                       * Note: Only use "marvell,mv78230-a0-i2c" for a
> > > > -                         very rare, initial version of the SoC which
> > > > -                         had broken offload support.  Linux
> > > > -                         auto-detects this and sets it appropriately.
> > > 
> > > I think we're losing knowledge here.  *We* know that attempting to
> > > enable transaction offload on A0 SoCs is bad news.  Other OS's would now
> > > need to dig through the Linux kernel code for clues as to what's
> > > happening.
> > > 
> > > Perhaps we should retain the info in the form of a note at the bottom of
> > > this file?
> > 
> > Hi Jason
> > 
> > I did wounder about this a bit. I've not looked, but now that XP
> > datasheets are public, i assume there is an errata for this, so it at
> > least should be documented by Marvell. But anybody looking in
> > /proc/device-tree on an A0 is going to see this undocumented string
> > which might raise questions.
> > 
> > So i'm happy to document it at the end of the binding.
> > 
> > Arnd, what do you say?
> 
> The final consequence of the API change would be to no longer
> change the compatible string in the fixup, but instead to call an
> API from the driver to find out the SoC revision when it encounters
> an mv78230-i2c.
> 
> I remember this being discussed when the quirk was initially added,
> but it seemed cleaner to handle this in the platform code at the time
> when it was just for one particular board. Now that it's basically
> an accepted feature of the i2c device that you have to know the
> SoC version, that should probably become a proper API.
> 
> Also, we now have drivers/soc/ and can move the soc-id code there
> with a publically documented API.
> 
> I agree it's sad to lose the knowledge about the revision, but we
> already have that as we now accept the generic string even on the
> a0 machines other than ax3.

What's the advantage on pushing the soc-detection to the drivers
themselves?

I'd really liked the idea that mvebu implements following this patch,
with the soc detection and DT fixup being done in the machine, because
it seems like it has a major benefit of centralizing every thing in a
single place.

That way it's much easier to have a status of what's supported and
what's not, that you wouldn't have if every driver was supposed to
handled that itself.

Plus, you seemed to be interested in having the quirks disabled, which
wouldn't be (easily) doable if we move individual quirks to the
drivers.

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140730/34ed86bf/attachment.sig>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [Patch V2 1/2] ARM: mvebu: armada xp: Generalize use of i2c quirk
  2014-07-26 17:20 ` [Patch V2 1/2] ARM: mvebu: armada xp: Generalize use of i2c quirk Andrew Lunn
@ 2014-10-15 15:25   ` Jason Cooper
  0 siblings, 0 replies; 16+ messages in thread
From: Jason Cooper @ 2014-10-15 15:25 UTC (permalink / raw)
  To: linux-arm-kernel

Andrew,

On Sat, Jul 26, 2014 at 07:20:37PM +0200, Andrew Lunn wrote:
> A second product has come to light which makes use of the A0 stepping
> of the Armada XP SoC. A0 stepping has a hardware bug in the i2c core
> meaning that hardware offload does not work, resulting in the kernel
> failing to boot. The quirk detects that the kernel is running on an A0
> stepping SoC and disables the use of hardware offload.
> 
> Currently the quirk is only enabled for PlatHome Openblocks AX3. The
> AX3 has been produced with both A0 and B0 stepping SoCs. The second
> product is the Lenovo Iomega IX4-300d. It seems likely that this
> device will also swap from A0 to B0 SoC sometime during its life.
> 
> If there are two products using A0, it seems likely there are more
> products with A0. Also, since the number of A0 SoCs is limited, these
> products are also likely to transition to B0. Hence detecting at run
> time is the safest option. So enable the quirk for all Armada XP
> boards.
> 
> Tested on an AX3 with A0 stepping.
> 
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> Acked-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
> 
> V2:
> Added two Acked-by:
> s/B1/B0
> ---
>  arch/arm/mach-mvebu/board-v7.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied to mvebu/fixes and Cc'd to stable for v3.12+

thx,

Jason.

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2014-10-15 15:25 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-26 17:20 [Patch V2 0/2] Generalize use of i2c quirk for Armada XP Andrew Lunn
2014-07-26 17:20 ` [Patch V2 1/2] ARM: mvebu: armada xp: Generalize use of i2c quirk Andrew Lunn
2014-10-15 15:25   ` Jason Cooper
2014-07-26 17:20 ` [Patch V2 2/2] i2c: mv64xxx: Remove internal compatible string from Documentation Andrew Lunn
2014-07-28 13:22   ` Jason Cooper
2014-07-28 13:27     ` Andrew Lunn
2014-07-28 13:51       ` Arnd Bergmann
2014-07-28 14:12         ` Andrew Lunn
2014-07-28 14:35           ` Arnd Bergmann
2014-07-28 15:25             ` Andrew Lunn
2014-07-28 15:47               ` Arnd Bergmann
2014-07-28 15:52                 ` Andrew Lunn
2014-07-28 16:03                   ` Arnd Bergmann
2014-07-28 18:29         ` Jason Gunthorpe
2014-07-29 10:21           ` Arnd Bergmann
2014-07-30  8:44         ` Maxime Ripard

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.