Re: [PATCH v3 1/2] phy: cadence: cdns-dphy: Fix PLL lock and O_CMN_READY polling

[Devarsh Thakkar <devarsht@ti.com>]

Hi Tomi,

Thanks for the review.

On 26/05/25 16:29, Tomi Valkeinen wrote:
> Hi,
> 
> On 02/05/2025 06:34, Devarsh Thakkar wrote:
>> PLL lockup and O_CMN_READY assertion can only happen after common state
>> machine gets enabled (by programming DPHY_CMN_SSM register), but driver was
>> polling them before the common state machine was enabled. To fix this :
>>
>> - Add new function callbacks for polling on PLL lock and O_CMN_READY
>>    assertion.
>> - As state machine and clocks get enabled in power_on callback only, move
>>    the clock related programming part from configure callback to power_on
>>    callback and poll for the PLL lockup and O_CMN_READY assertion after
>>    state machine gets enabled.
>> - The configure callback only saves the PLL configuration received from the
>>    client driver which will be applied later on in power_on callback.
>> - Add checks to ensure configure is called before power_on and state
>>    machine is in disabled state before power_on callback is called.
>> - Disable state machine in power_off so that client driver can
>>    re-configure the PLL by following up a power_off, configure, power_on
>>    sequence.
> 
> Is the DPHY & PLL documented in the TRM somewhere?
> 

I had got this information from cadence support. But I think it is also 
documented in J721E TRM [1]. DPHY Tx startup sequence is same as DPHY Tx 
and there is a initialization diagram for the same in J721E TRM [1] 
referenced at 12.7.2.4.1.2.1 Start-up Sequence Timing Diagram. It shows 
O_CMN_READY polling at the end after common configuration pin setup and 
Table 12-1533. Common Configuration-Related Setup mentions state machine 
enable part under the common configuration setup which happens before 
the polling. And the observations with this patch do sync with 
understanding as we see PLL locking up faster without any timeout which 
was the case before this patch.

> I just find the sequence a bit odd. For example, you wait for the PLL to
> lock, and after that, you enable the PLL ref clock. Maybe I'm missing
> something here, but... that should not work.

I think it's my bad, but it works somehow. But it makes sense to enable 
the clock before we start polling for PLL lock, so will update it in 
next revision.

> 
>   Tomi
> 
Regards
Devarsh