usb: typec: ps883x: follow-up fixes
Subsystems (3)
cover usb: typec: ps883x: follow-up fixes
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/[email protected]/
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(-)
Signature · 1 line
2.45.3
PATCH 1/3 usb: typec: ps883x: fix registration race
Make sure that the retimer is fully setup before registering it to avoid having consumers try to access it while it is being reset.
--- drivers/usb/typec/mux/ps883x.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-)
drivers/usb/typec/mux/ps883x.c +16 -15
@@ -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:
Signature · 1 line
2.45.3
Re: [PATCH 1/3] usb: typec: ps883x: fix registration race
Quoted 6 lines · 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 <[email protected]>Signed-off-by Johan Hovold <[email protected]>
Quoted 55 lines
--- drivers/usb/typec/mux/ps883x.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-)drivers/usb/typec/mux/ps883x.c +16
-15@@ -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:Signature · 1 line
2.45.3
Signature · 1 line
heikki
PATCH 2/3 usb: typec: ps883x: fix missing accessibility check
In reply to Johan HovoldMake 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
--- drivers/usb/typec/mux/ps883x.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
drivers/usb/typec/mux/ps883x.c +11 -0
@@ -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;
Signature · 1 line
2.45.3
Re: [PATCH 2/3] usb: typec: ps883x: fix missing accessibility check
Quoted 9 lines · 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 <[email protected]>Signed-off-by Johan Hovold <[email protected]>
Quoted 35 lines
--- drivers/usb/typec/mux/ps883x.c | 11 +++++++++++ 1 file changed, 11 insertions(+)drivers/usb/typec/mux/ps883x.c +11
-0@@ -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;Signature · 1 line
2.45.3
Signature · 1 line
heikki
Re: [PATCH 2/3] usb: typec: ps883x: fix missing accessibility check
In reply to Johan HovoldHi Johan,
Quoted 9 lines · 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 <[email protected]>Signed-off-by Johan Hovold <[email protected]>
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
Hi Jens,
Quoted 7 lines · Jens Glathe
Quoted 6 lines · 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
Quoted 7 lines
Quoted 3 lines
Fixes 257a087c8b52 ("usb: typec: Add support for Parade PS8830 Type-C Retimer")Cc Abel Vesa <[email protected]>Signed-off-by Johan Hovold <[email protected]>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.
Quoted 2 lines
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
In reply to Johan HovoldPropagate 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
--- drivers/usb/typec/mux/ps883x.c | 36 ++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-)
drivers/usb/typec/mux/ps883x.c +26 -10
@@ -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,
Signature · 1 line
2.45.3
Re: [PATCH 3/3] usb: typec: ps883x: fix configuration error handling
Quoted 9 lines · 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: -5Fixes 257a087c8b52 ("usb: typec: Add support for Parade PS8830 Type-C Retimer")Cc Abel Vesa <[email protected]>Signed-off-by Johan Hovold <[email protected]>
Quoted 68 lines
--- drivers/usb/typec/mux/ps883x.c | 36 ++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-)drivers/usb/typec/mux/ps883x.c +26
-10@@ -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,Signature · 1 line
2.45.3
Signature · 1 line
heikki