Disconnect interrupt generation for QC targets when role switch is enabled

Disconnect interrupt generation for QC targets when role switch is enabled

[Krishna Kurapati PSSNV]

Hi Thinh, Johan, Bjorn, Konrad,

   On QC targets today, the disconnect event in device mode is 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 conndone/disconnect 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. In 
case the user wants to enable runtime suspend for dwc3 and we don't 
generate a disconnect event, the dwc->connected flag will be "true" and 
it would block suspend entry.

Today, in dwc3-qcom, this qscratch modification is being done only for 
vbus/host notifiers where I assume dwc3-qcom would be receiving these 
notifications from charger driver regarding cable connect and removal 
and since we are receiving a copy of the information in dwc3-qcom as 
well, we would be knowing when to set/clear the VBUS_VALID bit.

But, when we have role switch in play, the role_set callback goes to DRD 
and Qcom driver is agnostic of what is actually happening. While this 
doesn't hinder mode switch, the role change notification never really 
reaches dwc3-qcom and we would never set this bit to "0" and disconnect 
event is not generated upon cable plug-out. Is there a way we can 
properly provide this notification to qcom glue driver ?

I had some idea on how to get the role notification reach qcom glue 
driver but wanted your opinion on whether they can be used or not:

1. Register a vendor_hook from glue driver and invoke that during 
__dwc3_set_mode.

2. Let the role notification reach dwc3-qcom first and then let qcom 
driver invoke role_set of drd. Something similar to what was implemented 
by Wesley on [1].

But both the options require dwc3_probe to be done in sync with 
of_platform_populate or we need to defer qcom probe if dwc3_probe is 
deferred. Since we are leaning towards async probe, not sure if the 
above two options would be proper.

Couple of more reasons to ask for the above two options:

1. We would know when we are in device/host mode and we can avoid having 
any layering violations in dwc3-qcom.

2. When it comes to runtime pm implementation for dwc3-qcom, an effort 
was done by Manivannan Sadhasivam previously [2], we can avoid modifying 
xhci-plat.c to set autosuspend delay to any specific value. We would be 
able to enable runtime/autosuspend for dwc->xhci->dev from dwc3-qcom 
itself upon entering host mode.

Can you help let know your thoughts on the above two options or if there 
is any way we can implement the role change / cable plug in(out) callback.

[1]: 
https://patchwork.kernel.org/project/linux-usb/patch/20201009082843.28503-4-wcheng@codeaurora.org/
[2]: 
https://patchwork.kernel.org/project/linux-usb/cover/20230325165217.31069-1-manivannan.sadhasivam@linaro.org/

Regards,
Krishna,

Re: Disconnect interrupt generation for QC targets when role switch is enabled

[Manivannan Sadhasivam]

On Fri, Aug 11, 2023 at 05:21:03PM +0530, Krishna Kurapati PSSNV wrote:
> Hi Thinh, Johan, Bjorn, Konrad,
> 
>   On QC targets today, the disconnect event in device mode is 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 conndone/disconnect 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. In case the user
> wants to enable runtime suspend for dwc3 and we don't generate a disconnect
> event, the dwc->connected flag will be "true" and it would block suspend
> entry.
> 
> Today, in dwc3-qcom, this qscratch modification is being done only for
> vbus/host notifiers where I assume dwc3-qcom would be receiving these
> notifications from charger driver regarding cable connect and removal and
> since we are receiving a copy of the information in dwc3-qcom as well, we
> would be knowing when to set/clear the VBUS_VALID bit.
> 
> But, when we have role switch in play, the role_set callback goes to DRD and
> Qcom driver is agnostic of what is actually happening. While this doesn't
> hinder mode switch, the role change notification never really reaches
> dwc3-qcom and we would never set this bit to "0" and disconnect event is not
> generated upon cable plug-out. Is there a way we can properly provide this
> notification to qcom glue driver ?
> 
> I had some idea on how to get the role notification reach qcom glue driver
> but wanted your opinion on whether they can be used or not:
> 
> 1. Register a vendor_hook from glue driver and invoke that during
> __dwc3_set_mode.
> 
> 2. Let the role notification reach dwc3-qcom first and then let qcom driver
> invoke role_set of drd. Something similar to what was implemented by Wesley
> on [1].
> 
> But both the options require dwc3_probe to be done in sync with
> of_platform_populate or we need to defer qcom probe if dwc3_probe is
> deferred. Since we are leaning towards async probe, not sure if the above
> two options would be proper.
> 
> Couple of more reasons to ask for the above two options:
> 
> 1. We would know when we are in device/host mode and we can avoid having any
> layering violations in dwc3-qcom.
> 
> 2. When it comes to runtime pm implementation for dwc3-qcom, an effort was
> done by Manivannan Sadhasivam previously [2], we can avoid modifying
> xhci-plat.c to set autosuspend delay to any specific value. We would be able
> to enable runtime/autosuspend for dwc->xhci->dev from dwc3-qcom itself upon
> entering host mode.
> 

Since you have quoted my series, I'd like to get clarification on one issue I'm
seeing. When resuming from runtime suspend in host mode, dwc3-qcom driver gets
the wakeup event and it resumes the xhci driver. But the XHCI IRQs are received
after some delay. Due to this, xhci driver doesn't resume the device, instead,
all the drivers (dwc3-qcom, dwc3-core, xhci) went to runtime suspend again.

But once the XHCI IRQs are received, dwc3-qcom gets the wakeup event and this
time, xhci driver resumes the device.

This is the reason why I added the autosuspend delay of 200ms to allow the xhci
driver to wait for IRQs before going to runtime suspend.

Can you clarify why there is a delay in receiving XHCI IRQs?

- Mani

> Can you help let know your thoughts on the above two options or if there is
> any way we can implement the role change / cable plug in(out) callback.
> 
> [1]: https://patchwork.kernel.org/project/linux-usb/patch/20201009082843.28503-4-wcheng@codeaurora.org/
> [2]: https://patchwork.kernel.org/project/linux-usb/cover/20230325165217.31069-1-manivannan.sadhasivam@linaro.org/
> 
> Regards,
> Krishna,

-- 
மணிவண்ணன் சதாசிவம்

Re: Disconnect interrupt generation for QC targets when role switch is enabled

[Krishna Kurapati PSSNV]

On 8/16/2023 10:24 PM, Manivannan Sadhasivam wrote:
> 
> Since you have quoted my series, I'd like to get clarification on one issue I'm
> seeing. When resuming from runtime suspend in host mode, dwc3-qcom driver gets
> the wakeup event and it resumes the xhci driver. But the XHCI IRQs are received
> after some delay. Due to this, xhci driver doesn't resume the device, instead,
> all the drivers (dwc3-qcom, dwc3-core, xhci) went to runtime suspend again.
> 
> But once the XHCI IRQs are received, dwc3-qcom gets the wakeup event and this
> time, xhci driver resumes the device.
> 
> This is the reason why I added the autosuspend delay of 200ms to allow the xhci
> driver to wait for IRQs before going to runtime suspend.
> 
> Can you clarify why there is a delay in receiving XHCI IRQs?
> 

Hi Manivannan,

  Apologies for the delay in response. Can you help check if the phy 
clocks are ON at the instant we see that the xhci irq not coming up. I 
got to know that after resume, the clocks need to be ON for phy for it 
to signal the controller which would then generate the xhci irq. I also 
see that in resume_irq call in dwc3-qcom, we call runtime_resume for 
xhci->dev, probably causing it to resume before dwc3->dev (I might be 
wrong though). Can you confirm if the clocks for phy and GDSC for 
controller are ON when this issue is seen. Also can you let know how 
much time its taking for the xhci irq to be generated after resume.

Regards,
Krishna,

Re: Disconnect interrupt generation for QC targets when role switch is enabled

[Krishna Kurapati PSSNV]

On 8/29/2023 6:42 PM, Krishna Kurapati PSSNV wrote:
> 
> 
> On 8/16/2023 10:24 PM, Manivannan Sadhasivam wrote:
>>
>> Since you have quoted my series, I'd like to get clarification on one 
>> issue I'm
>> seeing. When resuming from runtime suspend in host mode, dwc3-qcom 
>> driver gets
>> the wakeup event and it resumes the xhci driver. But the XHCI IRQs are 
>> received
>> after some delay. Due to this, xhci driver doesn't resume the device, 
>> instead,
>> all the drivers (dwc3-qcom, dwc3-core, xhci) went to runtime suspend 
>> again.
>>
>> But once the XHCI IRQs are received, dwc3-qcom gets the wakeup event 
>> and this
>> time, xhci driver resumes the device.
>>
>> This is the reason why I added the autosuspend delay of 200ms to allow 
>> the xhci
>> driver to wait for IRQs before going to runtime suspend.
>>
>> Can you clarify why there is a delay in receiving XHCI IRQs?
>>
> 
> Hi Manivannan,
> 
>   Apologies for the delay in response. Can you help check if the phy 
> clocks are ON at the instant we see that the xhci irq not coming up. I 
> got to know that after resume, the clocks need to be ON for phy for it 
> to signal the controller which would then generate the xhci irq. I also 
> see that in resume_irq call in dwc3-qcom, we call runtime_resume for 
> xhci->dev, probably causing it to resume before dwc3->dev (I might be 
> wrong though). Can you confirm if the clocks for phy and GDSC for 
> controller are ON when this issue is seen. Also can you let know how 
> much time its taking for the xhci irq to be generated after resume.
> 
Hi Manivannan,

  Sorry for reviving an old thread. Can you help check whether 
autosuspend is being triggered by pm framework before run_stop bit is 
cleared as part of xhci_suspend in your case ?

I am trying to repro the issue, but I am seeing a different error, "host 
controller error"

Regards,
Krishna,

Re: Disconnect interrupt generation for QC targets when role switch is enabled

[Bjorn Andersson]

On Fri, Aug 11, 2023 at 05:21:03PM +0530, Krishna Kurapati PSSNV wrote:
> Hi Thinh, Johan, Bjorn, Konrad,
> 
>   On QC targets today, the disconnect event in device mode is 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 conndone/disconnect 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. In case the user
> wants to enable runtime suspend for dwc3 and we don't generate a disconnect
> event, the dwc->connected flag will be "true" and it would block suspend
> entry.
> 
> Today, in dwc3-qcom, this qscratch modification is being done only for
> vbus/host notifiers where I assume dwc3-qcom would be receiving these
> notifications from charger driver regarding cable connect and removal and
> since we are receiving a copy of the information in dwc3-qcom as well, we
> would be knowing when to set/clear the VBUS_VALID bit.
> 
> But, when we have role switch in play, the role_set callback goes to DRD and
> Qcom driver is agnostic of what is actually happening. While this doesn't
> hinder mode switch, the role change notification never really reaches
> dwc3-qcom and we would never set this bit to "0" and disconnect event is not
> generated upon cable plug-out. Is there a way we can properly provide this
> notification to qcom glue driver ?
> 
> I had some idea on how to get the role notification reach qcom glue driver
> but wanted your opinion on whether they can be used or not:
> 
> 1. Register a vendor_hook from glue driver and invoke that during
> __dwc3_set_mode.
> 
> 2. Let the role notification reach dwc3-qcom first and then let qcom driver
> invoke role_set of drd. Something similar to what was implemented by Wesley
> on [1].
> 
> But both the options require dwc3_probe to be done in sync with
> of_platform_populate or we need to defer qcom probe if dwc3_probe is
> deferred. Since we are leaning towards async probe, not sure if the above
> two options would be proper.
> 
> Couple of more reasons to ask for the above two options:
> 
> 1. We would know when we are in device/host mode and we can avoid having any
> layering violations in dwc3-qcom.
> 
> 2. When it comes to runtime pm implementation for dwc3-qcom, an effort was
> done by Manivannan Sadhasivam previously [2], we can avoid modifying
> xhci-plat.c to set autosuspend delay to any specific value. We would be able
> to enable runtime/autosuspend for dwc->xhci->dev from dwc3-qcom itself upon
> entering host mode.
> 
> Can you help let know your thoughts on the above two options or if there is
> any way we can implement the role change / cable plug in(out) callback.
> 

As mentioned, this need has been identified a few times by now, so
nothing strange in your request/proposal.

But so far no one has come up with a good way to register glue code
callbacks with the core; we can't pass arbitrary data (such as a
function pointer to such callback), and we don't know when the core is
registered, so we can't call a register operation when that happens.

Regards,
Bjorn

> [1]: https://patchwork.kernel.org/project/linux-usb/patch/20201009082843.28503-4-wcheng@codeaurora.org/
> [2]: https://patchwork.kernel.org/project/linux-usb/cover/20230325165217.31069-1-manivannan.sadhasivam@linaro.org/
> 
> Regards,
> Krishna,

Re: Disconnect interrupt generation for QC targets when role switch is enabled

[Krishna Kurapati PSSNV]

On 8/28/2023 10:50 PM, Bjorn Andersson wrote:
>>
>> I had some idea on how to get the role notification reach qcom glue driver
>> but wanted your opinion on whether they can be used or not:
>>
>> 1. Register a vendor_hook from glue driver and invoke that during
>> __dwc3_set_mode.
>>
>> 2. Let the role notification reach dwc3-qcom first and then let qcom driver
>> invoke role_set of drd. Something similar to what was implemented by Wesley
>> on [1].
>>
>> But both the options require dwc3_probe to be done in sync with
>> of_platform_populate or we need to defer qcom probe if dwc3_probe is
>> deferred. Since we are leaning towards async probe, not sure if the above
>> two options would be proper.
>>

...

> As mentioned, this need has been identified a few times by now, so
> nothing strange in your request/proposal.
> 
> But so far no one has come up with a good way to register glue code
> callbacks with the core; we can't pass arbitrary data (such as a
> function pointer to such callback), and we don't know when the core is
> registered, so we can't call a register operation when that happens.
> 
> Regards,
> Bjorn
> 
>> [1]: https://patchwork.kernel.org/project/linux-usb/patch/20201009082843.28503-4-wcheng@codeaurora.org/
>> [2]: https://patchwork.kernel.org/project/linux-usb/cover/20230325165217.31069-1-manivannan.sadhasivam@linaro.org/
>>

Hi Bjorn,

  How about we use Component framework to let the glue layer know that 
the child probe is complete. That way we don't need to defer QCOM probe 
and in the bind call back coming to master (in this case, the glue 
layer), we can register the vendor hook or role switch we need and we 
can pass the role notifications from core to glue as needed.

Regards,
Krishna,

Re: Disconnect interrupt generation for QC targets when role switch is enabled

[Pavan Kondeti]

On Wed, Aug 30, 2023 at 09:57:46AM +0530, Krishna Kurapati PSSNV wrote:
> 
> 
> On 8/28/2023 10:50 PM, Bjorn Andersson wrote:
> > > 
> > > I had some idea on how to get the role notification reach qcom glue driver
> > > but wanted your opinion on whether they can be used or not:
> > > 
> > > 1. Register a vendor_hook from glue driver and invoke that during
> > > __dwc3_set_mode.
> > > 
> > > 2. Let the role notification reach dwc3-qcom first and then let qcom driver
> > > invoke role_set of drd. Something similar to what was implemented by Wesley
> > > on [1].
> > > 
> > > But both the options require dwc3_probe to be done in sync with
> > > of_platform_populate or we need to defer qcom probe if dwc3_probe is
> > > deferred. Since we are leaning towards async probe, not sure if the above
> > > two options would be proper.
> > > 
> 
> ...
> 
> > As mentioned, this need has been identified a few times by now, so
> > nothing strange in your request/proposal.
> > 
> > But so far no one has come up with a good way to register glue code
> > callbacks with the core; we can't pass arbitrary data (such as a
> > function pointer to such callback), and we don't know when the core is
> > registered, so we can't call a register operation when that happens.
> > 
> > Regards,
> > Bjorn
> > 
> > > [1]: https://patchwork.kernel.org/project/linux-usb/patch/20201009082843.28503-4-wcheng@codeaurora.org/
> > > [2]: https://patchwork.kernel.org/project/linux-usb/cover/20230325165217.31069-1-manivannan.sadhasivam@linaro.org/
> > > 
> 
> Hi Bjorn,
> 
>  How about we use Component framework to let the glue layer know that the
> child probe is complete. That way we don't need to defer QCOM probe and in
> the bind call back coming to master (in this case, the glue layer), we can
> register the vendor hook or role switch we need and we can pass the role
> notifications from core to glue as needed.
> 

Would device_driver::sync_state() help here? The qcom glue driver
creates a DL_FLAG_SYNC_STATE_ONLY device link with dwc3 core. If it
works, we can avoid component framework related changes in dwc3 core.


Thanks,
Pavan

Re: Disconnect interrupt generation for QC targets when role switch is enabled

[Krishna Kurapati PSSNV]

On 9/6/2023 9:22 PM, Pavan Kondeti wrote:

>>> As mentioned, this need has been identified a few times by now, so
>>> nothing strange in your request/proposal.
>>>
>>> But so far no one has come up with a good way to register glue code
>>> callbacks with the core; we can't pass arbitrary data (such as a
>>> function pointer to such callback), and we don't know when the core is
>>> registered, so we can't call a register operation when that happens.
>>>
>>> Regards,
>>> Bjorn
>>>
>>>> [1]: https://patchwork.kernel.org/project/linux-usb/patch/20201009082843.28503-4-wcheng@codeaurora.org/
>>>> [2]: https://patchwork.kernel.org/project/linux-usb/cover/20230325165217.31069-1-manivannan.sadhasivam@linaro.org/
>>>>
>>
>> Hi Bjorn,
>>
>>   How about we use Component framework to let the glue layer know that the
>> child probe is complete. That way we don't need to defer QCOM probe and in
>> the bind call back coming to master (in this case, the glue layer), we can
>> register the vendor hook or role switch we need and we can pass the role
>> notifications from core to glue as needed.
>>
> 
> Would device_driver::sync_state() help here? The qcom glue driver
> creates a DL_FLAG_SYNC_STATE_ONLY device link with dwc3 core. If it
> works, we can avoid component framework related changes in dwc3 core.
> 
> 

Hi Pavan,

  Thanks for the suggestion. We can use sync state but we might need to 
suppress unbind path for dwc3 core to prevent remove from being called 
(not sure if that is fine on upstream). Reason being if remove is 
called, we won't get any sync state callback for us to un-register glue 
hook. Other ideas that came from team internally were to add a list and 
iterate through in core probe to find out required glue ops and invoke 
them when required.

Regards,
Krishna,