From: Rajendra Nayak <redacted>
The patch is the output from a python script which converts
from the old OMAP clk format to COMMON clk format using a
JSON parser in between which was developed by Paul Walmsley.
Signed-off-by: Rajendra Nayak <redacted>
[paul at pwsan.com: AM3517/05: dropped bogus hsotgusb "ick" and "fck"
clkdev aliases; added hsotgusb_fck alias; added emac_ick and emac_fck
aliases; replace omap2_init_clksel_parent() with
omap2_clksel_find_parent_index(); reflow macros and parent name
lists; add clkdm_name argument to DEFINE_STRUCT_CLK_HW_OMAP macros]
Signed-off-by: Mike Turquette <redacted>
Am seeing warnings during the disable-unused-clocks phase of the boot
on the OMAP3 test boards here. Log is included at the bottom of this
E-mail, with some debugging added for extra context. The problem
appears to be that the clock code is disabling clocks that are active
in the hardware, but for which the clockdomain use count is 0, since
they've never been enabled.
Ideally there would be some way for the core CCF code to indicate to the
underlying clock hardware implementation that the disable-unused-clock
process is a 'force disable'. The OMAP clock hardware implementation code
could then also put the clockdomain to sleep (and skip the warning) in
such a circumstance.
- Paul
[ 2.685943] disabling sys_clkout1: ec = 0
[ 2.690277] disabling wdt1_ick: ec = 0
[ 2.694366] disabling cam_mclk: ec = 0
[ 2.698333] ------------[ cut here ]------------
[ 2.703216] WARNING: at arch/arm/mach-omap2/clockdomain.c:961 _clkdm_clk_hwmod_disable+0xc4/0xd8()
[ 2.712646] Modules linked in:
[ 2.715911] [<c001c38c>] (unwind_backtrace+0x0/0xf0) from [<c0043cd8>] (warn_slowpath_common+0x4c/0x64)
[ 2.725799] [<c0043cd8>] (warn_slowpath_common+0x4c/0x64) from [<c0043d0c>] (warn_slowpath_null+0x1c/0x24)
[ 2.735961] [<c0043d0c>] (warn_slowpath_null+0x1c/0x24) from [<c003705c>] (_clkdm_clk_hwmod_disable+0xc4/0xd8)
[ 2.746520] [<c003705c>] (_clkdm_clk_hwmod_disable+0xc4/0xd8) from [<c0437bd0>] (clk_disable_unused_subtree+0xb0/0xbc)
[ 2.757781] [<c0437bd0>] (clk_disable_unused_subtree+0xb0/0xbc) from [<c0437b40>] (clk_disable_unused_subtree+0x20/0xbc)
[ 2.769226] ---[ end trace ebefd5468131571a ]---
[ 2.774261] disabling mspro_fck: ec = 0
[ 2.778320] disabling ssi_ssr_fck_3430es2: ec = 0
[ 2.783355] disabling pka_ick: ec = 0
[ 2.787231] disabling sad2d_ick: ec = 0
[ 2.791259] ------------[ cut here ]------------
[ 2.796142] WARNING: at arch/arm/mach-omap2/clockdomain.c:961 _clkdm_clk_hwmod_disable+0xc4/0xd8()
[ 2.805572] Modules linked in:
[ 2.808807] [<c001c38c>] (unwind_backtrace+0x0/0xf0) from [<c0043cd8>] (warn_slowpath_common+0x4c/0x64)
[ 2.818695] [<c0043cd8>] (warn_slowpath_common+0x4c/0x64) from [<c0043d0c>] (warn_slowpath_null+0x1c/0x24)
[ 2.828857] [<c0043d0c>] (warn_slowpath_null+0x1c/0x24) from [<c003705c>] (_clkdm_clk_hwmod_disable+0xc4/0xd8)
[ 2.839416] [<c003705c>] (_clkdm_clk_hwmod_disable+0xc4/0xd8) from [<c0437bd0>] (clk_disable_unused_subtree+0xb0/0xbc)
[ 2.850677] [<c0437bd0>] (clk_disable_unused_subtree+0xb0/0xbc) from [<c0437b40>] (clk_disable_unused_subtree+0x20/0xbc)
[ 2.862121] ---[ end trace ebefd5468131571b ]---
[ 2.867065] disabling wdt3_ick: ec = 0
[ 2.871032] disabling des1_ick: ec = 0
[ 2.875030] disabling sha11_ick: ec = 0
[ 2.879089] disabling rng_ick: ec = 0
[ 2.882995] disabling aes1_ick: ec = 0
[ 2.886932] disabling ssi_ick_3430es2: ec = 0
[ 2.891601] disabling mspro_ick: ec = 0
[ 2.895660] disabling des2_ick: ec = 0
[ 2.899658] disabling sha12_ick: ec = 0
[ 2.903686] disabling aes2_ick: ec = 0
[ 2.907684] disabling icr_ick: ec = 0
[ 2.911621] disabling wdt3_fck: ec = 0
From: Mike Turquette <hidden> Date: 2012-11-08 21:52:50
Quoting Paul Walmsley (2012-11-08 10:08:16)
On Wed, 7 Nov 2012, Mike Turquette wrote:
quoted
From: Rajendra Nayak <redacted>
The patch is the output from a python script which converts
from the old OMAP clk format to COMMON clk format using a
JSON parser in between which was developed by Paul Walmsley.
Signed-off-by: Rajendra Nayak <redacted>
[paul at pwsan.com: AM3517/05: dropped bogus hsotgusb "ick" and "fck"
clkdev aliases; added hsotgusb_fck alias; added emac_ick and emac_fck
aliases; replace omap2_init_clksel_parent() with
omap2_clksel_find_parent_index(); reflow macros and parent name
lists; add clkdm_name argument to DEFINE_STRUCT_CLK_HW_OMAP macros]
Signed-off-by: Mike Turquette <redacted>
Am seeing warnings during the disable-unused-clocks phase of the boot
on the OMAP3 test boards here. Log is included at the bottom of this
E-mail, with some debugging added for extra context. The problem
appears to be that the clock code is disabling clocks that are active
in the hardware, but for which the clockdomain use count is 0, since
they've never been enabled.
Right. The old omap2_disabled_unused_clocks code used to call
omap2_clk_enable before calling omap2_clk_disable. That approach is
sort of "cooking the books" and the generic implementation in
drivers/clk/clk.c simply disables the clocks (without first enabling),
which is why these WARNs are new.
Ideally there would be some way for the core CCF code to indicate to the
underlying clock hardware implementation that the disable-unused-clock
process is a 'force disable'. The OMAP clock hardware implementation code
could then also put the clockdomain to sleep (and skip the warning) in
such a circumstance.
I'm looking at a few different ways to do that now.
Regards,
Mike
Am seeing warnings during the disable-unused-clocks phase of the boot on
the OMAP3 test boards here.
Similar problems during system suspend on 3530ES3 Beagle. Not sure
what's causing these yet. At this point the clockdomain usecounts
should be accurate.
Here's a redacted debugging log for these cases. The suspend events start
around the 30 second mark.
One observation is that dpll4_m5x2_ck and dpll4_m6x2_ck are never enabled.
The tracebacks occur when something in the suspend path tries to disable
those clocks.
- Paul
[ 0.135528] enabling clkdm dpll4_clkdm during enable of clk dpll4_ck
[ 0.135559] clockdomain: dpll4_clkdm: enabled
[ 0.135589] enabling clkdm dpll4_clkdm during enable of clk dpll4_m2x2_ck
[ 0.135681] disabling clkdm dpll4_clkdm during disable of clk dpll4_m2x2_ck
[ 0.135681] disabling clkdm dpll4_clkdm during disable of clk dpll4_ck
[ 0.135711] clockdomain: dpll4_clkdm: disabled
[ 0.135772] enabling clkdm dpll4_clkdm during enable of clk dpll4_ck
[ 0.135803] clockdomain: dpll4_clkdm: enabled
[ 0.135833] enabling clkdm dpll4_clkdm during enable of clk dpll4_m2x2_ck
[ 0.135894] disabling clkdm dpll4_clkdm during disable of clk dpll4_m2x2_ck
[ 0.135925] disabling clkdm dpll4_clkdm during disable of clk dpll4_ck
[ 0.135955] clockdomain: dpll4_clkdm: disabled
[ 0.135986] enabling clkdm dpll4_clkdm during enable of clk dpll4_ck
[ 0.136016] clockdomain: dpll4_clkdm: enabled
[ 0.136047] enabling clkdm dpll4_clkdm during enable of clk dpll4_m2x2_ck
[ 0.136138] disabling clkdm dpll4_clkdm during disable of clk dpll4_m2x2_ck
[ 0.136169] disabling clkdm dpll4_clkdm during disable of clk dpll4_ck
[ 0.136169] clockdomain: dpll4_clkdm: disabled
[ 0.136260] enabling clkdm dpll4_clkdm during enable of clk dpll4_ck
[ 0.136291] clockdomain: dpll4_clkdm: enabled
[ 0.136322] enabling clkdm dpll4_clkdm during enable of clk dpll4_m2x2_ck
[ 0.140594] enabling clkdm dpll4_clkdm during enable of clk dpll4_m4x2_ck
[ 0.140686] enabling clkdm dpll4_clkdm during enable of clk dpll4_m3x2_ck
[ 0.140838] disabling clkdm dpll4_clkdm during disable of clk dpll4_m3x2_ck
[ 0.140930] disabling clkdm dpll4_clkdm during disable of clk dpll4_m4x2_ck
[ 0.141479] enabling clkdm dpll4_clkdm during enable of clk dpll4_m4x2_ck
[ 0.141571] disabling clkdm dpll4_clkdm during disable of clk dpll4_m4x2_ck
[ 0.141601] enabling clkdm dpll4_clkdm during enable of clk dpll4_m4x2_ck
[ 0.141662] disabling clkdm dpll4_clkdm during disable of clk dpll4_m4x2_ck
[ 0.141693] enabling clkdm dpll4_clkdm during enable of clk dpll4_m4x2_ck
[ 0.141784] disabling clkdm dpll4_clkdm during disable of clk dpll4_m4x2_ck
[ 0.141815] enabling clkdm dpll4_clkdm during enable of clk dpll4_m3x2_ck
[ 0.141876] disabling clkdm dpll4_clkdm during disable of clk dpll4_m3x2_ck
[ 3.399200] disabling clkdm dpll4_clkdm during disable of clk dpll4_m6x2_ck
[ 3.417694] [<c001c1cc>] (unwind_backtrace+0x0/0xf0) from [<c0043680>] (warn_slowpath_common+0x4c/0x64)
[ 3.445251] ---[ end trace 72e2d7bdcf98ea8b ]---
[ 3.450134] disabling clkdm dpll4_clkdm during disable of clk dpll4_m5x2_ck
[ 3.453918] clockdomain: dpll4_clkdm: disabled
[ 3.477569] [<c001c1cc>] (unwind_backtrace+0x0/0xf0) from [<c0043680>] (warn_slowpath_common+0x4c/0x64)
[ 3.505096] ---[ end trace 72e2d7bdcf98ea8c ]---
[ 3.531280] [<c001c1cc>] (unwind_backtrace+0x0/0xf0) from [<c0043680>] (warn_slowpath_common+0x4c/0x64)
[ 3.558807] ---[ end trace 72e2d7bdcf98ea8d ]---
[ 38.999145] disabling clkdm dpll4_clkdm during disable of clk dpll4_m2x2_ck
[ 38.999267] [<c001c1cc>] (unwind_backtrace+0x0/0xf0) from [<c0043680>] (warn_slowpath_common+0x4c/0x64)
[ 38.999816] ---[ end trace 72e2d7bdcf98ea8e ]---
[ 38.999816] disabling clkdm dpll4_clkdm during disable of clk dpll4_ck
[ 38.999908] [<c001c1cc>] (unwind_backtrace+0x0/0xf0) from [<c0043680>] (warn_slowpath_common+0x4c/0x64)
[ 39.000366] ---[ end trace 72e2d7bdcf98ea8f ]---
[ 42.169647] enabling clkdm dpll4_clkdm during enable of clk dpll4_ck
[ 42.169647] clockdomain: dpll4_clkdm: enabled
[ 42.169677] enabling clkdm dpll4_clkdm during enable of clk dpll4_m2x2_ck
[ 45.730346] [<c001c1cc>] (unwind_backtrace+0x0/0xf0) from [<c0043680>] (warn_slowpath_common+0x4c/0x64)
[ 45.795654] ---[ end trace 72e2d7bdcf98ea90 ]---
One observation is that dpll4_m5x2_ck and dpll4_m6x2_ck are never enabled.
The tracebacks occur when something in the suspend path tries to disable
those clocks.
Sorry, this part is inaccurate - I misread the trace. The suspend path
errors are coming from a disable of dpll4_m2x2_ck which is probably caused
by the disable of i2c1_fck. Here's the unredacted trace of this section.
The enables and disables of dpll4_m2x2_ck from the entire log appear to be balanced:
[ 0.135589] enabling clkdm dpll4_clkdm during enable of clk dpll4_m2x2_ck
[ 0.135681] disabling clkdm dpll4_clkdm during disable of clk dpll4_m2x2_ck
[ 0.135833] enabling clkdm dpll4_clkdm during enable of clk dpll4_m2x2_ck
[ 0.135894] disabling clkdm dpll4_clkdm during disable of clk dpll4_m2x2_ck
[ 0.136047] enabling clkdm dpll4_clkdm during enable of clk dpll4_m2x2_ck
[ 0.136138] disabling clkdm dpll4_clkdm during disable of clk dpll4_m2x2_ck
[ 0.136322] enabling clkdm dpll4_clkdm during enable of clk dpll4_m2x2_ck
[ 38.999145] disabling clkdm dpll4_clkdm during disable of clk dpll4_m2x2_ck
- Paul
[ 38.986267] PM: suspend of devices complete after 129.760 msecs
[ 38.991668] PM: late suspend of devices complete after 5.340 msecs
[ 38.996459] disabling clkdm core_l4_clkdm during disable of clk mmchs1_fck
[ 38.998718] disabling clkdm per_clkdm during disable of clk uart3_fck
[ 38.998840] disabling clkdm core_l4_clkdm during disable of clk uart2_fck
[ 38.998931] disabling clkdm core_l4_clkdm during disable of clk uart1_fck
[ 38.999114] disabling clkdm core_l4_clkdm during disable of clk i2c1_fck
[ 38.999145] disabling clkdm dpll4_clkdm during disable of clk dpll4_m2x2_ck
[ 38.999145] ------------[ cut here ]------------
[ 38.999206] WARNING: at arch/arm/mach-omap2/clockdomain.c:962 _clkdm_clk_hwmod_disable+0xa4/0xf8()
[ 38.999206] Modules linked in:
[ 38.999267] [<c001c1cc>] (unwind_backtrace+0x0/0xf0) from [<c0043680>] (warn_slowpath_common+0x4c/0x64)
[ 38.999298] [<c0043680>] (warn_slowpath_common+0x4c/0x64) from [<c00436b4>] (warn_slowpath_null+0x1c/0x24)
[ 38.999328] [<c00436b4>] (warn_slowpath_null+0x1c/0x24) from [<c003695c>] (_clkdm_clk_hwmod_disable+0xa4/0xf8)
[ 38.999359] [<c003695c>] (_clkdm_clk_hwmod_disable+0xa4/0xf8) from [<c0437d38>] (__clk_disable+0x70/0xac)
[ 38.999389] [<c0437d38>] (__clk_disable+0x70/0xac) from [<c0437d94>] (clk_disable+0x20/0x34)
[ 38.999420] [<c0437d94>] (clk_disable+0x20/0x34) from [<c002a330>] (_disable_clocks+0x18/0x68)
[ 38.999420] [<c002a330>] (_disable_clocks+0x18/0x68) from [<c002b09c>] (_idle+0xf8/0x1b4)
[ 38.999450] [<c002b09c>] (_idle+0xf8/0x1b4) from [<c002bf08>] (omap_hwmod_idle+0x24/0x40)
[ 38.999481] [<c002bf08>] (omap_hwmod_idle+0x24/0x40) from [<c002ce34>] (omap_device_idle_hwmods+0x24/0x3c)
[ 38.999511] [<c002ce34>] (omap_device_idle_hwmods+0x24/0x3c) from [<c002d024>] (_omap_device_deactivate+0x9c/0x138)
[ 38.999511] [<c002d024>] (_omap_device_deactivate+0x9c/0x138) from [<c002d984>] (omap_device_idle+0x28/0x54)
[ 38.999542] [<c002d984>] (omap_device_idle+0x28/0x54) from [<c002da48>] (_od_suspend_noirq+0x74/0x7c)
[ 38.999572] [<c002da48>] (_od_suspend_noirq+0x74/0x7c) from [<c034cc6c>] (dpm_run_callback.clone.9+0x30/0xb4)
[ 38.999603] [<c034cc6c>] (dpm_run_callback.clone.9+0x30/0xb4) from [<c034d4c0>] (dpm_suspend_end+0x364/0x554)
[ 38.999603] [<c034d4c0>] (dpm_suspend_end+0x364/0x554) from [<c0084f48>] (suspend_devices_and_enter+0xbc/0x2d0)
[ 38.999633] [<c0084f48>] (suspend_devices_and_enter+0xbc/0x2d0) from [<c00852e8>] (pm_suspend+0x18c/0x208)
[ 38.999664] [<c00852e8>] (pm_suspend+0x18c/0x208) from [<c008457c>] (state_store+0x120/0x134)
[ 38.999694] [<c008457c>] (state_store+0x120/0x134) from [<c02d2a38>] (kobj_attr_store+0x14/0x20)
[ 38.999725] [<c02d2a38>] (kobj_attr_store+0x14/0x20) from [<c017a6b8>] (sysfs_write_file+0x100/0x184)
[ 38.999755] [<c017a6b8>] (sysfs_write_file+0x100/0x184) from [<c01166b8>] (vfs_write+0xb4/0x148)
[ 38.999755] [<c01166b8>] (vfs_write+0xb4/0x148) from [<c011693c>] (sys_write+0x40/0x6c)
[ 38.999786] [<c011693c>] (sys_write+0x40/0x6c) from [<c0013ee0>] (ret_fast_syscall+0x0/0x3c)
[ 38.999816] ---[ end trace 72e2d7bdcf98ea8e ]---
[ 38.999816] disabling clkdm dpll4_clkdm during disable of clk dpll4_ck
[ 38.999847] ------------[ cut here ]------------
[ 38.999847] WARNING: at arch/arm/mach-omap2/clockdomain.c:962 _clkdm_clk_hwmod_disable+0xa4/0xf8()
[ 38.999877] Modules linked in:
[ 38.999908] [<c001c1cc>] (unwind_backtrace+0x0/0xf0) from [<c0043680>] (warn_slowpath_common+0x4c/0x64)
[ 38.999938] [<c0043680>] (warn_slowpath_common+0x4c/0x64) from [<c00436b4>] (warn_slowpath_null+0x1c/0x24)
[ 38.999938] [<c00436b4>] (warn_slowpath_null+0x1c/0x24) from [<c003695c>] (_clkdm_clk_hwmod_disable+0xa4/0xf8)
[ 38.999969] [<c003695c>] (_clkdm_clk_hwmod_disable+0xa4/0xf8) from [<c0437d38>] (__clk_disable+0x70/0xac)
[ 38.999999] [<c0437d38>] (__clk_disable+0x70/0xac) from [<c0437d94>] (clk_disable+0x20/0x34)
[ 38.999999] [<c0437d94>] (clk_disable+0x20/0x34) from [<c002a330>] (_disable_clocks+0x18/0x68)
[ 39.000030] [<c002a330>] (_disable_clocks+0x18/0x68) from [<c002b09c>] (_idle+0xf8/0x1b4)
[ 39.000061] [<c002b09c>] (_idle+0xf8/0x1b4) from [<c002bf08>] (omap_hwmod_idle+0x24/0x40)
[ 39.000061] [<c002bf08>] (omap_hwmod_idle+0x24/0x40) from [<c002ce34>] (omap_device_idle_hwmods+0x24/0x3c)
[ 39.000091] [<c002ce34>] (omap_device_idle_hwmods+0x24/0x3c) from [<c002d024>] (_omap_device_deactivate+0x9c/0x138)
[ 39.000122] [<c002d024>] (_omap_device_deactivate+0x9c/0x138) from [<c002d984>] (omap_device_idle+0x28/0x54)
[ 39.000152] [<c002d984>] (omap_device_idle+0x28/0x54) from [<c002da48>] (_od_suspend_noirq+0x74/0x7c)
[ 39.000152] [<c002da48>] (_od_suspend_noirq+0x74/0x7c) from [<c034cc6c>] (dpm_run_callback.clone.9+0x30/0xb4)
[ 39.000183] [<c034cc6c>] (dpm_run_callback.clone.9+0x30/0xb4) from [<c034d4c0>] (dpm_suspend_end+0x364/0x554)
[ 39.000213] [<c034d4c0>] (dpm_suspend_end+0x364/0x554) from [<c0084f48>] (suspend_devices_and_enter+0xbc/0x2d0)
[ 39.000213] [<c0084f48>] (suspend_devices_and_enter+0xbc/0x2d0) from [<c00852e8>] (pm_suspend+0x18c/0x208)
[ 39.000244] [<c00852e8>] (pm_suspend+0x18c/0x208) from [<c008457c>] (state_store+0x120/0x134)
[ 39.000274] [<c008457c>] (state_store+0x120/0x134) from [<c02d2a38>] (kobj_attr_store+0x14/0x20)
[ 39.000274] [<c02d2a38>] (kobj_attr_store+0x14/0x20) from [<c017a6b8>] (sysfs_write_file+0x100/0x184)
[ 39.000305] [<c017a6b8>] (sysfs_write_file+0x100/0x184) from [<c01166b8>] (vfs_write+0xb4/0x148)
[ 39.000335] [<c01166b8>] (vfs_write+0xb4/0x148) from [<c011693c>] (sys_write+0x40/0x6c)
[ 39.000366] [<c011693c>] (sys_write+0x40/0x6c) from [<c0013ee0>] (ret_fast_syscall+0x0/0x3c)
[ 39.000366] ---[ end trace 72e2d7bdcf98ea8f ]---
[ 39.000823] disabling clkdm wkup_clkdm during disable of clk gpio1_ick
[ 39.000976] PM: noirq suspend of devices complete after 9.307 msecs
[ 39.001068] Disabling non-boot CPUs ...
The enables and disables of dpll4_m2x2_ck from the entire log appear to be balanced:
Looks like this is an artifact of the disable-unused-clocks problem. The
disable of dpll4_m6x2_ck during that phase removes the usecount that was
previously added by the enable of dpll4_m2x2_ck. So fixing that problem
should clean this one up.
- Paul
From: Mike Turquette <hidden> Date: 2012-11-09 00:57:27
Quoting Paul Walmsley (2012-11-08 16:11:12)
On Thu, 8 Nov 2012, Paul Walmsley wrote:
quoted
On Thu, 8 Nov 2012, Paul Walmsley wrote:
quoted
Am seeing warnings during the disable-unused-clocks phase of the boot on
the OMAP3 test boards here.
Similar problems during system suspend on 3530ES3 Beagle. Not sure
what's causing these yet. At this point the clockdomain usecounts
should be accurate.
Here's a redacted debugging log for these cases. The suspend events start
around the 30 second mark.
One observation is that dpll4_m5x2_ck and dpll4_m6x2_ck are never enabled.
The tracebacks occur when something in the suspend path tries to disable
those clocks.
Hi Paul,
My instrumentation shows that dpll4_ck & dpll4_m2x2_ck are triggering
the WARNs:
[ 25.214599] _clkdm_clk_hwmod_disable: dpll4_m2x2_ck
[ 25.214599] ------------[ cut here ]------------
[ 25.214660] WARNING: at arch/arm/mach-omap2/clockdomain.c:967
_clkdm_clk_hwmod_disable+0xd0/0x118()
...
[ 25.215209] _clkdm_clk_hwmod_disable: dpll4_ck
[ 25.215209] ------------[ cut here ]------------
[ 25.215240] WARNING: at arch/arm/mach-omap2/clockdomain.c:967
_clkdm_clk_hwmod_disable+0xd0/0x118()
Patch that give that information:
From: Mike Turquette <hidden> Date: 2012-11-08 23:31:56
The OMAP port to the common clk framework[1] resulted in spurious WARNs
while disable unused clocks. This is due to _clkdm_clk_hwmod_disable
catching clkdm->usecount's with a value of zero. Even less desirable it
would not allow the clkdm_clk_disable function pointer to get called due
to an early return of -ERANGE.
This patch adds a check for such a corner case by skipping the WARN and
early return in the event that clkdm->usecount and clk->enable_usecount
are both zero. Presumably this could only happen during the check for
unused clocks at boot-time.
[1] http://article.gmane.org/gmane.linux.ports.arm.omap/88824
Signed-off-by: Mike Turquette <redacted>
---
arch/arm/mach-omap2/clockdomain.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
The OMAP port to the common clk framework[1] resulted in spurious WARNs
while disable unused clocks. This is due to _clkdm_clk_hwmod_disable
catching clkdm->usecount's with a value of zero. Even less desirable it
would not allow the clkdm_clk_disable function pointer to get called due
to an early return of -ERANGE.
This patch adds a check for such a corner case by skipping the WARN and
early return in the event that clkdm->usecount and clk->enable_usecount
are both zero. Presumably this could only happen during the check for
unused clocks at boot-time.
[1] http://article.gmane.org/gmane.linux.ports.arm.omap/88824
Signed-off-by: Mike Turquette <redacted>
I don't think this is going to work, as it currently stands. The code
will just bypass the warning and the error return. The clockdomain
usecount still will be decremented, which is going to cause problems since
the usecount will be inaccurate.
- Paul
From: Mike Turquette <hidden> Date: 2012-11-09 01:17:38
Quoting Paul Walmsley (2012-11-08 16:58:21)
On Thu, 8 Nov 2012, Mike Turquette wrote:
quoted
The OMAP port to the common clk framework[1] resulted in spurious WARNs
while disable unused clocks. This is due to _clkdm_clk_hwmod_disable
catching clkdm->usecount's with a value of zero. Even less desirable it
would not allow the clkdm_clk_disable function pointer to get called due
to an early return of -ERANGE.
This patch adds a check for such a corner case by skipping the WARN and
early return in the event that clkdm->usecount and clk->enable_usecount
are both zero. Presumably this could only happen during the check for
unused clocks at boot-time.
[1] http://article.gmane.org/gmane.linux.ports.arm.omap/88824
Signed-off-by: Mike Turquette <redacted>
I don't think this is going to work, as it currently stands. The code
will just bypass the warning and the error return. The clockdomain
usecount still will be decremented, which is going to cause problems since
the usecount will be inaccurate.
You're right. In my rush I glossed over the clkdm decrement part. In
light of the suspend/resume issues I'm not sure this approach is really
valid. I think getting to the bottom of those issues will give the
final word.
Regards,
Mike
You're right. In my rush I glossed over the clkdm decrement part. In
light of the suspend/resume issues I'm not sure this approach is really
valid. I think getting to the bottom of those issues will give the
final word.
What do you think about something like this? It's still under test and
review here, but seems to avoid the warnings on 3530ES3 Beagle at least.
The usage of __clk_get_enable_count() in this code still seems like a hack
to me. It would be better for the CCF to call a different clk_hw_ops
function pointer for the disable-unused-clocks case. But if you agree,
and plan to fix this, or have some other cleaner fix in mind for the near
future, then something like this seems reasonable for the short term to
me. What do you think?
- Paul
You're right. In my rush I glossed over the clkdm decrement part. In
light of the suspend/resume issues I'm not sure this approach is really
valid. I think getting to the bottom of those issues will give the
final word.
What do you think about something like this? It's still under test and
review here, but seems to avoid the warnings on 3530ES3 Beagle at least.
The usage of __clk_get_enable_count() in this code still seems like a hack
to me. It would be better for the CCF to call a different clk_hw_ops
function pointer for the disable-unused-clocks case. But if you agree,
and plan to fix this, or have some other cleaner fix in mind for the near
future, then something like this seems reasonable for the short term to
me. What do you think?
Here's the patch. The changes to clkdm_clk_disable() are the important
ones, the rest can be ignored for the purposes of this review.
- Paul
From: Mike Turquette <redacted>
Date: Fri, 9 Nov 2012 11:28:42 -0700
Subject: [PATCH] ARM: OMAP2+: clockdomain: bypass clockdomain handling when
disabling unused clks
The OMAP port to the common clk framework[1] resulted in spurious WARNs
while disable unused clocks. This is due to _clkdm_clk_hwmod_disable
catching clkdm->usecount's with a value of zero. Even less desirable it
would not allow the clkdm_clk_disable function pointer to get called due
to an early return of -ERANGE.
This patch adds a check for such a corner case by skipping the WARN and
early return in the event that clkdm->usecount and clk->enable_usecount
are both zero. Presumably this could only happen during the check for
unused clocks at boot-time.
[1] http://article.gmane.org/gmane.linux.ports.arm.omap/88824
Signed-off-by: Mike Turquette <redacted>
[paul at pwsan.com: split the hwmod and clock disable cases; modified the
code to skip the clockdomain handling during the disable-unused-clocks phase]
---
arch/arm/mach-omap2/clockdomain.c | 91 ++++++++++++++++++++++---------------
1 file changed, 54 insertions(+), 37 deletions(-)
From: Mike Turquette <hidden> Date: 2012-11-09 19:40:23
Quoting Paul Walmsley (2012-11-09 11:08:00)
On Fri, 9 Nov 2012, Paul Walmsley wrote:
quoted
On Thu, 8 Nov 2012, Mike Turquette wrote:
quoted
You're right. In my rush I glossed over the clkdm decrement part. In
light of the suspend/resume issues I'm not sure this approach is really
valid. I think getting to the bottom of those issues will give the
final word.
What do you think about something like this? It's still under test and
review here, but seems to avoid the warnings on 3530ES3 Beagle at least.
The usage of __clk_get_enable_count() in this code still seems like a hack
to me. It would be better for the CCF to call a different clk_hw_ops
function pointer for the disable-unused-clocks case. But if you agree,
and plan to fix this, or have some other cleaner fix in mind for the near
future, then something like this seems reasonable for the short term to
me. What do you think?
For avoiding the WARNs, this seems fine to me. And I agree that a new
clk_ops function pointer is needed. Maybe something like,
void (*unused_disable)(struct clk_hw *hw);
But I'm OK with the below patch in the short term. I just have one
question: did you observe any PM regressions by skipping the clkdm
programming?
Thanks,
Mike
quoted hunk
Here's the patch. The changes to clkdm_clk_disable() are the important
ones, the rest can be ignored for the purposes of this review.
- Paul
From: Mike Turquette <redacted>
Date: Fri, 9 Nov 2012 11:28:42 -0700
Subject: [PATCH] ARM: OMAP2+: clockdomain: bypass clockdomain handling when
disabling unused clks
The OMAP port to the common clk framework[1] resulted in spurious WARNs
while disable unused clocks. This is due to _clkdm_clk_hwmod_disable
catching clkdm->usecount's with a value of zero. Even less desirable it
would not allow the clkdm_clk_disable function pointer to get called due
to an early return of -ERANGE.
This patch adds a check for such a corner case by skipping the WARN and
early return in the event that clkdm->usecount and clk->enable_usecount
are both zero. Presumably this could only happen during the check for
unused clocks at boot-time.
[1] http://article.gmane.org/gmane.linux.ports.arm.omap/88824
Signed-off-by: Mike Turquette <redacted>
[paul at pwsan.com: split the hwmod and clock disable cases; modified the
code to skip the clockdomain handling during the disable-unused-clocks phase]
---
arch/arm/mach-omap2/clockdomain.c | 91 ++++++++++++++++++++++---------------
1 file changed, 54 insertions(+), 37 deletions(-)
But I'm OK with the below patch in the short term. I just have one
question: did you observe any PM regressions by skipping the clkdm
programming?
It's still under test here but 3530ES3 Beagle passed the PM tests with it,
with no obvious warnings.
OK I'm satisfied with this general fix. Will have to break it up into
pieces and move it earlier in the patch stack to ensure that PM doesn't
break in the middle of the series, but the idea seems reasonably sound for
short-term use. A clock should only be disabled during the
disable-unused-clocks phase if its usecount is zero and it's enabled in
the hardware. As far as I can tell, this can only happen when the clock
is enabled by the bootloader/ROM code/PPA, and never touched by the kernel
before the disable-unused-clocks phase. The only regression cases that
come to mind at the moment are:
1. CONFIG_PM=n and a clockdomain only contains unused clocks; or
2. CONFIG_PM=y and a clockdomain only contains unused clocks and that same
clockdomain does not support hardware-supervised autoidle or force-idle
In those cases, I'd assume the clockdomain would effectively get stuck on.
#1 we don't really care about since CONFIG_PM=n anyway. #2 is more of a
concern but it should disappear completely once hwmods are present for all
of the IP blocks on the device. (The presence of the hwmods will ensure
that every clock that matters from an idle-management perspective is
enabled at least once when the IP block is reset.) For this reason, it's
important to note that when the hwmod reset code is moved to run late in
the boot process, it will have to run before the unused clocks are
disabled.
One nice side-effect of this bug is that it highlighted that we're still
missing OMAP3 hwmods for the camera subsystem, and that the reset code for
the SAD2D module needs to be moved out of
arch/arm/mach-omap2/pm34xx.c:omap3_d2d_idle() and into something that can
be called from the hwmod reset code.
- Paul