| Message ID | 20241202-x1e80100-qcp-t14-enable-usb-type-a-ports-v2-1-7360ed65c769@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: qcom: x1e80100: qcp/t14s: Enable USB multi-port controller related ports | expand |
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. Have you verified that these 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. > + }; > + > + 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). 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. No such issues with a FullSpeed keyboard. Johan
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
On Wed, Dec 04, 2024 at 10:56:59AM +0200, Abel Vesa wrote: > On 24-12-02 16:18:37, Johan Hovold wrote: > > On Mon, Dec 02, 2024 at 11:23:17AM +0200, Abel Vesa wrote: > > > +&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. Thanks for confirming. > > 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. Just triggered what appears to be a deadlock in the block layer by disconnecting a thumb drive while suspended. Not sure if that could have triggered the reset, but it is likely related to the lockdep splat I mentioned below. > > 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, if it was the false-positive PHY splat I would have said so. This appears to be a new block-layer splat with 6.13-rc1. Don't see it with 6.12. I'll send a report. Johan
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>; + #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>; + #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; + }; + + eusb6_reset_n: eusb6-reset-n-state { + pins = "gpio184"; + function = "gpio"; + drive-strength = <2>; + bias-disable; + output-low; + }; + 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>; }; + +&usb_mp { + status = "okay"; +}; + +&usb_mp_hsphy0 { + vdd-supply = <&vreg_l2e_0p8>; + vdda12-supply = <&vreg_l3e_1p2>; + + phys = <&eusb6_repeater>; + + status = "okay"; +}; + +&usb_mp_hsphy1 { + vdd-supply = <&vreg_l2e_0p8>; + vdda12-supply = <&vreg_l3e_1p2>; + + phys = <&eusb3_repeater>; + + status = "okay"; +}; + +&usb_mp_qmpphy0 { + vdda-phy-supply = <&vreg_l3e_1p2>; + vdda-pll-supply = <&vreg_l3c_0p8>; + + status = "okay"; +}; + +&usb_mp_qmpphy1 { + vdda-phy-supply = <&vreg_l3e_1p2>; + vdda-pll-supply = <&vreg_l3c_0p8>; + + status = "okay"; +};