| Message ID | 20230704050744.1196293-1-yangcong5@huaqin.corp-partner.google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panel: Add avdd/avee delay for Starry-himax83102-j02 and Starry-ili9882t panel | expand |
Hi, On Mon, Jul 3, 2023 at 10:07 PM Cong Yang <yangcong5@huaqin.corp-partner.google.com> wrote: > > From power on/off sequence for panel data sheet[1], T2 timing VSP to VSN > needs 1ms delay when power on, and VSN to VSP also needs 1ms delay when > power off. Some pmic may not be able to adjust the delay internally, so > let's add a delay between avdd/avee regulator gpio to meet the timing of > panel. Unless I'm mistaken, all of this is best handled via regulator constraints in the device tree. See the file: Documentation/devicetree/bindings/regulator/regulator.yaml Specifically, any delays related to actually ramping up / down the regulator can be specified in the device tree. Nominally, you could argue that the 1 ms delay actually _does_ belong in the driver, but IMO the 1 ms number there is really just there because someone thought it was weird to specify a delay of 0 ms. Given that you already need remp delays in the device tree, it feels OK to me to just include the 1 ms there. -Doug
Hi, On Fri, Jul 7, 2023 at 3:32 AM Doug Anderson <dianders@google.com> wrote: > > Hi, > > On Mon, Jul 3, 2023 at 10:07 PM Cong Yang > <yangcong5@huaqin.corp-partner.google.com> wrote: > > > > From power on/off sequence for panel data sheet[1], T2 timing VSP to VSN > > needs 1ms delay when power on, and VSN to VSP also needs 1ms delay when > > power off. Some pmic may not be able to adjust the delay internally, so > > let's add a delay between avdd/avee regulator gpio to meet the timing of > > panel. > > Unless I'm mistaken, all of this is best handled via regulator > constraints in the device tree. See the file: > > Documentation/devicetree/bindings/regulator/regulator.yaml > > Specifically, any delays related to actually ramping up / down the > regulator can be specified in the device tree. Nominally, you could > argue that the 1 ms delay actually _does_ belong in the driver, but > IMO the 1 ms number there is really just there because someone thought > it was weird to specify a delay of 0 ms. Given that you already need > remp delays in the device tree, it feels OK to me to just include the > 1 ms there. The regulator device tree has only the power on attribute "regulator-enable-ramp-delay", not has power off attribute. The regulator delay looks more like the HW voltage requirement of the power ic itself, and I just want to meet the panel spec requirement. I add regulator-enable-ramp-delay in dts he can also meet my requirement, but I have no way to control the power off delays. Thanks Doug. > > -Doug
Hi, On Thu, Jul 6, 2023 at 6:20 PM cong yang <yangcong5@huaqin.corp-partner.google.com> wrote: > > Hi, > > On Fri, Jul 7, 2023 at 3:32 AM Doug Anderson <dianders@google.com> wrote: > > > > Hi, > > > > On Mon, Jul 3, 2023 at 10:07 PM Cong Yang > > <yangcong5@huaqin.corp-partner.google.com> wrote: > > > > > > From power on/off sequence for panel data sheet[1], T2 timing VSP to VSN > > > needs 1ms delay when power on, and VSN to VSP also needs 1ms delay when > > > power off. Some pmic may not be able to adjust the delay internally, so > > > let's add a delay between avdd/avee regulator gpio to meet the timing of > > > panel. > > > > Unless I'm mistaken, all of this is best handled via regulator > > constraints in the device tree. See the file: > > > > Documentation/devicetree/bindings/regulator/regulator.yaml > > > > Specifically, any delays related to actually ramping up / down the > > regulator can be specified in the device tree. Nominally, you could > > argue that the 1 ms delay actually _does_ belong in the driver, but > > IMO the 1 ms number there is really just there because someone thought > > it was weird to specify a delay of 0 ms. Given that you already need > > remp delays in the device tree, it feels OK to me to just include the > > 1 ms there. > > The regulator device tree has only the power on attribute > "regulator-enable-ramp-delay", > not has power off attribute. The regulator delay looks more like the > HW voltage requirement > of the power ic itself, and I just want to meet the panel spec > requirement. I add regulator-enable-ramp-delay > in dts he can also meet my requirement, but I have no way to control > the power off delays. Hmmm, I guess the fact that the delay needed can be different for different boards / PMICs still makes me think that the delay doesn't belong in the panel driver. Different boards using the same panel would need different delays, right? So, thinking more... You're saying that you _can_ specify the enable delay in the device tree, but not the disable one, right? However, the timing diagram you provided doesn't seem to show the "disable" part. Since that's the part we're talking about now, could you provide a more complete timing diagram? Can you also talk to the panel vendor and confirm that the "1 ms" actually matters or if they just put that there to ensure ordering? In other words, is it simply important that VDD1 gets to ~90% before you turn on VSP, or do they truly need a full 1 ms delay? Can you provide any more details about the power IC you're using? Is it just a discrete PMIC with a GPIO enable, or is it something fancier? Correct me if I'm confused (entirely possible!), but I think some PMICs have a feature where they can turn on "active discharge" so that they ramp down more quickly when they're disabled. Any chance your PMIC has this? In general the fact that nobody has added "regulator-disable-ramp-delay" to the regulator framework already means that the problem you're facing isn't really a common problem. There are lots of devices out there that have more than one regulator but I don't see examples where drivers need to delay between turning all their regulators off. Are you positive that this is something that you really need to worry about? The above is a bit rambling (sorry!), but I guess the summary is: 1. Please confirm that the panel driver truly needs 1 ms between regulators enabled. 2. Please provide the power sequence diagram for disable. If there's a 1 ms delay between regulators being disabled then please confirm. 3. If the 1 ms delay isn't truly needed then we can just drop this patch, right? 4. IMO if the panel itself truly requires 1 ms between regulators being enabled and/or disabled, it would be OK to put the 1 ms delay in the driver but it feels wrong to be accounting for ramp time in the driver. This should be specified in the device tree. 5. If we really need to account for the ramp down time, it would at least be good to submit a regulator framework patch proposing a way to specify this. We'd have to figure out how to make this work since I'd imagine that most regulator consumers don't care that much about ramp down time. Mark would be the real person to get advice from, but perhaps an API call like "regulator_wait_discharged(percent)" that a client could call? -Doug
Hi, On Fri, Jul 7, 2023 at 11:15 PM Doug Anderson <dianders@google.com> wrote: > > Hi, > > On Thu, Jul 6, 2023 at 6:20 PM cong yang > <yangcong5@huaqin.corp-partner.google.com> wrote: > > > > Hi, > > > > On Fri, Jul 7, 2023 at 3:32 AM Doug Anderson <dianders@google.com> wrote: > > > > > > Hi, > > > > > > On Mon, Jul 3, 2023 at 10:07 PM Cong Yang > > > <yangcong5@huaqin.corp-partner.google.com> wrote: > > > > > > > > From power on/off sequence for panel data sheet[1], T2 timing VSP to VSN > > > > needs 1ms delay when power on, and VSN to VSP also needs 1ms delay when > > > > power off. Some pmic may not be able to adjust the delay internally, so > > > > let's add a delay between avdd/avee regulator gpio to meet the timing of > > > > panel. > > > > > > Unless I'm mistaken, all of this is best handled via regulator > > > constraints in the device tree. See the file: > > > > > > Documentation/devicetree/bindings/regulator/regulator.yaml > > > > > > Specifically, any delays related to actually ramping up / down the > > > regulator can be specified in the device tree. Nominally, you could > > > argue that the 1 ms delay actually _does_ belong in the driver, but > > > IMO the 1 ms number there is really just there because someone thought > > > it was weird to specify a delay of 0 ms. Given that you already need > > > remp delays in the device tree, it feels OK to me to just include the > > > 1 ms there. > > > > The regulator device tree has only the power on attribute > > "regulator-enable-ramp-delay", > > not has power off attribute. The regulator delay looks more like the > > HW voltage requirement > > of the power ic itself, and I just want to meet the panel spec > > requirement. I add regulator-enable-ramp-delay > > in dts he can also meet my requirement, but I have no way to control > > the power off delays. > > Hmmm, I guess the fact that the delay needed can be different for > different boards / PMICs still makes me think that the delay doesn't > belong in the panel driver. Different boards using the same panel > would need different delays, right? > > So, thinking more... > > You're saying that you _can_ specify the enable delay in the device > tree, but not the disable one, right? However, the timing diagram you > provided doesn't seem to show the "disable" part. Since that's the > part we're talking about now, could you provide a more complete timing > diagram? Can you also talk to the panel vendor and confirm that the "1 > ms" actually matters or if they just put that there to ensure > ordering? In other words, is it simply important that VDD1 gets to > ~90% before you turn on VSP, or do they truly need a full 1 ms delay? > > Can you provide any more details about the power IC you're using? Is > it just a discrete PMIC with a GPIO enable, or is it something > fancier? Correct me if I'm confused (entirely possible!), but I think > some PMICs have a feature where they can turn on "active discharge" so > that they ramp down more quickly when they're disabled. Any chance > your PMIC has this? > > In general the fact that nobody has added > "regulator-disable-ramp-delay" to the regulator framework already > means that the problem you're facing isn't really a common problem. > There are lots of devices out there that have more than one regulator > but I don't see examples where drivers need to delay between turning > all their regulators off. Are you positive that this is something that > you really need to worry about? > > The above is a bit rambling (sorry!), but I guess the summary is: > > 1. Please confirm that the panel driver truly needs 1 ms between > regulators enabled. > > 2. Please provide the power sequence diagram for disable. If there's a > 1 ms delay between regulators being disabled then please confirm. > > 3. If the 1 ms delay isn't truly needed then we can just drop this patch, right? https://github.com/ILITEK-LoganLin/Document/tree/main/ILITEK_Power_Sequence Ask the vendor to evaluate this 1ms delay again, they think that current ramp time does not need 1ms delay, so drop this patch. > > 4. IMO if the panel itself truly requires 1 ms between regulators > being enabled and/or disabled, it would be OK to put the 1 ms delay in > the driver but it feels wrong to be accounting for ramp time in the > driver. This should be specified in the device tree. > > 5. If we really need to account for the ramp down time, it would at > least be good to submit a regulator framework patch proposing a way to > specify this. We'd have to figure out how to make this work since I'd > imagine that most regulator consumers don't care that much about ramp > down time. Mark would be the real person to get advice from, but > perhaps an API call like "regulator_wait_discharged(percent)" that a > client could call? > > > -Doug
diff --git a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c index dc276c346fd1..b44a6871d97c 100644 --- a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c +++ b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c @@ -37,6 +37,7 @@ struct panel_desc { unsigned int lanes; bool discharge_on_disable; bool lp11_before_reset; + int avdd_avee_delay; }; struct boe_panel { @@ -1798,6 +1799,7 @@ static int boe_panel_unprepare(struct drm_panel *panel) if (boe->desc->discharge_on_disable) { regulator_disable(boe->avee); + usleep_range(boe->desc->avdd_avee_delay, boe->desc->avdd_avee_delay * 2); regulator_disable(boe->avdd); usleep_range(5000, 7000); gpiod_set_value(boe->enable_gpio, 0); @@ -1808,6 +1810,7 @@ static int boe_panel_unprepare(struct drm_panel *panel) gpiod_set_value(boe->enable_gpio, 0); usleep_range(1000, 2000); regulator_disable(boe->avee); + usleep_range(boe->desc->avdd_avee_delay, boe->desc->avdd_avee_delay * 2); regulator_disable(boe->avdd); usleep_range(5000, 7000); regulator_disable(boe->pp1800); @@ -1843,6 +1846,7 @@ static int boe_panel_prepare(struct drm_panel *panel) ret = regulator_enable(boe->avdd); if (ret < 0) goto poweroff1v8; + usleep_range(boe->desc->avdd_avee_delay, boe->desc->avdd_avee_delay * 2); ret = regulator_enable(boe->avee); if (ret < 0) goto poweroffavdd; @@ -2134,6 +2138,7 @@ static const struct panel_desc starry_himax83102_j02_desc = { MIPI_DSI_MODE_LPM, .init_cmds = starry_himax83102_j02_init_cmd, .lp11_before_reset = true, + .avdd_avee_delay = 1500, }; static const struct drm_display_mode starry_ili9882t_default_mode = { @@ -2162,6 +2167,7 @@ static const struct panel_desc starry_ili9882t_desc = { MIPI_DSI_MODE_LPM, .init_cmds = starry_ili9882t_init_cmd, .lp11_before_reset = true, + .avdd_avee_delay = 1500, }; static int boe_panel_get_modes(struct drm_panel *panel,
From power on/off sequence for panel data sheet[1], T2 timing VSP to VSN needs 1ms delay when power on, and VSN to VSP also needs 1ms delay when power off. Some pmic may not be able to adjust the delay internally, so let's add a delay between avdd/avee regulator gpio to meet the timing of panel. [1]: https://github.com/HimaxSoftware/Doc/tree/main/Himax_Chipset_Power_Sequence Fixes: 59bba51ec2a5 ("drm/panel: Fine tune Starry-ili9882t panel HFP and HBP") Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com> --- drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c | 6 ++++++ 1 file changed, 6 insertions(+)