[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@xxxxxxxxxx>
>
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@xxxxxx>
> ---
> .../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@xxxxxx>
> + * Santosh Shilimkar <santosh.shilimkar@xxxxxx>
> + *
> + * 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@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
