[U-Boot] [PATCH V2 2/2] i.MX6: mx6qsabrelite: Add splash screen support

[Eric Nelson <eric.nelson@boundarydevices.com>]

Thanks for the review, Stefano.

On 09/18/2012 12:47 AM, Stefano Babic wrote:
> On 18/09/2012 01:14, Eric Nelson wrote:
>
> Hi Eric,
>
>> Adds support for the Hannstar 1024 x 768 LVDS panel (Freescale part
>> number MCIMX-LVDS1) to SABRE-Lite board.
>>
>> This commit is a rebase Fabio Estevan's patch from 5/31 to
>> u-boot-video/master:
>>      http://patchwork.ozlabs.org/patch/162206/
>>
>> Modifications include:
>>      removal of i2c setup (unneeded)
>>      cleanup of lcd_iomux to use struct mxc_ccm_reg and anatop_regs
>>      and associated constants
>>
>
> All this stuff should not be part of the commit message, because it is
> more or less a changelog. Should you also include Fabio's signed-off ?
>

Okay. I'll take it out of V3.

I didn't include Fabio's sign off because he hadn't seen this
yet and I changed a fair amount of code.

>> Signed-off-by: Eric Nelson<eric.nelson@boundarydevices.com>
>> ---
>
>>   arch/arm/include/asm/arch-mx6/crm_regs.h      |    4 +
>>   board/freescale/mx6qsabrelite/mx6qsabrelite.c |   90 +++++++++++++++++++++++++
>>   include/configs/mx6qsabrelite.h               |   14 ++++-
>>   3 files changed, 107 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/arch-mx6/crm_regs.h b/arch/arm/include/asm/arch-mx6/crm_regs.h
>> index 8388e38..cffc0a1 100644
>> --- a/arch/arm/include/asm/arch-mx6/crm_regs.h
>> +++ b/arch/arm/include/asm/arch-mx6/crm_regs.h
>> @@ -294,6 +294,10 @@ struct mxc_ccm_reg {
>>   #define MXC_CCM_CHSCCDR_IPU1_DI0_CLK_SEL_MASK		(0x7)
>>   #define MXC_CCM_CHSCCDR_IPU1_DI0_CLK_SEL_OFFSET		0
>>
>> +#define CHSCCDR_CLK_SEL_LDB_DI0				3
>> +#define CHSCCDR_PODF_DIVIDE_BY_3			2
>> +#define CHSCCDR_IPU_PRE_CLK_540M_PFD			5
>> +
>>   /* Define the bits in register CSCDR2 */
>>   #define MXC_CCM_CSCDR2_ECSPI_CLK_PODF_MASK		(0x3F<<  19)
>>   #define MXC_CCM_CSCDR2_ECSPI_CLK_PODF_OFFSET		19
>> diff --git a/board/freescale/mx6qsabrelite/mx6qsabrelite.c b/board/freescale/mx6qsabrelite/mx6qsabrelite.c
>> index 4b4e89b..1632e7b 100644
>> --- a/board/freescale/mx6qsabrelite/mx6qsabrelite.c
>> +++ b/board/freescale/mx6qsabrelite/mx6qsabrelite.c
>> @@ -36,6 +36,9 @@
>>   #include<micrel.h>
>>   #include<miiphy.h>
>>   #include<netdev.h>
>> +#include<linux/fb.h>
>> +#include<ipu_pixfmt.h>
>> +#include<asm/arch/crm_regs.h>
>>   DECLARE_GLOBAL_DATA_PTR;
>>
>>   #define UART_PAD_CTRL  (PAD_CTL_PKE | PAD_CTL_PUE |	       \
>> @@ -195,6 +198,10 @@ static iomux_v3_cfg_t button_pads[] = {
>>   	MX6Q_PAD_GPIO_18__GPIO_7_13	| MUX_PAD_CTRL(BUTTON_PAD_CTRL),
>>   };
>>
>> +iomux_v3_cfg_t lcd_gpio[] = {
>> +	MX6Q_PAD_SD1_CMD__GPIO_1_18 | MUX_PAD_CTRL(NO_PAD_CTRL),
>> +};
>> +
>>   static void setup_iomux_enet(void)
>>   {
>>   	gpio_direction_output(IMX_GPIO_NR(3, 23), 0);
>> @@ -372,10 +379,84 @@ int setup_sata(void)
>>   }
>>   #endif
>>
>> +static struct fb_videomode lvds_xga = {
>> +	.name           = "Hannstar-XGA",
>> +	.refresh        = 60,
>> +	.xres           = 1024,
>> +	.yres           = 768,
>> +	.pixclock       = 15385,
>> +	.left_margin    = 220,
>> +	.right_margin   = 40,
>> +	.upper_margin   = 21,
>> +	.lower_margin   = 7,
>> +	.hsync_len      = 60,
>> +	.vsync_len      = 10,
>> +	.sync           = FB_SYNC_EXT,
>> +	.vmode          = FB_VMODE_NONINTERLACED
>> +};
>> +
>> +void lcd_iomux(void)
>> +{
>> +	struct mxc_ccm_reg *mxc_ccm = (struct mxc_ccm_reg *)CCM_BASE_ADDR;
>> +	struct anatop_regs *anatop = (struct anatop_regs *)ANATOP_BASE_ADDR;
>> +
>> +	int reg;
>> +	/* Turn on GPIO backlight */
>> +	imx_iomux_v3_setup_multiple_pads(lcd_gpio, ARRAY_SIZE(lcd_gpio));
>> +	gpio_direction_output(18, 1);
>> +
>> +	/* Turn on LDB0,IPU,IPU DI0 clocks */
>> +	reg = __raw_readl(&mxc_ccm->CCGR3);
>> +	reg |= 0x300F;
>> +	writel(reg,&mxc_ccm->CCGR3);
>
> I think we can add constants for these - at least for these constants,
> you could drop the not useful defines with offset like
> MXC_CCM_CCGR5_CGx_OFFSET. There is already a patch (not yet merged) for
> MX5, we should then doing the same for MX6.
>

Do you have a reference to the patch so I can follow precedent?

>> +
>> +	/* set PFD3_FRAC to 0x13 == 455 MHz (480*18)/0x13 */
>> +	writel(0x00003F00,&anatop->pfd_480_clr);
>> +	writel(0x00001300,&anatop->pfd_480_set);
>
> Add constants for these. They are not already defined, but they are
> added when needed, see for example ehci-mx6.c
>

Ok.

>> +
>> +	/* set LDB0, LDB1 clk select to 011/011 */
>> +	reg = readl(&mxc_ccm->cs2cdr);
>> +	reg&= ~(MXC_CCM_CS2CDR_LDB_DI0_CLK_SEL_MASK
>> +		 |MXC_CCM_CS2CDR_LDB_DI1_CLK_SEL_MASK);
>> +	reg |= (3<<MXC_CCM_CS2CDR_LDB_DI0_CLK_SEL_OFFSET)
>> +	      |(3<<MXC_CCM_CS2CDR_LDB_DI1_CLK_SEL_OFFSET);
>> +	writel(reg,&mxc_ccm->cs2cdr);
>> +
>> +	reg = readl(&mxc_ccm->cscmr2);
>> +	reg |= MXC_CCM_CSCMR2_LDB_DI0_IPU_DIV;
>> +	writel(reg,&mxc_ccm->cscmr2);
>> +
>> +	reg = readl(&mxc_ccm->chsccdr);
>> +	reg&= ~(MXC_CCM_CHSCCDR_IPU1_DI0_PRE_CLK_SEL_MASK
>> +		|MXC_CCM_CHSCCDR_IPU1_DI0_PODF_MASK
>> +		|MXC_CCM_CHSCCDR_IPU1_DI0_CLK_SEL_MASK);
>> +	/* derive clock from LDB_DI0 */
>> +	/* divide by 3 */
>> +	/* derive clock from 540M PFD */
>
> Wrong style for a multiline comment
>

Noted. These are leftovers I used while hand decoding the bits.

>> +	reg |= (CHSCCDR_CLK_SEL_LDB_DI0
>> +		<<MXC_CCM_CHSCCDR_IPU1_DI0_CLK_SEL_OFFSET)
>> +	      |(CHSCCDR_PODF_DIVIDE_BY_3
>> +		<<MXC_CCM_CHSCCDR_IPU1_DI0_PODF_OFFSET)
>> +	      |(CHSCCDR_IPU_PRE_CLK_540M_PFD
>> +		<<MXC_CCM_CHSCCDR_IPU1_DI0_PRE_CLK_SEL_OFFSET);
>> +	writel(reg,&mxc_ccm->chsccdr);
>> +
>> +	writel(0x201, IOMUXC_BASE_ADDR + 0x8);	/* 16 bpp */
>
> This is GPR2, right ? Nobody can easy understand. Use structure (not
> offset) and constants to set it.
>

Yep.

>> +}
>> +
>> +void lcd_enable(void)
>> +{
>> +
>> +	int ret = ipuv3_fb_init(&lvds_xga, 0, IPU_PIX_FMT_RGB666);
>> +	if (ret)
>> +		printf("LCD cannot be configured: %d\n", ret);
>> +}
>> +
>>   int board_early_init_f(void)
>>   {
>>   	setup_iomux_uart();
>>   	setup_buttons();
>> +	lcd_iomux();
>>
>>   	return 0;
>>   }
>> @@ -396,9 +477,18 @@ int board_init(void)
>>   	setup_sata();
>>   #endif
>>
>> +#ifdef CONFIG_VIDEO
>> +	lcd_enable();
>> +#endif
>>          return 0;
>>   }
>>
>> +int board_late_init(void)
>> +{
>> +	setenv("stdout", "serial");
>> +	return 0;
>
> This is wrong, we found a better way to do this. You should add a
> overwrite_console() function, see for example the MX5 boards.
>

Fabio noted that as well.

>> --- a/include/configs/mx6qsabrelite.h
>> +++ b/include/configs/mx6qsabrelite.h
>> @@ -39,9 +39,10 @@
>>   #define CONFIG_REVISION_TAG
>>
>>   /* Size of malloc() pool */
>> -#define CONFIG_SYS_MALLOC_LEN	       (CONFIG_ENV_SIZE + 2 * 1024 * 1024)
>> +#define CONFIG_SYS_MALLOC_LEN	       (10 * 1024 * 1024)
>>
>>   #define CONFIG_BOARD_EARLY_INIT_F
>> +#define CONFIG_BOARD_LATE_INIT
>>   #define CONFIG_MISC_INIT_R
>>   #define CONFIG_MXC_GPIO
>>
>> @@ -121,6 +122,17 @@
>>   /* Miscellaneous commands */
>>   #define CONFIG_CMD_BMODE
>>
>> +/* Framebuffer and LCD */
>> +#define CONFIG_VIDEO
>> +#define CONFIG_VIDEO_IPUV3
>> +#define CONFIG_CFB_CONSOLE
>> +#define CONFIG_VGA_AS_SINGLE_DEVICE
>> +#define CONFIG_VIDEO_BMP_RLE8
>> +#define CONFIG_SPLASH_SCREEN
>> +#define CONFIG_BMP_16BPP
>> +#define CONFIG_VIDEO_LOGO
>> +#define CONFIG_IPUV3_CLK 260000000
>
> ...adding also CONFIG_SYS_CONSOLE_OVERWRITE_ROUTINE
>

I'll send a note under separate cover about this.

> Best regards,
> Stefano Babic
>

Regards,


Eric