diff mbox series

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

Message ID 20250306065952.485809-1-andyshrk@163.com (mailing list archive)
State New
Headers show
Series phy: rockchip: usbdp: Check these parameters only when the corresponding set flags are set | expand

Commit Message

Andy Yan March 6, 2025, 6:59 a.m. UTC 
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(-)

Comments

Vinod Koul March 10, 2025, 7:26 a.m. UTC | #1 
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
Andy Yan March 11, 2025, 12:52 a.m. UTC | #2 
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
Vinod Koul March 11, 2025, 11:13 a.m. UTC | #3 
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
Andy Yan March 11, 2025, 12:22 p.m. UTC | #4 
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
diff mbox series

Patch

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,