On 24-12-03 12:30:37, Stephan Gerhold wrote:
> On Tue, Dec 03, 2024 at 11:20:48AM +0100, Johan Hovold wrote:
> > [ +CC: Krishna, Thinh and the USB list ]
> >
> > On Mon, Nov 18, 2024 at 11:34:29AM +0100, Stephan Gerhold wrote:
> > > The X1E80100 CRD has a Goodix fingerprint reader connected to the USB
> > > multiport controller on eUSB6. All other ports (including USB super-speed
> > > pins) are unused.
> > >
> > > Set it up in the device tree together with the NXP PTN3222 repeater.
> > >
> > > Signed-off-by: Stephan Gerhold <stephan.gerhold@xxxxxxxxxx>
> > > ---
> > > arch/arm64/boot/dts/qcom/x1e80100-crd.dts | 48 +++++++++++++++++++++++++++++++
> > > 1 file changed, 48 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
> > > index 39f9d9cdc10d..44942931c18f 100644
> > > --- a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
> > > +++ b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
> > > @@ -735,6 +735,26 @@ keyboard@3a {
> > > };
> > > };
> > >
> > > +&i2c5 {
> > > + clock-frequency = <400000>;
> > > +
> > > + status = "okay";
> > > +
> > > + eusb6_repeater: redriver@4f {
> > > + compatible = "nxp,ptn3222";
> > > + reg = <0x4f>;
> >
> > The driver does not currently check that there's actually anything at
> > this address. Did you verify that this is the correct address?
> >
> > (Abel is adding a check to the driver as we speak to catch any such
> > mistakes going forward).
> >
>
> Yes, I verified this using
> https://git.codelinaro.org/stephan.gerhold/linux/-/commit/45d5add498612387f88270ca944ee16e2236fddd
>
> (I sent this to Abel back then, so I'm surprised he didn't run that :-))
I don't remember seeing this commit back then. Maybe I didn't look
careful enough. Sorry.
Since you already did the work, can you send that on the list?
So if you remember, back then I hunted down all of these with i2cget on
my t14s (it has 3 such repeaters, unlike CRD).
>
> > > + #phy-cells = <0>;
> >
> > nit: I'd put provider properties like this one last.
> >
> > > + vdd3v3-supply = <&vreg_l13b_3p0>;
> > > + vdd1v8-supply = <&vreg_l4b_1p8>;
> >
> > Sort by supply name?
> >
>
> Ack.
>
> > > + reset-gpios = <&tlmm 184 GPIO_ACTIVE_LOW>;
> > > +
> > > + pinctrl-0 = <&eusb6_reset_n>;
> > > + pinctrl-names = "default";
> > > + };
> > > +};
> > > +
> > > &i2c8 {
> > > clock-frequency = <400000>;
> > >
> > > @@ -1047,6 +1067,14 @@ edp_reg_en: edp-reg-en-state {
> > > bias-disable;
> > > };
> > >
> > > + eusb6_reset_n: eusb6-reset-n-state {
> > > + pins = "gpio184";
> > > + function = "gpio";
> > > + drive-strength = <2>;
> > > + bias-disable;
> > > + output-low;
> >
> > I don't think the pin config should assert reset, that should be up to
> > the driver to control.
> >
>
> I can drop it I guess, but pinctrl is applied before the driver takes
> control of the GPIO. This means if the GPIO happens to be in input mode
> before the driver loads (with pull up or pull down), then we would
> briefly leave it floating when applying the bias-disable.
>
> Or I guess we could drop the bias-disable, since it shouldn't be
> relevant for a pin we keep in output mode all the time?
>
> > > + };
> > > +
> > > hall_int_n_default: hall-int-n-state {
> > > pins = "gpio92";
> > > function = "gpio";
> > > @@ -1260,3 +1288,23 @@ &usb_1_ss2_dwc3_hs {
> > > &usb_1_ss2_qmpphy_out {
> > > remote-endpoint = <&pmic_glink_ss2_ss_in>;
> > > };
> > > +
> > > +&usb_mp {
> > > + status = "okay";
> > > +};
> > > +
> > > +&usb_mp_dwc3 {
> > > + /* Limit to USB 2.0 and single port */
> > > + maximum-speed = "high-speed";
> > > + phys = <&usb_mp_hsphy1>;
> > > + phy-names = "usb2-1";
> > > +};
> >
> > The dwc3 driver determines (and acts on) the number of ports based on
> > the port interrupts in DT and controller capabilities.
> >
> > I'm not sure we can (should) just drop the other HS PHY and the SS PHYs
> > that would still be there in the SoC (possibly initialised by the boot
> > firmware).
> >
> > I had a local patch to enable the multiport controller (for the suspend
> > work) and I realise that you'd currently need to specify a repeater also
> > for the HS PHY which does not have one, but that should be possible to
> > fix somehow.
> >
>
> I think there are two separate questions here:
>
> 1. Should we (or do we even need to) enable unused PHYs?
> 2. Do we need to power off unused PHYs left enabled by the firmware?
>
> For (1), I'm not not sure if there is a technical reason that requires
> us to. And given that PHYs typically consume quite a bit of power, I'm
> not sure if we should. Perhaps it's not worth spending effort on this
> minor optimization now, but then the device tree would ideally still
> tell us which PHYs are actually used (for future optimizations).
>
> For (2), yes, we probably need to. But my impression so far is that this
> might be a larger problem that we need to handle on the SoC level. I
> have seen some firmware versions that blindly power up all USB
> controllers, even completely unused ones. Ideally we would power down
> unused components during startup and then leave them off.
>
> Thanks,
> Stephan
