Re: [PATCHv2 1/4] ARM: dts: meson8b: odroidc1: Add usb phy power node

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
To: Anand Moon <linux.amoon@gmail.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: [PATCHv2 1/4] ARM: dts: meson8b: odroidc1: Add usb phy power node
Date: Sun, 18 Jul 2021 16:01:34 +0200	[thread overview]
Message-ID: <CAFBinCCGA0b=LVjbQood+iN+DHLodY0RJ-GrSOcgdzrDFJ25pQ@mail.gmail.com> (raw)
In-Reply-To: <CANAwSgT9-5zGr67aGzBmW6WDBfeiAvjv_zv2UbRC+YbgAGg==A@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 4263 bytes --]

Hi Anand,

On Sun, Jul 18, 2021 at 3:29 PM Anand Moon <linux.amoon@gmail.com> wrote:
>
> Hi Martin,
>
> Thanks for your valuable feedback,
>
> On Sun, 18 Jul 2021 at 17:07, Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
> >
> > Hi Anand,
> >
> > On Sun, Jul 18, 2021 at 5:38 AM Anand Moon <linux.amoon@gmail.com> wrote:
> > [...]
> > > > > enable input power to USB ports, set it to Active Low.
> > > > >
> > > > > [    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
> > > > high prio:
> > > > Can you please describe how I can test this patch?
> > > > My concern is that previously I have tested your patch with ACTIVE_LOW
> > > > and ACTIVE_HIGH polarity.
> > > > In both cases USB is working and I cannot observe any change (apart
> > > > from this debug message being gone).
> > > >
> > > > In the Odroid-C1 schematics (page 1) GPIOAO.BIT5 is connected to USB_OTG *only*.
> > > > I cannot give my Acked-/Reviewed-/Tested-by without a description of
> > > > how I can actually test the GPIO potion of this patch.
> > This question is still open.
> > Even with all your explanations below I am missing a way to verify if
> > GPIOAO_5 is the correct GPIO to use.
>
> From the schematics [1]
> https://dn.odroid.com/S805/Schematics/odroid-c1+_rev0.4_20160226.pdf
>
> You could find references to PWREN  <--- GPIOAO.BIT5
> The second reference is USB HOST Power Switch
> The third reference is USB HOST POWER.
>
> Hope I am clean in my thought process now.
Can you please point out the page numbers for me?

What I am seeing on page 1 is:
GPIOAO_5 (called GPIOAO.BIT5) is connected as an INPUT to the USB_OTG
PWREN signal (that's the green box with the label "USB_OTG.SchDoc"
above it).

Below (still on page 1) there's a whole section for
USB_HOST_PWR_SW.SchDoc and USB_HUB_GL852G-OHG.SchDoc.
This box also uses a PWREN signal, but from my understanding it's a
different one:
- PWREN in this area is an OUTPUT on the USB hub
(USB_HUB_GL852G-OHG.SchDoc). This is confirmed by the PWREN signals on
page 14 of the GL852 datasheet: [4]
- Then PWREN is an INPUT for the overcurrent switches (USB_HOST_PWR_SW.SchDoc)
This section does not mention GPIOAO_5 (or GPIOAO.BIT5) at all.

For better illustration I have attached a screenshot with some
annotations from the schematics.
This is how I understand the whole USB setup.
This doesn't mean that I am right though.

[...]
> > Technically I can write a patch that makes GPIOAO_13 (which is
> > connected to the status LED) show up as being used by
> > regulator-usb-pwr-en in debugfs.
>
> Yep, you are correct, If I used GPIOAO_13 wrong pin, it will not
> enable the USB power. See below.
> [alarm@archl-c1e ~]$ sudo cat /sys/kernel/debug/gpio | grep usb
>  gpio-1953 (USB_HUB_RST_N       |usb-hub-reset       ) out hi
Did you also delete the "leds" node from meson8b-odroidc1.dts?

[...]
> > So for new .dts support phy-supply should not be used anymore for VBUS
> > because phy-supply (described as "Phandle to a regulator that provides
> > power to the PHY." in
> > Documentation/devicetree/bindings/phy/phy-bindings.txt) and
> > vbus-supply are two different things.
> >
>
> It just came to my notice your email on this issue sees below.
> [0] https://patchwork.kernel.org/project/linux-usb/patch/20190306212431.5779-1-martin.blumenstingl@googlemail.com/
> [1] https://patchwork.kernel.org/patch/10868515/
> [2] https://git.openwrt.org/?p=openwrt/openwrt.git;a=commitdiff;h=d8b475212bbf9e5f80c1c923a9701dca5ceb23e2
>
> From the openwrt commit  d8b475212bbf9e5f80c1c923a9701dca5ceb23e2
> and binding yaml  [3]
> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/usb/dwc2.yaml#L87
>
> USB DWC2 power is linked to vbus-supply, so it should be moved to usb node.
> Now I am getting your point correctly.
Yes, I think we're now on the same page on this topic
I totally forgot about some of these patches - great that you found
them and added them here for further documentation of this topic!

Best regards,
Martin

[4] https://datasheet.lcsc.com/szlcsc/Genesys-Logic-GL852G-HHG12_C136618.pdf

[-- Attachment #2: odroidc1-usb-pwren.png --]
[-- Type: image/png, Size: 123552 bytes --]