* query about _setup() in omap_hwmod.c
@ 2012-04-11 9:37 Archit Taneja
2012-04-13 8:09 ` Paul Walmsley
0 siblings, 1 reply; 6+ messages in thread
From: Archit Taneja @ 2012-04-11 9:37 UTC (permalink / raw)
To: Benoit Cousson, paul@pwsan.com; +Cc: linux-omap@vger.kernel.org
Hi,
I had a query about the _setup() function in
arch/arm/mach-omap2/omap_hwmod.c:
The function enables slave clocks right in the beginning, and based on
the value of postsetup_state, it either calls _idle(), _shutdown() or
does nothing.
Now, for the case of _idle() or _shutdown(), I guess it's the job of
_disable_clocks() to disable the slave clocks before we exit _setup().
If we compare how the slave clocks are enabled and disabled in _setup()
and _disable_clocks() respectively:
static int _setup(struct omap_hwmod *oh, void *data)
{
...
...
if (os->flags & OCPIF_SWSUP_IDLE) {
/* XXX omap_iclk_deny_idle(c); */
} else {
/* XXX omap_iclk_allow_idle(c); */
clk_enable(c);
}
}
...
...
}
static int _disable_clocks(struct omap_hwmod *oh)
{
...
...
if (c && (os->flags & OCPIF_SWSUP_IDLE))
clk_disable(c);
...
...
}
Both these functions are taking different decisions based on whether
os->flags has OCPIF_SWSUP_IDLE or not.
Would this lead to some sort of clk_enable() and clk_disable() mismatch?
If I boot a 3.4-rc1 kernel on beagle with omap2plus_defconfig, I get:
cat /sys/kernel/debug/clock/summary | grep dss
name parent freq use_count
dss_ick l4_ick 83000000 4
dss2_alwon_fck sys_ck 13000000 0
dss_96m_fck omap_96m_fck 96000000 0
dss_tv_fck omap_54m_fck 54000000 0
dss1_alwon_fck dpll4_m4x2_ck 144000000 0
dss_ick is the slave clock of all the dss hwmods on omap3. This doesn't
seem to be right, does it?
Thanks,
Archit
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: query about _setup() in omap_hwmod.c
2012-04-11 9:37 query about _setup() in omap_hwmod.c Archit Taneja
@ 2012-04-13 8:09 ` Paul Walmsley
2012-04-13 8:21 ` Archit Taneja
0 siblings, 1 reply; 6+ messages in thread
From: Paul Walmsley @ 2012-04-13 8:09 UTC (permalink / raw)
To: Archit Taneja; +Cc: Benoit Cousson, linux-omap@vger.kernel.org
Hello Archit,
On Wed, 11 Apr 2012, Archit Taneja wrote:
>
> If we compare how the slave clocks are enabled and disabled in _setup() and
> _disable_clocks() respectively:
>
> static int _setup(struct omap_hwmod *oh, void *data)
> {
> ...
> ...
> if (os->flags & OCPIF_SWSUP_IDLE) {
> /* XXX omap_iclk_deny_idle(c); */
> } else {
> /* XXX omap_iclk_allow_idle(c); */
> clk_enable(c);
> }
> }
> ...
> ...
> }
>
> static int _disable_clocks(struct omap_hwmod *oh)
> {
> ...
> ...
> if (c && (os->flags & OCPIF_SWSUP_IDLE))
> clk_disable(c);
> ...
> ...
> }
>
> Both these functions are taking different decisions based on whether os->flags
> has OCPIF_SWSUP_IDLE or not.
>
> Would this lead to some sort of clk_enable() and clk_disable() mismatch?
Looks like you didn't include the _enable_clocks() code in your E-mail:
if (c && (os->flags & OCPIF_SWSUP_IDLE))
clk_enable(c);
_disable_clocks() is intended to balance _enable_clocks(), not _setup().
> If I boot a 3.4-rc1 kernel on beagle with omap2plus_defconfig, I get:
>
> cat /sys/kernel/debug/clock/summary | grep dss
>
> name parent freq use_count
> dss_ick l4_ick 83000000 4
> dss2_alwon_fck sys_ck 13000000 0
> dss_96m_fck omap_96m_fck 96000000 0
> dss_tv_fck omap_54m_fck 54000000 0
> dss1_alwon_fck dpll4_m4x2_ck 144000000 0
>
> dss_ick is the slave clock of all the dss hwmods on omap3. This doesn't seem
> to be right, does it?
It's not 100% right, but it was intentional. The idea being that if the
interface clocks are autoidling, there's no need to enable or disable
them. We just enable them once during _setup(), and the hardware takes
care of it from then on.
Anyway, the hwmod interface clock handling needs to be cleaned up... maybe
it will get done for 3.5...
- Paul
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: query about _setup() in omap_hwmod.c
2012-04-13 8:09 ` Paul Walmsley
@ 2012-04-13 8:21 ` Archit Taneja
2012-04-13 8:35 ` Cousson, Benoit
2012-04-13 8:39 ` Paul Walmsley
0 siblings, 2 replies; 6+ messages in thread
From: Archit Taneja @ 2012-04-13 8:21 UTC (permalink / raw)
To: Paul Walmsley; +Cc: Benoit Cousson, linux-omap@vger.kernel.org, Rajendra Nayak
Hi,
On Friday 13 April 2012 01:39 PM, Paul Walmsley wrote:
> Hello Archit,
>
> On Wed, 11 Apr 2012, Archit Taneja wrote:
>
>>
>> If we compare how the slave clocks are enabled and disabled in _setup() and
>> _disable_clocks() respectively:
>>
>> static int _setup(struct omap_hwmod *oh, void *data)
>> {
>> ...
>> ...
>> if (os->flags& OCPIF_SWSUP_IDLE) {
>> /* XXX omap_iclk_deny_idle(c); */
>> } else {
>> /* XXX omap_iclk_allow_idle(c); */
>> clk_enable(c);
>> }
>> }
>> ...
>> ...
>> }
>>
>> static int _disable_clocks(struct omap_hwmod *oh)
>> {
>> ...
>> ...
>> if (c&& (os->flags& OCPIF_SWSUP_IDLE))
>> clk_disable(c);
>> ...
>> ...
>> }
>>
>> Both these functions are taking different decisions based on whether os->flags
>> has OCPIF_SWSUP_IDLE or not.
>>
>> Would this lead to some sort of clk_enable() and clk_disable() mismatch?
>
> Looks like you didn't include the _enable_clocks() code in your E-mail:
>
> if (c&& (os->flags& OCPIF_SWSUP_IDLE))
> clk_enable(c);
>
> _disable_clocks() is intended to balance _enable_clocks(), not _setup().
>
>> If I boot a 3.4-rc1 kernel on beagle with omap2plus_defconfig, I get:
>>
>> cat /sys/kernel/debug/clock/summary | grep dss
>>
>> name parent freq use_count
>> dss_ick l4_ick 83000000 4
>> dss2_alwon_fck sys_ck 13000000 0
>> dss_96m_fck omap_96m_fck 96000000 0
>> dss_tv_fck omap_54m_fck 54000000 0
>> dss1_alwon_fck dpll4_m4x2_ck 144000000 0
>>
>> dss_ick is the slave clock of all the dss hwmods on omap3. This doesn't seem
>> to be right, does it?
>
> It's not 100% right, but it was intentional. The idea being that if the
> interface clocks are autoidling, there's no need to enable or disable
> them. We just enable them once during _setup(), and the hardware takes
> care of it from then on.
Thanks for the clarification, Rajendra also explained the same to me
yesterday :)
There were still some issues related to this, particularly on OMAP4, I
have posted a patch set to fix this just a while back.
Thanks,
Archit
>
> Anyway, the hwmod interface clock handling needs to be cleaned up... maybe
> it will get done for 3.5...
>
>
> - Paul
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: query about _setup() in omap_hwmod.c
2012-04-13 8:21 ` Archit Taneja
@ 2012-04-13 8:35 ` Cousson, Benoit
2012-04-13 8:39 ` Paul Walmsley
1 sibling, 0 replies; 6+ messages in thread
From: Cousson, Benoit @ 2012-04-13 8:35 UTC (permalink / raw)
To: Archit Taneja; +Cc: Paul Walmsley, linux-omap@vger.kernel.org, Rajendra Nayak
Hi Archit,
On 4/13/2012 10:21 AM, Archit Taneja wrote:
> On Friday 13 April 2012 01:39 PM, Paul Walmsley wrote:
>> On Wed, 11 Apr 2012, Archit Taneja wrote:
...
>>> dss_ick is the slave clock of all the dss hwmods on omap3. This
>>> doesn't seem
>>> to be right, does it?
>>
>> It's not 100% right, but it was intentional. The idea being that if the
>> interface clocks are autoidling, there's no need to enable or disable
>> them. We just enable them once during _setup(), and the hardware takes
>> care of it from then on.
>
> Thanks for the clarification, Rajendra also explained the same to me
> yesterday :)
>
> There were still some issues related to this, particularly on OMAP4, I
> have posted a patch set to fix this just a while back.
Yep, but I'm afraid it is not the right fix :-)
I'll comment further on the patch.
Regards,
Benoit
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: query about _setup() in omap_hwmod.c
2012-04-13 8:21 ` Archit Taneja
2012-04-13 8:35 ` Cousson, Benoit
@ 2012-04-13 8:39 ` Paul Walmsley
2012-04-13 8:52 ` Archit Taneja
1 sibling, 1 reply; 6+ messages in thread
From: Paul Walmsley @ 2012-04-13 8:39 UTC (permalink / raw)
To: Archit Taneja; +Cc: Benoit Cousson, linux-omap@vger.kernel.org, Rajendra Nayak
Hi Archit,
On Fri, 13 Apr 2012, Archit Taneja wrote:
> There were still some issues related to this, particularly on OMAP4, I have
> posted a patch set to fix this just a while back.
Was looking at those. The first one, removing the OCPIF_SWSUP_IDLE flags,
makes sense to me. But as far as the second one goes, it would be nice if
we could use interface clocks again for interfaces, rather than functional
clocks. That's the root cause of the problem... Looks like this was added
in commit da7cdfac1b0c58d6863532dd3b432c3fbc034978. Maybe this is needed
because we don't properly enable parent hwmods before the dependent child
hwmods?
- Paul
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: query about _setup() in omap_hwmod.c
2012-04-13 8:39 ` Paul Walmsley
@ 2012-04-13 8:52 ` Archit Taneja
0 siblings, 0 replies; 6+ messages in thread
From: Archit Taneja @ 2012-04-13 8:52 UTC (permalink / raw)
To: Paul Walmsley; +Cc: Benoit Cousson, linux-omap@vger.kernel.org, Rajendra Nayak
Hi,
On Friday 13 April 2012 02:09 PM, Paul Walmsley wrote:
> Hi Archit,
>
> On Fri, 13 Apr 2012, Archit Taneja wrote:
>
>> There were still some issues related to this, particularly on OMAP4, I have
>> posted a patch set to fix this just a while back.
>
> Was looking at those. The first one, removing the OCPIF_SWSUP_IDLE flags,
> makes sense to me. But as far as the second one goes, it would be nice if
> we could use interface clocks again for interfaces, rather than functional
> clocks. That's the root cause of the problem... Looks like this was added
> in commit da7cdfac1b0c58d6863532dd3b432c3fbc034978. Maybe this is needed
> because we don't properly enable parent hwmods before the dependent child
> hwmods?
>
Yes, you are right. As you and Benoit in the other thread explained, if
we get the parent-child dependency right, we could switch back to using
l3_div interface clock.
Thanks,
Archit
>
> - Paul
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-04-13 8:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-04-11 9:37 query about _setup() in omap_hwmod.c Archit Taneja
2012-04-13 8:09 ` Paul Walmsley
2012-04-13 8:21 ` Archit Taneja
2012-04-13 8:35 ` Cousson, Benoit
2012-04-13 8:39 ` Paul Walmsley
2012-04-13 8:52 ` Archit Taneja
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).