Re: [PATCH] arm64: dts: rockchip: fix USB regulator on ROCK64

["Heiko Stübner" <heiko@sntech.de>]

Hi,

Am Montag, 10. April 2023, 18:01:57 CEST schrieb Lorenz Brun:
> 
> Am Di, 10. Jan 2023 um 22:38:26 +01:00:00 schrieb Lorenz Brun 
> <lorenz@brun.one>:
> > On Tue, Jan 10 2023 at 14:02:47 +01:00:00, Heiko Stübner 
> > <heiko@sntech.de> wrote:
> >> Am Donnerstag, 5. Januar 2023, 01:29:47 CET schrieb Peter Geis:
> >>>  On Wed, Jan 4, 2023 at 6:55 PM Lorenz Brun <lorenz@brun.one> wrote:
> >>>  >
> >>>  >
> >>>  > On Wed, Jan 4 2023 at 18:46:25 -05:00:00, Peter Geis
> >>>  > <pgwipeout@gmail.com> wrote:
> >>>  > > On Wed, Jan 4, 2023 at 3:55 PM Lorenz Brun <lorenz@brun.one> 
> >>> \x7f\x7fwrote:
> >>>  > >>
> >>>  > >>  Currently the ROCK64 device tree specifies two regulators,
> >>>  > >> vcc_host_5v
> >>>  > >>  and vcc_host1_5v for USB VBUS on the device. Both of those 
> >>> are
> >>>  > >> however
> >>>  > >>  specified with RK_PA2 as the GPIO enabling them, causing the
> >>>  > >> following
> >>>  > >>  error when booting:
> >>>  > >>
> >>>  > >>    rockchip-pinctrl pinctrl: pin gpio0-2 already requested by
> >>>  > >> vcc-host-5v-regulator; cannot claim for vcc-host1-5v-regulator
> >>>  > >>    rockchip-pinctrl pinctrl: pin-2 (vcc-host1-5v-regulator) 
> >>> \x7f\x7fstatus
> >>>  > >> -22
> >>>  > >>    rockchip-pinctrl pinctrl: could not request pin 2 
> >>> (gpio0-2) \x7f\x7ffrom
> >>>  > >> group usb20-host-drv  on device rockchip-pinctrl
> >>>  > >>    reg-fixed-voltage vcc-host1-5v-regulator: Error applying 
> >>> \x7f\x7fsetting,
> >>>  > >> reverse things back
> >>>  > >>
> >>>  > >>  Looking at the schematic, there are in fact three USB 
> >>> \x7f\x7fregulators,
> >>>  > >>  vcc_host_5v, vcc_host1_5v and vcc_otg_v5. But the enable 
> >>> \x7f\x7fsignal for
> >>>  > >> all
> >>>  > >>  three is driven by Q2604 which is in turn driven by 
> >>> \x7f\x7fGPIO_A2/PA2.
> >>>  > >>
> >>>  > >>  Since these three regulators are not controllable 
> >>> separately, \x7f\x7fI
> >>>  > >> removed
> >>>  > >>  the second one which was causing the error and left a comment
> >>>  > >> explaining
> >>>  > >>  that this regulator actually controls multiple rails.
> >>>  > >>
> >>>  > >>  Signed-off-by: Lorenz Brun <lorenz@brun.one>
> >>>  > >>  ---
> >>>  > >>   arch/arm64/boot/dts/rockchip/rk3328-rock64.dts | 14 
> >>> \x7f\x7f+++-----------
> >>>  > >>   1 file changed, 3 insertions(+), 11 deletions(-)
> >>>  > >>
> >>>  > >>  diff --git a/arch/arm64/boot/dts/rockchip/rk3328-rock64.dts
> >>>  > >> b/arch/arm64/boot/dts/rockchip/rk3328-rock64.dts
> >>>  > >>  index f69a38f42d2d..bd82bc80444d 100644
> >>>  > >>  --- a/arch/arm64/boot/dts/rockchip/rk3328-rock64.dts
> >>>  > >>  +++ b/arch/arm64/boot/dts/rockchip/rk3328-rock64.dts
> >>>  > >>  @@ -37,6 +37,9 @@ vcc_sd: sdmmc-regulator {
> >>>  > >>                  vin-supply = <&vcc_io>;
> >>>  > >>          };
> >>>  > >>
> >>>  > >>  +       // vcc_host_5v also controls the vcc_host1_5v and
> >>>  > >> vcc_otg_5v rails
> >>>  > >>  +       // but there is only one common control signal
> >>>  > >> (USB20_HOST_DRV) at
> >>>  > >>  +       // GPIO_A2
> >>>  > >>          vcc_host_5v: vcc-host-5v-regulator {
> >>>  > >>                  compatible = "regulator-fixed";
> >>>  > >>                  gpio = <&gpio0 RK_PA2 GPIO_ACTIVE_LOW>;
> >>>  > >>  @@ -48,17 +51,6 @@ vcc_host_5v: vcc-host-5v-regulator {
> >>>  > >>                  vin-supply = <&vcc_sys>;
> >>>  > >>          };
> >>>  > >>
> >>>  > >>  -       vcc_host1_5v: vcc_otg_5v: vcc-host1-5v-regulator {
> >>>  > >>  -               compatible = "regulator-fixed";
> >>>  > >>  -               gpio = <&gpio0 RK_PA2 GPIO_ACTIVE_LOW>;
> >>>  > >>  -               pinctrl-names = "default";
> >>>  > >>  -               pinctrl-0 = <&usb20_host_drv>;
> >>>  > >>  -               regulator-name = "vcc_host1_5v";
> >>>  > >>  -               regulator-always-on;
> >>>  > >>  -               regulator-boot-on;
> >>>  > >>  -               vin-supply = <&vcc_sys>;
> >>>  > >>  -       };
> >>>  > >
> >>>  > > Fixed-regulator supports multiple regulators sharing a gpio, 
> >>> \x7f\x7fthe issue
> >>>  > > is you have the pinctrl assigned multiple times which is not
> >>>  > > supported. Simply removing the pinctrl from all but one of the
> >>>  > > regulators will solve this issue.
> >>>  > Sure, I can just remove the pinctrl. Should I do anything about 
> >>> \x7f\x7fthe
> >>>  > fact that there are three USB switches on that GPIO, but only 
> >>> two \x7f\x7fof
> >>>  > them are described as regulators here? Seems a bit inconsistent 
> >>> \x7f\x7fto me.
> >>> 
> >>>  All hardware *should* be described, though it isn't uncommon to 
> >>> see \x7f\x7fa
> >>>  single fixed-regulator describe several individual switches that 
> >>> are
> >>>  all fed from a common source and controlled by the same gpio. If 
> >>> \x7f\x7fthey
> >>>  are not fed by a common source (for example, the otg port is often 
> >>> \x7f\x7ffed
> >>>  from a separate regulator), they must be modeled separately.
> >> 
> >> Which is essentially what Lorenz' patch already did - moving the 3 
> >> \x7fswitches
> >> into one regulator. So it essentially comes down to where does the 
> >> \x7fnot-yet
> >> modeled otg-regulator get its current from - also vcc_sys?
> >> 
> > Yeah, all regulators (they are just USB load/overload switches) are 
> > powered from VCC_SYS, there aren't that many power rails on the 
> > ROCK64.
> > 
> > The alternative to my first patch would essentially be this:
> > 
> > --- a/arch/arm64/boot/dts/rockchip/rk3328-rock64.dts
> > +++ b/arch/arm64/boot/dts/rockchip/rk3328-rock64.dts
> > @@ -48,11 +48,18 @@ vcc_host_5v: vcc-host-5v-regulator {
> >                vin-supply = <&vcc_sys>;
> >        };
> > 
> > -       vcc_host1_5v: vcc_otg_5v: vcc-host1-5v-regulator {
> > +       vcc_host1_5v: vcc-host1-5v-regulator {
> > +               compatible = "regulator-fixed";
> > +               gpio = <&gpio0 RK_PA2 GPIO_ACTIVE_LOW>;
> > +               regulator-name = "vcc_host1_5v";
> > +               regulator-always-on;
> > +               regulator-boot-on;
> > +               vin-supply = <&vcc_sys>;
> > +       };
> > +
> > +       vcc_otg_5v: vcc-otg-5v-regulator {
> >                compatible = "regulator-fixed";
> >                gpio = <&gpio0 RK_PA2 GPIO_ACTIVE_LOW>;
> > -               pinctrl-names = "default";
> > -               pinctrl-0 = <&usb20_host_drv>;
> >                regulator-name = "vcc_host1_5v";
> >                regulator-always-on;
> >                regulator-boot-on;
> > 
> > This drops the pinctrl from all but the first regulator (which is a 
> > bit weird, the first one is not really special) and then describes 
> > three separate regulators driven by PA2.
> > 
> > I have no real preference, the one-regulator solution has the 
> > downside of not accurately describing the three separate load 
> > switches, the three-regulator solution has the downside of the 
> > pinctrl definitions being weird.
> > 
> > Regards,
> > Lorenz
> 
> I never got a response to this, I am fine with both approaches but 
> would prefer if one of them goes in so I no longer have this error on 
> every boot. If the approach above with three regulators is chosen I can 
> send out a proper patch.

Sorry about that.

I guess the solution I'd prefer is the original patch, but with a twist ;-) .

vcc_host_5v: vcc_host1_5v: vcc_otg_5v: vcc-host-5v-regulator {
...
}

A node can have multiple phandles (see for example [0]), so we'd do the
one node for all regulators (with one gpio), but use the correct phandles
to identify the actual line to the consumer.

This way we still keep somewhat close to the schematics so people can
follow along, but also have the benefit that the regulator gets turned
of when none of the 3 users need it.

Heiko


[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3399-gru-chromebook.dtsi#n129



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip