Hi Martin,
Thanks for testing and sharing your view points.
On Sun, 20 Dec 2020 at 04:01, Martin Blumenstingl
<martin.blumensti...@googlemail.com> wrote:
>
> Hi Anand,
>
> On Sat, Dec 19, 2020 at 8:53 PM Anand Moon <linux.am...@gmail.com> wrote:
> [...]
> > I was also looking into this issue so I made some changes in the
> > phy driver to resolve the issue. Plz share your thoughts on the changes
> > below.
> first I have some questions :-)
> 1. do you see the same problem that Otto sees? this means: a) USB
> hotplug works as long as at least one device is plugged in at boot b)
> (if I understand Otto correctly then) it breaks once all USB devices
> have been removed
On C1/C2 issue is when single usb hdd is connected to board it will
not get detected.
unless you connect another usb keyboard or wireless dongle to the board.
*USB hot plug only works if two ore more usb devices are connected.*
> 2. does the mainline u-boot patch mentioned by Otto fix the problem
> for you? according to him it fixes the problem and he did not have to
> modify the USB PHY driver
>
I have not check this out, I will verify this later.
> > amoon@ThinkPad-T440s:~/mainline/linux-aml-5.y-devel$ git diff
> > diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> > b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> > index 7c029f552a23..363dd2ac17e6 100644
> > --- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> > +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> > @@ -20,6 +20,7 @@ usb0_phy: phy@c0000000 {
> > #phy-cells = <0>;
> > reg = <0x0 0xc0000000 0x0 0x20>;
> > resets = <&reset RESET_USB_OTG>;
> > + reset-names = "phy-reset";
> > clocks = <&clkc CLKID_USB>, <&clkc CLKID_USB0>;
> > clock-names = "usb_general", "usb";
> > status = "disabled";
> > @@ -30,6 +31,7 @@ usb1_phy: phy@c0000020 {
> > #phy-cells = <0>;
> > reg = <0x0 0xc0000020 0x0 0x20>;
> > resets = <&reset RESET_USB_OTG>;
> > + reset-names = "phy-reset";
> > clocks = <&clkc CLKID_USB>, <&clkc CLKID_USB1>;
> > clock-names = "usb_general", "usb";
> > status = "disabled";
> I don't see why the above two changes are needed
> see my comment about of_reset_control_get_shared below
>
Yep I borrowed this logic from rockchip usb phy controller.
but I missed the action on reset.
err = devm_add_action_or_reset(base->dev, rockchip_usb_phy_action, rk_phy);
if (err)
return err;
> > diff --git a/drivers/phy/amlogic/phy-meson8b-usb2.c
> > b/drivers/phy/amlogic/phy-meson8b-usb2.c
> > index 03c061dd5f0d..31523becc878 100644
> > --- a/drivers/phy/amlogic/phy-meson8b-usb2.c
> > +++ b/drivers/phy/amlogic/phy-meson8b-usb2.c
> > @@ -143,14 +143,6 @@ static int phy_meson8b_usb2_power_on(struct phy *phy)
> > u32 reg;
> > int ret;
> >
> > - if (!IS_ERR_OR_NULL(priv->reset)) {
> > - ret = reset_control_reset(priv->reset);
> > - if (ret) {
> > - dev_err(&phy->dev, "Failed to trigger USB reset\n");
> > - return ret;
> > - }
> > - }
> > -
> > ret = clk_prepare_enable(priv->clk_usb_general);
> > if (ret) {
> > dev_err(&phy->dev, "Failed to enable USB general clock\n");
> > @@ -222,9 +214,23 @@ static int phy_meson8b_usb2_power_off(struct phy *phy)
> > return 0;
> > }
> >
> > +static int phy_meson8b_usb2_reset(struct phy *phy)
> > +{
> > + struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy);
> > +
> > + if (priv->reset) {
> > + reset_control_assert(priv->reset);
> > + udelay(10);
> > + reset_control_deassert(priv->reset);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static const struct phy_ops phy_meson8b_usb2_ops = {
> > .power_on = phy_meson8b_usb2_power_on,
> > .power_off = phy_meson8b_usb2_power_off,
> > + .reset = phy_meson8b_usb2_reset,
> > .owner = THIS_MODULE,
> > };
> I tested this on my Odroid-C1: phy_meson8b_usb2_reset is never called
> after checking the dwc2 code this is expected: only in one very
> specific case the dwc2 driver calls phy_reset
> can you please find out how phy_meson8b_usb2_reset is called in your kernel?
>
Yep, Its not getting called on my board, l might be missing some thing.
> > @@ -271,6 +277,10 @@ static int phy_meson8b_usb2_probe(struct
> > platform_device *pdev)
> > return -EINVAL;
> > }
> >
> > + priv->reset = of_reset_control_get_shared(pdev->dev.of_node,
> > "phy-reset");
> this causes a memory-leak upon driver removal
> also a few lines above we are already getting the reset line, so why
> is this needed?
I had a feeling that USB_OTG reset will be shared between both the phy
controller.
Odroid C2 reset controller have additional RESET_USB_DDR_[0,3] reset,
but Odroid C1 dose not have this feature.
Next time I will try to do more testing on both the devices C1/C2.
before sending some code for review or testing.
Best Regards
-Anand
>
>
> Best regards,
> Martin
