[PATCHv4 0/2] add DT endianness binding support.

[PATCHv4 0/2] add DT endianness binding support.

[Xiubo Li]

Changes in V4:
- Fix one comment from Markus Pargmann.
- Fix some comments.

Changes in V3:
- Follow Mark Rutland's advice.
- Document common case of the endianess usage.

Changes in V2:
- Namespace the properties using the prefix sring.
- Add one binding ducomentation off regmap.




Xiubo Li (2):
  dt/bindings: Add the DT binding documentation for endianness
  regmap: add DT endianness binding support.

 .../devicetree/bindings/endianness/endianness.txt  |  48 +++++++
 drivers/base/regmap/regmap-i2c.c                   |   2 +
 drivers/base/regmap/regmap-spi.c                   |   2 +
 drivers/base/regmap/regmap.c                       | 147 +++++++++++++++++++--
 4 files changed, 188 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/endianness/endianness.txt

-- 
1.8.4

[PATCHv4 1/2] dt/bindings: Add the DT binding documentation for endianness

[Xiubo Li]

Device-Tree binding for device endianness

The endianness mode of CPU & Device scenarios:
Index    CPU       Device     Endianness properties
------------------------------------------------------------
1        LE        LE         -
2        LE        BE         'big-endian{,-*}'
3        BE        BE         -
4        BE        LE         'little-endian{,-*}'

{big,little}-endian{,-*}: these are boolean properties, if absent
meaning that the CPU and the Device are in the same endianness mode.

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

diff --git a/Documentation/devicetree/bindings/endianness/endianness.txt b/Documentation/devicetree/bindings/endianness/endianness.txt
new file mode 100644
index 0000000..cc5f7f8
--- /dev/null
+++ b/Documentation/devicetree/bindings/endianness/endianness.txt
@@ -0,0 +1,48 @@
+Device-Tree binding for device endianness
+
+The endianness mode of CPU & Device scenarios:
+Index    CPU       Device     Endianness properties
+------------------------------------------------------------
+1        LE        LE         -
+2        LE        BE         'big-endian{,-*}'
+3        BE        BE         -
+4        BE        LE         'little-endian{,-*}'
+
+For one device driver, which will run in different scenarios above
+on different SoCs using the devicetree, we need one way to simplify
+this.
+
+Required properties:
+- {big,little}-endian{,-*}: these are boolean properties, if absent
+  meaning that the CPU and the Device are in the same endianness mode.
+
+Examples:
+Scenario 1 : CPU in LE mode & device in LE mode.
+dev: dev@40031000 {
+	      compatible = "name";
+	      reg = <0x40031000 0x1000>;
+	      ...
+};
+
+Scenario 2 : CPU in LE mode & device in BE mode.
+dev: dev@40031000 {
+	      compatible = "name";
+	      reg = <0x40031000 0x1000>;
+	      ...
+	      big-endian{,-*};
+};
+
+Scenario 3 : CPU in BE mode & device in BE mode.
+dev: dev@40031000 {
+	      compatible = "name";
+	      reg = <0x40031000 0x1000>;
+	      ...
+};
+
+Scenario 4 : CPU in BE mode & device in LE mode.
+dev: dev@40031000 {
+	      compatible = "name";
+	      reg = <0x40031000 0x1000>;
+	      ...
+	      little-endian{,-*};
+};
-- 
1.8.4

Re: [PATCHv4 1/2] dt/bindings: Add the DT binding documentation for endianness

[Mark Rutland]

On Fri, May 09, 2014 at 03:04:32AM +0100, Xiubo Li wrote:
> Device-Tree binding for device endianness
> 
> The endianness mode of CPU & Device scenarios:
> Index    CPU       Device     Endianness properties
> ------------------------------------------------------------
> 1        LE        LE         -
> 2        LE        BE         'big-endian{,-*}'
> 3        BE        BE         -
> 4        BE        LE         'little-endian{,-*}'
> 
> {big,little}-endian{,-*}: these are boolean properties, if absent
> meaning that the CPU and the Device are in the same endianness mode.

That's not really true though. A device might usually be little-endian,
regardless of the endianness of a CPU. Some vendors may integrate it as
big-endian after a binding is added, and in the absence of a specified
endianness a driver is likely to assume LE.

I am not keen on stating in such a generic document that the device is
the same endianness as the CPU. As some CPUs can change endianness
dynamically it's meaningless to say so.

> 
> Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
> ---
>  .../devicetree/bindings/endianness/endianness.txt  | 48 ++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/endianness/endianness.txt
> 
> diff --git a/Documentation/devicetree/bindings/endianness/endianness.txt b/Documentation/devicetree/bindings/endianness/endianness.txt
> new file mode 100644
> index 0000000..cc5f7f8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/endianness/endianness.txt
> @@ -0,0 +1,48 @@
> +Device-Tree binding for device endianness
> +
> +The endianness mode of CPU & Device scenarios:
> +Index    CPU       Device     Endianness properties
> +------------------------------------------------------------
> +1        LE        LE         -
> +2        LE        BE         'big-endian{,-*}'
> +3        BE        BE         -
> +4        BE        LE         'little-endian{,-*}'
> +
> +For one device driver, which will run in different scenarios above
> +on different SoCs using the devicetree, we need one way to simplify
> +this.
> +
> +Required properties:
> +- {big,little}-endian{,-*}: these are boolean properties, if absent
> +  meaning that the CPU and the Device are in the same endianness mode.

As stated above, I disagree with this statement.

The endianness of the CPU should have nothing to do with the device
description. The DT should not consider the endianness of the CPU at all
because this can be a dynamic property of the system.  The kernel knows
which endianness the CPU is in, and in some cases the kernel will have
explicitly changed the endianness of the CPU.

All this document needs to say is that if a device may be integrated
with little-endian or big-endian registers, the preferred way to
distinguish between these cases is with a boolean big-endian or
little-endian property. Whether this is required and what the default
happens to be is entirely binding specific.

For those cases where the endianness of sub-componenets may vary (i.e. a
single regsiter may vary endianness, or a whole sub-block), then
big-endian-* and little-endian-* properties are the preferred way to
describe this.

This definitely cannot be required in general. We already have bindings
which optionally use this style of property.

Cheers,
Mark.

> +
> +Examples:
> +Scenario 1 : CPU in LE mode & device in LE mode.
> +dev: dev@40031000 {
> +	      compatible = "name";
> +	      reg = <0x40031000 0x1000>;
> +	      ...
> +};
> +
> +Scenario 2 : CPU in LE mode & device in BE mode.
> +dev: dev@40031000 {
> +	      compatible = "name";
> +	      reg = <0x40031000 0x1000>;
> +	      ...
> +	      big-endian{,-*};
> +};
> +
> +Scenario 3 : CPU in BE mode & device in BE mode.
> +dev: dev@40031000 {
> +	      compatible = "name";
> +	      reg = <0x40031000 0x1000>;
> +	      ...
> +};
> +
> +Scenario 4 : CPU in BE mode & device in LE mode.
> +dev: dev@40031000 {
> +	      compatible = "name";
> +	      reg = <0x40031000 0x1000>;
> +	      ...
> +	      little-endian{,-*};
> +};
> -- 
> 1.8.4
> 
> 

Re: [PATCHv4 1/2] dt/bindings: Add the DT binding documentation for endianness

[Mark Brown]

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

On Fri, May 09, 2014 at 05:32:43PM +0100, Mark Rutland wrote:
> On Fri, May 09, 2014 at 03:04:32AM +0100, Xiubo Li wrote:

> > {big,little}-endian{,-*}: these are boolean properties, if absent
> > meaning that the CPU and the Device are in the same endianness mode.

> That's not really true though. A device might usually be little-endian,
> regardless of the endianness of a CPU. Some vendors may integrate it as
> big-endian after a binding is added, and in the absence of a specified
> endianness a driver is likely to assume LE.

The default should be device specific rather than binding specific.

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

Re: [PATCHv4 1/2] dt/bindings: Add the DT binding documentation for endianness

[Mark Rutland]

On Fri, May 09, 2014 at 06:02:55PM +0100, Mark Brown wrote:
> On Fri, May 09, 2014 at 05:32:43PM +0100, Mark Rutland wrote:
> > On Fri, May 09, 2014 at 03:04:32AM +0100, Xiubo Li wrote:
> 
> > > {big,little}-endian{,-*}: these are boolean properties, if absent
> > > meaning that the CPU and the Device are in the same endianness mode.
> 
> > That's not really true though. A device might usually be little-endian,
> > regardless of the endianness of a CPU. Some vendors may integrate it as
> > big-endian after a binding is added, and in the absence of a specified
> > endianness a driver is likely to assume LE.
> 
> The default should be device specific rather than binding specific.

I'm taking binding here to mean the binding for a praticular device
rather than a class binding, so there's only a subtle distinction
between the two.

It's entirely possible that two developers independently come up with
bindings for something that is later realised to be the same device
(just integrated differently), for which they choose opposite defaults.

In that case the default is binding-specific rather than device
specific, though I would hope that in the vast majority of cases there
is only one binding per device, at which point the distinction is
meaningless.

Cheers,
Mark.

[PATCHv4 2/2] 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 endianness using regmap:
Index    CPU       Device     Endianess flag for DT bool property
------------------------------------------------------------
1        LE        LE         -
2        LE        BE         'big-endian-{val,reg}'
3        BE        BE         -
4        BE        LE         'little-endian-{val,reg}'

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

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
---
 drivers/base/regmap/regmap-i2c.c |   2 +
 drivers/base/regmap/regmap-spi.c |   2 +
 drivers/base/regmap/regmap.c     | 148 ++++++++++++++++++++++++++++++++++++---
 3 files changed, 141 insertions(+), 11 deletions(-)

diff --git a/drivers/base/regmap/regmap-i2c.c b/drivers/base/regmap/regmap-i2c.c
index ebd1895..9b0fbf9 100644
--- a/drivers/base/regmap/regmap-i2c.c
+++ b/drivers/base/regmap/regmap-i2c.c
@@ -95,6 +95,8 @@ static struct regmap_bus regmap_i2c = {
 	.write = regmap_i2c_write,
 	.gather_write = regmap_i2c_gather_write,
 	.read = regmap_i2c_read,
+	.reg_format_endian_default = REGMAP_ENDIAN_BIG,
+	.val_format_endian_default = REGMAP_ENDIAN_BIG,
 };
 
 /**
diff --git a/drivers/base/regmap/regmap-spi.c b/drivers/base/regmap/regmap-spi.c
index 0eb3097..53d1148 100644
--- a/drivers/base/regmap/regmap-spi.c
+++ b/drivers/base/regmap/regmap-spi.c
@@ -109,6 +109,8 @@ static struct regmap_bus regmap_spi = {
 	.async_alloc = regmap_spi_async_alloc,
 	.read = regmap_spi_read,
 	.read_flag_mask = 0x80,
+	.reg_format_endian_default = REGMAP_ENDIAN_BIG,
+	.val_format_endian_default = REGMAP_ENDIAN_BIG,
 };
 
 /**
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 63e30ef..4805246 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>
 
@@ -402,6 +403,133 @@ int regmap_attach_dev(struct device *dev, struct regmap *map,
 }
 EXPORT_SYMBOL_GPL(regmap_attach_dev);
 
+enum regmap_endian_type {
+	REGMAP_ENDIAN_REG,
+	REGMAP_ENDIAN_VAL,
+};
+
+/**
+ * of_regmap_endian_by_type() - Parses and lookups the endianness referenced
+ * by a device node
+ * @np: pointer to the consumer device node
+ * @type: endianness type for values or registers
+ *
+ * This function parses the device endianness properties, and uses them to
+ * determine the endianness of the device's registers and values.
+ */
+static int of_regmap_endian_by_type(struct device_node *np,
+					enum regmap_endian_type type,
+					enum regmap_endian *endian)
+{
+	if (!endian)
+		return -EINVAL;
+
+	switch (type) {
+	case REGMAP_ENDIAN_REG:
+		if (of_property_read_bool(np, "big-endian-reg"))
+			*endian = REGMAP_ENDIAN_BIG;
+		else if (of_property_read_bool(np, "little-endian-reg"))
+			*endian = REGMAP_ENDIAN_LITTLE;
+		else
+			*endian = REGMAP_ENDIAN_NATIVE;
+		break;
+	case REGMAP_ENDIAN_VAL:
+		if (of_property_read_bool(np, "big-endian-val"))
+			*endian = REGMAP_ENDIAN_BIG;
+		else if (of_property_read_bool(np, "little-endian-val"))
+			*endian = REGMAP_ENDIAN_LITTLE;
+		else
+			*endian = REGMAP_ENDIAN_NATIVE;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int of_regmap_get_endian(struct device *dev,
+				const struct regmap_bus *bus,
+				const struct regmap_config *config,
+				enum regmap_endian_type type,
+				enum regmap_endian *endian)
+{
+	int ret;
+
+	if (!endian || !config)
+		return -EINVAL;
+
+	/*
+	 * Firstly, try to parse the endianness from driver's config,
+	 * this is to be compatible with the none DT or the old drivers.
+	 * From the driver's config the endianness value maybe:
+	 *   REGMAP_ENDIAN_BIG,
+	 *   REGMAP_ENDIAN_LITTLE,
+	 *   REGMAP_ENDIAN_NATIVE,
+	 *   REGMAP_ENDIAN_DEFAULT.
+	 */
+	switch (type) {
+	case REGMAP_ENDIAN_REG:
+		*endian = config->reg_format_endian;
+		break;
+	case REGMAP_ENDIAN_VAL:
+		*endian = config->val_format_endian;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/*
+	 * If the endianness parsed from driver config is
+	 * REGMAP_ENDIAN_DEFAULT, that means maybe we are using the DT
+	 * node to specify the endianness information.
+	 */
+	if (*endian != REGMAP_ENDIAN_DEFAULT)
+		return 0;
+
+	/*
+	 * Secondly, try to parse the endianness from DT node if the
+	 * driver config does not specify it.
+	 * From the DT node the endianness value maybe:
+	 *   REGMAP_ENDIAN_BIG,
+	 *   REGMAP_ENDIAN_LITTLE,
+	 *   REGMAP_ENDIAN_NATIVE,
+	 */
+	if (dev) {
+		ret = of_regmap_endian_by_type(dev->of_node, type, endian);
+		if (ret < 0)
+			return ret;
+	}
+
+	/*
+	 * If the endianness parsed from DT node is REGMAP_ENDIAN_NATIVE, that
+	 * maybe means the DT does not care the endianness or it should use
+	 * the regmap bus's default endianness, then we should try to check
+	 * whether the regmap bus has specified the default endianness.
+	 */
+	if (*endian != REGMAP_ENDIAN_NATIVE)
+		return 0;
+
+	/*
+	 * Finally, try to parse the endianness from regmap bus config
+	 * if in device's DT node the endianness property is absent.
+	 */
+	switch (type) {
+	case REGMAP_ENDIAN_REG:
+		if (bus && bus->reg_format_endian_default)
+			*endian = bus->reg_format_endian_default;
+		break;
+	case REGMAP_ENDIAN_VAL:
+		if (bus && bus->val_format_endian_default)
+			*endian = bus->val_format_endian_default;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /**
  * regmap_init(): Initialise register map
  *
@@ -499,17 +627,15 @@ 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, REGMAP_ENDIAN_REG,
+				   &reg_endian);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ret = of_regmap_get_endian(dev, bus, config, REGMAP_ENDIAN_VAL,
+				   &val_endian);
+	if (ret)
+		return ERR_PTR(ret);
 
 	switch (config->reg_bits + map->reg_shift) {
 	case 2:
-- 
1.8.4

Re: [PATCHv4 2/2] regmap: add DT endianness binding support.

[Mark Rutland]

On Fri, May 09, 2014 at 03:04:33AM +0100, Xiubo Li wrote:
> For many drivers which will support rich endianness of CPU<-->Dev
> need define DT properties by itself without the binding support.
> 
> The endianness using regmap:
> Index    CPU       Device     Endianess flag for DT bool property
> ------------------------------------------------------------
> 1        LE        LE         -
> 2        LE        BE         'big-endian-{val,reg}'
> 3        BE        BE         -
> 4        BE        LE         'little-endian-{val,reg}'

Get rid of the CPU column. It has precisely _nothing_ to do with the
device.

If you happen to have a device that can be integrated with varying
endianness, the endianness should be described regardless of whether
this happens to be the same as the CPU endianness. The kernel can then
choose to do the right thing regardless.

Assuming LE or BE by default is sane if most implementations are one
rather than the other. Probing and figuring it out dynamically is also
fine. Assuming that it's the same as the kernel is broken in general,
and should be avoided -- those cases _require_ a *-endian property to
work if the CPU can function in either endianness.

> Please see the following documetation for detail:
>     Documentation/devicetree/bindings/endianness/endianness.txt

I don't think this is sufficient. That document describes the preferred
idiom, not the meaning w.r.t. a specific binding.

[...]

> +	case REGMAP_ENDIAN_REG:
> +		if (of_property_read_bool(np, "big-endian-reg"))
> +			*endian = REGMAP_ENDIAN_BIG;
> +		else if (of_property_read_bool(np, "little-endian-reg"))
> +			*endian = REGMAP_ENDIAN_LITTLE;

While this follows the guidelines you've added, context is still
required to understand precisely what this means. We need a binding
document describing what *-endian-reg means for this binding (i.e. what
does -reg cover? All registers? some? buffers?).

Imagine I added a little-endian-foo property. You'd be able to reason
that something is little endian, but you'd have no idea of precisely
what without reading documentation or code. As not everyone wants to
read several thousand lines of Linux kernel code to write a dts we
require documentation.

> +	case REGMAP_ENDIAN_VAL:
> +		if (of_property_read_bool(np, "big-endian-val"))
> +			*endian = REGMAP_ENDIAN_BIG;
> +		else if (of_property_read_bool(np, "little-endian-val"))
> +			*endian = REGMAP_ENDIAN_LITTLE;

Likewise.

Cheers,
Mark.

RE: [PATCHv4 2/2] regmap: add DT endianness binding support.

[Li.Xiubo]

> Subject: Re: [PATCHv4 2/2] regmap: add DT endianness binding support.
> 
> On Fri, May 09, 2014 at 03:04:33AM +0100, Xiubo Li wrote:
> > For many drivers which will support rich endianness of CPU<-->Dev
> > need define DT properties by itself without the binding support.
> >
> > The endianness using regmap:
> > Index    CPU       Device     Endianess flag for DT bool property
> > ------------------------------------------------------------
> > 1        LE        LE         -
> > 2        LE        BE         'big-endian-{val,reg}'
> > 3        BE        BE         -
> > 4        BE        LE         'little-endian-{val,reg}'
> 
> Get rid of the CPU column. It has precisely _nothing_ to do with the
> device.
> 
> If you happen to have a device that can be integrated with varying
> endianness, the endianness should be described regardless of whether
> this happens to be the same as the CPU endianness. The kernel can then
> choose to do the right thing regardless.
> 
> Assuming LE or BE by default is sane if most implementations are one
> rather than the other. Probing and figuring it out dynamically is also
> fine. Assuming that it's the same as the kernel is broken in general,
> and should be avoided -- those cases _require_ a *-endian property to
> work if the CPU can function in either endianness.
> 

Yes, If my understanding is correct, if we need inverting the bytes order, 
should be add one property here, regardless of the CPU's endianesses.


> > Please see the following documetation for detail:
> >     Documentation/devicetree/bindings/endianness/endianness.txt
> 
> I don't think this is sufficient. That document describes the preferred
> idiom, not the meaning w.r.t. a specific binding.
> 
> [...]
> 
> > +	case REGMAP_ENDIAN_REG:
> > +		if (of_property_read_bool(np, "big-endian-reg"))
> > +			*endian = REGMAP_ENDIAN_BIG;
> > +		else if (of_property_read_bool(np, "little-endian-reg"))
> > +			*endian = REGMAP_ENDIAN_LITTLE;
> 
> While this follows the guidelines you've added, context is still
> required to understand precisely what this means. We need a binding
> document describing what *-endian-reg means for this binding (i.e. what
> does -reg cover? All registers? some? buffers?).
> 

Yes, for now the 'reg' is for all registers of the device.
And the 'val' is for all the values and buffers of the device.

@Mark Brown,
Do you have any correction here ?


> Imagine I added a little-endian-foo property. You'd be able to reason
> that something is little endian, but you'd have no idea of precisely
> what without reading documentation or code. As not everyone wants to
> read several thousand lines of Linux kernel code to write a dts we
> require documentation.
> 

@Mark Rutland, @Mark Brown,
Yes, where should I locate the documentation ?
Is Documentation/devicetree/bindings/regmap/ okay ?


Thanks,

BRs
Xiubo


> > +	case REGMAP_ENDIAN_VAL:
> > +		if (of_property_read_bool(np, "big-endian-val"))
> > +			*endian = REGMAP_ENDIAN_BIG;
> > +		else if (of_property_read_bool(np, "little-endian-val"))
> > +			*endian = REGMAP_ENDIAN_LITTLE;
> 
> Likewise.
> 
> Cheers,
> Mark.