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

2011-08-24 Thread Helmut Raiger 
On 08/22/2011 07:13 PM, Stefano Babic wrote:
 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.
Ok.

 +static struct ctfb_res_modes *mode;
 +static struct ctfb_res_modes var_mode;
 +
 +
 One newline should be enough.
Sure.

 - * @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.
I'll update it.

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

I put that code in as I tried to adjust the porches (left and right 
margin) for our display.
If it is coerced the way I did, you'll never overwrite other bits in the 
same register field, so
you can still see the effect it has. I preferred it during display 
configuration, so I left it in.
We could output an error (not only during debug builds) but write the 
registers anyway.

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

Each display specific define found an equivalent in the ctfb_res_modes 
struct.
IF_CONF however is currently always 0, but might need adaption for SHARP 
TFT panels, which
I could not test.

  /* 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.
Ok.

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

Well I already thought that might need another line of comment. In the 
kernel the pixel_clk really
is a clock value, while it is a time (in pico seconds) in this driver. I 
could have calculated the
pixel clock from the pixel time value first:

pixel_clk = 1e12 / mode-pixclock;
div = ipu_clk * 16 / pixel_clk;

I simply put that into one calculation:

div = ipu_clk * 16 / (1e12 / mode-pixclock)

or

div = ipu_clk * mode-pixclock / ~60e6;

but this would provoke an overflow problem during the multiplication, so 
I split the division to
1024, 128 and 476837 which exactly gives 1e12 / 16 (~60e6). So I have 2 
shifts a multiplication and a division.

Probably I simply put the 2 code lines above into a comment. The name 
'pixel_clk' is actually misleading, but it sat there already. We could 
use 'pixel_ps' in ctfb_res_modes instead?

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

I'll fix the comment and yes you're 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...

Perfect. I could have done that already, but lacking hardware to test 
with ...

 Anatolij, should be this code not general ? It is not strictly related
 to the

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

2011-08-24 Thread Helmut Raiger 
Please ignore the 2 messages sent at 8:30, obviously the save and send 
buttons changed position.
Helmut


--
Scanned by MailScanner.

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2011-08-24 Thread Helmut Raiger 

I just found that the mx3fb driver uses:

static inline u32 reg_read(unsigned long reg)
{
return __REG(reg);
}

static inline void reg_write(u32 value, unsigned long reg)
{
__REG(reg) = value;
}

I am about to change this to readl() and writel(), should I do it in a 
separate 'cosmetic only' patch before the really thing?
As it is more or less a re-write anyway (350 of 900 lines changed), it 
think its probably ok to do it in one shot.

Helmut


--
Scanned by MailScanner.

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

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

2011-08-24 Thread Stefano Babic 
On 08/24/2011 05:34 PM, Helmut Raiger wrote:
 
 I just found that the mx3fb driver uses:
 
 static inline u32 reg_read(unsigned long reg)
 {
 return __REG(reg);
 }
 
 static inline void reg_write(u32 value, unsigned long reg)
 {
 __REG(reg) = value;
 }
 

i.MX31 is quite old, and it was pushed before general ARM accessors were
introduced in u-boot. This is the reason. I appreciate you will fix this
point.

 I am about to change this to readl() and writel(), should I do it in a 
 separate 'cosmetic only' patch before the really thing?
 As it is more or less a re-write anyway (350 of 900 lines changed), it 
 think its probably ok to do it in one shot.

IMHO you can do in one shot - you can mention this change in the commit
message.

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

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

2011-08-22 Thread Helmut Raiger 
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

Signed-off-by: Helmut Raiger helmut.rai...@hale.at
---
 drivers/video/Makefile  |2 +-
 drivers/video/cfb_console.c |7 +
 drivers/video/mx3fb.c   |  353 +--
 3 files changed, 213 insertions(+), 149 deletions(-)

diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 086dc05..2c0e500 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -34,7 +34,7 @@ COBJS-$(CONFIG_VIDEO_AMBA) += amba.o
 COBJS-$(CONFIG_VIDEO_CT69000) += ct69000.o videomodes.o
 COBJS-$(CONFIG_VIDEO_MB862xx) += mb862xx.o videomodes.o
 COBJS-$(CONFIG_VIDEO_MB86R0xGDC) += mb86r0xgdc.o videomodes.o
-COBJS-$(CONFIG_VIDEO_MX3) += mx3fb.o
+COBJS-$(CONFIG_VIDEO_MX3) += mx3fb.o videomodes.o
 COBJS-$(CONFIG_VIDEO_MX5) += mxc_ipuv3_fb.o ipu_common.o ipu_disp.o
 COBJS-$(CONFIG_VIDEO_SED13806) += sed13806.o
 COBJS-$(CONFIG_SED156X) += sed156x.o
diff --git a/drivers/video/cfb_console.c b/drivers/video/cfb_console.c
index 3a93b64..4e653b8 100644
--- a/drivers/video/cfb_console.c
+++ b/drivers/video/cfb_console.c
@@ -161,6 +161,13 @@
 #endif
 
 /*
+ * Defines for the i.MX31 driver (mx3fb.c)
+ */
+#ifdef CONFIG_VIDEO_MX3
+#define VIDEO_FB_16BPP_WORD_SWAP
+#endif
+
+/*
  * Include video_fb.h after definitions of VIDEO_HW_RECTFILL etc.
  */
 #include video_fb.h
diff --git a/drivers/video/mx3fb.c b/drivers/video/mx3fb.c
index 0c925a0..1808ec3 100644
--- a/drivers/video/mx3fb.c
+++ b/drivers/video/mx3fb.c
@@ -1,6 +1,8 @@
 /*
  * Copyright (C) 2009
  * Guennadi Liakhovetski, DENX Software Engineering, l...@denx.de
+ * Copyright (C) 2011
+ * HALE electronic GmbH, helmut.rai...@hale.at
  *
  * See file CREDITS for list of people who contributed to this
  * project.
@@ -21,100 +23,19 @@
  * MA 02111-1307 USA
  */
 #include common.h
-#include lcd.h
-#include asm/arch/clock.h
+#include malloc.h
+#include video_fb.h
+
 #include asm/arch/imx-regs.h
+#include asm/arch/clock.h
 #include asm/errno.h
 
-DECLARE_GLOBAL_DATA_PTR;
-
-void *lcd_base;/* Start of framebuffer memory  */
-void *lcd_console_address; /* Start of console buffer  */
-
-int lcd_line_length;
-int lcd_color_fg;
-int lcd_color_bg;
-
-short console_col;
-short console_row;
-
-void lcd_initcolregs(void)
-{
-}
-
-void lcd_setcolreg(ushort regno, ushort red, ushort green, ushort blue)
-{
-}
-
-void lcd_disable(void)
-{
-}
-
-void lcd_panel_disable(void)
-{
-}
+#include videomodes.h
 
-#define msleep(a) udelay(a * 1000)
-
-#if defined(CONFIG_DISPLAY_VBEST_VGG322403)
-#define XRES   320
-#define YRES   240
-#define PANEL_TYPE IPU_PANEL_TFT
-#define PIXEL_CLK  156000
-#define PIXEL_FMT  IPU_PIX_FMT_RGB666
-#define H_START_WIDTH  20  /* left_margin */
-#define H_SYNC_WIDTH   30  /* hsync_len */
-#define H_END_WIDTH(38 + 30)   /* right_margin + hsync_len */
-#define V_START_WIDTH  7   /* upper_margin */
-#define V_SYNC_WIDTH   3   /* vsync_len */
-#define V_END_WIDTH(26 + 3)/* lower_margin + vsync_len */
-#define SIG_POL(DI_D3_DRDY_SHARP_POL | DI_D3_CLK_POL)
-#define IF_CONF0
-#define IF_CLK_DIV 0x175
-#elif defined(CONFIG_DISPLAY_COM57H5M10XRC)
-#define XRES   640
-#define YRES   480
-#define PANEL_TYPE IPU_PANEL_TFT
-#define PIXEL_CLK  4
-#define PIXEL_FMT  IPU_PIX_FMT_RGB666
-#define H_START_WIDTH  120 /* left_margin */
-#define H_SYNC_WIDTH   30  /* hsync_len */
-#define H_END_WIDTH(10 + 30)   /* right_margin + hsync_len */
-#define V_START_WIDTH  35  /* upper_margin */
-#define V_SYNC_WIDTH   3   /* vsync_len */
-#define V_END_WIDTH(7 + 3) /* lower_margin + vsync_len */
-#define SIG_POL(DI_D3_DRDY_SHARP_POL | DI_D3_CLK_POL)
-#define IF_CONF0
-#define IF_CLK_DIV 0x55
-#else
-#define XRES   240
-#define YRES   320
-#define PANEL_TYPE IPU_PANEL_TFT
-#define PIXEL_CLK  185925
-#define PIXEL_FMT  IPU_PIX_FMT_RGB666
-#define H_START_WIDTH  9   /* left_margin */
-#define H_SYNC_WIDTH   1   /* hsync_len */
-#define H_END_WIDTH(16 + 1)/* right_margin + hsync_len */
-#define V_START_WIDTH  7   /* upper_margin */
-#define V_SYNC_WIDTH   1   /* vsync_len */
-#define V_END_WIDTH(9 + 1) /* lower_margin + vsync_len */
-#define SIG_POL(DI_D3_DRDY_SHARP_POL | DI_D3_CLK_POL)
+/* this might

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

2011-08-22 Thread Stefano Babic 
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_writediv / 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,