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

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

[Andy Yan]

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.

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;
+		}
+	}
 
 	/* 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;
+		}
 	}
 
 	/*
@@ -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

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

[Vinod Koul]

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!

> 
> 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?

> +	}
>  
>  	/* 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

>  	}
>  
>  	/*
> @@ -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

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

[Andy Yan]

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

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

[Vinod Koul]

On 11-03-25, 08:52, Andy Yan wrote:

> 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 ?

That would look better for sure

-- 
~Vinod

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

[Andy Yan]

Hi Vinod,
At 2025-03-11 19:13:46, "Vinod Koul" <vkoul@kernel.org> wrote:
>On 11-03-25, 08:52, Andy Yan wrote:
>
>> 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 ?
>
>That would look better for sure

I will do as that in V2.

Thanks

>
>-- 
>~Vinod