| Message ID | 20201105163724.v2.2.I0ed4abdd2b2916fbedf76be254bc3457fb8b9655@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] arm64: dts: qcom: sc7180: Add sc7180-lazor-r2 | expand |
Hi, On Thu, Nov 5, 2020 at 4:37 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts > index 0a281c24841c..6603f2102233 100644 > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts > @@ -58,10 +58,23 @@ ap_ts: touchscreen@10 { > }; > }; > > +&pp3300_hub { > + /* pp3300_l7c is used to power the USB hub */ > + /delete-property/regulator-always-on; > +}; > + > +&pp3300_l7c { > + regulator-always-on; Personally I always end up pairing "always-on" and "boot-on", but that might just be superstition from many kernel versions ago when there were weird quirks. The way you have it now you will sometimes have "boot-on" but not "always-on". Probably what you have is fine, though. > +}; > + > &sdhc_2 { > status = "okay"; > }; > > +&usb_hub { > + vdd-supply = <&pp3300_l7c>; > +}; > + > /* PINCTRL - board-specific pinctrl */ > > &tlmm { > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi > index bf875589d364..50e733412a7f 100644 > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi > @@ -174,6 +174,25 @@ pp3300_fp_tp: pp3300-fp-tp-regulator { > vin-supply = <&pp3300_a>; > }; > > + pp3300_hub: pp3300-hub { > + compatible = "regulator-fixed"; > + regulator-name = "pp3300_hub"; > + > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + > + gpio = <&tlmm 84 GPIO_ACTIVE_HIGH>; > + enable-active-high; > + pinctrl-names = "default"; > + pinctrl-0 = <&en_pp3300_hub>; > + > + /* AP turns on with en_pp3300_hub; always on for AP */ Delete the above comment. It's obvious based on the properties in this node. Other similar comments are useful because they describe how the _EC_ turns on regulators and why a regulator that has an enable still looks like an "always-on" regulator to the AP (because it's always on whenever the AP is on). If you want to add a comment, you could say: /* Always on until we have a way to specify it can go off in suspend */ > @@ -469,7 +488,6 @@ ppvar_l6c: ldo6 { > regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; > }; > > - pp3300_hub: > pp3300_l7c: ldo7 { > regulator-min-microvolt = <3304000>; > regulator-max-microvolt = <3304000>; Shouldn't you delete the "regulator-always-on;" from ldo7 since you're adding it for all the older revs?
On Thu, Nov 05, 2020 at 05:05:38PM -0800, Doug Anderson wrote: > Hi, > > On Thu, Nov 5, 2020 at 4:37 PM Matthias Kaehlcke <mka@chromium.org> wrote: > > > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts > > index 0a281c24841c..6603f2102233 100644 > > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts > > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts > > @@ -58,10 +58,23 @@ ap_ts: touchscreen@10 { > > }; > > }; > > > > +&pp3300_hub { > > + /* pp3300_l7c is used to power the USB hub */ > > + /delete-property/regulator-always-on; > > +}; > > + > > +&pp3300_l7c { > > + regulator-always-on; > > Personally I always end up pairing "always-on" and "boot-on", but that > might just be superstition from many kernel versions ago when there > were weird quirks. The way you have it now you will sometimes have > "boot-on" but not "always-on". Probably what you have is fine, > though. You are right, it makes a certain sense to have them paired, I'll change it even though it leads to a few more entries. > > +}; > > + > > &sdhc_2 { > > status = "okay"; > > }; > > > > +&usb_hub { > > + vdd-supply = <&pp3300_l7c>; > > +}; > > + > > /* PINCTRL - board-specific pinctrl */ > > > > &tlmm { > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi > > index bf875589d364..50e733412a7f 100644 > > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi > > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi > > @@ -174,6 +174,25 @@ pp3300_fp_tp: pp3300-fp-tp-regulator { > > vin-supply = <&pp3300_a>; > > }; > > > > + pp3300_hub: pp3300-hub { > > + compatible = "regulator-fixed"; > > + regulator-name = "pp3300_hub"; > > + > > + regulator-min-microvolt = <3300000>; > > + regulator-max-microvolt = <3300000>; > > + > > + gpio = <&tlmm 84 GPIO_ACTIVE_HIGH>; > > + enable-active-high; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&en_pp3300_hub>; > > + > > + /* AP turns on with en_pp3300_hub; always on for AP */ > > Delete the above comment. It's obvious based on the properties in > this node. Other similar comments are useful because they describe > how the _EC_ turns on regulators and why a regulator that has an > enable still looks like an "always-on" regulator to the AP (because > it's always on whenever the AP is on). > > If you want to add a comment, you could say: > > /* Always on until we have a way to specify it can go off in suspend */ ok > > @@ -469,7 +488,6 @@ ppvar_l6c: ldo6 { > > regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; > > }; > > > > - pp3300_hub: > > pp3300_l7c: ldo7 { > > regulator-min-microvolt = <3304000>; > > regulator-max-microvolt = <3304000>; > > Shouldn't you delete the "regulator-always-on;" from ldo7 since you're > adding it for all the older revs? Indeed, that was the intention, it didn't blow up into my face during testing since the downstream tree doesn't have it anymore.
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 ae4c23a4fe65..1d809269e7ef 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts @@ -14,6 +14,15 @@ / { compatible = "google,lazor-rev0", "qcom,sc7180"; }; +&pp3300_hub { + /* pp3300_l7c is used to power the USB hub */ + /delete-property/regulator-always-on; +}; + +&pp3300_l7c { + regulator-always-on; +}; + &sn65dsi86_out { /* * Lane 0 was incorrectly mapped on the cable, but we've now decided @@ -22,3 +31,7 @@ &sn65dsi86_out { */ lane-polarities = <1 0>; }; + +&usb_hub { + 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 f6ee1beba458..1d573523d6b6 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts @@ -13,3 +13,16 @@ / { model = "Google Lazor (rev1)"; compatible = "google,lazor-rev1", "qcom,sc7180"; }; + +&pp3300_hub { + /* pp3300_l7c is used to power the USB hub */ + /delete-property/regulator-always-on; +}; + +&pp3300_l7c { + regulator-always-on; +}; + +&usb_hub { + 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 0a281c24841c..6603f2102233 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts @@ -58,10 +58,23 @@ ap_ts: touchscreen@10 { }; }; +&pp3300_hub { + /* pp3300_l7c is used to power the USB hub */ + /delete-property/regulator-always-on; +}; + +&pp3300_l7c { + regulator-always-on; +}; + &sdhc_2 { status = "okay"; }; +&usb_hub { + vdd-supply = <&pp3300_l7c>; +}; + /* PINCTRL - board-specific pinctrl */ &tlmm { diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi index bf875589d364..50e733412a7f 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi @@ -174,6 +174,25 @@ pp3300_fp_tp: pp3300-fp-tp-regulator { vin-supply = <&pp3300_a>; }; + pp3300_hub: pp3300-hub { + compatible = "regulator-fixed"; + regulator-name = "pp3300_hub"; + + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + + gpio = <&tlmm 84 GPIO_ACTIVE_HIGH>; + enable-active-high; + pinctrl-names = "default"; + pinctrl-0 = <&en_pp3300_hub>; + + /* AP turns on with en_pp3300_hub; always on for AP */ + regulator-always-on; + regulator-boot-on; + + vin-supply = <&pp3300_a>; + }; + /* BOARD-SPECIFIC TOP LEVEL NODES */ backlight: backlight { @@ -469,7 +488,6 @@ ppvar_l6c: ldo6 { regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>; }; - pp3300_hub: pp3300_l7c: ldo7 { regulator-min-microvolt = <3304000>; regulator-max-microvolt = <3304000>; @@ -1164,6 +1182,19 @@ pinconf { }; }; + en_pp3300_hub: en-pp3300-hub { + pinmux { + pins = "gpio84"; + function = "gpio"; + }; + + pinconf { + pins = "gpio84"; + drive-strength = <2>; + bias-disable; + }; + }; + fpmcu_boot0: fpmcu-boot0 { pinmux { pins = "gpio10";
The trogdor design has two options for supplying the 'pp3300_hub' power rail, it can be supplied by 'pp3300_l7c' or 'pp3300_a'. The 'pp3300_a' path includes a load switch that can be controlled through GPIO84. Initially trogdor boards used 'pp3300_l7c' to power the USB hub, newer revisions (will) use 'pp3300_a' as supply for 'pp3300_hub'. Add a DT node for the 'pp3300_a' path and a pinctrl entry for the GPIO. Make this path the default and keep trogdor rev1, lazor rev0 and rev1 on 'pp3300_l7c'. These earlier revisions also allocated the GPIO to the purpose of controlling the power switch, so there is no need to limit the pinctrl config to newer revisions. Remove the platform-wide 'always-on' property from 'pp3300_l7c' and add it to the boards that use this supply. Also delete the 'always-on' of 'pp3300_hub' for these boards. Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- Changes in v2: - added 'always-on' and 'boot-on' properties for new 'pp3300_hub' - removed platform-wide 'always-on' property for 'pp3300_l7c' - added 'always-on' property to 'pp3300_l7c' for boards that still use 'pp3300_l7c' - delete 'always-on' property of 'pp3300_hub' for boards that still use 'pp3300_l7c' - got rid of 'pp3300_hub_7c' label, just use 'pp3300_l7c' - fixed position of 'en_pp3300_hub' node to respect ordering - updated commit message .../boot/dts/qcom/sc7180-trogdor-lazor-r0.dts | 13 ++++++++ .../boot/dts/qcom/sc7180-trogdor-lazor-r1.dts | 13 ++++++++ .../arm64/boot/dts/qcom/sc7180-trogdor-r1.dts | 13 ++++++++ arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 33 ++++++++++++++++++- 4 files changed, 71 insertions(+), 1 deletion(-)