diff mbox

i.MX6-SabreAuto: EIM: pull PAD_EIM_D18 low for NOR probe

Message ID 1421278609-8446-1-git-send-email-alison_chaiken@mentor.com
State Changes Requested
Headers show

Commit Message

Chaiken, Alison Jan. 14, 2015, 11:36 p.m. UTC
From: Alison Chaiken <alison_chaiken@mentor.com>

PAD_EIM_D18 must be pulled low at boot in order for the parallel NOR
connected to the EIM switch to probe properly.  Otherwise
cfi_qry_present() may return "U-V-]" rather than "Q-R-Y" if the
PAD_EIM_D18 is high.  Add a nor-gpios property to the nor node in the
SabreAuto device-tree and add a function to the imx-weim probe to set
GPIO5 to drive the pad.

Signed-off-by: Alison Chaiken <alison_chaiken@mentor.com>
---
 arch/arm/boot/dts/imx6qdl-sabreauto.dtsi |  1 +
 drivers/bus/imx-weim.c                   | 43 ++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

Comments

Brian Norris Jan. 15, 2015, 12:03 a.m. UTC | #1
+ linux-arm-kernel, maintainers, devicetree

Shawn, Sascha: should this driver be listed under the Freescale IMX
MAINTAINERS entry?

On Wed, Jan 14, 2015 at 03:36:49PM -0800, alison@she-devel.com wrote:
> From: Alison Chaiken <alison_chaiken@mentor.com>
> 
> PAD_EIM_D18 must be pulled low at boot in order for the parallel NOR
> connected to the EIM switch to probe properly.  Otherwise
> cfi_qry_present() may return "U-V-]" rather than "Q-R-Y" if the
> PAD_EIM_D18 is high.  Add a nor-gpios property to the nor node in the
> SabreAuto device-tree and add a function to the imx-weim probe to set
> GPIO5 to drive the pad.
> 
> Signed-off-by: Alison Chaiken <alison_chaiken@mentor.com>
> ---
>  arch/arm/boot/dts/imx6qdl-sabreauto.dtsi |  1 +
>  drivers/bus/imx-weim.c                   | 43 ++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
> index 009abd6..dd5e3bc 100644
> --- a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
> @@ -454,5 +454,6 @@
>  		bank-width = <2>;
>  		fsl,weim-cs-timing = <0x00620081 0x00000001 0x1c022000
>  				0x0000c000 0x1404a38e 0x00000000>;
> +		nor-gpios = <&gpio5 4 GPIO_ACTIVE_LOW>;

Such a binding needs to be documented. Also, if it's going to be named
generically like that, it needs to be generically useful and supported
under MTD code, not platform-specific bus code.

So what pin is this, exactly? A write-protect pin? An address pin? A
toaster control, where the toaster is keeping the flash too hot?

>  	};
>  };
> diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
> index 0958b69..b3c2ca6 100644
> --- a/drivers/bus/imx-weim.c
> +++ b/drivers/bus/imx-weim.c
> @@ -14,6 +14,8 @@
>  #include <linux/mfd/syscon.h>
>  #include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
>  #include <linux/regmap.h>
> +#include <linux/of_gpio.h>
> +#include <linux/gpio.h>
>  
>  struct imx_weim_devtype {
>  	unsigned int	cs_count;
> @@ -108,6 +110,40 @@ err:
>  	return -EINVAL;
>  }
>  
> +/* set the GPIO to control PAD_EIM_D18 so cfi_qry_present() works properly */
> +static int __init nor_gpio_setup(struct device_node *np, struct device *parent)
> +{
> +	unsigned nor_gpio, level;
> +	enum of_gpio_flags of_flags;
> +	int ret;
> +
> +	nor_gpio = of_get_named_gpio_flags(np, "nor-gpios", 0, &of_flags);
> +
> +	/* this child is not a NOR chip */
> +	if (!nor_gpio)
> +		return 0;
> +
> +	if (gpio_is_valid(nor_gpio)) {
> +		ret = devm_gpio_request_one(parent, nor_gpio,
> +					GPIOF_DIR_OUT, "nor-gpio");
> +	} else {
> +		ret = -ENODEV;
> +		goto out;
> +	}
> +
> +	if (ret < 0)
> +		goto out;
> +
> +	level = ((of_flags == OF_GPIO_ACTIVE_LOW) ? 0 : 1);
> +
> +	gpio_set_value(nor_gpio, level);
> +
> +	return 0;
> +out:
> +	dev_err(parent, "Unable to request EIM_D18 GPIO for NOR.\n");
> +	return ret;
> +}
> +
>  /* Parse and set the timing for this device. */
>  static int __init weim_timing_setup(struct device_node *np, void __iomem *base,
>  				    const struct imx_weim_devtype *devtype)
> @@ -160,6 +196,13 @@ static int __init weim_parse_dt(struct platform_device *pdev,
>  				child->full_name);
>  			return ret;
>  		}
> +
> +		ret = nor_gpio_setup(child, &pdev->dev);
> +		if (ret) {
> +			dev_err(&pdev->dev, "%s gpios setup failed.\n",
> +				child->full_name);
> +			return ret;
> +		}
>  	}
>  
>  	ret = of_platform_populate(pdev->dev.of_node,

Brian
Sascha Hauer Jan. 15, 2015, 7:56 a.m. UTC | #2
On Wed, Jan 14, 2015 at 04:03:34PM -0800, Brian Norris wrote:
> + linux-arm-kernel, maintainers, devicetree
> 
> Shawn, Sascha: should this driver be listed under the Freescale IMX
> MAINTAINERS entry?

This driver is purely i.MX, so yes, that would be good.

> 
> On Wed, Jan 14, 2015 at 03:36:49PM -0800, alison@she-devel.com wrote:
> > From: Alison Chaiken <alison_chaiken@mentor.com>
> > 
> > PAD_EIM_D18 must be pulled low at boot in order for the parallel NOR
> > connected to the EIM switch to probe properly.  Otherwise
> > cfi_qry_present() may return "U-V-]" rather than "Q-R-Y" if the
> > PAD_EIM_D18 is high.  Add a nor-gpios property to the nor node in the
> > SabreAuto device-tree and add a function to the imx-weim probe to set
> > GPIO5 to drive the pad.
> > 
> > Signed-off-by: Alison Chaiken <alison_chaiken@mentor.com>
> > ---
> >  arch/arm/boot/dts/imx6qdl-sabreauto.dtsi |  1 +
> >  drivers/bus/imx-weim.c                   | 43 ++++++++++++++++++++++++++++++++
> >  2 files changed, 44 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
> > index 009abd6..dd5e3bc 100644
> > --- a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
> > +++ b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
> > @@ -454,5 +454,6 @@
> >  		bank-width = <2>;
> >  		fsl,weim-cs-timing = <0x00620081 0x00000001 0x1c022000
> >  				0x0000c000 0x1404a38e 0x00000000>;
> > +		nor-gpios = <&gpio5 4 GPIO_ACTIVE_LOW>;
> 
> Such a binding needs to be documented. Also, if it's going to be named
> generically like that, it needs to be generically useful and supported
> under MTD code, not platform-specific bus code.
> 
> So what pin is this, exactly? A write-protect pin? An address pin? A
> toaster control, where the toaster is keeping the flash too hot?

Yeah, the gpio should probably be named after the input pin on the flash
where it's connected to. Otherwise people just use this property for any
GPIO they need to put in a certain direction.

Sascha
diff mbox

Patch

diff --git a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
index 009abd6..dd5e3bc 100644
--- a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
@@ -454,5 +454,6 @@ 
 		bank-width = <2>;
 		fsl,weim-cs-timing = <0x00620081 0x00000001 0x1c022000
 				0x0000c000 0x1404a38e 0x00000000>;
+		nor-gpios = <&gpio5 4 GPIO_ACTIVE_LOW>;
 	};
 };
diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
index 0958b69..b3c2ca6 100644
--- a/drivers/bus/imx-weim.c
+++ b/drivers/bus/imx-weim.c
@@ -14,6 +14,8 @@ 
 #include <linux/mfd/syscon.h>
 #include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
 #include <linux/regmap.h>
+#include <linux/of_gpio.h>
+#include <linux/gpio.h>
 
 struct imx_weim_devtype {
 	unsigned int	cs_count;
@@ -108,6 +110,40 @@  err:
 	return -EINVAL;
 }
 
+/* set the GPIO to control PAD_EIM_D18 so cfi_qry_present() works properly */
+static int __init nor_gpio_setup(struct device_node *np, struct device *parent)
+{
+	unsigned nor_gpio, level;
+	enum of_gpio_flags of_flags;
+	int ret;
+
+	nor_gpio = of_get_named_gpio_flags(np, "nor-gpios", 0, &of_flags);
+
+	/* this child is not a NOR chip */
+	if (!nor_gpio)
+		return 0;
+
+	if (gpio_is_valid(nor_gpio)) {
+		ret = devm_gpio_request_one(parent, nor_gpio,
+					GPIOF_DIR_OUT, "nor-gpio");
+	} else {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	if (ret < 0)
+		goto out;
+
+	level = ((of_flags == OF_GPIO_ACTIVE_LOW) ? 0 : 1);
+
+	gpio_set_value(nor_gpio, level);
+
+	return 0;
+out:
+	dev_err(parent, "Unable to request EIM_D18 GPIO for NOR.\n");
+	return ret;
+}
+
 /* Parse and set the timing for this device. */
 static int __init weim_timing_setup(struct device_node *np, void __iomem *base,
 				    const struct imx_weim_devtype *devtype)
@@ -160,6 +196,13 @@  static int __init weim_parse_dt(struct platform_device *pdev,
 				child->full_name);
 			return ret;
 		}
+
+		ret = nor_gpio_setup(child, &pdev->dev);
+		if (ret) {
+			dev_err(&pdev->dev, "%s gpios setup failed.\n",
+				child->full_name);
+			return ret;
+		}
 	}
 
 	ret = of_platform_populate(pdev->dev.of_node,