* [RFC PATCH 0/2] usb: dwc3: Support wake-up from USB suspend. @ 2023-03-20 9:34 Roger Quadros 2023-03-20 9:34 ` [RFC PATCH 1/2] dt-bindings: usb: snps,dwc3: Add 'snps,gadget-keep-connect-sys-sleep' Roger Quadros 2023-03-20 9:34 ` [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature Roger Quadros 0 siblings, 2 replies; 24+ messages in thread From: Roger Quadros @ 2023-03-20 9:34 UTC (permalink / raw) To: Thinh.Nguyen, stern Cc: gregkh, vigneshr, srk, r-gunasekaran, linux-usb, linux-kernel, Roger Quadros Hi, The current USB gadget driver behaviour is to stop the controller and disconnect from the bus during System sleep. This series introduces a new DT property that will change this behaviour and keep the controller active and connected to the bus during System sleep. This is useful for applications that want to enter a low power state when USB is suspended but remain connected so they can resume activity on USB resume. This feature introduces a new constraint if Gadget driver is connected to USB host: i.e. the gadget must be in USB suspend state to allow a System sleep as we cannot process any USB transactions when in System sleep. The system hardware is responsible to detect the end of USB suspend and wake up the system so we can begin processing the USB transactions as soon as possible. Some prior discussion about System suspend vs USB suspend can be found at [1] [1] - https://marc.info/?l=linux-usb&m=167645398109860&w=2 cheers, -roger Roger Quadros (2): dt-bindings: usb: snps,dwc3: Add 'snps,gadget-keep-connect-sys-sleep' usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature .../devicetree/bindings/usb/snps,dwc3.yaml | 5 ++++ drivers/usb/dwc3/core.c | 25 ++++++++++++++----- drivers/usb/dwc3/core.h | 2 ++ drivers/usb/dwc3/gadget.c | 25 +++++++++++++++++-- 4 files changed, 49 insertions(+), 8 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 24+ messages in thread
* [RFC PATCH 1/2] dt-bindings: usb: snps,dwc3: Add 'snps,gadget-keep-connect-sys-sleep' 2023-03-20 9:34 [RFC PATCH 0/2] usb: dwc3: Support wake-up from USB suspend Roger Quadros @ 2023-03-20 9:34 ` Roger Quadros 2023-03-20 13:11 ` Rob Herring 2023-03-20 13:22 ` Rob Herring 2023-03-20 9:34 ` [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature Roger Quadros 1 sibling, 2 replies; 24+ messages in thread From: Roger Quadros @ 2023-03-20 9:34 UTC (permalink / raw) To: Thinh.Nguyen, stern Cc: gregkh, vigneshr, srk, r-gunasekaran, linux-usb, linux-kernel, Roger Quadros, devicetree The current USB gadget driver behaviour is to stop the controller and disconnect from the bus during System sleep. The 'snps,gadget-keep-connect-sys-sleep' property can be used to change this behaviour and keep the controller active and connected to the bus during System sleep. This is useful for applications that want to enter a low power state when USB is suspended but remain connected so they can resume activity on USB resume. This feature introduces a new constraint if Gadget driver is connected to USB host: i.e. the gadget must be in USB suspend state to allow a System sleep as we cannot process any USB transactions when in System sleep. The system hardware is responsible to detect the end of USB suspend and wake up the system so we can begin processing the USB transactions as soon as possible. Cc: devicetree@vger.kernel.org Signed-off-by: Roger Quadros <rogerq@kernel.org> --- Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml index be36956af53b..1ce8008e7fef 100644 --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml @@ -262,6 +262,11 @@ properties: asserts utmi_sleep_n. type: boolean + snps,gadget-keep-connect-sys-sleep: + description: + If True then gadget driver will not disconnect during system sleep. + System sleep will not be allowed if gadget is not already in USB suspend. + snps,hird-threshold: description: HIRD threshold $ref: /schemas/types.yaml#/definitions/uint8 -- 2.34.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 1/2] dt-bindings: usb: snps,dwc3: Add 'snps,gadget-keep-connect-sys-sleep' 2023-03-20 9:34 ` [RFC PATCH 1/2] dt-bindings: usb: snps,dwc3: Add 'snps,gadget-keep-connect-sys-sleep' Roger Quadros @ 2023-03-20 13:11 ` Rob Herring 2023-03-20 13:22 ` Rob Herring 1 sibling, 0 replies; 24+ messages in thread From: Rob Herring @ 2023-03-20 13:11 UTC (permalink / raw) To: Roger Quadros Cc: stern, vigneshr, devicetree, Thinh.Nguyen, gregkh, srk, linux-usb, linux-kernel, r-gunasekaran On Mon, 20 Mar 2023 11:34:46 +0200, Roger Quadros wrote: > The current USB gadget driver behaviour is to stop the controller > and disconnect from the bus during System sleep. > > The 'snps,gadget-keep-connect-sys-sleep' property can be used to > change this behaviour and keep the controller active and connected > to the bus during System sleep. This is useful for applications > that want to enter a low power state when USB is suspended but > remain connected so they can resume activity on USB resume. > > This feature introduces a new constraint if Gadget driver is connected > to USB host: i.e. the gadget must be in USB suspend state to allow > a System sleep as we cannot process any USB transactions > when in System sleep. > > The system hardware is responsible to detect the end of USB suspend > and wake up the system so we can begin processing the USB transactions > as soon as possible. > > Cc: devicetree@vger.kernel.org > Signed-off-by: Roger Quadros <rogerq@kernel.org> > --- > Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 5 +++++ > 1 file changed, 5 insertions(+) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: ./Documentation/devicetree/bindings/usb/samsung,exynos-dwc3.yaml: Error in referenced schema matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml ./Documentation/devicetree/bindings/usb/intel,keembay-dwc3.yaml: Error in referenced schema matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml ./Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml: Error in referenced schema matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml ./Documentation/devicetree/bindings/usb/qcom,dwc3.yaml: Error in referenced schema matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml ./Documentation/devicetree/bindings/usb/rockchip,rk3399-dwc3.yaml: Error in referenced schema matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml ./Documentation/devicetree/bindings/usb/fsl,imx8mp-dwc3.yaml: Error in referenced schema matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml ./Documentation/devicetree/bindings/usb/ti,keystone-dwc3.yaml: Error in referenced schema matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/snps,dwc3.yaml: properties:snps,gadget-keep-connect-sys-sleep: 'oneOf' conditional failed, one must be fixed: 'type' is a required property hint: A vendor boolean property can use "type: boolean" /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/snps,dwc3.yaml: properties:snps,gadget-keep-connect-sys-sleep: 'oneOf' conditional failed, one must be fixed: 'enum' is a required property 'const' is a required property hint: A vendor string property with exact values has an implicit type from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/usb/snps,dwc3.yaml: properties:snps,gadget-keep-connect-sys-sleep: 'oneOf' conditional failed, one must be fixed: '$ref' is a required property 'allOf' is a required property hint: A vendor property needs a $ref to types.yaml from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml# hint: Vendor specific properties must have a type and description unless they have a defined, common suffix. from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml# ./Documentation/devicetree/bindings/usb/dwc3-xilinx.yaml: Error in referenced schema matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml ./Documentation/devicetree/bindings/usb/amlogic,meson-g12a-usb-ctrl.yaml: Error in referenced schema matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml ./Documentation/devicetree/bindings/usb/ti,am62-usb.yaml: Error in referenced schema matching $id: http://devicetree.org/schemas/usb/snps,dwc3.yaml doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230320093447.32105-2-rogerq@kernel.org The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 1/2] dt-bindings: usb: snps,dwc3: Add 'snps,gadget-keep-connect-sys-sleep' 2023-03-20 9:34 ` [RFC PATCH 1/2] dt-bindings: usb: snps,dwc3: Add 'snps,gadget-keep-connect-sys-sleep' Roger Quadros 2023-03-20 13:11 ` Rob Herring @ 2023-03-20 13:22 ` Rob Herring 2023-03-21 9:44 ` Roger Quadros 1 sibling, 1 reply; 24+ messages in thread From: Rob Herring @ 2023-03-20 13:22 UTC (permalink / raw) To: Roger Quadros Cc: Thinh.Nguyen, stern, gregkh, vigneshr, srk, r-gunasekaran, linux-usb, linux-kernel, devicetree On Mon, Mar 20, 2023 at 11:34:46AM +0200, Roger Quadros wrote: > The current USB gadget driver behaviour is to stop the controller > and disconnect from the bus during System sleep. What's USB gadget? ;) > The 'snps,gadget-keep-connect-sys-sleep' property can be used to > change this behaviour and keep the controller active and connected > to the bus during System sleep. This is useful for applications > that want to enter a low power state when USB is suspended but > remain connected so they can resume activity on USB resume. > > This feature introduces a new constraint if Gadget driver is connected > to USB host: i.e. the gadget must be in USB suspend state to allow > a System sleep as we cannot process any USB transactions > when in System sleep. > > The system hardware is responsible to detect the end of USB suspend > and wake up the system so we can begin processing the USB transactions > as soon as possible. Sounds like something the user/OS would want to choose rather than fixed by your board's firmware. Is this somehow DWC3 specific? If not, why a DWC3 specific property? > > Cc: devicetree@vger.kernel.org > Signed-off-by: Roger Quadros <rogerq@kernel.org> > --- > Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml > index be36956af53b..1ce8008e7fef 100644 > --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml > @@ -262,6 +262,11 @@ properties: > asserts utmi_sleep_n. > type: boolean > > + snps,gadget-keep-connect-sys-sleep: > + description: > + If True then gadget driver will not disconnect during system sleep. > + System sleep will not be allowed if gadget is not already in USB suspend. 'gadget' is a Linuxism. > + > snps,hird-threshold: > description: HIRD threshold > $ref: /schemas/types.yaml#/definitions/uint8 > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 1/2] dt-bindings: usb: snps,dwc3: Add 'snps,gadget-keep-connect-sys-sleep' 2023-03-20 13:22 ` Rob Herring @ 2023-03-21 9:44 ` Roger Quadros 0 siblings, 0 replies; 24+ messages in thread From: Roger Quadros @ 2023-03-21 9:44 UTC (permalink / raw) To: Rob Herring Cc: Thinh.Nguyen, stern, gregkh, vigneshr, srk, r-gunasekaran, linux-usb, linux-kernel, devicetree On 20/03/2023 15:22, Rob Herring wrote: > On Mon, Mar 20, 2023 at 11:34:46AM +0200, Roger Quadros wrote: >> The current USB gadget driver behaviour is to stop the controller >> and disconnect from the bus during System sleep. > > What's USB gadget? ;) :) > >> The 'snps,gadget-keep-connect-sys-sleep' property can be used to >> change this behaviour and keep the controller active and connected >> to the bus during System sleep. This is useful for applications >> that want to enter a low power state when USB is suspended but >> remain connected so they can resume activity on USB resume. >> >> This feature introduces a new constraint if Gadget driver is connected >> to USB host: i.e. the gadget must be in USB suspend state to allow >> a System sleep as we cannot process any USB transactions >> when in System sleep. >> >> The system hardware is responsible to detect the end of USB suspend >> and wake up the system so we can begin processing the USB transactions >> as soon as possible. > > Sounds like something the user/OS would want to choose rather than fixed > by your board's firmware. Yes. > > Is this somehow DWC3 specific? If not, why a DWC3 specific property? This is not DWC3 specific. Should we make this a UDC class device's sysfs attribute instead? Only concern is that in dual-role case, if a role switch from device mode to host mode and back to device mode happens, we loose the UDC device's attributes as we re-init the UDC device. Or should we make it a udc_core module parameter? This should be persistent between role switches. > >> >> Cc: devicetree@vger.kernel.org >> Signed-off-by: Roger Quadros <rogerq@kernel.org> >> --- >> Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml >> index be36956af53b..1ce8008e7fef 100644 >> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml >> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml >> @@ -262,6 +262,11 @@ properties: >> asserts utmi_sleep_n. >> type: boolean >> >> + snps,gadget-keep-connect-sys-sleep: >> + description: >> + If True then gadget driver will not disconnect during system sleep. >> + System sleep will not be allowed if gadget is not already in USB suspend. > > 'gadget' is a Linuxism. Got it. Will avoid using it ;) > >> + >> snps,hird-threshold: >> description: HIRD threshold >> $ref: /schemas/types.yaml#/definitions/uint8 >> -- >> 2.34.1 >> cheers, -roger ^ permalink raw reply [flat|nested] 24+ messages in thread
* [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature 2023-03-20 9:34 [RFC PATCH 0/2] usb: dwc3: Support wake-up from USB suspend Roger Quadros 2023-03-20 9:34 ` [RFC PATCH 1/2] dt-bindings: usb: snps,dwc3: Add 'snps,gadget-keep-connect-sys-sleep' Roger Quadros @ 2023-03-20 9:34 ` Roger Quadros 2023-03-20 18:52 ` Thinh Nguyen 1 sibling, 1 reply; 24+ messages in thread From: Roger Quadros @ 2023-03-20 9:34 UTC (permalink / raw) To: Thinh.Nguyen, stern Cc: gregkh, vigneshr, srk, r-gunasekaran, linux-usb, linux-kernel, Roger Quadros Implement 'snps,gadget-keep-connect-sys-sleep' property. Do not stop the gadget controller and disconnect if this property is present and we are connected to a USB Host. Prevent System sleep if Gadget is not in USB suspend. Signed-off-by: Roger Quadros <rogerq@kernel.org> --- drivers/usb/dwc3/core.c | 25 +++++++++++++++++++------ drivers/usb/dwc3/core.h | 2 ++ drivers/usb/dwc3/gadget.c | 25 +++++++++++++++++++++++-- 3 files changed, 44 insertions(+), 8 deletions(-) diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 476b63618511..a47bbaa27302 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -1575,6 +1575,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) dwc->dis_split_quirk = device_property_read_bool(dev, "snps,dis-split-quirk"); + dwc->gadget_keep_connect_sys_sleep = device_property_read_bool(dev, + "snps,gadget-keep-connect-sys-sleep"); + dwc->lpm_nyet_threshold = lpm_nyet_threshold; dwc->tx_de_emphasis = tx_de_emphasis; @@ -2027,14 +2030,20 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) { unsigned long flags; u32 reg; + int ret; switch (dwc->current_dr_role) { case DWC3_GCTL_PRTCAP_DEVICE: if (pm_runtime_suspended(dwc->dev)) break; - dwc3_gadget_suspend(dwc); + ret = dwc3_gadget_suspend(dwc); + if (ret) { + dev_err(dwc->dev, "gadget not suspended: %d\n", ret); + return ret; + } synchronize_irq(dwc->irq_gadget); - dwc3_core_exit(dwc); + if(!dwc->gadget_keep_connect_sys_sleep) + dwc3_core_exit(dwc); break; case DWC3_GCTL_PRTCAP_HOST: if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) { @@ -2088,11 +2097,15 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) switch (dwc->current_dr_role) { case DWC3_GCTL_PRTCAP_DEVICE: - ret = dwc3_core_init_for_resume(dwc); - if (ret) - return ret; + if (!dwc->gadget_keep_connect_sys_sleep) + { + ret = dwc3_core_init_for_resume(dwc); + if (ret) + return ret; + + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); + } - dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); dwc3_gadget_resume(dwc); break; case DWC3_GCTL_PRTCAP_HOST: diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index 582ebd9cf9c2..f84bac815bed 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -1328,6 +1328,8 @@ struct dwc3 { unsigned dis_split_quirk:1; unsigned async_callbacks:1; + unsigned gadget_keep_connect_sys_sleep:1; + u16 imod_interval; int max_cfg_eps; diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 3c63fa97a680..8062e44f63f6 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -4572,12 +4572,23 @@ void dwc3_gadget_exit(struct dwc3 *dwc) int dwc3_gadget_suspend(struct dwc3 *dwc) { unsigned long flags; + int link_state; if (!dwc->gadget_driver) return 0; - dwc3_gadget_run_stop(dwc, false, false); + if (dwc->gadget_keep_connect_sys_sleep && dwc->connected) { + link_state = dwc3_gadget_get_link_state(dwc); + /* Prevent PM Sleep if not in U3/L2 */ + if (link_state != DWC3_LINK_STATE_U3) + return -EBUSY; + + /* don't stop/disconnect */ + dwc3_gadget_disable_irq(dwc); + return 0; + } + dwc3_gadget_run_stop(dwc, false, false); spin_lock_irqsave(&dwc->lock, flags); dwc3_disconnect_gadget(dwc); __dwc3_gadget_stop(dwc); @@ -4588,11 +4599,21 @@ int dwc3_gadget_suspend(struct dwc3 *dwc) int dwc3_gadget_resume(struct dwc3 *dwc) { - int ret; + int ret; + irqreturn_t irq_t; if (!dwc->gadget_driver || !dwc->softconnect) return 0; + if (dwc->gadget_keep_connect_sys_sleep && dwc->connected) { + dwc3_gadget_enable_irq(dwc); + /* check pending events */ + irq_t = dwc3_check_event_buf(dwc->ev_buf); + if (irq_t == IRQ_WAKE_THREAD) + dwc3_process_event_buf(dwc->ev_buf); + return 0; + } + ret = __dwc3_gadget_start(dwc); if (ret < 0) goto err0; -- 2.34.1 ^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature 2023-03-20 9:34 ` [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature Roger Quadros @ 2023-03-20 18:52 ` Thinh Nguyen 2023-03-21 10:20 ` Roger Quadros 0 siblings, 1 reply; 24+ messages in thread From: Thinh Nguyen @ 2023-03-20 18:52 UTC (permalink / raw) To: Roger Quadros Cc: Thinh Nguyen, stern, gregkh, vigneshr, srk, r-gunasekaran, linux-usb, linux-kernel Hi, On Mon, Mar 20, 2023, Roger Quadros wrote: > Implement 'snps,gadget-keep-connect-sys-sleep' property. > > Do not stop the gadget controller and disconnect if this > property is present and we are connected to a USB Host. > > Prevent System sleep if Gadget is not in USB suspend. > > Signed-off-by: Roger Quadros <rogerq@kernel.org> > --- > drivers/usb/dwc3/core.c | 25 +++++++++++++++++++------ > drivers/usb/dwc3/core.h | 2 ++ > drivers/usb/dwc3/gadget.c | 25 +++++++++++++++++++++++-- > 3 files changed, 44 insertions(+), 8 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 476b63618511..a47bbaa27302 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1575,6 +1575,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) > dwc->dis_split_quirk = device_property_read_bool(dev, > "snps,dis-split-quirk"); > > + dwc->gadget_keep_connect_sys_sleep = device_property_read_bool(dev, > + "snps,gadget-keep-connect-sys-sleep"); > + > dwc->lpm_nyet_threshold = lpm_nyet_threshold; > dwc->tx_de_emphasis = tx_de_emphasis; > > @@ -2027,14 +2030,20 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > { > unsigned long flags; > u32 reg; > + int ret; > > switch (dwc->current_dr_role) { > case DWC3_GCTL_PRTCAP_DEVICE: > if (pm_runtime_suspended(dwc->dev)) > break; > - dwc3_gadget_suspend(dwc); > + ret = dwc3_gadget_suspend(dwc); > + if (ret) { > + dev_err(dwc->dev, "gadget not suspended: %d\n", ret); > + return ret; > + } > synchronize_irq(dwc->irq_gadget); > - dwc3_core_exit(dwc); > + if(!dwc->gadget_keep_connect_sys_sleep) > + dwc3_core_exit(dwc); > break; > case DWC3_GCTL_PRTCAP_HOST: > if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) { > @@ -2088,11 +2097,15 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) > > switch (dwc->current_dr_role) { > case DWC3_GCTL_PRTCAP_DEVICE: > - ret = dwc3_core_init_for_resume(dwc); > - if (ret) > - return ret; > + if (!dwc->gadget_keep_connect_sys_sleep) > + { > + ret = dwc3_core_init_for_resume(dwc); > + if (ret) > + return ret; > + > + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); > + } > > - dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); > dwc3_gadget_resume(dwc); > break; > case DWC3_GCTL_PRTCAP_HOST: > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 582ebd9cf9c2..f84bac815bed 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -1328,6 +1328,8 @@ struct dwc3 { > unsigned dis_split_quirk:1; > unsigned async_callbacks:1; > > + unsigned gadget_keep_connect_sys_sleep:1; > + > u16 imod_interval; > > int max_cfg_eps; > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 3c63fa97a680..8062e44f63f6 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -4572,12 +4572,23 @@ void dwc3_gadget_exit(struct dwc3 *dwc) > int dwc3_gadget_suspend(struct dwc3 *dwc) > { > unsigned long flags; > + int link_state; > > if (!dwc->gadget_driver) > return 0; > > - dwc3_gadget_run_stop(dwc, false, false); > + if (dwc->gadget_keep_connect_sys_sleep && dwc->connected) { > + link_state = dwc3_gadget_get_link_state(dwc); > + /* Prevent PM Sleep if not in U3/L2 */ > + if (link_state != DWC3_LINK_STATE_U3) > + return -EBUSY; > + > + /* don't stop/disconnect */ > + dwc3_gadget_disable_irq(dwc); We shouldn't disable event interrupt here. What will happen if the device is disconnected and reconnect to the host while the device is still in system suspend? The host would not be able to communicate with the device then. BR, Thinh > + return 0; > + } > > + dwc3_gadget_run_stop(dwc, false, false); > spin_lock_irqsave(&dwc->lock, flags); > dwc3_disconnect_gadget(dwc); > __dwc3_gadget_stop(dwc); > @@ -4588,11 +4599,21 @@ int dwc3_gadget_suspend(struct dwc3 *dwc) > > int dwc3_gadget_resume(struct dwc3 *dwc) > { > - int ret; > + int ret; > + irqreturn_t irq_t; > > if (!dwc->gadget_driver || !dwc->softconnect) > return 0; > > + if (dwc->gadget_keep_connect_sys_sleep && dwc->connected) { > + dwc3_gadget_enable_irq(dwc); > + /* check pending events */ > + irq_t = dwc3_check_event_buf(dwc->ev_buf); > + if (irq_t == IRQ_WAKE_THREAD) > + dwc3_process_event_buf(dwc->ev_buf); > + return 0; > + } > + > ret = __dwc3_gadget_start(dwc); > if (ret < 0) > goto err0; > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature 2023-03-20 18:52 ` Thinh Nguyen @ 2023-03-21 10:20 ` Roger Quadros 2023-03-21 18:43 ` Thinh Nguyen 0 siblings, 1 reply; 24+ messages in thread From: Roger Quadros @ 2023-03-21 10:20 UTC (permalink / raw) To: Thinh Nguyen Cc: stern, gregkh, vigneshr, srk, r-gunasekaran, linux-usb, linux-kernel Hi Thinh, On 20/03/2023 20:52, Thinh Nguyen wrote: > Hi, > > On Mon, Mar 20, 2023, Roger Quadros wrote: >> Implement 'snps,gadget-keep-connect-sys-sleep' property. >> >> Do not stop the gadget controller and disconnect if this >> property is present and we are connected to a USB Host. >> >> Prevent System sleep if Gadget is not in USB suspend. >> >> Signed-off-by: Roger Quadros <rogerq@kernel.org> >> --- >> drivers/usb/dwc3/core.c | 25 +++++++++++++++++++------ >> drivers/usb/dwc3/core.h | 2 ++ >> drivers/usb/dwc3/gadget.c | 25 +++++++++++++++++++++++-- >> 3 files changed, 44 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index 476b63618511..a47bbaa27302 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -1575,6 +1575,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) >> dwc->dis_split_quirk = device_property_read_bool(dev, >> "snps,dis-split-quirk"); >> >> + dwc->gadget_keep_connect_sys_sleep = device_property_read_bool(dev, >> + "snps,gadget-keep-connect-sys-sleep"); >> + >> dwc->lpm_nyet_threshold = lpm_nyet_threshold; >> dwc->tx_de_emphasis = tx_de_emphasis; >> >> @@ -2027,14 +2030,20 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) >> { >> unsigned long flags; >> u32 reg; >> + int ret; >> >> switch (dwc->current_dr_role) { >> case DWC3_GCTL_PRTCAP_DEVICE: >> if (pm_runtime_suspended(dwc->dev)) >> break; >> - dwc3_gadget_suspend(dwc); >> + ret = dwc3_gadget_suspend(dwc); >> + if (ret) { >> + dev_err(dwc->dev, "gadget not suspended: %d\n", ret); >> + return ret; >> + } >> synchronize_irq(dwc->irq_gadget); >> - dwc3_core_exit(dwc); >> + if(!dwc->gadget_keep_connect_sys_sleep) >> + dwc3_core_exit(dwc); >> break; >> case DWC3_GCTL_PRTCAP_HOST: >> if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) { >> @@ -2088,11 +2097,15 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) >> >> switch (dwc->current_dr_role) { >> case DWC3_GCTL_PRTCAP_DEVICE: >> - ret = dwc3_core_init_for_resume(dwc); >> - if (ret) >> - return ret; >> + if (!dwc->gadget_keep_connect_sys_sleep) >> + { >> + ret = dwc3_core_init_for_resume(dwc); >> + if (ret) >> + return ret; >> + >> + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); >> + } >> >> - dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); >> dwc3_gadget_resume(dwc); >> break; >> case DWC3_GCTL_PRTCAP_HOST: >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >> index 582ebd9cf9c2..f84bac815bed 100644 >> --- a/drivers/usb/dwc3/core.h >> +++ b/drivers/usb/dwc3/core.h >> @@ -1328,6 +1328,8 @@ struct dwc3 { >> unsigned dis_split_quirk:1; >> unsigned async_callbacks:1; >> >> + unsigned gadget_keep_connect_sys_sleep:1; >> + >> u16 imod_interval; >> >> int max_cfg_eps; >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 3c63fa97a680..8062e44f63f6 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -4572,12 +4572,23 @@ void dwc3_gadget_exit(struct dwc3 *dwc) >> int dwc3_gadget_suspend(struct dwc3 *dwc) >> { >> unsigned long flags; >> + int link_state; >> >> if (!dwc->gadget_driver) >> return 0; >> >> - dwc3_gadget_run_stop(dwc, false, false); >> + if (dwc->gadget_keep_connect_sys_sleep && dwc->connected) { >> + link_state = dwc3_gadget_get_link_state(dwc); >> + /* Prevent PM Sleep if not in U3/L2 */ >> + if (link_state != DWC3_LINK_STATE_U3) >> + return -EBUSY; >> + >> + /* don't stop/disconnect */ >> + dwc3_gadget_disable_irq(dwc); > > We shouldn't disable event interrupt here. What will happen if the Due to some reason, if I don't disable the event interrupts here then after USB resume the USB controller is malfunctioning. It no longer responds to any requests from Host. > device is disconnected and reconnect to the host while the device is > still in system suspend? The host would not be able to communicate with > the device then. In the TI platform, The system is woken up on any VBUS/linestate change and in dwc3_gadget_resume we enable the events again and check for pending events. Is it pointless to check for pending events there? cheers, -roger > > BR, > Thinh > >> + return 0; >> + } >> >> + dwc3_gadget_run_stop(dwc, false, false); >> spin_lock_irqsave(&dwc->lock, flags); >> dwc3_disconnect_gadget(dwc); >> __dwc3_gadget_stop(dwc); >> @@ -4588,11 +4599,21 @@ int dwc3_gadget_suspend(struct dwc3 *dwc) >> >> int dwc3_gadget_resume(struct dwc3 *dwc) >> { >> - int ret; >> + int ret; >> + irqreturn_t irq_t; >> >> if (!dwc->gadget_driver || !dwc->softconnect) >> return 0; >> >> + if (dwc->gadget_keep_connect_sys_sleep && dwc->connected) { >> + dwc3_gadget_enable_irq(dwc); >> + /* check pending events */ >> + irq_t = dwc3_check_event_buf(dwc->ev_buf); >> + if (irq_t == IRQ_WAKE_THREAD) >> + dwc3_process_event_buf(dwc->ev_buf); >> + return 0; >> + } >> + >> ret = __dwc3_gadget_start(dwc); >> if (ret < 0) >> goto err0; >> -- >> 2.34.1 ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature 2023-03-21 10:20 ` Roger Quadros @ 2023-03-21 18:43 ` Thinh Nguyen 2023-03-21 19:05 ` Thinh Nguyen 0 siblings, 1 reply; 24+ messages in thread From: Thinh Nguyen @ 2023-03-21 18:43 UTC (permalink / raw) To: Roger Quadros Cc: Thinh Nguyen, stern, gregkh, vigneshr, srk, r-gunasekaran, linux-usb, linux-kernel On Tue, Mar 21, 2023, Roger Quadros wrote: > Hi Thinh, > > On 20/03/2023 20:52, Thinh Nguyen wrote: > > Hi, > > > > On Mon, Mar 20, 2023, Roger Quadros wrote: > >> Implement 'snps,gadget-keep-connect-sys-sleep' property. > >> > >> Do not stop the gadget controller and disconnect if this > >> property is present and we are connected to a USB Host. > >> > >> Prevent System sleep if Gadget is not in USB suspend. > >> > >> Signed-off-by: Roger Quadros <rogerq@kernel.org> > >> --- > >> drivers/usb/dwc3/core.c | 25 +++++++++++++++++++------ > >> drivers/usb/dwc3/core.h | 2 ++ > >> drivers/usb/dwc3/gadget.c | 25 +++++++++++++++++++++++-- > >> 3 files changed, 44 insertions(+), 8 deletions(-) > >> > >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > >> index 476b63618511..a47bbaa27302 100644 > >> --- a/drivers/usb/dwc3/core.c > >> +++ b/drivers/usb/dwc3/core.c > >> @@ -1575,6 +1575,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) > >> dwc->dis_split_quirk = device_property_read_bool(dev, > >> "snps,dis-split-quirk"); > >> > >> + dwc->gadget_keep_connect_sys_sleep = device_property_read_bool(dev, > >> + "snps,gadget-keep-connect-sys-sleep"); > >> + > >> dwc->lpm_nyet_threshold = lpm_nyet_threshold; > >> dwc->tx_de_emphasis = tx_de_emphasis; > >> > >> @@ -2027,14 +2030,20 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > >> { > >> unsigned long flags; > >> u32 reg; > >> + int ret; > >> > >> switch (dwc->current_dr_role) { > >> case DWC3_GCTL_PRTCAP_DEVICE: > >> if (pm_runtime_suspended(dwc->dev)) > >> break; > >> - dwc3_gadget_suspend(dwc); > >> + ret = dwc3_gadget_suspend(dwc); > >> + if (ret) { > >> + dev_err(dwc->dev, "gadget not suspended: %d\n", ret); > >> + return ret; > >> + } > >> synchronize_irq(dwc->irq_gadget); > >> - dwc3_core_exit(dwc); > >> + if(!dwc->gadget_keep_connect_sys_sleep) > >> + dwc3_core_exit(dwc); > >> break; > >> case DWC3_GCTL_PRTCAP_HOST: > >> if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) { > >> @@ -2088,11 +2097,15 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) > >> > >> switch (dwc->current_dr_role) { > >> case DWC3_GCTL_PRTCAP_DEVICE: > >> - ret = dwc3_core_init_for_resume(dwc); > >> - if (ret) > >> - return ret; > >> + if (!dwc->gadget_keep_connect_sys_sleep) > >> + { > >> + ret = dwc3_core_init_for_resume(dwc); > >> + if (ret) > >> + return ret; > >> + > >> + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); > >> + } > >> > >> - dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); > >> dwc3_gadget_resume(dwc); > >> break; > >> case DWC3_GCTL_PRTCAP_HOST: > >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > >> index 582ebd9cf9c2..f84bac815bed 100644 > >> --- a/drivers/usb/dwc3/core.h > >> +++ b/drivers/usb/dwc3/core.h > >> @@ -1328,6 +1328,8 @@ struct dwc3 { > >> unsigned dis_split_quirk:1; > >> unsigned async_callbacks:1; > >> > >> + unsigned gadget_keep_connect_sys_sleep:1; > >> + > >> u16 imod_interval; > >> > >> int max_cfg_eps; > >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > >> index 3c63fa97a680..8062e44f63f6 100644 > >> --- a/drivers/usb/dwc3/gadget.c > >> +++ b/drivers/usb/dwc3/gadget.c > >> @@ -4572,12 +4572,23 @@ void dwc3_gadget_exit(struct dwc3 *dwc) > >> int dwc3_gadget_suspend(struct dwc3 *dwc) > >> { > >> unsigned long flags; > >> + int link_state; > >> > >> if (!dwc->gadget_driver) > >> return 0; > >> > >> - dwc3_gadget_run_stop(dwc, false, false); > >> + if (dwc->gadget_keep_connect_sys_sleep && dwc->connected) { > >> + link_state = dwc3_gadget_get_link_state(dwc); > >> + /* Prevent PM Sleep if not in U3/L2 */ > >> + if (link_state != DWC3_LINK_STATE_U3) > >> + return -EBUSY; > >> + > >> + /* don't stop/disconnect */ > >> + dwc3_gadget_disable_irq(dwc); > > > > We shouldn't disable event interrupt here. What will happen if the > > Due to some reason, if I don't disable the event interrupts here then > after USB resume the USB controller is malfunctioning. > It no longer responds to any requests from Host. You should look into this. These events are important as they can tell whether the host initiates resume. > > > device is disconnected and reconnect to the host while the device is > > still in system suspend? The host would not be able to communicate with > > the device then. > > In the TI platform, The system is woken up on any VBUS/linestate change > and in dwc3_gadget_resume we enable the events again and check for pending > events. Is it pointless to check for pending events there? > It seems fragile for the implementation to be dependent on platform specific feature right? Also, what will happen in a typical case when the host puts the device in suspend and initiates resume while the device is in system suspend (and stay in suspend over a period of time)? There is no VBUS change. There will be problem if host detects no response from device in time. Don't we need these events to wakeup the device? BR, Thinh ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature 2023-03-21 18:43 ` Thinh Nguyen @ 2023-03-21 19:05 ` Thinh Nguyen 2023-03-22 8:11 ` Roger Quadros 0 siblings, 1 reply; 24+ messages in thread From: Thinh Nguyen @ 2023-03-21 19:05 UTC (permalink / raw) To: Roger Quadros Cc: stern, gregkh, vigneshr, srk, r-gunasekaran, linux-usb, linux-kernel On Tue, Mar 21, 2023, Thinh Nguyen wrote: > On Tue, Mar 21, 2023, Roger Quadros wrote: > > Hi Thinh, > > > > On 20/03/2023 20:52, Thinh Nguyen wrote: > > > Hi, > > > > > > On Mon, Mar 20, 2023, Roger Quadros wrote: > > >> Implement 'snps,gadget-keep-connect-sys-sleep' property. > > >> > > >> Do not stop the gadget controller and disconnect if this > > >> property is present and we are connected to a USB Host. > > >> > > >> Prevent System sleep if Gadget is not in USB suspend. > > >> > > >> Signed-off-by: Roger Quadros <rogerq@kernel.org> > > >> --- > > >> drivers/usb/dwc3/core.c | 25 +++++++++++++++++++------ > > >> drivers/usb/dwc3/core.h | 2 ++ > > >> drivers/usb/dwc3/gadget.c | 25 +++++++++++++++++++++++-- > > >> 3 files changed, 44 insertions(+), 8 deletions(-) > > >> > > >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > >> index 476b63618511..a47bbaa27302 100644 > > >> --- a/drivers/usb/dwc3/core.c > > >> +++ b/drivers/usb/dwc3/core.c > > >> @@ -1575,6 +1575,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) > > >> dwc->dis_split_quirk = device_property_read_bool(dev, > > >> "snps,dis-split-quirk"); > > >> > > >> + dwc->gadget_keep_connect_sys_sleep = device_property_read_bool(dev, > > >> + "snps,gadget-keep-connect-sys-sleep"); > > >> + > > >> dwc->lpm_nyet_threshold = lpm_nyet_threshold; > > >> dwc->tx_de_emphasis = tx_de_emphasis; > > >> > > >> @@ -2027,14 +2030,20 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > > >> { > > >> unsigned long flags; > > >> u32 reg; > > >> + int ret; > > >> > > >> switch (dwc->current_dr_role) { > > >> case DWC3_GCTL_PRTCAP_DEVICE: > > >> if (pm_runtime_suspended(dwc->dev)) > > >> break; > > >> - dwc3_gadget_suspend(dwc); > > >> + ret = dwc3_gadget_suspend(dwc); > > >> + if (ret) { > > >> + dev_err(dwc->dev, "gadget not suspended: %d\n", ret); > > >> + return ret; > > >> + } > > >> synchronize_irq(dwc->irq_gadget); > > >> - dwc3_core_exit(dwc); > > >> + if(!dwc->gadget_keep_connect_sys_sleep) > > >> + dwc3_core_exit(dwc); > > >> break; > > >> case DWC3_GCTL_PRTCAP_HOST: > > >> if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) { > > >> @@ -2088,11 +2097,15 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) > > >> > > >> switch (dwc->current_dr_role) { > > >> case DWC3_GCTL_PRTCAP_DEVICE: > > >> - ret = dwc3_core_init_for_resume(dwc); > > >> - if (ret) > > >> - return ret; > > >> + if (!dwc->gadget_keep_connect_sys_sleep) > > >> + { > > >> + ret = dwc3_core_init_for_resume(dwc); > > >> + if (ret) > > >> + return ret; > > >> + > > >> + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); > > >> + } > > >> > > >> - dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); > > >> dwc3_gadget_resume(dwc); > > >> break; > > >> case DWC3_GCTL_PRTCAP_HOST: > > >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > > >> index 582ebd9cf9c2..f84bac815bed 100644 > > >> --- a/drivers/usb/dwc3/core.h > > >> +++ b/drivers/usb/dwc3/core.h > > >> @@ -1328,6 +1328,8 @@ struct dwc3 { > > >> unsigned dis_split_quirk:1; > > >> unsigned async_callbacks:1; > > >> > > >> + unsigned gadget_keep_connect_sys_sleep:1; > > >> + > > >> u16 imod_interval; > > >> > > >> int max_cfg_eps; > > >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > >> index 3c63fa97a680..8062e44f63f6 100644 > > >> --- a/drivers/usb/dwc3/gadget.c > > >> +++ b/drivers/usb/dwc3/gadget.c > > >> @@ -4572,12 +4572,23 @@ void dwc3_gadget_exit(struct dwc3 *dwc) > > >> int dwc3_gadget_suspend(struct dwc3 *dwc) > > >> { > > >> unsigned long flags; > > >> + int link_state; > > >> > > >> if (!dwc->gadget_driver) > > >> return 0; > > >> > > >> - dwc3_gadget_run_stop(dwc, false, false); > > >> + if (dwc->gadget_keep_connect_sys_sleep && dwc->connected) { > > >> + link_state = dwc3_gadget_get_link_state(dwc); > > >> + /* Prevent PM Sleep if not in U3/L2 */ > > >> + if (link_state != DWC3_LINK_STATE_U3) > > >> + return -EBUSY; > > >> + > > >> + /* don't stop/disconnect */ > > >> + dwc3_gadget_disable_irq(dwc); > > > > > > We shouldn't disable event interrupt here. What will happen if the > > > > Due to some reason, if I don't disable the event interrupts here then > > after USB resume the USB controller is malfunctioning. > > It no longer responds to any requests from Host. > > You should look into this. These events are important as they can tell > whether the host initiates resume. > > > > > > device is disconnected and reconnect to the host while the device is > > > still in system suspend? The host would not be able to communicate with > > > the device then. > > > > In the TI platform, The system is woken up on any VBUS/linestate change > > and in dwc3_gadget_resume we enable the events again and check for pending > > events. Is it pointless to check for pending events there? > > > > It seems fragile for the implementation to be dependent on platform > specific feature right? > > Also, what will happen in a typical case when the host puts the device > in suspend and initiates resume while the device is in system suspend > (and stay in suspend over a period of time)? There is no VBUS change. > There will be problem if host detects no response from device in time. > > Don't we need these events to wakeup the device? > We may not be able to suspend everything in system suspend for this case. I'm thinking of treating these events as if they are PME to wakeup the device, but they are not the same. It may not be simple to handle this. The lower layers may need to stay awake for the dwc3 to handle these events. Hm... it gets a bit complicated. Thanks, Thinh ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature 2023-03-21 19:05 ` Thinh Nguyen @ 2023-03-22 8:11 ` Roger Quadros 2023-03-22 17:31 ` Thinh Nguyen 0 siblings, 1 reply; 24+ messages in thread From: Roger Quadros @ 2023-03-22 8:11 UTC (permalink / raw) To: Thinh Nguyen Cc: stern, gregkh, vigneshr, srk, r-gunasekaran, linux-usb, linux-kernel On 21/03/2023 21:05, Thinh Nguyen wrote: > On Tue, Mar 21, 2023, Thinh Nguyen wrote: >> On Tue, Mar 21, 2023, Roger Quadros wrote: >>> Hi Thinh, >>> >>> On 20/03/2023 20:52, Thinh Nguyen wrote: >>>> Hi, >>>> >>>> On Mon, Mar 20, 2023, Roger Quadros wrote: >>>>> Implement 'snps,gadget-keep-connect-sys-sleep' property. >>>>> >>>>> Do not stop the gadget controller and disconnect if this >>>>> property is present and we are connected to a USB Host. >>>>> >>>>> Prevent System sleep if Gadget is not in USB suspend. >>>>> >>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org> >>>>> --- >>>>> drivers/usb/dwc3/core.c | 25 +++++++++++++++++++------ >>>>> drivers/usb/dwc3/core.h | 2 ++ >>>>> drivers/usb/dwc3/gadget.c | 25 +++++++++++++++++++++++-- >>>>> 3 files changed, 44 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>>>> index 476b63618511..a47bbaa27302 100644 >>>>> --- a/drivers/usb/dwc3/core.c >>>>> +++ b/drivers/usb/dwc3/core.c >>>>> @@ -1575,6 +1575,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) >>>>> dwc->dis_split_quirk = device_property_read_bool(dev, >>>>> "snps,dis-split-quirk"); >>>>> >>>>> + dwc->gadget_keep_connect_sys_sleep = device_property_read_bool(dev, >>>>> + "snps,gadget-keep-connect-sys-sleep"); >>>>> + >>>>> dwc->lpm_nyet_threshold = lpm_nyet_threshold; >>>>> dwc->tx_de_emphasis = tx_de_emphasis; >>>>> >>>>> @@ -2027,14 +2030,20 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) >>>>> { >>>>> unsigned long flags; >>>>> u32 reg; >>>>> + int ret; >>>>> >>>>> switch (dwc->current_dr_role) { >>>>> case DWC3_GCTL_PRTCAP_DEVICE: >>>>> if (pm_runtime_suspended(dwc->dev)) >>>>> break; >>>>> - dwc3_gadget_suspend(dwc); >>>>> + ret = dwc3_gadget_suspend(dwc); >>>>> + if (ret) { >>>>> + dev_err(dwc->dev, "gadget not suspended: %d\n", ret); >>>>> + return ret; >>>>> + } >>>>> synchronize_irq(dwc->irq_gadget); >>>>> - dwc3_core_exit(dwc); >>>>> + if(!dwc->gadget_keep_connect_sys_sleep) >>>>> + dwc3_core_exit(dwc); >>>>> break; >>>>> case DWC3_GCTL_PRTCAP_HOST: >>>>> if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) { >>>>> @@ -2088,11 +2097,15 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) >>>>> >>>>> switch (dwc->current_dr_role) { >>>>> case DWC3_GCTL_PRTCAP_DEVICE: >>>>> - ret = dwc3_core_init_for_resume(dwc); >>>>> - if (ret) >>>>> - return ret; >>>>> + if (!dwc->gadget_keep_connect_sys_sleep) >>>>> + { >>>>> + ret = dwc3_core_init_for_resume(dwc); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); >>>>> + } >>>>> >>>>> - dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); >>>>> dwc3_gadget_resume(dwc); >>>>> break; >>>>> case DWC3_GCTL_PRTCAP_HOST: >>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >>>>> index 582ebd9cf9c2..f84bac815bed 100644 >>>>> --- a/drivers/usb/dwc3/core.h >>>>> +++ b/drivers/usb/dwc3/core.h >>>>> @@ -1328,6 +1328,8 @@ struct dwc3 { >>>>> unsigned dis_split_quirk:1; >>>>> unsigned async_callbacks:1; >>>>> >>>>> + unsigned gadget_keep_connect_sys_sleep:1; >>>>> + >>>>> u16 imod_interval; >>>>> >>>>> int max_cfg_eps; >>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>>>> index 3c63fa97a680..8062e44f63f6 100644 >>>>> --- a/drivers/usb/dwc3/gadget.c >>>>> +++ b/drivers/usb/dwc3/gadget.c >>>>> @@ -4572,12 +4572,23 @@ void dwc3_gadget_exit(struct dwc3 *dwc) >>>>> int dwc3_gadget_suspend(struct dwc3 *dwc) >>>>> { >>>>> unsigned long flags; >>>>> + int link_state; >>>>> >>>>> if (!dwc->gadget_driver) >>>>> return 0; >>>>> >>>>> - dwc3_gadget_run_stop(dwc, false, false); >>>>> + if (dwc->gadget_keep_connect_sys_sleep && dwc->connected) { >>>>> + link_state = dwc3_gadget_get_link_state(dwc); >>>>> + /* Prevent PM Sleep if not in U3/L2 */ >>>>> + if (link_state != DWC3_LINK_STATE_U3) >>>>> + return -EBUSY; >>>>> + >>>>> + /* don't stop/disconnect */ >>>>> + dwc3_gadget_disable_irq(dwc); >>>> >>>> We shouldn't disable event interrupt here. What will happen if the >>> >>> Due to some reason, if I don't disable the event interrupts here then >>> after USB resume the USB controller is malfunctioning. >>> It no longer responds to any requests from Host. >> >> You should look into this. These events are important as they can tell >> whether the host initiates resume. >> >>> >>>> device is disconnected and reconnect to the host while the device is >>>> still in system suspend? The host would not be able to communicate with >>>> the device then. >>> >>> In the TI platform, The system is woken up on any VBUS/linestate change >>> and in dwc3_gadget_resume we enable the events again and check for pending >>> events. Is it pointless to check for pending events there? >>> >> >> It seems fragile for the implementation to be dependent on platform >> specific feature right? >> >> Also, what will happen in a typical case when the host puts the device >> in suspend and initiates resume while the device is in system suspend >> (and stay in suspend over a period of time)? There is no VBUS change. >> There will be problem if host detects no response from device in time. >> >> Don't we need these events to wakeup the device? That's why the TI implementation has line-state change detection to detect a USB resume. We are doing a out-of-band wake-up. The wake up events are configured in the wrapper driver (dwc3-am62.c). Do you know of any dwc3 implementation that uses in-band mechanism to wake up the System. i.e. it relies on events enabled in DEVTEN register? >> > > We may not be able to suspend everything in system suspend for this > case. I'm thinking of treating these events as if they are PME to wakeup > the device, but they are not the same. It may not be simple to handle > this. The lower layers may need to stay awake for the dwc3 to handle > these events. Hm... it gets a bit complicated. As we are going into suspend, we are not really in a position to handle any (DEVTEN) events till we have fully resumed. So yes, we need to rely on platform specific implementation to wake the System on any USB event. cheers, -roger ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature 2023-03-22 8:11 ` Roger Quadros @ 2023-03-22 17:31 ` Thinh Nguyen 2023-03-23 2:17 ` Thinh Nguyen 0 siblings, 1 reply; 24+ messages in thread From: Thinh Nguyen @ 2023-03-22 17:31 UTC (permalink / raw) To: Roger Quadros Cc: Thinh Nguyen, stern, gregkh, vigneshr, srk, r-gunasekaran, linux-usb, linux-kernel On Wed, Mar 22, 2023, Roger Quadros wrote: > On 21/03/2023 21:05, Thinh Nguyen wrote: > > On Tue, Mar 21, 2023, Thinh Nguyen wrote: > >> On Tue, Mar 21, 2023, Roger Quadros wrote: > >>> Hi Thinh, > >>> > >>> On 20/03/2023 20:52, Thinh Nguyen wrote: > >>>> Hi, > >>>> > >>>> On Mon, Mar 20, 2023, Roger Quadros wrote: > >>>>> Implement 'snps,gadget-keep-connect-sys-sleep' property. > >>>>> > >>>>> Do not stop the gadget controller and disconnect if this > >>>>> property is present and we are connected to a USB Host. > >>>>> > >>>>> Prevent System sleep if Gadget is not in USB suspend. > >>>>> > >>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org> > >>>>> --- > >>>>> drivers/usb/dwc3/core.c | 25 +++++++++++++++++++------ > >>>>> drivers/usb/dwc3/core.h | 2 ++ > >>>>> drivers/usb/dwc3/gadget.c | 25 +++++++++++++++++++++++-- > >>>>> 3 files changed, 44 insertions(+), 8 deletions(-) > >>>>> > >>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > >>>>> index 476b63618511..a47bbaa27302 100644 > >>>>> --- a/drivers/usb/dwc3/core.c > >>>>> +++ b/drivers/usb/dwc3/core.c > >>>>> @@ -1575,6 +1575,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) > >>>>> dwc->dis_split_quirk = device_property_read_bool(dev, > >>>>> "snps,dis-split-quirk"); > >>>>> > >>>>> + dwc->gadget_keep_connect_sys_sleep = device_property_read_bool(dev, > >>>>> + "snps,gadget-keep-connect-sys-sleep"); > >>>>> + > >>>>> dwc->lpm_nyet_threshold = lpm_nyet_threshold; > >>>>> dwc->tx_de_emphasis = tx_de_emphasis; > >>>>> > >>>>> @@ -2027,14 +2030,20 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > >>>>> { > >>>>> unsigned long flags; > >>>>> u32 reg; > >>>>> + int ret; > >>>>> > >>>>> switch (dwc->current_dr_role) { > >>>>> case DWC3_GCTL_PRTCAP_DEVICE: > >>>>> if (pm_runtime_suspended(dwc->dev)) > >>>>> break; > >>>>> - dwc3_gadget_suspend(dwc); > >>>>> + ret = dwc3_gadget_suspend(dwc); > >>>>> + if (ret) { > >>>>> + dev_err(dwc->dev, "gadget not suspended: %d\n", ret); > >>>>> + return ret; > >>>>> + } > >>>>> synchronize_irq(dwc->irq_gadget); > >>>>> - dwc3_core_exit(dwc); > >>>>> + if(!dwc->gadget_keep_connect_sys_sleep) > >>>>> + dwc3_core_exit(dwc); > >>>>> break; > >>>>> case DWC3_GCTL_PRTCAP_HOST: > >>>>> if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) { > >>>>> @@ -2088,11 +2097,15 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) > >>>>> > >>>>> switch (dwc->current_dr_role) { > >>>>> case DWC3_GCTL_PRTCAP_DEVICE: > >>>>> - ret = dwc3_core_init_for_resume(dwc); > >>>>> - if (ret) > >>>>> - return ret; > >>>>> + if (!dwc->gadget_keep_connect_sys_sleep) > >>>>> + { > >>>>> + ret = dwc3_core_init_for_resume(dwc); > >>>>> + if (ret) > >>>>> + return ret; > >>>>> + > >>>>> + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); > >>>>> + } > >>>>> > >>>>> - dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); > >>>>> dwc3_gadget_resume(dwc); > >>>>> break; > >>>>> case DWC3_GCTL_PRTCAP_HOST: > >>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > >>>>> index 582ebd9cf9c2..f84bac815bed 100644 > >>>>> --- a/drivers/usb/dwc3/core.h > >>>>> +++ b/drivers/usb/dwc3/core.h > >>>>> @@ -1328,6 +1328,8 @@ struct dwc3 { > >>>>> unsigned dis_split_quirk:1; > >>>>> unsigned async_callbacks:1; > >>>>> > >>>>> + unsigned gadget_keep_connect_sys_sleep:1; > >>>>> + > >>>>> u16 imod_interval; > >>>>> > >>>>> int max_cfg_eps; > >>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > >>>>> index 3c63fa97a680..8062e44f63f6 100644 > >>>>> --- a/drivers/usb/dwc3/gadget.c > >>>>> +++ b/drivers/usb/dwc3/gadget.c > >>>>> @@ -4572,12 +4572,23 @@ void dwc3_gadget_exit(struct dwc3 *dwc) > >>>>> int dwc3_gadget_suspend(struct dwc3 *dwc) > >>>>> { > >>>>> unsigned long flags; > >>>>> + int link_state; > >>>>> > >>>>> if (!dwc->gadget_driver) > >>>>> return 0; > >>>>> > >>>>> - dwc3_gadget_run_stop(dwc, false, false); > >>>>> + if (dwc->gadget_keep_connect_sys_sleep && dwc->connected) { > >>>>> + link_state = dwc3_gadget_get_link_state(dwc); > >>>>> + /* Prevent PM Sleep if not in U3/L2 */ > >>>>> + if (link_state != DWC3_LINK_STATE_U3) > >>>>> + return -EBUSY; > >>>>> + > >>>>> + /* don't stop/disconnect */ > >>>>> + dwc3_gadget_disable_irq(dwc); > >>>> > >>>> We shouldn't disable event interrupt here. What will happen if the > >>> > >>> Due to some reason, if I don't disable the event interrupts here then > >>> after USB resume the USB controller is malfunctioning. > >>> It no longer responds to any requests from Host. > >> > >> You should look into this. These events are important as they can tell > >> whether the host initiates resume. > >> > >>> > >>>> device is disconnected and reconnect to the host while the device is > >>>> still in system suspend? The host would not be able to communicate with > >>>> the device then. > >>> > >>> In the TI platform, The system is woken up on any VBUS/linestate change > >>> and in dwc3_gadget_resume we enable the events again and check for pending > >>> events. Is it pointless to check for pending events there? > >>> > >> > >> It seems fragile for the implementation to be dependent on platform > >> specific feature right? > >> > >> Also, what will happen in a typical case when the host puts the device > >> in suspend and initiates resume while the device is in system suspend > >> (and stay in suspend over a period of time)? There is no VBUS change. > >> There will be problem if host detects no response from device in time. > >> > >> Don't we need these events to wakeup the device? > > That's why the TI implementation has line-state change detection to > detect a USB resume. We are doing a out-of-band wake-up. The wake up > events are configured in the wrapper driver (dwc3-am62.c). > > Do you know of any dwc3 implementation that uses in-band mechanism > to wake up the System. i.e. it relies on events enabled in DEVTEN register? > We rely on PME. The PME is generated from the PMU of the usb controller when it detects a resume. If your platform supports hibernation and if the resume signal is connected to the lower layer power manager of your device, then you can wakeup the system one level at a time. For example, if your device is a pci device, that wakeup signal would tie to the pci power manager, waking up the pci layer before waking up the core of the usb controller. That's how the host wakes up the host system (e.g. from remote wakeup). For this to work, we expect something similar on the device side. > >> > > > > We may not be able to suspend everything in system suspend for this > > case. I'm thinking of treating these events as if they are PME to wakeup > > the device, but they are not the same. It may not be simple to handle > > this. The lower layers may need to stay awake for the dwc3 to handle > > these events. Hm... it gets a bit complicated. > > As we are going into suspend, we are not really in a position to handle any > (DEVTEN) events till we have fully resumed. > So yes, we need to rely on platform specific implementation to wake > the System on any USB event. > You may be able to detect vbus change through the connector controller. However, the usb controller is the one that detects host resume. What platform specific implementation do you have outside of the usb controller do you have to get around that? I'm not sure if your platform supports hibernation or if the PME signal on your platform can wakeup the system, but currently dwc3 driver doesn't handle hibernation (device side). If there's no hibernation, there's no PME. BR, Thinh ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature 2023-03-22 17:31 ` Thinh Nguyen @ 2023-03-23 2:17 ` Thinh Nguyen 2023-03-23 9:29 ` Roger Quadros 0 siblings, 1 reply; 24+ messages in thread From: Thinh Nguyen @ 2023-03-23 2:17 UTC (permalink / raw) To: Roger Quadros Cc: stern, gregkh, vigneshr, srk, r-gunasekaran, linux-usb, linux-kernel On Wed, Mar 22, 2023, Thinh Nguyen wrote: > On Wed, Mar 22, 2023, Roger Quadros wrote: > > On 21/03/2023 21:05, Thinh Nguyen wrote: > > > On Tue, Mar 21, 2023, Thinh Nguyen wrote: > > >> On Tue, Mar 21, 2023, Roger Quadros wrote: > > >>> Hi Thinh, > > >>> > > >>> On 20/03/2023 20:52, Thinh Nguyen wrote: > > >>>> Hi, > > >>>> > > >>>> On Mon, Mar 20, 2023, Roger Quadros wrote: > > >>>>> Implement 'snps,gadget-keep-connect-sys-sleep' property. > > >>>>> > > >>>>> Do not stop the gadget controller and disconnect if this > > >>>>> property is present and we are connected to a USB Host. > > >>>>> > > >>>>> Prevent System sleep if Gadget is not in USB suspend. > > >>>>> > > >>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org> > > >>>>> --- > > >>>>> drivers/usb/dwc3/core.c | 25 +++++++++++++++++++------ > > >>>>> drivers/usb/dwc3/core.h | 2 ++ > > >>>>> drivers/usb/dwc3/gadget.c | 25 +++++++++++++++++++++++-- > > >>>>> 3 files changed, 44 insertions(+), 8 deletions(-) > > >>>>> > > >>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > >>>>> index 476b63618511..a47bbaa27302 100644 > > >>>>> --- a/drivers/usb/dwc3/core.c > > >>>>> +++ b/drivers/usb/dwc3/core.c > > >>>>> @@ -1575,6 +1575,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) > > >>>>> dwc->dis_split_quirk = device_property_read_bool(dev, > > >>>>> "snps,dis-split-quirk"); > > >>>>> > > >>>>> + dwc->gadget_keep_connect_sys_sleep = device_property_read_bool(dev, > > >>>>> + "snps,gadget-keep-connect-sys-sleep"); > > >>>>> + > > >>>>> dwc->lpm_nyet_threshold = lpm_nyet_threshold; > > >>>>> dwc->tx_de_emphasis = tx_de_emphasis; > > >>>>> > > >>>>> @@ -2027,14 +2030,20 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > > >>>>> { > > >>>>> unsigned long flags; > > >>>>> u32 reg; > > >>>>> + int ret; > > >>>>> > > >>>>> switch (dwc->current_dr_role) { > > >>>>> case DWC3_GCTL_PRTCAP_DEVICE: > > >>>>> if (pm_runtime_suspended(dwc->dev)) > > >>>>> break; > > >>>>> - dwc3_gadget_suspend(dwc); > > >>>>> + ret = dwc3_gadget_suspend(dwc); > > >>>>> + if (ret) { > > >>>>> + dev_err(dwc->dev, "gadget not suspended: %d\n", ret); > > >>>>> + return ret; > > >>>>> + } > > >>>>> synchronize_irq(dwc->irq_gadget); > > >>>>> - dwc3_core_exit(dwc); > > >>>>> + if(!dwc->gadget_keep_connect_sys_sleep) > > >>>>> + dwc3_core_exit(dwc); > > >>>>> break; > > >>>>> case DWC3_GCTL_PRTCAP_HOST: > > >>>>> if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) { > > >>>>> @@ -2088,11 +2097,15 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) > > >>>>> > > >>>>> switch (dwc->current_dr_role) { > > >>>>> case DWC3_GCTL_PRTCAP_DEVICE: > > >>>>> - ret = dwc3_core_init_for_resume(dwc); > > >>>>> - if (ret) > > >>>>> - return ret; > > >>>>> + if (!dwc->gadget_keep_connect_sys_sleep) > > >>>>> + { > > >>>>> + ret = dwc3_core_init_for_resume(dwc); > > >>>>> + if (ret) > > >>>>> + return ret; > > >>>>> + > > >>>>> + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); > > >>>>> + } > > >>>>> > > >>>>> - dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); > > >>>>> dwc3_gadget_resume(dwc); > > >>>>> break; > > >>>>> case DWC3_GCTL_PRTCAP_HOST: > > >>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > > >>>>> index 582ebd9cf9c2..f84bac815bed 100644 > > >>>>> --- a/drivers/usb/dwc3/core.h > > >>>>> +++ b/drivers/usb/dwc3/core.h > > >>>>> @@ -1328,6 +1328,8 @@ struct dwc3 { > > >>>>> unsigned dis_split_quirk:1; > > >>>>> unsigned async_callbacks:1; > > >>>>> > > >>>>> + unsigned gadget_keep_connect_sys_sleep:1; > > >>>>> + > > >>>>> u16 imod_interval; > > >>>>> > > >>>>> int max_cfg_eps; > > >>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > > >>>>> index 3c63fa97a680..8062e44f63f6 100644 > > >>>>> --- a/drivers/usb/dwc3/gadget.c > > >>>>> +++ b/drivers/usb/dwc3/gadget.c > > >>>>> @@ -4572,12 +4572,23 @@ void dwc3_gadget_exit(struct dwc3 *dwc) > > >>>>> int dwc3_gadget_suspend(struct dwc3 *dwc) > > >>>>> { > > >>>>> unsigned long flags; > > >>>>> + int link_state; > > >>>>> > > >>>>> if (!dwc->gadget_driver) > > >>>>> return 0; > > >>>>> > > >>>>> - dwc3_gadget_run_stop(dwc, false, false); > > >>>>> + if (dwc->gadget_keep_connect_sys_sleep && dwc->connected) { > > >>>>> + link_state = dwc3_gadget_get_link_state(dwc); > > >>>>> + /* Prevent PM Sleep if not in U3/L2 */ > > >>>>> + if (link_state != DWC3_LINK_STATE_U3) > > >>>>> + return -EBUSY; > > >>>>> + > > >>>>> + /* don't stop/disconnect */ > > >>>>> + dwc3_gadget_disable_irq(dwc); > > >>>> > > >>>> We shouldn't disable event interrupt here. What will happen if the > > >>> > > >>> Due to some reason, if I don't disable the event interrupts here then > > >>> after USB resume the USB controller is malfunctioning. > > >>> It no longer responds to any requests from Host. > > >> > > >> You should look into this. These events are important as they can tell > > >> whether the host initiates resume. > > >> > > >>> > > >>>> device is disconnected and reconnect to the host while the device is > > >>>> still in system suspend? The host would not be able to communicate with > > >>>> the device then. > > >>> > > >>> In the TI platform, The system is woken up on any VBUS/linestate change > > >>> and in dwc3_gadget_resume we enable the events again and check for pending > > >>> events. Is it pointless to check for pending events there? > > >>> > > >> > > >> It seems fragile for the implementation to be dependent on platform > > >> specific feature right? > > >> > > >> Also, what will happen in a typical case when the host puts the device > > >> in suspend and initiates resume while the device is in system suspend > > >> (and stay in suspend over a period of time)? There is no VBUS change. > > >> There will be problem if host detects no response from device in time. > > >> > > >> Don't we need these events to wakeup the device? > > > > That's why the TI implementation has line-state change detection to > > detect a USB resume. We are doing a out-of-band wake-up. The wake up > > events are configured in the wrapper driver (dwc3-am62.c). > > > > Do you know of any dwc3 implementation that uses in-band mechanism > > to wake up the System. i.e. it relies on events enabled in DEVTEN register? > > > > We rely on PME. The PME is generated from the PMU of the usb controller > when it detects a resume. If your platform supports hibernation and if > the resume signal is connected to the lower layer power manager of your > device, then you can wakeup the system one level at a time. For example, > if your device is a pci device, that wakeup signal would tie to the pci > power manager, waking up the pci layer before waking up the core of the > usb controller. That's how the host wakes up the host system (e.g. from > remote wakeup). For this to work, we expect something similar on the > device side. > > > >> > > > > > > We may not be able to suspend everything in system suspend for this > > > case. I'm thinking of treating these events as if they are PME to wakeup > > > the device, but they are not the same. It may not be simple to handle > > > this. The lower layers may need to stay awake for the dwc3 to handle > > > these events. Hm... it gets a bit complicated. > > > > As we are going into suspend, we are not really in a position to handle any > > (DEVTEN) events till we have fully resumed. > > So yes, we need to rely on platform specific implementation to wake > > the System on any USB event. > > > > You may be able to detect vbus change through the connector controller. > However, the usb controller is the one that detects host resume. What > platform specific implementation do you have outside of the usb > controller do you have to get around that? > > I'm not sure if your platform supports hibernation or if the PME signal > on your platform can wakeup the system, but currently dwc3 driver > doesn't handle hibernation (device side). If there's no hibernation, > there's no PME. > Actually, I think the dwc3 core is still on during system suspend for you right? Then I think we can use the wakeup event to wakeup system suspend on host resume? You can ignore about PME in this case. You may need to look into what needs stay awake to allow for handling of the dwc3 event. BR, Thinh ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature 2023-03-23 2:17 ` Thinh Nguyen @ 2023-03-23 9:29 ` Roger Quadros 2023-03-23 20:51 ` Thinh Nguyen 0 siblings, 1 reply; 24+ messages in thread From: Roger Quadros @ 2023-03-23 9:29 UTC (permalink / raw) To: Thinh Nguyen Cc: stern, gregkh, vigneshr, srk, r-gunasekaran, linux-usb, linux-kernel On 23/03/2023 04:17, Thinh Nguyen wrote: > On Wed, Mar 22, 2023, Thinh Nguyen wrote: >> On Wed, Mar 22, 2023, Roger Quadros wrote: >>> On 21/03/2023 21:05, Thinh Nguyen wrote: >>>> On Tue, Mar 21, 2023, Thinh Nguyen wrote: >>>>> On Tue, Mar 21, 2023, Roger Quadros wrote: >>>>>> Hi Thinh, >>>>>> >>>>>> On 20/03/2023 20:52, Thinh Nguyen wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On Mon, Mar 20, 2023, Roger Quadros wrote: >>>>>>>> Implement 'snps,gadget-keep-connect-sys-sleep' property. >>>>>>>> >>>>>>>> Do not stop the gadget controller and disconnect if this >>>>>>>> property is present and we are connected to a USB Host. >>>>>>>> >>>>>>>> Prevent System sleep if Gadget is not in USB suspend. >>>>>>>> >>>>>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org> >>>>>>>> --- >>>>>>>> drivers/usb/dwc3/core.c | 25 +++++++++++++++++++------ >>>>>>>> drivers/usb/dwc3/core.h | 2 ++ >>>>>>>> drivers/usb/dwc3/gadget.c | 25 +++++++++++++++++++++++-- >>>>>>>> 3 files changed, 44 insertions(+), 8 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>>>>>>> index 476b63618511..a47bbaa27302 100644 >>>>>>>> --- a/drivers/usb/dwc3/core.c >>>>>>>> +++ b/drivers/usb/dwc3/core.c >>>>>>>> @@ -1575,6 +1575,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) >>>>>>>> dwc->dis_split_quirk = device_property_read_bool(dev, >>>>>>>> "snps,dis-split-quirk"); >>>>>>>> >>>>>>>> + dwc->gadget_keep_connect_sys_sleep = device_property_read_bool(dev, >>>>>>>> + "snps,gadget-keep-connect-sys-sleep"); >>>>>>>> + >>>>>>>> dwc->lpm_nyet_threshold = lpm_nyet_threshold; >>>>>>>> dwc->tx_de_emphasis = tx_de_emphasis; >>>>>>>> >>>>>>>> @@ -2027,14 +2030,20 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) >>>>>>>> { >>>>>>>> unsigned long flags; >>>>>>>> u32 reg; >>>>>>>> + int ret; >>>>>>>> >>>>>>>> switch (dwc->current_dr_role) { >>>>>>>> case DWC3_GCTL_PRTCAP_DEVICE: >>>>>>>> if (pm_runtime_suspended(dwc->dev)) >>>>>>>> break; >>>>>>>> - dwc3_gadget_suspend(dwc); >>>>>>>> + ret = dwc3_gadget_suspend(dwc); >>>>>>>> + if (ret) { >>>>>>>> + dev_err(dwc->dev, "gadget not suspended: %d\n", ret); >>>>>>>> + return ret; >>>>>>>> + } >>>>>>>> synchronize_irq(dwc->irq_gadget); >>>>>>>> - dwc3_core_exit(dwc); >>>>>>>> + if(!dwc->gadget_keep_connect_sys_sleep) >>>>>>>> + dwc3_core_exit(dwc); >>>>>>>> break; >>>>>>>> case DWC3_GCTL_PRTCAP_HOST: >>>>>>>> if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) { >>>>>>>> @@ -2088,11 +2097,15 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) >>>>>>>> >>>>>>>> switch (dwc->current_dr_role) { >>>>>>>> case DWC3_GCTL_PRTCAP_DEVICE: >>>>>>>> - ret = dwc3_core_init_for_resume(dwc); >>>>>>>> - if (ret) >>>>>>>> - return ret; >>>>>>>> + if (!dwc->gadget_keep_connect_sys_sleep) >>>>>>>> + { >>>>>>>> + ret = dwc3_core_init_for_resume(dwc); >>>>>>>> + if (ret) >>>>>>>> + return ret; >>>>>>>> + >>>>>>>> + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); >>>>>>>> + } >>>>>>>> >>>>>>>> - dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); >>>>>>>> dwc3_gadget_resume(dwc); >>>>>>>> break; >>>>>>>> case DWC3_GCTL_PRTCAP_HOST: >>>>>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >>>>>>>> index 582ebd9cf9c2..f84bac815bed 100644 >>>>>>>> --- a/drivers/usb/dwc3/core.h >>>>>>>> +++ b/drivers/usb/dwc3/core.h >>>>>>>> @@ -1328,6 +1328,8 @@ struct dwc3 { >>>>>>>> unsigned dis_split_quirk:1; >>>>>>>> unsigned async_callbacks:1; >>>>>>>> >>>>>>>> + unsigned gadget_keep_connect_sys_sleep:1; >>>>>>>> + >>>>>>>> u16 imod_interval; >>>>>>>> >>>>>>>> int max_cfg_eps; >>>>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>>>>>>> index 3c63fa97a680..8062e44f63f6 100644 >>>>>>>> --- a/drivers/usb/dwc3/gadget.c >>>>>>>> +++ b/drivers/usb/dwc3/gadget.c >>>>>>>> @@ -4572,12 +4572,23 @@ void dwc3_gadget_exit(struct dwc3 *dwc) >>>>>>>> int dwc3_gadget_suspend(struct dwc3 *dwc) >>>>>>>> { >>>>>>>> unsigned long flags; >>>>>>>> + int link_state; >>>>>>>> >>>>>>>> if (!dwc->gadget_driver) >>>>>>>> return 0; >>>>>>>> >>>>>>>> - dwc3_gadget_run_stop(dwc, false, false); >>>>>>>> + if (dwc->gadget_keep_connect_sys_sleep && dwc->connected) { >>>>>>>> + link_state = dwc3_gadget_get_link_state(dwc); >>>>>>>> + /* Prevent PM Sleep if not in U3/L2 */ >>>>>>>> + if (link_state != DWC3_LINK_STATE_U3) >>>>>>>> + return -EBUSY; >>>>>>>> + >>>>>>>> + /* don't stop/disconnect */ >>>>>>>> + dwc3_gadget_disable_irq(dwc); >>>>>>> >>>>>>> We shouldn't disable event interrupt here. What will happen if the >>>>>> >>>>>> Due to some reason, if I don't disable the event interrupts here then >>>>>> after USB resume the USB controller is malfunctioning. >>>>>> It no longer responds to any requests from Host. >>>>> >>>>> You should look into this. These events are important as they can tell >>>>> whether the host initiates resume. >>>>> >>>>>> >>>>>>> device is disconnected and reconnect to the host while the device is >>>>>>> still in system suspend? The host would not be able to communicate with >>>>>>> the device then. >>>>>> >>>>>> In the TI platform, The system is woken up on any VBUS/linestate change >>>>>> and in dwc3_gadget_resume we enable the events again and check for pending >>>>>> events. Is it pointless to check for pending events there? >>>>>> >>>>> >>>>> It seems fragile for the implementation to be dependent on platform >>>>> specific feature right? >>>>> >>>>> Also, what will happen in a typical case when the host puts the device >>>>> in suspend and initiates resume while the device is in system suspend >>>>> (and stay in suspend over a period of time)? There is no VBUS change. >>>>> There will be problem if host detects no response from device in time. >>>>> >>>>> Don't we need these events to wakeup the device? >>> >>> That's why the TI implementation has line-state change detection to >>> detect a USB resume. We are doing a out-of-band wake-up. The wake up >>> events are configured in the wrapper driver (dwc3-am62.c). >>> >>> Do you know of any dwc3 implementation that uses in-band mechanism >>> to wake up the System. i.e. it relies on events enabled in DEVTEN register? >>> >> >> We rely on PME. The PME is generated from the PMU of the usb controller >> when it detects a resume. If your platform supports hibernation and if >> the resume signal is connected to the lower layer power manager of your >> device, then you can wakeup the system one level at a time. For example, >> if your device is a pci device, that wakeup signal would tie to the pci >> power manager, waking up the pci layer before waking up the core of the >> usb controller. That's how the host wakes up the host system (e.g. from >> remote wakeup). For this to work, we expect something similar on the >> device side. >> >>>>> >>>> >>>> We may not be able to suspend everything in system suspend for this >>>> case. I'm thinking of treating these events as if they are PME to wakeup >>>> the device, but they are not the same. It may not be simple to handle >>>> this. The lower layers may need to stay awake for the dwc3 to handle >>>> these events. Hm... it gets a bit complicated. >>> >>> As we are going into suspend, we are not really in a position to handle any >>> (DEVTEN) events till we have fully resumed. >>> So yes, we need to rely on platform specific implementation to wake >>> the System on any USB event. >>> >> >> You may be able to detect vbus change through the connector controller. >> However, the usb controller is the one that detects host resume. What >> platform specific implementation do you have outside of the usb >> controller do you have to get around that? >> >> I'm not sure if your platform supports hibernation or if the PME signal >> on your platform can wakeup the system, but currently dwc3 driver >> doesn't handle hibernation (device side). If there's no hibernation, >> there's no PME. No, in this TI SoC, hibernation feature is not supported in the dwc3 core. >> > > Actually, I think the dwc3 core is still on during system suspend for > you right? Then I think we can use the wakeup event to wakeup system > suspend on host resume? You can ignore about PME in this case. You may > need to look into what needs stay awake to allow for handling of the > dwc3 event. But in SoC deep-sleep state, all clocks to the dwc3 core are stopped. So I'm not sure if dwc3 events will work. cheers, -roger ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature 2023-03-23 9:29 ` Roger Quadros @ 2023-03-23 20:51 ` Thinh Nguyen 2023-03-31 11:05 ` Roger Quadros 0 siblings, 1 reply; 24+ messages in thread From: Thinh Nguyen @ 2023-03-23 20:51 UTC (permalink / raw) To: Roger Quadros Cc: Thinh Nguyen, stern, gregkh, vigneshr, srk, r-gunasekaran, linux-usb, linux-kernel On Thu, Mar 23, 2023, Roger Quadros wrote: > > > On 23/03/2023 04:17, Thinh Nguyen wrote: > > On Wed, Mar 22, 2023, Thinh Nguyen wrote: > >> On Wed, Mar 22, 2023, Roger Quadros wrote: > >>> On 21/03/2023 21:05, Thinh Nguyen wrote: > >>>> On Tue, Mar 21, 2023, Thinh Nguyen wrote: > >>>>> On Tue, Mar 21, 2023, Roger Quadros wrote: > >>>>>> Hi Thinh, > >>>>>> > >>>>>> On 20/03/2023 20:52, Thinh Nguyen wrote: > >>>>>>> Hi, > >>>>>>> > >>>>>>> On Mon, Mar 20, 2023, Roger Quadros wrote: > >>>>>>>> Implement 'snps,gadget-keep-connect-sys-sleep' property. > >>>>>>>> > >>>>>>>> Do not stop the gadget controller and disconnect if this > >>>>>>>> property is present and we are connected to a USB Host. > >>>>>>>> > >>>>>>>> Prevent System sleep if Gadget is not in USB suspend. > >>>>>>>> > >>>>>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org> > >>>>>>>> --- > >>>>>>>> drivers/usb/dwc3/core.c | 25 +++++++++++++++++++------ > >>>>>>>> drivers/usb/dwc3/core.h | 2 ++ > >>>>>>>> drivers/usb/dwc3/gadget.c | 25 +++++++++++++++++++++++-- > >>>>>>>> 3 files changed, 44 insertions(+), 8 deletions(-) > >>>>>>>> > >>>>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > >>>>>>>> index 476b63618511..a47bbaa27302 100644 > >>>>>>>> --- a/drivers/usb/dwc3/core.c > >>>>>>>> +++ b/drivers/usb/dwc3/core.c > >>>>>>>> @@ -1575,6 +1575,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) > >>>>>>>> dwc->dis_split_quirk = device_property_read_bool(dev, > >>>>>>>> "snps,dis-split-quirk"); > >>>>>>>> > >>>>>>>> + dwc->gadget_keep_connect_sys_sleep = device_property_read_bool(dev, > >>>>>>>> + "snps,gadget-keep-connect-sys-sleep"); > >>>>>>>> + > >>>>>>>> dwc->lpm_nyet_threshold = lpm_nyet_threshold; > >>>>>>>> dwc->tx_de_emphasis = tx_de_emphasis; > >>>>>>>> > >>>>>>>> @@ -2027,14 +2030,20 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > >>>>>>>> { > >>>>>>>> unsigned long flags; > >>>>>>>> u32 reg; > >>>>>>>> + int ret; > >>>>>>>> > >>>>>>>> switch (dwc->current_dr_role) { > >>>>>>>> case DWC3_GCTL_PRTCAP_DEVICE: > >>>>>>>> if (pm_runtime_suspended(dwc->dev)) > >>>>>>>> break; > >>>>>>>> - dwc3_gadget_suspend(dwc); > >>>>>>>> + ret = dwc3_gadget_suspend(dwc); > >>>>>>>> + if (ret) { > >>>>>>>> + dev_err(dwc->dev, "gadget not suspended: %d\n", ret); > >>>>>>>> + return ret; > >>>>>>>> + } > >>>>>>>> synchronize_irq(dwc->irq_gadget); > >>>>>>>> - dwc3_core_exit(dwc); > >>>>>>>> + if(!dwc->gadget_keep_connect_sys_sleep) > >>>>>>>> + dwc3_core_exit(dwc); > >>>>>>>> break; > >>>>>>>> case DWC3_GCTL_PRTCAP_HOST: > >>>>>>>> if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) { > >>>>>>>> @@ -2088,11 +2097,15 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) > >>>>>>>> > >>>>>>>> switch (dwc->current_dr_role) { > >>>>>>>> case DWC3_GCTL_PRTCAP_DEVICE: > >>>>>>>> - ret = dwc3_core_init_for_resume(dwc); > >>>>>>>> - if (ret) > >>>>>>>> - return ret; > >>>>>>>> + if (!dwc->gadget_keep_connect_sys_sleep) > >>>>>>>> + { > >>>>>>>> + ret = dwc3_core_init_for_resume(dwc); > >>>>>>>> + if (ret) > >>>>>>>> + return ret; > >>>>>>>> + > >>>>>>>> + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); > >>>>>>>> + } > >>>>>>>> > >>>>>>>> - dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); > >>>>>>>> dwc3_gadget_resume(dwc); > >>>>>>>> break; > >>>>>>>> case DWC3_GCTL_PRTCAP_HOST: > >>>>>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > >>>>>>>> index 582ebd9cf9c2..f84bac815bed 100644 > >>>>>>>> --- a/drivers/usb/dwc3/core.h > >>>>>>>> +++ b/drivers/usb/dwc3/core.h > >>>>>>>> @@ -1328,6 +1328,8 @@ struct dwc3 { > >>>>>>>> unsigned dis_split_quirk:1; > >>>>>>>> unsigned async_callbacks:1; > >>>>>>>> > >>>>>>>> + unsigned gadget_keep_connect_sys_sleep:1; > >>>>>>>> + > >>>>>>>> u16 imod_interval; > >>>>>>>> > >>>>>>>> int max_cfg_eps; > >>>>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > >>>>>>>> index 3c63fa97a680..8062e44f63f6 100644 > >>>>>>>> --- a/drivers/usb/dwc3/gadget.c > >>>>>>>> +++ b/drivers/usb/dwc3/gadget.c > >>>>>>>> @@ -4572,12 +4572,23 @@ void dwc3_gadget_exit(struct dwc3 *dwc) > >>>>>>>> int dwc3_gadget_suspend(struct dwc3 *dwc) > >>>>>>>> { > >>>>>>>> unsigned long flags; > >>>>>>>> + int link_state; > >>>>>>>> > >>>>>>>> if (!dwc->gadget_driver) > >>>>>>>> return 0; > >>>>>>>> > >>>>>>>> - dwc3_gadget_run_stop(dwc, false, false); > >>>>>>>> + if (dwc->gadget_keep_connect_sys_sleep && dwc->connected) { > >>>>>>>> + link_state = dwc3_gadget_get_link_state(dwc); > >>>>>>>> + /* Prevent PM Sleep if not in U3/L2 */ > >>>>>>>> + if (link_state != DWC3_LINK_STATE_U3) > >>>>>>>> + return -EBUSY; > >>>>>>>> + > >>>>>>>> + /* don't stop/disconnect */ > >>>>>>>> + dwc3_gadget_disable_irq(dwc); > >>>>>>> > >>>>>>> We shouldn't disable event interrupt here. What will happen if the > >>>>>> > >>>>>> Due to some reason, if I don't disable the event interrupts here then > >>>>>> after USB resume the USB controller is malfunctioning. > >>>>>> It no longer responds to any requests from Host. > >>>>> > >>>>> You should look into this. These events are important as they can tell > >>>>> whether the host initiates resume. > >>>>> > >>>>>> > >>>>>>> device is disconnected and reconnect to the host while the device is > >>>>>>> still in system suspend? The host would not be able to communicate with > >>>>>>> the device then. > >>>>>> > >>>>>> In the TI platform, The system is woken up on any VBUS/linestate change > >>>>>> and in dwc3_gadget_resume we enable the events again and check for pending > >>>>>> events. Is it pointless to check for pending events there? > >>>>>> > >>>>> > >>>>> It seems fragile for the implementation to be dependent on platform > >>>>> specific feature right? > >>>>> > >>>>> Also, what will happen in a typical case when the host puts the device > >>>>> in suspend and initiates resume while the device is in system suspend > >>>>> (and stay in suspend over a period of time)? There is no VBUS change. > >>>>> There will be problem if host detects no response from device in time. > >>>>> > >>>>> Don't we need these events to wakeup the device? > >>> > >>> That's why the TI implementation has line-state change detection to > >>> detect a USB resume. We are doing a out-of-band wake-up. The wake up > >>> events are configured in the wrapper driver (dwc3-am62.c). > >>> > >>> Do you know of any dwc3 implementation that uses in-band mechanism > >>> to wake up the System. i.e. it relies on events enabled in DEVTEN register? > >>> > >> > >> We rely on PME. The PME is generated from the PMU of the usb controller > >> when it detects a resume. If your platform supports hibernation and if > >> the resume signal is connected to the lower layer power manager of your > >> device, then you can wakeup the system one level at a time. For example, > >> if your device is a pci device, that wakeup signal would tie to the pci > >> power manager, waking up the pci layer before waking up the core of the > >> usb controller. That's how the host wakes up the host system (e.g. from > >> remote wakeup). For this to work, we expect something similar on the > >> device side. > >> > >>>>> > >>>> > >>>> We may not be able to suspend everything in system suspend for this > >>>> case. I'm thinking of treating these events as if they are PME to wakeup > >>>> the device, but they are not the same. It may not be simple to handle > >>>> this. The lower layers may need to stay awake for the dwc3 to handle > >>>> these events. Hm... it gets a bit complicated. > >>> > >>> As we are going into suspend, we are not really in a position to handle any > >>> (DEVTEN) events till we have fully resumed. > >>> So yes, we need to rely on platform specific implementation to wake > >>> the System on any USB event. > >>> > >> > >> You may be able to detect vbus change through the connector controller. > >> However, the usb controller is the one that detects host resume. What > >> platform specific implementation do you have outside of the usb > >> controller do you have to get around that? > >> > >> I'm not sure if your platform supports hibernation or if the PME signal > >> on your platform can wakeup the system, but currently dwc3 driver > >> doesn't handle hibernation (device side). If there's no hibernation, > >> there's no PME. > > No, in this TI SoC, hibernation feature is not supported in the dwc3 core. > > >> > > > > Actually, I think the dwc3 core is still on during system suspend for > > you right? Then I think we can use the wakeup event to wakeup system > > suspend on host resume? You can ignore about PME in this case. You may > > need to look into what needs stay awake to allow for handling of the > > dwc3 event. > > But in SoC deep-sleep state, all clocks to the dwc3 core are stopped. > So I'm not sure if dwc3 events will work. > Right, you need to keep those clocks running to detect host resume. There's still some power saving through the dwc3 controller's handling in suspend. You may have some limited power saving from other suspended devices on your setup. However, I don't think we can expect the platform to go into deep-sleep and also handle host resume. Thanks, Thinh ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature 2023-03-23 20:51 ` Thinh Nguyen @ 2023-03-31 11:05 ` Roger Quadros 2023-04-03 23:37 ` Thinh Nguyen 0 siblings, 1 reply; 24+ messages in thread From: Roger Quadros @ 2023-03-31 11:05 UTC (permalink / raw) To: Thinh Nguyen Cc: stern, gregkh, vigneshr, srk, r-gunasekaran, linux-usb, linux-kernel Hi, On 23/03/2023 22:51, Thinh Nguyen wrote: > On Thu, Mar 23, 2023, Roger Quadros wrote: >> >> >> On 23/03/2023 04:17, Thinh Nguyen wrote: >>> On Wed, Mar 22, 2023, Thinh Nguyen wrote: >>>> On Wed, Mar 22, 2023, Roger Quadros wrote: >>>>> On 21/03/2023 21:05, Thinh Nguyen wrote: >>>>>> On Tue, Mar 21, 2023, Thinh Nguyen wrote: >>>>>>> On Tue, Mar 21, 2023, Roger Quadros wrote: >>>>>>>> Hi Thinh, >>>>>>>> >>>>>>>> On 20/03/2023 20:52, Thinh Nguyen wrote: >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> On Mon, Mar 20, 2023, Roger Quadros wrote: >>>>>>>>>> Implement 'snps,gadget-keep-connect-sys-sleep' property. >>>>>>>>>> >>>>>>>>>> Do not stop the gadget controller and disconnect if this >>>>>>>>>> property is present and we are connected to a USB Host. >>>>>>>>>> >>>>>>>>>> Prevent System sleep if Gadget is not in USB suspend. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org> >>>>>>>>>> --- >>>>>>>>>> drivers/usb/dwc3/core.c | 25 +++++++++++++++++++------ >>>>>>>>>> drivers/usb/dwc3/core.h | 2 ++ >>>>>>>>>> drivers/usb/dwc3/gadget.c | 25 +++++++++++++++++++++++-- >>>>>>>>>> 3 files changed, 44 insertions(+), 8 deletions(-) >>>>>>>>>> >>>>>>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>>>>>>>>> index 476b63618511..a47bbaa27302 100644 >>>>>>>>>> --- a/drivers/usb/dwc3/core.c >>>>>>>>>> +++ b/drivers/usb/dwc3/core.c >>>>>>>>>> @@ -1575,6 +1575,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) >>>>>>>>>> dwc->dis_split_quirk = device_property_read_bool(dev, >>>>>>>>>> "snps,dis-split-quirk"); >>>>>>>>>> >>>>>>>>>> + dwc->gadget_keep_connect_sys_sleep = device_property_read_bool(dev, >>>>>>>>>> + "snps,gadget-keep-connect-sys-sleep"); >>>>>>>>>> + >>>>>>>>>> dwc->lpm_nyet_threshold = lpm_nyet_threshold; >>>>>>>>>> dwc->tx_de_emphasis = tx_de_emphasis; >>>>>>>>>> >>>>>>>>>> @@ -2027,14 +2030,20 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) >>>>>>>>>> { >>>>>>>>>> unsigned long flags; >>>>>>>>>> u32 reg; >>>>>>>>>> + int ret; >>>>>>>>>> >>>>>>>>>> switch (dwc->current_dr_role) { >>>>>>>>>> case DWC3_GCTL_PRTCAP_DEVICE: >>>>>>>>>> if (pm_runtime_suspended(dwc->dev)) >>>>>>>>>> break; >>>>>>>>>> - dwc3_gadget_suspend(dwc); >>>>>>>>>> + ret = dwc3_gadget_suspend(dwc); >>>>>>>>>> + if (ret) { >>>>>>>>>> + dev_err(dwc->dev, "gadget not suspended: %d\n", ret); >>>>>>>>>> + return ret; >>>>>>>>>> + } >>>>>>>>>> synchronize_irq(dwc->irq_gadget); >>>>>>>>>> - dwc3_core_exit(dwc); >>>>>>>>>> + if(!dwc->gadget_keep_connect_sys_sleep) >>>>>>>>>> + dwc3_core_exit(dwc); >>>>>>>>>> break; >>>>>>>>>> case DWC3_GCTL_PRTCAP_HOST: >>>>>>>>>> if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) { >>>>>>>>>> @@ -2088,11 +2097,15 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) >>>>>>>>>> >>>>>>>>>> switch (dwc->current_dr_role) { >>>>>>>>>> case DWC3_GCTL_PRTCAP_DEVICE: >>>>>>>>>> - ret = dwc3_core_init_for_resume(dwc); >>>>>>>>>> - if (ret) >>>>>>>>>> - return ret; >>>>>>>>>> + if (!dwc->gadget_keep_connect_sys_sleep) >>>>>>>>>> + { >>>>>>>>>> + ret = dwc3_core_init_for_resume(dwc); >>>>>>>>>> + if (ret) >>>>>>>>>> + return ret; >>>>>>>>>> + >>>>>>>>>> + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); >>>>>>>>>> + } >>>>>>>>>> >>>>>>>>>> - dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); >>>>>>>>>> dwc3_gadget_resume(dwc); >>>>>>>>>> break; >>>>>>>>>> case DWC3_GCTL_PRTCAP_HOST: >>>>>>>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >>>>>>>>>> index 582ebd9cf9c2..f84bac815bed 100644 >>>>>>>>>> --- a/drivers/usb/dwc3/core.h >>>>>>>>>> +++ b/drivers/usb/dwc3/core.h >>>>>>>>>> @@ -1328,6 +1328,8 @@ struct dwc3 { >>>>>>>>>> unsigned dis_split_quirk:1; >>>>>>>>>> unsigned async_callbacks:1; >>>>>>>>>> >>>>>>>>>> + unsigned gadget_keep_connect_sys_sleep:1; >>>>>>>>>> + >>>>>>>>>> u16 imod_interval; >>>>>>>>>> >>>>>>>>>> int max_cfg_eps; >>>>>>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>>>>>>>>> index 3c63fa97a680..8062e44f63f6 100644 >>>>>>>>>> --- a/drivers/usb/dwc3/gadget.c >>>>>>>>>> +++ b/drivers/usb/dwc3/gadget.c >>>>>>>>>> @@ -4572,12 +4572,23 @@ void dwc3_gadget_exit(struct dwc3 *dwc) >>>>>>>>>> int dwc3_gadget_suspend(struct dwc3 *dwc) >>>>>>>>>> { >>>>>>>>>> unsigned long flags; >>>>>>>>>> + int link_state; >>>>>>>>>> >>>>>>>>>> if (!dwc->gadget_driver) >>>>>>>>>> return 0; >>>>>>>>>> >>>>>>>>>> - dwc3_gadget_run_stop(dwc, false, false); >>>>>>>>>> + if (dwc->gadget_keep_connect_sys_sleep && dwc->connected) { >>>>>>>>>> + link_state = dwc3_gadget_get_link_state(dwc); >>>>>>>>>> + /* Prevent PM Sleep if not in U3/L2 */ >>>>>>>>>> + if (link_state != DWC3_LINK_STATE_U3) >>>>>>>>>> + return -EBUSY; >>>>>>>>>> + >>>>>>>>>> + /* don't stop/disconnect */ >>>>>>>>>> + dwc3_gadget_disable_irq(dwc); >>>>>>>>> >>>>>>>>> We shouldn't disable event interrupt here. What will happen if the >>>>>>>> >>>>>>>> Due to some reason, if I don't disable the event interrupts here then >>>>>>>> after USB resume the USB controller is malfunctioning. >>>>>>>> It no longer responds to any requests from Host. >>>>>>> >>>>>>> You should look into this. These events are important as they can tell >>>>>>> whether the host initiates resume. >>>>>>> >>>>>>>> >>>>>>>>> device is disconnected and reconnect to the host while the device is >>>>>>>>> still in system suspend? The host would not be able to communicate with >>>>>>>>> the device then. >>>>>>>> >>>>>>>> In the TI platform, The system is woken up on any VBUS/linestate change >>>>>>>> and in dwc3_gadget_resume we enable the events again and check for pending >>>>>>>> events. Is it pointless to check for pending events there? >>>>>>>> >>>>>>> >>>>>>> It seems fragile for the implementation to be dependent on platform >>>>>>> specific feature right? >>>>>>> >>>>>>> Also, what will happen in a typical case when the host puts the device >>>>>>> in suspend and initiates resume while the device is in system suspend >>>>>>> (and stay in suspend over a period of time)? There is no VBUS change. >>>>>>> There will be problem if host detects no response from device in time. >>>>>>> >>>>>>> Don't we need these events to wakeup the device? >>>>> >>>>> That's why the TI implementation has line-state change detection to >>>>> detect a USB resume. We are doing a out-of-band wake-up. The wake up >>>>> events are configured in the wrapper driver (dwc3-am62.c). >>>>> >>>>> Do you know of any dwc3 implementation that uses in-band mechanism >>>>> to wake up the System. i.e. it relies on events enabled in DEVTEN register? >>>>> >>>> >>>> We rely on PME. The PME is generated from the PMU of the usb controller >>>> when it detects a resume. If your platform supports hibernation and if >>>> the resume signal is connected to the lower layer power manager of your >>>> device, then you can wakeup the system one level at a time. For example, >>>> if your device is a pci device, that wakeup signal would tie to the pci >>>> power manager, waking up the pci layer before waking up the core of the >>>> usb controller. That's how the host wakes up the host system (e.g. from >>>> remote wakeup). For this to work, we expect something similar on the >>>> device side. >>>> >>>>>>> >>>>>> >>>>>> We may not be able to suspend everything in system suspend for this >>>>>> case. I'm thinking of treating these events as if they are PME to wakeup >>>>>> the device, but they are not the same. It may not be simple to handle >>>>>> this. The lower layers may need to stay awake for the dwc3 to handle >>>>>> these events. Hm... it gets a bit complicated. >>>>> >>>>> As we are going into suspend, we are not really in a position to handle any >>>>> (DEVTEN) events till we have fully resumed. >>>>> So yes, we need to rely on platform specific implementation to wake >>>>> the System on any USB event. >>>>> >>>> >>>> You may be able to detect vbus change through the connector controller. >>>> However, the usb controller is the one that detects host resume. What >>>> platform specific implementation do you have outside of the usb >>>> controller do you have to get around that? >>>> >>>> I'm not sure if your platform supports hibernation or if the PME signal >>>> on your platform can wakeup the system, but currently dwc3 driver >>>> doesn't handle hibernation (device side). If there's no hibernation, >>>> there's no PME. >> >> No, in this TI SoC, hibernation feature is not supported in the dwc3 core. >> >>>> >>> >>> Actually, I think the dwc3 core is still on during system suspend for >>> you right? Then I think we can use the wakeup event to wakeup system >>> suspend on host resume? You can ignore about PME in this case. You may >>> need to look into what needs stay awake to allow for handling of the >>> dwc3 event. >> >> But in SoC deep-sleep state, all clocks to the dwc3 core are stopped. >> So I'm not sure if dwc3 events will work. >> > > Right, you need to keep those clocks running to detect host resume. > There's still some power saving through the dwc3 controller's handling > in suspend. You may have some limited power saving from other suspended > devices on your setup. However, I don't think we can expect the platform > to go into deep-sleep and also handle host resume. Why not? if the PHY can detect the host resume and wake up the SoC it will work right? cheers, -roger ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature 2023-03-31 11:05 ` Roger Quadros @ 2023-04-03 23:37 ` Thinh Nguyen 2023-04-04 8:01 ` Roger Quadros 0 siblings, 1 reply; 24+ messages in thread From: Thinh Nguyen @ 2023-04-03 23:37 UTC (permalink / raw) To: Roger Quadros Cc: Thinh Nguyen, stern, gregkh, vigneshr, srk, r-gunasekaran, linux-usb, linux-kernel On Fri, Mar 31, 2023, Roger Quadros wrote: > Hi, > > On 23/03/2023 22:51, Thinh Nguyen wrote: > > On Thu, Mar 23, 2023, Roger Quadros wrote: > >> > >> > >> On 23/03/2023 04:17, Thinh Nguyen wrote: > >>> On Wed, Mar 22, 2023, Thinh Nguyen wrote: > >>>> On Wed, Mar 22, 2023, Roger Quadros wrote: > >>>>> On 21/03/2023 21:05, Thinh Nguyen wrote: > >>>>>> On Tue, Mar 21, 2023, Thinh Nguyen wrote: > >>>>>>> On Tue, Mar 21, 2023, Roger Quadros wrote: > >>>>>>>> Hi Thinh, > >>>>>>>> > >>>>>>>> On 20/03/2023 20:52, Thinh Nguyen wrote: > >>>>>>>>> Hi, > >>>>>>>>> > >>>>>>>>> On Mon, Mar 20, 2023, Roger Quadros wrote: > >>>>>>>>>> Implement 'snps,gadget-keep-connect-sys-sleep' property. > >>>>>>>>>> > >>>>>>>>>> Do not stop the gadget controller and disconnect if this > >>>>>>>>>> property is present and we are connected to a USB Host. > >>>>>>>>>> > >>>>>>>>>> Prevent System sleep if Gadget is not in USB suspend. > >>>>>>>>>> > >>>>>>>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org> > >>>>>>>>>> --- > >>>>>>>>>> drivers/usb/dwc3/core.c | 25 +++++++++++++++++++------ > >>>>>>>>>> drivers/usb/dwc3/core.h | 2 ++ > >>>>>>>>>> drivers/usb/dwc3/gadget.c | 25 +++++++++++++++++++++++-- > >>>>>>>>>> 3 files changed, 44 insertions(+), 8 deletions(-) > >>>>>>>>>> > >>>>>>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > >>>>>>>>>> index 476b63618511..a47bbaa27302 100644 > >>>>>>>>>> --- a/drivers/usb/dwc3/core.c > >>>>>>>>>> +++ b/drivers/usb/dwc3/core.c > >>>>>>>>>> @@ -1575,6 +1575,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) > >>>>>>>>>> dwc->dis_split_quirk = device_property_read_bool(dev, > >>>>>>>>>> "snps,dis-split-quirk"); > >>>>>>>>>> > >>>>>>>>>> + dwc->gadget_keep_connect_sys_sleep = device_property_read_bool(dev, > >>>>>>>>>> + "snps,gadget-keep-connect-sys-sleep"); > >>>>>>>>>> + > >>>>>>>>>> dwc->lpm_nyet_threshold = lpm_nyet_threshold; > >>>>>>>>>> dwc->tx_de_emphasis = tx_de_emphasis; > >>>>>>>>>> > >>>>>>>>>> @@ -2027,14 +2030,20 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > >>>>>>>>>> { > >>>>>>>>>> unsigned long flags; > >>>>>>>>>> u32 reg; > >>>>>>>>>> + int ret; > >>>>>>>>>> > >>>>>>>>>> switch (dwc->current_dr_role) { > >>>>>>>>>> case DWC3_GCTL_PRTCAP_DEVICE: > >>>>>>>>>> if (pm_runtime_suspended(dwc->dev)) > >>>>>>>>>> break; > >>>>>>>>>> - dwc3_gadget_suspend(dwc); > >>>>>>>>>> + ret = dwc3_gadget_suspend(dwc); > >>>>>>>>>> + if (ret) { > >>>>>>>>>> + dev_err(dwc->dev, "gadget not suspended: %d\n", ret); > >>>>>>>>>> + return ret; > >>>>>>>>>> + } > >>>>>>>>>> synchronize_irq(dwc->irq_gadget); > >>>>>>>>>> - dwc3_core_exit(dwc); > >>>>>>>>>> + if(!dwc->gadget_keep_connect_sys_sleep) > >>>>>>>>>> + dwc3_core_exit(dwc); > >>>>>>>>>> break; > >>>>>>>>>> case DWC3_GCTL_PRTCAP_HOST: > >>>>>>>>>> if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) { > >>>>>>>>>> @@ -2088,11 +2097,15 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) > >>>>>>>>>> > >>>>>>>>>> switch (dwc->current_dr_role) { > >>>>>>>>>> case DWC3_GCTL_PRTCAP_DEVICE: > >>>>>>>>>> - ret = dwc3_core_init_for_resume(dwc); > >>>>>>>>>> - if (ret) > >>>>>>>>>> - return ret; > >>>>>>>>>> + if (!dwc->gadget_keep_connect_sys_sleep) > >>>>>>>>>> + { > >>>>>>>>>> + ret = dwc3_core_init_for_resume(dwc); > >>>>>>>>>> + if (ret) > >>>>>>>>>> + return ret; > >>>>>>>>>> + > >>>>>>>>>> + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); > >>>>>>>>>> + } > >>>>>>>>>> > >>>>>>>>>> - dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); > >>>>>>>>>> dwc3_gadget_resume(dwc); > >>>>>>>>>> break; > >>>>>>>>>> case DWC3_GCTL_PRTCAP_HOST: > >>>>>>>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > >>>>>>>>>> index 582ebd9cf9c2..f84bac815bed 100644 > >>>>>>>>>> --- a/drivers/usb/dwc3/core.h > >>>>>>>>>> +++ b/drivers/usb/dwc3/core.h > >>>>>>>>>> @@ -1328,6 +1328,8 @@ struct dwc3 { > >>>>>>>>>> unsigned dis_split_quirk:1; > >>>>>>>>>> unsigned async_callbacks:1; > >>>>>>>>>> > >>>>>>>>>> + unsigned gadget_keep_connect_sys_sleep:1; > >>>>>>>>>> + > >>>>>>>>>> u16 imod_interval; > >>>>>>>>>> > >>>>>>>>>> int max_cfg_eps; > >>>>>>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > >>>>>>>>>> index 3c63fa97a680..8062e44f63f6 100644 > >>>>>>>>>> --- a/drivers/usb/dwc3/gadget.c > >>>>>>>>>> +++ b/drivers/usb/dwc3/gadget.c > >>>>>>>>>> @@ -4572,12 +4572,23 @@ void dwc3_gadget_exit(struct dwc3 *dwc) > >>>>>>>>>> int dwc3_gadget_suspend(struct dwc3 *dwc) > >>>>>>>>>> { > >>>>>>>>>> unsigned long flags; > >>>>>>>>>> + int link_state; > >>>>>>>>>> > >>>>>>>>>> if (!dwc->gadget_driver) > >>>>>>>>>> return 0; > >>>>>>>>>> > >>>>>>>>>> - dwc3_gadget_run_stop(dwc, false, false); > >>>>>>>>>> + if (dwc->gadget_keep_connect_sys_sleep && dwc->connected) { > >>>>>>>>>> + link_state = dwc3_gadget_get_link_state(dwc); > >>>>>>>>>> + /* Prevent PM Sleep if not in U3/L2 */ > >>>>>>>>>> + if (link_state != DWC3_LINK_STATE_U3) > >>>>>>>>>> + return -EBUSY; > >>>>>>>>>> + > >>>>>>>>>> + /* don't stop/disconnect */ > >>>>>>>>>> + dwc3_gadget_disable_irq(dwc); > >>>>>>>>> > >>>>>>>>> We shouldn't disable event interrupt here. What will happen if the > >>>>>>>> > >>>>>>>> Due to some reason, if I don't disable the event interrupts here then > >>>>>>>> after USB resume the USB controller is malfunctioning. > >>>>>>>> It no longer responds to any requests from Host. > >>>>>>> > >>>>>>> You should look into this. These events are important as they can tell > >>>>>>> whether the host initiates resume. > >>>>>>> > >>>>>>>> > >>>>>>>>> device is disconnected and reconnect to the host while the device is > >>>>>>>>> still in system suspend? The host would not be able to communicate with > >>>>>>>>> the device then. > >>>>>>>> > >>>>>>>> In the TI platform, The system is woken up on any VBUS/linestate change > >>>>>>>> and in dwc3_gadget_resume we enable the events again and check for pending > >>>>>>>> events. Is it pointless to check for pending events there? > >>>>>>>> > >>>>>>> > >>>>>>> It seems fragile for the implementation to be dependent on platform > >>>>>>> specific feature right? > >>>>>>> > >>>>>>> Also, what will happen in a typical case when the host puts the device > >>>>>>> in suspend and initiates resume while the device is in system suspend > >>>>>>> (and stay in suspend over a period of time)? There is no VBUS change. > >>>>>>> There will be problem if host detects no response from device in time. > >>>>>>> > >>>>>>> Don't we need these events to wakeup the device? > >>>>> > >>>>> That's why the TI implementation has line-state change detection to > >>>>> detect a USB resume. We are doing a out-of-band wake-up. The wake up > >>>>> events are configured in the wrapper driver (dwc3-am62.c). > >>>>> > >>>>> Do you know of any dwc3 implementation that uses in-band mechanism > >>>>> to wake up the System. i.e. it relies on events enabled in DEVTEN register? > >>>>> > >>>> > >>>> We rely on PME. The PME is generated from the PMU of the usb controller > >>>> when it detects a resume. If your platform supports hibernation and if > >>>> the resume signal is connected to the lower layer power manager of your > >>>> device, then you can wakeup the system one level at a time. For example, > >>>> if your device is a pci device, that wakeup signal would tie to the pci > >>>> power manager, waking up the pci layer before waking up the core of the > >>>> usb controller. That's how the host wakes up the host system (e.g. from > >>>> remote wakeup). For this to work, we expect something similar on the > >>>> device side. > >>>> > >>>>>>> > >>>>>> > >>>>>> We may not be able to suspend everything in system suspend for this > >>>>>> case. I'm thinking of treating these events as if they are PME to wakeup > >>>>>> the device, but they are not the same. It may not be simple to handle > >>>>>> this. The lower layers may need to stay awake for the dwc3 to handle > >>>>>> these events. Hm... it gets a bit complicated. > >>>>> > >>>>> As we are going into suspend, we are not really in a position to handle any > >>>>> (DEVTEN) events till we have fully resumed. > >>>>> So yes, we need to rely on platform specific implementation to wake > >>>>> the System on any USB event. > >>>>> > >>>> > >>>> You may be able to detect vbus change through the connector controller. > >>>> However, the usb controller is the one that detects host resume. What > >>>> platform specific implementation do you have outside of the usb > >>>> controller do you have to get around that? > >>>> > >>>> I'm not sure if your platform supports hibernation or if the PME signal > >>>> on your platform can wakeup the system, but currently dwc3 driver > >>>> doesn't handle hibernation (device side). If there's no hibernation, > >>>> there's no PME. > >> > >> No, in this TI SoC, hibernation feature is not supported in the dwc3 core. > >> > >>>> > >>> > >>> Actually, I think the dwc3 core is still on during system suspend for > >>> you right? Then I think we can use the wakeup event to wakeup system > >>> suspend on host resume? You can ignore about PME in this case. You may > >>> need to look into what needs stay awake to allow for handling of the > >>> dwc3 event. > >> > >> But in SoC deep-sleep state, all clocks to the dwc3 core are stopped. > >> So I'm not sure if dwc3 events will work. > >> > > > > Right, you need to keep those clocks running to detect host resume. > > There's still some power saving through the dwc3 controller's handling > > in suspend. You may have some limited power saving from other suspended > > devices on your setup. However, I don't think we can expect the platform > > to go into deep-sleep and also handle host resume. > > Why not? if the PHY can detect the host resume and wake up the SoC it will > work right? > Hm... I supposed it may be possible. But it may need some unconventional design? The dwc3 controller is currently registered to the phy. For that to work, your phy needs to be able to talk to both the dwc3 controller and some other controller (equivalent to dwc3 PMU) that manages power/interrupt. The dwc3 controller would need to relinquish control to this other phy controller on suspend. The phy driver would then be able to assert interrupt waking up the system on resume sigal detection, which in turn relinquish control to the dwc3 controller. All of this has to work while the phy signaling remains synchronized with the dwc3 controller. From the patches you sent, I don't see the changes necesssary for this to work. If there is something that I'm missing, please also note it or add it here to the series. Thanks, Thinh ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature 2023-04-03 23:37 ` Thinh Nguyen @ 2023-04-04 8:01 ` Roger Quadros 2023-04-04 21:53 ` Thinh Nguyen 0 siblings, 1 reply; 24+ messages in thread From: Roger Quadros @ 2023-04-04 8:01 UTC (permalink / raw) To: Thinh Nguyen Cc: stern, gregkh, vigneshr, srk, r-gunasekaran, linux-usb, linux-kernel On 04/04/2023 02:37, Thinh Nguyen wrote: > On Fri, Mar 31, 2023, Roger Quadros wrote: >> Hi, >> >> On 23/03/2023 22:51, Thinh Nguyen wrote: >>> On Thu, Mar 23, 2023, Roger Quadros wrote: >>>> >>>> >>>> On 23/03/2023 04:17, Thinh Nguyen wrote: >>>>> On Wed, Mar 22, 2023, Thinh Nguyen wrote: >>>>>> On Wed, Mar 22, 2023, Roger Quadros wrote: >>>>>>> On 21/03/2023 21:05, Thinh Nguyen wrote: >>>>>>>> On Tue, Mar 21, 2023, Thinh Nguyen wrote: >>>>>>>>> On Tue, Mar 21, 2023, Roger Quadros wrote: >>>>>>>>>> Hi Thinh, >>>>>>>>>> >>>>>>>>>> On 20/03/2023 20:52, Thinh Nguyen wrote: >>>>>>>>>>> Hi, >>>>>>>>>>> >>>>>>>>>>> On Mon, Mar 20, 2023, Roger Quadros wrote: >>>>>>>>>>>> Implement 'snps,gadget-keep-connect-sys-sleep' property. >>>>>>>>>>>> >>>>>>>>>>>> Do not stop the gadget controller and disconnect if this >>>>>>>>>>>> property is present and we are connected to a USB Host. >>>>>>>>>>>> >>>>>>>>>>>> Prevent System sleep if Gadget is not in USB suspend. >>>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org> >>>>>>>>>>>> --- >>>>>>>>>>>> drivers/usb/dwc3/core.c | 25 +++++++++++++++++++------ >>>>>>>>>>>> drivers/usb/dwc3/core.h | 2 ++ >>>>>>>>>>>> drivers/usb/dwc3/gadget.c | 25 +++++++++++++++++++++++-- >>>>>>>>>>>> 3 files changed, 44 insertions(+), 8 deletions(-) >>>>>>>>>>>> >>>>>>>>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>>>>>>>>>>> index 476b63618511..a47bbaa27302 100644 >>>>>>>>>>>> --- a/drivers/usb/dwc3/core.c >>>>>>>>>>>> +++ b/drivers/usb/dwc3/core.c >>>>>>>>>>>> @@ -1575,6 +1575,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) >>>>>>>>>>>> dwc->dis_split_quirk = device_property_read_bool(dev, >>>>>>>>>>>> "snps,dis-split-quirk"); >>>>>>>>>>>> >>>>>>>>>>>> + dwc->gadget_keep_connect_sys_sleep = device_property_read_bool(dev, >>>>>>>>>>>> + "snps,gadget-keep-connect-sys-sleep"); >>>>>>>>>>>> + >>>>>>>>>>>> dwc->lpm_nyet_threshold = lpm_nyet_threshold; >>>>>>>>>>>> dwc->tx_de_emphasis = tx_de_emphasis; >>>>>>>>>>>> >>>>>>>>>>>> @@ -2027,14 +2030,20 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) >>>>>>>>>>>> { >>>>>>>>>>>> unsigned long flags; >>>>>>>>>>>> u32 reg; >>>>>>>>>>>> + int ret; >>>>>>>>>>>> >>>>>>>>>>>> switch (dwc->current_dr_role) { >>>>>>>>>>>> case DWC3_GCTL_PRTCAP_DEVICE: >>>>>>>>>>>> if (pm_runtime_suspended(dwc->dev)) >>>>>>>>>>>> break; >>>>>>>>>>>> - dwc3_gadget_suspend(dwc); >>>>>>>>>>>> + ret = dwc3_gadget_suspend(dwc); >>>>>>>>>>>> + if (ret) { >>>>>>>>>>>> + dev_err(dwc->dev, "gadget not suspended: %d\n", ret); >>>>>>>>>>>> + return ret; >>>>>>>>>>>> + } >>>>>>>>>>>> synchronize_irq(dwc->irq_gadget); >>>>>>>>>>>> - dwc3_core_exit(dwc); >>>>>>>>>>>> + if(!dwc->gadget_keep_connect_sys_sleep) >>>>>>>>>>>> + dwc3_core_exit(dwc); >>>>>>>>>>>> break; >>>>>>>>>>>> case DWC3_GCTL_PRTCAP_HOST: >>>>>>>>>>>> if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) { >>>>>>>>>>>> @@ -2088,11 +2097,15 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) >>>>>>>>>>>> >>>>>>>>>>>> switch (dwc->current_dr_role) { >>>>>>>>>>>> case DWC3_GCTL_PRTCAP_DEVICE: >>>>>>>>>>>> - ret = dwc3_core_init_for_resume(dwc); >>>>>>>>>>>> - if (ret) >>>>>>>>>>>> - return ret; >>>>>>>>>>>> + if (!dwc->gadget_keep_connect_sys_sleep) >>>>>>>>>>>> + { >>>>>>>>>>>> + ret = dwc3_core_init_for_resume(dwc); >>>>>>>>>>>> + if (ret) >>>>>>>>>>>> + return ret; >>>>>>>>>>>> + >>>>>>>>>>>> + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); >>>>>>>>>>>> + } >>>>>>>>>>>> >>>>>>>>>>>> - dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); >>>>>>>>>>>> dwc3_gadget_resume(dwc); >>>>>>>>>>>> break; >>>>>>>>>>>> case DWC3_GCTL_PRTCAP_HOST: >>>>>>>>>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >>>>>>>>>>>> index 582ebd9cf9c2..f84bac815bed 100644 >>>>>>>>>>>> --- a/drivers/usb/dwc3/core.h >>>>>>>>>>>> +++ b/drivers/usb/dwc3/core.h >>>>>>>>>>>> @@ -1328,6 +1328,8 @@ struct dwc3 { >>>>>>>>>>>> unsigned dis_split_quirk:1; >>>>>>>>>>>> unsigned async_callbacks:1; >>>>>>>>>>>> >>>>>>>>>>>> + unsigned gadget_keep_connect_sys_sleep:1; >>>>>>>>>>>> + >>>>>>>>>>>> u16 imod_interval; >>>>>>>>>>>> >>>>>>>>>>>> int max_cfg_eps; >>>>>>>>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>>>>>>>>>>> index 3c63fa97a680..8062e44f63f6 100644 >>>>>>>>>>>> --- a/drivers/usb/dwc3/gadget.c >>>>>>>>>>>> +++ b/drivers/usb/dwc3/gadget.c >>>>>>>>>>>> @@ -4572,12 +4572,23 @@ void dwc3_gadget_exit(struct dwc3 *dwc) >>>>>>>>>>>> int dwc3_gadget_suspend(struct dwc3 *dwc) >>>>>>>>>>>> { >>>>>>>>>>>> unsigned long flags; >>>>>>>>>>>> + int link_state; >>>>>>>>>>>> >>>>>>>>>>>> if (!dwc->gadget_driver) >>>>>>>>>>>> return 0; >>>>>>>>>>>> >>>>>>>>>>>> - dwc3_gadget_run_stop(dwc, false, false); >>>>>>>>>>>> + if (dwc->gadget_keep_connect_sys_sleep && dwc->connected) { >>>>>>>>>>>> + link_state = dwc3_gadget_get_link_state(dwc); >>>>>>>>>>>> + /* Prevent PM Sleep if not in U3/L2 */ >>>>>>>>>>>> + if (link_state != DWC3_LINK_STATE_U3) >>>>>>>>>>>> + return -EBUSY; >>>>>>>>>>>> + >>>>>>>>>>>> + /* don't stop/disconnect */ >>>>>>>>>>>> + dwc3_gadget_disable_irq(dwc); >>>>>>>>>>> >>>>>>>>>>> We shouldn't disable event interrupt here. What will happen if the >>>>>>>>>> >>>>>>>>>> Due to some reason, if I don't disable the event interrupts here then >>>>>>>>>> after USB resume the USB controller is malfunctioning. >>>>>>>>>> It no longer responds to any requests from Host. >>>>>>>>> >>>>>>>>> You should look into this. These events are important as they can tell >>>>>>>>> whether the host initiates resume. >>>>>>>>> >>>>>>>>>> >>>>>>>>>>> device is disconnected and reconnect to the host while the device is >>>>>>>>>>> still in system suspend? The host would not be able to communicate with >>>>>>>>>>> the device then. >>>>>>>>>> >>>>>>>>>> In the TI platform, The system is woken up on any VBUS/linestate change >>>>>>>>>> and in dwc3_gadget_resume we enable the events again and check for pending >>>>>>>>>> events. Is it pointless to check for pending events there? >>>>>>>>>> >>>>>>>>> >>>>>>>>> It seems fragile for the implementation to be dependent on platform >>>>>>>>> specific feature right? >>>>>>>>> >>>>>>>>> Also, what will happen in a typical case when the host puts the device >>>>>>>>> in suspend and initiates resume while the device is in system suspend >>>>>>>>> (and stay in suspend over a period of time)? There is no VBUS change. >>>>>>>>> There will be problem if host detects no response from device in time. >>>>>>>>> >>>>>>>>> Don't we need these events to wakeup the device? >>>>>>> >>>>>>> That's why the TI implementation has line-state change detection to >>>>>>> detect a USB resume. We are doing a out-of-band wake-up. The wake up >>>>>>> events are configured in the wrapper driver (dwc3-am62.c). >>>>>>> >>>>>>> Do you know of any dwc3 implementation that uses in-band mechanism >>>>>>> to wake up the System. i.e. it relies on events enabled in DEVTEN register? >>>>>>> >>>>>> >>>>>> We rely on PME. The PME is generated from the PMU of the usb controller >>>>>> when it detects a resume. If your platform supports hibernation and if >>>>>> the resume signal is connected to the lower layer power manager of your >>>>>> device, then you can wakeup the system one level at a time. For example, >>>>>> if your device is a pci device, that wakeup signal would tie to the pci >>>>>> power manager, waking up the pci layer before waking up the core of the >>>>>> usb controller. That's how the host wakes up the host system (e.g. from >>>>>> remote wakeup). For this to work, we expect something similar on the >>>>>> device side. >>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> We may not be able to suspend everything in system suspend for this >>>>>>>> case. I'm thinking of treating these events as if they are PME to wakeup >>>>>>>> the device, but they are not the same. It may not be simple to handle >>>>>>>> this. The lower layers may need to stay awake for the dwc3 to handle >>>>>>>> these events. Hm... it gets a bit complicated. >>>>>>> >>>>>>> As we are going into suspend, we are not really in a position to handle any >>>>>>> (DEVTEN) events till we have fully resumed. >>>>>>> So yes, we need to rely on platform specific implementation to wake >>>>>>> the System on any USB event. >>>>>>> >>>>>> >>>>>> You may be able to detect vbus change through the connector controller. >>>>>> However, the usb controller is the one that detects host resume. What >>>>>> platform specific implementation do you have outside of the usb >>>>>> controller do you have to get around that? >>>>>> >>>>>> I'm not sure if your platform supports hibernation or if the PME signal >>>>>> on your platform can wakeup the system, but currently dwc3 driver >>>>>> doesn't handle hibernation (device side). If there's no hibernation, >>>>>> there's no PME. >>>> >>>> No, in this TI SoC, hibernation feature is not supported in the dwc3 core. >>>> >>>>>> >>>>> >>>>> Actually, I think the dwc3 core is still on during system suspend for >>>>> you right? Then I think we can use the wakeup event to wakeup system >>>>> suspend on host resume? You can ignore about PME in this case. You may >>>>> need to look into what needs stay awake to allow for handling of the >>>>> dwc3 event. >>>> >>>> But in SoC deep-sleep state, all clocks to the dwc3 core are stopped. >>>> So I'm not sure if dwc3 events will work. >>>> >>> >>> Right, you need to keep those clocks running to detect host resume. >>> There's still some power saving through the dwc3 controller's handling >>> in suspend. You may have some limited power saving from other suspended >>> devices on your setup. However, I don't think we can expect the platform >>> to go into deep-sleep and also handle host resume. >> >> Why not? if the PHY can detect the host resume and wake up the SoC it will >> work right? >> > > Hm... I supposed it may be possible. But it may need some unconventional > design? The dwc3 controller is currently registered to the phy. For that > to work, your phy needs to be able to talk to both the dwc3 controller > and some other controller (equivalent to dwc3 PMU) that manages > power/interrupt. The dwc3 controller would need to relinquish control to > this other phy controller on suspend. The phy driver would then be able > to assert interrupt waking up the system on resume sigal detection, > which in turn relinquish control to the dwc3 controller. All of this has > to work while the phy signaling remains synchronized with the dwc3 > controller. My understanding is that all this is taken care by PHY integration design with DWC3 core on the TI SoC. > > From the patches you sent, I don't see the changes necesssary for this > to work. If there is something that I'm missing, please also note it or > add it here to the series. There is nothing more as the details are taken care by PHY logic and necessary integration with DWC3. For the PHY wake-up programming details you have already checked this series [1]. [1] - https://lore.kernel.org/all/20230316131226.89540-1-rogerq@kernel.org/ cheers, -roger ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature 2023-04-04 8:01 ` Roger Quadros @ 2023-04-04 21:53 ` Thinh Nguyen 2023-04-05 8:56 ` Roger Quadros 0 siblings, 1 reply; 24+ messages in thread From: Thinh Nguyen @ 2023-04-04 21:53 UTC (permalink / raw) To: Roger Quadros Cc: Thinh Nguyen, stern, gregkh, vigneshr, srk, r-gunasekaran, linux-usb, linux-kernel On Tue, Apr 04, 2023, Roger Quadros wrote: > > > On 04/04/2023 02:37, Thinh Nguyen wrote: > > On Fri, Mar 31, 2023, Roger Quadros wrote: > >> Hi, > >> > >> On 23/03/2023 22:51, Thinh Nguyen wrote: > >>> On Thu, Mar 23, 2023, Roger Quadros wrote: > >>>> > >>>> > >>>> On 23/03/2023 04:17, Thinh Nguyen wrote: > >>>>> On Wed, Mar 22, 2023, Thinh Nguyen wrote: > >>>>>> On Wed, Mar 22, 2023, Roger Quadros wrote: > >>>>>>> On 21/03/2023 21:05, Thinh Nguyen wrote: > >>>>>>>> On Tue, Mar 21, 2023, Thinh Nguyen wrote: > >>>>>>>>> On Tue, Mar 21, 2023, Roger Quadros wrote: > >>>>>>>>>> Hi Thinh, > >>>>>>>>>> > >>>>>>>>>> On 20/03/2023 20:52, Thinh Nguyen wrote: > >>>>>>>>>>> Hi, > >>>>>>>>>>> > >>>>>>>>>>> On Mon, Mar 20, 2023, Roger Quadros wrote: > >>>>>>>>>>>> Implement 'snps,gadget-keep-connect-sys-sleep' property. > >>>>>>>>>>>> > >>>>>>>>>>>> Do not stop the gadget controller and disconnect if this > >>>>>>>>>>>> property is present and we are connected to a USB Host. > >>>>>>>>>>>> > >>>>>>>>>>>> Prevent System sleep if Gadget is not in USB suspend. > >>>>>>>>>>>> > >>>>>>>>>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org> > >>>>>>>>>>>> --- > >>>>>>>>>>>> drivers/usb/dwc3/core.c | 25 +++++++++++++++++++------ > >>>>>>>>>>>> drivers/usb/dwc3/core.h | 2 ++ > >>>>>>>>>>>> drivers/usb/dwc3/gadget.c | 25 +++++++++++++++++++++++-- > >>>>>>>>>>>> 3 files changed, 44 insertions(+), 8 deletions(-) > >>>>>>>>>>>> > >>>>>>>>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > >>>>>>>>>>>> index 476b63618511..a47bbaa27302 100644 > >>>>>>>>>>>> --- a/drivers/usb/dwc3/core.c > >>>>>>>>>>>> +++ b/drivers/usb/dwc3/core.c > >>>>>>>>>>>> @@ -1575,6 +1575,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) > >>>>>>>>>>>> dwc->dis_split_quirk = device_property_read_bool(dev, > >>>>>>>>>>>> "snps,dis-split-quirk"); > >>>>>>>>>>>> > >>>>>>>>>>>> + dwc->gadget_keep_connect_sys_sleep = device_property_read_bool(dev, > >>>>>>>>>>>> + "snps,gadget-keep-connect-sys-sleep"); > >>>>>>>>>>>> + > >>>>>>>>>>>> dwc->lpm_nyet_threshold = lpm_nyet_threshold; > >>>>>>>>>>>> dwc->tx_de_emphasis = tx_de_emphasis; > >>>>>>>>>>>> > >>>>>>>>>>>> @@ -2027,14 +2030,20 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > >>>>>>>>>>>> { > >>>>>>>>>>>> unsigned long flags; > >>>>>>>>>>>> u32 reg; > >>>>>>>>>>>> + int ret; > >>>>>>>>>>>> > >>>>>>>>>>>> switch (dwc->current_dr_role) { > >>>>>>>>>>>> case DWC3_GCTL_PRTCAP_DEVICE: > >>>>>>>>>>>> if (pm_runtime_suspended(dwc->dev)) > >>>>>>>>>>>> break; > >>>>>>>>>>>> - dwc3_gadget_suspend(dwc); > >>>>>>>>>>>> + ret = dwc3_gadget_suspend(dwc); > >>>>>>>>>>>> + if (ret) { > >>>>>>>>>>>> + dev_err(dwc->dev, "gadget not suspended: %d\n", ret); > >>>>>>>>>>>> + return ret; > >>>>>>>>>>>> + } > >>>>>>>>>>>> synchronize_irq(dwc->irq_gadget); > >>>>>>>>>>>> - dwc3_core_exit(dwc); > >>>>>>>>>>>> + if(!dwc->gadget_keep_connect_sys_sleep) > >>>>>>>>>>>> + dwc3_core_exit(dwc); > >>>>>>>>>>>> break; > >>>>>>>>>>>> case DWC3_GCTL_PRTCAP_HOST: > >>>>>>>>>>>> if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) { > >>>>>>>>>>>> @@ -2088,11 +2097,15 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) > >>>>>>>>>>>> > >>>>>>>>>>>> switch (dwc->current_dr_role) { > >>>>>>>>>>>> case DWC3_GCTL_PRTCAP_DEVICE: > >>>>>>>>>>>> - ret = dwc3_core_init_for_resume(dwc); > >>>>>>>>>>>> - if (ret) > >>>>>>>>>>>> - return ret; > >>>>>>>>>>>> + if (!dwc->gadget_keep_connect_sys_sleep) > >>>>>>>>>>>> + { > >>>>>>>>>>>> + ret = dwc3_core_init_for_resume(dwc); > >>>>>>>>>>>> + if (ret) > >>>>>>>>>>>> + return ret; > >>>>>>>>>>>> + > >>>>>>>>>>>> + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); > >>>>>>>>>>>> + } > >>>>>>>>>>>> > >>>>>>>>>>>> - dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); > >>>>>>>>>>>> dwc3_gadget_resume(dwc); > >>>>>>>>>>>> break; > >>>>>>>>>>>> case DWC3_GCTL_PRTCAP_HOST: > >>>>>>>>>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > >>>>>>>>>>>> index 582ebd9cf9c2..f84bac815bed 100644 > >>>>>>>>>>>> --- a/drivers/usb/dwc3/core.h > >>>>>>>>>>>> +++ b/drivers/usb/dwc3/core.h > >>>>>>>>>>>> @@ -1328,6 +1328,8 @@ struct dwc3 { > >>>>>>>>>>>> unsigned dis_split_quirk:1; > >>>>>>>>>>>> unsigned async_callbacks:1; > >>>>>>>>>>>> > >>>>>>>>>>>> + unsigned gadget_keep_connect_sys_sleep:1; > >>>>>>>>>>>> + > >>>>>>>>>>>> u16 imod_interval; > >>>>>>>>>>>> > >>>>>>>>>>>> int max_cfg_eps; > >>>>>>>>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > >>>>>>>>>>>> index 3c63fa97a680..8062e44f63f6 100644 > >>>>>>>>>>>> --- a/drivers/usb/dwc3/gadget.c > >>>>>>>>>>>> +++ b/drivers/usb/dwc3/gadget.c > >>>>>>>>>>>> @@ -4572,12 +4572,23 @@ void dwc3_gadget_exit(struct dwc3 *dwc) > >>>>>>>>>>>> int dwc3_gadget_suspend(struct dwc3 *dwc) > >>>>>>>>>>>> { > >>>>>>>>>>>> unsigned long flags; > >>>>>>>>>>>> + int link_state; > >>>>>>>>>>>> > >>>>>>>>>>>> if (!dwc->gadget_driver) > >>>>>>>>>>>> return 0; > >>>>>>>>>>>> > >>>>>>>>>>>> - dwc3_gadget_run_stop(dwc, false, false); > >>>>>>>>>>>> + if (dwc->gadget_keep_connect_sys_sleep && dwc->connected) { > >>>>>>>>>>>> + link_state = dwc3_gadget_get_link_state(dwc); > >>>>>>>>>>>> + /* Prevent PM Sleep if not in U3/L2 */ > >>>>>>>>>>>> + if (link_state != DWC3_LINK_STATE_U3) > >>>>>>>>>>>> + return -EBUSY; > >>>>>>>>>>>> + > >>>>>>>>>>>> + /* don't stop/disconnect */ > >>>>>>>>>>>> + dwc3_gadget_disable_irq(dwc); > >>>>>>>>>>> > >>>>>>>>>>> We shouldn't disable event interrupt here. What will happen if the > >>>>>>>>>> > >>>>>>>>>> Due to some reason, if I don't disable the event interrupts here then > >>>>>>>>>> after USB resume the USB controller is malfunctioning. > >>>>>>>>>> It no longer responds to any requests from Host. > >>>>>>>>> > >>>>>>>>> You should look into this. These events are important as they can tell > >>>>>>>>> whether the host initiates resume. > >>>>>>>>> > >>>>>>>>>> > >>>>>>>>>>> device is disconnected and reconnect to the host while the device is > >>>>>>>>>>> still in system suspend? The host would not be able to communicate with > >>>>>>>>>>> the device then. > >>>>>>>>>> > >>>>>>>>>> In the TI platform, The system is woken up on any VBUS/linestate change > >>>>>>>>>> and in dwc3_gadget_resume we enable the events again and check for pending > >>>>>>>>>> events. Is it pointless to check for pending events there? > >>>>>>>>>> > >>>>>>>>> > >>>>>>>>> It seems fragile for the implementation to be dependent on platform > >>>>>>>>> specific feature right? > >>>>>>>>> > >>>>>>>>> Also, what will happen in a typical case when the host puts the device > >>>>>>>>> in suspend and initiates resume while the device is in system suspend > >>>>>>>>> (and stay in suspend over a period of time)? There is no VBUS change. > >>>>>>>>> There will be problem if host detects no response from device in time. > >>>>>>>>> > >>>>>>>>> Don't we need these events to wakeup the device? > >>>>>>> > >>>>>>> That's why the TI implementation has line-state change detection to > >>>>>>> detect a USB resume. We are doing a out-of-band wake-up. The wake up > >>>>>>> events are configured in the wrapper driver (dwc3-am62.c). > >>>>>>> > >>>>>>> Do you know of any dwc3 implementation that uses in-band mechanism > >>>>>>> to wake up the System. i.e. it relies on events enabled in DEVTEN register? > >>>>>>> > >>>>>> > >>>>>> We rely on PME. The PME is generated from the PMU of the usb controller > >>>>>> when it detects a resume. If your platform supports hibernation and if > >>>>>> the resume signal is connected to the lower layer power manager of your > >>>>>> device, then you can wakeup the system one level at a time. For example, > >>>>>> if your device is a pci device, that wakeup signal would tie to the pci > >>>>>> power manager, waking up the pci layer before waking up the core of the > >>>>>> usb controller. That's how the host wakes up the host system (e.g. from > >>>>>> remote wakeup). For this to work, we expect something similar on the > >>>>>> device side. > >>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>>> We may not be able to suspend everything in system suspend for this > >>>>>>>> case. I'm thinking of treating these events as if they are PME to wakeup > >>>>>>>> the device, but they are not the same. It may not be simple to handle > >>>>>>>> this. The lower layers may need to stay awake for the dwc3 to handle > >>>>>>>> these events. Hm... it gets a bit complicated. > >>>>>>> > >>>>>>> As we are going into suspend, we are not really in a position to handle any > >>>>>>> (DEVTEN) events till we have fully resumed. > >>>>>>> So yes, we need to rely on platform specific implementation to wake > >>>>>>> the System on any USB event. > >>>>>>> > >>>>>> > >>>>>> You may be able to detect vbus change through the connector controller. > >>>>>> However, the usb controller is the one that detects host resume. What > >>>>>> platform specific implementation do you have outside of the usb > >>>>>> controller do you have to get around that? > >>>>>> > >>>>>> I'm not sure if your platform supports hibernation or if the PME signal > >>>>>> on your platform can wakeup the system, but currently dwc3 driver > >>>>>> doesn't handle hibernation (device side). If there's no hibernation, > >>>>>> there's no PME. > >>>> > >>>> No, in this TI SoC, hibernation feature is not supported in the dwc3 core. > >>>> > >>>>>> > >>>>> > >>>>> Actually, I think the dwc3 core is still on during system suspend for > >>>>> you right? Then I think we can use the wakeup event to wakeup system > >>>>> suspend on host resume? You can ignore about PME in this case. You may > >>>>> need to look into what needs stay awake to allow for handling of the > >>>>> dwc3 event. > >>>> > >>>> But in SoC deep-sleep state, all clocks to the dwc3 core are stopped. > >>>> So I'm not sure if dwc3 events will work. > >>>> > >>> > >>> Right, you need to keep those clocks running to detect host resume. > >>> There's still some power saving through the dwc3 controller's handling > >>> in suspend. You may have some limited power saving from other suspended > >>> devices on your setup. However, I don't think we can expect the platform > >>> to go into deep-sleep and also handle host resume. > >> > >> Why not? if the PHY can detect the host resume and wake up the SoC it will > >> work right? > >> > > > > Hm... I supposed it may be possible. But it may need some unconventional > > design? The dwc3 controller is currently registered to the phy. For that > > to work, your phy needs to be able to talk to both the dwc3 controller > > and some other controller (equivalent to dwc3 PMU) that manages > > power/interrupt. The dwc3 controller would need to relinquish control to > > this other phy controller on suspend. The phy driver would then be able > > to assert interrupt waking up the system on resume sigal detection, > > which in turn relinquish control to the dwc3 controller. All of this has > > to work while the phy signaling remains synchronized with the dwc3 > > controller. > > My understanding is that all this is taken care by PHY integration design with > DWC3 core on the TI SoC. > > > > > From the patches you sent, I don't see the changes necesssary for this > > to work. If there is something that I'm missing, please also note it or > > add it here to the series. > > There is nothing more as the details are taken care by PHY logic and > necessary integration with DWC3. > > For the PHY wake-up programming details you have already checked this series [1]. > > [1] - https://urldefense.com/v3/__https://lore.kernel.org/all/20230316131226.89540-1-rogerq@kernel.org/__;!!A4F2R9G_pg!ayqpaWwnWIO7SKktk4kI2EJLwDTIv2nYYCgBGHUlt56KTzeYkDEdq2Q5ZvZFsuCWIlcXGET230YY5J3y_sHV$ > I may have misunderstood your platform implementation. My understanding is that it can only detect VBUS and that it can only resume on VBUS valid. Does the "LINESTATE" here gets asserted if say there's a LFPS detection? Thanks, Thinh ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature 2023-04-04 21:53 ` Thinh Nguyen @ 2023-04-05 8:56 ` Roger Quadros 2023-04-06 1:38 ` Thinh Nguyen 0 siblings, 1 reply; 24+ messages in thread From: Roger Quadros @ 2023-04-05 8:56 UTC (permalink / raw) To: Thinh Nguyen Cc: stern, gregkh, vigneshr, srk, r-gunasekaran, linux-usb, linux-kernel On 05/04/2023 00:53, Thinh Nguyen wrote: > On Tue, Apr 04, 2023, Roger Quadros wrote: >> >> >> On 04/04/2023 02:37, Thinh Nguyen wrote: >>> On Fri, Mar 31, 2023, Roger Quadros wrote: >>>> Hi, >>>> >>>> On 23/03/2023 22:51, Thinh Nguyen wrote: >>>>> On Thu, Mar 23, 2023, Roger Quadros wrote: >>>>>> >>>>>> >>>>>> On 23/03/2023 04:17, Thinh Nguyen wrote: >>>>>>> On Wed, Mar 22, 2023, Thinh Nguyen wrote: >>>>>>>> On Wed, Mar 22, 2023, Roger Quadros wrote: >>>>>>>>> On 21/03/2023 21:05, Thinh Nguyen wrote: >>>>>>>>>> On Tue, Mar 21, 2023, Thinh Nguyen wrote: >>>>>>>>>>> On Tue, Mar 21, 2023, Roger Quadros wrote: >>>>>>>>>>>> Hi Thinh, >>>>>>>>>>>> >>>>>>>>>>>> On 20/03/2023 20:52, Thinh Nguyen wrote: >>>>>>>>>>>>> Hi, >>>>>>>>>>>>> >>>>>>>>>>>>> On Mon, Mar 20, 2023, Roger Quadros wrote: >>>>>>>>>>>>>> Implement 'snps,gadget-keep-connect-sys-sleep' property. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Do not stop the gadget controller and disconnect if this >>>>>>>>>>>>>> property is present and we are connected to a USB Host. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Prevent System sleep if Gadget is not in USB suspend. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org> >>>>>>>>>>>>>> --- >>>>>>>>>>>>>> drivers/usb/dwc3/core.c | 25 +++++++++++++++++++------ >>>>>>>>>>>>>> drivers/usb/dwc3/core.h | 2 ++ >>>>>>>>>>>>>> drivers/usb/dwc3/gadget.c | 25 +++++++++++++++++++++++-- >>>>>>>>>>>>>> 3 files changed, 44 insertions(+), 8 deletions(-) >>>>>>>>>>>>>> >>>>>>>>>>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>>>>>>>>>>>>> index 476b63618511..a47bbaa27302 100644 >>>>>>>>>>>>>> --- a/drivers/usb/dwc3/core.c >>>>>>>>>>>>>> +++ b/drivers/usb/dwc3/core.c >>>>>>>>>>>>>> @@ -1575,6 +1575,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) >>>>>>>>>>>>>> dwc->dis_split_quirk = device_property_read_bool(dev, >>>>>>>>>>>>>> "snps,dis-split-quirk"); >>>>>>>>>>>>>> >>>>>>>>>>>>>> + dwc->gadget_keep_connect_sys_sleep = device_property_read_bool(dev, >>>>>>>>>>>>>> + "snps,gadget-keep-connect-sys-sleep"); >>>>>>>>>>>>>> + >>>>>>>>>>>>>> dwc->lpm_nyet_threshold = lpm_nyet_threshold; >>>>>>>>>>>>>> dwc->tx_de_emphasis = tx_de_emphasis; >>>>>>>>>>>>>> >>>>>>>>>>>>>> @@ -2027,14 +2030,20 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) >>>>>>>>>>>>>> { >>>>>>>>>>>>>> unsigned long flags; >>>>>>>>>>>>>> u32 reg; >>>>>>>>>>>>>> + int ret; >>>>>>>>>>>>>> >>>>>>>>>>>>>> switch (dwc->current_dr_role) { >>>>>>>>>>>>>> case DWC3_GCTL_PRTCAP_DEVICE: >>>>>>>>>>>>>> if (pm_runtime_suspended(dwc->dev)) >>>>>>>>>>>>>> break; >>>>>>>>>>>>>> - dwc3_gadget_suspend(dwc); >>>>>>>>>>>>>> + ret = dwc3_gadget_suspend(dwc); >>>>>>>>>>>>>> + if (ret) { >>>>>>>>>>>>>> + dev_err(dwc->dev, "gadget not suspended: %d\n", ret); >>>>>>>>>>>>>> + return ret; >>>>>>>>>>>>>> + } >>>>>>>>>>>>>> synchronize_irq(dwc->irq_gadget); >>>>>>>>>>>>>> - dwc3_core_exit(dwc); >>>>>>>>>>>>>> + if(!dwc->gadget_keep_connect_sys_sleep) >>>>>>>>>>>>>> + dwc3_core_exit(dwc); >>>>>>>>>>>>>> break; >>>>>>>>>>>>>> case DWC3_GCTL_PRTCAP_HOST: >>>>>>>>>>>>>> if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) { >>>>>>>>>>>>>> @@ -2088,11 +2097,15 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) >>>>>>>>>>>>>> >>>>>>>>>>>>>> switch (dwc->current_dr_role) { >>>>>>>>>>>>>> case DWC3_GCTL_PRTCAP_DEVICE: >>>>>>>>>>>>>> - ret = dwc3_core_init_for_resume(dwc); >>>>>>>>>>>>>> - if (ret) >>>>>>>>>>>>>> - return ret; >>>>>>>>>>>>>> + if (!dwc->gadget_keep_connect_sys_sleep) >>>>>>>>>>>>>> + { >>>>>>>>>>>>>> + ret = dwc3_core_init_for_resume(dwc); >>>>>>>>>>>>>> + if (ret) >>>>>>>>>>>>>> + return ret; >>>>>>>>>>>>>> + >>>>>>>>>>>>>> + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); >>>>>>>>>>>>>> + } >>>>>>>>>>>>>> >>>>>>>>>>>>>> - dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); >>>>>>>>>>>>>> dwc3_gadget_resume(dwc); >>>>>>>>>>>>>> break; >>>>>>>>>>>>>> case DWC3_GCTL_PRTCAP_HOST: >>>>>>>>>>>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >>>>>>>>>>>>>> index 582ebd9cf9c2..f84bac815bed 100644 >>>>>>>>>>>>>> --- a/drivers/usb/dwc3/core.h >>>>>>>>>>>>>> +++ b/drivers/usb/dwc3/core.h >>>>>>>>>>>>>> @@ -1328,6 +1328,8 @@ struct dwc3 { >>>>>>>>>>>>>> unsigned dis_split_quirk:1; >>>>>>>>>>>>>> unsigned async_callbacks:1; >>>>>>>>>>>>>> >>>>>>>>>>>>>> + unsigned gadget_keep_connect_sys_sleep:1; >>>>>>>>>>>>>> + >>>>>>>>>>>>>> u16 imod_interval; >>>>>>>>>>>>>> >>>>>>>>>>>>>> int max_cfg_eps; >>>>>>>>>>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>>>>>>>>>>>>> index 3c63fa97a680..8062e44f63f6 100644 >>>>>>>>>>>>>> --- a/drivers/usb/dwc3/gadget.c >>>>>>>>>>>>>> +++ b/drivers/usb/dwc3/gadget.c >>>>>>>>>>>>>> @@ -4572,12 +4572,23 @@ void dwc3_gadget_exit(struct dwc3 *dwc) >>>>>>>>>>>>>> int dwc3_gadget_suspend(struct dwc3 *dwc) >>>>>>>>>>>>>> { >>>>>>>>>>>>>> unsigned long flags; >>>>>>>>>>>>>> + int link_state; >>>>>>>>>>>>>> >>>>>>>>>>>>>> if (!dwc->gadget_driver) >>>>>>>>>>>>>> return 0; >>>>>>>>>>>>>> >>>>>>>>>>>>>> - dwc3_gadget_run_stop(dwc, false, false); >>>>>>>>>>>>>> + if (dwc->gadget_keep_connect_sys_sleep && dwc->connected) { >>>>>>>>>>>>>> + link_state = dwc3_gadget_get_link_state(dwc); >>>>>>>>>>>>>> + /* Prevent PM Sleep if not in U3/L2 */ >>>>>>>>>>>>>> + if (link_state != DWC3_LINK_STATE_U3) >>>>>>>>>>>>>> + return -EBUSY; >>>>>>>>>>>>>> + >>>>>>>>>>>>>> + /* don't stop/disconnect */ >>>>>>>>>>>>>> + dwc3_gadget_disable_irq(dwc); >>>>>>>>>>>>> >>>>>>>>>>>>> We shouldn't disable event interrupt here. What will happen if the >>>>>>>>>>>> >>>>>>>>>>>> Due to some reason, if I don't disable the event interrupts here then >>>>>>>>>>>> after USB resume the USB controller is malfunctioning. >>>>>>>>>>>> It no longer responds to any requests from Host. >>>>>>>>>>> >>>>>>>>>>> You should look into this. These events are important as they can tell >>>>>>>>>>> whether the host initiates resume. >>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>>> device is disconnected and reconnect to the host while the device is >>>>>>>>>>>>> still in system suspend? The host would not be able to communicate with >>>>>>>>>>>>> the device then. >>>>>>>>>>>> >>>>>>>>>>>> In the TI platform, The system is woken up on any VBUS/linestate change >>>>>>>>>>>> and in dwc3_gadget_resume we enable the events again and check for pending >>>>>>>>>>>> events. Is it pointless to check for pending events there? >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> It seems fragile for the implementation to be dependent on platform >>>>>>>>>>> specific feature right? >>>>>>>>>>> >>>>>>>>>>> Also, what will happen in a typical case when the host puts the device >>>>>>>>>>> in suspend and initiates resume while the device is in system suspend >>>>>>>>>>> (and stay in suspend over a period of time)? There is no VBUS change. >>>>>>>>>>> There will be problem if host detects no response from device in time. >>>>>>>>>>> >>>>>>>>>>> Don't we need these events to wakeup the device? >>>>>>>>> >>>>>>>>> That's why the TI implementation has line-state change detection to >>>>>>>>> detect a USB resume. We are doing a out-of-band wake-up. The wake up >>>>>>>>> events are configured in the wrapper driver (dwc3-am62.c). >>>>>>>>> >>>>>>>>> Do you know of any dwc3 implementation that uses in-band mechanism >>>>>>>>> to wake up the System. i.e. it relies on events enabled in DEVTEN register? >>>>>>>>> >>>>>>>> >>>>>>>> We rely on PME. The PME is generated from the PMU of the usb controller >>>>>>>> when it detects a resume. If your platform supports hibernation and if >>>>>>>> the resume signal is connected to the lower layer power manager of your >>>>>>>> device, then you can wakeup the system one level at a time. For example, >>>>>>>> if your device is a pci device, that wakeup signal would tie to the pci >>>>>>>> power manager, waking up the pci layer before waking up the core of the >>>>>>>> usb controller. That's how the host wakes up the host system (e.g. from >>>>>>>> remote wakeup). For this to work, we expect something similar on the >>>>>>>> device side. >>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> We may not be able to suspend everything in system suspend for this >>>>>>>>>> case. I'm thinking of treating these events as if they are PME to wakeup >>>>>>>>>> the device, but they are not the same. It may not be simple to handle >>>>>>>>>> this. The lower layers may need to stay awake for the dwc3 to handle >>>>>>>>>> these events. Hm... it gets a bit complicated. >>>>>>>>> >>>>>>>>> As we are going into suspend, we are not really in a position to handle any >>>>>>>>> (DEVTEN) events till we have fully resumed. >>>>>>>>> So yes, we need to rely on platform specific implementation to wake >>>>>>>>> the System on any USB event. >>>>>>>>> >>>>>>>> >>>>>>>> You may be able to detect vbus change through the connector controller. >>>>>>>> However, the usb controller is the one that detects host resume. What >>>>>>>> platform specific implementation do you have outside of the usb >>>>>>>> controller do you have to get around that? >>>>>>>> >>>>>>>> I'm not sure if your platform supports hibernation or if the PME signal >>>>>>>> on your platform can wakeup the system, but currently dwc3 driver >>>>>>>> doesn't handle hibernation (device side). If there's no hibernation, >>>>>>>> there's no PME. >>>>>> >>>>>> No, in this TI SoC, hibernation feature is not supported in the dwc3 core. >>>>>> >>>>>>>> >>>>>>> >>>>>>> Actually, I think the dwc3 core is still on during system suspend for >>>>>>> you right? Then I think we can use the wakeup event to wakeup system >>>>>>> suspend on host resume? You can ignore about PME in this case. You may >>>>>>> need to look into what needs stay awake to allow for handling of the >>>>>>> dwc3 event. >>>>>> >>>>>> But in SoC deep-sleep state, all clocks to the dwc3 core are stopped. >>>>>> So I'm not sure if dwc3 events will work. >>>>>> >>>>> >>>>> Right, you need to keep those clocks running to detect host resume. >>>>> There's still some power saving through the dwc3 controller's handling >>>>> in suspend. You may have some limited power saving from other suspended >>>>> devices on your setup. However, I don't think we can expect the platform >>>>> to go into deep-sleep and also handle host resume. >>>> >>>> Why not? if the PHY can detect the host resume and wake up the SoC it will >>>> work right? >>>> >>> >>> Hm... I supposed it may be possible. But it may need some unconventional >>> design? The dwc3 controller is currently registered to the phy. For that >>> to work, your phy needs to be able to talk to both the dwc3 controller >>> and some other controller (equivalent to dwc3 PMU) that manages >>> power/interrupt. The dwc3 controller would need to relinquish control to >>> this other phy controller on suspend. The phy driver would then be able >>> to assert interrupt waking up the system on resume sigal detection, >>> which in turn relinquish control to the dwc3 controller. All of this has >>> to work while the phy signaling remains synchronized with the dwc3 >>> controller. >> >> My understanding is that all this is taken care by PHY integration design with >> DWC3 core on the TI SoC. >> >>> >>> From the patches you sent, I don't see the changes necesssary for this >>> to work. If there is something that I'm missing, please also note it or >>> add it here to the series. >> >> There is nothing more as the details are taken care by PHY logic and >> necessary integration with DWC3. >> >> For the PHY wake-up programming details you have already checked this series [1]. >> >> [1] - https://urldefense.com/v3/__https://lore.kernel.org/all/20230316131226.89540-1-rogerq@kernel.org/__;!!A4F2R9G_pg!ayqpaWwnWIO7SKktk4kI2EJLwDTIv2nYYCgBGHUlt56KTzeYkDEdq2Q5ZvZFsuCWIlcXGET230YY5J3y_sHV$ >> > > I may have misunderstood your platform implementation. My understanding > is that it can only detect VBUS and that it can only resume on VBUS > valid. > > Does the "LINESTATE" here gets asserted if say there's a LFPS detection? Yes. The wake up logic on the SoC is snooping the UTMI lines from the PHY and on any change it can detect and wake up the SoC. cheers, -roger ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature 2023-04-05 8:56 ` Roger Quadros @ 2023-04-06 1:38 ` Thinh Nguyen 2023-04-12 7:46 ` Roger Quadros 0 siblings, 1 reply; 24+ messages in thread From: Thinh Nguyen @ 2023-04-06 1:38 UTC (permalink / raw) To: Roger Quadros Cc: Thinh Nguyen, stern, gregkh, vigneshr, srk, r-gunasekaran, linux-usb, linux-kernel On Wed, Apr 05, 2023, Roger Quadros wrote: > > > On 05/04/2023 00:53, Thinh Nguyen wrote: > > On Tue, Apr 04, 2023, Roger Quadros wrote: > >> > >> > >> On 04/04/2023 02:37, Thinh Nguyen wrote: > >>> On Fri, Mar 31, 2023, Roger Quadros wrote: > >>>> Hi, > >>>> > >>>> On 23/03/2023 22:51, Thinh Nguyen wrote: > >>>>> On Thu, Mar 23, 2023, Roger Quadros wrote: > >>>>>> > >>>>>> > >>>>>> On 23/03/2023 04:17, Thinh Nguyen wrote: > >>>>>>> On Wed, Mar 22, 2023, Thinh Nguyen wrote: > >>>>>>>> On Wed, Mar 22, 2023, Roger Quadros wrote: > >>>>>>>>> On 21/03/2023 21:05, Thinh Nguyen wrote: > >>>>>>>>>> On Tue, Mar 21, 2023, Thinh Nguyen wrote: > >>>>>>>>>>> On Tue, Mar 21, 2023, Roger Quadros wrote: > >>>>>>>>>>>> Hi Thinh, > >>>>>>>>>>>> > >>>>>>>>>>>> On 20/03/2023 20:52, Thinh Nguyen wrote: > >>>>>>>>>>>>> Hi, > >>>>>>>>>>>>> > >>>>>>>>>>>>> On Mon, Mar 20, 2023, Roger Quadros wrote: > >>>>>>>>>>>>>> Implement 'snps,gadget-keep-connect-sys-sleep' property. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Do not stop the gadget controller and disconnect if this > >>>>>>>>>>>>>> property is present and we are connected to a USB Host. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Prevent System sleep if Gadget is not in USB suspend. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org> > >>>>>>>>>>>>>> --- > >>>>>>>>>>>>>> drivers/usb/dwc3/core.c | 25 +++++++++++++++++++------ > >>>>>>>>>>>>>> drivers/usb/dwc3/core.h | 2 ++ > >>>>>>>>>>>>>> drivers/usb/dwc3/gadget.c | 25 +++++++++++++++++++++++-- > >>>>>>>>>>>>>> 3 files changed, 44 insertions(+), 8 deletions(-) > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > >>>>>>>>>>>>>> index 476b63618511..a47bbaa27302 100644 > >>>>>>>>>>>>>> --- a/drivers/usb/dwc3/core.c > >>>>>>>>>>>>>> +++ b/drivers/usb/dwc3/core.c > >>>>>>>>>>>>>> @@ -1575,6 +1575,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) > >>>>>>>>>>>>>> dwc->dis_split_quirk = device_property_read_bool(dev, > >>>>>>>>>>>>>> "snps,dis-split-quirk"); > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> + dwc->gadget_keep_connect_sys_sleep = device_property_read_bool(dev, > >>>>>>>>>>>>>> + "snps,gadget-keep-connect-sys-sleep"); > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> dwc->lpm_nyet_threshold = lpm_nyet_threshold; > >>>>>>>>>>>>>> dwc->tx_de_emphasis = tx_de_emphasis; > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> @@ -2027,14 +2030,20 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > >>>>>>>>>>>>>> { > >>>>>>>>>>>>>> unsigned long flags; > >>>>>>>>>>>>>> u32 reg; > >>>>>>>>>>>>>> + int ret; > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> switch (dwc->current_dr_role) { > >>>>>>>>>>>>>> case DWC3_GCTL_PRTCAP_DEVICE: > >>>>>>>>>>>>>> if (pm_runtime_suspended(dwc->dev)) > >>>>>>>>>>>>>> break; > >>>>>>>>>>>>>> - dwc3_gadget_suspend(dwc); > >>>>>>>>>>>>>> + ret = dwc3_gadget_suspend(dwc); > >>>>>>>>>>>>>> + if (ret) { > >>>>>>>>>>>>>> + dev_err(dwc->dev, "gadget not suspended: %d\n", ret); > >>>>>>>>>>>>>> + return ret; > >>>>>>>>>>>>>> + } > >>>>>>>>>>>>>> synchronize_irq(dwc->irq_gadget); > >>>>>>>>>>>>>> - dwc3_core_exit(dwc); > >>>>>>>>>>>>>> + if(!dwc->gadget_keep_connect_sys_sleep) > >>>>>>>>>>>>>> + dwc3_core_exit(dwc); > >>>>>>>>>>>>>> break; > >>>>>>>>>>>>>> case DWC3_GCTL_PRTCAP_HOST: > >>>>>>>>>>>>>> if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) { > >>>>>>>>>>>>>> @@ -2088,11 +2097,15 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> switch (dwc->current_dr_role) { > >>>>>>>>>>>>>> case DWC3_GCTL_PRTCAP_DEVICE: > >>>>>>>>>>>>>> - ret = dwc3_core_init_for_resume(dwc); > >>>>>>>>>>>>>> - if (ret) > >>>>>>>>>>>>>> - return ret; > >>>>>>>>>>>>>> + if (!dwc->gadget_keep_connect_sys_sleep) > >>>>>>>>>>>>>> + { > >>>>>>>>>>>>>> + ret = dwc3_core_init_for_resume(dwc); > >>>>>>>>>>>>>> + if (ret) > >>>>>>>>>>>>>> + return ret; > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); > >>>>>>>>>>>>>> + } > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> - dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); > >>>>>>>>>>>>>> dwc3_gadget_resume(dwc); > >>>>>>>>>>>>>> break; > >>>>>>>>>>>>>> case DWC3_GCTL_PRTCAP_HOST: > >>>>>>>>>>>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > >>>>>>>>>>>>>> index 582ebd9cf9c2..f84bac815bed 100644 > >>>>>>>>>>>>>> --- a/drivers/usb/dwc3/core.h > >>>>>>>>>>>>>> +++ b/drivers/usb/dwc3/core.h > >>>>>>>>>>>>>> @@ -1328,6 +1328,8 @@ struct dwc3 { > >>>>>>>>>>>>>> unsigned dis_split_quirk:1; > >>>>>>>>>>>>>> unsigned async_callbacks:1; > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> + unsigned gadget_keep_connect_sys_sleep:1; > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> u16 imod_interval; > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> int max_cfg_eps; > >>>>>>>>>>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > >>>>>>>>>>>>>> index 3c63fa97a680..8062e44f63f6 100644 > >>>>>>>>>>>>>> --- a/drivers/usb/dwc3/gadget.c > >>>>>>>>>>>>>> +++ b/drivers/usb/dwc3/gadget.c > >>>>>>>>>>>>>> @@ -4572,12 +4572,23 @@ void dwc3_gadget_exit(struct dwc3 *dwc) > >>>>>>>>>>>>>> int dwc3_gadget_suspend(struct dwc3 *dwc) > >>>>>>>>>>>>>> { > >>>>>>>>>>>>>> unsigned long flags; > >>>>>>>>>>>>>> + int link_state; > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> if (!dwc->gadget_driver) > >>>>>>>>>>>>>> return 0; > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> - dwc3_gadget_run_stop(dwc, false, false); > >>>>>>>>>>>>>> + if (dwc->gadget_keep_connect_sys_sleep && dwc->connected) { > >>>>>>>>>>>>>> + link_state = dwc3_gadget_get_link_state(dwc); > >>>>>>>>>>>>>> + /* Prevent PM Sleep if not in U3/L2 */ > >>>>>>>>>>>>>> + if (link_state != DWC3_LINK_STATE_U3) > >>>>>>>>>>>>>> + return -EBUSY; > >>>>>>>>>>>>>> + > >>>>>>>>>>>>>> + /* don't stop/disconnect */ > >>>>>>>>>>>>>> + dwc3_gadget_disable_irq(dwc); > >>>>>>>>>>>>> > >>>>>>>>>>>>> We shouldn't disable event interrupt here. What will happen if the > >>>>>>>>>>>> > >>>>>>>>>>>> Due to some reason, if I don't disable the event interrupts here then > >>>>>>>>>>>> after USB resume the USB controller is malfunctioning. > >>>>>>>>>>>> It no longer responds to any requests from Host. > >>>>>>>>>>> > >>>>>>>>>>> You should look into this. These events are important as they can tell > >>>>>>>>>>> whether the host initiates resume. > >>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>>> device is disconnected and reconnect to the host while the device is > >>>>>>>>>>>>> still in system suspend? The host would not be able to communicate with > >>>>>>>>>>>>> the device then. > >>>>>>>>>>>> > >>>>>>>>>>>> In the TI platform, The system is woken up on any VBUS/linestate change > >>>>>>>>>>>> and in dwc3_gadget_resume we enable the events again and check for pending > >>>>>>>>>>>> events. Is it pointless to check for pending events there? > >>>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> It seems fragile for the implementation to be dependent on platform > >>>>>>>>>>> specific feature right? > >>>>>>>>>>> > >>>>>>>>>>> Also, what will happen in a typical case when the host puts the device > >>>>>>>>>>> in suspend and initiates resume while the device is in system suspend > >>>>>>>>>>> (and stay in suspend over a period of time)? There is no VBUS change. > >>>>>>>>>>> There will be problem if host detects no response from device in time. > >>>>>>>>>>> > >>>>>>>>>>> Don't we need these events to wakeup the device? > >>>>>>>>> > >>>>>>>>> That's why the TI implementation has line-state change detection to > >>>>>>>>> detect a USB resume. We are doing a out-of-band wake-up. The wake up > >>>>>>>>> events are configured in the wrapper driver (dwc3-am62.c). > >>>>>>>>> > >>>>>>>>> Do you know of any dwc3 implementation that uses in-band mechanism > >>>>>>>>> to wake up the System. i.e. it relies on events enabled in DEVTEN register? > >>>>>>>>> > >>>>>>>> > >>>>>>>> We rely on PME. The PME is generated from the PMU of the usb controller > >>>>>>>> when it detects a resume. If your platform supports hibernation and if > >>>>>>>> the resume signal is connected to the lower layer power manager of your > >>>>>>>> device, then you can wakeup the system one level at a time. For example, > >>>>>>>> if your device is a pci device, that wakeup signal would tie to the pci > >>>>>>>> power manager, waking up the pci layer before waking up the core of the > >>>>>>>> usb controller. That's how the host wakes up the host system (e.g. from > >>>>>>>> remote wakeup). For this to work, we expect something similar on the > >>>>>>>> device side. > >>>>>>>> > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> We may not be able to suspend everything in system suspend for this > >>>>>>>>>> case. I'm thinking of treating these events as if they are PME to wakeup > >>>>>>>>>> the device, but they are not the same. It may not be simple to handle > >>>>>>>>>> this. The lower layers may need to stay awake for the dwc3 to handle > >>>>>>>>>> these events. Hm... it gets a bit complicated. > >>>>>>>>> > >>>>>>>>> As we are going into suspend, we are not really in a position to handle any > >>>>>>>>> (DEVTEN) events till we have fully resumed. > >>>>>>>>> So yes, we need to rely on platform specific implementation to wake > >>>>>>>>> the System on any USB event. > >>>>>>>>> > >>>>>>>> > >>>>>>>> You may be able to detect vbus change through the connector controller. > >>>>>>>> However, the usb controller is the one that detects host resume. What > >>>>>>>> platform specific implementation do you have outside of the usb > >>>>>>>> controller do you have to get around that? > >>>>>>>> > >>>>>>>> I'm not sure if your platform supports hibernation or if the PME signal > >>>>>>>> on your platform can wakeup the system, but currently dwc3 driver > >>>>>>>> doesn't handle hibernation (device side). If there's no hibernation, > >>>>>>>> there's no PME. > >>>>>> > >>>>>> No, in this TI SoC, hibernation feature is not supported in the dwc3 core. > >>>>>> > >>>>>>>> > >>>>>>> > >>>>>>> Actually, I think the dwc3 core is still on during system suspend for > >>>>>>> you right? Then I think we can use the wakeup event to wakeup system > >>>>>>> suspend on host resume? You can ignore about PME in this case. You may > >>>>>>> need to look into what needs stay awake to allow for handling of the > >>>>>>> dwc3 event. > >>>>>> > >>>>>> But in SoC deep-sleep state, all clocks to the dwc3 core are stopped. > >>>>>> So I'm not sure if dwc3 events will work. > >>>>>> > >>>>> > >>>>> Right, you need to keep those clocks running to detect host resume. > >>>>> There's still some power saving through the dwc3 controller's handling > >>>>> in suspend. You may have some limited power saving from other suspended > >>>>> devices on your setup. However, I don't think we can expect the platform > >>>>> to go into deep-sleep and also handle host resume. > >>>> > >>>> Why not? if the PHY can detect the host resume and wake up the SoC it will > >>>> work right? > >>>> > >>> > >>> Hm... I supposed it may be possible. But it may need some unconventional > >>> design? The dwc3 controller is currently registered to the phy. For that > >>> to work, your phy needs to be able to talk to both the dwc3 controller > >>> and some other controller (equivalent to dwc3 PMU) that manages > >>> power/interrupt. The dwc3 controller would need to relinquish control to > >>> this other phy controller on suspend. The phy driver would then be able > >>> to assert interrupt waking up the system on resume sigal detection, > >>> which in turn relinquish control to the dwc3 controller. All of this has > >>> to work while the phy signaling remains synchronized with the dwc3 > >>> controller. > >> > >> My understanding is that all this is taken care by PHY integration design with > >> DWC3 core on the TI SoC. > >> > >>> > >>> From the patches you sent, I don't see the changes necesssary for this > >>> to work. If there is something that I'm missing, please also note it or > >>> add it here to the series. > >> > >> There is nothing more as the details are taken care by PHY logic and > >> necessary integration with DWC3. > >> > >> For the PHY wake-up programming details you have already checked this series [1]. > >> > >> [1] - https://urldefense.com/v3/__https://lore.kernel.org/all/20230316131226.89540-1-rogerq@kernel.org/__;!!A4F2R9G_pg!ayqpaWwnWIO7SKktk4kI2EJLwDTIv2nYYCgBGHUlt56KTzeYkDEdq2Q5ZvZFsuCWIlcXGET230YY5J3y_sHV$ > >> > > > > I may have misunderstood your platform implementation. My understanding > > is that it can only detect VBUS and that it can only resume on VBUS > > valid. > > > > Does the "LINESTATE" here gets asserted if say there's a LFPS detection? > > Yes. The wake up logic on the SoC is snooping the UTMI lines from the PHY and on any > change it can detect and wake up the SoC. > Are you referring to the utmi_linestate signal? Isn't that for usb2 speed only? Does your platform support usb3 speed? Thanks, Thinh ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature 2023-04-06 1:38 ` Thinh Nguyen @ 2023-04-12 7:46 ` Roger Quadros 2023-04-12 20:59 ` Thinh Nguyen 0 siblings, 1 reply; 24+ messages in thread From: Roger Quadros @ 2023-04-12 7:46 UTC (permalink / raw) To: Thinh Nguyen Cc: stern, gregkh, vigneshr, srk, r-gunasekaran, linux-usb, linux-kernel On 06/04/2023 04:38, Thinh Nguyen wrote: > On Wed, Apr 05, 2023, Roger Quadros wrote: >> >> >> On 05/04/2023 00:53, Thinh Nguyen wrote: >>> On Tue, Apr 04, 2023, Roger Quadros wrote: >>>> >>>> >>>> On 04/04/2023 02:37, Thinh Nguyen wrote: >>>>> On Fri, Mar 31, 2023, Roger Quadros wrote: >>>>>> Hi, >>>>>> >>>>>> On 23/03/2023 22:51, Thinh Nguyen wrote: >>>>>>> On Thu, Mar 23, 2023, Roger Quadros wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 23/03/2023 04:17, Thinh Nguyen wrote: >>>>>>>>> On Wed, Mar 22, 2023, Thinh Nguyen wrote: >>>>>>>>>> On Wed, Mar 22, 2023, Roger Quadros wrote: >>>>>>>>>>> On 21/03/2023 21:05, Thinh Nguyen wrote: >>>>>>>>>>>> On Tue, Mar 21, 2023, Thinh Nguyen wrote: >>>>>>>>>>>>> On Tue, Mar 21, 2023, Roger Quadros wrote: >>>>>>>>>>>>>> Hi Thinh, >>>>>>>>>>>>>> >>>>>>>>>>>>>> On 20/03/2023 20:52, Thinh Nguyen wrote: >>>>>>>>>>>>>>> Hi, >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On Mon, Mar 20, 2023, Roger Quadros wrote: >>>>>>>>>>>>>>>> Implement 'snps,gadget-keep-connect-sys-sleep' property. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Do not stop the gadget controller and disconnect if this >>>>>>>>>>>>>>>> property is present and we are connected to a USB Host. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Prevent System sleep if Gadget is not in USB suspend. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org> >>>>>>>>>>>>>>>> --- >>>>>>>>>>>>>>>> drivers/usb/dwc3/core.c | 25 +++++++++++++++++++------ >>>>>>>>>>>>>>>> drivers/usb/dwc3/core.h | 2 ++ >>>>>>>>>>>>>>>> drivers/usb/dwc3/gadget.c | 25 +++++++++++++++++++++++-- >>>>>>>>>>>>>>>> 3 files changed, 44 insertions(+), 8 deletions(-) >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>>>>>>>>>>>>>>> index 476b63618511..a47bbaa27302 100644 >>>>>>>>>>>>>>>> --- a/drivers/usb/dwc3/core.c >>>>>>>>>>>>>>>> +++ b/drivers/usb/dwc3/core.c >>>>>>>>>>>>>>>> @@ -1575,6 +1575,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) >>>>>>>>>>>>>>>> dwc->dis_split_quirk = device_property_read_bool(dev, >>>>>>>>>>>>>>>> "snps,dis-split-quirk"); >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> + dwc->gadget_keep_connect_sys_sleep = device_property_read_bool(dev, >>>>>>>>>>>>>>>> + "snps,gadget-keep-connect-sys-sleep"); >>>>>>>>>>>>>>>> + >>>>>>>>>>>>>>>> dwc->lpm_nyet_threshold = lpm_nyet_threshold; >>>>>>>>>>>>>>>> dwc->tx_de_emphasis = tx_de_emphasis; >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> @@ -2027,14 +2030,20 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) >>>>>>>>>>>>>>>> { >>>>>>>>>>>>>>>> unsigned long flags; >>>>>>>>>>>>>>>> u32 reg; >>>>>>>>>>>>>>>> + int ret; >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> switch (dwc->current_dr_role) { >>>>>>>>>>>>>>>> case DWC3_GCTL_PRTCAP_DEVICE: >>>>>>>>>>>>>>>> if (pm_runtime_suspended(dwc->dev)) >>>>>>>>>>>>>>>> break; >>>>>>>>>>>>>>>> - dwc3_gadget_suspend(dwc); >>>>>>>>>>>>>>>> + ret = dwc3_gadget_suspend(dwc); >>>>>>>>>>>>>>>> + if (ret) { >>>>>>>>>>>>>>>> + dev_err(dwc->dev, "gadget not suspended: %d\n", ret); >>>>>>>>>>>>>>>> + return ret; >>>>>>>>>>>>>>>> + } >>>>>>>>>>>>>>>> synchronize_irq(dwc->irq_gadget); >>>>>>>>>>>>>>>> - dwc3_core_exit(dwc); >>>>>>>>>>>>>>>> + if(!dwc->gadget_keep_connect_sys_sleep) >>>>>>>>>>>>>>>> + dwc3_core_exit(dwc); >>>>>>>>>>>>>>>> break; >>>>>>>>>>>>>>>> case DWC3_GCTL_PRTCAP_HOST: >>>>>>>>>>>>>>>> if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(dwc->dev)) { >>>>>>>>>>>>>>>> @@ -2088,11 +2097,15 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> switch (dwc->current_dr_role) { >>>>>>>>>>>>>>>> case DWC3_GCTL_PRTCAP_DEVICE: >>>>>>>>>>>>>>>> - ret = dwc3_core_init_for_resume(dwc); >>>>>>>>>>>>>>>> - if (ret) >>>>>>>>>>>>>>>> - return ret; >>>>>>>>>>>>>>>> + if (!dwc->gadget_keep_connect_sys_sleep) >>>>>>>>>>>>>>>> + { >>>>>>>>>>>>>>>> + ret = dwc3_core_init_for_resume(dwc); >>>>>>>>>>>>>>>> + if (ret) >>>>>>>>>>>>>>>> + return ret; >>>>>>>>>>>>>>>> + >>>>>>>>>>>>>>>> + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); >>>>>>>>>>>>>>>> + } >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> - dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); >>>>>>>>>>>>>>>> dwc3_gadget_resume(dwc); >>>>>>>>>>>>>>>> break; >>>>>>>>>>>>>>>> case DWC3_GCTL_PRTCAP_HOST: >>>>>>>>>>>>>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >>>>>>>>>>>>>>>> index 582ebd9cf9c2..f84bac815bed 100644 >>>>>>>>>>>>>>>> --- a/drivers/usb/dwc3/core.h >>>>>>>>>>>>>>>> +++ b/drivers/usb/dwc3/core.h >>>>>>>>>>>>>>>> @@ -1328,6 +1328,8 @@ struct dwc3 { >>>>>>>>>>>>>>>> unsigned dis_split_quirk:1; >>>>>>>>>>>>>>>> unsigned async_callbacks:1; >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> + unsigned gadget_keep_connect_sys_sleep:1; >>>>>>>>>>>>>>>> + >>>>>>>>>>>>>>>> u16 imod_interval; >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> int max_cfg_eps; >>>>>>>>>>>>>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>>>>>>>>>>>>>>> index 3c63fa97a680..8062e44f63f6 100644 >>>>>>>>>>>>>>>> --- a/drivers/usb/dwc3/gadget.c >>>>>>>>>>>>>>>> +++ b/drivers/usb/dwc3/gadget.c >>>>>>>>>>>>>>>> @@ -4572,12 +4572,23 @@ void dwc3_gadget_exit(struct dwc3 *dwc) >>>>>>>>>>>>>>>> int dwc3_gadget_suspend(struct dwc3 *dwc) >>>>>>>>>>>>>>>> { >>>>>>>>>>>>>>>> unsigned long flags; >>>>>>>>>>>>>>>> + int link_state; >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> if (!dwc->gadget_driver) >>>>>>>>>>>>>>>> return 0; >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> - dwc3_gadget_run_stop(dwc, false, false); >>>>>>>>>>>>>>>> + if (dwc->gadget_keep_connect_sys_sleep && dwc->connected) { >>>>>>>>>>>>>>>> + link_state = dwc3_gadget_get_link_state(dwc); >>>>>>>>>>>>>>>> + /* Prevent PM Sleep if not in U3/L2 */ >>>>>>>>>>>>>>>> + if (link_state != DWC3_LINK_STATE_U3) >>>>>>>>>>>>>>>> + return -EBUSY; >>>>>>>>>>>>>>>> + >>>>>>>>>>>>>>>> + /* don't stop/disconnect */ >>>>>>>>>>>>>>>> + dwc3_gadget_disable_irq(dwc); >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> We shouldn't disable event interrupt here. What will happen if the >>>>>>>>>>>>>> >>>>>>>>>>>>>> Due to some reason, if I don't disable the event interrupts here then >>>>>>>>>>>>>> after USB resume the USB controller is malfunctioning. >>>>>>>>>>>>>> It no longer responds to any requests from Host. >>>>>>>>>>>>> >>>>>>>>>>>>> You should look into this. These events are important as they can tell >>>>>>>>>>>>> whether the host initiates resume. >>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>>> device is disconnected and reconnect to the host while the device is >>>>>>>>>>>>>>> still in system suspend? The host would not be able to communicate with >>>>>>>>>>>>>>> the device then. >>>>>>>>>>>>>> >>>>>>>>>>>>>> In the TI platform, The system is woken up on any VBUS/linestate change >>>>>>>>>>>>>> and in dwc3_gadget_resume we enable the events again and check for pending >>>>>>>>>>>>>> events. Is it pointless to check for pending events there? >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> It seems fragile for the implementation to be dependent on platform >>>>>>>>>>>>> specific feature right? >>>>>>>>>>>>> >>>>>>>>>>>>> Also, what will happen in a typical case when the host puts the device >>>>>>>>>>>>> in suspend and initiates resume while the device is in system suspend >>>>>>>>>>>>> (and stay in suspend over a period of time)? There is no VBUS change. >>>>>>>>>>>>> There will be problem if host detects no response from device in time. >>>>>>>>>>>>> >>>>>>>>>>>>> Don't we need these events to wakeup the device? >>>>>>>>>>> >>>>>>>>>>> That's why the TI implementation has line-state change detection to >>>>>>>>>>> detect a USB resume. We are doing a out-of-band wake-up. The wake up >>>>>>>>>>> events are configured in the wrapper driver (dwc3-am62.c). >>>>>>>>>>> >>>>>>>>>>> Do you know of any dwc3 implementation that uses in-band mechanism >>>>>>>>>>> to wake up the System. i.e. it relies on events enabled in DEVTEN register? >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> We rely on PME. The PME is generated from the PMU of the usb controller >>>>>>>>>> when it detects a resume. If your platform supports hibernation and if >>>>>>>>>> the resume signal is connected to the lower layer power manager of your >>>>>>>>>> device, then you can wakeup the system one level at a time. For example, >>>>>>>>>> if your device is a pci device, that wakeup signal would tie to the pci >>>>>>>>>> power manager, waking up the pci layer before waking up the core of the >>>>>>>>>> usb controller. That's how the host wakes up the host system (e.g. from >>>>>>>>>> remote wakeup). For this to work, we expect something similar on the >>>>>>>>>> device side. >>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> We may not be able to suspend everything in system suspend for this >>>>>>>>>>>> case. I'm thinking of treating these events as if they are PME to wakeup >>>>>>>>>>>> the device, but they are not the same. It may not be simple to handle >>>>>>>>>>>> this. The lower layers may need to stay awake for the dwc3 to handle >>>>>>>>>>>> these events. Hm... it gets a bit complicated. >>>>>>>>>>> >>>>>>>>>>> As we are going into suspend, we are not really in a position to handle any >>>>>>>>>>> (DEVTEN) events till we have fully resumed. >>>>>>>>>>> So yes, we need to rely on platform specific implementation to wake >>>>>>>>>>> the System on any USB event. >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> You may be able to detect vbus change through the connector controller. >>>>>>>>>> However, the usb controller is the one that detects host resume. What >>>>>>>>>> platform specific implementation do you have outside of the usb >>>>>>>>>> controller do you have to get around that? >>>>>>>>>> >>>>>>>>>> I'm not sure if your platform supports hibernation or if the PME signal >>>>>>>>>> on your platform can wakeup the system, but currently dwc3 driver >>>>>>>>>> doesn't handle hibernation (device side). If there's no hibernation, >>>>>>>>>> there's no PME. >>>>>>>> >>>>>>>> No, in this TI SoC, hibernation feature is not supported in the dwc3 core. >>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>> Actually, I think the dwc3 core is still on during system suspend for >>>>>>>>> you right? Then I think we can use the wakeup event to wakeup system >>>>>>>>> suspend on host resume? You can ignore about PME in this case. You may >>>>>>>>> need to look into what needs stay awake to allow for handling of the >>>>>>>>> dwc3 event. >>>>>>>> >>>>>>>> But in SoC deep-sleep state, all clocks to the dwc3 core are stopped. >>>>>>>> So I'm not sure if dwc3 events will work. >>>>>>>> >>>>>>> >>>>>>> Right, you need to keep those clocks running to detect host resume. >>>>>>> There's still some power saving through the dwc3 controller's handling >>>>>>> in suspend. You may have some limited power saving from other suspended >>>>>>> devices on your setup. However, I don't think we can expect the platform >>>>>>> to go into deep-sleep and also handle host resume. >>>>>> >>>>>> Why not? if the PHY can detect the host resume and wake up the SoC it will >>>>>> work right? >>>>>> >>>>> >>>>> Hm... I supposed it may be possible. But it may need some unconventional >>>>> design? The dwc3 controller is currently registered to the phy. For that >>>>> to work, your phy needs to be able to talk to both the dwc3 controller >>>>> and some other controller (equivalent to dwc3 PMU) that manages >>>>> power/interrupt. The dwc3 controller would need to relinquish control to >>>>> this other phy controller on suspend. The phy driver would then be able >>>>> to assert interrupt waking up the system on resume sigal detection, >>>>> which in turn relinquish control to the dwc3 controller. All of this has >>>>> to work while the phy signaling remains synchronized with the dwc3 >>>>> controller. >>>> >>>> My understanding is that all this is taken care by PHY integration design with >>>> DWC3 core on the TI SoC. >>>> >>>>> >>>>> From the patches you sent, I don't see the changes necesssary for this >>>>> to work. If there is something that I'm missing, please also note it or >>>>> add it here to the series. >>>> >>>> There is nothing more as the details are taken care by PHY logic and >>>> necessary integration with DWC3. >>>> >>>> For the PHY wake-up programming details you have already checked this series [1]. >>>> >>>> [1] - https://urldefense.com/v3/__https://lore.kernel.org/all/20230316131226.89540-1-rogerq@kernel.org/__;!!A4F2R9G_pg!ayqpaWwnWIO7SKktk4kI2EJLwDTIv2nYYCgBGHUlt56KTzeYkDEdq2Q5ZvZFsuCWIlcXGET230YY5J3y_sHV$ >>>> >>> >>> I may have misunderstood your platform implementation. My understanding >>> is that it can only detect VBUS and that it can only resume on VBUS >>> valid. >>> >>> Does the "LINESTATE" here gets asserted if say there's a LFPS detection? >> >> Yes. The wake up logic on the SoC is snooping the UTMI lines from the PHY and on any >> change it can detect and wake up the SoC. >> > > Are you referring to the utmi_linestate signal? Isn't that for usb2 > speed only? Does your platform support usb3 speed? The wake-up on deepSleep feature is only supported for USB2 on this particular SoC. cheers, -roger ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature 2023-04-12 7:46 ` Roger Quadros @ 2023-04-12 20:59 ` Thinh Nguyen 2023-04-13 11:31 ` Roger Quadros 0 siblings, 1 reply; 24+ messages in thread From: Thinh Nguyen @ 2023-04-12 20:59 UTC (permalink / raw) To: Roger Quadros Cc: Thinh Nguyen, stern, gregkh, vigneshr, srk, r-gunasekaran, linux-usb, linux-kernel On Wed, Apr 12, 2023, Roger Quadros wrote: > > > On 06/04/2023 04:38, Thinh Nguyen wrote: > > On Wed, Apr 05, 2023, Roger Quadros wrote: > >> > >> > >> On 05/04/2023 00:53, Thinh Nguyen wrote: > >>> > >>> I may have misunderstood your platform implementation. My understanding > >>> is that it can only detect VBUS and that it can only resume on VBUS > >>> valid. > >>> > >>> Does the "LINESTATE" here gets asserted if say there's a LFPS detection? > >> > >> Yes. The wake up logic on the SoC is snooping the UTMI lines from the PHY and on any > >> change it can detect and wake up the SoC. > >> > > > > Are you referring to the utmi_linestate signal? Isn't that for usb2 > > speed only? Does your platform support usb3 speed? > > The wake-up on deepSleep feature is only supported for USB2 on this particular SoC. > I mean can your platform operate in usb3 speed. If that's the case, then how do you plan to handle it here. Also, when you tested this in highspeed, did you observe successful resume? Or did the host have to perform a port reset? Thanks, Thinh ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature 2023-04-12 20:59 ` Thinh Nguyen @ 2023-04-13 11:31 ` Roger Quadros 0 siblings, 0 replies; 24+ messages in thread From: Roger Quadros @ 2023-04-13 11:31 UTC (permalink / raw) To: Thinh Nguyen Cc: stern, gregkh, vigneshr, srk, r-gunasekaran, linux-usb, linux-kernel On 12/04/2023 23:59, Thinh Nguyen wrote: > On Wed, Apr 12, 2023, Roger Quadros wrote: >> >> >> On 06/04/2023 04:38, Thinh Nguyen wrote: >>> On Wed, Apr 05, 2023, Roger Quadros wrote: >>>> >>>> >>>> On 05/04/2023 00:53, Thinh Nguyen wrote: >>>>> >>>>> I may have misunderstood your platform implementation. My understanding >>>>> is that it can only detect VBUS and that it can only resume on VBUS >>>>> valid. >>>>> >>>>> Does the "LINESTATE" here gets asserted if say there's a LFPS detection? >>>> >>>> Yes. The wake up logic on the SoC is snooping the UTMI lines from the PHY and on any >>>> change it can detect and wake up the SoC. >>>> >>> >>> Are you referring to the utmi_linestate signal? Isn't that for usb2 >>> speed only? Does your platform support usb3 speed? >> >> The wake-up on deepSleep feature is only supported for USB2 on this particular SoC. >> > > I mean can your platform operate in usb3 speed. If that's the case, then > how do you plan to handle it here. No, this SoC can only support up to USB2 speed. > > Also, when you tested this in highspeed, did you observe successful > resume? Or did the host have to perform a port reset? I definitely didn't see a disconnect. Not sure about port reset. I will check and get back. cheers, -roger ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2023-04-13 11:32 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-20 9:34 [RFC PATCH 0/2] usb: dwc3: Support wake-up from USB suspend Roger Quadros 2023-03-20 9:34 ` [RFC PATCH 1/2] dt-bindings: usb: snps,dwc3: Add 'snps,gadget-keep-connect-sys-sleep' Roger Quadros 2023-03-20 13:11 ` Rob Herring 2023-03-20 13:22 ` Rob Herring 2023-03-21 9:44 ` Roger Quadros 2023-03-20 9:34 ` [RFC PATCH 2/2] usb: dwc3: Support 'snps,gadget-keep-connect-sys-sleep' feature Roger Quadros 2023-03-20 18:52 ` Thinh Nguyen 2023-03-21 10:20 ` Roger Quadros 2023-03-21 18:43 ` Thinh Nguyen 2023-03-21 19:05 ` Thinh Nguyen 2023-03-22 8:11 ` Roger Quadros 2023-03-22 17:31 ` Thinh Nguyen 2023-03-23 2:17 ` Thinh Nguyen 2023-03-23 9:29 ` Roger Quadros 2023-03-23 20:51 ` Thinh Nguyen 2023-03-31 11:05 ` Roger Quadros 2023-04-03 23:37 ` Thinh Nguyen 2023-04-04 8:01 ` Roger Quadros 2023-04-04 21:53 ` Thinh Nguyen 2023-04-05 8:56 ` Roger Quadros 2023-04-06 1:38 ` Thinh Nguyen 2023-04-12 7:46 ` Roger Quadros 2023-04-12 20:59 ` Thinh Nguyen 2023-04-13 11:31 ` Roger Quadros
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).