Re: [PATCH v2 1/4] phy: zynqmp: Enable reference clock correctly

[Sean Anderson]

On 6/14/24 01:30, Pandey, Radhey Shyam wrote:
>> -----Original Message-----
>> From: Sean Anderson <sean.anderson@xxxxxxxxx>
>> Sent: Monday, May 6, 2024 10:31 PM
>> To: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>; linux-
>> phy@xxxxxxxxxxxxxxxxxxx
>> Cc: Vinod Koul <vkoul@xxxxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
>> linux-kernel@xxxxxxxxxxxxxxx; Michal Simek <michal.simek@xxxxxxx>;
>> Kishon Vijay Abraham I <kishon@xxxxxxxxxx>; Sean Anderson
>> <sean.anderson@xxxxxxxxx>
>> Subject: [PATCH v2 1/4] phy: zynqmp: Enable reference clock correctly
>> 
>> Lanes can use other lanes' reference clocks, as determined by refclk.
>> Use refclk to determine the clock to enable/disable instead of always
>> using the lane's own reference clock. This ensures the clock selected in
>> xpsgtr_configure_pll is the one enabled.
>> 
>> For the other half of the equation, always program REF_CLK_SEL even when
>> we are selecting the lane's own clock. This ensures that Linux's idea of
>> the reference clock matches the hardware. We use the "local" clock mux
>> for this instead of going through the ref clock network.
>> 
>> Fixes: 25d700833513 ("phy: xilinx: phy-zynqmp: dynamic clock support for
>> power-save")
>> Signed-off-by: Sean Anderson <sean.anderson@xxxxxxxxx>
>> ---
>> 
>> Changes in v2:
>> - New
>> 
>>  drivers/phy/xilinx/phy-zynqmp.c | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-
>> zynqmp.c
>> index f72c5257d712..5a434382356c 100644
>> --- a/drivers/phy/xilinx/phy-zynqmp.c
>> +++ b/drivers/phy/xilinx/phy-zynqmp.c
>> @@ -80,7 +80,8 @@
>> 
>>  /* Reference clock selection parameters */
>>  #define L0_Ln_REF_CLK_SEL(n)		(0x2860 + (n) * 4)
>> -#define L0_REF_CLK_SEL_MASK		0x8f
>> +#define L0_REF_CLK_LCL_SEL		BIT(7)
>> +#define L0_REF_CLK_SEL_MASK		0x9f
>> 
>>  /* Calibration digital logic parameters */
>>  #define L3_TM_CALIB_DIG19		0xec4c
>> @@ -349,11 +350,14 @@ static void xpsgtr_configure_pll(struct xpsgtr_phy
>> *gtr_phy)
>>  		       PLL_FREQ_MASK, ssc->pll_ref_clk);
>> 
>>  	/* Enable lane clock sharing, if required */
>> -	if (gtr_phy->refclk != gtr_phy->lane) {
>> +	if (gtr_phy->refclk == gtr_phy->lane)
>> +		/* Lane3 Ref Clock Selection Register */
> 
> This is common ref clock selection and not lane 3?

This is copied from the existing comment. I will remove it.

--Sean

>> +		xpsgtr_clr_set(gtr_phy->dev, L0_Ln_REF_CLK_SEL(gtr_phy-
>> >lane),
>> +			       L0_REF_CLK_SEL_MASK, L0_REF_CLK_LCL_SEL);
>> +	else
>>  		/* Lane3 Ref Clock Selection Register */
>>  		xpsgtr_clr_set(gtr_phy->dev, L0_Ln_REF_CLK_SEL(gtr_phy-
>> >lane),
>>  			       L0_REF_CLK_SEL_MASK, 1 << gtr_phy->refclk);
>> -	}
>> 
>>  	/* SSC step size [7:0] */
>>  	xpsgtr_clr_set_phy(gtr_phy, L0_PLL_SS_STEP_SIZE_0_LSB,
>> @@ -573,7 +577,7 @@ static int xpsgtr_phy_init(struct phy *phy)
>>  	mutex_lock(&gtr_dev->gtr_mutex);
>> 
>>  	/* Configure and enable the clock when peripheral phy_init call */
>> -	if (clk_prepare_enable(gtr_dev->clk[gtr_phy->lane]))
>> +	if (clk_prepare_enable(gtr_dev->clk[gtr_phy->refclk]))
>>  		goto out;
>> 
>>  	/* Skip initialization if not required. */
>> @@ -625,7 +629,7 @@ static int xpsgtr_phy_exit(struct phy *phy)
>>  	gtr_phy->skip_phy_init = false;
>> 
>>  	/* Ensure that disable clock only, which configure for lane */
>> -	clk_disable_unprepare(gtr_dev->clk[gtr_phy->lane]);
>> +	clk_disable_unprepare(gtr_dev->clk[gtr_phy->refclk]);
>> 
>>  	return 0;
>>  }
>> --
>> 2.35.1.1320.gc452695387.dirty
>