On Fri, Dec 11, 2009 at 19:35 +0900, Marc Zyngier wrote: > -----Original Message----- > From: linux-samsung-soc-owner@xxxxxxxxxxxxxxx [mailto:linux-samsung-soc- > owner@xxxxxxxxxxxxxxx] On Behalf Of Marc Zyngier > Sent: Friday, December 11, 2009 7:35 PM > To: Kukjin Kim > Cc: linux-samsung-soc@xxxxxxxxxxxxxxx; ben-linux@xxxxxxxxx; 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 <kgene.kim@xxxxxxxxxxx> 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 <kgene.kim@xxxxxxxxxxx>, 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |