Re: [PATCH] dwc3: gadget: Defer starting the gadget device until gadget is power on

[Baolin Wang]

Hi Felipe,

On 13 May 2016 at 18:40, Felipe Balbi <balbi@xxxxxxxxxx> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@xxxxxxxxxx> writes:
>> Currently on some platforms, the gadget device can be power off to
>> save power when the Vbus is off, which means no cable plugging in
>> now. In this situation we should defer starting the gadget until the
>> gadget device is power on by connecting host.
>
> okay, you need to be a looooooooooot more specific about this. From a
> basic look at the patch, there's no way I'll ever accept it, see below
>
>> Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxx>
>> ---
>>  drivers/usb/dwc3/core.c          |    5 +-
>>  drivers/usb/dwc3/core.h          |   14 ++++
>>  drivers/usb/dwc3/gadget.c        |  144 ++++++++++++++++++++++++++++++--------
>>  drivers/usb/dwc3/platform_data.h |    1 +
>>  4 files changed, 131 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 34277ce..825462a 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -109,7 +109,7 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc)
>>   * dwc3_soft_reset - Issue soft reset
>>   * @dwc: Pointer to our controller context structure
>>   */
>> -static int dwc3_soft_reset(struct dwc3 *dwc)
>> +int dwc3_soft_reset(struct dwc3 *dwc)
>
> this *CANNOT* and *WILL* not be exposed to anything other than
> core.c. We don't want anybody else resetting dwc3 willy-nilly.
>
>> @@ -253,7 +253,7 @@ static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned length)
>>   *
>>   * Returns 0 on success otherwise negative errno.
>>   */
>> -static int dwc3_event_buffers_setup(struct dwc3 *dwc)
>> +int dwc3_event_buffers_setup(struct dwc3 *dwc)
>
> likewise
>
>> @@ -948,6 +948,7 @@ static int dwc3_probe(struct platform_device *pdev)
>>
>>               dwc->hsphy_interface = pdata->hsphy_interface;
>>               fladj = pdata->fladj_value;
>> +             dwc->can_save_power = pdata->can_save_power;
>
> sounds like a pointless flag IMHO.
>
>>       }
>>
>>       dwc->lpm_nyet_threshold = lpm_nyet_threshold;
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 6254b2f..dada5c6 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -734,6 +734,9 @@ struct dwc3_scratchpad_array {
>>   *   1       - -3.5dB de-emphasis
>>   *   2       - No de-emphasis
>>   *   3       - Reserved
>> + * @can_save_power: set if the gadget will power off when no cable plug in.
>
> nope
>
>> + * @need_restart: set if we need to restart the gadget.
>
> why does it need restart? Why is dwc3 powered off? Who powers it off?

Because when the dwc3 Vbus is off (no cable pluging in now),
especially for some mobile device, the system need to power off the
dwc3 to save power in this situation.

>
> This looks like a *really* bad power management implementation. Do you
> have hibernation enabled? Do you have Clock gating enabled? Which dwc3
> version are you using? How was it configured?

This is not hibernation, we want to power off the dwc3 to save power
when no cable plugging in. Yes, we have clock gating, at this
situation we will disable the clock and shutdown the phy to save
power. For mobile device, most time no cable plugging in, so we need
to think about the power consuming. How do you think this requirement?
Thanks.

>
>> + * @cable_connected: set if one usb cable is plugging in.
>
> not necessary, we already infer that from RUN/STOP and reset interrupt.
>
>> @@ -876,6 +879,9 @@ struct dwc3 {
>>
>>       unsigned                tx_de_emphasis_quirk:1;
>>       unsigned                tx_de_emphasis:2;
>> +     unsigned                can_save_power:1;
>> +     unsigned                need_restart:1;
>> +     unsigned                cable_connected:1;
>>  };
>>
>>  /* -------------------------------------------------------------------------- */
>> @@ -1026,6 +1032,8 @@ struct dwc3_gadget_ep_cmd_params {
>>  /* prototypes */
>>  void dwc3_set_mode(struct dwc3 *dwc, u32 mode);
>>  int dwc3_gadget_resize_tx_fifos(struct dwc3 *dwc);
>> +int dwc3_soft_reset(struct dwc3 *dwc);
>> +int dwc3_event_buffers_setup(struct dwc3 *dwc);
>
> makes me cringe
>
>> @@ -1052,6 +1060,8 @@ int dwc3_gadget_set_link_state(struct dwc3 *dwc, enum dwc3_link_state state);
>>  int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
>>               unsigned cmd, struct dwc3_gadget_ep_cmd_params *params);
>>  int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned cmd, u32 param);
>> +void dwc3_gadget_connect(struct dwc3 *dwc);
>> +void dwc3_gadget_disconnect(struct dwc3 *dwc);
>
> hell no
>
>>  #else
>>  static inline int dwc3_gadget_init(struct dwc3 *dwc)
>>  { return 0; }
>> @@ -1071,6 +1081,10 @@ static inline int dwc3_send_gadget_ep_cmd(struct dwc3 *dwc, unsigned ep,
>>  static inline int dwc3_send_gadget_generic_command(struct dwc3 *dwc,
>>               int cmd, u32 param)
>>  { return 0; }
>> +static inline void dwc3_gadget_connect(struct dwc3 *dwc)
>> +{ }
>> +static inline void dwc3_gadget_disconnect(struct dwc3 *dwc)
>> +{ }
>>  #endif
>>
>>  /* power management interface */
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 8e4a1b1..90805f9 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -1512,13 +1512,75 @@ static int dwc3_gadget_set_selfpowered(struct usb_gadget *g,
>>       return 0;
>>  }
>>
>> +void dwc3_gadget_connect(struct dwc3 *dwc)
>> +{
>> +     unsigned long flags;
>> +
>> +     spin_lock_irqsave(&dwc->lock, flags);
>> +     dwc->cable_connected = true;
>> +     spin_unlock_irqrestore(&dwc->lock, flags);
>> +}
>> +
>> +void dwc3_gadget_disconnect(struct dwc3 *dwc)
>> +{
>> +     unsigned long flags;
>> +
>> +     spin_lock_irqsave(&dwc->lock, flags);
>> +     dwc->cable_connected = false;
>> +     spin_unlock_irqrestore(&dwc->lock, flags);
>> +}
>> +
>
> nope
>
>> +static bool dwc3_gadget_is_connected(struct dwc3 *dwc)
>> +{
>> +     /*
>> +      * If the gadget is always power on, then no need to check if the
>> +      * cable is plugin or not.
>> +      */
>> +     if (!dwc->can_save_power)
>> +             return true;
>
> this is wrong! Really, really wrong! If cable is detached, you're saying
> it's always attached. No. Don't do it. Don't lie to the driver.
>
>> +static int __dwc3_gadget_start(struct dwc3 *dwc);
>> +
>>  static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
>>  {
>>       u32                     reg;
>>       u32                     timeout = 500;
>> +     int                     ret;
>> +
>> +     if (!dwc3_gadget_is_connected(dwc) || !dwc->gadget_driver)
>> +             return 0;
>>
>>       reg = dwc3_readl(dwc->regs, DWC3_DCTL);
>>       if (is_on) {
>> +             if (dwc->need_restart) {
>> +                     /*
>> +                      * We need to reset the device firstly when the device
>> +                      * is power on.
>> +                      */
>> +                     ret = dwc3_soft_reset(dwc);
>> +                     if (ret)
>> +                             return ret;
>> +
>> +                     /*
>> +                      * After resetting the device, it need to re-setup the
>> +                      * event buffer.
>> +                      */
>> +                     ret = dwc3_event_buffers_setup(dwc);
>> +                     if (ret) {
>> +                             dev_err(dwc->dev,
>> +                                     "failed to setup event buffers\n");
>> +                             return ret;
>> +                     }
>> +
>> +                     /* Start the gadget */
>> +                     ret = __dwc3_gadget_start(dwc);
>> +                     if (ret)
>> +                             return ret;
>> +             }
>
> no way
>
>> +static int dwc3_gadget_start(struct usb_gadget *g,
>> +             struct usb_gadget_driver *driver)
>> +{
>> +     struct dwc3             *dwc = gadget_to_dwc(g);
>> +     unsigned long           flags;
>> +     int                     ret = 0;
>> +     int                     irq;
>> +
>> +     irq = platform_get_irq(to_platform_device(dwc->dev), 0);
>> +     ret = request_threaded_irq(irq, dwc3_interrupt, dwc3_thread_interrupt,
>> +                     IRQF_SHARED, "dwc3", dwc);
>> +     if (ret) {
>> +             dev_err(dwc->dev, "failed to request irq #%d --> %d\n",
>> +                             irq, ret);
>> +             goto err0;
>> +     }
>> +
>> +     spin_lock_irqsave(&dwc->lock, flags);
>> +
>> +     if (dwc->gadget_driver) {
>> +             dev_err(dwc->dev, "%s is already bound to %s\n",
>> +                             dwc->gadget.name,
>> +                             dwc->gadget_driver->driver.name);
>> +             ret = -EBUSY;
>> +             goto err1;
>> +     }
>> +
>> +     dwc->gadget_driver      = driver;
>> +
>> +     /*
>> +      * If the gadget can be power off when there is no cable plug in, we
>> +      * need to check if the device power is on or not. If not, we should
>> +      * not access the device registers.
>> +      */
>> +     if (!dwc3_gadget_is_connected(dwc)) {
>> +             dwc->need_restart = 1;
>> +             spin_unlock_irqrestore(&dwc->lock, flags);
>> +             return 0;
>> +     }
>> +
>> +     ret = __dwc3_gadget_start(dwc);
>> +     if (ret)
>> +             goto err2;
>> +
>
> this is similar to a patch I wrote to improve system suspend/resume (not
> runtime). See:
>
> https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=dwc3-fix-suspend&id=d1c2d86ef61b8f7ac793036aee9d501fa44d9b8a
>
> https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=dwc3-fix-suspend&id=b6dc23f16389ee8803aba6a4c0265aac547f9c57
>
> https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=dwc3-fix-suspend&id=7c38518f5ea748ecccce651d899cf7714dfb653f
>
> I haven't sent because I didn't finish testing yet

OK.

>
> Anyway, which platform are you dealing with? Why is dwc3 off while VBUS
> is off? How do you handle host mode?

On Spreadtrum platform, for thinking about some mobile devices with
strict power management. This is just for gadget mode, we don't power
off the dwc3 when it is host mode. Thanks.

>
> --
> balbi



-- 
Baolin.wang
Best Regards