| Message ID | 20220429204519.241549-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/stm: dsi: Enable wrapper glue regulator early | expand |
Hi Marek, Thanks for your patch On 4/29/22 22:45, Marek Vasut wrote: > Certain DSI bridge chips like TC358767/TC358867/TC9595 expect the DSI D0 > data lane to be in LP-11 state when released from reset. Currently the > STM32MP157 DSI host wrapper glue logic keeps D0 data lane in LP-00 state > until DSI init happens, which confuses the TC358767 into entering some > sort of test mode and the chip cannot be brought out of this test mode > in any way. > > Enable the wrapper glue logic regulator in probe callback already and > disable it in remove callback to satisfy this requirement. The D0 data > lane is in LP-11 mode when the TC358767 bridge chip is brought up and > the chip is not confused anymore. > > Signed-off-by: Marek Vasut <marex@denx.de> > Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> > Cc: Antonio Borneo <antonio.borneo@foss.st.com> > Cc: Philippe Cornu <philippe.cornu@foss.st.com> > Cc: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com> > Cc: Robert Foss <robert.foss@linaro.org> > Cc: Yannick Fertre <yannick.fertre@foss.st.com> > --- > drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 30 +++++++++++++++++++++++---- > 1 file changed, 26 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c > index 89897d5f5c72..c403633ffeae 100644 > --- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c > +++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c > @@ -194,16 +194,29 @@ static int dsi_pll_get_params(struct dw_mipi_dsi_stm *dsi, > return 0; > } > > +static int dw_mipi_dsi_phy_regulator_on(struct dw_mipi_dsi_stm *dsi) > +{ > + u32 val; > + > + /* Enable the regulator */ > + dsi_set(dsi, DSI_WRPCR, WRPCR_REGEN | WRPCR_BGREN); > + return readl_poll_timeout(dsi->base + DSI_WISR, val, val & WISR_RRS, > + SLEEP_US, TIMEOUT_US); > +} > + > +static void dw_mipi_dsi_phy_regulator_off(struct dw_mipi_dsi_stm *dsi) > +{ > + /* Disable the regulator */ > + dsi_clear(dsi, DSI_WRPCR, WRPCR_REGEN | WRPCR_BGREN); > +} > + > static int dw_mipi_dsi_phy_init(void *priv_data) > { > struct dw_mipi_dsi_stm *dsi = priv_data; > u32 val; > int ret; > > - /* Enable the regulator */ > - dsi_set(dsi, DSI_WRPCR, WRPCR_REGEN | WRPCR_BGREN); > - ret = readl_poll_timeout(dsi->base + DSI_WISR, val, val & WISR_RRS, > - SLEEP_US, TIMEOUT_US); > + ret = dw_mipi_dsi_phy_regulator_on(dsi); > if (ret) > DRM_DEBUG_DRIVER("!TIMEOUT! waiting REGU, let's continue\n"); > > @@ -499,8 +512,16 @@ static int dw_mipi_dsi_stm_probe(struct platform_device *pdev) > } > > dsi->hw_version = dsi_read(dsi, DSI_VERSION) & VERSION; > + > + ret = dw_mipi_dsi_phy_regulator_on(dsi); > clk_disable_unprepare(pclk); > > + if (ret) { > + DRM_ERROR("%s: Failed to enable wrapper regulator, ret=%d\n", > + __func__, ret); > + goto err_dsi_probe; > + } > + I have no problem until here. If I understand this correctly, it enables the regulator during all the life of the driver. If you feel an urge to merge this patch into the Linux kernel, the st display team could gladly do it because it enables more hardware bridges. However another solution could be to rework a bit the regulator part of the driver so that you would have only device tree to change, introducing a 'reg-always-on' property. This driver needs in fact a bit of a rework with the power management. A solution could be to move the regulator-related part in dw_mipi_dsi_stm_power_on/off() so that it is only activated when needed. Those functions would integrate the enabling of the regulator, the switch for the internal regulator, and eventually handle the PLL if it cannot lock when the regulator is off. With the DT property, the power management would be only in the probe()/remove(). In that way the DSI bridges would have the logic they need to work. Ultimately there is two possibilities : * You really need this patch to be merged asap * You are ok to wait until we send the solution described above If you want to write those patches (each for DT and regulator), feel free to do it. What do you think about it ? Regards, Raphaël G-P > if (dsi->hw_version != HWVER_130 && dsi->hw_version != HWVER_131) { > ret = -ENODEV; > DRM_ERROR("bad dsi hardware version\n"); > @@ -542,6 +563,7 @@ static int dw_mipi_dsi_stm_remove(struct platform_device *pdev) > struct dw_mipi_dsi_stm *dsi = platform_get_drvdata(pdev); > > dw_mipi_dsi_remove(dsi->dsi); > + dw_mipi_dsi_phy_regulator_off(dsi); > clk_disable_unprepare(dsi->pllref_clk); > regulator_disable(dsi->vdd_supply); >
On 5/4/22 09:59, Raphael Gallais-Pou wrote: > Hi Marek, Hi, [...] >> @@ -499,8 +512,16 @@ static int dw_mipi_dsi_stm_probe(struct platform_device *pdev) >> } >> >> dsi->hw_version = dsi_read(dsi, DSI_VERSION) & VERSION; >> + >> + ret = dw_mipi_dsi_phy_regulator_on(dsi); >> clk_disable_unprepare(pclk); >> >> + if (ret) { >> + DRM_ERROR("%s: Failed to enable wrapper regulator, ret=%d\n", >> + __func__, ret); >> + goto err_dsi_probe; >> + } >> + > > I have no problem until here. If I understand this correctly, it enables the regulator during all the life of the driver. > > If you feel an urge to merge this patch into the Linux kernel, the st display team could gladly do it because it enables more hardware bridges. However another solution could be to rework a bit the regulator part of the driver so that you would have only device tree to change, introducing a 'reg-always-on' property. > > This driver needs in fact a bit of a rework with the power management. A solution could be to move the regulator-related part in dw_mipi_dsi_stm_power_on/off() so that it is only activated when needed. Those functions would integrate the enabling of the regulator, the switch for the internal regulator, and eventually handle the PLL if it cannot lock when the regulator is off. > > With the DT property, the power management would be only in the probe()/remove(). In that way the DSI bridges would have the logic they need to work. > > Ultimately there is two possibilities : > * You really need this patch to be merged asap > * You are ok to wait until we send the solution described above > > If you want to write those patches (each for DT and regulator), feel free to do it. > > What do you think about it ? Maybe a more generic question first -- is there a way to pull the data lanes to LP11 without enabling the regulator ? Also note that you likely want to wait with this patch, there is likely soon going to be a discussion about how to handle all those different requirements for initial DSI LP states and clock needed by DSI bridges, encoding such policy into DT is not the right approach.
On 5/5/22 19:40, Marek Vasut wrote: > On 5/4/22 09:59, Raphael Gallais-Pou wrote: >> Hi Marek, > > Hi, > > [...] > >>> @@ -499,8 +512,16 @@ static int dw_mipi_dsi_stm_probe(struct platform_device >>> *pdev) >>> } >>> dsi->hw_version = dsi_read(dsi, DSI_VERSION) & VERSION; >>> + >>> + ret = dw_mipi_dsi_phy_regulator_on(dsi); >>> clk_disable_unprepare(pclk); >>> + if (ret) { >>> + DRM_ERROR("%s: Failed to enable wrapper regulator, ret=%d\n", >>> + __func__, ret); >>> + goto err_dsi_probe; >>> + } >>> + >> >> I have no problem until here. If I understand this correctly, it enables the >> regulator during all the life of the driver. >> >> If you feel an urge to merge this patch into the Linux kernel, the st display >> team could gladly do it because it enables more hardware bridges. However >> another solution could be to rework a bit the regulator part of the driver so >> that you would have only device tree to change, introducing a 'reg-always-on' >> property. >> >> This driver needs in fact a bit of a rework with the power management. A >> solution could be to move the regulator-related part in >> dw_mipi_dsi_stm_power_on/off() so that it is only activated when needed. >> Those functions would integrate the enabling of the regulator, the switch for >> the internal regulator, and eventually handle the PLL if it cannot lock when >> the regulator is off. >> >> With the DT property, the power management would be only in the >> probe()/remove(). In that way the DSI bridges would have the logic they need >> to work. >> >> Ultimately there is two possibilities : >> * You really need this patch to be merged asap >> * You are ok to wait until we send the solution described above >> >> If you want to write those patches (each for DT and regulator), feel free to >> do it. >> >> What do you think about it ? > > Maybe a more generic question first -- is there a way to pull the data lanes > to LP11 without enabling the regulator ? > > Also note that you likely want to wait with this patch, there is likely soon > going to be a discussion about how to handle all those different requirements > for initial DSI LP states and clock needed by DSI bridges, encoding such > policy into DT is not the right approach. After quite some time of internal research, it is unfortunately not possible to adjust data lanes state to LP11 without the regulator enabled. So I guess, without a change to handle DSI LP states differently within DRM, your patch may be the best approach to operate such bridges. Note that I am still trying to understand how other chip vendors managed the case. Maybe their hardware can effectively handle the DL states without enabling their regulator ? I wonder if another solution could be to move the TC356787 bridge reset outside the probe, so we could also delay the regulator_enable on STM driver side. Anyway I agree with you that modifying the device tree is not the right method, and having the driver always powered is not so nice either. Raphaël
On 5/10/22 10:51, Raphael Gallais-Pou wrote: > > On 5/5/22 19:40, Marek Vasut wrote: >> On 5/4/22 09:59, Raphael Gallais-Pou wrote: >>> Hi Marek, >> >> Hi, >> >> [...] >> >>>> @@ -499,8 +512,16 @@ static int dw_mipi_dsi_stm_probe(struct platform_device >>>> *pdev) >>>> } >>>> dsi->hw_version = dsi_read(dsi, DSI_VERSION) & VERSION; >>>> + >>>> + ret = dw_mipi_dsi_phy_regulator_on(dsi); >>>> clk_disable_unprepare(pclk); >>>> + if (ret) { >>>> + DRM_ERROR("%s: Failed to enable wrapper regulator, ret=%d\n", >>>> + __func__, ret); >>>> + goto err_dsi_probe; >>>> + } >>>> + >>> >>> I have no problem until here. If I understand this correctly, it enables the >>> regulator during all the life of the driver. >>> >>> If you feel an urge to merge this patch into the Linux kernel, the st display >>> team could gladly do it because it enables more hardware bridges. However >>> another solution could be to rework a bit the regulator part of the driver so >>> that you would have only device tree to change, introducing a 'reg-always-on' >>> property. >>> >>> This driver needs in fact a bit of a rework with the power management. A >>> solution could be to move the regulator-related part in >>> dw_mipi_dsi_stm_power_on/off() so that it is only activated when needed. >>> Those functions would integrate the enabling of the regulator, the switch for >>> the internal regulator, and eventually handle the PLL if it cannot lock when >>> the regulator is off. >>> >>> With the DT property, the power management would be only in the >>> probe()/remove(). In that way the DSI bridges would have the logic they need >>> to work. >>> >>> Ultimately there is two possibilities : >>> * You really need this patch to be merged asap >>> * You are ok to wait until we send the solution described above >>> >>> If you want to write those patches (each for DT and regulator), feel free to >>> do it. >>> >>> What do you think about it ? >> >> Maybe a more generic question first -- is there a way to pull the data lanes >> to LP11 without enabling the regulator ? >> >> Also note that you likely want to wait with this patch, there is likely soon >> going to be a discussion about how to handle all those different requirements >> for initial DSI LP states and clock needed by DSI bridges, encoding such >> policy into DT is not the right approach. > > > After quite some time of internal research, it is unfortunately not possible to > adjust data lanes state to LP11 without the regulator enabled. > > So I guess, without a change to handle DSI LP states differently within DRM, > your patch may be the best approach to operate such bridges. > > Note that I am still trying to understand how other chip vendors managed the > case. Maybe their hardware can effectively handle the DL states without enabling > their regulator ? I think they just keep the DSI data lanes in LP11 when the bridge comes up, but this is probably currently inconsistent and totally ad-hoc, hence the need for some sort of new API to control these states better. > I wonder if another solution could be to move the TC356787 bridge reset outside > the probe, so we could also delay the regulator_enable on STM driver side. No, you cannot do that, because of the (e)DP aux channel which has to be available early on that bridge, i.e. in probe. That is also something which makes clocking it from DSI HS clock hard on that chip. > Anyway I agree with you that modifying the device tree is not the right method, > and having the driver always powered is not so nice either. Let's postpone this patch until we know what to do about the LP states in general. Of course, if another bridge which has problems like this TC358767 pops up on STM32, you can just apply this patch then, oh well.
diff --git a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c index 89897d5f5c72..c403633ffeae 100644 --- a/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c +++ b/drivers/gpu/drm/stm/dw_mipi_dsi-stm.c @@ -194,16 +194,29 @@ static int dsi_pll_get_params(struct dw_mipi_dsi_stm *dsi, return 0; } +static int dw_mipi_dsi_phy_regulator_on(struct dw_mipi_dsi_stm *dsi) +{ + u32 val; + + /* Enable the regulator */ + dsi_set(dsi, DSI_WRPCR, WRPCR_REGEN | WRPCR_BGREN); + return readl_poll_timeout(dsi->base + DSI_WISR, val, val & WISR_RRS, + SLEEP_US, TIMEOUT_US); +} + +static void dw_mipi_dsi_phy_regulator_off(struct dw_mipi_dsi_stm *dsi) +{ + /* Disable the regulator */ + dsi_clear(dsi, DSI_WRPCR, WRPCR_REGEN | WRPCR_BGREN); +} + static int dw_mipi_dsi_phy_init(void *priv_data) { struct dw_mipi_dsi_stm *dsi = priv_data; u32 val; int ret; - /* Enable the regulator */ - dsi_set(dsi, DSI_WRPCR, WRPCR_REGEN | WRPCR_BGREN); - ret = readl_poll_timeout(dsi->base + DSI_WISR, val, val & WISR_RRS, - SLEEP_US, TIMEOUT_US); + ret = dw_mipi_dsi_phy_regulator_on(dsi); if (ret) DRM_DEBUG_DRIVER("!TIMEOUT! waiting REGU, let's continue\n"); @@ -499,8 +512,16 @@ static int dw_mipi_dsi_stm_probe(struct platform_device *pdev) } dsi->hw_version = dsi_read(dsi, DSI_VERSION) & VERSION; + + ret = dw_mipi_dsi_phy_regulator_on(dsi); clk_disable_unprepare(pclk); + if (ret) { + DRM_ERROR("%s: Failed to enable wrapper regulator, ret=%d\n", + __func__, ret); + goto err_dsi_probe; + } + if (dsi->hw_version != HWVER_130 && dsi->hw_version != HWVER_131) { ret = -ENODEV; DRM_ERROR("bad dsi hardware version\n"); @@ -542,6 +563,7 @@ static int dw_mipi_dsi_stm_remove(struct platform_device *pdev) struct dw_mipi_dsi_stm *dsi = platform_get_drvdata(pdev); dw_mipi_dsi_remove(dsi->dsi); + dw_mipi_dsi_phy_regulator_off(dsi); clk_disable_unprepare(dsi->pllref_clk); regulator_disable(dsi->vdd_supply);
Certain DSI bridge chips like TC358767/TC358867/TC9595 expect the DSI D0 data lane to be in LP-11 state when released from reset. Currently the STM32MP157 DSI host wrapper glue logic keeps D0 data lane in LP-00 state until DSI init happens, which confuses the TC358767 into entering some sort of test mode and the chip cannot be brought out of this test mode in any way. Enable the wrapper glue logic regulator in probe callback already and disable it in remove callback to satisfy this requirement. The D0 data lane is in LP-11 mode when the TC358767 bridge chip is brought up and the chip is not confused anymore. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com> Cc: Antonio Borneo <antonio.borneo@foss.st.com> Cc: Philippe Cornu <philippe.cornu@foss.st.com> Cc: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com> Cc: Robert Foss <robert.foss@linaro.org> Cc: Yannick Fertre <yannick.fertre@foss.st.com> --- drivers/gpu/drm/stm/dw_mipi_dsi-stm.c | 30 +++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-)