This is the version 2 patch for Freescale DIU driver. Thanks for all the feedback.
The first patch is the driver. The second patch adds platform support for MPC8610HPCD board.

To compile, enable the "Freescale MPC8610/MPC5121 DIU framebuffer support" in the menuconfig.

--

The following features are supported:

plane 0 works as a regular frame buffer, can be accessed by /dev/fb0

plane 1 has two AOIs (area of interest), can be accessed by /dev/fb1 and /dev/fb2

plane 2 has two AOIs, can be accessed by /dev/fb3 and /dev/fb4

Special ioctls support AOIs
All /dev/fb* can be used as regular frame buffer devices, except hardware change can

only be made through /dev/fb0. Changing pixel clock has no effect on other fbs.
Limitation of usage of AOIs:

AOIs on the same plane can not be horizonally overlapped

AOIs have horizonal order, i.e. AOI0 should be always on top of AOI1

AOIs can not beyond phisical display area. Application should check AOI geometry

before changing physical resolution on /dev/fb0
required command line parameters to preallocate memory for frame buffer

diufb=15M
optional command line parameters to set modes and monitor

video=fslfb:[resolution][,bpp][,monitor]

Syntax:
Resolution

xres x yres-bpp@refresh_rate, the -bpp and @refresh_rate are optional

eg, 1024x768, 1280x1024, 1280x1024-32, 1280x1024@60, 1280x1024-32@60, 1280x480-32@60
Bpp

bpp=32, bpp=24, or bpp=16
Monitor

monitor=0, monitor=1, monitor=2

0 is DVI

1 is Single link LVDS

2 is Double link LVDS
Note: switching monitor is a board feather, not DIU feather. MPC8610HPCD has three

monitor ports to swtich to. MPC5121ADS doesn't have additional monitor port. So switching

monirot port for MPC5121ADS has no effect.
If compiled as a module, it takes pamameters mode, bpp, monitor with the same syntax above.
Signed-off-by: York Sun <yorksun@freescale.com>

Signed-off-by: Timur Tabi <timur@freescale.com>

---

This patch is targeting 2.6.26 kernel.
 Documentation/powerpc/booting-without-of.txt |   34 +

 arch/powerpc/boot/dts/mpc8610_hpcd.dts       |   13 +

 drivers/video/Kconfig                        |   10 +

 drivers/video/Makefile                       |    1 +

 drivers/video/fsl-diu-fb.c                   | 1735 ++++++++++++++++++++++++++

 drivers/video/fsl-d...
[ message continues ]

On Wed, 19 Mar 2008 13:50:26 -0500
I'm not clear on what's supposed to happen with __SPIN_LOCK_UNLOCKED().  I

do know that its documentation is crap.
I guess you should have used SPIN_LOCK_UNLOCKED there rather than

open-coding its equivalent.  And SPIN_LOCK_UNLOCKED _is_ documented.  It

says "don't use this".
Now, s/documentation/guesswork-and-grep/ says that you're supposed to pick

a tree-wide-unique string here as your lockdep key.
So I did this:
--- a/drivers/video/fsl-diu-fb.c~fbdev-driver-for-freescale-8610-and-5121-diu-fix

+++ a/drivers/video/fsl-diu-fb.c

@@ -274,7 +274,7 @@ static struct mfb_info mfb_template[] = 
 static struct diu_hw dr = {

 	.mode = MFB_MODE1,

-	.reg_lock = __SPIN_LOCK_UNLOCKED(old_style_spin_init),

+	.reg_lock = __SPIN_LOCK_UNLOCKED(diu_hw.reg_lock),

 };
hm, I'd have expected checkpatch to whine about the space after the cast

there.  Whatever.
checkpatch does turn up some significant problems:
WARNING: Use #include <linux/uaccess.h> instead of <asm/uaccess.h>

#205: FILE: drivers/video/fsl-diu-fb.c:32:

+#include <asm/uaccess.h>
WARNING: consider using strict_strtoul in preference to simple_strtoul

#1599: FILE: drivers/video/fsl-diu-fb.c:1426:

+       val = simple_strtoul(buf, last, 0);
WARNING: consider using strict_strtoul in preference to simple_strtoul

#1821: FILE: drivers/video/fsl-diu-fb.c:1648:

+                       val = simple_strtoul(opt + 8, NULL, 0);
WARNING: consider using strict_strtoul in preference to simple_strtoul

#1825: FILE: drivers/video/fsl-diu-fb.c:1652:

+                       default_bpp = simple_strtoul(opt + 4, NULL, 0);
Like this:
static int fsl_diu_check_var(struct fb_var_screeninfo *var,

				struct fb_info *info)
Conventionally we'll do
#else

#define fsl_diu_suspend NULL

#define fsl_diu_resume NULL
Please use ARRAY_SIZE() here, and in all the other places which open-code

--

Yes, this is better.  Did you already make this change when you applied it to
So does that mean that "GFP_DMA | GFP_KERNEL" is always wrong?  If so, this
Sorry, but I don't understand what's wrong with this code.
We'll make the other changes you've suggested and repost.
--

Timur Tabi

Linux kernel developer at Freescale

--

I did:
--- a/drivers/video/fsl-diu-fb.c~fbdev-driver-for-freescale-8610-and-5121-diu-fix

+++ a/drivers/video/fsl-diu-fb.c

@@ -274,7 +274,7 @@ static struct mfb_info mfb_template[] = 
 static struct diu_hw dr = {

 	.mode = MFB_MODE1,

-	.reg_lock = __SPIN_LOCK_UNLOCKED(old_style_spin_init),

+	.reg_lock = __SPIN_LOCK_UNLOCKED(dr.reg_lock),

 };
No, that's OK too.  It's just that GFP_DMA|GFP_ATOMIC is a bit redundant

and misleading.  GFP_DMA is already atomic; the only effect of adding

GFP_ATOMIC to GFP_DMA is to add __GFP_HIGH.
You snipped a bit.  Earlier, request_irq() failures were ignored.  So I

think there's a code path where free_irq_local() can free an IRQ which this
--

Well, maybe we don't want GFP_ATOMIC then, because I don't think we want

__GFP_HIGH.  Looking at the code, it appears the __GFP_HIGH has nothing to do

with HIGHMEM (which on PowerPC is the not 1-to-1 mapping memory from 0xF000000

to 0xFFFFFFFF).  Further examination of the cools shows the __GFP_HIGH says to

try access the "emergency pool", and I see this code snippet:
	if (alloc_flags & ALLOC_HIGH)

		min -= min / 2;
I guess this means that we reduce the amount of memory that can be available in

order for the allocate to succeed.
Considering that the amount of memory that we allocate is in the order of

megabytes, and it really isn't that important, I would think that we don't want

to touch the emergency pool.  Does that sound right?
--

Timur Tabi

Linux kernel developer at Freescale

--

On Mon, 24 Mar 2008 09:53:16 -0500
yup.  The absence of __GFP_WAIT already causes the page allocator to try a

bit harder.  Adding __GFP_HIGH would make it try harder still.
You do need to be sure that the driver will robustly and correctly recover

from an allocation failure here.
--

#define __SPIN_LOCK_UNLOCKED(lockname)
seems pretty suggestive to me, also DEFINE_SPINLOCK(x) shows the proper

usage; the right thing would have been:
static struct diu_hw dr = {

 	.mode = MFB_MODE1,

-	.reg_lock = __SPIN_LOCK_UNLOCKED(old_style_spin_init),

+	.reg_lock = __SPIN_LOCK_UNLOCKED(dr.reg_lock),

};
Where 'dr.reg_lock' is the expression to locate the actual object.
Does something like this make sense to you:
---

Subject: lockdep: document __SPIN_LOCK_UNLOCKED() usage
Small comment clarifying the intended usage of __SPIN_LOCK_UNLOCKED()
[ for the observant readers; yes this prescribes a usage that is more

  than strictly needed, it does however enable interesting future uses ]
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>

---

diff --git a/include/linux/spinlock_types.h b/include/linux/spinlock_types.h

index 68d88f7..4278558 100644

--- a/include/linux/spinlock_types.h

+++ b/include/linux/spinlock_types.h

@@ -63,6 +63,10 @@ typedef struct {

 # define RW_DEP_MAP_INIT(lockname)

 #endif
+/*

+ * __*_LOCK_UNLOCKED(lockname), where 'lockname' is a valid C expression

+ * that evaluates to the actual object being initialized.

+ */

 #ifdef CONFIG_DEBUG_SPINLOCK

 # define __SPIN_LOCK_UNLOCKED(lockname)					\

 	(spinlock_t)	{	.raw_lock = __RAW_SPIN_LOCK_UNLOCKED,	\

---
Perhaps I should make the macros do something like:
  typecheck(spinlock_t, lockname)
FWIW, I prefer the form: GFP_type | __GFP_modifiers
For instance: 'GFP_KERNEL | __GFP_DMA | __GFP_ZERO'
--

Add platform code to support Freescale DIU. The platform code includes

framebuffer memory allocation, pixel format, monitor port, etc.
Signed-off-by: York Sun <yorksun@freescale.com>

---

This patch is targeting 2.6.26 kernel.
 arch/powerpc/platforms/86xx/mpc8610_hpcd.c |  182 +++++++++++++++++++++++++++-

 arch/powerpc/sysdev/fsl_soc.c              |   41 ++++++

 arch/powerpc/sysdev/fsl_soc.h              |   22 ++++

 3 files changed, 243 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/86xx/mpc8610_hpcd.c b/arch/powerpc/platforms/86xx/mpc8610_hpcd.c

index 0b07485..e087aa3 100644

--- a/arch/powerpc/platforms/86xx/mpc8610_hpcd.c

+++ b/arch/powerpc/platforms/86xx/mpc8610_hpcd.c

@@ -3,6 +3,7 @@

  *

  * Initial author: Xianghua Xiao <x.xiao@freescale.com>

  * Recode: Jason Jin <jason.jin@freescale.com>

+ *         York Sun <yorksun@freescale.com>

  *

  * Rewrite the interrupt routing. remove the 8259PIC support,

  * All the integrated device in ULI use sideband interrupt.

@@ -38,6 +39,8 @@

 #include <sysdev/fsl_pci.h>

 #include <sysdev/fsl_soc.h>
+static unsigned char *pixis_bdcfg0, *pixis_arch;

+

 static struct of_device_id __initdata mpc8610_ids[] = {

 	{ .compatible = "fsl,mpc8610-immr", },

 	{}

@@ -161,12 +164,160 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AL, 0x5229, quirk_uli5229);

 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AL, 0x5288, final_uli5288);

 #endif /* CONFIG_PCI */
+#if defined(CONFIG_FB_FSL_DIU) || defined(CONFIG_FB_FSL_DIU_MODULE)

+

+static u32 get_busfreq(void)

+{

+	struct device_node *node;

+

+	u32 fs_busfreq = 0;

+	node = of_find_node_by_type(NULL, "cpu");

+	if (node) {

+		unsigned int size;

+		const unsigned int *prop =

+			of_get_property(node, "bus-frequency", &size);

+		if (prop)

+			fs_busfreq = *prop;

+		of_node_put(node);

+	};

+	return fs_busfreq;

+}

+

+unsigned int mpc8610hpcd_get_pixel_format

+	(unsigned int bits_per_pixel, int monitor_port)

+{

+	static const unsigned lon...
[ message continues ]

On Wed, 19 Mar 2008 13:50:27 -0500
Again, please do
unsigned int mpc8610hpcd_get_pixel_format(unsigned int bits_per_pixel,

					int monitor_port)
Nope, please don't put extern declarations in .c files.  Find a suitable

header for it - one which is included by the defining file and by all users

of the symbol.
--

This seems like it might be detectable, does this seem like something we

should try an report?
WARNING: arguments for function declarations should follow identifier

#7: FILE: Z110.c:7:
Yeah, we did only look for explicitly extern'd declarations.  But this

form seems detectable, will be in v0.17.
WARNING: externs should be avoided in .c files

#2: FILE: Z110.c:2:

+int __init preallocate_diu_videomemory(void);
-apw

--

On Tue, 25 Mar 2008 12:43:22 +0000
I guess so.  It's quite unusual.

--