v2 patch for Freescale DIU driver

+int __init preallocate_diu_videomemory(void);

}
#endif
+

arch/powerpc/platforms/86xx/mpc8610_hpcd.c DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AL, 0x5229, quirk_uli5229); · line 187

Quoted 7 lines · On Wed, 19 Mar 2008 13:50:27 -0500 York Sun <[email protected]> wrote:

Add platform code to support Freescale DIU. The platform code includes framebuffer memory allocation, pixel format, monitor port, etc.

...

arch/powerpc/platforms/86xx/mpc8610_hpcd.c +2 -0

+unsigned int mpc8610hpcd_get_pixel_format
+	(unsigned int bits_per_pixel, int monitor_port)

Again, please do

unsigned int mpc8610hpcd_get_pixel_format(unsigned int bits_per_pixel,
int monitor_port)

(and anywhere else where this was done)

Quoted 1 line

+int __init preallocate_diu_videomemory(void);

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.

Andy, checkpatch missed this.

Quoted 4 lines

arch/powerpc/platforms/86xx/mpc8610_hpcd.c DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AL, 0x5229, quirk_uli5229); · line 187

Quoted 4 lines

...

arch/powerpc/platforms/86xx/mpc8610_hpcd.c +2 -0

+unsigned int mpc8610hpcd_get_pixel_format
+	(unsigned int bits_per_pixel, int monitor_port)

This seems like it might be detectable, does this seem like something we should try an report?

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.

...

(quoted diff) +6 -0

+static struct diu_pool pool;
+
+/*	To allocate memory for framebuffer. First try __get_free_pages(). If it
+ *	fails, try rh_alloc. The reason is __get_free_pages() cannot allocate
+ *	very large memory (more than 4MB). We don't want to allocate all memory
+ *	in rheap since small memory allocation/deallocation will fragment the

(quoted diff) +27 -0

+	if (virt) {
+		*phys = virt_to_bus(virt);
+		memset(virt, 0, size);
+	}
+
+	pr_debug("rh virt=%p phys=%lx\n", virt, *phys);
+
+	return virt;
+}
+
+void fsl_diu_free(void *p, unsigned long size)
+{
+	pr_debug("p=%p size=%lu\n", p, size);
+
+	if (!p)
+		return;
+
+	if ((p >= diu_ops.diu_mem) &&
+	    (p < (diu_ops.diu_mem + diu_ops.diu_size))) {
+		pr_debug("rh\n");
+		rh_free(&diu_ops.diu_rh_info, (unsigned long) p);
+	} else {
+		pr_debug("dma\n");
+		free_pages((unsigned long)p, get_order(size));
+	}
+}
+

(quoted diff) +8 -0

+{
+	unsigned long htotal, vtotal;
+	struct mfb_info *pmfbi, *cmfbi, *mfbi = info->par;
+	struct fsl_diu_data *machine_data = mfbi->parent;
+	int index = mfbi->index;
+
 
..
 
+static void update_lcdc(struct fb_info *info)
+{

(quoted diff) +21 -0

+	hw = dr.diu_reg;
+
+	if (mfbi->type == MFB_TYPE_OFF) {
+		fsl_diu_disable_panel(info);
+		return;
+	}
 
...
 
+static void request_irq_local(int irq)
+{
+	unsigned long status, ints;
+	struct diu *hw;
+
+	hw = dr.diu_reg;
+
+	/* Read to clear the status */
+	status = in_be32(&(hw->int_status));
+
+	if (request_irq(irq, fsl_diu_isr, 0, "diu", 0))
+		pr_info("Request diu IRQ failed.\n");
+	else {
+		ints = INT_PARERR | INT_LS_BF_VS;
+#if !defined(CONFIG_NOT_COHERENT_CACHE)

(quoted diff) +14 -0

+#ifdef CONFIG_PM
+/*
+ * Power management hooks. Note that we won't be called from IRQ context,
+ * unlike the blank functions above, so we may sleep.
+ */
+static int fsl_diu_suspend(struct of_device *dev, pm_message_t state)
+{
+	struct fsl_diu_data *machine_data;
+
+	machine_data = dev_get_drvdata(&ofdev->dev);
+	disable_lcdc(machine_data->fsl_diu_info[0]);
+
+	return 0;
+}

(quoted diff) +6 -0

+#endif				/* CONFIG_PM */
+
+/* Align to 64-bit(8-byte), 32-byte, etc. */
+static int allocate_buf(struct diu_addr *buf, u32 size, u32 bytes_align)
+{
+	u32 offset, ssize;

(quoted diff) +16 -0

+	mask = bytes_align - 1;
+	offset = (u32)buf->paddr & mask;
+	if (offset) {
+		buf->offset = bytes_align - offset;
+		buf->paddr = (u32)buf->paddr + offset;
+	} else
+		buf->offset = 0;
+	return 0;
+}
+
+static int fsl_diu_probe(struct of_device *ofdev,
+	const struct of_device_id *match)
+{
+	struct device_node *np = ofdev->node;
+	struct mfb_info *mfbi;
+	phys_addr_t dummy_ad_addr;

Review

Quoted 5 lines

(quoted diff) +4 -0

+static struct diu_hw dr = {
+	.mode = MFB_MODE1,
+	.reg_lock = __SPIN_LOCK_UNLOCKED(old_style_spin_init),
+};

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:

drivers/video/fsl-diu-fb.c renamed from drivers/video/fsl-diu-fb.c~fbdev-driver-for-freescale-8610-and-5121-diu-fix +0 -0

 static struct diu_pool pool;
 

Quoted 10 lines

(quoted diff) +10 -0

+static struct diu_pool pool;
+
+/*	To allocate memory for framebuffer. First try __get_free_pages(). If it
+ *	fails, try rh_alloc. The reason is __get_free_pages() cannot allocate
+ *	very large memory (more than 4MB). We don't want to allocate all memory
+ *	in rheap since small memory allocation/deallocation will fragment the
+ *	rheap and make the furture large allocation fail.
+ */
+
+void *fsl_diu_alloc(unsigned long size, phys_addr_t *phys)

Review

Quoted 6 lines

(quoted diff) +6 -0

+{
+	void *virt;
+
+	pr_debug("size=%lu\n", size);
+
+	virt = (void *)__get_free_pages(GFP_DMA | GFP_ATOMIC, get_order(size));

GFP_DMA implies GFP_ATOMIC, but it's appropriate for documentation purposes.

Review

Quoted 4 lines

(quoted diff) +4 -0

+static struct diu_hw dr = {
+	.mode = MFB_MODE1,
+	.reg_lock = __SPIN_LOCK_UNLOCKED(old_style_spin_init),
+};

I'm not clear on what's supposed to happen with __SPIN_LOCK_UNLOCKED(). I do know that its documentation is crap.

GFP_DMA implies GFP_ATOMIC, but it's appropriate for documentation purposes.

Review

Quoted 4 lines

(quoted diff) +4 -0

+	if (virt) {
+		*phys = virt_to_phys(virt);
+		pr_debug("virt %p, phys=%llx\n", virt, (uint64_t) *phys);
+		memset(virt, 0, size);

Could have used __GFP_ZERO, I guess.

Review

Quoted 1 line

(quoted diff) +1 -0

+	virt = (void *) rh_alloc(&diu_ops.diu_rh_info, size, "DIU");

hm, I'd have expected checkpatch to whine about the space after the cast there. Whatever.

please take a look, and please use checkpatch on all future patches.

Review

Quoted 9 lines

(quoted diff) +9 -0

+static void free_irq_local(int irq)
+{
+	struct diu *hw = dr.diu_reg;
+
+	/* Disable all LCDC interrupt */
+	out_be32(&(hw->int_mask), 0x1f);
+
+	free_irq(irq, 0);
+}

and the free_irq() will go splat?

Quoted 7 lines · Andrew Morton

Review

Quoted 4 lines

(quoted diff) +4 -0

+static struct diu_hw dr = {
+	.mode = MFB_MODE1,
+	.reg_lock = __SPIN_LOCK_UNLOCKED(old_style_spin_init),
+};

I'm not clear on what's supposed to happen with __SPIN_LOCK_UNLOCKED(). I do know that its documentation is crap.

Yes, "__SPIN_LOCK_UNLOCKED(old_style_spin_init)" is wrong. We'll fix it.

Review

Quoted 5 lines

static struct diu_hw dr = {

(quoted diff) +1 -1

 	.mode = MFB_MODE1,
-	.reg_lock = __SPIN_LOCK_UNLOCKED(old_style_spin_init),
+	.reg_lock = __SPIN_LOCK_UNLOCKED(diu_hw.reg_lock),
 };

Yes, this is better. Did you already make this change when you applied it to your -mm repo?

Quoted 1 line

GFP_DMA implies GFP_ATOMIC, but it's appropriate for documentation purposes.

So does that mean that "GFP_DMA | GFP_KERNEL" is always wrong?

If so, this
combination is used a lot in the kernel today.

Quoted 6 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+	if (virt) {
+		*phys = virt_to_phys(virt);
+		pr_debug("virt %p, phys=%llx\n", virt, (uint64_t) *phys);
+		memset(virt, 0, size);

Could have used __GFP_ZERO, I guess.

I had completely forgotten about __GFP_ZERO. Thanks.

Quoted 4 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	virt = (void *) rh_alloc(&diu_ops.diu_rh_info, size, "DIU");

hm, I'd have expected checkpatch to whine about the space after the cast there. Whatever.

I thought a space after a cast is the right thing to do?

Quoted 1 line

please take a look, and please use checkpatch on all future patches.

Sorry, we forgot to run it again after our second version of the patch.

Quoted 11 lines

Review

Quoted 9 lines

(quoted diff) +9 -0

+static void free_irq_local(int irq)
+{
+	struct diu *hw = dr.diu_reg;
+
+	/* Disable all LCDC interrupt */
+	out_be32(&(hw->int_mask), 0x1f);
+
+	free_irq(irq, 0);
+}

and the free_irq() will go splat?

Sorry, but I don't understand what's wrong with this code.

We'll make the other changes you've suggested and repost.

Quoted 2 lines

Quoted 1 line

GFP_DMA implies GFP_ATOMIC, but it's appropriate for documentation purposes.

So does that mean that "GFP_DMA | GFP_KERNEL" is always wrong?

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.

Don't wory about it ;)

Quoted 8 lines · Andrew Morton

Quoted 2 lines

Quoted 1 line

GFP_DMA implies GFP_ATOMIC, but it's appropriate for documentation purposes.

So does that mean that "GFP_DMA | GFP_KERNEL" is always wrong?

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.

Don't wory about it ;)

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?