Re:Re: [PATCH] phy: rockchip: usbdp: Check these parameters only when the corresponding set flags are set

["Andy Yan" <andyshrk@163.com>]

Hi Vinod,

At 2025-03-10 15:26:50, "Vinod Koul" <vkoul@kernel.org> wrote:
>On 06-03-25, 14:59, Andy Yan wrote:
>> From: Andy Yan <andy.yan@rock-chips.com>
>> 
>> According documentation of phy_configure_opts_dp, at the configure
>> stage, we should only verify/configure the link_rate when set_rate
>> flag is set, the same applies to lanes and voltage.
>> 
>> So we do it as the documentation says, also record the link rate
>> and lanes in phy internal for set_voltate stage.
>
>Whenever you say also, that is a sign that it should be another patch!

This patch is inspired by Dmitry's suggestion for DP/eDP driver which calls phy_configure[0][1]:
the phy driver should skip the values for which the .set_foo isn't set. So we should not set rates and
lanes when we set_voltate, but the rates and lanes are needed for voltage set, this is why I  record 
rates and lanes in this patch.
I can split the record to another patch,  but then this patch will not work correctly itself, 
I'm not sure if this is suitable.


[0]https://patchwork.freedesktop.org/patch/629780/?series=141829&rev=3
[1]https://patchwork.freedesktop.org/patch/638961/?series=145286&rev=1

>
>> 
>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>> ---
>> 
>>  drivers/phy/rockchip/phy-rockchip-usbdp.c | 63 +++++++++++------------
>>  1 file changed, 31 insertions(+), 32 deletions(-)
>> 
>> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
>> index c04cf64f8a35..d1bbdf382aa2 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
>> @@ -187,6 +187,8 @@ struct rk_udphy {
>>  	u32 dp_aux_din_sel;
>>  	bool dp_sink_hpd_sel;
>>  	bool dp_sink_hpd_cfg;
>> +	unsigned int link_rate;
>> +	unsigned int lanes;
>>  	u8 bw;
>>  	int id;
>>  
>> @@ -1102,42 +1104,39 @@ static int rk_udphy_dp_phy_power_off(struct phy *phy)
>>  	return 0;
>>  }
>>  
>> -static int rk_udphy_dp_phy_verify_link_rate(unsigned int link_rate)
>> -{
>> -	switch (link_rate) {
>> -	case 1620:
>> -	case 2700:
>> -	case 5400:
>> -	case 8100:
>> -		break;
>> -
>> -	default:
>> -		return -EINVAL;
>> -	}
>> -
>> -	return 0;
>> -}
>> -
>>  static int rk_udphy_dp_phy_verify_config(struct rk_udphy *udphy,
>>  					 struct phy_configure_opts_dp *dp)
>>  {
>> -	int i, ret;
>> +	int i;
>>  
>> -	/* If changing link rate was required, verify it's supported. */
>> -	ret = rk_udphy_dp_phy_verify_link_rate(dp->link_rate);
>> -	if (ret)
>> -		return ret;
>> +	/* Verify link rate. */
>> +	if (dp->set_rate) {
>> +		switch (dp->link_rate) {
>> +		case 1620:
>> +		case 2700:
>> +		case 5400:
>> +		case 8100:
>> +			udphy->link_rate = dp->link_rate;
>> +			break;
>> +
>> +		default:
>> +			return -EINVAL;
>> +		}
>
>why drop helper? Why not set the rate on success?

I drop helper just because the flowing check for lanes and votages don't 
have helper.
The rate is set in rk_udphy_dp_phy_configure.
>
>> +	}
>>  
>>  	/* Verify lane count. */
>> -	switch (dp->lanes) {
>> -	case 1:
>> -	case 2:
>> -	case 4:
>> -		/* valid lane count. */
>> -		break;
>> +	if (dp->set_lanes) {
>> +		switch (dp->lanes) {
>> +		case 1:
>> +		case 2:
>> +		case 4:
>> +			/* valid lane count. */
>> +			udphy->lanes = dp->lanes;
>> +			break;
>>  
>> -	default:
>> -		return -EINVAL;
>> +		default:
>> +			return -EINVAL;
>> +		}
>
>another change where helper would have made this look better


Do you mean we should do it like this:

static int rk_udphy_dp_phy_configure(struct phy *phy,
                                     union phy_configure_opts *opts)
{
        struct rk_udphy *udphy = phy_get_drvdata(phy);
        struct phy_configure_opts_dp *dp = &opts->dp;
        u32 i, val, lane;
        int ret;

       .............
      if (dp->set_rate)
           	ret = rk_udphy_dp_phy_verify_link_rate(dp->link_rate);
     if (dp->set_lanes)
                ret = rk_udphy_dp_phy_verify_link_lanes(dp->lanes);
     if (dp->set_voltates)
               ret = rk_udphy_dp_phy_verify_link_voltate(dp->voltate);


Add helper for each of them ?
      


>
>>  	}
>>  
>>  	/*
>> @@ -1146,7 +1145,7 @@ static int rk_udphy_dp_phy_verify_config(struct rk_udphy *udphy,
>>  	 */
>>  	if (dp->set_voltages) {
>>  		/* Lane count verified previously. */
>> -		for (i = 0; i < dp->lanes; i++) {
>> +		for (i = 0; i < udphy->lanes; i++) {
>>  			if (dp->voltage[i] > 3 || dp->pre[i] > 3)
>>  				return -EINVAL;
>>  
>> @@ -1243,9 +1242,9 @@ static int rk_udphy_dp_phy_configure(struct phy *phy,
>>  	}
>>  
>>  	if (dp->set_voltages) {
>> -		for (i = 0; i < dp->lanes; i++) {
>> +		for (i = 0; i < udphy->lanes; i++) {
>>  			lane = udphy->dp_lane_sel[i];
>> -			switch (dp->link_rate) {
>> +			switch (udphy->link_rate) {
>>  			case 1620:
>>  			case 2700:
>>  				regmap_update_bits(udphy->pma_regmap,
>> -- 
>> 2.34.1
>
>-- 
>~Vinod
>
>_______________________________________________
>Linux-rockchip mailing list
>Linux-rockchip@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-rockchip
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip