Re: [PATCH v2 03/18] drm/tidss: Adjust the pclk based on the HW capabilities

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

Hi Michael (and Aradhya, Devarsh),

On 27/05/2025 12:13, Michael Walle wrote:
> Hi Tomi,
> 
> While testing Aardhya's OLDI support patches [1], I've noticed that
> the resulting LVDS clock is wrong if this patch is applied.
> 
>> In practice, with the current K3 SoCs, the display PLL is capable of
>> producing very exact clocks, so most likely the rounded rate is the same
>> as the original one.
> 
> This is now what I'm seeing. Most SoCs have that fixed clock thingy
> for (some?) VPs, e.g. [2]. And clk_round_rate() will return the
> fixed clock rate for this clock, which will then result in an LVDS
> clock which is way off.
> 
> I'm testing on an AM67A (J722S) and I've backported some of the
> patches as well as dtsi fragmets from downstream. Thus, it might be
> as well the case that the fixed-factor-clock node is wrong here.
> OTOH other K3 SoCs do this in mainline as well.

Thanks for findings this (It's not a fixed clock, but a (fixed)
divider). I can reproduce on my AM62 SK's OLDI output.

I didn't see AM625 TRM explaining the DSS + OLDI clocking. I remember it
was a bit "interesting". Afaics from testing, the VP clock is derived
from the OLDI serial clock divided by 7. To change the VP clock, we need
to set the OLDI clock's rate. But the code we have at the moment is
using clk_round_rate/set_rate to the VP clock.

And we get the crtc atomic_check called before setting the OLDI clock
rate, so it doesn't even work by luck (i.e. if the OLDI clock was set
earlier, the VP clock would already have the right rate, and it would
seem that everything is ok). In the atomic_check we see the OLDI bypass
clock (25 MHz), which results in 3571428 Hz VP clock.

And with this patch, the code then decides that 3571428 Hz is what the
HW can do, and uses it as the pixel clock.

Aradhya, Devarsh, do you remember how the clocking goes here? Or if it's
in the TRM, please point me to it...

 Tomi

>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>>  drivers/gpu/drm/tidss/tidss_crtc.c  | 23 +++++++++++++++++++----
>>  drivers/gpu/drm/tidss/tidss_dispc.c |  6 ++++++
>>  drivers/gpu/drm/tidss/tidss_dispc.h |  2 ++
>>  3 files changed, 27 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
>> index 1604eca265ef..6c3967f70510 100644
>> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
>> @@ -91,7 +91,7 @@ static int tidss_crtc_atomic_check(struct drm_crtc *crtc,
>>  	struct dispc_device *dispc = tidss->dispc;
>>  	struct tidss_crtc *tcrtc = to_tidss_crtc(crtc);
>>  	u32 hw_videoport = tcrtc->hw_videoport;
>> -	const struct drm_display_mode *mode;
>> +	struct drm_display_mode *adjusted_mode;
>>  	enum drm_mode_status ok;
>>  
>>  	dev_dbg(ddev->dev, "%s\n", __func__);
>> @@ -99,12 +99,27 @@ static int tidss_crtc_atomic_check(struct drm_crtc *crtc,
>>  	if (!crtc_state->enable)
>>  		return 0;
>>  
>> -	mode = &crtc_state->adjusted_mode;
>> +	adjusted_mode = &crtc_state->adjusted_mode;
> 
> Here, adjusted_mode->clock is still the correct pixel clock.
> 
>> -	ok = dispc_vp_mode_valid(dispc, hw_videoport, mode);
>> +	if (drm_atomic_crtc_needs_modeset(crtc_state)) {
>> +		long rate;
>> +
>> +		rate = dispc_vp_round_clk_rate(tidss->dispc,
>> +					       tcrtc->hw_videoport,
>> +					       adjusted_mode->clock * 1000);
>> +		if (rate < 0)
>> +			return -EINVAL;
>> +
>> +		adjusted_mode->clock = rate / 1000;
> 
> While after this statement, adjusted_mode->clock is 300MHz in my
> case (the VP1 clock seems to be 2.1GHz, divided by 7).
> 
> -michael
> 
> [1] https://lore.kernel.org/all/20250525151721.567042-1-aradhya.bhatia@linux.dev/
> [2] https://elixir.bootlin.com/linux/v6.15/source/arch/arm64/boot/dts/ti/k3-am62.dtsi#L110
> 
>> +
>> +		drm_mode_set_crtcinfo(adjusted_mode, 0);
>> +	}
>> +
>> +	ok = dispc_vp_mode_valid(dispc, hw_videoport, adjusted_mode);
>>  	if (ok != MODE_OK) {
>>  		dev_dbg(ddev->dev, "%s: bad mode: %ux%u pclk %u kHz\n",
>> -			__func__, mode->hdisplay, mode->vdisplay, mode->clock);
>> +			__func__, adjusted_mode->hdisplay,
>> +			adjusted_mode->vdisplay, adjusted_mode->clock); >  		return -EINVAL;
>>  	}
>>  
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.c b/drivers/gpu/drm/tidss/tidss_dispc.c
>> index a5107f2732b1..3930fb7f03c2 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.c
>> @@ -1318,6 +1318,12 @@ unsigned int dispc_pclk_diff(unsigned long rate, unsigned long real_rate)
>>  	return (unsigned int)(abs(((rr - r) * 100) / r));
>>  }
>>  
>> +long dispc_vp_round_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
>> +			     unsigned long rate)
>> +{
>> +	return clk_round_rate(dispc->vp_clk[hw_videoport], rate);
>> +}
>> +
>>  int dispc_vp_set_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
>>  			  unsigned long rate)
>>  {
>> diff --git a/drivers/gpu/drm/tidss/tidss_dispc.h b/drivers/gpu/drm/tidss/tidss_dispc.h
>> index c31b477a18b0..d4c335e918fb 100644
>> --- a/drivers/gpu/drm/tidss/tidss_dispc.h
>> +++ b/drivers/gpu/drm/tidss/tidss_dispc.h
>> @@ -120,6 +120,8 @@ enum drm_mode_status dispc_vp_mode_valid(struct dispc_device *dispc,
>>  					 const struct drm_display_mode *mode);
>>  int dispc_vp_enable_clk(struct dispc_device *dispc, u32 hw_videoport);
>>  void dispc_vp_disable_clk(struct dispc_device *dispc, u32 hw_videoport);
>> +long dispc_vp_round_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
>> +			     unsigned long rate);
>>  int dispc_vp_set_clk_rate(struct dispc_device *dispc, u32 hw_videoport,
>>  			  unsigned long rate);
>>  void dispc_vp_setup(struct dispc_device *dispc, u32 hw_videoport,
>