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

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

[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 <prabhakar.kushwaha@nxp.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
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(+)

diff --git a/Documentation/devicetree/bindings/mtd/mtd-physmap.txt b/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
index 4a0a48bf4ecb..961a8eed9d0d 100644
--- a/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
+++ b/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
@@ -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.
 
-- 
2.14.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Prabhakar Kushwaha]

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 <jagdish.gediya@nxp.com>
Signed-off-by: Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com>
---
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(+)

diff --git a/drivers/mtd/maps/physmap_of_core.c b/drivers/mtd/maps/physmap_of_core.c
index b1bd4faecfb2..527b1682381f 100644
--- a/drivers/mtd/maps/physmap_of_core.c
+++ b/drivers/mtd/maps/physmap_of_core.c
@@ -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;
-- 
2.14.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Prabhakar Kushwaha]

Dear Boris,

> -----Original Message-----
> From: Prabhakar Kushwaha
> Sent: Wednesday, February 28, 2018 1:04 PM
> To: linux-mtd@lists.infradead.org; devicetree@vger.kernel.org;
> robh@kernel.org; mark.rutland@arm.com; shawnguo@kernel.org
> Cc: linux-arm-kernel@lists.infradead.org; boris.brezillon@free-
> electrons.com; cyrille.pitchen@wedev4u.fr; computersforpeace@gmail.com;
> oss@buserror.net; Leo Li <leoyang.li@nxp.com>; Prabhakar Kushwaha
> <prabhakar.kushwaha@nxp.com>; Jagdish Gediya
> <jagdish.gediya@nxp.com>
> 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 <jagdish.gediya@nxp.com>
> Signed-off-by: Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com>
> ---
> 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(+)
> 
> diff --git a/drivers/mtd/maps/physmap_of_core.c
> b/drivers/mtd/maps/physmap_of_core.c
> index b1bd4faecfb2..527b1682381f 100644
> --- a/drivers/mtd/maps/physmap_of_core.c
> +++ b/drivers/mtd/maps/physmap_of_core.c
> @@ -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;
> --
> 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

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

[Boris Brezillon]

Hi Prabhakar,

On Tue, 6 Mar 2018 06:02:08 +0000
Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com> wrote:

> Dear Boris,
> 
> > -----Original Message-----
> > From: Prabhakar Kushwaha
> > Sent: Wednesday, February 28, 2018 1:04 PM
> > To: linux-mtd@lists.infradead.org; devicetree@vger.kernel.org;
> > robh@kernel.org; mark.rutland@arm.com; shawnguo@kernel.org
> > Cc: linux-arm-kernel@lists.infradead.org; boris.brezillon@free-
> > electrons.com; cyrille.pitchen@wedev4u.fr; computersforpeace@gmail.com;
> > oss@buserror.net; Leo Li <leoyang.li@nxp.com>; Prabhakar Kushwaha
> > <prabhakar.kushwaha@nxp.com>; Jagdish Gediya
> > <jagdish.gediya@nxp.com>
> > 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 <jagdish.gediya@nxp.com>
> > Signed-off-by: Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com>
> > ---
> > 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(+)
> > 
> > diff --git a/drivers/mtd/maps/physmap_of_core.c
> > b/drivers/mtd/maps/physmap_of_core.c
> > index b1bd4faecfb2..527b1682381f 100644
> > --- a/drivers/mtd/maps/physmap_of_core.c
> > +++ b/drivers/mtd/maps/physmap_of_core.c
> > @@ -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;
> > --
> > 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.

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

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

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

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

[Boris Brezillon]

On Wed, 28 Feb 2018 13:03:34 +0530
Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com> wrote:

> 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 <prabhakar.kushwaha@nxp.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
> 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(+)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/mtd-physmap.txt b/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
> index 4a0a48bf4ecb..961a8eed9d0d 100644
> --- a/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
> +++ b/Documentation/devicetree/bindings/mtd/mtd-physmap.txt
> @@ -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).

> If this property is absent, flash is considered
> +			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.

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



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