Date: Fri, 10 May 2013 11:15:25 -0700
From: =?utf-8?B?U8O2cmVu?= Brinkmann <soren.brinkmann@xilinx.com>
To: Emilio =?utf-8?B?TMOzcGV6?= <emilio@elopez.com.ar>
CC: Mike Turquette <mturquette@linaro.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH RFC] clk: Introduce userspace clock driver
References: <1368207091-32538-1-git-send-email-soren.brinkmann@xilinx.com>
 <1368207091-32538-2-git-send-email-soren.brinkmann@xilinx.com>
 <518D320C.4010707@elopez.com.ar>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
In-Reply-To: <518D320C.4010707@elopez.com.ar>
User-Agent: Mutt/1.5.21 (2010-09-15)
Message-ID: <fe9e603e-a432-4a29-a195-fd393b875a7e@TX2EHSMHS006.ehs.local>
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 7064
Lines: 211

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@xilinx.com>
> > ---
> >  .../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?

> 
> > 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@xilinx.com>
> > + *
> > + * 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?

> Maybe this belongs to debugfs instead of sysfs though.
Well, the more generic use-case probably. My Zynq use-case rather not,
IMHO.

	Sören


--
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/