Re: [PATCH v1 2/2] usb: dwc3: Add Qualcomm DWC3 glue driver

[Felipe Balbi]

Hi,

Manu Gautam <mgautam@xxxxxxxxxxxxxx> writes:
> Hi,
>
>
> On 3/13/2018 4:38 PM, Felipe Balbi wrote:
>> Hi,
>>
>> +Andy
>>
>> Manu Gautam <mgautam@xxxxxxxxxxxxxx> writes:
>>> DWC3 controller on Qualcomm SOCs has a Qscratch wrapper.
>>> Some of its uses are described below resulting in need to
>>> have a separate glue driver instead of using dwc3-of-simple:
>>>  - It exposes register interface to override vbus-override
>>>    and lane0-pwr-present signals going to hardware. These
>>>    must be updated in peripheral mode for DWC3 if vbus lines
>>>    are not connected to hardware block. Otherwise RX termination
>>>    in SS mode or DP pull-up is not applied by device controller.
>> right, core needs to know that VBUS is above 4.4V. Why wasn't this a
>> problem when the original glue layer was first published?
>
> Thanks for reviewing.
> Original glue layer supported only host mode, hence this wasn't needed.

okay

>>>  - pwr_events_irq_stat support to ensure USB2 PHY is in L2 state
>>>    before glue driver can turn-off clocks and suspend PHY.
>> Core manages PHY suspend automatically. Isn't that working for you? Why?
>
> Yes, it is not supported with QUSB2 PHY (usb2-phy on Qualcomm SOCs).

but the PHY doesn't need to support it, DWC3 does :-)

> Issue is that If core suspends USB2 PHY (say in host mode if some SS device connected),
> USB2 PHY stops responding to any attach event as it can't exit suspend state by itself.

Okay, so we miss remote wakeup events. Fair enough.

>>>  - Support to replace pip3 clock going to DWC3 with utmi clock
>>>    for hardware configuration where SSPHY is not used with DWC3.
>> Is that SW configurable? Really? In any case seems like this and SESSVLD
>> valid should be handled using Hans' and Heikki's mux support.
>
> Yes, with this we can use dwc3 without using SSPHY. Please point me to
> those patches. There are only bunch of register writes in glue wrapper to
> achieve the same.

https://www.spinics.net/lists/linux-usb/msg160868.html

>>> +static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
>>> +{
>>> +	struct dwc3	*dwc = platform_get_drvdata(qcom->dwc3);
>> nope! Glue shouldn't touch dwc3 at all.
> Other than PHY handles, need this to fail runtime suspend if dwc3 hasn't
> probed yet.

Will that even happen? You should pm_runtime_forbid() by default,
anyway and expect it to be enabled via sysfs later, no?

>>> +	dwc3_qcom_suspend_hsphy(qcom);
>>> +
>>> +	if (dwc->usb2_generic_phy)
>>> +		phy_pm_runtime_put_sync(dwc->usb2_generic_phy);
>>> +	if (dwc->usb3_generic_phy)
>>> +		phy_pm_runtime_put_sync(dwc->usb3_generic_phy);
>> core.c should do this.
> Recommended sequence from h/w programming guide is that:
> 1. PHY autosuspend must be left disabled - snps,dis_u2_susphy_quirk/dis_enblslpm_quirk
> 2. During runtime-suspend (say on xhci bus_suspend) , PHY should be suspended
> Â Â using GUSB2PHYCFG register

this is something that dwc3 core can do on its own suspend implementation.

> 3. Wait until pwr_event_irq_stat in qscratch reflects PHY transition to L2.

this is interesting part. Is this register accessible by the PHY driver?
Seems like that would be the best place to stuff this...

> 3. USB2 PHY driver can suspend - enable wakeup events and turns off clocks etc.

... together with this.

> 4. dwc3 glue driver can suspend.
>
> Since, pwr_event_irq stat can't be checked in core driver, I added this handling
> in glue driver. Alternative approach I can think of is to let dwc3 core suspend
> PHY using GUSBPHYCFG register on suspend, add some delay before
> suspending PHY. Glue driver can check for pwr_event_irq stat and throw a
> warning if PHY not in L2.

almost :-) core_suspend fiddles with GUSB2PHYCFG for suspend and calls
phy_suspend() (or whatever the function is called heh), that will go to
your phy driver's suspend callback, which checks pwr_event_irq_stat and
then pm_runtime_put() to schedule ->runtime_suspend() so that can enable
wakeups and switch off clocks.

>>> +	irq = platform_get_irq_byname(pdev, "dp_hs_phy_irq");
>>> +	if (irq > 0) {
>>> +		irq_set_status_flags(irq, IRQ_NOAUTOEN);
>> why do you need to set this flag?
> These wakeup_irqs should be enabled only during suspend. With this flag I
> don't need to disable irq immediately after requesting it.

oh, okay. You may want to add a comment here :-)

>
>>
>>> +		ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
>>> +					qcom_dwc3_resume_irq,
>>> +					IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
>>> +					"qcom_dwc3 DP_HS", qcom);
>> this is the same as devm_request_irq()
> I am passing hard_irq handler as NULL whereas thread_fn is not NULL.
> devm_request_irq is just the opposite.

oh, indeed. I misparsed it.

>>> +static const struct dev_pm_ops dwc3_qcom_dev_pm_ops = {
>>> +	SET_SYSTEM_SLEEP_PM_OPS(dwc3_qcom_pm_suspend, dwc3_qcom_pm_resume)
>>> +	SET_RUNTIME_PM_OPS(dwc3_qcom_runtime_suspend, dwc3_qcom_runtime_resume,
>>> +			   NULL)
>> why don't you have runtime_idle?
>
> I didn't see any need for that. rpm_idle will invoke rpm_suspend if idle callback
> is not specified.

Right, but ->runtime_idle() helps making things a little more
readable. It is, however, a matter of taste. No strong feelings here. :-)

-- 
balbi

Attachment: signature.asc
Description: PGP signature