[1/2] arm/mx5: parse iomuxc pad configuratoin from device tree

It adds function mxc_iomuxc_dt_init() to parse iomuxc pad configuration
from device tree.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
---
 .../devicetree/bindings/arm/fsl/iomuxc.txt         |   47 +++++++++++++
 arch/arm/mach-mx5/Makefile                         |    2 +
 arch/arm/mach-mx5/iomuxc-dt.c                      |   72 ++++++++++++++++++++
 arch/arm/plat-mxc/include/mach/common.h            |    3 +
 4 files changed, 124 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/fsl/iomuxc.txt
 create mode 100644 arch/arm/mach-mx5/iomuxc-dt.c

On Mon, Jul 25, 2011 at 11:07:46PM +0800, Shawn Guo wrote:
> It adds function mxc_iomuxc_dt_init() to parse iomuxc pad configuration
> from device tree.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  .../devicetree/bindings/arm/fsl/iomuxc.txt         |   47 +++++++++++++
>  arch/arm/mach-mx5/Makefile                         |    2 +
>  arch/arm/mach-mx5/iomuxc-dt.c                      |   72 ++++++++++++++++++++
>  arch/arm/plat-mxc/include/mach/common.h            |    3 +
>  4 files changed, 124 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/fsl/iomuxc.txt
>  create mode 100644 arch/arm/mach-mx5/iomuxc-dt.c
> 
> diff --git a/Documentation/devicetree/bindings/arm/fsl/iomuxc.txt b/Documentation/devicetree/bindings/arm/fsl/iomuxc.txt
> new file mode 100644
> index 0000000..ae9292b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/fsl/iomuxc.txt
> @@ -0,0 +1,47 @@
> +* Freescale i.MX IOMUX Controller (IOMUXC)
> +
> +Required properties:
> +- compatible : "fsl,<soc>-iomuxc";
> +
> +Sub-nodes present individual PAD configuration, and node name is the
> +PAD name given by hardware document.
> +
> +Required properties:
> +- reg : Should contain the offset of registers
> +  IOMUXC_SW_MUX_CTL_PAD_<pad-name> and IOMUXC_SW_PAD_CTL_PAD_<pad-name>.
> +- fsl,iomuxc-mux-mode : Should specify the MUX_MODE setting of register
> +  IOMUXC_SW_MUX_CTL_PAD_<pad-name>.
> +
> +Optional properties:
> +- fsl,iomuxc-sion : Indicates that bit SION of register
> +  IOMUXC_SW_MUX_CTL_PAD_<pad-name> needs to be set for given MUX_MODE
> +  setting of the PAD.
> +- fsl,iomuxc-select-input : Specify the offset of register
> +  IOMUXC_<...>_SELECT_INPUT and the value of bit-field DAISY for given
> +  MUX_MODE setting of the PAD.

This could get really verbose in a really big hurry.  Fortunately the
dtb format is sophisticated enough to only store each unique property
name once, so the data shouldn't be huge, but it is still going to
make for huge source files.  Can you think of a more concise
representation?

The current linux code has each pin config simply a u64.  Now, an
engineer certainly wouldn't be asked to write raw u64 values, but we
could add some form of #define or macro syntax to dtc so that the
symbolic names currently used in the board.c file would continue to
work.  Alternately, we could just make each pin configuration a string
that can be parsed by the kernel at runtime.  A string would certainly
be the most extensible, at the cost of some more init code in the
kernel.

Something along the lines of:
	iomuxc@53fa8000 {
		compatible = "fsl,imx53-iomuxc"
		fsl,iomux-pads = "csi0-dat10,uart1-txd",
				"csi0-dat11,uart1-rxd",
				...;
	};

Or if you prefer doing the parsing in dtc:
	iomuxc@53fa8000 {
		compatible = "fsl,imx53-iomuxc"
		fsl,iomux-pads = <MX53_PAD_CSI0_DAT10__UART1_TXD_MUX
				  MX53_PAD_CSI0_DAT11__UART2_RXD_MUX
				  ...
				  >;
	};

And we'd need a syntax for the macros.  However, right now
iomux-mx53.h is *huge*, which I'm not hugely keen on also bringing
into DTC...  but there might just not be any good way around that
because pinmux configuration is by definition complexe.  It would
certainly be better to be pre-processing that large data block in dtc
rather than trying to encode the whole thing into kernel setup code.

A third option would be to create the pinmux table in C code with gcc and
objdump, and include it into the dtb with a /bin-inc/ statement.  That
option would work right now without having to change the format of the
iomux #defines.

g.

> +
> +Examples:
> +
> +iomuxc@53fa8000 {
> +	#address-cells = <2>;
> +	#size-cells = <0>;
> +	compatible = "fsl,imx53-iomuxc";
> +	reg = <0x53fa8000 0x4000>;
> +
> +	/*
> +	 * I2C2
> +	 */
> +	key-col3 { /* I2C2_SCL */
> +		reg = <0x3c 0x364>;
> +		fsl,iomuxc-mux-mode = <4>;
> +		fsl,iomuxc-sion;
> +		fsl,iomuxc-select-input = <0x81c 0x0>;
> +	};
> +
> +	key-row3 { /* I2C2_SDA */
> +		reg = <0x40 0x368>;
> +		fsl,iomuxc-mux-mode = <4>;
> +		fsl,iomuxc-sion;
> +		fsl,iomuxc-select-input = <0x820 0x0>;
> +	};
> +};
> diff --git a/arch/arm/mach-mx5/Makefile b/arch/arm/mach-mx5/Makefile
> index 383e7cd..71379f6 100644
> --- a/arch/arm/mach-mx5/Makefile
> +++ b/arch/arm/mach-mx5/Makefile
> @@ -22,3 +22,5 @@ obj-$(CONFIG_MX51_EFIKA_COMMON) += mx51_efika.o
>  obj-$(CONFIG_MACH_MX51_EFIKAMX) += board-mx51_efikamx.o
>  obj-$(CONFIG_MACH_MX51_EFIKASB) += board-mx51_efikasb.o
>  obj-$(CONFIG_MACH_MX50_RDP) += board-mx50_rdp.o
> +
> +obj-$(CONFIG_OF) += iomuxc-dt.o
> diff --git a/arch/arm/mach-mx5/iomuxc-dt.c b/arch/arm/mach-mx5/iomuxc-dt.c
> new file mode 100644
> index 0000000..2cfe6e7
> --- /dev/null
> +++ b/arch/arm/mach-mx5/iomuxc-dt.c
> @@ -0,0 +1,72 @@
> +/*
> + * Copyright 2011 Freescale Semiconductor, Inc. All Rights Reserved.
> + * Copyright 2011 Linaro Ltd.
> + *
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include <linux/of.h>
> +#include <asm/io.h>
> +
> +#define IOMUXC_CONFIG_SION	(1 << 4)
> +
> +void mxc_iomuxc_dt_init(const struct of_device_id *match)
> +{
> +	struct device_node *node = of_find_matching_node(NULL, match);
> +	struct device_node *child;
> +	void __iomem *base;
> +	u32 reg[2], select_input[2];
> +	u32 mux_mode, pad_ctl;
> +
> +	if (!node) {
> +		pr_warn("%s: no iomuxc node found\n", __func__);
> +		return;
> +	}
> +
> +	if (of_property_read_u32_array(node, "reg", reg, ARRAY_SIZE(reg))) {
> +		pr_warn("%s: property 'reg' not found\n", __func__);
> +		goto out;
> +	}
> +
> +	base = ioremap(reg[0], reg[1]);
> +	if (!base) {
> +		pr_warn("%s: ioremap failed\n", __func__);
> +		goto out;
> +	}
> +
> +	for_each_child_of_node(node, child) {
> +		/* get regsister offset of mux_ctl and pad_ctl */
> +		if (of_property_read_u32_array(child, "reg", reg,
> +					       ARRAY_SIZE(reg)))
> +			continue;
> +
> +		/* set register mux_ctl */
> +		if (of_property_read_u32(child, "fsl,iomuxc-mux-mode",
> +					 &mux_mode))
> +			continue;
> +		if (of_get_property(child, "fsl,iomuxc-sion", NULL))
> +			mux_mode |= IOMUXC_CONFIG_SION;
> +		writel(mux_mode, base + reg[0]);
> +
> +		/* set register pad_ctl */
> +		if (!of_property_read_u32(child, "fsl,iomuxc-pad-ctl",
> +					  &pad_ctl))
> +			writel(pad_ctl, base + reg[1]);
> +
> +		/* get offset/value pair and set select_input register */
> +		if (!of_property_read_u32_array(child,
> +				"fsl,iomuxc-select-input", select_input,
> +				ARRAY_SIZE(select_input)))
> +			writel(select_input[1], base + select_input[0]);
> +	}
> +
> +	iounmap(base);
> +
> +out:
> +	of_node_put(node);
> +}
> diff --git a/arch/arm/plat-mxc/include/mach/common.h b/arch/arm/plat-mxc/include/mach/common.h
> index 4e3d978..12b7499 100644
> --- a/arch/arm/plat-mxc/include/mach/common.h
> +++ b/arch/arm/plat-mxc/include/mach/common.h
> @@ -13,6 +13,7 @@
>  
>  struct platform_device;
>  struct clk;
> +struct of_device_id;
>  
>  extern void mx1_map_io(void);
>  extern void mx21_map_io(void);
> @@ -72,4 +73,6 @@ extern void mxc_arch_reset_init(void __iomem *);
>  extern void mx51_efikamx_reset(void);
>  extern int mx53_revision(void);
>  extern int mx53_display_revision(void);
> +
> +extern void mxc_iomuxc_dt_init(const struct of_device_id *match);
>  #endif
> -- 
> 1.7.4.1
> 
>

On Mon, Jul 25, 2011 at 02:46:30PM -0600, Grant Likely wrote:
> On Mon, Jul 25, 2011 at 11:07:46PM +0800, Shawn Guo wrote:
> > It adds function mxc_iomuxc_dt_init() to parse iomuxc pad configuration
> > from device tree.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Grant Likely <grant.likely@secretlab.ca>
> > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  .../devicetree/bindings/arm/fsl/iomuxc.txt         |   47 +++++++++++++
> >  arch/arm/mach-mx5/Makefile                         |    2 +
> >  arch/arm/mach-mx5/iomuxc-dt.c                      |   72 ++++++++++++++++++++
> >  arch/arm/plat-mxc/include/mach/common.h            |    3 +
> >  4 files changed, 124 insertions(+), 0 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/arm/fsl/iomuxc.txt
> >  create mode 100644 arch/arm/mach-mx5/iomuxc-dt.c
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/fsl/iomuxc.txt b/Documentation/devicetree/bindings/arm/fsl/iomuxc.txt
> > new file mode 100644
> > index 0000000..ae9292b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/fsl/iomuxc.txt
> > @@ -0,0 +1,47 @@
> > +* Freescale i.MX IOMUX Controller (IOMUXC)
> > +
> > +Required properties:
> > +- compatible : "fsl,<soc>-iomuxc";
> > +
> > +Sub-nodes present individual PAD configuration, and node name is the
> > +PAD name given by hardware document.
> > +
> > +Required properties:
> > +- reg : Should contain the offset of registers
> > +  IOMUXC_SW_MUX_CTL_PAD_<pad-name> and IOMUXC_SW_PAD_CTL_PAD_<pad-name>.
> > +- fsl,iomuxc-mux-mode : Should specify the MUX_MODE setting of register
> > +  IOMUXC_SW_MUX_CTL_PAD_<pad-name>.
> > +
> > +Optional properties:
> > +- fsl,iomuxc-sion : Indicates that bit SION of register
> > +  IOMUXC_SW_MUX_CTL_PAD_<pad-name> needs to be set for given MUX_MODE
> > +  setting of the PAD.
> > +- fsl,iomuxc-select-input : Specify the offset of register
> > +  IOMUXC_<...>_SELECT_INPUT and the value of bit-field DAISY for given
> > +  MUX_MODE setting of the PAD.
> 
> This could get really verbose in a really big hurry.  Fortunately the
> dtb format is sophisticated enough to only store each unique property
> name once, so the data shouldn't be huge, but it is still going to
> make for huge source files.  Can you think of a more concise

We have hundreds of real PADs on real SoC.  If we are trying to
describe real hard on dts its own, dts has to be big.

> representation?
> 
> The current linux code has each pin config simply a u64.  Now, an
> engineer certainly wouldn't be asked to write raw u64 values, but we
> could add some form of #define or macro syntax to dtc so that the
> symbolic names currently used in the board.c file would continue to
> work.

I was told that the binding should not be bonded to Linux
implementation.  Now I'm told to go the opposite direction? ;)

> Alternately, we could just make each pin configuration a string
> that can be parsed by the kernel at runtime.  A string would certainly
> be the most extensible, at the cost of some more init code in the
> kernel.
> 
> Something along the lines of:
> 	iomuxc@53fa8000 {
> 		compatible = "fsl,imx53-iomuxc"
> 		fsl,iomux-pads = "csi0-dat10,uart1-txd",
> 				"csi0-dat11,uart1-rxd",
> 				...;
> 	};
> 
Specifying string in dts, and then parsing the string, mapping it to
one of the huge MX53_PAD_ definition in kernel init code?

One thing I'm trying to do is to replace those huge iomux-mx*.h headers
with dts binding, and then get rid of those headers from Linux tree.
I guess moving those huge hardware details that kernel does not care
to dts is one goal of device tree support, no?

> Or if you prefer doing the parsing in dtc:
> 	iomuxc@53fa8000 {
> 		compatible = "fsl,imx53-iomuxc"
> 		fsl,iomux-pads = <MX53_PAD_CSI0_DAT10__UART1_TXD_MUX
> 				  MX53_PAD_CSI0_DAT11__UART2_RXD_MUX
> 				  ...
> 				  >;
> 	};
> 
> And we'd need a syntax for the macros.  However, right now
> iomux-mx53.h is *huge*, which I'm not hugely keen on also bringing
> into DTC...  but there might just not be any good way around that
> because pinmux configuration is by definition complexe.  It would
> certainly be better to be pre-processing that large data block in dtc
> rather than trying to encode the whole thing into kernel setup code.
> 
We end up with continuing maintaining those huge iomux-mx*.h headers
in kernel tree.

Getting kernel tree cleaned-up is one major reason why we are doing
device tree.  But I feel you are trying to make dts clean and leave
the 'crap' stay in kernel tree.

> A third option would be to create the pinmux table in C code with gcc and
> objdump, and include it into the dtb with a /bin-inc/ statement.  That
> option would work right now without having to change the format of the
> iomux #defines.
> 
This seems working for me.  But it still has the problem of being
bonded to Linux implementation, and it's not easy to change pad setting
for debugging.

Overall, I'm not convinced that any of these 3 options is better than
what I have right now :)

On Tue, Jul 26, 2011 at 10:43:55AM +0800, Shawn Guo wrote:
> On Mon, Jul 25, 2011 at 02:46:30PM -0600, Grant Likely wrote:
> > On Mon, Jul 25, 2011 at 11:07:46PM +0800, Shawn Guo wrote:
> > > It adds function mxc_iomuxc_dt_init() to parse iomuxc pad configuration
> > 
> Specifying string in dts, and then parsing the string, mapping it to
> one of the huge MX53_PAD_ definition in kernel init code?
> 
> One thing I'm trying to do is to replace those huge iomux-mx*.h headers
> with dts binding, and then get rid of those headers from Linux tree.

I don't like these files either, so getting rid of them looks promising.
OTOH don't forget that we need some of the pins during runtime for
gpio/function switching.

Sascha

On Mon, Jul 25, 2011 at 11:07:46PM +0800, Shawn Guo wrote:
> It adds function mxc_iomuxc_dt_init() to parse iomuxc pad configuration
> from device tree.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  .../devicetree/bindings/arm/fsl/iomuxc.txt         |   47 +++++++++++++
>  arch/arm/mach-mx5/Makefile                         |    2 +
>  arch/arm/mach-mx5/iomuxc-dt.c                      |   72 ++++++++++++++++++++
>  arch/arm/plat-mxc/include/mach/common.h            |    3 +
>  4 files changed, 124 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/fsl/iomuxc.txt
>  create mode 100644 arch/arm/mach-mx5/iomuxc-dt.c
> 
> diff --git a/Documentation/devicetree/bindings/arm/fsl/iomuxc.txt b/Documentation/devicetree/bindings/arm/fsl/iomuxc.txt
> new file mode 100644
> index 0000000..ae9292b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/fsl/iomuxc.txt
> @@ -0,0 +1,47 @@
> +* Freescale i.MX IOMUX Controller (IOMUXC)
> +
> +Required properties:
> +- compatible : "fsl,<soc>-iomuxc";
> +
> +Sub-nodes present individual PAD configuration, and node name is the
> +PAD name given by hardware document.
> +
> +Required properties:
> +- reg : Should contain the offset of registers
> +  IOMUXC_SW_MUX_CTL_PAD_<pad-name> and IOMUXC_SW_PAD_CTL_PAD_<pad-name>.
> +- fsl,iomuxc-mux-mode : Should specify the MUX_MODE setting of register
> +  IOMUXC_SW_MUX_CTL_PAD_<pad-name>.
> +
> +Optional properties:
> +- fsl,iomuxc-sion : Indicates that bit SION of register
> +  IOMUXC_SW_MUX_CTL_PAD_<pad-name> needs to be set for given MUX_MODE
> +  setting of the PAD.
> +- fsl,iomuxc-select-input : Specify the offset of register
> +  IOMUXC_<...>_SELECT_INPUT and the value of bit-field DAISY for given
> +  MUX_MODE setting of the PAD.
> +
> +Examples:
> +
> +iomuxc@53fa8000 {
> +	#address-cells = <2>;
> +	#size-cells = <0>;
> +	compatible = "fsl,imx53-iomuxc";
> +	reg = <0x53fa8000 0x4000>;
> +
> +	/*
> +	 * I2C2
> +	 */
> +	key-col3 { /* I2C2_SCL */
> +		reg = <0x3c 0x364>;
> +		fsl,iomuxc-mux-mode = <4>;
> +		fsl,iomuxc-sion;
> +		fsl,iomuxc-select-input = <0x81c 0x0>;
> +	};
> +
> +	key-row3 { /* I2C2_SDA */
> +		reg = <0x40 0x368>;
> +		fsl,iomuxc-mux-mode = <4>;
> +		fsl,iomuxc-sion;
> +		fsl,iomuxc-select-input = <0x820 0x0>;
> +	};
> +};

If we want to move the iomux setting to the device tree, wouldn't it be
necessary to find some format which can be shared across different
platforms? At least all i.MXs should be covered.

Sascha

On Mon, Jul 25, 2011 at 11:07:46PM +0800, Shawn Guo wrote:
> It adds function mxc_iomuxc_dt_init() to parse iomuxc pad configuration
> from device tree.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  .../devicetree/bindings/arm/fsl/iomuxc.txt         |   47 +++++++++++++
>  arch/arm/mach-mx5/Makefile                         |    2 +
>  arch/arm/mach-mx5/iomuxc-dt.c                      |   72 ++++++++++++++++++++
>  arch/arm/plat-mxc/include/mach/common.h            |    3 +
>  4 files changed, 124 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/fsl/iomuxc.txt
>  create mode 100644 arch/arm/mach-mx5/iomuxc-dt.c
> 
> + */
> +
> +#include <linux/of.h>
> +#include <asm/io.h>

linux/io.h

> +
> +#define IOMUXC_CONFIG_SION	(1 << 4)
> +
> +void mxc_iomuxc_dt_init(const struct of_device_id *match)
> +{
> +	struct device_node *node = of_find_matching_node(NULL, match);
> +	struct device_node *child;
> +	void __iomem *base;
> +	u32 reg[2], select_input[2];
> +	u32 mux_mode, pad_ctl;
> +
> +	if (!node) {
> +		pr_warn("%s: no iomuxc node found\n", __func__);
> +		return;
> +	}

Please remove this warning. Some boards may intentionally do the iomux
setup in the bootloader and skip the iomux setup nodes in the device
tree.

Sascha

On Tue, Jul 26, 2011 at 4:46 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Mon, Jul 25, 2011 at 11:07:46PM +0800, Shawn Guo wrote:
>> It adds function mxc_iomuxc_dt_init() to parse iomuxc pad configuration
>> from device tree.
>>
>> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
>> Cc: Grant Likely <grant.likely@secretlab.ca>
>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>> ---
>>  .../devicetree/bindings/arm/fsl/iomuxc.txt         |   47 +++++++++++++
>>  arch/arm/mach-mx5/Makefile                         |    2 +
>>  arch/arm/mach-mx5/iomuxc-dt.c                      |   72 ++++++++++++++++++++
>>  arch/arm/plat-mxc/include/mach/common.h            |    3 +
>>  4 files changed, 124 insertions(+), 0 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/arm/fsl/iomuxc.txt
>>  create mode 100644 arch/arm/mach-mx5/iomuxc-dt.c
>>
>> diff --git a/Documentation/devicetree/bindings/arm/fsl/iomuxc.txt b/Documentation/devicetree/bindings/arm/fsl/iomuxc.txt
>> new file mode 100644
>> index 0000000..ae9292b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/fsl/iomuxc.txt
>> @@ -0,0 +1,47 @@
>> +* Freescale i.MX IOMUX Controller (IOMUXC)
>> +
>> +Required properties:
>> +- compatible : "fsl,<soc>-iomuxc";
>> +
>> +Sub-nodes present individual PAD configuration, and node name is the
>> +PAD name given by hardware document.
>> +
>> +Required properties:
>> +- reg : Should contain the offset of registers
>> +  IOMUXC_SW_MUX_CTL_PAD_<pad-name> and IOMUXC_SW_PAD_CTL_PAD_<pad-name>.
>> +- fsl,iomuxc-mux-mode : Should specify the MUX_MODE setting of register
>> +  IOMUXC_SW_MUX_CTL_PAD_<pad-name>.
>> +
>> +Optional properties:
>> +- fsl,iomuxc-sion : Indicates that bit SION of register
>> +  IOMUXC_SW_MUX_CTL_PAD_<pad-name> needs to be set for given MUX_MODE
>> +  setting of the PAD.
>> +- fsl,iomuxc-select-input : Specify the offset of register
>> +  IOMUXC_<...>_SELECT_INPUT and the value of bit-field DAISY for given
>> +  MUX_MODE setting of the PAD.
>
> This could get really verbose in a really big hurry.  Fortunately the
> dtb format is sophisticated enough to only store each unique property
> name once, so the data shouldn't be huge, but it is still going to
> make for huge source files.  Can you think of a more concise
> representation?
>
> The current linux code has each pin config simply a u64.  Now, an
> engineer certainly wouldn't be asked to write raw u64 values, but we
> could add some form of #define or macro syntax to dtc so that the
> symbolic names currently used in the board.c file would continue to
> work.  Alternately, we could just make each pin configuration a string
> that can be parsed by the kernel at runtime.  A string would certainly
> be the most extensible, at the cost of some more init code in the
> kernel.
>
> Something along the lines of:
>        iomuxc@53fa8000 {
>                compatible = "fsl,imx53-iomuxc"
>                fsl,iomux-pads = "csi0-dat10,uart1-txd",
>                                "csi0-dat11,uart1-rxd",
>                                ...;
>        };

This format looks very promising (and apparently better than the one
below in my humble POV). However, I still have some concerns:

1. there has to be a table to interpret the pin name, e.g. "csi0-dat10"
to the register, and the function name "uart1-txd" to register bits. Yet
the this mapping itself is kind of description and is supposed to be
encoded by the DT data itself

2. how we gonna implement dynamic IOMUX change, there are callbacks
during run-time where pin IOMUX properties are changed. e.g. when SD
card slot is empty, the Card Detect pin could be configured to normal
GPIO and triggers an interrupt when a card presence is detected. And
one other example is some pins are multiplexed by two components
and they could be properly managed to make both components usable
at run-time.

>
> Or if you prefer doing the parsing in dtc:
>        iomuxc@53fa8000 {
>                compatible = "fsl,imx53-iomuxc"
>                fsl,iomux-pads = <MX53_PAD_CSI0_DAT10__UART1_TXD_MUX
>                                  MX53_PAD_CSI0_DAT11__UART2_RXD_MUX
>                                  ...
>                                  >;
>        };
>
> And we'd need a syntax for the macros.  However, right now
> iomux-mx53.h is *huge*, which I'm not hugely keen on also bringing
> into DTC...  but there might just not be any good way around that
> because pinmux configuration is by definition complexe.  It would
> certainly be better to be pre-processing that large data block in dtc
> rather than trying to encode the whole thing into kernel setup code.
>
> A third option would be to create the pinmux table in C code with gcc and
> objdump, and include it into the dtb with a /bin-inc/ statement.  That
> option would work right now without having to change the format of the
> iomux #defines.
>
> g.
>
>> +
>> +Examples:
>> +
>> +iomuxc@53fa8000 {
>> +     #address-cells = <2>;
>> +     #size-cells = <0>;
>> +     compatible = "fsl,imx53-iomuxc";
>> +     reg = <0x53fa8000 0x4000>;
>> +
>> +     /*
>> +      * I2C2
>> +      */
>> +     key-col3 { /* I2C2_SCL */
>> +             reg = <0x3c 0x364>;
>> +             fsl,iomuxc-mux-mode = <4>;
>> +             fsl,iomuxc-sion;
>> +             fsl,iomuxc-select-input = <0x81c 0x0>;
>> +     };
>> +
>> +     key-row3 { /* I2C2_SDA */
>> +             reg = <0x40 0x368>;
>> +             fsl,iomuxc-mux-mode = <4>;
>> +             fsl,iomuxc-sion;
>> +             fsl,iomuxc-select-input = <0x820 0x0>;
>> +     };
>> +};
>> diff --git a/arch/arm/mach-mx5/Makefile b/arch/arm/mach-mx5/Makefile
>> index 383e7cd..71379f6 100644
>> --- a/arch/arm/mach-mx5/Makefile
>> +++ b/arch/arm/mach-mx5/Makefile
>> @@ -22,3 +22,5 @@ obj-$(CONFIG_MX51_EFIKA_COMMON) += mx51_efika.o
>>  obj-$(CONFIG_MACH_MX51_EFIKAMX) += board-mx51_efikamx.o
>>  obj-$(CONFIG_MACH_MX51_EFIKASB) += board-mx51_efikasb.o
>>  obj-$(CONFIG_MACH_MX50_RDP) += board-mx50_rdp.o
>> +
>> +obj-$(CONFIG_OF) += iomuxc-dt.o
>> diff --git a/arch/arm/mach-mx5/iomuxc-dt.c b/arch/arm/mach-mx5/iomuxc-dt.c
>> new file mode 100644
>> index 0000000..2cfe6e7
>> --- /dev/null
>> +++ b/arch/arm/mach-mx5/iomuxc-dt.c
>> @@ -0,0 +1,72 @@
>> +/*
>> + * Copyright 2011 Freescale Semiconductor, Inc. All Rights Reserved.
>> + * Copyright 2011 Linaro Ltd.
>> + *
>> + * The code contained herein is licensed under the GNU General Public
>> + * License. You may obtain a copy of the GNU General Public License
>> + * Version 2 or later at the following locations:
>> + *
>> + * http://www.opensource.org/licenses/gpl-license.html
>> + * http://www.gnu.org/copyleft/gpl.html
>> + */
>> +
>> +#include <linux/of.h>
>> +#include <asm/io.h>
>> +
>> +#define IOMUXC_CONFIG_SION   (1 << 4)
>> +
>> +void mxc_iomuxc_dt_init(const struct of_device_id *match)
>> +{
>> +     struct device_node *node = of_find_matching_node(NULL, match);
>> +     struct device_node *child;
>> +     void __iomem *base;
>> +     u32 reg[2], select_input[2];
>> +     u32 mux_mode, pad_ctl;
>> +
>> +     if (!node) {
>> +             pr_warn("%s: no iomuxc node found\n", __func__);
>> +             return;
>> +     }
>> +
>> +     if (of_property_read_u32_array(node, "reg", reg, ARRAY_SIZE(reg))) {
>> +             pr_warn("%s: property 'reg' not found\n", __func__);
>> +             goto out;
>> +     }
>> +
>> +     base = ioremap(reg[0], reg[1]);
>> +     if (!base) {
>> +             pr_warn("%s: ioremap failed\n", __func__);
>> +             goto out;
>> +     }
>> +
>> +     for_each_child_of_node(node, child) {
>> +             /* get regsister offset of mux_ctl and pad_ctl */
>> +             if (of_property_read_u32_array(child, "reg", reg,
>> +                                            ARRAY_SIZE(reg)))
>> +                     continue;
>> +
>> +             /* set register mux_ctl */
>> +             if (of_property_read_u32(child, "fsl,iomuxc-mux-mode",
>> +                                      &mux_mode))
>> +                     continue;
>> +             if (of_get_property(child, "fsl,iomuxc-sion", NULL))
>> +                     mux_mode |= IOMUXC_CONFIG_SION;
>> +             writel(mux_mode, base + reg[0]);
>> +
>> +             /* set register pad_ctl */
>> +             if (!of_property_read_u32(child, "fsl,iomuxc-pad-ctl",
>> +                                       &pad_ctl))
>> +                     writel(pad_ctl, base + reg[1]);
>> +
>> +             /* get offset/value pair and set select_input register */
>> +             if (!of_property_read_u32_array(child,
>> +                             "fsl,iomuxc-select-input", select_input,
>> +                             ARRAY_SIZE(select_input)))
>> +                     writel(select_input[1], base + select_input[0]);
>> +     }
>> +
>> +     iounmap(base);
>> +
>> +out:
>> +     of_node_put(node);
>> +}
>> diff --git a/arch/arm/plat-mxc/include/mach/common.h b/arch/arm/plat-mxc/include/mach/common.h
>> index 4e3d978..12b7499 100644
>> --- a/arch/arm/plat-mxc/include/mach/common.h
>> +++ b/arch/arm/plat-mxc/include/mach/common.h
>> @@ -13,6 +13,7 @@
>>
>>  struct platform_device;
>>  struct clk;
>> +struct of_device_id;
>>
>>  extern void mx1_map_io(void);
>>  extern void mx21_map_io(void);
>> @@ -72,4 +73,6 @@ extern void mxc_arch_reset_init(void __iomem *);
>>  extern void mx51_efikamx_reset(void);
>>  extern int mx53_revision(void);
>>  extern int mx53_display_revision(void);
>> +
>> +extern void mxc_iomuxc_dt_init(const struct of_device_id *match);
>>  #endif
>> --
>> 1.7.4.1
>>
>>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

On Tue, Jul 26, 2011 at 08:29:32AM +0200, Sascha Hauer wrote:
> On Tue, Jul 26, 2011 at 10:43:55AM +0800, Shawn Guo wrote:
> > On Mon, Jul 25, 2011 at 02:46:30PM -0600, Grant Likely wrote:
> > > On Mon, Jul 25, 2011 at 11:07:46PM +0800, Shawn Guo wrote:
> > > > It adds function mxc_iomuxc_dt_init() to parse iomuxc pad configuration
> > > 
> > Specifying string in dts, and then parsing the string, mapping it to
> > one of the huge MX53_PAD_ definition in kernel init code?
> > 
> > One thing I'm trying to do is to replace those huge iomux-mx*.h headers
> > with dts binding, and then get rid of those headers from Linux tree.
> 
> I don't like these files either, so getting rid of them looks promising.
> OTOH don't forget that we need some of the pins during runtime for
> gpio/function switching.
> 
Do we have plan to use Linus.W's pinmux subsystem, which will perfectly
work for runtime configuration?

On Tue, Jul 26, 2011 at 08:31:44AM +0200, Sascha Hauer wrote:
> On Mon, Jul 25, 2011 at 11:07:46PM +0800, Shawn Guo wrote:
> > It adds function mxc_iomuxc_dt_init() to parse iomuxc pad configuration
> > from device tree.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Grant Likely <grant.likely@secretlab.ca>
> > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  .../devicetree/bindings/arm/fsl/iomuxc.txt         |   47 +++++++++++++
> >  arch/arm/mach-mx5/Makefile                         |    2 +
> >  arch/arm/mach-mx5/iomuxc-dt.c                      |   72 ++++++++++++++++++++
> >  arch/arm/plat-mxc/include/mach/common.h            |    3 +
> >  4 files changed, 124 insertions(+), 0 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/arm/fsl/iomuxc.txt
> >  create mode 100644 arch/arm/mach-mx5/iomuxc-dt.c
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/fsl/iomuxc.txt b/Documentation/devicetree/bindings/arm/fsl/iomuxc.txt
> > new file mode 100644
> > index 0000000..ae9292b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/fsl/iomuxc.txt
> > @@ -0,0 +1,47 @@
> > +* Freescale i.MX IOMUX Controller (IOMUXC)
> > +
> > +Required properties:
> > +- compatible : "fsl,<soc>-iomuxc";
> > +
> > +Sub-nodes present individual PAD configuration, and node name is the
> > +PAD name given by hardware document.
> > +
> > +Required properties:
> > +- reg : Should contain the offset of registers
> > +  IOMUXC_SW_MUX_CTL_PAD_<pad-name> and IOMUXC_SW_PAD_CTL_PAD_<pad-name>.
> > +- fsl,iomuxc-mux-mode : Should specify the MUX_MODE setting of register
> > +  IOMUXC_SW_MUX_CTL_PAD_<pad-name>.
> > +
> > +Optional properties:
> > +- fsl,iomuxc-sion : Indicates that bit SION of register
> > +  IOMUXC_SW_MUX_CTL_PAD_<pad-name> needs to be set for given MUX_MODE
> > +  setting of the PAD.
> > +- fsl,iomuxc-select-input : Specify the offset of register
> > +  IOMUXC_<...>_SELECT_INPUT and the value of bit-field DAISY for given
> > +  MUX_MODE setting of the PAD.
> > +
> > +Examples:
> > +
> > +iomuxc@53fa8000 {
> > +	#address-cells = <2>;
> > +	#size-cells = <0>;
> > +	compatible = "fsl,imx53-iomuxc";
> > +	reg = <0x53fa8000 0x4000>;
> > +
> > +	/*
> > +	 * I2C2
> > +	 */
> > +	key-col3 { /* I2C2_SCL */
> > +		reg = <0x3c 0x364>;
> > +		fsl,iomuxc-mux-mode = <4>;
> > +		fsl,iomuxc-sion;
> > +		fsl,iomuxc-select-input = <0x81c 0x0>;
> > +	};
> > +
> > +	key-row3 { /* I2C2_SDA */
> > +		reg = <0x40 0x368>;
> > +		fsl,iomuxc-mux-mode = <4>;
> > +		fsl,iomuxc-sion;
> > +		fsl,iomuxc-select-input = <0x820 0x0>;
> > +	};
> > +};
> 
> If we want to move the iomux setting to the device tree, wouldn't it be
> necessary to find some format which can be shared across different
> platforms? At least all i.MXs should be covered.
> 
Right now, I really just focus on iomux-v3.  After people buy in this
approach, we can start see how iomux-v1 will fit in.  It's not so
urgent before we actually start converting those old i.mx platforms
(iomux-v1 users) to device tree.

On Tue, Jul 26, 2011 at 08:39:14AM +0200, Sascha Hauer wrote:
> On Mon, Jul 25, 2011 at 11:07:46PM +0800, Shawn Guo wrote:
> > It adds function mxc_iomuxc_dt_init() to parse iomuxc pad configuration
> > from device tree.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Grant Likely <grant.likely@secretlab.ca>
> > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  .../devicetree/bindings/arm/fsl/iomuxc.txt         |   47 +++++++++++++
> >  arch/arm/mach-mx5/Makefile                         |    2 +
> >  arch/arm/mach-mx5/iomuxc-dt.c                      |   72 ++++++++++++++++++++
> >  arch/arm/plat-mxc/include/mach/common.h            |    3 +
> >  4 files changed, 124 insertions(+), 0 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/arm/fsl/iomuxc.txt
> >  create mode 100644 arch/arm/mach-mx5/iomuxc-dt.c
> > 
> > + */
> > +
> > +#include <linux/of.h>
> > +#include <asm/io.h>
> 
> linux/io.h
> 
Ok.

> > +
> > +#define IOMUXC_CONFIG_SION	(1 << 4)
> > +
> > +void mxc_iomuxc_dt_init(const struct of_device_id *match)
> > +{
> > +	struct device_node *node = of_find_matching_node(NULL, match);
> > +	struct device_node *child;
> > +	void __iomem *base;
> > +	u32 reg[2], select_input[2];
> > +	u32 mux_mode, pad_ctl;
> > +
> > +	if (!node) {
> > +		pr_warn("%s: no iomuxc node found\n", __func__);
> > +		return;
> > +	}
> 
> Please remove this warning. Some boards may intentionally do the iomux
> setup in the bootloader and skip the iomux setup nodes in the device
> tree.
Ok.

Thanks for review, Sascha.

On Tue, Jul 26, 2011 at 10:43:55AM +0800, Shawn Guo wrote:
> On Mon, Jul 25, 2011 at 02:46:30PM -0600, Grant Likely wrote:
> > On Mon, Jul 25, 2011 at 11:07:46PM +0800, Shawn Guo wrote:
> > The current linux code has each pin config simply a u64.  Now, an
> > engineer certainly wouldn't be asked to write raw u64 values, but we
> > could add some form of #define or macro syntax to dtc so that the
> > symbolic names currently used in the board.c file would continue to
> > work.
> 
> I was told that the binding should not be bonded to Linux
> implementation.  Now I'm told to go the opposite direction? ;)

Nope; it's perfectly valid to use the current Linux implementation for
inspiration, since a real implementation can be proven to actually
/work/.  :-)  However, the binding must be documented from the
perspective of how the hardware works, not from the perspective of
what Linux currently wants.

g.

Hi Grant, Shawn,



On Mon, Jul 25, 2011 at 3:46 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> This could get really verbose in a really big hurry.  Fortunately the
> dtb format is sophisticated enough to only store each unique property
> name once, so the data shouldn't be huge, but it is still going to
> make for huge source files.  Can you think of a more concise
> representation?

Yes: no representation at all. The correct place for IOMUX setup being
done is *inside the boot firmware as soon as physically possible* and
not seconds into boot after U-Boot has made a console, done a boot
timeout, loaded scripts, kernels and ramdisks from media and then
uncompressed and entered a Linux kernel.

The only beneficial place for setting up IOMUX inside Linux is when
you really need to modify the behavior of an SoC unit that is acting
weird, the only real example I can think of being the situation with
eCSPI errata on the MX51 (ENGcm09397 whereby slave select stays
asserted at  the end of a transfer when SSB_POL = 1). That in itself
was only needed for Freescale's solution (which was kind of funky and
hoped the chip would do the right thing at the start and give control
back at the end). The current mainline solution is to handle slave
select as GPIO and twiddle it as needed for correct operation. I can't
think of - or find - a single example after that where IOMUX settings
are configured at runtime, and the method currently used in
Freescale's BSP and in mainline is just fundamentally wrong.

If you think the solution is not so great due to the complexity of
describing the IOMUX settings, including the pad definitions as binary
blobs or so such that Linux can read them out, please feel free to
take the hint and go nag the U-Boot developers at Linaro to go put
this in the right place - in U-Boot. The device tree is absolutely not
the place to define pin multiplexing settings for later parsing and
configuration by the Kernel. They should have been set up correctly
already, and they should not be being *changed* based on an arbitrary
configuration file. Consider that the i2c pin definitions you used in
your example *absolutely will not change* for the lifetime of the
board, and in most cases, will have been set up by U-Boot anyway.
There is no point telling Linux to copy in identical settings again.
What's missing from U-Boot and set up by Linux, should be moved out of
Linux back into U-Boot.

That said, as a reference to be able to view the settings of the IOMUX
pads, it is useful. In theory, once you set up your controller in
U-Boot, you may want to use sysfs or so to read out the configuration
of that pin in userspace to confirm the board settings, or other debug
processes. By naming it and providing the offsets and mode definitions
somewhere other than a reference manual you are enabling this to get
done.. but please, please, please don't make the mistake of
considering that just because every board right now does this at Linux
init, that it therefore it must be the correct solution and maintained
:(

On Thu, Aug 04, 2011 at 06:07:15PM -0500, Matt Sealey wrote:
> Hi Grant, Shawn,
> 
> On Mon, Jul 25, 2011 at 3:46 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> > This could get really verbose in a really big hurry.  Fortunately the
> > dtb format is sophisticated enough to only store each unique property
> > name once, so the data shouldn't be huge, but it is still going to
> > make for huge source files.  Can you think of a more concise
> > representation?
> 
> Yes: no representation at all. The correct place for IOMUX setup being
> done is *inside the boot firmware as soon as physically possible* and
> not seconds into boot after U-Boot has made a console, done a boot
> timeout, loaded scripts, kernels and ramdisks from media and then
> uncompressed and entered a Linux kernel.

This is true in situations where we have control over the bootloader,
but that isn't always the case.

David

On Fri, Aug 5, 2011 at 2:07 AM, David Brown <davidb@codeaurora.org> wrote:
> On Thu, Aug 04, 2011 at 06:07:15PM -0500, Matt Sealey wrote:
>> Hi Grant, Shawn,
>>
>> On Mon, Jul 25, 2011 at 3:46 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
>> > This could get really verbose in a really big hurry.  Fortunately the
>> > dtb format is sophisticated enough to only store each unique property
>> > name once, so the data shouldn't be huge, but it is still going to
>> > make for huge source files.  Can you think of a more concise
>> > representation?
>>
>> Yes: no representation at all. The correct place for IOMUX setup being
>> done is *inside the boot firmware as soon as physically possible* and
>> not seconds into boot after U-Boot has made a console, done a boot
>> timeout, loaded scripts, kernels and ramdisks from media and then
>> uncompressed and entered a Linux kernel.
>
> This is true in situations where we have control over the bootloader,
> but that isn't always the case.

Indeed it is not, but then it is "their" fault the board won't boot
Linux, and not yours, right? :)

I think it is a given that when designing hardware (and we do that)
and proprietary firmware that the Linux kernel guys can't "control",
you have to keep up with the changes when reasonable. While sometimes
that is very difficult, this is not one of those "sometimes" -
providing a setup that can boot Linux implies that you configured the
chip correctly such that Linux is supplementing that configuration,
not reimplementing it from scratch.

Back in the days when device trees were dynamically generated for
hardware based on their in-firmware state and then passed to Linux,
and not just bolted into the kernel binary, if you left a device in
the tree that didn't have it's pins muxed, you could consider that a
fatal firmware flaw, and the response we usually got from Linux
developers was "well, fix your firmware". It is far more prudent to
treat device trees as a state description of the hardware Linux was
booted on, and not a configuration file for drivers.. we've already
been through this a thousand times on PowerPC.

I know that FDT doesn't have the capability of a real OpenFirmware or
even an ACPI device tree, but the core concept that was most beautiful
and the most engaging that made us all want to have device trees and
real dynamic firmware was the fact that you could, if described in the
tree, bind a driver to it. This implies giving it the address of the
device in the memory map, information that describes it's behavior. At
the lowest level it maps almost 1:1 with the Linux "resource" concept
(i.e. memory, dma channels, buses, interrupts) and allows probing
based on human-readable strings and identifiers, so that differences
in PCB design and pin routing of different products can be reported to
Linux. And that's the thing here; reported to Linux, not "I left it
completely unconfigured, but here are the instructions on how to make
it work". At the point you end up rewriting a device tree to tell
Linux to configure this, and managed to make your firmware load it,
you may as well have modified the firmware to do the work and give
Linux an easy ride.

Yes, it puts the onus of the work on the firmware guys, but they're
the ones writing the device trees for their hardware anyway.

I understand that if Device Vendor A decides they want to support
Windows 8 and leave the system configured in a state such that
basically only Windows 8 works on it because the Right Thing To Do For
Linux(tm) is not the same as what it actually does to get up to the
point it brings up Windows 8, Linux is not going to work unless some
effort is expended to basically get the "right thing" done. This is
not necessarily a good fit for a device tree.. this is probably where
effort needs to be expended in putting a custom U-Boot on it so you
can do the right thing. Or writing a second-stage bootloader (like
Luigi for the Cr-48) which does the right thing.. and then providing
the device tree, which is the defacto standard here, based on the
right things it did.

On 08/05/2011 01:36 PM, Matt Sealey wrote:
> On Fri, Aug 5, 2011 at 2:07 AM, David Brown <davidb@codeaurora.org> wrote:
>> On Thu, Aug 04, 2011 at 06:07:15PM -0500, Matt Sealey wrote:
>>> Hi Grant, Shawn,
>>>
>>> On Mon, Jul 25, 2011 at 3:46 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
>>>> This could get really verbose in a really big hurry.  Fortunately the
>>>> dtb format is sophisticated enough to only store each unique property
>>>> name once, so the data shouldn't be huge, but it is still going to
>>>> make for huge source files.  Can you think of a more concise
>>>> representation?
>>>
>>> Yes: no representation at all. The correct place for IOMUX setup being
>>> done is *inside the boot firmware as soon as physically possible* and
>>> not seconds into boot after U-Boot has made a console, done a boot
>>> timeout, loaded scripts, kernels and ramdisks from media and then
>>> uncompressed and entered a Linux kernel.
>>
>> This is true in situations where we have control over the bootloader,
>> but that isn't always the case.
> 
> Indeed it is not, but then it is "their" fault the board won't boot
> Linux, and not yours, right? :)
> 
> I think it is a given that when designing hardware (and we do that)
> and proprietary firmware that the Linux kernel guys can't "control",
> you have to keep up with the changes when reasonable. While sometimes
> that is very difficult, this is not one of those "sometimes" -
> providing a setup that can boot Linux implies that you configured the
> chip correctly such that Linux is supplementing that configuration,
> not reimplementing it from scratch.

In the absence of a time machine, situations where one might not want to
upgrade firmware are not limited to proprietary firmware.  The means to
recover from a bricked board are not always available and convenient.

This is why we did pin setup in Linux for 8xx/82xx, and why we did cuImage.

If you haven't yet shipped the boards with bad firmware to an extent
that requires compatibility, that's a different situation of course.

> Yes, it puts the onus of the work on the firmware guys, but they're
> the ones writing the device trees for their hardware anyway.

Sometimes.

-Scott

On Fri, Aug 05, 2011 at 03:26:29PM -0500, Scott Wood wrote:

> > Yes, it puts the onus of the work on the firmware guys, but they're
> > the ones writing the device trees for their hardware anyway.
> 
> Sometimes.

I'm not even sure that is going to be common.  At least our setup is
going to have the device tree living in flash somewhere.  The
bootloader only knows enough about the FDT to insert arguments and
memory information into it.  They certainly aren't the appropriate
team to be defining the device tree.

David

On Fri, Aug 5, 2011 at 3:36 PM, David Brown <davidb@codeaurora.org> wrote:
> On Fri, Aug 05, 2011 at 03:26:29PM -0500, Scott Wood wrote:
>
>> > Yes, it puts the onus of the work on the firmware guys, but they're
>> > the ones writing the device trees for their hardware anyway.
>>
>> Sometimes.
>
> I'm not even sure that is going to be common.  At least our setup is
> going to have the device tree living in flash somewhere.  The
> bootloader only knows enough about the FDT to insert arguments and
> memory information into it.  They certainly aren't the appropriate
> team to be defining the device tree.
>
> David

In any company through some kind of collaborative process of firmware
and Linux development, someone sits down and defines this device tree.
They will have an intimate knowledge of the hardware.. if you have to
flash the device tree to NOR or NAND or EEPROM or something anyway,
then flashing a new firmware binary at the same time is not really any
more complex.

At some point someone will have to write the information down to
configure each of the pins the way you want it, either in tree form or
in procedures in the firmware code itself. You have to figure out who
that is yourself: but putting it in tree form just shifts the blame
from outside Linux in the firmware, to outside Linux in the tree.
Someone is going to have to make the decision which pins they set up
in firmware (required for boot, or just good policy to make sure SoC
inputs are hooked to device outputs, and device inputs hooked SoC
outputs..) and which are put in the tree. USB might not be required
for boot, so it might not be touched in firmware, and left for Linux..
but the NAND controller for example, it makes no sense to configure it
in firmware to read Linux for boot, and then list it in the tree for
later reconfiguration. So some things are going to be in one list and
not in the other.

So why not take the complexity and choice out of it, and do it in the
firmware,, one list, all configured at boot time, before Linux is even
in the picture, and make sure this is a requirement for booting Linux
that these pins are set up already?

On 08/05/2011 04:29 PM, Matt Sealey wrote:
> On Fri, Aug 5, 2011 at 3:36 PM, David Brown <davidb@codeaurora.org> wrote:
>> On Fri, Aug 05, 2011 at 03:26:29PM -0500, Scott Wood wrote:
>>
>>>> Yes, it puts the onus of the work on the firmware guys, but they're
>>>> the ones writing the device trees for their hardware anyway.
>>>
>>> Sometimes.
>>
>> I'm not even sure that is going to be common.  At least our setup is
>> going to have the device tree living in flash somewhere.  The
>> bootloader only knows enough about the FDT to insert arguments and
>> memory information into it.  They certainly aren't the appropriate
>> team to be defining the device tree.
>>
>> David
> 
> In any company through some kind of collaborative process of firmware
> and Linux development, someone sits down and defines this device tree.
> They will have an intimate knowledge of the hardware.. 

That doesn't mean they have intimate knowledge of what will be accepted
upstream, or that they involve upstream early enough.  Even within the
company, those who work with upstream may have little influence over
what gets flashed on the boards during manufacturing, or when hardware
details can be disclosed in the form of submitting patches.  Or they
might have just been rushed by schedule to get some firmware done that
can load a kernel so boards can be shipped, and enabling certain
peripherals gets dealt with later.

> if you have to
> flash the device tree to NOR or NAND or EEPROM or something anyway,
> then flashing a new firmware binary at the same time is not really any
> more complex.

If the firmware doesn't depend on the device tree to boot, you don't
need to worry about bricking the board by reflashing the device tree.

> So why not take the complexity and choice out of it, and do it in the
> firmware,, one list, all configured at boot time, before Linux is even
> in the picture, and make sure this is a requirement for booting Linux
> that these pins are set up already?

I fully agree that that's the nicer approach -- if you're not yet in a
position where you need to support existing firmware.  Is that the case
here?

-Scott

On Thu, Aug 04, 2011 at 06:07:15PM -0500, Matt Sealey wrote:
> Hi Grant, Shawn,
> 
> 
> 
> On Mon, Jul 25, 2011 at 3:46 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> > This could get really verbose in a really big hurry.  Fortunately the
> > dtb format is sophisticated enough to only store each unique property
> > name once, so the data shouldn't be huge, but it is still going to
> > make for huge source files.  Can you think of a more concise
> > representation?
> 
> Yes: no representation at all. The correct place for IOMUX setup being
> done is *inside the boot firmware as soon as physically possible* and
> not seconds into boot after U-Boot has made a console, done a boot
> timeout, loaded scripts, kernels and ramdisks from media and then
> uncompressed and entered a Linux kernel.

We've had this argument before.  There are many use cases where the
firmware simply cannot be relied upon to do the right thing.

g.

On 8/5/2011 12:58 PM, Grant Likely wrote:
> On Thu, Aug 04, 2011 at 06:07:15PM -0500, Matt Sealey wrote:
>> Hi Grant, Shawn,
>>
>>
>>
>> On Mon, Jul 25, 2011 at 3:46 PM, Grant Likely<grant.likely@secretlab.ca>  wrote:
>>> This could get really verbose in a really big hurry.  Fortunately the
>>> dtb format is sophisticated enough to only store each unique property
>>> name once, so the data shouldn't be huge, but it is still going to
>>> make for huge source files.  Can you think of a more concise
>>> representation?
>>
>> Yes: no representation at all. The correct place for IOMUX setup being
>> done is *inside the boot firmware as soon as physically possible* and
>> not seconds into boot after U-Boot has made a console, done a boot
>> timeout, loaded scripts, kernels and ramdisks from media and then
>> uncompressed and entered a Linux kernel.
>
> We've had this argument before.  There are many use cases where the
> firmware simply cannot be relied upon to do the right thing.

I am a firmware engineer who tries very hard to deliver quality firmware 
that does the right thing.  It often seems to me that the Linux kernel 
community's general distrust of firmware is a self-fulfilling prophecy. 
  People often "live down to expectations"; if Linux expects the 
firmware to be worthless, worthless is often what you get.

In the PC BIOS world, the quality of BIOSes went up dramatically when 
Microsoft began insisting that the BIOS must meet stringent standards, 
enforced via an exhaustive suite of compatibility tests.

>
> g.
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>
>

On Fri, Aug 05, 2011 at 01:31:10PM -1000, Mitch Bradley wrote:
> On 8/5/2011 12:58 PM, Grant Likely wrote:

> >We've had this argument before.  There are many use cases where the
> >firmware simply cannot be relied upon to do the right thing.

> I am a firmware engineer who tries very hard to deliver quality
> firmware that does the right thing.  It often seems to me that the
> Linux kernel community's general distrust of firmware is a
> self-fulfilling prophecy.  People often "live down to expectations";
> if Linux expects the firmware to be worthless, worthless is often
> what you get.

> In the PC BIOS world, the quality of BIOSes went up dramatically
> when Microsoft began insisting that the BIOS must meet stringent
> standards, enforced via an exhaustive suite of compatibility tests.

It's not just quality concerns, although obviously that is a serious
issue, it's also a totally different way of designing and building
systems.  A bootloader update will tend to brick the board if it goes
wrong so you want to minimize the amount of code in it, and you're going
to be doing updates to things like pin configuration throughout the
development cycle which often need to be done in lock step with the
drivers using them.  Even if the bootloader works perfectly and is of
excellent quality it's just more effort and risk to put functionality in
there.

On Fri, Aug 05, 2011 at 01:36:56PM -0500, Matt Sealey wrote:
> On Fri, Aug 5, 2011 at 2:07 AM, David Brown <davidb@codeaurora.org> wrote:
> > On Thu, Aug 04, 2011 at 06:07:15PM -0500, Matt Sealey wrote:
> >> Hi Grant, Shawn,
> >>
> >> On Mon, Jul 25, 2011 at 3:46 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> >> > This could get really verbose in a really big hurry.  Fortunately the
> >> > dtb format is sophisticated enough to only store each unique property
> >> > name once, so the data shouldn't be huge, but it is still going to
> >> > make for huge source files.  Can you think of a more concise
> >> > representation?
> >>
> >> Yes: no representation at all. The correct place for IOMUX setup being
> >> done is *inside the boot firmware as soon as physically possible* and
> >> not seconds into boot after U-Boot has made a console, done a boot
> >> timeout, loaded scripts, kernels and ramdisks from media and then
> >> uncompressed and entered a Linux kernel.
> >
> > This is true in situations where we have control over the bootloader,
> > but that isn't always the case.
> 
> Indeed it is not, but then it is "their" fault the board won't boot
> Linux, and not yours, right? :)
> 
> I think it is a given that when designing hardware (and we do that)
> and proprietary firmware that the Linux kernel guys can't "control",
> you have to keep up with the changes when reasonable. While sometimes
> that is very difficult, this is not one of those "sometimes" -
> providing a setup that can boot Linux implies that you configured the
> chip correctly such that Linux is supplementing that configuration,
> not reimplementing it from scratch.

This whole argument is ridiculous.  Assume for a moment that I agree
and that the firmware (in the embedded context) can be relied upon to
leave the hardware in a sane state.  Even then, an initialization
sequence binding is completely appropriate.  Hardware not required to
boot doesn't need to be initialized, and there are some real reasons
(power consumption for instance) for leaving some pin io left
unconfigured.

Besides, trusting a pin mux init sequence passed in the device tree
inherently means you're trusting firmware has passed accurate data
*and* that the initial state is also sane.

g.

On Thu, Aug 04, 2011 at 06:07:15PM -0500, Matt Sealey wrote:
> 
> If you think the solution is not so great due to the complexity of
> describing the IOMUX settings, including the pad definitions as binary
> blobs or so such that Linux can read them out, please feel free to
> take the hint and go nag the U-Boot developers at Linaro to go put
> this in the right place - in U-Boot. The device tree is absolutely not
> the place to define pin multiplexing settings for later parsing and
> configuration by the Kernel. They should have been set up correctly
> already, and they should not be being *changed* based on an arbitrary
> configuration file. Consider that the i2c pin definitions you used in
> your example *absolutely will not change* for the lifetime of the
> board, and in most cases, will have been set up by U-Boot anyway.
> There is no point telling Linux to copy in identical settings again.
> What's missing from U-Boot and set up by Linux, should be moved out of
> Linux back into U-Boot.

System on module vendors have quite a different point of view here.
On these systems the functionality of a pin differs depending on the
baseboard they are attached to. Still you want to be able to use the
same bootloader for all usecases. Putting the pinmux into the bootloader
would mean that each user of these systems has to flash and maintain
a custom bootloader.

Sascha

On Fri, Aug 05, 2011 at 01:36:56PM -0500, Matt Sealey wrote:
> On Fri, Aug 5, 2011 at 2:07 AM, David Brown <davidb@codeaurora.org> wrote:
> > On Thu, Aug 04, 2011 at 06:07:15PM -0500, Matt Sealey wrote:
> >> Hi Grant, Shawn,
> >>
> >> On Mon, Jul 25, 2011 at 3:46 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> >> > This could get really verbose in a really big hurry.  Fortunately the
> >> > dtb format is sophisticated enough to only store each unique property
> >> > name once, so the data shouldn't be huge, but it is still going to
> >> > make for huge source files.  Can you think of a more concise
> >> > representation?
> >>
> >> Yes: no representation at all. The correct place for IOMUX setup being
> >> done is *inside the boot firmware as soon as physically possible* and
> >> not seconds into boot after U-Boot has made a console, done a boot
> >> timeout, loaded scripts, kernels and ramdisks from media and then
> >> uncompressed and entered a Linux kernel.
> >
> > This is true in situations where we have control over the bootloader,
> > but that isn't always the case.
> 
> Indeed it is not, but then it is "their" fault the board won't boot
> Linux, and not yours, right? :)

Not normally.  What happens is that you're provided with the boot loader.
You're told to get Linux working on the board.  Anything which the boot
loader doesn't do ends up being hacked around in the kernel.

Don't say that isn't what happens, because that comment is made with a
substantial body experience in dealing with patches from multiple
different sources containing crap to work around boot loader bugs.  Most
boot loader bugs go _unfixed_ on hardware even after being reported.

So please, don't tell us that boot loaders should do things right.  We
know, and we know (again from a vast body of experience) that relying on
boot loaders is plain idiotic and totally unworkable.