Re: [PATCH v4 2/2] phy: cadence: cdns-dphy: Update calibration wait time for startup state machine

[Devarsh Thakkar <devarsht@ti.com>]

Hi Tomi,

Thanks for the review.
On 23/07/25 13:36, Tomi Valkeinen wrote:
> Hi,
> 
> On 04/07/2025 15:59, Devarsh Thakkar wrote:
>> Do read-modify-write so that we re-use the characterized reset value as
>> specified in TRM [1] to program calibration wait time which defines number
>> of cycles to wait for after startup state machine is in bandgap enable
>> state.
>>
>> This fixes PLL lock timeout error faced while using RPi DSI Panel on TI's
>> AM62L and J721E SoC since earlier calibration wait time was getting
>> overwritten to zero value thus failing the PLL to lockup and causing
>> timeout.
>>
>> [1] AM62P TRM (Section 14.8.6.3.2.1.1 DPHY_TX_DPHYTX_CMN0_CMN_DIG_TBIT2):
>> Link: https://www.ti.com/lit/pdf/spruj83
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 7a343c8bf4b5 ("phy: Add Cadence D-PHY support")
>> Signed-off-by: Devarsh Thakkar <devarsht@ti.com>
>> ---
>> V4: No change
>> V3:
>> - Do read-modify-write to preserve reset value for calibration wait
>>    time
>> V2:
>> Introduced this as as separate patch
>>
>>   drivers/phy/cadence/cdns-dphy.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
>> index da8de0a9d086..24a25606996c 100644
>> --- a/drivers/phy/cadence/cdns-dphy.c
>> +++ b/drivers/phy/cadence/cdns-dphy.c
>> @@ -30,6 +30,7 @@
>>   
>>   #define DPHY_CMN_SSM			DPHY_PMA_CMN(0x20)
>>   #define DPHY_CMN_SSM_EN			BIT(0)
>> +#define DPHY_CMN_SSM_CAL_WAIT_TIME	GENMASK(8, 1)
>>   #define DPHY_CMN_TX_MODE_EN		BIT(9)
>>   
>>   #define DPHY_CMN_PWM			DPHY_PMA_CMN(0x40)
>> @@ -410,7 +411,8 @@ static int cdns_dphy_power_on(struct phy *phy)
>>   	writel(reg, dphy->regs + DPHY_BAND_CFG);
>>   
>>   	/* Start TX state machine. */
>> -	writel(DPHY_CMN_SSM_EN | DPHY_CMN_TX_MODE_EN,
>> +	reg = readl(dphy->regs + DPHY_CMN_SSM);
>> +	writel((reg & DPHY_CMN_SSM_CAL_WAIT_TIME) | DPHY_CMN_SSM_EN | DPHY_CMN_TX_MODE_EN,
>>   	       dphy->regs + DPHY_CMN_SSM);
> 
> That's not how you do read-modify-write. You should first read the
> register, then clear the fields you want to set/clear, then set the
> fields, then write.
> 
> reg = readl(dphy->regs + DPHY_CMN_SSM, dphy->regs + DPHY_CMN_SSM);
> reg &= ~(DPHY_CMN_SSM_EN | DPHY_CMN_TX_MODE_EN)
> reg |= DPHY_CMN_SSM_EN | DPHY_CMN_TX_MODE_EN;
> writel(reg, dphy->regs + DPHY_CMN_SSM);
> 
> That's the general form, in this particular case the and-line is not
> necessary, of course.
> 
> There's also FIELD_MODIFY() (and related), but I'm not sure if I like
> them... Up to you.

As discussed offline, here we just wanted to preserve the value for 
DPHY_CMN_SSM_CAL_WAIT_TIME bits as these are holding the calibration 
wait time, reset rest and then enable the SSM by setting DPHY_CMN_SSM_EN 
| DPHY_CMN_TX_MODE_EN and the patch rightly achieves this. I assume we 
are aligned on this as I see your review here [1].

Kindly let me know if otherwise.

[1]: 
https://lore.kernel.org/all/2bec3583-6078-4650-a8d0-6cfe8fcec3f3@ideasonboard.com/

Regards
Devarsh

> 
>   Tomi
>