| Message ID | 1375719147-7578-3-git-send-email-santosh.shilimkar@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
[adding dt maintainers] On Mon, Aug 05, 2013 at 05:12:21PM +0100, Santosh Shilimkar wrote: > Add the driver for the clock gate control which uses PSC (Power Sleep > Controller) IP on Keystone 2 based SOCs. It is responsible for enabling and > disabling of the clocks for different IPs present in the SoC. > > Cc: Mike Turquette <mturquette@linaro.org> > > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > --- > .../devicetree/bindings/clock/keystone-gate.txt | 30 ++ > drivers/clk/keystone/gate.c | 306 ++++++++++++++++++++ > include/linux/clk/keystone.h | 1 + > 3 files changed, 337 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/keystone-gate.txt > create mode 100644 drivers/clk/keystone/gate.c > > diff --git a/Documentation/devicetree/bindings/clock/keystone-gate.txt b/Documentation/devicetree/bindings/clock/keystone-gate.txt > new file mode 100644 > index 0000000..40afef5 > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/keystone-gate.txt > @@ -0,0 +1,30 @@ > +Binding for Keystone gate control driver which uses PSC controller IP. > + > +This binding uses the common clock binding[1]. > + > +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt > + > +Required properties: > +- compatible : shall be "keystone,psc-clk". Similarly to my comments on patch 1, this should probably be something more like "ti,keystone-psc-clock" > +- #clock-cells : from common clock binding; shall be set to 0. > +- clocks : parent clock phandle > +- reg : psc address address space > + > +Optional properties: > +- clock-output-names : From common clock binding to override the > + default output clock name > +- status : "enabled" if clock is always enabled Huh? This is a standard property, for which ePAPR defines "okay", "disabled", "fail", and "fail-sss", and not "enabled". This also doesn't seem to follow the standard meaning judging by the code. It looks like this could be a boolean property ("clock-always-enabled"?), but that assumes it's necessary. Why do you need this? > +- lpsc : lpsc module id, if not set defaults to zero What's that needed for? Are there multiple clocks sharing a common register bank? > +- pd : power domain number, if not set defaults to zero (always ON) Please don't use abbreviations unnecessarily. "power-domain-id" or similar would be far better. What exactly does this represent? Does the clock IP itself handle power domains, or is there some external unit that controls power domains? > +- gpsc : gpsc number, if not set defaults to zero How does that interact with the lpsc property? When are these necessary? > + > +Example: > + clock { > + #clock-cells = <0>; > + compatible = "keystone,psc-clk"; > + clocks = <&chipclk3>; > + clock-output-names = "debugss_trc"; > + reg = <0x02350000 4096>; > + lpsc = <5>; > + pd = <1>; > + }; > diff --git a/drivers/clk/keystone/gate.c b/drivers/clk/keystone/gate.c > new file mode 100644 > index 0000000..72ac478 > --- /dev/null > +++ b/drivers/clk/keystone/gate.c > @@ -0,0 +1,306 @@ > +/* > + * Clock driver for Keystone 2 based devices > + * > + * Copyright (C) 2013 Texas Instruments. > + * Murali Karicheri <m-karicheri2@ti.com> > + * Santosh Shilimkar <santosh.shilimkar@ti.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > +#include <linux/clk.h> > +#include <linux/clk-provider.h> > +#include <linux/delay.h> > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/slab.h> > +#include <linux/of_address.h> > +#include <linux/of.h> > +#include <linux/module.h> > + > +/* PSC register offsets */ > +#define PTCMD 0x120 > +#define PTSTAT 0x128 > +#define PDSTAT 0x200 > +#define PDCTL 0x300 > +#define MDSTAT 0x800 > +#define MDCTL 0xa00 > + > +/* PSC module states */ > +#define PSC_STATE_SWRSTDISABLE 0 > +#define PSC_STATE_SYNCRST 1 > +#define PSC_STATE_DISABLE 2 > +#define PSC_STATE_ENABLE 3 > + > +#define MDSTAT_STATE_MASK 0x3f > +#define MDSTAT_MCKOUT BIT(12) > +#define PDSTAT_STATE_MASK 0x1f > +#define MDCTL_FORCE BIT(31) > +#define MDCTL_LRESET BIT(8) > +#define PDCTL_NEXT BIT(0) > + > +/* PSC flags. Disable state is SwRstDisable */ > +#define PSC_SWRSTDISABLE BIT(0) > +/* Force module state transtition */ > +#define PSC_FORCE BIT(1) > +/* Keep module in local reset */ > +#define PSC_LRESET BIT(2) > +#define NUM_GPSC 2 > +#define REG_OFFSET 4 > + > +/** > + * struct clk_psc_data - PSC data > + * @base: Base address for a PSC > + * @flags: framework flags > + * @lpsc: Is it local PSC > + * @gpsc: Is it global PSC > + * @domain: PSC domain > + * > + */ > +struct clk_psc_data { > + void __iomem *base; > + u32 flags; > + u32 psc_flags; > + u8 lpsc; > + u8 gpsc; > + u8 domain; > +}; > + > +/** > + * struct clk_psc - PSC clock structure > + * @hw: clk_hw for the psc > + * @psc_data: PSC driver specific data > + * @lock: Spinlock used by the driver > + */ > +struct clk_psc { > + struct clk_hw hw; > + struct clk_psc_data *psc_data; > + spinlock_t *lock; > +}; > + > +struct reg_psc { > + u32 phy_base; > + u32 size; > + void __iomem *io_base; > +}; > + > +static struct reg_psc psc_addr[NUM_GPSC]; > +static DEFINE_SPINLOCK(psc_lock); > + > +#define to_clk_psc(_hw) container_of(_hw, struct clk_psc, hw) > + > +static void psc_config(void __iomem *psc_base, unsigned int domain, > + unsigned int id, bool enable, u32 flags) > +{ > + u32 ptcmd, pdstat, pdctl, mdstat, mdctl, next_state; > + > + if (enable) > + next_state = PSC_STATE_ENABLE; > + else if (flags & PSC_SWRSTDISABLE) > + next_state = PSC_STATE_SWRSTDISABLE; > + else > + next_state = PSC_STATE_DISABLE; > + > + mdctl = readl(psc_base + MDCTL + REG_OFFSET * id); > + mdctl &= ~MDSTAT_STATE_MASK; > + mdctl |= next_state; > + if (flags & PSC_FORCE) > + mdctl |= MDCTL_FORCE; > + if (flags & PSC_LRESET) > + mdctl &= ~MDCTL_LRESET; > + writel(mdctl, psc_base + MDCTL + REG_OFFSET * id); > + > + pdstat = readl(psc_base + PDSTAT + REG_OFFSET * domain); > + if (!(pdstat & PDSTAT_STATE_MASK)) { > + pdctl = readl(psc_base + PDCTL + REG_OFFSET * domain); > + pdctl |= PDCTL_NEXT; > + writel(pdctl, psc_base + PDCTL + REG_OFFSET * domain); > + } > + > + ptcmd = 1 << domain; > + writel(ptcmd, psc_base + PTCMD); > + while ((readl(psc_base + PTSTAT) >> domain) & 1) > + ; > + > + do { > + mdstat = readl(psc_base + MDSTAT + REG_OFFSET * id); > + } while (!((mdstat & MDSTAT_STATE_MASK) == next_state)); > +} > + > +static int keystone_clk_is_enabled(struct clk_hw *hw) > +{ > + struct clk_psc *psc = to_clk_psc(hw); > + struct clk_psc_data *data = psc->psc_data; > + u32 mdstat = readl(data->base + MDSTAT + REG_OFFSET * data->lpsc); > + > + return (mdstat & MDSTAT_MCKOUT) ? 1 : 0; > +} > + > +static int keystone_clk_enable(struct clk_hw *hw) > +{ > + struct clk_psc *psc = to_clk_psc(hw); > + struct clk_psc_data *data = psc->psc_data; > + unsigned long flags = 0; > + > + if (psc->lock) > + spin_lock_irqsave(psc->lock, flags); > + > + psc_config(data->base, data->domain, data->lpsc, 1, data->psc_flags); > + > + if (psc->lock) > + spin_unlock_irqrestore(psc->lock, flags); > + > + return 0; > +} > + > +static void keystone_clk_disable(struct clk_hw *hw) > +{ > + struct clk_psc *psc = to_clk_psc(hw); > + struct clk_psc_data *data = psc->psc_data; > + unsigned long flags = 0; > + > + if (psc->lock) > + spin_lock_irqsave(psc->lock, flags); > + > + psc_config(data->base, data->domain, data->lpsc, 0, data->psc_flags); > + > + if (psc->lock) > + spin_unlock_irqrestore(psc->lock, flags); > +} > + > +static const struct clk_ops clk_psc_ops = { > + .enable = keystone_clk_enable, > + .disable = keystone_clk_disable, > + .is_enabled = keystone_clk_is_enabled, > +}; > + > +/** > + * clk_register_psc - register psc clock > + * @dev: device that is registering this clock > + * @name: name of this clock > + * @parent_name: name of clock's parent > + * @psc_data: platform data to configure this clock > + * @lock: spinlock used by this clock > + */ > +static struct clk *clk_register_psc(struct device *dev, > + const char *name, > + const char *parent_name, > + struct clk_psc_data *psc_data, > + spinlock_t *lock) > +{ > + struct clk_init_data init; > + struct clk_psc *psc; > + struct clk *clk; > + > + psc = kzalloc(sizeof(*psc), GFP_KERNEL); > + if (!psc) > + return ERR_PTR(-ENOMEM); > + > + init.name = name; > + init.ops = &clk_psc_ops; > + init.flags = psc_data->flags; > + init.parent_names = (parent_name ? &parent_name : NULL); Is &parent_name not a pointer to a pointer on the stack? Is this only used once? > + init.num_parents = (parent_name ? 1 : 0); > + > + psc->psc_data = psc_data; > + psc->lock = lock; > + psc->hw.init = &init; That's definitely a pointer to some data on the stack, and that seems to be kept around. Is this only used for one-time initialisation, or might it be used later? > + > + clk = clk_register(NULL, &psc->hw); > + if (IS_ERR(clk)) > + kfree(psc); > + > + return clk; > +} > + > +/** > + * of_psc_clk_init - initialize psc clock through DT > + * @node: device tree node for this clock > + * @lock: spinlock used by this clock > + */ > +static void __init of_psc_clk_init(struct device_node *node, spinlock_t *lock) > +{ > + const char *parent_name, *status = NULL, *base_flags = NULL; > + struct clk_psc_data *data; > + const char *clk_name = node->name; > + u32 gpsc = 0, lpsc = 0, pd = 0; > + struct resource res; > + struct clk *clk; > + int rc; > + > + data = kzalloc(sizeof(*data), GFP_KERNEL); > + WARN_ON(!data); Does that not mean things will blow up later? return now? > + > + if (of_address_to_resource(node, 0, &res)) { > + pr_err("psc_clk_init - no reg property defined\n"); > + goto out; > + } > + > + of_property_read_u32(node, "gpsc", &gpsc); > + of_property_read_u32(node, "lpsc", &lpsc); > + of_property_read_u32(node, "pd", &pd); > + > + if (gpsc >= NUM_GPSC) { > + pr_err("psc_clk_init - no reg property defined\n"); Wrong error message. > + goto out; > + } > + > + of_property_read_string(node, > + "clock-output-names", &clk_name); > + parent_name = of_clk_get_parent_name(node, 0); > + WARN_ON(!parent_name); Do you require the parent name? If so you need to fail gracefully, if not you don't need the WARN_ON (perhaps a pr_warn would be better?). > + > + /* Expected that same phy_base is used for all psc clocks of > + * a give gpsc. So ioremap is done only once. > + */ So these clocks are all components of a larger IP block? It might make more sense to describe the containing block, with the actual clocks as sub-nodes (or if the set of clocks for the containing block is known, just the containing block). > + if (psc_addr[gpsc].phy_base) { > + if (psc_addr[gpsc].phy_base != res.start) { > + pr_err("Different psc base for same GPSC\n"); > + goto out; > + } > + } else { > + psc_addr[gpsc].phy_base = res.start; > + psc_addr[gpsc].io_base = > + ioremap(res.start, resource_size(&res)); > + } > + > + WARN_ON(!psc_addr[gpsc].io_base); Surely things will blow up later if that's the case? return here instead? > + data->base = psc_addr[gpsc].io_base; > + data->lpsc = lpsc; > + data->gpsc = gpsc; > + data->domain = pd; > + > + if (of_property_read_bool(node, "ti,psc-lreset")) > + data->psc_flags |= PSC_LRESET; > + if (of_property_read_bool(node, "ti,psc-force")) > + data->psc_flags |= PSC_FORCE; Neither of these were defined in the binding, and they don't appear to have been inherited form another binding. What are they for? Why are they needed? > + > + clk = clk_register_psc(NULL, clk_name, parent_name, > + data, lock); > + > + if (clk) { > + of_clk_add_provider(node, of_clk_src_simple_get, clk); > + > + rc = of_property_read_string(node, "status", &status); > + if (status && !strcmp(status, "enabled")) > + clk_prepare_enable(clk); > + return; > + } As mentioned above, this abuses the standard status property, and it's not clear why this is necessary. Thanks, Mark. > + pr_err("psc_clk_init - error registering psc clk %s\n", node->name); > +out: > + kfree(data); > + return; > +} > + > +/** > + * of_keystone_psc_clk_init - initialize psc clock through DT > + * @node: device tree node for this clock > + */ > +void __init of_keystone_psc_clk_init(struct device_node *node) > +{ > + of_psc_clk_init(node, &psc_lock); > +} > +EXPORT_SYMBOL_GPL(of_keystone_psc_clk_init); > +CLK_OF_DECLARE(keystone_gate_clk, "keystone,psc-clk", of_keystone_psc_clk_init); > diff --git a/include/linux/clk/keystone.h b/include/linux/clk/keystone.h > index 1ade95d..7b3e154 100644 > --- a/include/linux/clk/keystone.h > +++ b/include/linux/clk/keystone.h > @@ -14,5 +14,6 @@ > #define __LINUX_CLK_KEYSTONE_H_ > > extern void of_keystone_pll_clk_init(struct device_node *node); > +extern void of_keystone_psc_clk_init(struct device_node *node); > > #endif /* __LINUX_CLK_KEYSTONE_H_ */ > -- > 1.7.9.5 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On Tuesday 13 August 2013 12:13 PM, Mark Rutland wrote: > [adding dt maintainers] > > On Mon, Aug 05, 2013 at 05:12:21PM +0100, Santosh Shilimkar wrote: >> Add the driver for the clock gate control which uses PSC (Power Sleep >> Controller) IP on Keystone 2 based SOCs. It is responsible for enabling and >> disabling of the clocks for different IPs present in the SoC. >> >> Cc: Mike Turquette <mturquette@linaro.org> >> >> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >> --- >> .../devicetree/bindings/clock/keystone-gate.txt | 30 ++ >> drivers/clk/keystone/gate.c | 306 ++++++++++++++++++++ >> include/linux/clk/keystone.h | 1 + >> 3 files changed, 337 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/clock/keystone-gate.txt >> create mode 100644 drivers/clk/keystone/gate.c >> >> diff --git a/Documentation/devicetree/bindings/clock/keystone-gate.txt b/Documentation/devicetree/bindings/clock/keystone-gate.txt >> new file mode 100644 >> index 0000000..40afef5 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/clock/keystone-gate.txt >> @@ -0,0 +1,30 @@ >> +Binding for Keystone gate control driver which uses PSC controller IP. >> + >> +This binding uses the common clock binding[1]. >> + >> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt >> + >> +Required properties: >> +- compatible : shall be "keystone,psc-clk". > > Similarly to my comments on patch 1, this should probably be something > more like "ti,keystone-psc-clock" > >> +- #clock-cells : from common clock binding; shall be set to 0. >> +- clocks : parent clock phandle >> +- reg : psc address address space >> + >> +Optional properties: >> +- clock-output-names : From common clock binding to override the >> + default output clock name >> +- status : "enabled" if clock is always enabled > > Huh? This is a standard property, for which ePAPR defines "okay", > "disabled", "fail", and "fail-sss", and not "enabled". This also doesn't > seem to follow the standard meaning judging by the code. > > It looks like this could be a boolean property > ("clock-always-enabled"?), but that assumes it's necessary. > > Why do you need this? > I dropped this already. Forgot to update the documentation. >> +- lpsc : lpsc module id, if not set defaults to zero > > What's that needed for? Are there multiple clocks sharing a common > register bank? > >> +- pd : power domain number, if not set defaults to zero (always ON) > > Please don't use abbreviations unnecessarily. "power-domain-id" or > similar would be far better. What exactly does this represent? Does the > clock IP itself handle power domains, or is there some external unit > that controls power domains? > pd is commonly used but I can expand it. This represent the power domain number. >> +- gpsc : gpsc number, if not set defaults to zero > > How does that interact with the lpsc property? When are these necessary? > lpsc is local clock/module control where as gpsc sits on top of it. >> diff --git a/drivers/clk/keystone/gate.c b/drivers/clk/keystone/gate.c >> new file mode 100644 >> index 0000000..72ac478 >> --- /dev/null >> +++ b/drivers/clk/keystone/gate.c [..] >> +/** >> + * clk_register_psc - register psc clock >> + * @dev: device that is registering this clock >> + * @name: name of this clock >> + * @parent_name: name of clock's parent >> + * @psc_data: platform data to configure this clock >> + * @lock: spinlock used by this clock >> + */ >> +static struct clk *clk_register_psc(struct device *dev, >> + const char *name, >> + const char *parent_name, >> + struct clk_psc_data *psc_data, >> + spinlock_t *lock) >> +{ >> + struct clk_init_data init; >> + struct clk_psc *psc; >> + struct clk *clk; >> + >> + psc = kzalloc(sizeof(*psc), GFP_KERNEL); >> + if (!psc) >> + return ERR_PTR(-ENOMEM); >> + >> + init.name = name; >> + init.ops = &clk_psc_ops; >> + init.flags = psc_data->flags; >> + init.parent_names = (parent_name ? &parent_name : NULL); > > Is &parent_name not a pointer to a pointer on the stack? Is this only > used once? > >> + init.num_parents = (parent_name ? 1 : 0); >> + >> + psc->psc_data = psc_data; >> + psc->lock = lock; >> + psc->hw.init = &init; > > That's definitely a pointer to some data on the stack, and that seems to > be kept around. Is this only used for one-time initialisation, or might > it be used later? > Its init only. >> + data->base = psc_addr[gpsc].io_base; >> + data->lpsc = lpsc; >> + data->gpsc = gpsc; >> + data->domain = pd; >> + >> + if (of_property_read_bool(node, "ti,psc-lreset")) >> + data->psc_flags |= PSC_LRESET; >> + if (of_property_read_bool(node, "ti,psc-force")) >> + data->psc_flags |= PSC_FORCE; > > Neither of these were defined in the binding, and they don't appear to > have been inherited form another binding. What are they for? Why are > they needed? > They represent the hardware support transition method needed to have proper clock enable/disable sequence to work. Will update the binding along with other comments which I agree. >> + >> + clk = clk_register_psc(NULL, clk_name, parent_name, >> + data, lock); >> + >> + if (clk) { >> + of_clk_add_provider(node, of_clk_src_simple_get, clk); >> + >> + rc = of_property_read_string(node, "status", &status); >> + if (status && !strcmp(status, "enabled")) >> + clk_prepare_enable(clk); >> + return; >> + } > > As mentioned above, this abuses the standard status property, and it's > not clear why this is necessary. > Actually I have removed this one from dt nodes. Looks like missed to pick the right patch while posting. Have dropped this change already. Regards, Santosh
On Tue, Aug 13, 2013 at 05:36:50PM +0100, Santosh Shilimkar wrote: > On Tuesday 13 August 2013 12:13 PM, Mark Rutland wrote: > > [adding dt maintainers] > > > > On Mon, Aug 05, 2013 at 05:12:21PM +0100, Santosh Shilimkar wrote: > >> Add the driver for the clock gate control which uses PSC (Power Sleep > >> Controller) IP on Keystone 2 based SOCs. It is responsible for enabling and > >> disabling of the clocks for different IPs present in the SoC. > >> > >> Cc: Mike Turquette <mturquette@linaro.org> > >> > >> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > >> --- > >> .../devicetree/bindings/clock/keystone-gate.txt | 30 ++ > >> drivers/clk/keystone/gate.c | 306 ++++++++++++++++++++ > >> include/linux/clk/keystone.h | 1 + > >> 3 files changed, 337 insertions(+) > >> create mode 100644 Documentation/devicetree/bindings/clock/keystone-gate.txt > >> create mode 100644 drivers/clk/keystone/gate.c > >> > >> diff --git a/Documentation/devicetree/bindings/clock/keystone-gate.txt b/Documentation/devicetree/bindings/clock/keystone-gate.txt > >> new file mode 100644 > >> index 0000000..40afef5 > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/clock/keystone-gate.txt > >> @@ -0,0 +1,30 @@ > >> +Binding for Keystone gate control driver which uses PSC controller IP. > >> + > >> +This binding uses the common clock binding[1]. > >> + > >> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt > >> + > >> +Required properties: > >> +- compatible : shall be "keystone,psc-clk". > > > > Similarly to my comments on patch 1, this should probably be something > > more like "ti,keystone-psc-clock" > > > >> +- #clock-cells : from common clock binding; shall be set to 0. > >> +- clocks : parent clock phandle > >> +- reg : psc address address space > >> + > >> +Optional properties: > >> +- clock-output-names : From common clock binding to override the > >> + default output clock name > >> +- status : "enabled" if clock is always enabled > > > > Huh? This is a standard property, for which ePAPR defines "okay", > > "disabled", "fail", and "fail-sss", and not "enabled". This also doesn't > > seem to follow the standard meaning judging by the code. > > > > It looks like this could be a boolean property > > ("clock-always-enabled"?), but that assumes it's necessary. > > > > Why do you need this? > > > I dropped this already. Forgot to update the documentation. Ok. > > >> +- lpsc : lpsc module id, if not set defaults to zero > > > > What's that needed for? Are there multiple clocks sharing a common > > register bank? > > > >> +- pd : power domain number, if not set defaults to zero (always ON) > > > > Please don't use abbreviations unnecessarily. "power-domain-id" or > > similar would be far better. What exactly does this represent? Does the > > clock IP itself handle power domains, or is there some external unit > > that controls power domains? > > > pd is commonly used but I can expand it. This represent the power > domain number. Does the the clock IP have some internal logic for handling power domains only covering its clocks, or is there some external power controller involved (i.e. do we possibly need to describe an external unit and a linkage to it?). > > >> +- gpsc : gpsc number, if not set defaults to zero > > > > How does that interact with the lpsc property? When are these necessary? > > > lpsc is local clock/module control where as gpsc sits on top of it. Ok. I'm not sure haivng these default to zero makes much sense, as that could easily hide errors in dts. It might make more sense to make them required and error out if they aren't present. Unless they're zero far more often? > > >> diff --git a/drivers/clk/keystone/gate.c b/drivers/clk/keystone/gate.c > >> new file mode 100644 > >> index 0000000..72ac478 > >> --- /dev/null > >> +++ b/drivers/clk/keystone/gate.c > > [..] > > >> +/** > >> + * clk_register_psc - register psc clock > >> + * @dev: device that is registering this clock > >> + * @name: name of this clock > >> + * @parent_name: name of clock's parent > >> + * @psc_data: platform data to configure this clock > >> + * @lock: spinlock used by this clock > >> + */ > >> +static struct clk *clk_register_psc(struct device *dev, > >> + const char *name, > >> + const char *parent_name, > >> + struct clk_psc_data *psc_data, > >> + spinlock_t *lock) > >> +{ > >> + struct clk_init_data init; > >> + struct clk_psc *psc; > >> + struct clk *clk; > >> + > >> + psc = kzalloc(sizeof(*psc), GFP_KERNEL); > >> + if (!psc) > >> + return ERR_PTR(-ENOMEM); > >> + > >> + init.name = name; > >> + init.ops = &clk_psc_ops; > >> + init.flags = psc_data->flags; > >> + init.parent_names = (parent_name ? &parent_name : NULL); > > > > Is &parent_name not a pointer to a pointer on the stack? Is this only > > used once? > > > >> + init.num_parents = (parent_name ? 1 : 0); > >> + > >> + psc->psc_data = psc_data; > >> + psc->lock = lock; > >> + psc->hw.init = &init; > > > > That's definitely a pointer to some data on the stack, and that seems to > > be kept around. Is this only used for one-time initialisation, or might > > it be used later? > > > Its init only. Ok. > > >> + data->base = psc_addr[gpsc].io_base; > >> + data->lpsc = lpsc; > >> + data->gpsc = gpsc; > >> + data->domain = pd; > >> + > >> + if (of_property_read_bool(node, "ti,psc-lreset")) > >> + data->psc_flags |= PSC_LRESET; > >> + if (of_property_read_bool(node, "ti,psc-force")) > >> + data->psc_flags |= PSC_FORCE; > > > > Neither of these were defined in the binding, and they don't appear to > > have been inherited form another binding. What are they for? Why are > > they needed? > > > They represent the hardware support transition method needed to have > proper clock enable/disable sequence to work. Will update the binding > along with other comments which I agree. Ok. > > >> + > >> + clk = clk_register_psc(NULL, clk_name, parent_name, > >> + data, lock); > >> + > >> + if (clk) { > >> + of_clk_add_provider(node, of_clk_src_simple_get, clk); > >> + > >> + rc = of_property_read_string(node, "status", &status); > >> + if (status && !strcmp(status, "enabled")) > >> + clk_prepare_enable(clk); > >> + return; > >> + } > > > > As mentioned above, this abuses the standard status property, and it's > > not clear why this is necessary. > > > Actually I have removed this one from dt nodes. Looks like missed > to pick the right patch while posting. Have dropped this change > already. Great! Thanks, Mark.
Quoting Mark Rutland (2013-08-13 09:53:44) > On Tue, Aug 13, 2013 at 05:36:50PM +0100, Santosh Shilimkar wrote: > > On Tuesday 13 August 2013 12:13 PM, Mark Rutland wrote: > > > [adding dt maintainers] > > > > > > On Mon, Aug 05, 2013 at 05:12:21PM +0100, Santosh Shilimkar wrote: > > >> Add the driver for the clock gate control which uses PSC (Power Sleep > > >> Controller) IP on Keystone 2 based SOCs. It is responsible for enabling and > > >> disabling of the clocks for different IPs present in the SoC. > > >> > > >> Cc: Mike Turquette <mturquette@linaro.org> > > >> > > >> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > > >> --- > > >> .../devicetree/bindings/clock/keystone-gate.txt | 30 ++ > > >> drivers/clk/keystone/gate.c | 306 ++++++++++++++++++++ > > >> include/linux/clk/keystone.h | 1 + > > >> 3 files changed, 337 insertions(+) > > >> create mode 100644 Documentation/devicetree/bindings/clock/keystone-gate.txt > > >> create mode 100644 drivers/clk/keystone/gate.c > > >> > > >> diff --git a/Documentation/devicetree/bindings/clock/keystone-gate.txt b/Documentation/devicetree/bindings/clock/keystone-gate.txt > > >> new file mode 100644 > > >> index 0000000..40afef5 > > >> --- /dev/null > > >> +++ b/Documentation/devicetree/bindings/clock/keystone-gate.txt > > >> @@ -0,0 +1,30 @@ > > >> +Binding for Keystone gate control driver which uses PSC controller IP. > > >> + > > >> +This binding uses the common clock binding[1]. > > >> + > > >> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt > > >> + > > >> +Required properties: > > >> +- compatible : shall be "keystone,psc-clk". > > > > > > Similarly to my comments on patch 1, this should probably be something > > > more like "ti,keystone-psc-clock" > > > > > >> +- #clock-cells : from common clock binding; shall be set to 0. > > >> +- clocks : parent clock phandle > > >> +- reg : psc address address space > > >> + > > >> +Optional properties: > > >> +- clock-output-names : From common clock binding to override the > > >> + default output clock name > > >> +- status : "enabled" if clock is always enabled > > > > > > Huh? This is a standard property, for which ePAPR defines "okay", > > > "disabled", "fail", and "fail-sss", and not "enabled". This also doesn't > > > seem to follow the standard meaning judging by the code. > > > > > > It looks like this could be a boolean property > > > ("clock-always-enabled"?), but that assumes it's necessary. > > > > > > Why do you need this? > > > > > I dropped this already. Forgot to update the documentation. > > Ok. > > > > > >> +- lpsc : lpsc module id, if not set defaults to zero > > > > > > What's that needed for? Are there multiple clocks sharing a common > > > register bank? > > > > > >> +- pd : power domain number, if not set defaults to zero (always ON) > > > > > > Please don't use abbreviations unnecessarily. "power-domain-id" or > > > similar would be far better. What exactly does this represent? Does the > > > clock IP itself handle power domains, or is there some external unit > > > that controls power domains? > > > > > pd is commonly used but I can expand it. This represent the power > > domain number. > > Does the the clock IP have some internal logic for handling power > domains only covering its clocks, or is there some external power > controller involved (i.e. do we possibly need to describe an external > unit and a linkage to it?). Hmm, does the clock own the power domain or does the power domain own the clock? As you well know on OMAP the clock resides within the power domain. So it seems to me that the better way to do this would be for the power domain to have it's own binding and node, and then reference the clock: power-domain { ... clocks = <&chipclk3>, <&chipclk4>; clock-names = "perclk", "uartclk"; ... }; Now maybe things are different on Keystone, but if not then I don't see why the clock binding should have anything to do with the power domain. > > > > > >> +- gpsc : gpsc number, if not set defaults to zero > > > > > > How does that interact with the lpsc property? When are these necessary? > > > > > lpsc is local clock/module control where as gpsc sits on top of it. Similar to the above, should the gpsc have it's own binding and node that contains the lpsc which also have their own bindings/nodes, which finally contain the clocks? Maybe that is over engineered but I want to consider this before the binding gets set in stone. Regards, Mike > > Ok. I'm not sure haivng these default to zero makes much sense, as that > could easily hide errors in dts. It might make more sense to make them > required and error out if they aren't present. > > Unless they're zero far more often? > > > > > >> diff --git a/drivers/clk/keystone/gate.c b/drivers/clk/keystone/gate.c > > >> new file mode 100644 > > >> index 0000000..72ac478 > > >> --- /dev/null > > >> +++ b/drivers/clk/keystone/gate.c > > > > [..] > > > > >> +/** > > >> + * clk_register_psc - register psc clock > > >> + * @dev: device that is registering this clock > > >> + * @name: name of this clock > > >> + * @parent_name: name of clock's parent > > >> + * @psc_data: platform data to configure this clock > > >> + * @lock: spinlock used by this clock > > >> + */ > > >> +static struct clk *clk_register_psc(struct device *dev, > > >> + const char *name, > > >> + const char *parent_name, > > >> + struct clk_psc_data *psc_data, > > >> + spinlock_t *lock) > > >> +{ > > >> + struct clk_init_data init; > > >> + struct clk_psc *psc; > > >> + struct clk *clk; > > >> + > > >> + psc = kzalloc(sizeof(*psc), GFP_KERNEL); > > >> + if (!psc) > > >> + return ERR_PTR(-ENOMEM); > > >> + > > >> + init.name = name; > > >> + init.ops = &clk_psc_ops; > > >> + init.flags = psc_data->flags; > > >> + init.parent_names = (parent_name ? &parent_name : NULL); > > > > > > Is &parent_name not a pointer to a pointer on the stack? Is this only > > > used once? > > > > > >> + init.num_parents = (parent_name ? 1 : 0); > > >> + > > >> + psc->psc_data = psc_data; > > >> + psc->lock = lock; > > >> + psc->hw.init = &init; > > > > > > That's definitely a pointer to some data on the stack, and that seems to > > > be kept around. Is this only used for one-time initialisation, or might > > > it be used later? > > > > > Its init only. > > Ok. > > > > > >> + data->base = psc_addr[gpsc].io_base; > > >> + data->lpsc = lpsc; > > >> + data->gpsc = gpsc; > > >> + data->domain = pd; > > >> + > > >> + if (of_property_read_bool(node, "ti,psc-lreset")) > > >> + data->psc_flags |= PSC_LRESET; > > >> + if (of_property_read_bool(node, "ti,psc-force")) > > >> + data->psc_flags |= PSC_FORCE; > > > > > > Neither of these were defined in the binding, and they don't appear to > > > have been inherited form another binding. What are they for? Why are > > > they needed? > > > > > They represent the hardware support transition method needed to have > > proper clock enable/disable sequence to work. Will update the binding > > along with other comments which I agree. > > Ok. > > > > > >> + > > >> + clk = clk_register_psc(NULL, clk_name, parent_name, > > >> + data, lock); > > >> + > > >> + if (clk) { > > >> + of_clk_add_provider(node, of_clk_src_simple_get, clk); > > >> + > > >> + rc = of_property_read_string(node, "status", &status); > > >> + if (status && !strcmp(status, "enabled")) > > >> + clk_prepare_enable(clk); > > >> + return; > > >> + } > > > > > > As mentioned above, this abuses the standard status property, and it's > > > not clear why this is necessary. > > > > > Actually I have removed this one from dt nodes. Looks like missed > > to pick the right patch while posting. Have dropped this change > > already. > > Great! > > Thanks, > Mark.
On Monday 19 August 2013 04:43 PM, Mike Turquette wrote: > Quoting Mark Rutland (2013-08-13 09:53:44) >> On Tue, Aug 13, 2013 at 05:36:50PM +0100, Santosh Shilimkar wrote: >>> On Tuesday 13 August 2013 12:13 PM, Mark Rutland wrote: >>>> [adding dt maintainers] >>>> >>>> On Mon, Aug 05, 2013 at 05:12:21PM +0100, Santosh Shilimkar wrote: >>>>> Add the driver for the clock gate control which uses PSC (Power Sleep >>>>> Controller) IP on Keystone 2 based SOCs. It is responsible for enabling and >>>>> disabling of the clocks for different IPs present in the SoC. >>>>> >>>>> Cc: Mike Turquette <mturquette@linaro.org> >>>>> >>>>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >>>>> --- >>>>> .../devicetree/bindings/clock/keystone-gate.txt | 30 ++ >>>>> drivers/clk/keystone/gate.c | 306 ++++++++++++++++++++ >>>>> include/linux/clk/keystone.h | 1 + >>>>> 3 files changed, 337 insertions(+) >>>>> create mode 100644 Documentation/devicetree/bindings/clock/keystone-gate.txt >>>>> create mode 100644 drivers/clk/keystone/gate.c >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/clock/keystone-gate.txt b/Documentation/devicetree/bindings/clock/keystone-gate.txt >>>>> new file mode 100644 >>>>> index 0000000..40afef5 >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/clock/keystone-gate.txt >>>>> @@ -0,0 +1,30 @@ >>>>> +Binding for Keystone gate control driver which uses PSC controller IP. >>>>> + >>>>> +This binding uses the common clock binding[1]. >>>>> + >>>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt >>>>> + >>>>> +Required properties: >>>>> +- compatible : shall be "keystone,psc-clk". >>>> >>>> Similarly to my comments on patch 1, this should probably be something >>>> more like "ti,keystone-psc-clock" >>>> >>>>> +- #clock-cells : from common clock binding; shall be set to 0. >>>>> +- clocks : parent clock phandle >>>>> +- reg : psc address address space >>>>> + >>>>> +Optional properties: >>>>> +- clock-output-names : From common clock binding to override the >>>>> + default output clock name >>>>> +- status : "enabled" if clock is always enabled >>>> >>>> Huh? This is a standard property, for which ePAPR defines "okay", >>>> "disabled", "fail", and "fail-sss", and not "enabled". This also doesn't >>>> seem to follow the standard meaning judging by the code. >>>> >>>> It looks like this could be a boolean property >>>> ("clock-always-enabled"?), but that assumes it's necessary. >>>> >>>> Why do you need this? >>>> >>> I dropped this already. Forgot to update the documentation. >> >> Ok. >> >>> >>>>> +- lpsc : lpsc module id, if not set defaults to zero >>>> >>>> What's that needed for? Are there multiple clocks sharing a common >>>> register bank? >>>> >>>>> +- pd : power domain number, if not set defaults to zero (always ON) >>>> >>>> Please don't use abbreviations unnecessarily. "power-domain-id" or >>>> similar would be far better. What exactly does this represent? Does the >>>> clock IP itself handle power domains, or is there some external unit >>>> that controls power domains? >>>> >>> pd is commonly used but I can expand it. This represent the power >>> domain number. >> >> Does the the clock IP have some internal logic for handling power >> domains only covering its clocks, or is there some external power >> controller involved (i.e. do we possibly need to describe an external >> unit and a linkage to it?). > > Hmm, does the clock own the power domain or does the power domain own > the clock? As you well know on OMAP the clock resides within the power > domain. So it seems to me that the better way to do this would be for > the power domain to have it's own binding and node, and then reference > the clock: > > power-domain { > ... > clocks = <&chipclk3>, <&chipclk4>; > clock-names = "perclk", "uartclk"; > ... > }; > > Now maybe things are different on Keystone, but if not then I don't see > why the clock binding should have anything to do with the power domain. > They are bit different w.r.t OMAP. LPSC itself is the clock control of the IP. The LPSC number in the bindings is actually the specific number which is used to reach to the address space of the clock control. One can view that one as clock control register index. The power domain as such are above the lpsc but the clock enable/disable needs to follow a recommended sequence which needs the access to those registers. As such there is no major validation done on dynamically switching off PDs in current generation devices. As I mentioned you to on IRC, the PD was the only odd part I have to keep around to address the sequencing need which is kind of interlinked. >> >>> >>>>> +- gpsc : gpsc number, if not set defaults to zero >>>> >>>> How does that interact with the lpsc property? When are these necessary? >>>> >>> lpsc is local clock/module control where as gpsc sits on top of it. > > Similar to the above, should the gpsc have it's own binding and node > that contains the lpsc which also have their own bindings/nodes, which > finally contain the clocks? Maybe that is over engineered but I want to > consider this before the binding gets set in stone. > Actually GPSC need not be even mentioned since its one per SOC. Let me see if I can drop it to avoid any confusion. Regards, Santosh
Quoting Santosh Shilimkar (2013-08-20 06:55:54) > On Monday 19 August 2013 04:43 PM, Mike Turquette wrote: > > Quoting Mark Rutland (2013-08-13 09:53:44) > >> On Tue, Aug 13, 2013 at 05:36:50PM +0100, Santosh Shilimkar wrote: > >>> On Tuesday 13 August 2013 12:13 PM, Mark Rutland wrote: > >>>> [adding dt maintainers] > >>>> > >>>> On Mon, Aug 05, 2013 at 05:12:21PM +0100, Santosh Shilimkar wrote: > >>>>> Add the driver for the clock gate control which uses PSC (Power Sleep > >>>>> Controller) IP on Keystone 2 based SOCs. It is responsible for enabling and > >>>>> disabling of the clocks for different IPs present in the SoC. > >>>>> > >>>>> Cc: Mike Turquette <mturquette@linaro.org> > >>>>> > >>>>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > >>>>> --- > >>>>> .../devicetree/bindings/clock/keystone-gate.txt | 30 ++ > >>>>> drivers/clk/keystone/gate.c | 306 ++++++++++++++++++++ > >>>>> include/linux/clk/keystone.h | 1 + > >>>>> 3 files changed, 337 insertions(+) > >>>>> create mode 100644 Documentation/devicetree/bindings/clock/keystone-gate.txt > >>>>> create mode 100644 drivers/clk/keystone/gate.c > >>>>> > >>>>> diff --git a/Documentation/devicetree/bindings/clock/keystone-gate.txt b/Documentation/devicetree/bindings/clock/keystone-gate.txt > >>>>> new file mode 100644 > >>>>> index 0000000..40afef5 > >>>>> --- /dev/null > >>>>> +++ b/Documentation/devicetree/bindings/clock/keystone-gate.txt > >>>>> @@ -0,0 +1,30 @@ > >>>>> +Binding for Keystone gate control driver which uses PSC controller IP. > >>>>> + > >>>>> +This binding uses the common clock binding[1]. > >>>>> + > >>>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt > >>>>> + > >>>>> +Required properties: > >>>>> +- compatible : shall be "keystone,psc-clk". > >>>> > >>>> Similarly to my comments on patch 1, this should probably be something > >>>> more like "ti,keystone-psc-clock" > >>>> > >>>>> +- #clock-cells : from common clock binding; shall be set to 0. > >>>>> +- clocks : parent clock phandle > >>>>> +- reg : psc address address space > >>>>> + > >>>>> +Optional properties: > >>>>> +- clock-output-names : From common clock binding to override the > >>>>> + default output clock name > >>>>> +- status : "enabled" if clock is always enabled > >>>> > >>>> Huh? This is a standard property, for which ePAPR defines "okay", > >>>> "disabled", "fail", and "fail-sss", and not "enabled". This also doesn't > >>>> seem to follow the standard meaning judging by the code. > >>>> > >>>> It looks like this could be a boolean property > >>>> ("clock-always-enabled"?), but that assumes it's necessary. > >>>> > >>>> Why do you need this? > >>>> > >>> I dropped this already. Forgot to update the documentation. > >> > >> Ok. > >> > >>> > >>>>> +- lpsc : lpsc module id, if not set defaults to zero > >>>> > >>>> What's that needed for? Are there multiple clocks sharing a common > >>>> register bank? > >>>> > >>>>> +- pd : power domain number, if not set defaults to zero (always ON) > >>>> > >>>> Please don't use abbreviations unnecessarily. "power-domain-id" or > >>>> similar would be far better. What exactly does this represent? Does the > >>>> clock IP itself handle power domains, or is there some external unit > >>>> that controls power domains? > >>>> > >>> pd is commonly used but I can expand it. This represent the power > >>> domain number. > >> > >> Does the the clock IP have some internal logic for handling power > >> domains only covering its clocks, or is there some external power > >> controller involved (i.e. do we possibly need to describe an external > >> unit and a linkage to it?). > > > > Hmm, does the clock own the power domain or does the power domain own > > the clock? As you well know on OMAP the clock resides within the power > > domain. So it seems to me that the better way to do this would be for > > the power domain to have it's own binding and node, and then reference > > the clock: > > > > power-domain { > > ... > > clocks = <&chipclk3>, <&chipclk4>; > > clock-names = "perclk", "uartclk"; > > ... > > }; > > > > Now maybe things are different on Keystone, but if not then I don't see > > why the clock binding should have anything to do with the power domain. > > > They are bit different w.r.t OMAP. LPSC itself is the clock control of the > IP. The LPSC number in the bindings is actually the specific number which > is used to reach to the address space of the clock control. One can view > that one as clock control register index. Thanks for the information. I have a further question about then: are the LPSC clocks really module clocks that belong to the IP that they are gating? If so then they could be defined within the node defining their parent IP. That might be enough to get rid of the LPSC index value. Again I might be over-engineering it. Just trying to get an understanding. > > The power domain as such are above the lpsc but the clock enable/disable needs > to follow a recommended sequence which needs the access to those registers. > As such there is no major validation done on dynamically switching off PDs > in current generation devices. > > As I mentioned you to on IRC, the PD was the only odd part I have to keep > around to address the sequencing need which is kind of interlinked. Right. Well maybe some day that part can go away but I understand the need for it now. Regards, Mike > > >> > >>> > >>>>> +- gpsc : gpsc number, if not set defaults to zero > >>>> > >>>> How does that interact with the lpsc property? When are these necessary? > >>>> > >>> lpsc is local clock/module control where as gpsc sits on top of it. > > > > Similar to the above, should the gpsc have it's own binding and node > > that contains the lpsc which also have their own bindings/nodes, which > > finally contain the clocks? Maybe that is over engineered but I want to > > consider this before the binding gets set in stone. > > > Actually GPSC need not be even mentioned since its one per SOC. Let me > see if I can drop it to avoid any confusion. > > Regards, > Santosh
On Tuesday 20 August 2013 05:30 PM, Mike Turquette wrote: > Quoting Santosh Shilimkar (2013-08-20 06:55:54) >> On Monday 19 August 2013 04:43 PM, Mike Turquette wrote: >>> Quoting Mark Rutland (2013-08-13 09:53:44) >>>> On Tue, Aug 13, 2013 at 05:36:50PM +0100, Santosh Shilimkar wrote: >>>>> On Tuesday 13 August 2013 12:13 PM, Mark Rutland wrote: >>>>>> [adding dt maintainers] >>>>>> >>>>>> On Mon, Aug 05, 2013 at 05:12:21PM +0100, Santosh Shilimkar wrote: >>>>>>> Add the driver for the clock gate control which uses PSC (Power Sleep >>>>>>> Controller) IP on Keystone 2 based SOCs. It is responsible for enabling and >>>>>>> disabling of the clocks for different IPs present in the SoC. >>>>>>> >>>>>>> Cc: Mike Turquette <mturquette@linaro.org> >>>>>>> >>>>>>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> >>>>>>> --- >>>>>>> .../devicetree/bindings/clock/keystone-gate.txt | 30 ++ >>>>>>> drivers/clk/keystone/gate.c | 306 ++++++++++++++++++++ >>>>>>> include/linux/clk/keystone.h | 1 + >>>>>>> 3 files changed, 337 insertions(+) >>>>>>> create mode 100644 Documentation/devicetree/bindings/clock/keystone-gate.txt >>>>>>> create mode 100644 drivers/clk/keystone/gate.c >>>>>>> >>>>>>> diff --git a/Documentation/devicetree/bindings/clock/keystone-gate.txt b/Documentation/devicetree/bindings/clock/keystone-gate.txt >>>>>>> new file mode 100644 >>>>>>> index 0000000..40afef5 >>>>>>> --- /dev/null >>>>>>> +++ b/Documentation/devicetree/bindings/clock/keystone-gate.txt >>>>>>> @@ -0,0 +1,30 @@ >>>>>>> +Binding for Keystone gate control driver which uses PSC controller IP. >>>>>>> + >>>>>>> +This binding uses the common clock binding[1]. >>>>>>> + >>>>>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt >>>>>>> + >>>>>>> +Required properties: >>>>>>> +- compatible : shall be "keystone,psc-clk". >>>>>> >>>>>> Similarly to my comments on patch 1, this should probably be something >>>>>> more like "ti,keystone-psc-clock" >>>>>> >>>>>>> +- #clock-cells : from common clock binding; shall be set to 0. >>>>>>> +- clocks : parent clock phandle >>>>>>> +- reg : psc address address space >>>>>>> + >>>>>>> +Optional properties: >>>>>>> +- clock-output-names : From common clock binding to override the >>>>>>> + default output clock name >>>>>>> +- status : "enabled" if clock is always enabled >>>>>> >>>>>> Huh? This is a standard property, for which ePAPR defines "okay", >>>>>> "disabled", "fail", and "fail-sss", and not "enabled". This also doesn't >>>>>> seem to follow the standard meaning judging by the code. >>>>>> >>>>>> It looks like this could be a boolean property >>>>>> ("clock-always-enabled"?), but that assumes it's necessary. >>>>>> >>>>>> Why do you need this? >>>>>> >>>>> I dropped this already. Forgot to update the documentation. >>>> >>>> Ok. >>>> >>>>> >>>>>>> +- lpsc : lpsc module id, if not set defaults to zero >>>>>> >>>>>> What's that needed for? Are there multiple clocks sharing a common >>>>>> register bank? >>>>>> >>>>>>> +- pd : power domain number, if not set defaults to zero (always ON) >>>>>> >>>>>> Please don't use abbreviations unnecessarily. "power-domain-id" or >>>>>> similar would be far better. What exactly does this represent? Does the >>>>>> clock IP itself handle power domains, or is there some external unit >>>>>> that controls power domains? >>>>>> >>>>> pd is commonly used but I can expand it. This represent the power >>>>> domain number. >>>> >>>> Does the the clock IP have some internal logic for handling power >>>> domains only covering its clocks, or is there some external power >>>> controller involved (i.e. do we possibly need to describe an external >>>> unit and a linkage to it?). >>> >>> Hmm, does the clock own the power domain or does the power domain own >>> the clock? As you well know on OMAP the clock resides within the power >>> domain. So it seems to me that the better way to do this would be for >>> the power domain to have it's own binding and node, and then reference >>> the clock: >>> >>> power-domain { >>> ... >>> clocks = <&chipclk3>, <&chipclk4>; >>> clock-names = "perclk", "uartclk"; >>> ... >>> }; >>> >>> Now maybe things are different on Keystone, but if not then I don't see >>> why the clock binding should have anything to do with the power domain. >>> >> They are bit different w.r.t OMAP. LPSC itself is the clock control of the >> IP. The LPSC number in the bindings is actually the specific number which >> is used to reach to the address space of the clock control. One can view >> that one as clock control register index. > > Thanks for the information. I have a further question about then: are > the LPSC clocks really module clocks that belong to the IP that they are > gating? > LPSC controls the clock enable/disable to the IP/module so answer is yes. In certain cases LPSC controls clock to more than one IP as well. > If so then they could be defined within the node defining their parent > IP. That might be enough to get rid of the LPSC index value. Again I > might be over-engineering it. Just trying to get an understanding. > Am not sure I follow you here on not having the LPSC index. Sorry. >> >> The power domain as such are above the lpsc but the clock enable/disable needs >> to follow a recommended sequence which needs the access to those registers. >> As such there is no major validation done on dynamically switching off PDs >> in current generation devices. >> >> As I mentioned you to on IRC, the PD was the only odd part I have to keep >> around to address the sequencing need which is kind of interlinked. > > Right. Well maybe some day that part can go away but I understand the > need for it now. > right. Thanks regards, Santosh
Quoting Santosh Shilimkar (2013-08-20 14:55:56) > On Tuesday 20 August 2013 05:30 PM, Mike Turquette wrote: > > Quoting Santosh Shilimkar (2013-08-20 06:55:54) > >> On Monday 19 August 2013 04:43 PM, Mike Turquette wrote: > >>> Quoting Mark Rutland (2013-08-13 09:53:44) > >>>> On Tue, Aug 13, 2013 at 05:36:50PM +0100, Santosh Shilimkar wrote: > >>>>> On Tuesday 13 August 2013 12:13 PM, Mark Rutland wrote: > >>>>>> [adding dt maintainers] > >>>>>> > >>>>>> On Mon, Aug 05, 2013 at 05:12:21PM +0100, Santosh Shilimkar wrote: > >>>>>>> Add the driver for the clock gate control which uses PSC (Power Sleep > >>>>>>> Controller) IP on Keystone 2 based SOCs. It is responsible for enabling and > >>>>>>> disabling of the clocks for different IPs present in the SoC. > >>>>>>> > >>>>>>> Cc: Mike Turquette <mturquette@linaro.org> > >>>>>>> > >>>>>>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > >>>>>>> --- > >>>>>>> .../devicetree/bindings/clock/keystone-gate.txt | 30 ++ > >>>>>>> drivers/clk/keystone/gate.c | 306 ++++++++++++++++++++ > >>>>>>> include/linux/clk/keystone.h | 1 + > >>>>>>> 3 files changed, 337 insertions(+) > >>>>>>> create mode 100644 Documentation/devicetree/bindings/clock/keystone-gate.txt > >>>>>>> create mode 100644 drivers/clk/keystone/gate.c > >>>>>>> > >>>>>>> diff --git a/Documentation/devicetree/bindings/clock/keystone-gate.txt b/Documentation/devicetree/bindings/clock/keystone-gate.txt > >>>>>>> new file mode 100644 > >>>>>>> index 0000000..40afef5 > >>>>>>> --- /dev/null > >>>>>>> +++ b/Documentation/devicetree/bindings/clock/keystone-gate.txt > >>>>>>> @@ -0,0 +1,30 @@ > >>>>>>> +Binding for Keystone gate control driver which uses PSC controller IP. > >>>>>>> + > >>>>>>> +This binding uses the common clock binding[1]. > >>>>>>> + > >>>>>>> +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt > >>>>>>> + > >>>>>>> +Required properties: > >>>>>>> +- compatible : shall be "keystone,psc-clk". > >>>>>> > >>>>>> Similarly to my comments on patch 1, this should probably be something > >>>>>> more like "ti,keystone-psc-clock" > >>>>>> > >>>>>>> +- #clock-cells : from common clock binding; shall be set to 0. > >>>>>>> +- clocks : parent clock phandle > >>>>>>> +- reg : psc address address space > >>>>>>> + > >>>>>>> +Optional properties: > >>>>>>> +- clock-output-names : From common clock binding to override the > >>>>>>> + default output clock name > >>>>>>> +- status : "enabled" if clock is always enabled > >>>>>> > >>>>>> Huh? This is a standard property, for which ePAPR defines "okay", > >>>>>> "disabled", "fail", and "fail-sss", and not "enabled". This also doesn't > >>>>>> seem to follow the standard meaning judging by the code. > >>>>>> > >>>>>> It looks like this could be a boolean property > >>>>>> ("clock-always-enabled"?), but that assumes it's necessary. > >>>>>> > >>>>>> Why do you need this? > >>>>>> > >>>>> I dropped this already. Forgot to update the documentation. > >>>> > >>>> Ok. > >>>> > >>>>> > >>>>>>> +- lpsc : lpsc module id, if not set defaults to zero > >>>>>> > >>>>>> What's that needed for? Are there multiple clocks sharing a common > >>>>>> register bank? > >>>>>> > >>>>>>> +- pd : power domain number, if not set defaults to zero (always ON) > >>>>>> > >>>>>> Please don't use abbreviations unnecessarily. "power-domain-id" or > >>>>>> similar would be far better. What exactly does this represent? Does the > >>>>>> clock IP itself handle power domains, or is there some external unit > >>>>>> that controls power domains? > >>>>>> > >>>>> pd is commonly used but I can expand it. This represent the power > >>>>> domain number. > >>>> > >>>> Does the the clock IP have some internal logic for handling power > >>>> domains only covering its clocks, or is there some external power > >>>> controller involved (i.e. do we possibly need to describe an external > >>>> unit and a linkage to it?). > >>> > >>> Hmm, does the clock own the power domain or does the power domain own > >>> the clock? As you well know on OMAP the clock resides within the power > >>> domain. So it seems to me that the better way to do this would be for > >>> the power domain to have it's own binding and node, and then reference > >>> the clock: > >>> > >>> power-domain { > >>> ... > >>> clocks = <&chipclk3>, <&chipclk4>; > >>> clock-names = "perclk", "uartclk"; > >>> ... > >>> }; > >>> > >>> Now maybe things are different on Keystone, but if not then I don't see > >>> why the clock binding should have anything to do with the power domain. > >>> > >> They are bit different w.r.t OMAP. LPSC itself is the clock control of the > >> IP. The LPSC number in the bindings is actually the specific number which > >> is used to reach to the address space of the clock control. One can view > >> that one as clock control register index. > > > > Thanks for the information. I have a further question about then: are > > the LPSC clocks really module clocks that belong to the IP that they are > > gating? > > > LPSC controls the clock enable/disable to the IP/module so answer is yes. > In certain cases LPSC controls clock to more than one IP as well. > > > If so then they could be defined within the node defining their parent > > IP. That might be enough to get rid of the LPSC index value. Again I > > might be over-engineering it. Just trying to get an understanding. > > > Am not sure I follow you here on not having the LPSC index. Sorry. How are the 'reg' property and the 'lpsc' property related? Does the lpsc property modify the register address used to access the clock control bits? Thanks, Mike > > >> > >> The power domain as such are above the lpsc but the clock enable/disable needs > >> to follow a recommended sequence which needs the access to those registers. > >> As such there is no major validation done on dynamically switching off PDs > >> in current generation devices. > >> > >> As I mentioned you to on IRC, the PD was the only odd part I have to keep > >> around to address the sequencing need which is kind of interlinked. > > > > Right. Well maybe some day that part can go away but I understand the > > need for it now. > > > right. Thanks > > regards, > Santosh
On Tuesday 20 August 2013 06:41 PM, Mike Turquette wrote: > Quoting Santosh Shilimkar (2013-08-20 14:55:56) >> On Tuesday 20 August 2013 05:30 PM, Mike Turquette wrote: [...] >>>> They are bit different w.r.t OMAP. LPSC itself is the clock control of the >>>> IP. The LPSC number in the bindings is actually the specific number which >>>> is used to reach to the address space of the clock control. One can view >>>> that one as clock control register index. >>> >>> Thanks for the information. I have a further question about then: are >>> the LPSC clocks really module clocks that belong to the IP that they are >>> gating? >>> >> LPSC controls the clock enable/disable to the IP/module so answer is yes. >> In certain cases LPSC controls clock to more than one IP as well. >> >>> If so then they could be defined within the node defining their parent >>> IP. That might be enough to get rid of the LPSC index value. Again I >>> might be over-engineering it. Just trying to get an understanding. >>> >> Am not sure I follow you here on not having the LPSC index. Sorry. > > How are the 'reg' property and the 'lpsc' property related? Does the > lpsc property modify the register address used to access the clock > control bits? > Yes it does. Currently all nodes use fix address and then lpsc is used as an index. But I think we can do better by just using the right(offset) address in the reg property. Will have a look at it and see what I can do here. Thanks for asking this questions Mike regards, Santosh
Quoting Santosh Shilimkar (2013-08-20 15:54:15) > On Tuesday 20 August 2013 06:41 PM, Mike Turquette wrote: > > Quoting Santosh Shilimkar (2013-08-20 14:55:56) > >> On Tuesday 20 August 2013 05:30 PM, Mike Turquette wrote: > > [...] > > >>>> They are bit different w.r.t OMAP. LPSC itself is the clock control of the > >>>> IP. The LPSC number in the bindings is actually the specific number which > >>>> is used to reach to the address space of the clock control. One can view > >>>> that one as clock control register index. > >>> > >>> Thanks for the information. I have a further question about then: are > >>> the LPSC clocks really module clocks that belong to the IP that they are > >>> gating? > >>> > >> LPSC controls the clock enable/disable to the IP/module so answer is yes. > >> In certain cases LPSC controls clock to more than one IP as well. > >> > >>> If so then they could be defined within the node defining their parent > >>> IP. That might be enough to get rid of the LPSC index value. Again I > >>> might be over-engineering it. Just trying to get an understanding. > >>> > >> Am not sure I follow you here on not having the LPSC index. Sorry. > > > > How are the 'reg' property and the 'lpsc' property related? Does the > > lpsc property modify the register address used to access the clock > > control bits? > > > Yes it does. Currently all nodes use fix address and then lpsc is > used as an index. Ok cool. Well the reason I brought that up was because I even had the idea to define these module clocks within the module nodes that own them in DT. I am way outside of my DT knowledge at this point but I wonder if the following type of binding is possible: module: module@4a308200 { #address-cells = <1>; #size-cells = <1>; reg = <0x4a308200 0x1000>; clock { #clock-cells = <0>; compatible = "keystone,psc-clk"; clocks = <&chipclk3>; clock-output-names = "debugss_trc"; reg = <0x0256>; pd = <1>; }; }; Again, my DT knowledge is pretty limited, but could the reg property of the clock be directly affected by the parent node? That seems like it could nicely model what the hardware is really doing. > But I think we can do better by just using the > right(offset) address in the reg property. Will have a look at it > and see what I can do here. This also solves the problem nicely. Thanks for putting up with my silly questions ;-) Regards, Mike > > Thanks for asking this questions Mike > > regards, > Santosh
On Tuesday 20 August 2013 10:22 PM, Mike Turquette wrote: > Quoting Santosh Shilimkar (2013-08-20 15:54:15) >> On Tuesday 20 August 2013 06:41 PM, Mike Turquette wrote: >>> Quoting Santosh Shilimkar (2013-08-20 14:55:56) >>>> On Tuesday 20 August 2013 05:30 PM, Mike Turquette wrote: >> >> [...] >> >>>>>> They are bit different w.r.t OMAP. LPSC itself is the clock control of the >>>>>> IP. The LPSC number in the bindings is actually the specific number which >>>>>> is used to reach to the address space of the clock control. One can view >>>>>> that one as clock control register index. >>>>> >>>>> Thanks for the information. I have a further question about then: are >>>>> the LPSC clocks really module clocks that belong to the IP that they are >>>>> gating? >>>>> >>>> LPSC controls the clock enable/disable to the IP/module so answer is yes. >>>> In certain cases LPSC controls clock to more than one IP as well. >>>> >>>>> If so then they could be defined within the node defining their parent >>>>> IP. That might be enough to get rid of the LPSC index value. Again I >>>>> might be over-engineering it. Just trying to get an understanding. >>>>> >>>> Am not sure I follow you here on not having the LPSC index. Sorry. >>> >>> How are the 'reg' property and the 'lpsc' property related? Does the >>> lpsc property modify the register address used to access the clock >>> control bits? >>> >> Yes it does. Currently all nodes use fix address and then lpsc is >> used as an index. > > Ok cool. Well the reason I brought that up was because I even had the > idea to define these module clocks within the module nodes that own them > in DT. I am way outside of my DT knowledge at this point but I wonder > if the following type of binding is possible: > > module: module@4a308200 { > #address-cells = <1>; > #size-cells = <1>; > reg = <0x4a308200 0x1000>; > > clock { > #clock-cells = <0>; > compatible = "keystone,psc-clk"; > clocks = <&chipclk3>; > clock-output-names = "debugss_trc"; > reg = <0x0256>; > pd = <1>; > }; > }; > > Again, my DT knowledge is pretty limited, but could the reg property of > the clock be directly affected by the parent node? That seems like it > could nicely model what the hardware is really doing. > The module(I assume you mean IP here) reg address space is separate than that used for clock control so that doesn't fit as such. Traditionally clock controls even though targeted for specific modules sits in different control as at least seen on OMAP and Keystone. OCP wrappers on OMAP were bit of exceptions but they were little bit of glue logic without much control within the address space. >> But I think we can do better by just using the >> right(offset) address in the reg property. Will have a look at it >> and see what I can do here. > > This also solves the problem nicely. Thanks for putting up with my > silly questions ;-) > You asked right and good questions. Regards, Santosh
Quoting Santosh Shilimkar (2013-08-21 06:16:33) > On Tuesday 20 August 2013 10:22 PM, Mike Turquette wrote: > > Quoting Santosh Shilimkar (2013-08-20 15:54:15) > >> On Tuesday 20 August 2013 06:41 PM, Mike Turquette wrote: > >>> Quoting Santosh Shilimkar (2013-08-20 14:55:56) > >>>> On Tuesday 20 August 2013 05:30 PM, Mike Turquette wrote: > >> > >> [...] > >> > >>>>>> They are bit different w.r.t OMAP. LPSC itself is the clock control of the > >>>>>> IP. The LPSC number in the bindings is actually the specific number which > >>>>>> is used to reach to the address space of the clock control. One can view > >>>>>> that one as clock control register index. > >>>>> > >>>>> Thanks for the information. I have a further question about then: are > >>>>> the LPSC clocks really module clocks that belong to the IP that they are > >>>>> gating? > >>>>> > >>>> LPSC controls the clock enable/disable to the IP/module so answer is yes. > >>>> In certain cases LPSC controls clock to more than one IP as well. > >>>> > >>>>> If so then they could be defined within the node defining their parent > >>>>> IP. That might be enough to get rid of the LPSC index value. Again I > >>>>> might be over-engineering it. Just trying to get an understanding. > >>>>> > >>>> Am not sure I follow you here on not having the LPSC index. Sorry. > >>> > >>> How are the 'reg' property and the 'lpsc' property related? Does the > >>> lpsc property modify the register address used to access the clock > >>> control bits? > >>> > >> Yes it does. Currently all nodes use fix address and then lpsc is > >> used as an index. > > > > Ok cool. Well the reason I brought that up was because I even had the > > idea to define these module clocks within the module nodes that own them > > in DT. I am way outside of my DT knowledge at this point but I wonder > > if the following type of binding is possible: > > > > module: module@4a308200 { > > #address-cells = <1>; > > #size-cells = <1>; > > reg = <0x4a308200 0x1000>; > > > > clock { > > #clock-cells = <0>; > > compatible = "keystone,psc-clk"; > > clocks = <&chipclk3>; > > clock-output-names = "debugss_trc"; > > reg = <0x0256>; > > pd = <1>; > > }; > > }; > > > > Again, my DT knowledge is pretty limited, but could the reg property of > > the clock be directly affected by the parent node? That seems like it > > could nicely model what the hardware is really doing. > > > The module(I assume you mean IP here) reg address space is separate than > that used for clock control so that doesn't fit as such. Traditionally > clock controls even though targeted for specific modules sits in different > control as at least seen on OMAP and Keystone. OCP wrappers on OMAP > were bit of exceptions but they were little bit of glue logic without > much control within the address space. Great, you perfectly answered my questions. I think that assigning the "final" address to the 'reg' property is the right way to go (fixed address + LPSC index). Regards, Mike > > >> But I think we can do better by just using the > >> right(offset) address in the reg property. Will have a look at it > >> and see what I can do here. > > > > This also solves the problem nicely. Thanks for putting up with my > > silly questions ;-) > > > You asked right and good questions. > > Regards, > Santosh
On Thursday 22 August 2013 04:12 AM, Mike Turquette wrote: > Quoting Santosh Shilimkar (2013-08-21 06:16:33) >> On Tuesday 20 August 2013 10:22 PM, Mike Turquette wrote: >>> Quoting Santosh Shilimkar (2013-08-20 15:54:15) >>>> On Tuesday 20 August 2013 06:41 PM, Mike Turquette wrote: >>>>> Quoting Santosh Shilimkar (2013-08-20 14:55:56) >>>>>> On Tuesday 20 August 2013 05:30 PM, Mike Turquette wrote: >>>> >>>> [...] >>>> >>>>>>>> They are bit different w.r.t OMAP. LPSC itself is the clock control of the >>>>>>>> IP. The LPSC number in the bindings is actually the specific number which >>>>>>>> is used to reach to the address space of the clock control. One can view >>>>>>>> that one as clock control register index. >>>>>>> >>>>>>> Thanks for the information. I have a further question about then: are >>>>>>> the LPSC clocks really module clocks that belong to the IP that they are >>>>>>> gating? >>>>>>> >>>>>> LPSC controls the clock enable/disable to the IP/module so answer is yes. >>>>>> In certain cases LPSC controls clock to more than one IP as well. >>>>>> >>>>>>> If so then they could be defined within the node defining their parent >>>>>>> IP. That might be enough to get rid of the LPSC index value. Again I >>>>>>> might be over-engineering it. Just trying to get an understanding. >>>>>>> >>>>>> Am not sure I follow you here on not having the LPSC index. Sorry. >>>>> >>>>> How are the 'reg' property and the 'lpsc' property related? Does the >>>>> lpsc property modify the register address used to access the clock >>>>> control bits? >>>>> >>>> Yes it does. Currently all nodes use fix address and then lpsc is >>>> used as an index. >>> >>> Ok cool. Well the reason I brought that up was because I even had the >>> idea to define these module clocks within the module nodes that own them >>> in DT. I am way outside of my DT knowledge at this point but I wonder >>> if the following type of binding is possible: >>> >>> module: module@4a308200 { >>> #address-cells = <1>; >>> #size-cells = <1>; >>> reg = <0x4a308200 0x1000>; >>> >>> clock { >>> #clock-cells = <0>; >>> compatible = "keystone,psc-clk"; >>> clocks = <&chipclk3>; >>> clock-output-names = "debugss_trc"; >>> reg = <0x0256>; >>> pd = <1>; >>> }; >>> }; >>> >>> Again, my DT knowledge is pretty limited, but could the reg property of >>> the clock be directly affected by the parent node? That seems like it >>> could nicely model what the hardware is really doing. >>> >> The module(I assume you mean IP here) reg address space is separate than >> that used for clock control so that doesn't fit as such. Traditionally >> clock controls even though targeted for specific modules sits in different >> control as at least seen on OMAP and Keystone. OCP wrappers on OMAP >> were bit of exceptions but they were little bit of glue logic without >> much control within the address space. > > Great, you perfectly answered my questions. I think that assigning the > "final" address to the 'reg' property is the right way to go (fixed > address + LPSC index). > Thanks Mike. Regards, Santosh
diff --git a/Documentation/devicetree/bindings/clock/keystone-gate.txt b/Documentation/devicetree/bindings/clock/keystone-gate.txt new file mode 100644 index 0000000..40afef5 --- /dev/null +++ b/Documentation/devicetree/bindings/clock/keystone-gate.txt @@ -0,0 +1,30 @@ +Binding for Keystone gate control driver which uses PSC controller IP. + +This binding uses the common clock binding[1]. + +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt + +Required properties: +- compatible : shall be "keystone,psc-clk". +- #clock-cells : from common clock binding; shall be set to 0. +- clocks : parent clock phandle +- reg : psc address address space + +Optional properties: +- clock-output-names : From common clock binding to override the + default output clock name +- status : "enabled" if clock is always enabled +- lpsc : lpsc module id, if not set defaults to zero +- pd : power domain number, if not set defaults to zero (always ON) +- gpsc : gpsc number, if not set defaults to zero + +Example: + clock { + #clock-cells = <0>; + compatible = "keystone,psc-clk"; + clocks = <&chipclk3>; + clock-output-names = "debugss_trc"; + reg = <0x02350000 4096>; + lpsc = <5>; + pd = <1>; + }; diff --git a/drivers/clk/keystone/gate.c b/drivers/clk/keystone/gate.c new file mode 100644 index 0000000..72ac478 --- /dev/null +++ b/drivers/clk/keystone/gate.c @@ -0,0 +1,306 @@ +/* + * Clock driver for Keystone 2 based devices + * + * Copyright (C) 2013 Texas Instruments. + * Murali Karicheri <m-karicheri2@ti.com> + * Santosh Shilimkar <santosh.shilimkar@ti.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ +#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/slab.h> +#include <linux/of_address.h> +#include <linux/of.h> +#include <linux/module.h> + +/* PSC register offsets */ +#define PTCMD 0x120 +#define PTSTAT 0x128 +#define PDSTAT 0x200 +#define PDCTL 0x300 +#define MDSTAT 0x800 +#define MDCTL 0xa00 + +/* PSC module states */ +#define PSC_STATE_SWRSTDISABLE 0 +#define PSC_STATE_SYNCRST 1 +#define PSC_STATE_DISABLE 2 +#define PSC_STATE_ENABLE 3 + +#define MDSTAT_STATE_MASK 0x3f +#define MDSTAT_MCKOUT BIT(12) +#define PDSTAT_STATE_MASK 0x1f +#define MDCTL_FORCE BIT(31) +#define MDCTL_LRESET BIT(8) +#define PDCTL_NEXT BIT(0) + +/* PSC flags. Disable state is SwRstDisable */ +#define PSC_SWRSTDISABLE BIT(0) +/* Force module state transtition */ +#define PSC_FORCE BIT(1) +/* Keep module in local reset */ +#define PSC_LRESET BIT(2) +#define NUM_GPSC 2 +#define REG_OFFSET 4 + +/** + * struct clk_psc_data - PSC data + * @base: Base address for a PSC + * @flags: framework flags + * @lpsc: Is it local PSC + * @gpsc: Is it global PSC + * @domain: PSC domain + * + */ +struct clk_psc_data { + void __iomem *base; + u32 flags; + u32 psc_flags; + u8 lpsc; + u8 gpsc; + u8 domain; +}; + +/** + * struct clk_psc - PSC clock structure + * @hw: clk_hw for the psc + * @psc_data: PSC driver specific data + * @lock: Spinlock used by the driver + */ +struct clk_psc { + struct clk_hw hw; + struct clk_psc_data *psc_data; + spinlock_t *lock; +}; + +struct reg_psc { + u32 phy_base; + u32 size; + void __iomem *io_base; +}; + +static struct reg_psc psc_addr[NUM_GPSC]; +static DEFINE_SPINLOCK(psc_lock); + +#define to_clk_psc(_hw) container_of(_hw, struct clk_psc, hw) + +static void psc_config(void __iomem *psc_base, unsigned int domain, + unsigned int id, bool enable, u32 flags) +{ + u32 ptcmd, pdstat, pdctl, mdstat, mdctl, next_state; + + if (enable) + next_state = PSC_STATE_ENABLE; + else if (flags & PSC_SWRSTDISABLE) + next_state = PSC_STATE_SWRSTDISABLE; + else + next_state = PSC_STATE_DISABLE; + + mdctl = readl(psc_base + MDCTL + REG_OFFSET * id); + mdctl &= ~MDSTAT_STATE_MASK; + mdctl |= next_state; + if (flags & PSC_FORCE) + mdctl |= MDCTL_FORCE; + if (flags & PSC_LRESET) + mdctl &= ~MDCTL_LRESET; + writel(mdctl, psc_base + MDCTL + REG_OFFSET * id); + + pdstat = readl(psc_base + PDSTAT + REG_OFFSET * domain); + if (!(pdstat & PDSTAT_STATE_MASK)) { + pdctl = readl(psc_base + PDCTL + REG_OFFSET * domain); + pdctl |= PDCTL_NEXT; + writel(pdctl, psc_base + PDCTL + REG_OFFSET * domain); + } + + ptcmd = 1 << domain; + writel(ptcmd, psc_base + PTCMD); + while ((readl(psc_base + PTSTAT) >> domain) & 1) + ; + + do { + mdstat = readl(psc_base + MDSTAT + REG_OFFSET * id); + } while (!((mdstat & MDSTAT_STATE_MASK) == next_state)); +} + +static int keystone_clk_is_enabled(struct clk_hw *hw) +{ + struct clk_psc *psc = to_clk_psc(hw); + struct clk_psc_data *data = psc->psc_data; + u32 mdstat = readl(data->base + MDSTAT + REG_OFFSET * data->lpsc); + + return (mdstat & MDSTAT_MCKOUT) ? 1 : 0; +} + +static int keystone_clk_enable(struct clk_hw *hw) +{ + struct clk_psc *psc = to_clk_psc(hw); + struct clk_psc_data *data = psc->psc_data; + unsigned long flags = 0; + + if (psc->lock) + spin_lock_irqsave(psc->lock, flags); + + psc_config(data->base, data->domain, data->lpsc, 1, data->psc_flags); + + if (psc->lock) + spin_unlock_irqrestore(psc->lock, flags); + + return 0; +} + +static void keystone_clk_disable(struct clk_hw *hw) +{ + struct clk_psc *psc = to_clk_psc(hw); + struct clk_psc_data *data = psc->psc_data; + unsigned long flags = 0; + + if (psc->lock) + spin_lock_irqsave(psc->lock, flags); + + psc_config(data->base, data->domain, data->lpsc, 0, data->psc_flags); + + if (psc->lock) + spin_unlock_irqrestore(psc->lock, flags); +} + +static const struct clk_ops clk_psc_ops = { + .enable = keystone_clk_enable, + .disable = keystone_clk_disable, + .is_enabled = keystone_clk_is_enabled, +}; + +/** + * clk_register_psc - register psc clock + * @dev: device that is registering this clock + * @name: name of this clock + * @parent_name: name of clock's parent + * @psc_data: platform data to configure this clock + * @lock: spinlock used by this clock + */ +static struct clk *clk_register_psc(struct device *dev, + const char *name, + const char *parent_name, + struct clk_psc_data *psc_data, + spinlock_t *lock) +{ + struct clk_init_data init; + struct clk_psc *psc; + struct clk *clk; + + psc = kzalloc(sizeof(*psc), GFP_KERNEL); + if (!psc) + return ERR_PTR(-ENOMEM); + + init.name = name; + init.ops = &clk_psc_ops; + init.flags = psc_data->flags; + init.parent_names = (parent_name ? &parent_name : NULL); + init.num_parents = (parent_name ? 1 : 0); + + psc->psc_data = psc_data; + psc->lock = lock; + psc->hw.init = &init; + + clk = clk_register(NULL, &psc->hw); + if (IS_ERR(clk)) + kfree(psc); + + return clk; +} + +/** + * of_psc_clk_init - initialize psc clock through DT + * @node: device tree node for this clock + * @lock: spinlock used by this clock + */ +static void __init of_psc_clk_init(struct device_node *node, spinlock_t *lock) +{ + const char *parent_name, *status = NULL, *base_flags = NULL; + struct clk_psc_data *data; + const char *clk_name = node->name; + u32 gpsc = 0, lpsc = 0, pd = 0; + struct resource res; + struct clk *clk; + int rc; + + data = kzalloc(sizeof(*data), GFP_KERNEL); + WARN_ON(!data); + + if (of_address_to_resource(node, 0, &res)) { + pr_err("psc_clk_init - no reg property defined\n"); + goto out; + } + + of_property_read_u32(node, "gpsc", &gpsc); + of_property_read_u32(node, "lpsc", &lpsc); + of_property_read_u32(node, "pd", &pd); + + if (gpsc >= NUM_GPSC) { + pr_err("psc_clk_init - no reg property defined\n"); + goto out; + } + + of_property_read_string(node, + "clock-output-names", &clk_name); + parent_name = of_clk_get_parent_name(node, 0); + WARN_ON(!parent_name); + + /* Expected that same phy_base is used for all psc clocks of + * a give gpsc. So ioremap is done only once. + */ + if (psc_addr[gpsc].phy_base) { + if (psc_addr[gpsc].phy_base != res.start) { + pr_err("Different psc base for same GPSC\n"); + goto out; + } + } else { + psc_addr[gpsc].phy_base = res.start; + psc_addr[gpsc].io_base = + ioremap(res.start, resource_size(&res)); + } + + WARN_ON(!psc_addr[gpsc].io_base); + data->base = psc_addr[gpsc].io_base; + data->lpsc = lpsc; + data->gpsc = gpsc; + data->domain = pd; + + if (of_property_read_bool(node, "ti,psc-lreset")) + data->psc_flags |= PSC_LRESET; + if (of_property_read_bool(node, "ti,psc-force")) + data->psc_flags |= PSC_FORCE; + + clk = clk_register_psc(NULL, clk_name, parent_name, + data, lock); + + if (clk) { + of_clk_add_provider(node, of_clk_src_simple_get, clk); + + rc = of_property_read_string(node, "status", &status); + if (status && !strcmp(status, "enabled")) + clk_prepare_enable(clk); + return; + } + pr_err("psc_clk_init - error registering psc clk %s\n", node->name); +out: + kfree(data); + return; +} + +/** + * of_keystone_psc_clk_init - initialize psc clock through DT + * @node: device tree node for this clock + */ +void __init of_keystone_psc_clk_init(struct device_node *node) +{ + of_psc_clk_init(node, &psc_lock); +} +EXPORT_SYMBOL_GPL(of_keystone_psc_clk_init); +CLK_OF_DECLARE(keystone_gate_clk, "keystone,psc-clk", of_keystone_psc_clk_init); diff --git a/include/linux/clk/keystone.h b/include/linux/clk/keystone.h index 1ade95d..7b3e154 100644 --- a/include/linux/clk/keystone.h +++ b/include/linux/clk/keystone.h @@ -14,5 +14,6 @@ #define __LINUX_CLK_KEYSTONE_H_ extern void of_keystone_pll_clk_init(struct device_node *node); +extern void of_keystone_psc_clk_init(struct device_node *node); #endif /* __LINUX_CLK_KEYSTONE_H_ */
Add the driver for the clock gate control which uses PSC (Power Sleep Controller) IP on Keystone 2 based SOCs. It is responsible for enabling and disabling of the clocks for different IPs present in the SoC. Cc: Mike Turquette <mturquette@linaro.org> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> --- .../devicetree/bindings/clock/keystone-gate.txt | 30 ++ drivers/clk/keystone/gate.c | 306 ++++++++++++++++++++ include/linux/clk/keystone.h | 1 + 3 files changed, 337 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/keystone-gate.txt create mode 100644 drivers/clk/keystone/gate.c