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

[PATCH 1/2][v6] 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
Changes for v6: Updated binding when endianness property is absent

 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..691c98f7301d 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, connection is described
+			by the CFI_DEFAULT_ENDIAN.
+
 The device tree may optionally contain sub-nodes describing partitions of the
 address space. See partition.txt for more detail.
 
-- 
2.14.1

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

[Prabhakar Kushwaha]

Hi Boris,


> -----Original Message-----
> From: Prabhakar Kushwaha
> Sent: Monday, March 12, 2018 1:41 PM
> To: linux-mtd@lists.infradead.org; devicetree@vger.kernel.org;
> robh@kernel.org; mark.rutland@arm.com; shawnguo@kernel.org
> Cc: boris.brezillon@free-electrons.com; computersforpeace@gmail.com;
> oss@buserror.net; Leo Li <leoyang.li@nxp.com>; linux-arm-
> kernel@lists.infradead.org; Prabhakar Kushwaha
> <prabhakar.kushwaha@nxp.com>
> Subject: [PATCH 1/2][v6] 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 <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
> Changes for v6: Updated binding when endianness property is absent
> 

I hope this version is good enough to be accepted. 

May I go ahead and send device tree changes for other NXP SoCs.

--pk

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

[Boris Brezillon]

On Mon, 12 Mar 2018 13:41:28 +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
> Changes for v6: Updated binding when endianness property is absent
> 
>  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..691c98f7301d 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

You still haven't answered the comments I made on your v5. To me, this
does not represent how the controller and chip pins are connected, but
how the chip was programmed and which endianness should be used by the
controller to correctly read the data back. Maybe I'm wrong, hence my
question.

> +			flash. If this property is absent, connection is described
> +			by the CFI_DEFAULT_ENDIAN.

Nope, you can't refer to linux-specific config options in a DT binding,
because those bindings are supposed to be OS-agnostic. Maybe something
like "If this property is missing, the endianness is chosen by the
system (potentially based on extra configuration options).".

Rob, any suggestion other suggestion?

Regards,

Boris


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

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

[Prabhakar Kushwaha]

Hi Boris,

> -----Original Message-----
> From: Boris Brezillon [mailto:boris.brezillon@bootlin.com]
> Sent: Friday, March 23, 2018 2:04 PM
> To: Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com>;
> robh@kernel.org
> Cc: linux-mtd@lists.infradead.org; devicetree@vger.kernel.org;
> mark.rutland@arm.com; shawnguo@kernel.org; boris.brezillon@free-
> electrons.com; computersforpeace@gmail.com; oss@buserror.net; Leo Li
> <leoyang.li@nxp.com>; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH 1/2][v6] dt-bindings: mtd-physmap: Add endianness
> supports
> 
> On Mon, 12 Mar 2018 13:41:28 +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://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat
> >
> chwork.ozlabs.org%2Fpatch%2F842543%2F&data=02%7C01%7Cprabhakar.ku
> shwah
> >
> a%40nxp.com%7C4e3c62614d144bdbb76408d59098d999%7C686ea1d3bc2b4c
> 6fa92cd
> >
> 99c5c301635%7C0%7C0%7C636573908516332353&sdata=Od7aqu%2BqV4RHB
> EmT08UOb
> > H2EFGgWmGfftCRKlOlj1kY%3D&reserved=0
> > Changes for v5: Sending as it is
> > Changes for v6: Updated binding when endianness property is absent
> >
> >  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..691c98f7301d 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
> 
> You still haven't answered the comments I made on your v5. To me, this does
> not represent how the controller and chip pins are connected, but how the
> chip was programmed and which endianness should be used by the
> controller to correctly read the data back. Maybe I'm wrong, hence my
> question.
> 

NXP's ARM SoC has IFC module which interface with NOR flash. Here IFC is big endian module connected with NOR flash. 
As SoC has ARM processor(Littler Endian),  CONFIG_MTD_CFI_BE_BYTE_SWAP needs to be enabled  to make sure data is read correctly.  
This is the reason, I wrote about connection between controller and flash.

In a way, I agree with you.  It is not about connection. 
It is about how controller read the data (inherently ARM processor)

So I am planning to write below text
little-endian or big-endian: It represents endianness of controller for correct data reading from flash. 
                                                 If this property is missing, the endianness is chosen by the system (potentially based
                                                on extra configuration options).

please let me know if above definition requires more changes. 

Regards,
Prabhakar

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

[Boris Brezillon]

On Tue, 27 Mar 2018 12:06:41 +0000
Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com> wrote:

> Hi Boris,
> 
> > -----Original Message-----
> > From: Boris Brezillon [mailto:boris.brezillon@bootlin.com]
> > Sent: Friday, March 23, 2018 2:04 PM
> > To: Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com>;
> > robh@kernel.org
> > Cc: linux-mtd@lists.infradead.org; devicetree@vger.kernel.org;
> > mark.rutland@arm.com; shawnguo@kernel.org; boris.brezillon@free-
> > electrons.com; computersforpeace@gmail.com; oss@buserror.net; Leo Li
> > <leoyang.li@nxp.com>; linux-arm-kernel@lists.infradead.org
> > Subject: Re: [PATCH 1/2][v6] dt-bindings: mtd-physmap: Add endianness
> > supports
> > 
> > On Mon, 12 Mar 2018 13:41:28 +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://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat  
> > >  
> > chwork.ozlabs.org%2Fpatch%2F842543%2F&data=02%7C01%7Cprabhakar.ku
> > shwah  
> > >  
> > a%40nxp.com%7C4e3c62614d144bdbb76408d59098d999%7C686ea1d3bc2b4c
> > 6fa92cd  
> > >  
> > 99c5c301635%7C0%7C0%7C636573908516332353&sdata=Od7aqu%2BqV4RHB
> > EmT08UOb  
> > > H2EFGgWmGfftCRKlOlj1kY%3D&reserved=0
> > > Changes for v5: Sending as it is
> > > Changes for v6: Updated binding when endianness property is absent
> > >
> > >  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..691c98f7301d 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  
> > 
> > You still haven't answered the comments I made on your v5. To me, this does
> > not represent how the controller and chip pins are connected, but how the
> > chip was programmed and which endianness should be used by the
> > controller to correctly read the data back. Maybe I'm wrong, hence my
> > question.
> >   
> 
> NXP's ARM SoC has IFC module which interface with NOR flash. Here IFC is big endian module connected with NOR flash. 
> As SoC has ARM processor(Littler Endian),  CONFIG_MTD_CFI_BE_BYTE_SWAP needs to be enabled  to make sure data is read correctly.  
> This is the reason, I wrote about connection between controller and flash.
> 
> In a way, I agree with you.  It is not about connection. 
> It is about how controller read the data (inherently ARM processor)
> 
> So I am planning to write below text
> little-endian or big-endian: It represents endianness of controller for correct data reading from flash.

"
 Represents the endianness that should be used by the controller to
 properly read/write data from/to the flash.
"

>                                                  If this property is missing, the endianness is chosen by the system (potentially based
>                                                 on extra configuration options).

This part is good.

> 
> please let me know if above definition requires more changes. 
> 
> Regards,
> Prabhakar



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

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

[Scott Wood]

On Tue, 2018-03-27 at 12:06 +0000, Prabhakar Kushwaha wrote:
> Hi Boris,
> 
> > -----Original Message-----
> > From: Boris Brezillon [mailto:boris.brezillon@bootlin.com]
> > Sent: Friday, March 23, 2018 2:04 PM
> > To: Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com>;
> > robh@kernel.org
> > Cc: linux-mtd@lists.infradead.org; devicetree@vger.kernel.org;
> > mark.rutland@arm.com; shawnguo@kernel.org; boris.brezillon@free-
> > electrons.com; computersforpeace@gmail.com; oss@buserror.net; Leo Li
> > <leoyang.li@nxp.com>; linux-arm-kernel@lists.infradead.org
> > Subject: Re: [PATCH 1/2][v6] dt-bindings: mtd-physmap: Add endianness
> > supports
> > 
> > You still haven't answered the comments I made on your v5. To me, this
> > does
> > not represent how the controller and chip pins are connected, but how the
> > chip was programmed and which endianness should be used by the
> > controller to correctly read the data back. Maybe I'm wrong, hence my
> > question.
> > 
> 
> NXP's ARM SoC has IFC module which interface with NOR flash. Here IFC is big
> endian module connected with NOR flash. 
> As SoC has ARM processor(Littler Endian),  CONFIG_MTD_CFI_BE_BYTE_SWAP needs
> to be enabled  to make sure data is read correctly.  
> This is the reason, I wrote about connection between controller and flash.
> 
> In a way, I agree with you.  It is not about connection. 
> It is about how controller read the data (inherently ARM processor)

It is not about anything inherent to ARM (which, like PPC, is a bi-endian
arch).  It's about the programming model of IFC on certain SoCs.

-Scott