Hi,
Wesley Cheng wrote:
> The USB gadget/UDC driver can restrict the DWC3 controller speed using
> dwc3_gadget_set_speed(). Store this setting into a variable, in order for
> this setting to persist across controller resets due to runtime PM.
Why do we need to do this? DCFG should persist unless we overwrite it.
The current PM shouldn't update the current speed setting.
BR,
Thinh
> Signed-off-by: Wesley Cheng <[email protected]>
> ---
> drivers/usb/dwc3/core.h | 1 +
> drivers/usb/dwc3/gadget.c | 108 ++++++++++++++++++++------------------
> 2 files changed, 58 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 2f04b3e42bf1..390d3deef0ba 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1119,6 +1119,7 @@ struct dwc3 {
> u32 nr_scratch;
> u32 u1u2;
> u32 maximum_speed;
> + u32 gadget_max_speed;
>
> u32 ip;
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 6364429b2122..1a93b92a5e6f 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1941,6 +1941,61 @@ static void dwc3_stop_active_transfers(struct dwc3 *dwc)
> }
> }
>
> +static void __dwc3_gadget_set_speed(struct dwc3 *dwc)
> +{
> + u32 reg;
> +
> + reg = dwc3_readl(dwc->regs, DWC3_DCFG);
> + reg &= ~(DWC3_DCFG_SPEED_MASK);
> +
> + /*
> + * WORKAROUND: DWC3 revision < 2.20a have an issue
> + * which would cause metastability state on Run/Stop
> + * bit if we try to force the IP to USB2-only mode.
> + *
> + * Because of that, we cannot configure the IP to any
> + * speed other than the SuperSpeed
> + *
> + * Refers to:
> + *
> + * STAR#9000525659: Clock Domain Crossing on DCTL in
> + * USB 2.0 Mode
> + */
> + if (DWC3_VER_IS_PRIOR(DWC3, 220A) &&
> + !dwc->dis_metastability_quirk) {
> + reg |= DWC3_DCFG_SUPERSPEED;
> + } else {
> + switch (dwc->gadget_max_speed) {
> + case USB_SPEED_LOW:
> + reg |= DWC3_DCFG_LOWSPEED;
> + break;
> + case USB_SPEED_FULL:
> + reg |= DWC3_DCFG_FULLSPEED;
> + break;
> + case USB_SPEED_HIGH:
> + reg |= DWC3_DCFG_HIGHSPEED;
> + break;
> + case USB_SPEED_SUPER:
> + reg |= DWC3_DCFG_SUPERSPEED;
> + break;
> + case USB_SPEED_SUPER_PLUS:
> + if (DWC3_IP_IS(DWC3))
> + reg |= DWC3_DCFG_SUPERSPEED;
> + else
> + reg |= DWC3_DCFG_SUPERSPEED_PLUS;
> + break;
> + default:
> + dev_err(dwc->dev, "invalid speed (%d)\n", dwc->gadget_max_speed);
> +
> + if (DWC3_IP_IS(DWC3))
> + reg |= DWC3_DCFG_SUPERSPEED;
> + else
> + reg |= DWC3_DCFG_SUPERSPEED_PLUS;
> + }
> + }
> + dwc3_writel(dwc->regs, DWC3_DCFG, reg);
> +}
> +
> static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
> {
> u32 reg;
> @@ -1963,6 +2018,7 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
> if (dwc->has_hibernation)
> reg |= DWC3_DCTL_KEEP_CONNECT;
>
> + __dwc3_gadget_set_speed(dwc);
> dwc->pullups_connected = true;
> } else {
> reg &= ~DWC3_DCTL_RUN_STOP;
> @@ -2318,59 +2374,9 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
> {
> struct dwc3 *dwc = gadget_to_dwc(g);
> unsigned long flags;
> - u32 reg;
>
> spin_lock_irqsave(&dwc->lock, flags);
> - reg = dwc3_readl(dwc->regs, DWC3_DCFG);
> - reg &= ~(DWC3_DCFG_SPEED_MASK);
> -
> - /*
> - * WORKAROUND: DWC3 revision < 2.20a have an issue
> - * which would cause metastability state on Run/Stop
> - * bit if we try to force the IP to USB2-only mode.
> - *
> - * Because of that, we cannot configure the IP to any
> - * speed other than the SuperSpeed
> - *
> - * Refers to:
> - *
> - * STAR#9000525659: Clock Domain Crossing on DCTL in
> - * USB 2.0 Mode
> - */
> - if (DWC3_VER_IS_PRIOR(DWC3, 220A) &&
> - !dwc->dis_metastability_quirk) {
> - reg |= DWC3_DCFG_SUPERSPEED;
> - } else {
> - switch (speed) {
> - case USB_SPEED_LOW:
> - reg |= DWC3_DCFG_LOWSPEED;
> - break;
> - case USB_SPEED_FULL:
> - reg |= DWC3_DCFG_FULLSPEED;
> - break;
> - case USB_SPEED_HIGH:
> - reg |= DWC3_DCFG_HIGHSPEED;
> - break;
> - case USB_SPEED_SUPER:
> - reg |= DWC3_DCFG_SUPERSPEED;
> - break;
> - case USB_SPEED_SUPER_PLUS:
> - if (DWC3_IP_IS(DWC3))
> - reg |= DWC3_DCFG_SUPERSPEED;
> - else
> - reg |= DWC3_DCFG_SUPERSPEED_PLUS;
> - break;
> - default:
> - dev_err(dwc->dev, "invalid speed (%d)\n", speed);
> -
> - if (DWC3_IP_IS(DWC3))
> - reg |= DWC3_DCFG_SUPERSPEED;
> - else
> - reg |= DWC3_DCFG_SUPERSPEED_PLUS;
> - }
> - }
> - dwc3_writel(dwc->regs, DWC3_DCFG, reg);
> -
> + dwc->gadget_max_speed = speed;
> spin_unlock_irqrestore(&dwc->lock, flags);
> }
>
On 10/28/2020 5:43 PM, Thinh Nguyen wrote:
> Hi,
>
> Wesley Cheng wrote:
>> The USB gadget/UDC driver can restrict the DWC3 controller speed using
>> dwc3_gadget_set_speed(). Store this setting into a variable, in order for
>> this setting to persist across controller resets due to runtime PM.
>
> Why do we need to do this? DCFG should persist unless we overwrite it.
> The current PM shouldn't update the current speed setting.
>
> BR,
> Thinh
>
Hi Thinh,
During runtime PM suspend, the dwc3_suspend_common() will call
dwc3_core_exit(). On some platforms they register the DWC3 reset
control to the DWC3 core driver (otherwise could be handled in the DWC3
glue drivers), which will be asserted here:
static void dwc3_core_exit(struct dwc3 *dwc)
{
...
reset_control_assert(dwc->reset);
From the SNPS databook (Table 2-2 Resets for Registers) it mentions that
assertion of the reset signal will reset the DCFG register.
In addition to the above, with the change to allow runtime PM suspend
during UDC unbind, we need a way to avoid writing to the DCFG during the
UDC bind path. (if we entered suspend before re-binding the UDC) If we
add an early exit based on the PM state (in
dwc3_gadget_set_udc_speed()), then we basically ignore the max speed
request from the UDC/gadget layer.
Since it looks like the DWC3 gadget driver doesn't like using
synchronous PM runtime resumes, by going this route, we can allow the
async runtime resume handler to do everything, such as writing the speed
config and re-enabling the controller.
Thanks
Regards,
Wesley Cheng
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Wesley Cheng wrote:
>
> On 10/28/2020 5:43 PM, Thinh Nguyen wrote:
>> Hi,
>>
>> Wesley Cheng wrote:
>>> The USB gadget/UDC driver can restrict the DWC3 controller speed using
>>> dwc3_gadget_set_speed(). Store this setting into a variable, in order for
>>> this setting to persist across controller resets due to runtime PM.
>> Why do we need to do this? DCFG should persist unless we overwrite it.
>> The current PM shouldn't update the current speed setting.
>>
>> BR,
>> Thinh
>>
> Hi Thinh,
>
> During runtime PM suspend, the dwc3_suspend_common() will call
> dwc3_core_exit(). On some platforms they register the DWC3 reset
> control to the DWC3 core driver (otherwise could be handled in the DWC3
> glue drivers), which will be asserted here:
>
> static void dwc3_core_exit(struct dwc3 *dwc)
> {
> ...
> reset_control_assert(dwc->reset);
>
> From the SNPS databook (Table 2-2 Resets for Registers) it mentions that
> assertion of the reset signal will reset the DCFG register.
I see. There's a hard reset on some platforms.
>
> In addition to the above, with the change to allow runtime PM suspend
> during UDC unbind, we need a way to avoid writing to the DCFG during the
> UDC bind path. (if we entered suspend before re-binding the UDC) If we
> add an early exit based on the PM state (in
> dwc3_gadget_set_udc_speed()), then we basically ignore the max speed
> request from the UDC/gadget layer.
Then shouldn't we restore the speed setting when dwc3_gadget_resume()
instead of in dwc3_gadget_run_stop()?
>
> Since it looks like the DWC3 gadget driver doesn't like using
> synchronous PM runtime resumes, by going this route, we can allow the
> async runtime resume handler to do everything, such as writing the speed
> config and re-enabling the controller.
>
> Thanks
>
> Regards,
> Wesley Cheng
Thanks,
Thinh
Thinh Nguyen wrote:
> Wesley Cheng wrote:
>> On 10/28/2020 5:43 PM, Thinh Nguyen wrote:
>>> Hi,
>>>
>>> Wesley Cheng wrote:
>>>> The USB gadget/UDC driver can restrict the DWC3 controller speed using
>>>> dwc3_gadget_set_speed(). Store this setting into a variable, in order for
>>>> this setting to persist across controller resets due to runtime PM.
>>> Why do we need to do this? DCFG should persist unless we overwrite it.
>>> The current PM shouldn't update the current speed setting.
>>>
>>> BR,
>>> Thinh
>>>
>> Hi Thinh,
>>
>> During runtime PM suspend, the dwc3_suspend_common() will call
>> dwc3_core_exit(). On some platforms they register the DWC3 reset
>> control to the DWC3 core driver (otherwise could be handled in the DWC3
>> glue drivers), which will be asserted here:
>>
>> static void dwc3_core_exit(struct dwc3 *dwc)
>> {
>> ...
>> reset_control_assert(dwc->reset);
>>
>> From the SNPS databook (Table 2-2 Resets for Registers) it mentions that
>> assertion of the reset signal will reset the DCFG register.
> I see. There's a hard reset on some platforms.
>
>> In addition to the above, with the change to allow runtime PM suspend
>> during UDC unbind, we need a way to avoid writing to the DCFG during the
>> UDC bind path. (if we entered suspend before re-binding the UDC) If we
>> add an early exit based on the PM state (in
>> dwc3_gadget_set_udc_speed()), then we basically ignore the max speed
>> request from the UDC/gadget layer.
> Then shouldn't we restore the speed setting when dwc3_gadget_resume()
> instead of in dwc3_gadget_run_stop()?
Actually, ignore this comment. This is fine, and it may save some
register read/write operations. I was thinking of save/restore from
suspend and resume similar to hibernation.
Thanks,
Thinh
>
>> Since it looks like the DWC3 gadget driver doesn't like using
>> synchronous PM runtime resumes, by going this route, we can allow the
>> async runtime resume handler to do everything, such as writing the speed
>> config and re-enabling the controller.
>>
>> Thanks
>>
>> Regards,
>> Wesley Cheng
> Thanks,
> Thinh