video: Add new driver for Silicon Motion SM501/SM502 Part 1/2

[PATCH] video: Add new driver for Silicon Motion SM501/SM502

This patch adds a new driver for SM501/SM502. Compared to the existing driver it allows dynamic selection of resolution (environment: videomode).

The drive is based on Vincent Sanders and Ben Dooks' linux kernel driver.

Use CONFIG_VIDEO_SM501NEW to enable the driver.

The patch is against "latest" u-boot git-repository

Please (still) be patient if style of submission or patches are offending.

Signed-off-by Stefan Althoefer <[email protected]>

----

drivers/video/sm501new.h renamed from /drivers/video/sm501new.h +29 -0

+#define SM501FB_FLAG_USE_INIT_MODE	(1<<0)
+#define SM501FB_FLAG_DISABLE_AT_EXIT	(1<<1)
+#define SM501FB_FLAG_USE_HWCURSOR	(1<<2)
+#define SM501FB_FLAG_USE_HWACCEL	(1<<3)
+
+struct sm501_platdata_fbsub {
+	struct fb_videomode	*def_mode;
+	unsigned int		 def_bpp;
+	unsigned long		 max_mem;
+	unsigned int		 flags;
+};
+
+enum sm501_fb_routing {
+	SM501_FB_OWN		= 0,	/* CRT=>CRT, Panel=>Panel */
+	SM501_FB_CRT_PANEL	= 1,	/* Panel=>CRT, Panel=>Panel */
+};
+
+/* sm501_platdata_fb flag field bit definitions */
+
+#define SM501_FBPD_SWAP_FB_ENDIAN	(1<<0)	/* need to endian swap */
+
+/* sm501_platdata_fb
+ *
+ * configuration data for the framebuffer driver
+*/
+
+struct sm501_platdata_fb {
+	enum sm501_fb_routing		 fb_route;
+	unsigned int			 flags;

drivers/video/sm501new.h renamed from /drivers/video/sm501new.h +5 -0

+/* sm501_platdata
+ *
+ * This is passed with the platform device to allow the board
+ * to control the behaviour of the SM501 driver(s) which attach
+ * to the device.

diff -uprN u-boot-orig//drivers/video/sm501new-regs.h u-boot/drivers/video/sm501new-regs.h

Quoted 1 line

Use CONFIG_VIDEO_SM501NEW to enable the driver.

not sure if CONFIG_VIDEO_SM501NEW is a good chose here. Maybe we should use s.th. like CONFIG_VIDEO_SM50x. This applies to the file names too: sm50x.h, sm50x.c, etc. Even better would be a merge with the existing driver.

Quoted 29 lines

drivers/video/sm501new.h renamed from /drivers/video/sm501new.h +29 -0

+#define SM501FB_FLAG_USE_INIT_MODE	(1<<0)
+#define SM501FB_FLAG_DISABLE_AT_EXIT	(1<<1)
+#define SM501FB_FLAG_USE_HWCURSOR	(1<<2)
+#define SM501FB_FLAG_USE_HWACCEL	(1<<3)
+
+struct sm501_platdata_fbsub {
+	struct fb_videomode	*def_mode;
+	unsigned int		 def_bpp;
+	unsigned long		 max_mem;
+	unsigned int		 flags;
+};
+
+enum sm501_fb_routing {
+	SM501_FB_OWN		= 0,	/* CRT=>CRT, Panel=>Panel */
+	SM501_FB_CRT_PANEL	= 1,	/* Panel=>CRT, Panel=>Panel */
+};
+
+/* sm501_platdata_fb flag field bit definitions */
+
+#define SM501_FBPD_SWAP_FB_ENDIAN	(1<<0)	/* need to endian swap */
+
+/* sm501_platdata_fb
+ *
+ * configuration data for the framebuffer driver
+*/
+
+struct sm501_platdata_fb {
+	enum sm501_fb_routing		 fb_route;
+	unsigned int			 flags;

drivers/video/sm501new.h line 62

Quoted 10 lines

drivers/video/sm501new.h renamed from /drivers/video/sm501new.h +10 -0

+	struct sm501_platdata_fbsub	*fb_crt;
+	struct sm501_platdata_fbsub	*fb_pnl;
+};
+
+/* gpio i2c */
+
+struct sm501_platdata_gpio_i2c {
+	unsigned int		pin_sda;
+	unsigned int		pin_scl;
+};

all this stuff above starting from
"#define SM501FB_FLAG_USE_INIT_MODE"
can be removed if you remove "init_gpiop", "fb", "gpio_i2c" and "gpio_i2c_nr" from the "struct sm501_platdata" definition below. Also remove ".fb = &sm501_fb_pdata" from "sm501_pci_platdata" declaration in drivers/video/sm501new.c. These structure members aren't referenced in this U-Boot driver, so they aren't needed (unused and nearly dead code and data).

<snip>

drivers/video/sm501new.h line 83

Quoted 9 lines

drivers/video/sm501new.h renamed from /drivers/video/sm501new.h +9 -0

+#define SM501_USE_USB_HOST	(1<<0)
+#define SM501_USE_USB_SLAVE	(1<<1)
+#define SM501_USE_SSP0		(1<<2)
+#define SM501_USE_SSP1		(1<<3)
+#define SM501_USE_UART0		(1<<4)
+#define SM501_USE_UART1		(1<<5)
+#define SM501_USE_FBACCEL	(1<<6)
+#define SM501_USE_AC97		(1<<7)
+#define SM501_USE_I2S		(1<<8)

These macros aren't used, remove them too.

drivers/video/sm501new.h line 93

Quoted 10 lines

drivers/video/sm501new.h renamed from /drivers/video/sm501new.h +10 -0

+
+#define SM501_USE_ALL		(0xffffffff)
+
+struct sm501_initdata {
+	struct sm501_reg_init	gpio_low;
+	struct sm501_reg_init	gpio_high;
+	struct sm501_reg_init	misc_timing;
+	struct sm501_reg_init	misc_control;
+
+	unsigned long		devices;

"devices" member is only initialized in "sm501_pci_initdata" declaration and isn't referenced anywhere in the code, so another candidate to remove.

Quoted 5 lines

drivers/video/sm501new.h renamed from /drivers/video/sm501new.h +5 -0

+/* sm501_platdata
+ *
+ * This is passed with the platform device to allow the board
+ * to control the behaviour of the SM501 driver(s) which attach
+ * to the device.

drivers/video/sm501new.h line 124

Quoted 10 lines

drivers/video/sm501new.h renamed from /drivers/video/sm501new.h +10 -0

+ *
+*/
+
+struct sm501_platdata {
+	struct sm501_initdata		*init;
+	struct sm501_init_gpio		*init_gpiop;
+	struct sm501_platdata_fb	*fb;
+
+	struct sm501_platdata_gpio_i2c	*gpio_i2c;
+	unsigned int			 gpio_i2c_nr;

see above suggestion to remove init_gpiop, fb, gpio_i2c and gpio_i2c_nr.

Quoted 4 lines

diff -uprN u-boot-orig//drivers/video/sm501new-regs.h u-boot/drivers/video/sm501new-regs.h

drivers/video/sm501new-regs.h line 10

Quoted 10 lines

drivers/video/sm501new-regs.h renamed from /drivers/video/sm501new-regs.h +10 -0

@@ -0,0 +1,365 @@
+/* sm501-regs.h
+ *
+ * Copyright 2006 Simtec Electronics
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Silicon Motion SM501 register definitions
+*/

please, fix multi-line comment style.

Also check all below defined macros in this file. If some macro isn't used in the driver code, then remove it.

Quoted 13 lines

drivers/video/sm501new-regs.h renamed from /drivers/video/sm501new-regs.h +13 -0

+/* System Configuration area */
+/* System config base */
+#define SM501_SYS_CONFIG		(0x000000)
+
+/* config 1 */
+#define SM501_SYSTEM_CONTROL 		(0x000000)
+#define SM501_MISC_CONTROL		(0x000004)
+
+#define SM501_MISC_BUS_SH		(0x0)
+#define SM501_MISC_BUS_PCI		(0x1)
+#define SM501_MISC_BUS_XSCALE		(0x2)
+#define SM501_MISC_BUS_NEC		(0x6)
+#define SM501_MISC_BUS_MASK		(0x7)

<snip>

Quoted 2 lines

Use CONFIG_VIDEO_SM501NEW to enable the driver.

not sure if CONFIG_VIDEO_SM501NEW is a good chose here. Maybe we should use s.th. like CONFIG_VIDEO_SM50x. This applies to the file names too: sm50x.h, sm50x.c, etc. Even better would be a merge with the existing driver.

Dear Anatolij & Stefan,

Quoted 1 line · In message <[email protected]> you wrote:

Quoted 6 lines

Quoted 1 line

Use CONFIG_VIDEO_SM501NEW to enable the driver.

not sure if CONFIG_VIDEO_SM501NEW is a good chose here. Maybe we should use s.th. like CONFIG_VIDEO_SM50x. This applies to the file names too: sm50x.h, sm50x.c, etc. Even better would be a merge with the existing driver.

I agree with Anatolij here. A merge would definitely be best.

As for the SM502, it is a multi-function chip that has more than just video. I'm writing an I2C driver for it right now and I want to ask you guys for your opinion.

MPC8548 has _TWO_ I2C controllers and it is taken care of in fsl_i2c.c by defining a "bus number." There is a function that one uses to set a bus for consecutive operations with i2c_read() and friends (they do NOT take a bus number as a parameter.) That means one sets a bus and then all the operations are done on that bus until it is changed again.

This might be OK but it is definitely not portable and SOC-specific. In my

case I have a _THIRD_ I2C bus off of SM502... What do you think would be a best approach to address this:

a.) Rewrite _ALL_ I2C code to make those i2c_read() etc. functions to take
an additional parameter, bus number

b.) Make a global var i2c_bus or something and add a global function kinda
i2c_set_bus() so all i2c_read()/i2c_write() functions use that variable

c.) Add a third bus for SM502 I2C adapter to fsl_i2c.c (horrible hack)

d.) Make SM502 I2C a totally separate entity with its own set of functions

     (like sm502_i2c_read() etc.)

Please tell what you think. I personally lean towards option b.) but I might be wrong :)

And there is another useful part of SM502 that's begging for implementation -- USB controller with legacy KBD/Mouse support...

Dear ksi at koi8.net,

In message <[email protected]> you

Quoted 1 line · wrote:

Quoted 1 line

As for the SM502, it is a multi-function chip that has more than just

video.

Quoted 1 line

I'm writing an I2C driver for it right now and I want to ask you guys

for

Quoted 1 line

your opinion.

That's not new - the SM501 is pretty similar.

Quoted 1 line

MPC8548 has _TWO_ I2C controllers and it is taken care of in fsl_i2c.c

by

Quoted 1 line

defining a "bus number." There is a function that one uses to set a

bus for

Quoted 1 line

consecutive operations with i2c_read() and friends (they do NOT take a

bus

Quoted 2 lines

number as a parameter.) That means one sets a bus and then all the operations are done on that bus until it is changed again.

That's the general idea, also for example for device handling: we set a current device (or bus) with one command, and then continue to use that.

Quoted 1 line

This might be OK but it is definitely not portable and SOC-specific.

In my

I think the "this" here refers to the specific implementation, not the general approach, right?

Quoted 1 line

case I have a _THIRD_ I2C bus off of SM502... What do you think would

be a

Quoted 3 lines

best approach to address this:

a.) Rewrite _ALL_ I2C code to make those i2c_read() etc. functions to

take

Quoted 3 lines

an additional parameter, bus number

b.) Make a global var i2c_bus or something and add a global function

kinda

Quoted 1 line

i2c_set_bus() so all i2c_read()/i2c_write() functions use that

variable

Quoted 4 lines

c.) Add a third bus for SM502 I2C adapter to fsl_i2c.c (horrible hack)

d.) Make SM502 I2C a totally separate entity with its own set of

functions

Quoted 3 lines

     (like sm502_i2c_read() etc.)

Please tell what you think. I personally lean towards option b.) but I

might

Quoted 1 line

be wrong :)

I agree.

Quoted 1 line

And there is another useful part of SM502 that's begging for

implementation

Quoted 1 line

-- USB controller with legacy KBD/Mouse support...

Feel free to submit patches :-)

As a matter of fact, SM501 is an older, now nonexistent version of SM502.

Its support in U-Boot is rudimentary. I'd rather removed that old sm501.c altogether and replaced it with a new sm502.c. That should work for old SM501 chips if there are still any in existence.

Support is much better in Linux kernel -- it is almost fully supported out of the box, video, USB, sound etc.

Dear ksi at koi8.net,

In message <[email protected]> you

Quoted 2 lines · wrote:

As a matter of fact, SM501 is an older, now nonexistent version of

SM502.

Yeah. Tell that to some hardware companies who are shipping boards with the SM501 on it. :-)

Quoted 1 line

Its support in U-Boot is rudimentary. I'd rather removed that old

sm501.c

Quoted 1 line

altogether and replaced it with a new sm502.c. That should work for

old

Quoted 1 line

SM501 chips if there are still any in existence.

They are, and will be in existence at least for a couple of years (my expectation for couple is a two-digit number).

Quoted 1 line

Support is much better in Linux kernel -- it is almost fully supported

out

Quoted 1 line

of the box, video, USB, sound etc.

Linux is an OS - U-Boot is a mere boot loader ;-)

Dear Anatolij & Stefan,

Quoted 5 lines · In message <[email protected]> you wrote:

Quoted 1 line

Use CONFIG_VIDEO_SM501NEW to enable the driver.

not sure if CONFIG_VIDEO_SM501NEW is a good chose here. Maybe we should use s.th. like CONFIG_VIDEO_SM50x. This applies to the file names too: sm50x.h, sm50x.c, etc. Even better would be a merge with the existing driver.

I agree with Anatolij here. A merge would definitely be best.

Dear All,

Quoted 4 lines

Dear Anatolij & Stefan,

Quoted 1 line · In message <[email protected]> you wrote:

Quoted 1 line

Use CONFIG_VIDEO_SM501NEW to enable the driver.

Quoted 5 lines

Quoted 4 lines

not sure if CONFIG_VIDEO_SM501NEW is a good chose here. Maybe we should use s.th. like CONFIG_VIDEO_SM50x. This applies to the file names too: sm50x.h, sm50x.c, etc. Even better would be a merge with the existing driver.

I agree with Anatolij here. A merge would definitely be best.

Before I start to implement the good suggestions, I'd like to know how such a merge would look like in your opinion.

Over the long term, I see the old driver vanishing, as it is too complicated to use (you need to be a video and SMI register expert). It is not much more than a jump into the board specific setup routines. However, removing the old driver would break half a dozen of boards (inacceptable).