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

[Devarsh Thakkar]

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@xxxxxxxxxxxxxxx
> > Fixes: 7a343c8bf4b5 ("phy: Add Cadence D-PHY support")
> > Signed-off-by: Devarsh Thakkar <devarsht@xxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxx/

Regards
Devarsh

>   Tomi