| Message ID | 1348682889-9509-2-git-send-email-m-karicheri2@ti.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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
>> -----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
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
>> -----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
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
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
>> -----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
>> -----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
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> + +#include <mach/cputype.h> + +#define PLLM 0x110 +#define PLLM_PLLM_MASK 0xff +#define PREDIV 0x114 +#define POSTDIV 0x128 +#define PLLDIV_EN BIT(15) + +/** + * struct clk_davinci_pll - DaVinci Main pll clock + * @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; + unsigned long rate = parent_rate; + + /* If there is a device specific recalc defined invoke it. Otherwise + * fallback to default one + */ + mult = __raw_readl(pll_data->pllm); + 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; + 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; +} + +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; +} diff --git a/include/linux/platform_data/clk-davinci-pll.h b/include/linux/platform_data/clk-davinci-pll.h new file mode 100644 index 0000000..4c2920b --- /dev/null +++ b/include/linux/platform_data/clk-davinci-pll.h @@ -0,0 +1,54 @@ +/* + * TI DaVinci clk-pll driver platform data definitions + * + * 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 version 2 as + * published by the Free Software Foundation. + * + * This program is distributed "as is" WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#ifndef __CLK_DAVINCI_PLL_H +#define __CLK_DAVINCI_PLL_H + +/* PLL flags */ +#define CLK_DAVINCI_PLL_HAS_PREDIV BIT(0) +#define CLK_DAVINCI_PLL_HAS_POSTDIV BIT(1) + +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; +}; + +extern struct clk *clk_register_davinci_pll(struct device *dev, + const char *name, const char *parent_name, + struct clk_davinci_pll_data *pll_data); +#endif /* CLK_DAVINCI_PLL_H */
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>