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-kariche...@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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/