RE: [PATCH 02/13] clk: davinci - add PSC clock driver

From: Karicheri, Muralidharan
Date: Wed Oct 10 2012 - 10:36:04 EST

Next message: Matt Porter: "Re: [PATCH] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode"
Previous message: Matt Porter: "Re: [PATCH] ARM: OMAP2+: AM33XX: clock data: fix mcasp entries"
In reply to: Karicheri, Muralidharan: "RE: [PATCH 02/13] clk: davinci - add PSC clock driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

>> -----Original Message-----
>> From: Nori, Sekhar
>> Sent: Wednesday, October 10, 2012 8:36 AM
>> To: Karicheri, Muralidharan
>> Cc: mturquette@xxxxxxxxxx; arnd@xxxxxxxx; akpm@xxxxxxxxxxxxxxxxxxxx;
>> shawn.guo@xxxxxxxxxx; rob.herring@xxxxxxxxxxx; linus.walleij@xxxxxxxxxx;
>> viresh.linux@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Hilman, Kevin;
>> linux@xxxxxxxxxxxxxxxx; davinci-linux-open-source@xxxxxxxxxxxxxxxxxxxx; linux-arm-
>> kernel@xxxxxxxxxxxxxxxxxxx; linux-keystone@xxxxxxxxxxx - Linux developers for Keystone
>> family of devices (May contain non-TIers); linux-c6x-dev@xxxxxxxxxxxxx; Chemparathy,
>> Cyril
>> Subject: Re: [PATCH 02/13] clk: davinci - add PSC clock driver
>>
>> 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@xxxxxx>
>> >
>> > 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

Thanks. I will take care of this in the next revision.

Murali
N‹§²æìr¸›yúèšØb²X¬¶ÇvØ^–)Þ{.nÇ+‰·¥Š{±‘êçzX§¶›¡Ü}©ž²ÆzÚ&j:+v‰¨¾«‘êçzZ+€Ê+zf£¢·hšˆ§~††Ûiÿûàz¹®w¥¢¸?™¨èÚ&¢)ßf”ù^jÇy§m…á@A«a¶Úÿ0¶ìh®å’i

Next message: Matt Porter: "Re: [PATCH] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode"
Previous message: Matt Porter: "Re: [PATCH] ARM: OMAP2+: AM33XX: clock data: fix mcasp entries"
In reply to: Karicheri, Muralidharan: "RE: [PATCH 02/13] clk: davinci - add PSC clock driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]