2021-08-17 04:18:33

by Anand Moon

[permalink] [raw]
Subject: [PATCHv3 0/6] 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 hotplug 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
8
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 reorder of code changes as of now.

V1 > changes Fixed the GPIO input signal on Odroid C1+/C2
New patch added to fix Odroid C2.

[1] https://lore.kernel.org/linux-devicetree/[email protected]/

V2 > changes Fixed the GPIO polarity for Odroid C1
fix the power source from phy-supply to vbus-supply
added new patches to fix resolve some issues.

Thanks
-Anand

Anand Moon (6):
ARM: dts: meson8b: odroidc1: Add usb phy power node
ARM: dts: meson8b: odroidc1: Set usb power source to always on
arm64: dts: amlogic: odroidc2: Fix the chip enable signal for usb
power
arm64: dts: amlogic: odroidc2: use vbus-supply for power source for
usb nodes
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 | 21 ++++++++++++++++++-
.../boot/dts/amlogic/meson-gxbb-odroidc2.dts | 10 ++++-----
drivers/phy/amlogic/phy-meson8b-usb2.c | 8 +++++--
3 files changed, 30 insertions(+), 9 deletions(-)

--
2.32.0


2021-08-17 04:18:39

by Anand Moon

[permalink] [raw]
Subject: [PATCHv3 1/6] ARM: dts: meson8b: odroidc1: Add usb phy power node

Add missing usb phy power node for usb node fix below warning.
P5V0 regulator supply input voltage range to USB host controller.
As described in the C1+ schematics, GPIO GPIOAO_5 is used to
enable input power to USB ports, set it to Active Low.

[ 1.260772] dwc2 c90c0000.usb: Looking up vbus-supply from device tree
[ 1.260784] dwc2 c90c0000.usb: Looking up vbus-supply property in
mode /soc/usb@c90c0000 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]>
---
V2 > changes gpio polarity ACTIVE_HIGH to ACTIVE_LOW.
Fix the source power from phy-supply to vbus-supply.

[1] https://lore.kernel.org/linux-devicetree/[email protected]/

V1 > Fix the Input GPIO polarity from HIGH to LOW.

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

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

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 | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/meson8b-odroidc1.dts b/arch/arm/boot/dts/meson8b-odroidc1.dts
index c440ef94e082..30ec6a7f20c7 100644
--- a/arch/arm/boot/dts/meson8b-odroidc1.dts
+++ b/arch/arm/boot/dts/meson8b-odroidc1.dts
@@ -32,6 +32,23 @@ 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_OTG_PWR";
+
+ regulator-min-microvolt = <5000000>;
+ regulator-max-microvolt = <5000000>;
+ /*
+ * signal name from schematics: USB_POWER - P5V0
+ */
+ vin-supply = <&p5v0>;
+ /*
+ * signal name from schematics: PWREN - GPIOAO.BIT5
+ */
+ gpios = <&gpio_ao GPIOAO_5 GPIO_ACTIVE_LOW>;
+ };
+
leds {
compatible = "gpio-leds";
blue {
@@ -119,7 +136,6 @@ vcc_3v3: regulator-vcc-3v3 {
regulator-name = "VCC3V3";
regulator-min-microvolt = <3300000>;
regulator-max-microvolt = <3300000>;
-
vin-supply = <&p5v0>;
};

@@ -382,4 +398,5 @@ &usb1_phy {

&usb1 {
status = "okay";
+ vbus-supply = <&usb_pwr_en>;
};
--
2.32.0

2021-08-17 04:19:14

by Anand Moon

[permalink] [raw]
Subject: [PATCHv3 3/6] arm64: dts: amlogic: odroidc2: Fix the chip enable signal for usb power

Fix the chip enable signal changing from Active High to Active Low
to enable input power to USB power. Also updated signal name as per
the schematics.

Fixes: 5a0803bd5ae2 ("ARM64: dts: meson-gxbb-odroidc2: Enable USB Nodes")

Cc: Martin Blumenstingl <[email protected]>
Signed-off-by: Anand Moon <[email protected]>
---
v2:
Fix the typo: shematics -> schematics
---
arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts b/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts
index 201596247fd9..3f4f16a5dedd 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts
@@ -38,12 +38,11 @@ usb_otg_pwr: regulator-usb-pwrs {
regulator-max-microvolt = <5000000>;

/*
- * signal name from schematics: PWREN
+ * signal name from schematics: PWREN - GPIOAO.BIT5
*/
- gpio = <&gpio_ao GPIOAO_5 GPIO_ACTIVE_HIGH>;
- enable-active-high;
+ gpios = <&gpio_ao GPIOAO_5 GPIO_ACTIVE_LOW>;
/*
- * signal name from schematics: USB_POWER
+ * signal name from schematics: USB_POWER - P5V0
*/
vin-supply = <&p5v0>;
};
--
2.32.0

2021-08-17 04:19:17

by Anand Moon

[permalink] [raw]
Subject: [PATCHv3 4/6] arm64: dts: amlogic: odroidc2: use vbus-supply for power source for usb nodes

Use vbus-supply instead of phy-supply as power source for dwc2
nodes. Drop vbus-supply for usb0 node, as it will handle later.

Fixes: e841ec956e53 ("ARM64: dts: meson-gxbb-odroidc2:
fix usb1 power supply")

Cc: Martin Blumenstingl <[email protected]>
Signed-off-by: Anand Moon <[email protected]>
---
New patch in this series.
---
arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts b/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts
index 3f4f16a5dedd..01bbfc4c091e 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts
@@ -400,12 +400,10 @@ &uart_AO {

&usb0_phy {
status = "disabled";
- phy-supply = <&usb_otg_pwr>;
};

&usb1_phy {
status = "okay";
- phy-supply = <&usb_otg_pwr>;
};

&usb0 {
@@ -414,4 +412,5 @@ &usb0 {

&usb1 {
status = "okay";
+ vbus-supply = <&usb_otg_pwr>;
};
--
2.32.0

2021-08-17 04:19:26

by Anand Moon

[permalink] [raw]
Subject: [PATCHv3 5/6] phy: amlogic: meson8b-usb2: Power off the PHY by putting it into reset mode

Power off the PHY by putting it into reset mode.

Cc: Martin Blumenstingl <[email protected]>
Acked-by: Martin Blumenstingl <[email protected]>
Signed-off-by: Anand Moon <[email protected]>
---
v2 - None
---
drivers/phy/amlogic/phy-meson8b-usb2.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/drivers/phy/amlogic/phy-meson8b-usb2.c b/drivers/phy/amlogic/phy-meson8b-usb2.c
index 03c061dd5f0d..2aad45c55494 100644
--- a/drivers/phy/amlogic/phy-meson8b-usb2.c
+++ b/drivers/phy/amlogic/phy-meson8b-usb2.c
@@ -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;
}

--
2.32.0

2021-08-17 04:19:53

by Anand Moon

[permalink] [raw]
Subject: [PATCHv3 2/6] ARM: dts: meson8b: odroidc1: Set usb power source to always on

Set P5V0 and vcc_3v3 power source to USB to always on
so that regulator should not enter in suspend state.

Cc: Martin Blumenstingl <[email protected]>
Signed-off-by: Anand Moon <[email protected]>
---
New patch in this series.
---
arch/arm/boot/dts/meson8b-odroidc1.dts | 2 ++
1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/meson8b-odroidc1.dts b/arch/arm/boot/dts/meson8b-odroidc1.dts
index 30ec6a7f20c7..5e5953152452 100644
--- a/arch/arm/boot/dts/meson8b-odroidc1.dts
+++ b/arch/arm/boot/dts/meson8b-odroidc1.dts
@@ -65,6 +65,7 @@ p5v0: regulator-p5v0 {
regulator-name = "P5V0";
regulator-min-microvolt = <5000000>;
regulator-max-microvolt = <5000000>;
+ regulator-always-on;
};

tflash_vdd: regulator-tflash_vdd {
@@ -136,6 +137,7 @@ vcc_3v3: regulator-vcc-3v3 {
regulator-name = "VCC3V3";
regulator-min-microvolt = <3300000>;
regulator-max-microvolt = <3300000>;
+ regulator-always-on;
vin-supply = <&p5v0>;
};

--
2.32.0

2021-08-17 04:20:47

by Anand Moon

[permalink] [raw]
Subject: [PATCHv3 6/6] phy: amlogic: meson8b-usb2: don't log an error on -EPROBE_DEFER

devm_phy_create can return -EPROBE_DEFER if the vbus-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]>
---
None.
---
drivers/phy/amlogic/phy-meson8b-usb2.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/amlogic/phy-meson8b-usb2.c b/drivers/phy/amlogic/phy-meson8b-usb2.c
index 2aad45c55494..cf10bed40528 100644
--- a/drivers/phy/amlogic/phy-meson8b-usb2.c
+++ b/drivers/phy/amlogic/phy-meson8b-usb2.c
@@ -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);
--
2.32.0

2021-08-17 10:32:54

by Vinod Koul

[permalink] [raw]
Subject: Re: [PATCHv3 5/6] phy: amlogic: meson8b-usb2: Power off the PHY by putting it into reset mode

On 17-08-21, 09:45, Anand Moon wrote:
> Power off the PHY by putting it into reset mode.

Applied, thanks

--
~Vinod

2021-08-17 10:35:18

by Vinod Koul

[permalink] [raw]
Subject: Re: [PATCHv3 6/6] phy: amlogic: meson8b-usb2: don't log an error on -EPROBE_DEFER

On 17-08-21, 09:45, Anand Moon wrote:
> devm_phy_create can return -EPROBE_DEFER if the vbus-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.

Applied, thanks

--
~Vinod

2021-08-28 15:56:07

by Martin Blumenstingl

[permalink] [raw]
Subject: Re: [PATCHv3 1/6] ARM: dts: meson8b: odroidc1: Add usb phy power node

Hi Anand,

sorry for the late reply
I have three very small comments below, apart from these, this is looking good!

On Tue, Aug 17, 2021 at 6:17 AM Anand Moon <[email protected]> wrote:
>
> Add missing usb phy power node for usb node fix below warning.
> P5V0 regulator supply input voltage range to USB host controller.
> As described in the C1+ schematics, GPIO GPIOAO_5 is used to
> enable input power to USB ports, set it to Active Low.
I would phrase this last sentence as:
"enable USB VBUS on the Micro-USB port using an active high signal"
My idea here is to 1) clarify that it's about enabling USB VBUS only
on the Micro-USB port and 2) use "active high" like the changes inside
the patch itself

> [ 1.260772] dwc2 c90c0000.usb: Looking up vbus-supply from device tree
> [ 1.260784] dwc2 c90c0000.usb: Looking up vbus-supply property in
> mode /soc/usb@c90c0000 failed
>
> Fixes: 2eb79a4d15ff (ARM: dts: meson: enabling the USB Host
> controller on Odroid-C1/C1+ board)
>
please drop this empty line

[...]
> @@ -119,7 +136,6 @@ vcc_3v3: regulator-vcc-3v3 {
> regulator-name = "VCC3V3";
> regulator-min-microvolt = <3300000>;
> regulator-max-microvolt = <3300000>;
> -
I don't think that we should make any VCC3V3 regulator changes in this patch
so please keep this empty line as-is.


Best regards,
Martin

2021-08-28 15:58:02

by Martin Blumenstingl

[permalink] [raw]
Subject: Re: [PATCHv3 2/6] ARM: dts: meson8b: odroidc1: Set usb power source to always on

Hi Anand,

On Tue, Aug 17, 2021 at 6:17 AM Anand Moon <[email protected]> wrote:
>
> Set P5V0 and vcc_3v3 power source to USB to always on
> so that regulator should not enter in suspend state.
Neither of these two regulators can be controlled by Linux in any way
(there's no GPIO to turn them off, no PWM signal to change the output
voltage, etc.).
So can you please explain what this patch changes from a functional perspective?


Best regards,
Martin

2021-08-28 16:01:50

by Martin Blumenstingl

[permalink] [raw]
Subject: Re: [PATCHv3 3/6] arm64: dts: amlogic: odroidc2: Fix the chip enable signal for usb power

Hi Anand,

On Tue, Aug 17, 2021 at 6:17 AM Anand Moon <[email protected]> wrote:
>
> Fix the chip enable signal changing from Active High to Active Low
> to enable input power to USB power. Also updated signal name as per
> the schematics.
according to [0] (page 12) Odroid-C2 uses an "RT9715EGB" for USB OTG power
The datasheet for that power switch [1] mentions on page page 1 that
the "E" stands for "E : 1.1A/Active High"

Can you please elaborate how you have tested this to confirm that
active low is the correct polarity?


Best regards,
Martin


[0] https://dn.odroid.com/S905/Schematic/odroid-c2_rev0.1_20150930.pdf
[1] https://www.richtek.com/assets/product_file/RT9715/DS9715-03.pdf

2021-08-28 16:09:07

by Martin Blumenstingl

[permalink] [raw]
Subject: Re: [PATCHv3 4/6] arm64: dts: amlogic: odroidc2: use vbus-supply for power source for usb nodes

Hi Anand,

On Tue, Aug 17, 2021 at 6:18 AM Anand Moon <[email protected]> wrote:
>
> Use vbus-supply instead of phy-supply as power source for dwc2
> nodes. Drop vbus-supply for usb0 node, as it will handle later.
This is more of a question than a review comment:
Do you think that the USB power setup on Odroid-C1 and Odroid-C2 is
different or the same?
On Odroid-C1 we know that only the PWREN signal which goes to the
Micro-USB connector is configurable while VBUS for the USB host ports
is always enabled.

> Fixes: e841ec956e53 ("ARM64: dts: meson-gxbb-odroidc2:
> fix usb1 power supply")
>
most Linux commits which I have seen don't use a blank line here
Also I think it's recommended not to break the "Fixes" tag into
multiple lines, even if the text is long
see [0] for example


Best regards,
Martin


[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=425bec0032f59eeee12520085cd054fac09cc66e

2021-08-30 07:47:45

by Neil Armstrong

[permalink] [raw]
Subject: Re: [PATCHv3 1/6] ARM: dts: meson8b: odroidc1: Add usb phy power node

Hi,

On 17/08/2021 06:15, Anand Moon wrote:
> Add missing usb phy power node for usb node fix below warning.
> P5V0 regulator supply input voltage range to USB host controller.
> As described in the C1+ schematics, GPIO GPIOAO_5 is used to
> enable input power to USB ports, set it to Active Low.
>
> [ 1.260772] dwc2 c90c0000.usb: Looking up vbus-supply from device tree
> [ 1.260784] dwc2 c90c0000.usb: Looking up vbus-supply property in
> mode /soc/usb@c90c0000 failed

First of all, DT is not here to fix boot message.

Secondly, if the vbus-supply is optional, the message should be removed from
the driver/regulator core instead.

Finally, I looked at the Odroid-C1 schematics and the GPIOAO.BIT5 is an input
to the S805, and the PWREN signal is controlled by the USB Hub so this regulator
should not be added at all.

Neil

>
> 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]>
> ---
> V2 > changes gpio polarity ACTIVE_HIGH to ACTIVE_LOW.
> Fix the source power from phy-supply to vbus-supply.
>
> [1] https://lore.kernel.org/linux-devicetree/[email protected]/
>
> V1 > Fix the Input GPIO polarity from HIGH to LOW.
>
> previous version
> [0] https://patchwork.kernel.org/project/linux-amlogic/patch/[email protected]
>
> changes fix the vbus-suppy to phy-supply, drop enable usb0
>
> 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 | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/meson8b-odroidc1.dts b/arch/arm/boot/dts/meson8b-odroidc1.dts
> index c440ef94e082..30ec6a7f20c7 100644
> --- a/arch/arm/boot/dts/meson8b-odroidc1.dts
> +++ b/arch/arm/boot/dts/meson8b-odroidc1.dts
> @@ -32,6 +32,23 @@ 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_OTG_PWR";
> +
> + regulator-min-microvolt = <5000000>;
> + regulator-max-microvolt = <5000000>;
> + /*
> + * signal name from schematics: USB_POWER - P5V0
> + */
> + vin-supply = <&p5v0>;
> + /*
> + * signal name from schematics: PWREN - GPIOAO.BIT5
> + */
> + gpios = <&gpio_ao GPIOAO_5 GPIO_ACTIVE_LOW>;
> + };
> +
> leds {
> compatible = "gpio-leds";
> blue {
> @@ -119,7 +136,6 @@ vcc_3v3: regulator-vcc-3v3 {
> regulator-name = "VCC3V3";
> regulator-min-microvolt = <3300000>;
> regulator-max-microvolt = <3300000>;
> -
> vin-supply = <&p5v0>;

As martin said, only a single functional change per patch please, this line removal should go in another patch.

> };
>
> @@ -382,4 +398,5 @@ &usb1_phy {
>
> &usb1 {
> status = "okay";
> + vbus-supply = <&usb_pwr_en>;
> };
>

2021-08-30 19:39:46

by Martin Blumenstingl

[permalink] [raw]
Subject: Re: [PATCHv3 1/6] ARM: dts: meson8b: odroidc1: Add usb phy power node

Hi Neil,

On Mon, Aug 30, 2021 at 9:45 AM Neil Armstrong <[email protected]> wrote:
>
> Hi,
>
> On 17/08/2021 06:15, Anand Moon wrote:
> > Add missing usb phy power node for usb node fix below warning.
> > P5V0 regulator supply input voltage range to USB host controller.
> > As described in the C1+ schematics, GPIO GPIOAO_5 is used to
> > enable input power to USB ports, set it to Active Low.
> >
> > [ 1.260772] dwc2 c90c0000.usb: Looking up vbus-supply from device tree
> > [ 1.260784] dwc2 c90c0000.usb: Looking up vbus-supply property in
> > mode /soc/usb@c90c0000 failed
>
> First of all, DT is not here to fix boot message.
Anand mentioned elsewhere that this is a debug/info message

> Finally, I looked at the Odroid-C1 schematics and the GPIOAO.BIT5 is an input
> to the S805, and the PWREN signal is controlled by the USB Hub so this regulator
> should not be added at all.
I think there's a misunderstanding because there's two PWREN signals
with different meanings.
The PWREN signal for the USB host ports is hard-wired and not
connected to the SoC at all.
The PWREN signal for the Micro-USB port (which Anand is adding here)
is controlled by GPIOAO_5. odroid-c1+_rev0.4_20150615.pdf [0] shows it
as an input to "USB_OTG" on page 1. "USB_OTG" consists of a power
switch and the connector itself as shown on page 28.

Personally I think that the change from Anand itself is good.
If you feel otherwise then please speak up.
As I pointed out three smaller changes I am hoping that Anand will
re-send the updated patch anyways. At that point he can also add the
changes from your feedback.


Best regards,
Martin


[0] https://dn.odroid.com/S805/Schematics/odroid-c1+_rev0.4_20150615.pdf

2021-08-31 14:51:07

by Neil Armstrong

[permalink] [raw]
Subject: Re: [PATCHv3 1/6] ARM: dts: meson8b: odroidc1: Add usb phy power node

Hi,

On 30/08/2021 21:37, Martin Blumenstingl wrote:
> Hi Neil,
>
> On Mon, Aug 30, 2021 at 9:45 AM Neil Armstrong <[email protected]> wrote:
>>
>> Hi,
>>
>> On 17/08/2021 06:15, Anand Moon wrote:
>>> Add missing usb phy power node for usb node fix below warning.
>>> P5V0 regulator supply input voltage range to USB host controller.
>>> As described in the C1+ schematics, GPIO GPIOAO_5 is used to
>>> enable input power to USB ports, set it to Active Low.
>>>
>>> [ 1.260772] dwc2 c90c0000.usb: Looking up vbus-supply from device tree
>>> [ 1.260784] dwc2 c90c0000.usb: Looking up vbus-supply property in
>>> mode /soc/usb@c90c0000 failed
>>
>> First of all, DT is not here to fix boot message.
> Anand mentioned elsewhere that this is a debug/info message
>
>> Finally, I looked at the Odroid-C1 schematics and the GPIOAO.BIT5 is an input
>> to the S805, and the PWREN signal is controlled by the USB Hub so this regulator
>> should not be added at all.
> I think there's a misunderstanding because there's two PWREN signals
> with different meanings.
> The PWREN signal for the USB host ports is hard-wired and not
> connected to the SoC at all.
> The PWREN signal for the Micro-USB port (which Anand is adding here)
> is controlled by GPIOAO_5. odroid-c1+_rev0.4_20150615.pdf [0] shows it
> as an input to "USB_OTG" on page 1. "USB_OTG" consists of a power
> switch and the connector itself as shown on page 28.
>
> Personally I think that the change from Anand itself is good.
> If you feel otherwise then please speak up.

Ok thanks for the clarification, then the change is ok, but not the commit message.

>> Add missing usb phy power node for usb node fix below warning.

is not a good reason for a DT change. A proper reason should be added.

And the commit message doesn't specify the change is for the Micro-USB port,
this should be clarified.

Neil

> As I pointed out three smaller changes I am hoping that Anand will
> re-send the updated patch anyways. At that point he can also add the
> changes from your feedback.
>
>
> Best regards,
> Martin
>
>
> [0] https://dn.odroid.com/S805/Schematics/odroid-c1+_rev0.4_20150615.pdf
>

2021-08-31 20:49:24

by Anand Moon

[permalink] [raw]
Subject: Re: [PATCHv3 1/6] ARM: dts: meson8b: odroidc1: Add usb phy power node

Hi Neil / Martin,

Thanks for your review comments.

On Tue, 31 Aug 2021 at 20:20, Neil Armstrong <[email protected]> wrote:
>
> Hi,
>
> On 30/08/2021 21:37, Martin Blumenstingl wrote:
> > Hi Neil,
> >
> > On Mon, Aug 30, 2021 at 9:45 AM Neil Armstrong <[email protected]> wrote:
> >>
> >> Hi,
> >>
> >> On 17/08/2021 06:15, Anand Moon wrote:
> >>> Add missing usb phy power node for usb node fix below warning.
> >>> P5V0 regulator supply input voltage range to USB host controller.
> >>> As described in the C1+ schematics, GPIO GPIOAO_5 is used to
> >>> enable input power to USB ports, set it to Active Low.
> >>>
> >>> [ 1.260772] dwc2 c90c0000.usb: Looking up vbus-supply from device tree
> >>> [ 1.260784] dwc2 c90c0000.usb: Looking up vbus-supply property in
> >>> mode /soc/usb@c90c0000 failed
> >>
> >> First of all, DT is not here to fix boot message.
> > Anand mentioned elsewhere that this is a debug/info message
> >
> >> Finally, I looked at the Odroid-C1 schematics and the GPIOAO.BIT5 is an input
> >> to the S805, and the PWREN signal is controlled by the USB Hub so this regulator
> >> should not be added at all.
> > I think there's a misunderstanding because there's two PWREN signals
> > with different meanings.
> > The PWREN signal for the USB host ports is hard-wired and not
> > connected to the SoC at all.
> > The PWREN signal for the Micro-USB port (which Anand is adding here)
> > is controlled by GPIOAO_5. odroid-c1+_rev0.4_20150615.pdf [0] shows it
> > as an input to "USB_OTG" on page 1. "USB_OTG" consists of a power
> > switch and the connector itself as shown on page 28.
> >
> > Personally I think that the change from Anand itself is good.
> > If you feel otherwise then please speak up.
>
> Ok thanks for the clarification, then the change is ok, but not the commit message.
>
> >> Add missing usb phy power node for usb node fix below warning.
>
> is not a good reason for a DT change. A proper reason should be added.
>
> And the commit message doesn't specify the change is for the Micro-USB port,
> this should be clarified.
>
> Neil
>
> > As I pointed out three smaller changes I am hoping that Anand will
> > re-send the updated patch anyways. At that point he can also add the
> > changes from your feedback.
> >
Ok I will try to address your feedback in the next version.

After enabling CONFIG_REGULATOR_DEBUG, with this patch applied
I still not getting the USB regulator to enable.
Do you see different output at your end?

On Odroid C1+
[ 5.737571] reg-fixed-voltage regulator-usb-pwr-en: GPIO lookup for
consumer (null)
[ 5.737630] reg-fixed-voltage regulator-usb-pwr-en: using device
tree for GPIO lookup
[ 5.737711] of_get_named_gpiod_flags: can't parse 'gpios' property
of node '/regulator-usb-pwr-en[0]'
[ 5.737906] of_get_named_gpiod_flags: parsed 'gpio' property of
node '/regulator-usb-pwr-en[0]' - status (0)
[ 5.738209] gpio_stub_drv gpiochip0: Persistence not supported for GPIO 5
[ 5.738490] USB_OTG_PWR: 5000 mV, disabled
[ 5.740313] reg-fixed-voltage regulator-usb-pwr-en: Looking up
vin-supply from device tree
[ 5.740394] USB_OTG_PWR: supplied by P5V0
[ 5.741235] reg-fixed-voltage regulator-usb-pwr-en: USB_OTG_PWR
supplying 5000000uV

Odroid N2.
[ 3.047813] reg-fixed-voltage regulator-hub_5v: HUB_5V supplying 5000000uV
[ 3.049282] reg-fixed-voltage regulator-usb_pwr_en: GPIO lookup for
consumer (null)
[ 3.049305] reg-fixed-voltage regulator-usb_pwr_en: using device
tree for GPIO lookup
[ 3.049370] of_get_named_gpiod_flags: can't parse 'gpios' property
of node '/regulator-usb_pwr_en[0]'
[ 3.049500] of_get_named_gpiod_flags: parsed 'gpio' property of
node '/regulator-usb_pwr_en[0]' - status (0)
[ 3.049622] gpio_stub_drv gpiochip0: Persistence not supported for GPIO 22
[ 3.049759] USB_PWR_EN: 5000 mV, disabled
[ 3.051257] reg-fixed-voltage regulator-usb_pwr_en: Looking up
vin-supply from device tree
[ 3.051320] USB_PWR_EN: supplied by 5V

Thanks
-Anand



> >
> > Best regards,
> > Martin
> >
> >
> > [0] https://dn.odroid.com/S805/Schematics/odroid-c1+_rev0.4_20150615.pdf
> >
>

2021-09-21 04:09:03

by Martin Blumenstingl

[permalink] [raw]
Subject: Re: [PATCHv3 1/6] ARM: dts: meson8b: odroidc1: Add usb phy power node

Hi Anand,

On Tue, Aug 31, 2021 at 10:48 PM Anand Moon <[email protected]> wrote:
[...]
> After enabling CONFIG_REGULATOR_DEBUG, with this patch applied
> I still not getting the USB regulator to enable.
> Do you see different output at your end?
I don't have much time for testing and debugging currently but I'll
put it on my TODO-list
Until either of us has found the issue I suggest not merging this patch.


Best regards,
Martin

2021-09-25 17:43:44

by Anand Moon

[permalink] [raw]
Subject: Re: [PATCHv3 1/6] ARM: dts: meson8b: odroidc1: Add usb phy power node

Hi Martin,

On Tue, 21 Sept 2021 at 00:56, Martin Blumenstingl
<[email protected]> wrote:
>
> Hi Anand,
>
> On Tue, Aug 31, 2021 at 10:48 PM Anand Moon <[email protected]> wrote:
> [...]
> > After enabling CONFIG_REGULATOR_DEBUG, with this patch applied
> > I still not getting the USB regulator to enable.
> > Do you see different output at your end?
> I don't have much time for testing and debugging currently but I'll
> put it on my TODO-list
> Until either of us has found the issue I suggest not merging this patch.
>
Ok no problem.

Basically, I have just roughly gone through the architecture of
Amlogic's OTG framework.
Below is the global configuration registers for DWC2 OTG framer work

[0] https://github.com/hardkernel/linux/blob/odroidc-3.10.y/drivers/amlogic/usb/dwc_otg/310/dwc_otg_regs.h#L66-L151

Within some configurations, it helps tune the power for vbus and interrupt
For example
[1] https://github.com/hardkernel/linux/blob/odroidc-3.10.y/drivers/amlogic/usb/dwc_otg/310/dwc_otg_attr.c#L666-L717

Amlogic basically used the mode parameter to external tune the DWC2 driver
it could help more fine-tuning the driver.

[2] https://github.com/hardkernel/linux/blob/odroidc-3.10.y/drivers/amlogic/usb/dwc_otg/310/dwc_otg_driver.c#L1461-L1703

I feel we need to identify many more PHY tuning parameters to make the
USB work correctly.






Thanks
-Anand

>
> Best regards,
> Martin