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

[Felipe Balbi]

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?

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?

> + * @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

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

-- 
balbi

Attachment: signature.asc
Description: PGP signature