[PATCH v3 13/17] drm/bridge: cdns-dsi: Fix REG_WAKEUP_TIME value

[Tomi Valkeinen]

On 15/04/2025 23:10, Aradhya Bhatia wrote:
> Hi Tomi,
> 
> On 14/04/25 16:41, Tomi Valkeinen wrote:
>> The driver tries to calculate the value for REG_WAKEUP_TIME. However,
>> the calculation itself is not correct, and to add on it, the resulting
>> value is almost always larger than the field's size, so the actual
>> result is more or less random.>
>> According to the docs, figuring out the value for REG_WAKEUP_TIME
>> requires HW characterization and there's no way to have a generic
>> algorithm to come up with the value. That doesn't help at all...
>>
>> However, we know that the value must be smaller than the line time, and,
>> at least in my understanding, the proper value for it is quite small.
>> Testing shows that setting it to 1/10 of the line time seems to work
>> well. All video modes from my HDMI monitor work with this algorithm.
>>
>> Hopefully we'll get more information on how to calculate the value, and
>> we can then update this.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
>> ---
>>   drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> index 182845c54c3d..fb0623d3f854 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
>> @@ -786,7 +786,13 @@ static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
>>   
>>   	tx_byte_period = DIV_ROUND_DOWN_ULL((u64)NSEC_PER_SEC * 8,
>>   					    phy_cfg->hs_clk_rate);
>> -	reg_wakeup = (phy_cfg->hs_prepare + phy_cfg->hs_zero) / tx_byte_period;
> 
> I think the primary point of failure in the original calculation is due
> to fact that the hs_prepare and hs_zero are defined in picoseconds (ps),
> and the tx_byte_period is in nanoseconds (ns) as evident by the usage of
> NSEC_PER_SEC macro.
> 
> The resulting tx_by_period is 1000 times smaller, and the reg_wakeup - a
> 1000 times larger. =)

That is true. It didn't work with that fixed either =).

> Further, the TRM does indeed mention that some characterization is
> required to fine tune the exact reg_wakeup value, but it ends up giving
> a vague-ish formula -
> 
> -> reg_wakeup_time = wakeup_time_dsi + wakeup_time_cl + wakeup_time_dl +
> 		     (hs_host_eot × 4 / lane_nb)
> 
> I think the characterization may only be required for the
> wakeup_time_dsi component. The existing formula in the driver (after
> corrected for time unit) is the wakeup_time_dl component. wakeup_time_cl
> seems to be a range of constants, which the phy-core is auto-settling on
> defaults. The document never specifically mentions "hs_host_eot" other
> than the equation, but on the off-chance it is same as phy_cfg->eot,
> then that's 0 and avoidable.

Yep, I tried to decipher how to calculate it using the TRM and the MIPI 
spec, but I just couldn't figure it out. In any case we need to 
characterization (or a known safe value that will be enough for all 
cases) to use a formula. Unfortunately I haven't gotten any details 
about this.

  Tomi