ARM: mvebu: fix bus width handling in mvebu-devbus

PATCH New Thomas Petazzoni 4 participants 13 messages Apr 14, 2014 – Apr 17, 2014

Subsystems (2)

Device tree bindings Memory controller drivers

lore

Share & tools

lore

Thomas Petazzoni

author

2014-04-14 15:29 UTC #0 lore raw

v2 cover ARM: mvebu: fix bus width handling in mvebu-devbus

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

Signature · 1 line

1.8.3.2

Thomas Petazzoni

author

2014-04-14 15:29 UTC #1 lore raw

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

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 <[email protected]>

Cc stable at vger.kernel.org

---
 drivers/memory/mvebu-devbus.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

drivers/memory/mvebu-devbus.c +13 -2

index 110c036..b59a17f 100644
@@ -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);

Signature · 1 line

1.8.3.2

Jason Cooper

2014-04-16 21:09 UTC #2 lore raw

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

Thomas,

Quoted 38 lines · 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 <[email protected]>
Cc stable at vger.kernel.org
---
 drivers/memory/mvebu-devbus.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)
drivers/memory/mvebu-devbus.c +3 -2
index 110c036..b59a17f 100644
@@ -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,

drivers/memory/mvebu-devbus.c static int devbus_set_timing_params(struct devbus *devbus, · line 123

Quoted 10 lines

drivers/memory/mvebu-devbus.c +10 -0

+	 * 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.

Thomas Petazzoni

author

2014-04-16 21:28 UTC #3 lore raw

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

Dear Jason Cooper,

Quoted 3 lines · Jason Cooper

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

Signature · 3 lines

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

Jason Cooper

Applied

2014-04-17 04:48 UTC #4 lore raw

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

In reply to Thomas Petazzoni

Quoted 19 lines · 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.

Quoted 5 lines

Signed-off-by Thomas Petazzoni <[email protected]>
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.

Thomas Petazzoni

author

2014-04-14 15:29 UTC #5 lore raw

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

In reply to 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 <[email protected]>

Cc stable at vger.kernel.org

---
 arch/arm/boot/dts/armada-xp-gp.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/boot/dts/armada-xp-gp.dts +1 -1

index 61bda68..3a359fd 100644
@@ -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>;

Signature · 1 line

1.8.3.2

Jason Cooper

2014-04-17 05:08 UTC #6 lore raw

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

Quoted 17 lines · 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.

Quoted 5 lines

Signed-off-by Thomas Petazzoni <[email protected]>
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.

Thomas Petazzoni

author

2014-04-17 07:09 UTC #7 lore raw

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

Dear Jason Cooper,

Quoted 2 lines · Jason Cooper

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

Thanks.

Quoted 3 lines

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.

Quoted 2 lines

------
Link: ...

What should this link point to?

Quoted 1 line

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.

Quoted 1 line

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

Signature · 3 lines

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

Jason Cooper

2014-04-17 14:23 UTC #8 lore raw

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

Quoted 10 lines · Thomas Petazzoni

Quoted 3 lines · Jason Cooper
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.

Quoted 4 lines

Quoted 2 lines
------
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.

Quoted 3 lines

Quoted 1 line
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. :(

Quoted 7 lines

Quoted 1 line
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.

Quoted 1 line

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.

Thomas Petazzoni

author

2014-04-14 15:29 UTC #9 lore raw

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

In reply to 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 <[email protected]>

Cc stable at vger.kernel.org

---
 arch/arm/boot/dts/armada-xp-db.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/boot/dts/armada-xp-db.dts +1 -1

index 448373c..90f0bf6 100644
@@ -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>;

Signature · 1 line

1.8.3.2

Thomas Petazzoni

author

2014-04-14 15:29 UTC #10 lore raw

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

In reply to 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 <[email protected]>

Cc stable at vger.kernel.org

---
 arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm/boot/dts/armada-xp-openblocks-ax3-4.dts +1 -1

index 985948c..5d42feb 100644
@@ -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>;

Signature · 1 line

1.8.3.2

Ezequiel Garcia

2014-04-14 17:00 UTC #11 lore raw

v2 cover ARM: mvebu: fix bus width handling in mvebu-devbus

In reply to Thomas Petazzoni

Quoted 21 lines · 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.

Quoted 6 lines

* 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 <[email protected]>

Signature · 3 lines

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

Gregory CLEMENT

2014-04-16 08:17 UTC #12 lore raw

v2 cover ARM: mvebu: fix bus width handling in mvebu-devbus

In reply to Thomas Petazzoni

Hi Thomas,

Quoted 21 lines · 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.

Quoted 5 lines

* 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 <[email protected]>

Thanks,

Gregory

Quoted 17 lines

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

Signature · 4 lines

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