Hi Stefano, On Tue, Oct 20, 2015 at 03:39:32PM +0200, Stefano Babic wrote: >Hi Peng, > >On 20/10/2015 13:39, Peng Fan wrote: >> Implement mxs_set_lcdclk, enable_lcdif_clock and enable_pll_video. >> The three API can be used to configure lcdif related clock when >> CONFIG_VIDEO_MXS enabled. >> >> Signed-off-by: Peng Fan <peng....@freescale.com> >> Cc: Stefano Babic <sba...@denx.de> >> --- >> >> V2: >> none >> >> arch/arm/cpu/armv7/mx6/clock.c | 239 >> ++++++++++++++++++++++++++++++++++ >> arch/arm/include/asm/arch-mx6/clock.h | 2 + >> 2 files changed, 241 insertions(+) >> >> diff --git a/arch/arm/cpu/armv7/mx6/clock.c b/arch/arm/cpu/armv7/mx6/clock.c >> index 11efd12..8a88378 100644 >> --- a/arch/arm/cpu/armv7/mx6/clock.c >> +++ b/arch/arm/cpu/armv7/mx6/clock.c >> @@ -473,6 +473,245 @@ static u32 get_mmdc_ch0_clk(void) >> } >> } >> >> +#if defined(CONFIG_VIDEO_MXS) >> +static int enable_pll_video(u32 pll_div, u32 pll_num, u32 pll_denom, >> + u32 post_div) >> +{ >> + u32 reg = 0; >> + ulong start; >> + >> + debug("pll5 div = %d, num = %d, denom = %d\n", >> + pll_div, pll_num, pll_denom); >> + >> + /* Power up PLL5 video */ >> + writel(BM_ANADIG_PLL_VIDEO_POWERDOWN | >> + BM_ANADIG_PLL_VIDEO_BYPASS | >> + BM_ANADIG_PLL_VIDEO_DIV_SELECT | >> + BM_ANADIG_PLL_VIDEO_POST_DIV_SELECT, >> + &imx_ccm->analog_pll_video_clr); >> + >> + /* Set div, num and denom */ >> + switch (post_div) { >> + case 1: >> + writel(BF_ANADIG_PLL_VIDEO_DIV_SELECT(pll_div) | >> + BF_ANADIG_PLL_VIDEO_POST_DIV_SELECT(0x2), >> + &imx_ccm->analog_pll_video_set); >> + break; >> + case 2: >> + writel(BF_ANADIG_PLL_VIDEO_DIV_SELECT(pll_div) | >> + BF_ANADIG_PLL_VIDEO_POST_DIV_SELECT(0x1), >> + &imx_ccm->analog_pll_video_set); >> + break; >> + case 4: >> + writel(BF_ANADIG_PLL_VIDEO_DIV_SELECT(pll_div) | >> + BF_ANADIG_PLL_VIDEO_POST_DIV_SELECT(0x0), >> + &imx_ccm->analog_pll_video_set); >> + break; >> + default: >> + puts("Wrong test_div!\n"); >> + return -EINVAL; >> + } >> + >> + writel(BF_ANADIG_PLL_VIDEO_NUM_A(pll_num), >> + &imx_ccm->analog_pll_video_num); >> + writel(BF_ANADIG_PLL_VIDEO_DENOM_B(pll_denom), >> + &imx_ccm->analog_pll_video_denom); >> + >> + /* Wait PLL5 lock */ >> + start = get_timer(0); /* Get current timestamp */ >> + >> + do { >> + reg = readl(&imx_ccm->analog_pll_video); >> + if (reg & BM_ANADIG_PLL_VIDEO_LOCK) { >> + /* Enable PLL out */ >> + writel(BM_ANADIG_PLL_VIDEO_ENABLE, >> + &imx_ccm->analog_pll_video_set); >> + return 0; >> + } >> + } while (get_timer(0) < (start + 10)); /* Wait 10ms */ >> + >> + printf("Lock PLL5 timeout\n"); > >Maybe puts() is better here
Thanks, will use puts in V3. > >> + >> + return -ETIME; >> +} >> + >> +/* >> + * 24M--> PLL_VIDEO -> LCDIFx_PRED -> LCDIFx_PODF -> LCD >> + * >> + * 'freq' using KHz as unit, see driver/video/mxsfb.c. >> + */ >> +void mxs_set_lcdclk(u32 base_addr, u32 freq) >> +{ >> + u32 reg = 0; >> + u32 hck = MXC_HCLK / 1000; >> + /* DIV_SELECT ranges from 27 to 54 */ >> + u32 min = hck * 27; >> + u32 max = hck * 54; >> + u32 temp, best = 0; >> + u32 i, j, max_pred = 8, max_postd = 8, pred = 1, postd = 1; >> + u32 pll_div, pll_num, pll_denom, post_div = 1; >> + >> + debug("mxs_set_lcdclk, freq = %dKHz\n", freq); >> + >> + if ((!is_cpu_type(MXC_CPU_MX6SX)) && !is_cpu_type(MXC_CPU_MX6UL)) >> + return; >> + >> + if (base_addr == LCDIF1_BASE_ADDR) { > >As mentioned before: base_addr is really an index, it is not used as >address. The interface is then confusing. Will switch to use index. > >> + reg = readl(&imx_ccm->cscdr2); >> + /* Can't change clocks when clock not from pre-mux */ >> + if ((reg & MXC_CCM_CSCDR2_LCDIF1_CLK_SEL_MASK) != 0) >> + return; >> + } >> + >> + if (is_cpu_type(MXC_CPU_MX6SX)) { >> + reg = readl(&imx_ccm->cscdr2); >> + /* Can't change clocks when clock not from pre-mux */ >> + if ((reg & MXC_CCM_CSCDR2_LCDIF2_CLK_SEL_MASK) != 0) >> + return; >> + } >> + >> + temp = freq * max_pred * max_postd; >> + if (temp > max) { >> + puts("Please decrease freq, too large!\n"); >> + return; >> + } >> + if (temp < min) { >> + /* >> + * Register: PLL_VIDEO >> + * Bit Field: POST_DIV_SELECT >> + * 00 — Divide by 4. >> + * 01 — Divide by 2. >> + * 10 — Divide by 1. >> + * 11 — Reserved >> + * No need to check post_div(1) >> + */ >> + for (post_div = 2; post_div <= 4; post_div <<= 1) { >> + if ((temp * post_div) > min) { >> + freq *= post_div; >> + break; >> + } >> + } >> + >> + if (post_div > 4) { >> + printf("Fail to set rate to %dkhz", freq); >> + return; >> + } >> + } > >It is not clear what happens in the calling function in error case. The >error is not propagate to the caller. Of course, we see the error on the >console. Is it enough ? The caller will tzry to go on setting the LCD >controller even in case this function fails. Maybe we should change the prototype of "void mxs_set_lcdclk(u32 freq)" to "int mxs_set_lcdclk(u32 index, u32 freq);" > > >> + >> + /* Choose the best pred and postd to match freq for lcd */ >> + for (i = 1; i <= max_pred; i++) { >> + for (j = 1; j <= max_postd; j++) { >> + temp = freq * i * j; >> + if (temp > max || temp < min) >> + continue; >> + if (best == 0 || temp < best) { >> + best = temp; >> + pred = i; >> + postd = j; >> + } >> + } >> + } >> + >> + if (best == 0) { >> + printf("Fail to set rate to %dKHz", freq); >> + return; >> + } >> + >> + debug("best %d, pred = %d, postd = %d\n", best, pred, postd); >> + >> + pll_div = best / hck; >> + pll_denom = 1000000; >> + pll_num = (best - hck * pll_div) * pll_denom / hck; >> + >> + /* >> + * pll_num >> + * (24MHz * (pll_div + --------- )) >> + * pll_denom >> + *freq KHz = -------------------------------- >> + * post_div * pred * postd * 1000 >> + */ >> + >> + if (base_addr == LCDIF1_BASE_ADDR) { >> + if (enable_pll_video(pll_div, pll_num, pll_denom, post_div)) >> + return; >> + >> + /* Select pre-lcd clock to PLL5 and set pre divider */ >> + clrsetbits_le32(&imx_ccm->cscdr2, >> + MXC_CCM_CSCDR2_LCDIF1_PRED_SEL_MASK | >> + MXC_CCM_CSCDR2_LCDIF1_PRE_DIV_MASK, >> + (0x2 << MXC_CCM_CSCDR2_LCDIF1_PRED_SEL_OFFSET) | >> + ((pred - 1) << >> + MXC_CCM_CSCDR2_LCDIF1_PRE_DIV_OFFSET)); >> + >> + /* Set the post divider */ >> + clrsetbits_le32(&imx_ccm->cbcmr, >> + MXC_CCM_CBCMR_LCDIF1_PODF_MASK, >> + ((postd - 1) << >> + MXC_CCM_CBCMR_LCDIF1_PODF_OFFSET)); >> + } >> + >> + if (is_cpu_type(MXC_CPU_MX6SX)) { >> + if (enable_pll_video(pll_div, pll_num, pll_denom, post_div)) >> + return; > >This is not very good readable. If the cpu is i.MX6SX and LCD is LCDIF1, >is enable_pll_video called twice ? Why ? My bad. Will fix this in V3. > >Should we not use something like: > > if (lcd == LCDIF1 || is_cpu_type(MXC_CPU_MX6SX)) { > if (enable_pll_video(pll_div, pll_num, pll_denom, post_div)) > return; > > } > >> + >> + /* Select pre-lcd clock to PLL5 and set pre divider */ >> + clrsetbits_le32(&imx_ccm->cscdr2, >> + MXC_CCM_CSCDR2_LCDIF2_PRED_SEL_MASK | >> + MXC_CCM_CSCDR2_LCDIF2_PRE_DIV_MASK, >> + (0x2 << MXC_CCM_CSCDR2_LCDIF2_PRED_SEL_OFFSET) | >> + ((pred - 1) << >> + MXC_CCM_CSCDR2_LCDIF2_PRE_DIV_OFFSET)); >> + >> + /* Set the post divider */ >> + clrsetbits_le32(&imx_ccm->cscmr1, >> + MXC_CCM_CSCMR1_LCDIF2_PODF_MASK, >> + ((postd - 1) << >> + MXC_CCM_CSCMR1_LCDIF2_PODF_OFFSET)); >> + } >> +} >> + >> +void enable_lcdif_clock(u32 index) >> +{ > >You see, you are already using an index. Now you have a mix sometimes >with an enumeration (this index), sometimes with "base_addr". Will fix global in the patch set about usage of base_addr. Switch to use index. Thanks, Peng. > >> + u32 reg = 0; >> + u32 lcdif_clk_sel_mask, lcdif_ccgr3_mask; >> + >> + if (is_cpu_type(MXC_CPU_MX6SX)) { >> + if (index > 1) >> + return; >> + /* Set to pre-mux clock at default */ >> + lcdif_clk_sel_mask = (index == 1) ? >> + MXC_CCM_CSCDR2_LCDIF2_CLK_SEL_MASK : >> + MXC_CCM_CSCDR2_LCDIF1_CLK_SEL_MASK; >> + lcdif_ccgr3_mask = (index == 1) ? >> + (MXC_CCM_CCGR3_LCDIF2_PIX_MASK | >> + MXC_CCM_CCGR3_DISP_AXI_MASK) : >> + (MXC_CCM_CCGR3_LCDIF1_PIX_MASK | >> + MXC_CCM_CCGR3_DISP_AXI_MASK); >> + } else if (is_cpu_type(MXC_CPU_MX6UL)) { >> + if (index > 0) >> + return; >> + /* Set to pre-mux clock at default */ >> + lcdif_clk_sel_mask = MXC_CCM_CSCDR2_LCDIF1_CLK_SEL_MASK; >> + lcdif_ccgr3_mask = MXC_CCM_CCGR3_LCDIF1_PIX_MASK; >> + } else { >> + return; >> + } >> + >> + reg = readl(&imx_ccm->cscdr2); >> + reg &= ~lcdif_clk_sel_mask; >> + writel(reg, &imx_ccm->cscdr2); >> + >> + /* Enable the LCDIF pix clock */ >> + reg = readl(&imx_ccm->CCGR3); >> + reg |= lcdif_ccgr3_mask; >> + writel(reg, &imx_ccm->CCGR3); >> + >> + reg = readl(&imx_ccm->CCGR2); >> + reg |= MXC_CCM_CCGR2_LCD_MASK; >> + writel(reg, &imx_ccm->CCGR2); >> +} >> +#endif >> + >> #ifdef CONFIG_FSL_QSPI >> /* qspi_num can be from 0 - 1 */ >> void enable_qspi_clk(int qspi_num) >> diff --git a/arch/arm/include/asm/arch-mx6/clock.h >> b/arch/arm/include/asm/arch-mx6/clock.h >> index 2b220d6..15d14f0 100644 >> --- a/arch/arm/include/asm/arch-mx6/clock.h >> +++ b/arch/arm/include/asm/arch-mx6/clock.h >> @@ -66,6 +66,8 @@ int enable_spi_clk(unsigned char enable, unsigned spi_num); >> void enable_ipu_clock(void); >> int enable_fec_anatop_clock(int fec_id, enum enet_freq freq); >> void enable_enet_clk(unsigned char enable); >> +void enable_lcdif_clock(unsigned int index); >> void enable_qspi_clk(int qspi_num); >> void enable_thermal_clk(void); >> +void mxs_set_lcdclk(u32 base_addr, u32 freq); >> #endif /* __ASM_ARCH_CLOCK_H */ >> > >Best regards, >Stefano Babic > >-- >===================================================================== >DENX Software Engineering GmbH, Managing Director: Wolfgang Denk >HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany >Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sba...@denx.de >===================================================================== -- _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot