Re: [PATCH v9 2/2] mtd: nand: Add support for Arasan NAND Flash Controller

[Miquel Raynal <miquel.raynal@free-electrons.com>]

Hi Naga,

-> Boris, there is one question for you below (about NVDDR).

> > > +static inline struct anfc_nand_chip *to_anfc_nand(struct nand_chip
> > > *nand) +{
> > > +	return container_of(nand, struct anfc_nand_chip, chip); }
> > > +
> > > +static inline struct anfc *to_anfc(struct nand_hw_control *ctrl) {
> > > +	return container_of(ctrl, struct anfc, controller); }
> > > +
> > > +static u8 anfc_page(u32 pagesize)
> > > +{
> > > +	switch (pagesize) {
> > > +	case 512:
> > > +		return REG_PAGE_SIZE_512;
> > > +	case 1024:
> > > +		return REG_PAGE_SIZE_1K;
> > > +	case 2048:
> > > +		return REG_PAGE_SIZE_2K;
> > > +	case 4096:
> > > +		return REG_PAGE_SIZE_4K;
> > > +	case 8192:
> > > +		return REG_PAGE_SIZE_8K;
> > > +	case 16384:  
> > 
> > Maybe you can use macros like SZ_512, SZ_1K, SZ_2K, etc, see
> > include/linux/sizes.h  
> These are not really page size values.
> In CMD_REG[25:23] we need to specify page size as below
> 000: 512-byte Page size.
> 001: 2KB Page size.
> 010: 4KB Page size.
> 011: 8KB Page size.
> 100: 16KB Page size, hence for naming conventions we used the above macros. 
> Means, REG_PAGE_SIZE_512 = 0.

I meant for the 'case' statement, not the return value.
ie. s/case 2048:/case SZ_2K:/

> >   
> > > +		return REG_PAGE_SIZE_16K;
> > > +	default:
> > > +		break;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static inline void anfc_enable_intrs(struct anfc *nfc, u32 val) {
> > > +	writel(val, nfc->base + INTR_STS_EN_OFST);
> > > +	writel(val, nfc->base + INTR_SIG_EN_OFST); }
> > > +
> > > +static inline void anfc_config_ecc(struct anfc *nfc, int on) {
> > > +	u32 val;
> > > +
> > > +	val = readl(nfc->base + CMD_OFST);
> > > +	if (on)
> > > +		val |= ECC_ENABLE;
> > > +	else
> > > +		val &= ~ECC_ENABLE;
> > > +	writel(val, nfc->base + CMD_OFST);
> > > +}
> > > +
> > > +static inline void anfc_config_dma(struct anfc *nfc, int on) {
> > > +	u32 val;
> > > +
> > > +	val = readl(nfc->base + CMD_OFST);
> > > +	val &= ~DMA_EN_MASK;
> > > +	if (on)
> > > +		val |= DMA_ENABLE << DMA_EN_SHIFT;
> > > +	writel(val, nfc->base + CMD_OFST);
> > > +}  
> > 
> > Nitpick: both anfc_config_ecc/dma() functions do similar actions but the syntax
> > is different.  
> Yes, we can make it generic, But to identify whether we are configuring ecc or dma, these > two functions are added.

Sure, I am not discussing the two functions, but just to achieve the
same goal (put an enable bit or wipe it out) either you do

if (enable)
     enable;
else
     disable;

while right after you do:

val = disable;
if (enable)
     val =  enable;

I don't care which one you will chose but maybe you could use only one
style.


> > > +}
> > > +
> > > +static int anfc_read_page_hwecc(struct mtd_info *mtd,
> > > +				struct nand_chip *chip, uint8_t *buf,
> > > +				int oob_required, int page)
> > > +{
> > > +	u32 val;
> > > +	struct anfc *nfc = to_anfc(chip->controller);
> > > +	struct anfc_nand_chip *achip = to_anfc_nand(chip);
> > > +	u8 *ecc_code = chip->buffers->ecccode;
> > > +	u8 *p = buf;
> > > +	int eccsize = chip->ecc.size;
> > > +	int eccbytes = chip->ecc.bytes;
> > > +	int eccsteps = chip->ecc.steps;
> > > +	int stat = 0, i;
> > > +
> > > +	anfc_set_eccsparecmd(nfc, achip, NAND_CMD_RNDOUT,
> > > NAND_CMD_RNDOUTSTART);
> > > +	anfc_config_ecc(nfc, 1);
> > > +
> > > +	chip->read_buf(mtd, buf, mtd->writesize);
> > > +
> > > +	val = readl(nfc->base + ECC_ERR_CNT_OFST);
> > > +	val = ((val & PAGE_ERR_CNT_MASK) >> 8);  
> > 
> > If I understand this correctly, there is no point doing the upper two lines out of
> > the if statement?  
> Yes, I will move this to if statement.
> >   
> > > +	if (achip->bch) {
> > > +		mtd->ecc_stats.corrected += val;  
> > 
> > This is strange that there is no handling of a failing situation when using BCH
> > algorithm. You should probably add some logic here that either increments
> > ecc_stats.corrected or ecc_stats.failed like it is done below.  
> Yes, in case of BCH, upto 24 bit errors it will correct. After that it won't even detect the errors.
> Hence when BCH we are not updating errors.

That's weird... Could you please double check this assumption? I find
weird that you have no way to distinguish a "there was no bitflips"
with a "there are too much bitflips so I can't even correct them".

> 
> >   
> > > +	} else {
> > > +		val = readl(nfc->base + ECC_ERR_CNT_1BIT_OFST);
> > > +		mtd->ecc_stats.corrected += val;
> > > +		val = readl(nfc->base + ECC_ERR_CNT_2BIT_OFST);
> > > +		mtd->ecc_stats.failed += val;
> > > +		/* Clear ecc error count register 1Bit, 2Bit */
> > > +		writel(0x0, nfc->base + ECC_ERR_CNT_1BIT_OFST);
> > > +		writel(0x0, nfc->base + ECC_ERR_CNT_2BIT_OFST);
> > > +	}
> > > +
> > > +	if (oob_required)
> > > +		chip->ecc.read_oob(mtd, chip, page);
> > > +
> > > +	anfc_config_ecc(nfc, 0);
> > > +  
> > 
> > Some comments would be welcome from here to the end, that is not entirely
> > clear to me what you try to achieve here.
> >   
> > > +	if (val) {  
> > 
> > When using Hamming, val != 0 means there is an uncorrectable error, while
> > when using BCH, it may just mean there was some corrected bitflips. I think you
> > should enter this statement only if Hamming or BCH failed to recover bitflips?  
> Yes, I will update.
> >   
> > > +		chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
> > > +		chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);  
> > 
> > If OOB area was required, you are most likely overwriting OOB bytes that were
> > maybe corrected by the ECC engine before it was turned off, by bytes retried
> > with a raw read (potentially with bitflips).  
> This code is added to handle below case.
> When a page was erased, ECC won't update for erased page.
> And when we do an  empty page read, ECC will be updated and there is mismatch between ECC data 
> At the time erasing and reading, causing ECC errors.
> Hence we are reading entire OOB and updating the correct ECC value for all 0xff's case.
> This we have seen when using UBIFS, sometimes read happens on erased page. 

I see what you try to achieve but actually I don't think this is the
right way. Instead, you should, upon error (val != 0), use the core
helper nand_check_erased_ecc_chunk(). This way you check if the page is
good or not depending on the strength used and can discriminate a bad
written page (almost full of 0xff) and a clean erased page without ECC
bytes for which the ECC calculation failed.

> >   
> > > +		mtd_ooblayout_get_eccbytes(mtd, ecc_code,
> > > chip->oob_poi, 0,
> > > +					   chip->ecc.total);
> > > +		for (i = 0 ; eccsteps; eccsteps--, i += eccbytes,
> > > +		     p += eccsize) {
> > > +			stat = nand_check_erased_ecc_chunk(p,
> > > chip->ecc.size, &ecc_code[i], eccbytes, NULL, 0, chip->ecc.strength);  
> > 
> > Do you actually check the entire OOB area here ?
> > 
> > Can't you just do the check on chip->oob_poi?  
> 
> Above comments answers this.
> >   
> > > +		}
> > > +		if (stat < 0)  
> > 
> > Please check this, but I think you should increment ecc_stats.failed here.  
> Yes,  I will update.
> >   
> > > +			stat = 0;
> > > +		else
> > > +			mtd->ecc_stats.corrected += stat;
> > > +		return stat;
> > > +	}
> > > +
> > > +	return 0;
> > > +}  
> > 
> > Can you run nandbiterrs test (from mtd-utils) with both Hamming and BCH and
> > check it fails after the right number of bitflips?  
> Ok, I will run.
> >   
> > > +
> > > +static int anfc_write_page_hwecc(struct mtd_info *mtd,
> > > +				 struct nand_chip *chip, const
> > > uint8_t *buf,
> > > +				 int oob_required, int page)
> > > +{
> > > +	int ret;
> > > +	struct anfc *nfc = to_anfc(chip->controller);
> > > +	struct anfc_nand_chip *achip = to_anfc_nand(chip);
> > > +	u8 *ecc_calc = chip->buffers->ecccalc;
> > > +
> > > +	anfc_set_eccsparecmd(nfc, achip, NAND_CMD_RNDIN, 0);
> > > +	anfc_config_ecc(nfc, 1);
> > > +
> > > +	chip->write_buf(mtd, buf, mtd->writesize);
> > > +
> > > +	if (oob_required) {
> > > +		chip->waitfunc(mtd, chip);
> > > +		chip->cmdfunc(mtd, NAND_CMD_READOOB, 0, page);
> > > +		chip->read_buf(mtd, ecc_calc, mtd->oobsize);
> > > +		ret = mtd_ooblayout_set_eccbytes(mtd, ecc_calc,
> > > chip->oob_poi,
> > > +						 0, chip->ecc.total);
> > > +		if (ret)
> > > +			return ret;
> > > +		chip->ecc.write_oob(mtd, chip, page);
> > > +	}
> > > +	anfc_config_ecc(nfc, 0);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static u8 anfc_read_byte(struct mtd_info *mtd) {
> > > +	struct nand_chip *chip = mtd_to_nand(mtd);
> > > +	struct anfc *nfc = to_anfc(chip->controller);
> > > +
> > > +	if (nfc->curr_cmd == NAND_CMD_STATUS)
> > > +		return readl(nfc->base + FLASH_STS_OFST);
> > > +	else
> > > +		return nfc->buf[nfc->bufshift++];
> > > +}
> > > +
> > > +static int anfc_extra_init(struct anfc *nfc, struct anfc_nand_chip
> > > *achip) +{
> > > +	int mode, err;
> > > +	unsigned int feature[2];
> > > +	u32 inftimeval;
> > > +	struct nand_chip *chip = &achip->chip;
> > > +	struct mtd_info *mtd = nand_to_mtd(chip);
> > > +	bool change_sdr_clk = false;
> > > +
> > > +	if (!chip->onfi_version ||
> > > +	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
> > > +	    & ONFI_OPT_CMD_SET_GET_FEATURES))
> > > +		return -EINVAL;  
> > 
> > I think this check is already done by the core.  
> See my comments below.
> >   
> > > +
> > > +	/*
> > > +	 * If the controller is already in the same mode as flash
> > > device
> > > +	 * then no need to change the timing mode again.
> > > +	 */
> > > +	if (readl(nfc->base + DATA_INTERFACE_OFST) ==
> > > achip->inftimeval)
> > > +		return 0;
> > > +
> > > +	memset(feature, 0, NVDDR_MODE_PACKET_SIZE);
> > > +	/* Get nvddr timing modes */
> > > +	mode = onfi_get_sync_timing_mode(chip) & 0xff;
> > > +	if (!mode) {
> > > +		mode = fls(onfi_get_async_timing_mode(chip)) - 1;
> > > +		inftimeval = mode;
> > > +		if (mode >= 2 && mode <= 5)
> > > +			change_sdr_clk = true;
> > > +	} else {
> > > +		mode = fls(mode) - 1;
> > > +		inftimeval = NVDDR_MODE | (mode <<
> > > NVDDR_TIMING_MODE_SHIFT);
> > > +		mode |= ONFI_DATA_INTERFACE_NVDDR;
> > > +	}
> > > +	feature[0] = mode;
> > > +	chip->select_chip(mtd, achip->csnum);
> > > +	err = chip->onfi_set_features(mtd, chip,
> > > ONFI_FEATURE_ADDR_TIMING_MODE,
> > > +		(uint8_t *)feature);
> > > +	chip->select_chip(mtd, -1);
> > > +	if (err)
> > > +		return err;  
> > 
> > You should forget all the NAND chip related code here, the core already handles
> > it, and then calls ->setup_data_interface() to tune timings on the controller side
> > only.  
> Since core doesn't have support for NVDDR, 
> enum nand_data_interface_type {
> 	NAND_SDR_IFACE,
> };
> we added this logic in setup_data_interface. To achieve the same
> So what we are doing here is, 
> When core calls setup_data_interface, the flash changes its operating mode to SDR.
> And when flash supports NVDDR, we are changing the 
> Flash operating mode to NVDDR.

Not sure this is actually supported by the core. Maybe Boris could give
us some clues?

> Also the setup_data_interface call will happen during probe time, and at this time we don't have
> Parameter page info, hence I added 

I sent a patch series to fix that. Let's keep it for now but soon this
will have to be removed.

> if (!chip->onfi_version ||
> 	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
> 	    & ONFI_OPT_CMD_SET_GET_FEATURES))
> 		return -EINVAL;
> >   
> > > +	/*
> > > +	 * SDR timing modes 2-5 will not work for the arasan nand
> > > when
> > > +	 * freq > 90 MHz, so reduce the freq in SDR modes 2-5 to <
> > > 90Mhz
> > > +	 */
> > > +	if (change_sdr_clk) {
> > > +		clk_disable_unprepare(nfc->clk_sys);
> > > +		err = clk_set_rate(nfc->clk_sys,
> > > SDR_MODE_DEFLT_FREQ);
> > > +		if (err) {
> > > +			dev_err(nfc->dev, "Can't set the clock
> > > rate\n");
> > > +			return err;
> > > +		}
> > > +		err = clk_prepare_enable(nfc->clk_sys);
> > > +		if (err) {
> > > +			dev_err(nfc->dev, "Unable to enable sys
> > > clock.\n");
> > > +			clk_disable_unprepare(nfc->clk_sys);
> > > +			return err;
> > > +		}  
> > 
> > You do this twice as it is already handled in ->setup_data_interface().  
> Yes, we added it twice, once when core sends SDR info
> And the second, because there is a HW bug in controller, i.e SDR timing modes 2-5 will not work, when freq > 90MHz.
> Hence when Flash operates at SDR modes 2-5, we are changing the clock rate to below 90MHz

1/ If you do it twice because of a HW bug: please add a comment.
2/ What if there are several NAND chips using different timing
modes? You probably should handle this in ->select_chip() then.

> >   
> > > +	}
> > > +	achip->inftimeval = inftimeval;
> > > +	if (mode & ONFI_DATA_INTERFACE_NVDDR)
> > > +		achip->spktsize = NVDDR_MODE_PACKET_SIZE;
> > > +	return 0;
> > > +}  
> > 
> > I don't think this should be in a separate function and instead should be in -  
> > >setup_data_interface().  
> Yes, we can do it in single function, but as mentioned in comments, we added this extra_init().
> >   
> > > +
> > > +static int anfc_ecc_init(struct mtd_info *mtd,
> > > +			 struct nand_ecc_ctrl *ecc)
> > > +{
> > > +	u32 ecc_addr;
> > > +	unsigned int bchmode, steps;
> > > +	struct nand_chip *chip = mtd_to_nand(mtd);
> > > +	struct anfc_nand_chip *achip = to_anfc_nand(chip);
> > > +
> > > +	ecc->mode = NAND_ECC_HW;
> > > +	ecc->read_page = anfc_read_page_hwecc;
> > > +	ecc->write_page = anfc_write_page_hwecc;
> > > +	ecc->write_oob = anfc_write_oob;
> > > +	mtd_set_ooblayout(mtd, &anfc_ooblayout_ops);
> > > +
> > > +	steps = mtd->writesize / chip->ecc_step_ds;
> > > +
> > > +	switch (chip->ecc_strength_ds) {
> > > +	case 12:
> > > +		bchmode = 0x1;
> > > +		break;
> > > +	case 8:
> > > +		bchmode = 0x2;
> > > +		break;
> > > +	case 4:
> > > +		bchmode = 0x3;
> > > +		break;
> > > +	case 24:
> > > +		bchmode = 0x4;
> > > +		break;
> > > +	default:
> > > +		bchmode = 0x0;
> > > +	}
> > > +
> > > +	if (!bchmode)
> > > +		ecc->total = 3 * steps;
> > > +	else
> > > +		ecc->total =
> > > +		     DIV_ROUND_UP(fls(8 * chip->ecc_step_ds) *
> > > +			 chip->ecc_strength_ds * steps, 8);
> > > +
> > > +	ecc->strength = chip->ecc_strength_ds;
> > > +	ecc->size = chip->ecc_step_ds;
> > > +	ecc->bytes = ecc->total / steps;
> > > +	ecc->steps = steps;
> > > +	achip->bchmode = bchmode;
> > > +	achip->bch = achip->bchmode;
> > > +	ecc_addr = mtd->writesize + (mtd->oobsize - ecc->total);
> > > +
> > > +	achip->eccval = ecc_addr | (ecc->total << ECC_SIZE_SHIFT) |
> > > +			(achip->bch << BCH_EN_SHIFT);
> > > +
> > > +	if (chip->ecc_step_ds >= 1024)
> > > +		achip->pktsize = 1024;
> > > +	else
> > > +		achip->pktsize = 512;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void anfc_cmd_function(struct mtd_info *mtd,
> > > +			      unsigned int cmd, int column, int
> > > page_addr) +{
> > > +	struct nand_chip *chip = mtd_to_nand(mtd);
> > > +	struct anfc_nand_chip *achip = to_anfc_nand(chip);
> > > +	struct anfc *nfc = to_anfc(chip->controller);
> > > +	bool wait = false;
> > > +	u32 addrcycles, prog;
> > > +
> > > +	nfc->bufshift = 0;
> > > +	nfc->curr_cmd = cmd;
> > > +
> > > +	if (page_addr == -1)
> > > +		page_addr = 0;
> > > +	if (column == -1)
> > > +		column = 0;
> > > +
> > > +	switch (cmd) {
> > > +	case NAND_CMD_RESET:
> > > +		anfc_prepare_cmd(nfc, cmd, 0, 0, 0, 0);
> > > +		prog = PROG_RST;
> > > +		wait = true;
> > > +		break;
> > > +	case NAND_CMD_SEQIN:
> > > +		addrcycles = achip->raddr_cycles +
> > > achip->caddr_cycles;
> > > +		anfc_prepare_cmd(nfc, cmd, NAND_CMD_PAGEPROG, 1,
> > > +				 mtd->writesize, addrcycles);
> > > +		anfc_setpagecoladdr(nfc, page_addr, column);
> > > +		break;
> > > +	case NAND_CMD_READOOB:
> > > +		column += mtd->writesize;
> > > +	case NAND_CMD_READ0:
> > > +	case NAND_CMD_READ1:
> > > +		addrcycles = achip->raddr_cycles +
> > > achip->caddr_cycles;
> > > +		anfc_prepare_cmd(nfc, NAND_CMD_READ0,
> > > NAND_CMD_READSTART, 1,
> > > +				 mtd->writesize, addrcycles);
> > > +		anfc_setpagecoladdr(nfc, page_addr, column);
> > > +		break;
> > > +	case NAND_CMD_RNDOUT:
> > > +		anfc_prepare_cmd(nfc, cmd, NAND_CMD_RNDOUTSTART, 1,
> > > +				 mtd->writesize, 2);
> > > +		anfc_setpagecoladdr(nfc, page_addr, column);
> > > +		break;
> > > +	case NAND_CMD_PARAM:
> > > +		anfc_prepare_cmd(nfc, cmd, 0, 0, 0, 1);
> > > +		anfc_setpagecoladdr(nfc, page_addr, column);
> > > +		anfc_rw_buf_pio(mtd, nfc->buf,
> > > +				(4 * sizeof(struct
> > > nand_onfi_params)),
> > > +				1, PROG_RDPARAM);
> > > +		break;
> > > +	case NAND_CMD_READID:
> > > +		anfc_prepare_cmd(nfc, cmd, 0, 0, 0, 1);
> > > +		anfc_setpagecoladdr(nfc, page_addr, column);
> > > +		anfc_rw_buf_pio(mtd, nfc->buf, ONFI_ID_LEN, 1,
> > > PROG_RDID);
> > > +		break;
> > > +	case NAND_CMD_ERASE1:
> > > +		addrcycles = achip->raddr_cycles;
> > > +		prog = PROG_ERASE;
> > > +		anfc_prepare_cmd(nfc, cmd, NAND_CMD_ERASE2, 0, 0,
> > > addrcycles);
> > > +		column = page_addr & 0xffff;
> > > +		page_addr = (page_addr >> PG_ADDR_SHIFT) & 0xffff;
> > > +		anfc_setpagecoladdr(nfc, page_addr, column);
> > > +		wait = true;
> > > +		break;
> > > +	case NAND_CMD_STATUS:
> > > +		anfc_prepare_cmd(nfc, cmd, 0, 0, 0, 0);
> > > +		anfc_setpktszcnt(nfc, achip->spktsize / 4, 1);
> > > +		anfc_setpagecoladdr(nfc, page_addr, column);
> > > +		prog = PROG_STATUS;
> > > +		wait = true;
> > > +		break;
> > > +	case NAND_CMD_GET_FEATURES:
> > > +		anfc_prepare_cmd(nfc, cmd, 0, 0, 0, 1);
> > > +		anfc_setpagecoladdr(nfc, page_addr, column);
> > > +		anfc_rw_buf_pio(mtd, nfc->buf, achip->spktsize, 1,
> > > +				PROG_GET_FEATURE);
> > > +		break;
> > > +	case NAND_CMD_SET_FEATURES:
> > > +		anfc_prepare_cmd(nfc, cmd, 0, 0, 0, 1);
> > > +		anfc_setpagecoladdr(nfc, page_addr, column);
> > > +		break;
> > > +	default:
> > > +		return;
> > > +	}
> > > +
> > > +	if (wait) {
> > > +		anfc_enable_intrs(nfc, XFER_COMPLETE);
> > > +		writel(prog, nfc->base + PROG_OFST);
> > > +		anfc_wait_for_event(nfc);
> > > +	}
> > > +	if (nfc->curr_cmd == NAND_CMD_STATUS)
> > > +		nfc->status = readl(nfc->base + FLASH_STS_OFST); }
> > > +
> > > +static void anfc_select_chip(struct mtd_info *mtd, int num) {
> > > +	u32 val;
> > > +	struct nand_chip *chip = mtd_to_nand(mtd);
> > > +	struct anfc_nand_chip *achip = to_anfc_nand(chip);
> > > +	struct anfc *nfc = to_anfc(chip->controller);
> > > +
> > > +	if (num == -1)
> > > +		return;
> > > +
> > > +	val = readl(nfc->base + MEM_ADDR2_OFST);
> > > +	val &= (val & ~(CS_MASK | BCH_MODE_MASK));
> > > +	val |= (achip->csnum << CS_SHIFT) | (achip->bchmode <<
> > > BCH_MODE_SHIFT);
> > > +	writel(val, nfc->base + MEM_ADDR2_OFST);
> > > +	nfc->csnum = achip->csnum;
> > > +	writel(achip->eccval, nfc->base + ECC_OFST);
> > > +	writel(achip->inftimeval, nfc->base + DATA_INTERFACE_OFST); }
> > > +
> > > +static irqreturn_t anfc_irq_handler(int irq, void *ptr) {
> > > +	struct anfc *nfc = ptr;
> > > +	u32 status;
> > > +
> > > +	status = readl(nfc->base + INTR_STS_OFST);
> > > +	if (status & EVENT_MASK) {
> > > +		complete(&nfc->event);
> > > +		writel((status & EVENT_MASK), nfc->base +
> > > INTR_STS_OFST);
> > > +		writel(0, nfc->base + INTR_STS_EN_OFST);
> > > +		writel(0, nfc->base + INTR_SIG_EN_OFST);
> > > +		return IRQ_HANDLED;
> > > +	}
> > > +
> > > +	return IRQ_NONE;
> > > +}
> > > +
> > > +static int anfc_onfi_set_features(struct mtd_info *mtd, struct
> > > nand_chip *chip,
> > > +				  int addr, uint8_t
> > > *subfeature_param) +{
> > > +	struct anfc_nand_chip *achip = to_anfc_nand(chip);
> > > +	int status;
> > > +
> > > +	if (!chip->onfi_version)
> > > +		return -EINVAL;
> > > +
> > > +	if (!(le16_to_cpu(chip->onfi_params.opt_cmd) &
> > > +		ONFI_OPT_CMD_SET_GET_FEATURES))
> > > +		return -EINVAL;
> > > +
> > > +	chip->cmdfunc(mtd, NAND_CMD_SET_FEATURES, addr, -1);
> > > +	anfc_rw_buf_pio(mtd, subfeature_param, achip->spktsize,
> > > +			0, PROG_SET_FEATURE);
> > > +	status = chip->waitfunc(mtd, chip);
> > > +	if (status & NAND_STATUS_FAIL)
> > > +		return -EIO;
> > > +
> > > +	return 0;
> > > +}  
> > 
> > Are you sure this function is needed? If yes can you add a comment to explain
> > why? Otherwise I think the core already handles that kind of logic.  
> We added this to set/get features of on-die ECC. At the time of adding this, core doesn’t have support
> For this. I will remove it now, since micron on-die ecc driver is there.
> >   
> > > +
> > > +static int anfc_setup_data_interface(struct mtd_info *mtd, int
> > > csline,
> > > +				     const struct
> > > nand_data_interface *conf) +{
> > > +	struct nand_chip *chip = mtd_to_nand(mtd);
> > > +	struct anfc *nfc = to_anfc(chip->controller);
> > > +	int err;
> > > +	struct anfc_nand_chip *achip = to_anfc_nand(chip);
> > > +
> > > +	if (csline == NAND_DATA_IFACE_CHECK_ONLY)
> > > +		return 0;
> > > +
> > > +	clk_disable_unprepare(nfc->clk_sys);
> > > +	err = clk_set_rate(nfc->clk_sys, SDR_MODE_DEFLT_FREQ);
> > > +	if (err) {
> > > +		dev_err(nfc->dev, "Can't set the clock rate\n");
> > > +		return err;
> > > +	}
> > > +	err = clk_prepare_enable(nfc->clk_sys);
> > > +	if (err) {
> > > +		dev_err(nfc->dev, "Unable to enable sys clock.\n");
> > > +		clk_disable_unprepare(nfc->clk_sys);
> > > +		return err;
> > > +	}
> > > +	achip->inftimeval = 0;
> > > +	anfc_extra_init(nfc, achip);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int anfc_nand_chip_init(struct anfc *nfc,
> > > +			       struct anfc_nand_chip *anand_chip,
> > > +			       struct device_node *np)
> > > +{
> > > +	struct nand_chip *chip = &anand_chip->chip;
> > > +	struct mtd_info *mtd = nand_to_mtd(chip);
> > > +	int ret;
> > > +
> > > +	ret = of_property_read_u32(np, "reg", &anand_chip->csnum);
> > > +	if (ret) {
> > > +		dev_err(nfc->dev, "can't get chip-select\n");
> > > +		return -ENXIO;
> > > +	}
> > > +
> > > +	mtd->name = devm_kasprintf(nfc->dev, GFP_KERNEL,
> > > "arasan_nand.%d",
> > > +				   anand_chip->csnum);
> > > +	mtd->dev.parent = nfc->dev;
> > > +
> > > +	chip->cmdfunc = anfc_cmd_function;
> > > +	chip->chip_delay = 30;
> > > +	chip->controller = &nfc->controller;
> > > +	chip->read_buf = anfc_read_buf;
> > > +	chip->write_buf = anfc_write_buf;
> > > +	chip->read_byte = anfc_read_byte;
> > > +	chip->options = NAND_BUSWIDTH_AUTO |  
> > NAND_NO_SUBPAGE_WRITE;  
> > > +	chip->bbt_options = NAND_BBT_USE_FLASH;
> > > +	chip->select_chip = anfc_select_chip;
> > > +	chip->onfi_set_features = anfc_onfi_set_features;
> > > +	chip->setup_data_interface = anfc_setup_data_interface;
> > > +	nand_set_flash_node(chip, np);
> > > +
> > > +	anand_chip->spktsize = SDR_MODE_PACKET_SIZE;
> > > +	ret = nand_scan_ident(mtd, 1, NULL);
> > > +	if (ret) {
> > > +		dev_err(nfc->dev, "nand_scan_ident for NAND
> > > failed\n");
> > > +		return ret;
> > > +	}
> > > +	if (chip->onfi_version) {
> > > +		anand_chip->raddr_cycles =
> > > chip->onfi_params.addr_cycles & 0xf;
> > > +		anand_chip->caddr_cycles =
> > > +				(chip->onfi_params.addr_cycles >> 4)
> > > & 0xf;
> > > +	} else {
> > > +		/* For non-ONFI devices, configuring the address
> > > cyles as 5 */
> > > +		anand_chip->raddr_cycles = 3;
> > > +		anand_chip->caddr_cycles = 2;
> > > +	}  
> > 
> > I think you should remove this block and instead decide how many cycles you
> > will need depending on "chip->options & NAND_ROW_ADDR_3".  
> If chip supports ONFI, then we are reading addr cycles from parameter page, if not 
> We are setting with default values.
> I am not checking the sizes here, instead reading it from parameter page.
> I didn't get your comment, can you please brief it?
> I saw the patch https://patchwork.kernel.org/patch/9950377/.
> These are based on sizes, and cycles are getting updated using " NAND_ROW_ADDR_3"
> But the case here is I am setting some default values if not ONFI. 

I suppose this is already handled by the core. You should probably just
choose the number of address cycles depending on the NAND_ROW_ADDR_3
flag presence in chip->options. Logic should be in the core and shared
across all NAND controllers anyway.

> >   
> > > +
> > > +	ret = anfc_ecc_init(mtd, &chip->ecc);
> > > +	if (ret)
> > > +		return ret;
> > > +  
> > 
> > Shouldn't the controller set mtd->name here if empty?  
> Yes, driver is not setting the name if empty.
> I will add this in next version of patch.
> >   
> > > +	ret = nand_scan_tail(mtd);
> > > +	if (ret) {
> > > +		dev_err(nfc->dev, "nand_scan_tail for NAND
> > > failed\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	return mtd_device_register(mtd, NULL, 0); }
> > > +
> > > +static int anfc_probe(struct platform_device *pdev) {
> > > +	struct anfc *nfc;
> > > +	struct anfc_nand_chip *anand_chip;
> > > +	struct device_node *np = pdev->dev.of_node, *child;
> > > +	struct resource *res;
> > > +	int err;
> > > +
> > > +	nfc = devm_kzalloc(&pdev->dev, sizeof(*nfc), GFP_KERNEL);
> > > +	if (!nfc)
> > > +		return -ENOMEM;
> > > +
> > > +	init_waitqueue_head(&nfc->controller.wq);
> > > +	INIT_LIST_HEAD(&nfc->chips);
> > > +	init_completion(&nfc->event);
> > > +	nfc->dev = &pdev->dev;
> > > +	platform_set_drvdata(pdev, nfc);
> > > +	nfc->csnum = -1;
> > > +
> > > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +	nfc->base = devm_ioremap_resource(&pdev->dev, res);
> > > +	if (IS_ERR(nfc->base))
> > > +		return PTR_ERR(nfc->base);
> > > +	nfc->dma = of_property_read_bool(pdev->dev.of_node,
> > > +					 "arasan,has-mdma");
> > > +	nfc->irq = platform_get_irq(pdev, 0);
> > > +	if (nfc->irq < 0) {
> > > +		dev_err(&pdev->dev, "platform_get_irq failed\n");
> > > +		return -ENXIO;
> > > +	}
> > > +	dma_set_mask(&pdev->dev, DMA_BIT_MASK(64));
> > > +	err = devm_request_irq(&pdev->dev, nfc->irq,
> > > anfc_irq_handler,
> > > +			       0, "arasannfc", nfc);
> > > +	if (err)
> > > +		return err;
> > > +	nfc->clk_sys = devm_clk_get(&pdev->dev, "sys");
> > > +	if (IS_ERR(nfc->clk_sys)) {
> > > +		dev_err(&pdev->dev, "sys clock not found.\n");
> > > +		return PTR_ERR(nfc->clk_sys);
> > > +	}
> > > +
> > > +	nfc->clk_flash = devm_clk_get(&pdev->dev, "flash");
> > > +	if (IS_ERR(nfc->clk_flash)) {
> > > +		dev_err(&pdev->dev, "flash clock not found.\n");
> > > +		return PTR_ERR(nfc->clk_flash);
> > > +	}
> > > +
> > > +	err = clk_prepare_enable(nfc->clk_sys);
> > > +	if (err) {
> > > +		dev_err(&pdev->dev, "Unable to enable sys clock.\n");
> > > +		return err;
> > > +	}
> > > +
> > > +	err = clk_prepare_enable(nfc->clk_flash);
> > > +	if (err) {
> > > +		dev_err(&pdev->dev, "Unable to enable flash
> > > clock.\n");
> > > +		goto clk_dis_sys;
> > > +	}
> > > +
> > > +	for_each_available_child_of_node(np, child) {
> > > +		anand_chip = devm_kzalloc(&pdev->dev,
> > > sizeof(*anand_chip),
> > > +					  GFP_KERNEL);
> > > +		if (!anand_chip) {
> > > +			of_node_put(child);
> > > +			err = -ENOMEM;
> > > +			goto nandchip_clean_up;
> > > +		}
> > > +
> > > +		err = anfc_nand_chip_init(nfc, anand_chip, child);
> > > +		if (err) {
> > > +			devm_kfree(&pdev->dev, anand_chip);
> > > +			continue;
> > > +		}
> > > +
> > > +		list_add_tail(&anand_chip->node, &nfc->chips);
> > > +	}
> > > +
> > > +	return 0;
> > > +
> > > +nandchip_clean_up:
> > > +	list_for_each_entry(anand_chip, &nfc->chips, node)
> > > +		nand_release(nand_to_mtd(&anand_chip->chip));
> > > +	clk_disable_unprepare(nfc->clk_flash);
> > > +clk_dis_sys:
> > > +	clk_disable_unprepare(nfc->clk_sys);
> > > +
> > > +	return err;
> > > +}
> > > +
> > > +static int anfc_remove(struct platform_device *pdev) {
> > > +	struct anfc *nfc = platform_get_drvdata(pdev);
> > > +	struct anfc_nand_chip *anand_chip;
> > > +
> > > +	list_for_each_entry(anand_chip, &nfc->chips, node)
> > > +		nand_release(nand_to_mtd(&anand_chip->chip));
> > > +
> > > +	clk_disable_unprepare(nfc->clk_sys);
> > > +	clk_disable_unprepare(nfc->clk_flash);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct of_device_id anfc_ids[] = {
> > > +	{ .compatible = "arasan,nfc-v3p10" },
> > > +	{ .compatible = "xlnx,zynqmp-nand" },
> > > +	{  }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, anfc_ids);
> > > +
> > > +static struct platform_driver anfc_driver = {
> > > +	.driver = {
> > > +		.name = DRIVER_NAME,
> > > +		.of_match_table = anfc_ids,
> > > +	},
> > > +	.probe = anfc_probe,
> > > +	.remove = anfc_remove,
> > > +};
> > > +module_platform_driver(anfc_driver);
> > > +
> > > +MODULE_LICENSE("GPL");
> > > +MODULE_AUTHOR("Xilinx, Inc");
> > > +MODULE_DESCRIPTION("Arasan NAND Flash Controller Driver");  
> > 
> > 
> > 
> > --
> > Miquel Raynal, Free Electrons
> > Embedded Linux and Kernel engineering
> > http://free-electrons.com  
> 
> I will restructure the driver to sync with exec_op().
> Thanks again for reviewing the driver.
> 
> Thanks,
> Naga Sureshkumar Relli.

Thanks for your work,
Miquèl

-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com