On 12/3/2024 5:00 PM, 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 :-))
+ #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.
This question might be a dumb one, if so please ignore it.
But if we skip adding unused phys in DTS node, the core driver wouldn't
have access to all phys and we wouldn't be able to get references to
unused ones (un-mentioned ones in DTS). So how can we power them off
from core driver if we don't have reference to them ?
Regards,
Krishna,
