twitter: linux.kernel - 26 new messages in 17 topics

linux.kernel
http://groups.google.com/group/linux.kernel?hl=en

Today's topics:

* sched: remove SYNC_WAKEUPS feature - 3 messages, 1 author
http://groups.google.com/group/linux.kernel/t/bb2824874ddfedaf?hl=en
* smatch_scripts/whitespace_only.sh - 1 messages, 1 author
http://groups.google.com/group/linux.kernel/t/54309c8685b91349?hl=en
* kbuild: Really don't clean bounds.h and asm-offsets.h - 1 messages, 1 author
http://groups.google.com/group/linux.kernel/t/0eb84fa2e4624cb0?hl=en
* proc: cleanup: remove unused assignments - 2 messages, 2 authors
http://groups.google.com/group/linux.kernel/t/a0570dc2177842ac?hl=en
* ST SPEAr: Added basic header files for SPEAr platform - 6 messages, 2
authors
http://groups.google.com/group/linux.kernel/t/ce805d5c50983fd8?hl=en
* linux-next: build failure after merge of the pcmcia tree - 1 messages, 1
author
http://groups.google.com/group/linux.kernel/t/550261f3edfeadd9?hl=en
* KVM: MMU: Make tdp_enabled a mmu-context parameter - 1 messages, 1 author
http://groups.google.com/group/linux.kernel/t/b442f3f63f0421c3?hl=en
* cpuset,mm: update task's mems_allowed lazily - 2 messages, 2 authors
http://groups.google.com/group/linux.kernel/t/8cf36eef7f96d9f5?hl=en
* kbuild: Do not unnecessarily regenerate modules.builtin - 1 messages, 1
author
http://groups.google.com/group/linux.kernel/t/a2bfa67b9e0dbb06?hl=en
* 2.6.34-rc1: kernel BUG at mm/slab.c:2989! - 1 messages, 1 author
http://groups.google.com/group/linux.kernel/t/affffdf23d5859d0?hl=en
* inotify: Don't leak user struct on inotify release - 1 messages, 1 author
http://groups.google.com/group/linux.kernel/t/2348447991674c09?hl=en
* mmotm boot panic bootmem-avoid-dma32-zone-by-default.patch - 1 messages, 1
author
http://groups.google.com/group/linux.kernel/t/5e704e56a1401f7a?hl=en
* Patch for tracing c states (power_end) on x86 - 1 messages, 1 author
http://groups.google.com/group/linux.kernel/t/91e464354797bdad?hl=en
* x86: remove rdc321x_defs.h - 1 messages, 1 author
http://groups.google.com/group/linux.kernel/t/7a5f3f3567fe68c3?hl=en
* lp3971: Fix BUCK_VOL_CHANGE_SHIFT logic - 1 messages, 1 author
http://groups.google.com/group/linux.kernel/t/e038a561bd808851?hl=en
* davinci: MMC: Pass number of SG segments as platform data - 1 messages, 1
author
http://groups.google.com/group/linux.kernel/t/432793a9cfd7c2de?hl=en
* execve for script don't return ENOEXEC, bug ? - 1 messages, 1 author
http://groups.google.com/group/linux.kernel/t/53d156cf36c58e4f?hl=en

==============================================================================
TOPIC: sched: remove SYNC_WAKEUPS feature
http://groups.google.com/group/linux.kernel/t/bb2824874ddfedaf?hl=en
==============================================================================

== 1 of 3 ==
Date: Thurs, Mar 11 2010 2:10 am
From: Mike Galbraith

sched: remove SYNC_WAKEUPS feature

Sync wakeups are critical functionality with a long history. Remove it, we don't
need the branch or icache footprint.

Signed-off-by: Mike Galbraith <efault@gmx.de>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <new-submission>

---
kernel/sched.c | 3 ---
kernel/sched_features.h | 8 --------
2 files changed, 11 deletions(-)

Index: linux-2.6/kernel/sched.c
===================================================================
--- linux-2.6.orig/kernel/sched.c
+++ linux-2.6/kernel/sched.c
@@ -2369,9 +2369,6 @@ static int try_to_wake_up(struct task_st
unsigned long flags;
struct rq *rq, *orig_rq;

- if (!sched_feat(SYNC_WAKEUPS))
- wake_flags &= ~WF_SYNC;
-
this_cpu = get_cpu();

smp_wmb();
Index: linux-2.6/kernel/sched_features.h
===================================================================
--- linux-2.6.orig/kernel/sched_features.h
+++ linux-2.6/kernel/sched_features.h
@@ -17,14 +17,6 @@ SCHED_FEAT(START_DEBIT, 1)
SCHED_FEAT(WAKEUP_PREEMPT, 1)

/*
- * Use the SYNC wakeup hint, pipes and the likes use this to indicate
- * the remote end is likely to consume the data we just wrote, and
- * therefore has cache benefit from being placed on the same cpu, see
- * also AFFINE_WAKEUPS.
- */
-SCHED_FEAT(SYNC_WAKEUPS, 1)
-
-/*
* Based on load and program behaviour, see if it makes sense to place
* a newly woken task on the same cpu as the task that woke it --
* improve cache locality. Typically used with SYNC wakeups as

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

== 2 of 3 ==
Date: Thurs, Mar 11 2010 2:10 am
From: Mike Galbraith

sched: remove AFFINE_WAKEUPS feature

Disabling affine wakeups is too horrible to contemplate. Remove it.

Signed-off-by: Mike Galbraith <efault@gmx.de>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <new-submission>

---
kernel/sched_fair.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

Index: linux-2.6/kernel/sched_fair.c
===================================================================
--- linux-2.6.orig/kernel/sched_fair.c
+++ linux-2.6/kernel/sched_fair.c
@@ -1433,8 +1433,7 @@ static int select_task_rq_fair(struct ta
int sync = wake_flags & WF_SYNC;

if (sd_flag & SD_BALANCE_WAKE) {
- if (sched_feat(AFFINE_WAKEUPS) &&
- cpumask_test_cpu(cpu, &p->cpus_allowed))
+ if (cpumask_test_cpu(cpu, &p->cpus_allowed))
want_affine = 1;
new_cpu = prev_cpu;
}

== 3 of 3 ==
Date: Thurs, Mar 11 2010 2:10 am
From: Mike Galbraith

sched: remove ASYM_GRAN feature

This features has been enabled for quite a while, after testing showed that
easing preemption for light tasks was harmful to high priority threads.

Remove it.

Signed-off-by: Mike Galbraith <efault@gmx.de>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <new-submission>

---
kernel/sched_fair.c | 28 +++++++++++-----------------
kernel/sched_features.h | 6 ------
2 files changed, 11 insertions(+), 23 deletions(-)

Index: linux-2.6/kernel/sched_fair.c
===================================================================
--- linux-2.6.orig/kernel/sched_fair.c
+++ linux-2.6/kernel/sched_fair.c
@@ -1582,24 +1582,18 @@ wakeup_gran(struct sched_entity *curr, s
/*
* Since its curr running now, convert the gran from real-time
* to virtual-time in his units.
+ *
+ * By using 'se' instead of 'curr' we penalize light tasks, so
+ * they get preempted easier. That is, if 'se' < 'curr' then
+ * the resulting gran will be larger, therefore penalizing the
+ * lighter, if otoh 'se' > 'curr' then the resulting gran will
+ * be smaller, again penalizing the lighter task.
+ *
+ * This is especially important for buddies when the leftmost
+ * task is higher priority than the buddy.
*/
- if (sched_feat(ASYM_GRAN)) {
- /*
- * By using 'se' instead of 'curr' we penalize light tasks, so
- * they get preempted easier. That is, if 'se' < 'curr' then
- * the resulting gran will be larger, therefore penalizing the
- * lighter, if otoh 'se' > 'curr' then the resulting gran will
- * be smaller, again penalizing the lighter task.
- *
- * This is especially important for buddies when the leftmost
- * task is higher priority than the buddy.
- */
- if (unlikely(se->load.weight != NICE_0_LOAD))
- gran = calc_delta_fair(gran, se);
- } else {
- if (unlikely(curr->load.weight != NICE_0_LOAD))
- gran = calc_delta_fair(gran, curr);
- }
+ if (unlikely(se->load.weight != NICE_0_LOAD))
+ gran = calc_delta_fair(gran, se);

return gran;
}
Index: linux-2.6/kernel/sched_features.h
===================================================================
--- linux-2.6.orig/kernel/sched_features.h
+++ linux-2.6/kernel/sched_features.h
@@ -17,12 +17,6 @@ SCHED_FEAT(START_DEBIT, 1)
SCHED_FEAT(WAKEUP_PREEMPT, 1)

/*
- * When converting the wakeup granularity to virtual time, do it such
- * that heavier tasks preempting a lighter task have an edge.
- */
-SCHED_FEAT(ASYM_GRAN, 1)
-
-/*
* Use the SYNC wakeup hint, pipes and the likes use this to indicate
* the remote end is likely to consume the data we just wrote, and
* therefore has cache benefit from being placed on the same cpu, see

==============================================================================
TOPIC: smatch_scripts/whitespace_only.sh
http://groups.google.com/group/linux.kernel/t/54309c8685b91349?hl=en
==============================================================================

== 1 of 1 ==
Date: Thurs, Mar 11 2010 2:10 am
From: Dan Carpenter

On Wed, Mar 10, 2010 at 10:11:49AM -0800, Joe Perches wrote:
> On Wed, 2010-03-10 at 15:38 +0100, Julia Lawall wrote:
> > > > On Wed, 10 Mar 2010, Dan Carpenter wrote:
> > > > > I wrote a script to check that a patch only changes white space.
>
> Perhaps you could use #define __LINE__ 0 to avoid
> any vertical line position comparison checks?
>

I tried something similar to that at first but I found that line
numbers were used in other ways. In the end, I added a -no-lineno to
sparse so it thinks everything is on line 123456.

It might be better to just right a perl script that strips out all the
whitespace and compares the result blobs before and after the patch.
That would handle the #if 0 code as well.

regards,
dan carpenter

> > Maybe checkpatch could be changed to not complain about > 80 characters
> > when the line contains a string?
>
> That's been at least partially done in commit
> 691e669ba8c64d31ac08d87b1751e6acfa3ff65e
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

==============================================================================
TOPIC: kbuild: Really don't clean bounds.h and asm-offsets.h
http://groups.google.com/group/linux.kernel/t/0eb84fa2e4624cb0?hl=en
==============================================================================

== 1 of 1 ==
Date: Thurs, Mar 11 2010 2:20 am
From: Michal Marek

Commit 7d3cc8b tried to keep bounds.h and asm-offsets.h during make
clean by filtering these out of $(clean-files), but they are listed in
$(targets) and $(always) and thus removed automatically. Introduce a new
$(no-clean-files) variable to really skip such files in Makefile.clean.

Signed-off-by: Michal Marek <mmarek@suse.cz>
---
Documentation/kbuild/makefiles.txt | 7 +++++++
Kbuild | 4 ++--
scripts/Makefile.clean | 2 ++
3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/Documentation/kbuild/makefiles.txt b/Documentation/kbuild/makefiles.txt
index 0135155..fcd1a98 100644
--- a/Documentation/kbuild/makefiles.txt
+++ b/Documentation/kbuild/makefiles.txt
@@ -779,6 +779,13 @@ This will delete the directory debian, including all subdirectories.
Kbuild will assume the directories to be in the same relative path as the
Makefile if no absolute path is specified (path does not start with '/').

+To exclude certain files from make clean, use the $(no-clean-files) variable.
+This is only a special case used in the top level Kbuild file:
+
+ Example:
+ #Kbuild
+ no-clean-files := $(bounds-file) $(offsets-file)
+
Usually kbuild descends down in subdirectories due to "obj-* := dir/",
but in the architecture makefiles where the kbuild infrastructure
is not sufficient this sometimes needs to be explicit.
diff --git a/Kbuild b/Kbuild
index e3737ad..18a8bfb 100644
--- a/Kbuild
+++ b/Kbuild
@@ -94,5 +94,5 @@ PHONY += missing-syscalls
missing-syscalls: scripts/checksyscalls.sh FORCE
$(call cmd,syscalls)

-# Delete all targets during make clean
-clean-files := $(addprefix $(objtree)/,$(filter-out $(bounds-file) $(offsets-file),$(targets)))
+# Keep these two files during make clean
+no-clean-files := $(bounds-file) $(offsets-file)
diff --git a/scripts/Makefile.clean b/scripts/Makefile.clean
index 6f89fbb..686cb0d 100644
--- a/scripts/Makefile.clean
+++ b/scripts/Makefile.clean
@@ -45,6 +45,8 @@ __clean-files := $(extra-y) $(always) \
$(host-progs) \
$(hostprogs-y) $(hostprogs-m) $(hostprogs-)

+__clean-files := $(filter-out $(no-clean-files), $(__clean-files))
+
# as clean-files is given relative to the current directory, this adds
# a $(obj) prefix, except for absolute paths

--
1.6.6.1

==============================================================================
TOPIC: proc: cleanup: remove unused assignments
http://groups.google.com/group/linux.kernel/t/a0570dc2177842ac?hl=en
==============================================================================

== 1 of 2 ==
Date: Thurs, Mar 11 2010 2:20 am
From: "Helight.Xu"

Dan Carpenter wrote:
> I removed 3 unused assignments. The first two get reset on the first
> statement of their functions. For "err" in root.c we don't return an
> error and we don't use the variable again.
>
> Signed-off-by: Dan Carpenter <error27@gmail.com>
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index a731084..875d636 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2434,7 +2434,7 @@ static struct dentry *proc_base_instantiate(struct inode *dir,
> const struct pid_entry *p = ptr;
> struct inode *inode;
> struct proc_inode *ei;
> - struct dentry *error = ERR_PTR(-EINVAL);
> + struct dentry *error;
>
> /* Allocate the inode */
> error = ERR_PTR(-ENOMEM);
>
why not do it like this:

@@ -2434,7 +2434,7 @@ static struct dentry *proc_base_instantiate(struct inode *dir,
const struct pid_entry *p = ptr;
struct inode *inode;
struct proc_inode *ei;
- struct dentry *error = ERR_PTR(-EINVAL);
+ struct dentry *error = ERR_PTR(-ENOMEM);

/* Allocate the inode */
- error = ERR_PTR(-ENOMEM);

> @@ -2784,7 +2784,7 @@ out:
>
> struct dentry *proc_pid_lookup(struct inode *dir, struct dentry * dentry, struct nameidata *nd)
> {
> - struct dentry *result = ERR_PTR(-ENOENT);
> + struct dentry *result;
> struct task_struct *task;
> unsigned tgid;
> struct pid_namespace *ns;
> diff --git a/fs/proc/root.c b/fs/proc/root.c
> index 757c069..4258384 100644
> --- a/fs/proc/root.c
> +++ b/fs/proc/root.c
> @@ -110,7 +110,6 @@ void __init proc_root_init(void)
> if (err)
> return;
> proc_mnt = kern_mount_data(&proc_fs_type, &init_pid_ns);
> - err = PTR_ERR(proc_mnt);
> if (IS_ERR(proc_mnt)) {
> unregister_filesystem(&proc_fs_type);
> return;
>
>

--
Zhenwen Xu - Seven Helight
Home Page: http://zhwen.org

== 2 of 2 ==
Date: Thurs, Mar 11 2010 2:50 am
From: Dan Carpenter

On Thu, Mar 11, 2010 at 06:17:58PM +0800, Helight.Xu wrote:
> Dan Carpenter wrote:
>> I removed 3 unused assignments. The first two get reset on the first
>> statement of their functions. For "err" in root.c we don't return an
>> error and we don't use the variable again.
>>
>> Signed-off-by: Dan Carpenter <error27@gmail.com>
>>
>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>> index a731084..875d636 100644
>> --- a/fs/proc/base.c
>> +++ b/fs/proc/base.c
>> @@ -2434,7 +2434,7 @@ static struct dentry *proc_base_instantiate(struct inode *dir,
>> const struct pid_entry *p = ptr;
>> struct inode *inode;
>> struct proc_inode *ei;
>> - struct dentry *error = ERR_PTR(-EINVAL);
>> + struct dentry *error;
>> /* Allocate the inode */
>> error = ERR_PTR(-ENOMEM);
>>
> why not do it like this:
>
> @@ -2434,7 +2434,7 @@ static struct dentry *proc_base_instantiate(struct inode *dir,
> const struct pid_entry *p = ptr;
> struct inode *inode;
> struct proc_inode *ei;
> - struct dentry *error = ERR_PTR(-EINVAL);
> + struct dentry *error = ERR_PTR(-ENOMEM);
>
> /* Allocate the inode */
> - error = ERR_PTR(-ENOMEM);
>

It's a personal preference. If anyone read the code in initializers, I
wouldn't have had had anything to clean up in the first place. ;)

regards,
dan carpenter

>
>
>> @@ -2784,7 +2784,7 @@ out:
>> struct dentry *proc_pid_lookup(struct inode *dir, struct dentry *
>> dentry, struct nameidata *nd)
>> {
>> - struct dentry *result = ERR_PTR(-ENOENT);
>> + struct dentry *result;
>> struct task_struct *task;
>> unsigned tgid;
>> struct pid_namespace *ns;
>> diff --git a/fs/proc/root.c b/fs/proc/root.c
>> index 757c069..4258384 100644
>> --- a/fs/proc/root.c
>> +++ b/fs/proc/root.c
>> @@ -110,7 +110,6 @@ void __init proc_root_init(void)
>> if (err)
>> return;
>> proc_mnt = kern_mount_data(&proc_fs_type, &init_pid_ns);
>> - err = PTR_ERR(proc_mnt);
>> if (IS_ERR(proc_mnt)) {
>> unregister_filesystem(&proc_fs_type);
>> return;
>>
>>
>
>
> --
> Zhenwen Xu - Seven Helight
> Home Page: http://zhwen.org
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

==============================================================================
TOPIC: ST SPEAr: Added basic header files for SPEAr platform
http://groups.google.com/group/linux.kernel/t/ce805d5c50983fd8?hl=en
==============================================================================

== 1 of 6 ==
Date: Thurs, Mar 11 2010 2:20 am
From: Shiraz HASHIM

Hello Linus,

On 3/11/2010 12:13 PM, Linus Walleij wrote:
> 2010/3/10 Viresh KUMAR <viresh.kumar@st.com>:
>
>> One timer (or two channels) are used by clock event and clock source, but rest of
>> the timers are still available. So we need some way to export this functionality
>> of our hardware. It can be considered simply as a driver for GPT.
>
> The timer people have already explained well enough I think, we have some 16
> timers on U8500, one we use as clocksource, one for clock events and 14 are
> currently unused. I plan to add them as clock events as soon as there is some
> practical use for them.
> I think there is nothing stopping you from modelling some extra clock events
> for the remaining timers if you absolutely want to, the kernel will
> just disregard
> them currently, which is just as good/bad as the way you're exposing the custom
> API in these patches.

Can we use clockevents directly? I though they are abstracted by the kernel
framework and using them directly is not advisable/feasible.

regards
Shiraz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

== 2 of 6 ==
Date: Thurs, Mar 11 2010 2:20 am
From: Shiraz HASHIM

Hello Linus,
Thanks for your comments. Replying on some of these. For the rest
Viresh would reply tomorrow.

On 3/11/2010 12:30 PM, Linus Walleij wrote:
> 2010/3/3 Viresh KUMAR <viresh.kumar@st.com>:
>
>> Clock framework for SPEAr is based upon clkdev framework for ARM
>> (... clock tree definition looks OK to me ...)
>
>> diff --git a/arch/arm/plat-spear/clock.c b/arch/arm/plat-spear/clock.c
>> new file mode 100755
>> index 0000000..bdcae0c
>> --- /dev/null
>> +++ b/arch/arm/plat-spear/clock.c
>> @@ -0,0 +1,433 @@
>> +/*
>> + * arch/arm/plat-spear/clock.c
>> + *
>> + * Clock framework for SPEAr platform
>> + *
>> + * Copyright (C) 2009 ST Microelectronics
>> + * Viresh Kumar<viresh.kumar@st.com>
>> + *
>> + * This file is licensed under the terms of the GNU General Public
>> + * License version 2. This program is licensed "as is" without any
>> + * warranty of any kind, whether express or implied.
>> + */
>> +
>> +#include <linux/bug.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/list.h>
>> +#include <linux/module.h>
>> +#include <linux/spinlock.h>
>> +#include <mach/misc_regs.h>
>> +#include <plat/clock.h>
>> +
>> +static DEFINE_SPINLOCK(clocks_lock);
>> +static LIST_HEAD(root_clks);
>> +
>> +static void propagate_rate(struct list_head *);
>> +
>> +static int generic_clk_enable(struct clk *clk)
>> +{
>> + unsigned int val;
>> +
>> + if (!clk->en_reg)
>> + return -EFAULT;
>> +
>> + val = readl(clk->en_reg);
>> + if (unlikely(clk->flags & RESET_TO_ENABLE))
>> + val &= ~(1 << clk->en_reg_bit);
>> + else
>> + val |= 1 << clk->en_reg_bit;
>> + writel(val, clk->en_reg);
>
> I don't understand one bit of this. What happens if the RESET_TO_ENABLE
> flag is set for the clock? The exact same bit is &-masked and then
> immediately |:ed to 1 again. Then it is written to the register. Practical
> effect: absolutely none.
>
> Is there a writel(val, clk->en_reg); missing from the unlikely() execution
> path?

The intention to use RESET_TO_ENABLE flag is to generalize clock
enable/disable across platforms. To enable particular clock, we
may require to set/reset some particular bit in system register.
If RESET_TO_ENABLE is set in the flags, this means we need to reset the
corresponding bit (clk->en_reg_bit) to enable the clock else we need to set
it.

>> +
>> + return 0;
>> +}
>> +
>> +static void generic_clk_disable(struct clk *clk)
>> +{
>> + unsigned int val;
>> +
>> + if (!clk->en_reg)
>> + return;
>> +
>> + val = readl(clk->en_reg);
>> + if (unlikely(clk->flags & RESET_TO_ENABLE))
>> + val |= 1 << clk->en_reg_bit;
>> + else
>> + val &= ~(1 << clk->en_reg_bit);
>
> Same issue here...
>

The purpose of RESET_TO_ENABLE is explained above. So in this case if this flag
is present we need to set the bit to disable the clock.

>> +
>> + writel(val, clk->en_reg);
>> +}
>> +
>> +/* generic clk ops */
>> +static struct clkops generic_clkops = {
>> + .enable = generic_clk_enable,
>> + .disable = generic_clk_disable,
>> +};
>> +
>> +/*
>> + * clk_enable - inform the system when the clock source should be running.
>> + * @clk: clock source
>> + *
>> + * If the clock can not be enabled/disabled, this should return success.
>> + *
>> + * Returns success (0) or negative errno.
>> + */
>> +int clk_enable(struct clk *clk)
>> +{
>> + unsigned long flags;
>> + int ret = 0;
>> +
>> + if (!clk || IS_ERR(clk))
>> + return -EFAULT;
>> +
>> + spin_lock_irqsave(&clocks_lock, flags);
>> + if (clk->usage_count++ == 0) {
>
> Isnt if (++clk.>usage_count == 1) easier to understand, or is it just me?
> BTW doing this:
> clk->usage_count++;
> if (clk->usage_count == 1)
> will not use more memory, the compiler optimize this, so choose the
> version you think is most readable. If you think this is very readable, by
> all means keep it.

I think you are right. I would let Viresh reply on this.

>> + if (clk->ops && clk->ops->enable)
>> + ret = clk->ops->enable(clk);
>> + }
>> + spin_unlock_irqrestore(&clocks_lock, flags);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(clk_enable);
>> +
>> +/*
>> + * clk_disable - inform the system when the clock source is no longer required.
>> + * @clk: clock source
>> + *
>> + * Inform the system that a clock source is no longer required by
>> + * a driver and may be shut down.
>> + *
>> + * Implementation detail: if the clock source is shared between
>> + * multiple drivers, clk_enable() calls must be balanced by the
>> + * same number of clk_disable() calls for the clock source to be
>> + * disabled.
>> + */
>> +void clk_disable(struct clk *clk)
>> +{
>> + unsigned long flags;
>> +
>> + if (!clk || IS_ERR(clk))
>> + return;
>> +
>> + WARN_ON(clk->usage_count == 0);
>> +
>> + spin_lock_irqsave(&clocks_lock, flags);
>> + if (--clk->usage_count == 0) {
>
> Same readability issue here.
>> + if (clk->ops && clk->ops->disable)
>> + clk->ops->disable(clk);
>> + }
>> + spin_unlock_irqrestore(&clocks_lock, flags);
>> +}
>> +EXPORT_SYMBOL(clk_disable);
>> +
>> +/**
>> + * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
>> + * This is only valid once the clock source has been enabled.
>> + * @clk: clock source
>> + */
>> +unsigned long clk_get_rate(struct clk *clk)
>> +{
>> + unsigned long flags, rate;
>> +
>> + spin_lock_irqsave(&clocks_lock, flags);
>> + rate = clk->rate;
>> + spin_unlock_irqrestore(&clocks_lock, flags);
>> +
>> + return rate;
>> +}
>> +EXPORT_SYMBOL(clk_get_rate);
>> +
>> +/**
>> + * clk_set_parent - set the parent clock source for this clock
>> + * @clk: clock source
>> + * @parent: parent clock source
>> + *
>> + * Returns success (0) or negative errno.
>> + */
>> +int clk_set_parent(struct clk *clk, struct clk *parent)
>> +{
>> + int i, found = 0, val = 0;
>> + unsigned long flags;
>> +
>> + if (!clk || IS_ERR(clk) || !parent || IS_ERR(parent))
>> + return -EFAULT;
>> + if (clk->usage_count == 0)
>> + return -EBUSY;
>
> Why will the clk_set_parent() call fail if there are *no* users of the clock?
> Should it not be the other way around? Or what am I misunderstanding here?

I think you are right. The parent can be selected even if clock is disabled. I
would let Viresh answer on this.

>> + if (!clk->pclk_sel)
>> + return -EPERM;
>> + if (clk->pclk == parent)
>> + return 0;
>> +
>> + for (i = 0; i < clk->pclk_sel->pclk_count; i++) {
>> + if (clk->pclk_sel->pclk_info[i].pclk == parent) {
>> + found = 1;
>> + break;
>> + }
>> + }
>> +
>> + if (!found)
>> + return -EPERM;
>
> What about -ENODEV or so? (I don't know what people typically
> use for clocks that don't exist.)
>

yes, atleast -EPERM doesn't seem suitable here. May be -EINVAL or -ENOENT?

>> +
>> + spin_lock_irqsave(&clocks_lock, flags);
>> + /* reflect parent change in hardware */
>> + val = readl(clk->pclk_sel->pclk_sel_reg);
>> + val &= ~(clk->pclk_sel->pclk_sel_mask << clk->pclk_sel_shift);
>> + val |= clk->pclk_sel->pclk_info[i].pclk_mask << clk->pclk_sel_shift;
>> + writel(val, clk->pclk_sel->pclk_sel_reg);
>> + spin_unlock_irqrestore(&clocks_lock, flags);
>> +
>> + /* reflect parent change in software */
>> + clk->recalc(clk);
>> + propagate_rate(&clk->children);
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(clk_set_parent);
>> +
>> +/* registers clock in platform clock framework */
>> +void clk_register(struct clk_lookup *cl)
>> +{
>> + struct clk *clk = cl->clk;
>> + unsigned long flags;
>> +
>> + if (!clk || IS_ERR(clk))
>> + return;
>> +
>> + spin_lock_irqsave(&clocks_lock, flags);
>> +
>> + INIT_LIST_HEAD(&clk->children);
>> + if (clk->flags & ALWAYS_ENABLED)
>> + clk->ops = NULL;
>> + else if (!clk->ops)
>> + clk->ops = &generic_clkops;
>> +
>> + /* root clock don't have any parents */
>> + if (!clk->pclk && !clk->pclk_sel) {
>> + list_add(&clk->sibling, &root_clks);
>> + /* add clocks with only one parent to parent's children list */
>> + } else if (clk->pclk && !clk->pclk_sel) {
>> + list_add(&clk->sibling, &clk->pclk->children);
>> + } else {
>> + /* add clocks with > 1 parent to 1st parent's children list */
>> + list_add(&clk->sibling,
>> + &clk->pclk_sel->pclk_info[0].pclk->children);
>> + }
>> + spin_unlock_irqrestore(&clocks_lock, flags);
>> +
>> + /* add clock to arm clockdev framework */
>> + clkdev_add(cl);
>> +}
>> +
>> +/**
>> + * propagate_rate - recalculate and propagate all clocks in list head
>> + *
>> + * Recalculates all root clocks in list head, which if the clock's .recalc is
>> + * set correctly, should also propagate their rates.
>> + */
>> +static void propagate_rate(struct list_head *lhead)
>> +{
>> + struct clk *clkp, *_temp;
>> +
>> + list_for_each_entry_safe(clkp, _temp, lhead, sibling) {
>> + if (clkp->recalc)
>> + clkp->recalc(clkp);
>> + propagate_rate(&clkp->children);
>> + }
>> +}
>> +
>> +/* returns current programmed clocks clock info structure */
>> +static struct pclk_info *pclk_info_get(struct clk *clk)
>> +{
>> + unsigned int mask, i;
>> + unsigned long flags;
>> + struct pclk_info *info;
>> +
>> + spin_lock_irqsave(&clocks_lock, flags);
>> + mask = (readl(clk->pclk_sel->pclk_sel_reg) >> clk->pclk_sel_shift)
>> + & clk->pclk_sel->pclk_sel_mask;
>> +
>> + for (i = 0; i < clk->pclk_sel->pclk_count; i++) {
>> + if (clk->pclk_sel->pclk_info[i].pclk_mask == mask)
>> + info = &clk->pclk_sel->pclk_info[i];
>> + }
>> + spin_unlock_irqrestore(&clocks_lock, flags);
>> +
>> + return info;
>> +}
>> +
>> +/*
>> + * Set pclk as cclk's parent and add clock sibling node to current parents
>> + * children list
>> + */
>> +static void change_parent(struct clk *cclk, struct clk *pclk)
>> +{
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&clocks_lock, flags);
>> + list_del(&cclk->sibling);
>> + list_add(&cclk->sibling, &pclk->children);
>> +
>> + cclk->pclk = pclk;
>> + spin_unlock_irqrestore(&clocks_lock, flags);
>> +}
>> +
>> +/*
>> + * calculates current programmed rate of pll1
>> + *
>> + * In normal mode
>> + * rate = (2 * M[15:8] * Fin)/(N * 2^P)
>> + *
>> + * In Dithered mode
>> + * rate = (2 * M[15:0] * Fin)/(256 * N * 2^P)
>> + */
>> +void pll1_clk_recalc(struct clk *clk)
>> +{
>> + struct pll_clk_config *config = clk->private_data;
>> + unsigned int num = 2, den = 0, val, mode = 0;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&clocks_lock, flags);
>> + mode = (readl(config->mode_reg) >> PLL_MODE_SHIFT) &
>> + PLL_MODE_MASK;
>> +
>> + val = readl(config->cfg_reg);
>> + /* calculate denominator */
>> + den = (val >> PLL_DIV_P_SHIFT) & PLL_DIV_P_MASK;
>> + den = 1 << den;
>> + den *= (val >> PLL_DIV_N_SHIFT) & PLL_DIV_N_MASK;
>> +
>> + /* calculate numerator & denominator */
>> + if (!mode) {
>> + /* Normal mode */
>> + num *= (val >> PLL_NORM_FDBK_M_SHIFT) & PLL_NORM_FDBK_M_MASK;
>> + } else {
>> + /* Dithered mode */
>> + num *= (val >> PLL_DITH_FDBK_M_SHIFT) & PLL_DITH_FDBK_M_MASK;
>> + den *= 256;
>> + }
>> +
>> + clk->rate = (((clk->pclk->rate/10000) * num) / den) * 10000;
>> + spin_unlock_irqrestore(&clocks_lock, flags);
>> +}
>> +
>> +/* calculates current programmed rate of ahb or apb bus */
>> +void bus_clk_recalc(struct clk *clk)
>> +{
>> + struct bus_clk_config *config = clk->private_data;
>> + unsigned int div;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&clocks_lock, flags);
>> + div = ((readl(config->reg) >> config->shift) & config->mask) + 1;
>> + clk->rate = (unsigned long)clk->pclk->rate / div;
>> + spin_unlock_irqrestore(&clocks_lock, flags);
>> +}
>> +
>> +/*
>> + * calculates current programmed rate of auxiliary synthesizers
>> + * used by: UART, FIRDA
>> + *
>> + * Fout from synthesizer can be given from two equations:
>> + * Fout1 = (Fin * X/Y)/2
>> + * Fout2 = Fin * X/Y
>> + *
>> + * Selection of eqn 1 or 2 is programmed in register
>> + */
>> +void aux_clk_recalc(struct clk *clk)
>> +{
>> + struct aux_clk_config *config = clk->private_data;
>> + struct pclk_info *pclk_info = NULL;
>> + unsigned int num = 1, den = 1, val, eqn;
>> + unsigned long flags;
>> +
>> + /* get current programmed parent */
>> + pclk_info = pclk_info_get(clk);
>> + if (!pclk_info) {
>> + spin_lock_irqsave(&clocks_lock, flags);
>> + clk->pclk = NULL;
>> + clk->rate = 0;
>> + spin_unlock_irqrestore(&clocks_lock, flags);
>> + return;
>> + }
>> +
>> + change_parent(clk, pclk_info->pclk);
>> +
>> + spin_lock_irqsave(&clocks_lock, flags);
>> + if (pclk_info->scalable) {
>> + val = readl(config->synth_reg);
>> +
>> + eqn = (val >> AUX_EQ_SEL_SHIFT) & AUX_EQ_SEL_MASK;
>> + if (eqn == AUX_EQ1_SEL)
>> + den *= 2;
>> +
>> + /* calculate numerator */
>> + num = (val >> AUX_XSCALE_SHIFT) & AUX_XSCALE_MASK;
>> +
>> + /* calculate denominator */
>> + den *= (val >> AUX_YSCALE_SHIFT) & AUX_YSCALE_MASK;
>> + val = (((clk->pclk->rate/10000) * num) / den) * 10000;
>> + } else
>> + val = clk->pclk->rate;
>> +
>> + clk->rate = val;
>> + spin_unlock_irqrestore(&clocks_lock, flags);
>> +}
>> +
>> +/*
>> + * calculates current programmed rate of gpt synthesizers
>> + * Fout from synthesizer can be given from below equations:
>> + * Fout= Fin/((2 ^ (N+1)) * (M+1))
>> + */
>> +void gpt_clk_recalc(struct clk *clk)
>> +{
>> + struct aux_clk_config *config = clk->private_data;
>> + struct pclk_info *pclk_info = NULL;
>> + unsigned int div = 1, val;
>> + unsigned long flags;
>> +
>> + pclk_info = pclk_info_get(clk);
>> + if (!pclk_info) {
>> + spin_lock_irqsave(&clocks_lock, flags);
>> + clk->pclk = NULL;
>> + clk->rate = 0;
>> + spin_unlock_irqrestore(&clocks_lock, flags);
>> + return;
>> + }
>> +
>> + change_parent(clk, pclk_info->pclk);
>> +
>> + spin_lock_irqsave(&clocks_lock, flags);
>> + if (pclk_info->scalable) {
>> + val = readl(config->synth_reg);
>> + div += (val >> GPT_MSCALE_SHIFT) & GPT_MSCALE_MASK;
>> + div *= 1 << (((val >> GPT_NSCALE_SHIFT) & GPT_NSCALE_MASK) + 1);
>> + }
>> +
>> + clk->rate = (unsigned long)clk->pclk->rate / div;
>> + spin_unlock_irqrestore(&clocks_lock, flags);
>> +}
>> +
>> +/*
>> + * Used for clocks that always have same value as the parent clock divided by a
>> + * fixed divisor
>> + */
>> +void follow_parent(struct clk *clk)
>> +{
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&clocks_lock, flags);
>> + clk->rate = clk->pclk->rate;
>> + spin_unlock_irqrestore(&clocks_lock, flags);
>> +}
>> +
>> +/**
>> + * recalc_root_clocks - recalculate and propagate all root clocks
>> + *
>> + * Recalculates all root clocks (clocks with no parent), which if the
>> + * clock's .recalc is set correctly, should also propagate their rates.
>> + */
>> +void recalc_root_clocks(void)
>> +{
>> + propagate_rate(&root_clks);
>> +}
>
> I understand (I think) how speed change can propagate through the clocks.
> However I think it will be hard to notify users that the clock rate has changed,
> because there is nothing in the clk framework that supports that. If you have
> drivers with dynamic clocks, do you have any plans on how you will
> notify drivers?

yes, the clock framework doesn't really defines interface to notify
frequency change. But at all the times it should contain and reflect the correct
system state w.r.t. the frequency and clock selection and hence propagate_rate
is must.

> OMAP uses CPUfreq but that is really about the CPU. As it happens, all
> their clk:s always change frequency at the same operating points as the
> CPU. So they can have pre/post calls from CPUfreq in their code, but
> this will not work with things like PrimeCells where other users of the cell
> may not have operating points correlated with CPU operating points.
>
> (I'm not requesting you to solve this problem, more to be aware of it.)

I think generally in embedded systems (at least in our case :) ) the CPU clock
itself is not completly independent. It is generally tied with some system
clock, which has an impact on bus and peripheral clocks. In that sense cpu freq
would be a better mean to notify frequency change.
In any case, clock framework don't intend to do it. It only need to reflect
correct system state. Is this understanding correct?

>> diff --git a/arch/arm/plat-spear/include/plat/clkdev.h b/arch/arm/plat-spear/include/plat/clkdev.h
>> new file mode 100644
>> index 0000000..9b1cd54
>> --- /dev/null
>> +++ b/arch/arm/plat-spear/include/plat/clkdev.h
>> @@ -0,0 +1,20 @@
>> +/*
>> + * arch/arm/plat-spear/include/plat/clkdev.h
>> + *
>> + * Clock Dev framework definitions for SPEAr platform
>> + *
>> + * Copyright (C) 2009 ST Microelectronics
>> + * Viresh Kumar<viresh.kumar@st.com>
>> + *
>> + * This file is licensed under the terms of the GNU General Public
>> + * License version 2. This program is licensed "as is" without any
>> + * warranty of any kind, whether express or implied.
>> + */
>> +
>> +#ifndef __ASM_PLAT_CLKDEV_H
>> +#define __ASM_PLAT_CLKDEV_H
>> +
>> +#define __clk_get(clk) ({ 1; })
>> +#define __clk_put(clk) do { } while (0)
>> +
>> +

twitter

Thursday, March 11, 2010

linux.kernel - 26 new messages in 17 topics - digest

0 Comments:

Post a Comment

About Me

Previous Posts