On Fri, Dec 11, 2009 at 19:35 +0900, Marc Zyngier wrote:
> -----Original Message-----
> From: [email protected] [mailto:linux-samsung-soc-
> [email protected]] On Behalf Of Marc Zyngier
> Sent: Friday, December 11, 2009 7:35 PM
> To: Kukjin Kim
> Cc: [email protected]; [email protected]; Kukjin Kim;
> Adityapratap Sharma; Thomas Abraham; Atul Dahiya
> Subject: Re: [PATCH 3/7] ARM: S5P6440: Clock and PLL support
>
> On Fri, 11 Dec 2009 18:54:31 +0900
> Kukjin Kim <[email protected]> wrote:
>
> A few problems spotted here:
Hi Marc Zyngier,
Ok.
> > +static struct clk init_clocks_disable[] = {
> > + {
> > + .name = "nand",
> > + .id = -1,
> > + .parent = &clk_h,
> > + .enable = s5p6440_mem_ctrl,
> > + .ctrlbit = S5P_CLKCON_MEM0_HCLK_NFCON,
> > + }, {
> > + .name = "adc",
> > + .id = -1,
> > + .parent = &clk_p,
> > + .enable = s5p6440_pclk_ctrl,
> > + .ctrlbit = S5P_CLKCON_PCLK_TSADC,
> > + }, {
> > + .name = "i2c",
> > + .id = -1,
> > + .parent = &clk_p,
> > + .enable = s5p6440_pclk_ctrl,
> > + .ctrlbit = S5P_CLKCON_PCLK_IIC0,
> > + }, {
> > + .name = "i2s_v40",
> > + .id = 0,
> > + .parent = &clk_p,
> > + .enable = s5p6440_pclk_ctrl,
> > + .ctrlbit = S5P_CLKCON_PCLK_IIS2,
> > + }, {
> > + .name = "spi",
> > + .id = 0,
> > + .parent = &clk_p,
> > + .enable = s5p6440_pclk_ctrl,
> > + .ctrlbit = S5P_CLKCON_PCLK_SPI0,
> > + }, {
> > + .name = "spi",
> > + .id = 1,
> > + .parent = &clk_p,
> > + .enable = s5p6440_pclk_ctrl,
> > + .ctrlbit = S5P_CLKCON_PCLK_SPI1,
> > + }, {
> > + .name = "sclk_spi_48",
> > + .id = 0,
> > + .parent = &clk_48m,
> > + .enable = s5p6440_sclk_ctrl,
> > + .ctrlbit = S5P_CLKCON_SCLK0_SPI0_48,
> > + }, {
> > + .name = "sclk_spi_48",
> > + .id = 1,
> > + .parent = &clk_48m,
> > + .enable = s5p6440_sclk_ctrl,
> > + .ctrlbit = S5P_CLKCON_SCLK0_SPI1_48,
> > + }, {
> > + .name = "48m",
> > + .id = 0,
> > + .parent = &clk_48m,
> > + .enable = s5p6440_sclk_ctrl,
> > + .ctrlbit = S5P_CLKCON_SCLK0_MMC0_48,
> > + }, {
> > + .name = "48m",
> > + .id = 1,
> > + .parent = &clk_48m,
> > + .enable = s5p6440_sclk_ctrl,
> > + .ctrlbit = S5P_CLKCON_SCLK0_MMC1_48,
> > + }, {
> > + .name = "48m",
> > + .id = 2,
> > + .parent = &clk_48m,
> > + .enable = s5p6440_sclk_ctrl,
> > + .ctrlbit = S5P_CLKCON_SCLK0_MMC2_48,
> > + }, {
> > + .name = "otg",
> > + .id = -1,
> > + .parent = &clk_h,
> > + .enable = s5p6440_hclk0_ctrl,
> > + .ctrlbit = S5P_CLKCON_HCLK0_USB
> > + }, {
> > + .name = "post",
> > + .id = -1,
> > + .parent = &clk_h,
> > + .enable = s5p6440_hclk0_ctrl,
> > + .ctrlbit = S5P_CLKCON_HCLK0_POST0
> > + }, {
> > + .name = "lcd",
> > + .id = -1,
> > + .parent = &clk_h,
> > + .enable = s5p6440_hclk1_ctrl,
> > + .ctrlbit = S5P_CLKCON_HCLK1_DISPCON,
> > + }, {
> > + .name = "hsmmc",
> > + .id = 0,
> > + .parent = &clk_h,
> > + .enable = s5p6440_hclk0_ctrl,
> > + .ctrlbit = S5P_CLKCON_HCLK0_HSMMC0,
> > + }, {
> > + .name = "hsmmc",
> > + .id = 1,
> > + .parent = &clk_h,
> > + .enable = s5p6440_hclk0_ctrl,
> > + .ctrlbit = S5P_CLKCON_HCLK0_HSMMC1,
> > + }, {
> > + .name = "hsmmc",
> > + .id = 2,
> > + .parent = &clk_h,
> > + .enable = s5p6440_hclk0_ctrl,
> > + .ctrlbit = S5P_CLKCON_HCLK0_HSMMC2,
> > + }, {
> > + .name = "rtc",
> > + .id = -1,
> > + .parent = &clk_p,
> > + .enable = s5p6440_pclk_ctrl,
> > + .ctrlbit = S5P_CLKCON_PCLK_RTC,
> > + }, {
> > + .name = "watchdog",
> > + .id = -1,
> > + .parent = &clk_p,
> > + .enable = s5p6440_pclk_ctrl,
> > + .ctrlbit = S5P_CLKCON_PCLK_WDT,
> > + }
> > +};
> >
> > +/*
> > + * The following clocks will be enabled during clock initialization.
> > + */
> > +static struct clk init_clocks[] = {
> > + {
> > + .name = "gpio",
> > + .id = -1,
> > + .parent = &clk_p,
> > + .enable = s5p6440_pclk_ctrl,
> > + .ctrlbit = S5P_CLKCON_PCLK_GPIO,
> > + }, {
> > + .name = "timers",
> > + .id = -1,
> > + .parent = &clk_p,
> > + .enable = s5p6440_pclk_ctrl,
> > + .ctrlbit = S5P_CLKCON_PCLK_PWM,
> > + }, {
> > + .name = "uart",
> > + .id = 0,
> > + .parent = &clk_p,
> > + .enable = s5p6440_pclk_ctrl,
> > + .ctrlbit = S5P_CLKCON_PCLK_UART0,
> > + }, {
> > + .name = "uart",
> > + .id = 1,
> > + .parent = &clk_p,
> > + .enable = s5p6440_pclk_ctrl,
> > + .ctrlbit = S5P_CLKCON_PCLK_UART1,
> > + }, {
> > + .name = "uart",
> > + .id = 2,
> > + .parent = &clk_p,
> > + .enable = s5p6440_pclk_ctrl,
> > + .ctrlbit = S5P_CLKCON_PCLK_UART2,
> > + }, {
> > + .name = "uart",
> > + .id = 3,
> > + .parent = &clk_p,
> > + .enable = s5p6440_pclk_ctrl,
> > + .ctrlbit = S5P_CLKCON_PCLK_UART3,
> > + }
> > +};
> The parent for most of these clocks are wrong. Looking at the user
> manual, they are connected to the low frequency domain (PCLK_LOW or
> HCLK_LOW). The parent must then be set to clk_p_low or clk_h_low.
Yes, you're right. I had thought that PCLK and HCLK were PCLK_LOW and
HCLK_LOW, respectively. However, it differs with user manual. I will change
it.
> Failure to do so leads to some "interesting" situations when HCLK and
> HCLK_LOW have different rates...
Yes, it works when HCLK and HCLK_LOW are same. But if they are different, it
doesn't work. I will change it.
> > +void __init_or_cpufreq s5p6440_setup_clocks(void)
> > +{
> > + struct clk *xtal_clk;
> > + unsigned long xtal;
> > + unsigned long fclk;
> > + unsigned long hclk;
> > + unsigned long hclk_low;
> > + unsigned long pclk;
> > + unsigned long pclk_low;
> > + unsigned long epll;
> > + unsigned long apll;
> > + unsigned long mpll;
> > + unsigned int ptr;
> > + u32 clkdiv0;
> > + u32 clkdiv3;
> > +
> > + /* Set S5P6440 functions for clk_fout_epll */
> > + clk_fout_epll.enable = s5p6440_epll_enable;
> > + clk_fout_epll.ops = &s5p6440_epll_ops;
> > +
> > + /* Set S5P6440 functions for arm clock */
> > + clk_arm.parent = &clk_mout_apll.clk;
> > + clk_arm.ops = &s5p6440_clkarm_ops;
> > +
> > + clkdiv0 = __raw_readl(S5P_CLK_DIV0);
> > + clkdiv3 = __raw_readl(S5P_CLK_DIV3);
> > +
> > + xtal_clk = clk_get(NULL, "ext_xtal");
> > + BUG_ON(IS_ERR(xtal_clk));
> > +
> > + xtal = clk_get_rate(xtal_clk);
> > + clk_put(xtal_clk);
> > +
> > + epll = s5p6440_get_epll(xtal);
> > + mpll = s5p_get_pll(xtal, __raw_readl(S5P_MPLL_CON));
> > + apll = s5p_get_pll(xtal, __raw_readl(S5P_APLL_CON));
> > +
> > + printk(KERN_INFO "S5P6440: PLL settings, A=%ld.%ldMHz,
> M=%ld.%ldMHz," \
> > + " E=%ld.%ldMHz\n",
> > + print_mhz(apll), print_mhz(mpll), print_mhz(epll));
> > +
> > + fclk = apll / GET_DIV(clkdiv0, S5P_CLKDIV0_ARM);
> > + hclk = fclk / GET_DIV(clkdiv0, S5P_CLKDIV0_HCLK);
> > + pclk = hclk / GET_DIV(clkdiv0, S5P_CLKDIV0_PCLK);
> > +
> > + if (__raw_readl(S5P_OTHERS) & S5P_OTHERS_HCLK_LOW_SEL_MPLL) {
> > + /* Synchronous mode */
> > + hclk_low = apll / GET_DIV(clkdiv3, S5P_CLKDIV3_HCLK_LOW);
> > + } else {
> > + /* Asynchronous mode */
> > + hclk_low = mpll / GET_DIV(clkdiv3, S5P_CLKDIV3_HCLK_LOW);
> > + }
>
> The test is inverted. S5P_OTHERS::S5P_OTHERS_HCLK_LOW_SEL_MPLL is set
> when MPLL is the source clock for HCLK_LOW.
Yes, you're right. They inverted. I will change them as follows :
+ if (__raw_readl(S5P_OTHERS) & S5P_OTHERS_HCLK_LOW_SEL_MPLL) {
+ hclk_low = mpll / GET_DIV(clkdiv3, S5P_CLKDIV3_HCLK_LOW);
+ } else {
+ hclk_low = apll / GET_DIV(clkdiv3, S5P_CLKDIV3_HCLK_LOW);
+ }
> > + pclk_low = hclk_low / GET_DIV(clkdiv3, S5P_CLKDIV3_PCLK_LOW);
> > +
> > + printk(KERN_INFO "S5P6440: HCLK=%ld.%ldMHz,
> HCLK_LOW=%ld.%ldMHz," \
> > + " PCLK=%ld.%ldMHz, PCLK_LOW=%ld.%ldMHz\n",
> > + print_mhz(hclk), print_mhz(hclk_low),
> > + print_mhz(pclk), print_mhz(pclk_low));
> > +
> > + clk_fout_mpll.rate = mpll;
> > + clk_fout_epll.rate = epll;
> > + clk_fout_apll.rate = apll;
> > +
> > + clk_f.rate = fclk;
> > + clk_h.rate = hclk;
> > + clk_p.rate = pclk;
> > + clk_h_low.rate = hclk_low;
> > + clk_p_low.rate = pclk_low;
> > +
> > + for (ptr = 0; ptr < ARRAY_SIZE(init_parents); ptr++)
> > + s3c_set_clksrc(init_parents[ptr]);
> > +
> > + for (ptr = 0; ptr < ARRAY_SIZE(clksrcs); ptr++)
> > + s3c_set_clksrc(&clksrcs[ptr]);
> > +}
>
> --
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <[email protected]>, Senior Engineer,
System LSI Division, SAMSUNG ELECTRONICS CO., LTD.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
