| Message ID | 1311606467-28985-2-git-send-email-shawn.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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>; + }; +}; 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
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