| Message ID | 1516468460-4908-21-git-send-email-david@lechnology.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/20/2018 11:13 AM, David Lechner wrote: > This adds the new board-specific clock init in mach-davinci/da830.c > using the new common clock framework drivers. > > The #ifdefs are needed to prevent compile errors until the entire > ARCH_DAVINCI is converted. > > Also clean up the #includes since we are adding some here. > > Signed-off-by: David Lechner <david@lechnology.com> > --- > > v6 changes: > - add blank lines between function calls > - include da8xx_register_cfgchip() > > arch/arm/mach-davinci/da830.c | 49 +++++++++++++++++++++++++++++++++++++------ > 1 file changed, 43 insertions(+), 6 deletions(-) > > diff --git a/arch/arm/mach-davinci/da830.c b/arch/arm/mach-davinci/da830.c ... > + clk = clk_register_fixed_factor(NULL, "i2c0", "pll0_aux_clk", 0, 1, 1); Should be "pll0_auxclk" instead of "pll0_aux_clk" here and 2 more below. > + clk_register_clkdev(clk, NULL, "i2c_davinci.1"); > + > + clk = clk_register_fixed_factor(NULL, "timer0", "pll0_aux_clk", 0, 1, 1); > + clk_register_clkdev(clk, "timer0", NULL); > + > + clk = clk_register_fixed_factor(NULL, "timer1", "pll0_aux_clk", 0, 1, 1); > + clk_register_clkdev(clk, NULL, "davinci-wdt"); > + > + clk = clk_register_fixed_factor(NULL, "rmii", "pll0_sysclk7", 0, 1, 1); > + clk_register_clkdev(clk, "rmii", NULL); > +#else > da8xx_register_cfgchip(); > davinci_clk_init(da830_clks); > +#endif > davinci_timer_init(); > } >
On Saturday 20 January 2018 10:43 PM, David Lechner wrote: > void __init da830_init_time(void) > { > +#ifdef CONFIG_COMMON_CLK > + void __iomem *pll0, *psc0, *psc1; > + struct clk *clk; > + > + pll0 = ioremap(DA8XX_PLL0_BASE, SZ_4K); > + psc0 = ioremap(DA8XX_PSC0_BASE, SZ_4K); > + psc1 = ioremap(DA8XX_PSC1_BASE, SZ_4K); > + > + da8xx_register_cfgchip(); > + > + clk_register_fixed_rate(NULL, "ref_clk", NULL, 0, DA830_REF_FREQ); > + > + da830_pll_clk_init(pll0); > + > + da830_psc_clk_init(psc0, psc1); > + > + clk = clk_register_fixed_factor(NULL, "i2c0", "pll0_aux_clk", 0, 1, 1); > + clk_register_clkdev(clk, NULL, "i2c_davinci.1"); > + > + clk = clk_register_fixed_factor(NULL, "timer0", "pll0_aux_clk", 0, 1, 1); > + clk_register_clkdev(clk, "timer0", NULL); > + > + clk = clk_register_fixed_factor(NULL, "timer1", "pll0_aux_clk", 0, 1, 1); > + clk_register_clkdev(clk, NULL, "davinci-wdt"); Isn't this better done in da830_pll_clk_init() ? I think we can get rid of the dummy fixed factor clock too and directly use the pll0_auxclk. That reminds me, is "pll0_aux_clk" above correct, or should it be "pll0_auxclk" like in da830_pll_clk_init()? > + > + clk = clk_register_fixed_factor(NULL, "rmii", "pll0_sysclk7", 0, 1, 1); > + clk_register_clkdev(clk, "rmii", NULL); I don't see any driver looking for this clock using con_id "rmii". I know this came from existing code. But its most likely a vestige and can be dropped. Thanks, Sekhar
On 02/02/2018 08:12 AM, Sekhar Nori wrote: > On Saturday 20 January 2018 10:43 PM, David Lechner wrote: >> void __init da830_init_time(void) >> { >> +#ifdef CONFIG_COMMON_CLK >> + void __iomem *pll0, *psc0, *psc1; >> + struct clk *clk; >> + >> + pll0 = ioremap(DA8XX_PLL0_BASE, SZ_4K); >> + psc0 = ioremap(DA8XX_PSC0_BASE, SZ_4K); >> + psc1 = ioremap(DA8XX_PSC1_BASE, SZ_4K); >> + >> + da8xx_register_cfgchip(); >> + >> + clk_register_fixed_rate(NULL, "ref_clk", NULL, 0, DA830_REF_FREQ); >> + >> + da830_pll_clk_init(pll0); >> + >> + da830_psc_clk_init(psc0, psc1); >> + > >> + clk = clk_register_fixed_factor(NULL, "i2c0", "pll0_aux_clk", 0, 1, 1); >> + clk_register_clkdev(clk, NULL, "i2c_davinci.1"); >> + >> + clk = clk_register_fixed_factor(NULL, "timer0", "pll0_aux_clk", 0, 1, 1); >> + clk_register_clkdev(clk, "timer0", NULL); >> + >> + clk = clk_register_fixed_factor(NULL, "timer1", "pll0_aux_clk", 0, 1, 1); >> + clk_register_clkdev(clk, NULL, "davinci-wdt"); > > Isn't this better done in da830_pll_clk_init() ? I think we can get rid > of the dummy fixed factor clock too and directly use the pll0_auxclk. I considered it, but I kind of like keeping the fixed factor clocks for debugging purposes. If you just have "pll0_auxclk" the enable count is not helpful because you don't know which driver did the enabling. On the other hand, once things are working, you don't really need to do any debugging. > That reminds me, is "pll0_aux_clk" above correct, or should it be > "pll0_auxclk" like in da830_pll_clk_init()? Yes, it should be "pll0_auxclk". > >> + >> + clk = clk_register_fixed_factor(NULL, "rmii", "pll0_sysclk7", 0, 1, 1); >> + clk_register_clkdev(clk, "rmii", NULL); > > I don't see any driver looking for this clock using con_id "rmii". I > know this came from existing code. But its most likely a vestige and can > be dropped. > > Thanks, > Sekhar >
On Friday 02 February 2018 11:33 PM, David Lechner wrote: > On 02/02/2018 08:12 AM, Sekhar Nori wrote: >> On Saturday 20 January 2018 10:43 PM, David Lechner wrote: >>> void __init da830_init_time(void) >>> { >>> +#ifdef CONFIG_COMMON_CLK >>> + void __iomem *pll0, *psc0, *psc1; >>> + struct clk *clk; >>> + >>> + pll0 = ioremap(DA8XX_PLL0_BASE, SZ_4K); >>> + psc0 = ioremap(DA8XX_PSC0_BASE, SZ_4K); >>> + psc1 = ioremap(DA8XX_PSC1_BASE, SZ_4K); >>> + >>> + da8xx_register_cfgchip(); >>> + >>> + clk_register_fixed_rate(NULL, "ref_clk", NULL, 0, DA830_REF_FREQ); >>> + >>> + da830_pll_clk_init(pll0); >>> + >>> + da830_psc_clk_init(psc0, psc1); >>> + >> >>> + clk = clk_register_fixed_factor(NULL, "i2c0", "pll0_aux_clk", 0, >>> 1, 1); >>> + clk_register_clkdev(clk, NULL, "i2c_davinci.1"); >>> + >>> + clk = clk_register_fixed_factor(NULL, "timer0", "pll0_aux_clk", >>> 0, 1, 1); >>> + clk_register_clkdev(clk, "timer0", NULL); >>> + >>> + clk = clk_register_fixed_factor(NULL, "timer1", "pll0_aux_clk", >>> 0, 1, 1); >>> + clk_register_clkdev(clk, NULL, "davinci-wdt"); >> >> Isn't this better done in da830_pll_clk_init() ? I think we can get rid >> of the dummy fixed factor clock too and directly use the pll0_auxclk. > > > I considered it, but I kind of like keeping the fixed factor clocks for > debugging purposes. If you just have "pll0_auxclk" the enable count is > not helpful because you don't know which driver did the enabling. I think it is better to more or less reflect the hardware here. We would not be doing this in the DT case, for example. I see your point on debugging. Such code can perhaps be temporarily introduced if really debugging such an issue. This will be the case with any shared clock. Thanks, Sekhar
diff --git a/arch/arm/mach-davinci/da830.c b/arch/arm/mach-davinci/da830.c index 39de5a6..0b6d0f3 100644 --- a/arch/arm/mach-davinci/da830.c +++ b/arch/arm/mach-davinci/da830.c @@ -8,23 +8,29 @@ * is licensed "as is" without any warranty of any kind, whether express * or implied. */ +#include <linux/clk-provider.h> +#include <linux/clk.h> +#include <linux/clk/davinci.h> +#include <linux/clkdev.h> #include <linux/gpio.h> #include <linux/init.h> -#include <linux/clk.h> #include <linux/platform_data/gpio-davinci.h> #include <asm/mach/map.h> -#include "psc.h" -#include <mach/irqs.h> -#include <mach/cputype.h> #include <mach/common.h> -#include <mach/time.h> +#include <mach/cputype.h> #include <mach/da8xx.h> +#include <mach/irqs.h> +#include <mach/time.h> -#include "clock.h" #include "mux.h" +#ifndef CONFIG_COMMON_CLK +#include "clock.h" +#include "psc.h" +#endif + /* Offsets of the 8 compare registers on the da830 */ #define DA830_CMP12_0 0x60 #define DA830_CMP12_1 0x64 @@ -37,6 +43,7 @@ #define DA830_REF_FREQ 24000000 +#ifndef CONFIG_COMMON_CLK static struct pll_data pll0_data = { .num = 1, .phys_base = DA8XX_PLL0_BASE, @@ -432,6 +439,7 @@ static struct clk_lookup da830_clks[] = { CLK(NULL, "rmii", &rmii_clk), CLK(NULL, NULL, NULL), }; +#endif /* * Device specific mux setup @@ -1223,7 +1231,36 @@ void __init da830_init(void) void __init da830_init_time(void) { +#ifdef CONFIG_COMMON_CLK + void __iomem *pll0, *psc0, *psc1; + struct clk *clk; + + pll0 = ioremap(DA8XX_PLL0_BASE, SZ_4K); + psc0 = ioremap(DA8XX_PSC0_BASE, SZ_4K); + psc1 = ioremap(DA8XX_PSC1_BASE, SZ_4K); + + da8xx_register_cfgchip(); + + clk_register_fixed_rate(NULL, "ref_clk", NULL, 0, DA830_REF_FREQ); + + da830_pll_clk_init(pll0); + + da830_psc_clk_init(psc0, psc1); + + clk = clk_register_fixed_factor(NULL, "i2c0", "pll0_aux_clk", 0, 1, 1); + clk_register_clkdev(clk, NULL, "i2c_davinci.1"); + + clk = clk_register_fixed_factor(NULL, "timer0", "pll0_aux_clk", 0, 1, 1); + clk_register_clkdev(clk, "timer0", NULL); + + clk = clk_register_fixed_factor(NULL, "timer1", "pll0_aux_clk", 0, 1, 1); + clk_register_clkdev(clk, NULL, "davinci-wdt"); + + clk = clk_register_fixed_factor(NULL, "rmii", "pll0_sysclk7", 0, 1, 1); + clk_register_clkdev(clk, "rmii", NULL); +#else da8xx_register_cfgchip(); davinci_clk_init(da830_clks); +#endif davinci_timer_init(); }
This adds the new board-specific clock init in mach-davinci/da830.c using the new common clock framework drivers. The #ifdefs are needed to prevent compile errors until the entire ARCH_DAVINCI is converted. Also clean up the #includes since we are adding some here. Signed-off-by: David Lechner <david@lechnology.com> --- v6 changes: - add blank lines between function calls - include da8xx_register_cfgchip() arch/arm/mach-davinci/da830.c | 49 +++++++++++++++++++++++++++++++++++++------ 1 file changed, 43 insertions(+), 6 deletions(-)