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