* [PATCH 1/3] pwm: core: Rearrange pwm lock.
2013-12-18 11:36 [PATCH 0/3] pwm: ti: Miscellaneous Fixes and cleanup for pwm Sourav Poddar
@ 2013-12-18 11:36 ` Sourav Poddar
2014-01-23 13:54 ` Thierry Reding
2013-12-18 11:36 ` [PATCH 2/3] driver: pwm: ti-ecap: Remove duplicate put_sync call Sourav Poddar
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Sourav Poddar @ 2013-12-18 11:36 UTC (permalink / raw)
To: thierry.reding; +Cc: linux-pwm, linux-omap, balbi, Sourav Poddar
When tiecap is used as a module, then while doing a rmmod I
get the following dump.
root@am437x-evm:/# rmmod pwm_tiecap
[ 219.539245]
[ 219.540771] ======================================================
[ 219.546936] [ INFO: possible circular locking dependency detected ]
[ 219.553192] 3.12.4-01557-g9921cde-dirty #134 Not tainted
[ 219.558471] -------------------------------------------------------
[ 219.564727] rmmod/1517 is trying to acquire lock:
[ 219.569427] (s_active#35){++++.+}, at: [<c017ab00>] sysfs_hash_and_remove+0x4c/0x8c
[ 219.577239]
[ 219.577239] but task is already holding lock:
[ 219.583068] (pwm_lock){+.+.+.}, at: [<c0303598>] pwmchip_remove+0x14/0xf8
[ 219.589996]
[ 219.589996] which lock already depends on the new lock.
[ 219.589996]
[ 219.598144]
[ 219.598144] the existing dependency chain (in reverse order) is:
[ 219.605590]
-> #1 (pwm_lock){+.+.+.}:
[ 219.609497] [<c00a2d1c>] lock_acquire+0x9c/0x128
[ 219.614746] [<c0639bc0>] mutex_lock_nested+0x50/0x3dc
[ 219.620391] [<c0303974>] pwm_request_from_chip+0x38/0x6c
[ 219.626312] [<c0303fe0>] pwm_export_store+0x50/0x140
[ 219.631896] [<c039aba8>] dev_attr_store+0x18/0x24
[ 219.637207] [<c017aff0>] sysfs_write_file+0x16c/0x1a0
[ 219.642883] [<c0119084>] vfs_write+0xb0/0x188
[ 219.647857] [<c0119478>] SyS_write+0x3c/0x70
[ 219.652770] [<c0014100>] ret_fast_syscall+0x0/0x48
[ 219.658172]
-> #0 (s_active#35){++++.+}:
[ 219.662353] [<c00a2778>] __lock_acquire+0x1b28/0x1b70
[ 219.667999] [<c00a2d1c>] lock_acquire+0x9c/0x128
[ 219.673248] [<c017c780>] sysfs_addrm_finish+0xe8/0x158
[ 219.678985] [<c017ab00>] sysfs_hash_and_remove+0x4c/0x8c
[ 219.684906] [<c017e224>] remove_files+0x38/0x74
[ 219.690063] [<c017e2a4>] sysfs_remove_group+0x44/0x108
[ 219.695800] [<c017e38c>] sysfs_remove_groups+0x24/0x34
[ 219.701538] [<c039bc2c>] device_del+0xec/0x178
[ 219.706604] [<c039bcc4>] device_unregister+0xc/0x18
[ 219.712097] [<c0303658>] pwmchip_remove+0xd4/0xf8
[ 219.717407] [<c039fdc4>] platform_drv_remove+0x18/0x1c
[ 219.723175] [<c039e6c4>] __device_release_driver+0x70/0xc8
[ 219.729248] [<c039eec8>] driver_detach+0xb4/0xb8
[ 219.734497] [<c039e4ec>] bus_remove_driver+0x8c/0xd0
[ 219.740081] [<c00abd2c>] SyS_delete_module+0x118/0x22c
[ 219.745819] [<c0014100>] ret_fast_syscall+0x0/0x48
[ 219.751220]
[ 219.751220] other info that might help us debug this:
[ 219.751220]
[ 219.759216] Possible unsafe locking scenario:
[ 219.759216]
[ 219.765106] CPU0 CPU1
[ 219.769622] ---- ----
[ 219.774139] lock(pwm_lock);
[ 219.777130] lock(s_active#35);
[ 219.782897] lock(pwm_lock);
[ 219.788391] lock(s_active#35);
[ 219.791656]
[ 219.791656] *** DEADLOCK ***
[ 219.791656]
[ 219.797546] 3 locks held by rmmod/1517:
[ 219.801391] #0: (&__lockdep_no_validate__){......}, at: [<c039ee58>] driver_detach+0x44/0xb8
[ 219.810028] #1: (&__lockdep_no_validate__){......}, at: [<c039ee64>] driver_detach+0x50/0xb8
[ 219.818695] #2: (pwm_lock){+.+.+.}, at: [<c0303598>] pwmchip_remove+0x14/0xf8
[ 219.826049]
[ 219.826049] stack backtrace:
[ 219.830413] CPU: 0 PID: 1517 Comm: rmmod Not tainted 3.12.4-01557-g9921cde-dirty #134
[ 219.838256] [<c001cc98>] (unwind_backtrace+0x0/0xf0) from [<c0018124>] (show_stack+0x10/0x14)
[ 219.846771] [<c0018124>] (show_stack+0x10/0x14) from [<c0636728>] (dump_stack+0x74/0xb4)
[ 219.854858] [<c0636728>] (dump_stack+0x74/0xb4) from [<c06344e4>] (print_circular_bug+0x284/0x2d8)
[ 219.863830] [<c06344e4>] (print_circular_bug+0x284/0x2d8) from [<c00a2778>] (__lock_acquire+0x1b28/0x1b70)
[ 219.873443] [<c00a2778>] (__lock_acquire+0x1b28/0x1b70) from [<c00a2d1c>] (lock_acquire+0x9c/0x128)
[ 219.882476] [<c00a2d1c>] (lock_acquire+0x9c/0x128) from [<c017c780>] (sysfs_addrm_finish+0xe8/0x158)
[ 219.891601] [<c017c780>] (sysfs_addrm_finish+0xe8/0x158) from [<c017ab00>] (sysfs_hash_and_remove+0x4c/0x8c)
[ 219.901397] [<c017ab00>] (sysfs_hash_and_remove+0x4c/0x8c) from [<c017e224>] (remove_files+0x38/0x74)
[ 219.910614] [<c017e224>] (remove_files+0x38/0x74) from [<c017e2a4>] (sysfs_remove_group+0x44/0x108)
[ 219.919647] [<c017e2a4>] (sysfs_remove_group+0x44/0x108) from [<c017e38c>] (sysfs_remove_groups+0x24/0x34)
[ 219.929260] [<c017e38c>] (sysfs_remove_groups+0x24/0x34) from [<c039bc2c>] (device_del+0xec/0x178)
[ 219.938201] [<c039bc2c>] (device_del+0xec/0x178) from [<c039bcc4>] (device_unregister+0xc/0x18)
[ 219.946899] [<c039bcc4>] (device_unregister+0xc/0x18) from [<c0303658>] (pwmchip_remove+0xd4/0xf8)
[ 219.955841] [<c0303658>] (pwmchip_remove+0xd4/0xf8) from [<c039fdc4>] (platform_drv_remove+0x18/0x1c)
[ 219.965057] [<c039fdc4>] (platform_drv_remove+0x18/0x1c) from [<c039e6c4>] (__device_release_driver+0x70/0xc8)
[ 219.975006] [<c039e6c4>] (__device_release_driver+0x70/0xc8) from [<c039eec8>] (driver_detach+0xb4/0xb8)
[ 219.984466] [<c039eec8>] (driver_detach+0xb4/0xb8) from [<c039e4ec>] (bus_remove_driver+0x8c/0xd0)
[ 219.993438] [<c039e4ec>] (bus_remove_driver+0x8c/0xd0) from [<c00abd2c>] (SyS_delete_module+0x118/0x22c)
[ 220.002899] [<c00abd2c>] (SyS_delete_module+0x118/0x22c) from [<c0014100>] (ret_fast_syscall+0x0/0x48)
Looks like s_active lock cannot be held while pwm lock is held.
The patch fixes the above issue by unlocking the pwm lock before acquiring the
sysfs lock.
Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---
drivers/pwm/core.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 2ca9504..3e1d499 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -300,6 +300,7 @@ int pwmchip_remove(struct pwm_chip *chip)
if (test_bit(PWMF_REQUESTED, &pwm->flags)) {
ret = -EBUSY;
+ mutex_unlock(&pwm_lock);
goto out;
}
}
@@ -311,10 +312,11 @@ int pwmchip_remove(struct pwm_chip *chip)
free_pwms(chip);
+ mutex_unlock(&pwm_lock);
+
pwmchip_sysfs_unexport(chip);
out:
- mutex_unlock(&pwm_lock);
return ret;
}
EXPORT_SYMBOL_GPL(pwmchip_remove);
--
1.7.1
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH 1/3] pwm: core: Rearrange pwm lock.
2013-12-18 11:36 ` [PATCH 1/3] pwm: core: Rearrange pwm lock Sourav Poddar
@ 2014-01-23 13:54 ` Thierry Reding
0 siblings, 0 replies; 8+ messages in thread
From: Thierry Reding @ 2014-01-23 13:54 UTC (permalink / raw)
To: Sourav Poddar; +Cc: linux-pwm, linux-omap, balbi
[-- Attachment #1: Type: text/plain, Size: 6311 bytes --]
On Wed, Dec 18, 2013 at 05:06:53PM +0530, Sourav Poddar wrote:
> When tiecap is used as a module, then while doing a rmmod I
> get the following dump.
>
> root@am437x-evm:/# rmmod pwm_tiecap
> [ 219.539245]
> [ 219.540771] ======================================================
> [ 219.546936] [ INFO: possible circular locking dependency detected ]
> [ 219.553192] 3.12.4-01557-g9921cde-dirty #134 Not tainted
> [ 219.558471] -------------------------------------------------------
> [ 219.564727] rmmod/1517 is trying to acquire lock:
> [ 219.569427] (s_active#35){++++.+}, at: [<c017ab00>] sysfs_hash_and_remove+0x4c/0x8c
> [ 219.577239]
> [ 219.577239] but task is already holding lock:
> [ 219.583068] (pwm_lock){+.+.+.}, at: [<c0303598>] pwmchip_remove+0x14/0xf8
> [ 219.589996]
> [ 219.589996] which lock already depends on the new lock.
> [ 219.589996]
> [ 219.598144]
> [ 219.598144] the existing dependency chain (in reverse order) is:
> [ 219.605590]
> -> #1 (pwm_lock){+.+.+.}:
> [ 219.609497] [<c00a2d1c>] lock_acquire+0x9c/0x128
> [ 219.614746] [<c0639bc0>] mutex_lock_nested+0x50/0x3dc
> [ 219.620391] [<c0303974>] pwm_request_from_chip+0x38/0x6c
> [ 219.626312] [<c0303fe0>] pwm_export_store+0x50/0x140
> [ 219.631896] [<c039aba8>] dev_attr_store+0x18/0x24
> [ 219.637207] [<c017aff0>] sysfs_write_file+0x16c/0x1a0
> [ 219.642883] [<c0119084>] vfs_write+0xb0/0x188
> [ 219.647857] [<c0119478>] SyS_write+0x3c/0x70
> [ 219.652770] [<c0014100>] ret_fast_syscall+0x0/0x48
> [ 219.658172]
> -> #0 (s_active#35){++++.+}:
> [ 219.662353] [<c00a2778>] __lock_acquire+0x1b28/0x1b70
> [ 219.667999] [<c00a2d1c>] lock_acquire+0x9c/0x128
> [ 219.673248] [<c017c780>] sysfs_addrm_finish+0xe8/0x158
> [ 219.678985] [<c017ab00>] sysfs_hash_and_remove+0x4c/0x8c
> [ 219.684906] [<c017e224>] remove_files+0x38/0x74
> [ 219.690063] [<c017e2a4>] sysfs_remove_group+0x44/0x108
> [ 219.695800] [<c017e38c>] sysfs_remove_groups+0x24/0x34
> [ 219.701538] [<c039bc2c>] device_del+0xec/0x178
> [ 219.706604] [<c039bcc4>] device_unregister+0xc/0x18
> [ 219.712097] [<c0303658>] pwmchip_remove+0xd4/0xf8
> [ 219.717407] [<c039fdc4>] platform_drv_remove+0x18/0x1c
> [ 219.723175] [<c039e6c4>] __device_release_driver+0x70/0xc8
> [ 219.729248] [<c039eec8>] driver_detach+0xb4/0xb8
> [ 219.734497] [<c039e4ec>] bus_remove_driver+0x8c/0xd0
> [ 219.740081] [<c00abd2c>] SyS_delete_module+0x118/0x22c
> [ 219.745819] [<c0014100>] ret_fast_syscall+0x0/0x48
> [ 219.751220]
> [ 219.751220] other info that might help us debug this:
> [ 219.751220]
> [ 219.759216] Possible unsafe locking scenario:
> [ 219.759216]
> [ 219.765106] CPU0 CPU1
> [ 219.769622] ---- ----
> [ 219.774139] lock(pwm_lock);
> [ 219.777130] lock(s_active#35);
> [ 219.782897] lock(pwm_lock);
> [ 219.788391] lock(s_active#35);
> [ 219.791656]
> [ 219.791656] *** DEADLOCK ***
> [ 219.791656]
> [ 219.797546] 3 locks held by rmmod/1517:
> [ 219.801391] #0: (&__lockdep_no_validate__){......}, at: [<c039ee58>] driver_detach+0x44/0xb8
> [ 219.810028] #1: (&__lockdep_no_validate__){......}, at: [<c039ee64>] driver_detach+0x50/0xb8
> [ 219.818695] #2: (pwm_lock){+.+.+.}, at: [<c0303598>] pwmchip_remove+0x14/0xf8
> [ 219.826049]
> [ 219.826049] stack backtrace:
> [ 219.830413] CPU: 0 PID: 1517 Comm: rmmod Not tainted 3.12.4-01557-g9921cde-dirty #134
> [ 219.838256] [<c001cc98>] (unwind_backtrace+0x0/0xf0) from [<c0018124>] (show_stack+0x10/0x14)
> [ 219.846771] [<c0018124>] (show_stack+0x10/0x14) from [<c0636728>] (dump_stack+0x74/0xb4)
> [ 219.854858] [<c0636728>] (dump_stack+0x74/0xb4) from [<c06344e4>] (print_circular_bug+0x284/0x2d8)
> [ 219.863830] [<c06344e4>] (print_circular_bug+0x284/0x2d8) from [<c00a2778>] (__lock_acquire+0x1b28/0x1b70)
> [ 219.873443] [<c00a2778>] (__lock_acquire+0x1b28/0x1b70) from [<c00a2d1c>] (lock_acquire+0x9c/0x128)
> [ 219.882476] [<c00a2d1c>] (lock_acquire+0x9c/0x128) from [<c017c780>] (sysfs_addrm_finish+0xe8/0x158)
> [ 219.891601] [<c017c780>] (sysfs_addrm_finish+0xe8/0x158) from [<c017ab00>] (sysfs_hash_and_remove+0x4c/0x8c)
> [ 219.901397] [<c017ab00>] (sysfs_hash_and_remove+0x4c/0x8c) from [<c017e224>] (remove_files+0x38/0x74)
> [ 219.910614] [<c017e224>] (remove_files+0x38/0x74) from [<c017e2a4>] (sysfs_remove_group+0x44/0x108)
> [ 219.919647] [<c017e2a4>] (sysfs_remove_group+0x44/0x108) from [<c017e38c>] (sysfs_remove_groups+0x24/0x34)
> [ 219.929260] [<c017e38c>] (sysfs_remove_groups+0x24/0x34) from [<c039bc2c>] (device_del+0xec/0x178)
> [ 219.938201] [<c039bc2c>] (device_del+0xec/0x178) from [<c039bcc4>] (device_unregister+0xc/0x18)
> [ 219.946899] [<c039bcc4>] (device_unregister+0xc/0x18) from [<c0303658>] (pwmchip_remove+0xd4/0xf8)
> [ 219.955841] [<c0303658>] (pwmchip_remove+0xd4/0xf8) from [<c039fdc4>] (platform_drv_remove+0x18/0x1c)
> [ 219.965057] [<c039fdc4>] (platform_drv_remove+0x18/0x1c) from [<c039e6c4>] (__device_release_driver+0x70/0xc8)
> [ 219.975006] [<c039e6c4>] (__device_release_driver+0x70/0xc8) from [<c039eec8>] (driver_detach+0xb4/0xb8)
> [ 219.984466] [<c039eec8>] (driver_detach+0xb4/0xb8) from [<c039e4ec>] (bus_remove_driver+0x8c/0xd0)
> [ 219.993438] [<c039e4ec>] (bus_remove_driver+0x8c/0xd0) from [<c00abd2c>] (SyS_delete_module+0x118/0x22c)
> [ 220.002899] [<c00abd2c>] (SyS_delete_module+0x118/0x22c) from [<c0014100>] (ret_fast_syscall+0x0/0x48)
>
> Looks like s_active lock cannot be held while pwm lock is held.
> The patch fixes the above issue by unlocking the pwm lock before acquiring the
> sysfs lock.
I've been trying to reproduce this, but I can't. I've enabled LOCKDEP
and PROVE_LOCKING in Kconfig, booted a Tegra-based board and did a
couple of modprobe pwm-tegra && modprobe -r pwm-tegra. But I never saw
LOCKDEP complain.
Can you reproduce the issue on latest linux-next? Or is there something
else I should be doing to trigger this?
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/3] driver: pwm: ti-ecap: Remove duplicate put_sync call.
2013-12-18 11:36 [PATCH 0/3] pwm: ti: Miscellaneous Fixes and cleanup for pwm Sourav Poddar
2013-12-18 11:36 ` [PATCH 1/3] pwm: core: Rearrange pwm lock Sourav Poddar
@ 2013-12-18 11:36 ` Sourav Poddar
2014-01-23 14:19 ` Thierry Reding
2013-12-18 11:36 ` [PATCH 3/3] driver: pwmss: Disable stop clk bit during enable clock call Sourav Poddar
2014-01-08 6:47 ` [PATCH 0/3] pwm: ti: Miscellaneous Fixes and cleanup for pwm sourav
3 siblings, 1 reply; 8+ messages in thread
From: Sourav Poddar @ 2013-12-18 11:36 UTC (permalink / raw)
To: thierry.reding; +Cc: linux-pwm, linux-omap, balbi, Sourav Poddar
Remove duplicate 'pm_runtime_put_sync' in the remove path.
Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---
drivers/pwm/pwm-tiecap.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/drivers/pwm/pwm-tiecap.c b/drivers/pwm/pwm-tiecap.c
index 4e5c3d1..032092c 100644
--- a/drivers/pwm/pwm-tiecap.c
+++ b/drivers/pwm/pwm-tiecap.c
@@ -279,7 +279,6 @@ static int ecap_pwm_remove(struct platform_device *pdev)
pwmss_submodule_state_change(pdev->dev.parent, PWMSS_ECAPCLK_STOP_REQ);
pm_runtime_put_sync(&pdev->dev);
- pm_runtime_put_sync(&pdev->dev);
pm_runtime_disable(&pdev->dev);
return pwmchip_remove(&pc->chip);
}
--
1.7.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] driver: pwm: ti-ecap: Remove duplicate put_sync call.
2013-12-18 11:36 ` [PATCH 2/3] driver: pwm: ti-ecap: Remove duplicate put_sync call Sourav Poddar
@ 2014-01-23 14:19 ` Thierry Reding
0 siblings, 0 replies; 8+ messages in thread
From: Thierry Reding @ 2014-01-23 14:19 UTC (permalink / raw)
To: Sourav Poddar; +Cc: linux-pwm, linux-omap, balbi
[-- Attachment #1: Type: text/plain, Size: 314 bytes --]
On Wed, Dec 18, 2013 at 05:06:54PM +0530, Sourav Poddar wrote:
> Remove duplicate 'pm_runtime_put_sync' in the remove path.
>
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
> ---
> drivers/pwm/pwm-tiecap.c | 1 -
> 1 files changed, 0 insertions(+), 1 deletions(-)
Applied, thanks.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] driver: pwmss: Disable stop clk bit during enable clock call.
2013-12-18 11:36 [PATCH 0/3] pwm: ti: Miscellaneous Fixes and cleanup for pwm Sourav Poddar
2013-12-18 11:36 ` [PATCH 1/3] pwm: core: Rearrange pwm lock Sourav Poddar
2013-12-18 11:36 ` [PATCH 2/3] driver: pwm: ti-ecap: Remove duplicate put_sync call Sourav Poddar
@ 2013-12-18 11:36 ` Sourav Poddar
2014-01-23 14:17 ` Thierry Reding
2014-01-08 6:47 ` [PATCH 0/3] pwm: ti: Miscellaneous Fixes and cleanup for pwm sourav
3 siblings, 1 reply; 8+ messages in thread
From: Sourav Poddar @ 2013-12-18 11:36 UTC (permalink / raw)
To: thierry.reding; +Cc: linux-pwm, linux-omap, balbi, Sourav Poddar
Writing to ecap register on second insmod crashes with an external
abort. This happens becuase the STOP_CLK bit remains set(from rmmod)
during the second insmod thereby not allowing the clocks to get enabled.
So, we disable STOP clock bit while doing a clock enable.
Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---
drivers/pwm/pwm-tipwmss.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/pwm/pwm-tipwmss.c b/drivers/pwm/pwm-tipwmss.c
index 3b119bc..4749866 100644
--- a/drivers/pwm/pwm-tipwmss.c
+++ b/drivers/pwm/pwm-tipwmss.c
@@ -40,6 +40,8 @@ u16 pwmss_submodule_state_change(struct device *dev, int set)
mutex_lock(&info->pwmss_lock);
val = readw(info->mmio_base + PWMSS_CLKCONFIG);
+ if (set == PWMSS_ECAPCLK_EN)
+ val &= ~PWMSS_ECAPCLK_STOP_REQ;
val |= set;
writew(val , info->mmio_base + PWMSS_CLKCONFIG);
mutex_unlock(&info->pwmss_lock);
--
1.7.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] driver: pwmss: Disable stop clk bit during enable clock call.
2013-12-18 11:36 ` [PATCH 3/3] driver: pwmss: Disable stop clk bit during enable clock call Sourav Poddar
@ 2014-01-23 14:17 ` Thierry Reding
0 siblings, 0 replies; 8+ messages in thread
From: Thierry Reding @ 2014-01-23 14:17 UTC (permalink / raw)
To: Sourav Poddar; +Cc: linux-pwm, linux-omap, balbi
[-- Attachment #1: Type: text/plain, Size: 3244 bytes --]
On Wed, Dec 18, 2013 at 05:06:55PM +0530, Sourav Poddar wrote:
> Writing to ecap register on second insmod crashes with an external
> abort. This happens becuase the STOP_CLK bit remains set(from rmmod)
> during the second insmod thereby not allowing the clocks to get enabled.
>
> So, we disable STOP clock bit while doing a clock enable.
>
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
> ---
> drivers/pwm/pwm-tipwmss.c | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/pwm/pwm-tipwmss.c b/drivers/pwm/pwm-tipwmss.c
> index 3b119bc..4749866 100644
> --- a/drivers/pwm/pwm-tipwmss.c
> +++ b/drivers/pwm/pwm-tipwmss.c
> @@ -40,6 +40,8 @@ u16 pwmss_submodule_state_change(struct device *dev, int set)
>
> mutex_lock(&info->pwmss_lock);
> val = readw(info->mmio_base + PWMSS_CLKCONFIG);
> + if (set == PWMSS_ECAPCLK_EN)
> + val &= ~PWMSS_ECAPCLK_STOP_REQ;
Should this be done for set == PWMSS_EPWMCLK_EN as well? Also how does
this behave if somebody goes and passes:
PWMSS_ECAPCLK_EN | PWMSS_ECAPCLK_STOP_REQ
as the "set" parameter.
I think that perhaps the pwmss_submodule_state_change() API should be
rethought. Instead of taking a value that's directly written into the
register, perhaps it should abstract away what this does.
From my understanding this is used to enable (or disable) the clock for
a specific submodule (ECAP or EHRPWM). Perhaps an interface like the
following would be more intuitive:
bool pwmss_module_enable(struct device *dev, enum pwmss_module module)
{
struct pwmss_info *info = dev_get_drvdata(dev);
u16 value, mask, state, ack;
switch (module) {
case PWMSS_MODULE_ECAP:
mask = PWMSS_ECAPCLK_EN | PWMSS_ECAPCLK_STOP_REQ;
state = PWMSS_ECAPCLK_EN;
ack = PWMSS_ECAPCLK_EN_ACK;
break;
case PWMSS_MODULE_EPWM:
mask = PWMSS_EPWMCLK_EN | PWMSS_EPWMCLK_STOP_REQ;
state = PWMSS_EPWMCLK_EN;
ack = PWMSS_ECAPCLK_EN_ACK;
break;
default:
return false;
}
mutex_lock(&info->pwmss_lock);
value = readw(info->mmio + PWMSS_CLKCONFIG);
value &= ~mask;
value |= state;
writew(value, info->mmio + PWMSS_CLKCONFIG);
mutex_unlock(&info->pwmss_lock);
value = readw(info->mmio + PWMSS_CLKSTATUS);
return (value & ack) != 0;
}
void pwmss_module_disable(struct device *dev, enum pwmss_module module)
{
struct pwmss_info *info = dev_get_drvdata(dev);
u16 value, mask, state;
switch (module) {
case PWMSS_MODULE_ECAP:
mask = PWMSS_ECAPCLK_EN | PWMSS_ECAPCLK_STOP_REQ;
state = PWMSS_ECAPCLK_STOP_REQ;
break;
case PWMSS_MODULE_EPWM:
mask = PWMSS_EPWMCLK_EN | PWMSS_EPWMCLK_STOP_REQ;
state = PWMSS_EPWMCLK_STOP_REQ;
break;
default:
return false;
}
mutex_lock(&info->pwmss_lock);
value = readw(info->mmio + PWMSS_CLKCONFIG);
value &= ~mask;
value |= state;
writew(value, info->mmio + PWMSS_CLKCONFIG);
mutex_unlock(&info->pwmss_lock);
}
One possible other interesting alternative would be to export this
functionality via the common clock framework, since you're essentially
exposing clk_enable() and clk_disable(). That's probably overkill,
though.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/3] pwm: ti: Miscellaneous Fixes and cleanup for pwm.
2013-12-18 11:36 [PATCH 0/3] pwm: ti: Miscellaneous Fixes and cleanup for pwm Sourav Poddar
` (2 preceding siblings ...)
2013-12-18 11:36 ` [PATCH 3/3] driver: pwmss: Disable stop clk bit during enable clock call Sourav Poddar
@ 2014-01-08 6:47 ` sourav
3 siblings, 0 replies; 8+ messages in thread
From: sourav @ 2014-01-08 6:47 UTC (permalink / raw)
To: Sourav Poddar; +Cc: thierry.reding, linux-pwm, linux-omap, balbi
On Wednesday 18 December 2013 05:06 PM, Sourav Poddar wrote:
> This patch series caters to the issue faced while using tiecap
> as a module.
>
> The patch fix lock dependency issue which leads to crash during rmmod.
> Also fixes the clock control register setup values during CLK EN call.
>
> Sourav Poddar (3):
> pwm: core: Rearrange pwm lock usage.
> driver: pwm: ti-ecap: Rmove duplicate put_sync call.
> driver: pwmss: Disable stop during Enable clock call..
>
> drivers/pwm/core.c | 4 +++-
> drivers/pwm/pwm-tiecap.c | 1 -
> drivers/pwm/pwm-tipwmss.c | 2 ++
> 3 files changed, 5 insertions(+), 2 deletions(-)
>
Gentle ping on this..
^ permalink raw reply [flat|nested] 8+ messages in thread