Re: [PATCH] phy: cadence: cdns-dphy: Fix PLL lock and common ready poll timeout

[Devarsh Thakkar]

Hi Tomi,

Thanks for the review.
On 06/01/25 14:25, Tomi Valkeinen wrote:

<snip>
>>                   const struct cdns_dphy_cfg *cfg);
>> +    void (*wait_for_pll_lock)(struct cdns_dphy *dphy);
>> +    void (*wait_for_cmn_ready)(struct cdns_dphy *dphy);
> 
> I think both of these should return an error code.
>

Yes I thought about this but did not see much benefit. Similar to other
cdns_dphy_ops, there is no error recovery/handling in case of timeout for
above two wait* functions. The implementer function can just print a dev_err
with timeout message as being done already. Also this maintains homogeneity
with existing cdns_dphy_ops such as set_pll_cfg which used void function as
return value.

>>       unsigned long (*get_wakeup_time_ns)(struct cdns_dphy *dphy);
>>   };
>>   @@ -191,6 +193,18 @@ static unsigned long
>> cdns_dphy_get_wakeup_time_ns(struct cdns_dphy *dphy)
>>       return dphy->ops->get_wakeup_time_ns(dphy);
>>   }
>>   +static void cdns_dphy_wait_for_pll_lock(struct cdns_dphy *dphy)
>> +{
>> +    if (dphy->ops->wait_for_pll_lock)
>> +        dphy->ops->wait_for_pll_lock(dphy);
>> +}
>> +
>> +static void cdns_dphy_wait_for_cmn_ready(struct cdns_dphy *dphy)
>> +{
>> +    if (dphy->ops->wait_for_cmn_ready)
>> +        dphy->ops->wait_for_cmn_ready(dphy);
>> +}
>> +
>>   static unsigned long cdns_dphy_ref_get_wakeup_time_ns(struct cdns_dphy *dphy)
>>   {
>>       /* Default wakeup time is 800 ns (in a simulated environment). */
>> @@ -212,7 +226,7 @@ static void cdns_dphy_ref_set_pll_cfg(struct cdns_dphy
>> *dphy,
>>       writel(DPHY_CMN_FBDIV_FROM_REG |
>>              DPHY_CMN_FBDIV_VAL(fbdiv_low, fbdiv_high),
>>              dphy->regs + DPHY_CMN_FBDIV);
>> -    writel(DPHY_CMN_PWM_HIGH(6) | DPHY_CMN_PWM_LOW(0x101) |
>> +    writel(readl(dphy->regs + DPHY_CMN_PWM) | DPHY_CMN_PWM_HIGH(6) |
>> DPHY_CMN_PWM_LOW(0x101) |
> 
> This, and the rest in the patch, is probably not a good idea. You just "or"
> the bit fields, without clearing the fields in the register. If the register
> default values contain ones (or maybe e.g. bootloader has set the register
> already), this might result in wrong value written.
> 
> Maybe add a helper to update bit fields, similar to regmap_update_bits(). Or
> read the current reg value to a local, modify there, and write back.
> 

Yes, but if we want to be totally agnostic to current state of register
assuming bootloader or some other entity has already updated some bits, then
it means that the reset values for some or all of the register bits may have
also been over-written. In that case read-modify-write be it with
regmap_update_bits or any other mechanism may not help either.

To handle this, I am thinking to use macros for default reset bits and use it
always to program concerned registers.

Regards
Devarsh