diff mbox series

[v20,5/5] arm64: dts: qcom: sc7180-trogdor: Add nodes for onboard USB hub

Message ID 20220119124327.v20.5.Ie0d2c1214b767bb5551dd4cad38398bd40e4466f@changeid
State Superseded
Headers show
Series usb: misc: Add onboard_usb_hub driver | expand

Commit Message

Matthias Kaehlcke Jan. 19, 2022, 8:43 p.m. UTC 
Add nodes for the onboard USB hub on trogdor devices. Remove the
'always-on' property from the hub regulator, since the regulator
is now managed by the onboard_usb_hub driver.

Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v20:
- renamed hub labels to 'usb_hub_2/3_x'
- added comment for 'regulator-boot-on' of 'pp3300_hub'
- added 'Reviewed-by' tags from Stephen and Doug

Changes in v19:
- none

Changes in v18:
- also adjust config for pompom rev1

Changes in v17:
- none

Changes in v16:
- none

Changes in v15:
- none

Changes in v14:
- none

Changes in v13:
- none

Changes in v12:
- none

Changes in v11:
- rebased on qcom/arm64-for-5.14 (with the rest of the series)

Changes in v10:
- keep 'regulator-boot-on' property
- updated commit message

Changes in v9:
- none

Changes in v8:
- none

Changes in v7:
- rebased on qcom/arm64-for-5.13 (with the rest of the series)

Changes in v6:
- added 'companion-hub' entry to both USB devices
- added 'vdd-supply' also to hub@2

Changes in v5:
- patch added to the series

 .../boot/dts/qcom/sc7180-trogdor-lazor-r0.dts | 19 ++++++++----------
 .../boot/dts/qcom/sc7180-trogdor-lazor-r1.dts | 12 +++++------
 .../dts/qcom/sc7180-trogdor-pompom-r1.dts     | 11 ++++------
 .../arm64/boot/dts/qcom/sc7180-trogdor-r1.dts | 19 ++++++++----------
 arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi  | 20 ++++++++++++++++++-
 5 files changed, 44 insertions(+), 37 deletions(-)

Comments

Doug Anderson Feb. 15, 2022, 5:54 p.m. UTC | #1 
Hi,

On Tue, Feb 8, 2022 at 11:21 AM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> On Tue, Feb 08, 2022 at 11:57:20AM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Jan 19, 2022 at 12:43:45PM -0800, Matthias Kaehlcke wrote:
> > > Add nodes for the onboard USB hub on trogdor devices. Remove the
> > > 'always-on' property from the hub regulator, since the regulator
> > > is now managed by the onboard_usb_hub driver.
> > >
> > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> > > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > > ---
> >
> > No DT maintainer approval yet?  :(
>
> Bjorn usually just picks DT changes into the QCOM tree when they are
> ready, so I wouldn't interpret anything into the lack of an explicit
> Ack.

Right, so the expectation is that this patch wouldn't land through the
USB tree but would instead land through the Qualcomm tree, probably a
revision after the code lands in the USB tree to avoid dependency
problems.

-Doug
Greg KH Feb. 15, 2022, 6:55 p.m. UTC | #2 
On Tue, Feb 15, 2022 at 09:54:54AM -0800, Doug Anderson wrote:
> Hi,
> 
> On Tue, Feb 8, 2022 at 11:21 AM Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > On Tue, Feb 08, 2022 at 11:57:20AM +0100, Greg Kroah-Hartman wrote:
> > > On Wed, Jan 19, 2022 at 12:43:45PM -0800, Matthias Kaehlcke wrote:
> > > > Add nodes for the onboard USB hub on trogdor devices. Remove the
> > > > 'always-on' property from the hub regulator, since the regulator
> > > > is now managed by the onboard_usb_hub driver.
> > > >
> > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> > > > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > > > ---
> > >
> > > No DT maintainer approval yet?  :(
> >
> > Bjorn usually just picks DT changes into the QCOM tree when they are
> > ready, so I wouldn't interpret anything into the lack of an explicit
> > Ack.
> 
> Right, so the expectation is that this patch wouldn't land through the
> USB tree but would instead land through the Qualcomm tree, probably a
> revision after the code lands in the USB tree to avoid dependency
> problems.

But our tools pick up the whole series.  I can't just do "i will pick
patches 1-4 only" easily, and neither can any other maintainer.

Why not just get their ack so that I know it can come through the USB
tree?  That's what normally happens for other changes like this where a
driver change is required first.

thanks,

greg k-h
Doug Anderson Feb. 15, 2022, 7:03 p.m. UTC | #3 
Hi,

On Tue, Feb 15, 2022 at 10:55 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Feb 15, 2022 at 09:54:54AM -0800, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Feb 8, 2022 at 11:21 AM Matthias Kaehlcke <mka@chromium.org> wrote:
> > >
> > > On Tue, Feb 08, 2022 at 11:57:20AM +0100, Greg Kroah-Hartman wrote:
> > > > On Wed, Jan 19, 2022 at 12:43:45PM -0800, Matthias Kaehlcke wrote:
> > > > > Add nodes for the onboard USB hub on trogdor devices. Remove the
> > > > > 'always-on' property from the hub regulator, since the regulator
> > > > > is now managed by the onboard_usb_hub driver.
> > > > >
> > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > > > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> > > > > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > > > > ---
> > > >
> > > > No DT maintainer approval yet?  :(
> > >
> > > Bjorn usually just picks DT changes into the QCOM tree when they are
> > > ready, so I wouldn't interpret anything into the lack of an explicit
> > > Ack.
> >
> > Right, so the expectation is that this patch wouldn't land through the
> > USB tree but would instead land through the Qualcomm tree, probably a
> > revision after the code lands in the USB tree to avoid dependency
> > problems.
>
> But our tools pick up the whole series.  I can't just do "i will pick
> patches 1-4 only" easily, and neither can any other maintainer.
>
> Why not just get their ack so that I know it can come through the USB
> tree?  That's what normally happens for other changes like this where a
> driver change is required first.

Huh. That's the first time I've heard that and I'm pretty used to
patches in a series going through different trees, but it would be OK
w/ me if Bjorn was willing to Ack this. Bjorn: what say you? I guess
alternatively Matthias could send two series: one with the code and a
later one with the dts.

-Doug
Krzysztof Kozlowski Feb. 16, 2022, 7:36 a.m. UTC | #4 
On 15/02/2022 19:55, Greg Kroah-Hartman wrote:
> On Tue, Feb 15, 2022 at 09:54:54AM -0800, Doug Anderson wrote:
>> Hi,
>>
>> On Tue, Feb 8, 2022 at 11:21 AM Matthias Kaehlcke <mka@chromium.org> wrote:
>>>
>>> On Tue, Feb 08, 2022 at 11:57:20AM +0100, Greg Kroah-Hartman wrote:
>>>> On Wed, Jan 19, 2022 at 12:43:45PM -0800, Matthias Kaehlcke wrote:
>>>>> Add nodes for the onboard USB hub on trogdor devices. Remove the
>>>>> 'always-on' property from the hub regulator, since the regulator
>>>>> is now managed by the onboard_usb_hub driver.
>>>>>
>>>>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
>>>>> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>>>>> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>>>>> ---
>>>>
>>>> No DT maintainer approval yet?  :(
>>>
>>> Bjorn usually just picks DT changes into the QCOM tree when they are
>>> ready, so I wouldn't interpret anything into the lack of an explicit
>>> Ack.
>>
>> Right, so the expectation is that this patch wouldn't land through the
>> USB tree but would instead land through the Qualcomm tree, probably a
>> revision after the code lands in the USB tree to avoid dependency
>> problems.
> 
> But our tools pick up the whole series.  I can't just do "i will pick
> patches 1-4 only" easily, and neither can any other maintainer.

I don't have problems picking individual patches - either b4 am on each
patch or on entire series and dropping later unneeded commits.

> 
> Why not just get their ack so that I know it can come through the USB
> tree?  That's what normally happens for other changes like this where a
> driver change is required first.

DTS is a description of the hardware and we take it via separate
branches of SoC-fami0ly repositories. These are always separated from
the driver changes. Always. For several reasons:
1. By convention,
2. To be sure there is no dependency on driver code thus an ABI break,
3. To have a nice and clean history of DTS changes, properly organized.

What is more, if this was coming via my Samsung SoC tree towards SoC
folks, I could not take it in one branch. I would need to physically
split it, otherwise Arnd/Olof would bounce back my pull request saying I
am mixing DTS with driver. Of course you do not have such requirement -
I am just saying that splitting DTS is quite common and proper way.


Best regards,
Krzysztof
Bjorn Andersson Feb. 16, 2022, 7:45 p.m. UTC | #5 
On Tue 15 Feb 12:55 CST 2022, Greg Kroah-Hartman wrote:

> On Tue, Feb 15, 2022 at 09:54:54AM -0800, Doug Anderson wrote:
> > Hi,
> > 
> > On Tue, Feb 8, 2022 at 11:21 AM Matthias Kaehlcke <mka@chromium.org> wrote:
> > >
> > > On Tue, Feb 08, 2022 at 11:57:20AM +0100, Greg Kroah-Hartman wrote:
> > > > On Wed, Jan 19, 2022 at 12:43:45PM -0800, Matthias Kaehlcke wrote:
> > > > > Add nodes for the onboard USB hub on trogdor devices. Remove the
> > > > > 'always-on' property from the hub regulator, since the regulator
> > > > > is now managed by the onboard_usb_hub driver.
> > > > >
> > > > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > > > > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> > > > > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > > > > ---
> > > >
> > > > No DT maintainer approval yet?  :(
> > >
> > > Bjorn usually just picks DT changes into the QCOM tree when they are
> > > ready, so I wouldn't interpret anything into the lack of an explicit
> > > Ack.
> > 
> > Right, so the expectation is that this patch wouldn't land through the
> > USB tree but would instead land through the Qualcomm tree, probably a
> > revision after the code lands in the USB tree to avoid dependency
> > problems.
> 
> But our tools pick up the whole series.  I can't just do "i will pick
> patches 1-4 only" easily, and neither can any other maintainer.
> 

Most other maintainers uses -P to selectively pick the patches that
applies to their subsystem. That said, I really do recognize the
inconvenience on your part and the number of patches being sent your
way.

> Why not just get their ack so that I know it can come through the USB
> tree?  That's what normally happens for other changes like this where a
> driver change is required first.
> 

Because while the change looks good I don't think it's fine to take it
through the USB tree - the dts tree typically looks like a shotgun hit
across the dts files. And you and I have already seen several times that
dts changes do conflict when you take some of them in the USB tree.

Unfortunately I see only two ways around this problem, either you start
picking selectively or I manage to convince all contributors that they
must split their series to keep dts changes separate (which isn't a bad
idea in itself).

Regards,
Bjorn
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts
index 30e3e769d2b4..b69590a17cab 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts
@@ -14,17 +14,6 @@  / {
 	compatible = "google,lazor-rev0", "qcom,sc7180";
 };
 
-&pp3300_hub {
-	/* pp3300_l7c is used to power the USB hub */
-	/delete-property/regulator-always-on;
-	/delete-property/regulator-boot-on;
-};
-
-&pp3300_l7c {
-	regulator-always-on;
-	regulator-boot-on;
-};
-
 &sn65dsi86_out {
 	/*
 	 * Lane 0 was incorrectly mapped on the cable, but we've now decided
@@ -33,3 +22,11 @@  &sn65dsi86_out {
 	 */
 	lane-polarities = <1 0>;
 };
+
+&usb_hub_2_x {
+	 vdd-supply = <&pp3300_l7c>;
+};
+
+&usb_hub_3_x {
+	 vdd-supply = <&pp3300_l7c>;
+};
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts
index c2ef06367baf..25f178c3c6eb 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts
@@ -14,13 +14,11 @@  / {
 	compatible = "google,lazor-rev1", "google,lazor-rev2", "qcom,sc7180";
 };
 
-&pp3300_hub {
-	/* pp3300_l7c is used to power the USB hub */
-	/delete-property/regulator-always-on;
-	/delete-property/regulator-boot-on;
+
+&usb_hub_2_x {
+	 vdd-supply = <&pp3300_l7c>;
 };
 
-&pp3300_l7c {
-	regulator-always-on;
-	regulator-boot-on;
+&usb_hub_3_x {
+	 vdd-supply = <&pp3300_l7c>;
 };
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r1.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r1.dts
index 76a130bad60a..8467ff41e6d5 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r1.dts
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r1.dts
@@ -34,13 +34,10 @@  &pm6150_adc_tm {
 	/delete-node/ charger-thermistor@0;
 };
 
-&pp3300_hub {
-	/* pp3300_l7c is used to power the USB hub */
-	/delete-property/regulator-always-on;
-	/delete-property/regulator-boot-on;
+&usb_hub_2_x {
+	 vdd-supply = <&pp3300_l7c>;
 };
 
-&pp3300_l7c {
-	regulator-always-on;
-	regulator-boot-on;
+&usb_hub_3_x {
+	 vdd-supply = <&pp3300_l7c>;
 };
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts
index 457c25499863..0cbb7a68d58b 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts
@@ -43,17 +43,6 @@  &panel {
 	compatible = "auo,b116xa01";
 };
 
-&pp3300_hub {
-	/* pp3300_l7c is used to power the USB hub */
-	/delete-property/regulator-always-on;
-	/delete-property/regulator-boot-on;
-};
-
-&pp3300_l7c {
-	regulator-always-on;
-	regulator-boot-on;
-};
-
 &sdhc_2 {
 	status = "okay";
 };
@@ -62,6 +51,14 @@  &trackpad {
 	interrupts = <58 IRQ_TYPE_EDGE_FALLING>;
 };
 
+&usb_hub_2_x {
+	 vdd-supply = <&pp3300_l7c>;
+};
+
+&usb_hub_3_x {
+	 vdd-supply = <&pp3300_l7c>;
+};
+
 /* PINCTRL - modifications to sc7180-trogdor.dtsi */
 
 &trackpad_int_1v8_odl {
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
index d4f4441179fc..be06938872ca 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
@@ -202,7 +202,7 @@  pp3300_hub: pp3300-hub {
 		pinctrl-names = "default";
 		pinctrl-0 = <&en_pp3300_hub>;
 
-		regulator-always-on;
+		/* The BIOS leaves this regulator on */
 		regulator-boot-on;
 
 		vin-supply = <&pp3300_a>;
@@ -839,6 +839,24 @@  &usb_1 {
 
 &usb_1_dwc3 {
 	dr_mode = "host";
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	/* 2.x hub on port 1 */
+	usb_hub_2_x: hub@1 {
+		compatible = "usbbda,5411";
+		reg = <1>;
+		vdd-supply = <&pp3300_hub>;
+		companion-hub = <&usb_hub_3_x>;
+	};
+
+	/* 3.x hub on port 2 */
+	usb_hub_3_x: hub@2 {
+		compatible = "usbbda,411";
+		reg = <2>;
+		vdd-supply = <&pp3300_hub>;
+		companion-hub = <&usb_hub_2_x>;
+	};
 };
 
 &usb_1_hsphy {