[U-Boot] [PATCH 2/2] mx3fb: move to CONFIG_VIDEO to support videomodes

Mon Aug 22 19:13:28 CEST 2011

On 08/22/2011 03:41 PM, Helmut Raiger wrote:

Hi Helmut,

> mx3fb.c was based on CONFIG_LCD and is moved by this patch to
> CONFIG_VIDEO, which has greater freedom in selecting videomodes
> even at runtime.
> 
> This renders the accumulating list of display defines
> (CONFIG_DISPLAY_VBEST..., CONFIG_DISPLAY_C057...) obsolete as
> these may be setup through env variables:
> 
> setenv mydisplay "video=ctfb:x:240,y:320,depth:16,mode:0,pclk:185925,
> 	le:9,ri:17,up:7,lo:10,hs:1,vs:1,sync:100663296,vmode:0"
> setenv videomode mydisplay

Really betters as hard coded in driver...

I see you updated the code synchronizing it with the linux driver. Add
to the commit message the kernel version (better: the commit id) of the
kernel you used as base for your changes, so that everybody in future
can know where it comes from.

> +DECLARE_GLOBAL_DATA_PTR;
> +
> +/* graphics setup */
> +static GraphicDevice panel;
> +static struct ctfb_res_modes *mode;
> +static struct ctfb_res_modes var_mode;
> +
> +

One newline should be enough.

>  static inline u32 reg_read(unsigned long reg)
>  {
>  	return __REG(reg);
> @@ -452,28 +381,39 @@ static inline void reg_write(u32 value, unsigned long reg)
>   * sdc_init_panel() - initialize a synchronous LCD panel.
>   * @width:		width of panel in pixels.
>   * @height:		height of panel in pixels.
> - * @pixel_fmt:		pixel format of buffer as FOURCC ASCII code.

pixel_fmt is still in the parameter list, and di_panel should be added
to the description.

>   * @return:		0 on success or negative error code on failure.
>   */
> -static int sdc_init_panel(u16 width, u16 height, enum pixel_fmt pixel_fmt)
> +static int sdc_init_panel(u16 width, u16 height,
> +		enum pixel_fmt di_setup, enum ipu_panel di_panel)
>  {
> -	u32 reg;
> +	u32 reg, div;
>  	uint32_t old_conf;
>  
> +	debug("%s(width=%d, height=%d)\n", __func__, width, height);
> +
>  	/* Init panel size and blanking periods */
> -	reg = ((H_SYNC_WIDTH - 1) << 26) |
> -		((u32)(width + H_START_WIDTH + H_END_WIDTH - 1) << 16);
> +	reg = width + mode->left_margin + mode->right_margin - 1;
> +	if (reg > 1023) {
> +		debug("display width too large, coerced to 1023!");
> +		reg = 1023;

I do not if it is better to try to adapt the values if the caller pass
to the function wrong parameters. Probably it does not work. I think in
these case it is better to output an error (print instead of debug) and
return without with an error. What do you think ?

> -	switch (PANEL_TYPE) {
> +	switch (di_panel) {
>  	case IPU_PANEL_SHARP_TFT:
>  		reg_write(0x00FD0102L, SDC_SHARP_CONF_1);
>  		reg_write(0x00F500F4L, SDC_SHARP_CONF_2);
>  		reg_write(SDC_COM_SHARP | SDC_COM_TFT_COLOR, SDC_COM_CONF);
> +		/* TODO: probably IF_CONF must be adapted (see below)! */

I do not understand this comment.

>  	/* Init clocking */
>  
> -	/*
> -	 * Calculate divider: fractional part is 4 bits so simply multiple by
> -	 * 2^4 to get fractional part, as long as we stay under ~250MHz and on
> -	 * i.MX31 it (HSP_CLK) is <= 178MHz. Currently 128.267MHz
> +	/* Calculate divider: the IPU receives its clock from the hsp divder */
> +	/* fractional part is 4 bits so simply multiple by 2^4 to get it

Multiline comments, you should use the same style as in the removed lines.

> +	div = ((mxc_get_clock(MXC_IPU_CLK)/1024)*(mode->pixclock/128))/476837;

I try to understand this line. pixclock is in ps, as in kernel. I am
missing something. I am expecting to have the same formula as in kernel,
where I can read:

div = clk_get_rate(ipu_clk) * 16 / pixel_clk;

Where does 476837 come from ?

> +	debug("hsp_clk is %d, div=%d\n", mxc_get_clock(MXC_IPU_CLK), div);
> +	/* coerce to not less than 4.0, not more than 255.9375 */
> +	if (div < 0x40)
> +		div = 0x40;
> +	else if (div > 0xFFF)
> +		div = 0xFFF;
> +	/* DISP3_IF_CLK_DOWN_WR is half the divider value and 2 less
> +	 * fraction bits. Subtract 1 extra from DISP3_IF_CLK_DOWN_WR
> +	 * based on timing debug DISP3_IF_CLK_UP_WR is 0
> +	 */
> +	reg_write((((div / 8) - 1) << 22) | div, DI_DISP3_TIME_CONF);

Right. This code is now with the linux driver synchronized. If I could
understand how div is computed and because it is different from the
linux driver...

> +/*
> + * The current implementation is only tested for GDF_16BIT_565RGB!
> + * It was switched from the original CONFIG_LCD setup to CONFIG_VIDEO,
> + * because the lcd code seemed loaded with color table stuff, that
> + * does not relate to most modern TFTs. cfb_console.c looks more
> + * straight forward.
> + * This is the environment setting for the original setup
> +     "unknown=video=ctfb:x:240,y:320,depth:16,mode:0,pclk:185925,le:9,ri:17,
> +		up:7,lo:10,hs:1,vs:1,sync:100663296,vmode:0"
> +     "videomode=unknown"

Multiline comment. As "original setup" you mean the setup if not
CONFIG_DISPLAY_* was set, right ?

> +void *video_hw_init(void)
>  {
> -	return ((panel_info.vl_col * panel_info.vl_row *
> -		NBITS(panel_info.vl_bpix)) / 8) + PAGE_SIZE;
> +	char *penv;
> +	u32 memsize;
> +	unsigned long t1, hsynch, vsynch;
> +	int bits_per_pixel, i, tmp, vesa_idx = 0, videomode;
> +
> +	tmp = 0;
> +
> +	videomode = CONFIG_SYS_DEFAULT_VIDEO_MODE;

Ok. This is a way to fix qong/phycore after merging these patches, and
avoid that they do not work anymore if the videomode variable is not
set. I write it down...

> +	/* get video mode via environment */
> +	penv = getenv("videomode");
> +	if (penv) {
> +		/* decide if it is a string */
> +		if (penv[0] <= '9') {
> +			videomode = (int) simple_strtoul(penv, NULL, 16);
> +			tmp = 1;
> +		}
> +	} else {
> +		tmp = 1;
> +	}
> +	if (tmp) {
> +		/* parameter are vesa modes */
> +		/* search params */
> +		for (i = 0; i < VESA_MODES_COUNT; i++) {
> +			if (vesa_modes[i].vesanr == videomode)
> +				break;
> +		}
> +		if (i == VESA_MODES_COUNT) {
> +			printf("no VESA Mode found, switching to mode 0x%x ",
> +					CONFIG_SYS_DEFAULT_VIDEO_MODE);
> +			i = 0;
> +		}
> +		mode = (struct ctfb_res_modes *)
> +				&res_mode_init[vesa_modes[i].resindex];
> +		bits_per_pixel = vesa_modes[i].bits_per_pixel;
> +		vesa_idx = vesa_modes[i].resindex;
> +	} else {
> +		mode = (struct ctfb_res_modes *) &var_mode;
> +		bits_per_pixel = video_get_params(mode, penv);
> +	}

Anatolij, should be this code not general ? It is not strictly related
to the i.MX. Should we put it in another place ?

> +
> +	/* calculate hsynch and vsynch freq (info only) */
> +	t1 = (mode->left_margin + mode->xres +
> +	      mode->right_margin + mode->hsync_len) / 8;
> +	t1 *= 8;
> +	t1 *= mode->pixclock;
> +	t1 /= 1000;
> +	hsynch = 1000000000L / t1;
> +	t1 *= (mode->upper_margin + mode->yres +
> +	       mode->lower_margin + mode->vsync_len);
> +	t1 /= 1000;
> +	vsynch = 1000000000L / t1;
> +
> +	/* fill in Graphic device struct */
> +	sprintf(panel.modeIdent, "%dx%dx%d %ldkHz %ldHz",
> +			mode->xres, mode->yres,
> +			bits_per_pixel, (hsynch / 1000), (vsynch / 1000));
> +	printf("%s\n", panel.modeIdent);

Should we not use "debug" instead ?

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================