[U-Boot] [PATCH v2 2/4] nand: lpc32xx: add hardware ECC support

[Marek Vasut]

On Wednesday, July 29, 2015 at 06:14:08 PM, slemieux.tyco at gmail.com wrote:

Hi!

[...]

> +#if defined(CONFIG_DMA_LPC32XX) && !defined(CONFIG_SPL_BUILD)
> +/* Prepares DMA descriptors for NAND RD/WR operations */
> +/* If the size is < 256 Bytes then it is assumed to be
> + * an OOB transfer */
> +static void lpc32xx_nand_dma_configure(struct nand_chip *chip,
> +				       const void *buffer, int size, int read)
> +{
> +	uint32_t i, dmasrc, ctrl, ecc_ctrl, oob_ctrl, dmadst;
> +	void __iomem *base = chip->IO_ADDR_R;
> +	uint32_t *ecc_gen = ecc_buffer;
> +
> +	/* CTRL descriptor entry for reading ECC
> +	 * Copy Multiple times to sync DMA with Flash Controller
> +	 */
> +	ecc_ctrl =  (0x5 |
> +			DMAC_CHAN_SRC_BURST_1 |
> +			DMAC_CHAN_DEST_BURST_1 |
> +			DMAC_CHAN_SRC_WIDTH_32 |
> +			DMAC_CHAN_DEST_WIDTH_32 |
> +			DMAC_CHAN_DEST_AHB1);

The outer parenthesis are not needed.

> +	/* CTRL descriptor entry for reading/writing Data */
> +	ctrl =  64 | /* 256/4 */
> +			DMAC_CHAN_SRC_BURST_4 |
> +			DMAC_CHAN_DEST_BURST_4 |
> +			DMAC_CHAN_SRC_WIDTH_32 |
> +			DMAC_CHAN_DEST_WIDTH_32 |
> +			DMAC_CHAN_DEST_AHB1;

[...]

> +	for (i = 0; i < size/256; i++) {

You might want to introduce a variable here to avoid this constant 
dma_list[i*2]. Also, fix the datatypes to zap the insane casts all
around.

> +		dmalist[i*2].dma_src = (read ? (dmasrc) : (dmasrc + (i*256)));
> +		dmalist[i*2].dma_dest = (read ? (dmadst + (i*256)) : dmadst);
> +		dmalist[i*2].next_lli = (uint32_t)&dmalist[(i*2)+1];
> +		dmalist[i*2].next_ctrl = ctrl;
> +
> +		dmalist[(i*2) + 1].dma_src =
> +			(uint32_t)(base + offsetof(struct lpc32xx_nand_slc_regs,
> +						   ecc));
> +		dmalist[(i*2) + 1].dma_dest = (uint32_t)&ecc_gen[i];
> +		dmalist[(i*2) + 1].next_lli = (uint32_t)&dmalist[(i*2)+2];
> +		dmalist[(i*2) + 1].next_ctrl = ecc_ctrl;
> +	}
> +
> +	if (i) { /* Data only transfer */
> +		dmalist[(i*2) - 1].next_lli = 0;
> +		dmalist[(i*2) - 1].next_ctrl |= DMAC_CHAN_INT_TC_EN;
> +		return;
> +	}
> +
> +	/* OOB only transfer */
> +	if (read) {
> +		dmasrc = (uint32_t)(base +
> +				    offsetof(struct lpc32xx_nand_slc_regs,
> +						    dma_data));
> +		dmadst = (uint32_t)(buffer);
> +		oob_ctrl |= DMAC_CHAN_DEST_AUTOINC;
> +	} else {
> +		dmadst = (uint32_t)(base +
> +				    offsetof(struct lpc32xx_nand_slc_regs,
> +						    dma_data));
> +		dmasrc = (uint32_t)(buffer);
> +		oob_ctrl |= DMAC_CHAN_SRC_AUTOINC;
> +	}
> +
> +	/* Read/ Write Spare Area Data To/From Flash */
> +	dmalist[i*2].dma_src = dmasrc;
> +	dmalist[i*2].dma_dest = dmadst;
> +	dmalist[i*2].next_lli = 0;
> +	dmalist[i*2].next_ctrl = (oob_ctrl | DMAC_CHAN_INT_TC_EN);
> +}

[...]

>  static void lpc32xx_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
>  {
> +#if defined(CONFIG_DMA_LPC32XX) && !defined(CONFIG_SPL_BUILD)

I'm not a big fan of those ifdefs -- why not use DMA even in SPL ?

> +	lpc32xx_nand_xfer(mtd, buf, len, 1);
> +#else
>  	while (len-- > 0)
>  		*buf++ = readl(&lpc32xx_nand_slc_regs->data);
> +#endif
>  }