Re: [U-Boot] [PATCH 2/2] mx3fb: move to CONFIG_VIDEO to support videomodes
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
Re: [U-Boot] [PATCH 2/2] mx3fb: move to CONFIG_VIDEO to support videomodes
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
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
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 don
Re: [U-Boot] [PATCH 2/2] mx3fb: move to CONFIG_VIDEO to support videomodes
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"
> + "
[U-Boot] [PATCH 2/2] mx3fb: move to CONFIG_VIDEO to support videomodes
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
---
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
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,
+ * Copyright (C) 2011
+ * HALE electronic GmbH,
*
* See file CREDITS for list of people who contributed to this
* project.
@@ -21,100 +23,19 @@
* MA 02111-1307 USA
*/
#include
-#include
-#include
+#include
+#include
+
#include
+#include
#include
-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 need panel specific set-up as-well */
#define IF_CONF0
-#define IF_CLK_DIV 0x175
-#endif
-#define LCD_COLOR_IPU LCD_COLOR16
-
-stati
