* Question about suspend/resume clock handling in dwc3-of-simple.c
@ 2016-09-12 18:56 Guenter Roeck
2016-09-12 19:05 ` Felipe Balbi
0 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2016-09-12 18:56 UTC (permalink / raw)
To: Felipe Balbi; +Cc: linux-usb, linux-kernel
Hi folks,
In dwc3-of-simple.c:dwc3_of_simple_remove(), I see the following code.
for (i = 0; i < simple->num_clocks; i++) {
clk_unprepare(simple->clks[i]);
clk_put(simple->clks[i]);
}
What I don't understand is why clk_unprepare() is called instead
of clk_disable_unprepare(). Someone told me that it was due to
dwc3_of_simple_runtime_suspend(), which would call clk_disable().
That doesn't really make sense to me, since after all CONFIG_PM
can be disabled.
Should it be clk_disable_unprepare(), or maybe something like the
following
if (!pm_runtime_status_suspended(dev))
clk_disable_unprepare();
else
clk_unprepare();
or am I missing something ?
Thanks,
Guenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Question about suspend/resume clock handling in dwc3-of-simple.c
2016-09-12 18:56 Question about suspend/resume clock handling in dwc3-of-simple.c Guenter Roeck
@ 2016-09-12 19:05 ` Felipe Balbi
2016-09-12 19:43 ` Guenter Roeck
0 siblings, 1 reply; 6+ messages in thread
From: Felipe Balbi @ 2016-09-12 19:05 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1101 bytes --]
Hi Guenter,
Guenter Roeck <linux@roeck-us.net> writes:
> Hi folks,
>
> In dwc3-of-simple.c:dwc3_of_simple_remove(), I see the following code.
>
> for (i = 0; i < simple->num_clocks; i++) {
> clk_unprepare(simple->clks[i]);
> clk_put(simple->clks[i]);
> }
>
> What I don't understand is why clk_unprepare() is called instead
> of clk_disable_unprepare(). Someone told me that it was due to
> dwc3_of_simple_runtime_suspend(), which would call clk_disable().
good eyes :-) That was fixed though:
https://marc.info/?l=linux-usb&m=147343692631868&w=2
> Should it be clk_disable_unprepare(), or maybe something like the
> following
>
> if (!pm_runtime_status_suspended(dev))
> clk_disable_unprepare();
> else
> clk_unprepare();
I'm not sure how balanced those calls are, yeah. I don't have HW to test
PM with. But note that as it is, there is no actual runtime PM support,
so clk_disable_unprepare() will always be necessary.
Perhaps we will find further issues when someone tries to use runtime PM
with dwc3-of-simple. ;-)
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Question about suspend/resume clock handling in dwc3-of-simple.c
2016-09-12 19:05 ` Felipe Balbi
@ 2016-09-12 19:43 ` Guenter Roeck
2016-09-13 5:35 ` Felipe Balbi
0 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2016-09-12 19:43 UTC (permalink / raw)
To: Felipe Balbi; +Cc: linux-usb, linux-kernel
Hi Felipe,
On Mon, Sep 12, 2016 at 10:05:00PM +0300, Felipe Balbi wrote:
>
> Hi Guenter,
>
> Guenter Roeck <linux@roeck-us.net> writes:
> > Hi folks,
> >
> > In dwc3-of-simple.c:dwc3_of_simple_remove(), I see the following code.
> >
> > for (i = 0; i < simple->num_clocks; i++) {
> > clk_unprepare(simple->clks[i]);
> > clk_put(simple->clks[i]);
> > }
> >
> > What I don't understand is why clk_unprepare() is called instead
> > of clk_disable_unprepare(). Someone told me that it was due to
> > dwc3_of_simple_runtime_suspend(), which would call clk_disable().
>
> good eyes :-) That was fixed though:
>
> https://marc.info/?l=linux-usb&m=147343692631868&w=2
>
Great, thanks!
> > Should it be clk_disable_unprepare(), or maybe something like the
> > following
> >
> > if (!pm_runtime_status_suspended(dev))
> > clk_disable_unprepare();
> > else
> > clk_unprepare();
>
> I'm not sure how balanced those calls are, yeah. I don't have HW to test
> PM with. But note that as it is, there is no actual runtime PM support,
> so clk_disable_unprepare() will always be necessary.
>
> Perhaps we will find further issues when someone tries to use runtime PM
> with dwc3-of-simple. ;-)
>
We are working on code derived from it, so unless I can convince the author
that he can not just use clk_unprepare() I suspect we'll hit the problem.
If so, I'll let you know.
Thanks!
Guenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Question about suspend/resume clock handling in dwc3-of-simple.c
2016-09-12 19:43 ` Guenter Roeck
@ 2016-09-13 5:35 ` Felipe Balbi
2016-09-13 13:10 ` Guenter Roeck
0 siblings, 1 reply; 6+ messages in thread
From: Felipe Balbi @ 2016-09-13 5:35 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-usb, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 947 bytes --]
Hi,
Guenter Roeck <linux@roeck-us.net> writes:
>> > Should it be clk_disable_unprepare(), or maybe something like the
>> > following
>> >
>> > if (!pm_runtime_status_suspended(dev))
>> > clk_disable_unprepare();
>> > else
>> > clk_unprepare();
>>
>> I'm not sure how balanced those calls are, yeah. I don't have HW to test
>> PM with. But note that as it is, there is no actual runtime PM support,
>> so clk_disable_unprepare() will always be necessary.
>>
>> Perhaps we will find further issues when someone tries to use runtime PM
>> with dwc3-of-simple. ;-)
>>
>
> We are working on code derived from it, so unless I can convince the author
> that he can not just use clk_unprepare() I suspect we'll hit the problem.
> If so, I'll let you know.
Are you sending that upstream? Depending on your requirements, it might
be easier to patch dwc3-of-simple.c then adding yet another glue layer :-)
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Question about suspend/resume clock handling in dwc3-of-simple.c
2016-09-13 5:35 ` Felipe Balbi
@ 2016-09-13 13:10 ` Guenter Roeck
2016-09-13 14:05 ` Felipe Balbi
0 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2016-09-13 13:10 UTC (permalink / raw)
To: Felipe Balbi; +Cc: linux-usb, linux-kernel
On 09/12/2016 10:35 PM, Felipe Balbi wrote:
>
> Hi,
>
> Guenter Roeck <linux@roeck-us.net> writes:
>>>> Should it be clk_disable_unprepare(), or maybe something like the
>>>> following
>>>>
>>>> if (!pm_runtime_status_suspended(dev))
>>>> clk_disable_unprepare();
>>>> else
>>>> clk_unprepare();
>>>
>>> I'm not sure how balanced those calls are, yeah. I don't have HW to test
>>> PM with. But note that as it is, there is no actual runtime PM support,
>>> so clk_disable_unprepare() will always be necessary.
>>>
>>> Perhaps we will find further issues when someone tries to use runtime PM
>>> with dwc3-of-simple. ;-)
>>>
>>
>> We are working on code derived from it, so unless I can convince the author
>> that he can not just use clk_unprepare() I suspect we'll hit the problem.
>> If so, I'll let you know.
>
> Are you sending that upstream? Depending on your requirements, it might
> be easier to patch dwc3-of-simple.c then adding yet another glue layer :-)
>
Yes. It will be a glue layer. So far that looks like the cleanest solution.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Question about suspend/resume clock handling in dwc3-of-simple.c
2016-09-13 13:10 ` Guenter Roeck
@ 2016-09-13 14:05 ` Felipe Balbi
0 siblings, 0 replies; 6+ messages in thread
From: Felipe Balbi @ 2016-09-13 14:05 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-usb, linux-kernel
Hi,
Guenter Roeck <linux@roeck-us.net> writes:
> On 09/12/2016 10:35 PM, Felipe Balbi wrote:
>>
>> Hi,
>>
>> Guenter Roeck <linux@roeck-us.net> writes:
>>>>> Should it be clk_disable_unprepare(), or maybe something like the
>>>>> following
>>>>>
>>>>> if (!pm_runtime_status_suspended(dev))
>>>>> clk_disable_unprepare();
>>>>> else
>>>>> clk_unprepare();
>>>>
>>>> I'm not sure how balanced those calls are, yeah. I don't have HW to test
>>>> PM with. But note that as it is, there is no actual runtime PM support,
>>>> so clk_disable_unprepare() will always be necessary.
>>>>
>>>> Perhaps we will find further issues when someone tries to use runtime PM
>>>> with dwc3-of-simple. ;-)
>>>>
>>>
>>> We are working on code derived from it, so unless I can convince the author
>>> that he can not just use clk_unprepare() I suspect we'll hit the problem.
>>> If so, I'll let you know.
>>
>> Are you sending that upstream? Depending on your requirements, it might
>> be easier to patch dwc3-of-simple.c then adding yet another glue layer :-)
>>
> Yes. It will be a glue layer. So far that looks like the cleanest solution.
fair enough, take your time ;-)
--
balbi
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-09-13 14:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-09-12 18:56 Question about suspend/resume clock handling in dwc3-of-simple.c Guenter Roeck
2016-09-12 19:05 ` Felipe Balbi
2016-09-12 19:43 ` Guenter Roeck
2016-09-13 5:35 ` Felipe Balbi
2016-09-13 13:10 ` Guenter Roeck
2016-09-13 14:05 ` Felipe Balbi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox