dt-bindings: mtd-physmap: Add endianness supports

PATCH Superseded Prabhakar Kushwaha 2 participants 5 messages Feb 28, 2018 – Mar 6, 2018

Subsystems (2)

Device tree bindings Memory technology devices (MTD)

lore

Share & tools

lore

Prabhakar Kushwaha

author

2018-02-28 07:33 UTC #0 lore raw

PATCH v5 1/2 dt-bindings: mtd-physmap: Add endianness supports

Connection between flash and controller is not necessary to be always of same type. It may varies from platform to platform.

Adding endianness (optional) property to provide connection type information.

Signed-off-by Prabhakar Kushwaha <[email protected]>

Reviewed-by Rob Herring <[email protected]>

---
Changes for v2: updated subject
Changes for v3: fixed typo for "big-endian" Changes for v4: Moved binding definition in mtd-physmap.txt as discussed at https://patchwork.ozlabs.org/patch/842543/
Changes for v5: Sending as it is

 Documentation/devicetree/bindings/mtd/mtd-physmap.txt | 5 +++++
 1 file changed, 5 insertions(+)

Documentation/devicetree/bindings/mtd/mtd-physmap.txt +5 -0

index 4a0a48bf4ecb..961a8eed9d0d 100644
@@ -41,6 +41,11 @@ additional (optional) property is defined:
 
  - erase-size : The chip's physical erase block size in bytes.
 
+ The device tree may optionally contain endianness property.
+ little-endian or big-endian : It represents connection between controller and
+			flash. If this property is absent, flash is considered
+			to be host-endian.
+
 The device tree may optionally contain sub-nodes describing partitions of the
 address space. See partition.txt for more detail.

Signature · 1 line

2.14.1

Prabhakar Kushwaha

author

2018-02-28 07:33 UTC #1 lore raw

PATCH v5 2/2 driver: mtd: update struct map_info's swap as per map requirement

It is not necessary for all device's maps to be CFI_HOST_ENDIAN. Maps device can be Big endian or little endian.

Currently it is being taken care using CONFIG_MTD_CFI_LE_BYTE_SWAP or CONFIG_MTD_CFI_BE_BYTE_SWAP i.e. compile time.

Now update struct map_info's swap field based on device characteristics defined in device tree.

Signed-off-by Jagdish Gediya <[email protected]>

Signed-off-by Prabhakar Kushwaha <[email protected]>

---
Changes for v2: Incorporated review comments from Boris Brezillon
http://patchwork.ozlabs.org/patch/758381/
Changes for v3: None
Changes for v4: Updated code as per new defined binding of physmap.txt Changes for v5: Avoid map.swap set in case of endiannes not defined in dts file

 drivers/mtd/maps/physmap_of_core.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/mtd/maps/physmap_of_core.c +6 -0

index b1bd4faecfb2..527b1682381f 100644
@@ -20,6 +20,7 @@
 #include <linux/mtd/map.h>
 #include <linux/mtd/partitions.h>
 #include <linux/mtd/concat.h>
+#include <linux/mtd/cfi_endian.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_platform.h>

@@ -233,6 +234,11 @@ static int of_flash_probe(struct platform_device *dev)
 		info->list[i].map.bankwidth = be32_to_cpup(width);
 		info->list[i].map.device_node = dp;
 
+		if (of_property_read_bool(dp, "big-endian"))
+			info->list[i].map.swap = CFI_BIG_ENDIAN;
+		else if (of_property_read_bool(dp, "little-endian"))
+			info->list[i].map.swap = CFI_LITTLE_ENDIAN;
+
 		err = of_flash_probe_gemini(dev, dp, &info->list[i].map);
 		if (err)
 			goto err_out;

Signature · 1 line

2.14.1

Prabhakar Kushwaha

author

2018-03-06 06:02 UTC #2 lore raw

PATCH v5 2/2 driver: mtd: update struct map_info's swap as per map requirement

Dear Boris,

Quoted 60 lines

-----Original Message-----
From: Prabhakar Kushwaha
Sent: Wednesday, February 28, 2018 1:04 PM
To: linux-mtd at lists.infradead.org; devicetree at vger.kernel.org; robh at kernel.org; mark.rutland at arm.com; shawnguo at kernel.org
Cc: linux-arm-kernel at lists.infradead.org; boris.brezillon at free- electrons.com; cyrille.pitchen at wedev4u.fr; computersforpeace at gmail.com; oss at buserror.net; Leo Li <[email protected]>; Prabhakar Kushwaha <[email protected]>; Jagdish Gediya <[email protected]>
Subject: [PATCH 2/2][v5] driver: mtd: update struct map_info's swap as per map requirement
It is not necessary for all device's maps to be CFI_HOST_ENDIAN. Maps device can be Big endian or little endian.
Currently it is being taken care using CONFIG_MTD_CFI_LE_BYTE_SWAP or CONFIG_MTD_CFI_BE_BYTE_SWAP i.e. compile time.
Now update struct map_info's swap field based on device characteristics defined in device tree.
Signed-off-by Jagdish Gediya <[email protected]>
Signed-off-by Prabhakar Kushwaha <[email protected]>
---
Changes for v2: Incorporated review comments from Boris Brezillon
http://patchwork.ozlabs.org/patch/758381/
Changes for v3: None
Changes for v4: Updated code as per new defined binding of physmap.txt Changes for v5: Avoid map.swap set in case of endiannes not defined in dts file
 drivers/mtd/maps/physmap_of_core.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/mtd/maps/physmap_of_core.c +6 -0
index b1bd4faecfb2..527b1682381f 100644
@@ -20,6 +20,7 @@
 #include <linux/mtd/map.h>
 #include <linux/mtd/partitions.h>
 #include <linux/mtd/concat.h>
+#include <linux/mtd/cfi_endian.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_platform.h>
@@ -233,6 +234,11 @@ static int of_flash_probe(struct platform_device
 *dev)
 		info->list[i].map.bankwidth = be32_to_cpup(width);
 		info->list[i].map.device_node = dp;
 
+		if (of_property_read_bool(dp, "big-endian"))
+			info->list[i].map.swap = CFI_BIG_ENDIAN;
+		else if (of_property_read_bool(dp, "little-endian"))
+			info->list[i].map.swap = CFI_LITTLE_ENDIAN;
+
 		err = of_flash_probe_gemini(dev, dp, &info->list[i].map);
 		if (err)

Quoted 3 lines

goto err_out;
--
2.14.1

Is this patch correct with respect to device binding (http://patchwork.ozlabs.org/patch/878974/)
May I go ahead and send device tree patches for other NXP platforms

will http://patchwork.ozlabs.org/patch/878974/ along with this patch be send via linux-mtd repo?

--pk

Boris Brezillon

2018-03-06 08:14 UTC #3 lore raw

PATCH v5 2/2 driver: mtd: update struct map_info's swap as per map requirement

Hi Prabhakar,

Quoted 55 lines · On Tue, 6 Mar 2018 06:02:08 +0000 Prabhakar Kushwaha…

Dear Boris,
Quoted 53 lines
-----Original Message-----
From: Prabhakar Kushwaha
Sent: Wednesday, February 28, 2018 1:04 PM
To: linux-mtd at lists.infradead.org; devicetree at vger.kernel.org; robh at kernel.org; mark.rutland at arm.com; shawnguo at kernel.org
Cc: linux-arm-kernel at lists.infradead.org; boris.brezillon at free- electrons.com; cyrille.pitchen at wedev4u.fr; computersforpeace at gmail.com; oss at buserror.net; Leo Li <[email protected]>; Prabhakar Kushwaha <[email protected]>; Jagdish Gediya <[email protected]>
Subject: [PATCH 2/2][v5] driver: mtd: update struct map_info's swap as per map requirement
It is not necessary for all device's maps to be CFI_HOST_ENDIAN. Maps device can be Big endian or little endian.
Currently it is being taken care using CONFIG_MTD_CFI_LE_BYTE_SWAP or CONFIG_MTD_CFI_BE_BYTE_SWAP i.e. compile time.
Now update struct map_info's swap field based on device characteristics defined in device tree.
Signed-off-by Jagdish Gediya <[email protected]>
Signed-off-by Prabhakar Kushwaha <[email protected]>
---
Changes for v2: Incorporated review comments from Boris Brezillon
http://patchwork.ozlabs.org/patch/758381/
Changes for v3: None
Changes for v4: Updated code as per new defined binding of physmap.txt Changes for v5: Avoid map.swap set in case of endiannes not defined in dts file
 drivers/mtd/maps/physmap_of_core.c | 6 ++++++
 1 file changed, 6 insertions(+)
drivers/mtd/maps/physmap_of_core.c +1 -0
index b1bd4faecfb2..527b1682381f 100644
@@ -20,6 +20,7 @@
 #include <linux/mtd/map.h>
 #include <linux/mtd/partitions.h>
 #include <linux/mtd/concat.h>
+#include <linux/mtd/cfi_endian.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_platform.h>
@@ -233,6 +234,11 @@ static int of_flash_probe(struct platform_device
 *dev)
 		info->list[i].map.bankwidth = be32_to_cpup(width);
 		info->list[i].map.device_node = dp;
 

Quoted 12 lines

Quoted 10 lines
drivers/mtd/maps/physmap_of_core.c +5 -0
+		if (of_property_read_bool(dp, "big-endian"))
+			info->list[i].map.swap = CFI_BIG_ENDIAN;
+		else if (of_property_read_bool(dp, "little-endian"))
+			info->list[i].map.swap = CFI_LITTLE_ENDIAN;
+
 		err = of_flash_probe_gemini(dev, dp, &info->list[i].map);
 		if (err)
goto err_out;
--
2.14.1
Is this patch correct with respect to device binding (http://patchwork.ozlabs.org/patch/878974/)

This patch looks good, but patch 1 is not describing what really happens when the xxx-endian props are missing.

Quoted 1 line

May I go ahead and send device tree patches for other NXP platforms

Nope, please wait till we sorted out the last issues I pointed on patch 1.

Quoted 2 lines

will http://patchwork.ozlabs.org/patch/878974/ along with this patch be send via linux-mtd repo?

Yes, once you've fixed the description or explained why you think the current description is valid.

Regards,

Boris

Signature · 3 lines

Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Boris Brezillon

2018-03-01 17:12 UTC #4 lore raw

PATCH v5 1/2 dt-bindings: mtd-physmap: Add endianness supports

In reply to Prabhakar Kushwaha

Quoted 23 lines · On Wed, 28 Feb 2018 13:03:34 +0530 Prabhakar Kushwaha…

Connection between flash and controller is not necessary to be always of same type. It may varies from platform to platform.
Adding endianness (optional) property to provide connection type information.
Signed-off-by Prabhakar Kushwaha <[email protected]>
Reviewed-by Rob Herring <[email protected]>
---
Changes for v2: updated subject
Changes for v3: fixed typo for "big-endian" Changes for v4: Moved binding definition in mtd-physmap.txt as discussed at https://patchwork.ozlabs.org/patch/842543/
Changes for v5: Sending as it is
 Documentation/devicetree/bindings/mtd/mtd-physmap.txt | 5 +++++
 1 file changed, 5 insertions(+)
Documentation/devicetree/bindings/mtd/mtd-physmap.txt +0 -0
index 4a0a48bf4ecb..961a8eed9d0d 100644

Documentation/devicetree/bindings/mtd/mtd-physmap.txt additional (optional) property is defined: · line 46

Quoted 6 lines

Documentation/devicetree/bindings/mtd/mtd-physmap.txt +3 -0

@@ -41,6 +41,11 @@ additional (optional) property is defined:
 
  - erase-size : The chip's physical erase block size in bytes.
 
+ The device tree may optionally contain endianness property.
+ little-endian or big-endian : It represents connection between controller and
+			flash.

Hm, I'm not even sure it has to do with the HW connection of the data lines (D0:X), probably more something related to how the flash was programmed (some people program the flash using a dedicated programmer before soldering it on the board) and the current platform endianness (which for some platforms, can be changed with a Kconfig option).

Documentation/devicetree/bindings/mtd/mtd-physmap.txt additional (optional) property is defined: · line 47

Quoted 2 lines

If this property is absent, flash is considered
Documentation/devicetree/bindings/mtd/mtd-physmap.txt +1 -0
+			to be host-endian.

The second sentence is no longer valid. Now, if the property is missing the behavior is platform/configuration dependent, not necessarily host-endian.

Quoted 4 lines

+
The device tree may optionally contain sub-nodes describing partitions of the address space. See partition.txt for more detail.

Signature · 3 lines

Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com