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

[Naga Sureshkumar Relli]

Hi Boris,

Thanks for the review.

> -----Original Message-----
> From: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> Sent: Wednesday, July 3, 2019 11:56 AM
> To: Naga Sureshkumar Relli <nagasure@xxxxxxxxxx>
> Cc: miquel.raynal@xxxxxxxxxxx; helmut.grohne@xxxxxxxxxx; richard@xxxxxx;
> dwmw2@xxxxxxxxxxxxx; computersforpeace@xxxxxxxxx; marek.vasut@xxxxxxxxx;
> vigneshr@xxxxxx; bbrezillon@xxxxxxxxxx; yamada.masahiro@xxxxxxxxxxxxx; linux-
> mtd@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [LINUX PATCH v17 2/2] mtd: rawnand: pl353: Add basic driver for arm pl353
> smc nand interface
> 
> On Mon, 24 Jun 2019 22:46:30 -0600
> Naga Sureshkumar Relli <naga.sureshkumar.relli@xxxxxxxxxx> wrote:
> 
> 
> > +
> > +/**
> > + * pl353_nand_exec_op_cmd - 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_exec_op_cmd(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 = {};
> > +	struct pl353_nand_controller *xnfc = to_pl353_nand(chip);
> > +	unsigned long cmd_phase_data = 0, end_cmd_valid = 0;
> > +	unsigned long end_cmd;
> > +	unsigned int op_id, len;
> > +	bool reading;
> > +	u32 cmdphase_addrflags;
> > +
> > +	pl353_nfc_parse_instructions(chip, subop, &nfc_op);
> > +	instr = nfc_op.data_instr;
> > +	op_id = nfc_op.data_instr_idx;
> > +	pl353_smc_clr_nand_int();
> > +
> > +	/* Get the command phase address */
> > +	if (nfc_op.cmnds[1] != 0) {
> > +		if (nfc_op.cmnds[0] == NAND_CMD_SEQIN)
> > +			end_cmd_valid = 0;
> > +		else
> > +			end_cmd_valid = 1;
> 
> You're testing the opcode, again. As I said several times, the
> ->exec_op() implementation should be opcode agnostic, it should just try
> to match sequences of <CMD>-<ADDR>-<DATA> cycles.
> 
This driver uses common function for all patterns.
There was some discussion happened on v8 series 
https://lore.kernel.org/patchwork/patch/933639/
There the comments from Miquel was to use an optional property In the pattern
Matching, so with this approach, based on the command need to update the 
end_cmd_valid bit in command phase cycle.
So in order to follow that approach, we defined a common pattern matching function
And there we are checking the commands.
It significantly reduces the code repetition.

I understand your concern about not to check any NAND command in the drivers
under ->exec_op() implementation.
But do you see any issues/impact with this?
Functionality wise Helmut tested each series and we addressed all the comments in v17 series.

Could you please let me know what do you say?

> > +	}
> > +
> > +	end_cmd = nfc_op.cmnds[1];
> > +
> > +	/*
> > +	 * The SMC defines two phases of commands when transferring data to or
> > +	 * from NAND flash.
> > +	 * Command phase: Commands and optional address information are written
> > +	 * to the NAND flash.The command and address can be associated with
> > +	 * either a data phase operation to write to or read from the array,
> > +	 * or a status/ID register transfer.
> > +	 * Data phase: Data is either written to or read from the NAND flash.
> > +	 * This data can be either data transferred to or from the array,
> > +	 * or status/ID register information.
> > +	 */
> > +	cmdphase_addrflags = ((nfc_op.naddrs << ADDR_CYCLES_SHIFT) |
> > +			 (end_cmd_valid << END_CMD_VALID_SHIFT) |
> > +			 (COMMAND_PHASE) |
> > +			 (end_cmd << END_CMD_SHIFT) |
> > +			 (nfc_op.cmnds[0] << START_CMD_SHIFT));
> > +
> > +	/* Get the data phase address */
> > +	end_cmd_valid = 0;
> > +
> > +	xnfc->dataphase_addrflags = ((0x0 << CLEAR_CS_SHIFT) |
> > +			  (end_cmd_valid << END_CMD_VALID_SHIFT) |
> > +			  (DATA_PHASE) |
> > +			  (end_cmd << END_CMD_SHIFT) |
> > +			  (0x0 << ECC_LAST_SHIFT));
> > +
> > +	/* Command phase AXI Read & Write */
> > +	if (nfc_op.naddrs >= 5) {
> > +		if (mtd->writesize > PL353_NAND_ECC_SIZE) {
> > +			cmd_phase_data = nfc_op.addrs;
> > +
> > +			/* Another address cycle for devices > 128MiB */
> > +			if (chip->options & NAND_ROW_ADDR_3) {
> 
> Clearly, none of this belongs in the ->exec_op() implementation. Looks like something related
> to page read...
As I mentioned above in comments of pl353_exec_op(), the PL353 SMC
Controller uses command phase and data phase.
And in the Command phase, command and optional addresses are written to NAND flash.
And it is correct as you said, it looks like page reads but it is actually a command phase address
update.
> 
> > +				writel_relaxed(cmd_phase_data,
> > +					       xnfc->regs + cmdphase_addrflags);
> > +				cmd_phase_data = nfc_op.addrs_56;
> > +			}
> > +		}
> > +	}  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)) {
> > +				column >>= 1;
> 
> And you keep testing opcodes here. Note that the address cycles should have been adjusted by
> core already when we have 16-bit accesses.
> 
> 
> > +			}
> > +			cmd_phase_data = column;
> > +		}
> > +	}
> > +
> > +	writel_relaxed(cmd_phase_data, xnfc->regs + cmdphase_addrflags);
> > +	if (!nfc_op.data_instr) {
> > +		if (nfc_op.rdy_timeout_ms) {
> > +			if (pl353_wait_for_dev_ready(chip))
> > +				return -ETIMEDOUT;
> > +		}
> > +
> > +		return 0;
> > +	}
> > +
> > +	reading = (nfc_op.data_instr->type == NAND_OP_DATA_IN_INSTR);
> > +	if (!reading) {
> > +		len = nand_subop_get_data_len(subop, op_id);
> > +		pl353_nand_write_data_op(chip, instr->ctx.data.buf.out,
> > +					 len, instr->ctx.data.force_8bit);
> > +		if (nfc_op.rdy_timeout_ms) {
> > +			if (pl353_wait_for_dev_ready(chip))
> > +				return -ETIMEDOUT;
> > +		}
> > +
> > +		ndelay(nfc_op.rdy_delay_ns);
> > +	} else {
> > +		len = nand_subop_get_data_len(subop, op_id);
> > +		ndelay(nfc_op.rdy_delay_ns);
> > +		if (nfc_op.rdy_timeout_ms) {
> > +			if (pl353_wait_for_dev_ready(chip))
> > +				return -ETIMEDOUT;
> > +		}
> > +
> > +		pl353_nand_read_data_op(chip, instr->ctx.data.buf.in, len,
> > +					instr->ctx.data.force_8bit);
> > +	}
> > +
> > +	return 0;
> > +}

Thanks,
Naga Sureshkumar Relli