Re: [LINUX PATCH v8 2/2] mtd: rawnand: pl353: Add basic driver for arm pl353 smc nand interface

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Haris, Naga,

-linux-kernel@
+linux-mtd@
+Boris

> >>> +/**
> >>> + * pl353_nand_cmd_function - Send command to NAND device
> >>> + * @chip:	Pointer to the NAND chip info structure
> >>> + * @subop:	Pointer to array of instructions
> >>> + * Return:	Always return zero
> >>> + */
> >>> +static int pl353_nand_cmd_function(struct nand_chip *chip,
> >>> +					      const struct nand_subop *subop) {
> >>> +	struct mtd_info *mtd = nand_to_mtd(chip);
> >>> +	const struct nand_op_instr *instr;
> >>> +	struct pl353_nfc_op nfc_op;  
> >>
> >> nfc_op = {}; to initialize to 0.  
> > Ok, I will update it.  
> >>  
> >>> +	struct pl353_nand_info *xnand =
> >>> +		container_of(chip, struct pl353_nand_info, chip);
> >>> +	void __iomem *cmd_addr;
> >>> +	unsigned long cmd_data = 0, end_cmd_valid = 0;
> >>> +	unsigned long cmd_phase_addr, data_phase_addr, end_cmd;
> >>> +	unsigned long timeout = jiffies + PL353_NAND_DEV_BUSY_TIMEOUT;
> >>> +	u32 addrcycles = 0;
> >>> +	unsigned int op_id, len, offset;
> >>> +
> >>> +	pl353_nfc_parse_instructions(chip, subop, &nfc_op);
> >>> +	instr = nfc_op.data_instr;
> >>> +	op_id = nfc_op.data_instr_idx;
> >>> +	len = nand_subop_get_data_len(subop, op_id);
> >>> +	offset = nand_subop_get_data_start_off(subop, op_id);
> >>> +
> >>> +	if (nfc_op.cmnds[0] != -1) {
> >>> +		if (xnand->end_cmd_pending) {
> >>> +			/*
> >>> +			 * Check for end command if this command request is
> >>> +			 * same as the pending command then return
> >>> +			 */
> >>> +			if (xnand->end_cmd == nfc_op.cmnds[0]) {
> >>> +				xnand->end_cmd = 0;
> >>> +				xnand->end_cmd_pending = 0;
> >>> +				return 0;
> >>> +			}
> >>> +		}
> >>> +
> >>> +		/* Clear interrupt */
> >>> +		pl353_smc_clr_nand_int();
> >>> +		end_cmd_valid = 0;  
> >>
> >> Space  
> > Ok, I will correct it.  
> >>  
> >>> +		/* Get the command phase address */
> >>> +		if (nfc_op.cmnds[1] != -1) {
> >>> +			end_cmd_valid = 1;
> >>> +		} else {
> >>> +			if (nfc_op.cmnds[0] == NAND_CMD_READ0)
> >>> +				return 0;
> >>> +		}
> >>> +		if (nfc_op.end_cmd == NAND_CMD_NONE)
> >>> +			end_cmd = 0x0;
> >>> +		else
> >>> +			end_cmd = nfc_op.cmnds[1];
> >>> +
> >>> +		addrcycles = nfc_op.naddrs;
> >>> +		if (nfc_op.cmnds[0] == NAND_CMD_READ0 ||
> >>> +			nfc_op.cmnds[0] == NAND_CMD_SEQIN)
> >>> +			addrcycles = xnand->row_addr_cycles +
> >>> +					xnand->col_addr_cycles;
> >>> +		else if ((nfc_op.cmnds[0] == NAND_CMD_ERASE1) ||
> >>> +			(nfc_op.cmnds[0] == NAND_CMD_ERASE2))
> >>> +			addrcycles = xnand->row_addr_cycles;
> >>> +		else
> >>> +			addrcycles = nfc_op.naddrs;  
> >>
> >> A switch block would probably be appropriate.  
> > Yes we can use.  
> >>  
> >>> +		cmd_phase_addr = (unsigned long __force)xnand->nand_base +  
> >> (  
> >>> +				 (addrcycles << ADDR_CYCLES_SHIFT)	|
> >>> +				 (end_cmd_valid << END_CMD_VALID_SHIFT)  
> >> 	|  
> >>> +				 (COMMAND_PHASE)			|
> >>> +				 (end_cmd << END_CMD_SHIFT)		|
> >>> +				 (nfc_op.cmnds[0] << START_CMD_SHIFT));
> >>> +
> >>> +		cmd_addr = (void __iomem * __force)cmd_phase_addr;  
> >>
> >> Space  
> > Ok, I will correc it.  
> >>  
> >>> +		/* Get the data phase address */
> >>> +		end_cmd_valid = 0;
> >>> +
> >>> +		data_phase_addr = (unsigned long __force)xnand->nand_base +  
> >> (  
> >>> +				  (0x0 << CLEAR_CS_SHIFT)		|
> >>> +				  (end_cmd_valid << END_CMD_VALID_SHIFT)|
> >>> +				  (DATA_PHASE)				|
> >>> +				  (end_cmd << END_CMD_SHIFT)		|
> >>> +				  (0x0 << ECC_LAST_SHIFT));
> >>> +		chip->IO_ADDR_R = (void __iomem *  
> >> __force)data_phase_addr;
> >>
> >> Do you really need this "__force" ?  
> > Let me check and get back to you on this.  
> >>  
> >>> +		chip->IO_ADDR_W = chip->IO_ADDR_R;
> >>> +		/* Command phase AXI write */
> >>> +		/* Read & Write */
> >>> +		if (nfc_op.thirdrow) {
> >>> +			nfc_op.thirdrow = 0;
> >>> +			if (mtd->writesize > PL353_NAND_ECC_SIZE) {
> >>> +				cmd_data |= nfc_op.addrs << 16;
> >>> +				/* Another address cycle for devices > 128MiB  
> >> */  
> >>> +				if (chip->chipsize > (128 << 20)) {
> >>> +					pl353_nand_write32(cmd_addr,  
> >> cmd_data);  
> >>> +					cmd_data = (nfc_op.addrs >> 16);
> >>> +				}
> >>> +			}
> >>> +		}  else {
> >>> +			if (nfc_op.addrs != -1) {
> >>> +				int column = nfc_op.addrs;
> >>> +				/*
> >>> +				 * Change read/write column, read id etc
> >>> +				 * Adjust columns for 16 bit bus width
> >>> +				 */
> >>> +				if ((chip->options & NAND_BUSWIDTH_16) &&
> >>> +				((nfc_op.cmnds[0] == NAND_CMD_READ0) ||
> >>> +				(nfc_op.cmnds[0] == NAND_CMD_SEQIN) ||
> >>> +				(nfc_op.cmnds[0] == NAND_CMD_RNDOUT) ||
> >>> +				(nfc_op.cmnds[0] == NAND_CMD_RNDIN))) {  
> >>
> >> Alignment.  
> > Ok, I will update it.  
> >>  
>  [...]  
> >>
> >> Why?  
> > I found some errors during on_die testing, hence I added this delay.
> > Let me check once  
> 
> Either the controller or nand itself requires a settle period after commands are delivered, before data may be read/written. As I understand, this ndelay() is intended to facilitate that.

NAND chips expect a tCCS time (~500ns) after a RNDIN/RNDOUT. Does this
controller cannot take care of it directly? If yes you might want to
implement the ->setup_data_interface() hook.

Otherwise if you are sure this is an hardware bug, it would probably
be preferable to use Haris' barrier together with a comment explaining
the situation.

Thanks,
Miquèl

> 
> I discovered a small bug in this workflow a few years ago and submitted the following patch to linux-xlnx: https://github.com/Xilinx/linux-xlnx/pull/106
> 
> The first byte of a sub-page read is sometimes corrupted (I.e. READ0 followed by RNDOUT command sequence) because (I suspect) RNDOUT commands can linger somewhere on the interconnect between the cpu and pl353 during the 100ns delay period, whereas other commands are synchronized via status change or interrupt to indicate completion. A data store barrier prior to ndelay() seems to address this issue. It hasn't reproduced in about 6 months of read-stress testing on a Zynq-7020 SoC with this change.
> 
> Have you considered pulling this change?
> If not, I'm curious how (if at all) you'd recommend implementing partial page reads with the pl353?
> 
> Thanks,
> Haris
> 
> >>  
> >>> +		if (nfc_op.cmnds[0] == 0xef)
> >>> +			nfc_op.wait = false;
> >>> +		if (nfc_op.wait) {
> >>> +			nfc_op.wait = false;  
> >>
> >> You can remove this line.  
> > Yes, it is initializing to zero in cmd_function.  
> >>  
> >>> +			do {
> >>> +				if (chip->dev_ready(mtd))
> >>> +					break;
> >>> +				cpu_relax();
> >>> +			} while (!time_after_eq(jiffies, timeout));
> >>> +			if (time_after_eq(jiffies, timeout)) {
> >>> +				pr_err("%s timed out\n", __func__);
> >>> +				return -ETIMEDOUT;
> >>> +			}  
> >>
> >> Same comment about the wait.  
> > Ok, I will update it.  
> >>  
> >>> +			return 0;
> >>> +		}
> >>> +	}
> >>> +
> >>> +	if (instr == NULL)  
> >>
> >> if (!instr)  
> > I will modify it.  
> >>  
> >>> +		return 0;
> >>> +	if (instr->type == NAND_OP_DATA_IN_INSTR)
> >>> +		return pl353_nand_read_buf(chip, instr->ctx.data.buf.in, len);
> >>> +
> >>> +	if (instr->type == NAND_OP_DATA_OUT_INSTR) {
> >>> +		if ((nfc_op.cmnds[0] == NAND_CMD_PAGEPROG) ||
> >>> +			(nfc_op.cmnds[0] == NAND_CMD_SEQIN))
> >>> +			pl353_nand_write_page_raw(mtd, chip,
> >>> +			instr->ctx.data.buf.out, 0, nfc_op.addrs);
> >>> +		else
> >>> +			pl353_nand_write_buf_l(chip, instr->ctx.data.buf.out,
> >>> +				len);
> >>> +		return 0;
> >>> +	}
> >>> +	return 0;
> >>> +}
> >>> +
-- 
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com