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: off...@denx.de
=====================================================================
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
