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
