Re: [PATCHv1 1/3] ARM: dts: meson8b: odroidc1: Add usb phy power node

From: Anand Moon <linux.amoon@gmail.com>
To: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc: linux-phy@lists.infradead.org,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	linux-amlogic@lists.infradead.org,
	Linux Kernel <linux-kernel@vger.kernel.org>,
	Matt Corallo <oc2udbzfd@mattcorallo.com>,
	Rob Herring <robh+dt@kernel.org>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Kevin Hilman <khilman@baylibre.com>,
	Jerome Brunet <jbrunet@baylibre.com>,
	Kishon Vijay Abraham I <kishon@ti.com>,
	Vinod Koul <vkoul@kernel.org>,
	Emiliano Ingrassia <ingrassia@epigenesys.com>,
	devicetree <devicetree@vger.kernel.org>
Subject: Re: [PATCHv1 1/3] ARM: dts: meson8b: odroidc1: Add usb phy power node
Date: Wed, 14 Jul 2021 22:55:15 +0530	[thread overview]
Message-ID: <CANAwSgR5nNN3D+NZjqNNjyp=rVYE=RG2icnJAc=5F23xSh9Bvg@mail.gmail.com> (raw)
In-Reply-To: <CANAwSgTf4sjoufrZuK-EjE=+yA8zSoVbqhEyNhJJFBLUCOVZmQ@mail.gmail.com>

Hi Martin.

On Wed, 14 Jul 2021 at 17:29, Anand Moon <linux.amoon@gmail.com> wrote:
>
> Hi Martin,
>
> On Wed, 14 Jul 2021 at 02:05, Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
> >
> > Hi Anand,
> >
> > On Tue, Jul 13, 2021 at 8:45 PM Anand Moon <linux.amoon@gmail.com> wrote:
> > >
> > > Hi Martin,
> > >
> > > Thanks for reviewing the changes,
> > >
> > > On Tue, 13 Jul 2021 at 20:35, Martin Blumenstingl
> > > <martin.blumenstingl@googlemail.com> wrote:
> > > >
> > > > Hi Anand,
> > > >
> > > > On Tue, Jul 13, 2021 at 7:53 AM Anand Moon <linux.amoon@gmail.com> wrote:
> > > > >
> > > > > Add missing usb phy power node for phy mode fix below warning.
> > > > >
> > > > > [    1.253149] phy phy-c1108820.phy.0: Looking up phy-supply from device tree
> > > > > [    1.253166] phy phy-c1108820.phy.0: Looking up phy-supply property
> > > > >                 in node /soc/cbus@c1100000/phy@8820 failed
> > > > I did some testing on my own Odroid-C1+ and this patch is not doing
> > > > anything for me.
> > > > more information below.
> > > Some device node for USB will have
> > The mistake I made before is considering USB VBUS as PHY power supply.
> > I believe the USB PHY is actually powered by the AVDD18_USB_ADC and
> > USB33_VDDIOH signals. See the S905 datasheet [0], page 25
> > These are 1.8V and 3.3V signals while you are adding a 5V regulator.
> >
> OK, thanks.
> > [...]
> > > > > +               /*
> > > > > +                * signal name from schematics: USB_POWER
> > > > > +                */
> > > > Just a few lines below you're saying that the name from the schematics is PWREN
> > > > If this patch is getting another round then please clarify the actual
> > > > signal name, or name both signals if the schematics is actually using
> > > > both names.
> > > >
> > > As per the schematics.
> > > PWREN ---> GPIOAO.BIT5      gpio pin control
> > > USB_POWER ---> P5V0          power source regulator.
> > ah, thanks for clarifying this
> > my suggestion is to put that exact paragraph into the comment to avoid confusion
> >
> > [...]
> > > > Can you please give this a try on your Odroid-C1 as well?
> > > > The conclusion from my own testing is that GPIOAO_5 doesn't seem to be
> > > > related to USB1 (host-only) because if it was then inverting the
> > > > polarity (from active high to active low) should result in a change.
> > > >
> > >
> > > Ok I have modified as per above but not changes in gpio polarity
> > > from active high to active low. see below.
> > >
> > > # Odroid C1
> > > [alarm@archl-c1e ~]$ sudo cat /sys/kernel/debug/gpio | grep USB
> > >  gpio-1953 (USB_HUB_RST_N       |usb-hub-reset       ) out hi
> > >  gpio-1954 (USB_OTG_PWREN       |regulator-usbp_pwr_e) out hi
> > >
> > > # Odroid C2
> > > [alarm@archl-c2lm ~]$  sudo cat /sys/kernel/debug/gpio | grep usb
> > >  gpio-501 (USB HUB nRESET      |usb-hub-reset       ) out hi
> > >  gpio-502 (USB OTG Power En    |regulator-usb-pwrs  ) out hi
> > that's strange, my result is different
> >
> >   gpio = <&gpio_ao GPIOAO_5 GPIO_ACTIVE_HIGH>;
> >   enable-active-high;
> > gives me:
> >   # grep USB_OTG_PWREN /sys/kernel/debug/gpio
> >   gpio-418 (USB_OTG_PWREN       |regulator-usb-pwr-en) out hi
> >
> >   gpio = <&gpio_ao GPIOAO_5 GPIO_ACTIVE_LOW>;
> > gives me:
> >   # grep USB_OTG_PWREN /sys/kernel/debug/gpio
> >   gpio-418 (USB_OTG_PWREN       |regulator-usb-pwr-en) out lo ACTIVE LOW
> This gpio pin number dose not match the gpio pin on Odroid c1+, see below.
> >
> > Did you remove the "enable-active-high;" in your "active low" test?
> No
> > GPIO polarity for regulators is managed with that flag, not just with
> > GPIO_ACTIVE_{HIGH,LOW}
>
> It's just with changes the following, below
> +++ b/arch/arm/boot/dts/meson8b-odroidc1.dts
> @@ -47,7 +47,7 @@ usb_pwr_en: regulator-usb-pwr-en {
>                 /*
>                  * signal name from schematics: PWREN
>                  */
> -               gpio = <&gpio_ao GPIOAO_5 GPIO_ACTIVE_HIGH>;
> +               gpio = <&gpio_ao GPIOAO_5 GPIO_ACTIVE_LOW>;
>                 enable-active-high;
>         };
>

Can you give these small changes a try,
$ git diff

diff --git a/arch/arm/boot/dts/meson8b-odroidc1.dts
b/arch/arm/boot/dts/meson8b-odroidc1.dts
index 748f4c6a050a..066523f14074 100644
--- a/arch/arm/boot/dts/meson8b-odroidc1.dts
+++ b/arch/arm/boot/dts/meson8b-odroidc1.dts
@@ -47,8 +47,9 @@ usb_pwr_en: regulator-usb-pwr-en {
                /*
                 * signal name from schematics: PWREN
                 */
-               gpio = <&gpio_ao GPIOAO_5 GPIO_ACTIVE_HIGH>;
+               gpio = <&gpio_ao GPIOAO_5 GPIO_OPEN_DRAIN>;
                enable-active-high;
+               regulator-always-on;
        };

[alarm@archl-c1e ~]$ sudo cat /sys/kernel/debug/gpio | grep usb
 gpio-1953 (USB_HUB_RST_N       |usb-hub-reset       ) out hi
 gpio-1954 (USB_OTG_PWREN       |regulator-usb-pwr-en) out lo

Thanks
-Anand
