| Message ID | 20240622110929.3115714-4-a-bhatia1@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: cdns-dsi: Fix the color-shift issue | expand |
Hi, On 22/06/2024 14:09, Aradhya Bhatia wrote: > Initialize the Phy during the cdns-dsi _resume(), and de-initialize it > during the _suspend(). > > Also power-off the Phy from bridge_disable. > > Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework") > Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> > --- > drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c > index 5159c3f0853e..d89c32bae2b9 100644 > --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c > +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c > @@ -672,6 +672,10 @@ static void cdns_dsi_bridge_disable(struct drm_bridge *bridge) > if (dsi->platform_ops && dsi->platform_ops->disable) > dsi->platform_ops->disable(dsi); > > + phy_power_off(dsi->dphy); > + dsi->link_initialized = false; > + dsi->phy_initialized = false; > + > pm_runtime_put(dsi->base.dev); > } > > @@ -698,7 +702,6 @@ static void cdns_dsi_hs_init(struct cdns_dsi *dsi) > DPHY_CMN_PDN | DPHY_PLL_PDN, > dsi->regs + MCTL_DPHY_CFG0); > > - phy_init(dsi->dphy); > phy_set_mode(dsi->dphy, PHY_MODE_MIPI_DPHY); > phy_configure(dsi->dphy, &output->phy_opts); > phy_power_on(dsi->dphy); > @@ -1120,6 +1123,8 @@ static int __maybe_unused cdns_dsi_resume(struct device *dev) > clk_prepare_enable(dsi->dsi_p_clk); > clk_prepare_enable(dsi->dsi_sys_clk); > > + phy_init(dsi->dphy); > + > return 0; > } > > @@ -1127,10 +1132,11 @@ static int __maybe_unused cdns_dsi_suspend(struct device *dev) > { > struct cdns_dsi *dsi = dev_get_drvdata(dev); > > + phy_exit(dsi->dphy); > + > clk_disable_unprepare(dsi->dsi_sys_clk); > clk_disable_unprepare(dsi->dsi_p_clk); > reset_control_assert(dsi->dsi_p_rst); > - dsi->link_initialized = false; > return 0; > } > So with this patch, phy_init/exit will be called in the resume/suspend functions. That looks fine. But the phy_power_on/phy_power_off looks odd to me. Here you add phy_power_off() to cdns_dsi_bridge_disable(), which sounds fine. But phy_power_on() is called in cdns_dsi_hs_init(), and that is called in cdns_dsi_bridge_enable() (which sounds fine), but also in cdns_dsi_bridge_pre_enable(). So doesn't that mean cdns_dsi_hs_init() call in cdns_dsi_bridge_enable() is extra, as it effectively does nothing (it exists right away if dsi->phy_initialized == true)? Tomi
Hi Tomi, Thanks for reviewing the patches! On 26/06/24 15:55, Tomi Valkeinen wrote: > Hi, > > On 22/06/2024 14:09, Aradhya Bhatia wrote: >> Initialize the Phy during the cdns-dsi _resume(), and de-initialize it >> during the _suspend(). >> >> Also power-off the Phy from bridge_disable. >> >> Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework") >> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> >> --- >> drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c >> b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c >> index 5159c3f0853e..d89c32bae2b9 100644 >> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c >> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c >> @@ -672,6 +672,10 @@ static void cdns_dsi_bridge_disable(struct >> drm_bridge *bridge) >> if (dsi->platform_ops && dsi->platform_ops->disable) >> dsi->platform_ops->disable(dsi); >> + phy_power_off(dsi->dphy); >> + dsi->link_initialized = false; >> + dsi->phy_initialized = false; >> + >> pm_runtime_put(dsi->base.dev); >> } >> @@ -698,7 +702,6 @@ static void cdns_dsi_hs_init(struct cdns_dsi *dsi) >> DPHY_CMN_PDN | DPHY_PLL_PDN, >> dsi->regs + MCTL_DPHY_CFG0); >> - phy_init(dsi->dphy); >> phy_set_mode(dsi->dphy, PHY_MODE_MIPI_DPHY); >> phy_configure(dsi->dphy, &output->phy_opts); >> phy_power_on(dsi->dphy); >> @@ -1120,6 +1123,8 @@ static int __maybe_unused cdns_dsi_resume(struct >> device *dev) >> clk_prepare_enable(dsi->dsi_p_clk); >> clk_prepare_enable(dsi->dsi_sys_clk); >> + phy_init(dsi->dphy); >> + >> return 0; >> } >> @@ -1127,10 +1132,11 @@ static int __maybe_unused >> cdns_dsi_suspend(struct device *dev) >> { >> struct cdns_dsi *dsi = dev_get_drvdata(dev); >> + phy_exit(dsi->dphy); >> + >> clk_disable_unprepare(dsi->dsi_sys_clk); >> clk_disable_unprepare(dsi->dsi_p_clk); >> reset_control_assert(dsi->dsi_p_rst); >> - dsi->link_initialized = false; >> return 0; >> } >> > > So with this patch, phy_init/exit will be called in the resume/suspend > functions. That looks fine. > > But the phy_power_on/phy_power_off looks odd to me. Here you add > phy_power_off() to cdns_dsi_bridge_disable(), which sounds fine. But > phy_power_on() is called in cdns_dsi_hs_init(), and that is called in > cdns_dsi_bridge_enable() (which sounds fine), but also in > cdns_dsi_bridge_pre_enable(). > > So doesn't that mean cdns_dsi_hs_init() call in cdns_dsi_bridge_enable() > is extra, as it effectively does nothing (it exists right away if > dsi->phy_initialized == true)? That's right. When cdns_dsi_hs_init() is called from cdns_dsi_bridge_enable(), it is simply expected to return since phy_initialized is true. I am not aware about the exact reasoning behind this, but this gets addressed when I convert the _bridge_enable() to _bridge_pre_enable() and drop the older _bridge_pre_enable() entirely. Regards Aradhya
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c index 5159c3f0853e..d89c32bae2b9 100644 --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c @@ -672,6 +672,10 @@ static void cdns_dsi_bridge_disable(struct drm_bridge *bridge) if (dsi->platform_ops && dsi->platform_ops->disable) dsi->platform_ops->disable(dsi); + phy_power_off(dsi->dphy); + dsi->link_initialized = false; + dsi->phy_initialized = false; + pm_runtime_put(dsi->base.dev); } @@ -698,7 +702,6 @@ static void cdns_dsi_hs_init(struct cdns_dsi *dsi) DPHY_CMN_PDN | DPHY_PLL_PDN, dsi->regs + MCTL_DPHY_CFG0); - phy_init(dsi->dphy); phy_set_mode(dsi->dphy, PHY_MODE_MIPI_DPHY); phy_configure(dsi->dphy, &output->phy_opts); phy_power_on(dsi->dphy); @@ -1120,6 +1123,8 @@ static int __maybe_unused cdns_dsi_resume(struct device *dev) clk_prepare_enable(dsi->dsi_p_clk); clk_prepare_enable(dsi->dsi_sys_clk); + phy_init(dsi->dphy); + return 0; } @@ -1127,10 +1132,11 @@ static int __maybe_unused cdns_dsi_suspend(struct device *dev) { struct cdns_dsi *dsi = dev_get_drvdata(dev); + phy_exit(dsi->dphy); + clk_disable_unprepare(dsi->dsi_sys_clk); clk_disable_unprepare(dsi->dsi_p_clk); reset_control_assert(dsi->dsi_p_rst); - dsi->link_initialized = false; return 0; }
Initialize the Phy during the cdns-dsi _resume(), and de-initialize it during the _suspend(). Also power-off the Phy from bridge_disable. Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework") Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com> --- drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)