* [PATCH] PM-WIP-OPP: Fixing wrong target level being passed during Core DVFS. @ 2010-02-18 8:31 Thara Gopinath 2010-02-18 14:58 ` Menon, Nishanth 0 siblings, 1 reply; 5+ messages in thread From: Thara Gopinath @ 2010-02-18 8:31 UTC (permalink / raw) To: linux-omap; +Cc: Thara Gopinath, Kevin Hilman As per the current implementaion (u8*)&target_level is being passed to freq_to_opp in set_opp. This would result in updating just the first 8 bits of a u32 variable. Later target_level is passed to resource_set_opp_level as a u32 parameter. This maens a. Initially target_level was 0xabcdefgh. b. freq_to_opp updates the lower eight bits of target_level to 0xXX. Now target_level = 0xabcdefXX. c. We pass 0xabcdefXX as target_level to resource_set_opp_level when we want to pass just 0xXX. This is leading to some corrupted bookkeeping later on in the dvfs path. This patch ensures that target_level passed to resource_set_opp_level is actually the level that is intended by freq_to_opp API. Signed-off-by: Thara Gopinath <thara@ti.com> Cc: Kevin Hilman <khilman@deeprootsystems.com> --- arch/arm/mach-omap2/resource34xx.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-) diff --git a/arch/arm/mach-omap2/resource34xx.c b/arch/arm/mach-omap2/resource34xx.c index 3604a38..d2336d8 100644 --- a/arch/arm/mach-omap2/resource34xx.c +++ b/arch/arm/mach-omap2/resource34xx.c @@ -463,6 +463,7 @@ int set_opp(struct shared_resource *resp, u32 target_level) } else if (resp == vdd2_resp) { unsigned long req_l3_freq; struct omap_opp *oppx = NULL; + u8 opp; /* Convert the tput in KiB/s to Bus frequency in MHz */ req_l3_freq = (target_level * 1000)/4; @@ -478,10 +479,11 @@ int set_opp(struct shared_resource *resp, u32 target_level) /* uh uh.. no OPPs?? */ BUG_ON(IS_ERR(oppx)); - ret = freq_to_opp((u8 *)&target_level, OPP_L3, req_l3_freq); + ret = freq_to_opp(&opp, OPP_L3, req_l3_freq); /* we dont expect this to fail */ BUG_ON(ret); + target_level = opp; ret = resource_set_opp_level(VDD2_OPP, target_level, 0); } return 0; -- 1.5.4.7 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] PM-WIP-OPP: Fixing wrong target level being passed during Core DVFS. 2010-02-18 8:31 [PATCH] PM-WIP-OPP: Fixing wrong target level being passed during Core DVFS Thara Gopinath @ 2010-02-18 14:58 ` Menon, Nishanth 2010-02-23 5:28 ` Aguirre, Sergio 0 siblings, 1 reply; 5+ messages in thread From: Menon, Nishanth @ 2010-02-18 14:58 UTC (permalink / raw) To: Gopinath, Thara Cc: linux-omap@vger.kernel.org, Kevin Hilman, Kristo Tero (Nokia-D/Tampere) Gopinath, Thara said the following on 02/18/2010 10:31 AM: > As per the current implementaion (u8*)&target_level is being passed > to freq_to_opp in set_opp. This would result in updating just the first > 8 bits of a u32 variable. Later target_level is passed to > resource_set_opp_level as a u32 parameter. This maens > a. Initially target_level was 0xabcdefgh. > b. freq_to_opp updates the lower eight bits of target_level > to 0xXX. Now target_level = 0xabcdefXX. > c. We pass 0xabcdefXX as target_level to resource_set_opp_level > when we want to pass just 0xXX. > This is leading to some corrupted bookkeeping later on in the > dvfs path. > > This patch ensures that target_level passed to resource_set_opp_level > is actually the level that is intended by freq_to_opp API. Thanks.. good catch. [PATCH] PM-WIP-OPP: Fixing wrong target level being passed during Core DVFS. Suggestion on the subject: could you put it something like the following so that git-am will throw away PM-WIP-OPP? [PATCH] [PM-WIP-OPP] omap3: pm: Fix wrong target level during core dvfs ? Some suggestion below. > > Signed-off-by: Thara Gopinath <thara@ti.com> > Cc: Kevin Hilman <khilman@deeprootsystems.com> > --- > arch/arm/mach-omap2/resource34xx.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/arch/arm/mach-omap2/resource34xx.c b/arch/arm/mach-omap2/resource34xx.c > index 3604a38..d2336d8 100644 > --- a/arch/arm/mach-omap2/resource34xx.c > +++ b/arch/arm/mach-omap2/resource34xx.c > @@ -463,6 +463,7 @@ int set_opp(struct shared_resource *resp, u32 target_level) > } else if (resp == vdd2_resp) { > unsigned long req_l3_freq; > struct omap_opp *oppx = NULL; > + u8 opp; > > /* Convert the tput in KiB/s to Bus frequency in MHz */ > req_l3_freq = (target_level * 1000)/4; > @@ -478,10 +479,11 @@ int set_opp(struct shared_resource *resp, u32 target_level) > /* uh uh.. no OPPs?? */ > BUG_ON(IS_ERR(oppx)); > If you do target_level = 0; here, the entire patch is a oneliner :) > - ret = freq_to_opp((u8 *)&target_level, OPP_L3, req_l3_freq); > + ret = freq_to_opp(&opp, OPP_L3, req_l3_freq); > /* we dont expect this to fail */ > BUG_ON(ret); > > + target_level = opp; > ret = resource_set_opp_level(VDD2_OPP, target_level, 0); > } > return 0; -- Regards, Nishanth Menon ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] PM-WIP-OPP: Fixing wrong target level being passed during Core DVFS. 2010-02-18 14:58 ` Menon, Nishanth @ 2010-02-23 5:28 ` Aguirre, Sergio 2010-02-23 5:31 ` Menon, Nishanth 0 siblings, 1 reply; 5+ messages in thread From: Aguirre, Sergio @ 2010-02-23 5:28 UTC (permalink / raw) To: Menon, Nishanth, Gopinath, Thara Cc: linux-omap@vger.kernel.org, Kevin Hilman, Kristo Tero (Nokia-D/Tampere) Nishanth, > -----Original Message----- > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap- > owner@vger.kernel.org] On Behalf Of Menon, Nishanth > Sent: Thursday, February 18, 2010 8:58 AM > To: Gopinath, Thara > Cc: linux-omap@vger.kernel.org; Kevin Hilman; Kristo Tero (Nokia- > D/Tampere) > Subject: Re: [PATCH] PM-WIP-OPP: Fixing wrong target level being passed > during Core DVFS. > > Gopinath, Thara said the following on 02/18/2010 10:31 AM: > > As per the current implementaion (u8*)&target_level is being passed > > to freq_to_opp in set_opp. This would result in updating just the first > > 8 bits of a u32 variable. Later target_level is passed to > > resource_set_opp_level as a u32 parameter. This maens > > a. Initially target_level was 0xabcdefgh. > > b. freq_to_opp updates the lower eight bits of target_level > > to 0xXX. Now target_level = 0xabcdefXX. > > c. We pass 0xabcdefXX as target_level to resource_set_opp_level > > when we want to pass just 0xXX. > > This is leading to some corrupted bookkeeping later on in the > > dvfs path. > > > > This patch ensures that target_level passed to resource_set_opp_level > > is actually the level that is intended by freq_to_opp API. > > Thanks.. good catch. > [PATCH] PM-WIP-OPP: Fixing wrong target level being passed during Core > DVFS. > Suggestion on the subject: > could you put it something like the following so that git-am will > throw away PM-WIP-OPP? > [PATCH] [PM-WIP-OPP] omap3: pm: Fix wrong target level during core dvfs > > ? > Some suggestion below. > > > > > Signed-off-by: Thara Gopinath <thara@ti.com> > > Cc: Kevin Hilman <khilman@deeprootsystems.com> > > --- > > arch/arm/mach-omap2/resource34xx.c | 4 +++- > > 1 files changed, 3 insertions(+), 1 deletions(-) > > > > diff --git a/arch/arm/mach-omap2/resource34xx.c b/arch/arm/mach- > omap2/resource34xx.c > > index 3604a38..d2336d8 100644 > > --- a/arch/arm/mach-omap2/resource34xx.c > > +++ b/arch/arm/mach-omap2/resource34xx.c > > @@ -463,6 +463,7 @@ int set_opp(struct shared_resource *resp, u32 > target_level) > > } else if (resp == vdd2_resp) { > > unsigned long req_l3_freq; > > struct omap_opp *oppx = NULL; > > + u8 opp; > > > > /* Convert the tput in KiB/s to Bus frequency in MHz */ > > req_l3_freq = (target_level * 1000)/4; > > @@ -478,10 +479,11 @@ int set_opp(struct shared_resource *resp, u32 > target_level) > > /* uh uh.. no OPPs?? */ > > BUG_ON(IS_ERR(oppx)); > > > If you do target_level = 0; here, the entire patch is a oneliner :) Actually, IMHO will be even more clean, to standardize all OPP value passing to be u8. Do you really need to be prepared for 2^32 opp values? ;) Regards, Sergio > > > - ret = freq_to_opp((u8 *)&target_level, OPP_L3, req_l3_freq); > > + ret = freq_to_opp(&opp, OPP_L3, req_l3_freq); > > /* we dont expect this to fail */ > > BUG_ON(ret); > > > > + target_level = opp; > > ret = resource_set_opp_level(VDD2_OPP, target_level, 0); > > } > > return 0; > > -- > Regards, > Nishanth Menon > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] PM-WIP-OPP: Fixing wrong target level being passed during Core DVFS. 2010-02-23 5:28 ` Aguirre, Sergio @ 2010-02-23 5:31 ` Menon, Nishanth 2010-02-23 12:42 ` Aguirre, Sergio 0 siblings, 1 reply; 5+ messages in thread From: Menon, Nishanth @ 2010-02-23 5:31 UTC (permalink / raw) To: Aguirre, Sergio, Gopinath, Thara Cc: linux-omap@vger.kernel.org, Kevin Hilman, Kristo Tero (Nokia-D/Tampere) > From: Aguirre, Sergio > Sent: Tuesday, February 23, 2010 7:28 AM > To: Menon, Nishanth; Gopinath, Thara [...] > > > > > > > > Signed-off-by: Thara Gopinath <thara@ti.com> > > > Cc: Kevin Hilman <khilman@deeprootsystems.com> > > > --- > > > arch/arm/mach-omap2/resource34xx.c | 4 +++- > > > 1 files changed, 3 insertions(+), 1 deletions(-) > > > > > > diff --git a/arch/arm/mach-omap2/resource34xx.c b/arch/arm/mach- > > omap2/resource34xx.c > > > index 3604a38..d2336d8 100644 > > > --- a/arch/arm/mach-omap2/resource34xx.c > > > +++ b/arch/arm/mach-omap2/resource34xx.c > > > @@ -463,6 +463,7 @@ int set_opp(struct shared_resource *resp, u32 > > target_level) > > > } else if (resp == vdd2_resp) { > > > unsigned long req_l3_freq; > > > struct omap_opp *oppx = NULL; > > > + u8 opp; > > > > > > /* Convert the tput in KiB/s to Bus frequency in MHz */ > > > req_l3_freq = (target_level * 1000)/4; > > > @@ -478,10 +479,11 @@ int set_opp(struct shared_resource *resp, u32 > > target_level) > > > /* uh uh.. no OPPs?? */ > > > BUG_ON(IS_ERR(oppx)); > > > > > If you do target_level = 0; here, the entire patch is a oneliner :) > > Actually, IMHO will be even more clean, to standardize all OPP value > passing to be u8. > > Do you really need to be prepared for 2^32 opp values? ;) > Using OPP ID has to be completely removed from resource34xx.c -> this action is still pending. In this case, using u8 OR initing the target_level to 0 has the same impact. Why add code that will be removed later on anyways? Regards, Nishanth Menon ^ permalink raw reply [flat|nested] 5+ messages in thread
* RE: [PATCH] PM-WIP-OPP: Fixing wrong target level being passed during Core DVFS. 2010-02-23 5:31 ` Menon, Nishanth @ 2010-02-23 12:42 ` Aguirre, Sergio 0 siblings, 0 replies; 5+ messages in thread From: Aguirre, Sergio @ 2010-02-23 12:42 UTC (permalink / raw) To: Menon, Nishanth, Gopinath, Thara Cc: linux-omap@vger.kernel.org, Kevin Hilman, Kristo Tero (Nokia-D/Tampere) > -----Original Message----- > From: Menon, Nishanth > Sent: Monday, February 22, 2010 11:31 PM > To: Aguirre, Sergio; Gopinath, Thara > Cc: linux-omap@vger.kernel.org; Kevin Hilman; Kristo Tero (Nokia- > D/Tampere) > Subject: RE: [PATCH] PM-WIP-OPP: Fixing wrong target level being passed > during Core DVFS. > > > From: Aguirre, Sergio > > Sent: Tuesday, February 23, 2010 7:28 AM > > To: Menon, Nishanth; Gopinath, Thara > [...] > > > > > > > > > > > Signed-off-by: Thara Gopinath <thara@ti.com> > > > > Cc: Kevin Hilman <khilman@deeprootsystems.com> > > > > --- > > > > arch/arm/mach-omap2/resource34xx.c | 4 +++- > > > > 1 files changed, 3 insertions(+), 1 deletions(-) > > > > > > > > diff --git a/arch/arm/mach-omap2/resource34xx.c b/arch/arm/mach- > > > omap2/resource34xx.c > > > > index 3604a38..d2336d8 100644 > > > > --- a/arch/arm/mach-omap2/resource34xx.c > > > > +++ b/arch/arm/mach-omap2/resource34xx.c > > > > @@ -463,6 +463,7 @@ int set_opp(struct shared_resource *resp, u32 > > > target_level) > > > > } else if (resp == vdd2_resp) { > > > > unsigned long req_l3_freq; > > > > struct omap_opp *oppx = NULL; > > > > + u8 opp; > > > > > > > > /* Convert the tput in KiB/s to Bus frequency in MHz */ > > > > req_l3_freq = (target_level * 1000)/4; > > > > @@ -478,10 +479,11 @@ int set_opp(struct shared_resource *resp, u32 > > > target_level) > > > > /* uh uh.. no OPPs?? */ > > > > BUG_ON(IS_ERR(oppx)); > > > > > > > If you do target_level = 0; here, the entire patch is a oneliner :) > > > > Actually, IMHO will be even more clean, to standardize all OPP value > > passing to be u8. > > > > Do you really need to be prepared for 2^32 opp values? ;) > > > Using OPP ID has to be completely removed from resource34xx.c -> this > action is still pending. In this case, using u8 OR initing the > target_level to 0 has the same impact. Why add code that will be removed > later on anyways? Well, I'm not proposing for code addition, but to fix that code just by changing opp level parameters to u8, instead of u32, like this: -int set_opp(struct shared_resource *resp, u32 target_level) +int set_opp(struct shared_resource *resp, u8 target_level) If you're going to replace all this in the near future, then it's understandable to hold even this patch (target_level = 0). Regards, Sergio > > Regards, > Nishanth Menon ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-02-23 12:42 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-02-18 8:31 [PATCH] PM-WIP-OPP: Fixing wrong target level being passed during Core DVFS Thara Gopinath 2010-02-18 14:58 ` Menon, Nishanth 2010-02-23 5:28 ` Aguirre, Sergio 2010-02-23 5:31 ` Menon, Nishanth 2010-02-23 12:42 ` Aguirre, Sergio
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox