Re: [PATCH usb-next v10 3/8] usb: core: add a wrapper for the USB PHYs on the HCD

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,
On Sun, 2018-03-18 at 23:29 +0100, Martin Blumenstingl wrote:
> Hi Roger,
> 
> On Fri, Mar 16, 2018 at 3:32 PM, Roger Quadros <rogerq@xxxxxx> wrote:
> > +some TI folks
> >
> > Hi Martin,
> >
> > On 18/02/18 20:44, Martin Blumenstingl wrote:
> >> Many SoC platforms have separate devices for the USB PHY which are
> >> registered through the generic PHY framework. These PHYs have to be
> >> enabled to make the USB controller actually work. They also have to be
> >> disabled again on shutdown/suspend.
> >>
> >> Currently (at least) the following HCI platform drivers are using custom
> >> code to obtain all PHYs via devicetree for the roothub/controller and
> >> disable/enable them when required:
> >> - ehci-platform.c has ehci_platform_power_{on,off}
> >> - xhci-mtk.c has xhci_mtk_phy_{init,exit,power_on,power_off}
> >> - ohci-platform.c has ohci_platform_power_{on,off}
> >>
> >> With this new wrapper the USB PHYs can be specified directly in the
> >> USB controller's devicetree node (just like on the drivers listed
> >> above). This allows SoCs like the Amlogic Meson GXL family to operate
> >> correctly once this is wired up correctly. These SoCs use a dwc3
> >> controller and require all USB PHYs to be initialized (if one of the USB
> >> PHYs it not initialized then none of USB port works at all).
> >>
> >> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>
> >> Tested-by: Yixun Lan <yixun.lan@xxxxxxxxxxx>
> >> Cc: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> >> Cc: Chunfeng Yun <chunfeng.yun@xxxxxxxxxxxx>
> >
> > This patch is breaking low power cases on TI SoCs when USB is in host mode.
> > I'll explain why below.
> based on your explanation and reading the TI PHY drivers I am assuming
> that the affected SoCs are using the "phy-omap-usb2" driver
> 
[...]
> >> +
> >> +struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
> >> +{
> >> +     struct usb_phy_roothub *phy_roothub;
> >> +     struct usb_phy_roothub *roothub_entry;
> >> +     struct list_head *head;
> >> +     int i, num_phys, err;
> >> +
> >> +     num_phys = of_count_phandle_with_args(dev->of_node, "phys",
> >> +                                           "#phy-cells");
> >> +     if (num_phys <= 0)
> >> +             return NULL;
> >> +
> >> +     phy_roothub = usb_phy_roothub_alloc(dev);
> >> +     if (IS_ERR(phy_roothub))
> >> +             return phy_roothub;
> >> +
> >> +     for (i = 0; i < num_phys; i++) {
> >> +             err = usb_phy_roothub_add_phy(dev, i, &phy_roothub->list);
> >> +             if (err)
> >> +                     goto err_out;
> >> +     }
> >> +
> >> +     head = &phy_roothub->list;
> >> +
> >> +     list_for_each_entry(roothub_entry, head, list) {
> >> +             err = phy_init(roothub_entry->phy);
> >
> > The phy_init() function actually enables the PHY clocks.
> > It should be moved to the usb_phy_roothub_exit() routine just before calling phy_power_on().
> do you mean that phy_init should be moved to usb_phy_roothub_power_on
> (just before phy_power_on is called within usb_phy_roothub_power_on)?
> 
> an earlier version of my patch did exactly this, but it caused
> problems during a suspend/resume cycle on Mediatek devices
> Chunfeng Yun reported that issue here [0], quote from that mail for
> easier reading:
> "In order to keep link state on mt8173, we just power off all phys(not
> exit) when system enter suspend, then power on them again (needn't
> init, otherwise device will be disconnected) when system resume, this
> can avoid re-enumerating device."
> 
> >> +             if (err)
> >> +                     goto err_exit_phys;
> >> +     }
> >> +
> >> +     return phy_roothub;
> >> +
> >> +err_exit_phys:
> >> +     list_for_each_entry_continue_reverse(roothub_entry, head, list)
> >> +             phy_exit(roothub_entry->phy);
> >> +
> >> +err_out:
> >> +     return ERR_PTR(err);
> >> +}
> >> +EXPORT_SYMBOL_GPL(usb_phy_roothub_init);
> >> +
> >> +int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub)
> >> +{
> >> +     struct usb_phy_roothub *roothub_entry;
> >> +     struct list_head *head;
> >> +     int err, ret = 0;
> >> +
> >> +     if (!phy_roothub)
> >> +             return 0;
> >> +
> >> +     head = &phy_roothub->list;
> >> +
> >> +     list_for_each_entry(roothub_entry, head, list) {
> >> +             err = phy_exit(roothub_entry->phy);
> >> +             if (err)
> >> +                     ret = ret;
> >> +     }
> >
> > phy_exit() should be moved to usb_phy_roothub_poweroff() just after calling phy_power_off().
> if I understood Chunfeng Yun correctly this will require
> re-enumeration of the USB devices after a suspend/resume cycle on
> Mediatek SoCs
You are right
> 
> > With that there is nothing else being done here. Shouldn't we be doing the equivalent of
> > usb_phy_roothub_del_phy() and usb_phy_roothub_free() here?
> >
> >> +
> >> +     return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(usb_phy_roothub_exit);
> >> +
> >> +int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub)
> >> +{
> >> +     struct usb_phy_roothub *roothub_entry;
> >> +     struct list_head *head;
> >> +     int err;
> >> +
> >> +     if (!phy_roothub)
> >> +             return 0;
> >> +
> >> +     head = &phy_roothub->list;
> >> +
> >> +     list_for_each_entry(roothub_entry, head, list) {
> >> +             err = phy_power_on(roothub_entry->phy);
> >> +             if (err)
> >> +                     goto err_out;
> >> +     }
> >> +
> >> +     return 0;
> >> +
> >> +err_out:
> >> +     list_for_each_entry_continue_reverse(roothub_entry, head, list)
> >> +             phy_power_off(roothub_entry->phy);
> >> +
> >> +     return err;
> >> +}
> >> +EXPORT_SYMBOL_GPL(usb_phy_roothub_power_on);
> >> +
> >> +void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
> >> +{
> >> +     struct usb_phy_roothub *roothub_entry;
> >> +
> >> +     if (!phy_roothub)
> >> +             return;
> >> +
> >> +     list_for_each_entry_reverse(roothub_entry, &phy_roothub->list, list)
> >> +             phy_power_off(roothub_entry->phy);
> >
> > Not doing the phy_exit() here leaves the clocks enabled on our SoC and
> > we're no longer able to reach low power states on system suspend.
> I'm not sure where this problem should be solved:
> - set skip_phy_initialization in struct usb_hcd to 1 for the affected
> TI platforms
> - fix this in the usb_phy_roothub code
> - fix this in the PHY driver
> - somewhere else
> 
> >> +}
> >> +EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
> >> diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
> >> new file mode 100644
> >> index 000000000000..6fde59bfbff8
> >> --- /dev/null
> >> +++ b/drivers/usb/core/phy.h
> >> @@ -0,0 +1,7 @@
> >> +struct usb_phy_roothub;
> >> +
> >> +struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev);
> >> +int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
> >> +
> >> +int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);
> >> +void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub);
> >>
> >
> > The following patch fixes the issue for me. Let me know what you think and I can post it officially.
> Chunfeng: could you please test whether this patch breaks the
> suspend/resume cycle on your Mediatek platforms? see [0] where I used
> a similar approach with a much older version of the "initialize
> multiple PHYs per HCD" patch
If add Roger's one, it do disconnect plugged devices on MTK platforms,
due to re-initialize u2 phys when resume,as you mentioned above
> 
> > diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> > index 09b7c43..23232d3 100644
> > --- a/drivers/usb/core/phy.c
> > +++ b/drivers/usb/core/phy.c
> > @@ -59,8 +59,6 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
> >  struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
> >  {
> >         struct usb_phy_roothub *phy_roothub;
> > -       struct usb_phy_roothub *roothub_entry;
> > -       struct list_head *head;
> >         int i, num_phys, err;
> >
> >         num_phys = of_count_phandle_with_args(dev->of_node, "phys",
> > @@ -75,25 +73,10 @@ struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
> >         for (i = 0; i < num_phys; i++) {
> >                 err = usb_phy_roothub_add_phy(dev, i, &phy_roothub->list);
> >                 if (err)
> > -                       goto err_out;
> > -       }
> > -
> > -       head = &phy_roothub->list;
> > -
> > -       list_for_each_entry(roothub_entry, head, list) {
> > -               err = phy_init(roothub_entry->phy);
> > -               if (err)
> > -                       goto err_exit_phys;
> > +                       return ERR_PTR(err);
> >         }
> >
> >         return phy_roothub;
> > -
> > -err_exit_phys:
> > -       list_for_each_entry_continue_reverse(roothub_entry, head, list)
> > -               phy_exit(roothub_entry->phy);
> > -
> > -err_out:
> > -       return ERR_PTR(err);
> >  }
> >  EXPORT_SYMBOL_GPL(usb_phy_roothub_init);
> >
> > @@ -106,13 +89,8 @@ int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub)
> >         if (!phy_roothub)
> >                 return 0;
> >
> > -       head = &phy_roothub->list;
> > -
> > -       list_for_each_entry(roothub_entry, head, list) {
> > -               err = phy_exit(roothub_entry->phy);
> > -               if (err)
> > -                       ret = ret;
> > -       }
> > +       /* TODO: usb_phy_roothub_del_phy */
> > +       /* TODO: usb_phy_roothub_free */
> >
> >         return ret;
> >  }
> > @@ -130,16 +108,23 @@ int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub)
> >         head = &phy_roothub->list;
> >
> >         list_for_each_entry(roothub_entry, head, list) {
> > -               err = phy_power_on(roothub_entry->phy);
> > +               err = phy_init(roothub_entry->phy);
> >                 if (err)
> >                         goto err_out;
> > +               err = phy_power_on(roothub_entry->phy);
> > +               if (err) {
> > +                       phy_exit(roothub_entry->phy);
> > +                       goto err_out;
> > +               }
> >         }
> >
> >         return 0;
> >
> >  err_out:
> > -       list_for_each_entry_continue_reverse(roothub_entry, head, list)
> > +       list_for_each_entry_continue_reverse(roothub_entry, head, list) {
> >                 phy_power_off(roothub_entry->phy);
> > +               phy_exit(roothub_entry->phy);
> > +       }
> >
> >         return err;
> >  }
> > @@ -152,7 +137,9 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
> >         if (!phy_roothub)
> >                 return;
> >
> > -       list_for_each_entry_reverse(roothub_entry, &phy_roothub->list, list)
> > +       list_for_each_entry_reverse(roothub_entry, &phy_roothub->list, list) {
> >                 phy_power_off(roothub_entry->phy);
> > +               phy_exit(roothub_entry->phy);
> > +       }
> >  }
> >  EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
> >
> >
> > --
> > cheers,
> > -roger
> >
> > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> 
> 
> Regards
> Martin
> 
> 
> [0] http://lists.infradead.org/pipermail/linux-amlogic/2017-July/004374.html


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux