| Message ID | 1382695658-18757-3-git-send-email-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/25/2013 05:07 AM, Tomi Valkeinen wrote: > DISPLAY_SEL_GPIO and DLP_POWER_ON_GPIO are now handled in the .dts file, > so we can remove them from dss-common.c. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > arch/arm/mach-omap2/dss-common.c | 21 --------------------- > 1 file changed, 21 deletions(-) > > diff --git a/arch/arm/mach-omap2/dss-common.c b/arch/arm/mach-omap2/dss-common.c > index bf89eff..cc70cf9 100644 > --- a/arch/arm/mach-omap2/dss-common.c > +++ b/arch/arm/mach-omap2/dss-common.c > @@ -113,9 +113,6 @@ void __init omap4_panda_display_init_of(void) > > /* OMAP4 Blaze display data */ > > -#define DISPLAY_SEL_GPIO 59 /* LCD2/PicoDLP switch */ > -#define DLP_POWER_ON_GPIO 40 > - > static struct panel_dsicm_platform_data dsi1_panel = { > .name = "lcd", > .source = "dsi.0", > @@ -185,26 +182,8 @@ static struct omap_dss_board_info sdp4430_dss_data = { > .default_display_name = "lcd", > }; > > -/* > - * we select LCD2 by default (instead of Pico DLP) by setting DISPLAY_SEL_GPIO. > - * Setting DLP_POWER_ON gpio enables the VDLP_2V5 VDLP_1V8 and VDLP_1V0 rails > - * used by picodlp on the 4430sdp platform. Keep this gpio disabled as LCD2 is > - * selected by default > - */ > void __init omap_4430sdp_display_init_of(void) > { > - int r; > - > - r = gpio_request_one(DISPLAY_SEL_GPIO, GPIOF_OUT_INIT_HIGH, > - "display_sel"); > - if (r) > - pr_err("%s: Could not get display_sel GPIO\n", __func__); > - > - r = gpio_request_one(DLP_POWER_ON_GPIO, GPIOF_OUT_INIT_LOW, > - "DLP POWER ON"); > - if (r) > - pr_err("%s: Could not get DLP POWER ON GPIO\n", __func__); > - > omap_display_init(&sdp4430_dss_data); > > platform_device_register(&sdp4430_lcd_device); > would you not be depending on the weak IO pull done using mux to drive these GPIO pins since the GPIO is not requested and held? Could we not use Documentation/devicetree/bindings/gpio/gpio.txt binding to map to the right GPIO and drive it using the GPIO module?
On 25/10/13 13:18, Nishanth Menon wrote: >> void __init omap_4430sdp_display_init_of(void) >> { >> - int r; >> - >> - r = gpio_request_one(DISPLAY_SEL_GPIO, GPIOF_OUT_INIT_HIGH, >> - "display_sel"); >> - if (r) >> - pr_err("%s: Could not get display_sel GPIO\n", __func__); >> - >> - r = gpio_request_one(DLP_POWER_ON_GPIO, GPIOF_OUT_INIT_LOW, >> - "DLP POWER ON"); >> - if (r) >> - pr_err("%s: Could not get DLP POWER ON GPIO\n", __func__); >> - >> omap_display_init(&sdp4430_dss_data); >> >> platform_device_register(&sdp4430_lcd_device); >> > would you not be depending on the weak IO pull done using mux to drive > these GPIO pins since the GPIO is not requested and held? Yes. Is that not enough? > Could we not use Documentation/devicetree/bindings/gpio/gpio.txt > binding to map to the right GPIO and drive it using the GPIO module? Hmm, what do you mean? I do mux the pins to gpios, but there's nothing in the kernel that would use those gpios. That's why we had the hack above, but I'd love to get rid of it. Can I set the pins to GPIO mode, and set the GPIO to high/low in the .dts? If things were perfect, we probably would have a driver for the "switch" part. I have no idea what kind of driver that would be, though, so at the moment we've just gone with the use-LCD2-by-default route. Tomi
On 10/25/2013 05:25 AM, Tomi Valkeinen wrote: > On 25/10/13 13:18, Nishanth Menon wrote: > >>> void __init omap_4430sdp_display_init_of(void) >>> { >>> - int r; >>> - >>> - r = gpio_request_one(DISPLAY_SEL_GPIO, GPIOF_OUT_INIT_HIGH, >>> - "display_sel"); >>> - if (r) >>> - pr_err("%s: Could not get display_sel GPIO\n", __func__); >>> - >>> - r = gpio_request_one(DLP_POWER_ON_GPIO, GPIOF_OUT_INIT_LOW, >>> - "DLP POWER ON"); >>> - if (r) >>> - pr_err("%s: Could not get DLP POWER ON GPIO\n", __func__); >>> - >>> omap_display_init(&sdp4430_dss_data); >>> >>> platform_device_register(&sdp4430_lcd_device); >>> >> would you not be depending on the weak IO pull done using mux to drive >> these GPIO pins since the GPIO is not requested and held? > > Yes. Is that not enough? It depend on what the signal draw is and io drive strength which varies - original intent of weak pulls were to have a non-active default state which are overriden by GPIOs as needed. Else we would not be having strong pulls here in pads. Typical padmux drive strength for OMAP4460 is around 100uA, min is around 50uA. meanwhile as Documentation/gpio.txt generically states, the buffers driving at 1.8v on OMAP4460 could be around 6mA or upto 8mA. Again, these depend on the specific pin in discussion and Data manual explain is larger detail Lower current is fine if the switch is ok with it and risk for transients are reasonably safe. However, board designs generally assume the stronger GPIO drive strength. >> Could we not use Documentation/devicetree/bindings/gpio/gpio.txt >> binding to map to the right GPIO and drive it using the GPIO module? > > Hmm, what do you mean? > > I do mux the pins to gpios, but there's nothing in the kernel that would > use those gpios. That's why we had the hack above, but I'd love to get > rid of it. > > Can I set the pins to GPIO mode, and set the GPIO to high/low in the .dts? > > If things were perfect, we probably would have a driver for the "switch" > part. I have no idea what kind of driver that would be, though, so at > the moment we've just gone with the use-LCD2-by-default route. I meant you could, in theory provide the gpio numbers and pull directions in dts and allow the init to drive them as needed. Something like: drivers/i2c/busses/i2c-gpio.c as a reference and use of_get_named_gpio/of_get_gpio to pick themup..
On 25/10/13 13:54, Nishanth Menon wrote: >>> would you not be depending on the weak IO pull done using mux to drive >>> these GPIO pins since the GPIO is not requested and held? >> >> Yes. Is that not enough? > > It depend on what the signal draw is and io drive strength which > varies - original intent of weak pulls were to have a non-active > default state which are overriden by GPIOs as needed. Else we would > not be having strong pulls here in pads. > > Typical padmux drive strength for OMAP4460 is around 100uA, min is > around 50uA. meanwhile as Documentation/gpio.txt generically states, > the buffers driving at 1.8v on OMAP4460 could be around 6mA or upto > 8mA. Again, these depend on the specific pin in discussion and Data > manual explain is larger detail > > Lower current is fine if the switch is ok with it and risk for > transients are reasonably safe. However, board designs generally > assume the stronger GPIO drive strength. Ok. Well, it definitely sounds safer to have a proper gpio pull there, then. >>> Could we not use Documentation/devicetree/bindings/gpio/gpio.txt >>> binding to map to the right GPIO and drive it using the GPIO module? >> >> Hmm, what do you mean? >> >> I do mux the pins to gpios, but there's nothing in the kernel that would >> use those gpios. That's why we had the hack above, but I'd love to get >> rid of it. >> >> Can I set the pins to GPIO mode, and set the GPIO to high/low in the .dts? >> >> If things were perfect, we probably would have a driver for the "switch" >> part. I have no idea what kind of driver that would be, though, so at >> the moment we've just gone with the use-LCD2-by-default route. > > I meant you could, in theory provide the gpio numbers and pull > directions in dts and allow the init to drive them as needed. > > Something like: > drivers/i2c/busses/i2c-gpio.c as a reference and use > of_get_named_gpio/of_get_gpio to pick themup.. I'm still not quite following... What init are you referring to? The problem here is that the gpios don't really belong to anyone in the kernel, as we don't have a driver for the switch. Or did you mean that we'd still have the code in dss-common.c, but just get the gpio numbers from the .dts instead? That makes sense, although I'd want to get rid of that code altogether. Should we have support in the gpio-controller to define default values for gpios? I don't think we can rely on the boot loader to set things correctly. Tomi
On 10/25/2013 05:54 AM, Nishanth Menon wrote: > On 10/25/2013 05:25 AM, Tomi Valkeinen wrote: >> On 25/10/13 13:18, Nishanth Menon wrote: >> >>>> void __init omap_4430sdp_display_init_of(void) >>>> { >>>> - int r; >>>> - >>>> - r = gpio_request_one(DISPLAY_SEL_GPIO, GPIOF_OUT_INIT_HIGH, >>>> - "display_sel"); >>>> - if (r) >>>> - pr_err("%s: Could not get display_sel GPIO\n", __func__); >>>> - >>>> - r = gpio_request_one(DLP_POWER_ON_GPIO, GPIOF_OUT_INIT_LOW, >>>> - "DLP POWER ON"); >>>> - if (r) >>>> - pr_err("%s: Could not get DLP POWER ON GPIO\n", __func__); >>>> - >>>> omap_display_init(&sdp4430_dss_data); >>>> >>>> platform_device_register(&sdp4430_lcd_device); >>>> >>> would you not be depending on the weak IO pull done using mux to drive >>> these GPIO pins since the GPIO is not requested and held? >> >> Yes. Is that not enough? > > It depend on what the signal draw is and io drive strength which > varies - original intent of weak pulls were to have a non-active > default state which are overriden by GPIOs as needed. Else we would > not be having strong pulls here in pads. > > Typical padmux drive strength for OMAP4460 is around 100uA, min is > around 50uA. meanwhile as Documentation/gpio.txt generically states, > the buffers driving at 1.8v on OMAP4460 could be around 6mA or upto > 8mA. Again, these depend on the specific pin in discussion and Data > manual explain is larger detail > > Lower current is fine if the switch is ok with it and risk for > transients are reasonably safe. However, board designs generally > assume the stronger GPIO drive strength. > one additional angle before I forget - this is something we do as part of power optimization - to identify pins which are programmed for a pull in non-functional scenario as it has direct impact on idle power numbers. For example patch #3 in this series &omap4_pmx_core { pinctrl-0 ... &lcd2_pins .. } &lcd2_pins lcd2_pins: pinmux_lcd2_pins { + pinctrl-single,pins = < + 0x20 (PIN_OUTPUT_PULLDOWN | MUX_MODE3) /* gpio_40 */ + 0x46 (PIN_OUTPUT_PULLUP | MUX_MODE3) /* gpio_59 */ + 0x56 (PIN_OUTPUT_PULLDOWN | MUX_MODE3) /* gpio_104 */ + >; 3 pins are driven around 300uA at boot, even with display OFF -> which means wasted current that could have been optimized by hooking the pin to the dts node corresponding to the device and used by the driver appropriately. Unfortunately, folks feel simplifying the driver is traditionally a better alternative but with a 400 odd pins on a typical SoC of today, these defaults add up and end user tends to suffer with bad overall power numbers :(..
On 25/10/13 14:14, Nishanth Menon wrote: > one additional angle before I forget - this is something we do as part > of power optimization - to identify pins which are programmed for a > pull in non-functional scenario as it has direct impact on idle power > numbers. > > For example patch #3 in this series > &omap4_pmx_core { > pinctrl-0 > ... > &lcd2_pins > .. > } > &lcd2_pins > > lcd2_pins: pinmux_lcd2_pins { > + pinctrl-single,pins = < > + 0x20 (PIN_OUTPUT_PULLDOWN | MUX_MODE3) /* gpio_40 */ > + 0x46 (PIN_OUTPUT_PULLUP | MUX_MODE3) /* gpio_59 */ > + 0x56 (PIN_OUTPUT_PULLDOWN | MUX_MODE3) /* gpio_104 */ > + >; > > 3 pins are driven around 300uA at boot, even with display OFF -> which > means wasted current that could have been optimized by hooking the pin > to the dts node corresponding to the device and used by the driver > appropriately. > > Unfortunately, folks feel simplifying the driver is traditionally a > better alternative but with a 400 odd pins on a typical SoC of today, > these defaults add up and end user tends to suffer with bad overall > power numbers :(.. Good point. I guess that also makes my point of having default values for GPIOs a bit silly. Driving the GPIO high by default would be ever worse than the mux pull-up, I believe. Tomi
On 10/25/2013 06:13 AM, Tomi Valkeinen wrote: > On 25/10/13 13:54, Nishanth Menon wrote: [..] > >>>> Could we not use Documentation/devicetree/bindings/gpio/gpio.txt >>>> binding to map to the right GPIO and drive it using the GPIO module? >>> >>> Hmm, what do you mean? >>> >>> I do mux the pins to gpios, but there's nothing in the kernel that would >>> use those gpios. That's why we had the hack above, but I'd love to get >>> rid of it. >>> >>> Can I set the pins to GPIO mode, and set the GPIO to high/low in the .dts? >>> >>> If things were perfect, we probably would have a driver for the "switch" >>> part. I have no idea what kind of driver that would be, though, so at >>> the moment we've just gone with the use-LCD2-by-default route. >> >> I meant you could, in theory provide the gpio numbers and pull >> directions in dts and allow the init to drive them as needed. >> >> Something like: >> drivers/i2c/busses/i2c-gpio.c as a reference and use >> of_get_named_gpio/of_get_gpio to pick themup.. > > I'm still not quite following... What init are you referring to? > > The problem here is that the gpios don't really belong to anyone in the > kernel, as we don't have a driver for the switch. > > Or did you mean that we'd still have the code in dss-common.c, but just > get the gpio numbers from the .dts instead? That makes sense, although > I'd want to get rid of that code altogether. > > Should we have support in the gpio-controller to define default values > for gpios? I don't think we can rely on the boot loader to set things > correctly. > I am unfortunately, not in a position to know how you plan to architect dss_common or the various panels associated with it. if you model these as panels and a generic driver which controls the panel could request and control pins and gpios as needed I suppose. gpio controller cannot drive default pull direction, that is the job of the driver using the gpio. Simplest example that I can think of to use to point as reference is [1] [1] http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/leds/leds-gpio.c#n173
On 25/10/13 14:21, Nishanth Menon wrote: >> The problem here is that the gpios don't really belong to anyone in the >> kernel, as we don't have a driver for the switch. >> >> Or did you mean that we'd still have the code in dss-common.c, but just >> get the gpio numbers from the .dts instead? That makes sense, although >> I'd want to get rid of that code altogether. >> >> Should we have support in the gpio-controller to define default values >> for gpios? I don't think we can rely on the boot loader to set things >> correctly. >> > > I am unfortunately, not in a position to know how you plan to > architect dss_common or the various panels associated with it. if you The dss-common.c is just a hack, needed during the transition to DT. It's supposed to go away as soon as we have proper DT support for DSS. In this case it has just been a convenient place to set the gpios at boot time. The gpios are not touched after that. > model these as panels and a generic driver which controls the panel > could request and control pins and gpios as needed I suppose. > > gpio controller cannot drive default pull direction, that is the job > of the driver using the gpio. I agree. But what to do when there is no driver that uses the gpio, but the gpio still affects the drivers? That's more or less the situation here. The SDP board has an LCD and a PicoDLP projector, and the board designers have shared resources between those, meaning only one can be used at a time. Having the GPIO pulled down means that LCD2 won't have backlight (although the gpio doesn't actually enable the backlight, it just handles the routing if I'm not mistaken) , but PicoDLP will have something (parallel video datalines, if I recall right). I can't add that GPIO to either the LCD driver or the PicoDLP driver, as it's not a property of either of them. Tomi
On 25/10/13 14:14, Nishanth Menon wrote: > lcd2_pins: pinmux_lcd2_pins { > + pinctrl-single,pins = < > + 0x20 (PIN_OUTPUT_PULLDOWN | MUX_MODE3) /* gpio_40 */ > + 0x46 (PIN_OUTPUT_PULLUP | MUX_MODE3) /* gpio_59 */ > + 0x56 (PIN_OUTPUT_PULLDOWN | MUX_MODE3) /* gpio_104 */ > + >; > > 3 pins are driven around 300uA at boot, even with display OFF -> which > means wasted current that could have been optimized by hooking the pin > to the dts node corresponding to the device and used by the driver > appropriately. One more clarification question. The gpio 40 is used to enable powers for the picodlp. Shouldn't that one have a pinctrl pull-down in any case? If it's left floating, and the driver is not compiled in or doesn't start, the powers could get enabled depending on sunspot, right? And I guess the same goes for all gpios used to enable something. Tomi
On 10/25/2013 06:46 AM, Tomi Valkeinen wrote: > On 25/10/13 14:14, Nishanth Menon wrote: > >> lcd2_pins: pinmux_lcd2_pins { >> + pinctrl-single,pins = < >> + 0x20 (PIN_OUTPUT_PULLDOWN | MUX_MODE3) /* gpio_40 */ >> + 0x46 (PIN_OUTPUT_PULLUP | MUX_MODE3) /* gpio_59 */ >> + 0x56 (PIN_OUTPUT_PULLDOWN | MUX_MODE3) /* gpio_104 */ >> + >; >> >> 3 pins are driven around 300uA at boot, even with display OFF -> which >> means wasted current that could have been optimized by hooking the pin >> to the dts node corresponding to the device and used by the driver >> appropriately. > > One more clarification question. > > The gpio 40 is used to enable powers for the picodlp. Shouldn't that one > have a pinctrl pull-down in any case? If it's left floating, and the > driver is not compiled in or doesn't start, the powers could get enabled > depending on sunspot, right? > > And I guess the same goes for all gpios used to enable something. > Fair question. The selection of pull up, gpio control needs to be balanced. if the peripheral in question has a regulator controlled supply, none of the pins would matter - driver can adequately sequence this to ensure there are no weird side-effects. in such a scenario, i'd have a default MODE3 with no pulls, sequence as follows: a) control gpio to required default level (disabled) b) control regulator c) set GPIO to enable. if the peripheral in question is always on and controlled with just a enable pin, it is safer to keep the pin muxed with weak pull. If the enable has no real functional impact without setting another pin (say power_on), or if any transient glitches on the line has no functional impact, I might go with no pull configuration. It all depends on the schematics and peripherals involved w.r.t how you'd optimally select the configuration.
diff --git a/arch/arm/mach-omap2/dss-common.c b/arch/arm/mach-omap2/dss-common.c index bf89eff..cc70cf9 100644 --- a/arch/arm/mach-omap2/dss-common.c +++ b/arch/arm/mach-omap2/dss-common.c @@ -113,9 +113,6 @@ void __init omap4_panda_display_init_of(void) /* OMAP4 Blaze display data */ -#define DISPLAY_SEL_GPIO 59 /* LCD2/PicoDLP switch */ -#define DLP_POWER_ON_GPIO 40 - static struct panel_dsicm_platform_data dsi1_panel = { .name = "lcd", .source = "dsi.0", @@ -185,26 +182,8 @@ static struct omap_dss_board_info sdp4430_dss_data = { .default_display_name = "lcd", }; -/* - * we select LCD2 by default (instead of Pico DLP) by setting DISPLAY_SEL_GPIO. - * Setting DLP_POWER_ON gpio enables the VDLP_2V5 VDLP_1V8 and VDLP_1V0 rails - * used by picodlp on the 4430sdp platform. Keep this gpio disabled as LCD2 is - * selected by default - */ void __init omap_4430sdp_display_init_of(void) { - int r; - - r = gpio_request_one(DISPLAY_SEL_GPIO, GPIOF_OUT_INIT_HIGH, - "display_sel"); - if (r) - pr_err("%s: Could not get display_sel GPIO\n", __func__); - - r = gpio_request_one(DLP_POWER_ON_GPIO, GPIOF_OUT_INIT_LOW, - "DLP POWER ON"); - if (r) - pr_err("%s: Could not get DLP POWER ON GPIO\n", __func__); - omap_display_init(&sdp4430_dss_data); platform_device_register(&sdp4430_lcd_device);
DISPLAY_SEL_GPIO and DLP_POWER_ON_GPIO are now handled in the .dts file, so we can remove them from dss-common.c. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- arch/arm/mach-omap2/dss-common.c | 21 --------------------- 1 file changed, 21 deletions(-)