Re: [Patch v5 08/13] ARM: imx6q: add config-on-boot gpios

[Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>]

Dear Rob Herring,

> On 06/13/2012 10:19 AM, Marek Vasut wrote:
> > Dear Rob Herring,
> > 
> >> On 06/13/2012 07:34 AM, Richard Zhao wrote:
> >>> Sometimes, boards have gpios that don't own by any driver or owner
> >>> by a generic driver that don't like hacks. Such gpios is normally
> >>> output and need setup once on boot. So I introduce the config-on-boot
> >>> gpios.
> >>> 
> >>> Signed-off-by: Richard Zhao <richard.zhao-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >>> Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>> Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> >>> Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> >>> ---
> >>> 
> >>>  .../devicetree/bindings/arm/config-on-boot.txt     |   12 +++++++
> >>>  arch/arm/boot/dts/imx6q-sabrelite.dts              |    7 ++++
> >>>  arch/arm/mach-imx/mach-imx6q.c                     |   35
> >>>  ++++++++++++++++++++ 3 files changed, 54 insertions(+)
> >>>  create mode 100644
> >>>  Documentation/devicetree/bindings/arm/config-on-boot.txt
> >>> 
> >>> diff --git a/Documentation/devicetree/bindings/arm/config-on-boot.txt
> >>> b/Documentation/devicetree/bindings/arm/config-on-boot.txt new file
> >>> mode 100644
> >>> index 0000000..f98ed74
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/arm/config-on-boot.txt
> >>> @@ -0,0 +1,12 @@
> >>> +* Configure on Boot
> >>> +
> >>> +Node name: config-on-boot
> >>> +  It must be in root node. config-on-boot means to describe settings
> >>> that needs +  to be set one time on boot but aren't owned by any
> >>> driver, or the owned driver +  is too generic to handle such settings.
> >>> For example, usb hub uses generic +  driver in usb core code, a
> >>> on-board usb may need deassert reset pin.
> >> 
> >> NAK. This is not a h/w description and should be solved within the
> >> kernel or bootloader. Either fix this in u-boot
> > 
> > I don't want to be the bastard here, but this shouldn't go into uboot.
> 
> Just listing possible options. Obviously, no one wants the dirty stuff
> in their code...
> 
> There would never be a need in u-boot to support boot from USB mass
> storage?

We already support that obviously ;-)

> In which case you would have to handle this reset line leaving
> it deasserted and linux would likely never know. (Yes, I know we can't
> rely on bootloader setup.)

But we defer the initialization of USB to a point where user really needs it. 
Therefore if you boot from ie. NAND, you won't have the USB inited anyway.

> >> the platform code
> > 
> > Yes, it should be here. But then, it's some usb reset, so maybe the PHY
> > should somehow handle it?
> > 
> >> , or
> >> make the generic driver support this in a generic way.
> > 
> > Well, this is platform specific stuff, so it shouldn't be in any way part
> > of the driver.
> 
> An embedded hub with gpio controlled reset line sounds fairly generic to
> me. The only platform specific part is which gpio line which is nothing
> new to deal with.

And what about embedded hub that has two distinct power GPIOs? And we can go 
forever with this discussion.

> On the other hand, this is really a flaw in the h/w design as whether
> the hub is integrated on board or a separate hub should be transparent
> to s/w.

I don't think so. You might want to minimize the power consumption by disabling 
the hub power completely.

> So just keeping it contained in platform code may be the best
> option.

Agreed

> Rob
> 
> >> Rob
> >> 
> >>> +
> >>> +Optional properties:
> >>> +- output-gpios: Output gpio array that needs to set.
> >>> +- output-gpio-values: This property is required if output-gpios is
> >>> set. +  The value is a array of 0 or 1. Total count eaquals the number
> >>> of gpios. diff --git a/arch/arm/boot/dts/imx6q-sabrelite.dts
> >>> b/arch/arm/boot/dts/imx6q-sabrelite.dts index e0ec929..1dd2261 100644
> >>> --- a/arch/arm/boot/dts/imx6q-sabrelite.dts
> >>> +++ b/arch/arm/boot/dts/imx6q-sabrelite.dts
> >>> @@ -17,6 +17,13 @@
> >>> 
> >>>  	model = "Freescale i.MX6 Quad SABRE Lite Board";
> >>>  	compatible = "fsl,imx6q-sabrelite", "fsl,imx6q";
> >>> 
> >>> +	config-on-boot {
> >>> +		output-gpios = <
> >>> +				&gpio3 22 0>;	/* vbus reset */
> >>> +		output-gpio-values = <
> >>> +				1>;		/* vbus reset */
> >>> +	};
> >>> +
> >>> 
> >>>  	memory {
> >>>  	
> >>>  		reg = <0x10000000 0x40000000>;
> >>>  	
> >>>  	};
> >>> 
> >>> diff --git a/arch/arm/mach-imx/mach-imx6q.c
> >>> b/arch/arm/mach-imx/mach-imx6q.c index b47e98b..577cf19 100644
> >>> --- a/arch/arm/mach-imx/mach-imx6q.c
> >>> +++ b/arch/arm/mach-imx/mach-imx6q.c
> >>> @@ -19,6 +19,7 @@
> >>> 
> >>>  #include <linux/irqdomain.h>
> >>>  #include <linux/of.h>
> >>>  #include <linux/of_address.h>
> >>> 
> >>> +#include <linux/of_gpio.h>
> >>> 
> >>>  #include <linux/of_irq.h>
> >>>  #include <linux/of_platform.h>
> >>>  #include <linux/pinctrl/machine.h>
> >>> 
> >>> @@ -113,6 +114,38 @@ static void __init imx6q_sabrelite_init(void)
> >>> 
> >>>  	imx6q_sabrelite_cko1_setup();
> >>>  
> >>>  }
> >>> 
> >>> +static void __init imx6q_config_on_boot(void)
> >>> +{
> >>> +	struct device_node *np;
> >>> +	struct property *pp;
> >>> +	int cnt, len, i;
> >>> +	int gpio;
> >>> +
> >>> +	np = of_find_node_by_path("/config-on-boot");
> >>> +	if (!np)
> >>> +		return;
> >>> +	cnt = of_gpio_named_count(np, "output-gpios");
> >>> +	pp = of_find_property(np, "output-gpio-values", &len);
> >>> +	if (!pp || cnt != len / sizeof(u32)) {
> >>> +		pr_err("Invalid config-on-boot gpios!\n");
> >>> +		of_node_put(np);
> >>> +		return;
> >>> +	}
> >>> +	for (i = 0; i < cnt; i++) {
> >>> +		gpio = of_get_named_gpio(np, "output-gpios", i);
> >>> +		if (gpio_is_valid(gpio))
> >>> +			gpio_request_one(gpio, GPIOF_OUT_INIT_HIGH,
> >>> +					"config-on-boot");
> >>> +	}
> >>> +
> >>> +	of_node_put(np);
> >>> +}
> >>> +
> >>> +static void __init imx6q_post_populate(void)
> >>> +{
> >>> +	imx6q_config_on_boot();
> >>> +}
> >>> +
> >>> 
> >>>  static void __init imx6q_init_machine(void)
> >>>  {
> >>>  
> >>>  	/*
> >>> 
> >>> @@ -126,6 +159,8 @@ static void __init imx6q_init_machine(void)
> >>> 
> >>>  	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> >>> 
> >>> +	imx6q_post_populate();
> >>> +
> >>> 
> >>>  	imx6q_pm_init();
> >>>  
> >>>  }
> > 
> > Best regards,
> > Marek Vasut
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html