[RFC 1/8] TILER-DMM: DMM-PAT driver for TI TILER

Hi Laurent, 
Thanks for taking the time to review.  These are all very good comments and I will work on incorporating them into the next RFC version.
 
-David

-----Original Message-----
From: Laurent Pinchart [mailto:laurent.pinchart at ideasonboard.com] 
Sent: Monday, July 26, 2010 3:13 AM
To: linux-arm-kernel at lists.infradead.org
Cc: Sin, David; Molnar, Lajos
Subject: Re: [RFC 1/8] TILER-DMM: DMM-PAT driver for TI TILER

Hi David,

On Saturday 24 July 2010 01:28:07 David Sin wrote:

Why the double _ ?

+#define DMM_TILER_HWINFO      0x208
+#define DMM_TILER_OR__0       0x220
+#define DMM_TILER_OR__1       0x224
+#define DMM_PAT_HWINFO        0x408
+#define DMM_PAT_GEOMETRY      0x40C
+#define DMM_PAT_CONFIG        0x410
+#define DMM_PAT_VIEW__0       0x420
+#define DMM_PAT_VIEW__1       0x424
+#define DMM_PAT_VIEW_MAP__0   0x440
+#define DMM_PAT_VIEW_MAP_BASE 0x460
+#define DMM_PAT_IRQ_EOI       0x478
+#define DMM_PAT_IRQSTATUS_RAW 0x480
+#define DMM_PAT_IRQSTATUS     0x490
+#define DMM_PAT_IRQENABLE_SET 0x4A0
+#define DMM_PAT_IRQENABLE_CLR 0x4B0
+#define DMM_PAT_STATUS__0     0x4C0
+#define DMM_PAT_STATUS__1     0x4C4
+#define DMM_PAT_STATUS__2     0x4C8
+#define DMM_PAT_STATUS__3     0x4CC
+#define DMM_PAT_DESCR__0      0x500
+#define DMM_PAT_AREA__0       0x504
+#define DMM_PAT_CTRL__0       0x508
+#define DMM_PAT_DATA__0       0x50C
+#define DMM_PEG_HWINFO        0x608
+#define DMM_PEG_PRIO          0x620
+#define DMM_PEG_PRIO_PAT      0x640
+
+/**
+ * PAT refill programming mode.
+ */
+enum pat_mode {
+	MANUAL,
+	AUTO
+};

MANUAL and AUTO are pretty generic names to have in the top-level scope. 
Please at least rename them to PAT_MANUAL and PAT_AUTO.

Even better, the pat_ prefix might be better called dmm_pat_, as PAT is 
already used for Physical Address Extension on x86 (granted, the DMM-PAT is 
not likely to be used on x86 platforms).

An explanation of the DMM and PAT acronyms would be nice as well.

+
+/**
+ * Area definition for DMM physical address translator.
+ */
+struct pat_area {
+	s32 x0:8;
+	s32 y0:8;
+	s32 x1:8;
+	s32 y1:8;

What's wrong with four u8 fields ?

Why do you need a platform_driver if it's completely empty ? Instead of using 
the hardcoded DMM_BASE and DMM_SIZE constants in the driver, you should 
implement a probe function and retrieve them from the platform device 
resources.

+s32 dmm_pat_refill(struct dmm *dmm, struct pat *pd, enum pat_mode mode)
+{
+	void __iomem *r;
+	u32 v;
+
+	/* Only manual refill supported */
+	if (mode != MANUAL)
+		return -EFAULT;
+
+	/* Check that the DMM_PAT_STATUS register has not reported an error */
+	r = dmm->base + DMM_PAT_STATUS__0;
+	v = __raw_readl(r);

Adding dmm_read and dmm_write macros that compute the address using the base 
and offset would make the code simpler. You could also define dmm_clear_bits 
and dmm_set_bits macros.

+	if (WARN(v & 0xFC00, KERN_ERR "dmm_pat_refill() error.\n"))

Can you define constants for the field's bits and use them instead of the 
hardcoded 0xfc00 ?

Same comment for the other values you use in the driver (both the register 
values, field numbers, shift constants, ...).

+		return -EIO;
+
+	/* Set "next" register to NULL */
+	r = dmm->base + DMM_PAT_DESCR__0;
+	v = __raw_readl(r);
+	v = SET_FLD(v, 31, 4, (u32) NULL);
+	__raw_writel(v, r);
+
+	/* Set area to be refilled */
+	r = dmm->base + DMM_PAT_AREA__0;
+	v = __raw_readl(r);
+	v = SET_FLD(v, 30, 24, pd->area.y1);
+	v = SET_FLD(v, 23, 16, pd->area.x1);
+	v = SET_FLD(v, 14, 8, pd->area.y0);
+	v = SET_FLD(v, 7, 0, pd->area.x0);
+	__raw_writel(v, r);
+	wmb();
+
+	/* First, clear the DMM_PAT_IRQSTATUS register */
+	r = dmm->base + DMM_PAT_IRQSTATUS;
+	__raw_writel(0xFFFFFFFF, r);
+	wmb();
+
+	r = dmm->base + DMM_PAT_IRQSTATUS_RAW;
+	while (__raw_readl(r) != 0)
+		;

How long is this expected to last ? Shouldn't you add a timeout ?

+	/* Fill data register */
+	r = dmm->base + DMM_PAT_DATA__0;
+	v = __raw_readl(r);
+
+	v = SET_FLD(v, 31, 4, pd->data >> 4);
+	__raw_writel(v, r);
+	wmb();
+
+	/* Read back PAT_DATA__0 to see if write was successful */
+	while (__raw_readl(r) != pd->data)
+		;

Do you really need to read back every register after writing them ?

+	r = dmm->base + DMM_PAT_CTRL__0;
+	v = __raw_readl(r);
+	v = SET_FLD(v, 31, 28, pd->ctrl.ini);
+	v = SET_FLD(v, 16, 16, pd->ctrl.sync);
+	v = SET_FLD(v, 9, 8, pd->ctrl.lut_id);
+	v = SET_FLD(v, 6, 4, pd->ctrl.dir);
+	v = SET_FLD(v, 0, 0, pd->ctrl.start);
+	__raw_writel(v, r);
+	wmb();
+
+	/* Check if PAT_IRQSTATUS_RAW is set after the PAT has been refilled */
+	r = dmm->base + DMM_PAT_IRQSTATUS_RAW;
+	while ((__raw_readl(r) & 0x3) != 0x3)
+		;
+
+	/* Again, clear the DMM_PAT_IRQSTATUS register */
+	r = dmm->base + DMM_PAT_IRQSTATUS;
+	__raw_writel(0xFFFFFFFF, r);
+	wmb();
+
+	r = dmm->base + DMM_PAT_IRQSTATUS_RAW;
+	while (__raw_readl(r) != 0)
+		;
+
+	/* Again, set "next" register to NULL to clear any PAT STATUS errors */
+	r = dmm->base + DMM_PAT_DESCR__0;
+	v = __raw_readl(r);
+	v = SET_FLD(v, 31, 4, (u32) NULL);
+	__raw_writel(v, r);
+
+	/*
+	 * Now, check that the DMM_PAT_STATUS register
+	 * has not reported an error before exiting.
+	*/
+	r = dmm->base + DMM_PAT_STATUS__0;
+	v = __raw_readl(r);
+	if (WARN(v & 0xFC00, KERN_ERR "dmm_pat_refill() error.\n"))
+		return -EIO;
+
+	return 0;
+}
+EXPORT_SYMBOL(dmm_pat_refill);
+
+struct dmm *dmm_pat_init(u32 id)
+{
+	u32 base;
+	struct dmm *dmm;
+	switch (id) {
+	case 0:
+		/* only support id 0 for now */
+		base = DMM_BASE;
+		break;
+	default:
+		return NULL;
+	}

Shouldn't you protect against multiple driver trying to initialize the same 
DMM-PAT ?

+	dmm = kzalloc(sizeof(*dmm), GFP_KERNEL);
+	if (!dmm)
+		return NULL;
+
+	dmm->base = ioremap(base, DMM_SIZE);
+	if (!dmm->base) {
+		kfree(dmm);
+		return NULL;
+	}
+
+	__raw_writel(0x88888888, dmm->base + DMM_PAT_VIEW__0);
+	__raw_writel(0x88888888, dmm->base + DMM_PAT_VIEW__1);
+	__raw_writel(0x80808080, dmm->base + DMM_PAT_VIEW_MAP__0);
+	__raw_writel(0x80000000, dmm->base + DMM_PAT_VIEW_MAP_BASE);
+	__raw_writel(0x88888888, dmm->base + DMM_TILER_OR__0);
+	__raw_writel(0x88888888, dmm->base + DMM_TILER_OR__1);
+
+	return dmm;
+}
+EXPORT_SYMBOL(dmm_pat_init);
+
+/**
+ * Clean up the physical address translator.
+ * @param dmm    Device data
+ * @return an error status.
+ */
+void dmm_pat_release(struct dmm *dmm)
+{
+	if (dmm) {
+		iounmap(dmm->base);
+		kfree(dmm);
+	}
+}
+EXPORT_SYMBOL(dmm_pat_release);
+
+static s32 __init dmm_init(void)
+{
+	return platform_driver_register(&dmm_driver_ldm);
+}
+
+static void __exit dmm_exit(void)
+{
+	platform_driver_unregister(&dmm_driver_ldm);
+}
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("davidsin at ti.com");
+MODULE_AUTHOR("molnar at ti.com");
+module_init(dmm_init);
+module_exit(dmm_exit);

-- 
Regards,

Laurent Pinchart