[PATCH v2 0/2] usb: typec: Add new driver for Parade PS8830 Type-C Retimer

Abel Vesa posted 2 patches 1 month, 3 weeks ago
There is a newer version of this series
.../devicetree/bindings/usb/parade,ps8830.yaml     | 129 +++++++
drivers/usb/typec/mux/Kconfig                      |  10 +
drivers/usb/typec/mux/Makefile                     |   1 +
drivers/usb/typec/mux/ps8830.c                     | 424 +++++++++++++++++++++
4 files changed, 564 insertions(+)
[PATCH v2 0/2] usb: typec: Add new driver for Parade PS8830 Type-C Retimer
Posted by Abel Vesa 1 month, 3 weeks ago
The Parade PS8830 is a Type-C multi-protocol retimer that is controlled
via I2C. It provides altmode and orientation handling and usually sits
between the Type-C port and the PHY.

It is currently used alongside Qualcomm Snapdragon X Elite SoCs on quite
a few laptops already.

This new driver adds support for the following 3 modes:
 - DP 4lanes (pin assignments C and E)
 - DP 2lanes + USB3 (pin assignment D)
 - USB3

This retimer is a LTTPR (Link-Training Tunable PHY Repeater) which means
it can support link training from source to itself. This means that the
DP driver needs to be aware of the repeater presence and to handle
the link training accordingly. This is currently missing from msm dp
driver, but there is already effort going on to add it. Once done,
full external DP will be working on all X1E laptops that make use of
this retimer.

Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
Changes in v2:
- Addressed all comments from Johan and Konrad.
- Reworked the handling of the vregs so it would be more cleaner.
  Dropped the usage of bulk regulators API and handled them separately.
  Also discribed all regulators according to data sheet.
- Added all delays according to data sheet.
- Fixed coldplug (on boot) orientation detection.
- Didn't pick Krzysztof's R-b tag because the bindings changed w.r.t
  supplies.
- Link to v1: https://lore.kernel.org/r/20240829-x1e80100-ps8830-v1-0-bcc4790b1d45@linaro.org

---
Abel Vesa (2):
      dt-bindings: usb: Add Parade PS8830 Type-C retimer bindings
      usb: typec: Add support for Parade PS8830 Type-C Retimer

 .../devicetree/bindings/usb/parade,ps8830.yaml     | 129 +++++++
 drivers/usb/typec/mux/Kconfig                      |  10 +
 drivers/usb/typec/mux/Makefile                     |   1 +
 drivers/usb/typec/mux/ps8830.c                     | 424 +++++++++++++++++++++
 4 files changed, 564 insertions(+)
---
base-commit: c02d24a5af66a9806922391493205a344749f2c4
change-id: 20240521-x1e80100-ps8830-d5ccca95b557

Best regards,
-- 
Abel Vesa <abel.vesa@linaro.org>
Re: [PATCH v2 0/2] usb: typec: Add new driver for Parade PS8830 Type-C Retimer
Posted by Johan Hovold 1 month, 1 week ago
On Fri, Oct 04, 2024 at 04:57:36PM +0300, Abel Vesa wrote:
> The Parade PS8830 is a Type-C multi-protocol retimer that is controlled
> via I2C. It provides altmode and orientation handling and usually sits
> between the Type-C port and the PHY.
> 
> It is currently used alongside Qualcomm Snapdragon X Elite SoCs on quite
> a few laptops already.
> 
> This new driver adds support for the following 3 modes:
>  - DP 4lanes (pin assignments C and E)
>  - DP 2lanes + USB3 (pin assignment D)
>  - USB3
> 
> This retimer is a LTTPR (Link-Training Tunable PHY Repeater) which means
> it can support link training from source to itself. This means that the
> DP driver needs to be aware of the repeater presence and to handle
> the link training accordingly. This is currently missing from msm dp
> driver, but there is already effort going on to add it. Once done,
> full external DP will be working on all X1E laptops that make use of
> this retimer.

I was gonna ask you to include the devicetree changes that enables the
retimers as part of this series (to facilitate review and testing), but
perhaps you should indeed not post them again until LTTPR support is in
place.

> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> ---
> Changes in v2:
> - Addressed all comments from Johan and Konrad.
> - Reworked the handling of the vregs so it would be more cleaner.
>   Dropped the usage of bulk regulators API and handled them separately.
>   Also discribed all regulators according to data sheet.
> - Added all delays according to data sheet.
> - Fixed coldplug (on boot) orientation detection.

Coldplug orientation detection still does not work here with this series
applied.

I'm not entirely sure this whether worked better with v1, but with v2
my SuperSpeed ethernet device shows up as a HighSpeed device in one
orientation. It is also not disconnected an re-enumerated as SS as is
the case on the X13s (and possibly with v1):

	usb 1-1: new high-speed USB device number 2 using xhci-hcd

> - Didn't pick Krzysztof's R-b tag because the bindings changed w.r.t
>   supplies.
> - Link to v1: https://lore.kernel.org/r/20240829-x1e80100-ps8830-v1-0-bcc4790b1d45@linaro.org
> 
> ---
> Abel Vesa (2):
>       dt-bindings: usb: Add Parade PS8830 Type-C retimer bindings
>       usb: typec: Add support for Parade PS8830 Type-C Retimer

Johan
Re: [PATCH v2 0/2] usb: typec: Add new driver for Parade PS8830 Type-C Retimer
Posted by Abel Vesa 1 month, 1 week ago
On 24-10-15 14:41:25, Johan Hovold wrote:
> On Fri, Oct 04, 2024 at 04:57:36PM +0300, Abel Vesa wrote:
> > The Parade PS8830 is a Type-C multi-protocol retimer that is controlled
> > via I2C. It provides altmode and orientation handling and usually sits
> > between the Type-C port and the PHY.
> > 
> > It is currently used alongside Qualcomm Snapdragon X Elite SoCs on quite
> > a few laptops already.
> > 
> > This new driver adds support for the following 3 modes:
> >  - DP 4lanes (pin assignments C and E)
> >  - DP 2lanes + USB3 (pin assignment D)
> >  - USB3
> > 
> > This retimer is a LTTPR (Link-Training Tunable PHY Repeater) which means
> > it can support link training from source to itself. This means that the
> > DP driver needs to be aware of the repeater presence and to handle
> > the link training accordingly. This is currently missing from msm dp
> > driver, but there is already effort going on to add it. Once done,
> > full external DP will be working on all X1E laptops that make use of
> > this retimer.
> 
> I was gonna ask you to include the devicetree changes that enables the
> retimers as part of this series (to facilitate review and testing), but
> perhaps you should indeed not post them again until LTTPR support is in
> place.

I was thinking maybe we should not wait for LTTPR support as this series
brings orientation support as is. I still need to figure out how to
strip out the DP parts of it in such a way that orientation should still
be working but DP should not (until LTTPR is in).

> 
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > ---
> > Changes in v2:
> > - Addressed all comments from Johan and Konrad.
> > - Reworked the handling of the vregs so it would be more cleaner.
> >   Dropped the usage of bulk regulators API and handled them separately.
> >   Also discribed all regulators according to data sheet.
> > - Added all delays according to data sheet.
> > - Fixed coldplug (on boot) orientation detection.
> 
> Coldplug orientation detection still does not work here with this series
> applied.
> 
> I'm not entirely sure this whether worked better with v1, but with v2
> my SuperSpeed ethernet device shows up as a HighSpeed device in one
> orientation. It is also not disconnected an re-enumerated as SS as is
> the case on the X13s (and possibly with v1):
> 
> 	usb 1-1: new high-speed USB device number 2 using xhci-hcd

For coldplug, this series does the right thing as it leaves the retimer
initialized if it was left enabled at boot. There is a second part
needed for the coldplug to work. That is the regulator-boot-on property
in retimer's vregs nodes. That will ensure that the regulator is not
disabled until retimer driver probes and will keep the retimer initialized
until USB device is enumerated.

> 
> > - Didn't pick Krzysztof's R-b tag because the bindings changed w.r.t
> >   supplies.
> > - Link to v1: https://lore.kernel.org/r/20240829-x1e80100-ps8830-v1-0-bcc4790b1d45@linaro.org
> > 
> > ---
> > Abel Vesa (2):
> >       dt-bindings: usb: Add Parade PS8830 Type-C retimer bindings
> >       usb: typec: Add support for Parade PS8830 Type-C Retimer
> 
> Johan
Re: [PATCH v2 0/2] usb: typec: Add new driver for Parade PS8830 Type-C Retimer
Posted by Johan Hovold 1 month, 1 week ago
On Tue, Oct 15, 2024 at 04:03:53PM +0300, Abel Vesa wrote:
> On 24-10-15 14:41:25, Johan Hovold wrote:
> > On Fri, Oct 04, 2024 at 04:57:36PM +0300, Abel Vesa wrote:
> > > The Parade PS8830 is a Type-C multi-protocol retimer that is controlled
> > > via I2C. It provides altmode and orientation handling and usually sits
> > > between the Type-C port and the PHY.

> > > This retimer is a LTTPR (Link-Training Tunable PHY Repeater) which means
> > > it can support link training from source to itself. This means that the
> > > DP driver needs to be aware of the repeater presence and to handle
> > > the link training accordingly. This is currently missing from msm dp
> > > driver, but there is already effort going on to add it. Once done,
> > > full external DP will be working on all X1E laptops that make use of
> > > this retimer.
> > 
> > I was gonna ask you to include the devicetree changes that enables the
> > retimers as part of this series (to facilitate review and testing), but
> > perhaps you should indeed not post them again until LTTPR support is in
> > place.
> 
> I was thinking maybe we should not wait for LTTPR support as this series
> brings orientation support as is. I still need to figure out how to
> strip out the DP parts of it in such a way that orientation should still
> be working but DP should not (until LTTPR is in).

Yeah, possible, or you can at least include the DT patches here but mark
them as do-not-merge-yet or similar.

> > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > > ---
> > > Changes in v2:

> > > - Fixed coldplug (on boot) orientation detection.
> > 
> > Coldplug orientation detection still does not work here with this series
> > applied.
> > 
> > I'm not entirely sure this whether worked better with v1, but with v2
> > my SuperSpeed ethernet device shows up as a HighSpeed device in one
> > orientation. It is also not disconnected an re-enumerated as SS as is
> > the case on the X13s (and possibly with v1):
> > 
> > 	usb 1-1: new high-speed USB device number 2 using xhci-hcd
> 
> For coldplug, this series does the right thing as it leaves the retimer
> initialized if it was left enabled at boot. There is a second part
> needed for the coldplug to work. That is the regulator-boot-on property
> in retimer's vregs nodes. That will ensure that the regulator is not
> disabled until retimer driver probes and will keep the retimer initialized
> until USB device is enumerated.

I can confirm that marking the regulators as having been left on by the
bootloader so that they are not disabled temporarily during boot indeed
fixes the coldplug issue here.

That however makes me wonder whether something is missing in the driver
so that it still relies on setup having been done by the boot firmware.

Have you tried actually asserting reset during probe to verify that
driver can configure the retimers itself without relying on the boot
firmware?

Johan
Re: [PATCH v2 0/2] usb: typec: Add new driver for Parade PS8830 Type-C Retimer
Posted by Abel Vesa 1 month, 1 week ago
On 24-10-17 08:00:44, Johan Hovold wrote:
> On Tue, Oct 15, 2024 at 04:03:53PM +0300, Abel Vesa wrote:
> > On 24-10-15 14:41:25, Johan Hovold wrote:
> > > On Fri, Oct 04, 2024 at 04:57:36PM +0300, Abel Vesa wrote:
> > > > The Parade PS8830 is a Type-C multi-protocol retimer that is controlled
> > > > via I2C. It provides altmode and orientation handling and usually sits
> > > > between the Type-C port and the PHY.
> 
> > > > This retimer is a LTTPR (Link-Training Tunable PHY Repeater) which means
> > > > it can support link training from source to itself. This means that the
> > > > DP driver needs to be aware of the repeater presence and to handle
> > > > the link training accordingly. This is currently missing from msm dp
> > > > driver, but there is already effort going on to add it. Once done,
> > > > full external DP will be working on all X1E laptops that make use of
> > > > this retimer.
> > > 
> > > I was gonna ask you to include the devicetree changes that enables the
> > > retimers as part of this series (to facilitate review and testing), but
> > > perhaps you should indeed not post them again until LTTPR support is in
> > > place.
> > 
> > I was thinking maybe we should not wait for LTTPR support as this series
> > brings orientation support as is. I still need to figure out how to
> > strip out the DP parts of it in such a way that orientation should still
> > be working but DP should not (until LTTPR is in).
> 
> Yeah, possible, or you can at least include the DT patches here but mark
> them as do-not-merge-yet or similar.

Sure, will do that. Will have to split the DP part of it into separate
patches and mark only those as do-not-merge-yet. 

> 
> > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > > > ---
> > > > Changes in v2:
> 
> > > > - Fixed coldplug (on boot) orientation detection.
> > > 
> > > Coldplug orientation detection still does not work here with this series
> > > applied.
> > > 
> > > I'm not entirely sure this whether worked better with v1, but with v2
> > > my SuperSpeed ethernet device shows up as a HighSpeed device in one
> > > orientation. It is also not disconnected an re-enumerated as SS as is
> > > the case on the X13s (and possibly with v1):
> > > 
> > > 	usb 1-1: new high-speed USB device number 2 using xhci-hcd
> > 
> > For coldplug, this series does the right thing as it leaves the retimer
> > initialized if it was left enabled at boot. There is a second part
> > needed for the coldplug to work. That is the regulator-boot-on property
> > in retimer's vregs nodes. That will ensure that the regulator is not
> > disabled until retimer driver probes and will keep the retimer initialized
> > until USB device is enumerated.
> 
> I can confirm that marking the regulators as having been left on by the
> bootloader so that they are not disabled temporarily during boot indeed
> fixes the coldplug issue here.
> 
> That however makes me wonder whether something is missing in the driver
> so that it still relies on setup having been done by the boot firmware.
> 
> Have you tried actually asserting reset during probe to verify that
> driver can configure the retimers itself without relying on the boot
> firmware?

We do not want to reset the retimers on probe because we won't be able
to figure out the orientation config until next pmic glink notify comes.
The pmic glink notify only triggers on USB event, which never comes
until you replug the device. So in order to have coldplug orientation
configured correctly in the retimer, we need to make sure the retimer
holds state until unplug.

> 
> Johan
Re: [PATCH v2 0/2] usb: typec: Add new driver for Parade PS8830 Type-C Retimer
Posted by Johan Hovold 1 month ago
On Thu, Oct 17, 2024 at 11:25:20AM +0300, Abel Vesa wrote:
> On 24-10-17 08:00:44, Johan Hovold wrote:

> > I can confirm that marking the regulators as having been left on by the
> > bootloader so that they are not disabled temporarily during boot indeed
> > fixes the coldplug issue here.
> > 
> > That however makes me wonder whether something is missing in the driver
> > so that it still relies on setup having been done by the boot firmware.
> > 
> > Have you tried actually asserting reset during probe to verify that
> > driver can configure the retimers itself without relying on the boot
> > firmware?
> 
> We do not want to reset the retimers on probe because we won't be able
> to figure out the orientation config until next pmic glink notify comes.
> The pmic glink notify only triggers on USB event, which never comes
> until you replug the device. So in order to have coldplug orientation
> configured correctly in the retimer, we need to make sure the retimer
> holds state until unplug.

Ok, thanks for clarifying. As we've discussed off-list it should be
possible to retrieve the orientation from the orientation gpios, but I'm
sure there are bits missing in the kernel for propagating that
information to the retimers currently.

If I understood you correctly you did reset and reinitialise the
retimers in v1, which is useful during development at least to make sure
that the driver is complete.

Johan
Re: [PATCH v2 0/2] usb: typec: Add new driver for Parade PS8830 Type-C Retimer
Posted by Konrad Dybcio 1 month, 1 week ago

On 10/15/24 15:03, Abel Vesa wrote:
> On 24-10-15 14:41:25, Johan Hovold wrote:
>> On Fri, Oct 04, 2024 at 04:57:36PM +0300, Abel Vesa wrote:
>>> The Parade PS8830 is a Type-C multi-protocol retimer that is controlled
>>> via I2C. It provides altmode and orientation handling and usually sits
>>> between the Type-C port and the PHY.
>>>
>>> It is currently used alongside Qualcomm Snapdragon X Elite SoCs on quite
>>> a few laptops already.
>>>
>>> This new driver adds support for the following 3 modes:
>>>   - DP 4lanes (pin assignments C and E)
>>>   - DP 2lanes + USB3 (pin assignment D)
>>>   - USB3
>>>
>>> This retimer is a LTTPR (Link-Training Tunable PHY Repeater) which means
>>> it can support link training from source to itself. This means that the
>>> DP driver needs to be aware of the repeater presence and to handle
>>> the link training accordingly. This is currently missing from msm dp
>>> driver, but there is already effort going on to add it. Once done,
>>> full external DP will be working on all X1E laptops that make use of
>>> this retimer.
>>
>> I was gonna ask you to include the devicetree changes that enables the
>> retimers as part of this series (to facilitate review and testing), but
>> perhaps you should indeed not post them again until LTTPR support is in
>> place.
> 
> I was thinking maybe we should not wait for LTTPR support as this series
> brings orientation support as is.

It also happens to bring an undesired crash-on-unplug feature when
DP is enabled.. I suppose it's fine to bring this series in if you
separate enabling the retimer on devices from wiring DP up.

Konrad