* Re: [PATCH] PM / devfreq: missing rcu_read_lock() added for find_device_opp()
From: Serge E. Hallyn @ 2012-11-22 17:45 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: MyungJoo Ham, myungjoo.ham, linux-pm, rjw, keescook, serge.hallyn,
linux-kernel
In-Reply-To: <20121122140928.GA14567@mail.hallyn.com>
Quoting Serge E. Hallyn (serge@hallyn.com):
> Quoting MyungJoo Ham (myungjoo.ham@samsung.com):
> > opp_get_notifier() uses find_device_opp(), which requires to
> > held rcu_read_lock. In order to keep the notifier-header
> > valid, we have added rcu_read_lock().
> >
Well, to be honest, the opp locking isn't 100% clear to me, but IIUC
(1) things can be added but never removed, (2) opp_get_notifier doesn't
pin a refcount on what it returns, but it returns something which won't
be deleted.
So IIUC what's below is fine, bc it doesn't need to pin the nh for the
nh to remain valid outside of the rcu_read_lock. If I'm wrong about
that, then the below is not sufficient.
> Reviewed-by: Serge Hallyn <serge.hallyn@canonical.com>
>
> > Reported-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> > ---
> > drivers/devfreq/devfreq.c | 26 ++++++++++++++++++++------
> > 1 files changed, 20 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > index 1388d46..5275883 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> > @@ -1023,11 +1023,18 @@ struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq,
> > */
> > int devfreq_register_opp_notifier(struct device *dev, struct devfreq *devfreq)
> > {
> > - struct srcu_notifier_head *nh = opp_get_notifier(dev);
> > + struct srcu_notifier_head *nh;
> > + int ret = 0;
> >
> > + rcu_read_lock();
> > + nh = opp_get_notifier(dev);
> > if (IS_ERR(nh))
> > - return PTR_ERR(nh);
> > - return srcu_notifier_chain_register(nh, &devfreq->nb);
> > + ret = PTR_ERR(nh);
> > + rcu_read_unlock();
> > + if (!ret)
> > + ret = srcu_notifier_chain_register(nh, &devfreq->nb);
> > +
> > + return ret;
> > }
> >
> > /**
> > @@ -1042,11 +1049,18 @@ int devfreq_register_opp_notifier(struct device *dev, struct devfreq *devfreq)
> > */
> > int devfreq_unregister_opp_notifier(struct device *dev, struct devfreq *devfreq)
> > {
> > - struct srcu_notifier_head *nh = opp_get_notifier(dev);
> > + struct srcu_notifier_head *nh;
> > + int ret = 0;
> >
> > + rcu_read_lock();
> > + nh = opp_get_notifier(dev);
> > if (IS_ERR(nh))
> > - return PTR_ERR(nh);
> > - return srcu_notifier_chain_unregister(nh, &devfreq->nb);
> > + ret = PTR_ERR(nh);
> > + rcu_read_unlock();
> > + if (!ret)
> > + ret = srcu_notifier_chain_unregister(nh, &devfreq->nb);
> > +
> > + return ret;
> > }
> >
> > MODULE_AUTHOR("MyungJoo Ham <myungjoo.ham@samsung.com>");
> > --
> > 1.7.5.4
^ permalink raw reply
* Re: [PATCH v3] cpufreq: ondemand: handle SW coordinated CPUs
From: Fabio Baltieri @ 2012-11-22 17:02 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: cpufreq, linux-pm, Rickard Andersson, Vincent Guittot,
Linus Walleij, Lee Jones, linux-kernel
In-Reply-To: <1708590.LLn32jdgxe@vostro.rjw.lan>
Hello Rafael,
thanks for the review! I only have one concern before sending a v4:
On Thu, Nov 22, 2012 at 01:10:15AM +0100, Rafael J. Wysocki wrote:
> > @@ -627,32 +659,41 @@ static void do_dbs_timer(struct work_struct *work)
> > delay -= jiffies % delay;
> > }
> > } else {
> > - __cpufreq_driver_target(dbs_info->cur_policy,
> > - dbs_info->freq_lo, CPUFREQ_RELATION_H);
> > + if (sample)
> > + __cpufreq_driver_target(dbs_info->cur_policy,
> > + dbs_info->freq_lo,
> > + CPUFREQ_RELATION_H);
> > delay = dbs_info->freq_lo_jiffies;
> > }
> > - schedule_delayed_work_on(cpu, &dbs_info->work, delay);
> > + schedule_delayed_work_on(smp_processor_id(), dw, delay);
>
> We're not supposed to be using smp_processor_id() any more.
> get_cpu()/put_cpu() should be used instead.
That's going to add preemption protection, do I need that? The function
is called from a kworker with the affinity set on a specific CPU, so it
should not migrate to a different one during execution.
I agree with you for all the other comments.
Thanks,
Fabio
--
Fabio Baltieri
^ permalink raw reply
* Re: [PATCH] PM / devfreq: missing rcu_read_lock() added for find_device_opp()
From: Serge E. Hallyn @ 2012-11-22 14:09 UTC (permalink / raw)
To: MyungJoo Ham
Cc: serge, myungjoo.ham, linux-pm, rjw, keescook, serge.hallyn,
linux-kernel
In-Reply-To: <1353566192-7079-1-git-send-email-myungjoo.ham@samsung.com>
Quoting MyungJoo Ham (myungjoo.ham@samsung.com):
> opp_get_notifier() uses find_device_opp(), which requires to
> held rcu_read_lock. In order to keep the notifier-header
> valid, we have added rcu_read_lock().
>
Reviewed-by: Serge Hallyn <serge.hallyn@canonical.com>
> Reported-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> ---
> drivers/devfreq/devfreq.c | 26 ++++++++++++++++++++------
> 1 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 1388d46..5275883 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -1023,11 +1023,18 @@ struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq,
> */
> int devfreq_register_opp_notifier(struct device *dev, struct devfreq *devfreq)
> {
> - struct srcu_notifier_head *nh = opp_get_notifier(dev);
> + struct srcu_notifier_head *nh;
> + int ret = 0;
>
> + rcu_read_lock();
> + nh = opp_get_notifier(dev);
> if (IS_ERR(nh))
> - return PTR_ERR(nh);
> - return srcu_notifier_chain_register(nh, &devfreq->nb);
> + ret = PTR_ERR(nh);
> + rcu_read_unlock();
> + if (!ret)
> + ret = srcu_notifier_chain_register(nh, &devfreq->nb);
> +
> + return ret;
> }
>
> /**
> @@ -1042,11 +1049,18 @@ int devfreq_register_opp_notifier(struct device *dev, struct devfreq *devfreq)
> */
> int devfreq_unregister_opp_notifier(struct device *dev, struct devfreq *devfreq)
> {
> - struct srcu_notifier_head *nh = opp_get_notifier(dev);
> + struct srcu_notifier_head *nh;
> + int ret = 0;
>
> + rcu_read_lock();
> + nh = opp_get_notifier(dev);
> if (IS_ERR(nh))
> - return PTR_ERR(nh);
> - return srcu_notifier_chain_unregister(nh, &devfreq->nb);
> + ret = PTR_ERR(nh);
> + rcu_read_unlock();
> + if (!ret)
> + ret = srcu_notifier_chain_unregister(nh, &devfreq->nb);
> +
> + return ret;
> }
>
> MODULE_AUTHOR("MyungJoo Ham <myungjoo.ham@samsung.com>");
> --
> 1.7.5.4
^ permalink raw reply
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Grant Likely @ 2012-11-22 13:39 UTC (permalink / raw)
To: Thierry Reding
Cc: Alexandre Courbot,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Stephen Warren, linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
Mark Brown, Mark Zhang, Rob Herring,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Tomi Valkeinen, Tomi Valkeinen, Laurent Pinchart, Anton Vorontsov,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
David Woodhouse,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
In-Reply-To: <20121121151209.GA4048-RM9K5IK7kjIyiCvfTdI0JKcOhU4Rzj621B7CTYaBSLdn68oJJulU0Q@public.gmane.org>
On Wed, Nov 21, 2012 at 3:12 PM, Thierry Reding
<thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> On Thu, Nov 22, 2012 at 12:02:47AM +0900, Alexandre Courbot wrote:
>> Mmmm so maybe I am misinterpreting things, but it looks like we
>> have just buried the power sequences here, haven't we?
>
> I don't think so. In fact I was just starting to think that maybe for
> Tegra we could have a generic panel driver which used power sequences
> to abstract away the pin differences for powering up the panel. Since
> most likely that will be where the differences are there is a lot of
> potential to factor things out into sequences.
>
> Perhaps what we may want to postpone for now is the DT representation
> since that's what Tomi and Grant seem to be mostly opposed to.
Don't give up. I'm am being cautions and trying to think of all the
implications because that is what I have to do as a maintainer. I'm
not opposed to the feature but I need a good level of confidence that
it has been fully thought out.
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Grant Likely @ 2012-11-22 13:39 UTC (permalink / raw)
To: Alex Courbot
Cc: Alexandre Courbot,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Mark Brown,
Stephen Warren, linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
Mark Zhang, Rob Herring,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Anton Vorontsov,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
David Woodhouse,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
In-Reply-To: <13540495.epaCf4JVn9@percival>
On Wed, 21 Nov 2012 13:23:06 +0900, Alex Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Hi Grant,
>
> On Wednesday 21 November 2012 05:54:29 Grant Likely wrote:
> > > With the advent of the device tree and of ARM kernels that are not
> > > board-tied, we cannot rely on these board-specific hooks anymore but
> >
> > This isn't strictly true. It is still perfectly fine to have board
> > specific code when necessary. However, there is strong encouragement to
> > enable that code in device drivers as much as possible and new board
> > files need to have very strong justification.
>
> But doesn't introducing board-specific code into the kernel just defeats the
> purpose of the DT? If we extend this logic, we are heading straight back to
> board-definition files. To a lesser extent than before I agree, but the problem
> is fundamentally the same.
There is *always* board specific code. There is always something fiddly
enough, complex enough, or just plain ugly enough that it is best
handled in C code. The trick is to make the board-specific stuff the
exception, not the rule.
When you can factor out common behavour (like you are doing here), then
it is a really good candidate for a common binding, but still please
always ask yourself the question is this going to make things better or
worse in the long run.
> > I'm thinking about the scripts as trivial-to-parse ascii strings that
> > have a very simple set of commands. The commands use resources already
> > defined in the node. ie. 'g0' refers to the first entry in the gpios
> > property. 'r0' for the regulator, 'p0' for the pwms, 'd' means delay. By
> > no means take this as the format to use, it is just an example off the
> > top of my head, but it is already way easier to work with than putting
> > each command into a node.
>
> I'd argue that this opens the door wide open towards having a complete
> interpreter in the device tree. At least DT nodes restrict what we can do
> conveniently.
I disagree... see below.
> > The trick is still to define a syntax that doesn't fall apart when it
> > needs to be extended. I would also like to get opinions on whether or
> > not conditionals or loops should be supported (ie. loop waiting for a
> > status to change). If they should then we need to be a lot more careful
> > about the design (and due to my aforementioned nervousness, somebody may
> > need to get me therapy).
>
> That's IMHO the main argument in favor of using DT nodes here (the syntax
> extension, not your therapy).
hahaha :-)
> They can be extended simply by adding properties
> to them, and I was relying on this to make power seqs extensible while keeping
> things consistent (and backward-compatible). For instance, when we want to
> support voltage setting in regulators we can just add a "voltage" property for
> that.
>
> So to sum up the pros of the current node-base representation:
> - give a "data like" representation of powering sequences (what they should
> be, actually)
> - puts sanity barriers for not turning power seqs into a real code interpreter
> - easily extensible
>
> There are probably a few cons, the numbering of steps being one, but at least
> we don't need to design a new DSL and care too much about extensibility which
> is, in the nodes case, built-in and free.
No matter how it is encoded, this *IS* a new DSL. Using nodes and
properties doesn't change that. Extensibility is no more built-in and
free with nodes that it is with another encoding. If there aren't clear
guidelines on how to extend it then we'll get weird stuff. For example,
even with the nodes case someone might do this:
step3 {
type = "loop";
count = 10;
step1 {
type = "gpio";
gpio = <&gpio 1>;
value = 1;
};
step2 {
type = "delay";
value = 10000;
};
step3 {
type = "gpio";
gpio = <&gpio 1>;
value = 0;
};
step4 {
type = "delay";
value = 10000;
};
};
And even while cringing as I type the above, I also have to consider
that looping may just be a valid use case for sequences.
And even here, a very simple sequence fragment required 22 lines of
code.
Next, I'm concerned about where these will show up. Say for instance
there needs to be a power sequence added to an spi bus node. Already spi
bus child nodes have a defined meaning; they are spi slaves. How then
should the sequence be attached to the spi bus?
> If that makes you feel better, maybe we can try and fix what is wrong with the
> current bindings instead of introducing a new language that will be, by
> nature, more complex to handle and difficult to extend without breaking things?
Okay, here are 3 concrete objections with the proposed binding:
- The syntax concerns of it being too verbose and it effectively uses
line numbers for ordering (Do you remember fighting with BASIC?).
- There are many devices that won't be able to use the binding because
they already have a meaning for child nodes
- I think resources should be declared separate from the sequence based
on the assumption that multiple steps will be operating on the same
resource.
I do think that each sequence should be contained within a single
property, but I'm open to other suggestions.
g.
^ permalink raw reply
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Grant Likely @ 2012-11-22 13:01 UTC (permalink / raw)
To: Alex Courbot
Cc: Alexandre Courbot,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Stephen Warren, linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
Mark Brown, Mark Zhang, Rob Herring,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Tomi Valkeinen, Anton Vorontsov,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
David Woodhouse,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
In-Reply-To: <4275468.QAmZy3xUK6@percival>
On Wed, Nov 21, 2012 at 10:00 AM, Alex Courbot <acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> On Wednesday 21 November 2012 16:48:45 Tomi Valkeinen wrote:
>> If the power-off sequence disables a regulator that was supposed to be
>> enabled by the power-on sequence (but wasn't enabled because of an
>> error), the regulator_disable is still called when the driver runs the
>> power-off sequence, isn't it? Regulator enables and disables are ref
>> counted, and the enables should match the disables.
>
> And there collapses my theory.
>
>> > Failures might be better handled if sequences have some "recovery policy"
>> > about what to do when they fail, as mentioned in the link above. As you
>> > pointed out, the driver might not always know enough about the resources
>> > involved to do the right thing.
>>
>> Yes, I think such recovery policy would be needed.
>
> Indeed, from your last paragraph this makes even more sense now.
>
> Oh, and I noticed I forgot to reply to this:
>
>> This I didn't understand. Doesn't "<&pwm 2 xyz>" point to a single
>> device, no matter where and how many times it's used?
>
> That's true - however when dereferencing the phandle, the underlying framework
> will try to acquire the PWM, which will result in failure if the same resource
> is referenced several times.
>
> One could compare the phandles to avoid this, but in your example you must
> know that for PWMs the "xyz" part is not relevant for comparison.
>
> This makes referencing of resources by name much easier to implement and more
> elegant with respect to frameworks leveraging.
I would rather (at least for how the DT bindings settle out) see the
design separate the resource references from the sequence. ie. Declare
which resources are used by a device's sequences all in one place and
have the commands index into that.
g.
^ permalink raw reply
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Alexandre Courbot @ 2012-11-22 9:55 UTC (permalink / raw)
To: Linus Walleij
Cc: Ola LILJA2, linux-fbdev@vger.kernel.org, Mark Brown,
Stephen Warren, Arnd Bergmann, linux-pm@vger.kernel.org,
devicetree-discuss@lists.ozlabs.org, Thierry Reding, Mark Zhang,
Rob Herring, Linux Kernel Mailing List, Grant Likely,
Anton Vorontsov, linux-tegra@vger.kernel.org, David Woodhouse,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <CACRpkdaNmBpfONDpt7zFqLaqfiGm+ELpO-v5gZmM0rEi_AzijQ@mail.gmail.com>
On Thu, Nov 22, 2012 at 5:57 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> I have the same (limited by experience) opinion. Working sort of
> near som audio drivers I have understood that power sequencing
> is a big issue not only for things like this backlight example, but
> even more so in the area of audio to avoid clicks and pops.
>
> Is it correct to assume that this library will be useful also for ALSA
> SoC type of devices?
There should be nothing against that - provided ALSA's needs remain
similar to the current set of actions (regulators, gpios, ...) it
should be perfectly usable. Can you think of any usage that involves
more than dumb regulator/gpio manipulation?
If power seqs are to be used this way (as opposed to through the DT),
it will become urgent to implement that gpio handlers library we were
talking about in another thread, as we don't want to have GPIO numbers
hard-coded into the sequences.
Alex.
^ permalink raw reply
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Linus Walleij @ 2012-11-22 8:57 UTC (permalink / raw)
To: Mark Brown
Cc: Alexandre Courbot, linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
Stephen Warren, linux-pm-u79uwXL29TY76Z2rM5mHXA,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Mark Zhang,
Rob Herring, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Anton Vorontsov,
linux-tegra-u79uwXL29TY76Z2rM5mHXA, Ola LILJA2, David Woodhouse,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <20121121013133.GE4673-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
On Wed, Nov 21, 2012 at 2:31 AM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> As I said in my earlier reviews I think this is a useful thing to have
> as a utility library for drivers independantly of the DT bindings, it
> would allow drivers to become more data driven. Perhaps we can rework
> the series so that the DT bindings are added as a final patch? All the
> rest of the code seems OK.
I have the same (limited by experience) opinion. Working sort of
near som audio drivers I have understood that power sequencing
is a big issue not only for things like this backlight example, but
even more so in the area of audio to avoid clicks and pops.
Is it correct to assume that this library will be useful also for ALSA
SoC type of devices?
Then whether to couple that to DT or ACPI or SFI or whatever
is a different discussion altogether.
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH] PM / devfreq: missing rcu_read_lock() added for find_device_opp()
From: MyungJoo Ham @ 2012-11-22 6:36 UTC (permalink / raw)
To: serge; +Cc: myungjoo.ham, linux-pm, rjw, keescook, serge.hallyn, linux-kernel
In-Reply-To: <20121121134621.GA30575@mail.hallyn.com>
opp_get_notifier() uses find_device_opp(), which requires to
held rcu_read_lock. In order to keep the notifier-header
valid, we have added rcu_read_lock().
Reported-by: Kees Cook <keescook@chromium.org>
Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
---
drivers/devfreq/devfreq.c | 26 ++++++++++++++++++++------
1 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 1388d46..5275883 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -1023,11 +1023,18 @@ struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq,
*/
int devfreq_register_opp_notifier(struct device *dev, struct devfreq *devfreq)
{
- struct srcu_notifier_head *nh = opp_get_notifier(dev);
+ struct srcu_notifier_head *nh;
+ int ret = 0;
+ rcu_read_lock();
+ nh = opp_get_notifier(dev);
if (IS_ERR(nh))
- return PTR_ERR(nh);
- return srcu_notifier_chain_register(nh, &devfreq->nb);
+ ret = PTR_ERR(nh);
+ rcu_read_unlock();
+ if (!ret)
+ ret = srcu_notifier_chain_register(nh, &devfreq->nb);
+
+ return ret;
}
/**
@@ -1042,11 +1049,18 @@ int devfreq_register_opp_notifier(struct device *dev, struct devfreq *devfreq)
*/
int devfreq_unregister_opp_notifier(struct device *dev, struct devfreq *devfreq)
{
- struct srcu_notifier_head *nh = opp_get_notifier(dev);
+ struct srcu_notifier_head *nh;
+ int ret = 0;
+ rcu_read_lock();
+ nh = opp_get_notifier(dev);
if (IS_ERR(nh))
- return PTR_ERR(nh);
- return srcu_notifier_chain_unregister(nh, &devfreq->nb);
+ ret = PTR_ERR(nh);
+ rcu_read_unlock();
+ if (!ret)
+ ret = srcu_notifier_chain_unregister(nh, &devfreq->nb);
+
+ return ret;
}
MODULE_AUTHOR("MyungJoo Ham <myungjoo.ham@samsung.com>");
--
1.7.5.4
^ permalink raw reply related
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Alexandre Courbot @ 2012-11-22 3:09 UTC (permalink / raw)
To: Mark Brown
Cc: linux-fbdev@vger.kernel.org, Stephen Warren, Arnd Bergmann,
linux-pm@vger.kernel.org, devicetree-discuss@lists.ozlabs.org,
Thierry Reding, Mark Zhang, Rob Herring,
linux-kernel@vger.kernel.org, Grant Likely, Tomi Valkeinen,
Tomi Valkeinen, Laurent Pinchart, Anton Vorontsov,
linux-tegra@vger.kernel.org, David Woodhouse,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20121122020649.GH4371@opensource.wolfsonmicro.com>
On Thu, Nov 22, 2012 at 11:06 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Nov 22, 2012 at 11:01:34AM +0900, Alexandre Courbot wrote:
>
>> The thing I don't understand here is why would anyone want power
>> sequences without the DT representation. Guys, that's the whole point! :)
>
>> If we are to implement things into drivers, then callback functions
>> are going to serve us just as well - even better, for they are more
>> flexible. All we need to do is define a dedicated ops structure and
>> have the driver plug the right callback functions depending on the
>> "compatible" property of the DT device node. We don't need a framework
>> for that.
>
> It allows drivers (both board drivers and actual drivers) to write these
> things in a semi-scripted form instead of having to open code everything
> each time, it'd save a bunch of tedious stuff with resource requesting
> for example.
Mmm, I overlooked that point - that's fair enough. Guess I should
remove all DT support and stress that point in the documentation. Then
maybe we'll have a deal.
Alex.
^ permalink raw reply
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Mark Brown @ 2012-11-22 2:06 UTC (permalink / raw)
To: Alexandre Courbot
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Stephen Warren, linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
Mark Zhang, Rob Herring,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Tomi Valkeinen, Tomi Valkeinen, Laurent Pinchart, Anton Vorontsov,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
David Woodhouse,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
In-Reply-To: <CAAVeFuK_V6nb874bygioi35mFNxaWuveKOedLT=YYsP4JfVqgA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 807 bytes --]
On Thu, Nov 22, 2012 at 11:01:34AM +0900, Alexandre Courbot wrote:
> The thing I don't understand here is why would anyone want power
> sequences without the DT representation. Guys, that's the whole point! :)
> If we are to implement things into drivers, then callback functions
> are going to serve us just as well - even better, for they are more
> flexible. All we need to do is define a dedicated ops structure and
> have the driver plug the right callback functions depending on the
> "compatible" property of the DT device node. We don't need a framework
> for that.
It allows drivers (both board drivers and actual drivers) to write these
things in a semi-scripted form instead of having to open code everything
each time, it'd save a bunch of tedious stuff with resource requesting
for example.
[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 192 bytes --]
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
^ permalink raw reply
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Alexandre Courbot @ 2012-11-22 2:01 UTC (permalink / raw)
To: Thierry Reding
Cc: linux-fbdev@vger.kernel.org, Stephen Warren, Arnd Bergmann,
linux-pm@vger.kernel.org, devicetree-discuss@lists.ozlabs.org,
Mark Brown, Mark Zhang, Rob Herring, linux-kernel@vger.kernel.org,
Grant Likely, Tomi Valkeinen, Tomi Valkeinen, Laurent Pinchart,
Anton Vorontsov, linux-tegra@vger.kernel.org, David Woodhouse,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20121121151209.GA4048@avionic-0098.adnet.avionic-design.de>
On Thu, Nov 22, 2012 at 12:12 AM, Thierry Reding
<thierry.reding@avionic-design.de> wrote:
> On Thu, Nov 22, 2012 at 12:02:47AM +0900, Alexandre Courbot wrote:
>> Mmmm so maybe I am misinterpreting things, but it looks like we
>> have just buried the power sequences here, haven't we?
>
> I don't think so. In fact I was just starting to think that maybe for
> Tegra we could have a generic panel driver which used power sequences
> to abstract away the pin differences for powering up the panel. Since
> most likely that will be where the differences are there is a lot of
> potential to factor things out into sequences.
>
> Perhaps what we may want to postpone for now is the DT representation
> since that's what Tomi and Grant seem to be mostly opposed to.
The thing I don't understand here is why would anyone want power
sequences without the DT representation. Guys, that's the whole point! :)
If we are to implement things into drivers, then callback functions
are going to serve us just as well - even better, for they are more
flexible. All we need to do is define a dedicated ops structure and
have the driver plug the right callback functions depending on the
"compatible" property of the DT device node. We don't need a framework
for that.
And if we are not going to use power seqs that's probably what we
should do in order to get panels to work on Tegra boards for now. Then
see how things turn out with the panel framework and whether there is
a use for power seqs in it. But by the meantime, I don't see any
motivation to merge power seqs sans DT support.
Alex.
^ permalink raw reply
* [PATCH linux-next] cpuidle: fix a suspicious RCU usage in menu governor
From: Li Zhong @ 2012-11-22 1:45 UTC (permalink / raw)
To: linux-next list, linux-pm; +Cc: riel, youquan.song, rafael.j.wysocki, paulmck
Adding linux-pm list, and ACKs.
I saw this suspicious RCU usage on the next tree of 11/15
[ 67.123404] ===============================
[ 67.123413] [ INFO: suspicious RCU usage. ]
[ 67.123423] 3.7.0-rc5-next-20121115-dirty #1 Not tainted
[ 67.123434] -------------------------------
[ 67.123444] include/trace/events/timer.h:186 suspicious rcu_dereference_check() usage!
[ 67.123458]
[ 67.123458] other info that might help us debug this:
[ 67.123458]
[ 67.123474]
[ 67.123474] RCU used illegally from idle CPU!
[ 67.123474] rcu_scheduler_active = 1, debug_locks = 0
[ 67.123493] RCU used illegally from extended quiescent state!
[ 67.123507] 1 lock held by swapper/1/0:
[ 67.123516] #0: (&cpu_base->lock){-.-...}, at: [<c0000000000979b0>] .__hrtimer_start_range_ns+0x28c/0x524
[ 67.123555]
[ 67.123555] stack backtrace:
[ 67.123566] Call Trace:
[ 67.123576] [c0000001e2ccb920] [c00000000001275c] .show_stack+0x78/0x184 (unreliable)
[ 67.123599] [c0000001e2ccb9d0] [c0000000000c15a0] .lockdep_rcu_suspicious+0x120/0x148
[ 67.123619] [c0000001e2ccba70] [c00000000009601c] .enqueue_hrtimer+0x1c0/0x1c8
[ 67.123639] [c0000001e2ccbb00] [c000000000097aa0] .__hrtimer_start_range_ns+0x37c/0x524
[ 67.123660] [c0000001e2ccbc20] [c0000000005c9698] .menu_select+0x508/0x5bc
[ 67.123678] [c0000001e2ccbd20] [c0000000005c740c] .cpuidle_idle_call+0xa8/0x6e4
[ 67.123699] [c0000001e2ccbdd0] [c0000000000459a0] .pSeries_idle+0x10/0x34
[ 67.123717] [c0000001e2ccbe40] [c000000000014dc8] .cpu_idle+0x130/0x280
[ 67.123738] [c0000001e2ccbee0] [c0000000006ffa8c] .start_secondary+0x378/0x384
[ 67.123758] [c0000001e2ccbf90] [c00000000000936c] .start_secondary_prolog+0x10/0x14
hrtimer_start was added in 198fd638 and ae515197. The patch below tries
to use RCU_NONIDLE around it to avoid the above report.
Signed-off-by: Li Zhong <zhong@linux.vnet.ibm.com>
Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Reviewed-by: Rik van Riel <riel@redhat.com>
---
| 10 ++++++----
1 files changed, 6 insertions(+), 4 deletions(-)
--git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index 2efee27..bd40b94 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -407,8 +407,9 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
perfect_us = perfect_cstate_ms * 1000;
if (repeat && (4 * timer_us < data->expected_us)) {
- hrtimer_start(hrtmr, ns_to_ktime(1000 * timer_us),
- HRTIMER_MODE_REL_PINNED);
+ RCU_NONIDLE(hrtimer_start(hrtmr,
+ ns_to_ktime(1000 * timer_us),
+ HRTIMER_MODE_REL_PINNED));
/* In repeat case, menu hrtimer is started */
per_cpu(hrtimer_status, cpu) = MENU_HRTIMER_REPEAT;
} else if (perfect_us < data->expected_us) {
@@ -418,8 +419,9 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
* In that case, it makes sense to re-enter
* into a deeper C-state after some time.
*/
- hrtimer_start(hrtmr, ns_to_ktime(1000 * timer_us),
- HRTIMER_MODE_REL_PINNED);
+ RCU_NONIDLE(hrtimer_start(hrtmr,
+ ns_to_ktime(1000 * timer_us),
+ HRTIMER_MODE_REL_PINNED));
/* In general case, menu hrtimer is started */
per_cpu(hrtimer_status, cpu) = MENU_HRTIMER_GENERAL;
}
--
1.7.1
^ permalink raw reply related
* Re: [PATCH v3] cpufreq: ondemand: handle SW coordinated CPUs
From: Rafael J. Wysocki @ 2012-11-22 0:10 UTC (permalink / raw)
To: Fabio Baltieri
Cc: cpufreq, linux-pm, Rickard Andersson, Vincent Guittot,
Linus Walleij, Lee Jones, linux-kernel
In-Reply-To: <1353413176-21723-2-git-send-email-fabio.baltieri@linaro.org>
On Tuesday, November 20, 2012 01:06:16 PM Fabio Baltieri wrote:
> From: Rickard Andersson <rickard.andersson@stericsson.com>
>
> This patch fixes a bug that occurred when we had load on a secondary CPU
> and the primary CPU was sleeping. Only one sampling timer was spawned
> and it was spawned as a deferred timer on the primary CPU, so when a
> secondary CPU had a change in load this was not detected by the ondemand
> governor.
>
> This patch make sure that deferred timers are run on all CPUs in the
> case of software controlled CPUs that run on the same frequency.
While I basically don't have problems with the functionality of this,
I have some with the code organization.
> Signed-off-by: Rickard Andersson <rickard.andersson@stericsson.com>
> Signed-off-by: Fabio Baltieri <fabio.baltieri@linaro.org>
> ---
> drivers/cpufreq/cpufreq_ondemand.c | 141 ++++++++++++++++++++++++++++++++-----
> 1 file changed, 122 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c
> index 396322f..430f614 100644
> --- a/drivers/cpufreq/cpufreq_ondemand.c
> +++ b/drivers/cpufreq/cpufreq_ondemand.c
> @@ -93,6 +93,7 @@ struct cpu_dbs_info_s {
> * when user is changing the governor or limits.
> */
> struct mutex timer_mutex;
> + ktime_t time_stamp;
> };
> static DEFINE_PER_CPU(struct cpu_dbs_info_s, od_cpu_dbs_info);
>
> @@ -285,7 +286,7 @@ static void update_sampling_rate(unsigned int new_rate)
> policy = cpufreq_cpu_get(cpu);
> if (!policy)
> continue;
> - dbs_info = &per_cpu(od_cpu_dbs_info, policy->cpu);
> + dbs_info = &per_cpu(od_cpu_dbs_info, cpu);
> cpufreq_cpu_put(policy);
>
> mutex_lock(&dbs_info->timer_mutex);
> @@ -305,7 +306,7 @@ static void update_sampling_rate(unsigned int new_rate)
> cancel_delayed_work_sync(&dbs_info->work);
> mutex_lock(&dbs_info->timer_mutex);
>
> - schedule_delayed_work_on(dbs_info->cpu, &dbs_info->work,
> + schedule_delayed_work_on(cpu, &dbs_info->work,
> usecs_to_jiffies(new_rate));
>
> }
The above two changes don't belong to this patch. Please send a separate patch
with them and a matching description in the changelog.
> @@ -449,6 +450,16 @@ static struct attribute_group dbs_attr_group = {
>
> /************************** sysfs end ************************/
>
> +static bool dbs_sw_coordinated_cpus(struct cpu_dbs_info_s *dbs_info)
> +{
> + struct cpufreq_policy *policy = dbs_info->cur_policy;
> +
> + if (cpumask_weight(policy->cpus) > 1)
> + return true;
> + else
> + return false;
> +}
return cpumask_weight(policy->cpus) > 1;
pretty please.
> +
> static void dbs_freq_increase(struct cpufreq_policy *p, unsigned int freq)
> {
> if (dbs_tuners_ins.powersave_bias)
> @@ -598,20 +609,41 @@ static void dbs_check_cpu(struct cpu_dbs_info_s *this_dbs_info)
>
> static void do_dbs_timer(struct work_struct *work)
> {
> + struct delayed_work *dw = to_delayed_work(work);
> struct cpu_dbs_info_s *dbs_info =
> container_of(work, struct cpu_dbs_info_s, work.work);
> - unsigned int cpu = dbs_info->cpu;
> - int sample_type = dbs_info->sample_type;
> -
> + int sample_type;
> int delay;
> + bool sample = true;
> +
> + if (dbs_sw_coordinated_cpus(dbs_info)) {
> + ktime_t time_now;
> + s64 delta_us;
> +
> + /* use leader CPU's dbs_info */
> + dbs_info = &per_cpu(od_cpu_dbs_info, dbs_info->cpu);
> + mutex_lock(&dbs_info->timer_mutex);
>
> - mutex_lock(&dbs_info->timer_mutex);
> + time_now = ktime_get();
> + delta_us = ktime_us_delta(time_now, dbs_info->time_stamp);
> +
> + /* Do nothing if we recently have sampled */
> + if (delta_us < (s64)(dbs_tuners_ins.sampling_rate / 2))
> + sample = false;
> + else
> + dbs_info->time_stamp = time_now;
> + } else {
> + mutex_lock(&dbs_info->timer_mutex);
> + }
Please don't handle locking this way. Instead, please move the code you'll
run under the lock in both cases into a separate function (it may take "sample"
as an argument along with dbs_info) and call it between
mutex_lock() and mutex_unlock() in each block.
In addition to that, I'd move the whole block executed when
dbs_sw_coordinated_cpus(dbs_info) is true into a separate function where
dbs_info would be a local variable. This way it wouldn't mix two distinct
cases in the same piece of code that's remarkably hard to follow.
> +
> + sample_type = dbs_info->sample_type;
>
> /* Common NORMAL_SAMPLE setup */
> dbs_info->sample_type = DBS_NORMAL_SAMPLE;
> if (!dbs_tuners_ins.powersave_bias ||
> sample_type == DBS_NORMAL_SAMPLE) {
> - dbs_check_cpu(dbs_info);
> + if (sample)
> + dbs_check_cpu(dbs_info);
> if (dbs_info->freq_lo) {
> /* Setup timer for SUB_SAMPLE */
> dbs_info->sample_type = DBS_SUB_SAMPLE;
> @@ -627,32 +659,41 @@ static void do_dbs_timer(struct work_struct *work)
> delay -= jiffies % delay;
> }
> } else {
> - __cpufreq_driver_target(dbs_info->cur_policy,
> - dbs_info->freq_lo, CPUFREQ_RELATION_H);
> + if (sample)
> + __cpufreq_driver_target(dbs_info->cur_policy,
> + dbs_info->freq_lo,
> + CPUFREQ_RELATION_H);
> delay = dbs_info->freq_lo_jiffies;
> }
> - schedule_delayed_work_on(cpu, &dbs_info->work, delay);
> + schedule_delayed_work_on(smp_processor_id(), dw, delay);
We're not supposed to be using smp_processor_id() any more.
get_cpu()/put_cpu() should be used instead.
> mutex_unlock(&dbs_info->timer_mutex);
> }
>
> -static inline void dbs_timer_init(struct cpu_dbs_info_s *dbs_info)
> +static inline void dbs_timer_init(struct cpu_dbs_info_s *dbs_info, int cpu)
> {
> /* We want all CPUs to do sampling nearly on same jiffy */
> int delay = usecs_to_jiffies(dbs_tuners_ins.sampling_rate);
> + struct cpu_dbs_info_s *dbs_info_local = &per_cpu(od_cpu_dbs_info, cpu);
>
> if (num_online_cpus() > 1)
> delay -= jiffies % delay;
>
> + cancel_delayed_work_sync(&dbs_info_local->work);
> dbs_info->sample_type = DBS_NORMAL_SAMPLE;
> - INIT_DEFERRABLE_WORK(&dbs_info->work, do_dbs_timer);
> - schedule_delayed_work_on(dbs_info->cpu, &dbs_info->work, delay);
> + schedule_delayed_work_on(cpu, &dbs_info_local->work, delay);
> }
>
> -static inline void dbs_timer_exit(struct cpu_dbs_info_s *dbs_info)
> +static inline void dbs_timer_exit(int cpu)
> {
> + struct cpu_dbs_info_s *dbs_info = &per_cpu(od_cpu_dbs_info, cpu);
> cancel_delayed_work_sync(&dbs_info->work);
> }
>
> +static void dbs_timer_exit_per_cpu(struct work_struct *dummy)
> +{
> + dbs_timer_exit(smp_processor_id());
> +}
> +
> /*
> * Not all CPUs want IO time to be accounted as busy; this dependson how
> * efficient idling at a higher frequency/voltage is.
> @@ -676,6 +717,43 @@ static int should_io_be_busy(void)
> return 0;
> }
>
> +static int __cpuinit cpu_callback(struct notifier_block *nfb,
> + unsigned long action, void *hcpu)
> +{
> + unsigned int cpu = (unsigned long)hcpu;
> + struct device *cpu_dev;
> + struct cpu_dbs_info_s *dbs_info;
> +
> + dbs_info = &per_cpu(od_cpu_dbs_info, cpu);
> +
> + /* use leader CPU's dbs_info */
> + if (dbs_sw_coordinated_cpus(dbs_info))
> + dbs_info = &per_cpu(od_cpu_dbs_info, dbs_info->cpu);
> +
> + cpu_dev = get_cpu_device(cpu);
> + if (cpu_dev) {
> + switch (action) {
> + case CPU_ONLINE:
> + case CPU_ONLINE_FROZEN:
> + dbs_timer_init(dbs_info, cpu);
> + break;
> + case CPU_DOWN_PREPARE:
> + case CPU_DOWN_PREPARE_FROZEN:
> + dbs_timer_exit(cpu);
> + break;
> + case CPU_DOWN_FAILED:
> + case CPU_DOWN_FAILED_FROZEN:
> + dbs_timer_init(dbs_info, cpu);
Why don't you merge this with the CPU_ONLINE* cases?
> + break;
> + }
> + }
> + return NOTIFY_OK;
> +}
> +
> +static struct notifier_block __refdata ondemand_cpu_notifier = {
> + .notifier_call = cpu_callback,
> +};
> +
> static int cpufreq_governor_dbs(struct cpufreq_policy *policy,
> unsigned int event)
> {
> @@ -704,9 +782,13 @@ static int cpufreq_governor_dbs(struct cpufreq_policy *policy,
> if (dbs_tuners_ins.ignore_nice)
> j_dbs_info->prev_cpu_nice =
> kcpustat_cpu(j).cpustat[CPUTIME_NICE];
> +
> + mutex_init(&j_dbs_info->timer_mutex);
> + INIT_DEFERRABLE_WORK(&j_dbs_info->work, do_dbs_timer);
> +
> + j_dbs_info->rate_mult = 1;
> }
> this_dbs_info->cpu = cpu;
> - this_dbs_info->rate_mult = 1;
> ondemand_powersave_bias_init_cpu(cpu);
> /*
> * Start the timerschedule work, when this governor
> @@ -736,21 +818,42 @@ static int cpufreq_governor_dbs(struct cpufreq_policy *policy,
> }
> mutex_unlock(&dbs_mutex);
>
> - mutex_init(&this_dbs_info->timer_mutex);
> - dbs_timer_init(this_dbs_info);
> + /* If SW coordinated CPUs then register notifier */
> + if (dbs_sw_coordinated_cpus(this_dbs_info)) {
> + register_hotcpu_notifier(&ondemand_cpu_notifier);
> +
> + /* Initiate timer time stamp */
> + this_dbs_info->time_stamp = ktime_get();
> +
> + for_each_cpu(j, policy->cpus) {
> + struct cpu_dbs_info_s *j_dbs_info;
> +
> + j_dbs_info = &per_cpu(od_cpu_dbs_info, cpu);
> + dbs_timer_init(j_dbs_info, j);
> + }
> + } else {
> + dbs_timer_init(this_dbs_info, cpu);
> + }
> break;
>
> case CPUFREQ_GOV_STOP:
> - dbs_timer_exit(this_dbs_info);
> + dbs_timer_exit(cpu);
>
> mutex_lock(&dbs_mutex);
> mutex_destroy(&this_dbs_info->timer_mutex);
> dbs_enable--;
> mutex_unlock(&dbs_mutex);
> - if (!dbs_enable)
> + if (!dbs_enable) {
> sysfs_remove_group(cpufreq_global_kobject,
> &dbs_attr_group);
>
> + if (dbs_sw_coordinated_cpus(this_dbs_info)) {
> + /* Make sure all pending timers/works are
> + * stopped. */
> + schedule_on_each_cpu(dbs_timer_exit_per_cpu);
> + unregister_hotcpu_notifier(&ondemand_cpu_notifier);
> + }
> + }
> break;
>
> case CPUFREQ_GOV_LIMITS:
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH] EXYNOS: Use static for functions used in only this file
From: Rafael J. Wysocki @ 2012-11-21 23:42 UTC (permalink / raw)
To: Tushar Behera; +Cc: linux-pm, cpufreq, patches
In-Reply-To: <1353391154-32524-1-git-send-email-tushar.behera@linaro.org>
On Tuesday, November 20, 2012 11:29:14 AM Tushar Behera wrote:
> Fixes following sparse error.
> drivers/cpufreq/exynos-cpufreq.c:34:5: warning: symbol
> 'exynos_verify_speed' was not declared. Should it be static?
> drivers/cpufreq/exynos-cpufreq.c:40:14: warning: symbol
> 'exynos_getspeed' was not declared. Should it be static?
Applied to linux-pm.git/linux-next as v3.8 material.
Thanks,
Rafael
> Signed-off-by: Tushar Behera <tushar.behera@linaro.org>
> ---
> drivers/cpufreq/exynos-cpufreq.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/exynos-cpufreq.c b/drivers/cpufreq/exynos-cpufreq.c
> index af2d81e..eb057bf 100644
> --- a/drivers/cpufreq/exynos-cpufreq.c
> +++ b/drivers/cpufreq/exynos-cpufreq.c
> @@ -31,13 +31,13 @@ static unsigned int locking_frequency;
> static bool frequency_locked;
> static DEFINE_MUTEX(cpufreq_lock);
>
> -int exynos_verify_speed(struct cpufreq_policy *policy)
> +static int exynos_verify_speed(struct cpufreq_policy *policy)
> {
> return cpufreq_frequency_table_verify(policy,
> exynos_info->freq_table);
> }
>
> -unsigned int exynos_getspeed(unsigned int cpu)
> +static unsigned int exynos_getspeed(unsigned int cpu)
> {
> return clk_get_rate(exynos_info->cpu_clk) / 1000;
> }
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH] cpufreq: exynos: Broadcast frequency change notifications for all cores
From: Rafael J. Wysocki @ 2012-11-21 23:41 UTC (permalink / raw)
To: Tomasz Figa
Cc: Kukjin Kim, linux-pm, Tomasz Figa, Bartlomiej Zolnierkiewicz,
Kyungmin Park, linux-samsung-soc, linux-arm-kernel,
Marek Szyprowski
In-Reply-To: <3562672.u4N3XIHFEC@flatron>
On Wednesday, November 21, 2012 10:23:02 PM Tomasz Figa wrote:
> Hi Rafael,
>
> On Wednesday 21 of November 2012 21:47:42 Rafael J. Wysocki wrote:
> > On Wednesday, November 21, 2012 02:52:26 PM Tomasz Figa wrote:
> > > On Tuesday 13 of November 2012 10:26:12 Tomasz Figa wrote:
> > > > On Exynos SoCs all cores share the same frequency setting, so
> > > > changing
> > > > frequency of one core will affect rest of cores.
> > > >
> > > > This patch modifies the exynos-cpufreq driver to inform cpufreq core
> > > > about this behavior and broadcast frequency change notifications for
> > > > all cores.
> > > >
> > > > Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> > > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > > > ---
> > > >
> > > > drivers/cpufreq/exynos-cpufreq.c | 7 +++++--
> > > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/cpufreq/exynos-cpufreq.c
> > > > b/drivers/cpufreq/exynos-cpufreq.c index af2d81e..c0d54a8 100644
> > > > --- a/drivers/cpufreq/exynos-cpufreq.c
> > > > +++ b/drivers/cpufreq/exynos-cpufreq.c
> > > > @@ -100,7 +100,8 @@ static int exynos_target(struct cpufreq_policy
> > > > *policy, }
> > > >
> > > > arm_volt = volt_table[index];
> > > >
> > > > - cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> > > > + for_each_cpu(freqs.cpu, policy->cpus)
> > > > + cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> > > >
> > > > /* When the new frequency is higher than current frequency */
> > > > if ((freqs.new > freqs.old) && !safe_arm_volt) {
> > > >
> > > > @@ -115,7 +116,8 @@ static int exynos_target(struct cpufreq_policy
> > > > *policy, if (freqs.new != freqs.old)
> > > >
> > > > exynos_info->set_freq(old_index, index);
> > > >
> > > > - cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> > > > + for_each_cpu(freqs.cpu, policy->cpus)
> > > > + cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> > > >
> > > > /* When the new frequency is lower than current frequency */
> > > > if ((freqs.new < freqs.old) ||
> > > >
> > > > @@ -235,6 +237,7 @@ static int exynos_cpufreq_cpu_init(struct
> > > > cpufreq_policy *policy) cpumask_copy(policy->related_cpus,
> > > > cpu_possible_mask);
> > > >
> > > > cpumask_copy(policy->cpus, cpu_online_mask);
> > > >
> > > > } else {
> > > >
> > > > + policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
> > > >
> > > > cpumask_setall(policy->cpus);
> > > >
> > > > }
> > >
> > > Ping.
> >
> > Am I supposed to handle this?
>
> I guess. Would be really nice if you could apply this patch for 3.8.
OK
Applied to linux-pm.git/linux-next as v3.8 material.
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCH] cpufreq: exynos: Broadcast frequency change notifications for all cores
From: Tomasz Figa @ 2012-11-21 21:23 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Kukjin Kim, linux-pm, Tomasz Figa, Bartlomiej Zolnierkiewicz,
Kyungmin Park, linux-samsung-soc, linux-arm-kernel,
Marek Szyprowski
In-Reply-To: <4440154.l7bQzsqNex@vostro.rjw.lan>
Hi Rafael,
On Wednesday 21 of November 2012 21:47:42 Rafael J. Wysocki wrote:
> On Wednesday, November 21, 2012 02:52:26 PM Tomasz Figa wrote:
> > On Tuesday 13 of November 2012 10:26:12 Tomasz Figa wrote:
> > > On Exynos SoCs all cores share the same frequency setting, so
> > > changing
> > > frequency of one core will affect rest of cores.
> > >
> > > This patch modifies the exynos-cpufreq driver to inform cpufreq core
> > > about this behavior and broadcast frequency change notifications for
> > > all cores.
> > >
> > > Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > > ---
> > >
> > > drivers/cpufreq/exynos-cpufreq.c | 7 +++++--
> > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/cpufreq/exynos-cpufreq.c
> > > b/drivers/cpufreq/exynos-cpufreq.c index af2d81e..c0d54a8 100644
> > > --- a/drivers/cpufreq/exynos-cpufreq.c
> > > +++ b/drivers/cpufreq/exynos-cpufreq.c
> > > @@ -100,7 +100,8 @@ static int exynos_target(struct cpufreq_policy
> > > *policy, }
> > >
> > > arm_volt = volt_table[index];
> > >
> > > - cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> > > + for_each_cpu(freqs.cpu, policy->cpus)
> > > + cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> > >
> > > /* When the new frequency is higher than current frequency */
> > > if ((freqs.new > freqs.old) && !safe_arm_volt) {
> > >
> > > @@ -115,7 +116,8 @@ static int exynos_target(struct cpufreq_policy
> > > *policy, if (freqs.new != freqs.old)
> > >
> > > exynos_info->set_freq(old_index, index);
> > >
> > > - cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> > > + for_each_cpu(freqs.cpu, policy->cpus)
> > > + cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> > >
> > > /* When the new frequency is lower than current frequency */
> > > if ((freqs.new < freqs.old) ||
> > >
> > > @@ -235,6 +237,7 @@ static int exynos_cpufreq_cpu_init(struct
> > > cpufreq_policy *policy) cpumask_copy(policy->related_cpus,
> > > cpu_possible_mask);
> > >
> > > cpumask_copy(policy->cpus, cpu_online_mask);
> > >
> > > } else {
> > >
> > > + policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
> > >
> > > cpumask_setall(policy->cpus);
> > >
> > > }
> >
> > Ping.
>
> Am I supposed to handle this?
I guess. Would be really nice if you could apply this patch for 3.8.
Best regards,
Tomasz Figa
^ permalink raw reply
* Re: [PATCH] cpufreq: exynos: Broadcast frequency change notifications for all cores
From: Rafael J. Wysocki @ 2012-11-21 20:47 UTC (permalink / raw)
To: Tomasz Figa
Cc: linux-pm, linux-arm-kernel, linux-samsung-soc, Kyungmin Park,
Kukjin Kim, Marek Szyprowski, Bartlomiej Zolnierkiewicz
In-Reply-To: <1758658.Qt0SyRnJmS@amdc1227>
On Wednesday, November 21, 2012 02:52:26 PM Tomasz Figa wrote:
> On Tuesday 13 of November 2012 10:26:12 Tomasz Figa wrote:
> > On Exynos SoCs all cores share the same frequency setting, so changing
> > frequency of one core will affect rest of cores.
> >
> > This patch modifies the exynos-cpufreq driver to inform cpufreq core
> > about this behavior and broadcast frequency change notifications for all
> > cores.
> >
> > Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> > drivers/cpufreq/exynos-cpufreq.c | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/cpufreq/exynos-cpufreq.c
> > b/drivers/cpufreq/exynos-cpufreq.c index af2d81e..c0d54a8 100644
> > --- a/drivers/cpufreq/exynos-cpufreq.c
> > +++ b/drivers/cpufreq/exynos-cpufreq.c
> > @@ -100,7 +100,8 @@ static int exynos_target(struct cpufreq_policy
> > *policy, }
> > arm_volt = volt_table[index];
> >
> > - cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> > + for_each_cpu(freqs.cpu, policy->cpus)
> > + cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> >
> > /* When the new frequency is higher than current frequency */
> > if ((freqs.new > freqs.old) && !safe_arm_volt) {
> > @@ -115,7 +116,8 @@ static int exynos_target(struct cpufreq_policy
> > *policy, if (freqs.new != freqs.old)
> > exynos_info->set_freq(old_index, index);
> >
> > - cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> > + for_each_cpu(freqs.cpu, policy->cpus)
> > + cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> >
> > /* When the new frequency is lower than current frequency */
> > if ((freqs.new < freqs.old) ||
> > @@ -235,6 +237,7 @@ static int exynos_cpufreq_cpu_init(struct
> > cpufreq_policy *policy) cpumask_copy(policy->related_cpus,
> > cpu_possible_mask);
> > cpumask_copy(policy->cpus, cpu_online_mask);
> > } else {
> > + policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
> > cpumask_setall(policy->cpus);
> > }
>
> Ping.
Am I supposed to handle this?
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PULL REQUEST for Rafael] PM / devfreq: pull request-2 bugfixes
From: Rafael J. Wysocki @ 2012-11-21 20:45 UTC (permalink / raw)
To: MyungJoo Ham; +Cc: linux-pm, sachin.kamat, myungjoo.ham, Serge E. Hallyn
In-Reply-To: <1353494743-4581-1-git-send-email-myungjoo.ham@samsung.com>
On Wednesday, November 21, 2012 07:45:43 PM MyungJoo Ham wrote:
>
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> The following changes since commit aa0745b51987fbaf628ee0943907db177335522d:
>
> Merge branch 'pm-devfreq-next' into linux-next (2012-11-21 01:27:47 +0100)
> , which is HEAD of Rafael's linux-pm.git linux-next branch.
>
> are available in the git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/mzx/devfreq.git tags/pull_req_20121121
>
> MyungJoo Ham (2):
> PM / devfreq: remove compiler error when a governor is module
> PM / devfreq: missing rcu_read_lock() added for find_device_opp()
This one has a comment from Serge that looks like a valid point. Care to address it
and resend or drop the commit in question?
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Grant Likely @ 2012-11-21 16:44 UTC (permalink / raw)
To: Mark Brown
Cc: Alexandre Courbot, Anton Vorontsov, Stephen Warren,
Thierry Reding, Mark Zhang, Rob Herring, David Woodhouse,
Arnd Bergmann, linux-tegra, linux-arm-kernel, linux-kernel,
linux-fbdev, devicetree-discuss, linux-pm, Alexandre Courbot
In-Reply-To: <20121121013133.GE4673@opensource.wolfsonmicro.com>
On Wed, 21 Nov 2012 10:31:34 +0900, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Nov 20, 2012 at 09:54:29PM +0000, Grant Likely wrote:
> > On Sat, 17 Nov 2012 19:55:45 +0900, Alexandre Courbot <acourbot@nvidia.com> wrote:
>
> > > With the advent of the device tree and of ARM kernels that are not
> > > board-tied, we cannot rely on these board-specific hooks anymore but
>
> > This isn't strictly true. It is still perfectly fine to have board
> > specific code when necessary. However, there is strong encouragement to
> > enable that code in device drivers as much as possible and new board
> > files need to have very strong justification.
>
> This isn't the message that's gone over, and even for device drivers
> everyone seems to be taking the whole device tree thing as a move to
> pull all data out of the kernel. In some cases there are some real
> practical advantages to doing this but a lot of the people making these
> changes seem to view having things in DT as a goal in itself.
I know, and I do apologize for my part in not communicating and
shepherding enough. For anyone reading; please be pragmatic on putting
things into the DT. If a binding starts to look really complex, ask
whether or not it helps things.
> > I'm thinking about the scripts as trivial-to-parse ascii strings that
> > have a very simple set of commands. The commands use resources already
> > defined in the node. ie. 'g0' refers to the first entry in the gpios
> > property. 'r0' for the regulator, 'p0' for the pwms, 'd' means delay. By
> > no means take this as the format to use, it is just an example off the
> > top of my head, but it is already way easier to work with than putting
> > each command into a node.
>
> It does appear to have some legibility challenges, especially with using
> the indexes into the arrays of resources. On the other hand the arrays
> should be fairly small.
This is merely an example off the top of my head. I'm in no way wed to
the syntax.
> > > +Platform Data Format
> > > +--------------------
> > > +All relevant data structures for declaring power sequences are located in
> > > +include/linux/power_seq.h.
>
> > Hmm... If sequences are switched to a string instead, then platform_data
> > should also use it. The power sequence data structures can be created at
> > runtime by parsing the string.
>
> Seems like a step backwards to me - why not let people store the more
> parsed version of the data? It's going to be less convenient for users
> on non-DT systems.
>
> As I said in my earlier reviews I think this is a useful thing to have
> as a utility library for drivers independantly of the DT bindings, it
> would allow drivers to become more data driven. Perhaps we can rework
> the series so that the DT bindings are added as a final patch? All the
> rest of the code seems OK.
I'm fine with that if the bindings don't get merged yet.
g.
^ permalink raw reply
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Thierry Reding @ 2012-11-21 15:12 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Tomi Valkeinen, Tomi Valkeinen, Grant Likely, Anton Vorontsov,
Stephen Warren, Mark Zhang, Rob Herring, Mark Brown,
David Woodhouse, Arnd Bergmann, linux-tegra@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-fbdev@vger.kernel.org,
devicetree-discuss@lists.ozlabs.org, linux-pm@vger.kernel.org,
Laurent Pinchart
In-Reply-To: <CAAVeFuJofYwj926=3v0ApBCq_vNhCdC1PnzDc-B3d=LL_L40Uw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 660 bytes --]
On Thu, Nov 22, 2012 at 12:02:47AM +0900, Alexandre Courbot wrote:
> Mmmm so maybe I am misinterpreting things, but it looks like we
> have just buried the power sequences here, haven't we?
I don't think so. In fact I was just starting to think that maybe for
Tegra we could have a generic panel driver which used power sequences
to abstract away the pin differences for powering up the panel. Since
most likely that will be where the differences are there is a lot of
potential to factor things out into sequences.
Perhaps what we may want to postpone for now is the DT representation
since that's what Tomi and Grant seem to be mostly opposed to.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Alexandre Courbot @ 2012-11-21 15:02 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Thierry Reding, Tomi Valkeinen, Grant Likely, Anton Vorontsov,
Stephen Warren, Mark Zhang, Rob Herring, Mark Brown,
David Woodhouse, Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Laurent Pinchart
In-Reply-To: <50ACD7DC.5060405-l0cyMroinI0@public.gmane.org>
Mmmm so maybe I am misinterpreting things, but it looks like we
have just buried the power sequences here, haven't we?
Alex.
On Wed, Nov 21, 2012 at 10:32 PM, Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org> wrote:
> On 2012-11-21 15:00, Thierry Reding wrote:
>> On Wed, Nov 21, 2012 at 02:04:17PM +0200, Tomi Valkeinen wrote:
>>> On 2012-11-21 13:40, Thierry Reding wrote:
>>>> On Wed, Nov 21, 2012 at 01:06:03PM +0200, Tomi Valkeinen wrote:
>>>
>>> (sorry for bouncing back and forth with my private and my @ti addresses.
>>> I can't find an option in thunderbird to only use one sender address,
>>> and I always forget to change it when responding...)
>>>
>>>>> My suggestion would be to go forward with an in-driver solution, and
>>>>> look at the DT based solution later if we are seeing an increasing bloat
>>>>> in the drivers.
>>>>
>>>> Assuming we go with your approach, what's the plan? We're actually
>>>> facing this problem right now for Tegra. Basically we have a DRM driver
>>>> that can drive the panel, but we're still missing a way to hook up the
>>>> backlight and panel enabling code. So we effectively can't support any
>>>> of the LVDS devices out there without this series.
>>>
>>> Could you describe the hardware setup you have related to the LCD and
>>> backlight? Is it a public board with public schematics?
>>
>> I don't think any of the schematics are public. The Tamonten Evaluation
>> Carrier is available publicly from our website and the schematics are
>> available on demand as well. If required I can probably arrange to send
>> you a copy.
>
> No need, I think your answer below is enough.
>
>>> I've understood that you don't have anything special in your board, just
>>> an LCD and a backlight, and the power sequences are related to powering
>>> up the LCD and the backlight, without anything board specific. If so,
>>> there's no need for board specific code, but just improving the panel
>>> and backlight drivers to support the models you use.
>>
>> Correct. Basically we have two GPIOs that each enable the panel or the
>> backlight respectively and one PWM to control the brightness. Are the
>
> The panel GPIO goes to the panel hardware device, and enables the panel?
> And similarly for the PWM GPIO, it goes to the PWM hardware device? Just
> making sure there are no other components involved.
>
>> panel drivers that you refer to those in drivers/video? I'm not sure if
>> adding more ad-hoc drivers there just to move them to a generic
>> framework in the next cycle is a good idea. I'd rather spend some time
>> on helping to get the framework right and have drivers for that instead.
>
> We have panel drivers for omap in drivers/video/omap2/displays/. I'm not
> sure if other platforms have their own versions of panel drivers, but
> probably adding a simple panel driver system for a platform would not be
> too difficult. It could even be quite hardcoded, i.e. embedded directly
> into the display subsystem driver, just to get something working until
> the common panel framework is available.
>
> Yes, I agree it's not good idea to add more platform specific panel
> drivers. But it's unclear when CPF will be merged, so if you need to get
> the panel working now, I don't see a simple ad-hoc driver as too
> horrible. But, of course, I'm not the one making the decision whether to
> merge or not =).
>
>> From what I understand by looking at the OMAP display drivers, they also
>> provide the timings for the displays. Steffen's videomode helpers can be
>> used to represent these easily in DT, but I suppose if all of those per-
>
> Right. Note that I didn't present omap panel drivers as perfect
> examples, just examples =).
>
>> panel specifics are represented in the drivers then that won't be needed
>> anymore either.
>
> Yes, for most panels with just one native mode and nothing else, the
> panel driver can contain the timings.
>
> However, this subject has been discussed earlier a few times. If the
> panel in question doesn't need any special power-on/off sequences, just,
> perhaps, one gpio or such, we could still use DT video modes. This would
> simplify the cases where you have lots of different very simple panels.
>
> Obviously the same questions apply to DT video modes than to the power
> sequences, and while I do think it's better to handle the timings inside
> the driver, I'm not too much against video timings in DT. The reason
> being that the video modes are quite clear, simple and stable data,
> versus the much more complex and open-ended power sequences.
>
>>> I don't see any problem with adding small Tegra specific panel drivers
>>> for the time being, with the intention of converting to common panel
>>> framework when that's available.
>>
>> I can take a look at how such a driver could be implemented, but again,
>
> Don't look at the mainline omap panel drivers, at least not too closely
> =). They contain hackery that will be cleaned up with CPF.
>
> I think there are two methods to implements simple panel drivers:
>
> The hardcoded one, where the display subsystem driver manages a few
> different panel models. This is obviously not very expandable or
> "correct", but should probably work just fine for a few models, until
> CPF is usable.
>
> Something like CPF will have: have the panel device/driver as a platform
> device/driver, which will register itself to the display subsystem. And
> with "itself" I mean some kind of struct panel_entity, with a few ops
> implemented by the panel driver.
>
> Well, this goes a bit out of subject. If you want to discuss panel
> drivers more, please start a new thread. Laurent's upcoming CPF v2
> should give you good ideas what the model will be.
>
> Tomi
>
>
^ permalink raw reply
* Re: [PATCH] cpufreq: exynos: Broadcast frequency change notifications for all cores
From: Tomasz Figa @ 2012-11-21 13:52 UTC (permalink / raw)
To: linux-pm
Cc: linux-arm-kernel, linux-samsung-soc, Kyungmin Park, Kukjin Kim,
Marek Szyprowski, Bartlomiej Zolnierkiewicz, Rafael J. Wysocki
In-Reply-To: <1887364.fvxYYNb7Mm@amdc1227>
On Tuesday 13 of November 2012 10:26:12 Tomasz Figa wrote:
> On Exynos SoCs all cores share the same frequency setting, so changing
> frequency of one core will affect rest of cores.
>
> This patch modifies the exynos-cpufreq driver to inform cpufreq core
> about this behavior and broadcast frequency change notifications for all
> cores.
>
> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> drivers/cpufreq/exynos-cpufreq.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/exynos-cpufreq.c
> b/drivers/cpufreq/exynos-cpufreq.c index af2d81e..c0d54a8 100644
> --- a/drivers/cpufreq/exynos-cpufreq.c
> +++ b/drivers/cpufreq/exynos-cpufreq.c
> @@ -100,7 +100,8 @@ static int exynos_target(struct cpufreq_policy
> *policy, }
> arm_volt = volt_table[index];
>
> - cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
> + for_each_cpu(freqs.cpu, policy->cpus)
> + cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
>
> /* When the new frequency is higher than current frequency */
> if ((freqs.new > freqs.old) && !safe_arm_volt) {
> @@ -115,7 +116,8 @@ static int exynos_target(struct cpufreq_policy
> *policy, if (freqs.new != freqs.old)
> exynos_info->set_freq(old_index, index);
>
> - cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
> + for_each_cpu(freqs.cpu, policy->cpus)
> + cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
>
> /* When the new frequency is lower than current frequency */
> if ((freqs.new < freqs.old) ||
> @@ -235,6 +237,7 @@ static int exynos_cpufreq_cpu_init(struct
> cpufreq_policy *policy) cpumask_copy(policy->related_cpus,
> cpu_possible_mask);
> cpumask_copy(policy->cpus, cpu_online_mask);
> } else {
> + policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
> cpumask_setall(policy->cpus);
> }
Ping.
Best regards,
--
Tomasz Figa
Samsung Poland R&D Center
SW Solution Development, Linux Platform
^ permalink raw reply
* Re: [PATCH] PM / devfreq: missing rcu_read_lock() added for find_device_opp()
From: Serge E. Hallyn @ 2012-11-21 13:46 UTC (permalink / raw)
To: MyungJoo Ham
Cc: linux-pm, rjw, keescook, serge.hallyn, linux-kernel, myungjoo.ham
In-Reply-To: <1353492634-10730-1-git-send-email-myungjoo.ham@samsung.com>
Quoting MyungJoo Ham (myungjoo.ham@samsung.com):
> opp_get_notifier() uses find_device_opp(), which requires to
> held rcu_read_lock. In order to keep the notifier-header
> valid, we have added rcu_read_lock().
>
> Reported-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
> ---
> drivers/devfreq/devfreq.c | 26 ++++++++++++++++++++------
> 1 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> index 45e053e..e91cb22 100644
> --- a/drivers/devfreq/devfreq.c
> +++ b/drivers/devfreq/devfreq.c
> @@ -1023,11 +1023,18 @@ struct opp *devfreq_recommended_opp(struct device *dev, unsigned long *freq,
> */
> int devfreq_register_opp_notifier(struct device *dev, struct devfreq *devfreq)
> {
> - struct srcu_notifier_head *nh = opp_get_notifier(dev);
> + struct srcu_notifier_head *nh;
> + int ret = 0;
>
> + rcu_read_lock();
> + nh = opp_get_notifier(dev);
> if (IS_ERR(nh))
> - return PTR_ERR(nh);
> - return srcu_notifier_chain_register(nh, &devfreq->nb);
> + ret = PTR_ERR(nh);
> + if (!ret)
> + ret = srcu_notifier_chain_register(nh, &devfreq->nb);
Hm, but if I'm seeing right, srcu_notifier_chain_register calls
mutex_lock(), which sleeps, so you can't do that under rcu_read_lock().
-serge
^ permalink raw reply
* Re: [PATCHv9 1/3] Runtime Interpreted Power Sequences
From: Tomi Valkeinen @ 2012-11-21 13:32 UTC (permalink / raw)
To: Thierry Reding
Cc: Tomi Valkeinen, Alex Courbot, Grant Likely, Anton Vorontsov,
Stephen Warren, Mark Zhang, Rob Herring, Mark Brown,
David Woodhouse, Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
linux-pm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Alexandre Courbot, Laurent Pinchart
In-Reply-To: <20121121130039.GA12191-RM9K5IK7kjIyiCvfTdI0JKcOhU4Rzj621B7CTYaBSLdn68oJJulU0Q@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 5456 bytes --]
On 2012-11-21 15:00, Thierry Reding wrote:
> On Wed, Nov 21, 2012 at 02:04:17PM +0200, Tomi Valkeinen wrote:
>> On 2012-11-21 13:40, Thierry Reding wrote:
>>> On Wed, Nov 21, 2012 at 01:06:03PM +0200, Tomi Valkeinen wrote:
>>
>> (sorry for bouncing back and forth with my private and my @ti addresses.
>> I can't find an option in thunderbird to only use one sender address,
>> and I always forget to change it when responding...)
>>
>>>> My suggestion would be to go forward with an in-driver solution, and
>>>> look at the DT based solution later if we are seeing an increasing bloat
>>>> in the drivers.
>>>
>>> Assuming we go with your approach, what's the plan? We're actually
>>> facing this problem right now for Tegra. Basically we have a DRM driver
>>> that can drive the panel, but we're still missing a way to hook up the
>>> backlight and panel enabling code. So we effectively can't support any
>>> of the LVDS devices out there without this series.
>>
>> Could you describe the hardware setup you have related to the LCD and
>> backlight? Is it a public board with public schematics?
>
> I don't think any of the schematics are public. The Tamonten Evaluation
> Carrier is available publicly from our website and the schematics are
> available on demand as well. If required I can probably arrange to send
> you a copy.
No need, I think your answer below is enough.
>> I've understood that you don't have anything special in your board, just
>> an LCD and a backlight, and the power sequences are related to powering
>> up the LCD and the backlight, without anything board specific. If so,
>> there's no need for board specific code, but just improving the panel
>> and backlight drivers to support the models you use.
>
> Correct. Basically we have two GPIOs that each enable the panel or the
> backlight respectively and one PWM to control the brightness. Are the
The panel GPIO goes to the panel hardware device, and enables the panel?
And similarly for the PWM GPIO, it goes to the PWM hardware device? Just
making sure there are no other components involved.
> panel drivers that you refer to those in drivers/video? I'm not sure if
> adding more ad-hoc drivers there just to move them to a generic
> framework in the next cycle is a good idea. I'd rather spend some time
> on helping to get the framework right and have drivers for that instead.
We have panel drivers for omap in drivers/video/omap2/displays/. I'm not
sure if other platforms have their own versions of panel drivers, but
probably adding a simple panel driver system for a platform would not be
too difficult. It could even be quite hardcoded, i.e. embedded directly
into the display subsystem driver, just to get something working until
the common panel framework is available.
Yes, I agree it's not good idea to add more platform specific panel
drivers. But it's unclear when CPF will be merged, so if you need to get
the panel working now, I don't see a simple ad-hoc driver as too
horrible. But, of course, I'm not the one making the decision whether to
merge or not =).
> From what I understand by looking at the OMAP display drivers, they also
> provide the timings for the displays. Steffen's videomode helpers can be
> used to represent these easily in DT, but I suppose if all of those per-
Right. Note that I didn't present omap panel drivers as perfect
examples, just examples =).
> panel specifics are represented in the drivers then that won't be needed
> anymore either.
Yes, for most panels with just one native mode and nothing else, the
panel driver can contain the timings.
However, this subject has been discussed earlier a few times. If the
panel in question doesn't need any special power-on/off sequences, just,
perhaps, one gpio or such, we could still use DT video modes. This would
simplify the cases where you have lots of different very simple panels.
Obviously the same questions apply to DT video modes than to the power
sequences, and while I do think it's better to handle the timings inside
the driver, I'm not too much against video timings in DT. The reason
being that the video modes are quite clear, simple and stable data,
versus the much more complex and open-ended power sequences.
>> I don't see any problem with adding small Tegra specific panel drivers
>> for the time being, with the intention of converting to common panel
>> framework when that's available.
>
> I can take a look at how such a driver could be implemented, but again,
Don't look at the mainline omap panel drivers, at least not too closely
=). They contain hackery that will be cleaned up with CPF.
I think there are two methods to implements simple panel drivers:
The hardcoded one, where the display subsystem driver manages a few
different panel models. This is obviously not very expandable or
"correct", but should probably work just fine for a few models, until
CPF is usable.
Something like CPF will have: have the panel device/driver as a platform
device/driver, which will register itself to the display subsystem. And
with "itself" I mean some kind of struct panel_entity, with a few ops
implemented by the panel driver.
Well, this goes a bit out of subject. If you want to discuss panel
drivers more, please start a new thread. Laurent's upcoming CPF v2
should give you good ideas what the model will be.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox