powerpc: 'monitor' environment variable contains full video configuration

Update the "monitor" environment variable (for Freescale chips that have a DIU display controller) to designate the full video configuration, instead of just the output monitor.

The old definition of the "monitor" environment variable only determines which video port to use for output. This variable is set to a number (0, 1, or sometimes 2) to specify a DVI, LVDS, or Dual-LVDS port. The resolution is hard-coded into board-specific code. The Linux command-line arguments need to be hard-coded to the proper video definition string.

Instead, the "monitor" variable is now set to a string that indicates not only the part name, but also the resolution, color depth, and refresh rate. This string is then parsed for all needed information, and can be passed to the kernel via the 'diubootargs' environment variable.

---

This patch depends on my previous patch, "[v4] powerpc: clean up DIU macro definitions for Freescale reference boards".

 README                                        |    9 +-
 board/freescale/mpc8610hpcd/mpc8610hpcd_diu.c |   73 +-
 board/freescale/p1022ds/diu.c                 |   29 +-
 doc/README.video                              |   29 +
 drivers/video/fsl_diu_fb.c                    | 1019 ++++++++++++-------------
 include/configs/MPC8610HPCD.h                 |   16 +-
 include/configs/P1022DS.h                     |   12 +-
 include/fsl_diu_fb.h                          |   91 +--
 8 files changed, 631 insertions(+), 647 deletions(-)

rewrite drivers/video/fsl_diu_fb.c (63%)
rewrite include/fsl_diu_fb.h (62%)

index 9597fed..4fc35f4 100644
@@ -1072,12 +1072,9 @@ The following options need to be configured:
 			CONFIG_VIDEO_BMP_LOGO
 
 		The DIU driver will look for the 'monitor' environment variable,
-		and if defined, enable the DIU as a console during boot.  This
-		variable should be set to one of these values:
-
-			'0'	Output video to the DVI connector
-			'1'	Output video to the LVDS connector
-			'2'	Output video to the Dual-Link LVDS connector
+		and if defined, enable the DIU as a console during boot.  See
+		the documentation file README.video for a description of this
+		variable.
 
 - Keyboard Support:
 		CONFIG_KEYBOARD

index 8f5305c..544f254 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright 2010 Freescale Semiconductor, Inc.
+ * Copyright 2010-2011 Freescale Semiconductor, Inc.
  * Authors: Timur Tabi <[email protected]>
  *
  * FSL DIU Framebuffer driver

@@ -12,6 +12,7 @@
 
 #include <common.h>
 #include <command.h>
+#include <linux/ctype.h>
 #include <asm/io.h>
 #include <stdio_dev.h>
 #include <video_fb.h>

@@ -81,10 +82,11 @@ void diu_set_pixel_clock(unsigned int pixclock)
 	out_be32(&gur->clkdvdr, temp | 0x80000000 | ((pixval & 0x1F) << 16));
 }
 
-int platform_diu_init(unsigned int *xres, unsigned int *yres)
+int platform_diu_init(unsigned int xres, unsigned int yres)
 {
 	ccsr_gur_t *gur = (void *)(CONFIG_SYS_MPC85xx_GUTS_ADDR);
-	char *monitor_port;
+	const char *monitor_port;
+	const char *name;
 	u32 pixel_format;
 	u8 temp;
 

@@ -101,20 +103,23 @@ int platform_diu_init(unsigned int *xres, unsigned int *yres)
 	temp = in_8(&pixis->brdcfg1);
 
 	monitor_port = getenv("monitor");
-	if (!strncmp(monitor_port, "1", 1)) { /* 1 - Single link LVDS */
-		*xres = 1024;
-		*yres = 768;
-		/* Enable the DFP port, disable the DVI and the backlight */
-		temp &= ~(PX_BRDCFG1_DVIEN | PX_BRDCFG1_BACKLIGHT);
-		temp |= PX_BRDCFG1_DFPEN;
+	if (strncmp(monitor_port, "lvds", 4) == 0) {
+		/* Single link LVDS */
+		temp &= ~PX_BRDCFG1_DVIEN;
+		/*
+		 * LVDS also needs backlight enabled, otherwise the display
+		 * will be blank.
+		 */
+		temp |= (PX_BRDCFG1_DFPEN | PX_BRDCFG1_BACKLIGHT);
+		name = "Single-Link LVDS";
 	} else {	/* DVI */
-		*xres = 1280;
-		*yres = 1024;
 		/* Enable the DVI port, disable the DFP and the backlight */
 		temp &= ~(PX_BRDCFG1_DFPEN | PX_BRDCFG1_BACKLIGHT);
 		temp |= PX_BRDCFG1_DVIEN;
+		name = "DVI";
 	}
 
+	printf("DIU:   Switching to %s monitor @ %ux%u\n", name, xres, yres);
 	out_8(&pixis->brdcfg1, temp);
 
 	/*

@@ -136,7 +141,7 @@ int platform_diu_init(unsigned int *xres, unsigned int *yres)
 	clrsetbits_be32(&gur->pmuxcr, PMUXCR_ELBCDIU_MASK, PMUXCR_ELBCDIU_DIU);
 	pmuxcr = in_be32(&gur->pmuxcr);
 
-	return fsl_diu_init(*xres, pixel_format, 0);
+	return fsl_diu_init(xres, pixel_format, 0);
 }
 
 #ifdef CONFIG_CFI_FLASH_USE_WEAK_ACCESSORS

index 34e199c..c0d5170 100644
@@ -28,3 +28,32 @@ The driver has been tested with the following configurations:
 
 - MPC823FADS with AD7176 on a PAL TV (YCbYCr)	- arsenio at tin.it
 - GENIETV    with AD7177 on a PAL TV (YCbYCr)	- arsenio at tin.it
+
+
+Freescale DIU video controller
+===============================
+
+This controller is found on some Freescale PowerPC SOCs, such as the MPC8610
+and the P1022.  Currently, setting the 'stdout' environment variable to 'video'
+will *not* work with this controller.  Instead, the 'monitor' environment
+variable can be used to enable and configure the driver.
+
+	monitor=<port>,<xres>x<yres>-<depth>@<freq>
+
+Example: monitor=dvi,1280x1024-32 at 60
+
+	<port>	The monitor port, usually either "lvds" or "dvi".  This
+		is a board-specific value.
+
+	<xres>	The X resolution (in pixels) to use.
+
+	<yres>	The Y resolution (in pixels) to use.
+
+	<depth>	The color depth (in bits) to use.
+
+	<freq>	The frequency (in Hz) to use.
+
+The "diubootargs" environment variable is also set to the string
+"console=tty0 video=fslfb:<xres>x<yres>-<depth>@<freq>,monitor=<port>".  This
+variable allows the string to be passed to the Linux kernel via the kernel
+command line.

index de01c31..60f2a16 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright 2007, 2010 Freescale Semiconductor, Inc.
+ * Copyright 2007-2011 Freescale Semiconductor, Inc.
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License

@@ -23,6 +23,8 @@
 
 
 /* video */
+#define CONFIG_FSL_DIU_FB
+
 #ifdef CONFIG_FSL_DIU_FB
 #define CONFIG_SYS_DIU_ADDR	(CONFIG_SYS_CCSRBAR + 0x2c000)
 #define CONFIG_VIDEO

@@ -625,8 +627,6 @@
  "diuregs=md e002c000 1d\0" \
  "dium=mw e002c01c\0" \
  "diuerr=md e002c014 1\0" \
- "othbootargs=diufb=15M video=fslfb:1280x1024-32 at 60,monitor=0 debug\0" \
- "monitor=0-DVI\0" \
  "pmregs=md e00e1000 2b\0" \
  "lawregs=md e0000c08 4b\0" \
  "lbcregs=md e0005000 36\0" \

@@ -646,23 +646,21 @@
  "ramdiskfile=8610hpcd/ramdisk.uboot\0"                         \
  "fdtaddr=c00000\0"                                             \
  "fdtfile=8610hpcd/mpc8610_hpcd.dtb\0"                          \
- "bdev=sda3\0"							\
- "othbootargs=diufb=15M video=fslfb:1280x1024-32@60,monitor=0\0"\
- "monitor=0-DVI\0"
+ "bdev=sda3\0"
 #endif
 
 #define CONFIG_NFSBOOTCOMMAND					\
  "setenv bootargs root=/dev/nfs rw "				\
 	"nfsroot=$serverip:$rootpath "				\
 	"ip=$ipaddr:$serverip:$gatewayip:$netmask:$hostname:$netdev:off " \
-	"console=$consoledev,$baudrate $othbootargs;"		\
+	"console=$consoledev,$baudrate $diubootargs $othbootargs;"	\
  "tftp $loadaddr $bootfile;"					\
  "tftp $fdtaddr $fdtfile;"					\
  "bootm $loadaddr - $fdtaddr"
 
 #define CONFIG_RAMBOOTCOMMAND \
  "setenv bootargs root=/dev/ram rw "				\
-	"console=$consoledev,$baudrate $othbootargs;"		\
+	"console=$consoledev,$baudrate $diubootargs $othbootargs;"	\
  "tftp $ramdiskaddr $ramdiskfile;"				\
  "tftp $loadaddr $bootfile;"					\
  "tftp $fdtaddr $fdtfile;"					\

@@ -670,7 +668,7 @@
 
 #define CONFIG_BOOTCOMMAND		\
  "setenv bootargs root=/dev/$bdev rw "	\
-	"console=$consoledev,$baudrate $othbootargs;"	\
+	"console=$consoledev,$baudrate $diubootargs $othbootargs;"	\
  "tftp $loadaddr $bootfile;"		\
  "tftp $fdtaddr $fdtfile;"		\
  "bootm $loadaddr - $fdtaddr"

index ed2bada..29380a1 100644
@@ -185,6 +185,8 @@
 #define CONFIG_SYS_PROMPT_HUSH_PS2 "> "
 
 /* Video */
+#define CONFIG_FSL_DIU_FB
+
 #ifdef CONFIG_FSL_DIU_FB
 #define CONFIG_SYS_DIU_ADDR	(CONFIG_SYS_CCSRBAR + 0x10000)
 #define CONFIG_VIDEO

@@ -445,13 +447,11 @@
 	"bdev=sda3\0"		  			      		\
 	"diuregs=md e002c000 1d\0"			 		\
 	"dium=mw e002c01c\0" 						\
-	"diuerr=md e002c014 1\0" 					\
-	"othbootargs=diufb=15M video=fslfb:1280x1024-32 at 60,monitor=0 tty0\0" \
-	"monitor=0-DVI\0"
+	"diuerr=md e002c014 1\0"
 
 #define CONFIG_HDBOOT					\
 	"setenv bootargs root=/dev/$bdev rw "		\
-	"console=$consoledev,$baudrate $othbootargs;"	\
+	"console=$consoledev,$baudrate $diubootargs $othbootargs;"	\
 	"tftp $loadaddr $bootfile;"			\
 	"tftp $fdtaddr $fdtfile;"			\
 	"bootm $loadaddr - $fdtaddr"

@@ -460,14 +460,14 @@
 	"setenv bootargs root=/dev/nfs rw "				\
 	"nfsroot=$serverip:$rootpath "					\
 	"ip=$ipaddr:$serverip:$gatewayip:$netmask:$hostname:$netdev:off " \
-	"console=$consoledev,$baudrate $othbootargs;"			\
+	"console=$consoledev,$baudrate $diubootargs $othbootargs;"	\
 	"tftp $loadaddr $bootfile;"					\
 	"tftp $fdtaddr $fdtfile;"					\
 	"bootm $loadaddr - $fdtaddr"
 
 #define CONFIG_RAMBOOTCOMMAND						\
 	"setenv bootargs root=/dev/ram rw "				\
-	"console=$consoledev,$baudrate $othbootargs;"			\
+	"console=$consoledev,$baudrate $diubootargs $othbootargs;"	\
 	"tftp $ramdiskaddr $ramdiskfile;"				\
 	"tftp $loadaddr $bootfile;"					\
 	"tftp $fdtaddr $fdtfile;"					\

1.7.3.4

Dear Timur Tabi,

Looks as if this was a Freescale specific implementation?

Can this not be turned into generic code, usable for other boards / systems as well?

Please see the discussion we had about video modes for the i.MX systems.

I want to see handled most of this with generic code, not with processor / architecture specific one.

Best regards,

Wolfgang Denk

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Even historians fail to learn from history -- they repeat the same
mistakes.
	-- John Gill, "Patterns of Force", stardate 2534.7

Yes.

Possibly, but I would like to fix one bug at a time.

Can you point me to the thread or at least tell me the subject line? I have no idea what you're talking about.

This patch also cleans up a lot of the DIU code to make it handle more generic options.

Timur Tabi
Linux kernel developer

Dear Tabi Timur-B04825,

Agreed, but please not by introducing lots of new, probably later incompatible code.

See http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/88169/focus=88240

OK, so please split that patch: one patch should do this cleanup, and another one should contain the rest.

Instead of a non-standard and undocumented 'diubootargs' environment variable please use something (probably called "video-mode" :-) that can be passed as "video-mode=" boot argument to Linux.

At this point I wonder which use ther eis left for your "monitor" variable - it should be completely redundant now?

Best regards,

Wolfgang Denk

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A Freudian slip is when you say one thing but mean your mother.

Fair enough.

Ok, so what I get from that thread is to change the name of the variables ("monitor" -> "video" and "diubootargs" -> something else more generic) and update the videomodes.c code to parse the same video string that the kernel uses. I also gathered that no one has chosen to update videomodes.c, so I would have to do it.

Do I have that right?

I can't make any promises, since a lot of the code is intermingled.

Well, it may still be necessary to have two variables. One for the mode that U-Boot parses, and one for the string that is passed to the kernel. This is because the kernel also needs to see "console=tty0" on the command line only if video is supposed to be enabled. I want to make video mode completely dynamic, so that if the 'video-mode' variable is set, then the console is switched to the video device, and the kernel is told to do the same. Otherwise, all output will go to the serial port.

Timur Tabi
Linux kernel developer at Freescale

-- 
Timur Tabi
Linux kernel developer at Freescale

Dear Timur Tabi,

video-mode ?

I think so.

Anatolij?
Stefano?

No, this is exactly what should be avoided.

U-Boot and the Kernel should parse exactly the same variable.

This may or may not be the case. Frequently we still use a serial console even when booting with a graphics display enabled.

This is a different option, and does not belong into that setting.

NAK. video-mode settings and console settings are two separate things and must not be mangled into a single variable.

Best regards,

Wolfgang Denk

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
And now remains  That we find out the cause of this effect, Or rather
say, the cause of this defect...           -- Hamlet, Act II, Scene 2

Well, that's why I have two variables. My patch has the ability to set the kernel command line appropriately if the video display is configured and enabled in U-Boot. The second variable is used to assist in setting the actual kernel command-line, because that's the easiest and safest way to do it. An alternative that I tried to implement is to have do_bootm_linux() edit the kernel command line directly, removing any existing video= option and putting a new one in, but I found that to be too intrusive, especially since we don't have any good string editing functions already in U-Boot.

There are three things that need to be done:

1) The video mode needs to be configured
2) The video display needs to be enabled and the U-Boot console needs to be routed to it
3) The kernel command line needs to be set

For #1, we use video-mode. How do you want #2 and #3 handled?

Timur Tabi
Linux kernel developer at Freescale

Dear Timur Tabi,

Don't try to be more clever than the user. Instead of helping, you restrict him. That's bad.

NAK, NAK, NAK. All such automatic and unconditional editing is bad and should strictly be avoided.

Leave the decision which device to use as console to the user.

ACK.

NAK.

Wether the U-Boot console is attached to the serial port or the video console or netconsole or anything else should be left to the user.

A default setting is OK, but the user must be able to set anything he likes.

ACK. Again, the user must have free choice of options.

See above.

Best regards,

Wolfgang Denk

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The alternative to genuflecting before the  god  of  code-bumming  is
finding  a  better  algorithm.  It should be clear that none such was
available. If your code is too slow, you must make it faster.  If  no
better algorithm is available, you must trim cycles.
                                 - td at alice.UUCP (Tom Duff) 29 Aug 88

I'm not being more clever. The code is setting a variable (diubootargs) that is guaranteed to be the same video mode that U-Boot is running. If you want to ensure that Linux set to the same video mode, then use the variable. Otherwise, don't use the variable and set the command line manually.

You didn't understand my post. I was saying that I tried to implement it, but gave up because it got too complicated.

That's what the 'monitor' environment variable is for.

Again, that's what the variable is for. What's the point of configuring the video display if you're not going to enable it?

Are we speaking the same language? It doesn't appear that you're understanding anything I'm saying.

I still don't understand what you actually want.

Timur Tabi
Linux kernel developer at Freescale

Dear Timur Tabi,

You don't need another variable for setting the video mode, because we (will) have "video-mode" for that very purpose, that can and shall be used both in U-Boot and Linux.

I did understand your posting. I wanted to tell you that you should not even try doing such things.

"monitor" has nothing to do with the console, right?

typically we (here at DENX) use helper macros like

	setenv addcons 'setenv bootargs ${bootargs} console=${consdev},${baudrate}'
	setenv consdev ttyS0

Then you can have the "addcons" in some command sequence that builds up the bootargs.

"monitor"? No, this has _nothing_ to do with any console settings.

You misinterpret what I wrote. Of course we're going to enable the video display then.

But there is no reason to always and unconditionally put the console on that device - that is a completely separate and independent decision.

And vice versa. Probably you don't read what I'm writing either.

I want that you keep the console settings out of this topic. It has nothing to do with it.

And I want to make sure that we don't have several environment variables doing more or less the same thing.

Best regards,

Wolfgang Denk

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A Chairman was as necessary to a Board planet  as  the  zero  was  in
mathematics, but being a zero had big disadvantages...
                         - Terry Pratchett, _The Dark Side of the Sun_

I still don't understand the point of enabling the video display in U-Boot if we're not going to put the U-Boot console on it. The monitor will just have a blank screen.

We don't need to enable it for Linux' sake, because Linux can enable the monitor itself if it wants to. It just needs to be told what resolution to use.

Timur Tabi
Linux kernel developer at Freescale

Dear Timur Tabi,

It may be a blank screen, or a (previously loaded) splash screen, or (a bit later) some graphics application outputting data.

It's up to the user to decide that.

Best regards,

Wolfgang Denk

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"I've finally learned what `upward compatible' means. It means we get
to keep all our old mistakes." - Dennie van Tassel

Ok, I think I get it. But I'm going to miss this merge window, because now I have to rewrite half of my patch.

What is the standard mechanism for enabling the console on the video display?

Dear Timur Tabi,

You won't. We have a delay of at least 2 weeks.

That's the default. Users can overwrite it, for example in "preboot". See also CONFIG_ENV_OVERWRITE

Best regards,

Wolfgang Denk

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
People are very flexible and learn to adjust to strange  surroundings
--  they can become accustomed to read Lisp and Fortran programs, for
example.   - Leon Sterling and Ehud Shapiro, Art of Prolog, MIT Press

Then I'm confused, because video_hw_init() is what configures and enables the video display. So if that function succeeds (i.e. returns non-NULL), then U-Boot will put the console onto the video display. You said you wanted me to implement some mechanism where I enable the display, based on the value of the video-mode variable, without putting console on the display. However, from you just said, it appears that this mechanism already exists.

Timur Tabi
Linux kernel developer at Freescale

Dear Timur Tabi,

Please re-read the conversation then. Mind the context (U-Boot versus Linux). Mind what I wrote about defaults and users being able to chose the behaviour they want. And keep in mind what I wrote about enforcing desicions "in the best interest" of your users (who may happen to disagree).

Best regards,

Wolfgang Denk

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
News is what a chap who doesn't care much  about  anything  wants  to
read. And it's only news until he's read it. After that it's dead.
                           - Evelyn Waugh _Scoop_ (1938) bk. 1, ch. 5

After re-reading it, I've come to the conclusion is that as far as my patch is concerned, I just need to make the following changes:

1) Modify video_get_params() (in videomodes.c) to parse the "video-mode "environment variable, using the same format that Linux uses. Right now, it parses the variable "video", so that variable is now deprecated.

2) Update the DIU driver to use this feature of videomodes.c. This means taking the functionality of my function diu_get_video_params() and putting it into video_get_params().

3) Remove the code that sets diubootargs. If the user wants to pass parameters to Linux, he has to add "video=$video-mode console=tty0" to his command-line. If "video-mode" is not set, then he must *not* add "video=$video-mode console=tty0" to his command-line, otherwise the kernel will get confused. There is no way to automate this for the user.

And that's it. If this is wrong, please be detailed in what I have wrong, because I can't guess any more what you want.

So I have one concern:

My patch has a function called diu_get_video_params() which is like video_get_params(), but it returns a lot more information. video_get_params() is currently used by several other drivers, none of which I can test. I'm very hesitant to rewrite video_get_params() so that it returns the same data that diu_get_video_params() returns, because I can't test these other boards.

Timur Tabi
Linux kernel developer at Freescale