[v2,1/2] drm: bridge: samsung-dsim: Initialize bridge on attach

Commit Message

Marek Vasut June 25, 2024, 12:26 p.m. UTC

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 <[e-mail]>
---
Cc: Adam Ford <[e-mail]>
Cc: Alexander Stein <[e-mail]>
Cc: Andrzej Hajda <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
Cc: David Airlie <[e-mail]>
Cc: Frieder Schrempf <[e-mail]>
Cc: Inki Dae <[e-mail]>
Cc: Jagan Teki <[e-mail]>
Cc: Jernej Skrabec <[e-mail]>
Cc: Jonas Karlman <[e-mail]>
Cc: Laurent Pinchart <[e-mail]>
Cc: Lucas Stach <[e-mail]>
Cc: Maarten Lankhorst <[e-mail]>
Cc: Marek Szyprowski <[e-mail]>
Cc: Maxime Ripard <[e-mail]>
Cc: Michael Walle <[e-mail]>
Cc: Neil Armstrong <[e-mail]>
Cc: Robert Foss <[e-mail]>
Cc: Thomas Zimmermann <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
---
V2: Handle case where mode is not set yet
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 32 ++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 8 deletions(-)

Comments

Alexander Stein June 25, 2024, 2:37 p.m. UTC | #1

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 <[e-mail]>
> ---
> Cc: Adam Ford <[e-mail]>
> Cc: Alexander Stein <[e-mail]>
> Cc: Andrzej Hajda <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> Cc: David Airlie <[e-mail]>
> Cc: Frieder Schrempf <[e-mail]>
> Cc: Inki Dae <[e-mail]>
> Cc: Jagan Teki <[e-mail]>
> Cc: Jernej Skrabec <[e-mail]>
> Cc: Jonas Karlman <[e-mail]>
> Cc: Laurent Pinchart <[e-mail]>
> Cc: Lucas Stach <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Marek Szyprowski <[e-mail]>
> Cc: Maxime Ripard <[e-mail]>
> Cc: Michael Walle <[e-mail]>
> Cc: Neil Armstrong <[e-mail]>
> Cc: Robert Foss <[e-mail]>
> Cc: Thomas Zimmermann <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> ---
> 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 <[e-mail]>

>  	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 = {
>

Marek Vasut June 26, 2024, 3:21 a.m. UTC | #2

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 <[e-mail]>
>> ---
>> Cc: Adam Ford <[e-mail]>
>> Cc: Alexander Stein <[e-mail]>
>> Cc: Andrzej Hajda <[e-mail]>
>> Cc: Daniel Vetter <[e-mail]>
>> Cc: David Airlie <[e-mail]>
>> Cc: Frieder Schrempf <[e-mail]>
>> Cc: Inki Dae <[e-mail]>
>> Cc: Jagan Teki <[e-mail]>
>> Cc: Jernej Skrabec <[e-mail]>
>> Cc: Jonas Karlman <[e-mail]>
>> Cc: Laurent Pinchart <[e-mail]>
>> Cc: Lucas Stach <[e-mail]>
>> Cc: Maarten Lankhorst <[e-mail]>
>> Cc: Marek Szyprowski <[e-mail]>
>> Cc: Maxime Ripard <[e-mail]>
>> Cc: Michael Walle <[e-mail]>
>> Cc: Neil Armstrong <[e-mail]>
>> Cc: Robert Foss <[e-mail]>
>> Cc: Thomas Zimmermann <[e-mail]>
>> Cc: [e-mail]
>> Cc: [e-mail]
>> ---
>> 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 <[e-mail]>

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 ?

Michael Walle June 26, 2024, 8:02 a.m. UTC | #3

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

Marek Vasut July 11, 2024, 3:38 p.m. UTC | #4

On 6/26/24 10:02 AM, Michael Walle wrote:
> 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.

So ... I wonder ... shall I apply these patches or not ?

I'll wait about a week or two before applying them, to get some input.

Marek Szyprowski July 11, 2024, 3:57 p.m. UTC | #5

On 11.07.2024 17:38, Marek Vasut wrote:
> On 6/26/24 10:02 AM, Michael Walle wrote:
>> 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.
>
> So ... I wonder ... shall I apply these patches or not ?
>
> I'll wait about a week or two before applying them, to get some input.
>
I've pointed that they break current users of Samsung DSIM: Exynos-DSI 
and Samsung s6e3ha2/s6e3hf2 panels, but unfortunately I'm not able to 
provide datasheet nor any other documentation. Due to other tasks and 
holidays I'm not able to debug it further too, at least till the end of 
August. Maybe we could keep old behavior for "exynos-dsi" compatible device?

Best regards

Michael Walle July 12, 2024, 7:16 a.m. UTC | #6

Hi Marek,

> >> 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.
>
> So ... I wonder ... shall I apply these patches or not ?

As mentioned on IRC, I tried it to port it for the mediatek DSI
host, but I gave up and got doubts that this is the way to go. I
think this is too invasive (in a sense that it changes behavior) and
not that easy to implement on other drivers.

Given that this requirement is far more common across DSI bridges,
I'd favor a more general solution which isn't a workaround.

-michael

>
> I'll wait about a week or two before applying them, to get some input.

Marek Vasut July 16, 2024, 6:43 p.m. UTC | #7

On 7/11/24 5:57 PM, Marek Szyprowski wrote:
> On 11.07.2024 17:38, Marek Vasut wrote:
>> On 6/26/24 10:02 AM, Michael Walle wrote:
>>> 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.
>>
>> So ... I wonder ... shall I apply these patches or not ?
>>
>> I'll wait about a week or two before applying them, to get some input.
>>
> I've pointed that they break current users of Samsung DSIM: Exynos-DSI
> and Samsung s6e3ha2/s6e3hf2 panels, but unfortunately I'm not able to
> provide datasheet nor any other documentation. Due to other tasks and
> holidays I'm not able to debug it further too, at least till the end of
> August. Maybe we could keep old behavior for "exynos-dsi" compatible device?

Nope, let's not pile up workarounds. It would be good to figure out why 
does this display misbehave when the data lanes are in LP11 on start up. 
It seems most sinks do require data lanes in LP11 on start up, so what 
is special about this panel ? Do you have access to the datasheet and 
can you check once you're back ?

Marek Vasut July 16, 2024, 6:47 p.m. UTC | #8

On 7/12/24 9:16 AM, Michael Walle wrote:
> Hi Marek,

Hi,

>>>> 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.
>>
>> So ... I wonder ... shall I apply these patches or not ?
> 
> As mentioned on IRC, I tried it to port it for the mediatek DSI
> host, but I gave up and got doubts that this is the way to go. I
> think this is too invasive (in a sense that it changes behavior)

I would argue it makes the behavior well defined. If that breaks some 
drivers that depended on the undefined behavior before, those should be 
fixed too.

> and not that easy to implement on other drivers.

How so ? At least the DSIM and STM32 DW DSI host can switch lanes to 
LP11 state. Is the mediatek host not capable of that ?

> Given that this requirement is far more common across DSI bridges,
> I'd favor a more general solution which isn't a workaround.

I think we only had a look at the TI DSI83 / ICN6211 / Toshiba TC358767 
bridges, but we did not look at many panels, did we ? Do panels require 
lanes in non-LP11 state on start up ?

Was there any progress on the generic LP11 solution, I think you did 
mention something was in progress ? How would that even look like ?

Michael Walle July 16, 2024, 7:50 p.m. UTC | #9

> >>>> 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.
> >>
> >> So ... I wonder ... shall I apply these patches or not ?
> > 
> > As mentioned on IRC, I tried it to port it for the mediatek DSI
> > host, but I gave up and got doubts that this is the way to go. I
> > think this is too invasive (in a sense that it changes behavior)
>
> I would argue it makes the behavior well defined. If that breaks some 
> drivers that depended on the undefined behavior before, those should be 
> fixed too.

Then this behavior should be documented (and accepted) in the
corresponding section:
https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation

This will help DSI host driver developers and we can point all the
host DSI driver maintainers to that document along with the proper
implementation :)

> > and not that easy to implement on other drivers.
>
> How so ? At least the DSIM and STM32 DW DSI host can switch lanes to 
> LP11 state. Is the mediatek host not capable of that ?

The controller is certainly capable to do that. But the changes
seems pretty invasive to me and I fear some fallout. Although it's
basically just one line for the DSIM, you seem to be moving the init
of the DSIM to an earlier point(?). I'm no expert with all the DRM
stuff, so I might be wrong here.

> > Given that this requirement is far more common across DSI bridges,
> > I'd favor a more general solution which isn't a workaround.
>
> I think we only had a look at the TI DSI83 / ICN6211 / Toshiba TC358767 
> bridges, but we did not look at many panels, did we ? Do panels require 
> lanes in non-LP11 state on start up ?

I'm not talking about panels, just bridges. It's not just one bridge
with a weird behavior but most bridges.

> Was there any progress on the generic LP11 solution, I think you did 
> mention something was in progress ? How would that even look like ?

I had a new callback implemented:
https://lore.kernel.org/r/[e-mail]/

Not sure if that's any better though.

-michael

Marek Vasut July 17, 2024, 1 a.m. UTC | #10

On 7/16/24 9:50 PM, Michael Walle 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.
>>>>
>>>> So ... I wonder ... shall I apply these patches or not ?
>>>
>>> As mentioned on IRC, I tried it to port it for the mediatek DSI
>>> host, but I gave up and got doubts that this is the way to go. I
>>> think this is too invasive (in a sense that it changes behavior)
>>
>> I would argue it makes the behavior well defined. If that breaks some
>> drivers that depended on the undefined behavior before, those should be
>> fixed too.
> 
> Then this behavior should be documented (and accepted) in the
> corresponding section:
> https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation

I think so too.

> This will help DSI host driver developers and we can point all the
> host DSI driver maintainers to that document along with the proper
> implementation :)
> 
>>> and not that easy to implement on other drivers.
>>
>> How so ? At least the DSIM and STM32 DW DSI host can switch lanes to
>> LP11 state. Is the mediatek host not capable of that ?
> 
> The controller is certainly capable to do that. But the changes
> seems pretty invasive to me and I fear some fallout. Although it's
> basically just one line for the DSIM, you seem to be moving the init
> of the DSIM to an earlier point(?). I'm no expert with all the DRM
> stuff, so I might be wrong here.

I am moving some of the initialization to an earlier point, but only 
enough of it to configure the lanes to LP11 state before the next 
bridge(s) start to (pre)enable themselves. And the DSIM controller is 
RPM suspended again after the lanes are in LP11.

>>> Given that this requirement is far more common across DSI bridges,
>>> I'd favor a more general solution which isn't a workaround.
>>
>> I think we only had a look at the TI DSI83 / ICN6211 / Toshiba TC358767
>> bridges, but we did not look at many panels, did we ? Do panels require
>> lanes in non-LP11 state on start up ?
> 
> I'm not talking about panels, just bridges. It's not just one bridge
> with a weird behavior but most bridges.

What do you refer to by "weird behavior" ? It seems the DSI bridges we 
looked at all required data lanes in LP11 state on start up one way or 
the other, didn't they ?

>> Was there any progress on the generic LP11 solution, I think you did
>> mention something was in progress ? How would that even look like ?
> 
> I had a new callback implemented:
> https://lore.kernel.org/r/[e-mail]/
> 
> Not sure if that's any better though.

Wouldn't that callback be called by various controllers at various 
stages of initialization , without consistency on when that callback 
will be called ? That would be my concern.

At least here, the expectation is that the controller would put data 
lanes into LP11 before the next bridge can even pre_enable itself , 
which is not perfect though, because if a bridge needs DSI clock to 
probe() itself and then data lanes in LP11 to probe itself, that is a 
really bad situation (and the TC358767 is capable of being used that 
way, although this is currently not supported by the kernel driver and 
there is no real interest in supporting it).

Alexander Stein Aug. 1, 2024, 8:32 a.m. UTC | #11

Hi,

with more and more patches for TC9595 support got meged into linux-next,
only a few remain on my patch stack.

This is one of them and is necessary for DP support:
Tested-by: Alexander Stein <[e-mail]>

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 <[e-mail]>
> ---
> Cc: Adam Ford <[e-mail]>
> Cc: Alexander Stein <[e-mail]>
> Cc: Andrzej Hajda <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> Cc: David Airlie <[e-mail]>
> Cc: Frieder Schrempf <[e-mail]>
> Cc: Inki Dae <[e-mail]>
> Cc: Jagan Teki <[e-mail]>
> Cc: Jernej Skrabec <[e-mail]>
> Cc: Jonas Karlman <[e-mail]>
> Cc: Laurent Pinchart <[e-mail]>
> Cc: Lucas Stach <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Marek Szyprowski <[e-mail]>
> Cc: Maxime Ripard <[e-mail]>
> Cc: Michael Walle <[e-mail]>
> Cc: Neil Armstrong <[e-mail]>
> Cc: Robert Foss <[e-mail]>
> Cc: Thomas Zimmermann <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> ---
> 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;
>  
>  	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 = {
>

Michael Walle Aug. 5, 2024, 1:53 p.m. UTC | #12

Hi,

> with more and more patches for TC9595 support got meged into linux-next,
> only a few remain on my patch stack.
>
> This is one of them and is necessary for DP support:
> Tested-by: Alexander Stein <[e-mail]>

As mentioned in [1] I expect a new version of this series with a
proper documentation update that will then (hopefully) be acked by
the maintainers.

Apart from that, I'm still not convinced that the .attach() callback
is the correct place to put the lanes into LP-11 mode. E.g. the
MediaTek DSI host needs to already know the HS clock rate before
configuring the D-PHY. Something which isn't available at the time
.attach() is called. Marek mentioned, that one should start with
lowest possible clock rate and later reconfigure it to the correct
rate. I'm not sure this is possible without taking the clock lanes
out of LP-11 mode again (i.e. disabling the PHY altogether to change
its PLL). But doing so will violate the bridge requirements again.

-michael

[1] https://lore.kernel.org/dri-devel/[e-mail]/

Patch

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 = {