Re: [PATCH RFC v4 3/4] nand: pl353: Add ONDIE ECC support

[Brian Norris]

Hi Punnaiah,

I haven't looked too closely at everything here, but a few comments:

On Mon, Jul 28, 2014 at 09:01:39PM +0530, Punnaiah Choudary Kalluri wrote:
> Added ONDIE ECC support. Currently this ecc mode is supported for
> specific micron parts with oob size 64 bytes.
> 
> Signed-off-by: Punnaiah Choudary Kalluri <punnaia@xxxxxxxxxx>
> ---
> Changes in v4:
> - Updated the driver to sync with pl353_smc driver APIs
> ---
>  drivers/mtd/nand/pl353_nand.c |  162 ++++++++++++++++++++++++++++++++++++-----
>  1 files changed, 144 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/mtd/nand/pl353_nand.c b/drivers/mtd/nand/pl353_nand.c
> index 93dc4ca..5c9b60d 100644
> --- a/drivers/mtd/nand/pl353_nand.c
> +++ b/drivers/mtd/nand/pl353_nand.c
> @@ -140,6 +140,48 @@ static struct nand_ecclayout nand_oob_64 = {
>  		 .length = 50} }
>  };
>  
> +static struct nand_ecclayout ondie_nand_oob_64 = {
> +	.eccbytes = 32,
> +
> +	.eccpos = {
> +		8, 9, 10, 11, 12, 13, 14, 15,
> +		24, 25, 26, 27, 28, 29, 30, 31,
> +		40, 41, 42, 43, 44, 45, 46, 47,
> +		56, 57, 58, 59, 60, 61, 62, 63
> +	},
> +
> +	.oobfree = {
> +		{ .offset = 4, .length = 4 },
> +		{ .offset = 20, .length = 4 },
> +		{ .offset = 36, .length = 4 },
> +		{ .offset = 52, .length = 4 }
> +	}
> +};
> +
> +/* Generic flash bbt decriptors */
> +static uint8_t bbt_pattern[] = { 'B', 'b', 't', '0' };
> +static uint8_t mirror_pattern[] = { '1', 't', 'b', 'B' };
> +
> +static struct nand_bbt_descr bbt_main_descr = {
> +	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
> +		| NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP,
> +	.offs = 4,
> +	.len = 4,
> +	.veroffs = 20,
> +	.maxblocks = 4,
> +	.pattern = bbt_pattern
> +};
> +
> +static struct nand_bbt_descr bbt_mirror_descr = {
> +	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
> +		| NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP,
> +	.offs = 4,
> +	.len = 4,
> +	.veroffs = 20,
> +	.maxblocks = 4,
> +	.pattern = mirror_pattern
> +};

Why do you need a custom BBT descriptor? It's much better to use the
standard ones. Perhaps you just want the NAND_BBT_NO_OOB_BBM option, so
you get the bbt_{main,mirror}_no_oob_descr structs from nand_bbt.c.

> +
>  /**
>   * pl353_nand_calculate_hwecc - Calculate Hardware ECC
>   * @mtd:	Pointer to the mtd_info structure
> @@ -816,15 +858,74 @@ static int pl353_nand_device_ready(struct mtd_info *mtd)
>  }
>  
>  /**
> + * pl353_nand_detect_ondie_ecc - Get the flash ondie ecc state
> + * @mtd:	Pointer to the mtd_info structure
> + *
> + * This function enables the ondie ecc for the Micron ondie ecc capable devices
> + *
> + * Return:	1 on detect, 0 if fail to detect
> + */
> +static int pl353_nand_detect_ondie_ecc(struct mtd_info *mtd)

No. On-die ECC support should not be added to a specific NAND driver; it
needs to be written generically as part of nand_base. There have been
recent attempts to do this for Toshiba (try searching the linux-mtd
archives, in the last 4 months or so), but they fell a little short. I'd
rather start with a nand_base approach though.

> +{
> +	struct nand_chip *nand_chip = mtd->priv;
> +	u8 maf_id, dev_id, i, get_feature;
> +	u8 set_feature[4] = { 0x08, 0x00, 0x00, 0x00 };
> +
> +	/* Check if On-Die ECC flash */
> +	nand_chip->cmdfunc(mtd, NAND_CMD_RESET, -1, -1);
> +	nand_chip->cmdfunc(mtd, NAND_CMD_READID, 0x00, -1);
> +
> +	/* Read manufacturer and device IDs */
> +	maf_id = readb(nand_chip->IO_ADDR_R);
> +	dev_id = readb(nand_chip->IO_ADDR_R);
> +
> +	if ((maf_id == NAND_MFR_MICRON) &&
> +	    ((dev_id == 0xf1) || (dev_id == 0xa1) ||
> +	     (dev_id == 0xb1) || (dev_id == 0xaa) ||
> +	     (dev_id == 0xba) || (dev_id == 0xda) ||
> +	     (dev_id == 0xca) || (dev_id == 0xac) ||
> +	     (dev_id == 0xbc) || (dev_id == 0xdc) ||
> +	     (dev_id == 0xcc) || (dev_id == 0xa3) ||
> +	     (dev_id == 0xb3) ||
> +	     (dev_id == 0xd3) || (dev_id == 0xc3))) {

So you're writing this with a list of device IDs? Would it make more
sense to use the ONFI parameters?

> +
> +		nand_chip->cmdfunc(mtd, NAND_CMD_GET_FEATURES,
> +				   ONDIE_ECC_FEATURE_ADDR, -1);
> +		get_feature = readb(nand_chip->IO_ADDR_R);
> +
> +		if (get_feature & 0x08) {
> +			return 1;
> +		} else {
> +			nand_chip->cmdfunc(mtd, NAND_CMD_SET_FEATURES,
> +					   ONDIE_ECC_FEATURE_ADDR, -1);
> +			for (i = 0; i < 4; i++)
> +				writeb(set_feature[i], nand_chip->IO_ADDR_W);
> +
> +			ndelay(1000);
> +
> +			nand_chip->cmdfunc(mtd, NAND_CMD_GET_FEATURES,
> +					   ONDIE_ECC_FEATURE_ADDR, -1);
> +			get_feature = readb(nand_chip->IO_ADDR_R);
> +
> +			if (get_feature & 0x08)
> +				return 1;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/**
>   * pl353_nand_ecc_init - Initialize the ecc information as per the ecc mode
>   * @mtd:	Pointer to the mtd_info structure
> + * @ondie_ecc_state:	ondie ecc status
>   *
>   * This function initializes the ecc block and functional pointers as per the
>   * ecc mode
>   *
>   * Return:	Zero on success and error on failure.
>   */
> -static int pl353_nand_ecc_init(struct mtd_info *mtd)
> +static int pl353_nand_ecc_init(struct mtd_info *mtd, int ondie_ecc_state)
>  {
>  	struct nand_chip *nand_chip = mtd->priv;
>  	struct pl353_nand_info *xnand =
> @@ -838,27 +939,50 @@ static int pl353_nand_ecc_init(struct mtd_info *mtd)
>  
>  	switch (xnand->ecc_mode) {
>  	case NAND_ECC_HW:
> -		if (mtd->writesize > 2048) {
> +		if ((mtd->writesize > 2048) || ((ondie_ecc_state) &&
> +						(mtd->oobsize != 64))) {
>  			pr_warn("hardware ECC not possible\n");
>  			return -ENOTSUPP;
>  		}
>  
>  		nand_chip->ecc.mode = NAND_ECC_HW;
> -		nand_chip->ecc.calculate = pl353_nand_calculate_hwecc;
> -		nand_chip->ecc.correct = pl353_nand_correct_data;
> -		nand_chip->ecc.hwctl = NULL;
> -		nand_chip->ecc.read_page = pl353_nand_read_page_hwecc;
> -		nand_chip->ecc.size = PL353_NAND_ECC_SIZE;
> -		nand_chip->ecc.write_page = pl353_nand_write_page_hwecc;
> -		pl353_smc_set_ecc_pg_size(mtd->dev.parent, mtd->writesize);
> -		pl353_smc_set_ecc_mode(mtd->dev.parent, PL353_SMC_ECCMODE_APB);
> -		/* Hardware ECC generates 3 bytes ECC code for each 512 bytes */
> -		nand_chip->ecc.bytes = 3;
> -
> -		if (mtd->oobsize == 16)
> -			nand_chip->ecc.layout = &nand_oob_16;
> -		else
> -			nand_chip->ecc.layout = &nand_oob_64;
> +		if (ondie_ecc_state) {
> +			/* Bypass the controller ECC block */
> +			pl353_smc_set_ecc_mode(mtd->dev.parent,
> +						PL353_SMC_ECCMODE_BYPASS);
> +			nand_chip->ecc.bytes = 0;
> +			nand_chip->ecc.layout = &ondie_nand_oob_64;
> +			nand_chip->ecc.read_page = pl353_nand_read_page_raw;
> +			nand_chip->ecc.write_page = pl353_nand_write_page_raw;
> +			nand_chip->ecc.size = mtd->writesize;
> +			/*
> +			 * On-Die ECC spare bytes offset 8 is used for ECC codes
> +			 * Use the BBT pattern descriptors
> +			 */
> +			nand_chip->bbt_td = &bbt_main_descr;
> +			nand_chip->bbt_md = &bbt_mirror_descr;
> +		} else {
> +			nand_chip->ecc.calculate = pl353_nand_calculate_hwecc;
> +			nand_chip->ecc.correct = pl353_nand_correct_data;
> +			nand_chip->ecc.hwctl = NULL;
> +			nand_chip->ecc.read_page = pl353_nand_read_page_hwecc;
> +			nand_chip->ecc.size = PL353_NAND_ECC_SIZE;
> +			nand_chip->ecc.write_page = pl353_nand_write_page_hwecc;
> +			pl353_smc_set_ecc_pg_size(mtd->dev.parent,
> +							mtd->writesize);
> +			pl353_smc_set_ecc_mode(mtd->dev.parent,
> +							PL353_SMC_ECCMODE_APB);
> +			/*
> +			 * Hardware ECC generates 3 bytes ECC code for each
> +			 * 512 bytes
> +			 */
> +			nand_chip->ecc.bytes = 3;
> +
> +			if (mtd->oobsize == 16)
> +				nand_chip->ecc.layout = &nand_oob_16;
> +			else
> +				nand_chip->ecc.layout = &nand_oob_64;
> +		}
>  
>  		break;
>  	case NAND_ECC_SOFT:
> @@ -901,6 +1025,7 @@ static int pl353_nand_probe(struct platform_device *pdev)
>  	struct nand_chip *nand_chip;
>  	struct resource *res;
>  	struct mtd_part_parser_data ppdata;
> +	int ondie_ecc_state;
>  
>  	xnand = devm_kzalloc(&pdev->dev, sizeof(*xnand), GFP_KERNEL);
>  	if (!xnand)
> @@ -944,6 +1069,7 @@ static int pl353_nand_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, xnand);
>  
> +	ondie_ecc_state = pl353_nand_detect_ondie_ecc(mtd);
>  	/* first scan to find the device and get the page size */
>  	if (nand_scan_ident(mtd, 1, NULL)) {
>  		dev_err(&pdev->dev, "nand_scan_ident for NAND failed\n");
> @@ -954,7 +1080,7 @@ static int pl353_nand_probe(struct platform_device *pdev)
>  	if (xnand->ecc_mode < 0)
>  		xnand->ecc_mode = NAND_ECC_HW;
>  
> -	if (pl353_nand_ecc_init(mtd))
> +	if (pl353_nand_ecc_init(mtd, ondie_ecc_state))
>  		return -ENOTSUPP;
>  
>  	if (nand_chip->options & NAND_BUSWIDTH_16)

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/