Hi Doug,
On Wed, Nov 04, 2020 at 04:29:50PM -0800, Doug Anderson wrote:
> Hi,
>
> On Tue, Nov 3, 2020 at 10:38 AM Matthias Kaehlcke <mka@xxxxxxxxxxxx> wrote:
> >
> > The trogdor design has two options for supplying the pp3300_hub power rail,
> > it can be supplied by pp3300_l7c or pp3300_a. Initially pp3300_l7c was
> > used, newer revisions (will) use pp3300_a as supply.
> >
> > Add a DT node for the pp3300_a path which includes a power switch that is
> > controlled by a GPIO. Make this path the default and keep trogdor rev1,
> > lazor rev0 and rev1 on pp3300_l7c.
>
> It might not hurt to mention that even on early hardware that GPIO84
> was allocated to this purpose but that it was a stuff option for what
> actually provided power to the hub. This explains why it's OK to add
> the fixed regulator (just with no clients) even on old hardware. If
> GPIO84 had been used for something else on old hardware this would
> have been bad.
ok
> > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > index bf875589d364..2d64e75a2d6d 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
> > @@ -174,6 +174,21 @@ 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>;
> > +
> > + vin-supply = <&pp3300_a>;
>
> You're leaving things in a bit of an inconsistent state here. The
> "pp3300_hub_7c" is always_on / boot_on. This new one isn't.
Actually the new "pp3300_hub" it is also on at boot, the Chrome OS bootloader
asserts the GPIO.
> I know this is slightly more complicated due to the fact that downstream we
> have a way to control the hub power but didn't quite get that resolved
> upstream, but the way you have it now, on new hardware upstream will
> power off the hub but also keep "pp3300_hub_7c" powered on for no
> reason. Seems like that should be fixed?
Our EEs told me that it would be ok in terms of power to keep "pp3300_hub_7c"
powered, since there would be no significant power consumption without load.
In any case unused RPMH regulators are switched off by the kernel ~30s after
boot, so I think we are ok:
[ 31.202219] ldo7: disabling
The above is from the l7c regulator on a Lazor rev2.
> > + };
> > +
> > /* BOARD-SPECIFIC TOP LEVEL NODES */
> >
> > backlight: backlight {
> > @@ -469,7 +484,7 @@ ppvar_l6c: ldo6 {
> > regulator-initial-mode = <RPMH_REGULATOR_MODE_HPM>;
> > };
> >
> > - pp3300_hub:
> > + pp3300_hub_7c:
>
> nit: If it were me, I probably wouldn't have bothered introducing the
> "pp3300_hub_7c" alias since it's not a real thing in the schematic. I
> would have just had the older revisions refer to "pp3300_l7c". If you
> really love the "pp3300_hub_7c", though, I won't stand in your way.
true, it's not really needed, I'll get rid of it in the next version.
> > pp3300_l7c: ldo7 {
> > regulator-min-microvolt = <3304000>;
> > regulator-max-microvolt = <3304000>;
> > @@ -1151,6 +1166,19 @@ pinconf {
> > };
> > };
> >
> > + en_pp3300_hub: en-pp3300-hub {
> > + pinmux {
> > + pins = "gpio84";
> > + function = "gpio";
> > + };
> > +
> > + pinconf {
> > + pins = "gpio84";
> > + drive-strength = <2>;
> > + bias-disable;
> > + };
> > + };
> > +
> > en_pp3300_dx_edp: en-pp3300-dx-edp {
>
> "hub" sorts after "dx", so the ordering is slightly wrong here.
ack, will change
