| Message ID | 20201105163724.v2.1.I5a75056d573808f40fed22ab7d28ea6be5819f84@changeid |
|---|---|
| State | Superseded |
| 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: > > One important delta with respect to rev1 is a switch of the power > supply for the onboard USB hub from 'pp3300_l7c' to 'pp3300_a' + a > load switch. The actual regulator switch is done by the patch 'arm64: > dts: qcom: sc7180-trogdor: Make pp3300_a the default supply for > pp3300_hub', since it affects the entire trogdor platform. Here we > only add the .dts files for lazor rev2 and replace the generic > compatible entries in the rev1 .dts files. > > Signed-off-by: Matthias Kaehlcke <mka@chromium.org> > --- > > Changes in v2: > - patch added to the series > > arch/arm64/boot/dts/qcom/Makefile | 3 +++ > .../dts/qcom/sc7180-trogdor-lazor-r1-kb.dts | 4 ++-- > .../dts/qcom/sc7180-trogdor-lazor-r1-lte.dts | 4 ++-- > .../boot/dts/qcom/sc7180-trogdor-lazor-r1.dts | 4 ++-- > .../dts/qcom/sc7180-trogdor-lazor-r2-kb.dts | 17 +++++++++++++++++ > .../dts/qcom/sc7180-trogdor-lazor-r2-lte.dts | 18 ++++++++++++++++++ > .../boot/dts/qcom/sc7180-trogdor-lazor-r2.dts | 15 +++++++++++++++ > 7 files changed, 59 insertions(+), 6 deletions(-) So it's pretty unlikely that this change actually happened in "-rev2". "-rev2" was a _very_ small batch of boards that I don't think made it into too many people's hands. You probably want "-rev3". > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r2-kb.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r2-kb.dts > new file mode 100644 > index 000000000000..7c3a702ef209 > --- /dev/null > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r2-kb.dts > @@ -0,0 +1,17 @@ > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > +/* > + * Google Lazor board device tree source > + * > + * Copyright 2020 Google LLC. > + */ > + > +#include "sc7180-trogdor-lazor-r1.dts" Should have been updated to not point to '-r1', no? === If you want to compare, you can also look at my (abandoned) CL: https://crrev.com/c/2481550 ...that forked out a "-rev3" to tag the WiFi slightly differently, but we ended up abandoning it because we found a better way to handle the WiFi stuff. -Doug
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/Makefile b/arch/arm64/boot/dts/qcom/Makefile index fb4631f898fd..3d72c7b63c79 100644 --- a/arch/arm64/boot/dts/qcom/Makefile +++ b/arch/arm64/boot/dts/qcom/Makefile @@ -26,6 +26,9 @@ dtb-$(CONFIG_ARCH_QCOM) += sc7180-trogdor-lazor-r0.dtb dtb-$(CONFIG_ARCH_QCOM) += sc7180-trogdor-lazor-r1.dtb dtb-$(CONFIG_ARCH_QCOM) += sc7180-trogdor-lazor-r1-kb.dtb dtb-$(CONFIG_ARCH_QCOM) += sc7180-trogdor-lazor-r1-lte.dtb +dtb-$(CONFIG_ARCH_QCOM) += sc7180-trogdor-lazor-r2.dtb +dtb-$(CONFIG_ARCH_QCOM) += sc7180-trogdor-lazor-r2-kb.dtb +dtb-$(CONFIG_ARCH_QCOM) += sc7180-trogdor-lazor-r2-lte.dtb dtb-$(CONFIG_ARCH_QCOM) += sc7180-trogdor-r1.dtb dtb-$(CONFIG_ARCH_QCOM) += sc7180-trogdor-r1-lte.dtb dtb-$(CONFIG_ARCH_QCOM) += sdm630-sony-xperia-ganges-kirin.dtb diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1-kb.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1-kb.dts index c3f426c3c30a..99f2c240c339 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1-kb.dts +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1-kb.dts @@ -8,8 +8,8 @@ #include "sc7180-trogdor-lazor-r1.dts" / { - model = "Google Lazor (rev1+) with KB Backlight"; - compatible = "google,lazor-sku2", "qcom,sc7180"; + model = "Google Lazor (rev1) with KB Backlight"; + compatible = "google,lazor-rev1-sku2", "qcom,sc7180"; }; &keyboard_backlight { diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1-lte.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1-lte.dts index 73e59cf7752a..4074c62207ce 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1-lte.dts +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1-lte.dts @@ -9,8 +9,8 @@ #include "sc7180-trogdor-lte-sku.dtsi" / { - model = "Google Lazor (rev1+) with LTE"; - compatible = "google,lazor-sku0", "qcom,sc7180"; + model = "Google Lazor (rev1) with LTE"; + compatible = "google,lazor-rev1-sku0", "qcom,sc7180"; }; &keyboard_backlight { 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 3151ae31c1cc..f6ee1beba458 100644 --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts @@ -10,6 +10,6 @@ #include "sc7180-trogdor-lazor.dtsi" / { - model = "Google Lazor (rev1+)"; - compatible = "google,lazor", "qcom,sc7180"; + model = "Google Lazor (rev1)"; + compatible = "google,lazor-rev1", "qcom,sc7180"; }; diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r2-kb.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r2-kb.dts new file mode 100644 index 000000000000..7c3a702ef209 --- /dev/null +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r2-kb.dts @@ -0,0 +1,17 @@ +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) +/* + * Google Lazor board device tree source + * + * Copyright 2020 Google LLC. + */ + +#include "sc7180-trogdor-lazor-r1.dts" + +/ { + model = "Google Lazor (rev2+) with KB Backlight"; + compatible = "google,lazor-sku2", "qcom,sc7180"; +}; + +&keyboard_backlight { + status = "okay"; +}; diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r2-lte.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r2-lte.dts new file mode 100644 index 000000000000..e19bdfd329be --- /dev/null +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r2-lte.dts @@ -0,0 +1,18 @@ +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) +/* + * Google Lazor board device tree source + * + * Copyright 2020 Google LLC. + */ + +#include "sc7180-trogdor-lazor-r2.dts" +#include "sc7180-trogdor-lte-sku.dtsi" + +/ { + model = "Google Lazor (rev2+) with LTE"; + compatible = "google,lazor-sku0", "qcom,sc7180"; +}; + +&keyboard_backlight { + status = "okay"; +}; diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r2.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r2.dts new file mode 100644 index 000000000000..68c04f6dfc05 --- /dev/null +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r2.dts @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) +/* + * Google Lazor board device tree source + * + * Copyright 2020 Google LLC. + */ + +/dts-v1/; + +#include "sc7180-trogdor-lazor.dtsi" + +/ { + model = "Google Lazor (rev2+)"; + compatible = "google,lazor", "qcom,sc7180"; +};
One important delta with respect to rev1 is a switch of the power supply for the onboard USB hub from 'pp3300_l7c' to 'pp3300_a' + a load switch. The actual regulator switch is done by the patch 'arm64: dts: qcom: sc7180-trogdor: Make pp3300_a the default supply for pp3300_hub', since it affects the entire trogdor platform. Here we only add the .dts files for lazor rev2 and replace the generic compatible entries in the rev1 .dts files. Signed-off-by: Matthias Kaehlcke <mka@chromium.org> --- Changes in v2: - patch added to the series arch/arm64/boot/dts/qcom/Makefile | 3 +++ .../dts/qcom/sc7180-trogdor-lazor-r1-kb.dts | 4 ++-- .../dts/qcom/sc7180-trogdor-lazor-r1-lte.dts | 4 ++-- .../boot/dts/qcom/sc7180-trogdor-lazor-r1.dts | 4 ++-- .../dts/qcom/sc7180-trogdor-lazor-r2-kb.dts | 17 +++++++++++++++++ .../dts/qcom/sc7180-trogdor-lazor-r2-lte.dts | 18 ++++++++++++++++++ .../boot/dts/qcom/sc7180-trogdor-lazor-r2.dts | 15 +++++++++++++++ 7 files changed, 59 insertions(+), 6 deletions(-) create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r2-kb.dts create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r2-lte.dts create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r2.dts