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

[Devarsh Thakkar <devarsht@ti.com>]

Hello,

On 20/06/25 20:05, Devarsh Thakkar wrote:
> 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.
> 

Sorry, had missed to add the reference :

[1]: https://www.ti.com/lit/zip/spruil1

Regards
Devarsh

>>
>>   Tomi
>>
> Regards
> Devarsh