[PATCH] mtd: nand: pxa3xx_nand: add support for partial chunks

[Ezequiel Garcia]

Hi Thomas,

Thanks for working on this. Looks mostly good, just a couple minor
comments below.

Robert: Do you think you can take a look and test on PXA?

On 27 Jan 02:32 PM, Thomas Petazzoni wrote:
> This commit is needed to properly support the 8-bits ECC configuration
> with 4KB pages.
> 
> When pages larger than 2 KB are used on platforms using the PXA3xx
> NAND controller, the reading/programming operations need to be split
> in chunks of 2 KBs or less because the controller FIFO is limited to
> about 2 KB (i.e a bit more than 2 KB to accomodate OOB data). Due to
> this requirement, the data layout on NAND is a bit strange, with ECC
> interleaved with data, at the end of each chunk.
> 
> When a 4-bits ECC configuration is used with 4 KB pages, the physical
> data layout on the NAND looks like this:
> 
> | 2048 data | 32 spare | 30 ECC | 2048 data | 32 spare | 30 ECC |
> 
> So the data chunks have an equal size, 2080 bytes for each chunk,
> which the driver supports properly.
> 
> When a 8-bits ECC configuration is used with 4KB pages, the physical
> data layout on the NAND looks like this:
> 
> | 1024 data | 30 ECC | 1024 data | 30 ECC | 1024 data | 30 ECC | 1024 data | 30 ECC | 64 spare | 30 ECC |
> 
> So, the spare area is stored in its own chunk, which has a different
> size than the other chunks. Since OOB is not used by UBIFS, the initial
> implementation of the driver has chosen to not support reading this
> additional "spare" chunk of data.
> 
> Unfortunately, Marvell has chosen to store the BBT signature in the
> OOB area. Therefore, if the driver doesn't read this spare area, Linux
> has no way of finding the BBT. It thinks there is no BBT, and rewrites
> one, which U-Boot does not recognize, causing compability problem
> between the bootloader and the kernel in terms of NAND usage.
> 
> To fix this, this commit implements the support for reading a partial
> last chunk. This support is currently only useful for the case of 8
> bits ECC with 4 KB pages, but it will be useful in the future to
> enable other configurations such as 12 bits and 16 bits ECC with 4 KB
> pages, or 8 bits ECC with 8 KB pages, etc. All those configurations
> have a "last" chunk that doesn't have the same size as the other
> chunks.
> 
> In order to implement reading of the last chunk, this commit:
> 
>  - Adds a number of new fields to the pxa3xx_nand_info to describe how
>    many full chunks and how many chunks we have, the size of full
>    chunks and partial chunks, both in terms of data area and spare
>    area.
> 
>  - Fills in the step_chunk_size and step_spare_size variables to
>    describe how much data and spare should be read/written for the
>    current read/program step.
> 
>  - Reworks the state machine to accommodate doing the additional read
>    or program step when a last partial chunk is used.
> 
> This commit has been tested on a Marvell Armada 398 DB board, with a
> 4KB page NAND, tested in both 4 bits ECC and 8 bits ECC
> configurations. Testing on other platforms (with 512 bytes and 2 KB
> pages) would be useful.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni at free-electrons.com>
> ---
> This patch is really a fix, but since it is a quite complicated
> change, I don't think it should be pushed to the stable backports. The
> changes are too invasive to qualify as a stable backport IMO.
> 
>  drivers/mtd/nand/pxa3xx_nand.c | 147 ++++++++++++++++++++++++++---------------
>  1 file changed, 95 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c
> index 86fc245..4293ea7 100644
> --- a/drivers/mtd/nand/pxa3xx_nand.c
> +++ b/drivers/mtd/nand/pxa3xx_nand.c
> @@ -222,15 +222,44 @@ struct pxa3xx_nand_info {
>  	int			use_spare;	/* use spare ? */
>  	int			need_wait;
>  
> -	unsigned int		data_size;	/* data to be read from FIFO */
> -	unsigned int		chunk_size;	/* split commands chunk size */
> -	unsigned int		oob_size;
> +	/* Amount of real data per full chunk */
> +	unsigned int		chunk_size;
> +
> +	/* Amount of spare data per full chunk */
>  	unsigned int		spare_size;
> +
> +	/* Number of full chunks (i.e chunk_size + spare_size) */
> +	unsigned int            nfullchunks;
> +
> +	/*
> +	 * Total number of chunks. If equal to nfullchunks, then there
> +	 * are only full chunks. Otherwise, there is one last chunk of
> +	 * size (last_chunk_size + last_spare_size)
> +	 */
> +	unsigned int            ntotalchunks;
> +
> +	/* Amount of real data in the last chunk */
> +	unsigned int		last_chunk_size;
> +
> +	/* Amount of spare data in the last chunk */
> +	unsigned int		last_spare_size;
> +
>  	unsigned int		ecc_size;
>  	unsigned int		ecc_err_cnt;
>  	unsigned int		max_bitflips;
>  	int 			retcode;
>  
> +	/*
> +	 * Variables only valid during command
> +	 * execution. step_chunk_size and step_spare_size is the
> +	 * amount of real data and spare data in the current
> +	 * chunk. cur_chunk is the current chunk being
> +	 * read/programmed.
> +	 */
> +	unsigned int		step_chunk_size;
> +	unsigned int		step_spare_size;
> +	unsigned int            cur_chunk;
> +
>  	/* cached register value */
>  	uint32_t		reg_ndcr;
>  	uint32_t		ndtr0cs0;
> @@ -526,25 +555,6 @@ static int pxa3xx_nand_init(struct pxa3xx_nand_host *host)
>  	return 0;
>  }
>  
> -/*
> - * Set the data and OOB size, depending on the selected
> - * spare and ECC configuration.
> - * Only applicable to READ0, READOOB and PAGEPROG commands.
> - */
> -static void pxa3xx_set_datasize(struct pxa3xx_nand_info *info,
> -				struct mtd_info *mtd)
> -{
> -	int oob_enable = info->reg_ndcr & NDCR_SPARE_EN;
> -
> -	info->data_size = mtd->writesize;
> -	if (!oob_enable)
> -		return;
> -
> -	info->oob_size = info->spare_size;
> -	if (!info->use_ecc)
> -		info->oob_size += info->ecc_size;
> -}
> -
>  /**
>   * NOTE: it is a must to set ND_RUN firstly, then write
>   * command buffer, otherwise, it does not work.
> @@ -660,28 +670,26 @@ static void drain_fifo(struct pxa3xx_nand_info *info, void *data, int len)
>  
>  static void handle_data_pio(struct pxa3xx_nand_info *info)
>  {
> -	unsigned int do_bytes = min(info->data_size, info->chunk_size);
> -
>  	switch (info->state) {
>  	case STATE_PIO_WRITING:


>  		writesl(info->mmio_base + NDDB,
>  			info->data_buff + info->data_buff_pos,
> -			DIV_ROUND_UP(do_bytes, 4));
> +			DIV_ROUND_UP(info->step_chunk_size, 4));

Maybe we should check if step_chunk_size > 0 before calling writesl?
As far as I can see, we'll now call this to read spare only,
and it would be better not to rely in writesl checking the length.

>  
> -		if (info->oob_size > 0)
> +		if (info->step_spare_size)
>  			writesl(info->mmio_base + NDDB,
>  				info->oob_buff + info->oob_buff_pos,
> -				DIV_ROUND_UP(info->oob_size, 4));
> +				DIV_ROUND_UP(info->step_spare_size, 4));
>  		break;
>  	case STATE_PIO_READING:
>  		drain_fifo(info,
>  			   info->data_buff + info->data_buff_pos,
> -			   DIV_ROUND_UP(do_bytes, 4));
> +			   DIV_ROUND_UP(info->step_chunk_size, 4));
>  
> -		if (info->oob_size > 0)
> +		if (info->step_spare_size)
>  			drain_fifo(info,
>  				   info->oob_buff + info->oob_buff_pos,
> -				   DIV_ROUND_UP(info->oob_size, 4));
> +				   DIV_ROUND_UP(info->step_spare_size, 4));
>  		break;
>  	default:
>  		dev_err(&info->pdev->dev, "%s: invalid state %d\n", __func__,
> @@ -1236,22 +1259,30 @@ static void nand_cmdfunc_extended(struct mtd_info *mtd,
>  			break;
>  		}
>  
> +		/* Only a few commands new several steps */

s/new/need ?
 
> +		if (command != NAND_CMD_PAGEPROG &&
> +		    command != NAND_CMD_READ0    &&
> +		    command != NAND_CMD_READOOB)
> +			break;
> +
> +		info->cur_chunk++;
> +


-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar