[PATCH 0/3] usb: typec: ps883x: follow-up fixes

[PATCH 0/3] usb: typec: ps883x: follow-up fixes

[Johan Hovold]

Here are some further follow-up fixes for issues with the new ps883x
retimer driver.

They don't strictly depend on the probe error handling fix I posted
earlier today, so I'm not including it here even if that one ideally
should be applied before this series:

	https://lore.kernel.org/lkml/20250218082243.9318-1-johan+linaro@kernel.org/

Johan


Johan Hovold (3):
  usb: typec: ps883x: fix registration race
  usb: typec: ps883x: fix missing accessibility check
  usb: typec: ps883x: fix configuration error handling

 drivers/usb/typec/mux/ps883x.c | 78 +++++++++++++++++++++++-----------
 1 file changed, 53 insertions(+), 25 deletions(-)

-- 
2.45.3

[PATCH 1/3] usb: typec: ps883x: fix registration race

[Johan Hovold]

Make sure that the retimer is fully setup before registering it to avoid
having consumers try to access it while it is being reset.

Fixes: 257a087c8b52 ("usb: typec: Add support for Parade PS8830 Type-C Retimer")
Cc: Abel Vesa <abel.vesa@linaro.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/usb/typec/mux/ps883x.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/typec/mux/ps883x.c b/drivers/usb/typec/mux/ps883x.c
index ef086989231f..274de7abe585 100644
--- a/drivers/usb/typec/mux/ps883x.c
+++ b/drivers/usb/typec/mux/ps883x.c
@@ -346,6 +346,22 @@ static int ps883x_retimer_probe(struct i2c_client *client)
 		goto err_vregs_disable;
 	}
 
+	/* skip resetting if already configured */
+	if (regmap_test_bits(retimer->regmap, REG_USB_PORT_CONN_STATUS_0,
+			     CONN_STATUS_0_CONNECTION_PRESENT) == 1) {
+		gpiod_direction_output(retimer->reset_gpio, 0);
+	} else {
+		gpiod_direction_output(retimer->reset_gpio, 1);
+
+		/* VDD IO supply enable to reset release delay */
+		usleep_range(4000, 14000);
+
+		gpiod_set_value(retimer->reset_gpio, 0);
+
+		/* firmware initialization delay */
+		msleep(60);
+	}
+
 	sw_desc.drvdata = retimer;
 	sw_desc.fwnode = dev_fwnode(dev);
 	sw_desc.set = ps883x_sw_set;
@@ -368,21 +384,6 @@ static int ps883x_retimer_probe(struct i2c_client *client)
 		goto err_switch_unregister;
 	}
 
-	/* skip resetting if already configured */
-	if (regmap_test_bits(retimer->regmap, REG_USB_PORT_CONN_STATUS_0,
-			     CONN_STATUS_0_CONNECTION_PRESENT) == 1)
-		return gpiod_direction_output(retimer->reset_gpio, 0);
-
-	gpiod_direction_output(retimer->reset_gpio, 1);
-
-	/* VDD IO supply enable to reset release delay */
-	usleep_range(4000, 14000);
-
-	gpiod_set_value(retimer->reset_gpio, 0);
-
-	/* firmware initialization delay */
-	msleep(60);
-
 	return 0;
 
 err_switch_unregister:
-- 
2.45.3

Re: [PATCH 1/3] usb: typec: ps883x: fix registration race

[Heikki Krogerus]

On Tue, Feb 18, 2025 at 04:29:31PM +0100, Johan Hovold wrote:
> Make sure that the retimer is fully setup before registering it to avoid
> having consumers try to access it while it is being reset.
> 
> Fixes: 257a087c8b52 ("usb: typec: Add support for Parade PS8830 Type-C Retimer")
> Cc: Abel Vesa <abel.vesa@linaro.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/mux/ps883x.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/usb/typec/mux/ps883x.c b/drivers/usb/typec/mux/ps883x.c
> index ef086989231f..274de7abe585 100644
> --- a/drivers/usb/typec/mux/ps883x.c
> +++ b/drivers/usb/typec/mux/ps883x.c
> @@ -346,6 +346,22 @@ static int ps883x_retimer_probe(struct i2c_client *client)
>  		goto err_vregs_disable;
>  	}
>  
> +	/* skip resetting if already configured */
> +	if (regmap_test_bits(retimer->regmap, REG_USB_PORT_CONN_STATUS_0,
> +			     CONN_STATUS_0_CONNECTION_PRESENT) == 1) {
> +		gpiod_direction_output(retimer->reset_gpio, 0);
> +	} else {
> +		gpiod_direction_output(retimer->reset_gpio, 1);
> +
> +		/* VDD IO supply enable to reset release delay */
> +		usleep_range(4000, 14000);
> +
> +		gpiod_set_value(retimer->reset_gpio, 0);
> +
> +		/* firmware initialization delay */
> +		msleep(60);
> +	}
> +
>  	sw_desc.drvdata = retimer;
>  	sw_desc.fwnode = dev_fwnode(dev);
>  	sw_desc.set = ps883x_sw_set;
> @@ -368,21 +384,6 @@ static int ps883x_retimer_probe(struct i2c_client *client)
>  		goto err_switch_unregister;
>  	}
>  
> -	/* skip resetting if already configured */
> -	if (regmap_test_bits(retimer->regmap, REG_USB_PORT_CONN_STATUS_0,
> -			     CONN_STATUS_0_CONNECTION_PRESENT) == 1)
> -		return gpiod_direction_output(retimer->reset_gpio, 0);
> -
> -	gpiod_direction_output(retimer->reset_gpio, 1);
> -
> -	/* VDD IO supply enable to reset release delay */
> -	usleep_range(4000, 14000);
> -
> -	gpiod_set_value(retimer->reset_gpio, 0);
> -
> -	/* firmware initialization delay */
> -	msleep(60);
> -
>  	return 0;
>  
>  err_switch_unregister:
> -- 
> 2.45.3

-- 
heikki

[PATCH 2/3] usb: typec: ps883x: fix missing accessibility check

[Johan Hovold]

Make sure that the retimer is accessible before registering to avoid
having later consumer calls fail to configure it, something which, for
example, can lead to a hotplugged display not being recognised:

	[drm:msm_dp_panel_read_sink_caps [msm]] *ERROR* read dpcd failed -110

Fixes: 257a087c8b52 ("usb: typec: Add support for Parade PS8830 Type-C Retimer")
Cc: Abel Vesa <abel.vesa@linaro.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/usb/typec/mux/ps883x.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/usb/typec/mux/ps883x.c b/drivers/usb/typec/mux/ps883x.c
index 274de7abe585..f8b47187f4cf 100644
--- a/drivers/usb/typec/mux/ps883x.c
+++ b/drivers/usb/typec/mux/ps883x.c
@@ -291,6 +291,7 @@ static int ps883x_retimer_probe(struct i2c_client *client)
 	struct typec_switch_desc sw_desc = { };
 	struct typec_retimer_desc rtmr_desc = { };
 	struct ps883x_retimer *retimer;
+	unsigned int val;
 	int ret;
 
 	retimer = devm_kzalloc(dev, sizeof(*retimer), GFP_KERNEL);
@@ -360,6 +361,16 @@ static int ps883x_retimer_probe(struct i2c_client *client)
 
 		/* firmware initialization delay */
 		msleep(60);
+
+		/* make sure device is accessible */
+		ret = regmap_read(retimer->regmap, REG_USB_PORT_CONN_STATUS_0,
+				  &val);
+		if (ret) {
+			dev_err(dev, "failed to read conn_status_0: %d\n", ret);
+			if (ret == -ENXIO)
+				ret = -EIO;
+			goto err_clk_disable;
+		}
 	}
 
 	sw_desc.drvdata = retimer;
-- 
2.45.3

Re: [PATCH 2/3] usb: typec: ps883x: fix missing accessibility check

[Heikki Krogerus]

On Tue, Feb 18, 2025 at 04:29:32PM +0100, Johan Hovold wrote:
> Make sure that the retimer is accessible before registering to avoid
> having later consumer calls fail to configure it, something which, for
> example, can lead to a hotplugged display not being recognised:
> 
> 	[drm:msm_dp_panel_read_sink_caps [msm]] *ERROR* read dpcd failed -110
> 
> Fixes: 257a087c8b52 ("usb: typec: Add support for Parade PS8830 Type-C Retimer")
> Cc: Abel Vesa <abel.vesa@linaro.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/mux/ps883x.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/usb/typec/mux/ps883x.c b/drivers/usb/typec/mux/ps883x.c
> index 274de7abe585..f8b47187f4cf 100644
> --- a/drivers/usb/typec/mux/ps883x.c
> +++ b/drivers/usb/typec/mux/ps883x.c
> @@ -291,6 +291,7 @@ static int ps883x_retimer_probe(struct i2c_client *client)
>  	struct typec_switch_desc sw_desc = { };
>  	struct typec_retimer_desc rtmr_desc = { };
>  	struct ps883x_retimer *retimer;
> +	unsigned int val;
>  	int ret;
>  
>  	retimer = devm_kzalloc(dev, sizeof(*retimer), GFP_KERNEL);
> @@ -360,6 +361,16 @@ static int ps883x_retimer_probe(struct i2c_client *client)
>  
>  		/* firmware initialization delay */
>  		msleep(60);
> +
> +		/* make sure device is accessible */
> +		ret = regmap_read(retimer->regmap, REG_USB_PORT_CONN_STATUS_0,
> +				  &val);
> +		if (ret) {
> +			dev_err(dev, "failed to read conn_status_0: %d\n", ret);
> +			if (ret == -ENXIO)
> +				ret = -EIO;
> +			goto err_clk_disable;
> +		}
>  	}
>  
>  	sw_desc.drvdata = retimer;
> -- 
> 2.45.3

-- 
heikki

Re: [PATCH 2/3] usb: typec: ps883x: fix missing accessibility check

[Jens Glathe]

Hi Johan,

On 2/18/25 16:29, Johan Hovold wrote:
> Make sure that the retimer is accessible before registering to avoid
> having later consumer calls fail to configure it, something which, for
> example, can lead to a hotplugged display not being recognised:
>
> 	[drm:msm_dp_panel_read_sink_caps [msm]] *ERROR* read dpcd failed -110
>
> Fixes: 257a087c8b52 ("usb: typec: Add support for Parade PS8830 Type-C Retimer")
> Cc: Abel Vesa <abel.vesa@linaro.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

unfortunately, this one goes south on the HP Omnibook X14, and also on
the Elitebook G1Q. After excluding a lot of other causes, like inverted
resets and wrong i2c channels, I did a bisect and landed at this commit.

Looking at it, I speculatively increased the firmware initialization
delay to 200ms. To no effect. Reverting this patch "resolves" the issue.

with best regards

Jens

Re: [PATCH 2/3] usb: typec: ps883x: fix missing accessibility check

[Johan Hovold]

Hi Jens,

On Sun, Mar 02, 2025 at 02:34:41PM +0100, Jens Glathe wrote:
> On 2/18/25 16:29, Johan Hovold wrote:
> > Make sure that the retimer is accessible before registering to avoid
> > having later consumer calls fail to configure it, something which, for
> > example, can lead to a hotplugged display not being recognised:
> >
> > 	[drm:msm_dp_panel_read_sink_caps [msm]] *ERROR* read dpcd failed -110
> >
> > Fixes: 257a087c8b52 ("usb: typec: Add support for Parade PS8830 Type-C Retimer")
> > Cc: Abel Vesa <abel.vesa@linaro.org>
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> 
> unfortunately, this one goes south on the HP Omnibook X14, and also on
> the Elitebook G1Q. After excluding a lot of other causes, like inverted
> resets and wrong i2c channels, I did a bisect and landed at this commit.

According to the X14 ACPI tables there is no ps8830 on &i2c7i (I2C8),
which means that the devicetree is broken.

> Looking at it, I speculatively increased the firmware initialization
> delay to 200ms. To no effect. Reverting this patch "resolves" the issue.

This patch (series) only makes sure that there actually is a retimer at
the described address so it appears to work as intended.

You may unknowingly have been relying on firmware configuration or reset
values. Does orientation switching (SuperSpeed in both orientations) and
DP altmode work at all on the second USB-C port with this patch
reverted?

Johan

[PATCH 3/3] usb: typec: ps883x: fix configuration error handling

[Johan Hovold]

Propagate errors to the consumers when configuring the retimer so that
they can act on any failures as intended, for example:

	ps883x_retimer 2-0008: failed to write conn_status_0: -5
	pmic_glink_altmode.pmic_glink_altmode pmic_glink.altmode.0: failed to setup retimer to DP: -5

Fixes: 257a087c8b52 ("usb: typec: Add support for Parade PS8830 Type-C Retimer")
Cc: Abel Vesa <abel.vesa@linaro.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/usb/typec/mux/ps883x.c | 36 ++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/typec/mux/ps883x.c b/drivers/usb/typec/mux/ps883x.c
index f8b47187f4cf..ad59babf7cce 100644
--- a/drivers/usb/typec/mux/ps883x.c
+++ b/drivers/usb/typec/mux/ps883x.c
@@ -58,12 +58,31 @@ struct ps883x_retimer {
 	unsigned int svid;
 };
 
-static void ps883x_configure(struct ps883x_retimer *retimer, int cfg0,
-			     int cfg1, int cfg2)
+static int ps883x_configure(struct ps883x_retimer *retimer, int cfg0,
+			    int cfg1, int cfg2)
 {
-	regmap_write(retimer->regmap, REG_USB_PORT_CONN_STATUS_0, cfg0);
-	regmap_write(retimer->regmap, REG_USB_PORT_CONN_STATUS_1, cfg1);
-	regmap_write(retimer->regmap, REG_USB_PORT_CONN_STATUS_2, cfg2);
+	struct device *dev = &retimer->client->dev;
+	int ret;
+
+	ret = regmap_write(retimer->regmap, REG_USB_PORT_CONN_STATUS_0, cfg0);
+	if (ret) {
+		dev_err(dev, "failed to write conn_status_0: %d\n", ret);
+		return ret;
+	}
+
+	ret = regmap_write(retimer->regmap, REG_USB_PORT_CONN_STATUS_1, cfg1);
+	if (ret) {
+		dev_err(dev, "failed to write conn_status_1: %d\n", ret);
+		return ret;
+	}
+
+	ret = regmap_write(retimer->regmap, REG_USB_PORT_CONN_STATUS_2, cfg2);
+	if (ret) {
+		dev_err(dev, "failed to write conn_status_2: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
 }
 
 static int ps883x_set(struct ps883x_retimer *retimer)
@@ -74,8 +93,7 @@ static int ps883x_set(struct ps883x_retimer *retimer)
 
 	if (retimer->orientation == TYPEC_ORIENTATION_NONE ||
 	    retimer->mode == TYPEC_STATE_SAFE) {
-		ps883x_configure(retimer, cfg0, cfg1, cfg2);
-		return 0;
+		return ps883x_configure(retimer, cfg0, cfg1, cfg2);
 	}
 
 	if (retimer->mode != TYPEC_STATE_USB && retimer->svid != USB_TYPEC_DP_SID)
@@ -113,9 +131,7 @@ static int ps883x_set(struct ps883x_retimer *retimer)
 		return -EOPNOTSUPP;
 	}
 
-	ps883x_configure(retimer, cfg0, cfg1, cfg2);
-
-	return 0;
+	return ps883x_configure(retimer, cfg0, cfg1, cfg2);
 }
 
 static int ps883x_sw_set(struct typec_switch_dev *sw,
-- 
2.45.3

Re: [PATCH 3/3] usb: typec: ps883x: fix configuration error handling

[Heikki Krogerus]

On Tue, Feb 18, 2025 at 04:29:33PM +0100, Johan Hovold wrote:
> Propagate errors to the consumers when configuring the retimer so that
> they can act on any failures as intended, for example:
> 
> 	ps883x_retimer 2-0008: failed to write conn_status_0: -5
> 	pmic_glink_altmode.pmic_glink_altmode pmic_glink.altmode.0: failed to setup retimer to DP: -5
> 
> Fixes: 257a087c8b52 ("usb: typec: Add support for Parade PS8830 Type-C Retimer")
> Cc: Abel Vesa <abel.vesa@linaro.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/mux/ps883x.c | 36 ++++++++++++++++++++++++----------
>  1 file changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/typec/mux/ps883x.c b/drivers/usb/typec/mux/ps883x.c
> index f8b47187f4cf..ad59babf7cce 100644
> --- a/drivers/usb/typec/mux/ps883x.c
> +++ b/drivers/usb/typec/mux/ps883x.c
> @@ -58,12 +58,31 @@ struct ps883x_retimer {
>  	unsigned int svid;
>  };
>  
> -static void ps883x_configure(struct ps883x_retimer *retimer, int cfg0,
> -			     int cfg1, int cfg2)
> +static int ps883x_configure(struct ps883x_retimer *retimer, int cfg0,
> +			    int cfg1, int cfg2)
>  {
> -	regmap_write(retimer->regmap, REG_USB_PORT_CONN_STATUS_0, cfg0);
> -	regmap_write(retimer->regmap, REG_USB_PORT_CONN_STATUS_1, cfg1);
> -	regmap_write(retimer->regmap, REG_USB_PORT_CONN_STATUS_2, cfg2);
> +	struct device *dev = &retimer->client->dev;
> +	int ret;
> +
> +	ret = regmap_write(retimer->regmap, REG_USB_PORT_CONN_STATUS_0, cfg0);
> +	if (ret) {
> +		dev_err(dev, "failed to write conn_status_0: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_write(retimer->regmap, REG_USB_PORT_CONN_STATUS_1, cfg1);
> +	if (ret) {
> +		dev_err(dev, "failed to write conn_status_1: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regmap_write(retimer->regmap, REG_USB_PORT_CONN_STATUS_2, cfg2);
> +	if (ret) {
> +		dev_err(dev, "failed to write conn_status_2: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
>  }
>  
>  static int ps883x_set(struct ps883x_retimer *retimer)
> @@ -74,8 +93,7 @@ static int ps883x_set(struct ps883x_retimer *retimer)
>  
>  	if (retimer->orientation == TYPEC_ORIENTATION_NONE ||
>  	    retimer->mode == TYPEC_STATE_SAFE) {
> -		ps883x_configure(retimer, cfg0, cfg1, cfg2);
> -		return 0;
> +		return ps883x_configure(retimer, cfg0, cfg1, cfg2);
>  	}
>  
>  	if (retimer->mode != TYPEC_STATE_USB && retimer->svid != USB_TYPEC_DP_SID)
> @@ -113,9 +131,7 @@ static int ps883x_set(struct ps883x_retimer *retimer)
>  		return -EOPNOTSUPP;
>  	}
>  
> -	ps883x_configure(retimer, cfg0, cfg1, cfg2);
> -
> -	return 0;
> +	return ps883x_configure(retimer, cfg0, cfg1, cfg2);
>  }
>  
>  static int ps883x_sw_set(struct typec_switch_dev *sw,
> -- 
> 2.45.3

-- 
heikki