| Message ID | 20210817041548.1276-2-linux.amoon@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PATCHv3,1/6] ARM: dts: meson8b: odroidc1: Add usb phy power node | expand |
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 <linux.amoon@gmail.com> 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
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 <martin.blumenstingl@googlemail.com> > Signed-off-by: Anand Moon <linux.amoon@gmail.com> > --- > 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/20210716103651.1455-2-linux.amoon@gmail.com/ > > V1 > Fix the Input GPIO polarity from HIGH to LOW. > > previous version > [0] https://patchwork.kernel.org/project/linux-amlogic/patch/20190113181808.5768-1-linux.amoon@gmail.com > > 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>; > }; >
Hi Neil, On Mon, Aug 30, 2021 at 9:45 AM Neil Armstrong <narmstrong@baylibre.com> 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
Hi, On 30/08/2021 21:37, Martin Blumenstingl wrote: > Hi Neil, > > On Mon, Aug 30, 2021 at 9:45 AM Neil Armstrong <narmstrong@baylibre.com> 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 >
Hi Neil / Martin, Thanks for your review comments. On Tue, 31 Aug 2021 at 20:20, Neil Armstrong <narmstrong@baylibre.com> wrote: > > Hi, > > On 30/08/2021 21:37, Martin Blumenstingl wrote: > > Hi Neil, > > > > On Mon, Aug 30, 2021 at 9:45 AM Neil Armstrong <narmstrong@baylibre.com> 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 > > >
Hi Anand, On Tue, Aug 31, 2021 at 10:48 PM Anand Moon <linux.amoon@gmail.com> 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
Hi Martin, On Tue, 21 Sept 2021 at 00:56, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > > Hi Anand, > > On Tue, Aug 31, 2021 at 10:48 PM Anand Moon <linux.amoon@gmail.com> 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
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>; };
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 <martin.blumenstingl@googlemail.com> Signed-off-by: Anand Moon <linux.amoon@gmail.com> --- 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/20210716103651.1455-2-linux.amoon@gmail.com/ V1 > Fix the Input GPIO polarity from HIGH to LOW. previous version [0] https://patchwork.kernel.org/project/linux-amlogic/patch/20190113181808.5768-1-linux.amoon@gmail.com 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(-)