Hi, Baolin Wang <baolin.w...@linaro.org> 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.w...@linaro.org>
> ---
> 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
signature.asc
Description: PGP signature
