[PM-OPP][PATCH] OMAP: Modifying the frequency comparison logic.

[PM-OPP][PATCH] OMAP: Modifying the frequency comparison logic.

[thara gopinath]

From: Thara Gopinath <thara@ti.com>

In the current opp layer the frequency matching API's
tries to match the exact frequency passed in Hz. This leads
to problems in cases where dplls are locked to slightly differnet
frequencies due to sys clock speed or optimum M,N values.
Consider the following scenario
	a. dpll_x is supposed to be locked at 266Mhz but is
	   actually att 266.045 Mhz due to above mentioned reasons.
	b. The opp table has the entry as 266000000 hz.
	c. The user does a clk_get_rate(dpll_x) and passes the
	  corresponding rate <rate> into the opp API's to get back
	  a opp table entry.
	d. In this scenario if  the user has called into
	   opp_find_freq_exact it will return an error.
	   If the user has called into opp_find_freq_ceil
	   it will either return the opp entry corresponding
	   to the next higher frequency in the opp table
	   or return an error if 266 Mhz is the last entry
	   in the table.

The above is not correct as we expect the framework to return back
the opp table entry corresponding to 266 Mhz.

Now there are two ways of fixing this issue.
	a. Put the correct dpll lock frequency in the opp table.
	   This means opp table will now have entries like 266045000 hz.
	   But this is not scalable as these dpll lock frequencies
	   can change slightly based on sys clk speeds. Like for eg
	   at sys clk speed 38.4 Mhz we will able to get 266.045 Mhz
	   where as at sys clk speed 26 Mhz we will be able to get
	   onyl 266.124 Mhz etc. So depending on the platform we
	   will have to start changing the opp table.

	b. Do the comparison in Mhz in the opp layer rather than in Hz.
	   This would mean we will divide the rate passed into the opp layer
	  API and the rates stored in the opp tables by 1000000 to get the
	  rates in Mhz and do the necessary comparision. In this approach any
	  vague frequency like 266.045Mhz will get mapped to 266 Mhz in the
	  opp table. But if the passed rate is 267 Mhz, the opp framework
	  will still rerturn an error or the next highest opp table entry

This patch implements solution b. The scenario mentioned above is
esp true for OMAP4 dpll_iva where we do end up with such weird frequencies
due to sys clk being at 38.4 Mhz.

Signed-off-by: Thara Gopinath <thara@ti.com>
---
 arch/arm/plat-omap/opp.c |   37 ++++++++++++++++++++++++++-----------
 1 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/arch/arm/plat-omap/opp.c b/arch/arm/plat-omap/opp.c
index b9b7bda..ba7a554 100644
--- a/arch/arm/plat-omap/opp.c
+++ b/arch/arm/plat-omap/opp.c
@@ -212,15 +212,20 @@ struct omap_opp *opp_find_freq_exact(struct device *dev,
 {
 	struct device_opp *dev_opp;
 	struct omap_opp *temp_opp, *opp = ERR_PTR(-ENODEV);
+	unsigned long req_freq = freq / 1000000;
 
 	dev_opp = find_device_opp(dev);
 	if (IS_ERR(dev_opp))
 		return opp;
 
 	list_for_each_entry(temp_opp, &dev_opp->opp_list, node) {
-		if (temp_opp->enabled && temp_opp->rate == freq) {
-			opp = temp_opp;
-			break;
+		if (temp_opp->enabled) {
+			unsigned long rate = temp_opp->rate / 1000000;
+
+			if (rate == req_freq) {
+				opp = temp_opp;
+				break;
+			}
 		}
 	}
 
@@ -258,16 +263,21 @@ struct omap_opp *opp_find_freq_ceil(struct device *dev, unsigned long *freq)
 {
 	struct device_opp *dev_opp;
 	struct omap_opp *temp_opp, *opp = ERR_PTR(-ENODEV);
+	unsigned long req_freq = *freq / 1000000;
 
 	dev_opp = find_device_opp(dev);
 	if (IS_ERR(dev_opp))
 		return opp;
 
 	list_for_each_entry(temp_opp, &dev_opp->opp_list, node) {
-		if (temp_opp->enabled && temp_opp->rate >= *freq) {
-			opp = temp_opp;
-			*freq = opp->rate;
-			break;
+		if (temp_opp->enabled) {
+			unsigned long rate = temp_opp->rate / 1000000;
+
+			if (rate >= req_freq) {
+				opp = temp_opp;
+				*freq = opp->rate;
+				break;
+			}
 		}
 	}
 
@@ -305,16 +315,21 @@ struct omap_opp *opp_find_freq_floor(struct device *dev, unsigned long *freq)
 {
 	struct device_opp *dev_opp;
 	struct omap_opp *temp_opp, *opp = ERR_PTR(-ENODEV);
+	unsigned long req_freq = *freq / 1000000;
 
 	dev_opp = find_device_opp(dev);
 	if (IS_ERR(dev_opp))
 		return opp;
 
 	list_for_each_entry_reverse(temp_opp, &dev_opp->opp_list, node) {
-		if (temp_opp->enabled && temp_opp->rate <= *freq) {
-			opp = temp_opp;
-			*freq = opp->rate;
-			break;
+		if (temp_opp->enabled) {
+			unsigned long rate = temp_opp->rate / 1000000;
+
+			if (rate <= req_freq) {
+				opp = temp_opp;
+				*freq = opp->rate;
+				break;
+			}
 		}
 	}
 
-- 
1.7.1.GIT

Re: [PM-OPP][PATCH] OMAP: Modifying the frequency comparison logic.

[Kevin Hilman]

thara gopinath <thara@ti.com> writes:

> From: Thara Gopinath <thara@ti.com>
>
> In the current opp layer the frequency matching API's
> tries to match the exact frequency passed in Hz. This leads
> to problems in cases where dplls are locked to slightly differnet
> frequencies due to sys clock speed or optimum M,N values.
> Consider the following scenario
> 	a. dpll_x is supposed to be locked at 266Mhz but is
> 	   actually att 266.045 Mhz due to above mentioned reasons.
> 	b. The opp table has the entry as 266000000 hz.
> 	c. The user does a clk_get_rate(dpll_x) and passes the
> 	  corresponding rate <rate> into the opp API's to get back
> 	  a opp table entry.
> 	d. In this scenario if  the user has called into
> 	   opp_find_freq_exact it will return an error.
> 	   If the user has called into opp_find_freq_ceil
> 	   it will either return the opp entry corresponding
> 	   to the next higher frequency in the opp table
> 	   or return an error if 266 Mhz is the last entry
> 	   in the table.
>
> The above is not correct as we expect the framework to return back
> the opp table entry corresponding to 266 Mhz.
>
> Now there are two ways of fixing this issue.
> 	a. Put the correct dpll lock frequency in the opp table.
> 	   This means opp table will now have entries like 266045000 hz.
> 	   But this is not scalable as these dpll lock frequencies
> 	   can change slightly based on sys clk speeds. Like for eg
> 	   at sys clk speed 38.4 Mhz we will able to get 266.045 Mhz
> 	   where as at sys clk speed 26 Mhz we will be able to get
> 	   onyl 266.124 Mhz etc. So depending on the platform we
> 	   will have to start changing the opp table.
>
> 	b. Do the comparison in Mhz in the opp layer rather than in Hz.
> 	   This would mean we will divide the rate passed into the opp layer
> 	  API and the rates stored in the opp tables by 1000000 to get the
> 	  rates in Mhz and do the necessary comparision. In this approach any
> 	  vague frequency like 266.045Mhz will get mapped to 266 Mhz in the
> 	  opp table. But if the passed rate is 267 Mhz, the opp framework
> 	  will still rerturn an error or the next highest opp table entry
>
> This patch implements solution b. The scenario mentioned above is
> esp true for OMAP4 dpll_iva where we do end up with such weird frequencies
> due to sys clk being at 38.4 Mhz.

I agree that solution b is better, although it makes the '_exact'
function a bit less exact. :/

solution b is fine with me, but the kerneldoc for these find functions
should be updated to describe the new matching technique.

Kevin

> Signed-off-by: Thara Gopinath <thara@ti.com>
> ---
>  arch/arm/plat-omap/opp.c |   37 ++++++++++++++++++++++++++-----------
>  1 files changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/arch/arm/plat-omap/opp.c b/arch/arm/plat-omap/opp.c
> index b9b7bda..ba7a554 100644
> --- a/arch/arm/plat-omap/opp.c
> +++ b/arch/arm/plat-omap/opp.c
> @@ -212,15 +212,20 @@ struct omap_opp *opp_find_freq_exact(struct device *dev,
>  {
>  	struct device_opp *dev_opp;
>  	struct omap_opp *temp_opp, *opp = ERR_PTR(-ENODEV);
> +	unsigned long req_freq = freq / 1000000;
>  
>  	dev_opp = find_device_opp(dev);
>  	if (IS_ERR(dev_opp))
>  		return opp;
>  
>  	list_for_each_entry(temp_opp, &dev_opp->opp_list, node) {
> -		if (temp_opp->enabled && temp_opp->rate == freq) {
> -			opp = temp_opp;
> -			break;
> +		if (temp_opp->enabled) {
> +			unsigned long rate = temp_opp->rate / 1000000;
> +
> +			if (rate == req_freq) {
> +				opp = temp_opp;
> +				break;
> +			}
>  		}
>  	}
>  
> @@ -258,16 +263,21 @@ struct omap_opp *opp_find_freq_ceil(struct device *dev, unsigned long *freq)
>  {
>  	struct device_opp *dev_opp;
>  	struct omap_opp *temp_opp, *opp = ERR_PTR(-ENODEV);
> +	unsigned long req_freq = *freq / 1000000;
>  
>  	dev_opp = find_device_opp(dev);
>  	if (IS_ERR(dev_opp))
>  		return opp;
>  
>  	list_for_each_entry(temp_opp, &dev_opp->opp_list, node) {
> -		if (temp_opp->enabled && temp_opp->rate >= *freq) {
> -			opp = temp_opp;
> -			*freq = opp->rate;
> -			break;
> +		if (temp_opp->enabled) {
> +			unsigned long rate = temp_opp->rate / 1000000;
> +
> +			if (rate >= req_freq) {
> +				opp = temp_opp;
> +				*freq = opp->rate;
> +				break;
> +			}
>  		}
>  	}
>  
> @@ -305,16 +315,21 @@ struct omap_opp *opp_find_freq_floor(struct device *dev, unsigned long *freq)
>  {
>  	struct device_opp *dev_opp;
>  	struct omap_opp *temp_opp, *opp = ERR_PTR(-ENODEV);
> +	unsigned long req_freq = *freq / 1000000;
>  
>  	dev_opp = find_device_opp(dev);
>  	if (IS_ERR(dev_opp))
>  		return opp;
>  
>  	list_for_each_entry_reverse(temp_opp, &dev_opp->opp_list, node) {
> -		if (temp_opp->enabled && temp_opp->rate <= *freq) {
> -			opp = temp_opp;
> -			*freq = opp->rate;
> -			break;
> +		if (temp_opp->enabled) {
> +			unsigned long rate = temp_opp->rate / 1000000;
> +
> +			if (rate <= req_freq) {
> +				opp = temp_opp;
> +				*freq = opp->rate;
> +				break;
> +			}
>  		}
>  	}

RE: [PM-OPP][PATCH] OMAP: Modifying the frequency comparison logic.

[Menon, Nishanth]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> Sent: Tuesday, August 24, 2010 2:04 PM


> 
> thara gopinath <thara@ti.com> writes:
> 
> > From: Thara Gopinath <thara@ti.com>
> >
> > In the current opp layer the frequency matching API's
> > tries to match the exact frequency passed in Hz. This leads
> > to problems in cases where dplls are locked to slightly differnet
> > frequencies due to sys clock speed or optimum M,N values.
> > Consider the following scenario
> > 	a. dpll_x is supposed to be locked at 266Mhz but is
> > 	   actually att 266.045 Mhz due to above mentioned reasons.
> > 	b. The opp table has the entry as 266000000 hz.
> > 	c. The user does a clk_get_rate(dpll_x) and passes the
> > 	  corresponding rate <rate> into the opp API's to get back
> > 	  a opp table entry.
> > 	d. In this scenario if  the user has called into
> > 	   opp_find_freq_exact it will return an error.
> > 	   If the user has called into opp_find_freq_ceil
> > 	   it will either return the opp entry corresponding
> > 	   to the next higher frequency in the opp table
> > 	   or return an error if 266 Mhz is the last entry
> > 	   in the table.
> >
> > The above is not correct as we expect the framework to return back
> > the opp table entry corresponding to 266 Mhz.
> >
> > Now there are two ways of fixing this issue.
> > 	a. Put the correct dpll lock frequency in the opp table.
> > 	   This means opp table will now have entries like 266045000 hz.
> > 	   But this is not scalable as these dpll lock frequencies
> > 	   can change slightly based on sys clk speeds. Like for eg
> > 	   at sys clk speed 38.4 Mhz we will able to get 266.045 Mhz
> > 	   where as at sys clk speed 26 Mhz we will be able to get
> > 	   onyl 266.124 Mhz etc. So depending on the platform we
> > 	   will have to start changing the opp table.
> >
> > 	b. Do the comparison in Mhz in the opp layer rather than in Hz.
> > 	   This would mean we will divide the rate passed into the opp layer
> > 	  API and the rates stored in the opp tables by 1000000 to get the
> > 	  rates in Mhz and do the necessary comparision. In this approach
> any
> > 	  vague frequency like 266.045Mhz will get mapped to 266 Mhz in the
> > 	  opp table. But if the passed rate is 267 Mhz, the opp framework
> > 	  will still rerturn an error or the next highest opp table entry
> >
> > This patch implements solution b. The scenario mentioned above is
> > esp true for OMAP4 dpll_iva where we do end up with such weird
> frequencies
> > due to sys clk being at 38.4 Mhz.
> 
> I agree that solution b is better, although it makes the '_exact'
> function a bit less exact. :/
> 
> solution b is fine with me, but the kerneldoc for these find functions
> should be updated to describe the new matching technique.

I agree, I suggest one improvement though - the search accuracy will vary
Based on the silicon rev, one size will probably not fit every silicon and
Domains we have - I suggest having accuracy as a parameter as part of domain
Registration/configurable parameter
e.g.
> > +			unsigned long rate = temp_opp->rate / 1000000;
Will probably configurable to the "exactness" we expect to handle per domain/silicon family.


> 
> Kevin
> 
> > Signed-off-by: Thara Gopinath <thara@ti.com>
> > ---
> >  arch/arm/plat-omap/opp.c |   37 ++++++++++++++++++++++++++-----------
> >  1 files changed, 26 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/arm/plat-omap/opp.c b/arch/arm/plat-omap/opp.c
> > index b9b7bda..ba7a554 100644
> > --- a/arch/arm/plat-omap/opp.c
> > +++ b/arch/arm/plat-omap/opp.c
> > @@ -212,15 +212,20 @@ struct omap_opp *opp_find_freq_exact(struct device
> *dev,
> >  {
> >  	struct device_opp *dev_opp;
> >  	struct omap_opp *temp_opp, *opp = ERR_PTR(-ENODEV);
> > +	unsigned long req_freq = freq / 1000000;
> >
> >  	dev_opp = find_device_opp(dev);
> >  	if (IS_ERR(dev_opp))
> >  		return opp;
> >
> >  	list_for_each_entry(temp_opp, &dev_opp->opp_list, node) {
> > -		if (temp_opp->enabled && temp_opp->rate == freq) {
> > -			opp = temp_opp;
> > -			break;
> > +		if (temp_opp->enabled) {
> > +			unsigned long rate = temp_opp->rate / 1000000;
> > +
> > +			if (rate == req_freq) {
> > +				opp = temp_opp;
> > +				break;
> > +			}
> >  		}
> >  	}
> >
> > @@ -258,16 +263,21 @@ struct omap_opp *opp_find_freq_ceil(struct device
> *dev, unsigned long *freq)
> >  {
> >  	struct device_opp *dev_opp;
> >  	struct omap_opp *temp_opp, *opp = ERR_PTR(-ENODEV);
> > +	unsigned long req_freq = *freq / 1000000;
> >
> >  	dev_opp = find_device_opp(dev);
> >  	if (IS_ERR(dev_opp))
> >  		return opp;
> >
> >  	list_for_each_entry(temp_opp, &dev_opp->opp_list, node) {
> > -		if (temp_opp->enabled && temp_opp->rate >= *freq) {
> > -			opp = temp_opp;
> > -			*freq = opp->rate;
> > -			break;
> > +		if (temp_opp->enabled) {
> > +			unsigned long rate = temp_opp->rate / 1000000;
> > +
> > +			if (rate >= req_freq) {
> > +				opp = temp_opp;
> > +				*freq = opp->rate;
> > +				break;
> > +			}
> >  		}
> >  	}
> >
> > @@ -305,16 +315,21 @@ struct omap_opp *opp_find_freq_floor(struct device
> *dev, unsigned long *freq)
> >  {
> >  	struct device_opp *dev_opp;
> >  	struct omap_opp *temp_opp, *opp = ERR_PTR(-ENODEV);
> > +	unsigned long req_freq = *freq / 1000000;
> >
> >  	dev_opp = find_device_opp(dev);
> >  	if (IS_ERR(dev_opp))
> >  		return opp;
> >
> >  	list_for_each_entry_reverse(temp_opp, &dev_opp->opp_list, node) {
> > -		if (temp_opp->enabled && temp_opp->rate <= *freq) {
> > -			opp = temp_opp;
> > -			*freq = opp->rate;
> > -			break;
> > +		if (temp_opp->enabled) {
> > +			unsigned long rate = temp_opp->rate / 1000000;
> > +
> > +			if (rate <= req_freq) {
> > +				opp = temp_opp;
> > +				*freq = opp->rate;
> > +				break;
> > +			}
> >  		}
> >  	}


Regards,
Nishanth Menon

Re: [PM-OPP][PATCH] OMAP: Modifying the frequency comparison logic.

[Kevin Hilman]

"Menon, Nishanth" <nm@ti.com> writes:

>> 
>> thara gopinath <thara@ti.com> writes:
>> 
>> > From: Thara Gopinath <thara@ti.com>
>> >
>> > In the current opp layer the frequency matching API's
>> > tries to match the exact frequency passed in Hz. This leads
>> > to problems in cases where dplls are locked to slightly differnet
>> > frequencies due to sys clock speed or optimum M,N values.
>> > Consider the following scenario
>> > 	a. dpll_x is supposed to be locked at 266Mhz but is
>> > 	   actually att 266.045 Mhz due to above mentioned reasons.
>> > 	b. The opp table has the entry as 266000000 hz.
>> > 	c. The user does a clk_get_rate(dpll_x) and passes the
>> > 	  corresponding rate <rate> into the opp API's to get back
>> > 	  a opp table entry.
>> > 	d. In this scenario if  the user has called into
>> > 	   opp_find_freq_exact it will return an error.
>> > 	   If the user has called into opp_find_freq_ceil
>> > 	   it will either return the opp entry corresponding
>> > 	   to the next higher frequency in the opp table
>> > 	   or return an error if 266 Mhz is the last entry
>> > 	   in the table.
>> >
>> > The above is not correct as we expect the framework to return back
>> > the opp table entry corresponding to 266 Mhz.
>> >
>> > Now there are two ways of fixing this issue.
>> > 	a. Put the correct dpll lock frequency in the opp table.
>> > 	   This means opp table will now have entries like 266045000 hz.
>> > 	   But this is not scalable as these dpll lock frequencies
>> > 	   can change slightly based on sys clk speeds. Like for eg
>> > 	   at sys clk speed 38.4 Mhz we will able to get 266.045 Mhz
>> > 	   where as at sys clk speed 26 Mhz we will be able to get
>> > 	   onyl 266.124 Mhz etc. So depending on the platform we
>> > 	   will have to start changing the opp table.
>> >
>> > 	b. Do the comparison in Mhz in the opp layer rather than in Hz.
>> > 	   This would mean we will divide the rate passed into the opp layer
>> > 	  API and the rates stored in the opp tables by 1000000 to get the
>> > 	  rates in Mhz and do the necessary comparision. In this approach
>> any
>> > 	  vague frequency like 266.045Mhz will get mapped to 266 Mhz in the
>> > 	  opp table. But if the passed rate is 267 Mhz, the opp framework
>> > 	  will still rerturn an error or the next highest opp table entry
>> >
>> > This patch implements solution b. The scenario mentioned above is
>> > esp true for OMAP4 dpll_iva where we do end up with such weird
>> frequencies
>> > due to sys clk being at 38.4 Mhz.
>> 
>> I agree that solution b is better, although it makes the '_exact'
>> function a bit less exact. :/
>> 
>> solution b is fine with me, but the kerneldoc for these find functions
>> should be updated to describe the new matching technique.
>
> I agree, I suggest one improvement though - the search accuracy will vary
> Based on the silicon rev, one size will probably not fit every silicon and
> Domains we have - I suggest having accuracy as a parameter as part of domain
> Registration/configurable parameter
> e.g.
>> > +			unsigned long rate = temp_opp->rate / 1000000;
> Will probably configurable to the "exactness" we expect to handle per domain/silicon family.
>

The more I think about, I think we should leave the 'exact' find the way
it is, especially as we move to device OPPs we will probably want to
have more precise matching.

What about adding another function that does a "find closest"?

Kevin

RE: [PM-OPP][PATCH] OMAP: Modifying the frequency comparison logic.

[Menon, Nishanth]

> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> Sent: Tuesday, August 24, 2010 4:14 PM


> 
> "Menon, Nishanth" <nm@ti.com> writes:
> 
> >>
> >> thara gopinath <thara@ti.com> writes:
> >>
> >> > From: Thara Gopinath <thara@ti.com>
> >> >

[...]
> >> >
> >> > The above is not correct as we expect the framework to return back
> >> > the opp table entry corresponding to 266 Mhz.
> >> >

[...]
> >> >
> >> > 	b. Do the comparison in Mhz in the opp layer rather than in Hz.
> >> > 	   This would mean we will divide the rate passed into the opp layer
> >> > 	  API and the rates stored in the opp tables by 1000000 to get the
> >> > 	  rates in Mhz and do the necessary comparision. In this approach
> >> any
> >> > 	  vague frequency like 266.045Mhz will get mapped to 266 Mhz in the
> >> > 	  opp table. But if the passed rate is 267 Mhz, the opp framework
> >> > 	  will still rerturn an error or the next highest opp table entry
> >> >
> >> > This patch implements solution b. The scenario mentioned above is
> >> > esp true for OMAP4 dpll_iva where we do end up with such weird
> >> frequencies
> >> > due to sys clk being at 38.4 Mhz.
> >>
> >> I agree that solution b is better, although it makes the '_exact'
> >> function a bit less exact. :/
> >>
> >> solution b is fine with me, but the kerneldoc for these find functions
> >> should be updated to describe the new matching technique.
> >
> > I agree, I suggest one improvement though - the search accuracy will
> vary
> > Based on the silicon rev, one size will probably not fit every silicon
> and
> > Domains we have - I suggest having accuracy as a parameter as part of
> domain
> > Registration/configurable parameter
> > e.g.
> >> > +			unsigned long rate = temp_opp->rate / 1000000;
> > Will probably configurable to the "exactness" we expect to handle per
> domain/silicon family.
> >
> 
> The more I think about, I think we should leave the 'exact' find the way
> it is, especially as we move to device OPPs we will probably want to
> have more precise matching.
> 
> What about adding another function that does a "find closest"?
Just my 2cents:
With accuracy as a param? Then we fall back to the question - who would be 
the users of the "_ceil, _floor" functions? Probably exact might should mean exact, and ceil and floor should be using as they were originally intended..

But, the real users of ceil and floor are guys from cpufreq and frequency
searchers - they don't really want to know the minor deltas b/w 19.2MHz Vs
26MHz Vs 38.4MHz sysclk variations caused to precise clock definitions..
They like to do ciel(1Ghz) and get opp corresponding to 1Ghz if 19.2Mhz 
sysclk causes this to be 999,999Mhz it will fail in the current logic..

Also, we would like to users of these apis to remain consistent and not
Know about the variation of freq deltas. E.g.:
Omapx 12MHz to 38.4Mhz causes fluctuations around 100Mhz
OMAPy  19.2MHz to 38.4Mhz causes fluctuations around 10Mhz
The caller should not do:
If cpu_is_omapx()
	Find_closest_ciel(freq,100)
Else if cpu_is_omapy()
	Find_closest_ciel(freq,10)

That'd be nightmare..

Instead as part of cpu domain registration, we mention what is the accuracy
And the callers do find_ciel(freq) and that will "automagically" translate
To accuracy needed for that silicon on that domain..

Regards,
Nishanth Menon

Re: [PM-OPP][PATCH] OMAP: Modifying the frequency comparison logic.

[Kevin Hilman]

"Menon, Nishanth" <nm@ti.com> writes:

>> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>> Sent: Tuesday, August 24, 2010 4:14 PM
>
>
>> 
>> "Menon, Nishanth" <nm@ti.com> writes:
>> 
>> >>
>> >> thara gopinath <thara@ti.com> writes:
>> >>
>> >> > From: Thara Gopinath <thara@ti.com>
>> >> >
>
> [...]
>> >> >
>> >> > The above is not correct as we expect the framework to return back
>> >> > the opp table entry corresponding to 266 Mhz.
>> >> >
>
> [...]
>> >> >
>> >> > 	b. Do the comparison in Mhz in the opp layer rather than in Hz.
>> >> > 	   This would mean we will divide the rate passed into the opp layer
>> >> > 	  API and the rates stored in the opp tables by 1000000 to get the
>> >> > 	  rates in Mhz and do the necessary comparision. In this approach
>> >> any
>> >> > 	  vague frequency like 266.045Mhz will get mapped to 266 Mhz in the
>> >> > 	  opp table. But if the passed rate is 267 Mhz, the opp framework
>> >> > 	  will still rerturn an error or the next highest opp table entry
>> >> >
>> >> > This patch implements solution b. The scenario mentioned above is
>> >> > esp true for OMAP4 dpll_iva where we do end up with such weird
>> >> frequencies
>> >> > due to sys clk being at 38.4 Mhz.
>> >>
>> >> I agree that solution b is better, although it makes the '_exact'
>> >> function a bit less exact. :/
>> >>
>> >> solution b is fine with me, but the kerneldoc for these find functions
>> >> should be updated to describe the new matching technique.
>> >
>> > I agree, I suggest one improvement though - the search accuracy will
>> vary
>> > Based on the silicon rev, one size will probably not fit every silicon
>> and
>> > Domains we have - I suggest having accuracy as a parameter as part of
>> domain
>> > Registration/configurable parameter
>> > e.g.
>> >> > +			unsigned long rate = temp_opp->rate / 1000000;
>> > Will probably configurable to the "exactness" we expect to handle per
>> domain/silicon family.
>> >
>> 
>> The more I think about, I think we should leave the 'exact' find the way
>> it is, especially as we move to device OPPs we will probably want to
>> have more precise matching.
>> 
>> What about adding another function that does a "find closest"?

> Just my 2cents: With accuracy as a param? 

Why would you need accuracy for "find closest"?

> Then we fall back to the question - who would be the users of the
> "_ceil, _floor" functions?  Probably exact might should mean exact,
> and ceil and floor should be using as they were originally intended..

Yeah, I'm thinking so too.

> But, the real users of ceil and floor are guys from cpufreq and frequency
> searchers - they don't really want to know the minor deltas b/w 19.2MHz Vs
> 26MHz Vs 38.4MHz sysclk variations caused to precise clock definitions..
> They like to do ciel(1Ghz) and get opp corresponding to 1Ghz if 19.2Mhz 
> sysclk causes this to be 999,999Mhz it will fail in the current logic..

Yes, that's why I proposed "closest".

> Also, we would like to users of these apis to remain consistent and not
> Know about the variation of freq deltas. E.g.:
> Omapx 12MHz to 38.4Mhz causes fluctuations around 100Mhz
> OMAPy  19.2MHz to 38.4Mhz causes fluctuations around 10Mhz
> The caller should not do:
> If cpu_is_omapx()
> 	Find_closest_ciel(freq,100)
> Else if cpu_is_omapy()
> 	Find_closest_ciel(freq,10)
>
> That'd be nightmare..

Agreed.

Kevin

> Instead as part of cpu domain registration, we mention what is the accuracy
> And the callers do find_ciel(freq) and that will "automagically" translate
> To accuracy needed for that silicon on that domain..

RE: [PM-OPP][PATCH] OMAP: Modifying the frequency comparison logic.

[Menon, Nishanth]

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> Sent: Wednesday, August 25, 2010 5:41 PM
> To: Menon, Nishanth
> Cc: Gopinath, Thara; linux-omap@vger.kernel.org
> Subject: Re: [PM-OPP][PATCH] OMAP: Modifying the frequency comparison
> logic.
> 
> "Menon, Nishanth" <nm@ti.com> writes:
> 
> >> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> >> Sent: Tuesday, August 24, 2010 4:14 PM
> >
> >
> >>
> >> "Menon, Nishanth" <nm@ti.com> writes:
> >>
> >> >>
> >> >> thara gopinath <thara@ti.com> writes:
> >> >>
> >> >> > From: Thara Gopinath <thara@ti.com>
> >> >> >
> >
> > [...]
> >> >> >
> >> >> > The above is not correct as we expect the framework to return back
> >> >> > the opp table entry corresponding to 266 Mhz.
> >> >> >
> >
> > [...]
> >> >> >
> >> >> > 	b. Do the comparison in Mhz in the opp layer rather than in Hz.
> >> >> > 	   This would mean we will divide the rate passed into the opp
> layer
> >> >> > 	  API and the rates stored in the opp tables by 1000000 to get
> the
> >> >> > 	  rates in Mhz and do the necessary comparision. In this
> approach
> >> >> any
> >> >> > 	  vague frequency like 266.045Mhz will get mapped to 266 Mhz
> in the
> >> >> > 	  opp table. But if the passed rate is 267 Mhz, the opp
> framework
> >> >> > 	  will still rerturn an error or the next highest opp table
> entry
> >> >> >
> >> >> > This patch implements solution b. The scenario mentioned above is
> >> >> > esp true for OMAP4 dpll_iva where we do end up with such weird
> >> >> frequencies
> >> >> > due to sys clk being at 38.4 Mhz.
> >> >>
> >> >> I agree that solution b is better, although it makes the '_exact'
> >> >> function a bit less exact. :/
> >> >>
> >> >> solution b is fine with me, but the kerneldoc for these find
> functions
> >> >> should be updated to describe the new matching technique.
> >> >
> >> > I agree, I suggest one improvement though - the search accuracy will
> >> vary
> >> > Based on the silicon rev, one size will probably not fit every
> silicon
> >> and
> >> > Domains we have - I suggest having accuracy as a parameter as part of
> >> domain
> >> > Registration/configurable parameter
> >> > e.g.
> >> >> > +			unsigned long rate = temp_opp->rate / 1000000;
> >> > Will probably configurable to the "exactness" we expect to handle per
> >> domain/silicon family.
> >> >
> >>
> >> The more I think about, I think we should leave the 'exact' find the
> way
> >> it is, especially as we move to device OPPs we will probably want to
> >> have more precise matching.
> >>
> >> What about adding another function that does a "find closest"?
> 
> > Just my 2cents: With accuracy as a param?
> 
> Why would you need accuracy for "find closest"?
> 
How close should "closest" be? There has to be a parameter for this - 
something like as close as 10Mhz,  but beyond the match fails. 
Currently this patch introduces a hardcoded accuracy which could be
Carried forward.

But, On different silicon the deltas due to sysclk can vary - there is no 
silver  bullet algo we can write which will be accurate for all silicon and 
domain  combination..


Further, is'nt ceil and floor functions we have a more specific 
Implementation of "closest"? in the sense, it says closest to which 
Direction - up or down..

[...]
Rest of the discussion seems aligned.

Regards,
Nishanth Menon

RE: [PM-OPP][PATCH] OMAP: Modifying the frequency comparison logic.

[Gopinath, Thara]

>>-----Original Message-----
>>From: Menon, Nishanth
>>Sent: Thursday, August 26, 2010 6:00 AM
>>To: Kevin Hilman
>>Cc: Gopinath, Thara; linux-omap@vger.kernel.org
>>Subject: RE: [PM-OPP][PATCH] OMAP: Modifying the frequency comparison logic.
>>
>>> -----Original Message-----
>>> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>>> Sent: Wednesday, August 25, 2010 5:41 PM
>>> To: Menon, Nishanth
>>> Cc: Gopinath, Thara; linux-omap@vger.kernel.org
>>> Subject: Re: [PM-OPP][PATCH] OMAP: Modifying the frequency comparison
>>> logic.
>>>
>>> "Menon, Nishanth" <nm@ti.com> writes:
>>>
>>> >> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
>>> >> Sent: Tuesday, August 24, 2010 4:14 PM
>>> >
>>> >
>>> >>
>>> >> "Menon, Nishanth" <nm@ti.com> writes:
>>> >>
>>> >> >>
>>> >> >> thara gopinath <thara@ti.com> writes:
>>> >> >>
>>> >> >> > From: Thara Gopinath <thara@ti.com>
>>> >> >> >
>>> >
>>> > [...]
>>> >> >> >
>>> >> >> > The above is not correct as we expect the framework to return back
>>> >> >> > the opp table entry corresponding to 266 Mhz.
>>> >> >> >
>>> >
>>> > [...]
>>> >> >> >
>>> >> >> > 	b. Do the comparison in Mhz in the opp layer rather than in Hz.
>>> >> >> > 	   This would mean we will divide the rate passed into the opp
>>> layer
>>> >> >> > 	  API and the rates stored in the opp tables by 1000000 to get
>>> the
>>> >> >> > 	  rates in Mhz and do the necessary comparision. In this
>>> approach
>>> >> >> any
>>> >> >> > 	  vague frequency like 266.045Mhz will get mapped to 266 Mhz
>>> in the
>>> >> >> > 	  opp table. But if the passed rate is 267 Mhz, the opp
>>> framework
>>> >> >> > 	  will still rerturn an error or the next highest opp table
>>> entry
>>> >> >> >
>>> >> >> > This patch implements solution b. The scenario mentioned above is
>>> >> >> > esp true for OMAP4 dpll_iva where we do end up with such weird
>>> >> >> frequencies
>>> >> >> > due to sys clk being at 38.4 Mhz.
>>> >> >>
>>> >> >> I agree that solution b is better, although it makes the '_exact'
>>> >> >> function a bit less exact. :/
>>> >> >>
>>> >> >> solution b is fine with me, but the kerneldoc for these find
>>> functions
>>> >> >> should be updated to describe the new matching technique.
>>> >> >
>>> >> > I agree, I suggest one improvement though - the search accuracy will
>>> >> vary
>>> >> > Based on the silicon rev, one size will probably not fit every
>>> silicon
>>> >> and
>>> >> > Domains we have - I suggest having accuracy as a parameter as part of
>>> >> domain
>>> >> > Registration/configurable parameter
>>> >> > e.g.
>>> >> >> > +			unsigned long rate = temp_opp->rate / 1000000;
>>> >> > Will probably configurable to the "exactness" we expect to handle per
>>> >> domain/silicon family.
>>> >> >
>>> >>
>>> >> The more I think about, I think we should leave the 'exact' find the
>>> way
>>> >> it is, especially as we move to device OPPs we will probably want to
>>> >> have more precise matching.
>>> >>
>>> >> What about adding another function that does a "find closest"?
>>>
>>> > Just my 2cents: With accuracy as a param?
>>>
>>> Why would you need accuracy for "find closest"?
>>>
>>How close should "closest" be? There has to be a parameter for this -
>>something like as close as 10Mhz,  but beyond the match fails.
>>Currently this patch introduces a hardcoded accuracy which could be
>>Carried forward.
>>
>>But, On different silicon the deltas due to sysclk can vary - there is no
>>silver  bullet algo we can write which will be accurate for all silicon and
>>domain  combination..
>>
>>
>>Further, is'nt ceil and floor functions we have a more specific
>>Implementation of "closest"? in the sense, it says closest to which
>>Direction - up or down..
>>
>>[...]
>>Rest of the discussion seems aligned.

Hello Kevin,

I am not sure about the conclusion of this discussion.
For me we should avoid putting possible weird frequencies
like 266789501.. Hence the round off to /10^6. If on some platforms
due to sys clk we have frequency 267 instead of 266, I would prefer to
edit the opp table through the opp framework APIs. This is my opinion.
I may not have fully understood the concerns discussed in this forum.

Regards
Thara