[PATCH 02/13] clk: davinci - add PSC clock driver
Sekhar Nori
nsekhar at ti.com
Wed Oct 10 08:35:34 EDT 2012
On 9/26/2012 11:37 PM, Murali Karicheri wrote:
> This is the driver for the Power Sleep Controller (PSC) hardware
> found on DM SoCs as well Keystone SoCs (c6x). This driver borrowed
> code from arch/arm/mach-davinci/psc.c and implemented the driver
> as per common clock provider API. The PSC module is responsible for
> enabling/disabling the Power Domain and Clock domain for different IPs
> present in the SoC. The driver is configured through the platform
> data structure struct clk_davinci_psc_data.
>
> Signed-off-by: Murali Karicheri <m-karicheri2 at ti.com>
>
> diff --git a/drivers/clk/davinci/clk-davinci-psc.c b/drivers/clk/davinci/clk-davinci-psc.c
> new file mode 100644
> index 0000000..b7aa332
> --- /dev/null
> +++ b/drivers/clk/davinci/clk-davinci-psc.c
> @@ -0,0 +1,197 @@
> +/*
> + * PSC clk driver for DaVinci devices
> + *
> + * Copyright (C) 2006-2012 Texas Instruments.
> + * Copyright (C) 2008-2009 Deep Root Systems, LLC
> + *
> + * 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/platform_data/clk-davinci-psc.h>
Sort these alphabetically.
> +
> +/* PSC register offsets */
> +#define EPCPR 0x070
> +#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 PDSTAT_STATE_MASK 0x1f
> +#define MDCTL_FORCE BIT(31)
> +#define PDCTL_NEXT BIT(0)
> +#define PDCTL_EPCGOOD BIT(8)
> +
> +/* PSC flags */
> +#define PSC_SWRSTDISABLE BIT(0) /* Disable state is SwRstDisable */
> +#define PSC_FORCE BIT(1) /* Force module state transtition */
> +#define PSC_HAS_EXT_POWER_CNTL BIT(2) /* PSC has external power control
> + * available (for DM6446 SoC) */
Can you try and keep the comment above on a single line?
> +/**
> + * struct clk_psc - DaVinci PSC clock
> + * @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_davinci_psc_data *psc_data;
> + spinlock_t *lock;
> +};
> +
> +#define to_clk_psc(_hw) container_of(_hw, struct clk_psc, hw)
> +
> +/* Enable or disable a PSC domain */
> +static void clk_psc_config(void __iomem *base, unsigned int domain,
> + unsigned int id, bool enable, u32 flags)
> +{
> + u32 epcpr, ptcmd, ptstat, pdstat, pdctl, mdstat, mdctl;
> + u32 next_state = PSC_STATE_ENABLE;
> + void __iomem *psc_base = base;
> +
> + if (!enable) {
> + if (flags & PSC_SWRSTDISABLE)
> + next_state = PSC_STATE_SWRSTDISABLE;
> + else
> + next_state = PSC_STATE_DISABLE;
> + }
> +
> + mdctl = __raw_readl(psc_base + MDCTL + 4 * id);
Please convert all __raw_ variants to simple readl/writel() calls.
> + mdctl &= ~MDSTAT_STATE_MASK;
> + mdctl |= next_state;
> + if (flags & PSC_FORCE)
> + mdctl |= MDCTL_FORCE;
> + __raw_writel(mdctl, psc_base + MDCTL + 4 * id);
> +
> + pdstat = __raw_readl(psc_base + PDSTAT + 4 * domain);
> + if ((pdstat & PDSTAT_STATE_MASK) == 0) {
> + pdctl = __raw_readl(psc_base + PDCTL + 4 * domain);
> + pdctl |= PDCTL_NEXT;
> + __raw_writel(pdctl, psc_base + PDCTL + 4 * domain);
> +
> + ptcmd = 1 << domain;
> + __raw_writel(ptcmd, psc_base + PTCMD);
> +
> + if (flags & PSC_HAS_EXT_POWER_CNTL) {
> + do {
> + epcpr = __raw_readl(psc_base + EPCPR);
> + } while ((((epcpr >> domain) & 1) == 0));
> + }
> +
> + pdctl = __raw_readl(psc_base + PDCTL + 4 * domain);
> + pdctl |= 0x100;
> + __raw_writel(pdctl, psc_base + PDCTL + 4 * domain);
> +
> + pdctl = __raw_readl(psc_base + PDCTL + 4 * domain);
> + pdctl |= PDCTL_EPCGOOD;
> + __raw_writel(pdctl, psc_base + PDCTL + 4 * domain);
> + } else {
> + ptcmd = 1 << domain;
> + __raw_writel(ptcmd, psc_base + PTCMD);
> + }
> +
> + do {
> + ptstat = __raw_readl(psc_base + PTSTAT);
> + } while (!(((ptstat >> domain) & 1) == 0));
> +
> + do {
> + mdstat = __raw_readl(psc_base + MDSTAT + 4 * id);
> + } while (!((mdstat & MDSTAT_STATE_MASK) == next_state));
> +}
> +
> +static int clk_psc_is_enabled(struct clk_hw *hw)
> +{
> + struct clk_psc *psc = to_clk_psc(hw);
> + struct clk_davinci_psc_data *psc_data = psc->psc_data;
> + u32 mdstat;
> +
> + mdstat = __raw_readl(psc_data->base + MDSTAT + 4 * psc_data->lpsc);
> + /* if clocked, state can be "Enable" or "SyncReset" */
> + return (mdstat & BIT(12)) ? 1 : 0;
Can you include a blank line before the return. Also, since the API
seems to expect a 0 or 1, you can simply do:
return !!(mdstat & BIT(12);
> +}
> +
> +static int clk_psc_enable(struct clk_hw *hw)
> +{
> + struct clk_psc *psc = to_clk_psc(hw);
> + struct clk_davinci_psc_data *psc_data = psc->psc_data;
> + unsigned long flags = 0;
No need to initialize flags here.
> +
> + if (psc->lock)
> + spin_lock_irqsave(psc->lock, flags);
Is locking really optional here?
> +
> + clk_psc_config(psc_data->base, psc_data->domain, psc_data->lpsc,
> + 1, psc_data->psc_flags);
> +
> + if (psc->lock)
> + spin_unlock_irqrestore(psc->lock, flags);
> +
> + return 0;
> +}
> +
> +static void clk_psc_disable(struct clk_hw *hw)
> +{
> + struct clk_psc *psc = to_clk_psc(hw);
> + struct clk_davinci_psc_data *psc_data = psc->psc_data;
> + unsigned long flags = 0;
> +
> + if (psc->lock)
> + spin_lock_irqsave(psc->lock, flags);
> +
> + clk_psc_config(psc_data->base, psc_data->domain, psc_data->lpsc,
> + 0, psc_data->psc_flags);
> +
> + if (psc->lock)
> + spin_unlock_irqrestore(psc->lock, flags);
> +}
> +
> +static const struct clk_ops clk_psc_ops = {
> + .enable = clk_psc_enable,
> + .disable = clk_psc_disable,
> + .is_enabled = clk_psc_is_enabled,
> +};
> +
> +struct clk *clk_register_davinci_psc(struct device *dev, const char *name,
> + const char *parent_name,
> + struct clk_davinci_psc_data *psc_data,
> + spinlock_t *lock)
Why do you need the lock to be provided from outside of this file? You
can initialize a lock for serializing writes to PSC registers within
this file, no?
> +{
> + 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;
> +}
I guess, an unregister API will be useful here as well.
> diff --git a/include/linux/platform_data/clk-davinci-psc.h b/include/linux/platform_data/clk-davinci-psc.h
> new file mode 100644
> index 0000000..b805f72
> --- /dev/null
> +++ b/include/linux/platform_data/clk-davinci-psc.h
> @@ -0,0 +1,53 @@
> +/*
> + * DaVinci Power & Sleep Controller (PSC) clk driver platform data
> + *
> + * Copyright (C) 2006-2012 Texas Instruments.
> + *
> + * 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.
> + *
> + * THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESS OR IMPLIED
> + * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN
> + * NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
> + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
> + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
> + * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
> + * ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
Is this taken from GPL text? There should be no need to have this here.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 675 Mass Ave, Cambridge, MA 02139, USA.
This is not needed as well. ;-)
Thanks,
Sekhar
More information about the linux-arm-kernel
mailing list