query about _setup() in omap_hwmod.c
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
Hello Archit,
Quoted 29 lines · Archit Taneja
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); ... ... }
Quoted 4 lines
Both these functions are taking different decisions based on whether
os->flagshasOCPIF_SWSUP_IDLEor not.Would this lead to some sort of
clk_enable()andclk_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().
Quoted 5 lines
If I boot a 3.4-rc1 kernel on beagle with
omap2plus_defconfig, I get:cat
/sys/kernel/debug/clock/summary| grep dssname parent freq
use_count
Quoted 8 lines
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_ickis 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
Hi,
Quoted 56 lines · Paul Walmsley
Hello Archit,
Quoted 29 lines · Archit Taneja
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); ... ... }Quoted 4 lines
Both these functions are taking different decisions based on whether
os->flagshasOCPIF_SWSUP_IDLEor not.Would this lead to some sort of
clk_enable()andclk_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().Quoted 11 lines
If I boot a 3.4-rc1 kernel on beagle with
omap2plus_defconfig, I get:cat
/sys/kernel/debug/clock/summary| grep dssname parent freq
use_countdss_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
Quoted 7 lines
Quoted 2 lines
dss_ickis 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
Quoted 7 lines
Anyway, the hwmod interface clock handling needs to be cleaned up... maybe it will get done for 3.5...
- Paul
Hi Archit,
Quoted 2 lines · Archit Taneja
Quoted 1 line · Paul Walmsley
On Wed, 11 Apr 2012, Archit Taneja wrote:
...
Quoted 9 lines
Quoted 8 lines
Quoted 3 lines
dss_ickis 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.
Quoted 5 lines
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
Hi Archit,
Quoted 2 lines · Archit Taneja
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
Hi,
Quoted 7 lines · Paul Walmsley
Hi Archit,
Quoted 2 lines · Archit Taneja
There were still some issues related to this, particularly on OMAP4, I have posted a patch set to fix this just a while back.
Quoted 8 lines
Was looking at those. The first one, removing the
OCPIF_SWSUP_IDLEflags, 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 commitda7cdfac1b0c58d6863532dd3b432c3fbc034978. 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
Quoted 3 lines
- Paul