[BUG]odroid-c2 does not hotplug usb-devices

[Anand Moon]

Hi Martin,

On Sun, 20 Dec 2020 at 20:04, Martin Blumenstingl
<martin.blumenstingl at googlemail.com> wrote:
>
> Hi Anand,
>
> On Sun, Dec 20, 2020 at 2:46 PM Anand Moon <linux.amoon at gmail.com> wrote:
> [...]
> > > On Sat, Dec 19, 2020 at 8:53 PM Anand Moon <linux.amoon at 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.*
> I am not sure if that's really the same issue as described by Otto
>
OK.
> [...]
> > > > amoon at 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 at 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 at 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;
> (assuming that your comment relates to the of_reset_control_get_shared
> call below)
> I checked rockchip_usb_phy_action and it's not calling reset_control_put either
> this looks strange to me and I would say that there's a memory leak.
>
Ok, I will try to check this, my approach was just to get the reset code
to get trigger, but is seem that it is not getting triggered,

> [...]
> > > >  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.
> are you sure that your patch fixes USB hotplug for you?
>
> the new reset callback is a no-op because it's not called
> the code below (of_reset_control_get_shared) only fetches the same
> reset line that we are already using
> so I am not sure what changed behavior you are seeing - can you please
> explain this in more detail?

I am looking into the phy-meson-gxl-usb2.c and I will try to model these changes
and see if it works on my boards.

Thanks
-Anand

>
> > > > @@ -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.
> indeed, the USB_OTG reset is shared
>
> > Odroid C2 reset controller have additional RESET_USB_DDR_[0,3] reset,
> > but Odroid C1 dose not have this feature.
> have you found where the patched Amlogic kernel/u-boot use these reset lines?
> if they need to be managed by us then we probably have a bug, because
> we're not currently using these anywhere
>
> > Next time I will try to do more testing on both the devices C1/C2.
> > before sending some code for review or testing.
> I am happy about everyone who helps fixing the USB hotplug issue!
> it's a tricky issue where I don't even know if the USB PHY, dwc2
> controller or something else is the culprit
> so any help is appreciated :)
>
Ok,
>
> Best regards,
> Martin