Re: [PATCH v1] dts: rockpro64: Remove usb regulator-always-on

From: Shantur Rathore <i@shantur.com>
To: Hugh Cole-Baker <sigmaris@gmail.com>
Cc: Dragan Simic <dsimic@manjaro.org>,
	heiko@sntech.de, linux-rockchip@lists.infradead.org,
	 devicetree@vger.kernel.org
Subject: Re: [PATCH v1] dts: rockpro64: Remove usb regulator-always-on
Date: Fri, 22 Dec 2023 14:08:21 +0000	[thread overview]
Message-ID: <CABEcMwVvKLrB-h+t5=SozZiGPbr_wNDJ2zKY1wG=z_+Fx__Ayw@mail.gmail.com> (raw)
In-Reply-To: <CAAXNxMSo0pk0283h7Ti5FydXBkrfcte-mgD64xGPqXqjXXFRmw@mail.gmail.com>

Hi Cole,

On Fri, Dec 22, 2023 at 11:20 AM Hugh Cole-Baker <sigmaris@gmail.com> wrote:
>
> On Fri, 22 Dec 2023 at 00:54, Dragan Simic <dsimic@manjaro.org> wrote:
> >
> > On 2023-12-21 11:11, Shantur Rathore wrote:
> > > On Thu, Dec 14, 2023 at 2:35 PM Dragan Simic <dsimic@manjaro.org>
> > > wrote:
> > >> On 2023-12-14 15:24, Shantur Rathore wrote:
> > >> > On Sun, Dec 10, 2023 at 12:08 AM Dragan Simic <dsimic@manjaro.org>
> > >> > wrote:
> > >> >> On 2023-12-10 01:03, Shantur Rathore wrote:
> > >> >> > On Sat, Dec 9, 2023 at 11:43 PM Dragan Simic <dsimic@manjaro.org>
> > >> >> > wrote:
> > >> >> >> On 2023-12-10 00:35, Shantur Rathore wrote:
> > >> >> >> > USB port regulators should be controlled by PHYs
> > >> >> >> > so we remove always-on property and let PHYs manage the
> > >> >> >> > regulator.
> > >> >> >> >
> > >> >> >> > Typec port has misconfugred phy-supply and now that we are
> > >> >> >> > removing regulator-always-on, we need to fix the phy-supply
> > >> >> >> > so the PHYs are able to turn power to type-c port.
> > >> >> >> >
> > >> >> >> > Signed-off-by: Shantur Rathore <i@shantur.com>
> > >> >> >> > ---
> > >> >> >> > + devicetree
> > >> >> >> >
> > >> >> >> > After this patch the ports were confirmed to power up and down
> > >> >> >> > in u-boot when doing usb start and usb stop.
> > >> >> >> > At boot the regulators were off, the devices connected weren't
> > >> >> >> > powered up, on usb start the PHYs are able to power on the ports
> > >> >> >> > and on usb stop they were powered down.
> > >> >> >> >
> > >> >> >> > At the boot, the ports were powered down which was again powered
> > >> >> >> > up by Linux kernel when booting up.
> > >> >> >> >
> > >> >> >> >
> > >> >> >> >  arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi | 4 +---
> > >> >> >> >  1 file changed, 1 insertion(+), 3 deletions(-)
> > >> >> >> >
> > >> >> >> > diff --git a/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
> > >> >> >> > b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
> > >> >> >> > index bca2b50e0a..bd2824aa48 100644
> > >> >> >> > --- a/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
> > >> >> >> > +++ b/arch/arm64/boot/dts/rockchip/rk3399-rockpro64.dtsi
> > >> >> >> > @@ -192,7 +192,6 @@ vcc5v0_host: vcc5v0-host-regulator {
> > >> >> >> >               pinctrl-names = "default";
> > >> >> >> >               pinctrl-0 = <&vcc5v0_host_en>;
> > >> >> >> >               regulator-name = "vcc5v0_host";
> > >> >> >> > -             regulator-always-on;
> > >> >> >> >               vin-supply = <&vcc5v0_usb>;
> > >> >> >> >       };
> > >> >> >> >
> > >> >> >> > @@ -203,7 +202,6 @@ vcc5v0_typec: vcc5v0-typec-regulator {
> > >> >> >> >               pinctrl-names = "default";
> > >> >> >> >               pinctrl-0 = <&vcc5v0_typec_en>;
> > >> >> >> >               regulator-name = "vcc5v0_typec";
> > >> >> >> > -             regulator-always-on;
> > >> >> >> >               vin-supply = <&vcc5v0_usb>;
> > >> >> >> >       };
> > >> >> >> >
> > >> >> >> > @@ -863,7 +861,7 @@ u2phy0_otg: otg-port {
> > >> >> >> >       };
> > >> >> >> >
> > >> >> >> >       u2phy0_host: host-port {
> > >> >> >> > -             phy-supply = <&vcc5v0_host>;
> > >> >> >> > +             phy-supply = <&vcc5v0_typec>;
> > >> >> >> >               status = "okay";
> > >> >> >> >       };
> > >> >> >> >  };
> > >> >> >>
> > >> >> >> Quite frankly, something doesn't feel right there.  Would you mind,
> > >> >> >> please, to place this patch on hold until next week or so, at which
> > >> >> >> point I should have enough time to go through the RockPro64 schematic
> > >> >> >> thoroughly once again, and test the patch in detail?
> > >> >> >
> > >> >> > Sure, no worries.
> > >> >>
> > >> >> Great, thanks.
> > >> >>
> > >> >> > Would you mind letting me know which part doesn't feel right to you?
> > >> >>
> > >> >> Sure, it was about the last change in the patch.
> > >> >
> > >> > The TypeC port VBUS is VBUS_TYPEC on Page 20 of 33 marked for
> > >> > VBUS_[1-4] in schematic here - [0]
> > >> >
> > >> > [0] - https://files.pine64.org/doc/rockpro64/rockpro64_v21-SCH.pdf
> > >>
> > >> I see, thanks, and I already went once again through the RockPro64
> > >> schematic, but only briefly.  I need to do that again, but in detail,
> > >> and compare it thoroughly with the dts(i).
> > >
> > > Ping.
> >
> > Pong. :)  I haven't fogotten about this, but I simply haven't had enough
> > time yet, and I really want to have this checked and cleaned up.  I'm
> > sorry for the delay.
>
> I had a look over the schematic (Sheet 11, 19 and 20), and it shows the USB2.0
> PHY0 being used for both the TypeC port and one of the USB2.0 TypeA ports.
> VBUS for the TypeC port is VBUS_TYPEC, VBUS for the TypeA port is VCC5V0_HOST0.
>
> So I think the vcc5v0_host supply should be left as-is on the host-port node,
> and the phy-supply = <&vcc5v0_typec> should be added to the otg-port node a
> few lines above, instead.

You are right. I made a mistake by setting vcc5v0_typec to the wrong port node.
I have tested the change again.
With this turning off the vcc5v0_typec regulator the TypeC port is turned off
and turning the vcc5v0_host off all other TypeA ports are turned off.

I will send the v2 of this patch.

Regards,
Shantur