| Message ID | 1341767726-25630-1-git-send-email-sebastian.hesselbarth@googlemail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Sebastian Hesselbarth (2012-07-08 10:15:26) > This patch adds support for using clock gates (clk-gate) from DT based > on Rob Herrings DT clk binding support for 3.6. > > It adds a helper function to clk-gate to allocate all resources required by > a set of individual clock gates, i.e. register base address and lock. Each > clock gate is described as a child of the clock-gating-control in DT and > also created by the helper function. > Hi Sebastian, Thanks for submitting this. I'd prefer for Rob or someone with a more vested interest in DT to review your binding. I have some comments on the code below. <snip> > diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c > index 578465e..1e88907 100644 > --- a/drivers/clk/clk-gate.c > +++ b/drivers/clk/clk-gate.c > @@ -15,6 +15,9 @@ > #include <linux/io.h> > #include <linux/err.h> > #include <linux/string.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/of_platform.h> > > /** > * DOC: basic gatable clock which can gate and ungate it's ouput > @@ -148,3 +151,84 @@ struct clk *clk_register_gate(struct device *dev, const char *name, > > return clk; > } > + > +#ifdef CONFIG_OF > +/** > + * of_clock_gating_control_setup() - Setup function for clock gate control > + * This is a helper for using clk-gate from OF device tree. It allocates > + * a common lock for a base register and creates the individual clk-gates. > + */ > +void __init of_clock_gating_control_setup(struct device_node *np) > +{ > + struct device_node *child; > + const char *pclk_name; > + void __iomem *base; > + spinlock_t *lockp; > + unsigned int rnum; > + u64 addr; > + > + pclk_name = of_clk_get_parent_name(np, 0); > + if (!pclk_name) { > + pr_debug("%s: unable to get parent clock for %s\n", > + __func__, np->full_name); > + return; > + } > + > + lockp = kzalloc(sizeof(spinlock_t), GFP_KERNEL); > + if (!lockp) { > + pr_debug("%s: unable to allocate spinlock for %s\n", > + __func__, np->full_name); > + return; > + } > + > + spin_lock_init(lockp); The spinlocks for the basic clock types have always been optional. This code should reflect that and not assume the spinlock. Also I wonder if the assumption is true that a single spinlock corresponding to a device_node is always the right thing for every platform. What about a 32-bit register that contains some gating bits and a 3-bit wide field for a mux which we perform read-modify-write operations on? You'll have to pardon my DT ignorance. My concerns above may be totally crazy with respect to DT. > + base = of_iomap(np, 0); > + rnum = sizeof(resource_size_t) * 8; > + addr = of_translate_address(np, of_get_property(np, "reg", NULL)); > + > + pr_debug("create clock gate control %s\n", np->full_name); There are some inconsistent prints here. How about leading this trace with a __func__ like you do below for the error messages? > + > + for_each_child_of_node(np, child) { > + struct clk *cg; > + const char *cg_name; > + const char *cg_pclk_name; > + u32 propval[2]; > + unsigned int rbit; > + > + if (of_property_read_u32_array(child, "reg", propval, 2)) { > + pr_debug("%s: wrong #reg on %s\n", > + __func__, child->full_name); > + continue; > + } > + > + rbit = propval[0]; > + if (rbit >= rnum) { > + pr_debug("%s: bit position of %s exceeds resources\n", > + __func__, child->full_name); > + continue; > + } > + > + cg_pclk_name = of_clk_get_parent_name(child, 0); > + if (!pclk_name) > + cg_pclk_name = pclk_name; !pclk_name would have caused an early return above, so this conditional will never evaluate as true. Even if it did, I'm not sure I follow the logic. Why set cg_pclk_name to NULL if pclk_name is NULL? > + > + if (of_property_read_string(child, "clock-output-names", > + &cg_name)) { > + unsigned int nlen = 4 + 16 + strlen(child->name); > + char *name = kzalloc(nlen+1, GFP_KERNEL); > + if (!name) > + continue; > + snprintf(name, nlen, "%u@%llx.%s", rbit, > + (unsigned long long)addr, child->name); > + cg_name = name; > + } > + > + pr_debug(" create clock gate: %s\n", cg_name); Extra whitespace typo? Again, would be nice to lead this trace with a __func__ string. > + > + cg = clk_register_gate(NULL, cg_name, cg_pclk_name, 0, > + base, rbit, propval[1], lockp); > + if (cg) > + of_clk_add_provider(child, of_clk_src_simple_get, cg); Need to check if clk_register_gate fails and do memory leak cleanup of name and maybe lockp for the corner case where none of the clock registration operations succeed. Regards, Mike
On Thu, 2012-08-23 at 19:31 -0700, Mike Turquette wrote: > Quoting Sebastian Hesselbarth (2012-07-08 10:15:26) > > This patch adds support for using clock gates (clk-gate) from DT based > > on Rob Herrings DT clk binding support for 3.6. > > > > It adds a helper function to clk-gate to allocate all resources required by > > a set of individual clock gates, i.e. register base address and lock. Each > > clock gate is described as a child of the clock-gating-control in DT and > > also created by the helper function. > > > > Hi Sebastian, > > Thanks for submitting this. I'd prefer for Rob or someone with a more vested > interest in DT to review your binding. I have some comments on the code below. > > <snip> > > diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c > > index 578465e..1e88907 100644 > > --- a/drivers/clk/clk-gate.c > > +++ b/drivers/clk/clk-gate.c > > @@ -15,6 +15,9 @@ > > #include <linux/io.h> > > #include <linux/err.h> > > #include <linux/string.h> > > +#include <linux/of.h> > > +#include <linux/of_address.h> > > +#include <linux/of_platform.h> > > > > /** > > * DOC: basic gatable clock which can gate and ungate it's ouput > > @@ -148,3 +151,84 @@ struct clk *clk_register_gate(struct device *dev, const char *name, > > > > return clk; > > } > > + > > +#ifdef CONFIG_OF > > +/** > > + * of_clock_gating_control_setup() - Setup function for clock gate control > > + * This is a helper for using clk-gate from OF device tree. It allocates > > + * a common lock for a base register and creates the individual clk-gates. > > + */ > > +void __init of_clock_gating_control_setup(struct device_node *np) > > +{ > > + struct device_node *child; > > + const char *pclk_name; > > + void __iomem *base; > > + spinlock_t *lockp; > > + unsigned int rnum; > > + u64 addr; > > + > > + pclk_name = of_clk_get_parent_name(np, 0); > > + if (!pclk_name) { > > + pr_debug("%s: unable to get parent clock for %s\n", > > + __func__, np->full_name); > > + return; > > + } > > + > > + lockp = kzalloc(sizeof(spinlock_t), GFP_KERNEL); > > + if (!lockp) { > > + pr_debug("%s: unable to allocate spinlock for %s\n", > > + __func__, np->full_name); > > + return; > > + } > > + > > + spin_lock_init(lockp); > > The spinlocks for the basic clock types have always been optional. This > code should reflect that and not assume the spinlock. > > Also I wonder if the assumption is true that a single spinlock > corresponding to a device_node is always the right thing for every > platform. What about a 32-bit register that contains some gating bits > and a 3-bit wide field for a mux which we perform read-modify-write > operations on? > > You'll have to pardon my DT ignorance. My concerns above may be totally > crazy with respect to DT. This also requires you to split your gated clocks in seperate nodes by register if you don't want unrelated clocks locking each other out. Also, you need a method of 'sharing' the lock outside of the gated-clock for use by other clock types that may share the same register. > > > + base = of_iomap(np, 0); > > + rnum = sizeof(resource_size_t) * 8; > > + addr = of_translate_address(np, of_get_property(np, "reg", NULL)); > > + > > + pr_debug("create clock gate control %s\n", np->full_name); > > There are some inconsistent prints here. How about leading this trace > with a __func__ like you do below for the error messages? > > > + > > + for_each_child_of_node(np, child) { > > + struct clk *cg; > > + const char *cg_name; > > + const char *cg_pclk_name; > > + u32 propval[2]; > > + unsigned int rbit; > > + > > + if (of_property_read_u32_array(child, "reg", propval, 2)) { > > + pr_debug("%s: wrong #reg on %s\n", > > + __func__, child->full_name); > > + continue; > > + } Are you assuming that all gated-clocks will be contained within a parent node with: #address-cells = <1>; #size-cells = <1>; with no other types of clocks? This generally isn't how people are writing their device tree files. Looking through the current dts files, the platforms that are using clocks to any real degree are using address=1, size=0. The only ones I see with 1/1 are only defining a fixed-clock and don't use the reg property anyway so could just as well be 1/0. It might be better to pass the bit field as a separate property rather than requiring people change their reg property to suit. myclock { reg = <0x0120>; enable-bit = <22>; }; Regards Tony Prisk
On 08/24/2012 06:29 AM, Tony Prisk wrote: > On Thu, 2012-08-23 at 19:31 -0700, Mike Turquette wrote: >> Quoting Sebastian Hesselbarth (2012-07-08 10:15:26) >>> This patch adds support for using clock gates (clk-gate) from DT based >>> on Rob Herrings DT clk binding support for 3.6. >>> >>> It adds a helper function to clk-gate to allocate all resources required by >>> a set of individual clock gates, i.e. register base address and lock. Each >>> clock gate is described as a child of the clock-gating-control in DT and >>> also created by the helper function. >>> >> Thanks for submitting this. I'd prefer for Rob or someone with a more vested >> interest in DT to review your binding. I have some comments on the code below. Mike, Tony, thanks for reviewing this, I didn't even remember submitting it ;) >> <snip> >>> diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c >>> + >>> + lockp = kzalloc(sizeof(spinlock_t), GFP_KERNEL); >>> + if (!lockp) { >>> + pr_debug("%s: unable to allocate spinlock for %s\n", >>> + __func__, np->full_name); >>> + return; >>> + } >>> + >>> + spin_lock_init(lockp); >> >> The spinlocks for the basic clock types have always been optional. This >> code should reflect that and not assume the spinlock. >> >> Also I wonder if the assumption is true that a single spinlock >> corresponding to a device_node is always the right thing for every >> platform. What about a 32-bit register that contains some gating bits >> and a 3-bit wide field for a mux which we perform read-modify-write >> operations on? >> >> You'll have to pardon my DT ignorance. My concerns above may be totally >> crazy with respect to DT. > > This also requires you to split your gated clocks in seperate nodes by > register if you don't want unrelated clocks locking each other out. > Also, you need a method of 'sharing' the lock outside of the gated-clock > for use by other clock types that may share the same register. The clock-gatining-control was inspired by a single 32b register that only has clock gating bits. Some weeks of more insight into kernel code and DT, I agree to do this differently. > Are you assuming that all gated-clocks will be contained within a parent > node with: > #address-cells =<1>; > #size-cells =<1>; > with no other types of clocks? This generally isn't how people are > writing their device tree files. > > Looking through the current dts files, the platforms that are using > clocks to any real degree are using address=1, size=0. > The only ones I see with 1/1 are only defining a fixed-clock and don't > use the reg property anyway so could just as well be 1/0. > > It might be better to pass the bit field as a separate property rather > than requiring people change their reg property to suit. > > myclock { > reg =<0x0120>; > enable-bit =<22>; > }; Actually, today I would rather have some platform specific node for the clock gating register and don't name each individual clock but only use the clocks phandle to sort out the corresponding bit. cgc: clock-gating-control { compatible = "marvell,orion-clkctrl"; reg = <0x12340 0x8>; #clock-cells = <1>; }; some_device: gateable-device { ... clocks = <&cgc 13>; }; The platform specific node will take care of all requirements like spinlock or not. Maybe there will have to be a property to mask out reserved bits in the 32b register or maybe I can assume that reserved bits do no harm if set. Sebastian
diff --git a/Documentation/devicetree/bindings/clock/clock-gating-control.txt b/Documentation/devicetree/bindings/clock/clock-gating-control.txt new file mode 100644 index 0000000..05f5728 --- /dev/null +++ b/Documentation/devicetree/bindings/clock/clock-gating-control.txt @@ -0,0 +1,80 @@ +Binding for simple clock gating control based on clock gate register with one +bit per clock gate. This is a clock consumer and also a clock provider for a +set of gated clocks that are described as children of this node. Each clock gate +is described by a bit number and polarity to control the gate. + +This binding uses the common clock binding[1]. + +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt + +==Clock gating control== + +Required properties: +- compatible : shall be "clock-gating-control". +- reg : should contain the register physical address and length for + the clock gating control. +- clocks : shared parent clock for all gated clocks. +- #clock-cells : from common clock binding; shall be set to 0. +- #address-cells : number of cells required to describe a clock gate; + should be <2>. +- #size-cells : should be zero. + +Each individual clock gate bit is described as a child of the +corresponding gating control register with the following properties. + +Required child properties: +- reg : should contain the individual bit and polarity to control + the clock gate. A polarity of 0 means that by setting the + bit to 1 the clock passes through the clock gate while + setting the bit to 0 disables the clock. Any other value + for polarity inverts the meaning of the control bit. + +Optional child properties: +- clocks : overrides the shared parent clock for this clock gate + by the clock passed in this property. +- clock-output-names : from common clock binding; allows to set + a specific name for the gated clock output. + +==Example== + + /* fixed root clock */ + osc: oscillator { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <166666667>; + }; + + /* sata peripheral clock */ + sata_clk: ext-oscillator { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <25000000>; + }; + + /* register-based clock gating control */ + gating-control@f10d0038 { + compatible = "clock-gating-control"; + reg = <0xf10d0038 0x4>; + clocks = <&osc>; + #clock-cells = <0>; + #address-cells = <2>; + #size-cells = <0>; + + /* USB0 clock gate on register bit 0 with inverted polarity */ + cg_usb0: clockgate@0 { + reg = <0 1>; /* register bit 0, inverted polarity */ + }; + + /* PCIe0 clock gate on register bit 1 with normal polarity + * and named output clock */ + cg_pcie0: clockgate@1 { + reg = <1 0>; /* register bit 1, normal polarity */ + clock-output-names = "pcie0_clk"; + }; + + /* SATA clock gate with different parent clock */ + cg_sata: clockgate@3 { + reg = <3 0>; /* register bit 3, normal polarity */ + clocks = <&sata_clk>; + }; + }; diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c index 578465e..1e88907 100644 --- a/drivers/clk/clk-gate.c +++ b/drivers/clk/clk-gate.c @@ -15,6 +15,9 @@ #include <linux/io.h> #include <linux/err.h> #include <linux/string.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_platform.h> /** * DOC: basic gatable clock which can gate and ungate it's ouput @@ -148,3 +151,84 @@ struct clk *clk_register_gate(struct device *dev, const char *name, return clk; } + +#ifdef CONFIG_OF +/** + * of_clock_gating_control_setup() - Setup function for clock gate control + * This is a helper for using clk-gate from OF device tree. It allocates + * a common lock for a base register and creates the individual clk-gates. + */ +void __init of_clock_gating_control_setup(struct device_node *np) +{ + struct device_node *child; + const char *pclk_name; + void __iomem *base; + spinlock_t *lockp; + unsigned int rnum; + u64 addr; + + pclk_name = of_clk_get_parent_name(np, 0); + if (!pclk_name) { + pr_debug("%s: unable to get parent clock for %s\n", + __func__, np->full_name); + return; + } + + lockp = kzalloc(sizeof(spinlock_t), GFP_KERNEL); + if (!lockp) { + pr_debug("%s: unable to allocate spinlock for %s\n", + __func__, np->full_name); + return; + } + + spin_lock_init(lockp); + base = of_iomap(np, 0); + rnum = sizeof(resource_size_t) * 8; + addr = of_translate_address(np, of_get_property(np, "reg", NULL)); + + pr_debug("create clock gate control %s\n", np->full_name); + + for_each_child_of_node(np, child) { + struct clk *cg; + const char *cg_name; + const char *cg_pclk_name; + u32 propval[2]; + unsigned int rbit; + + if (of_property_read_u32_array(child, "reg", propval, 2)) { + pr_debug("%s: wrong #reg on %s\n", + __func__, child->full_name); + continue; + } + + rbit = propval[0]; + if (rbit >= rnum) { + pr_debug("%s: bit position of %s exceeds resources\n", + __func__, child->full_name); + continue; + } + + cg_pclk_name = of_clk_get_parent_name(child, 0); + if (!pclk_name) + cg_pclk_name = pclk_name; + + if (of_property_read_string(child, "clock-output-names", + &cg_name)) { + unsigned int nlen = 4 + 16 + strlen(child->name); + char *name = kzalloc(nlen+1, GFP_KERNEL); + if (!name) + continue; + snprintf(name, nlen, "%u@%llx.%s", rbit, + (unsigned long long)addr, child->name); + cg_name = name; + } + + pr_debug(" create clock gate: %s\n", cg_name); + + cg = clk_register_gate(NULL, cg_name, cg_pclk_name, 0, + base, rbit, propval[1], lockp); + if (cg) + of_clk_add_provider(child, of_clk_src_simple_get, cg); + } +} +#endif diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index b97f61e..499eac2 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -205,6 +205,8 @@ struct clk *clk_register_gate(struct device *dev, const char *name, void __iomem *reg, u8 bit_idx, u8 clk_gate_flags, spinlock_t *lock); +void of_clock_gating_control_setup(struct device_node *np); + /** * struct clk_divider - adjustable divider clock *
This patch adds support for using clock gates (clk-gate) from DT based on Rob Herrings DT clk binding support for 3.6. It adds a helper function to clk-gate to allocate all resources required by a set of individual clock gates, i.e. register base address and lock. Each clock gate is described as a child of the clock-gating-control in DT and also created by the helper function. Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@googlemail.com> Cc: Grant Likely <grant.likely@secretlab.ca> Cc: Rob Herring <rob.herring@calxeda.com> Cc: Rob Landley <rob@landley.net> Cc: Mike Turquette <mturquette@ti.com> Cc: devicetree-discuss@lists.ozlabs.org Cc: linux-doc@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org --- .../bindings/clock/clock-gating-control.txt | 80 +++++++++++++++++++ drivers/clk/clk-gate.c | 84 ++++++++++++++++++++ include/linux/clk-provider.h | 2 + 3 files changed, 166 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/clock-gating-control.txt