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

[Aradhya Bhatia <aradhya.bhatia@linux.dev>]

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@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. =)

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.

> +
> +	/*
> +	 * Estimated time [in clock cycles] to perform LP->HS on D-PHY.
> +	 * It is not clear how to calculate this, so for now,
> +	 * set it to 1/10 of the total number of clocks in a line.
> +	 */
> +	reg_wakeup = dsi_cfg.htotal / nlanes / 10;
>  	writel(REG_WAKEUP_TIME(reg_wakeup) | REG_LINE_DURATION(tmp),
>  	       dsi->regs + VID_DPHY_TIME);
>  
> 

-- 
Regards
Aradhya


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy