Meson-8b and Meson-gxbb Fix some missing code

PATCH Superseded Anand Moon 2 participants 12 messages Jul 13, 2021 – Jul 15, 2021

Subsystems (3)

Device tree bindings Arm/Amlogic Meson SoC support Generic PHY framework

lore

Share & tools

lore

Anand Moon

author

2021-07-13 05:52 UTC #0 lore raw

cover Meson-8b and Meson-gxbb Fix some missing code

On Odroid C1+ and Odroid C2 USB feature is broken

It's being observed the after initiation of USB phy
the USB port goes in to suspend state, If we pass usbcore.autosuspend=-1 via command line USB hot plug seen to be working.

Another issue I observed is increase of USB interrupts event even if there is not much activity on USB ports.

$ cat /proc/interrupts | grep usb
 35:   26462800          0          0          0     GIC-0  63 Level     c90c0000.usb, dwc2_hsotg:usb1

Changes added power node to usb phy and small code cleanup in usb phy.

Previous version RFC.
[0] https://patchwork.kernel.org/project/linux-amlogic/cover/[email protected]/
Dopped the reoder of code changes as of now.

Thanks
-Anand

Anand Moon (3):
ARM: dts: meson8b: odroidc1: Add usb phy power node
phy: amlogic: meson8b-usb2: Power off the PHY by putting it into reset
mode
phy: amlogic: meson8b-usb2: don't log an error on -EPROBE_DEFER

 arch/arm/boot/dts/meson8b-odroidc1.dts | 20 ++++++++++++++++++++
 drivers/phy/amlogic/phy-meson8b-usb2.c |  8 ++++++--
 2 files changed, 26 insertions(+), 2 deletions(-)

Signature · 7 lines

2.32.0


-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy

Anand Moon

author

2021-07-13 05:53 UTC #1 lore raw

PATCH 1/3 ARM: dts: meson8b: odroidc1: Add usb phy power node

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

Fixes 2eb79a4d15ff (ARM: dts: meson: enabling the USB Host

controller on Odroid-C1/C1+ board)

Cc Martin Blumenstingl <[email protected]>

Signed-off-by Anand Moon <[email protected]>

---
previous version
[0] https://patchwork.kernel.org/project/linux-amlogic/patch/[email protected]

changes fix the vbus-suppy to phy-supply, drop enable usb0

# sudo cat /sys/kernel/debug/regulator/regulator_summary
    USB_PWR                       2    1      0 unknown  5000mV     0mA  5000mV  5000mV
       phy-c1108820.phy.0-phy     2                                 0mA     0mV     0mV
---
 arch/arm/boot/dts/meson8b-odroidc1.dts | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

arch/arm/boot/dts/meson8b-odroidc1.dts +20 -0

index c440ef94e082..ced1ec1c4878 100644
@@ -32,6 +32,25 @@ emmc_pwrseq: emmc-pwrseq {
 		reset-gpios = <&gpio BOOT_9 GPIO_ACTIVE_LOW>;
 	};
 
+	usb_pwr_en: regulator-usb-pwr-en {
+		compatible = "regulator-fixed";
+
+		regulator-name = "USB_PWR";
+
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		/*
+		 * signal name from schematics: USB_POWER
+		 */
+		vin-supply = <&p5v0>;
+
+		/*
+		 * signal name from schematics: PWREN
+		 */
+		gpio = <&gpio_ao GPIOAO_5 GPIO_ACTIVE_HIGH>;
+		enable-active-high;
+	};
+
 	leds {
 		compatible = "gpio-leds";
 		blue {

@@ -378,6 +397,7 @@ &uart_AO {
 
 &usb1_phy {
 	status = "okay";
+	phy-supply = <&usb_pwr_en>;
 };
 
 &usb1 {

Signature · 1 line

2.32.0

Martin Blumenstingl

reviewer

2021-07-13 15:05 UTC #2 lore raw

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

Hi Anand,

Quoted 6 lines · Anand Moon

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.

[...]

arch/arm/boot/dts/meson8b-odroidc1.dts emmc_pwrseq: emmc-pwrseq { · line 44

Quoted 3 lines

arch/arm/boot/dts/meson8b-odroidc1.dts +3 -0

+               /*
+                * 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.

[...]

arch/arm/boot/dts/meson8b-odroidc1.dts emmc_pwrseq: emmc-pwrseq { · line 51

Quoted 2 lines

arch/arm/boot/dts/meson8b-odroidc1.dts +2 -0

+               gpio = <&gpio_ao GPIOAO_5 GPIO_ACTIVE_HIGH>;
+               enable-active-high;

I booted my Odroid-C1+ with this and USB was working fine. Then I replaced these two lines with:
gpio = <&gpio_ao GPIOAO_5 GPIO_ACTIVE_LOW>;
and I found that USB is still working.

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.

[...]

Quoted 3 lines

 &usb1_phy {
        status = "okay";
+ phy-supply = <&usb_pwr_en>;

From the schematics it seems that this is not the PHY supply (which I admittedly have used incorrectly for VBUS before).
In the schematics that I have (odroid-c1+_rev0.4_20150615.pdf) it seems to be enabling VBUS.
So in that case a vbus-supply property should be used inside &usb1 instead.

Best regards,
Martin

Anand Moon

author

2021-07-13 18:45 UTC #3 lore raw

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

Hi Martin,

Thanks for reviewing the changes,

Quoted 5 lines · Martin Blumenstingl

Hi Anand,
Quoted 1 line · Anand Moon

Quoted 8 lines

Quoted 5 lines
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

Quoted 10 lines

[...]
arch/arm/boot/dts/meson8b-odroidc1.dts emmc_pwrseq: emmc-pwrseq { · line 44
Quoted 3 lines
arch/arm/boot/dts/meson8b-odroidc1.dts +3 -0
+               /*
+                * 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.

Quoted 8 lines

[...]
arch/arm/boot/dts/meson8b-odroidc1.dts emmc_pwrseq: emmc-pwrseq { · line 51
Quoted 2 lines
arch/arm/boot/dts/meson8b-odroidc1.dts +2 -0
+               gpio = <&gpio_ao GPIOAO_5 GPIO_ACTIVE_HIGH>;
+               enable-active-high;
I booted my Odroid-C1+ with this and USB was working fine. Then I replaced these two lines with:
gpio = <&gpio_ao GPIOAO_5 GPIO_ACTIVE_LOW>;
and I found that USB is still working.

Yep it should be GPIOAO_5 GPIO_ACTIVE_LOW, my mistake

Quoted 5 lines

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

Quoted 10 lines

[...]
Quoted 3 lines
 &usb1_phy {
        status = "okay";
+ phy-supply = <&usb_pwr_en>;
From the schematics it seems that this is not the PHY supply (which I admittedly have used incorrectly for VBUS before).
In the schematics that I have (odroid-c1+_rev0.4_20150615.pdf) it seems to be enabling VBUS.
So in that case a vbus-supply property should be used inside &usb1 instead.

As per the debug log I have added this since core phy looking for this property

[    1.250044] phy phy-c1108820.phy.0: Looking up phy-supply from device tree
[    1.250060] phy phy-c1108820.phy.0: Looking up phy-supply property
in node /soc/cbus@c1100000/phy@8820 failed
[    7.222566] libphy: stmmac: probed

vbus-supply power is needed for dwc2 node see below.

[    1.257714] dwc2 c90c0000.usb: Looking up vbus-supply from device tree
[    1.257725] dwc2 c90c0000.usb: Looking up vbus-supply property in

node /soc/usb@c90c0000 failed

Quoted 3 lines

Best regards,
Martin

Thanks
-Anand

Martin Blumenstingl

reviewer

2021-07-13 20:35 UTC #4 lore raw

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

Hi Anand,

Quoted 14 lines · Anand Moon

Hi Martin,
Thanks for reviewing the changes,
Quoted 7 lines · Martin Blumenstingl
Hi Anand,
Quoted 3 lines · Anand Moon
Add missing usb phy power node for phy mode fix below warning.

Quoted 7 lines

Quoted 6 lines
Quoted 3 lines
[    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.

[...]

Quoted 11 lines

Quoted 8 lines
arch/arm/boot/dts/meson8b-odroidc1.dts emmc_pwrseq: emmc-pwrseq { · line 44
Quoted 3 lines
arch/arm/boot/dts/meson8b-odroidc1.dts +3 -0
+               /*
+                * 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

[...]

Quoted 14 lines

Quoted 5 lines
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

Quoted 4 lines

# 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

Did you remove the "enable-active-high;" in your "active low" test? GPIO polarity for regulators is managed with that flag, not just with GPIO_ACTIVE_{HIGH,LOW}

[...]

Quoted 10 lines

Quoted 9 lines
Quoted 3 lines
 &usb1_phy {
        status = "okay";
+ phy-supply = <&usb_pwr_en>;
From the schematics it seems that this is not the PHY supply (which I admittedly have used incorrectly for VBUS before).
In the schematics that I have (odroid-c1+_rev0.4_20150615.pdf) it seems to be enabling VBUS.
So in that case a vbus-supply property should be used inside &usb1 instead.
As per the debug log I have added this since core phy looking for this property

Let's discuss the results with different polarities first, then we can also discuss the rest.

Best regards,
Martin

Anand Moon

author

2021-07-14 11:59 UTC #5 lore raw

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

Hi Martin,

Quoted 21 lines · Martin Blumenstingl

Hi Anand,
Quoted 17 lines · Anand Moon
Hi Martin,
Thanks for reviewing the changes,
Quoted 10 lines · Martin Blumenstingl
Hi Anand,
Quoted 6 lines · Anand Moon
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

Quoted 9 lines

Quoted 4 lines
Quoted 3 lines
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.

Quoted 39 lines

[...]
Quoted 11 lines
Quoted 8 lines
arch/arm/boot/dts/meson8b-odroidc1.dts emmc_pwrseq: emmc-pwrseq { · line 44
Quoted 3 lines
arch/arm/boot/dts/meson8b-odroidc1.dts +3 -0
+               /*
+                * 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
[...]
Quoted 14 lines
Quoted 5 lines
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
Quoted 4 lines
# 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:

Quoted 7 lines

  # 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.

Quoted 2 lines

Did you remove the "enable-active-high;" in your "active low" test?

Quoted 2 lines

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;
        };

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

[alarm@archl-c1e ~]$ sudo cat /sys/kernel/debug/gpio
gpiochip1: GPIOs 1949-1964, parent: platform/c8100084.pinctrl, aobus-banks:

 gpio-1949 (UART TX             )
 gpio-1950 (UART RX             )
 gpio-1951 (                    )
 gpio-1952 (TF_3V3N_1V8_EN      |TF_IO               ) out lo
 gpio-1953 (USB_HUB_RST_N       |usb-hub-reset       ) out hi
 gpio-1954 (USB_OTG_PWREN       |regulator-usb-pwr-en) out hi
 gpio-1955 (J7 Header Pin 2     )
 gpio-1956 (IR_IN               )
 gpio-1957 (J7 Header Pin 4     )
 gpio-1958 (J7 Header Pin 6     )
 gpio-1959 (J7 Header Pin 5     )
 gpio-1960 (J7 Header Pin 7     )
 gpio-1961 (HDMI_CEC            )
 gpio-1962 (SYS_LED             |c1:blue:alive       ) out hi ACTIVE LOW
 gpio-1963 (                    )
 gpio-1964 (                    )

gpiochip0: GPIOs 1965-2047, parent: platform/c1109880.pinctrl, cbus-banks:

 gpio-1965 (J2 Header Pin 35    )
 gpio-1966 (J2 Header Pin 36    )
 gpio-1967 (J2 Header Pin 32    )
 gpio-1968 (J2 Header Pin 31    )
 gpio-1969 (J2 Header Pin 29    )
 gpio-1970 (J2 Header Pin 18    )
 gpio-1971 (J2 Header Pin 22    )
 gpio-1972 (J2 Header Pin 16    )
 gpio-1973 (J2 Header Pin 23    )
 gpio-1974 (J2 Header Pin 21    )
 gpio-1975 (J2 Header Pin 19    )
 gpio-1976 (J2 Header Pin 33    )
 gpio-1977 (J2 Header Pin 8     )
 gpio-1978 (J2 Header Pin 10    )
 gpio-1979 (J2 Header Pin 15    )
 gpio-1980 (J2 Header Pin 13    )
 gpio-1981 (J2 Header Pin 24    )
 gpio-1982 (J2 Header Pin 26    )
 gpio-1983 (Revision (upper)    )
 gpio-1984 (Revision (lower)    )
 gpio-1985 (J2 Header Pin 7     )
 gpio-1986 (                    )
 gpio-1987 (J2 Header Pin 12    )
 gpio-1988 (J2 Header Pin 11    )
 gpio-1989 (                    )
 gpio-1990 (                    )
 gpio-1991 (                    )
 gpio-1992 (TFLASH_VDD_EN       |regulator-tflash_vdd) out lo
 gpio-1993 (                    )
 gpio-1994 (                    )
 gpio-1995 (VCCK_PWM (PWM_C)    )
 gpio-1996 (I2CA_SDA            )
 gpio-1997 (I2CA_SCL            )
 gpio-1998 (I2CB_SDA            )
 gpio-1999 (I2CB_SCL            )
 gpio-2000 (VDDEE_PWM (PWM_D)   )
 gpio-2001 (                    )
 gpio-2002 (HDMI_HPD            )
 gpio-2003 (HDMI_I2C_SDA        )
 gpio-2004 (HDMI_I2C_SCL        )
 gpio-2005 (ETH_PHY_INTR        )
 gpio-2006 (ETH_PHY_NRST        |PHY reset           ) out hi ACTIVE LOW
 gpio-2007 (ETH_TXD1            )
 gpio-2008 (ETH_TXD0            )
 gpio-2009 (ETH_TXD3            )
 gpio-2010 (ETH_TXD2            )
 gpio-2011 (ETH_RGMII_TX_CLK    )
 gpio-2012 (SD_DATA1 (SDB_D1)   )
 gpio-2013 (SD_DATA0 (SDB_D0)   )
 gpio-2014 (SD_CLK              )
 gpio-2015 (SD_CMD              )
 gpio-2016 (SD_DATA3 (SDB_D3)   )
 gpio-2017 (SD_DATA2 (SDB_D2)   )
 gpio-2018 (SD_CDN (SD_DET_N)   |cd                  ) in  hi ACTIVE LOW
 gpio-2019 (SDC_D0 (EMMC)       )
 gpio-2020 (SDC_D1 (EMMC)       )
 gpio-2021 (SDC_D2 (EMMC)       )
 gpio-2022 (SDC_D3 (EMMC)       )
 gpio-2023 (SDC_D4 (EMMC)       )
 gpio-2024 (SDC_D5 (EMMC)       )
 gpio-2025 (SDC_D6 (EMMC)       )
 gpio-2026 (SDC_D7 (EMMC)       )
 gpio-2027 (SDC_CLK (EMMC)      )
 gpio-2028 (SDC_RSTn (EMMC)     |reset               ) out hi ACTIVE LOW
 gpio-2029 (SDC_CMD (EMMC)      )
 gpio-2030 (BOOT_SEL            )
 gpio-2031 (                    )
 gpio-2032 (                    )
 gpio-2033 (                    )
 gpio-2034 (                    )
 gpio-2035 (                    )
 gpio-2036 (                    )
 gpio-2037 (                    )
 gpio-2038 (ETH_RXD1            )
 gpio-2039 (ETH_RXD0            )
 gpio-2040 (ETH_RX_DV           )
 gpio-2041 (RGMII_RX_CLK        )
 gpio-2042 (ETH_RXD3            )
 gpio-2043 (ETH_RXD2            )
 gpio-2044 (ETH_TXEN            )

gpio-2045 (ETH_PHY_REF_CLK_25MO)
gpio-2046 (ETH_MDC )
gpio-2047 (ETH_MDIO )

Quoted 11 lines

[...]
Quoted 9 lines
Quoted 9 lines
Quoted 3 lines
 &usb1_phy {
        status = "okay";
+ phy-supply = <&usb_pwr_en>;
From the schematics it seems that this is not the PHY supply (which I admittedly have used incorrectly for VBUS before).
In the schematics that I have (odroid-c1+_rev0.4_20150615.pdf) it seems to be enabling VBUS.
So in that case a vbus-supply property should be used inside &usb1 instead.

Quoted 7 lines

Quoted 1 line
As per the debug log I have added this since core phy looking for this property
Let's discuss the results with different polarities first, then we can also discuss the rest.
Best regards,
Martin

Thanks
Anand

Anand Moon

author

2021-07-14 17:25 UTC #6 lore raw

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

Hi Martin.

Quoted 92 lines · Anand Moon

Hi Martin,
Quoted 21 lines · Martin Blumenstingl
Hi Anand,
Quoted 17 lines · Anand Moon
Hi Martin,
Thanks for reviewing the changes,
Quoted 10 lines · Martin Blumenstingl
Hi Anand,
Quoted 6 lines · Anand Moon
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
Quoted 9 lines
Quoted 4 lines
Quoted 3 lines
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.
Quoted 39 lines
[...]
Quoted 11 lines
Quoted 8 lines
Review
Quoted 3 lines
(quoted diff) +3 -0
+               /*
+                * 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
[...]
Quoted 14 lines
Quoted 5 lines
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
Quoted 4 lines
# 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:
Quoted 7 lines
  # 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.
Quoted 2 lines
Did you remove the "enable-active-high;" in your "active low" test?
No
Quoted 2 lines
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

Review usb_pwr_en: regulator-usb-pwr-en { · line 53

Quoted 8 lines

(quoted diff) +1 -1

@@ -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

(quoted diff) +0 -0

b/arch/arm/boot/dts/meson8b-odroidc1.dts
index 748f4c6a050a..066523f14074 100644

arch/arm/boot/dts/meson8b-odroidc1.dts +2 -1

@@ -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

Martin Blumenstingl

reviewer

2021-07-14 23:30 UTC #7 lore raw

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

Hi Anand,

On Wed, Jul 14, 2021 at 7:25 PM Anand Moon <[email protected]> wrote:
[...]

Quoted 17 lines

Can you give these small changes a try,

$ git diff

arch/arm/boot/dts/meson8b-odroidc1.dts +2 -1

index 748f4c6a050a..066523f14074 100644
@@ -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;
        };

Quoted 3 lines

[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

I can reproduce the /sys/kernel/debug/gpio output with this patch

Still USB works for me regardless of whether USB_OTG_PWREN is HIGH or LOW This is something that is not possible if the regulator is really connected on the board like you are describing in this patch. If this .dts change was correct then I would expect that USB is breaking when inverting the GPIO polarity.

I am using the "inverted GPIO polarity" approach to find the Ethernet PHY reset GPIO when working on boards for which I don't have the schematics:
1) make an assumption of which GPIO to use
2) try with GPIO_ACTIVE_LOW -> PHY should be detected
3) change it to GPIO_ACTIVE_HIGH -> PHY should not be found anymore (because it's in reset)
4) before submitting the board.dts upstream I of course change it back to GPIO_ACTIVE_LOW

If during step 3) the PHY is still found then I know that it's not the correct GPIO.
I am seeing the same behavior with this USB regulator. My interpretation of this is: either you are not using the right GPIO or the GPIO is not related to &usb1 (or it's PHY).

Best regards,
Martin

Anand Moon

author

2021-07-15 10:24 UTC #8 lore raw

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

Hi Martin,

Quoted 33 lines · Martin Blumenstingl

Hi Anand,
On Wed, Jul 14, 2021 at 7:25 PM Anand Moon <[email protected]> wrote:
[...]
Quoted 17 lines
Can you give these small changes a try,
$ git diff
arch/arm/boot/dts/meson8b-odroidc1.dts +2 -1
index 748f4c6a050a..066523f14074 100644
@@ -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;
        };
 
Quoted 3 lines
[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
I can reproduce the /sys/kernel/debug/gpio output with this patch
Still USB works for me regardless of whether USB_OTG_PWREN is HIGH or LOW This is something that is not possible if the regulator is really connected on the board like you are describing in this patch. If this .dts change was correct then I would expect that USB is breaking when inverting the GPIO polarity.

Quoted 3 lines

I am using the "inverted GPIO polarity" approach to find the Ethernet PHY reset GPIO when working on boards for which I don't have the schematics:

Thanks for the hint,

Quoted 6 lines

1) make an assumption of which GPIO to use
2) try with GPIO_ACTIVE_LOW -> PHY should be detected
3) change it to GPIO_ACTIVE_HIGH -> PHY should not be found anymore (because it's in reset)
4) before submitting the board.dts upstream I of course change it back to GPIO_ACTIVE_LOW

Yes I am going to changes this to GPIO_ACTIVE_LOW in the next version. These dts changes just assist in power through PHY to USB ports. After going through the previous email I got this working see below.

[alarm@archl-c1e linux-amlogic-5.y-devel]$ 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 ACTIVE LOW

Quoted 7 lines

If during step 3) the PHY is still found then I know that it's not the correct GPIO.
I am seeing the same behavior with this USB regulator. My interpretation of this is: either you are not using the right GPIO or the GPIO is not related to &usb1 (or it's PHY).

With reference to the schematic odroid-c1+_rev0.4_20160226 section USB HOST POWER ---- MP62551DGT-LF-Z
Both USB POWER and PWREN help control the power to USB Ports.

Quoted 3 lines

Best regards,
Martin

Thanks
-Anand

Anand Moon

author

2021-07-13 05:53 UTC #9 lore raw

PATCH 2/3 phy: amlogic: meson8b-usb2: Power off the PHY by putting it into reset mode

In reply to Anand Moon

Power off the PHY by putting it into reset mode.

Cc Martin Blumenstingl <[email protected]>

Signed-off-by Anand Moon <[email protected]>

---
Fix the commit message.
rfc - Keep vendor driver sequence for the power on reset of usb phy
[0] https://patchwork.kernel.org/project/linux-amlogic/patch/[email protected]/

---
 drivers/phy/amlogic/phy-meson8b-usb2.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/phy/amlogic/phy-meson8b-usb2.c +4 -0

index 03c061dd5f0d..2aad45c55494 100644
@@ -219,6 +219,10 @@ static int phy_meson8b_usb2_power_off(struct phy *phy)
 	clk_disable_unprepare(priv->clk_usb);
 	clk_disable_unprepare(priv->clk_usb_general);
 
+	/* power off the PHY by putting it into reset mode */
+	regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_POWER_ON_RESET,
+			   REG_CTRL_POWER_ON_RESET);
+
 	return 0;
 }

Signature · 1 line

2.32.0

Martin Blumenstingl

reviewer Acked-by on v1 2/3

2021-07-13 15:10 UTC #10 lore raw

Re: [PATCHv1 2/3] phy: amlogic: meson8b-usb2: Power off the PHY by putting it into reset mode

Quoted 5 lines · Anand Moon

Power off the PHY by putting it into reset mode.
Cc Martin Blumenstingl <[email protected]>
Signed-off-by Anand Moon <[email protected]>

based on code from the vendor kernel [0] this gets my:

Acked-by Martin Blumenstingl <[email protected]>

[0] https://github.com/endlessm/linux-meson/blob/0672f0b61eb92ba63c91d858a678d2c3a0bba06a/drivers/amlogic/usb/dwc_otg/310/dwc_otg_attr.c#L706

Anand Moon

author

2021-07-13 05:53 UTC #11 lore raw

PATCH 3/3 phy: amlogic: meson8b-usb2: don't log an error on -EPROBE_DEFER

In reply to Anand Moon

devm_phy_create can return -EPROBE_DEFER if the phy-supply is not ready yet. Silence this warning as the driver framework will re-attempt registering the PHY. Use dev_err_probe() for phy resources to indicate the deferral reason when waiting for the resource to come up.

Cc Martin Blumenstingl <[email protected]>

Reviewed-by Martin Blumenstingl <[email protected]>

Signed-off-by Anand Moon <[email protected]>

---
[0] https://patchwork.kernel.org/project/linux-amlogic/patch/[email protected]/
---

---
 drivers/phy/amlogic/phy-meson8b-usb2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/phy/amlogic/phy-meson8b-usb2.c +2 -2

index 2aad45c55494..cf10bed40528 100644
@@ -277,8 +277,8 @@ static int phy_meson8b_usb2_probe(struct platform_device *pdev)
 
 	phy = devm_phy_create(&pdev->dev, NULL, &phy_meson8b_usb2_ops);
 	if (IS_ERR(phy)) {
-		dev_err(&pdev->dev, "failed to create PHY\n");
-		return PTR_ERR(phy);
+		return dev_err_probe(&pdev->dev, PTR_ERR(phy),
+				     "failed to create PHY\n");
 	}
 
 	phy_set_drvdata(phy, priv);

Signature · 1 line

2.32.0