| Message ID | 1337592237-5090-8-git-send-email-richard.zhao@freescale.com |
|---|---|
| State | New |
| Headers | show |
Apart from my comments below, we really need to know Rob and Grant's opinion on this. On Mon, May 21, 2012 at 05:23:52PM +0800, 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@freescale.com> > Cc: Shawn Guo <shawn.guo@linaro.org> > Cc: Rob Herring <rob.herring@calxeda.com> > Cc: Grant Likely <grant.likely@secretlab.ca> > --- > arch/arm/boot/dts/imx6q-sabrelite.dts | 7 ++++++ > arch/arm/mach-imx/mach-imx6q.c | 35 +++++++++++++++++++++++++++++++++ > 2 files changed, 42 insertions(+), 0 deletions(-) > > 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 */ > + }; > + So it looks like something not specific to imx board but generic to other boards. If so, we may need to come up with a generic binding document for this. Is this only used to configure output pin? Will there be any pin that needs to be configured as input but not owned by any driver? Hopefully not. > 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) Do you intend to make this function non-gpio-config only. Otherwise, we may need to have "gpio" in the function name, as well as the "config-on-boot" node name. Also, if we define the binding as generic one, we may need to find a common place to implement the function. > +{ > + 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"); So all the gpios will use the same name. And doesn't the reset generally need a pulse or edge signal, e.g. pull down for a msec and pull up? Regards, Shawn > + } > + > + 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(); > } > > -- > 1.7.5.4 > >
Dear Shawn Guo, > Apart from my comments below, we really need to know Rob and Grant's > opinion on this. Hm ... so why don't you actually write per-board driver ? :-) That'd handle every single board very well, you'd configure the GPIOs there, every single additional necessary thing that has to be configured would sink there too etc. > On Mon, May 21, 2012 at 05:23:52PM +0800, 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@freescale.com> > > Cc: Shawn Guo <shawn.guo@linaro.org> > > Cc: Rob Herring <rob.herring@calxeda.com> > > Cc: Grant Likely <grant.likely@secretlab.ca> > > --- > > > > arch/arm/boot/dts/imx6q-sabrelite.dts | 7 ++++++ > > arch/arm/mach-imx/mach-imx6q.c | 35 > > +++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 0 > > deletions(-) > > > > 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 */ > > + }; > > + > > So it looks like something not specific to imx board but generic to > other boards. If so, we may need to come up with a generic binding > document for this. > > Is this only used to configure output pin? Will there be any pin that > needs to be configured as input but not owned by any driver? Hopefully > not. > > > 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) > > Do you intend to make this function non-gpio-config only. Otherwise, > we may need to have "gpio" in the function name, as well as the > "config-on-boot" node name. > > Also, if we define the binding as generic one, we may need to find a > common place to implement the function. > > > +{ > > + 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"); > > So all the gpios will use the same name. > > And doesn't the reset generally need a pulse or edge signal, e.g. pull > down for a msec and pull up? > > Regards, > Shawn > > > + } > > + > > + 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
On 22 May 2012 11:25, Marek Vasut <marex@denx.de> wrote: > Dear Shawn Guo, > >> Apart from my comments below, we really need to know Rob and Grant's >> opinion on this. > > Hm ... so why don't you actually write per-board driver ? :-) That'd handle > every single board very well, you'd configure the GPIOs there, every single > additional necessary thing that has to be configured would sink there too etc. > I'm not completely sure about how the per-board driver looks like exactly. Can you have some code demonstrating it? Regards, Shawn
On Tue, May 22, 2012 at 11:17:12AM +0800, Shawn Guo wrote: > Apart from my comments below, we really need to know Rob and Grant's > opinion on this. Exactly. I cced them to draw their attention. > > On Mon, May 21, 2012 at 05:23:52PM +0800, 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@freescale.com> > > Cc: Shawn Guo <shawn.guo@linaro.org> > > Cc: Rob Herring <rob.herring@calxeda.com> > > Cc: Grant Likely <grant.likely@secretlab.ca> > > --- > > arch/arm/boot/dts/imx6q-sabrelite.dts | 7 ++++++ > > arch/arm/mach-imx/mach-imx6q.c | 35 +++++++++++++++++++++++++++++++++ > > 2 files changed, 42 insertions(+), 0 deletions(-) > > > > 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 */ > > + }; > > + > > So it looks like something not specific to imx board but generic to > other boards. If so, we may need to come up with a generic binding > document for this. Yes. > > Is this only used to configure output pin? Will there be any pin that > needs to be configured as input but not owned by any driver? Hopefully > not. I didn't see the case yet. Input pin normally has a corresponding driver. > > > 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) > > Do you intend to make this function non-gpio-config only. Otherwise, > we may need to have "gpio" in the function name, as well as the > "config-on-boot" node name. config-on-boot is not specific for gpio, but may be for others. For example, regulators. For now, it only has output gpios. > > Also, if we define the binding as generic one, we may need to find a > common place to implement the function. Yes. But it's better be called at machine init after populate devices. > > > +{ > > + 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"); > > So all the gpios will use the same name. Yes. > > And doesn't the reset generally need a pulse or edge signal, e.g. pull > down for a msec and pull up? strict reset is too complicated for config-on-boot. It may need delay some time case by case. I don't have the case yet. But someone may add it if it really needed, probably with addon properties. Thanks Richard > > Regards, > Shawn > > > + } > > + > > + 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(); > > } > > > > -- > > 1.7.5.4 > > > > >
Dear Shawn Guo, > On 22 May 2012 11:25, Marek Vasut <marex@denx.de> wrote: > > Dear Shawn Guo, > > > >> Apart from my comments below, we really need to know Rob and Grant's > >> opinion on this. > > > > Hm ... so why don't you actually write per-board driver ? :-) That'd > > handle every single board very well, you'd configure the GPIOs there, > > every single additional necessary thing that has to be configured would > > sink there too etc. > > I'm not completely sure about how the per-board driver looks like > exactly. Can you have some code demonstrating it? Well such per-board driver would handle the board-specific init, like configuring the hub GPIO (as Richard has to do so on mx6q) or flick some other GPIOs, configure registers etc. You can not depend on a bootloader to do such init for you, even though the bootloader should do it and should do it right. > > Regards, > Shawn Best regards, Marek Vasut
On Tue, May 22, 2012 at 06:20:16AM +0200, Marek Vasut wrote: > Dear Shawn Guo, > > > On 22 May 2012 11:25, Marek Vasut <marex@denx.de> wrote: > > > Dear Shawn Guo, > > > > > >> Apart from my comments below, we really need to know Rob and Grant's > > >> opinion on this. > > > > > > Hm ... so why don't you actually write per-board driver ? :-) That'd > > > handle every single board very well, you'd configure the GPIOs there, > > > every single additional necessary thing that has to be configured would > > > sink there too etc. > > > > I'm not completely sure about how the per-board driver looks like > > exactly. Can you have some code demonstrating it? > > Well such per-board driver would handle the board-specific init, like > configuring the hub GPIO (as Richard has to do so on mx6q) or flick some other > GPIOs, configure registers etc. > So when you say driver, you just meant a C file like imx6q-sabrelite.c containing a number of init functions to be called by mach-imx6q.c?
Dear Shawn Guo, > On Tue, May 22, 2012 at 06:20:16AM +0200, Marek Vasut wrote: > > Dear Shawn Guo, > > > > > On 22 May 2012 11:25, Marek Vasut <marex@denx.de> wrote: > > > > Dear Shawn Guo, > > > > > > > >> Apart from my comments below, we really need to know Rob and Grant's > > > >> opinion on this. > > > > > > > > Hm ... so why don't you actually write per-board driver ? :-) That'd > > > > handle every single board very well, you'd configure the GPIOs there, > > > > every single additional necessary thing that has to be configured > > > > would sink there too etc. > > > > > > I'm not completely sure about how the per-board driver looks like > > > exactly. Can you have some code demonstrating it? > > > > Well such per-board driver would handle the board-specific init, like > > configuring the hub GPIO (as Richard has to do so on mx6q) or flick some > > other GPIOs, configure registers etc. > > So when you say driver, you just meant a C file like imx6q-sabrelite.c > containing a number of init functions to be called by mach-imx6q.c? Something like that ... but didn't Linus complain that we have too many files in arch/arm/ ? Best regards, Marek Vasut
On 22 May 2012 13:22, Marek Vasut <marex@denx.de> wrote: > Something like that ... but didn't Linus complain that we have too many files in > arch/arm/ ? So what do you exactly mean by per-board driver? Regards, Shawn
Dear Shawn Guo, > On 22 May 2012 13:22, Marek Vasut <marex@denx.de> wrote: > > Something like that ... but didn't Linus complain that we have too many > > files in arch/arm/ ? > > So what do you exactly mean by per-board driver? Exactly such fixup as there's for mx6q-sabrelite. But then, if you have too broken bootloader, you'll need such a fixup for every such board. Or do you want to let mach-imx6q.c grow in size with each broken board? > Regards, > Shawn Best regards, Marek Vasut
On Tue, May 22, 2012 at 07:27:30AM +0200, Marek Vasut wrote: > Dear Shawn Guo, > > > On 22 May 2012 13:22, Marek Vasut <marex@denx.de> wrote: > > > Something like that ... but didn't Linus complain that we have too many > > > files in arch/arm/ ? > > > > So what do you exactly mean by per-board driver? > > Exactly such fixup as there's for mx6q-sabrelite. But then, if you have too > broken bootloader, you'll need such a fixup for every such board. Or do you want > to let mach-imx6q.c grow in size with each broken board? As Shawn said, this patch is trying to abstract common things. We don't need to repeat the code that interpret output-gpios. Generally discuss often draw no conclusion. So let's talk about code. Thanks Richard > > > Regards, > > Shawn > > Best regards, > Marek Vasut >
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(); }
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@freescale.com> Cc: Shawn Guo <shawn.guo@linaro.org> Cc: Rob Herring <rob.herring@calxeda.com> Cc: Grant Likely <grant.likely@secretlab.ca> --- arch/arm/boot/dts/imx6q-sabrelite.dts | 7 ++++++ arch/arm/mach-imx/mach-imx6q.c | 35 +++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 0 deletions(-)