[PATCHv2 0/3] regmap: add LE and DT binding for endianness

[PATCHv2 0/3] regmap: add LE and DT binding for endianness

[Xiubo Li]

For this patch series has dependency, so resend them together.


Change in v2:
- resend 'regmap: implement LE formatting/parsing for 16/32-bit values.'
- resend 'regmap: Add the DT binding documentation for endianness'
- fix the commit message of 'regmap: add DT endianness binding support.'




Xiubo Li (3):
  regmap: implement LE formatting/parsing for 16/32-bit values.
  regmap: Add the DT binding documentation for endianness
  regmap: add DT endianness binding support.

 .../bindings/regmap/regmap-endianness.txt          |  49 +++++++
 drivers/base/regmap/regmap.c                       | 141 +++++++++++++++++++--
 2 files changed, 180 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/regmap/regmap-endianness.txt

-- 
1.8.4

[PATCHv2 1/3] regmap: implement LE formatting/parsing for 16/32-bit values.

[Xiubo Li]

Allow busses to request little endianness formatting and
parsing for 16- and 32-bit values. This will be useful to
support regmap-mmio.

For the following the scenarios using the regmap-mmio,
for example:

Index    CPU       Device     Endianess flag for values
----------------------------------------------------------
1        LE        LE         REGMAP_ENDIAN_DEFAULT/NATIVE
2        LE        BE         REGMAP_ENDIAN_BIG
3        BE        BE         REGMAP_ENDIAN_DEFAULT/NATIVE
4        BE        LE         REGMAP_ENDIAN_LITTLE

For one device driver, which will support all the cases above,
needs two boolean properties in DT node like: 'big-endian'
for case 2 and 'little-endian' for case 4, and for cases 1
and 3 they all will be absent.

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
---
 drivers/base/regmap/regmap.c | 52 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 6a19515..8e8cea1 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -192,6 +192,13 @@ static void regmap_format_16_be(void *buf, unsigned int val, unsigned int shift)
 	b[0] = cpu_to_be16(val << shift);
 }
 
+static void regmap_format_16_le(void *buf, unsigned int val, unsigned int shift)
+{
+	__le16 *b = buf;
+
+	b[0] = cpu_to_le16(val << shift);
+}
+
 static void regmap_format_16_native(void *buf, unsigned int val,
 				    unsigned int shift)
 {
@@ -216,6 +223,13 @@ static void regmap_format_32_be(void *buf, unsigned int val, unsigned int shift)
 	b[0] = cpu_to_be32(val << shift);
 }
 
+static void regmap_format_32_le(void *buf, unsigned int val, unsigned int shift)
+{
+	__le32 *b = buf;
+
+	b[0] = cpu_to_le32(val << shift);
+}
+
 static void regmap_format_32_native(void *buf, unsigned int val,
 				    unsigned int shift)
 {
@@ -240,6 +254,13 @@ static unsigned int regmap_parse_16_be(const void *buf)
 	return be16_to_cpu(b[0]);
 }
 
+static unsigned int regmap_parse_16_le(const void *buf)
+{
+	const __le16 *b = buf;
+
+	return le16_to_cpu(b[0]);
+}
+
 static void regmap_parse_16_be_inplace(void *buf)
 {
 	__be16 *b = buf;
@@ -247,6 +268,13 @@ static void regmap_parse_16_be_inplace(void *buf)
 	b[0] = be16_to_cpu(b[0]);
 }
 
+static void regmap_parse_16_le_inplace(void *buf)
+{
+	__le16 *b = buf;
+
+	b[0] = le16_to_cpu(b[0]);
+}
+
 static unsigned int regmap_parse_16_native(const void *buf)
 {
 	return *(u16 *)buf;
@@ -269,6 +297,13 @@ static unsigned int regmap_parse_32_be(const void *buf)
 	return be32_to_cpu(b[0]);
 }
 
+static unsigned int regmap_parse_32_le(const void *buf)
+{
+	const __le32 *b = buf;
+
+	return le32_to_cpu(b[0]);
+}
+
 static void regmap_parse_32_be_inplace(void *buf)
 {
 	__be32 *b = buf;
@@ -276,6 +311,13 @@ static void regmap_parse_32_be_inplace(void *buf)
 	b[0] = be32_to_cpu(b[0]);
 }
 
+static void regmap_parse_32_le_inplace(void *buf)
+{
+	__le32 *b = buf;
+
+	b[0] = le32_to_cpu(b[0]);
+}
+
 static unsigned int regmap_parse_32_native(const void *buf)
 {
 	return *(u32 *)buf;
@@ -585,6 +627,11 @@ struct regmap *regmap_init(struct device *dev,
 			map->format.parse_val = regmap_parse_16_be;
 			map->format.parse_inplace = regmap_parse_16_be_inplace;
 			break;
+		case REGMAP_ENDIAN_LITTLE:
+			map->format.format_val = regmap_format_16_le;
+			map->format.parse_val = regmap_parse_16_le;
+			map->format.parse_inplace = regmap_parse_16_le_inplace;
+			break;
 		case REGMAP_ENDIAN_NATIVE:
 			map->format.format_val = regmap_format_16_native;
 			map->format.parse_val = regmap_parse_16_native;
@@ -606,6 +653,11 @@ struct regmap *regmap_init(struct device *dev,
 			map->format.parse_val = regmap_parse_32_be;
 			map->format.parse_inplace = regmap_parse_32_be_inplace;
 			break;
+		case REGMAP_ENDIAN_LITTLE:
+			map->format.format_val = regmap_format_32_le;
+			map->format.parse_val = regmap_parse_32_le;
+			map->format.parse_inplace = regmap_parse_32_le_inplace;
+			break;
 		case REGMAP_ENDIAN_NATIVE:
 			map->format.format_val = regmap_format_32_native;
 			map->format.parse_val = regmap_parse_32_native;
-- 
1.8.4

Re: [PATCHv2 1/3] regmap: implement LE formatting/parsing for 16/32-bit values.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 218 bytes --]

On Wed, Apr 02, 2014 at 06:09:07PM +0800, Xiubo Li wrote:
> Allow busses to request little endianness formatting and
> parsing for 16- and 32-bit values. This will be useful to
> support regmap-mmio.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCHv2 2/3] regmap: Add the DT binding documentation for endianness

[Xiubo Li]

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
---
 .../bindings/regmap/regmap-endianness.txt          | 49 ++++++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regmap/regmap-endianness.txt

diff --git a/Documentation/devicetree/bindings/regmap/regmap-endianness.txt b/Documentation/devicetree/bindings/regmap/regmap-endianness.txt
new file mode 100644
index 0000000..045d347
--- /dev/null
+++ b/Documentation/devicetree/bindings/regmap/regmap-endianness.txt
@@ -0,0 +1,49 @@
+Device-Tree bindings for regmap endianness
+
+Required properties:
+- reg-endian: Register endianness, this string property could be absent
+  as default endianness, or must be one of 'BE', 'LE' and 'NT'.
+- val-endian: Value endianness, this string property could be absent as
+  default endianness, or must be one of 'BE', 'LE' and 'NT'.
+
+The Endianness flags supported by regmap:
+
+DT properties           Macros
+----------------------------------------
+    'LE'          REGMAP_ENDIAN_LITTLE
+    'BE'          REGMAP_ENDIAN_BIG
+    'NT'          REGMAP_ENDIAN_NATIVE
+    Absent        REGMAP_ENDIAN_DEFAULT
+
+Examples:
+Case 1 : CPU in LE mode & SAI device in BE mode, using mmio.
+sai2: sai@40031000 {
+	      compatible = "fsl,vf610-sai";
+	      reg = <0x40031000 0x1000>;
+	      ...
+	      val-endian = 'BE';
+};
+
+Case 2 : CPU in BE mode & SAI device in LE mode, using mmio.
+sai2: sai@40031000 {
+	      compatible = "fsl,vf610-sai";
+	      reg = <0x40031000 0x1000>;
+	      ...
+	      val-endian = 'LE';
+};
+
+Case 3 : CPU in LE mode & SAI device in LE mode, using mmio.
+sai2: sai@40031000 {
+	      compatible = "fsl,vf610-sai";
+	      reg = <0x40031000 0x1000>;
+	      ...
+	      val-endian = 'NT'; or just absent.
+};
+
+Case 4 : CPU in BE mode & SAI device in BE mode, using mmio.
+sai2: sai@40031000 {
+	      compatible = "fsl,vf610-sai";
+	      reg = <0x40031000 0x1000>;
+	      ...
+	      val-endian = 'NT'; or just absent.
+};
-- 
1.8.4

Re: [PATCHv2 2/3] regmap: Add the DT binding documentation for endianness

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 919 bytes --]

On Wed, Apr 02, 2014 at 06:09:08PM +0800, Xiubo Li wrote:

> +sai2: sai@40031000 {
> +	      compatible = "fsl,vf610-sai";
> +	      reg = <0x40031000 0x1000>;
> +	      ...
> +	      val-endian = 'LE';
> +};

This is mostly OK as a binding (though it should be CCed to the DT list
and maintiners as all DT bindings should) except using upper case
doesn't really seem idiomatic for DT - lower case is more normal - and I
don't think we can make these properties mandatory in themselves.
Individual bindings would need to make them mandatory.  It's also odd to
have a mandatory property which may be absent!

It'd probably be better if the binding defined what the default
endianess was too, or just didn't say what happens in cases where
nothing is specified, the latter seems better.  Generally just not
mentioning regmap is better for a binding definition, the binding should
be usable by all OSs and not just Linux.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCHv2 2/3] regmap: Add the DT binding documentation for endianness

[Li.Xiubo]

> Subject: Re: [PATCHv2 2/3] regmap: Add the DT binding documentation for
> endianness
> 
> On Wed, Apr 02, 2014 at 06:09:08PM +0800, Xiubo Li wrote:
> 
> > +sai2: sai@40031000 {
> > +	      compatible = "fsl,vf610-sai";
> > +	      reg = <0x40031000 0x1000>;
> > +	      ...
> > +	      val-endian = 'LE';
> > +};
> 
> This is mostly OK as a binding (though it should be CCed to the DT list
> and maintiners as all DT bindings should) except using upper case
> doesn't really seem idiomatic for DT - lower case is more normal - 

I will your advices.


> and I
> don't think we can make these properties mandatory in themselves.
> Individual bindings would need to make them mandatory.  It's also odd to
> have a mandatory property which may be absent!

Well, yes, It is.

The absent one is just to compatible with the old drivers.



> 
> It'd probably be better if the binding defined what the default
> endianess was too, or just didn't say what happens in cases where
> nothing is specified, the latter seems better.  

I will think it over carefully.


> Generally just not
> mentioning regmap is better for a binding definition, the binding should
> be usable by all OSs and not just Linux.

How about move the endianness OF parsing to the driver/of/ ?
Is this will be better ?

Thanks,

BRs
Xiubo

Re: [PATCHv2 2/3] regmap: Add the DT binding documentation for endianness

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]

On Thu, Apr 03, 2014 at 07:04:00AM +0000, Li.Xiubo@freescale.com wrote:

> > Generally just not
> > mentioning regmap is better for a binding definition, the binding should
> > be usable by all OSs and not just Linux.

> How about move the endianness OF parsing to the driver/of/ ?
> Is this will be better ?

Where the code is is sensible enough, it's an issue about how the
binding documentation was written rather than about the code.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCHv2 2/3] regmap: Add the DT binding documentation for endianness

[Li.Xiubo]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@kernel.org]
> Sent: Tuesday, April 15, 2014 5:10 AM
> To: Xiubo Li-B47053
> Cc: linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCHv2 2/3] regmap: Add the DT binding documentation for
> endianness
> 
> On Thu, Apr 03, 2014 at 07:04:00AM +0000, Li.Xiubo@freescale.com wrote:
> 
> > > Generally just not
> > > mentioning regmap is better for a binding definition, the binding should
> > > be usable by all OSs and not just Linux.
> 
> > How about move the endianness OF parsing to the driver/of/ ?
> > Is this will be better ?
> 
> Where the code is is sensible enough, it's an issue about how the
> binding documentation was written rather than about the code.

Sorry for late.

Well, I will try to enhance this.

Thanks,

BRs
Xiubo

[PATCHv2 3/3] regmap: add DT endianness binding support.

[Xiubo Li]

For many drivers which will support rich endianness of CPU<-->Dev
need define DT properties by itself without the binding support.

The value endianness using regmap-mmio, for example:
Index    CPU       Device     Endianess flag for DT property
------------------------------------------------------------
1        LE        LE         -
2        LE        BE         'big-endian'
3        BE        BE         -
4        BE        LE         'little-endian'

============

Here add DT endianness binding support will define two string
properties of the register and value endiannesses:

'reg-endian' and 'val-endian'.

And the value of them will be:
'LE' : REGMAP_ENDIAN_LITTLE
'BE' : REGMAP_ENDIAN_BIG
'NT' : REGMAP_ENDIAN_NATIVE
Absent : REGMAP_ENDIAN_DEFAULT

The value endianness using regmap-mmio, for example:
Index    CPU       Device     Endianess flag for DT property
------------------------------------------------------------
1        LE        LE         'NT' or absent
2        LE        BE         'BE'
3        BE        BE         'NT' or absent
4        BE        LE         'LE'

Please see the following documetation for detail usage:
    Documentation/devicetree/bindings/regmap/regmap-endianness.txt

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
---
 drivers/base/regmap/regmap.c | 91 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 80 insertions(+), 11 deletions(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 8e8cea1..36a96cc 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -15,6 +15,7 @@
 #include <linux/export.h>
 #include <linux/mutex.h>
 #include <linux/err.h>
+#include <linux/of.h>
 #include <linux/rbtree.h>
 #include <linux/sched.h>
 
@@ -423,6 +424,78 @@ static void regmap_range_exit(struct regmap *map)
 }
 
 /**
+ * of_regmap_endian_get_by_name() - Parse and lookup the endianness referenced
+ * by a device node
+ * @np: pointer to clock consumer node
+ * @name: name of consumer's endianness input
+ *
+ * This function parses the device endianness property, and uses them to
+ * determine the endianness of the registers and values.
+ */
+static int of_regmap_endian_get_by_name(struct device_node *np,
+					const char *endian_name,
+					enum regmap_endian *out_endian)
+{
+	const char *endianness;
+	int ret;
+
+	if (!out_endian)
+		return -EINVAL;
+
+	/* Set the default endianness */
+	*out_endian = REGMAP_ENDIAN_DEFAULT;
+
+	/* Absent or being to use the flag from config of the drivers */
+	if (!of_find_property(np, endian_name, NULL))
+		return 0;
+
+	ret = of_property_read_string(np, endian_name, &endianness);
+	if (ret)
+		return ret;
+
+	if (!strcmp("LE", endianness))
+		*out_endian = REGMAP_ENDIAN_LITTLE;
+	else if (!strcmp("BE", endianness))
+		*out_endian = REGMAP_ENDIAN_BIG;
+	else if (!strcmp("NT", endianness))
+		*out_endian = REGMAP_ENDIAN_NATIVE;
+
+	return 0;
+}
+
+static int of_regmap_get_endian(struct device *dev,
+				const struct regmap_bus *bus,
+				const struct regmap_config *config,
+				const char *endian_name,
+				enum regmap_endian *out_endian)
+{
+	int ret;
+
+	if (!out_endian)
+		return -EINVAL;
+
+	if (dev) {
+		ret = of_regmap_endian_get_by_name(dev->of_node, endian_name,
+						   out_endian);
+		if (ret)
+			return ret;
+	}
+
+	/* To be compatible with the none DT or the old drivers */
+	if (*out_endian != REGMAP_ENDIAN_DEFAULT)
+		return 0;
+
+	/* Parsing the endianness from driver's config or bus */
+	*out_endian = config->reg_format_endian;
+	if (*out_endian == REGMAP_ENDIAN_DEFAULT)
+		*out_endian = bus->reg_format_endian_default;
+	if (*out_endian == REGMAP_ENDIAN_DEFAULT)
+		*out_endian = REGMAP_ENDIAN_BIG;
+
+	return 0;
+}
+
+/**
  * regmap_init(): Initialise register map
  *
  * @dev: Device that will be interacted with
@@ -518,17 +591,13 @@ struct regmap *regmap_init(struct device *dev,
 		map->reg_read  = _regmap_bus_read;
 	}
 
-	reg_endian = config->reg_format_endian;
-	if (reg_endian == REGMAP_ENDIAN_DEFAULT)
-		reg_endian = bus->reg_format_endian_default;
-	if (reg_endian == REGMAP_ENDIAN_DEFAULT)
-		reg_endian = REGMAP_ENDIAN_BIG;
-
-	val_endian = config->val_format_endian;
-	if (val_endian == REGMAP_ENDIAN_DEFAULT)
-		val_endian = bus->val_format_endian_default;
-	if (val_endian == REGMAP_ENDIAN_DEFAULT)
-		val_endian = REGMAP_ENDIAN_BIG;
+	ret = of_regmap_get_endian(dev, bus, config, "reg_endian", &reg_endian);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ret = of_regmap_get_endian(dev, bus, config, "val_endian", &val_endian);
+	if (ret)
+		return ERR_PTR(ret);
 
 	switch (config->reg_bits + map->reg_shift) {
 	case 2:
-- 
1.8.4

Re: [PATCHv2 3/3] regmap: add DT endianness binding support.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 474 bytes --]

On Wed, Apr 02, 2014 at 06:09:09PM +0800, Xiubo Li wrote:

> +	ret = of_regmap_get_endian(dev, bus, config, "reg_endian", &reg_endian);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ret = of_regmap_get_endian(dev, bus, config, "val_endian", &val_endian);
> +	if (ret)
> +		return ERR_PTR(ret);

Actually, if we're going to be doing this for all devices then we
probably need to namespace the properties too.  Not sure what to do for
a prefix though.  One for the DT folks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCHv2 3/3] regmap: add DT endianness binding support.

[Li.Xiubo]

> Subject: Re: [PATCHv2 3/3] regmap: add DT endianness binding support.
> 
> On Wed, Apr 02, 2014 at 06:09:09PM +0800, Xiubo Li wrote:
> 
> > +	ret = of_regmap_get_endian(dev, bus, config, "reg_endian", &reg_endian);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> > +
> > +	ret = of_regmap_get_endian(dev, bus, config, "val_endian", &val_endian);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> 
> Actually, if we're going to be doing this for all devices then we
> probably need to namespace the properties too.  Not sure what to do for
> a prefix though.  One for the DT folks.

How about using one prefix string of each regmap bus?
Such as:
Prefix 'regmap-mmio' for regmap-mmio,
Prefix 'regmap-i2c' for regmap-i2c,
Precix 'regmap-spi' for regmap-spi
...

Or just using the same prefix 'regmap' for all of them ?

Thanks,

BRs
Xiubo

Re: [PATCHv2 3/3] regmap: add DT endianness binding support.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 595 bytes --]

On Thu, Apr 03, 2014 at 05:26:57AM +0000, Li.Xiubo@freescale.com wrote:

> > Actually, if we're going to be doing this for all devices then we
> > probably need to namespace the properties too.  Not sure what to do for
> > a prefix though.  One for the DT folks.

> How about using one prefix string of each regmap bus?
> Such as:
> Prefix 'regmap-mmio' for regmap-mmio,
> Prefix 'regmap-i2c' for regmap-i2c,
> Precix 'regmap-spi' for regmap-spi
> ...

> Or just using the same prefix 'regmap' for all of them ?

I don't see any point in defining separate per bus properties so just
one prefix.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCHv2 0/3] regmap: add LE and DT binding for endianness

[Haijun.Zhang]

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Xiubo Li
> Sent: Wednesday, April 02, 2014 6:09 PM
> To: broonie@kernel.org
> Cc: linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org; Xiubo Li-
> B47053
> Subject: [PATCHv2 0/3] regmap: add LE and DT binding for endianness
>


This patch set is very useful for eSDHC driver, which will support
different endiannesses on both powerpc and arm platforms at the
same time.

And this patch set looks good to me.

> 
> For this patch series has dependency, so resend them together.
> 
> 
> Change in v2:
> - resend 'regmap: implement LE formatting/parsing for 16/32-bit values.'
> - resend 'regmap: Add the DT binding documentation for endianness'
> - fix the commit message of 'regmap: add DT endianness binding support.'
> 
> 
> 
> 
> Xiubo Li (3):
>   regmap: implement LE formatting/parsing for 16/32-bit values.
>   regmap: Add the DT binding documentation for endianness
>   regmap: add DT endianness binding support.
> 
>  .../bindings/regmap/regmap-endianness.txt          |  49 +++++++
>  drivers/base/regmap/regmap.c                       | 141
> +++++++++++++++++++--
>  2 files changed, 180 insertions(+), 10 deletions(-)  create mode 100644
> Documentation/devicetree/bindings/regmap/regmap-endianness.txt
> 
> --
> 1.8.4
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>