[PATCH v2 0/4] ARM: mvebu: fix bus width handling in mvebu-devbus

[PATCH v2 0/4] ARM: mvebu: fix bus width handling in mvebu-devbus

[Thomas Petazzoni]

Jason, Greg, Andrew, Sebastian,

This set of patches fix a bug in the mvebu-devbus driver, which was
leading to a misinterpretation of Device Tree provided value: a NOR
bus width declared as 8 bits in the Device Tree was in fact configured
as 16 bits at the hardware level.

This bug was not noticed until now, because it was compensated by a
symetric mistake in the Device Tree files.

This set of commits therefore fix both the driver itself, and the
incorrect Device Tree files. The patches for the Device Tree files
have been separated, because some of them should apply all the way to
v3.10, while some of them apply only up to v3.11.

Changes since v1:

 * Added patches to fix the Device Tree files to indicate the correct
   bus width, as documented in the board schematics. Noticed by
   Ezequiel Garcia.

 * Changed the bus_width conversion logic in the driver to only accept
   valid values: 8 bits and 16 bits. All other values lead to an error
   being returned. Suggested by Sebastian Hesselbarth.

 * Rebased on top of v3.15-rc1.

Thanks,

Thomas

Thomas Petazzoni (4):
  memory: mvebu-devbus: fix the conversion of the bus width
  ARM: mvebu: fix NOR bus-width in Armada XP GP Device Tree
  ARM: mvebu: fix NOR bus-width in Armada XP DB Device Tree
  ARM: mvebu: fix NOR bus-width in Armada XP OpenBlocks AX3 Device Tree

 arch/arm/boot/dts/armada-xp-db.dts               |  2 +-
 arch/arm/boot/dts/armada-xp-gp.dts               |  2 +-
 arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts |  2 +-
 drivers/memory/mvebu-devbus.c                    | 15 +++++++++++++--
 4 files changed, 16 insertions(+), 5 deletions(-)

-- 
1.8.3.2

[PATCH v2 1/4] memory: mvebu-devbus: fix the conversion of the bus width

[Thomas Petazzoni]

According to the Armada 370 and Armada XP datasheets, the part of the
Device Bus register that configure the bus width should contain 0 for
a 8 bits bus width, and 1 for a 16 bits bus width (other values are
unsupported/reserved).

However, the current conversion done in the driver to convert from a
bus width in bits to the value expected by the register leads to
setting the register to 1 for a 8 bits bus, and 2 for a 16 bits bus.

This mistake was compensated by a mistake in the existing Device Tree
files for Armada 370/XP platforms: they were declaring a 8 bits bus
width, while the hardware in fact uses a 16 bits bus width.

This commit fixes that by adjusting the conversion logic.

This patch fixes a bug that was introduced in
3edad321b1bd2e6c8b5f38146c115c8982438f06 ('drivers: memory: Introduce
Marvell EBU Device Bus driver'), which was merged in v3.11.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: stable at vger.kernel.org
---
 drivers/memory/mvebu-devbus.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/memory/mvebu-devbus.c b/drivers/memory/mvebu-devbus.c
index 110c036..b59a17f 100644
--- a/drivers/memory/mvebu-devbus.c
+++ b/drivers/memory/mvebu-devbus.c
@@ -108,8 +108,19 @@ static int devbus_set_timing_params(struct devbus *devbus,
 			node->full_name);
 		return err;
 	}
-	/* Convert bit width to byte width */
-	r.bus_width /= 8;
+
+	/*
+	 * The bus width is encoded into the register as 0 for 8 bits,
+	 * and 1 for 16 bits, so we do the necessary conversion here.
+	 */
+	if (r.bus_width == 8)
+		r.bus_width = 0;
+	else if (r.bus_width == 16)
+		r.bus_width = 1;
+	else {
+		dev_err(devbus->dev, "invalid bus width %d\n", r.bus_width);
+		return -EINVAL;
+	}
 
 	err = get_timing_param_ps(devbus, node, "devbus,badr-skew-ps",
 				 &r.badr_skew);
-- 
1.8.3.2

[PATCH v2 1/4] memory: mvebu-devbus: fix the conversion of the bus width

[Jason Cooper]

Thomas,

On Mon, Apr 14, 2014 at 05:29:18PM +0200, Thomas Petazzoni wrote:
> According to the Armada 370 and Armada XP datasheets, the part of the
> Device Bus register that configure the bus width should contain 0 for
> a 8 bits bus width, and 1 for a 16 bits bus width (other values are
> unsupported/reserved).
> 
> However, the current conversion done in the driver to convert from a
> bus width in bits to the value expected by the register leads to
> setting the register to 1 for a 8 bits bus, and 2 for a 16 bits bus.
> 
> This mistake was compensated by a mistake in the existing Device Tree
> files for Armada 370/XP platforms: they were declaring a 8 bits bus
> width, while the hardware in fact uses a 16 bits bus width.
> 
> This commit fixes that by adjusting the conversion logic.
> 
> This patch fixes a bug that was introduced in
> 3edad321b1bd2e6c8b5f38146c115c8982438f06 ('drivers: memory: Introduce
> Marvell EBU Device Bus driver'), which was merged in v3.11.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: stable at vger.kernel.org
> ---
>  drivers/memory/mvebu-devbus.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/memory/mvebu-devbus.c b/drivers/memory/mvebu-devbus.c
> index 110c036..b59a17f 100644
> --- a/drivers/memory/mvebu-devbus.c
> +++ b/drivers/memory/mvebu-devbus.c
> @@ -108,8 +108,19 @@ static int devbus_set_timing_params(struct devbus *devbus,
>  			node->full_name);
>  		return err;
>  	}
> -	/* Convert bit width to byte width */
> -	r.bus_width /= 8;
> +
> +	/*
> +	 * The bus width is encoded into the register as 0 for 8 bits,
> +	 * and 1 for 16 bits, so we do the necessary conversion here.
> +	 */
> +	if (r.bus_width == 8)
> +		r.bus_width = 0;
> +	else if (r.bus_width == 16)
> +		r.bus_width = 1;
> +	else {
> +		dev_err(devbus->dev, "invalid bus width %d\n", r.bus_width);
> +		return -EINVAL;
> +	}

This looks strikingly similar to patch 4/29 of your own orion5x DT
conversion series, but with completely different code...  Which one
would you prefer I take?

thx,

Jason.

[PATCH v2 1/4] memory: mvebu-devbus: fix the conversion of the bus width

[Thomas Petazzoni]

Dear Jason Cooper,

On Wed, 16 Apr 2014 17:09:24 -0400, Jason Cooper wrote:

> This looks strikingly similar to patch 4/29 of your own orion5x DT
> conversion series, but with completely different code...  Which one
> would you prefer I take?

Clearly this one. The one in the Orion patch series was a previous
version of this patch, which was not doing any validation on the bus
width value coming from the DT. Once this version is merged somewhere,
I'll base my Orion patch series on it (I'm planning on resending a new
version of my Orion series soon, to take into account all the review
comments I've received).

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v2 1/4] memory: mvebu-devbus: fix the conversion of the bus width

[Jason Cooper]

On Mon, Apr 14, 2014 at 05:29:18PM +0200, Thomas Petazzoni wrote:
> According to the Armada 370 and Armada XP datasheets, the part of the
> Device Bus register that configure the bus width should contain 0 for
> a 8 bits bus width, and 1 for a 16 bits bus width (other values are
> unsupported/reserved).
> 
> However, the current conversion done in the driver to convert from a
> bus width in bits to the value expected by the register leads to
> setting the register to 1 for a 8 bits bus, and 2 for a 16 bits bus.
> 
> This mistake was compensated by a mistake in the existing Device Tree
> files for Armada 370/XP platforms: they were declaring a 8 bits bus
> width, while the hardware in fact uses a 16 bits bus width.
> 
> This commit fixes that by adjusting the conversion logic.
> 
> This patch fixes a bug that was introduced in
> 3edad321b1bd2e6c8b5f38146c115c8982438f06 ('drivers: memory: Introduce
> Marvell EBU Device Bus driver'), which was merged in v3.11.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: stable at vger.kernel.org
> ---
>  drivers/memory/mvebu-devbus.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)

Applied to mvebu/fixes

thx,

Jason.

[PATCH v2 2/4] ARM: mvebu: fix NOR bus-width in Armada XP GP Device Tree

[Thomas Petazzoni]

The mvebu-devbus driver had a serious bug, which lead to a 8 bits bus
width declared in the Device Tree being considered as a 16 bits bus
width when configuring the hardware.

This bug in mvebu-devbus driver was compensated by a symetric mistake
in the Armada XP GP Device Tree: a 8 bits bus width was declared, even
though the hardware actually has a 16 bits bus width connection with
the NOR flash.

Now that we have fixed the mvebu-devbus driver to behave according to
its Device Tree binding, this commit fixes the problematic Device Tree
files as well.

This bug was introduced in commit
da8d1b38356853c37116f9afa29f15648d7fb159 ('ARM: mvebu: Add support for
NOR flash device on Armada XP-GP board') which was merged in v3.10.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: stable at vger.kernel.org
---
 arch/arm/boot/dts/armada-xp-gp.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/armada-xp-gp.dts b/arch/arm/boot/dts/armada-xp-gp.dts
index 61bda68..3a359fd 100644
--- a/arch/arm/boot/dts/armada-xp-gp.dts
+++ b/arch/arm/boot/dts/armada-xp-gp.dts
@@ -59,7 +59,7 @@
 			/* Device Bus parameters are required */
 
 			/* Read parameters */
-			devbus,bus-width    = <8>;
+			devbus,bus-width    = <16>;
 			devbus,turn-off-ps  = <60000>;
 			devbus,badr-skew-ps = <0>;
 			devbus,acc-first-ps = <124000>;
-- 
1.8.3.2

[PATCH v2 2/4] ARM: mvebu: fix NOR bus-width in Armada XP GP Device Tree

[Jason Cooper]

On Mon, Apr 14, 2014 at 05:29:19PM +0200, Thomas Petazzoni wrote:
> The mvebu-devbus driver had a serious bug, which lead to a 8 bits bus
> width declared in the Device Tree being considered as a 16 bits bus
> width when configuring the hardware.
> 
> This bug in mvebu-devbus driver was compensated by a symetric mistake
> in the Armada XP GP Device Tree: a 8 bits bus width was declared, even
> though the hardware actually has a 16 bits bus width connection with
> the NOR flash.
> 
> Now that we have fixed the mvebu-devbus driver to behave according to
> its Device Tree binding, this commit fixes the problematic Device Tree
> files as well.
> 
> This bug was introduced in commit
> da8d1b38356853c37116f9afa29f15648d7fb159 ('ARM: mvebu: Add support for
> NOR flash device on Armada XP-GP board') which was merged in v3.10.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: stable at vger.kernel.org
> ---
>  arch/arm/boot/dts/armada-xp-gp.dts | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Patches 2, 3, and 4 applied to mvebu/dt-fixes for stable with Gregory
and Ezequiel's Acks.

Also, a gentle nit:  You're doing all the -stable info digging, would
you mind putting it in the following format so I don't have to hand edit
each patch?

------
Link: ...
Fixes: da8d1b383568 ('ARM: mvebu: Add support for NOR flash device on Armada XP-GP board')
Cc: stable at vger.kernel.org # v3.10+
------

thx,

Jason.

[PATCH v2 2/4] ARM: mvebu: fix NOR bus-width in Armada XP GP Device Tree

[Thomas Petazzoni]

Dear Jason Cooper,

On Thu, 17 Apr 2014 01:08:32 -0400, Jason Cooper wrote:

> Patches 2, 3, and 4 applied to mvebu/dt-fixes for stable with Gregory
> and Ezequiel's Acks.

Thanks.

> Also, a gentle nit:  You're doing all the -stable info digging, would
> you mind putting it in the following format so I don't have to hand edit
> each patch?

Sure! So far I've just been blindly following
Documentation/stable_kernel_rules.txt, and besides the "Cc:
stable at vger.kernel.org" it doesn't say anything else of what you're
suggesting below.

> ------
> Link: ...

What should this link point to?

> Fixes: da8d1b383568 ('ARM: mvebu: Add support for NOR flash device on Armada XP-GP board')

Ok. But this is not described in Documentation/stable_kernel_rules.txt.

> Cc: stable at vger.kernel.org # v3.10+

The Cc: to stable@ is documented, but the use of a comment with the
kernel version afterwards is only mentioned when the commit in question
requires other commits as dependencies to be applied to stable. When
the commit is self-sufficient, stable_kernel_rules.txt doesn't indicate
that a comment is needed.

Maybe an update to stable_kernel_rules.txt is in order? :-)

Thanks again!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH v2 2/4] ARM: mvebu: fix NOR bus-width in Armada XP GP Device Tree

[Jason Cooper]

On Thu, Apr 17, 2014 at 09:09:36AM +0200, Thomas Petazzoni wrote:
> On Thu, 17 Apr 2014 01:08:32 -0400, Jason Cooper wrote:
> > Also, a gentle nit:  You're doing all the -stable info digging, would
> > you mind putting it in the following format so I don't have to hand edit
> > each patch?
> 
> Sure! So far I've just been blindly following
> Documentation/stable_kernel_rules.txt, and besides the "Cc:
> stable at vger.kernel.org" it doesn't say anything else of what you're
> suggesting below.
> 
> > ------
> > Link: ...
> 
> What should this link point to?

Oops, it was late.  I thought I was copying the relevant lines you added
under your SoB.  I forgot that I added code to my import script that
automagically adds a line:

Link: https://lkml.kernel.org/r/1397489361-5833-2-git-send-email-thomas.petazzoni at free-electrons.com

built from the Message-Id of the email.  Very helpful to lookup ML
conversations for a given commit.

Anyway, ignore that line because I automatically add it when I pull the
patch in.

> > Fixes: da8d1b383568 ('ARM: mvebu: Add support for NOR flash device on Armada XP-GP board')
> 
> Ok. But this is not described in Documentation/stable_kernel_rules.txt.

It's what we settled on at 2013 KS.  I could have sworn we updated the
docs to reflect that...  A quick grep says no. :(

> > Cc: stable at vger.kernel.org # v3.10+
> 
> The Cc: to stable@ is documented, but the use of a comment with the
> kernel version afterwards is only mentioned when the commit in question
> requires other commits as dependencies to be applied to stable. When
> the commit is self-sufficient, stable_kernel_rules.txt doesn't indicate
> that a comment is needed.

Indeed, I've always added the comment even for single patches because it
makes the -stable teams job easier.  We know the code better, so it's
easier for us to determine how far back it should go.

Just to be clear, I'm not asking you or anyone else to do the above for
patch submissions.  Only if you're inclined to do the digging for me, it
would be nice to have it in the proper format.

> Maybe an update to stable_kernel_rules.txt is in order? :-)

Yes, it would seem so.  Perhaps some young, motivated, new kernel hacker
would like to take a stab at it? :-)

thx,

Jason.

[PATCH v2 3/4] ARM: mvebu: fix NOR bus-width in Armada XP DB Device Tree

[Thomas Petazzoni]

The mvebu-devbus driver had a serious bug, which lead to a 8 bits bus
width declared in the Device Tree being considered as a 16 bits bus
width when configuring the hardware.

This bug in mvebu-devbus driver was compensated by a symetric mistake
in the Armada XP DB Device Tree: a 8 bits bus width was declared, even
though the hardware actually has a 16 bits bus width connection with
the NOR flash.

Now that we have fixed the mvebu-devbus driver to behave according to
its Device Tree binding, this commit fixes the problematic Device Tree
files as well.

This bug was introduced in commit
b484ff42df475c5087d614c4d477273e1906bcb9 ('ARM: mvebu: Add support for
NOR flash device on Armada XP-DB board') which was merged in v3.11.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: stable at vger.kernel.org
---
 arch/arm/boot/dts/armada-xp-db.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/armada-xp-db.dts b/arch/arm/boot/dts/armada-xp-db.dts
index 448373c..90f0bf6 100644
--- a/arch/arm/boot/dts/armada-xp-db.dts
+++ b/arch/arm/boot/dts/armada-xp-db.dts
@@ -49,7 +49,7 @@
 			/* Device Bus parameters are required */
 
 			/* Read parameters */
-			devbus,bus-width    = <8>;
+			devbus,bus-width    = <16>;
 			devbus,turn-off-ps  = <60000>;
 			devbus,badr-skew-ps = <0>;
 			devbus,acc-first-ps = <124000>;
-- 
1.8.3.2

[PATCH v2 4/4] ARM: mvebu: fix NOR bus-width in Armada XP OpenBlocks AX3 Device Tree

[Thomas Petazzoni]

The mvebu-devbus driver had a serious bug, which lead to a 8 bits bus
width declared in the Device Tree being considered as a 16 bits bus
width when configuring the hardware.

This bug in mvebu-devbus driver was compensated by a symetric mistake
in the Armada XP OpenBlocks AX3 Device Tree: a 8 bits bus width was
declared, even though the hardware actually has a 16 bits bus width
connection with the NOR flash.

Now that we have fixed the mvebu-devbus driver to behave according to
its Device Tree binding, this commit fixes the problematic Device Tree
files as well.

This bug was introduced in commit
a7d4f81821f7eec3175f8e23dd6949c71ab2da43 ('ARM: mvebu: Add support for
NOR flash device on Openblocks AX3 board') which was merged in v3.10.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: stable at vger.kernel.org
---
 arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts b/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
index 985948c..5d42feb 100644
--- a/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
+++ b/arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts
@@ -39,7 +39,7 @@
 			/* Device Bus parameters are required */
 
 			/* Read parameters */
-			devbus,bus-width    = <8>;
+			devbus,bus-width    = <16>;
 			devbus,turn-off-ps  = <60000>;
 			devbus,badr-skew-ps = <0>;
 			devbus,acc-first-ps = <124000>;
-- 
1.8.3.2

[PATCH v2 0/4] ARM: mvebu: fix bus width handling in mvebu-devbus

[Ezequiel Garcia]

On Apr 14, Thomas Petazzoni wrote:
> Jason, Greg, Andrew, Sebastian,
> 
> This set of patches fix a bug in the mvebu-devbus driver, which was
> leading to a misinterpretation of Device Tree provided value: a NOR
> bus width declared as 8 bits in the Device Tree was in fact configured
> as 16 bits at the hardware level.
> 
> This bug was not noticed until now, because it was compensated by a
> symetric mistake in the Device Tree files.
> 
> This set of commits therefore fix both the driver itself, and the
> incorrect Device Tree files. The patches for the Device Tree files
> have been separated, because some of them should apply all the way to
> v3.10, while some of them apply only up to v3.11.
> 
> Changes since v1:
> 
>  * Added patches to fix the Device Tree files to indicate the correct
>    bus width, as documented in the board schematics. Noticed by
>    Ezequiel Garcia.
> 
>  * Changed the bus_width conversion logic in the driver to only accept
>    valid values: 8 bits and 16 bits. All other values lead to an error
>    being returned. Suggested by Sebastian Hesselbarth.
> 
>  * Rebased on top of v3.15-rc1.
> 

Thanks a lot for taking care of this!

Acked-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>

-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[PATCH v2 0/4] ARM: mvebu: fix bus width handling in mvebu-devbus

[Gregory CLEMENT]

Hi Thomas,

On 14/04/2014 17:29, Thomas Petazzoni wrote:
> Jason, Greg, Andrew, Sebastian,
> 
> This set of patches fix a bug in the mvebu-devbus driver, which was
> leading to a misinterpretation of Device Tree provided value: a NOR
> bus width declared as 8 bits in the Device Tree was in fact configured
> as 16 bits at the hardware level.
> 
> This bug was not noticed until now, because it was compensated by a
> symetric mistake in the Device Tree files.
> 
> This set of commits therefore fix both the driver itself, and the
> incorrect Device Tree files. The patches for the Device Tree files
> have been separated, because some of them should apply all the way to
> v3.10, while some of them apply only up to v3.11.
> 
> Changes since v1:
> 
>  * Added patches to fix the Device Tree files to indicate the correct
>    bus width, as documented in the board schematics. Noticed by
>    Ezequiel Garcia.
> 
>  * Changed the bus_width conversion logic in the driver to only accept
>    valid values: 8 bits and 16 bits. All other values lead to an error
>    being returned. Suggested by Sebastian Hesselbarth.
> 
>  * Rebased on top of v3.15-rc1.

Everything looks good so for the whole series:

Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Thanks,

Gregory


> 
> Thanks,
> 
> Thomas
> 
> Thomas Petazzoni (4):
>   memory: mvebu-devbus: fix the conversion of the bus width
>   ARM: mvebu: fix NOR bus-width in Armada XP GP Device Tree
>   ARM: mvebu: fix NOR bus-width in Armada XP DB Device Tree
>   ARM: mvebu: fix NOR bus-width in Armada XP OpenBlocks AX3 Device Tree
> 
>  arch/arm/boot/dts/armada-xp-db.dts               |  2 +-
>  arch/arm/boot/dts/armada-xp-gp.dts               |  2 +-
>  arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts |  2 +-
>  drivers/memory/mvebu-devbus.c                    | 15 +++++++++++++--
>  4 files changed, 16 insertions(+), 5 deletions(-)
> 


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