[PATCH 1/2] User access to internal clocks

[PATCH 1/2] User access to internal clocks

[Davide Rizzo]

To: Greg KH <gregkh-l3A5Bk7waGM@public.gmane.org>
Cc: "Hans J. Koch" <hjk-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

This driver is for user level programs to interact with system clocks.
It allows to read and modify rates and parents, using virtual files.
It requires the implementation of 2 additional functions in the clk interface:
 clk_for_each() and clk_name().
Actually I implemented that functions only for Samsung S3C24xx platform.

Signed-off-by: Davide Rizzo <elpa.rizzo-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
diff -urpN linux-2.6.29-rc2/drivers/misc/Kconfig
linux-2.6.29-rc2.elpa/drivers/misc/Kconfig
--- linux-2.6.29-rc2/drivers/misc/Kconfig       2009-01-17
11:32:40.000000000 +0100
+++ linux-2.6.29-rc2.elpa/drivers/misc/Kconfig  2009-01-28
19:21:43.000000000 +0100
@@ -232,4 +232,19 @@ config DELL_LAPTOP

 source "drivers/misc/c2port/Kconfig"

+config UM_CLOCK
+       bool "User mode clock driver"
+       depends on SYSFS
+       default n
+       ---help---
+       Allows user mode programs to configure and control internal clocks'
+        rates and parents through virtual files in sysfs.
+       This driver requires implementation of some additional functions
+        in architecture specific low-level drivers: clk_for_each() and
+        clk_get_name(). Moreover, it requires clk_get() to recognize the
+        "name.id" format
+       Currently they're implemented only on Samsung S3C24xx platforms.
+
+       If unsure, say N.
+
 endif # MISC_DEVICES
diff -urpN linux-2.6.29-rc2/drivers/misc/Makefile
linux-2.6.29-rc2.elpa/drivers/misc/Makefile
--- linux-2.6.29-rc2/drivers/misc/Makefile      2009-01-17
11:32:40.000000000 +0100
+++ linux-2.6.29-rc2.elpa/drivers/misc/Makefile 2009-01-28
19:21:43.000000000 +0100
@@ -20,3 +20,4 @@ obj-$(CONFIG_SGI_XP)          += sgi-xp/
 obj-$(CONFIG_SGI_GRU)          += sgi-gru/
 obj-$(CONFIG_HP_ILO)           += hpilo.o
 obj-$(CONFIG_C2PORT)           += c2port/
+obj-$(CONFIG_UM_CLOCK) += um_clock.o
diff -urpN linux-2.6.29-rc2/drivers/misc/um_clock.c
linux-2.6.29-rc2.elpa/drivers/misc/um_clock.c
--- linux-2.6.29-rc2/drivers/misc/um_clock.c    1970-01-01
01:00:00.000000000 +0100
+++ linux-2.6.29-rc2.elpa/drivers/misc/um_clock.c       2009-01-28
19:21:43.000000000 +0100
@@ -0,0 +1,174 @@
+/*
+ driver/misc/clock.c
+
+ Written Feb 2008 by Davide Rizzo <elpa.rizzo-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+
+ This driver allows to read and modify internal clocks' rates using
+  virtual files. User can also read and modify parents.
+
+ This driver requires implementation of clk_name() and clk_enum() functions
+  in architecture specific clock.c
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 2 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program; if not, write to the Free Software
+ Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+*/
+
+#include <linux/device.h>
+#include <linux/clk.h>
+#include <linux/kdev_t.h>
+#include <linux/err.h>
+
+static DEFINE_MUTEX(mutex);
+
+static ssize_t rate_show(struct device *dev, struct device_attribute *attr,
+       char *buffer)
+{
+       unsigned long rate;
+       struct clk *clk;
+
+       clk = dev_get_drvdata(dev);
+       rate = 0;
+
+       mutex_lock(&mutex);
+
+       rate = clk_get_rate(clk);
+
+       mutex_unlock(&mutex);
+
+       return sprintf(buffer, "%ld\n", rate);
+}
+
+static ssize_t rate_store(struct device *dev, struct device_attribute *attr,
+       const char *buffer, size_t count)
+{
+       unsigned long rate;
+       struct clk *clk;
+       int err;
+
+       clk = dev_get_drvdata(dev);
+       err = strict_strtoul(buffer, 10, &rate);
+       if (err)
+               return err;
+
+       mutex_lock(&mutex);
+
+       if (rate != 0) {
+               clk_set_rate(clk, clk_round_rate(clk, rate));
+               clk_enable(clk);
+       } else
+               clk_disable(clk);
+
+       mutex_unlock(&mutex);
+
+       return count;
+}
+
+static ssize_t parent_show(struct device *dev,
+       struct device_attribute *attr, char *buffer)
+{
+       struct clk *parent, *clk;
+
+       clk = dev_get_drvdata(dev);
+
+       mutex_lock(&mutex);
+
+       parent = clk_get_parent(clk);
+       if (parent && !IS_ERR(parent)) {
+               const char *name = clk_get_name(parent);
+               if (IS_ERR(name)) {
+                       mutex_unlock(&mutex);
+                       return PTR_ERR(name);
+               }
+               strlcpy(buffer, name, PAGE_SIZE);
+       } else
+               buffer[0] = '\0';
+       strlcat(buffer, "\n", PAGE_SIZE);
+
+       mutex_unlock(&mutex);
+
+       return strlen(buffer);
+}
+
+static ssize_t parent_store(struct device *dev,
+       struct device_attribute *attr, const char *buffer, size_t count)
+{
+       struct clk *parent;
+
+       char *s = kstrdup(buffer, GFP_KERNEL);
+
+       if (!s)
+               return -ENOMEM;
+       if (s[strlen(s) - 1] == '\n')
+               s[strlen(s) - 1] = '\0';
+       parent = clk_get(dev, s);
+       kfree(s);
+       if (IS_ERR(parent))
+               return PTR_ERR(parent);
+
+       mutex_lock(&mutex);
+
+       clk_set_parent(dev_get_drvdata(dev), parent);
+
+       mutex_unlock(&mutex);
+
+       clk_put(parent);
+       return count;
+}
+
+static DEVICE_ATTR(rate, S_IRUGO | S_IWUSR, rate_show, rate_store);
+static DEVICE_ATTR(parent, S_IRUGO | S_IWUSR, parent_show, parent_store);
+
+static const struct attribute *clock_attrs[] = {
+       &dev_attr_rate.attr,
+       &dev_attr_parent.attr,
+       NULL,
+};
+
+static const struct attribute_group clock_attr_group = {
+       .attrs = (struct attribute **) clock_attrs,
+};
+
+static struct class gpclock_class = {
+       .name =         "clock",
+       .owner =        THIS_MODULE,
+};
+
+static int create_clock_attr(struct clk *clk, void *data)
+{
+       struct device *dev;
+       const char *name;
+
+       name = clk_get_name(clk);
+       if (name == NULL || IS_ERR(name)) {
+               pr_debug("Invalid clock's name\n");
+               return PTR_ERR(name);
+       }
+       dev = device_create(&gpclock_class, (struct device *)data, MKDEV(0, 0),
+               clk, name);
+       if (!dev)
+               return -ENODEV;
+       return sysfs_create_group(&dev->kobj, &clock_attr_group);
+}
+
+static int __init gpclock_init(void)
+{
+       int ret;
+
+       ret = class_register(&gpclock_class);
+       if (ret >= 0)
+               ret = clk_for_each(create_clock_attr, NULL);
+       return ret;
+}
+subsys_initcall(gpclock_init);
+
diff -urpN linux-2.6.29-rc2/Documentation/ABI/testing/sysfs-class-clock
linux-2.6.29-rc2.elpa/Documentation/ABI/testing/sysfs-class-clock
--- linux-2.6.29-rc2/Documentation/ABI/testing/sysfs-class-clock
 1970-01-01
01:00:00.000000000 +0100
+++ linux-2.6.29-rc2.elpa/Documentation/ABI/testing/sysfs-class-clock
 2009-01-28
19:21:43.000000000 +0100
@@ -0,0 +1,20 @@
+What:          /sys/class/clock
+Date:          January 2009
+Contact:       Davide Rizzo <elpa.rizzo-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
+Description:
+               The /sys/class/clock directory will consist of a group of
+               subdirectories each one describing an internal clock
in the kernel.
+               The name of the subdirectory is the clock's name.
+               If a clock is device specific, its name is in the form
"name.id".
+               In each directory there are 2 virtual files: rate and parent.
+               Both these files are r/w, but there can be some
architecture-dependent
+               limitations on write access on one or both of them.
+               For this interface to work, it needs some low-level
implementations (look
+               at include/linux/clk.h):
+               - clk_for_each()
+               - clk_name()
+               - clk_get() should recognize also the "name.id" format
+               Actually I implemented these only for Samsung S3C SoC in
+               arch/arm/plat-s3c/clock.c
+
+Users:
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] User access to internal clocks

[Russell King - ARM Linux]

On Mon, Feb 09, 2009 at 06:04:29PM +0100, Davide Rizzo wrote:
> To: Greg KH <gregkh-l3A5Bk7waGM@public.gmane.org>
> Cc: "Hans J. Koch" <hjk-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
> ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> 
> This driver is for user level programs to interact with system clocks.
> It allows to read and modify rates and parents, using virtual files.
> It requires the implementation of 2 additional functions in the clk interface:
>  clk_for_each() and clk_name().
> Actually I implemented that functions only for Samsung S3C24xx platform.

NAK.

> +       name = clk_get_name(clk);

This implies that there is a 1:1 relationship between a 'name' and a
struct clk.  No such thing exists (and where it does, it's being
eliminated in ARM because it's just plain and simple WRONG.)
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] User access to internal clocks

[Hans J. Koch]

On Mon, Feb 09, 2009 at 06:04:29PM +0100, Davide Rizzo wrote:
> To: Greg KH <gregkh-l3A5Bk7waGM@public.gmane.org>
> Cc: "Hans J. Koch" <hjk-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
> ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

Davide,
please remove me from CC:, I'm not one of the clock gurus (which you might
want to CC instead).

Thanks,
Hans


> 
> This driver is for user level programs to interact with system clocks.
> It allows to read and modify rates and parents, using virtual files.
> It requires the implementation of 2 additional functions in the clk interface:
>  clk_for_each() and clk_name().
> Actually I implemented that functions only for Samsung S3C24xx platform.
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] User access to internal clocks

[Davide Rizzo]

>> This driver is for user level programs to interact with system clocks.
>> It allows to read and modify rates and parents, using virtual files.
>> It requires the implementation of 2 additional functions in the clk interface:
>>  clk_for_each() and clk_name().
>> Actually I implemented that functions only for Samsung S3C24xx platform.
>
> NAK.
>
>> +       name = clk_get_name(clk);
>
> This implies that there is a 1:1 relationship between a 'name' and a
> struct clk.  No such thing exists (and where it does, it's being
> eliminated in ARM because it's just plain and simple WRONG.)
>
Not exactly, clk_get() could work in both way: a struct clk can be
exactly identified by name and device id (like clk_get is acting now)
or by full name, in the format clkname.device
Why do you say that it's wrong ? This is the same syntax used for
devices, why couldn't it be used also for clocks ? This should not
break backward compatibility.
May be clk_get_name() should be renamed as clk_get_full_name() ? Or it
would be better to make a function (or 2 different functions) that
returns separated name and device id ?
Otherwise, what do you suggest to enumerate and distinguish all system clocks ?
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] User access to internal clocks

[Russell King - ARM Linux]

On Mon, Feb 09, 2009 at 06:44:40PM +0100, Davide Rizzo wrote:
> >> This driver is for user level programs to interact with system clocks.
> >> It allows to read and modify rates and parents, using virtual files.
> >> It requires the implementation of 2 additional functions in the clk interface:
> >>  clk_for_each() and clk_name().
> >> Actually I implemented that functions only for Samsung S3C24xx platform.
> >
> > NAK.
> >
> >> +       name = clk_get_name(clk);
> >
> > This implies that there is a 1:1 relationship between a 'name' and a
> > struct clk.  No such thing exists (and where it does, it's being
> > eliminated in ARM because it's just plain and simple WRONG.)
>
> Not exactly, clk_get() could work in both way: a struct clk can be
> exactly identified by name and device id (like clk_get is acting now)
> or by full name, in the format clkname.device

You're not understanding the issue(s).

1. there is no 1:1 mapping between the identifiers (struct device +
   connection ID) and the struct clk.  It's actually a many-to-one
   mapping.

   That means there is _no_ name associated with a struct clk.

   Conceptually, clk_get() gives you a struct clk for the struct device
   and a connection ID.  How that mapping is achieved isn't specified
   in the API, all that's required is that such a mapping is performed.
   It is _specifically_ intended that more than one set of {device,id}
   pairs will map to the same clk.

   So, now to insist that you can go from a struct clk to some kind of
   string identifier for it is changing this - you're now requiring
   that every struct clk has a unique name.  This is not the case.
   PXA, for instance, struct clk's are now completely nameless.  They
   have no identifier.

   If you want to have a string identifier which works in every case,
   this will:

	sprintf(identifer, "%p", clk);

   Or, I guess you could force every struct clk to have a 'sysfs_name'
   field just to export them out via sysfs - and that'll be all that
   it's used for.

2. there is no generic way to walk a set of struct clk's - indeed, there
   may be no list of them (and there exists implmentations where that is
   true) and the only list which does exist is a set ID to clk mapping
   structures.

> Otherwise, what do you suggest to enumerate and distinguish all system
> clocks ?

There exists no such concept in the API, what you're asking for is
implementation specific.
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] User access to internal clocks

[Russell King - ARM Linux]

On Mon, Feb 09, 2009 at 06:04:29PM +0100, Davide Rizzo wrote:
> +static ssize_t parent_store(struct device *dev,
> +       struct device_attribute *attr, const char *buffer, size_t count)
> +{
> +       struct clk *parent;
> +
> +       char *s = kstrdup(buffer, GFP_KERNEL);
> +
> +       if (!s)
> +               return -ENOMEM;
> +       if (s[strlen(s) - 1] == '\n')
> +               s[strlen(s) - 1] = '\0';
> +       parent = clk_get(dev, s);
> +       kfree(s);
> +       if (IS_ERR(parent))
> +               return PTR_ERR(parent);
> +
> +       mutex_lock(&mutex);
> +
> +       clk_set_parent(dev_get_drvdata(dev), parent);

BTW, MXC doesn't (and won't) support run-time changing of the clk tree.
OMAP sort-of does, but it's currently buggy.  It looks like S3C is also
buggy.

These implementations are currently only usable under a very strict set
of circumstances (which are not currently checked by the implementations)
and if the parent is changed while the clock is in use, it'll screw up
the clock tree - details below.

Arguably, that's a problem which the clk_set_parent() implementations need
to be solved (and I hope are going to be resolved), but it's an example
where exposing the API to allow userspace to fiddle with things produces
new ways to make things explode.  Certainly the MXC folk responded saying
"we don't support run-time parent changes".

FYI, the problem basically is that the sequence:

1. clk_enable(clk);
2. clk_set_parent(clk, new_parent);
3. clk_disable(clk);

will result in the old parent having a permanently incremented usecount,
and the new_parent having a negative usecount.  You could say "don't
permit enabled clocks to change parents" but I believe such a statement
would be provably incompatible with OMAP.

(This issue has just been pointed out to the relevant ARM maintainers
today.)

Re: [PATCH 1/2] User access to internal clocks

[Davide Rizzo]

> You're not understanding the issue(s).
>
> 1. there is no 1:1 mapping between the identifiers (struct device +
>   connection ID) and the struct clk.  It's actually a many-to-one
>   mapping.
>
>   That means there is _no_ name associated with a struct clk.
>
>   Conceptually, clk_get() gives you a struct clk for the struct device
>   and a connection ID.  How that mapping is achieved isn't specified
>   in the API, all that's required is that such a mapping is performed.
>   It is _specifically_ intended that more than one set of {device,id}
>   pairs will map to the same clk.
>
>   So, now to insist that you can go from a struct clk to some kind of
>   string identifier for it is changing this - you're now requiring
>   that every struct clk has a unique name.  This is not the case.
>   PXA, for instance, struct clk's are now completely nameless.  They
>   have no identifier.
>
>   If you want to have a string identifier which works in every case,
>   this will:
>
>        sprintf(identifer, "%p", clk);
>
>   Or, I guess you could force every struct clk to have a 'sysfs_name'
>   field just to export them out via sysfs - and that'll be all that
>   it's used for.
>
> 2. there is no generic way to walk a set of struct clk's - indeed, there
>   may be no list of them (and there exists implmentations where that is
>   true) and the only list which does exist is a set ID to clk mapping
>   structures.
>
>> Otherwise, what do you suggest to enumerate and distinguish all system
>> clocks ?
>
> There exists no such concept in the API, what you're asking for is
> implementation specific.
>

Thank you for this explanation, now it's much more clear to me.

What about adding a dedicated function to clk api that registers a struct clk
 in a linked list and associates it with a name, with the only specific purpose
 to enumerate and list (and possibly change rate and parents) all
clocks in sysfs ?
Obviously only specifically registered clocks will be visible in sysfs...
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] User access to internal clocks

[Davide Rizzo]

> What about adding a dedicated function to clk api that registers a struct clk
>  in a linked list and associates it with a name, with the only specific purpose
>  to enumerate and list (and possibly change rate and parents) all
> clocks in sysfs ?
> Obviously only specifically registered clocks will be visible in sysfs...
>

Or better, only proposed functions:
int clk_for_each(int(*fn)(struct clk *, void *), void *data)
and
const char *clk_sysname(struct clk *)
need to be added to clk api, the function to register the struct clk
in the linked list could be implementation specific.
Maybe a function like this:
struct clk *clk_get_by_sysname(const char *sysname);
could be useful to be added to api interface, too ?
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] User access to internal clocks

[Russell King - ARM Linux]

On Mon, Feb 09, 2009 at 07:54:57PM +0100, Davide Rizzo wrote:
> > What about adding a dedicated function to clk api that registers a struct clk
> >  in a linked list and associates it with a name, with the only specific purpose
> >  to enumerate and list (and possibly change rate and parents) all
> > clocks in sysfs ?
> > Obviously only specifically registered clocks will be visible in sysfs...
> >
> 
> Or better, only proposed functions:
> int clk_for_each(int(*fn)(struct clk *, void *), void *data)
> and
> const char *clk_sysname(struct clk *)
> need to be added to clk api, the function to register the struct clk
> in the linked list could be implementation specific.
> Maybe a function like this:
> struct clk *clk_get_by_sysname(const char *sysname);
> could be useful to be added to api interface, too ?

I'm not going to reply unless you copy your replies to linux-arm-kernel.
By not copying that list, you're excluding maintainers for quite a number
of implementation maintainers from this discussion.
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] User access to internal clocks

[Davide Rizzo]

What about adding a dedicated function to clk api that registers a struct clk
 in a linked list and associates it with a name, with the only specific purpose
 to enumerate and list (and possibly change rate and parents) all
clocks in sysfs ?
Obviously only specifically registered clocks will be visible in sysfs...

Or better, only proposed functions:
int clk_for_each(int(*fn)(struct clk *, void *), void *data)
and
const char *clk_sysname(struct clk *)
need to be added to clk api, the function to register the struct clk
in the linked list could be implementation specific.
Maybe a function like this:
struct clk *clk_get_by_sysname(const char *sysname);
could be useful to be added to api interface, too ?
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html