| Message ID | 20240625122824.148163-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/2] drm: bridge: samsung-dsim: Initialize bridge on attach | expand |
Hi Marek, Am Dienstag, 25. Juni 2024, 14:26:10 CEST schrieb Marek Vasut: > Initialize the bridge on attach already, to force lanes into LP11 > state, since attach does trigger attach of downstream bridges which > may trigger (e)DP AUX channel mode read. > > This fixes a corner case where DSIM with TC9595 attached to it fails > to operate the DP AUX channel, because the TC9595 enters some debug > mode when it is released from reset without lanes in LP11 mode. By > ensuring the DSIM lanes are in LP11, the TC9595 (tc358767.c driver) > can be reset in its attach callback called from DSIM attach callback, > and recovered out of the debug mode just before TC9595 performs first > AUX channel access later in its attach callback. > > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Adam Ford <aford173@gmail.com> > Cc: Alexander Stein <alexander.stein@ew.tq-group.com> > Cc: Andrzej Hajda <andrzej.hajda@intel.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: David Airlie <airlied@gmail.com> > Cc: Frieder Schrempf <frieder.schrempf@kontron.de> > Cc: Inki Dae <inki.dae@samsung.com> > Cc: Jagan Teki <jagan@amarulasolutions.com> > Cc: Jernej Skrabec <jernej.skrabec@gmail.com> > Cc: Jonas Karlman <jonas@kwiboo.se> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > Cc: Lucas Stach <l.stach@pengutronix.de> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Marek Szyprowski <m.szyprowski@samsung.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Michael Walle <mwalle@kernel.org> > Cc: Neil Armstrong <neil.armstrong@linaro.org> > Cc: Robert Foss <rfoss@kernel.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: dri-devel@lists.freedesktop.org > Cc: kernel@dh-electronics.com > --- > V2: Handle case where mode is not set yet > --- > drivers/gpu/drm/bridge/samsung-dsim.c | 32 ++++++++++++++++++++------- > 1 file changed, 24 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c > index e7e53a9e42afb..22d3bbd866d97 100644 > --- a/drivers/gpu/drm/bridge/samsung-dsim.c > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > @@ -699,20 +699,24 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi, > > static int samsung_dsim_enable_clock(struct samsung_dsim *dsi) > { > - unsigned long hs_clk, byte_clk, esc_clk, pix_clk; > + unsigned long hs_clk, byte_clk, esc_clk; > unsigned long esc_div; > u32 reg; > struct drm_display_mode *m = &dsi->mode; > int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format); > > - /* m->clock is in KHz */ > - pix_clk = m->clock * 1000; > - > - /* Use burst_clk_rate if available, otherwise use the pix_clk */ > + /* > + * Use burst_clk_rate if available, otherwise use the mode clock > + * if mode is already set and available, otherwise fall back to > + * PLL input clock and operate in 1:1 lowest frequency mode until > + * a mode is set. > + */ > if (dsi->burst_clk_rate) > hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate); > + else if (m) /* m->clock is in KHz */ > + hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(m->clock * 1000 * bpp, dsi->lanes)); > else > - hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(pix_clk * bpp, dsi->lanes)); > + hs_clk = dsi->pll_clk_rate; > I can't reproduce that mentioned corner case and presumably this problem does not exist otherwise. If samsung,burst-clock-frequency is unset I get a sluggish image on the monitor. It seems the calculation is using a adjusted clock frequency: samsung-dsim 32e60000.dsi: Using pixel clock for HS clock frequency samsung-dsim 32e60000.dsi: [drm:samsung_dsim_host_attach [samsung_dsim]] Attached tc358767 device (lanes:4 bpp:24 mode-flags:0xc03) samsung-dsim 32e60000.dsi: PLL ref clock freq 24000000 samsung-dsim 32e60000.dsi: failed to find PLL PMS for requested frequency samsung-dsim 32e60000.dsi: failed to configure DSI PLL samsung-dsim 32e60000.dsi: PLL ref clock freq 24000000 samsung-dsim 32e60000.dsi: PLL freq 883636363, (p 11, m 810, s 1) samsung-dsim 32e60000.dsi: hs_clk = 883636363, byte_clk = 110454545, esc_clk = 9204545 samsung-dsim 32e60000.dsi: calculated hfp: 60, hbp: 105, hsa: 27 samsung-dsim 32e60000.dsi: LCD size = 1920x1080 891 MHz is the nominal one for 148.5 MHz pixelclock. But even setting samsung,burst-clock-frequency to 891 MHz results in a sluggish image. Maybe this usecase is nothing I need to consider while using DSI-DP with EDID timings available. As the burst clock needs to provide more bandwidth than actually needed, I consider this a different usecase not providing samsung,burst-clock-frequency for DSI->DP usage. But the initialization upon attach is working intended, so: Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com> > if (!hs_clk) { > dev_err(dsi->dev, "failed to configure DSI PLL\n"); > @@ -1643,9 +1647,21 @@ static int samsung_dsim_attach(struct drm_bridge *bridge, > enum drm_bridge_attach_flags flags) > { > struct samsung_dsim *dsi = bridge_to_dsi(bridge); > + int ret; > > - return drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge, > - flags); > + ret = pm_runtime_resume_and_get(dsi->dev); > + if (ret < 0) > + return ret; > + > + ret = samsung_dsim_init(dsi); > + if (ret < 0) > + goto err; > + > + ret = drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge, > + flags); > +err: > + pm_runtime_put_sync(dsi->dev); > + return ret; > } > > static const struct drm_bridge_funcs samsung_dsim_bridge_funcs = { >
On 6/25/24 4:37 PM, Alexander Stein wrote: > Hi Marek, Hi, > Am Dienstag, 25. Juni 2024, 14:26:10 CEST schrieb Marek Vasut: >> Initialize the bridge on attach already, to force lanes into LP11 >> state, since attach does trigger attach of downstream bridges which >> may trigger (e)DP AUX channel mode read. >> >> This fixes a corner case where DSIM with TC9595 attached to it fails >> to operate the DP AUX channel, because the TC9595 enters some debug >> mode when it is released from reset without lanes in LP11 mode. By >> ensuring the DSIM lanes are in LP11, the TC9595 (tc358767.c driver) >> can be reset in its attach callback called from DSIM attach callback, >> and recovered out of the debug mode just before TC9595 performs first >> AUX channel access later in its attach callback. >> >> Signed-off-by: Marek Vasut <marex@denx.de> >> --- >> Cc: Adam Ford <aford173@gmail.com> >> Cc: Alexander Stein <alexander.stein@ew.tq-group.com> >> Cc: Andrzej Hajda <andrzej.hajda@intel.com> >> Cc: Daniel Vetter <daniel@ffwll.ch> >> Cc: David Airlie <airlied@gmail.com> >> Cc: Frieder Schrempf <frieder.schrempf@kontron.de> >> Cc: Inki Dae <inki.dae@samsung.com> >> Cc: Jagan Teki <jagan@amarulasolutions.com> >> Cc: Jernej Skrabec <jernej.skrabec@gmail.com> >> Cc: Jonas Karlman <jonas@kwiboo.se> >> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> >> Cc: Lucas Stach <l.stach@pengutronix.de> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Cc: Marek Szyprowski <m.szyprowski@samsung.com> >> Cc: Maxime Ripard <mripard@kernel.org> >> Cc: Michael Walle <mwalle@kernel.org> >> Cc: Neil Armstrong <neil.armstrong@linaro.org> >> Cc: Robert Foss <rfoss@kernel.org> >> Cc: Thomas Zimmermann <tzimmermann@suse.de> >> Cc: dri-devel@lists.freedesktop.org >> Cc: kernel@dh-electronics.com >> --- >> V2: Handle case where mode is not set yet >> --- >> drivers/gpu/drm/bridge/samsung-dsim.c | 32 ++++++++++++++++++++------- >> 1 file changed, 24 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c >> index e7e53a9e42afb..22d3bbd866d97 100644 >> --- a/drivers/gpu/drm/bridge/samsung-dsim.c >> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c >> @@ -699,20 +699,24 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi, >> >> static int samsung_dsim_enable_clock(struct samsung_dsim *dsi) >> { >> - unsigned long hs_clk, byte_clk, esc_clk, pix_clk; >> + unsigned long hs_clk, byte_clk, esc_clk; >> unsigned long esc_div; >> u32 reg; >> struct drm_display_mode *m = &dsi->mode; >> int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format); >> >> - /* m->clock is in KHz */ >> - pix_clk = m->clock * 1000; >> - >> - /* Use burst_clk_rate if available, otherwise use the pix_clk */ >> + /* >> + * Use burst_clk_rate if available, otherwise use the mode clock >> + * if mode is already set and available, otherwise fall back to >> + * PLL input clock and operate in 1:1 lowest frequency mode until >> + * a mode is set. >> + */ >> if (dsi->burst_clk_rate) >> hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate); >> + else if (m) /* m->clock is in KHz */ >> + hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(m->clock * 1000 * bpp, dsi->lanes)); >> else >> - hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(pix_clk * bpp, dsi->lanes)); >> + hs_clk = dsi->pll_clk_rate; >> > > I can't reproduce that mentioned corner case and presumably this problem > does not exist otherwise. If samsung,burst-clock-frequency is unset I get > a sluggish image on the monitor. > > It seems the calculation is using a adjusted clock frequency: > samsung-dsim 32e60000.dsi: Using pixel clock for HS clock frequency > samsung-dsim 32e60000.dsi: [drm:samsung_dsim_host_attach [samsung_dsim]] Attached tc358767 device (lanes:4 bpp:24 mode-flags:0xc03) > samsung-dsim 32e60000.dsi: PLL ref clock freq 24000000 > samsung-dsim 32e60000.dsi: failed to find PLL PMS for requested frequency > samsung-dsim 32e60000.dsi: failed to configure DSI PLL > samsung-dsim 32e60000.dsi: PLL ref clock freq 24000000 > samsung-dsim 32e60000.dsi: PLL freq 883636363, (p 11, m 810, s 1) > samsung-dsim 32e60000.dsi: hs_clk = 883636363, byte_clk = 110454545, esc_clk = 9204545 > samsung-dsim 32e60000.dsi: calculated hfp: 60, hbp: 105, hsa: 27 > samsung-dsim 32e60000.dsi: LCD size = 1920x1080 > > 891 MHz is the nominal one for 148.5 MHz pixelclock. But even setting > samsung,burst-clock-frequency to 891 MHz results in a sluggish image. > Maybe this usecase is nothing I need to consider while using DSI-DP > with EDID timings available. > > As the burst clock needs to provide more bandwidth than actually needed, > I consider this a different usecase not providing > samsung,burst-clock-frequency for DSI->DP usage. > > But the initialization upon attach is working intended, so: > Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com> Thank you for testing and keeping up with this. I will wait for more feedback if there is any (Frieder? Lucas? Michael?). If there are no objections, then I can merge it in a week or two ?
On Wed Jun 26, 2024 at 5:21 AM CEST, Marek Vasut wrote: > Thank you for testing and keeping up with this. I will wait for more > feedback if there is any (Frieder? Lucas? Michael?). If there are no > objections, then I can merge it in a week or two ? I'll try to use your approach on the tc358775. Hopefully, I'll find some time this week. -michael
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c index e7e53a9e42afb..22d3bbd866d97 100644 --- a/drivers/gpu/drm/bridge/samsung-dsim.c +++ b/drivers/gpu/drm/bridge/samsung-dsim.c @@ -699,20 +699,24 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi, static int samsung_dsim_enable_clock(struct samsung_dsim *dsi) { - unsigned long hs_clk, byte_clk, esc_clk, pix_clk; + unsigned long hs_clk, byte_clk, esc_clk; unsigned long esc_div; u32 reg; struct drm_display_mode *m = &dsi->mode; int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format); - /* m->clock is in KHz */ - pix_clk = m->clock * 1000; - - /* Use burst_clk_rate if available, otherwise use the pix_clk */ + /* + * Use burst_clk_rate if available, otherwise use the mode clock + * if mode is already set and available, otherwise fall back to + * PLL input clock and operate in 1:1 lowest frequency mode until + * a mode is set. + */ if (dsi->burst_clk_rate) hs_clk = samsung_dsim_set_pll(dsi, dsi->burst_clk_rate); + else if (m) /* m->clock is in KHz */ + hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(m->clock * 1000 * bpp, dsi->lanes)); else - hs_clk = samsung_dsim_set_pll(dsi, DIV_ROUND_UP(pix_clk * bpp, dsi->lanes)); + hs_clk = dsi->pll_clk_rate; if (!hs_clk) { dev_err(dsi->dev, "failed to configure DSI PLL\n"); @@ -1643,9 +1647,21 @@ static int samsung_dsim_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) { struct samsung_dsim *dsi = bridge_to_dsi(bridge); + int ret; - return drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge, - flags); + ret = pm_runtime_resume_and_get(dsi->dev); + if (ret < 0) + return ret; + + ret = samsung_dsim_init(dsi); + if (ret < 0) + goto err; + + ret = drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge, + flags); +err: + pm_runtime_put_sync(dsi->dev); + return ret; } static const struct drm_bridge_funcs samsung_dsim_bridge_funcs = {
Initialize the bridge on attach already, to force lanes into LP11 state, since attach does trigger attach of downstream bridges which may trigger (e)DP AUX channel mode read. This fixes a corner case where DSIM with TC9595 attached to it fails to operate the DP AUX channel, because the TC9595 enters some debug mode when it is released from reset without lanes in LP11 mode. By ensuring the DSIM lanes are in LP11, the TC9595 (tc358767.c driver) can be reset in its attach callback called from DSIM attach callback, and recovered out of the debug mode just before TC9595 performs first AUX channel access later in its attach callback. Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Adam Ford <aford173@gmail.com> Cc: Alexander Stein <alexander.stein@ew.tq-group.com> Cc: Andrzej Hajda <andrzej.hajda@intel.com> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: David Airlie <airlied@gmail.com> Cc: Frieder Schrempf <frieder.schrempf@kontron.de> Cc: Inki Dae <inki.dae@samsung.com> Cc: Jagan Teki <jagan@amarulasolutions.com> Cc: Jernej Skrabec <jernej.skrabec@gmail.com> Cc: Jonas Karlman <jonas@kwiboo.se> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> Cc: Lucas Stach <l.stach@pengutronix.de> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Marek Szyprowski <m.szyprowski@samsung.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Michael Walle <mwalle@kernel.org> Cc: Neil Armstrong <neil.armstrong@linaro.org> Cc: Robert Foss <rfoss@kernel.org> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: dri-devel@lists.freedesktop.org Cc: kernel@dh-electronics.com --- V2: Handle case where mode is not set yet --- drivers/gpu/drm/bridge/samsung-dsim.c | 32 ++++++++++++++++++++------- 1 file changed, 24 insertions(+), 8 deletions(-)