Re: [PATCH 01/13] clk: davinci - add Main PLL clock driver

Re: [PATCH 01/13] clk: davinci - add Main PLL clock driver

[Linus Walleij]

On Wed, Sep 26, 2012 at 8:07 PM, Murali Karicheri <m-karicheri2@ti.com> wrote:

> +struct clk_davinci_pll_data {
> +       /* physical addresses set by platform code */
> +       u32 phy_pllm;
> +       /* if PLL has a prediv register this should be non zero */
> +       u32 phy_prediv;
> +       /* if PLL has a postdiv register this should be non zero */
> +       u32 phy_postdiv;
> +       /* mapped addresses. should be initialized by  */
> +       void __iomem *pllm;
> +       void __iomem *prediv;
> +       void __iomem *postdiv;
> +       u32 pllm_mask;
> +       u32 prediv_mask;
> +       u32 postdiv_mask;
> +       u32 num;
> +       /* framework flags */
> +       u32 flags;
> +       /* pll flags */
> +       u32 pll_flags;
> +       /* use this value for prediv */
> +       u32 fixed_prediv;
> +       /* multiply PLLM by this factor. By default most SOC set this to zero
> +        * that translates to a multiplier of 1 and incrementer of 1.
> +        * To override default, set this factor
> +        */
> +       u32 pllm_multiplier;
> +};
> +

No, that's not what I meant.

I meant like this:

/**
 * struct clk_davinci_pll_data - struct holding the PLL data
 * phy_pllm: physical addresses set by platform code
 * phy_prediv: ...
(...)
 */
struct clk_davinci_pll_data {
      u32 phy_pllm;
      u32 phy_prediv;
(...)
};

Yours,
Linus Walleij

RE: [linux-keystone] Re: [PATCH 01/13] clk: davinci - add Main PLL clock driver

[Karicheri, Muralidharan]

>> -----Original Message-----
>> From: Linus Walleij [mailto:linus.walleij@linaro.org]
>> Sent: Thursday, September 27, 2012 9:20 AM
>> To: Karicheri, Muralidharan
>> Cc: mturquette@linaro.org; arnd@arndb.de; akpm@linux-foundation.org;
>> shawn.guo@linaro.org; rob.herring@calxeda.com; viresh.linux@gmail.com; linux-
>> kernel@vger.kernel.org; Nori, Sekhar; Hilman, Kevin; linux@arm.linux.org.uk; davinci-
>> linux-open-source@linux.davincidsp.com; linux-arm-kernel@lists.infradead.org; linux-
>> keystone@list.ti.com - Linux developers for Keystone family of devices (May contain non-
>> TIers); linux-c6x-dev@linux-c6x.org; Chemparathy, Cyril
>> Subject: [linux-keystone] Re: [PATCH 01/13] clk: davinci - add Main PLL clock driver
>> 
>> On Wed, Sep 26, 2012 at 8:07 PM, Murali Karicheri <m-karicheri2@ti.com> wrote:
>> 
>> > +struct clk_davinci_pll_data {
>> > +       /* physical addresses set by platform code */
>> > +       u32 phy_pllm;
>> > +       /* if PLL has a prediv register this should be non zero */
>> > +       u32 phy_prediv;
>> > +       /* if PLL has a postdiv register this should be non zero */
>> > +       u32 phy_postdiv;
>> > +       /* mapped addresses. should be initialized by  */
>> > +       void __iomem *pllm;
>> > +       void __iomem *prediv;
>> > +       void __iomem *postdiv;
>> > +       u32 pllm_mask;
>> > +       u32 prediv_mask;
>> > +       u32 postdiv_mask;
>> > +       u32 num;
>> > +       /* framework flags */
>> > +       u32 flags;
>> > +       /* pll flags */
>> > +       u32 pll_flags;
>> > +       /* use this value for prediv */
>> > +       u32 fixed_prediv;
>> > +       /* multiply PLLM by this factor. By default most SOC set this to zero
>> > +        * that translates to a multiplier of 1 and incrementer of 1.
>> > +        * To override default, set this factor
>> > +        */
>> > +       u32 pllm_multiplier;
>> > +};
>> > +
>> 
>> No, that's not what I meant.
>> 
>> I meant like this:
>> 
>> /**
>>  * struct clk_davinci_pll_data - struct holding the PLL data
>>  * phy_pllm: physical addresses set by platform code
>>  * phy_prediv: ...
>> (...)
>>  */
>> struct clk_davinci_pll_data {
>>       u32 phy_pllm;
>>       u32 phy_prediv;
>> (...)
>> };
>> 
Ok. Will do in the next revision.

Murali Karicheri
Software Design Engineer

>> Yours,
>> Linus Walleij

Re: [PATCH 01/13] clk: davinci - add Main PLL clock driver

[Sekhar Nori]

Hi Murali,

On 9/26/2012 11:37 PM, Murali Karicheri wrote:
> This is the driver for the main PLL clock hardware found on DM SoCs.
> This driver borrowed code from arch/arm/mach-davinci/clock.c and
> implemented the driver as per common clock provider API. The main PLL
> hardware typically has a multiplier, a pre-divider and a post-divider.
> Some of the SoCs has the divider fixed meaning they can not be
> configured through a register. HAS_PREDIV and HAS_POSTDIV flags are used
> to tell the driver if a hardware has these dividers present or not.
> Driver is configured through the structure clk_davinci_pll_data
> that has the platform data for the driver.
> 
> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>

Are you using git-format-patch to generate the patches? It should have
added a diffstat here by default which is very useful in quickly
understanding what the patch is touching.
> 
> diff --git a/drivers/clk/davinci/clk-davinci-pll.c b/drivers/clk/davinci/clk-davinci-pll.c
> new file mode 100644
> index 0000000..13e1690
> --- /dev/null
> +++ b/drivers/clk/davinci/clk-davinci-pll.c
> @@ -0,0 +1,128 @@
> +/*
> + * PLL clk driver 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.
> + * TODO - Add set_parent_rate()
> + */
> +#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-pll.h>

It will be nice to keep these sorted in alphabetical order. You got it
almost right, except the last one. Keeping it sorted keeps out duplicates.

> +
> +#include <mach/cputype.h>

Hmm, why is this needed? mach/ includes in driver files make it tough to
use this on other architectures/machines. You have plan to use this on
driver on keystone and c6x as well, right? It will also break
multi-platform ARM build.

> +
> +#define PLLM		0x110
> +#define PLLM_PLLM_MASK  0xff
> +#define PREDIV          0x114
> +#define POSTDIV         0x128
> +#define PLLDIV_EN       BIT(15)

Can you use tabs for indentation?

> +
> +/**
> + * struct clk_davinci_pll - DaVinci Main pll clock

no capitalization on 'M' needed, I think.

> + * @hw: clk_hw for the pll
> + * @pll_data: PLL driver specific data
> + */
> +struct clk_davinci_pll {
> +	struct clk_hw hw;
> +	struct clk_davinci_pll_data *pll_data;
> +};
> +
> +#define to_clk_pll(_hw) container_of(_hw, struct clk_davinci_pll, hw)
> +
> +static unsigned long clk_pllclk_recalc(struct clk_hw *hw,
> +					unsigned long parent_rate)
> +{
> +	struct clk_davinci_pll *pll = to_clk_pll(hw);
> +	struct clk_davinci_pll_data *pll_data = pll->pll_data;
> +	u32 mult = 1, prediv = 1, postdiv = 1;

No need to initialize mult here since you are doing it right away below.

> +	unsigned long rate = parent_rate;
> +
> +	/* If there is a device specific recalc defined invoke it. Otherwise
> +	 * fallback to default one
> +	 */

This is not following the multi-line comment style defined in
Documentation/CodingStyle.

> +	mult = __raw_readl(pll_data->pllm);

Do not use __raw_ variants since they are not safe on ARMv7. Use
readl/writel instead.

> +	if (pll_data->pllm_multiplier)
> +		mult =  pll_data->pllm_multiplier *
> +				(mult & pll_data->pllm_mask);
> +	else
> +		mult = (mult & pll_data->pllm_mask) + 1;
> +
> +	if (pll_data->flags & CLK_DAVINCI_PLL_HAS_PREDIV) {
> +		/* pre-divider is fixed, take prediv value from pll_data  */
> +		if (pll_data->fixed_prediv)
> +			prediv = pll_data->fixed_prediv;

Since else is multi-line, if needs braces as well.

> +		else {
> +			prediv = __raw_readl(pll_data->prediv);
> +			if (prediv & PLLDIV_EN)
> +				prediv = (prediv & pll_data->prediv_mask) + 1;
> +			else
> +				prediv = 1;
> +		}
> +	}
> +
> +	if (pll_data->flags & CLK_DAVINCI_PLL_HAS_POSTDIV) {
> +		postdiv = __raw_readl(pll_data->postdiv);
> +		if (postdiv & PLLDIV_EN)
> +			postdiv = (postdiv & pll_data->postdiv_mask) + 1;
> +		else
> +			postdiv = 1;
> +	}
> +
> +	rate /= prediv;
> +	rate *= mult;
> +	rate /= postdiv;
> +
> +	pr_debug("PLL%d: input = %lu MHz [ ",
> +		 pll_data->num, parent_rate / 1000000);
> +	if (prediv > 1)
> +		pr_debug("/ %d ", prediv);
> +	if (mult > 1)
> +		pr_debug("* %d ", mult);
> +	if (postdiv > 1)
> +		pr_debug("/ %d ", postdiv);
> +	pr_debug("] --> %lu MHz output.\n", rate / 1000000);
> +	return rate;

Have a blank like before the return, its easier to read that way.

> +}
> +
> +static const struct clk_ops clk_pll_ops = {
> +	.recalc_rate = clk_pllclk_recalc,
> +};
> +
> +struct clk *clk_register_davinci_pll(struct device *dev, const char *name,
> +			const char *parent_name,
> +			struct clk_davinci_pll_data *pll_data)
> +{
> +	struct clk_init_data init;
> +	struct clk_davinci_pll *pll;
> +	struct clk *clk;
> +
> +	if (!pll_data)
> +		return ERR_PTR(-ENODEV);
> +
> +	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
> +	if (!pll)
> +		return ERR_PTR(-ENOMEM);
> +	init.name = name;
> +	init.ops = &clk_pll_ops;
> +	init.flags = pll_data->flags;
> +	init.parent_names = (parent_name ? &parent_name : NULL);
> +	init.num_parents = (parent_name ? 1 : 0);
> +
> +	pll->pll_data	= pll_data;
> +	pll->hw.init = &init;
> +
> +	clk = clk_register(NULL, &pll->hw);
> +	if (IS_ERR(clk))
> +		kfree(pll);
> +
> +	return clk;
> +}

I guess there is an an "unregister" required as well which will free the
pll memory allocated above and unregister the clock? Not sure if you
would ever unregister a PLL, but providing this will probably help symmetry.

Thanks,
Sekhar

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

[Sekhar Nori]

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@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

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

[Sekhar Nori]

On 10/10/2012 6:05 PM, Sekhar Nori wrote:

>> +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?

Looking again, it seems like the common clock framework defines an
"enable_lock" in drivers/clk/clk.c to serialize the clock enable/disable
calls. Unless I am missing something, this lock seems unnecessary.

Thanks,
Sekhar

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

[Karicheri, Muralidharan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1602 bytes --]

>> -----Original Message-----
>> From: Nori, Sekhar
>> Sent: Wednesday, October 10, 2012 8:46 AM
>> To: Nori, Sekhar
>> Cc: Karicheri, Muralidharan; Hilman, Kevin; davinci-linux-open-
>> source@linux.davincidsp.com; mturquette@linaro.org; linux-c6x-dev@linux-c6x.org;
>> arnd@arndb.de; linus.walleij@linaro.org; linux-kernel@vger.kernel.org;
>> rob.herring@calxeda.com; linux-keystone@list.ti.com - Linux developers for Keystone
>> family of devices (May contain non-TIers); viresh.linux@gmail.com;
>> linux@arm.linux.org.uk; akpm@linux-foundation.org; shawn.guo@linaro.org; linux-arm-
>> kernel@lists.infradead.org
>> Subject: Re: [PATCH 02/13] clk: davinci - add PSC clock driver
>> 
>> On 10/10/2012 6:05 PM, Sekhar Nori wrote:
>> 
>> >> +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?
>> 
>> Looking again, it seems like the common clock framework defines an "enable_lock" in
>> drivers/clk/clk.c to serialize the clock enable/disable calls. Unless I am missing something,
>> this lock seems unnecessary.
>> 

I think you are right. For enable() api, enable_lock is sufficient and I will remove this.

>> Thanks,
>> Sekhar
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 01/13] calk: davinci - add Main PLL clock driver

[Karicheri, Muralidharan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 7638 bytes --]

>> -----Original Message-----
>> From: Nori, Sekhar
>> Sent: Wednesday, October 10, 2012 8:02 AM
>> To: Karicheri, Muralidharan
>> Cc: mturquette@linaro.org; arnd@arndb.de; akpm@linux-foundation.org;
>> shawn.guo@linaro.org; rob.herring@calxeda.com; linus.walleij@linaro.org;
>> viresh.linux@gmail.com; linux-kernel@vger.kernel.org; Hilman, Kevin;
>> linux@arm.linux.org.uk; davinci-linux-open-source@linux.davincidsp.com; linux-arm-
>> kernel@lists.infradead.org; linux-keystone@list.ti.com - Linux developers for Keystone
>> family of devices (May contain non-TIers); linux-c6x-dev@linux-c6x.org; Chemparathy,
>> Cyril
>> Subject: Re: [PATCH 01/13] clk: davinci - add Main PLL clock driver
>> 
>> Hi Murali,
>> 
>> On 9/26/2012 11:37 PM, Murali Karicheri wrote:
>> > This is the driver for the main PLL clock hardware found on DM SoCs.
>> > This driver borrowed code from arch/arm/mach-davinci/clock.c and
>> > implemented the driver as per common clock provider API. The main PLL
>> > hardware typically has a multiplier, a pre-divider and a post-divider.
>> > Some of the SoCs has the divider fixed meaning they can not be
>> > configured through a register. HAS_PREDIV and HAS_POSTDIV flags are
>> > used to tell the driver if a hardware has these dividers present or not.
>> > Driver is configured through the structure clk_davinci_pll_data that
>> > has the platform data for the driver.
>> >
>> > Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>> 
>> Are you using git-format-patch to generate the patches? It should have added a diffstat
>> here by default which is very useful in quickly understanding what the patch is touching.
>> >
>> > diff --git a/drivers/clk/davinci/clk-davinci-pll.c
>> > b/drivers/clk/davinci/clk-davinci-pll.c
>> > new file mode 100644
>> > index 0000000..13e1690
>> > --- /dev/null
>> > +++ b/drivers/clk/davinci/clk-davinci-pll.c
>> > @@ -0,0 +1,128 @@
>> > +/*
>> > + * PLL clk driver 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.
>> > + * TODO - Add set_parent_rate()
>> > + */
>> > +#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-pll.h>
>> 
>> It will be nice to keep these sorted in alphabetical order. You got it almost right, except
>> the last one. Keeping it sorted keeps out duplicates.
>> 
>> > +
>> > +#include <mach/cputype.h>
>> 
>> Hmm, why is this needed? mach/ includes in driver files make it tough to use this on
>> other architectures/machines. You have plan to use this on driver on keystone and c6x as
>> well, right? It will also break multi-platform ARM build.
>> 
>> > +
>> > +#define PLLM		0x110
>> > +#define PLLM_PLLM_MASK  0xff
>> > +#define PREDIV          0x114
>> > +#define POSTDIV         0x128
>> > +#define PLLDIV_EN       BIT(15)
>> 
>> Can you use tabs for indentation?
>> 
>> > +
>> > +/**
>> > + * struct clk_davinci_pll - DaVinci Main pll clock
>> 
>> no capitalization on 'M' needed, I think.
>> 
>> > + * @hw: clk_hw for the pll
>> > + * @pll_data: PLL driver specific data  */ struct clk_davinci_pll {
>> > +	struct clk_hw hw;
>> > +	struct clk_davinci_pll_data *pll_data; };
>> > +
>> > +#define to_clk_pll(_hw) container_of(_hw, struct clk_davinci_pll, hw)
>> > +
>> > +static unsigned long clk_pllclk_recalc(struct clk_hw *hw,
>> > +					unsigned long parent_rate)
>> > +{
>> > +	struct clk_davinci_pll *pll = to_clk_pll(hw);
>> > +	struct clk_davinci_pll_data *pll_data = pll->pll_data;
>> > +	u32 mult = 1, prediv = 1, postdiv = 1;
>> 
>> No need to initialize mult here since you are doing it right away below.
>> 
>> > +	unsigned long rate = parent_rate;
>> > +
>> > +	/* If there is a device specific recalc defined invoke it. Otherwise
>> > +	 * fallback to default one
>> > +	 */
>> 
>> This is not following the multi-line comment style defined in Documentation/CodingStyle.
>> 
>> > +	mult = __raw_readl(pll_data->pllm);
>> 
>> Do not use __raw_ variants since they are not safe on ARMv7. Use readl/writel instead.
>> 
>> > +	if (pll_data->pllm_multiplier)
>> > +		mult =  pll_data->pllm_multiplier *
>> > +				(mult & pll_data->pllm_mask);
>> > +	else
>> > +		mult = (mult & pll_data->pllm_mask) + 1;
>> > +
>> > +	if (pll_data->flags & CLK_DAVINCI_PLL_HAS_PREDIV) {
>> > +		/* pre-divider is fixed, take prediv value from pll_data  */
>> > +		if (pll_data->fixed_prediv)
>> > +			prediv = pll_data->fixed_prediv;
>> 
>> Since else is multi-line, if needs braces as well.
>> 
>> > +		else {
>> > +			prediv = __raw_readl(pll_data->prediv);
>> > +			if (prediv & PLLDIV_EN)
>> > +				prediv = (prediv & pll_data->prediv_mask) + 1;
>> > +			else
>> > +				prediv = 1;
>> > +		}
>> > +	}
>> > +
>> > +	if (pll_data->flags & CLK_DAVINCI_PLL_HAS_POSTDIV) {
>> > +		postdiv = __raw_readl(pll_data->postdiv);
>> > +		if (postdiv & PLLDIV_EN)
>> > +			postdiv = (postdiv & pll_data->postdiv_mask) + 1;
>> > +		else
>> > +			postdiv = 1;
>> > +	}
>> > +
>> > +	rate /= prediv;
>> > +	rate *= mult;
>> > +	rate /= postdiv;
>> > +
>> > +	pr_debug("PLL%d: input = %lu MHz [ ",
>> > +		 pll_data->num, parent_rate / 1000000);
>> > +	if (prediv > 1)
>> > +		pr_debug("/ %d ", prediv);
>> > +	if (mult > 1)
>> > +		pr_debug("* %d ", mult);
>> > +	if (postdiv > 1)
>> > +		pr_debug("/ %d ", postdiv);
>> > +	pr_debug("] --> %lu MHz output.\n", rate / 1000000);
>> > +	return rate;
>> 
>> Have a blank like before the return, its easier to read that way.
>> 
>> > +}
>> > +
>> > +static const struct clk_ops clk_pll_ops = {
>> > +	.recalc_rate = clk_pllclk_recalc,
>> > +};
>> > +
>> > +struct clk *clk_register_davinci_pll(struct device *dev, const char *name,
>> > +			const char *parent_name,
>> > +			struct clk_davinci_pll_data *pll_data) {
>> > +	struct clk_init_data init;
>> > +	struct clk_davinci_pll *pll;
>> > +	struct clk *clk;
>> > +
>> > +	if (!pll_data)
>> > +		return ERR_PTR(-ENODEV);
>> > +
>> > +	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
>> > +	if (!pll)
>> > +		return ERR_PTR(-ENOMEM);
>> > +	init.name = name;
>> > +	init.ops = &clk_pll_ops;
>> > +	init.flags = pll_data->flags;
>> > +	init.parent_names = (parent_name ? &parent_name : NULL);
>> > +	init.num_parents = (parent_name ? 1 : 0);
>> > +
>> > +	pll->pll_data	= pll_data;
>> > +	pll->hw.init = &init;
>> > +
>> > +	clk = clk_register(NULL, &pll->hw);
>> > +	if (IS_ERR(clk))
>> > +		kfree(pll);
>> > +
>> > +	return clk;
>> > +}
>> 
>> I guess there is an an "unregister" required as well which will free the pll memory
>> allocated above and unregister the clock? Not sure if you would ever unregister a PLL,
>> but providing this will probably help symmetry.
Sekhar,

clk_unregister() itself is a null statement in clk.c. Besides none of the clk drivers presently have implemented the unregister(). So I believe this is unnecessary. 
>> 
>> Thanks,
>> Sekhar

All of your other comments will be addressed in v3 patch.

BTW, please review the v2 patch for the rest of the series. For the one you have already reviewed, it should be fine.

Murali

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Karicheri, Muralidharan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 10676 bytes --]

>> -----Original Message-----
>> From: Nori, Sekhar
>> Sent: Wednesday, October 10, 2012 8:36 AM
>> To: Karicheri, Muralidharan
>> Cc: mturquette@linaro.org; arnd@arndb.de; akpm@linux-foundation.org;
>> shawn.guo@linaro.org; rob.herring@calxeda.com; linus.walleij@linaro.org;
>> viresh.linux@gmail.com; linux-kernel@vger.kernel.org; Hilman, Kevin;
>> linux@arm.linux.org.uk; davinci-linux-open-source@linux.davincidsp.com; linux-arm-
>> kernel@lists.infradead.org; linux-keystone@list.ti.com - Linux developers for Keystone
>> family of devices (May contain non-TIers); linux-c6x-dev@linux-c6x.org; 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@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

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

Murali
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 01/13] calk: davinci - add Main PLL clock driver

[Sekhar Nori]

On 10/10/2012 8:04 PM, Karicheri, Muralidharan wrote:

>>>> +struct clk *clk_register_davinci_pll(struct device *dev, const char *name,
>>>> +			const char *parent_name,
>>>> +			struct clk_davinci_pll_data *pll_data) {
>>>> +	struct clk_init_data init;
>>>> +	struct clk_davinci_pll *pll;
>>>> +	struct clk *clk;
>>>> +
>>>> +	if (!pll_data)
>>>> +		return ERR_PTR(-ENODEV);
>>>> +
>>>> +	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
>>>> +	if (!pll)
>>>> +		return ERR_PTR(-ENOMEM);
>>>> +	init.name = name;
>>>> +	init.ops = &clk_pll_ops;
>>>> +	init.flags = pll_data->flags;
>>>> +	init.parent_names = (parent_name ? &parent_name : NULL);
>>>> +	init.num_parents = (parent_name ? 1 : 0);
>>>> +
>>>> +	pll->pll_data	= pll_data;
>>>> +	pll->hw.init = &init;
>>>> +
>>>> +	clk = clk_register(NULL, &pll->hw);
>>>> +	if (IS_ERR(clk))
>>>> +		kfree(pll);
>>>> +
>>>> +	return clk;
>>>> +}
>>>
>>> I guess there is an an "unregister" required as well which will free the pll memory
>>> allocated above and unregister the clock? Not sure if you would ever unregister a PLL,
>>> but providing this will probably help symmetry.
> Sekhar,
> 
> clk_unregister() itself is a null statement in clk.c. Besides none of the clk drivers presently have implemented the unregister(). So I believe this is unnecessary. 

I am ok with this.

> BTW, please review the v2 patch for the rest of the series. For the one you have already reviewed, it should be fine.

Okay. I see those now. BTW, this series also has a v2 in its 0/13. Are
there any differences between this and the other v2, or is that merely a
resend?

Thanks,
Sekhar

Re: [PATCH 01/13] clk: davinci - add Main PLL clock driver

[Sekhar Nori]

On 10/10/2012 5:32 PM, Sekhar Nori wrote:
> Hi Murali,
> 
> On 9/26/2012 11:37 PM, Murali Karicheri wrote:
>> This is the driver for the main PLL clock hardware found on DM SoCs.
>> This driver borrowed code from arch/arm/mach-davinci/clock.c and
>> implemented the driver as per common clock provider API. The main PLL
>> hardware typically has a multiplier, a pre-divider and a post-divider.
>> Some of the SoCs has the divider fixed meaning they can not be
>> configured through a register. HAS_PREDIV and HAS_POSTDIV flags are used
>> to tell the driver if a hardware has these dividers present or not.
>> Driver is configured through the structure clk_davinci_pll_data
>> that has the platform data for the driver.
>>
>> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
> 
> Are you using git-format-patch to generate the patches? It should have
> added a diffstat here by default which is very useful in quickly
> understanding what the patch is touching.
>>
>> diff --git a/drivers/clk/davinci/clk-davinci-pll.c b/drivers/clk/davinci/clk-davinci-pll.c

Looking at how common clock framework for mxs has been implemented, this
file should simply be clk-pll.c. That makes sense as you are creating a
davinci folder anyway. Similar change required for psc as well.

Thanks,
Sekhar

RE: [PATCH 01/13] calk: davinci - add Main PLL clock driver

[Karicheri, Muralidharan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2697 bytes --]

>> -----Original Message-----
>> From: Nori, Sekhar
>> Sent: Thursday, October 11, 2012 6:16 AM
>> To: Karicheri, Muralidharan
>> Cc: mturquette@linaro.org; arnd@arndb.de; akpm@linux-foundation.org;
>> shawn.guo@linaro.org; rob.herring@calxeda.com; linus.walleij@linaro.org;
>> viresh.linux@gmail.com; linux-kernel@vger.kernel.org; Hilman, Kevin;
>> linux@arm.linux.org.uk; davinci-linux-open-source@linux.davincidsp.com; linux-arm-
>> kernel@lists.infradead.org; linux-keystone@list.ti.com - Linux developers for Keystone
>> family of devices (May contain non-TIers); linux-c6x-dev@linux-c6x.org; Chemparathy,
>> Cyril
>> Subject: Re: [PATCH 01/13] calk: davinci - add Main PLL clock driver
>> 
>> On 10/10/2012 8:04 PM, Karicheri, Muralidharan wrote:
>> 
>> >>>> +struct clk *clk_register_davinci_pll(struct device *dev, const char *name,
>> >>>> +			const char *parent_name,
>> >>>> +			struct clk_davinci_pll_data *pll_data) {
>> >>>> +	struct clk_init_data init;
>> >>>> +	struct clk_davinci_pll *pll;
>> >>>> +	struct clk *clk;
>> >>>> +
>> >>>> +	if (!pll_data)
>> >>>> +		return ERR_PTR(-ENODEV);
>> >>>> +
>> >>>> +	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
>> >>>> +	if (!pll)
>> >>>> +		return ERR_PTR(-ENOMEM);
>> >>>> +	init.name = name;
>> >>>> +	init.ops = &clk_pll_ops;
>> >>>> +	init.flags = pll_data->flags;
>> >>>> +	init.parent_names = (parent_name ? &parent_name : NULL);
>> >>>> +	init.num_parents = (parent_name ? 1 : 0);
>> >>>> +
>> >>>> +	pll->pll_data	= pll_data;
>> >>>> +	pll->hw.init = &init;
>> >>>> +
>> >>>> +	clk = clk_register(NULL, &pll->hw);
>> >>>> +	if (IS_ERR(clk))
>> >>>> +		kfree(pll);
>> >>>> +
>> >>>> +	return clk;
>> >>>> +}
>> >>>
>> >>> I guess there is an an "unregister" required as well which will free
>> >>> the pll memory allocated above and unregister the clock? Not sure if
>> >>> you would ever unregister a PLL, but providing this will probably help symmetry.
>> > Sekhar,
>> >
>> > clk_unregister() itself is a null statement in clk.c. Besides none of the clk drivers
>> presently have implemented the unregister(). So I believe this is unnecessary.
>> 
>> I am ok with this.
>> 
>> > BTW, please review the v2 patch for the rest of the series. For the one you have
>> already reviewed, it should be fine.
>> 
>> Okay. I see those now. BTW, this series also has a v2 in its 0/13. Are there any
>> differences between this and the other v2, or is that merely a resend?
>> 

You are right. I did a re-send to add v2 in all of the patch subject. We are fine.

>> Thanks,
>> Sekhar
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH 01/13] clk: davinci - add Main PLL clock driver

[Karicheri, Muralidharan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2458 bytes --]

>> -----Original Message-----
>> From: Nori, Sekhar
>> Sent: Thursday, October 11, 2012 6:35 AM
>> To: Nori, Sekhar
>> Cc: Karicheri, Muralidharan; Hilman, Kevin; davinci-linux-open-
>> source@linux.davincidsp.com; mturquette@linaro.org; linux-c6x-dev@linux-c6x.org;
>> arnd@arndb.de; linus.walleij@linaro.org; linux-kernel@vger.kernel.org;
>> rob.herring@calxeda.com; linux-keystone@list.ti.com - Linux developers for Keystone
>> family of devices (May contain non-TIers); viresh.linux@gmail.com;
>> linux@arm.linux.org.uk; akpm@linux-foundation.org; shawn.guo@linaro.org; linux-arm-
>> kernel@lists.infradead.org
>> Subject: Re: [PATCH 01/13] clk: davinci - add Main PLL clock driver
>> 
>> On 10/10/2012 5:32 PM, Sekhar Nori wrote:
>> > Hi Murali,
>> >
>> > On 9/26/2012 11:37 PM, Murali Karicheri wrote:
>> >> This is the driver for the main PLL clock hardware found on DM SoCs.
>> >> This driver borrowed code from arch/arm/mach-davinci/clock.c and
>> >> implemented the driver as per common clock provider API. The main PLL
>> >> hardware typically has a multiplier, a pre-divider and a post-divider.
>> >> Some of the SoCs has the divider fixed meaning they can not be
>> >> configured through a register. HAS_PREDIV and HAS_POSTDIV flags are
>> >> used to tell the driver if a hardware has these dividers present or not.
>> >> Driver is configured through the structure clk_davinci_pll_data that
>> >> has the platform data for the driver.
>> >>
>> >> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com>
>> >
>> > Are you using git-format-patch to generate the patches? It should have
>> > added a diffstat here by default which is very useful in quickly
>> > understanding what the patch is touching.
>> >>
>> >> diff --git a/drivers/clk/davinci/clk-davinci-pll.c
>> >> b/drivers/clk/davinci/clk-davinci-pll.c
>> 
>> Looking at how common clock framework for mxs has been implemented, this file should
>> simply be clk-pll.c. That makes sense as you are creating a davinci folder anyway. Similar
>> change required for psc as well.
>> 

Alternately, do we need  a davinci folder? Can't we just add it to the clk/ directory? These IPs are re-used in c6x and keystone architectures. So it make sense to keep in the clk folder. If agree, I can make this change in v3.

>> Thanks,
>> Sekhar
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥