Hi Raffaele, On Wed, Jul 21, 2010 at 16:21:49, Raffaele Recalcati wrote: > From: Davide Bonfanti <[email protected]> > > 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. In the new patch please describe how the existing code isn't getting the right ancestor. 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 [email protected] 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. > git tree and tested on bmx board. > > Signed-off-by: Davide Bonfanti <[email protected]> > Signed-off-by: Raffaele Recalcati <[email protected]> > --- > 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. [...] > @@ -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. [...] > 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. > > 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. > +{ > + 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. > + > + /* 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. 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. Thanks, Sekhar _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
