RE: Linux-omap PM: Fix dead lock condition in resource_release(vdd1_opp)
Hi Mike, Actually chunqiu and I still have this problem, if you can push out the patch, that will be good. Thanks, Limei -Original Message- From: Mike Chan [mailto:m...@android.com] Sent: Thursday, September 03, 2009 12:45 PM To: Kevin Hilman Cc: Wang Limei-E12499; linux-omap@vger.kernel.org; Wang Sawsd-A24013 Subject: Re: Linux-omap PM: Fix dead lock condition in resource_release(vdd1_opp) On Thu, Sep 3, 2009 at 7:01 AM, Kevin Hilmankhil...@deeprootsystems.com wrote: Mike Chan m...@android.com writes: On Tue, Aug 18, 2009 at 8:04 AM, Kevin Hilmankhil...@deeprootsystems.com wrote: Wang Limei-E12499 e12...@motorola.com writes: Seems like I did not submit the patch in the right way, before I setup my mail correctly, attach the patches in the mail. You're patches are still line-wrapped. I strongly recommend using git-format-patch and git-send-email to submit patches. Chunqiu was able to do this. Please consult him. Also, no need to CC linux-omap-owner. linux-omap is all that is needed. This patch has been reviewed and merged into our android-omap-2.6.29 tree http://android.git.kernel.org/?p=kernel/omap.git;a=commit;h=0b6a9b651 4c7ccfa0c76e4defdaea3dcbc617633 Hmm, I don't see any other review/signoff that the authors on that link. Apologies, I was not aware of the reviewed-by policy but will keep that in mind for future patches we pull into our branch. In general any patches that have been applied to the android-omap-2.6.29 tree have gone under some review/testing. Kevin if you're having line wrap problems feel free to pull it from here, assuming everyone's feedback has been addressed It's not me who has the line-wrap problem. I could unwrap pretty easily myself, but it gets very old working around various email client problems, so I choose to reject patches until they can be sent in a usable form. I'm still waiting for this so it can get a full review on-list. Very understandable, if Chunqiu is still having problems I can push one out to l-o for review on behalf of Chinqiu. Its in our best interest to get this into mainline so we have less 1-off's to maintain. --Mike Thanks, Kevin Thanks, Kevin PATCH1:0001-Add-per-resource-mutex-for-OMAP-resource-framework.patc h From b4e9cc01f9d1aaeec39cc1ee794e5efaec61c781 Mon Sep 17 00:00:00 2001 From: Chunqiu Wang cqw...@motorola.com Date: Fri, 14 Aug 2009 17:34:32 +0800 Subject: [PATCH] Add per-resource mutex for OMAP resource framework Current OMAP resource fwk uses a global res_mutex for resource_request and resource_release calls for all the available resources.It may cause dead lock if resource_request/resource_release is called recursively. For current OMAP3 VDD1/VDD2 resource, the change_level implementation is mach-omap2/resource34xx.c/set_opp(), when using resource_release to remove vdd1 constraint, this function may call resource_release again to release Vdd2 constrait if target vdd1 level is less than OPP3. in this scenario, the global res_mutex down operation will be called again, this will cause the second down operation hang there. To fix the problem, per-resource mutex is added to avoid hangup when resource_request/resource_release is called recursively. Signed-off-by: Chunqiu Wang cqw...@motorola.com --- arch/arm/plat-omap/include/mach/resource.h | 2 ++ arch/arm/plat-omap/resource.c | 27 +++ 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/arch/arm/plat-omap/include/mach/resource.h b/arch/arm/plat-omap/include/mach/resource.h index f91d8ce..d482fb8 100644 --- a/arch/arm/plat-omap/include/mach/resource.h +++ b/arch/arm/plat-omap/include/mach/resource.h @@ -46,6 +46,8 @@ struct shared_resource { /* Shared resource operations */ struct shared_resource_ops *ops; struct list_head node; + /* Protect each resource */ + struct mutex resource_mutex; }; struct shared_resource_ops { diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c index ec31727..5eae4e8 100644 --- a/arch/arm/plat-omap/resource.c +++ b/arch/arm/plat-omap/resource.c @@ -264,6 +264,7 @@ int resource_register(struct shared_resource *resp) return -EEXIST; INIT_LIST_HEAD(resp-users_list); + mutex_init(resp-resource_mutex); down(res_mutex); /* Add the resource to the resource list */ @@ -326,14 +327,14 @@ int resource_request(const char *name, struct device *dev, struct users_list *user; int found = 0, ret = 0; - down(res_mutex); - resp = _resource_lookup(name); + resp = resource_lookup(name); if (!resp) { printk(KERN_ERR resource_request: Invalid resource name\n); ret = -EINVAL; - goto res_unlock; + goto ret; } + mutex_lock(resp-resource_mutex); /* Call the resource specific
RE: Linux-omap PM: Fix dead lock condition in resource_release(vdd1_opp)
); return ret; } EXPORT_SYMBOL(resource_get_level); -- 1.5.4.3 PATCH2:0002-Move-the-resource-level-update-into-mutex_lock-block.patch From 9cc371b5d7f2e049fe72bc946dcb8ec8e1de826c Mon Sep 17 00:00:00 2001 From: Chunqiu Wang cqw...@motorola.com Date: Fri, 14 Aug 2009 17:43:13 +0800 Subject: [PATCH] Move the resource level update into mutex_lock block. The update_resource_level is called outside of the mutex lock protection block due to an out of date spin lock mechanism, now mutex is used, so move the update_resource_level into mutex protection block. Signed-off-by: Chunqiu Wang cqw...@motorola.com --- arch/arm/plat-omap/resource.c | 11 +++ 1 files changed, 3 insertions(+), 8 deletions(-) diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c index 5eae4e8..e2a003a 100644 --- a/arch/arm/plat-omap/resource.c +++ b/arch/arm/plat-omap/resource.c @@ -362,16 +362,11 @@ int resource_request(const char *name, struct device *dev, } user-level = level; + /* Recompute and set the current level for the resource */ + ret = update_resource_level(resp); + res_unlock: mutex_unlock(resp-resource_mutex); - /* -* Recompute and set the current level for the resource. -* NOTE: update_resource level moved out of spin_lock, as it may call -* pm_qos_add_requirement, which does a kzmalloc. This won't be allowed -* in iterrupt context. The spin_lock still protects add/remove users. -*/ - if (!ret) - ret = update_resource_level(resp); ret: return ret; } -- 1.5.4.3 -Original Message- From: Wang Limei-E12499 Sent: Monday, August 17, 2009 11:13 AM To: 'khil...@deeprootsystems.com' Cc: Wang Limei-E12499; Wang Sawsd-A24013 Subject: RE: Linux-omap PM: Fix dead lock condition in resource_release(vdd1_opp) Kevin, Seems like I did not submit the patch in the recommended way,I will try to be better in the future. If you can review the patch and feedback, I will apperciate it. Thanks, Limei -Original Message- From: Wang Limei-E12499 Sent: Friday, August 14, 2009 5:44 PM To: Kevin Hilman Cc: Romit Dasgupta; linux-omap@vger.kernel.org; Wang Sawsd-A24013; Wang Limei-E12499 Subject: RE: Linux-omap PM: Fix dead lock condition in resource_release(vdd1_opp) Kevin, Thanks for reviewing the patch. Chunqiu and I revised the patch. Pls see the attachment. Thanks, Limei,chunqiu -Original Message- From: Kevin Hilman [mailto:khil...@deeprootsystems.com] Sent: Thursday, August 13, 2009 8:02 AM To: Wang Limei-E12499 Cc: Romit Dasgupta; linux-omap@vger.kernel.org; Wang Sawsd-A24013 Subject: Re: Linux-omap PM: Fix dead lock condition in resource_release(vdd1_opp) Wang Limei-E12499 e12...@motorola.com writes: Kevin and Romit, I agreed with you, thanks Kevin and Romit for the comments! Chunqiu Wang coded resource-based mutex, below is the patch. Pls review and let us know your feedback. From 31f87ffb8eb1f854a9adb7fd96011d490f4655fa Mon Sep 17 00:00:00 2001 From: Chunqiu Wang cqw...@motorola.com Date: Wed, 12 Aug 2009 16:22:09 +0800 Subject: [PATCH] Fix resource framework mutex lock issue when resource_get or resource_release are called nestedly. Could use a shorter summary (subject) and a more detailed changelog. This patch is doing too many things in a single patch without enough explanation. Not only does it convert the global semaphore to a resource-specific semaphore, but it also changing the locking slightly by moving some things in/out of lock protection. That should be described in the changelog as well. Even better would be a first patch that simply converts the semaphore to a resource-specific *mutex* (not resource-specific semaphore.) IOW, use mutex API in linux/mutex.h: struct mutex; init_mutex() mutex_lock() mutex_unlock() mutex_is_lockec() ... Then, add a 2nd patch which does any reworking of the critical sections. Kevin Signed-off-by: Chunqiu Wang cqw...@motorola.com --- arch/arm/plat-omap/include/mach/resource.h |2 + arch/arm/plat-omap/resource.c | 38 +-- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/arch/arm/plat-omap/include/mach/resource.h b/arch/arm/plat-omap/include/mach/resource.h index f91d8ce..389cb67 100644 --- a/arch/arm/plat-omap/include/mach/resource.h +++ b/arch/arm/plat-omap/include/mach/resource.h @@ -22,6 +22,7 @@ #include linux/list.h #include linux/mutex.h +#include linux/semaphore.h #include linux/device.h #include mach/cpu.h @@ -46,6 +47,7 @@ struct shared_resource { /* Shared resource operations */ struct shared_resource_ops *ops; struct list_head node; + struct semaphore resource_mutex; }; struct shared_resource_ops { diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c index ec31727..758a138 100644 --- a/arch/arm/plat-omap
RE: Linux-omap PM: Fix dead lock condition in resource_release(vdd1_opp)
Kevin, Thanks for reviewing the patch. Chunqiu and I revised the patch. Pls see the attachment. Thanks, Limei,chunqiu -Original Message- From: Kevin Hilman [mailto:khil...@deeprootsystems.com] Sent: Thursday, August 13, 2009 8:02 AM To: Wang Limei-E12499 Cc: Romit Dasgupta; linux-omap@vger.kernel.org; Wang Sawsd-A24013 Subject: Re: Linux-omap PM: Fix dead lock condition in resource_release(vdd1_opp) Wang Limei-E12499 e12...@motorola.com writes: Kevin and Romit, I agreed with you, thanks Kevin and Romit for the comments! Chunqiu Wang coded resource-based mutex, below is the patch. Pls review and let us know your feedback. From 31f87ffb8eb1f854a9adb7fd96011d490f4655fa Mon Sep 17 00:00:00 2001 From: Chunqiu Wang cqw...@motorola.com Date: Wed, 12 Aug 2009 16:22:09 +0800 Subject: [PATCH] Fix resource framework mutex lock issue when resource_get or resource_release are called nestedly. Could use a shorter summary (subject) and a more detailed changelog. This patch is doing too many things in a single patch without enough explanation. Not only does it convert the global semaphore to a resource-specific semaphore, but it also changing the locking slightly by moving some things in/out of lock protection. That should be described in the changelog as well. Even better would be a first patch that simply converts the semaphore to a resource-specific *mutex* (not resource-specific semaphore.) IOW, use mutex API in linux/mutex.h: struct mutex; init_mutex() mutex_lock() mutex_unlock() mutex_is_lockec() ... Then, add a 2nd patch which does any reworking of the critical sections. Kevin Signed-off-by: Chunqiu Wang cqw...@motorola.com --- arch/arm/plat-omap/include/mach/resource.h |2 + arch/arm/plat-omap/resource.c | 38 +-- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/arch/arm/plat-omap/include/mach/resource.h b/arch/arm/plat-omap/include/mach/resource.h index f91d8ce..389cb67 100644 --- a/arch/arm/plat-omap/include/mach/resource.h +++ b/arch/arm/plat-omap/include/mach/resource.h @@ -22,6 +22,7 @@ #include linux/list.h #include linux/mutex.h +#include linux/semaphore.h #include linux/device.h #include mach/cpu.h @@ -46,6 +47,7 @@ struct shared_resource { /* Shared resource operations */ struct shared_resource_ops *ops; struct list_head node; + struct semaphore resource_mutex; }; struct shared_resource_ops { diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c index ec31727..758a138 100644 --- a/arch/arm/plat-omap/resource.c +++ b/arch/arm/plat-omap/resource.c @@ -264,6 +264,7 @@ int resource_register(struct shared_resource *resp) return -EEXIST; INIT_LIST_HEAD(resp-users_list); + sema_init(resp-resource_mutex, 1); down(res_mutex); /* Add the resource to the resource list */ @@ -326,14 +327,14 @@ int resource_request(const char *name, struct device *dev, struct users_list *user; int found = 0, ret = 0; - down(res_mutex); - resp = _resource_lookup(name); + resp = resource_lookup(name); if (!resp) { printk(KERN_ERR resource_request: Invalid resource name\n); ret = -EINVAL; - goto res_unlock; + goto ret; } + down(resp-resource_mutex); /* Call the resource specific validate function */ if (resp-ops-validate_level) { ret = resp-ops-validate_level(resp, level); @@ -361,16 +362,12 @@ int resource_request(const char *name, struct device *dev, } user-level = level; + /* Recompute and set the current level for the resource */ + ret = update_resource_level(resp); res_unlock: - up(res_mutex); - /* - * Recompute and set the current level for the resource. - * NOTE: update_resource level moved out of spin_lock, as it may call - * pm_qos_add_requirement, which does a kzmalloc. This won't be allowed - * in iterrupt context. The spin_lock still protects add/remove users. - */ - if (!ret) - ret = update_resource_level(resp); + up(resp-resource_mutex); + +ret: return ret; } EXPORT_SYMBOL(resource_request); @@ -393,14 +390,14 @@ int resource_release(const char *name, struct device *dev) struct users_list *user; int found = 0, ret = 0; - down(res_mutex); - resp = _resource_lookup(name); + resp = resource_lookup(name); if (!resp) { printk(KERN_ERR resource_release: Invalid resource name\n); ret = -EINVAL; - goto res_unlock; + goto ret; } + down(resp-resource_mutex); list_for_each_entry(user, resp-users_list, node) { if (user-dev == dev) { found = 1; @@ -421,7 +418,9 @@ int
RE: Linux-omap PM: Fix dead lock condition in resource_release(vdd1_opp)
Kevin and Romit, I agreed with you, thanks Kevin and Romit for the comments! Chunqiu Wang coded resource-based mutex, below is the patch. Pls review and let us know your feedback. From 31f87ffb8eb1f854a9adb7fd96011d490f4655fa Mon Sep 17 00:00:00 2001 From: Chunqiu Wang cqw...@motorola.com Date: Wed, 12 Aug 2009 16:22:09 +0800 Subject: [PATCH] Fix resource framework mutex lock issue when resource_get or resource_release are called nestedly. Signed-off-by: Chunqiu Wang cqw...@motorola.com --- arch/arm/plat-omap/include/mach/resource.h |2 + arch/arm/plat-omap/resource.c | 38 +-- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/arch/arm/plat-omap/include/mach/resource.h b/arch/arm/plat-omap/include/mach/resource.h index f91d8ce..389cb67 100644 --- a/arch/arm/plat-omap/include/mach/resource.h +++ b/arch/arm/plat-omap/include/mach/resource.h @@ -22,6 +22,7 @@ #include linux/list.h #include linux/mutex.h +#include linux/semaphore.h #include linux/device.h #include mach/cpu.h @@ -46,6 +47,7 @@ struct shared_resource { /* Shared resource operations */ struct shared_resource_ops *ops; struct list_head node; + struct semaphore resource_mutex; }; struct shared_resource_ops { diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c index ec31727..758a138 100644 --- a/arch/arm/plat-omap/resource.c +++ b/arch/arm/plat-omap/resource.c @@ -264,6 +264,7 @@ int resource_register(struct shared_resource *resp) return -EEXIST; INIT_LIST_HEAD(resp-users_list); + sema_init(resp-resource_mutex, 1); down(res_mutex); /* Add the resource to the resource list */ @@ -326,14 +327,14 @@ int resource_request(const char *name, struct device *dev, struct users_list *user; int found = 0, ret = 0; - down(res_mutex); - resp = _resource_lookup(name); + resp = resource_lookup(name); if (!resp) { printk(KERN_ERR resource_request: Invalid resource name\n); ret = -EINVAL; - goto res_unlock; + goto ret; } + down(resp-resource_mutex); /* Call the resource specific validate function */ if (resp-ops-validate_level) { ret = resp-ops-validate_level(resp, level); @@ -361,16 +362,12 @@ int resource_request(const char *name, struct device *dev, } user-level = level; + /* Recompute and set the current level for the resource */ + ret = update_resource_level(resp); res_unlock: - up(res_mutex); - /* -* Recompute and set the current level for the resource. -* NOTE: update_resource level moved out of spin_lock, as it may call -* pm_qos_add_requirement, which does a kzmalloc. This won't be allowed -* in iterrupt context. The spin_lock still protects add/remove users. -*/ - if (!ret) - ret = update_resource_level(resp); + up(resp-resource_mutex); + +ret: return ret; } EXPORT_SYMBOL(resource_request); @@ -393,14 +390,14 @@ int resource_release(const char *name, struct device *dev) struct users_list *user; int found = 0, ret = 0; - down(res_mutex); - resp = _resource_lookup(name); + resp = resource_lookup(name); if (!resp) { printk(KERN_ERR resource_release: Invalid resource name\n); ret = -EINVAL; - goto res_unlock; + goto ret; } + down(resp-resource_mutex); list_for_each_entry(user, resp-users_list, node) { if (user-dev == dev) { found = 1; @@ -421,7 +418,9 @@ int resource_release(const char *name, struct device *dev) /* Recompute and set the current level for the resource */ ret = update_resource_level(resp); res_unlock: - up(res_mutex); + up(resp-resource_mutex); + +ret: return ret; } EXPORT_SYMBOL(resource_release); @@ -438,15 +437,14 @@ int resource_get_level(const char *name) struct shared_resource *resp; u32 ret; - down(res_mutex); - resp = _resource_lookup(name); + resp = resource_lookup(name); if (!resp) { printk(KERN_ERR resource_release: Invalid resource name\n); - up(res_mutex); return -EINVAL; } + down(resp-resource_mutex); ret = resp-curr_level; - up(res_mutex); + up(resp-resource_mutex); return ret; } EXPORT_SYMBOL(resource_get_level); -- 1.5.4.3 -Original Message- From: Kevin Hilman [mailto:khil...@deeprootsystems.com] Sent: Monday, August 10, 2009 11:23 AM To: Wang Limei-E12499 Cc: linux-omap@vger.kernel.org Subject: Re: Linux-omap PM: Fix dead lock condition in resource_release(vdd1_opp) Wang Limei-E12499 e12...@motorola.com writes: I am using
Linux-omap PM: Fix dead lock condition in resource_release(vdd1_opp)
Kevin, I am using linux-omap pm-2.6.29 http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-pm.git;a=s hortlog;h=pm-2.6.29 branch,found a dead lock condition in: arch/arm/plat-omap/resource.c-resource_release(). The dead lock happens when using resource_request(vdd1_opp,dev,...) and resource_release(vdd1_opp, dev) to set and release vdd1 opp constraint. The scenario is: plat-omap/resource.c/resource_release(vdd1_opp, dev)==resource.c/update_resource_level()=mach-omap2/resource34xx.c/se t_opp(). In set_opp(), if the target_level of vdd1 is less than OPP3,will release the constraint set on VDD2 by calling resource_release(), but it will never return because can not get the mutex which is holding by the previous caller. int resource_release(const char *name, struct device *dev) { ... down(res_mutex); /* Recompute and set the current level for the resource */ ret = update_resource_level(resp); res_unlock: up(res_mutex); return ret; } int set_opp(struct shared_resource *resp, u32 target_level) { . if (resp == vdd1_resp) { if (target_level 3) resource_release(vdd2_opp, vdd2_dev); } The patch to fix this issue is below, will you pls review it and let me know your feedback? From: Limei Wang e12...@motorola.com Date: Fri, 7 Aug 2009 11:40:35 -0500 Subject: [PATCH] OMAP PM: Fix dead lock bug in resourc_release(vdd1_opp). Signed-off-by: Limei Wang e12...@motorola.com --- arch/arm/plat-omap/resource.c |6 -- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c index ec31727..876fd12 100644 --- a/arch/arm/plat-omap/resource.c +++ b/arch/arm/plat-omap/resource.c @@ -418,10 +418,12 @@ int resource_release(const char *name, struct device *dev) list_del(user-node); free_user(user); - /* Recompute and set the current level for the resource */ - ret = update_resource_level(resp); res_unlock: up(res_mutex); + + /* Recompute and set the current level for the resource */ + if (!ret) + ret = update_resource_level(resp); return ret; } EXPORT_SYMBOL(resource_release); -- 1.5.6.3 Thanks, Limei -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Linux-omap PM: Fix dead lock condition in resource_release(vdd1_opp)
Re-sent to linux-omap-ow...@vger.kernel.org. -Original Message- From: Wang Limei-E12499 Sent: Friday, August 07, 2009 12:05 PM To: linux-omap@vger.kernel.org; Kevin Hilman Cc: Wang Limei-E12499 Subject: Linux-omap PM: Fix dead lock condition in resource_release(vdd1_opp) Kevin, I am using linux-omap pm-2.6.29 http://git.kernel.org/?p=linux/kernel/git/khilman/linux-omap-pm.git;a=s hortlog;h=pm-2.6.29 branch,found a dead lock condition in: arch/arm/plat-omap/resource.c-resource_release(). The dead lock happens when using resource_request(vdd1_opp,dev,...) and resource_release(vdd1_opp, dev) to set and release vdd1 opp constraint. The scenario is: plat-omap/resource.c/resource_release(vdd1_opp, dev)==resource.c/update_resource_level()=mach-omap2/resource34xx.c/se t_opp(). In set_opp(), if the target_level of vdd1 is less than OPP3,will release the constraint set on VDD2 by calling resource_release(), but it will never return because can not get the mutex which is holding by the previous caller. int resource_release(const char *name, struct device *dev) { ... down(res_mutex); /* Recompute and set the current level for the resource */ ret = update_resource_level(resp); res_unlock: up(res_mutex); return ret; } int set_opp(struct shared_resource *resp, u32 target_level) { . if (resp == vdd1_resp) { if (target_level 3) resource_release(vdd2_opp, vdd2_dev); } The patch to fix this issue is below, will you pls review it and let me know your feedback? From: Limei Wang e12...@motorola.com Date: Fri, 7 Aug 2009 11:40:35 -0500 Subject: [PATCH] OMAP PM: Fix dead lock bug in resourc_release(vdd1_opp). Signed-off-by: Limei Wang e12...@motorola.com --- arch/arm/plat-omap/resource.c |6 -- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/arch/arm/plat-omap/resource.c b/arch/arm/plat-omap/resource.c index ec31727..876fd12 100644 --- a/arch/arm/plat-omap/resource.c +++ b/arch/arm/plat-omap/resource.c @@ -418,10 +418,12 @@ int resource_release(const char *name, struct device *dev) list_del(user-node); free_user(user); - /* Recompute and set the current level for the resource */ - ret = update_resource_level(resp); res_unlock: up(res_mutex); + + /* Recompute and set the current level for the resource */ + if (!ret) + ret = update_resource_level(resp); return ret; } EXPORT_SYMBOL(resource_release); -- 1.5.6.3 Thanks, Limei -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] OMAP3: PM: Fix freeze when scaling CORE dpll to 83Mhz
Paul, Thank you very much for copying me! During the investigation, Richard and Girish from TI helped, the mail is attached. Rajendra, Yesterday night, I sent mail to paul and kevin, I also submmited below patch along with detailed issue report and analysis. -#define FIXEDDELAY_MASK (0xff FIXEDDELAY_SHIFT) +#define FIXEDDELAY_MASK 0x00FF Thanks, limei -Original Message- From: Paul Walmsley [mailto:p...@pwsan.com] Sent: Thursday, July 09, 2009 3:44 PM To: Rajendra Nayak Cc: Wang Limei-E12499; khil...@deeprootsystems.com; linux-omap@vger.kernel.org Subject: Re: [PATCH] OMAP3: PM: Fix freeze when scaling CORE dpll to 83Mhz Hi Rajendra, On Thu, 9 Jul 2009, Rajendra Nayak wrote: This patch fixes a bug in the CORE dpll scaling sequence which was errouneously clearing some bits in the SDRC DLLA CTRL register and hence causing a freeze. The issue was observed only on platforms which scale CORE dpll to 83Mhz and hence program the DLL in fixed delay mode. Signed-off-by: Rajendra Nayak rna...@ti.com Thanks, looks good, I'll queue this up for a fixes series to rmk. One question: I got an E-mail on this earlier today from Limei Wang about this same issue. Looks like we should credit Limei also in this patch with finding the bug. Is this acceptable to you? - Paul --- arch/arm/mach-omap2/sram34xx.S |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/arch/arm/mach-omap2/sram34xx.S b/arch/arm/mach-omap2/sram34xx.S index 481f912..9be09a7 100644 --- a/arch/arm/mach-omap2/sram34xx.S +++ b/arch/arm/mach-omap2/sram34xx.S @@ -113,7 +113,7 @@ return_to_sdram: unlock_dll: ldr r11, omap3_sdrc_dlla_ctrl ldr r12, [r11] - and r12, r12, #FIXEDDELAY_MASK + bic r12, r12, #FIXEDDELAY_MASK orr r12, r12, #FIXEDDELAY_DEFAULT orr r12, r12, #DLLIDLE_MASK str r12, [r11] @ (no OCP barrier needed) -- 1.5.4.7 -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ---BeginMessage--- Good finding. I’m not sure what motivated Paul to make this change. Perhaps we should inquire on list. He is an open source contractor who Nokia has under contract to make code ‘ready’ for open source. I suspect he felt it was an optimization. I am aware he filed some question to TI about the fixed delay value but not about the enable/disable sequence. It has been my experience that he is quick to make changes with code purity in mind. He does gladly take back bug fixes for those changes. There are many cases that the TI code has arrived at a working sequence which is re-coded in open source ‘clean ups’. Generally the code cleanness does improve but functional regressions are not uncommon. This is part of open source maturity. I’m not sure of answer to sequencing issue you raise. Some research is necessary to find the answer. Maybe Girish or Rajendra know off hand. Regards, Richard W. From: Wang Limei-E12499 [mailto:e12...@motorola.com] Sent: Wednesday, July 08, 2009 3:26 PM To: Wang Limei-E12499; Woodruff, Richard; Hunter, Jon; Ghongdemath, Girish; Nayak, Rajendra; mikec...@google.com Cc: Sripathi Srinivas-A14759; Wang Sawsd-A24013; Falempe Jocelyn-XHP836; De Chanterac Cyril-cdlc01; WIDZER NOAH-KFQG76; Liu Haiyang-DGRW68 Subject: RE: Mem freeze at omap3_sram_configure_core_dpll in K29 open source PM Just talked with Noah, he confirmed DLL should be enabled even when DLL is in unlock mode. DLL is disabled in unlock_dll is the reason of mem corruption. From the snapshot captured yesterday, SDRC_DLLA_CTRL is changed from 0x0F08 to 0x0F04 after omap3_configure_core_dpll is executed, means DLLLOCK is changed from lock mode to unlock mode,this is expected,but ENADLL bit is cleared, so DLL is disabled. SDRC_DLLA_CTRL is updated when running unlock_dll code, see 1 and 2 snapshot attached, it is masked when setting default FixedDelay. If enable DLL manually in LB, code can be executed correctly till hitting this function again. Since FixedDelay is already set to 0x0F before running these piece of code, pls see snapshot, if skipping these two instructions in RED, vdd2 dvfs works well. BTW, in K27 kernel sram-fn_34xx.S, does not include these. unlock_dll: ldr r11, omap3_sdrc_dlla_ctrl ldr r12, [r11] - and r12, r12, #FIXEDDELAY_MASK - orr r12, r12, #FIXEDDELAY_DEFAULT orr r12, r12, #DLLIDLE_MASK str r12, [r11] @ (no OCP barrier needed) bx lr This change is made