[U-Boot] [PATCH V2 07/14] imx: mx6: add clock api for lcdif

[Stefano Babic <sbabic@denx.de>]

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.Fan@freescale.com>
> Cc: Stefano Babic <sbabic@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

> +
> +	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.

> +		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.


> +
> +	/* 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 ?

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".

> +	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: sbabic at denx.de
=====================================================================