Re: [PATCH 11/11][v2] ppc440spe-adma: ADMA driver for PPC440SP(e) systems

Hello Yuri,

On Tue, Jan 13, 2009 at 03:43:55AM +0300, Yuri Tikhonov wrote:

Adds the platform device definitions and the architecture specific support
routines for the ppc440spe adma driver.

Any board equipped with PPC440SP(e) controller may utilize this driver.

Signed-off-by: Yuri Tikhonov <redacted>
Signed-off-by: Ilya Yanok <redacted>
---

Quite complex and interesting driver, I must say.
Have you thought about splitting ppc440spe-adma.c into multiple
files, btw?


A few comments down below...

[...]

+typedef struct ppc440spe_adma_device {

Please avoid typedefs.

[...]

+/*
+ * Descriptor of allocated CDB
+ */
+typedef struct {
+	dma_cdb_t		*vaddr;	/* virtual address of CDB */
+	dma_addr_t		paddr;	/* physical address of CDB */
+	/*
+	 * Additional fields
+	 */
+	struct list_head 	link;	/* link in processing list */
+	u32			status;	/* status of the CDB */
+	/* status bits:  */
+	#define	DMA_CDB_DONE	(1<<0)	/* CDB processing competed */
+	#define DMA_CDB_CANCEL	(1<<1)	/* waiting thread was interrupted */
+} dma_cdbd_t;

It seems there are no users of this struct.

[...]

+typedef struct {
+	xor_cb_t		*vaddr;
+	dma_addr_t		paddr;
+
+	/*
+	 * Additional fields
+	 */
+	struct list_head	link;	/* link to processing CBs */
+	u32			status;	/* status of the CB */
+	/* status bits: */
+	#define XOR_CB_DONE	(1<<0)	/* CB processing competed */
+	#define XOR_CB_CANCEL	(1<<1)	/* waiting thread was interrupted */
+#if 0
+	#define XOR_CB_STALLOC	(1<<2)	/* CB allocated statically */
+#endif

Dead code.

[...]

+static void ppc440spe_configure_raid_devices(void)
+{
+	void *fifo_buf;
+	volatile i2o_regs_t *i2o_reg;
+	volatile dma_regs_t *dma_reg0, *dma_reg1;
+	volatile xor_regs_t *xor_reg;
+	u32 mask;
+
+	/*
+	 * Map registers and allocate fifo buffer
+	 */
+	if (!(i2o_reg  = ioremap(I2O_MMAP_BASE, I2O_MMAP_SIZE))) {
+		printk(KERN_ERR "I2O registers mapping failed.\n");
+		return;
+	}

i2o_reg  = ioremap(I2O_MMAP_BASE, I2O_MMAP_SIZE);
if (!i20_reg)
	...;

would look better.

+	if (!(dma_reg0 = ioremap(DMA0_MMAP_BASE, DMA_MMAP_SIZE))) {
+		printk(KERN_ERR "DMA0 registers mapping failed.\n");
+		goto err1;
+	}
+	if (!(dma_reg1 = ioremap(DMA1_MMAP_BASE, DMA_MMAP_SIZE))) {
+		printk(KERN_ERR "DMA1 registers mapping failed.\n");
+		goto err2;
+	}
+	if (!(xor_reg  = ioremap(XOR_MMAP_BASE,XOR_MMAP_SIZE))) {
+		printk(KERN_ERR "XOR registers mapping failed.\n");
+		goto err3;
+	}

[...]

It's a tristate, but module_exit() disabled with #if 0...

[...]

+/******************************************************************************
+ * Command (Descriptor) Blocks low-level routines
+ ******************************************************************************/
+/**
+ * ppc440spe_desc_init_interrupt - initialize the descriptor for INTERRUPT
+ * pseudo operation
+ */
+static inline void ppc440spe_desc_init_interrupt (ppc440spe_desc_t *desc,
+							ppc440spe_ch_t *chan)
+{

Isn't this function way too big for inline?

+	xor_cb_t *p;
+
+	switch (chan->device->id) {
+	case PPC440SPE_XOR_ID:
+		p = desc->hw_desc;
+		memset (desc->hw_desc, 0, sizeof(xor_cb_t));
+		/* NOP with Command Block Complete Enable */
+		p->cbc = XOR_CBCR_CBCE_BIT;
+		break;
+	case PPC440SPE_DMA0_ID:
+	case PPC440SPE_DMA1_ID:
+		memset (desc->hw_desc, 0, sizeof(dma_cdb_t));
+		/* NOP with interrupt */
+		set_bit(PPC440SPE_DESC_INT, &desc->flags);
+		break;
+	default:
+		printk(KERN_ERR "Unsupported id %d in %s\n", chan->device->id,
+				__func__);
+		break;
+	}
+}
+
+/**
+ * ppc440spe_desc_init_null_xor - initialize the descriptor for NULL XOR
+ * pseudo operation
+ */
+static inline void ppc440spe_desc_init_null_xor(ppc440spe_desc_t *desc)
+{

I'd drop the inline.

+	memset (desc->hw_desc, 0, sizeof(xor_cb_t));
+	desc->hw_next = NULL;
+	desc->src_cnt = 0;
+	desc->dst_cnt = 1;
+}
+
+/**
+ * ppc440spe_desc_init_xor - initialize the descriptor for XOR operation
+ */
+static inline void ppc440spe_desc_init_xor(ppc440spe_desc_t *desc, int src_cnt,
+		unsigned long flags)
+{

Ditto.

+	xor_cb_t *hw_desc = desc->hw_desc;
+
+	memset (desc->hw_desc, 0, sizeof(xor_cb_t));
+	desc->hw_next = NULL;
+	desc->src_cnt = src_cnt;
+	desc->dst_cnt = 1;
+
+	hw_desc->cbc = XOR_CBCR_TGT_BIT | src_cnt;
+	if (flags & DMA_PREP_INTERRUPT)
+		/* Enable interrupt on complete */
+		hw_desc->cbc |= XOR_CBCR_CBCE_BIT;
+}
+
+/**
+ * ppc440spe_desc_init_dma2pq - initialize the descriptor for PQ
+ * operation in DMA2 controller
+ */
+static inline void ppc440spe_desc_init_dma2pq(ppc440spe_desc_t *desc,
+		int dst_cnt, int src_cnt, unsigned long flags)
+{

Ditto.

+	xor_cb_t *hw_desc = desc->hw_desc;
+
+	memset (desc->hw_desc, 0, sizeof(xor_cb_t));
+	desc->hw_next = NULL;
+	desc->src_cnt = src_cnt;
+	desc->dst_cnt = dst_cnt;
+	memset (desc->reverse_flags, 0, sizeof (desc->reverse_flags));
+	desc->descs_per_op = 0;
+
+	hw_desc->cbc = XOR_CBCR_TGT_BIT;
+	if (flags & DMA_PREP_INTERRUPT)
+		/* Enable interrupt on complete */
+		hw_desc->cbc |= XOR_CBCR_CBCE_BIT;
+}
+
+/**
+ * ppc440spe_desc_init_dma01pq - initialize the descriptors for PQ operation
+ * qith DMA0/1
+ */
+static inline void ppc440spe_desc_init_dma01pq(ppc440spe_desc_t *desc,
+		int dst_cnt, int src_cnt, unsigned long flags,
+		unsigned long op)
+{

Way to big for inline. The same for all the inlines.

Btw, ppc_async_tx_find_best_channel() looks too big for inline
and also too big to be in a .h file.

+	dma_cdb_t *hw_desc;
+	ppc440spe_desc_t *iter;
+	u8 dopc;
+
+	/* Common initialization of a PQ descriptors chain */
+	set_bits(op, &desc->flags);
+	desc->src_cnt = src_cnt;
+	desc->dst_cnt = dst_cnt;
+
+	/* WXOR MULTICAST if both P and Q are being computed
+	 * MV_SG1_SG2 if Q only
+	 */
+	dopc = (desc->dst_cnt == DMA_DEST_MAX_NUM) ?
+		DMA_CDB_OPC_MULTICAST : DMA_CDB_OPC_MV_SG1_SG2;
+
+	list_for_each_entry(iter, &desc->group_list, chain_node) {
+		hw_desc = iter->hw_desc;
+		memset (iter->hw_desc, 0, sizeof(dma_cdb_t));
+
+		if (likely(!list_is_last(&iter->chain_node,
+				&desc->group_list))) {
+			/* set 'next' pointer */
+			iter->hw_next = list_entry(iter->chain_node.next,
+				ppc440spe_desc_t, chain_node);
+			clear_bit(PPC440SPE_DESC_INT, &iter->flags);
+		} else {
+			/* this is the last descriptor.
+			 * this slot will be pasted from ADMA level
+			 * each time it wants to configure parameters
+			 * of the transaction (src, dst, ...)
+			 */
+			iter->hw_next = NULL;
+			if (flags & DMA_PREP_INTERRUPT)
+				set_bit(PPC440SPE_DESC_INT, &iter->flags);
+			else
+				clear_bit(PPC440SPE_DESC_INT, &iter->flags);
+		}
+	}
+
+	/* Set OPS depending on WXOR/RXOR type of operation */
+	if (!test_bit(PPC440SPE_DESC_RXOR, &desc->flags)) {
+		/* This is a WXOR only chain:
+		 * - first descriptors are for zeroing destinations
+		 *   if PPC440SPE_ZERO_P/Q set;
+		 * - descriptors remained are for GF-XOR operations.
+		 */
+		iter = list_first_entry(&desc->group_list,
+					ppc440spe_desc_t, chain_node);
+
+		if (test_bit(PPC440SPE_ZERO_P, &desc->flags)) {
+			hw_desc = iter->hw_desc;
+			hw_desc->opc = DMA_CDB_OPC_MV_SG1_SG2;
+			iter = list_first_entry(&iter->chain_node,
+					ppc440spe_desc_t, chain_node);
+		}
+
+		if (test_bit(PPC440SPE_ZERO_Q, &desc->flags)) {
+			hw_desc = iter->hw_desc;
+			hw_desc->opc = DMA_CDB_OPC_MV_SG1_SG2;
+			iter = list_first_entry(&iter->chain_node,
+					ppc440spe_desc_t, chain_node);
+		}
+
+		list_for_each_entry_from(iter, &desc->group_list, chain_node) {
+			hw_desc = iter->hw_desc;
+			hw_desc->opc = dopc;
+		}
+	} else {
+		/* This is either RXOR-only or mixed RXOR/WXOR */
+
+		/* The first 1 or 2 slots in chain are always RXOR,
+		 * if need to calculate P & Q, then there are two
+		 * RXOR slots; if only P or only Q, then there is one
+		 */
+		iter = list_first_entry(&desc->group_list,
+					ppc440spe_desc_t, chain_node);
+		hw_desc = iter->hw_desc;
+		hw_desc->opc = DMA_CDB_OPC_MV_SG1_SG2;
+
+		if (desc->dst_cnt == DMA_DEST_MAX_NUM) {
+			iter = list_first_entry(&iter->chain_node,
+						ppc440spe_desc_t, chain_node);
+			hw_desc = iter->hw_desc;
+			hw_desc->opc = DMA_CDB_OPC_MV_SG1_SG2;
+		}
+
+		/* The remain descs (if any) are WXORs */
+		if (test_bit(PPC440SPE_DESC_WXOR, &desc->flags)) {
+			iter = list_first_entry(&iter->chain_node,
+						ppc440spe_desc_t, chain_node);
+			list_for_each_entry_from(iter, &desc->group_list,
+						chain_node) {
+				hw_desc = iter->hw_desc;
+				hw_desc->opc = dopc;
+			}
+		}
+	}
+}

[...]

+/**
+ * ppc440spe_chan_xor_slot_count - get the number of slots necessary for
+ * XOR operation

IIRC kerneldoc does not permit multiline short descriptions. Kdoc tools
will warn about it, I presume.

[...]

+static int ppc440spe_test_raid6 (ppc440spe_ch_t *chan)
+{
+	ppc440spe_desc_t *sw_desc, *iter;
+	struct page *pg;
+	char *a;
+	dma_addr_t dma_addr, addrs[2];
+	unsigned long op = 0;
+	int rval = 0;
+
+	/*FIXME*/

?

+
+	set_bit(PPC440SPE_DESC_WXOR, &op);
+
+	pg = alloc_page(GFP_KERNEL);
+	if (!pg)
+		return -ENOMEM;
+

+
+/**
+ * ppc440spe_adma_probe - probe the asynch device
+ */
+static int __devinit ppc440spe_adma_probe(struct platform_device *pdev)
+{
+	struct resource *res;

Why is this a platform driver? What's the point of describing
DMA nodes in the device tree w/o actually using them (don't count
interrupts)? There are a lot of hard-coded addresses in the code...
:-/

And arch/powerpc/platforms/44x/ppc440spe_dma_engines.c file
reminds me arch/ppc-style bindings. ;-)

+	int ret=0, irq1, irq2, initcode = PPC_ADMA_INIT_OK;
+	void *regs;
+	ppc440spe_dev_t *adev;
+	ppc440spe_ch_t *chan;
+	ppc440spe_aplat_t *plat_data;
+	struct ppc_dma_chan_ref *ref;
+	struct device_node *dp;
+	char s[10];
+

[...]

+static int __init ppc440spe_adma_init (void)
+{
+	int rval, i;
+	struct proc_dir_entry *p;
+
+	for (i = 0; i < PPC440SPE_ADMA_ENGINES_NUM; i++)
+		ppc_adma_devices[i] = -1;
+
+	rval = platform_driver_register(&ppc440spe_adma_driver);
+
+	if (rval == 0) {
+		/* Create /proc entries */
+		ppc440spe_proot = proc_mkdir(PPC440SPE_R6_PROC_ROOT, NULL);
+		if (!ppc440spe_proot) {
+			printk(KERN_ERR "%s: failed to create %s proc "
+			    "directory\n",__func__,PPC440SPE_R6_PROC_ROOT);
+			/* User will not be able to enable h/w RAID-6 */
+			return rval;
+		}

/proc? Why /proc? The driver has nothing to do with Linux VM subsystem
or processes. I think /sys/ interface would suit better for this, no?
Either way, userspace interfaces should be documented somehow
(probably Documentation/ABI/, or at least some comments in the
code).

+		/* GF polynome to use */
+		p = create_proc_entry("poly", 0, ppc440spe_proot);
+		if (p) {
+			p->read_proc = ppc440spe_poly_read;
+			p->write_proc = ppc440spe_poly_write;
+		}
+
+		/* RAID-6 h/w enable entry */
+		p = create_proc_entry("enable", 0, ppc440spe_proot);
+		if (p) {
+			p->read_proc = ppc440spe_r6ena_read;
+			p->write_proc = ppc440spe_r6ena_write;
+		}
+
+		/* initialization status */
+		p = create_proc_entry("devices", 0, ppc440spe_proot);
+		if (p) {
+			p->read_proc = ppc440spe_status_read;
+		}
+	}
+	return rval;
+}

[...]

+#if 0
+static void __exit ppc440spe_adma_exit (void)
+{
+	platform_driver_unregister(&ppc440spe_adma_driver);
+	return;
+}
+module_exit(ppc440spe_adma_exit);
+#endif

Dead code.


Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
`irc://irc.freenode.net/bd2