Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Wed, Dec 21, 2011 at 10:19:11PM +0800, Richard Zhao wrote: > Even cpu node is device, I still need to find a way to get it. I think it's > better have another patch to fix the regulator dt binding in cpu node. I'll > not include it in this patch series. I'd expect this to be easy if we can find any device tree data for the CPU at all? It's just another piece of data no different to the clock rates or whatever. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Wed, Dec 21, 2011 at 13:12, Mark Brown wrote: > On Wed, Dec 21, 2011 at 12:44:57PM +0100, Kay Sievers wrote: > >> We will convert all classes to buses over time time, and have a single >> type of device and a single type of subsystem. > > Are there any conversions that have been done already that I can look at > for reference? The first step is the conversion from 'sys_device' to 'device', which is here: http://git.kernel.org/?p=linux/kernel/git/kay/patches.git;a=tree That should hit the tree soon, if all works according to plan. All sys_devices and sysdev classes will be gone for forever. The 'class' to 'bus' work is simpler, because the logic in both of them is very similar and both use the same 'struct device' already. We'll need to add some convenience APIs to bus, and add code to make sure the converted stuff has compat symlinks in /sys/class when needed. Then we can convert-over one 'struct class' to 'struct bus_type' after the other until 'struct class' can be deleted. This work has not yet started, because we are busy with the sys_device stuff at the moment. No new stuff should use 'struct class' or 'struct sys_device', they should all start right away with 'struct bus_type'. Kay ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Wed, Dec 21, 2011 at 10:43, Arnd Bergmann wrote: > On Wednesday 21 December 2011, Richard Zhao wrote: >> On Wed, Dec 21, 2011 at 09:20:46AM +0800, Richard Zhao wrote: > >> > > > > You also need to define how the core supplies get looked up. >> > > >> > > > It's pure software. platform uses this driver have to define "cpu" >> > > > consumer. >> > > >> > > You still need to define this in the binding. >> > You mean regulator DT binding? already in ? I'll check it. >> Mark, cpu node is not a struct device, sys_device instead. I can not find >> regulator via device/dt node. Can I still use the string to get regulator >> after converting to DT? > > I believe Kay and Greg have the plan to unify "class" and "bus" in sysfs, > which > implies turning sys_device into a derived class of device instead of kobject. > If that understanding is correct, we might as well do that now so we can > attach a device_node to a sys_device. > > Kay, does this make sense? Yes, it's all converted already: http://git.kernel.org/?p=linux/kernel/git/kay/patches.git;a=tree The sysdev stuff will entirely go away, the devices are just 'struct device' and the sysdev classes are all converted to buses. We will convert all classes to buses over time time, and have a single type of device and a single type of subsystem. Kay ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Wed, Dec 21, 2011 at 01:49:07PM +0100, Kay Sievers wrote: > On Wed, Dec 21, 2011 at 13:12, Mark Brown > wrote: > > On Wed, Dec 21, 2011 at 12:44:57PM +0100, Kay Sievers wrote: > > > >> We will convert all classes to buses over time time, and have a single > >> type of device and a single type of subsystem. > > > > Are there any conversions that have been done already that I can look at > > for reference? > > The first step is the conversion from 'sys_device' to 'device', which is here: > http://git.kernel.org/?p=linux/kernel/git/kay/patches.git;a=tree > > That should hit the tree soon, if all works according to plan. All > sys_devices and sysdev classes will be gone for forever. Even cpu node is device, I still need to find a way to get it. I think it's better have another patch to fix the regulator dt binding in cpu node. I'll not include it in this patch series. Richard > > The 'class' to 'bus' work is simpler, because the logic in both of > them is very similar and both use the same 'struct device' already. > > We'll need to add some convenience APIs to bus, and add code to make > sure the converted stuff has compat symlinks in /sys/class when > needed. Then we can convert-over one 'struct class' to 'struct > bus_type' after the other until 'struct class' can be deleted. > > This work has not yet started, because we are busy with the sys_device > stuff at the moment. > > No new stuff should use 'struct class' or 'struct sys_device', they > should all start right away with 'struct bus_type'. > > Kay ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Wed, Dec 21, 2011 at 12:44:57PM +0100, Kay Sievers wrote: > We will convert all classes to buses over time time, and have a single > type of device and a single type of subsystem. Are there any conversions that have been done already that I can look at for reference? ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Wed, Dec 21, 2011 at 09:43:34AM +, Arnd Bergmann wrote: > On Wednesday 21 December 2011, Richard Zhao wrote: > > Mark, cpu node is not a struct device, sys_device instead. I can not find > > regulator via device/dt node. Can I still use the string to get regulator > > after converting to DT? > I believe Kay and Greg have the plan to unify "class" and "bus" in sysfs, > which > implies turning sys_device into a derived class of device instead of kobject. > If that understanding is correct, we might as well do that now so we can > attach a device_node to a sys_device. > Kay, does this make sense? I'm noy Kay but I think even if it's not what we end uo doing internally it's a sensible design for the device tree representation. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Wednesday 21 December 2011, Richard Zhao wrote: > On Wed, Dec 21, 2011 at 09:20:46AM +0800, Richard Zhao wrote: > > > > > You also need to define how the core supplies get looked up. > > > > > > > It's pure software. platform uses this driver have to define "cpu" > > > > consumer. > > > > > > You still need to define this in the binding. > > You mean regulator DT binding? already in ? I'll check it. > Mark, cpu node is not a struct device, sys_device instead. I can not find > regulator via device/dt node. Can I still use the string to get regulator > after converting to DT? I believe Kay and Greg have the plan to unify "class" and "bus" in sysfs, which implies turning sys_device into a derived class of device instead of kobject. If that understanding is correct, we might as well do that now so we can attach a device_node to a sys_device. Kay, does this make sense? Arnd ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Wed, Dec 21, 2011 at 09:20:46AM +0800, Richard Zhao wrote:
> Hi Mark,
>
> On Tue, Dec 20, 2011 at 11:48:45PM +, Mark Brown wrote:
> > On Wed, Dec 21, 2011 at 07:27:03AM +0800, Richard Zhao wrote:
> > > On Tue, Dec 20, 2011 at 02:59:04PM +, Mark Brown wrote:
> >
> > > > My comments on the previous version of the patch still apply:
> >
> > > > - The voltage ranges being set need to be specified as ranges.
> >
> > > cpu normally need strict voltages. and only proved operating opints
> > > are allowed to set in dts. If the voltage changes slightly especially
> > > for high frequency, it's easy to cause unstable.
> >
> > Clearly there will be limits which get more and more restrictive as the
> > frequencies get higher but there will always be at least some play in
> > the numbers as one must at a minimum specify tolerance ranges, and at
> > lower frequencies the ranges specified will typically get compartively
> > large.
> hmm, reasonable. I'll add it in dts. Thanks.
> >
> > Note also that not all hardware specifies things in terms of a fixed set
> > of operating points, sometimes only the minimum voltage specification is
> > varied with frequency or sometimes you see maximum and minimum stepping
> > independently.
> cpus that don't use freq table is out of scope of this driver.
> >
> > Further note that if all hardware really does have as tight a set of
> > requirements as you suggest then the regulator support in the driver
> > needs to be non-optional otherwise a board without software regulator
> > control might drop the frequency without also dropping the voltage.
> It's ok to only adjuct freq without changes voltage. You can find many
> arm soc cpufreq drivers don't change voltage.
> If dts specify voltage but don't have such regulator. I'll assume it
> always runs on the initial volatage (highest for most cases).
> >
> > > > - Frequencies that can't be supported due to limitations of the
> > > >available supplies shouldn't be exposed to users.
> >
> > > As I said, only proved operation points are allowed.
> >
> > This statement appears to be unrelated to the comment you're replying
> > to.
> I meant the exact voltage can always successfull set. Anyway, I'll add
> regulator_set_voltage return value checking.
> >
> > > > You also need to define how the core supplies get looked up.
> >
> > > It's pure software. platform uses this driver have to define "cpu"
> > > consumer.
> >
> > You still need to define this in the binding.
> You mean regulator DT binding? already in ? I'll check it.
Mark, cpu node is not a struct device, sys_device instead. I can not find
regulator via device/dt node. Can I still use the string to get regulator
after converting to DT?
And regulator binding is not on cpufreq git tree.
Thanks
Richard
> >
> > > > > + pr_info("Generic CPU frequency driver\n");
> >
> > > > This seems noisy...
> >
> > > Why? Do you think only errors and warnings can print out?
> >
> > Yes.
>
> Maybe I can remove it. But I'd probably add freq table dump. It's more
> important.
> Agree?
>
> I checked pr_fmt. Thanks very much. I'll use below and remove __func_.
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
>
> Thanks
> Richard
> >
> > ___
> > linux-arm-kernel mailing list
> > linux-arm-ker...@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Wed, Dec 21, 2011 at 02:33:36AM +, Mark Brown wrote: > On Wed, Dec 21, 2011 at 10:24:53AM +0800, Richard Zhao wrote: > > On Wed, Dec 21, 2011 at 01:32:21AM +, Mark Brown wrote: > > > > That's not the point - the point is that you may do something like > > > specify a defined set of frequencies and just drop the minimum supported > > > voltage without altering the maximum supported voltage as the frequency > > > gets lower. > > > What do you mean by "drop"? > > Drop is a synonym for lower in this context. > > > cpu-volts = < > > /* min max */ > > 110 120 /* 1G HZ */ > > 100 120 /* 800M HZ */ > > 90 120 /* 600M HZ */ > > >; > > The above sample dts could meet your point? > > Yes. > > > > While this is important the driver should also be enumerating the > > > supported voltages at probe time and eliminating unsupportable settings > > > so that governors aren't constantly trying to set things that can never > > > be supported. The s3c64xx cpufreq driver provides an implementation of > > > this. > > > I'll use regulator_is_supported_voltage pre-check the cpu-volts. > > For clock, conditions ((clk_round_rate(cpu-freq)/1000) * 1000 == cpu-freq) > > seems too strict. Because cpu clock is SoC dependent, I'll not add the > > check. > > The issue that was there for is that there are multiple runtime > detectable variant clocking configurations which couldn't be switched > between so the driver needs to select only the rates that can be reached > from the current configuration. I'd imagine that might be an issue > elsewhere. one case is that, cpu freq is 800M, clock may only reach 799M. I'd rather let clock code to decide how to handle 800M request. That's why I said the condition check is too strict. Thanks Richard > ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Wed, Dec 21, 2011 at 10:24:53AM +0800, Richard Zhao wrote: > On Wed, Dec 21, 2011 at 01:32:21AM +, Mark Brown wrote: > > That's not the point - the point is that you may do something like > > specify a defined set of frequencies and just drop the minimum supported > > voltage without altering the maximum supported voltage as the frequency > > gets lower. > What do you mean by "drop"? Drop is a synonym for lower in this context. > cpu-volts = < > /* min max */ > 110 120 /* 1G HZ */ > 100 120 /* 800M HZ */ > 90 120 /* 600M HZ */ > >; > The above sample dts could meet your point? Yes. > > While this is important the driver should also be enumerating the > > supported voltages at probe time and eliminating unsupportable settings > > so that governors aren't constantly trying to set things that can never > > be supported. The s3c64xx cpufreq driver provides an implementation of > > this. > I'll use regulator_is_supported_voltage pre-check the cpu-volts. > For clock, conditions ((clk_round_rate(cpu-freq)/1000) * 1000 == cpu-freq) > seems too strict. Because cpu clock is SoC dependent, I'll not add the check. The issue that was there for is that there are multiple runtime detectable variant clocking configurations which couldn't be switched between so the driver needs to select only the rates that can be reached from the current configuration. I'd imagine that might be an issue elsewhere. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Wed, Dec 21, 2011 at 01:32:21AM +, Mark Brown wrote: > On Wed, Dec 21, 2011 at 09:20:46AM +0800, Richard Zhao wrote: > > On Tue, Dec 20, 2011 at 11:48:45PM +, Mark Brown wrote: > > > > Note also that not all hardware specifies things in terms of a fixed set > > > of operating points, sometimes only the minimum voltage specification is > > > varied with frequency or sometimes you see maximum and minimum stepping > > > independently. > > > cpus that don't use freq table is out of scope of this driver. > > That's not the point - the point is that you may do something like > specify a defined set of frequencies and just drop the minimum supported > voltage without altering the maximum supported voltage as the frequency > gets lower. What do you mean by "drop"? cpu-volts = < /* min max */ 110 120 /* 1G HZ */ 100 120 /* 800M HZ */ 90 120 /* 600M HZ */ >; The above sample dts could meet your point? > > > > Further note that if all hardware really does have as tight a set of > > > requirements as you suggest then the regulator support in the driver > > > needs to be non-optional otherwise a board without software regulator > > > control might drop the frequency without also dropping the voltage. > > > It's ok to only adjuct freq without changes voltage. You can find many > > arm soc cpufreq drivers don't change voltage. > > If dts specify voltage but don't have such regulator. I'll assume it > > always runs on the initial volatage (highest for most cases). > > My point exactly; such devices are examples of the policy outlined above > where only the minimum voltage changes with frequency and the maximum > voltage is constant. The cpufreq driver would lower the supported > voltage when possible but wouldn't actually care if the voltage changes. accepted seting voltage range. I guess the above sample dts code meet your point. > > > > > > - Frequencies that can't be supported due to limitations of the > > > > >available supplies shouldn't be exposed to users. > > > > > As I said, only proved operation points are allowed. > > > > This statement appears to be unrelated to the comment you're replying > > > to. > > > I meant the exact voltage can always successfull set. Anyway, I'll add > > This is just not the case. Regulators don't offer a continuous range of > voltages, they offer steps of varying sizes which may miss setting some > voltages, and board designers may choose not to support some of the > voltage range. > > > regulator_set_voltage return value checking. > > While this is important the driver should also be enumerating the > supported voltages at probe time and eliminating unsupportable settings > so that governors aren't constantly trying to set things that can never > be supported. The s3c64xx cpufreq driver provides an implementation of > this. I'll use regulator_is_supported_voltage pre-check the cpu-volts. For clock, conditions ((clk_round_rate(cpu-freq)/1000) * 1000 == cpu-freq) seems too strict. Because cpu clock is SoC dependent, I'll not add the check. > > > Maybe I can remove it. But I'd probably add freq table dump. It's more > > important. > > Agree? > > That seems like something the core should be doing if hmm, cpu table dumps it with pr_debug. Thanks Richard > ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Wed, Dec 21, 2011 at 09:20:46AM +0800, Richard Zhao wrote: > On Tue, Dec 20, 2011 at 11:48:45PM +, Mark Brown wrote: > > Note also that not all hardware specifies things in terms of a fixed set > > of operating points, sometimes only the minimum voltage specification is > > varied with frequency or sometimes you see maximum and minimum stepping > > independently. > cpus that don't use freq table is out of scope of this driver. That's not the point - the point is that you may do something like specify a defined set of frequencies and just drop the minimum supported voltage without altering the maximum supported voltage as the frequency gets lower. > > Further note that if all hardware really does have as tight a set of > > requirements as you suggest then the regulator support in the driver > > needs to be non-optional otherwise a board without software regulator > > control might drop the frequency without also dropping the voltage. > It's ok to only adjuct freq without changes voltage. You can find many > arm soc cpufreq drivers don't change voltage. > If dts specify voltage but don't have such regulator. I'll assume it > always runs on the initial volatage (highest for most cases). My point exactly; such devices are examples of the policy outlined above where only the minimum voltage changes with frequency and the maximum voltage is constant. The cpufreq driver would lower the supported voltage when possible but wouldn't actually care if the voltage changes. > > > > - Frequencies that can't be supported due to limitations of the > > > >available supplies shouldn't be exposed to users. > > > As I said, only proved operation points are allowed. > > This statement appears to be unrelated to the comment you're replying > > to. > I meant the exact voltage can always successfull set. Anyway, I'll add This is just not the case. Regulators don't offer a continuous range of voltages, they offer steps of varying sizes which may miss setting some voltages, and board designers may choose not to support some of the voltage range. > regulator_set_voltage return value checking. While this is important the driver should also be enumerating the supported voltages at probe time and eliminating unsupportable settings so that governors aren't constantly trying to set things that can never be supported. The s3c64xx cpufreq driver provides an implementation of this. > Maybe I can remove it. But I'd probably add freq table dump. It's more > important. > Agree? That seems like something the core should be doing if ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
Hi Mark,
On Tue, Dec 20, 2011 at 11:48:45PM +, Mark Brown wrote:
> On Wed, Dec 21, 2011 at 07:27:03AM +0800, Richard Zhao wrote:
> > On Tue, Dec 20, 2011 at 02:59:04PM +, Mark Brown wrote:
>
> > > My comments on the previous version of the patch still apply:
>
> > > - The voltage ranges being set need to be specified as ranges.
>
> > cpu normally need strict voltages. and only proved operating opints
> > are allowed to set in dts. If the voltage changes slightly especially
> > for high frequency, it's easy to cause unstable.
>
> Clearly there will be limits which get more and more restrictive as the
> frequencies get higher but there will always be at least some play in
> the numbers as one must at a minimum specify tolerance ranges, and at
> lower frequencies the ranges specified will typically get compartively
> large.
hmm, reasonable. I'll add it in dts. Thanks.
>
> Note also that not all hardware specifies things in terms of a fixed set
> of operating points, sometimes only the minimum voltage specification is
> varied with frequency or sometimes you see maximum and minimum stepping
> independently.
cpus that don't use freq table is out of scope of this driver.
>
> Further note that if all hardware really does have as tight a set of
> requirements as you suggest then the regulator support in the driver
> needs to be non-optional otherwise a board without software regulator
> control might drop the frequency without also dropping the voltage.
It's ok to only adjuct freq without changes voltage. You can find many
arm soc cpufreq drivers don't change voltage.
If dts specify voltage but don't have such regulator. I'll assume it
always runs on the initial volatage (highest for most cases).
>
> > > - Frequencies that can't be supported due to limitations of the
> > >available supplies shouldn't be exposed to users.
>
> > As I said, only proved operation points are allowed.
>
> This statement appears to be unrelated to the comment you're replying
> to.
I meant the exact voltage can always successfull set. Anyway, I'll add
regulator_set_voltage return value checking.
>
> > > You also need to define how the core supplies get looked up.
>
> > It's pure software. platform uses this driver have to define "cpu" consumer.
>
> You still need to define this in the binding.
You mean regulator DT binding? already in ? I'll check it.
>
> > > > + pr_info("Generic CPU frequency driver\n");
>
> > > This seems noisy...
>
> > Why? Do you think only errors and warnings can print out?
>
> Yes.
Maybe I can remove it. But I'd probably add freq table dump. It's more
important.
Agree?
I checked pr_fmt. Thanks very much. I'll use below and remove __func_.
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
Thanks
Richard
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Wed, Dec 21, 2011 at 07:27:03AM +0800, Richard Zhao wrote:
> On Tue, Dec 20, 2011 at 02:59:04PM +, Mark Brown wrote:
> > My comments on the previous version of the patch still apply:
> > - The voltage ranges being set need to be specified as ranges.
> cpu normally need strict voltages. and only proved operating opints
> are allowed to set in dts. If the voltage changes slightly especially
> for high frequency, it's easy to cause unstable.
Clearly there will be limits which get more and more restrictive as the
frequencies get higher but there will always be at least some play in
the numbers as one must at a minimum specify tolerance ranges, and at
lower frequencies the ranges specified will typically get compartively
large.
Note also that not all hardware specifies things in terms of a fixed set
of operating points, sometimes only the minimum voltage specification is
varied with frequency or sometimes you see maximum and minimum stepping
independently.
Further note that if all hardware really does have as tight a set of
requirements as you suggest then the regulator support in the driver
needs to be non-optional otherwise a board without software regulator
control might drop the frequency without also dropping the voltage.
> > - Frequencies that can't be supported due to limitations of the
> >available supplies shouldn't be exposed to users.
> As I said, only proved operation points are allowed.
This statement appears to be unrelated to the comment you're replying
to.
> > You also need to define how the core supplies get looked up.
> It's pure software. platform uses this driver have to define "cpu" consumer.
You still need to define this in the binding.
> > > + pr_info("Generic CPU frequency driver\n");
> > This seems noisy...
> Why? Do you think only errors and warnings can print out?
Yes.
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Tue, Dec 20, 2011 at 02:59:04PM +, Mark Brown wrote:
> On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
> > It support single core and multi-core ARM SoCs. But currently it assume
> > all cores share the same frequency and voltage.
>
> My comments on the previous version of the patch still apply:
>
> - The voltage ranges being set need to be specified as ranges.
cpu normally need strict voltages. and only proved operating opints
are allowed to set in dts. If the voltage changes slightly especially
for high frequency, it's easy to cause unstable.
> - Frequencies that can't be supported due to limitations of the
>available supplies shouldn't be exposed to users.
As I said, only proved operation points are allowed.
> - The driver needs to handle errors.
Yes.
>
> > +Required properties in /cpus/cpu@0:
> > +- compatible : "generic-cpufreq"
> > +- cpu-freqs : cpu frequency points it support
> > +- cpu-volts : cpu voltages required by the frequency point at the same
> > index
> > +- trans-latency : transition_latency
>
> You need to define units for all of these, and for the transition
> latency you need to be clear about what's being measured (it looks like
> the CPU time only, not any voltage ramping).
right.
>
> You also need to define how the core supplies get looked up.
It's pure software. platform uses this driver have to define "cpu" consumer.
>
> > + /* Manual states, that PLL stabilizes in two CLK32 periods */
> > + policy->cpuinfo.transition_latency = trans_latency;
>
> I guess this comment is a cut'n'paste error.
right, thanks.
>
> > +
> > + ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
> > +
> > + if (ret < 0) {
> > + pr_err("%s: invalid frequency table for cpu %d\n",
> > + __func__, policy->cpu);
>
> You should define pr_fmt to always include __func__ in the messages
> rather than open coding - ensures consistency and is less noisy in the
> code.
I'll check it.
>
> > + pr_info("Generic CPU frequency driver\n");
>
> This seems noisy...
Why? Do you think only errors and warnings can print out?
Thanks
Richard
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
在 2011-12-20 下午11:13，"Rob Herring" 写道： > > On 12/19/2011 07:59 PM, Richard Zhao wrote: > > On Mon, Dec 19, 2011 at 09:00:44AM -0600, Rob Herring wrote: > >> On 12/19/2011 08:39 AM, Jamie Iles wrote: > >>> On Mon, Dec 19, 2011 at 10:19:29PM +0800, Richard Zhao wrote: > On Mon, Dec 19, 2011 at 10:05:12AM +, Jamie Iles wrote: > > Hi Richard, > > > > On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote: > >> It support single core and multi-core ARM SoCs. But currently it assume > >> all cores share the same frequency and voltage. > >> > >> Signed-off-by: Richard Zhao > >> --- > >> .../devicetree/bindings/cpufreq/generic-cpufreq|7 + > >> drivers/cpufreq/Kconfig|8 + > >> drivers/cpufreq/Makefile |2 + > >> drivers/cpufreq/generic-cpufreq.c | 251 > >> 4 files changed, 268 insertions(+), 0 deletions(-) > >> create mode 100644 Documentation/devicetree/bindings/cpufreq/generic-cpufreq > >> create mode 100644 drivers/cpufreq/generic-cpufreq.c > >> > >> diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq > >> new file mode 100644 > >> index 000..15dd780 > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq > >> @@ -0,0 +1,7 @@ > >> +Generic cpufreq driver > >> + > >> +Required properties in /cpus/cpu@0: > >> +- compatible : "generic-cpufreq" > > > > I'm not convinced this is the best way to do this. By requiring a > > generic-cpufreq compatible string we're encoding Linux driver > > information into the hardware description. The only way I can see to > > avoid this is to provide a generic_clk_cpufreq_init() function that > > platforms can call in their machine init code to use the driver. > >> > >> Agreed on the compatible string. > > Assume you reject to use compatible string. > >> It's putting Linux specifics into DT. > >> > >> You could flip this around and have the module make a call into the > >> kernel to determine whether to initialize or not. Then platforms could > >> set a flag to indicate this. > > Could you make it more clear? kernel global variable, macro, or function? > > Any of those. Of course, direct access to variables across modules is > discouraged, so it would probably be a function that checks a variable. why not use function pointer? arch that don't use this driver do not have to set it. if use function, everyone should define it. > > > - Following your idea, I think, we can add in driver/cpufreq/cpufreq.c: > > int (*clk_reg_cpufreq_get_op_table) (struct op_table *tbl, int *size); > > SoC code set the callback. If it's NULL, driver will exit. We can get rid > > of DT. It'll make cpufreq core dirty, but it's the only file built-in. > > But aren't you getting the operating points from the DT? Then you don't > want to put this code into each platform. the variable is mainly used to check whether some platform want to use this driver. getting ride of dt is side affect. > > > > > - Drop module support. SoC call generic_clk_cpufreq_init as Jamie said. > > > >> > It'll prevent the driver from being a kernel module. > >>> > >>> Hmm, that's not very nice either! I guess you _could_ add an > >>> of_machine_is_compatible() check against a list of compatible machines > >>> in the driver but that feels a little gross. Hopefully Rob or Grant > >>> have a good alternative! > >>> > >> > >> What does cpufreq core do if multiple drivers are registered? > > current cpufreq core only support one cpufreq_driver. Others will fail > > except the first time. > > Then whoever gets there first wins. Make your driver register late and > if someone doesn't want to use it they can register a custom driver earlier. this driver did > > Rob > > >> Perhaps a > >> ranking is needed and this would only get enabled if there are no other > >> drivers and other conditions like having the clock "cpu" present are met. > > We'd better keep cpufreq core simple. For this driver, register cpufreq_driver > > is the last thing after checking all conditions. > > > >> > >> Rob > >> > Hi Grant & Rob, > > Could you comment? > > > > >> +- cpu-freqs : cpu frequency points it support > >> +- cpu-volts : cpu voltages required by the frequency point at the same index > >> +- trans-latency : transition_latency > >> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig > >> index e24a2a1..216eecd 100644 > >> --- a/drivers/cpufreq/Kconfig > >> +++ b/drivers/cpufreq/Kconfig > >> @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE > >> > >>If in doubt, say N. > >> > >> +config GENERIC_CPUFREQ_DRIVER > >> +bool "Generic cpufreq driver using clock/regulator/devicetree"
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
在 2011-12-20 下午11:22，"Arnd Bergmann" 写道： > > On Tuesday 20 December 2011, Richard Zhao wrote: > > > +Generic cpufreq driver > > > + > > > +Required properties in /cpus/cpu@0: > > > +- compatible : "generic-cpufreq" > > > >>> > > > >>> I'm not convinced this is the best way to do this. By requiring a > > > >>> generic-cpufreq compatible string we're encoding Linux driver > > > >>> information into the hardware description. The only way I can see to > > > >>> avoid this is to provide a generic_clk_cpufreq_init() function that > > > >>> platforms can call in their machine init code to use the driver. > > > > > > Agreed on the compatible string. > > Assume you reject to use compatible string. > > > It's putting Linux specifics into DT. > > > > > > You could flip this around and have the module make a call into the > > > kernel to determine whether to initialize or not. Then platforms could > > > set a flag to indicate this. > > Could you make it more clear? kernel global variable, macro, or function? > > > > - Following your idea, I think, we can add in driver/cpufreq/cpufreq.c: > > int (*clk_reg_cpufreq_get_op_table) (struct op_table *tbl, int *size); > > SoC code set the callback. If it's NULL, driver will exit. We can get rid > > of DT. It'll make cpufreq core dirty, but it's the only file built-in. > > > > - Drop module support. SoC call generic_clk_cpufreq_init as Jamie said. > > I think you don't actually need a "compatible" property here, as long as we > ensure that the "cpu-freqs", "cpu-volts" and "trans-latency" properties are > present in the cpu node if and only if the machine works with this driver > (why else would you put them there?). It is more easy for me. Rob, agree? Richard > > CPUs are special in the device trees in a number of ways, so I think we > can treat this as a logical extension. This way, you can still make the > generic cpufreq driver a loadable module. You don't get module autoloading > with this structure, but that is already the case because the cpu nodes > in the device tree are not translated into linux devices. > >Arnd ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Tuesday 20 December 2011, Richard Zhao wrote: > > +Generic cpufreq driver > > + > > +Required properties in /cpus/cpu@0: > > +- compatible : "generic-cpufreq" > > >>> > > >>> I'm not convinced this is the best way to do this. By requiring a > > >>> generic-cpufreq compatible string we're encoding Linux driver > > >>> information into the hardware description. The only way I can see to > > >>> avoid this is to provide a generic_clk_cpufreq_init() function that > > >>> platforms can call in their machine init code to use the driver. > > > > Agreed on the compatible string. > Assume you reject to use compatible string. > > It's putting Linux specifics into DT. > > > > You could flip this around and have the module make a call into the > > kernel to determine whether to initialize or not. Then platforms could > > set a flag to indicate this. > Could you make it more clear? kernel global variable, macro, or function? > > - Following your idea, I think, we can add in driver/cpufreq/cpufreq.c: > int (*clk_reg_cpufreq_get_op_table) (struct op_table *tbl, int *size); > SoC code set the callback. If it's NULL, driver will exit. We can get rid > of DT. It'll make cpufreq core dirty, but it's the only file built-in. > > - Drop module support. SoC call generic_clk_cpufreq_init as Jamie said. I think you don't actually need a "compatible" property here, as long as we ensure that the "cpu-freqs", "cpu-volts" and "trans-latency" properties are present in the cpu node if and only if the machine works with this driver (why else would you put them there?). CPUs are special in the device trees in a number of ways, so I think we can treat this as a logical extension. This way, you can still make the generic cpufreq driver a loadable module. You don't get module autoloading with this structure, but that is already the case because the cpu nodes in the device tree are not translated into linux devices. Arnd ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On 12/19/2011 07:59 PM, Richard Zhao wrote: > On Mon, Dec 19, 2011 at 09:00:44AM -0600, Rob Herring wrote: >> On 12/19/2011 08:39 AM, Jamie Iles wrote: >>> On Mon, Dec 19, 2011 at 10:19:29PM +0800, Richard Zhao wrote: On Mon, Dec 19, 2011 at 10:05:12AM +, Jamie Iles wrote: > Hi Richard, > > On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote: >> It support single core and multi-core ARM SoCs. But currently it assume >> all cores share the same frequency and voltage. >> >> Signed-off-by: Richard Zhao >> --- >> .../devicetree/bindings/cpufreq/generic-cpufreq|7 + >> drivers/cpufreq/Kconfig|8 + >> drivers/cpufreq/Makefile |2 + >> drivers/cpufreq/generic-cpufreq.c | 251 >> >> 4 files changed, 268 insertions(+), 0 deletions(-) >> create mode 100644 >> Documentation/devicetree/bindings/cpufreq/generic-cpufreq >> create mode 100644 drivers/cpufreq/generic-cpufreq.c >> >> diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq >> b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq >> new file mode 100644 >> index 000..15dd780 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq >> @@ -0,0 +1,7 @@ >> +Generic cpufreq driver >> + >> +Required properties in /cpus/cpu@0: >> +- compatible : "generic-cpufreq" > > I'm not convinced this is the best way to do this. By requiring a > generic-cpufreq compatible string we're encoding Linux driver > information into the hardware description. The only way I can see to > avoid this is to provide a generic_clk_cpufreq_init() function that > platforms can call in their machine init code to use the driver. >> >> Agreed on the compatible string. > Assume you reject to use compatible string. >> It's putting Linux specifics into DT. >> >> You could flip this around and have the module make a call into the >> kernel to determine whether to initialize or not. Then platforms could >> set a flag to indicate this. > Could you make it more clear? kernel global variable, macro, or function? Any of those. Of course, direct access to variables across modules is discouraged, so it would probably be a function that checks a variable. > - Following your idea, I think, we can add in driver/cpufreq/cpufreq.c: > int (*clk_reg_cpufreq_get_op_table) (struct op_table *tbl, int *size); > SoC code set the callback. If it's NULL, driver will exit. We can get rid > of DT. It'll make cpufreq core dirty, but it's the only file built-in. But aren't you getting the operating points from the DT? Then you don't want to put this code into each platform. > > - Drop module support. SoC call generic_clk_cpufreq_init as Jamie said. > >> It'll prevent the driver from being a kernel module. >>> >>> Hmm, that's not very nice either! I guess you _could_ add an >>> of_machine_is_compatible() check against a list of compatible machines >>> in the driver but that feels a little gross. Hopefully Rob or Grant >>> have a good alternative! >>> >> >> What does cpufreq core do if multiple drivers are registered? > current cpufreq core only support one cpufreq_driver. Others will fail > except the first time. Then whoever gets there first wins. Make your driver register late and if someone doesn't want to use it they can register a custom driver earlier. Rob >> Perhaps a >> ranking is needed and this would only get enabled if there are no other >> drivers and other conditions like having the clock "cpu" present are met. > We'd better keep cpufreq core simple. For this driver, register cpufreq_driver > is the last thing after checking all conditions. > >> >> Rob >> Hi Grant & Rob, Could you comment? > >> +- cpu-freqs : cpu frequency points it support >> +- cpu-volts : cpu voltages required by the frequency point at the same >> index >> +- trans-latency : transition_latency >> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig >> index e24a2a1..216eecd 100644 >> --- a/drivers/cpufreq/Kconfig >> +++ b/drivers/cpufreq/Kconfig >> @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE >> >>If in doubt, say N. >> >> +config GENERIC_CPUFREQ_DRIVER >> +bool "Generic cpufreq driver using clock/regulator/devicetree" >> +help >> + This adds generic CPUFreq driver. It assumes all >> + cores of the CPU share the same clock and voltage. >> + >> + If in doubt, say N. > > I think this needs dependencies on HAVE_CLK, OF and REGULATOR. right, Thanks. I can not check clk before generic clock framework come in. Added: depends on OF && REGULATOR select CPU_FREQ_TABLE >>> >>> You can still us
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
> It support single core and multi-core ARM SoCs. But currently it assume
> all cores share the same frequency and voltage.
My comments on the previous version of the patch still apply:
- The voltage ranges being set need to be specified as ranges.
- Frequencies that can't be supported due to limitations of the
available supplies shouldn't be exposed to users.
- The driver needs to handle errors.
> +Required properties in /cpus/cpu@0:
> +- compatible : "generic-cpufreq"
> +- cpu-freqs : cpu frequency points it support
> +- cpu-volts : cpu voltages required by the frequency point at the same index
> +- trans-latency : transition_latency
You need to define units for all of these, and for the transition
latency you need to be clear about what's being measured (it looks like
the CPU time only, not any voltage ramping).
You also need to define how the core supplies get looked up.
> + /* Manual states, that PLL stabilizes in two CLK32 periods */
> + policy->cpuinfo.transition_latency = trans_latency;
I guess this comment is a cut'n'paste error.
> +
> + ret = cpufreq_frequency_table_cpuinfo(policy, freq_table);
> +
> + if (ret < 0) {
> + pr_err("%s: invalid frequency table for cpu %d\n",
> +__func__, policy->cpu);
You should define pr_fmt to always include __func__ in the messages
rather than open coding - ensures consistency and is less noisy in the
code.
> + pr_info("Generic CPU frequency driver\n");
This seems noisy...
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Mon, Dec 19, 2011 at 09:00:44AM -0600, Rob Herring wrote: > On 12/19/2011 08:39 AM, Jamie Iles wrote: > > On Mon, Dec 19, 2011 at 10:19:29PM +0800, Richard Zhao wrote: > >> On Mon, Dec 19, 2011 at 10:05:12AM +, Jamie Iles wrote: > >>> Hi Richard, > >>> > >>> On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote: > It support single core and multi-core ARM SoCs. But currently it assume > all cores share the same frequency and voltage. > > Signed-off-by: Richard Zhao > --- > .../devicetree/bindings/cpufreq/generic-cpufreq|7 + > drivers/cpufreq/Kconfig|8 + > drivers/cpufreq/Makefile |2 + > drivers/cpufreq/generic-cpufreq.c | 251 > > 4 files changed, 268 insertions(+), 0 deletions(-) > create mode 100644 > Documentation/devicetree/bindings/cpufreq/generic-cpufreq > create mode 100644 drivers/cpufreq/generic-cpufreq.c > > diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq > b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq > new file mode 100644 > index 000..15dd780 > --- /dev/null > +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq > @@ -0,0 +1,7 @@ > +Generic cpufreq driver > + > +Required properties in /cpus/cpu@0: > +- compatible : "generic-cpufreq" > >>> > >>> I'm not convinced this is the best way to do this. By requiring a > >>> generic-cpufreq compatible string we're encoding Linux driver > >>> information into the hardware description. The only way I can see to > >>> avoid this is to provide a generic_clk_cpufreq_init() function that > >>> platforms can call in their machine init code to use the driver. > > Agreed on the compatible string. Assume you reject to use compatible string. > It's putting Linux specifics into DT. > > You could flip this around and have the module make a call into the > kernel to determine whether to initialize or not. Then platforms could > set a flag to indicate this. Could you make it more clear? kernel global variable, macro, or function? - Following your idea, I think, we can add in driver/cpufreq/cpufreq.c: int (*clk_reg_cpufreq_get_op_table) (struct op_table *tbl, int *size); SoC code set the callback. If it's NULL, driver will exit. We can get rid of DT. It'll make cpufreq core dirty, but it's the only file built-in. - Drop module support. SoC call generic_clk_cpufreq_init as Jamie said. > > >> It'll prevent the driver from being a kernel module. > > > > Hmm, that's not very nice either! I guess you _could_ add an > > of_machine_is_compatible() check against a list of compatible machines > > in the driver but that feels a little gross. Hopefully Rob or Grant > > have a good alternative! > > > > What does cpufreq core do if multiple drivers are registered? current cpufreq core only support one cpufreq_driver. Others will fail except the first time. > Perhaps a > ranking is needed and this would only get enabled if there are no other > drivers and other conditions like having the clock "cpu" present are met. We'd better keep cpufreq core simple. For this driver, register cpufreq_driver is the last thing after checking all conditions. > > Rob > > >> Hi Grant & Rob, > >> > >> Could you comment? > >> > >>> > +- cpu-freqs : cpu frequency points it support > +- cpu-volts : cpu voltages required by the frequency point at the same > index > +- trans-latency : transition_latency > diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig > index e24a2a1..216eecd 100644 > --- a/drivers/cpufreq/Kconfig > +++ b/drivers/cpufreq/Kconfig > @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE > > If in doubt, say N. > > +config GENERIC_CPUFREQ_DRIVER > +bool "Generic cpufreq driver using clock/regulator/devicetree" > +help > + This adds generic CPUFreq driver. It assumes all > + cores of the CPU share the same clock and voltage. > + > + If in doubt, say N. > >>> > >>> I think this needs dependencies on HAVE_CLK, OF and REGULATOR. > >> right, Thanks. I can not check clk before generic clock framework > >> come in. > >> Added: > >>depends on OF && REGULATOR > >>select CPU_FREQ_TABLE > > > > You can still use HAVE_CLK. That symbol has been around for ages and > > any platform implementing the clk API should select it so it's fine to > > depend on it even before there is a generic struct clk. You are right. Thanks. Richard > > > > Jamie > > ___ > linux-arm-kernel mailing list > linux-arm-ker...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > ___ linaro-dev mailing
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
Hi Richard,
On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
> It support single core and multi-core ARM SoCs. But currently it assume
> all cores share the same frequency and voltage.
>
> Signed-off-by: Richard Zhao
> ---
> .../devicetree/bindings/cpufreq/generic-cpufreq|7 +
> drivers/cpufreq/Kconfig|8 +
> drivers/cpufreq/Makefile |2 +
> drivers/cpufreq/generic-cpufreq.c | 251
>
> 4 files changed, 268 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> create mode 100644 drivers/cpufreq/generic-cpufreq.c
>
> diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> new file mode 100644
> index 000..15dd780
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> @@ -0,0 +1,7 @@
> +Generic cpufreq driver
> +
> +Required properties in /cpus/cpu@0:
> +- compatible : "generic-cpufreq"
I'm not convinced this is the best way to do this. By requiring a
generic-cpufreq compatible string we're encoding Linux driver
information into the hardware description. The only way I can see to
avoid this is to provide a generic_clk_cpufreq_init() function that
platforms can call in their machine init code to use the driver.
> +- cpu-freqs : cpu frequency points it support
> +- cpu-volts : cpu voltages required by the frequency point at the same index
> +- trans-latency : transition_latency
> diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> index e24a2a1..216eecd 100644
> --- a/drivers/cpufreq/Kconfig
> +++ b/drivers/cpufreq/Kconfig
> @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE
>
> If in doubt, say N.
>
> +config GENERIC_CPUFREQ_DRIVER
> + bool "Generic cpufreq driver using clock/regulator/devicetree"
> + help
> + This adds generic CPUFreq driver. It assumes all
> + cores of the CPU share the same clock and voltage.
> +
> + If in doubt, say N.
I think this needs dependencies on HAVE_CLK, OF and REGULATOR.
> +
> menu "x86 CPU frequency scaling drivers"
> depends on X86
> source "drivers/cpufreq/Kconfig.x86"
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index ce75fcb..2dbdab1 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -13,6 +13,8 @@ obj-$(CONFIG_CPU_FREQ_GOV_CONSERVATIVE) +=
> cpufreq_conservative.o
> # CPUfreq cross-arch helpers
> obj-$(CONFIG_CPU_FREQ_TABLE) += freq_table.o
>
> +obj-$(CONFIG_GENERIC_CPUFREQ_DRIVER) += generic-cpufreq.o
> +
>
> ##
> # x86 drivers.
> # Link order matters. K8 is preferred to ACPI because of firmware bugs in
> early
> diff --git a/drivers/cpufreq/generic-cpufreq.c
> b/drivers/cpufreq/generic-cpufreq.c
> new file mode 100644
> index 000..781bb9b
> --- /dev/null
> +++ b/drivers/cpufreq/generic-cpufreq.c
> @@ -0,0 +1,251 @@
> +/*
> + * Copyright (C) 2011 Freescale Semiconductor, Inc.
> + */
> +
> +/*
> + * The code contained herein is licensed under the GNU General Public
> + * License. You may obtain a copy of the GNU General Public License
> + * Version 2 or later at the following locations:
> + *
> + * http://www.opensource.org/licenses/gpl-license.html
> + * http://www.gnu.org/copyleft/gpl.html
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +static u32 *cpu_freqs; /* HZ */
> +static u32 *cpu_volts; /* uV */
> +static u32 trans_latency; /* ns */
> +static int cpu_op_nr;
> +
> +static struct clk *cpu_clk;
> +static struct regulator *cpu_reg;
> +static struct cpufreq_frequency_table *freq_table;
> +
> +static int set_cpu_freq(unsigned long freq, int index, int higher)
> +{
> + int ret = 0;
> +
> + if (higher && cpu_reg)
> + regulator_set_voltage(cpu_reg,
> + cpu_volts[index], cpu_volts[index]);
> +
> + ret = clk_set_rate(cpu_clk, freq);
> + if (ret != 0) {
> + pr_err("generic-cpufreq: cannot set CPU clock rate\n");
> + return ret;
> + }
> +
> + if (!higher && cpu_reg)
> + regulator_set_voltage(cpu_reg,
> + cpu_volts[index], cpu_volts[index]);
> +
> + return ret;
> +}
> +
> +static int generic_verify_speed(struct cpufreq_policy *policy)
> +{
> + return cpufreq_frequency_table_verify(policy, freq_table);
> +}
> +
> +static unsigned int generic_get_speed(unsigned int cpu)
> +{
> + return clk_get_rate(cpu_clk) / 1000;
> +}
> +
> +static int generic_set_target(struct cpufreq_policy *policy,
> + unsigned int target_freq, unsigned int relation)
> +{
> + struct cpufreq_freqs freqs;
> + unsigned long freq_Hz;
> + int cpu;
> + int ret = 0;
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Mon, Dec 19, 2011 at 10:19:29PM +0800, Richard Zhao wrote: > On Mon, Dec 19, 2011 at 10:05:12AM +, Jamie Iles wrote: > > Hi Richard, > > > > On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote: > > > It support single core and multi-core ARM SoCs. But currently it assume > > > all cores share the same frequency and voltage. > > > > > > Signed-off-by: Richard Zhao > > > --- > > > .../devicetree/bindings/cpufreq/generic-cpufreq|7 + > > > drivers/cpufreq/Kconfig|8 + > > > drivers/cpufreq/Makefile |2 + > > > drivers/cpufreq/generic-cpufreq.c | 251 > > > > > > 4 files changed, 268 insertions(+), 0 deletions(-) > > > create mode 100644 > > > Documentation/devicetree/bindings/cpufreq/generic-cpufreq > > > create mode 100644 drivers/cpufreq/generic-cpufreq.c > > > > > > diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq > > > b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq > > > new file mode 100644 > > > index 000..15dd780 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq > > > @@ -0,0 +1,7 @@ > > > +Generic cpufreq driver > > > + > > > +Required properties in /cpus/cpu@0: > > > +- compatible : "generic-cpufreq" > > > > I'm not convinced this is the best way to do this. By requiring a > > generic-cpufreq compatible string we're encoding Linux driver > > information into the hardware description. The only way I can see to > > avoid this is to provide a generic_clk_cpufreq_init() function that > > platforms can call in their machine init code to use the driver. > It'll prevent the driver from being a kernel module. Hmm, that's not very nice either! I guess you _could_ add an of_machine_is_compatible() check against a list of compatible machines in the driver but that feels a little gross. Hopefully Rob or Grant have a good alternative! > Hi Grant & Rob, > > Could you comment? > > > > > > +- cpu-freqs : cpu frequency points it support > > > +- cpu-volts : cpu voltages required by the frequency point at the same > > > index > > > +- trans-latency : transition_latency > > > diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig > > > index e24a2a1..216eecd 100644 > > > --- a/drivers/cpufreq/Kconfig > > > +++ b/drivers/cpufreq/Kconfig > > > @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE > > > > > > If in doubt, say N. > > > > > > +config GENERIC_CPUFREQ_DRIVER > > > + bool "Generic cpufreq driver using clock/regulator/devicetree" > > > + help > > > + This adds generic CPUFreq driver. It assumes all > > > + cores of the CPU share the same clock and voltage. > > > + > > > + If in doubt, say N. > > > > I think this needs dependencies on HAVE_CLK, OF and REGULATOR. > right, Thanks. I can not check clk before generic clock framework > come in. > Added: > depends on OF && REGULATOR > select CPU_FREQ_TABLE You can still use HAVE_CLK. That symbol has been around for ages and any platform implementing the clk API should select it so it's fine to depend on it even before there is a generic struct clk. Jamie ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On 12/19/2011 08:39 AM, Jamie Iles wrote: > On Mon, Dec 19, 2011 at 10:19:29PM +0800, Richard Zhao wrote: >> On Mon, Dec 19, 2011 at 10:05:12AM +, Jamie Iles wrote: >>> Hi Richard, >>> >>> On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote: It support single core and multi-core ARM SoCs. But currently it assume all cores share the same frequency and voltage. Signed-off-by: Richard Zhao --- .../devicetree/bindings/cpufreq/generic-cpufreq|7 + drivers/cpufreq/Kconfig|8 + drivers/cpufreq/Makefile |2 + drivers/cpufreq/generic-cpufreq.c | 251 4 files changed, 268 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/cpufreq/generic-cpufreq create mode 100644 drivers/cpufreq/generic-cpufreq.c diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq new file mode 100644 index 000..15dd780 --- /dev/null +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq @@ -0,0 +1,7 @@ +Generic cpufreq driver + +Required properties in /cpus/cpu@0: +- compatible : "generic-cpufreq" >>> >>> I'm not convinced this is the best way to do this. By requiring a >>> generic-cpufreq compatible string we're encoding Linux driver >>> information into the hardware description. The only way I can see to >>> avoid this is to provide a generic_clk_cpufreq_init() function that >>> platforms can call in their machine init code to use the driver. Agreed on the compatible string. It's putting Linux specifics into DT. You could flip this around and have the module make a call into the kernel to determine whether to initialize or not. Then platforms could set a flag to indicate this. >> It'll prevent the driver from being a kernel module. > > Hmm, that's not very nice either! I guess you _could_ add an > of_machine_is_compatible() check against a list of compatible machines > in the driver but that feels a little gross. Hopefully Rob or Grant > have a good alternative! > What does cpufreq core do if multiple drivers are registered? Perhaps a ranking is needed and this would only get enabled if there are no other drivers and other conditions like having the clock "cpu" present are met. Rob >> Hi Grant & Rob, >> >> Could you comment? >> >>> +- cpu-freqs : cpu frequency points it support +- cpu-volts : cpu voltages required by the frequency point at the same index +- trans-latency : transition_latency diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig index e24a2a1..216eecd 100644 --- a/drivers/cpufreq/Kconfig +++ b/drivers/cpufreq/Kconfig @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE If in doubt, say N. +config GENERIC_CPUFREQ_DRIVER + bool "Generic cpufreq driver using clock/regulator/devicetree" + help +This adds generic CPUFreq driver. It assumes all +cores of the CPU share the same clock and voltage. + +If in doubt, say N. >>> >>> I think this needs dependencies on HAVE_CLK, OF and REGULATOR. >> right, Thanks. I can not check clk before generic clock framework >> come in. >> Added: >> depends on OF && REGULATOR >> select CPU_FREQ_TABLE > > You can still use HAVE_CLK. That symbol has been around for ages and > any platform implementing the clk API should select it so it's fine to > depend on it even before there is a generic struct clk. > > Jamie ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH V3 4/7] cpufreq: add generic cpufreq driver
On Mon, Dec 19, 2011 at 10:05:12AM +, Jamie Iles wrote:
> Hi Richard,
>
> On Mon, Dec 19, 2011 at 11:21:40AM +0800, Richard Zhao wrote:
> > It support single core and multi-core ARM SoCs. But currently it assume
> > all cores share the same frequency and voltage.
> >
> > Signed-off-by: Richard Zhao
> > ---
> > .../devicetree/bindings/cpufreq/generic-cpufreq|7 +
> > drivers/cpufreq/Kconfig|8 +
> > drivers/cpufreq/Makefile |2 +
> > drivers/cpufreq/generic-cpufreq.c | 251
> >
> > 4 files changed, 268 insertions(+), 0 deletions(-)
> > create mode 100644
> > Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> > create mode 100644 drivers/cpufreq/generic-cpufreq.c
> >
> > diff --git a/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> > b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> > new file mode 100644
> > index 000..15dd780
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/cpufreq/generic-cpufreq
> > @@ -0,0 +1,7 @@
> > +Generic cpufreq driver
> > +
> > +Required properties in /cpus/cpu@0:
> > +- compatible : "generic-cpufreq"
>
> I'm not convinced this is the best way to do this. By requiring a
> generic-cpufreq compatible string we're encoding Linux driver
> information into the hardware description. The only way I can see to
> avoid this is to provide a generic_clk_cpufreq_init() function that
> platforms can call in their machine init code to use the driver.
It'll prevent the driver from being a kernel module.
Hi Grant & Rob,
Could you comment?
>
> > +- cpu-freqs : cpu frequency points it support
> > +- cpu-volts : cpu voltages required by the frequency point at the same
> > index
> > +- trans-latency : transition_latency
> > diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> > index e24a2a1..216eecd 100644
> > --- a/drivers/cpufreq/Kconfig
> > +++ b/drivers/cpufreq/Kconfig
> > @@ -179,6 +179,14 @@ config CPU_FREQ_GOV_CONSERVATIVE
> >
> > If in doubt, say N.
> >
> > +config GENERIC_CPUFREQ_DRIVER
> > + bool "Generic cpufreq driver using clock/regulator/devicetree"
> > + help
> > + This adds generic CPUFreq driver. It assumes all
> > + cores of the CPU share the same clock and voltage.
> > +
> > + If in doubt, say N.
>
> I think this needs dependencies on HAVE_CLK, OF and REGULATOR.
right, Thanks. I can not check clk before generic clock framework
come in.
Added:
depends on OF && REGULATOR
select CPU_FREQ_TABLE
>
> > +
> > menu "x86 CPU frequency scaling drivers"
> > depends on X86
> > source "drivers/cpufreq/Kconfig.x86"
> > diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> > index ce75fcb..2dbdab1 100644
> > --- a/drivers/cpufreq/Makefile
> > +++ b/drivers/cpufreq/Makefile
> > @@ -13,6 +13,8 @@ obj-$(CONFIG_CPU_FREQ_GOV_CONSERVATIVE) +=
> > cpufreq_conservative.o
> > # CPUfreq cross-arch helpers
> > obj-$(CONFIG_CPU_FREQ_TABLE) += freq_table.o
> >
> > +obj-$(CONFIG_GENERIC_CPUFREQ_DRIVER) += generic-cpufreq.o
> > +
> >
> > ##
> > # x86 drivers.
> > # Link order matters. K8 is preferred to ACPI because of firmware bugs in
> > early
> > diff --git a/drivers/cpufreq/generic-cpufreq.c
> > b/drivers/cpufreq/generic-cpufreq.c
> > new file mode 100644
> > index 000..781bb9b
> > --- /dev/null
> > +++ b/drivers/cpufreq/generic-cpufreq.c
> > @@ -0,0 +1,251 @@
> > +/*
> > + * Copyright (C) 2011 Freescale Semiconductor, Inc.
> > + */
> > +
> > +/*
> > + * The code contained herein is licensed under the GNU General Public
> > + * License. You may obtain a copy of the GNU General Public License
> > + * Version 2 or later at the following locations:
> > + *
> > + * http://www.opensource.org/licenses/gpl-license.html
> > + * http://www.gnu.org/copyleft/gpl.html
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +static u32 *cpu_freqs; /* HZ */
> > +static u32 *cpu_volts; /* uV */
> > +static u32 trans_latency; /* ns */
> > +static int cpu_op_nr;
> > +
> > +static struct clk *cpu_clk;
> > +static struct regulator *cpu_reg;
> > +static struct cpufreq_frequency_table *freq_table;
> > +
> > +static int set_cpu_freq(unsigned long freq, int index, int higher)
> > +{
> > + int ret = 0;
> > +
> > + if (higher && cpu_reg)
> > + regulator_set_voltage(cpu_reg,
> > + cpu_volts[index], cpu_volts[index]);
> > +
> > + ret = clk_set_rate(cpu_clk, freq);
> > + if (ret != 0) {
> > + pr_err("generic-cpufreq: cannot set CPU clock rate\n");
> > + return ret;
> > + }
> > +
> > + if (!higher && cpu_reg)
> > + regulator_set_voltage(cpu_reg,
> > + cpu_volts[index], cpu_volts[index]);
> >
