Re: [PATCH RFC] clk: Introduce userspace clock driver

[SÃren Brinkmann]

On Fri, May 10, 2013 at 03:18:10PM -0700, Mike Turquette wrote:
> On Fri, May 10, 2013 at 11:49 AM, Emilio LÃpez <emilio@xxxxxxxxxxxxx> wrote:
> > Hi,
> >
> > El 10/05/13 15:15, SÃren Brinkmann escribiÃ:
> >> Hi Emilio,
> >>
> >> On Fri, May 10, 2013 at 02:44:44PM -0300, Emilio LÃpez wrote:
> >>> Hi,
> >>>
> >>> El 10/05/13 14:31, Soren Brinkmann escribiÃ:
> >>>> The userspace clock driver can be used to expose clock controls through
> >>>> sysfs to userspace. The driver creates entries in /sys/class/clk.
> >>>>
> >>>> Signed-off-by: Soren Brinkmann <soren.brinkmann@xxxxxxxxxx>
> >>>> ---
> >>>>  .../devicetree/bindings/clock/clk-userspace.txt    |   7 +
> >>>>  drivers/clk/Kconfig                                |   9 ++
> >>>>  drivers/clk/Makefile                               |   1 +
> >>>>  drivers/clk/clk-userspace.c                        | 169 +++++++++++++++++++++
> >>>>  4 files changed, 186 insertions(+)
> >>>>  create mode 100644 Documentation/devicetree/bindings/clock/clk-userspace.txt
> >>>>  create mode 100644 drivers/clk/clk-userspace.c
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/clock/clk-userspace.txt b/Documentation/devicetree/bindings/clock/clk-userspace.txt
> >>>> new file mode 100644
> >>>> index 0000000..2d153c7
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/clock/clk-userspace.txt
> >>>> @@ -0,0 +1,7 @@
> >>>> +
> >>>> +Example:
> >>>> +   usclk: usclk {
> >>>> +           compatible = "clk-userspace";
> >>>> +           clocks = <&foo 15>, <&bar>;
> >>>> +           clock-count = <2>;
> >>>> +   };
> >>>
> >>> Does this belong on DT? It isn't describing hardware, is it?
> >> I guess, strictly speaking you are right. Do you have a good
> >> alternative?
> >
> > If this was part of the framework instead of a consumer, I suppose a
> > flag on the DT node defining the clock that indicates it should be
> > exported would be acceptable.
> >
> > Another possibility would be letting the user export what they need,
> > like GPIO does, see "Paths in Sysfs" in
> >
> > https://www.kernel.org/doc/Documentation/gpio.txt
> >
> >>>> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> >>>> index 0357ac4..b35b62c 100644
> >>>> --- a/drivers/clk/Kconfig
> >>>> +++ b/drivers/clk/Kconfig
> >>>> @@ -81,6 +81,15 @@ config COMMON_CLK_AXI_CLKGEN
> >>>>       Support for the Analog Devices axi-clkgen pcore clock generator for Xilinx
> >>>>       FPGAs. It is commonly used in Analog Devices' reference designs.
> >>>>
> >>>> +config COMMON_CLK_USERSPACE
> >>>> +   bool "Userspace Clock Controls"
> >>>> +   depends on OF
> >>>> +   depends on SYSFS
> >>>> +   help
> >>>> +   ---help---
> >>>> +     Expose clock controls through sysfs to userspace. Clocks are selected
> >>>> +     through the device tree and the controls are exposed in
> >>>> +     /sys/class/clk.
> >>>>  endmenu
> >>>>
> >>>>  source "drivers/clk/mvebu/Kconfig"
> >>>> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> >>>> index fa435bc..f2f68c8 100644
> >>>> --- a/drivers/clk/Makefile
> >>>> +++ b/drivers/clk/Makefile
> >>>> @@ -8,6 +8,7 @@ obj-$(CONFIG_COMMON_CLK)    += clk-fixed-rate.o
> >>>>  obj-$(CONFIG_COMMON_CLK)   += clk-gate.o
> >>>>  obj-$(CONFIG_COMMON_CLK)   += clk-mux.o
> >>>>  obj-$(CONFIG_COMMON_CLK)   += clk-composite.o
> >>>> +obj-$(CONFIG_COMMON_CLK_USERSPACE) += clk-userspace.o
> >>>>
> >>>>  # SoCs specific
> >>>>  obj-$(CONFIG_ARCH_BCM2835) += clk-bcm2835.o
> >>>> diff --git a/drivers/clk/clk-userspace.c b/drivers/clk/clk-userspace.c
> >>>> new file mode 100644
> >>>> index 0000000..931cf92
> >>>> --- /dev/null
> >>>> +++ b/drivers/clk/clk-userspace.c
> >>>> @@ -0,0 +1,169 @@
> >>>> +/*
> >>>> + * Userspace clock driver
> >>>> + *
> >>>> + *  Copyright (C) 2013 Xilinx
> >>>> + *
> >>>> + *  SÃren Brinkmann <soren.brinkmann@xxxxxxxxxx>
> >>>> + *
> >>>> + * This program is free software: you can redistribute it and/or modify
> >>>> + * it under the terms of the GNU General Public License v2 as published by
> >>>> + * the Free Software Foundation.
> >>>> + *
> >>>> + * 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, see <http://www.gnu.org/licenses/>.
> >>>> + *
> >>>> + * Expose clock controls through sysfs to userspace.
> >>>> + *
> >>>> + * By writing 0/1 to 'enable' the clock can be disabled/enabled. Reading
> >>>> + * that file returns the current state - 0 = disabled, 1 = enabled.
> >>>> + *
> >>>> + * Reading 'set_rate' returns the current clock frequency in Hz. Writing
> >>>> + * the file requests setting a new frequency in Hz.
> >>>> + */
> >>>> +
> >>>> +#include <linux/clk-provider.h>
> >>>> +#include <linux/fs.h>
> >>>> +#include <linux/module.h>
> >>>> +#include <linux/of.h>
> >>>> +#include <linux/device.h>
> >>>> +#include <linux/slab.h>
> >>>> +
> >>>> +#define DRIVER_NAME        "clk-userspace"
> >>>> +
> >>>> +struct usclk_data {
> >>>> +   struct clk *clk;
> >>>> +   int enabled;
> >>>> +};
> >>>> +
> >>>> +static ssize_t enable_show(struct device *dev, struct device_attribute *attr,
> >>>> +           char *buf)
> >>>> +{
> >>>> +   struct usclk_data *pdata = dev_get_drvdata(dev);
> >>>> +
> >>>> +   return scnprintf(buf, PAGE_SIZE, "%u\n", pdata->enabled);
> >>>> +}
> >>>> +
> >>>> +static ssize_t enable_store(struct device *dev, struct device_attribute *attr,
> >>>> +           const char *buf, size_t count)
> >>>> +{
> >>>> +   unsigned long enable;
> >>>> +   int ret;
> >>>> +   struct usclk_data *pdata = dev_get_drvdata(dev);
> >>>> +
> >>>> +   ret = kstrtoul(buf, 0, &enable);
> >>>> +   if (ret)
> >>>> +           return -EINVAL;
> >>>> +
> >>>> +   enable = !!enable;
> >>>> +   if (enable == pdata->enabled)
> >>>> +           return count;
> >>>> +
> >>>> +   if (enable)
> >>>> +           ret = clk_prepare_enable(pdata->clk);
> >>>> +   else
> >>>> +           clk_disable_unprepare(pdata->clk);
> >>>> +
> >>>> +   if (ret)
> >>>> +           return -EBUSY;
> >>>> +
> >>>> +   pdata->enabled = enable;
> >>>> +   return count;
> >>>> +}
> >>>> +
> >>>> +static DEVICE_ATTR(enable, 0644, enable_show, enable_store);
> >>>> +
> >>>> +static ssize_t set_rate_show(struct device *dev, struct device_attribute *attr,
> >>>> +           char *buf)
> >>>> +{
> >>>> +   struct usclk_data *pdata = dev_get_drvdata(dev);
> >>>> +
> >>>> +   return scnprintf(buf, PAGE_SIZE, "%lu\n", clk_get_rate(pdata->clk));
> >>>> +}
> >>>> +
> >>>> +static ssize_t set_rate_store(struct device *dev, struct device_attribute *attr,
> >>>> +           const char *buf, size_t count)
> >>>> +{
> >>>> +   int ret = 0;
> >>>> +   unsigned long rate;
> >>>> +   struct usclk_data *pdata = dev_get_drvdata(dev);
> >>>> +
> >>>> +   ret = kstrtoul(buf, 0, &rate);
> >>>> +   if (ret)
> >>>> +           return -EINVAL;
> >>>> +
> >>>> +   rate = clk_round_rate(pdata->clk, rate);
> >>>> +   ret = clk_set_rate(pdata->clk, rate);
> >>>> +   if (ret)
> >>>> +           return -EBUSY;
> >>>> +
> >>>> +   return count;
> >>>> +}
> >>>> +
> >>>> +static DEVICE_ATTR(set_rate, 0644, set_rate_show, set_rate_store);
> >>>> +
> >>>> +static const struct attribute *usclk_attrs[] = {
> >>>> +   &dev_attr_enable.attr,
> >>>> +   &dev_attr_set_rate.attr,
> >>>> +   NULL
> >>>> +};
> >>>
> >>> For debugging purposes, being able to change parents would be nice too.
> >> This is difficult and I don't have a good solution for it, hence it's
> >> missing. A clock consumer like a device driver or this driver, just
> >> knows about it's input clock, but not about the topology further up.
> >> Therefore it is pretty much impossible to implement reparent operations
> >> in a clock consumer, IMHO.
> >> IOW: For a given input clock, how do you figure out it's possible
> >> parents?
> >
> > The parent is just a number
> >
> > int (*set_parent)(struct clk_hw *hw, u8 index);
> > u8 (*get_parent)(struct clk_hw *hw);
> >
> > If you are debugging, you know what the possible parents are, and you
> > can reparent with that information.
> >
> > After checking the clk code however, I didn't find any exposed way to
> > reparent with just the parent indexes. Maybe an interface that takes a n
> > arbitrary string representing the parent name, and gets that clock and
> > then sets the parent would fit.
> >
> >>
> >>> Maybe this belongs to debugfs instead of sysfs though.
> >> Well, the more generic use-case probably. My Zynq use-case rather not,
> >> IMHO.
> >
> > The framework already exposes some information on debugfs, maybe
> > expanding that instead of implementing it as a consumer on sysfs would
> > be best for the debugging use case. @Mike, what's your thoughts on this?
> >
> 
> In the previous thread on this topic we discussed a generic approach
> to exposing clock controls via debugfs.
I have to search for this. I didn't see that discussion.

> 
> One way to do it is to introduce a new config option,
> CONFIG_COMMON_CLK_DEBUG_CONTROL that would expose the controls for
> every clock in the existing debugfs infrastructure.  The downside to
> this approach is that it would get abused and ship in millions of
> Android products using horrible userspace hacks to control clocks.
> Maybe that's not our problem to solve, maybe it is.
> 
> If CONFIG_COMMON_CLK_DEBUG_CONTROL existed it might be a good idea to
> intentionally break the abi compatibility with every new release.
> That would certainly reinforce that this is not a condoned or stable
> api (which is true for all debugfs).
I kinda like these ideas. The unstable API may be a problem though.
Also, I preferred a solution which limits the exposed controls to a few
selected clocks, instead of exposing them all. My thinking was, that I
have those mentioned FPGA clocks which are likely to be exported, but
everything else should not be exposed.
For debugging though, there is no reason for this limitation.

> 
> I think that Soren wants something with a stable interface that he can
> use for his Zynq use case.  Regarding that, why not write an actual
> device driver to do what you want to do from userspace?
An "actual device driver" would not look that different than this one,
would it?
I could change the probing mechanism a little bit to make it an actual
device - w/o being a physical device though. But I don't think it would
look much different.
So, in the end it would be a platform device which is calling clk_get()
and then exposes enable and set_rate functionality to sysfs. I.e. this
device driver is rather a dummy and could be used by anybody to control
any clock visible in DT, hence my approach to make it a generic driver.
I tried avoiding the 'device driver' solution, because that would mean
adding a device node in DT on the platform bus for something which is
not an actual device - which I ended up to do for this one anyway, but
making it part of debugfs and the core framework might work.

Looks a bit like, that the debugging use case and Zynq have too
different requirements.

	SÃren


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