| Message ID | 20231017131851.8299-3-quic_kriskura@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,1/8] dt-bindings: usb: qcom,dwc3: Add bindings to enable runtime | expand |
On 17/10/2023 14:18, Krishna Kurapati wrote: > Currently on QC targets, the conndone/disconnect events in device mode are > generated by controller when software writes to QSCRATCH registers in qcom > glue layer rather than the vbus line being routed to dwc3 core IP for it > to recognize and generate these events. > > We need to write '1' to UTMI_OTG_VBUS_VALID bit of QSCRATCH_HS_PHY_CTRL > register to generate a connection done event and "0" if we need to > generate a disconnect event during cable removal or mode switch. Exactly > what is done by "dwc3_qcom_vbus_override_enable" call in dwc3-qcom. > > When the disconnect is not generated upon cable removal, the connected > flag of dwc3 is left marked as "true" and it blocks runtime suspend. > > The goal of these vendor hooks is to let the mode change and cable removal > notifications from core reach the glue layers so that glue can take > necessary action. > > Before flattening the device tree, glue driver is not sure when the core > probe was completed as core probe can be deferred. In this case, glue is > not sure when to register vendor hooks. So mandate enabling runtime only > for flattened device node platforms so that glue can know when to register > vendor hooks. > > The following are the requirements aimed in this implementation: > > 1. When enum in device mode, Glue/core must stay active. > > 2. When cable is connected but UDC is not written yet, then glue/core > must be suspended. > > 3. Upon removing cable in device mode, the disconnect event must be > generated and unblock runtime suspend for dwc3 core. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> What happens to this code if you static int count; 1. sleep in dwc3_probe for 10 milliseconds 2. return -EPROBE_DEFER 3. if count++ < 5 goto 1 i.e. if we simulate say waiting on a PHY driver to probe in dwc3_probe() and what happens if we introduce a 100 millsecond sleep into dwc3_qcom_probe() - and run a fake disconnect event from dwc3_qcom_probe_core() directly ? In other words if make it that dwc3_probe() completes and struct dwc3_glue_ops->notify_cable_disconnect() fires prior to dwc3_qcom_probe_core() completing ? i.e. I don't immediately see how you've solved the probe() completion race condition here. --- bod
On 11/3/2023 8:44 PM, Bryan O'Donoghue wrote: > On 17/10/2023 14:18, Krishna Kurapati wrote: >> >> The following are the requirements aimed in this implementation: >> >> 1. When enum in device mode, Glue/core must stay active. >> >> 2. When cable is connected but UDC is not written yet, then glue/core >> must be suspended. >> >> 3. Upon removing cable in device mode, the disconnect event must be >> generated and unblock runtime suspend for dwc3 core. >> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > Hi Bryan, > What happens to this code if you > > static int count; > > 1. sleep in dwc3_probe for 10 milliseconds > 2. return -EPROBE_DEFER > 3. if count++ < 5 goto 1 > > i.e. if we simulate say waiting on a PHY driver to probe in dwc3_probe() > The vendor hooks are used in __dwc3_set_mode and role_switch_set calls in core and drd files respectively. These are invoked only if we are OTG capable. The drd_work is initialized in core_init_mode which is called at the end of dwc3_probe. If dwc3_probe fails and gets deferred before that, none of the vendor hooks will be fired and dwc3_qcom_probe is also deferred. However I see that if core_init_mode fails (the cleanup is already done in drd to prevent set_role from getting invoked already), I need to cleanup vendor hooks in error path of dwc3_probe(). > and what happens if we introduce a 100 millsecond sleep into > dwc3_qcom_probe() - and run a fake disconnect event from > dwc3_qcom_probe_core() directly ? > > In other words if make it that dwc3_probe() completes and struct > dwc3_glue_ops->notify_cable_disconnect() fires prior to > dwc3_qcom_probe_core() completing ? > > i.e. I don't immediately see how you've solved the probe() completion > race condition here. > Just wanted to understand the situation clearly. Is this the sequence you are referring to ? 1. dwc3_probe is successful and role switch is registered properly. 2. added delay after dwc3_qcom_probe_core and before interconnect_init 3. Between this delay, we got a disconnect notificiation from glink 4. We are clearing the qscratch reg in case of device mode and un-registering notifier in case of host mode. If so, firstly I don't see any issue if we process disconnect event before qcom probe is complete. If we reached this stage, the clocks/gdsc is definitely ON and register accesses are good to go. If we are in host mode at this point, we would just unregister to usb-core notifier and mark last busy. If we are in device mode, we would just clear the hs_phy_ctrl reg of qscratch. After the 100ms delay you mentioned we would call dwc3_remove anyways and cleanup the vendor hooks. But is the concern here that, what if we enter runtime_suspend at this point ? Regards, Krishna,
On 11/4/2023 12:15 AM, Krishna Kurapati PSSNV wrote: > > > On 11/3/2023 8:44 PM, Bryan O'Donoghue wrote: >> On 17/10/2023 14:18, Krishna Kurapati wrote: >>> >>> The following are the requirements aimed in this implementation: >>> >>> 1. When enum in device mode, Glue/core must stay active. >>> >>> 2. When cable is connected but UDC is not written yet, then glue/core >>> must be suspended. >>> >>> 3. Upon removing cable in device mode, the disconnect event must be >>> generated and unblock runtime suspend for dwc3 core. >>> >>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> > > Hi Bryan, > >> What happens to this code if you >> >> static int count; >> >> 1. sleep in dwc3_probe for 10 milliseconds >> 2. return -EPROBE_DEFER >> 3. if count++ < 5 goto 1 >> >> i.e. if we simulate say waiting on a PHY driver to probe in dwc3_probe() >> > The vendor hooks are used in __dwc3_set_mode and role_switch_set calls > in core and drd files respectively. These are invoked only if we are OTG > capable. The drd_work is initialized in core_init_mode which is called > at the end of dwc3_probe. If dwc3_probe fails and gets deferred before > that, none of the vendor hooks will be fired and dwc3_qcom_probe is also > deferred. > > However I see that if core_init_mode fails (the cleanup is already done > in drd to prevent set_role from getting invoked already), I need to > cleanup vendor hooks in error path of dwc3_probe(). > >> and what happens if we introduce a 100 millsecond sleep into >> dwc3_qcom_probe() - and run a fake disconnect event from >> dwc3_qcom_probe_core() directly ? >> >> In other words if make it that dwc3_probe() completes and struct >> dwc3_glue_ops->notify_cable_disconnect() fires prior to >> dwc3_qcom_probe_core() completing ? >> >> i.e. I don't immediately see how you've solved the probe() completion >> race condition here. >> > Just wanted to understand the situation clearly. Is this the sequence > you are referring to ? > > 1. dwc3_probe is successful and role switch is registered properly. > 2. added delay after dwc3_qcom_probe_core and before interconnect_init > 3. Between this delay, we got a disconnect notificiation from glink > 4. We are clearing the qscratch reg in case of device mode and > un-registering notifier in case of host mode. > > If so, firstly I don't see any issue if we process disconnect event > before qcom probe is complete. If we reached this stage, the clocks/gdsc > is definitely ON and register accesses are good to go. > > If we are in host mode at this point, we would just unregister to > usb-core notifier and mark last busy. If we are in device mode, we would > just clear the hs_phy_ctrl reg of qscratch. After the 100ms delay you > mentioned we would call dwc3_remove anyways and cleanup the vendor > hooks. But is the concern here that, what if we enter runtime_suspend at > this point ? > Just to clarify one more thing. The probe completion requirement came in because, before the device tree was flattened, dwc3-qcom and core are two different platform devices. And if the dwc3 core device probe got deferred, dwc3-qcom probe still gets successfully completed. The glue would never know when to register vendor hook callbacks to dwc3-core as it would never know when the core probe was completed. That is the reason we wanted to find out accurate point where core probe is done to ensure we can properly register these callbacks. Regards, Krishna,
On 03/11/2023 18:49, Krishna Kurapati PSSNV wrote: > > > On 11/4/2023 12:15 AM, Krishna Kurapati PSSNV wrote: >> >> >> On 11/3/2023 8:44 PM, Bryan O'Donoghue wrote: >>> On 17/10/2023 14:18, Krishna Kurapati wrote: >>>> >>>> The following are the requirements aimed in this implementation: >>>> >>>> 1. When enum in device mode, Glue/core must stay active. >>>> >>>> 2. When cable is connected but UDC is not written yet, then glue/core >>>> must be suspended. >>>> >>>> 3. Upon removing cable in device mode, the disconnect event must be >>>> generated and unblock runtime suspend for dwc3 core. >>>> >>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >>> >> >> Hi Bryan, >> >>> What happens to this code if you >>> >>> static int count; >>> >>> 1. sleep in dwc3_probe for 10 milliseconds >>> 2. return -EPROBE_DEFER >>> 3. if count++ < 5 goto 1 >>> >>> i.e. if we simulate say waiting on a PHY driver to probe in dwc3_probe() >>> >> The vendor hooks are used in __dwc3_set_mode and role_switch_set calls >> in core and drd files respectively. These are invoked only if we are >> OTG capable. The drd_work is initialized in core_init_mode which is >> called at the end of dwc3_probe. If dwc3_probe fails and gets deferred >> before that, none of the vendor hooks will be fired and >> dwc3_qcom_probe is also deferred. >> >> However I see that if core_init_mode fails (the cleanup is already >> done in drd to prevent set_role from getting invoked already), I need >> to cleanup vendor hooks in error path of dwc3_probe(). >> >>> and what happens if we introduce a 100 millsecond sleep into >>> dwc3_qcom_probe() - and run a fake disconnect event from >>> dwc3_qcom_probe_core() directly ? >>> >>> In other words if make it that dwc3_probe() completes and struct >>> dwc3_glue_ops->notify_cable_disconnect() fires prior to >>> dwc3_qcom_probe_core() completing ? >>> >>> i.e. I don't immediately see how you've solved the probe() completion >>> race condition here. >>> >> Just wanted to understand the situation clearly. Is this the sequence >> you are referring to ? >> >> 1. dwc3_probe is successful and role switch is registered properly. >> 2. added delay after dwc3_qcom_probe_core and before interconnect_init >> 3. Between this delay, we got a disconnect notificiation from glink >> 4. We are clearing the qscratch reg in case of device mode and >> un-registering notifier in case of host mode. >> >> If so, firstly I don't see any issue if we process disconnect event >> before qcom probe is complete. If we reached this stage, the >> clocks/gdsc is definitely ON and register accesses are good to go. >> >> If we are in host mode at this point, we would just unregister to >> usb-core notifier and mark last busy. If we are in device mode, we >> would just clear the hs_phy_ctrl reg of qscratch. After the 100ms >> delay you mentioned we would call dwc3_remove anyways and cleanup the >> vendor hooks. But is the concern here that, what if we enter >> runtime_suspend at this point ? >> > > Just to clarify one more thing. The probe completion requirement came in > because, before the device tree was flattened, dwc3-qcom and core are > two different platform devices. And if the dwc3 core device probe got > deferred, dwc3-qcom probe still gets successfully completed. The glue > would never know when to register vendor hook callbacks to dwc3-core as > it would never know when the core probe was completed. > > That is the reason we wanted to find out accurate point where core probe > is done to ensure we can properly register these callbacks. Are you saying to you require/rely on both of these series being applied first ? [1]: https://lore.kernel.org/all/af60c05b-4a0f-51b8-486a-1fc601602515@quicinc.com/ [2]: https://lore.kernel.org/all/20231016-dwc3-refactor-v1-0-ab4a84165470@quicinc.com/ Must be, nothing applies for me in this series.
>>> Hi Bryan, >>> >>>> What happens to this code if you >>>> >>>> static int count; >>>> >>>> 1. sleep in dwc3_probe for 10 milliseconds >>>> 2. return -EPROBE_DEFER >>>> 3. if count++ < 5 goto 1 >>>> >>>> i.e. if we simulate say waiting on a PHY driver to probe in >>>> dwc3_probe() >>>> >>> The vendor hooks are used in __dwc3_set_mode and role_switch_set >>> calls in core and drd files respectively. These are invoked only if >>> we are OTG capable. The drd_work is initialized in core_init_mode >>> which is called at the end of dwc3_probe. If dwc3_probe fails and >>> gets deferred before that, none of the vendor hooks will be fired and >>> dwc3_qcom_probe is also deferred. >>> >>> However I see that if core_init_mode fails (the cleanup is already >>> done in drd to prevent set_role from getting invoked already), I >>> need to cleanup vendor hooks in error path of dwc3_probe(). >>> >>>> and what happens if we introduce a 100 millsecond sleep into >>>> dwc3_qcom_probe() - and run a fake disconnect event from >>>> dwc3_qcom_probe_core() directly ? >>>> >>>> In other words if make it that dwc3_probe() completes and struct >>>> dwc3_glue_ops->notify_cable_disconnect() fires prior to >>>> dwc3_qcom_probe_core() completing ? >>>> >>>> i.e. I don't immediately see how you've solved the probe() >>>> completion race condition here. >>>> >>> Just wanted to understand the situation clearly. Is this the sequence >>> you are referring to ? >>> >>> 1. dwc3_probe is successful and role switch is registered properly. >>> 2. added delay after dwc3_qcom_probe_core and before interconnect_init >>> 3. Between this delay, we got a disconnect notificiation from glink >>> 4. We are clearing the qscratch reg in case of device mode and >>> un-registering notifier in case of host mode. >>> >>> If so, firstly I don't see any issue if we process disconnect event >>> before qcom probe is complete. If we reached this stage, the >>> clocks/gdsc is definitely ON and register accesses are good to go. >>> >>> If we are in host mode at this point, we would just unregister to >>> usb-core notifier and mark last busy. If we are in device mode, we >>> would just clear the hs_phy_ctrl reg of qscratch. After the 100ms >>> delay you mentioned we would call dwc3_remove anyways and cleanup the >>> vendor hooks. But is the concern here that, what if we enter >>> runtime_suspend at this point ? >>> >> >> Just to clarify one more thing. The probe completion requirement came >> in because, before the device tree was flattened, dwc3-qcom and core >> are two different platform devices. And if the dwc3 core device probe >> got deferred, dwc3-qcom probe still gets successfully completed. The >> glue would never know when to register vendor hook callbacks to >> dwc3-core as it would never know when the core probe was completed. >> >> That is the reason we wanted to find out accurate point where core >> probe is done to ensure we can properly register these callbacks. > > Are you saying to you require/rely on both of these series being applied > first ? > > [1]: > https://lore.kernel.org/all/af60c05b-4a0f-51b8-486a-1fc601602515@quicinc.com/ > [2]: > https://lore.kernel.org/all/20231016-dwc3-refactor-v1-0-ab4a84165470@quicinc.com/ > > Must be, nothing applies for me in this series. The first one is not a patch. It is just a discussion thread I started to get community's opinion before on disconnect interrupt handling. The current series is based on top of [2] made by Bjorn (as you already found out) and as I mentioned in cover letter of my series. Regards, Krishna,
On 07/11/2023 08:33, Krishna Kurapati PSSNV wrote: > > > On 11/4/2023 10:32 PM, Krishna Kurapati PSSNV wrote: >>> >>> Are you saying to you require/rely on both of these series being >>> applied first ? >>> >>> [1]: >>> https://lore.kernel.org/all/af60c05b-4a0f-51b8-486a-1fc601602515@quicinc.com/ >>> [2]: >>> https://lore.kernel.org/all/20231016-dwc3-refactor-v1-0-ab4a84165470@quicinc.com/ >>> >>> Must be, nothing applies for me in this series. >> >> The first one is not a patch. It is just a discussion thread I started >> to get community's opinion before on disconnect interrupt handling. >> The current series is based on top of [2] made by Bjorn (as you >> already found out) and as I mentioned in cover letter of my series. >> > > Hi Bryan, > > Are you able to apply the series after including Bjorn's patches ? > Also can you confirm if the comments provided to your queries on [1] are > proper and if you have any other comments w.r.t probe deferral. > > [1]: > https://lore.kernel.org/all/e700133b-58f7-4a4d-8e5c-0d04441b789b@linaro.org/ > > Regards, > Krishna, I wonder could you give a base SHA to apply the various series on ? Your referenced precursor doesn't apply to usb-next deckard@sagittarius-a:~/Development/qualcomm/qlt-kernel$ b4 shazam 20231016-dwc3-refactor-v1-0-ab4a84165470@quicinc.com Grabbing thread from lore.kernel.org/all/20231016-dwc3-refactor-v1-0-ab4a84165470@quicinc.com/t.mbox.gz Checking for newer revisions Grabbing search results from lore.kernel.org Analyzing 27 messages in the thread Checking attestation on all messages, may take a moment... --- [PATCH 1/12] dt-bindings: usb: qcom,dwc3: Add qcom,sc8180x-dwc3 [PATCH 2/12] usb: dwc3: qcom: Rename dwc3 platform_device reference + Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> [PATCH 3/12] usb: dwc3: qcom: Merge resources from urs_usb device [PATCH 4/12] usb: dwc3: Expose core driver as library [PATCH 5/12] usb: dwc3: Override end of dwc3 memory resource + Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org> [PATCH 6/12] usb: dwc3: qcom: Add dwc3 core reference in driver state [PATCH 7/12] usb: dwc3: qcom: Instantiate dwc3 core directly [PATCH 8/12] usb: dwc3: qcom: Inline the qscratch constants + Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> [PATCH 9/12] dt-bindings: usb: qcom,dwc3: Rename to "glue" + Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> [PATCH 10/12] dt-bindings: usb: qcom,dwc3: Introduce flattened qcom,dwc3 binding [PATCH 11/12] usb: dwc3: qcom: Flatten the Qualcomm dwc3 binding and implementation [PATCH 12/12] arm64: dts: qcom: sc8180x: flatten usb_sec node --- ✗ No key: ed25519/quic_bjorande@quicinc.com --- NOTE: install dkimpy for DKIM signature verification --- Total patches: 12 --- Base: using specified base-commit 4d0515b235dec789578d135a5db586b25c5870cb Applying: dt-bindings: usb: qcom,dwc3: Add qcom,sc8180x-dwc3 Patch failed at 0001 dt-bindings: usb: qcom,dwc3: Add qcom,sc8180x-dwc3 When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". error: patch failed: Documentation/devicetree/bindings/usb/qcom,dwc3.yaml:29 error: Documentation/devicetree/bindings/usb/qcom,dwc3.yaml: patch does not apply hint: Use 'git am --show-current-patch=diff' to see the failed patch deckard@sagittarius-a:~/Development/qualcomm/qlt-kernel$ git diff deckard@sagittarius-a:~/Development/qualcomm/qlt-kernel$ git am --skip Applying: usb: dwc3: qcom: Rename dwc3 platform_device reference error: patch failed: drivers/usb/dwc3/dwc3-qcom.c:67 error: drivers/usb/dwc3/dwc3-qcom.c: patch does not apply Patch failed at 0002 usb: dwc3: qcom: Rename dwc3 platform_device reference hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". deckard@sagittarius-a:~/Development/qualcomm/qlt-kernel$ git am --skip Applying: usb: dwc3: qcom: Merge resources from urs_usb device error: patch failed: drivers/usb/dwc3/dwc3-qcom.c:68 error: drivers/usb/dwc3/dwc3-qcom.c: patch does not apply Patch failed at 0003 usb: dwc3: qcom: Merge resources from urs_usb device hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". deckard@sagittarius-a:~/Development/qualcomm/qlt-kernel$ git am --skip Applying: usb: dwc3: Expose core driver as library error: patch failed: drivers/usb/dwc3/core.c:1876 error: drivers/usb/dwc3/core.c: patch does not apply error: patch failed: drivers/usb/dwc3/core.h:1568 error: drivers/usb/dwc3/core.h: patch does not apply Patch failed at 0004 usb: dwc3: Expose core driver as library hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". deckard@sagittarius-a:~/Development/qualcomm/qlt-kernel$ git am --skip Applying: usb: dwc3: Override end of dwc3 memory resource error: patch failed: drivers/usb/dwc3/core.c:1908 error: drivers/usb/dwc3/core.c: patch does not apply Patch failed at 0005 usb: dwc3: Override end of dwc3 memory resource hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". deckard@sagittarius-a:~/Development/qualcomm/qlt-kernel$ git am --skip Applying: usb: dwc3: qcom: Add dwc3 core reference in driver state error: patch failed: drivers/usb/dwc3/dwc3-qcom.c:67 error: drivers/usb/dwc3/dwc3-qcom.c: patch does not apply Patch failed at 0006 usb: dwc3: qcom: Add dwc3 core reference in driver state hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem, run "git am --continue". If you prefer to skip this patch, run "git am --skip" instead. To restore the original branch and stop patching, run "git am --abort". deckard@sagittarius-a:~/Development/qualcomm/qlt-kernel$ git am --abort --- bod
On 07/11/2023 10:41, Bryan O'Donoghue wrote: > On 07/11/2023 08:33, Krishna Kurapati PSSNV wrote: >> >> >> On 11/4/2023 10:32 PM, Krishna Kurapati PSSNV wrote: >>>> >>>> Are you saying to you require/rely on both of these series being >>>> applied first ? >>>> >>>> [1]: >>>> https://lore.kernel.org/all/af60c05b-4a0f-51b8-486a-1fc601602515@quicinc.com/ >>>> [2]: >>>> https://lore.kernel.org/all/20231016-dwc3-refactor-v1-0-ab4a84165470@quicinc.com/ >>>> >>>> Must be, nothing applies for me in this series. >>> >>> The first one is not a patch. It is just a discussion thread I >>> started to get community's opinion before on disconnect interrupt >>> handling. The current series is based on top of [2] made by Bjorn (as >>> you already found out) and as I mentioned in cover letter of my series. >>> >> >> Hi Bryan, >> >> Are you able to apply the series after including Bjorn's patches ? >> Also can you confirm if the comments provided to your queries on [1] >> are proper and if you have any other comments w.r.t probe deferral. >> >> [1]: >> https://lore.kernel.org/all/e700133b-58f7-4a4d-8e5c-0d04441b789b@linaro.org/ >> >> Regards, >> Krishna, > > I wonder could you give a base SHA to apply the various series on ? > > Your referenced precursor doesn't apply to usb-next Well now, that doesn't point where I thought it pointed usb-next/master is extremely old b3a9e3b9622ae - (HEAD -> usb-next-23-10-07-usb-glue-test, tag: v5.8-rc1, usb-next/master, origin/tracking-qcomlt-sm8150-gcc, linaro/tracking-qcomlt-sm8150-gcc, fecked-old, delete-this-branch2, delete-this-branch) Linux 5.8-rc1 (3 years, 5 months ago) I want usb-next/main * d2f51b3516dad - (usb-next/usb-testing, usb-next/usb-next, usb-next/main) Merge tag 'rtc-6.7' of git://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux (32 hours ago) Everything applies there. Anyway, your pointing to Bjorn's series answers my question re: sequencing of the probe. --- bod
On 11/7/2023 4:25 PM, Bryan O'Donoghue wrote: > On 07/11/2023 10:41, Bryan O'Donoghue wrote: >> On 07/11/2023 08:33, Krishna Kurapati PSSNV wrote: >>> >>> >>> On 11/4/2023 10:32 PM, Krishna Kurapati PSSNV wrote: >>>>> >>>>> Are you saying to you require/rely on both of these series being >>>>> applied first ? >>>>> >>>>> [1]: >>>>> https://lore.kernel.org/all/af60c05b-4a0f-51b8-486a-1fc601602515@quicinc.com/ >>>>> [2]: >>>>> https://lore.kernel.org/all/20231016-dwc3-refactor-v1-0-ab4a84165470@quicinc.com/ >>>>> >>>>> Must be, nothing applies for me in this series. >>>> >>>> The first one is not a patch. It is just a discussion thread I >>>> started to get community's opinion before on disconnect interrupt >>>> handling. The current series is based on top of [2] made by Bjorn >>>> (as you already found out) and as I mentioned in cover letter of my >>>> series. >>>> >>> >>> Hi Bryan, >>> >>> Are you able to apply the series after including Bjorn's patches ? >>> Also can you confirm if the comments provided to your queries on [1] >>> are proper and if you have any other comments w.r.t probe deferral. >>> >>> [1]: >>> https://lore.kernel.org/all/e700133b-58f7-4a4d-8e5c-0d04441b789b@linaro.org/ >>> >>> Regards, >>> Krishna, >> >> I wonder could you give a base SHA to apply the various series on ? >> >> Your referenced precursor doesn't apply to usb-next > > Well now, that doesn't point where I thought it pointed usb-next/master > is extremely old > > b3a9e3b9622ae - (HEAD -> usb-next-23-10-07-usb-glue-test, tag: > v5.8-rc1, usb-next/master, origin/tracking-qcomlt-sm8150-gcc, > linaro/tracking-qcomlt-sm8150-gcc, fecked-old, delete-this-branch2, > delete-this-branch) Linux 5.8-rc1 (3 years, 5 months ago) > > I want usb-next/main > > * d2f51b3516dad - (usb-next/usb-testing, usb-next/usb-next, > usb-next/main) Merge tag 'rtc-6.7' of > git://git.kernel.org/pub/scm/linux/kernel/git/abelloni/linux (32 hours ago) > > Everything applies there. Hi Bryan, I should have mentioned that series is pushed on top of usb-next. Apologies. > > Anyway, your pointing to Bjorn's series answers my question re: > sequencing of the probe. Perfect. Thanks for the confirmation. Regards, Krishna,
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 923a6c623850..53a8d92ad663 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -137,6 +137,8 @@ static void __dwc3_set_mode(struct work_struct *work) if (!desired_dr_role) goto out; + dwc3_notify_set_mode(dwc, desired_dr_role); + if (desired_dr_role == dwc->current_dr_role) goto out; @@ -1751,7 +1753,8 @@ static int dwc3_get_clocks(struct dwc3 *dwc) return 0; } -struct dwc3 *dwc3_probe(struct platform_device *pdev) +struct dwc3 *dwc3_probe(struct platform_device *pdev, + struct dwc3_glue_data *glue_data) { struct device *dev = &pdev->dev; struct resource *res, dwc_res; @@ -1765,6 +1768,11 @@ struct dwc3 *dwc3_probe(struct platform_device *pdev) dwc->dev = dev; + if (glue_data) { + dwc->glue_data = glue_data->glue_data; + dwc->glue_ops = glue_data->ops; + } + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (!res) { dev_err(dev, "missing memory resource\n"); @@ -1914,7 +1922,7 @@ static int dwc3_plat_probe(struct platform_device *pdev) { struct dwc3 *dwc; - dwc = dwc3_probe(pdev); + dwc = dwc3_probe(pdev, NULL); if (IS_ERR(dwc)) return PTR_ERR(dwc); @@ -1925,6 +1933,9 @@ static int dwc3_plat_probe(struct platform_device *pdev) void dwc3_remove(struct dwc3 *dwc) { + dwc->glue_data = NULL; + dwc->glue_ops = NULL; + pm_runtime_get_sync(dwc->dev); dwc3_core_exit_mode(dwc); diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index 3ec4a961befe..aefcb0d388b7 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -968,6 +968,23 @@ struct dwc3_scratchpad_array { __le64 dma_adr[DWC3_MAX_HIBER_SCRATCHBUFS]; }; +/* + * struct dwc3_glue_ops - The ops indicate the notifications that + * need to be passed on to glue layer. + * @notify_cable_disconnect: Notify glue of cable removal + * irrespective of host or device mode. + * @set_mode: Notify glue before mode change is about to happen. + */ +struct dwc3_glue_ops { + void (*notify_cable_disconnect)(void *glue_data); + void (*set_mode)(void *glue_data, u32 desired_dr_role); +}; + +struct dwc3_glue_data { + void *glue_data; + struct dwc3_glue_ops *ops; +}; + /** * struct dwc3 - representation of our controller * @drd_work: workqueue used for role swapping @@ -1124,6 +1141,9 @@ struct dwc3_scratchpad_array { * @num_ep_resized: carries the current number endpoints which have had its tx * fifo resized. * @debug_root: root debugfs directory for this device to put its files in. + * @glue_data: Private data stored by core to be passed on to glue during + * role switch notifications. + * @glue_ops: Store pointers to glue notifcation callbacks. */ struct dwc3 { struct work_struct drd_work; @@ -1340,6 +1360,9 @@ struct dwc3 { int last_fifo_depth; int num_ep_resized; struct dentry *debug_root; + + void *glue_data; + const struct dwc3_glue_ops *glue_ops; }; #define INCRX_BURST_MODE 0 @@ -1553,7 +1576,8 @@ void dwc3_event_buffers_cleanup(struct dwc3 *dwc); int dwc3_core_soft_reset(struct dwc3 *dwc); -struct dwc3 *dwc3_probe(struct platform_device *pdev); +struct dwc3 *dwc3_probe(struct platform_device *pdev, + struct dwc3_glue_data *glue_data); void dwc3_remove(struct dwc3 *dwc); int dwc3_runtime_suspend(struct dwc3 *dwc); @@ -1563,6 +1587,19 @@ int dwc3_suspend(struct dwc3 *dwc); int dwc3_resume(struct dwc3 *dwc); void dwc3_complete(struct dwc3 *dwc); +static inline void dwc3_notify_cable_disconnect(struct dwc3 *dwc) +{ + if (dwc->glue_ops && dwc->glue_ops->notify_cable_disconnect) + dwc->glue_ops->notify_cable_disconnect(dwc->glue_data); +} + +static inline void dwc3_notify_set_mode(struct dwc3 *dwc, + u32 desired_dr_role) +{ + if (dwc->glue_ops && dwc->glue_ops->set_mode) + dwc->glue_ops->set_mode(dwc->glue_data, desired_dr_role); +} + #if IS_ENABLED(CONFIG_USB_DWC3_HOST) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE) int dwc3_host_init(struct dwc3 *dwc); void dwc3_host_exit(struct dwc3 *dwc); diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c index 039bf241769a..947faeef0e4d 100644 --- a/drivers/usb/dwc3/drd.c +++ b/drivers/usb/dwc3/drd.c @@ -461,6 +461,15 @@ static int dwc3_usb_role_switch_set(struct usb_role_switch *sw, break; } + /* + * When cable is removed, the role changes to default mode. + * In the case we are in device mode and removed the cable, the + * glue needs to know that we are disconnected. It must not notify + * the change of mode to default mode. + */ + if (role == USB_ROLE_NONE) + dwc3_notify_cable_disconnect(dwc); + dwc3_set_mode(dwc, mode); return 0; } diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index 3c9a2b5cd559..4013a5e6c6c0 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -88,6 +88,9 @@ struct dwc3_qcom { bool pm_suspended; struct icc_path *icc_path_ddr; struct icc_path *icc_path_apps; + + bool enable_rt; + enum usb_role current_role; }; static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val) @@ -673,16 +676,71 @@ static const struct software_node dwc3_qcom_swnode = { .properties = dwc3_qcom_acpi_properties, }; +static void dwc3_qcom_handle_cable_disconnect(void *data) +{ + struct dwc3_qcom *qcom = (struct dwc3_qcom *)data; + /* + * If we are in device mode and get a cable disconnect, + * handle it by clearing OTG_VBUS_VALID bit in wrapper. + * The next set_mode to default role can be ignored. + */ + if (qcom->current_role == USB_ROLE_DEVICE) { + pm_runtime_get_sync(qcom->dev); + dwc3_qcom_vbus_override_enable(qcom, false); + pm_runtime_put_autosuspend(qcom->dev); + } + + pm_runtime_mark_last_busy(qcom->dev); + qcom->current_role = USB_ROLE_NONE; +} + +static void dwc3_qcom_handle_set_mode(void *data, u32 desired_dr_role) +{ + struct dwc3_qcom *qcom = (struct dwc3_qcom *)data; + + /* + * If we are in device mode and get a cable disconnect, + * handle it by clearing OTG_VBUS_VALID bit in wrapper. + * The next set_mode to default role can be ignored and + * so the OTG_VBUS_VALID should be set iff the current role + * is NONE and we need to enter DEVICE mode. + */ + if ((qcom->current_role == USB_ROLE_NONE) && + (desired_dr_role == DWC3_GCTL_PRTCAP_DEVICE)) { + dwc3_qcom_vbus_override_enable(qcom, true); + qcom->current_role = USB_ROLE_DEVICE; + } else if ((desired_dr_role == DWC3_GCTL_PRTCAP_HOST) && + (qcom->current_role != USB_ROLE_HOST)) { + qcom->current_role = USB_ROLE_HOST; + } + + pm_runtime_mark_last_busy(qcom->dev); +} + +struct dwc3_glue_ops dwc3_qcom_glue_hooks = { + .notify_cable_disconnect = dwc3_qcom_handle_cable_disconnect, + .set_mode = dwc3_qcom_handle_set_mode, +}; + static int dwc3_qcom_probe_core(struct platform_device *pdev, struct dwc3_qcom *qcom) { struct dwc3 *dwc; - dwc = dwc3_probe(pdev); + struct dwc3_glue_data qcom_glue_data = { + .glue_data = qcom, + .ops = &dwc3_qcom_glue_hooks, + }; + + dwc = dwc3_probe(pdev, + qcom->enable_rt ? &qcom_glue_data : NULL); if (IS_ERR(dwc)) return PTR_ERR(dwc); qcom->dwc = dwc; + if (qcom->enable_rt) + pm_runtime_allow(qcom->dev); + return 0; } @@ -897,6 +955,23 @@ static int dwc3_qcom_probe(struct platform_device *pdev) if (ignore_pipe_clk) dwc3_qcom_select_utmi_clk(qcom); + qcom->enable_rt = device_property_read_bool(dev, + "qcom,enable-rt"); + if (!legacy_binding) { + /* + * If we are enabling runtime, then we are using flattened + * device implementation. + */ + qcom->mode = usb_get_dr_mode(dev); + + if (qcom->mode == USB_DR_MODE_HOST) + qcom->current_role = USB_ROLE_HOST; + else if (qcom->mode == USB_DR_MODE_PERIPHERAL) + qcom->current_role = USB_ROLE_DEVICE; + else + qcom->current_role = USB_ROLE_NONE; + } + if (legacy_binding) ret = dwc3_qcom_of_register_core(pdev); else @@ -913,8 +988,6 @@ static int dwc3_qcom_probe(struct platform_device *pdev) if (qcom->dwc_dev) qcom->mode = usb_get_dr_mode(&qcom->dwc_dev->dev); - else - qcom->mode = usb_get_dr_mode(dev); /* enable vbus override for device mode */ if (qcom->mode != USB_DR_MODE_HOST)
Currently on QC targets, the conndone/disconnect events in device mode are generated by controller when software writes to QSCRATCH registers in qcom glue layer rather than the vbus line being routed to dwc3 core IP for it to recognize and generate these events. We need to write '1' to UTMI_OTG_VBUS_VALID bit of QSCRATCH_HS_PHY_CTRL register to generate a connection done event and "0" if we need to generate a disconnect event during cable removal or mode switch. Exactly what is done by "dwc3_qcom_vbus_override_enable" call in dwc3-qcom. When the disconnect is not generated upon cable removal, the connected flag of dwc3 is left marked as "true" and it blocks runtime suspend. The goal of these vendor hooks is to let the mode change and cable removal notifications from core reach the glue layers so that glue can take necessary action. Before flattening the device tree, glue driver is not sure when the core probe was completed as core probe can be deferred. In this case, glue is not sure when to register vendor hooks. So mandate enabling runtime only for flattened device node platforms so that glue can know when to register vendor hooks. The following are the requirements aimed in this implementation: 1. When enum in device mode, Glue/core must stay active. 2. When cable is connected but UDC is not written yet, then glue/core must be suspended. 3. Upon removing cable in device mode, the disconnect event must be generated and unblock runtime suspend for dwc3 core. Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> --- drivers/usb/dwc3/core.c | 15 ++++++- drivers/usb/dwc3/core.h | 39 +++++++++++++++++- drivers/usb/dwc3/drd.c | 9 ++++ drivers/usb/dwc3/dwc3-qcom.c | 79 ++++++++++++++++++++++++++++++++++-- 4 files changed, 136 insertions(+), 6 deletions(-)