Re: [PATCH v2 1/2] arm64: dts: qcom: x1e78100-t14s: Enable support for both Type-A USB ports

[Abel Vesa <abel.vesa@linaro.org>]

On 24-12-02 16:18:37, Johan Hovold wrote:
> On Mon, Dec 02, 2024 at 11:23:17AM +0200, Abel Vesa wrote:
> > The Thinkpad T14s has 2 USB-A ports, both connected to the USB
> > multiport controller, each one via a separate NXP PTN3222 eUSB2-to-USB2
> > redriver to the eUSB2 PHY for High-Speed support, with a dedicated QMP
> > PHY for SuperSpeed support.
> > 
> > Describe each redriver and then enable each pair of PHYs and the
> > USB controller itself, in order to enable support for the 2 USB-A ports.
> > 
> > Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> > ---
> >  .../dts/qcom/x1e78100-lenovo-thinkpad-t14s.dts     | 86 ++++++++++++++++++++++
> >  1 file changed, 86 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dts b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dts
> > index 975550139e1024420ed335a2a46e4d54df7ee423..f936e3246ec87972746a60080c3a48d646a356f2 100644
> > --- a/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dts
> > +++ b/arch/arm64/boot/dts/qcom/x1e78100-lenovo-thinkpad-t14s.dts
> > @@ -495,6 +495,40 @@ keyboard@3a {
> >  	};
> >  };
> >  
> > +&i2c5 {
> > +	clock-frequency = <400000>;
> > +
> > +	status = "okay";
> > +
> > +	eusb3_repeater: redriver@47 {
> > +		compatible = "nxp,ptn3222";
> > +		reg = <0x47>;
> 
> The driver doesn't seem to actually communicate with these devices
> currently and the addresses you specify here do not match what the
> schematics says.

Schematics have the addressess left shifted for the wr/rd bit.

> 
> Have you verified that these addresses are correct?

Reading the chip id regs confirms the addresses are correct.

> 
> > +		#phy-cells = <0>;
> > +
> > +		vdd3v3-supply = <&vreg_l13b_3p0>;
> > +		vdd1v8-supply = <&vreg_l4b_1p8>;
> > +
> > +		reset-gpios = <&tlmm 6 GPIO_ACTIVE_LOW>;
> > +
> > +		pinctrl-0 = <&eusb3_reset_n>;
> > +		pinctrl-names = "default";
> > +	};
> > +
> > +	eusb6_repeater: redriver@4f {
> > +		compatible = "nxp,ptn3222";
> > +		reg = <0x4f>;
> 
> Same here.
> 
> > +		#phy-cells = <0>;
> > +
> > +		vdd3v3-supply = <&vreg_l13b_3p0>;
> > +		vdd1v8-supply = <&vreg_l4b_1p8>;
> > +
> > +		reset-gpios = <&tlmm 184 GPIO_ACTIVE_LOW>;
> > +
> > +		pinctrl-0 = <&eusb6_reset_n>;
> > +		pinctrl-names = "default";
> > +	};
> > +};
> > +
> >  &i2c8 {
> >  	clock-frequency = <400000>;
> >  
> > @@ -651,6 +685,22 @@ &tlmm {
> >  			       <72 2>, /* Secure EC I2C connection (?) */
> >  			       <238 1>; /* UFS Reset */
> >  
> > +	eusb3_reset_n: eusb3-reset-n-state {
> > +		pins = "gpio6";
> > +		function = "gpio";
> > +		drive-strength = <2>;
> > +		bias-disable;
> > +		output-low;
> 
> I don't think the pin configuration should assert reset, that should be
> left up to the driver to decide, that is,  when (and if) it's an
> appropriate thing to do.

Yep. The driver needs changes for that.

> 
> > +	};
> > +
> > +	eusb6_reset_n: eusb6-reset-n-state {
> > +		pins = "gpio184";
> > +		function = "gpio";
> > +		drive-strength = <2>;
> > +		bias-disable;
> > +		output-low;
> 
> Same here.
> 
> > +	};
> > +
> >  	tpad_default: tpad-default-state {
> >  		pins = "gpio3";
> >  		function = "gpio";
> > @@ -808,3 +858,39 @@ &usb_1_ss1_dwc3_hs {
> >  &usb_1_ss1_qmpphy_out {
> >  	remote-endpoint = <&pmic_glink_ss1_ss_in>;
> >  };
> 
> And last, but not least, the T14s may hard reset if you disconnect a
> thumb drive connected to one of these ports while suspended (6.13-rc1).

Wasn't able to reproduce this issue yet. Will spend some more time on it
in the following days.

> 
> Once it survived with a lockdep splat indicating a circular locking
> dependency. I see that on the CRD as well, so possibly not related to
> the hard reset.

This is most definitely the same splat triggered by the repeater PHY ops
being called from the eUSB2 PHY driver. We are already in discussion
with Vinod on how to handle multi PHY levels in the generic framework.

> 
> No such issues with a FullSpeed keyboard.
> 
> Johan

Thanks for reviewing,

Abel