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

[Peng Fan <b51431@freescale.com>]

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

After a thought, I think use base_addr is better. In drivers/video/mxs_fb.c,
MXS_LCDIF_BASE is used, I do not want to introduce another macro such as
MXS_LCDIF_INDEX. So choose base_addr can avoid add more macro definitions
in board header files. I'll switch to use base_addr in the patch set.

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

--