[RFC] clk: Introduce userspace clock driver

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

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?

> 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.
Maybe this belongs to debugfs instead of sysfs though.

> +
> +static const struct attribute_group usclk_attr_grp = {
> +	.attrs = (struct attribute **)usclk_attrs,
> +};
> +
> +static int usclk_setup(void)
> +{
> +	int ret;
> +	int i;
> +	struct usclk_data *pdata;
> +	u32 clock_count;
> +	struct class *clk_class;
> +	struct device *dev;
> +	struct device_node *np = of_find_compatible_node(NULL, NULL,
> +			"clk-userspace");
> +
> +	ret = of_property_read_u32(np, "clock-count", &clock_count);
> +	if (ret || !clock_count)
> +		return ret;
> +
> +	pdata = kzalloc(clock_count * sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;
> +
> +	clk_class = class_create(THIS_MODULE, "clk");
> +	if (!clk_class) {
> +		pr_err("unable to create class\n");
> +		goto err_free;
> +	}
> +
> +	for (i = 0; i < clock_count; i++) {
> +		pdata[i].clk = of_clk_get(np, i);
> +		if (IS_ERR(pdata[i].clk)) {
> +			pr_warn("input clock #%u not found\n", i);
> +			continue;
> +		}
> +
> +		dev = device_create(clk_class, NULL, MKDEV(0, 0), NULL,
> +				of_clk_get_parent_name(np, i));
> +		if (!dev) {
> +			pr_warn("unable to create device #%d\n", i);
> +			continue;
> +		}
> +
> +		dev_set_drvdata(dev, &pdata[i]);
> +		sysfs_create_group(&dev->kobj, &usclk_attr_grp);
> +	}
> +
> +	return 0;
> +
> +err_free:
> +	kfree(pdata);
> +
> +	return ret;
> +}
> +late_initcall(usclk_setup);
>

Hi  Andy,

first, can you please send plain text email? The formatting gets messed up a
bit for me otherwise.

On Fri, May 10, 2013 at 08:42:34PM +0300, Andy Shevchenko wrote:
>    10.5.2013 20.32 "Soren Brinkmann" <soren.brinkmann@xilinx.com> kirjoitti:
>    >
>    > 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>;
>    > +       };
>    > 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);
> 
>    strbool()?
Can you explain this a little more, please? I grepped the kernel sources for
'strbool' and didn't get a single match.

	Sören

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

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@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?

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@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?

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?

Emilio

On Fri, May 10, 2013 at 10:31:31AM -0700, Soren Brinkmann wrote:

> +Example:
> +	usclk: usclk {
> +		compatible = "clk-userspace";
> +		clocks = <&foo 15>, <&bar>;
> +		clock-count = <2>;
> +	};

This is clearly *very* Linux specific so needs to be a linux vendor
thing (everything should have a namespaced name anyway).  It's not at
all obvious to me that this should be in device tree, though, since it's
not hardware description but a detail of how the OS is currently
implemented.

For your use case should these things be exposed by the FPGA device
asking for that rather than by having the clocks available separately?
Or is this part of the DT blob that's loaded incrementally along with
the FPGA (which does make things more interesting of course...).

> + * 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.

This needs to be covered in Documentation/ABI since it's adding new
sysfs stuff.

> +	if (enable)
> +		ret = clk_prepare_enable(pdata->clk);
> +	else
> +		clk_disable_unprepare(pdata->clk);

> +	if (ret)
> +		return -EBUSY;

Why not pass back the actual error?

> +	pdata = kzalloc(clock_count * sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;

devm?

> +late_initcall(usclk_setup);

Why not just a regular driver?

On Fri, May 10, 2013 at 11:49 AM, Emilio López <emilio@elopez.com.ar> 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@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?
>
> 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@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?
>
> 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.

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

Regards,
Mike

> Emilio

On 05/10/2013 03:18 PM, Mike Turquette wrote:
> On Fri, May 10, 2013 at 11:49 AM, Emilio López <emilio@elopez.com.ar> 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@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?
>>
>> 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@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?
>>
>> 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.
>
> 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.

We already have this for MSM. But I seem to have managed to keep our 
userspace guys away from abusing it. YMMV.

> 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).

+1 if we can do this. Just in a minor way so that we don't end up making 
it unusable for humans. We also have userspace test scripts for that 
that we can try to upstream (I can't guarantees) -- so we can't go all 
crazy when we do the intentional ABI breaking. We could make them 
root-only in hopes of discouraging abuse of the API. In the sense, using 
this API introduces security concerns because their userspace will be 
running as root.

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

Exposing clock control to userspace production use is a terrible idea. A 
misbehaving userspace can easily kill the system. This is not so try for 
GPIO. So, exposing GPIOs to userspace is relatively less of a concern.

-Saravana

On Fri, May 10, 2013 at 04:01:25PM -0700, Saravana Kannan wrote:
> On 05/10/2013 03:18 PM, Mike Turquette wrote:
> >On Fri, May 10, 2013 at 11:49 AM, Emilio López <emilio@elopez.com.ar> 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@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?
> >>
> >>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@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?
> >>
> >>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.
> >
> >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.
> 
> We already have this for MSM. But I seem to have managed to keep our
> userspace guys away from abusing it. YMMV.
> 
> >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).
> 
> +1 if we can do this. Just in a minor way so that we don't end up
> making it unusable for humans. We also have userspace test scripts
> for that that we can try to upstream (I can't guarantees) -- so we
> can't go all crazy when we do the intentional ABI breaking. We could
> make them root-only in hopes of discouraging abuse of the API. In
> the sense, using this API introduces security concerns because their
> userspace will be running as root.
> 
> >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?
> 
> Exposing clock control to userspace production use is a terrible
> idea. A misbehaving userspace can easily kill the system. This is
> not so try for GPIO. So, exposing GPIOs to userspace is relatively
> less of a concern.
Well, the FPGA clocks are only used by stuff in the FPGA. They cannot
mess up the Linux on the A9s. I my use-case is kinda special. And people
request functionality to easily adjust the frequency for their FPGA
design in SW from Linux.
Nevertheless, there is no real protection from taking the driver I'm
proposing to control the FPGA clocks to control a clock vital to the
system.

	Sören

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@elopez.com.ar> 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@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?
> >
> > 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@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?
> >
> > 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

On 05/10/2013 04:06 PM, Sören Brinkmann wrote:
> On Fri, May 10, 2013 at 04:01:25PM -0700, Saravana Kannan wrote:
>> On 05/10/2013 03:18 PM, Mike Turquette wrote:

>>> 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?
>>
>> Exposing clock control to userspace production use is a terrible
>> idea. A misbehaving userspace can easily kill the system. This is
>> not so try for GPIO. So, exposing GPIOs to userspace is relatively
>> less of a concern.
> Well, the FPGA clocks are only used by stuff in the FPGA. They cannot
> mess up the Linux on the A9s. I my use-case is kinda special. And people
> request functionality to easily adjust the frequency for their FPGA
> design in SW from Linux.

How do you talk to the FPGA? What happens if the FPGA clock gets turned 
off when the Linux is communicating with it? At the least the I2C or 
whatever bus you used to talk to it could hang. You need to explain more 
about why it's "special" before people might turn around to give 
userspace ABI for clock control.

> Nevertheless, there is no real protection from taking the driver I'm
> proposing to control the FPGA clocks to control a clock vital to the
> system.

If we are talking about changing the kernel to control different clocks, 
that true for any driver.

If your idea of this driver was something that will take a clock name 
and rate and change that clock's rate, then that's not a good design. 
What Mike probably meant was a FPGA specific driver that will only 
clk_get() the clocks related to the FPGA, and expose options to 
userspace. Not the actual rate or enable/disable capability.

For example, opening the device could cause clk_prepare_enable() and 
closing it would cause clk_disable_unprepare(). You might have ioctls to 
let userspace pick one of different modes of operation with each 
corresponding to a different clock rate and other corresponding FPGA 
configuration changes, etc. That's just a rough sketch. If you write 
such a driver, userspace can't misuse it to mess with other clocks or 
leave the FPGA clock in a bad state.

-Saravana

On Fri, May 10, 2013 at 04:25:45PM -0700, Saravana Kannan wrote:
> On 05/10/2013 04:06 PM, Sören Brinkmann wrote:
> >On Fri, May 10, 2013 at 04:01:25PM -0700, Saravana Kannan wrote:
> >>On 05/10/2013 03:18 PM, Mike Turquette wrote:
> 
> >>>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?
> >>
> >>Exposing clock control to userspace production use is a terrible
> >>idea. A misbehaving userspace can easily kill the system. This is
> >>not so try for GPIO. So, exposing GPIOs to userspace is relatively
> >>less of a concern.
> >Well, the FPGA clocks are only used by stuff in the FPGA. They cannot
> >mess up the Linux on the A9s. I my use-case is kinda special. And people
> >request functionality to easily adjust the frequency for their FPGA
> >design in SW from Linux.
> 
> How do you talk to the FPGA? What happens if the FPGA clock gets
> turned off when the Linux is communicating with it? At the least the
> I2C or whatever bus you used to talk to it could hang. You need to
> explain more about why it's "special" before people might turn
> around to give userspace ABI for clock control.
> 
> >Nevertheless, there is no real protection from taking the driver I'm
> >proposing to control the FPGA clocks to control a clock vital to the
> >system.
> 
> If we are talking about changing the kernel to control different
> clocks, that true for any driver.
> 
> If your idea of this driver was something that will take a clock
> name and rate and change that clock's rate, then that's not a good
> design. What Mike probably meant was a FPGA specific driver that
> will only clk_get() the clocks related to the FPGA, and expose
> options to userspace. Not the actual rate or enable/disable
> capability.
How? You do this through device tree. If you give that driver a
different clock than the one it should get it might mess up. But this
does apply to all device drivers. Assume you give you ethernet driver
the wrong clock reference. When it tries to adjust the link speed it
will mess up the clock. There is no protection against this.

> For example, opening the device could cause clk_prepare_enable() and
> closing it would cause clk_disable_unprepare().
In the current state: Enable/disable is explicitly done through the 'enable'
file in sysfs.  The driver takes care of that all enable/disable is
balanced. I.e. prepare_enable is called if non-zero is written to enable
and the driver didn't enable the clock yet and similar for disable.

> You might have
> ioctls to let userspace pick one of different modes of operation
> with each corresponding to a different clock rate and other
> corresponding FPGA configuration changes, etc.
It's an FPGA and therefore fully programmable even during runtime.

> That's just a rough
> sketch. If you write such a driver, userspace can't misuse it to
> mess with other clocks or leave the FPGA clock in a bad state.
Currently, you cannot mess with the enable counts and whether the
frequency is sane or not is up to the user. Unless you have a known
design and appropriate DT bindings there is no way of knowing this
upfront, which is kinda the point of having the driver.
The first use case in my mind is just some simple bring up. Having the
clocks let some LEDs blink and now let them blink faster/slower by
changing the frequency.

	Sören

On Fri, May 10, 2013 at 04:01:25PM -0700, Saravana Kannan wrote:
> On 05/10/2013 03:18 PM, Mike Turquette wrote:

Guys please delete irrelevant context from replies...

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

> We already have this for MSM. But I seem to have managed to keep our
> userspace guys away from abusing it. YMMV.

It's much harder when it's in the standard kernel and there's no contact
with some of the users.  I've pushed back pretty strongly on some of
your equivalent stuff for regulators for this reason.

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

> Exposing clock control to userspace production use is a terrible
> idea. A misbehaving userspace can easily kill the system. This is
> not so try for GPIO. So, exposing GPIOs to userspace is relatively
> less of a concern.

Pick the wrong GPIO and you face the same issue.

Note that we do have a UIO framework in place so there is some concept
of doing this sort of thing; that is all about picking specific things
and exposing them (in a somewhat similar fashion to this) rather than a
default available thing though.

On Fri, May 10, 2013 at 10:24:22PM +0100, Mark Brown wrote:
> On Fri, May 10, 2013 at 10:31:31AM -0700, Soren Brinkmann wrote:
> 
> > +Example:
> > +	usclk: usclk {
> > +		compatible = "clk-userspace";
> > +		clocks = <&foo 15>, <&bar>;
> > +		clock-count = <2>;
> > +	};
> 
> This is clearly *very* Linux specific so needs to be a linux vendor
> thing (everything should have a namespaced name anyway).  It's not at
> all obvious to me that this should be in device tree, though, since it's
> not hardware description but a detail of how the OS is currently
> implemented.
> 
> For your use case should these things be exposed by the FPGA device
> asking for that rather than by having the clocks available separately?
> Or is this part of the DT blob that's loaded incrementally along with
> the FPGA (which does make things more interesting of course...).
Here may be some misunderstanding.
The clocks are not in the FPGA. The clocks are always there and part of
the processing system (PS), they are just routed to the FPGA where they
can be used as clocks for the FPGA design.

So, hopefully, in most cases there will be a "normal" device drivers for the
IP in the FPGA. E.g. if there is a soft IP Ethernet core in the FPGA an
appropriate device driver will use clk_get() to claim the appropriate
clock from its provider, which can be one of these FPGA clocks.
In this case there is no userspace exposure needed at all and the here
proposed driver is not used.

But if your design isn't that complex or there is no device driver
available for it (yet), there is the desire to have simple userspace
controls to adjust the frequency easily during runtime. In this case the
proposed driver can be used to expose the clock controls for the wanted
clocks through sysfs.

I hope that will be the way it used. I didn't intent to provide a
userspace API people rely on for more than some simple adjustments.
The more complex the IP in the FPGA becomes, the requirements and
limitations on clocking become more complex too. Then people will have
to write/use a proper dedicated driver for the IP, IMHO.

> 
> > + * 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.
> 
> This needs to be covered in Documentation/ABI since it's adding new
> sysfs stuff.
I wanted to avoid becoming the provider of a stable userspace API. We'll
see how this goes.

> 
> > +	if (enable)
> > +		ret = clk_prepare_enable(pdata->clk);
> > +	else
> > +		clk_disable_unprepare(pdata->clk);
> 
> > +	if (ret)
> > +		return -EBUSY;
> 
> Why not pass back the actual error?
Good question. Was there some spec saying, that these sysfs callbacks
should return this error? Otherwise this will be fixed.

> 
> > +	pdata = kzalloc(clock_count * sizeof(*pdata), GFP_KERNEL);
> > +	if (!pdata)
> > +		return -ENOMEM;
> 
> devm?
In the current state, there is no real device and devices aren't
destroyed either. If this changes the managed framework may become an
option.

> 
> > +late_initcall(usclk_setup);
> 
> Why not just a regular driver?
Looks like I'll go that way. I didn't want it originally, since there
is no physical HW for this driver. I wanted to use the clock probing
mechanism, but ran into problems with that because it takes place too
early during boot.


	Sören

On Sat, May 11, 2013 at 09:54:22AM -0700, Sören Brinkmann wrote:
> On Fri, May 10, 2013 at 10:24:22PM +0100, Mark Brown wrote:

> > For your use case should these things be exposed by the FPGA device
> > asking for that rather than by having the clocks available separately?
> > Or is this part of the DT blob that's loaded incrementally along with
> > the FPGA (which does make things more interesting of course...).

> Here may be some misunderstanding.
> The clocks are not in the FPGA. The clocks are always there and part of
> the processing system (PS), they are just routed to the FPGA where they
> can be used as clocks for the FPGA design.

No, there's no confusion here - the clocks that are being exposed to
userspace are the clocks which enter the FPGA.  The driver or whatever
that understands the FPGA can do what is needed to control them,
including routing them on to subdevices it instantiates or exposing them
to userspace.

> > > +		clk_disable_unprepare(pdata->clk);
> > 
> > > +	if (ret)
> > > +		return -EBUSY;
> > 
> > Why not pass back the actual error?

> Good question. Was there some spec saying, that these sysfs callbacks
> should return this error? Otherwise this will be fixed.

Not to my knowledge.

On Sun, May 12, 2013 at 06:33:44PM +0400, Mark Brown wrote:
> On Sat, May 11, 2013 at 09:54:22AM -0700, Sören Brinkmann wrote:
> > On Fri, May 10, 2013 at 10:24:22PM +0100, Mark Brown wrote:
> 
> > > For your use case should these things be exposed by the FPGA device
> > > asking for that rather than by having the clocks available separately?
> > > Or is this part of the DT blob that's loaded incrementally along with
> > > the FPGA (which does make things more interesting of course...).
> 
> > Here may be some misunderstanding.
> > The clocks are not in the FPGA. The clocks are always there and part of
> > the processing system (PS), they are just routed to the FPGA where they
> > can be used as clocks for the FPGA design.
> 
> No, there's no confusion here - the clocks that are being exposed to
> userspace are the clocks which enter the FPGA.  The driver or whatever
> that understands the FPGA can do what is needed to control them,
> including routing them on to subdevices it instantiates or exposing them
> to userspace.
Such a driver does not exist in general.
For some IP cores, Linux drivers do exist and then
they are supposed to directly use the CCF, IMHO, no need to expose
things to userspace in that case.
I'm trying to cover cases, in which there is no driver available/needed for
the FPGA design, other than some simple clock controls.

As simple example, if you wrote your own HW blinken lights design, you
wouldn't have nor need a Linux driver for it, but if you used these
clocks as inputs you could change the blinking speed by adjusting the
frequency.

	Sören

On Sun, May 12, 2013 at 12:05:04PM -0700, Sören Brinkmann wrote:
> On Sun, May 12, 2013 at 06:33:44PM +0400, Mark Brown wrote:

> > No, there's no confusion here - the clocks that are being exposed to
> > userspace are the clocks which enter the FPGA.  The driver or whatever
> > that understands the FPGA can do what is needed to control them,
> > including routing them on to subdevices it instantiates or exposing them
> > to userspace.

> Such a driver does not exist in general.
> For some IP cores, Linux drivers do exist and then
> they are supposed to directly use the CCF, IMHO, no need to expose
> things to userspace in that case.
> I'm trying to cover cases, in which there is no driver available/needed for
> the FPGA design, other than some simple clock controls.

You're not understanding the point here.  If you've got a
reprogrammmable FPGA you at least need some way to get the FPGA image in
there.  This driver is presumably responsible for instantiating whatever
is needed to control what is on the FPGA, that could include punting the
clocks to userspace if that's sane.

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

We are doing the same. I don't think we can prevent people from abusing this.
If we don't provide it, they will just implement it themselves :)

> 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 would rather not have to change our automated tests for every new release
though...

Cheers,

Peter.

On Mon, May 13, 2013 at 09:21:35AM +0400, Mark Brown wrote:
> On Sun, May 12, 2013 at 12:05:04PM -0700, Sören Brinkmann wrote:
> > On Sun, May 12, 2013 at 06:33:44PM +0400, Mark Brown wrote:
> 
> > > No, there's no confusion here - the clocks that are being exposed to
> > > userspace are the clocks which enter the FPGA.  The driver or whatever
> > > that understands the FPGA can do what is needed to control them,
> > > including routing them on to subdevices it instantiates or exposing them
> > > to userspace.
> 
> > Such a driver does not exist in general.
> > For some IP cores, Linux drivers do exist and then
> > they are supposed to directly use the CCF, IMHO, no need to expose
> > things to userspace in that case.
> > I'm trying to cover cases, in which there is no driver available/needed for
> > the FPGA design, other than some simple clock controls.
> 
> You're not understanding the point here.  If you've got a
> reprogrammmable FPGA you at least need some way to get the FPGA image in
> there.  This driver is presumably responsible for instantiating whatever
> is needed to control what is on the FPGA, that could include punting the
> clocks to userspace if that's sane.
Well, that driver actually exists. But that just programs a bitstream
you give it to program. It does not know anything about the design it
programs and cannot make any kind of decision whether the clocks should
be userspace controlled or not.

	Sören

On 05/13/13 18:09, Sören Brinkmann wrote:
> On Mon, May 13, 2013 at 09:21:35AM +0400, Mark Brown wrote:
>> On Sun, May 12, 2013 at 12:05:04PM -0700, Sören Brinkmann wrote:
>>> On Sun, May 12, 2013 at 06:33:44PM +0400, Mark Brown wrote:
>>>> No, there's no confusion here - the clocks that are being exposed to
>>>> userspace are the clocks which enter the FPGA.  The driver or whatever
>>>> that understands the FPGA can do what is needed to control them,
>>>> including routing them on to subdevices it instantiates or exposing them
>>>> to userspace.
>>
>>> Such a driver does not exist in general.
>>> For some IP cores, Linux drivers do exist and then
>>> they are supposed to directly use the CCF, IMHO, no need to expose
>>> things to userspace in that case.
>>> I'm trying to cover cases, in which there is no driver available/needed for
>>> the FPGA design, other than some simple clock controls.
>>
>> You're not understanding the point here.  If you've got a
>> reprogrammmable FPGA you at least need some way to get the FPGA image in
>> there.  This driver is presumably responsible for instantiating whatever
>> is needed to control what is on the FPGA, that could include punting the
>> clocks to userspace if that's sane.
> Well, that driver actually exists. But that just programs a bitstream
> you give it to program. It does not know anything about the design it
> programs and cannot make any kind of decision whether the clocks should
> be userspace controlled or not.

Soeren,

what Mark wants to point out is that you add fabric clocks to the Xilinx
driver instead. This way, you will have user-space controllable clocks
but only if you loaded the xilinx driver first.

IIRC the fabric clock controller provided by Zynq _is_ always there and
accessible from ARM CPUs. You just don't have a new generic driver
allowing to poke with all clocks, but a xilinx only driver allowing you
to set the (xilinx only) fabric clocks.

I've played with Zynq a while ago, did Xilinx mainline the bitfile
driver already? If not, why don't you give it a shot?

Sebastian

On Mon, May 13, 2013 at 06:21:13PM +0200, Sebastian Hesselbarth wrote:
> On 05/13/13 18:09, Sören Brinkmann wrote:
> >On Mon, May 13, 2013 at 09:21:35AM +0400, Mark Brown wrote:
> >>On Sun, May 12, 2013 at 12:05:04PM -0700, Sören Brinkmann wrote:
> >>>On Sun, May 12, 2013 at 06:33:44PM +0400, Mark Brown wrote:
> >>>>No, there's no confusion here - the clocks that are being exposed to
> >>>>userspace are the clocks which enter the FPGA.  The driver or whatever
> >>>>that understands the FPGA can do what is needed to control them,
> >>>>including routing them on to subdevices it instantiates or exposing them
> >>>>to userspace.
> >>
> >>>Such a driver does not exist in general.
> >>>For some IP cores, Linux drivers do exist and then
> >>>they are supposed to directly use the CCF, IMHO, no need to expose
> >>>things to userspace in that case.
> >>>I'm trying to cover cases, in which there is no driver available/needed for
> >>>the FPGA design, other than some simple clock controls.
> >>
> >>You're not understanding the point here.  If you've got a
> >>reprogrammmable FPGA you at least need some way to get the FPGA image in
> >>there.  This driver is presumably responsible for instantiating whatever
> >>is needed to control what is on the FPGA, that could include punting the
> >>clocks to userspace if that's sane.
> >Well, that driver actually exists. But that just programs a bitstream
> >you give it to program. It does not know anything about the design it
> >programs and cannot make any kind of decision whether the clocks should
> >be userspace controlled or not.
> 
> Soeren,
> 
> what Mark wants to point out is that you add fabric clocks to the Xilinx
> driver instead. This way, you will have user-space controllable clocks
> but only if you loaded the xilinx driver first.
What "Xilinx driver", are we talking about?

> 
> IIRC the fabric clock controller provided by Zynq _is_ always there and
> accessible from ARM CPUs. You just don't have a new generic driver
> allowing to poke with all clocks, but a xilinx only driver allowing you
> to set the (xilinx only) fabric clocks.
Right.

> 
> I've played with Zynq a while ago, did Xilinx mainline the bitfile
> driver already? If not, why don't you give it a shot?
The device config driver is not in mainline, AFAIK. And I think it's in
rather bad shape and needs a lot of work before it is upstreamable. But
this is probably the right place to put this.
On the other hand, currently this driver is only required for
programming the FPGA from Linux, which is not required. One of the
bootloaders could do this for you earlier.

	Sören

On 05/13/13 19:24, Sören Brinkmann wrote:
> On Mon, May 13, 2013 at 06:21:13PM +0200, Sebastian Hesselbarth wrote:
>>> Well, that driver actually exists. But that just programs a bitstream
>>> you give it to program. It does not know anything about the design it
>>> programs and cannot make any kind of decision whether the clocks should
>>> be userspace controlled or not.
>>
>> what Mark wants to point out is that you add fabric clocks to the Xilinx
>> driver instead. This way, you will have user-space controllable clocks
>> but only if you loaded the xilinx driver first.
> What "Xilinx driver", are we talking about?

The device config driver you mention below.

>> IIRC the fabric clock controller provided by Zynq _is_ always there and
>> accessible from ARM CPUs. You just don't have a new generic driver
>> allowing to poke with all clocks, but a xilinx only driver allowing you
>> to set the (xilinx only) fabric clocks.
> Right.
>
>> I've played with Zynq a while ago, did Xilinx mainline the bitfile
>> driver already? If not, why don't you give it a shot?
> The device config driver is not in mainline, AFAIK. And I think it's in
> rather bad shape and needs a lot of work before it is upstreamable. But
> this is probably the right place to put this.

It is, as it will only expose clocks on Zynq and that's what Mark and
Mike are worried about. Expose clocks to user space and you will have
people mess with it for sure.

About the shape of it, I didn't expect that to change at all. Just
wondering, if it still requires you to manually end it's endianess
mess with the bitfiles. If you go at it, consider reading the magic
hidden in the bitfile and swap it when it is required. But that will
go OT here.

> On the other hand, currently this driver is only required for
> programming the FPGA from Linux, which is not required. One of the
> bootloaders could do this for you earlier.

Well, exposing clocks to user space is not required _at all_ on all
other platforms you can think of. So, if you have a Zynq, you can
always load a Zynq driver even if you are not going to use it's
bitfile programming capabilities but only to configure clocks.

If you don't want to merge both drivers, have a Zynq-only clock
fabric driver instead?

Sebastian

On Mon, May 13, 2013 at 07:37:23PM +0200, Sebastian Hesselbarth wrote:
> On 05/13/13 19:24, Sören Brinkmann wrote:
> >On Mon, May 13, 2013 at 06:21:13PM +0200, Sebastian Hesselbarth wrote:
> >>>Well, that driver actually exists. But that just programs a bitstream
> >>>you give it to program. It does not know anything about the design it
> >>>programs and cannot make any kind of decision whether the clocks should
> >>>be userspace controlled or not.
> >>
> >>what Mark wants to point out is that you add fabric clocks to the Xilinx
> >>driver instead. This way, you will have user-space controllable clocks
> >>but only if you loaded the xilinx driver first.
> >What "Xilinx driver", are we talking about?
> 
> The device config driver you mention below.
> 
> >>IIRC the fabric clock controller provided by Zynq _is_ always there and
> >>accessible from ARM CPUs. You just don't have a new generic driver
> >>allowing to poke with all clocks, but a xilinx only driver allowing you
> >>to set the (xilinx only) fabric clocks.
> >Right.
> >
> >>I've played with Zynq a while ago, did Xilinx mainline the bitfile
> >>driver already? If not, why don't you give it a shot?
> >The device config driver is not in mainline, AFAIK. And I think it's in
> >rather bad shape and needs a lot of work before it is upstreamable. But
> >this is probably the right place to put this.
> 
> It is, as it will only expose clocks on Zynq and that's what Mark and
> Mike are worried about. Expose clocks to user space and you will have
> people mess with it for sure.
Well, even if you contain it in that driver you can still mess with
other clocks. Just give it the "wrong" input clock references in DT and
you are free to control them. As I said before, there is no protection
against such misuse.

> 
> About the shape of it, I didn't expect that to change at all. Just
> wondering, if it still requires you to manually end it's endianess
> mess with the bitfiles. If you go at it, consider reading the magic
> hidden in the bitfile and swap it when it is required. But that will
> go OT here.
It still takes byteswapped, binary images as input, unfortunately.

> 
> >On the other hand, currently this driver is only required for
> >programming the FPGA from Linux, which is not required. One of the
> >bootloaders could do this for you earlier.
> 
> Well, exposing clocks to user space is not required _at all_ on all
> other platforms you can think of. So, if you have a Zynq, you can
> always load a Zynq driver even if you are not going to use it's
> bitfile programming capabilities but only to configure clocks.
> 
> If you don't want to merge both drivers, have a Zynq-only clock
> fabric driver instead?
That was my original intention. But due to the nature of it, it will
always be possible to use it with other clocks too. Hence my generic
approach.
I actually like the idea of making it part of the device config driver.
The downside of it is, that this driver seems a bit far from mainline.

	Sören

On Mon, May 13, 2013 at 06:21:13PM +0200, Sebastian Hesselbarth wrote:
> On 05/13/13 18:09, Sören Brinkmann wrote:

> >Well, that driver actually exists. But that just programs a bitstream
> >you give it to program. It does not know anything about the design it
> >programs and cannot make any kind of decision whether the clocks should
> >be userspace controlled or not.

> what Mark wants to point out is that you add fabric clocks to the Xilinx
> driver instead. This way, you will have user-space controllable clocks
> but only if you loaded the xilinx driver first.

Yup, that's part of it.  Though I have to say that if all we can do is
stuff a bitstream in there there is the question about how anything that
does require a real driver gets that driver instantiated...

On 05/13/13 19:58, Sören Brinkmann wrote:
> On Mon, May 13, 2013 at 07:37:23PM +0200, Sebastian Hesselbarth wrote:
>> It is, as it will only expose clocks on Zynq and that's what Mark and
>> Mike are worried about. Expose clocks to user space and you will have
>> people mess with it for sure.
> Well, even if you contain it in that driver you can still mess with
> other clocks. Just give it the "wrong" input clock references in DT and
> you are free to control them. As I said before, there is no protection
> against such misuse.

Put the wrong clock in DT is not "misuse" but "bug" ;) More important,
it is quite static as you cannot change it easily by echo'ing into some
sysfs file. And to inject a DT you need access on boot loader level,
not kernel user space (yet).

>> About the shape of it, I didn't expect that to change at all. Just
>> wondering, if it still requires you to manually end it's endianess
>> mess with the bitfiles. If you go at it, consider reading the magic
>> hidden in the bitfile and swap it when it is required. But that will
>> go OT here.
> It still takes byteswapped, binary images as input, unfortunately.

Please, if you ever mainline any kernel driver for it, make it
auto-swap.

>> If you don't want to merge both drivers, have a Zynq-only clock
>> fabric driver instead?
> That was my original intention. But due to the nature of it, it will
> always be possible to use it with other clocks too. Hence my generic
> approach.

I already tried a generic "expose clocks to user space" and failed for
the very same reasons Mike and Mark are repeating over and over again -
and I agree with them.

> I actually like the idea of making it part of the device config driver.
> The downside of it is, that this driver seems a bit far from mainline.

Have a skeleton driver that exposes Zynq clocks first and implement
device config later? IIRC, device config isn't that complicated to
implement. Unfortunately, interpreting Xilinx datasheets or source code
is.

Sebastian

On 05/13/13 20:16, Mark Brown wrote:
> On Mon, May 13, 2013 at 06:21:13PM +0200, Sebastian Hesselbarth wrote:
>> what Mark wants to point out is that you add fabric clocks to the Xilinx
>> driver instead. This way, you will have user-space controllable clocks
>> but only if you loaded the xilinx driver first.
>
> Yup, that's part of it.  Though I have to say that if all we can do is
> stuff a bitstream in there there is the question about how anything that
> does require a real driver gets that driver instantiated...

Back when I played with Zynq, I remember looking at CONFIG_DYNAMIC_OF
discussions. ;)

Sebastian

On Mon, May 13, 2013 at 08:20:18PM +0200, Sebastian Hesselbarth wrote:
> On 05/13/13 20:16, Mark Brown wrote:

> >Yup, that's part of it.  Though I have to say that if all we can do is
> >stuff a bitstream in there there is the question about how anything that
> >does require a real driver gets that driver instantiated...

> Back when I played with Zynq, I remember looking at CONFIG_DYNAMIC_OF
> discussions. ;)

Which then goes back to the whole idea of this being a device - and
possibly that this should be done as part of representing UIO in DT
rather than anything else.

Quoting Sören Brinkmann (2013-05-13 10:58:49)
> On Mon, May 13, 2013 at 07:37:23PM +0200, Sebastian Hesselbarth wrote:
> > On 05/13/13 19:24, Sören Brinkmann wrote:
> > >On Mon, May 13, 2013 at 06:21:13PM +0200, Sebastian Hesselbarth wrote:
> > >>>Well, that driver actually exists. But that just programs a bitstream
> > >>>you give it to program. It does not know anything about the design it
> > >>>programs and cannot make any kind of decision whether the clocks should
> > >>>be userspace controlled or not.
> > >>
> > >>what Mark wants to point out is that you add fabric clocks to the Xilinx
> > >>driver instead. This way, you will have user-space controllable clocks
> > >>but only if you loaded the xilinx driver first.
> > >What "Xilinx driver", are we talking about?
> > 
> > The device config driver you mention below.
> > 
> > >>IIRC the fabric clock controller provided by Zynq _is_ always there and
> > >>accessible from ARM CPUs. You just don't have a new generic driver
> > >>allowing to poke with all clocks, but a xilinx only driver allowing you
> > >>to set the (xilinx only) fabric clocks.
> > >Right.
> > >
> > >>I've played with Zynq a while ago, did Xilinx mainline the bitfile
> > >>driver already? If not, why don't you give it a shot?
> > >The device config driver is not in mainline, AFAIK. And I think it's in
> > >rather bad shape and needs a lot of work before it is upstreamable. But
> > >this is probably the right place to put this.
> > 
> > It is, as it will only expose clocks on Zynq and that's what Mark and
> > Mike are worried about. Expose clocks to user space and you will have
> > people mess with it for sure.
> Well, even if you contain it in that driver you can still mess with
> other clocks. Just give it the "wrong" input clock references in DT and
> you are free to control them. As I said before, there is no protection
> against such misuse.

My objections are not about creating the most secure platform ever.  My
objections are about lowering the barrier of entry for folks to misuse
the clk api.  Clearly anyone that can compile a kernel and flash it to a
device can misuse anything they want.  Likewise if someone can compile a
DTB and flash it then there is another vector for misuse.

However, making an api available to any userspace application
substantially lowers the barrier to misuse.  Physical access to the
device is no longer needed in case of flashing new stuff to nand, or
emmc or whatever.  And by misuse I do not only mean people trying to
steal your credit card (I would be impressed if controlling clocks was a
part of that!), but I do not want to encourage vendors to implement crap
userspace drivers to control clocks instead of upstreaming proper Linux
device drivers.  And it is clearly possible to destroy a device with
irresponsible clock control.

While most of us agree that exposing clock controls to userspace is
useful for debugging, at this point I do not think the positive aspects
of mainlining the feature are strong enough to overcome the negative
aspects.

I'm sure implementations of this functionality will exist out-of-tree
(as pointed out by Peter), but not everything should be merged to
mainline.

Regards,
Mike

> 
> > 
> > About the shape of it, I didn't expect that to change at all. Just
> > wondering, if it still requires you to manually end it's endianess
> > mess with the bitfiles. If you go at it, consider reading the magic
> > hidden in the bitfile and swap it when it is required. But that will
> > go OT here.
> It still takes byteswapped, binary images as input, unfortunately.
> 
> > 
> > >On the other hand, currently this driver is only required for
> > >programming the FPGA from Linux, which is not required. One of the
> > >bootloaders could do this for you earlier.
> > 
> > Well, exposing clocks to user space is not required _at all_ on all
> > other platforms you can think of. So, if you have a Zynq, you can
> > always load a Zynq driver even if you are not going to use it's
> > bitfile programming capabilities but only to configure clocks.
> > 
> > If you don't want to merge both drivers, have a Zynq-only clock
> > fabric driver instead?
> That was my original intention. But due to the nature of it, it will
> always be possible to use it with other clocks too. Hence my generic
> approach.
> I actually like the idea of making it part of the device config driver.
> The downside of it is, that this driver seems a bit far from mainline.
> 
>         Sören

On 05/14/2013 12:46 PM, Mike Turquette wrote:
> Quoting Sören Brinkmann (2013-05-13 10:58:49)
>> On Mon, May 13, 2013 at 07:37:23PM +0200, Sebastian Hesselbarth wrote:
>>> On 05/13/13 19:24, Sören Brinkmann wrote:
>>>> On Mon, May 13, 2013 at 06:21:13PM +0200, Sebastian Hesselbarth wrote:
>>>>>> Well, that driver actually exists. But that just programs a bitstream
>>>>>> you give it to program. It does not know anything about the design it
>>>>>> programs and cannot make any kind of decision whether the clocks should
>>>>>> be userspace controlled or not.
>>>>>
>>>>> what Mark wants to point out is that you add fabric clocks to the Xilinx
>>>>> driver instead. This way, you will have user-space controllable clocks
>>>>> but only if you loaded the xilinx driver first.
>>>> What "Xilinx driver", are we talking about?
>>>
>>> The device config driver you mention below.
>>>
>>>>> IIRC the fabric clock controller provided by Zynq _is_ always there and
>>>>> accessible from ARM CPUs. You just don't have a new generic driver
>>>>> allowing to poke with all clocks, but a xilinx only driver allowing you
>>>>> to set the (xilinx only) fabric clocks.
>>>> Right.
>>>>
>>>>> I've played with Zynq a while ago, did Xilinx mainline the bitfile
>>>>> driver already? If not, why don't you give it a shot?
>>>> The device config driver is not in mainline, AFAIK. And I think it's in
>>>> rather bad shape and needs a lot of work before it is upstreamable. But
>>>> this is probably the right place to put this.
>>>
>>> It is, as it will only expose clocks on Zynq and that's what Mark and
>>> Mike are worried about. Expose clocks to user space and you will have
>>> people mess with it for sure.
>> Well, even if you contain it in that driver you can still mess with
>> other clocks. Just give it the "wrong" input clock references in DT and
>> you are free to control them. As I said before, there is no protection
>> against such misuse.
>
> My objections are not about creating the most secure platform ever.  My
> objections are about lowering the barrier of entry for folks to misuse
> the clk api.  Clearly anyone that can compile a kernel and flash it to a
> device can misuse anything they want.  Likewise if someone can compile a
> DTB and flash it then there is another vector for misuse.
>
> However, making an api available to any userspace application
> substantially lowers the barrier to misuse.  Physical access to the
> device is no longer needed in case of flashing new stuff to nand, or
> emmc or whatever.  And by misuse I do not only mean people trying to
> steal your credit card (I would be impressed if controlling clocks was a
> part of that!), but I do not want to encourage vendors to implement crap
> userspace drivers to control clocks instead of upstreaming proper Linux
> device drivers.  And it is clearly possible to destroy a device with
> irresponsible clock control.
>
> While most of us agree that exposing clock controls to userspace is
> useful for debugging, at this point I do not think the positive aspects
> of mainlining the feature are strong enough to overcome the negative
> aspects.
>
> I'm sure implementations of this functionality will exist out-of-tree
> (as pointed out by Peter), but not everything should be merged to
> mainline.
>

I'm skimming this thread and am concerned there is a level of 
misunderstanding by many people how Zynq works :)

First of all, the driver that loads the bitstream into the fpga fabric 
does not know ANYTHING about what the bitstream does. So it cannot do 
any setup based on the contents of the file that is loaded. (And this 
can also be loaded during the SoC bootup, bypassing this driver completely)

Second, there are four clocks that feed the FPGA fabric. We will want to 
set these clocks from user space somehow. It is perfectly valid to use a 
uio driver to interface with logic in the fpga. If we take the approach 
of using such general purpose techniques to interface with fpga logic, 
we must have ways for the user to control the fpga clocks.

I give the Xilinx guys a lot of crap for not upstreaming their drivers 
fast enough for me :), so I feel obligated to help explain why users 
want to do this and I do not want to carry out of tree code that is 
critical to using these devices effectively.

I imagine the Altera soc-fpga parts will have similar issues. I suppose 
I should read the TRM for them one day.

Philip

> Regards,
> Mike
>
>>
>>>
>>> About the shape of it, I didn't expect that to change at all. Just
>>> wondering, if it still requires you to manually end it's endianess
>>> mess with the bitfiles. If you go at it, consider reading the magic
>>> hidden in the bitfile and swap it when it is required. But that will
>>> go OT here.
>> It still takes byteswapped, binary images as input, unfortunately.
>>
>>>
>>>> On the other hand, currently this driver is only required for
>>>> programming the FPGA from Linux, which is not required. One of the
>>>> bootloaders could do this for you earlier.
>>>
>>> Well, exposing clocks to user space is not required _at all_ on all
>>> other platforms you can think of. So, if you have a Zynq, you can
>>> always load a Zynq driver even if you are not going to use it's
>>> bitfile programming capabilities but only to configure clocks.
>>>
>>> If you don't want to merge both drivers, have a Zynq-only clock
>>> fabric driver instead?
>> That was my original intention. But due to the nature of it, it will
>> always be possible to use it with other clocks too. Hence my generic
>> approach.
>> I actually like the idea of making it part of the device config driver.
>> The downside of it is, that this driver seems a bit far from mainline.
>>
>>          Sören

On Tue, May 14, 2013 at 02:09:47PM -0400, Philip Balister wrote:

> First of all, the driver that loads the bitstream into the fpga
> fabric does not know ANYTHING about what the bitstream does. So it
> cannot do any setup based on the contents of the file that is
> loaded. (And this can also be loaded during the SoC bootup,
> bypassing this driver completely)

This is a problem that is going to need to be fixed - there will be some
things going on the FPGAs that do need drivers and so there needs to be
some way to instantiate a driver for a FPGA image.  Things like adding
extra DT blobs along with the FPGA image have been talked about

> Second, there are four clocks that feed the FPGA fabric. We will
> want to set these clocks from user space somehow. It is perfectly
> valid to use a uio driver to interface with logic in the fpga. If we
> take the approach of using such general purpose techniques to
> interface with fpga logic, we must have ways for the user to control
> the fpga clocks.

Right, and if the specific device is being controlled by UIO then having
UIO create some clocks makes sense but then that should be integrated
into the UIO instantiation rather than done as a separate thing.

On 05/11/2013 07:21 AM, Mark Brown wrote:
> On Fri, May 10, 2013 at 04:01:25PM -0700, Saravana Kannan wrote:
>> On 05/10/2013 03:18 PM, Mike Turquette wrote:
>
> Guys please delete irrelevant context from replies...
>
>>> 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.
>
>> We already have this for MSM. But I seem to have managed to keep our
>> userspace guys away from abusing it. YMMV.
>
> It's much harder when it's in the standard kernel and there's no contact
> with some of the users.  I've pushed back pretty strongly on some of
> your equivalent stuff for regulators for this reason.

Agreed. I don't really keep up with what's going on with the MSM regulators.

But at some point, I'll start requesting for this for debugfs. It's 
terribly handy to debug power issues, run unit tests and for debug 
sessions (debugfs. So, clearly!).

-Saravana

On 05/14/2013 09:46 PM, Mark Brown wrote:
> On Tue, May 14, 2013 at 02:09:47PM -0400, Philip Balister wrote:
>
>> First of all, the driver that loads the bitstream into the fpga
>> fabric does not know ANYTHING about what the bitstream does. So it
>> cannot do any setup based on the contents of the file that is
>> loaded. (And this can also be loaded during the SoC bootup,
>> bypassing this driver completely)
>
> This is a problem that is going to need to be fixed - there will be some
> things going on the FPGAs that do need drivers and so there needs to be
> some way to instantiate a driver for a FPGA image.  Things like adding
> extra DT blobs along with the FPGA image have been talked about
>
>> Second, there are four clocks that feed the FPGA fabric. We will
>> want to set these clocks from user space somehow. It is perfectly
>> valid to use a uio driver to interface with logic in the fpga. If we
>> take the approach of using such general purpose techniques to
>> interface with fpga logic, we must have ways for the user to control
>> the fpga clocks.
>
> Right, and if the specific device is being controlled by UIO then having
> UIO create some clocks makes sense but then that should be integrated
> into the UIO instantiation rather than done as a separate thing.

Agreed. I was about to reply with exactly the same point. I haven't done 
any UIO coding, but that device file will eventually have to be opened. 
Turn on the clocks in the open and turn them off at close.

Rate to request can be a DT property.

-Saravana

On 05/16/2013 12:28 AM, Saravana Kannan wrote:
> On 05/14/2013 09:46 PM, Mark Brown wrote:
>> On Tue, May 14, 2013 at 02:09:47PM -0400, Philip Balister wrote:
>>
>>> First of all, the driver that loads the bitstream into the fpga
>>> fabric does not know ANYTHING about what the bitstream does. So it
>>> cannot do any setup based on the contents of the file that is
>>> loaded. (And this can also be loaded during the SoC bootup,
>>> bypassing this driver completely)
>>
>> This is a problem that is going to need to be fixed - there will be some
>> things going on the FPGAs that do need drivers and so there needs to be
>> some way to instantiate a driver for a FPGA image.  Things like adding
>> extra DT blobs along with the FPGA image have been talked about
>>
>>> Second, there are four clocks that feed the FPGA fabric. We will
>>> want to set these clocks from user space somehow. It is perfectly
>>> valid to use a uio driver to interface with logic in the fpga. If we
>>> take the approach of using such general purpose techniques to
>>> interface with fpga logic, we must have ways for the user to control
>>> the fpga clocks.
>>
>> Right, and if the specific device is being controlled by UIO then having
>> UIO create some clocks makes sense but then that should be integrated
>> into the UIO instantiation rather than done as a separate thing.
> 
> Agreed. I was about to reply with exactly the same point. I haven't done
> any UIO coding, but that device file will eventually have to be opened.
> Turn on the clocks in the open and turn them off at close.
> 
> Rate to request can be a DT property.

But you are assuming each device implemented in the fpga uses one clock.
This assumption is clearly not valid since devices will ahve to share
clocks.

Philip

On Thu, May 16, 2013 at 10:44:16AM -0400, Philip Balister wrote:
> On 05/16/2013 12:28 AM, Saravana Kannan wrote:

> > Agreed. I was about to reply with exactly the same point. I haven't done
> > any UIO coding, but that device file will eventually have to be opened.
> > Turn on the clocks in the open and turn them off at close.

> > Rate to request can be a DT property.

> But you are assuming each device implemented in the fpga uses one clock.
> This assumption is clearly not valid since devices will ahve to share
> clocks.

The binding should obviously be defined as a set of clocks rather than a
single clock - something like how we specify regulators on PMICs
probably makes sense.  Each clock can have its own properties.

On Wed, May 15, 2013 at 09:23:03PM -0700, Saravana Kannan wrote:
> On 05/11/2013 07:21 AM, Mark Brown wrote:

> >It's much harder when it's in the standard kernel and there's no contact
> >with some of the users.  I've pushed back pretty strongly on some of
> >your equivalent stuff for regulators for this reason.

> Agreed. I don't really keep up with what's going on with the MSM regulators.

> But at some point, I'll start requesting for this for debugfs. It's
> terribly handy to debug power issues, run unit tests and for debug
> sessions (debugfs. So, clearly!).

Sure, it's handy for debugging - but it's also far too handy for writing
silly things in userspace which isn't something we want to encourage.

On Thu, May 16, 2013 at 06:26:05PM +0100, Mark Brown wrote:
> On Thu, May 16, 2013 at 10:44:16AM -0400, Philip Balister wrote:
> > On 05/16/2013 12:28 AM, Saravana Kannan wrote:
> 
> > > Agreed. I was about to reply with exactly the same point. I haven't done
> > > any UIO coding, but that device file will eventually have to be opened.
> > > Turn on the clocks in the open and turn them off at close.
> 
> > > Rate to request can be a DT property.
> 
> > But you are assuming each device implemented in the fpga uses one clock.
> > This assumption is clearly not valid since devices will ahve to share
> > clocks.
> 
> The binding should obviously be defined as a set of clocks rather than a
> single clock - something like how we specify regulators on PMICs
> probably makes sense.  Each clock can have its own properties.
For a device driver as clock consumer, the bindings etc. are well
defined. Just add the 'clocks' and 'clock-names' props and use the CCF.

	Sören

On Thu, May 16, 2013 at 11:55:45AM -0700, Sören Brinkmann wrote:
> On Thu, May 16, 2013 at 06:26:05PM +0100, Mark Brown wrote:

> > The binding should obviously be defined as a set of clocks rather than a
> > single clock - something like how we specify regulators on PMICs
> > probably makes sense.  Each clock can have its own properties.

> For a device driver as clock consumer, the bindings etc. are well
> defined. Just add the 'clocks' and 'clock-names' props and use the CCF.

That's not really intended to be used for enumeration by the child,
though - you'd at least need to define that the child looks through
the same binding to figure out what clocks to request.