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

[Manu Gautam <mgautam@codeaurora.org>]

Hi,


On 3/14/2018 2:20 PM, Felipe Balbi wrote:
> Hi,
>
> Manu Gautam <mgautam@codeaurora.org> writes:
>
[snip]
>>>>  - 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

I looked at the patchset and thinking that adding mux for this may
not be of much help here. Qscratch is part of dwc3 wrapper
and uses same clock domain for its registers. Hence, I can't
have a separate mux driver for same. Will have to register
mux controller from this driver for these and use only in this driver
as I dont expect any other client for same. So, can I proceed with
existing logic?

>
>>>> +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?

It happens if I enable runtime_pm from probe which is the case right now.
I will disable it from probe as you suggested.
In any case dwc3 uses pm_runtime_forbid and expects it to be enabled from sysfs,
so I dont get any benefit of enabling it from glue driver probe.

Other than this, I need to access 'dwc->xhci' to resume root hub on remote wakeup.
That I missed to mention earlier.

>
>>>> +	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...

This register is in controller wrapper which PHY driver can't access.
Also clock domain is different.

>
>> 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.

Since phy driver can not access pwr_event_irq_stat, should I just use some delay
and assume PHY gets suspended?

>
>>>> +	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 :-)
Sure.

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project