2010/7/22 Nori, Sekhar <nsek...@ti.com> > Hi Raffaele, > > On Wed, Jul 21, 2010 at 16:21:49, Raffaele Recalcati wrote: > > From: Davide Bonfanti <davide.bonfa...@bticino.it> > > > > Clockout2 is added as a child of pll1_sysclk9, because they have > > the same pll divisor. > > Added dm365_clkout2_set_rate to properly set clockout2 frequency. > > > Modified the davinci_set_sysclk_rate function in order > > to get the right ancestor. > > This change should be carved into a separate patch since > it is not directly related to adding clockout2 support. >
ok, it will be done > > In the new patch please describe how the existing code isn't > getting the right ancestor. > now it is not more needed. there was a mistake in the call. > > Also, that patch should note below the '---' that it depends > on this patch submitted to the mailing list: > > https://patchwork.kernel.org/patch/112994/ > > This helps maintainer derive the correct order in which patches > need to be applied. > > > > > This patch has been developed against the > > > http://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-davinci.git > > As, mentioned before, this is implied when submitting to > davinci-linux-open-source@linux.davincidsp.com and so can > be removed. If you want to note it, please note below the > '---' in the patch so it wont make it to the commit log. > ok,thx > > git tree and tested on bmx board. > > > > Signed-off-by: Davide Bonfanti <davide.bonfa...@bticino.it> > > Signed-off-by: Raffaele Recalcati <raffaele.recalc...@bticino.it> > > --- > > arch/arm/mach-davinci/clock.c | 32 ++++++++++++---- > > arch/arm/mach-davinci/clock.h | 5 ++ > > arch/arm/mach-davinci/dm365.c | 57 > ++++++++++++++++++++++++++++ > > arch/arm/mach-davinci/include/mach/dm365.h | 1 + > > 4 files changed, 87 insertions(+), 8 deletions(-) > > > > diff --git a/arch/arm/mach-davinci/clock.c > b/arch/arm/mach-davinci/clock.c > > index f29a526..6e45808 100644 > > --- a/arch/arm/mach-davinci/clock.c > > +++ b/arch/arm/mach-davinci/clock.c > > @@ -254,7 +254,15 @@ static unsigned long clk_sysclk_recalc(struct clk > *clk) > > u32 v, plldiv; > > struct pll_data *pll; > > unsigned long rate = clk->rate; > > + struct clk *parent = clk; > > > > + if (clk == NULL || IS_ERR(clk)) > > + return -EINVAL; > > + while (parent->parent->parent) > > + parent = parent->parent; > > + > > + if (parent == clk) > > + return -EPERM; > > It is not clear to me why this change in needed. It is not > described in the patch description as well. Most likely this > needs to be carved into a separate patch as well describing > what is wrong with the existing clk_sysclk_recalc() routine. > now, whith the last check, we don't need that modifications, but only /* Otherwise, the parent must be a PLL */ - if (WARN_ON(!parent->pll_data)) + if (!clk->parent->pll_data) clkout2 is a sub-divider and so its parent is not a pll. > > [...] > > > @@ -293,26 +301,33 @@ int davinci_set_sysclk_rate(struct clk *clk, > unsigned long rate) > > struct pll_data *pll; > > unsigned long input; > > unsigned ratio = 0; > > + struct clk *parent = clk; > > + > > + /* searching the right ancestor (pll1_clk or pll2_clk) */ > > + while (parent->parent->parent) > > + parent = parent->parent; > > + if (parent == clk) > > + return -EPERM; > > As noted above, please carve into separate patch. > ok > > [...] > > > diff --git a/arch/arm/mach-davinci/clock.h > b/arch/arm/mach-davinci/clock.h > > index a717d98..df36d73 100644 > > --- a/arch/arm/mach-davinci/clock.h > > +++ b/arch/arm/mach-davinci/clock.h > > @@ -50,6 +50,11 @@ > > #define PLLDIV_EN BIT(15) > > #define PLLDIV_RATIO_MASK 0x1f > > > > +#define PERI_CLKCTL 0x48 > > +#define CLOCKOUT2EN 2 > > +#define CLOCKOUT1EN 1 > > +#define CLOCKOUT0EN 0 > > + > > /* > > * OMAP-L138 system reference guide recommends a wait for 4 OSCIN/CLKIN > > * cycles to ensure that the PLLC has switched to bypass mode. Delay of > 1us > > diff --git a/arch/arm/mach-davinci/dm365.c > b/arch/arm/mach-davinci/dm365.c > > index 42fd4a4..902e9a0 100644 > > --- a/arch/arm/mach-davinci/dm365.c > > +++ b/arch/arm/mach-davinci/dm365.c > > @@ -40,6 +40,11 @@ > > #include "mux.h" > > > > #define DM365_REF_FREQ 24000000 /* 24 MHz on the > DM365 EVM */ > > +#define PINMUX0 0x00 > > +#define PINMUX1 0x04 > > +#define PINMUX2 0x08 > > +#define PINMUX3 0x0c > > +#define PINMUX4 0x10 > > Why are PINMUX defines added here? You don't seem to use > these elsewhere in the patch. > deleting ... > > > > > static struct pll_data pll1_data = { > > .num = 1, > > @@ -124,6 +129,7 @@ static struct clk pll1_sysclk6 = { > > .parent = &pll1_clk, > > .flags = CLK_PLL, > > .div_reg = PLLDIV6, > > + .set_rate = davinci_set_sysclk_rate, > > }; > > > > static struct clk pll1_sysclk7 = { > > @@ -145,6 +151,14 @@ static struct clk pll1_sysclk9 = { > > .parent = &pll1_clk, > > .flags = CLK_PLL, > > .div_reg = PLLDIV9, > > + .set_rate = davinci_set_sysclk_rate, > > +}; > > + > > +static struct clk clkout2_clk = { > > + .name = "clkout2", > > + .parent = &pll1_sysclk9, > > + .flags = CLK_PLL, > > + .set_rate = dm365_clkout2_set_rate, > > }; > > > > static struct clk pll2_clk = { > > @@ -421,6 +435,7 @@ static struct clk_lookup dm365_clks[] = { > > CLK(NULL, "pll1_sysclk7", &pll1_sysclk7), > > CLK(NULL, "pll1_sysclk8", &pll1_sysclk8), > > CLK(NULL, "pll1_sysclk9", &pll1_sysclk9), > > + CLK(NULL, "clkout2", &clkout2_clk), > > CLK(NULL, "pll2", &pll2_clk), > > CLK(NULL, "pll2_aux", &pll2_aux_clk), > > CLK(NULL, "clkout1", &clkout1_clk), > > @@ -657,6 +672,48 @@ static struct resource dm365_spi0_resources[] = { > > }, > > }; > > > > +int dm365_clkout2_set_rate(unsigned long rate) > > Is clockout2 specific to DM365? DM355/DM6446 manuals mention > clkout signal as well. If this routine can cater to more SoCs > with simple modifications, you can attempt to generalize it. > we check in dm355 and clkout2 is really a different clock. it seems difficult to integrate. we'd prefer not to do it. > > > +{ > > + int ret = -EINVAL; > > + int i, err, min_err, i_min_err; > > + u32 regval; > > + struct clk *clk; > > + static void __iomem *system_module_base; > > + > > + clk = &clkout2_clk; > > + system_module_base = ioremap(DAVINCI_SYSTEM_MODULE_BASE, SZ_4K); > > + regval = __raw_readl(system_module_base + PERI_CLKCTL); > > This part of the code would make it specific to DM365. May be > the div_reg present in clock structure can be used to pass this > register address from platform file? It will then be a matter of > seeing whether the register bit definitions line up across > platforms. > > You don't have to necessarily test on all platforms as long as > the code is written generically enough. > yes, but again we have not so a deep davinci knowledge to write code without testing it. > > > + > > + /* check all possibilities to get best fitting for the required > freq */ > > + i_min_err = min_err = INT_MAX; > > + for (i = 0x0F; i > 0; i--) { > > + if (clk->parent->set_rate) { > > + ret = clk_set_rate(clk->parent, rate * i) ; > > + err = clk_get_rate(clk->parent) - rate * i; > > + if (min_err > abs(err)) { > > + min_err = abs(err); > > + i_min_err = i; > > + } > > + } > > + } > > Why should the child touch the parent's clock output? Users of the > clock framework should be able to set these rates independently. > right. we tried. the problem is that the clkout2 is used for uda1345 system clock. without chenig the parent we can't get close. the sound is really too fast. > > Can you please check if there is a need to do this even with the > latest patch I posted? In that patch, if the 'maxrate' the sysclk > can support is known, the sysclk rate set code using DIV_ROUND_CLOSEST() > which should give the least error already. > yes. in our application we need to change pll1_sysclk9. we can't find another possibility. an idea should be to "occupy", if possible, the pll1_sysclk9 only for our goal, so other drivers can't discover a wrong clock after our call. > > Thanks, > Sekhar > > Tomorrow, if you agree, I'll send you 2 patches: -patch1: clkout2 -patch2: removing warn from sysclk recalc Please let me know how to proceed. Raffaele
_______________________________________________ Davinci-linux-open-source mailing list Davinci-linux-open-source@linux.davincidsp.com http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
