[PATCH V2 0/6] ARM: imx6q{dl}: add the WEIM driver

[PATCH V2 0/6] ARM: imx6q{dl}: add the WEIM driver

[Huang Shijie]

This patch set adds a new driver for WEIM in the imx6q{dl}-sabreauto boards.

The WEIM(Wireless External Interface Module) works like a bus.
You can attach many different devices on it, such as NOR, onenand.

In the case of i.MX6q-sabreauto, only the NOR is connected to WEIM.

v1 --> v2:
	[0] remove used PADs.
	[1] fix the wrong pad values, (thanks Alison Chaiken)
	[2] remove the weim-cs-index.
	[3] add "fsl" for the weim-cs-timing.
	[4] remove the partitions info in the document.
	[5] rewrite the drivers(follow Sascha and Shawn's comments)

Huang Shijie (6):
  drivers: bus: add a new driver for WEIM
  ARM: dts: imx6q{dl}: fix the pin conflict between SPI and WEIM
  ARM: dts: imx6qdl: add more information for WEIM
  ARM: dts: imx6q: add pinctrl for WEIM NOR
  ARM: dts: imx6dl: add a pinctrl for WEIM NOR
  ARM: dts: imx6qdl: enable the WEIM NOR

 Documentation/devicetree/bindings/bus/imx-weim.txt |   50 +++++++
 arch/arm/boot/dts/imx6dl-sabreauto.dts             |    9 +-
 arch/arm/boot/dts/imx6dl.dtsi                      |   55 ++++++++
 arch/arm/boot/dts/imx6q-sabreauto.dts              |    9 +-
 arch/arm/boot/dts/imx6q.dtsi                       |   56 ++++++++
 arch/arm/boot/dts/imx6qdl-sabreauto.dtsi           |   22 +++-
 arch/arm/boot/dts/imx6qdl.dtsi                     |    4 +-
 drivers/bus/Kconfig                                |    9 ++
 drivers/bus/Makefile                               |    1 +
 drivers/bus/imx-weim.c                             |  146 ++++++++++++++++++++
 10 files changed, 357 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/bus/imx-weim.txt
 create mode 100644 drivers/bus/imx-weim.c

[PATCH V2 1/6] drivers: bus: add a new driver for WEIM

[Huang Shijie]

The WEIM(Wireless External Interface Module) works like a bus.
You can attach many different devices on it, such as NOR, onenand.

In the case of i.MX6q-sabreauto, the NOR is connected to WEIM.

This patch also adds the devicetree binding document.
The driver only works when the devicetree is enabled.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 Documentation/devicetree/bindings/bus/imx-weim.txt |   50 +++++++
 drivers/bus/Kconfig                                |    9 ++
 drivers/bus/Makefile                               |    1 +
 drivers/bus/imx-weim.c                             |  146 ++++++++++++++++++++
 4 files changed, 206 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/bus/imx-weim.txt
 create mode 100644 drivers/bus/imx-weim.c

diff --git a/Documentation/devicetree/bindings/bus/imx-weim.txt b/Documentation/devicetree/bindings/bus/imx-weim.txt
new file mode 100644
index 0000000..3db588e
--- /dev/null
+++ b/Documentation/devicetree/bindings/bus/imx-weim.txt
@@ -0,0 +1,50 @@
+Device tree bindings for i.MX Wireless External Interface Module (WEIM)
+
+The term "wireless" does not imply that the WEIM is literally an interface
+without wires. It simply means that this module was originally designed for
+wireless and mobile applications that use low-power technology.
+
+The actual devices are instantiated from the child nodes of a WEIM node.
+
+Required properties:
+
+ - compatible:		Should be set to "fsl,imx6q-weim"
+ - reg:			A resource specifier for the register space
+			(see the example below)
+ - interrupts:		the interrupt number, see the example below.
+ - clocks:		the clock, see the example below.
+ - #address-cells:	Must be set to 2 to allow memory address translation
+ - #size-cells:		Must be set to 1 to allow CS address passing
+ - ranges:		Must be set up to reflect the memory layout with four
+			integer values for each chip-select line in use:
+
+			   <cs-number> 0 <physical address of mapping> <size>
+
+Timing property for child nodes. It is mandatory, not optional.
+
+ - fsl,weim-cs-timing:	The timing array, contains 6 timing values for the
+			child node. We can get the CS index from the child
+			node's "reg" property.
+
+Example for an imx6q-sabreauto board, the NOR flash connected to the WEIM:
+
+	weim: weim at 021b8000 {
+		compatible = "fsl,imx6q-weim";
+		reg = <0x021b8000 0x4000>;
+		interrupts = <0 14 0x04>;
+		clocks = <&clks 196>;
+		#address-cells = <2>;
+		#size-cells = <1>;
+		ranges = <0 0 0x08000000 0x08000000>;
+
+		nor at 0,0 {
+			compatible = "cfi-flash";
+			reg = <0 0 0x02000000>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			bank-width = <2>;
+
+			fsl,weim-cs-timing = <0x00620081 0x00000001 0x1C022000
+					0x0000C000 0x1404a38e 0x00000000>;
+		};
+	};
diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index b05ecab..488702b 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -4,6 +4,15 @@
 
 menu "Bus devices"
 
+config IMX_WEIM
+	tristate "Freescale EIM DRIVER"
+	depends on ARCH_MXC
+	help
+	  Driver for i.MX6 WEIM controller.
+	  The WEIM(Wireless External Interface Module) works like a bus.
+	  You can attach many different devices on it, such as NOR, onenand.
+	  But now, we only support the Parallel NOR.
+
 config MVEBU_MBUS
 	bool
 	depends on PLAT_ORION
diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
index 3c7b53c..436bbcc 100644
--- a/drivers/bus/Makefile
+++ b/drivers/bus/Makefile
@@ -2,6 +2,7 @@
 # Makefile for the bus drivers.
 #
 
+obj-$(CONFIG_IMX_WEIM)	+= imx-weim.o
 obj-$(CONFIG_MVEBU_MBUS) += mvebu-mbus.o
 obj-$(CONFIG_OMAP_OCP2SCP)	+= omap-ocp2scp.o
 
diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
new file mode 100644
index 0000000..b932add
--- /dev/null
+++ b/drivers/bus/imx-weim.c
@@ -0,0 +1,146 @@
+/*
+ * EIM driver for Freescale's i.MX chips
+ *
+ * Copyright (C) 2013 Freescale Semiconductor, Inc.
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+#include <linux/module.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/of_device.h>
+
+struct imx_weim {
+	void __iomem *base;
+	struct clk *clk;
+};
+
+static const struct of_device_id weim_id_table[] = {
+	{ .compatible = "fsl,imx6q-weim", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, weim_id_table);
+
+#define CS_TIMING_LEN 6
+#define CS_REG_RANGE  0x18
+
+/* Parse and set the timing for this device. */
+static int
+weim_timing_setup(struct platform_device *pdev, struct device_node *np)
+{
+	struct imx_weim *weim = platform_get_drvdata(pdev);
+	u32 value[CS_TIMING_LEN];
+	u32 cs_idx;
+	int ret;
+	int i;
+
+	/* get the CS index from this child node's "reg" property. */
+	ret = of_property_read_u32(np, "reg", &cs_idx);
+	if (ret)
+		return ret;
+
+	/* The weim has four chip selects. */
+	if (cs_idx > 3)
+		return -EINVAL;
+
+	ret = of_property_read_u32_array(np, "fsl,weim-cs-timing",
+					value, CS_TIMING_LEN);
+	if (ret)
+		return ret;
+
+	/* set the timing for WEIM */
+	for (i = 0; i < CS_TIMING_LEN; i++)
+		writel(value[i], weim->base + cs_idx * CS_REG_RANGE + i * 4);
+	return 0;
+}
+
+static int weim_parse_dt(struct platform_device *pdev)
+{
+	struct device_node *child;
+	int ret;
+
+	for_each_child_of_node(pdev->dev.of_node, child) {
+		if (!child->name)
+			continue;
+
+		ret = weim_timing_setup(pdev, child);
+		if (ret)
+			goto parse_fail;
+
+		if (!of_platform_device_create(child, NULL, &pdev->dev)) {
+			ret = -EINVAL;
+			goto parse_fail;
+		}
+
+	}
+	return 0;
+
+parse_fail:
+	return ret;
+}
+
+static int weim_probe(struct platform_device *pdev)
+{
+	struct imx_weim *weim;
+	struct resource *res;
+	int ret = -EINVAL;
+
+	weim = devm_kzalloc(&pdev->dev, sizeof(*weim), GFP_KERNEL);
+	if (!weim) {
+		ret = -ENOMEM;
+		goto weim_err;
+	}
+	platform_set_drvdata(pdev, weim);
+
+	/* get the resource */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	weim->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(weim->base)) {
+		ret = PTR_ERR(weim->base);
+		goto weim_err;
+	}
+
+	/* get the clock */
+	weim->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(weim->clk))
+		goto weim_err;
+
+	clk_prepare_enable(weim->clk);
+
+	/* parse the device node */
+	ret = weim_parse_dt(pdev);
+	if (ret) {
+		clk_disable_unprepare(weim->clk);
+		goto weim_err;
+	}
+
+	dev_info(&pdev->dev, "WEIM driver registered.\n");
+	return 0;
+
+weim_err:
+	return ret;
+}
+
+static int weim_remove(struct platform_device *pdev)
+{
+	struct imx_weim *weim = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(weim->clk);
+	return 0;
+}
+
+static struct platform_driver weim_driver = {
+	.driver = {
+		.name = "imx-weim",
+		.of_match_table = weim_id_table,
+	},
+	.probe   = weim_probe,
+	.remove  = weim_remove,
+};
+
+module_platform_driver(weim_driver);
+MODULE_AUTHOR("Freescale Semiconductor Inc.");
+MODULE_DESCRIPTION("i.MX EIM Controller Driver");
+MODULE_LICENSE("GPL");
-- 
1.7.1

Re: [PATCH V2 1/6] drivers: bus: add a new driver for WEIM

[Alexander Shiyan]

> The WEIM(Wireless External Interface Module) works like a bus.
> You can attach many different devices on it, such as NOR, onenand.
> 
> In the case of i.MX6q-sabreauto, the NOR is connected to WEIM.
> 
> This patch also adds the devicetree binding document.
> The driver only works when the devicetree is enabled.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  Documentation/devicetree/bindings/bus/imx-weim.txt |   50 +++++++
>  drivers/bus/Kconfig                                |    9 ++
>  drivers/bus/Makefile                               |    1 +
>  drivers/bus/imx-weim.c                             |  146 ++++++++++++++++++++
>  4 files changed, 206 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/bus/imx-weim.txt
>  create mode 100644 drivers/bus/imx-weim.c
> 
> diff --git a/Documentation/devicetree/bindings/bus/imx-weim.txt b/Documentation/devicetree/bindings/bus/imx-weim.txt
> new file mode 100644
> index 0000000..3db588e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/bus/imx-weim.txt
> @@ -0,0 +1,50 @@
> +Device tree bindings for i.MX Wireless External Interface Module (WEIM)
> +
> +The term "wireless" does not imply that the WEIM is literally an interface
> +without wires. It simply means that this module was originally designed for
> +wireless and mobile applications that use low-power technology.
> +
> +The actual devices are instantiated from the child nodes of a WEIM node.
> +
> +Required properties:
> +
> + - compatible:		Should be set to "fsl,imx6q-weim"
> + - reg:			A resource specifier for the register space
> +			(see the example below)
> + - interrupts:		the interrupt number, see the example below.

Where interrupt is used in the driver?

> + - clocks:		the clock, see the example below.
> + - #address-cells:	Must be set to 2 to allow memory address translation
> + - #size-cells:		Must be set to 1 to allow CS address passing
> + - ranges:		Must be set up to reflect the memory layout with four
> +			integer values for each chip-select line in use:
> +
> +			   <cs-number> 0 <physical address of mapping> <size>
> +
> +Timing property for child nodes. It is mandatory, not optional.
> +
> + - fsl,weim-cs-timing:	The timing array, contains 6 timing values for the
> +			child node. We can get the CS index from the child
> +			node's "reg" property.
> +
> +Example for an imx6q-sabreauto board, the NOR flash connected to the WEIM:
> +
> +	weim: weim at 021b8000 {
> +		compatible = "fsl,imx6q-weim";
> +		reg = <0x021b8000 0x4000>;
> +		interrupts = <0 14 0x04>;
> +		clocks = <&clks 196>;
> +		#address-cells = <2>;
> +		#size-cells = <1>;
> +		ranges = <0 0 0x08000000 0x08000000>;
> +
> +		nor at 0,0 {
> +			compatible = "cfi-flash";
> +			reg = <0 0 0x02000000>;
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			bank-width = <2>;
> +
> +			fsl,weim-cs-timing = <0x00620081 0x00000001 0x1C022000
> +					0x0000C000 0x1404a38e 0x00000000>;
> +		};
> +	};
> diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
> index b05ecab..488702b 100644
> --- a/drivers/bus/Kconfig
> +++ b/drivers/bus/Kconfig
> @@ -4,6 +4,15 @@
>  
>  menu "Bus devices"
>  
> +config IMX_WEIM
> +	tristate "Freescale EIM DRIVER"
> +	depends on ARCH_MXC
> +	help
> +	  Driver for i.MX6 WEIM controller.
> +	  The WEIM(Wireless External Interface Module) works like a bus.
> +	  You can attach many different devices on it, such as NOR, onenand.
> +	  But now, we only support the Parallel NOR.
> +
>  config MVEBU_MBUS
>  	bool
>  	depends on PLAT_ORION
> diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
> index 3c7b53c..436bbcc 100644
> --- a/drivers/bus/Makefile
> +++ b/drivers/bus/Makefile
> @@ -2,6 +2,7 @@
>  # Makefile for the bus drivers.
>  #
>  
> +obj-$(CONFIG_IMX_WEIM)	+= imx-weim.o
>  obj-$(CONFIG_MVEBU_MBUS) += mvebu-mbus.o
>  obj-$(CONFIG_OMAP_OCP2SCP)	+= omap-ocp2scp.o
>  
> diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
> new file mode 100644
> index 0000000..b932add
> --- /dev/null
> +++ b/drivers/bus/imx-weim.c
> @@ -0,0 +1,146 @@
> +/*
> + * EIM driver for Freescale's i.MX chips
> + *
> + * Copyright (C) 2013 Freescale Semiconductor, Inc.
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +#include <linux/module.h>
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/of_device.h>
> +
> +struct imx_weim {
> +	void __iomem *base;
> +	struct clk *clk;
> +};
> +
> +static const struct of_device_id weim_id_table[] = {
> +	{ .compatible = "fsl,imx6q-weim", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, weim_id_table);
> +
> +#define CS_TIMING_LEN 6
> +#define CS_REG_RANGE  0x18
> +
> +/* Parse and set the timing for this device. */
> +static int
> +weim_timing_setup(struct platform_device *pdev, struct device_node *np)
> +{
> +	struct imx_weim *weim = platform_get_drvdata(pdev);
> +	u32 value[CS_TIMING_LEN];
> +	u32 cs_idx;
> +	int ret;
> +	int i;
> +
> +	/* get the CS index from this child node's "reg" property. */
> +	ret = of_property_read_u32(np, "reg", &cs_idx);
> +	if (ret)
> +		return ret;
> +
> +	/* The weim has four chip selects. */
> +	if (cs_idx > 3)
> +		return -EINVAL;
> +
> +	ret = of_property_read_u32_array(np, "fsl,weim-cs-timing",
> +					value, CS_TIMING_LEN);
> +	if (ret)
> +		return ret;
> +
> +	/* set the timing for WEIM */
> +	for (i = 0; i < CS_TIMING_LEN; i++)
> +		writel(value[i], weim->base + cs_idx * CS_REG_RANGE + i * 4);
> +	return 0;
> +}
> +
> +static int weim_parse_dt(struct platform_device *pdev)
> +{
> +	struct device_node *child;
> +	int ret;
> +
> +	for_each_child_of_node(pdev->dev.of_node, child) {
> +		if (!child->name)
> +			continue;
> +
> +		ret = weim_timing_setup(pdev, child);
> +		if (ret)
> +			goto parse_fail;
> +
> +		if (!of_platform_device_create(child, NULL, &pdev->dev)) {
> +			ret = -EINVAL;
> +			goto parse_fail;
> +		}
> +
> +	}
> +	return 0;
> +
> +parse_fail:
> +	return ret;
> +}
> +
> +static int weim_probe(struct platform_device *pdev)
> +{
> +	struct imx_weim *weim;
> +	struct resource *res;
> +	int ret = -EINVAL;
> +
> +	weim = devm_kzalloc(&pdev->dev, sizeof(*weim), GFP_KERNEL);
> +	if (!weim) {
> +		ret = -ENOMEM;
> +		goto weim_err;
> +	}
> +	platform_set_drvdata(pdev, weim);
> +
> +	/* get the resource */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	weim->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(weim->base)) {
> +		ret = PTR_ERR(weim->base);
> +		goto weim_err;
> +	}
> +
> +	/* get the clock */
> +	weim->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(weim->clk))
> +		goto weim_err;
> +
> +	clk_prepare_enable(weim->clk);
> +
> +	/* parse the device node */
> +	ret = weim_parse_dt(pdev);
> +	if (ret) {
> +		clk_disable_unprepare(weim->clk);
> +		goto weim_err;
> +	}
> +
> +	dev_info(&pdev->dev, "WEIM driver registered.\n");
> +	return 0;
> +
> +weim_err:
> +	return ret;
> +}
> +
> +static int weim_remove(struct platform_device *pdev)
> +{
> +	struct imx_weim *weim = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(weim->clk);
> +	return 0;
> +}
> +
> +static struct platform_driver weim_driver = {
> +	.driver = {
> +		.name = "imx-weim",
> +		.of_match_table = weim_id_table,
> +	},
> +	.probe   = weim_probe,
> +	.remove  = weim_remove,
> +};
> +
> +module_platform_driver(weim_driver);
> +MODULE_AUTHOR("Freescale Semiconductor Inc.");
> +MODULE_DESCRIPTION("i.MX EIM Controller Driver");
> +MODULE_LICENSE("GPL");
> -- 

---

[PATCH V2 1/6] drivers: bus: add a new driver for WEIM

[Huang Shijie]

? 2013?05?23? 16:46, Alexander Shiyan ??:
> Where interrupt is used in the driver?
the driver does not use this interrtupt now, but i am not sure whether 
we use it in
the future.

thanks
Huang Shijie

[PATCH V2 1/6] drivers: bus: add a new driver for WEIM

[Sascha Hauer]

On Thu, May 23, 2013 at 04:16:13PM +0800, Huang Shijie wrote:
> The WEIM(Wireless External Interface Module) works like a bus.
> You can attach many different devices on it, such as NOR, onenand.
> 
> In the case of i.MX6q-sabreauto, the NOR is connected to WEIM.
> 
> This patch also adds the devicetree binding document.
> The driver only works when the devicetree is enabled.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  Documentation/devicetree/bindings/bus/imx-weim.txt |   50 +++++++
>  drivers/bus/Kconfig                                |    9 ++
>  drivers/bus/Makefile                               |    1 +
>  drivers/bus/imx-weim.c                             |  146 ++++++++++++++++++++
>  4 files changed, 206 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/bus/imx-weim.txt
>  create mode 100644 drivers/bus/imx-weim.c
> 
> diff --git a/Documentation/devicetree/bindings/bus/imx-weim.txt b/Documentation/devicetree/bindings/bus/imx-weim.txt
> new file mode 100644
> index 0000000..3db588e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/bus/imx-weim.txt
> @@ -0,0 +1,50 @@
> +Device tree bindings for i.MX Wireless External Interface Module (WEIM)
> +
> +The term "wireless" does not imply that the WEIM is literally an interface
> +without wires. It simply means that this module was originally designed for
> +wireless and mobile applications that use low-power technology.
> +
> +The actual devices are instantiated from the child nodes of a WEIM node.
> +
> +
> + - compatible:		Should be set to "fsl,imx6q-weim"
> + - reg:			A resource specifier for the register space
> +			(see the example below)
> + - interrupts:		the interrupt number, see the example below.
> + - clocks:		the clock, see the example below.
> + - #address-cells:	Must be set to 2 to allow memory address translation
> + - #size-cells:		Must be set to 1 to allow CS address passing
> + - ranges:		Must be set up to reflect the memory layout with four
> +			integer values for each chip-select line in use:
> +
> +			   <cs-number> 0 <physical address of mapping> <size>
> +
> +Timing property for child nodes. It is mandatory, not optional.
> +
> + - fsl,weim-cs-timing:	The timing array, contains 6 timing values for the
> +			child node. We can get the CS index from the child
> +			node's "reg" property.

This should be more detailed, something like:

This contains the values for the registers EIM_CSnGCR1, EIM_CSnGCR2,
EIM_CSnRCR1, EIM_CSnRCR2, EIM_CSnWCR1, EIM_CSnWCR2 in this order.

> +static int weim_parse_dt(struct platform_device *pdev)
> +{
> +	struct device_node *child;
> +	int ret;
> +
> +	for_each_child_of_node(pdev->dev.of_node, child) {
> +		if (!child->name)
> +			continue;
> +
> +		ret = weim_timing_setup(pdev, child);
> +		if (ret)
> +			goto parse_fail;
> +
> +		if (!of_platform_device_create(child, NULL, &pdev->dev)) {
> +			ret = -EINVAL;
> +			goto parse_fail;
> +		}

I would print some warning here for the failures in this loop, but I
don't think it's necessary to bail out. No need to make all WEIM devices
fail if one has an erroneous device node.

> +
> +	/* get the resource */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	weim->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(weim->base)) {
> +		ret = PTR_ERR(weim->base);
> +		goto weim_err;
> +	}
> +
> +	/* get the clock */
> +	weim->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(weim->clk))
> +		goto weim_err;
> +
> +	clk_prepare_enable(weim->clk);
> +
> +	/* parse the device node */
> +	ret = weim_parse_dt(pdev);
> +	if (ret) {
> +		clk_disable_unprepare(weim->clk);
> +		goto weim_err;
> +	}
> +
> +	dev_info(&pdev->dev, "WEIM driver registered.\n");
> +	return 0;
> +
> +weim_err:
> +	return ret;
> +}
> +
> +static int weim_remove(struct platform_device *pdev)
> +{
> +	struct imx_weim *weim = platform_get_drvdata(pdev);
> +
> +	clk_disable_unprepare(weim->clk);

Once again: Is this clock needed for the child devices? If yes, you
can't disable it here and leave the child devices registered.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH V2 1/6] drivers: bus: add a new driver for WEIM

[Huang Shijie]

? 2013?05?23? 17:23, Sascha Hauer ??:
> On Thu, May 23, 2013 at 04:16:13PM +0800, Huang Shijie wrote:
>> +			<cs-number>  0<physical address of mapping>  <size>
>> +
>> +Timing property for child nodes. It is mandatory, not optional.
>> +
>> + - fsl,weim-cs-timing:	The timing array, contains 6 timing values for the
>> +			child node. We can get the CS index from the child
>> +			node's "reg" property.
> This should be more detailed, something like:
>
> This contains the values for the registers EIM_CSnGCR1, EIM_CSnGCR2,
> EIM_CSnRCR1, EIM_CSnRCR2, EIM_CSnWCR1, EIM_CSnWCR2 in this order.
>


do you mean i should add some new properties, such as
"fsl,eim_csnrcr1", "fsl,eim_csnrcr2" ...
>> +static int weim_parse_dt(struct platform_device *pdev)
>> +{
>> +	struct device_node *child;
>> +	int ret;
>> +
>> +	for_each_child_of_node(pdev->dev.of_node, child) {
>> +		if (!child->name)
>> +			continue;
>> +
>> +		ret = weim_timing_setup(pdev, child);
>> +		if (ret)
>> +			goto parse_fail;
>> +
>> +		if (!of_platform_device_create(child, NULL,&pdev->dev)) {
>> +			ret = -EINVAL;
>> +			goto parse_fail;
>> +		}
> I would print some warning here for the failures in this loop, but I
> don't think it's necessary to bail out. No need to make all WEIM devices
> fail if one has an erroneous device node.
>
ok.
>> +
>> +	/* get the resource */
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	weim->base = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(weim->base)) {
>> +		ret = PTR_ERR(weim->base);
>> +		goto weim_err;
>> +	}
>> +
>> +	/* get the clock */
>> +	weim->clk = devm_clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(weim->clk))
>> +		goto weim_err;
>> +
>> +	clk_prepare_enable(weim->clk);
>> +
>> +	/* parse the device node */
>> +	ret = weim_parse_dt(pdev);
>> +	if (ret) {
>> +		clk_disable_unprepare(weim->clk);
>> +		goto weim_err;
>> +	}
>> +
>> +	dev_info(&pdev->dev, "WEIM driver registered.\n");
>> +	return 0;
>> +
>> +weim_err:
>> +	return ret;
>> +}
>> +
>> +static int weim_remove(struct platform_device *pdev)
>> +{
>> +	struct imx_weim *weim = platform_get_drvdata(pdev);
>> +
>> +	clk_disable_unprepare(weim->clk);
> Once again: Is this clock needed for the child devices? If yes, you
> can't disable it here and leave the child devices registered.
>
again. yes. we need this clock.


thanks
Huang Shijie

[PATCH V2 1/6] drivers: bus: add a new driver for WEIM

[Arnd Bergmann]

On Thursday 23 May 2013, Huang Shijie wrote:
> ? 2013?05?23? 17:23, Sascha Hauer ??:
> > On Thu, May 23, 2013 at 04:16:13PM +0800, Huang Shijie wrote:
> >> +			<cs-number>  0<physical address of mapping>  <size>
> >> +
> >> +Timing property for child nodes. It is mandatory, not optional.
> >> +
> >> + - fsl,weim-cs-timing:	The timing array, contains 6 timing values for the
> >> +			child node. We can get the CS index from the child
> >> +			node's "reg" property.
> > This should be more detailed, something like:
> >
> > This contains the values for the registers EIM_CSnGCR1, EIM_CSnGCR2,
> > EIM_CSnRCR1, EIM_CSnRCR2, EIM_CSnWCR1, EIM_CSnWCR2 in this order.
> >
> 
> 
> do you mean i should add some new properties, such as
> "fsl,eim_csnrcr1", "fsl,eim_csnrcr2" ...

No, the property is fine, just update the documentation the way that
Sascha suggested.

> >> +static int weim_remove(struct platform_device *pdev)
> >> +{
> >> +	struct imx_weim *weim = platform_get_drvdata(pdev);
> >> +
> >> +	clk_disable_unprepare(weim->clk);
> > Once again: Is this clock needed for the child devices? If yes, you
> > can't disable it here and leave the child devices registered.
> >

But weim_remove will not be called as long as there are children
registered, right?

	Arnd

[PATCH V2 1/6] drivers: bus: add a new driver for WEIM

[Sascha Hauer]

On Thu, May 23, 2013 at 11:53:37AM +0200, Arnd Bergmann wrote:
> On Thursday 23 May 2013, Huang Shijie wrote:
> > ? 2013?05?23? 17:23, Sascha Hauer ??:
> > > On Thu, May 23, 2013 at 04:16:13PM +0800, Huang Shijie wrote:
> > >> +			<cs-number>  0<physical address of mapping>  <size>
> > >> +
> > >> +Timing property for child nodes. It is mandatory, not optional.
> > >> +
> > >> + - fsl,weim-cs-timing:	The timing array, contains 6 timing values for the
> > >> +			child node. We can get the CS index from the child
> > >> +			node's "reg" property.
> > > This should be more detailed, something like:
> > >
> > > This contains the values for the registers EIM_CSnGCR1, EIM_CSnGCR2,
> > > EIM_CSnRCR1, EIM_CSnRCR2, EIM_CSnWCR1, EIM_CSnWCR2 in this order.
> > >
> > 
> > 
> > do you mean i should add some new properties, such as
> > "fsl,eim_csnrcr1", "fsl,eim_csnrcr2" ...
> 
> No, the property is fine, just update the documentation the way that
> Sascha suggested.

Yes, that's what I meant.

> 
> > >> +static int weim_remove(struct platform_device *pdev)
> > >> +{
> > >> +	struct imx_weim *weim = platform_get_drvdata(pdev);
> > >> +
> > >> +	clk_disable_unprepare(weim->clk);
> > > Once again: Is this clock needed for the child devices? If yes, you
> > > can't disable it here and leave the child devices registered.
> > >
> 
> But weim_remove will not be called as long as there are children
> registered, right?

But then weim_remove would never be called since nobody unregisters the
children, right?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH V2 1/6] drivers: bus: add a new driver for WEIM

[Arnd Bergmann]

On Thursday 23 May 2013, Sascha Hauer wrote:
> > > >> +static int weim_remove(struct platform_device *pdev)
> > > >> +{
> > > >> +        struct imx_weim *weim = platform_get_drvdata(pdev);
> > > >> +
> > > >> +        clk_disable_unprepare(weim->clk);
> > > > Once again: Is this clock needed for the child devices? If yes, you
> > > > can't disable it here and leave the child devices registered.
> > > >
> > 
> > But weim_remove will not be called as long as there are children
> > registered, right?
> 
> But then weim_remove would never be called since nobody unregisters the
> children, right?
> 

Ah, right. That is actually another problem: if the probe() function
creates the child devices, the remove() function should tear them down
again. If we do that, we can ensure that the clock is only turned
off after the last child is gone.

OTOH, I agree that it would be nicer if the clk could remain turned
off as long as no children are active. Can we do a clk_disable()
after setting up the timings for the children and then expect those
to actually start up the clk again when they need it?

Or to take things further: would it make sense to represent WEIM
itself as a clock driver and perform the settings for each child
only when it sets up its own clk?

I guess it would also make sense to use of_platform_populate() instead
of of_platform_device_create() when creating the children, so we actually
create the entire hierarchy in case of the children itself is a
"simple-bus".

	Arnd

[PATCH V2 1/6] drivers: bus: add a new driver for WEIM

[Huang Shijie]

? 2013?05?23? 21:09, Arnd Bergmann ??:
> OTOH, I agree that it would be nicer if the clk could remain turned
> off as long as no children are active. Can we do a clk_disable()
> after setting up the timings for the children and then expect those
> to actually start up the clk again when they need it?
>
> Or to take things further: would it make sense to represent WEIM
> itself as a clock driver and perform the settings for each child
> only when it sets up its own clk?
>
If the child's subsystem supports it, we can do it.

The mtd nand does support this feature, it has @select_chip() hook.
We can enable/disable the clock in the @select_chip().

But the mtd NOR does _not_ supports it.
Please read the /drivers/mtd/maps/physmap_of.c and 
/drivers/mtd/chips/cfi_cmd_set_xxx.c.
Of course, we can add the feature to NOR subsystem. But that's another 
issue.

So the weim should enable the clock all the time now.


> I guess it would also make sense to use of_platform_populate() instead
> of of_platform_device_create() when creating the children, so we actually
I tried the of_platform_populate(), but failed.

firstly, we should set the timing for the device _before_ we do the 
of_platform_populate().
and i rewrite the weim_parse_dt() to:
------------------------------------------------------------------------------------------
static void weim_parse_dt(struct platform_device *pdev)
{
     struct device_node *child;

     for_each_child_of_node(pdev->dev.of_node, child) {
         if (!child->name)
             continue;

         if (weim_timing_setup(pdev, child)) {
             dev_err(&pdev->dev, "%s set timing failed.\n",
                 child->full_name);
             continue;
         }

     }
     if (!of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev))
         dev_err(&pdev->dev, "%s device create failed.\n",
             child->full_name);
}
------------------------------------------------------------------------------------------

The system hang at :
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++

## Booting kernel from Legacy Image at 10800000 ...
    Image Name:   Linux
    Image Type:   ARM Linux Kernel Image (uncompressed)
    Data Size:    4259851 Bytes =  4.1 MB
    Load Address: 10008000
    Entry Point:  10008000
    Verifying Checksum ... OK
    Loading Kernel Image ... OK
OK

Starting kernel ...
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++==



thanks
Huang Shijie

[PATCH V2 1/6] drivers: bus: add a new driver for WEIM

[Sascha Hauer]

On Fri, May 24, 2013 at 03:16:53PM +0800, Huang Shijie wrote:
> ? 2013?05?23? 21:09, Arnd Bergmann ??:
> >OTOH, I agree that it would be nicer if the clk could remain turned
> >off as long as no children are active. Can we do a clk_disable()
> >after setting up the timings for the children and then expect those
> >to actually start up the clk again when they need it?
> >
> >Or to take things further: would it make sense to represent WEIM
> >itself as a clock driver and perform the settings for each child
> >only when it sets up its own clk?
> >
> If the child's subsystem supports it, we can do it.
> 
> The mtd nand does support this feature, it has @select_chip() hook.
> We can enable/disable the clock in the @select_chip().
> 
> But the mtd NOR does _not_ supports it.
> Please read the /drivers/mtd/maps/physmap_of.c and
> /drivers/mtd/chips/cfi_cmd_set_xxx.c.
> Of course, we can add the feature to NOR subsystem. But that's
> another issue.
> 
> So the weim should enable the clock all the time now.

Personally I'm fine with this, but you should not disable the clock when
the children are active, so either make sure

- the WEIM driver can't be unloaded, or
- the WEIM driver unregisters its children, or
- just leave the clock enabled.

> ------------------------------------------------------------------------------------------
> 
> The system hang at :
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 
> ## Booting kernel from Legacy Image at 10800000 ...
>    Image Name:   Linux
>    Image Type:   ARM Linux Kernel Image (uncompressed)
>    Data Size:    4259851 Bytes =  4.1 MB
>    Load Address: 10008000
>    Entry Point:  10008000
>    Verifying Checksum ... OK
>    Loading Kernel Image ... OK
> OK
> 
> Starting kernel ...
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++==

Use EARLY_PRINTK or move your driver init to a late_initcall to make it
initialize after the console.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH V2 1/6] drivers: bus: add a new driver for WEIM

[Russell King - ARM Linux]

On Thu, May 23, 2013 at 04:16:13PM +0800, Huang Shijie wrote:
> +	/* get the clock */
> +	weim->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(weim->clk))
> +		goto weim_err;
> +
> +	clk_prepare_enable(weim->clk);

I notice people are getting lazy about this.  clk_prepare_enable() can
return an error...

[PATCH V2 1/6] drivers: bus: add a new driver for WEIM

[Huang Shijie]

? 2013?05?23? 18:19, Russell King - ARM Linux ??:
> On Thu, May 23, 2013 at 04:16:13PM +0800, Huang Shijie wrote:
>> +	/* get the clock */
>> +	weim->clk = devm_clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(weim->clk))
>> +		goto weim_err;
>> +
>> +	clk_prepare_enable(weim->clk);
> I notice people are getting lazy about this.  clk_prepare_enable() can
> return an error...
>
i will add the error check in next version.

thanks
Huang Shijie

[PATCH V2 2/6] ARM: dts: imx6q{dl}: fix the pin conflict between SPI and WEIM

[Huang Shijie]

In the imx6q-sabreauto and imx6dl-sabreauto boards,
the pin MX6Q{DL}_PAD_EIM_D19 is used as a GPIO for SPI NOR, but
it is used as a data pin for the WEIM NOR.

In order to fix the conflict, this patch removes the pin from the hog,
and adds a new board-level pinctrl: pinctrl_ecspi1_sabreauto.

The SPI NOR selects this pinctrl_ecspi1_sabreauto when it is enabled.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 arch/arm/boot/dts/imx6dl-sabreauto.dts   |    9 ++++++++-
 arch/arm/boot/dts/imx6q-sabreauto.dts    |    9 ++++++++-
 arch/arm/boot/dts/imx6qdl-sabreauto.dtsi |    2 +-
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/imx6dl-sabreauto.dts b/arch/arm/boot/dts/imx6dl-sabreauto.dts
index 60f3038..95da711 100644
--- a/arch/arm/boot/dts/imx6dl-sabreauto.dts
+++ b/arch/arm/boot/dts/imx6dl-sabreauto.dts
@@ -25,7 +25,14 @@
 			fsl,pins = <
 				MX6DL_PAD_NANDF_CS2__GPIO6_IO15 0x80000000
 				MX6DL_PAD_SD2_DAT2__GPIO1_IO13  0x80000000
-				MX6DL_PAD_EIM_D19__GPIO3_IO19 	0x80000000
+			>;
+		};
+	};
+
+	ecspi1 {
+		pinctrl_ecspi1_sabreauto: ecspi1-sabreauto {
+			fsl,pins = <
+				MX6DL_PAD_EIM_D19__GPIO3_IO19  0x80000000
 			>;
 		};
 	};
diff --git a/arch/arm/boot/dts/imx6q-sabreauto.dts b/arch/arm/boot/dts/imx6q-sabreauto.dts
index 9fb3e99..09a7580 100644
--- a/arch/arm/boot/dts/imx6q-sabreauto.dts
+++ b/arch/arm/boot/dts/imx6q-sabreauto.dts
@@ -29,7 +29,14 @@
 			fsl,pins = <
 				MX6Q_PAD_NANDF_CS2__GPIO6_IO15 0x80000000
 				MX6Q_PAD_SD2_DAT2__GPIO1_IO13  0x80000000
-				MX6Q_PAD_EIM_D19__GPIO3_IO19   0x80000000
+			>;
+		};
+	};
+
+	ecspi1 {
+		pinctrl_ecspi1_sabreauto: ecspi1-sabreauto {
+			fsl,pins = <
+				MX6Q_PAD_EIM_D19__GPIO3_IO19  0x80000000
 			>;
 		};
 	};
diff --git a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
index d6baa51..a4466e6 100644
--- a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
@@ -20,7 +20,7 @@
 	fsl,spi-num-chipselects = <1>;
 	cs-gpios = <&gpio3 19 0>;
 	pinctrl-names = "default";
-	pinctrl-0 = <&pinctrl_ecspi1_1>;
+	pinctrl-0 = <&pinctrl_ecspi1_1 &pinctrl_ecspi1_sabreauto>;
 	status = "disabled"; /* pin conflict with WEIM NOR */
 
 	flash: m25p80 at 0 {
-- 
1.7.1

[PATCH V2 3/6] ARM: dts: imx6qdl: add more information for WEIM

[Huang Shijie]

Add the clock and compatible information for the weim.
Also adds the weim label.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 arch/arm/boot/dts/imx6qdl.dtsi |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index 42e461c..f21d259 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -744,9 +744,11 @@
 				reg = <0x021b4000 0x4000>;
 			};
 
-			weim at 021b8000 {
+			weim: weim at 021b8000 {
+				compatible = "fsl,imx6q-weim";
 				reg = <0x021b8000 0x4000>;
 				interrupts = <0 14 0x04>;
+				clocks = <&clks 196>;
 			};
 
 			ocotp at 021bc000 {
-- 
1.7.1

[PATCH V2 4/6] ARM: dts: imx6q: add pinctrl for WEIM NOR

[Huang Shijie]

Add a pinctrl for WEIM nor.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 arch/arm/boot/dts/imx6q.dtsi |   56 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 56 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index ed11bcf..2a6327a 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -323,6 +323,62 @@
 						>;
 					};
 				};
+
+				weim {
+					pinctrl_weim_nor_1: weim_norgrp-1 {
+						fsl,pins = <
+							MX6Q_PAD_EIM_CS0__EIM_CS0_B   0xb0b1
+							MX6Q_PAD_EIM_OE__EIM_OE_B     0xb0b1
+							MX6Q_PAD_EIM_RW__EIM_RW       0xb0b1
+							MX6Q_PAD_EIM_WAIT__EIM_WAIT_B 0xb060
+
+							/* data */
+							MX6Q_PAD_EIM_D16__EIM_DATA16 0xb0b1
+							MX6Q_PAD_EIM_D17__EIM_DATA17 0xb0b1
+							MX6Q_PAD_EIM_D18__EIM_DATA18 0xb0b1
+							MX6Q_PAD_EIM_D19__EIM_DATA19 0xb0b1
+							MX6Q_PAD_EIM_D20__EIM_DATA20 0xb0b1
+							MX6Q_PAD_EIM_D21__EIM_DATA21 0xb0b1
+							MX6Q_PAD_EIM_D22__EIM_DATA22 0xb0b1
+							MX6Q_PAD_EIM_D23__EIM_DATA23 0xb0b1
+							MX6Q_PAD_EIM_D24__EIM_DATA24 0xb0b1
+							MX6Q_PAD_EIM_D25__EIM_DATA25 0xb0b1
+							MX6Q_PAD_EIM_D26__EIM_DATA26 0xb0b1
+							MX6Q_PAD_EIM_D27__EIM_DATA27 0xb0b1
+							MX6Q_PAD_EIM_D28__EIM_DATA28 0xb0b1
+							MX6Q_PAD_EIM_D29__EIM_DATA29 0xb0b1
+							MX6Q_PAD_EIM_D30__EIM_DATA30 0xb0b1
+							MX6Q_PAD_EIM_D31__EIM_DATA31 0xb0b1
+
+							/* address */
+							MX6Q_PAD_EIM_A23__EIM_ADDR23 0xb0b1
+							MX6Q_PAD_EIM_A22__EIM_ADDR22 0xb0b1
+							MX6Q_PAD_EIM_A21__EIM_ADDR21 0xb0b1
+							MX6Q_PAD_EIM_A20__EIM_ADDR20 0xb0b1
+							MX6Q_PAD_EIM_A19__EIM_ADDR19 0xb0b1
+							MX6Q_PAD_EIM_A18__EIM_ADDR18 0xb0b1
+							MX6Q_PAD_EIM_A17__EIM_ADDR17 0xb0b1
+							MX6Q_PAD_EIM_A16__EIM_ADDR16 0xb0b1
+							MX6Q_PAD_EIM_DA15__EIM_AD15  0xb0b1
+							MX6Q_PAD_EIM_DA14__EIM_AD14  0xb0b1
+							MX6Q_PAD_EIM_DA13__EIM_AD13  0xb0b1
+							MX6Q_PAD_EIM_DA12__EIM_AD12  0xb0b1
+							MX6Q_PAD_EIM_DA11__EIM_AD11  0xb0b1
+							MX6Q_PAD_EIM_DA10__EIM_AD10  0xb0b1
+							MX6Q_PAD_EIM_DA9__EIM_AD09   0xb0b1
+							MX6Q_PAD_EIM_DA8__EIM_AD08   0xb0b1
+							MX6Q_PAD_EIM_DA7__EIM_AD07   0xb0b1
+							MX6Q_PAD_EIM_DA6__EIM_AD06   0xb0b1
+							MX6Q_PAD_EIM_DA5__EIM_AD05   0xb0b1
+							MX6Q_PAD_EIM_DA4__EIM_AD04   0xb0b1
+							MX6Q_PAD_EIM_DA3__EIM_AD03   0xb0b1
+							MX6Q_PAD_EIM_DA2__EIM_AD02   0xb0b1
+							MX6Q_PAD_EIM_DA1__EIM_AD01   0xb0b1
+							MX6Q_PAD_EIM_DA0__EIM_AD00   0xb0b1
+						>;
+					};
+
+				};
 			};
 		};
 
-- 
1.7.1

Re: [PATCH V2 4/6] ARM: dts: imx6q: add pinctrl for WEIM NOR

[Alexander Shiyan]

> Add a pinctrl for WEIM nor.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  arch/arm/boot/dts/imx6q.dtsi |   56 ++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 56 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
> index ed11bcf..2a6327a 100644
> --- a/arch/arm/boot/dts/imx6q.dtsi
> +++ b/arch/arm/boot/dts/imx6q.dtsi
> @@ -323,6 +323,62 @@
>  						>;
>  					};
>  				};
> +
> +				weim {
> +					pinctrl_weim_nor_1: weim_norgrp-1 {

I suggest remove "nor" label here since it can be used for other devices later.

> +						fsl,pins = <
> +							MX6Q_PAD_EIM_CS0__EIM_CS0_B   0xb0b1

Since driver can support multiple chipselects it would be nice to
define EIM_CSX in other place for possible using other chipselects.

> +							MX6Q_PAD_EIM_OE__EIM_OE_B     0xb0b1
> +							MX6Q_PAD_EIM_RW__EIM_RW       0xb0b1
> +							MX6Q_PAD_EIM_WAIT__EIM_WAIT_B 0xb060
> +
> +							/* data */
> +							MX6Q_PAD_EIM_D16__EIM_DATA16 0xb0b1
> +							MX6Q_PAD_EIM_D17__EIM_DATA17 0xb0b1
> +							MX6Q_PAD_EIM_D18__EIM_DATA18 0xb0b1
> +							MX6Q_PAD_EIM_D19__EIM_DATA19 0xb0b1
> +							MX6Q_PAD_EIM_D20__EIM_DATA20 0xb0b1
> +							MX6Q_PAD_EIM_D21__EIM_DATA21 0xb0b1
> +							MX6Q_PAD_EIM_D22__EIM_DATA22 0xb0b1
> +							MX6Q_PAD_EIM_D23__EIM_DATA23 0xb0b1
> +							MX6Q_PAD_EIM_D24__EIM_DATA24 0xb0b1
> +							MX6Q_PAD_EIM_D25__EIM_DATA25 0xb0b1
> +							MX6Q_PAD_EIM_D26__EIM_DATA26 0xb0b1
> +							MX6Q_PAD_EIM_D27__EIM_DATA27 0xb0b1
> +							MX6Q_PAD_EIM_D28__EIM_DATA28 0xb0b1
> +							MX6Q_PAD_EIM_D29__EIM_DATA29 0xb0b1
> +							MX6Q_PAD_EIM_D30__EIM_DATA30 0xb0b1
> +							MX6Q_PAD_EIM_D31__EIM_DATA31 0xb0b1
> +
> +							/* address */
> +							MX6Q_PAD_EIM_A23__EIM_ADDR23 0xb0b1
> +							MX6Q_PAD_EIM_A22__EIM_ADDR22 0xb0b1
> +							MX6Q_PAD_EIM_A21__EIM_ADDR21 0xb0b1
> +							MX6Q_PAD_EIM_A20__EIM_ADDR20 0xb0b1
> +							MX6Q_PAD_EIM_A19__EIM_ADDR19 0xb0b1
> +							MX6Q_PAD_EIM_A18__EIM_ADDR18 0xb0b1
> +							MX6Q_PAD_EIM_A17__EIM_ADDR17 0xb0b1
> +							MX6Q_PAD_EIM_A16__EIM_ADDR16 0xb0b1
> +							MX6Q_PAD_EIM_DA15__EIM_AD15  0xb0b1
> +							MX6Q_PAD_EIM_DA14__EIM_AD14  0xb0b1
> +							MX6Q_PAD_EIM_DA13__EIM_AD13  0xb0b1
> +							MX6Q_PAD_EIM_DA12__EIM_AD12  0xb0b1
> +							MX6Q_PAD_EIM_DA11__EIM_AD11  0xb0b1
> +							MX6Q_PAD_EIM_DA10__EIM_AD10  0xb0b1
> +							MX6Q_PAD_EIM_DA9__EIM_AD09   0xb0b1
> +							MX6Q_PAD_EIM_DA8__EIM_AD08   0xb0b1
> +							MX6Q_PAD_EIM_DA7__EIM_AD07   0xb0b1
> +							MX6Q_PAD_EIM_DA6__EIM_AD06   0xb0b1
> +							MX6Q_PAD_EIM_DA5__EIM_AD05   0xb0b1
> +							MX6Q_PAD_EIM_DA4__EIM_AD04   0xb0b1
> +							MX6Q_PAD_EIM_DA3__EIM_AD03   0xb0b1
> +							MX6Q_PAD_EIM_DA2__EIM_AD02   0xb0b1
> +							MX6Q_PAD_EIM_DA1__EIM_AD01   0xb0b1
> +							MX6Q_PAD_EIM_DA0__EIM_AD00   0xb0b1
> +						>;
> +					};
> +
> +				};
>  			};
>  		};
>  
> -- 

---

[PATCH V2 4/6] ARM: dts: imx6q: add pinctrl for WEIM NOR

[Huang Shijie]

? 2013?05?23? 17:14, Alexander Shiyan ??:
> I suggest remove "nor" label here since it can be used for other devices later.
this pinctrl is just for NOR.

If we attach other device to WEIM, it maybe needs other PAD, such as

MX6Q_PAD_EIM_LBA__EIM_LBA_B.  We need to add another pinctrl then.



>> >  +						fsl,pins =<
>> >  +							MX6Q_PAD_EIM_CS0__EIM_CS0_B   0xb0b1
> Since driver can support multiple chipselects it would be nice to
> define EIM_CSX in other place for possible using other chipselects.
>
Ditto.

thanks
Huang Shijie

Re[2]: [PATCH V2 4/6] ARM: dts: imx6q: add pinctrl for WEIM NOR

[Alexander Shiyan]

> > I suggest remove "nor" label here since it can be used for other devices later.
> this pinctrl is just for NOR.
> 
> If we attach other device to WEIM, it maybe needs other PAD, such as
> MX6Q_PAD_EIM_LBA__EIM_LBA_B. We need to add another pinctrl then.

How we should define CS1 pin in your example for second device?

---

[PATCH V2 4/6] ARM: dts: imx6q: add pinctrl for WEIM NOR

[Huang Shijie]

? 2013?05?23? 17:39, Alexander Shiyan ??:
> How we should define CS1 pin in your example for second device?
>

But in the imx6q{dl}-sabreauto boards, we only have the NOR connected to 
WEIM.
I do not need to worry about the second device.


If in customer's boards, they connect two devices to the weim.
they should add their own special pinctrl for the two devices, such as 
split out the CS pad.

But for the imx6q{dl}-sabreauto boards, i do not need to do so.




thanks
Huang Shijie

Re[2]: [PATCH V2 4/6] ARM: dts: imx6q: add pinctrl for WEIM NOR

[Alexander Shiyan]

> > How we should define CS1 pin in your example for second device?
> 
> But in the imx6q{dl}-sabreauto boards, we only have the NOR connected to 
> WEIM.
> I do not need to worry about the second device.

OK, but you add "weim-NOR" to the arch/arm/boot/dts/imx6q.dtsi, not
in imx6qdl-sabreauto.dtsi, so you mean that this definition is generic.
Should it be moved to imx6qdl-sabreauto.dtsi then?

---

[PATCH V2 4/6] ARM: dts: imx6q: add pinctrl for WEIM NOR

[Huang Shijie]

? 2013?05?23? 17:55, Alexander Shiyan ??:
>>> How we should define CS1 pin in your example for second device?
>> But in the imx6q{dl}-sabreauto boards, we only have the NOR connected to
>> WEIM.
>> I do not need to worry about the second device.
> OK, but you add "weim-NOR" to the arch/arm/boot/dts/imx6q.dtsi, not
> in imx6qdl-sabreauto.dtsi, so you mean that this definition is generic.
> Should it be moved to imx6qdl-sabreauto.dtsi then?
okay. I will split out the CS pad to single pinctrl.

thanks for your review.

Huang Shijie

[PATCH V2 5/6] ARM: dts: imx6dl: add a pinctrl for WEIM NOR

[Huang Shijie]

Add a new pinctrl for WEIN NOR.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 arch/arm/boot/dts/imx6dl.dtsi |   55 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 55 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/imx6dl.dtsi b/arch/arm/boot/dts/imx6dl.dtsi
index 4b13454..4c8f838 100644
--- a/arch/arm/boot/dts/imx6dl.dtsi
+++ b/arch/arm/boot/dts/imx6dl.dtsi
@@ -183,6 +183,61 @@
 					};
 				};
 
+				weim {
+					pinctrl_weim_nor_1: weim_norgrp-1 {
+						fsl,pins = <
+							MX6DL_PAD_EIM_CS0__EIM_CS0_B   0xb0b1
+							MX6DL_PAD_EIM_OE__EIM_OE_B     0xb0b1
+							MX6DL_PAD_EIM_RW__EIM_RW       0xb0b1
+							MX6DL_PAD_EIM_WAIT__EIM_WAIT_B 0xb060
+
+							/* data */
+							MX6DL_PAD_EIM_D16__EIM_DATA16 0xb0b1
+							MX6DL_PAD_EIM_D17__EIM_DATA17 0xb0b1
+							MX6DL_PAD_EIM_D18__EIM_DATA18 0xb0b1
+							MX6DL_PAD_EIM_D19__EIM_DATA19 0xb0b1
+							MX6DL_PAD_EIM_D20__EIM_DATA20 0xb0b1
+							MX6DL_PAD_EIM_D21__EIM_DATA21 0xb0b1
+							MX6DL_PAD_EIM_D22__EIM_DATA22 0xb0b1
+							MX6DL_PAD_EIM_D23__EIM_DATA23 0xb0b1
+							MX6DL_PAD_EIM_D24__EIM_DATA24 0xb0b1
+							MX6DL_PAD_EIM_D25__EIM_DATA25 0xb0b1
+							MX6DL_PAD_EIM_D26__EIM_DATA26 0xb0b1
+							MX6DL_PAD_EIM_D27__EIM_DATA27 0xb0b1
+							MX6DL_PAD_EIM_D28__EIM_DATA28 0xb0b1
+							MX6DL_PAD_EIM_D29__EIM_DATA29 0xb0b1
+							MX6DL_PAD_EIM_D30__EIM_DATA30 0xb0b1
+							MX6DL_PAD_EIM_D31__EIM_DATA31 0xb0b1
+
+							/* address */
+							MX6DL_PAD_EIM_A23__EIM_ADDR23 0xb0b1
+							MX6DL_PAD_EIM_A22__EIM_ADDR22 0xb0b1
+							MX6DL_PAD_EIM_A21__EIM_ADDR21 0xb0b1
+							MX6DL_PAD_EIM_A20__EIM_ADDR20 0xb0b1
+							MX6DL_PAD_EIM_A19__EIM_ADDR19 0xb0b1
+							MX6DL_PAD_EIM_A18__EIM_ADDR18 0xb0b1
+							MX6DL_PAD_EIM_A17__EIM_ADDR17 0xb0b1
+							MX6DL_PAD_EIM_A16__EIM_ADDR16 0xb0b1
+							MX6DL_PAD_EIM_DA15__EIM_AD15  0xb0b1
+							MX6DL_PAD_EIM_DA14__EIM_AD14  0xb0b1
+							MX6DL_PAD_EIM_DA13__EIM_AD13  0xb0b1
+							MX6DL_PAD_EIM_DA12__EIM_AD12  0xb0b1
+							MX6DL_PAD_EIM_DA11__EIM_AD11  0xb0b1
+							MX6DL_PAD_EIM_DA10__EIM_AD10  0xb0b1
+							MX6DL_PAD_EIM_DA9__EIM_AD09   0xb0b1
+							MX6DL_PAD_EIM_DA8__EIM_AD08   0xb0b1
+							MX6DL_PAD_EIM_DA7__EIM_AD07   0xb0b1
+							MX6DL_PAD_EIM_DA6__EIM_AD06   0xb0b1
+							MX6DL_PAD_EIM_DA5__EIM_AD05   0xb0b1
+							MX6DL_PAD_EIM_DA4__EIM_AD04   0xb0b1
+							MX6DL_PAD_EIM_DA3__EIM_AD03   0xb0b1
+							MX6DL_PAD_EIM_DA2__EIM_AD02   0xb0b1
+							MX6DL_PAD_EIM_DA1__EIM_AD01   0xb0b1
+							MX6DL_PAD_EIM_DA0__EIM_AD00   0xb0b1
+						>;
+					};
+
+				};
 
 			};
 
-- 
1.7.1

[PATCH V2 6/6] ARM: dts: imx6qdl: enable the WEIM NOR

[Huang Shijie]

Enable the WEIM NOR for imx6q{dl} boards.

For the pin conflict with SPI NOR, its status is set to "disabled".

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 arch/arm/boot/dts/imx6qdl-sabreauto.dtsi |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
index a4466e6..684b208 100644
--- a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
@@ -58,3 +58,23 @@
 	wp-gpios = <&gpio1 13 0>;
 	status = "okay";
 };
+
+&weim {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_weim_nor_1>;
+	#address-cells = <2>;
+	#size-cells = <1>;
+	ranges = <0 0 0x08000000 0x08000000>;
+	status = "disabled"; /* pin conflict with SPI NOR */
+
+	nor at 0,0 {
+		compatible = "cfi-flash";
+		reg = <0 0 0x02000000>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+		bank-width = <2>;
+
+		fsl,weim-cs-timing = <0x00620081 0x00000001 0x1C022000
+				0x0000C000 0x1404a38e 0x00000000>;
+	};
+};
-- 
1.7.1