[PATCH v3 3/3] phy: rockchip: phy-rockchip-inno-csidphy: add clock lane phase tuning

[Gerald Loacker]

Hi Michael,

Thanks for your comments and suggestions.

Am 10.07.2026 um 11:31 schrieb Michael Riesch:
> Hi Gerald,
> 
> Thanks for your patch.
> 
> On 6/30/26 09:48, Gerald Loacker wrote:
>> At high data rates like 4K60 (2500 Mbps), such as when using an
>> LT6911GXD bridge chip on an RK3588 board, fixed default timing parameters
>> can cause signal integrity issues and clock-data recovery failures.
>> The driver currently lacks a mechanism to adjust the clock lane sampling
>> phase to compensate for board-specific trace variations.
>>
>> Resolve this by parsing and applying the optional 'rockchip,clk-lane-phase'
>> device tree property. This enables board-specific tuning of the clock
>> lane sampling phase in ~40 ps steps (range 0-7) to optimize link
>> stability. If the property is absent, the driver falls back to the
>> hardware default.
>>
>> Signed-off-by: Gerald Loacker <gerald.loacker at wolfvision.net>
>> ---
>>  drivers/phy/rockchip/phy-rockchip-inno-csidphy.c | 25 ++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-csidphy.c b/drivers/phy/rockchip/phy-rockchip-inno-csidphy.c
>> index 5281f8dea0ad3..3a15840e86cad 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-inno-csidphy.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-inno-csidphy.c
>> @@ -69,6 +69,10 @@
>>  #define RK1808_CSIDPHY_CLK_CALIB_EN		0x168
>>  #define RK3568_CSIDPHY_CLK_CALIB_EN		0x168
>>  
>> +#define CSIDPHY_LANE_CLK_3_PHASE		0x38
>> +#define CSIDPHY_CLK_PHASE_MASK			GENMASK(6, 4)
>> +#define CSIDPHY_CLK_PHASE_DEFAULT		3
> 
> This default value definition is unused right now, but...
> 

ok, I will use this default value in v4

>> +
>>  #define RESETS_MAX				2
>>  
>>  /*
>> @@ -151,6 +155,7 @@ struct rockchip_inno_csidphy {
>>  	const struct dphy_drv_data *drv_data;
>>  	struct phy_configure_opts_mipi_dphy config;
>>  	u8 hsfreq;
>> +	int clk_phase;
>>  };
>>  
>>  static inline void write_grf_reg(struct rockchip_inno_csidphy *priv,
>> @@ -304,6 +309,13 @@ static int rockchip_inno_csidphy_power_on(struct phy *phy)
>>  		rockchip_inno_csidphy_ths_settle(priv, priv->hsfreq,
>>  						 CSIDPHY_LANE_THS_SETTLE(i));
>>  
>> +	if (priv->clk_phase >= 0) {
> 
> ...you can make sure that clk_phase has a valid value in any case (apply
> default value defined above if DT does not define it or defines
> something invalid) and write the register unconditionally.
> 

see below

>> +		val = readl(priv->phy_base + CSIDPHY_LANE_CLK_3_PHASE);
>> +		val &= ~CSIDPHY_CLK_PHASE_MASK;
>> +		val |= FIELD_PREP(CSIDPHY_CLK_PHASE_MASK, priv->clk_phase);
>> +		writel(val, priv->phy_base + CSIDPHY_LANE_CLK_3_PHASE);
>> +	}
>> +
>>  	write_grf_reg(priv, GRF_DPHY_CSIPHY_CLKLANE_EN, 0x1);
>>  	write_grf_reg(priv, GRF_DPHY_CSIPHY_DATALANE_EN,
>>  		      GENMASK(priv->config.lanes - 1, 0));
>> @@ -449,6 +461,7 @@ static int rockchip_inno_csidphy_probe(struct platform_device *pdev)
>>  	struct device *dev = &pdev->dev;
>>  	struct phy_provider *phy_provider;
>>  	struct phy *phy;
>> +	u32 phase;
>>  	int ret;
>>  
>>  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> @@ -464,6 +477,18 @@ static int rockchip_inno_csidphy_probe(struct platform_device *pdev)
>>  		return -ENODEV;
>>  	}
>>  
>> +	priv->clk_phase = -1;
>> +	if (device_property_read_u32(dev, "rockchip,clk-lane-phase",
>> +				     &phase) == 0) {
>> +		if (phase >= BIT(3)) {
> 
> if (phase > 7)
> 

Yes, that reads a bit easier.

>> +			dev_err(dev,
>> +				"rockchip,clk-lane-phase %u out of range [0,7]\n",
>> +				phase);
>> +			return -EINVAL;
> 
> Seems a bit harsh. What would you think about printing a warning and
> applying the default value?
> 

Good input, will do that.

>> +		}
>> +		priv->clk_phase = phase;
>> +	}
> 
> Maybe
> 
> 	ret = device_property_read_u32(dev, "rockchip,clk-lane-phase",
> 				       &priv->clk_phase);
> 	if (ret < 0 || priv->clk_phase > 7) {
> 		dev_info(dev,
> 			 "found %s value for rockchip,clk-lane-phase,"
> 			 "assuming default value",
> 			 ret < 0 ? "no" : "invalid");
> 		priv->clk_phase = CSIDPHY_CLK_PHASE_DEFAULT;
> 	}
> 
> would do the trick too?
> 

I would not write the default value to the register in case the property
is not defined in the DTS, just to make sure there is absolutely no
change to existing implementations. If you agree, I will send a v4
including your above comments, but sticking more to the original version.

Best regards,
Gerald

> Best regards,
> Michael
> 
>> +
>>  	priv->grf = syscon_regmap_lookup_by_phandle(dev->of_node,
>>  						    "rockchip,grf");
>>  	if (IS_ERR(priv->grf)) {
>>
> 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip