Re: [LINUX PATCH v10 2/2] mtd: rawnand: arasan: Add support for Arasan NAND Flash Controller

[Boris Brezillon]

On Sat, 18 Aug 2018 05:49:32 +0000
Naga Sureshkumar Relli <nagasure@xxxxxxxxxx> wrote:

> Hi Boris,
> 
> Thanks for the review.
> 
> > -----Original Message-----
> > From: Boris Brezillon [mailto:boris.brezillon@xxxxxxxxxxx]
> > Sent: Friday, August 17, 2018 11:29 PM
> > To: Naga Sureshkumar Relli <nagasure@xxxxxxxxxx>
> > Cc: miquel.raynal@xxxxxxxxxxx; richard@xxxxxx; dwmw2@xxxxxxxxxxxxx;
> > computersforpeace@xxxxxxxxx; marek.vasut@xxxxxxxxx; kyungmin.park@xxxxxxxxxxx;
> > absahu@xxxxxxxxxxxxxx; peterpandong@xxxxxxxxxx; frieder.schrempf@xxxxxxxxx; linux-
> > mtd@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Michal Simek <michals@xxxxxxxxxx>;
> > nagasureshkumarrelli@xxxxxxxxx
> > Subject: Re: [LINUX PATCH v10 2/2] mtd: rawnand: arasan: Add support for Arasan
> > NAND Flash Controller
> > 
> > Hi Naga,
> > 
> > On Fri, 17 Aug 2018 18:49:24 +0530
> > Naga Sureshkumar Relli <naga.sureshkumar.relli@xxxxxxxxxx> wrote:
> >   
> > > +static int anfc_exec_op_cmd(struct nand_chip *chip,
> > > +				   const struct nand_subop *subop) {
> > > +	const struct nand_op_instr *instr;
> > > +	struct anfc_op nfc_op = {};
> > > +	struct anfc_nand_chip *achip = to_anfc_nand(chip);
> > > +	struct anfc_nand_controller *nfc = to_anfc(chip->controller);
> > > +	struct mtd_info *mtd = nand_to_mtd(chip);
> > > +	u32 addrcycles;
> > > +	unsigned int op_id, len = 0;
> > > +	bool reading;
> > > +
> > > +	anfc_parse_instructions(chip, subop, &nfc_op);
> > > +	instr = nfc_op.data_instr;
> > > +	op_id = nfc_op.data_instr_idx;
> > > +	if (nfc_op.data_instr)
> > > +		len = nand_subop_get_data_len(subop, op_id);
> > > +
> > > +	/*
> > > +	 * The switch case is to prepare a command and to set page/column
> > > +	 * address. Arasan NAND controller has program register(Off: 0x10)),
> > > +	 * which needs to be set for every command.
> > > +	 * Ex: When NAND_CMD_RESET is issued, then we need to set reset bit
> > > +	 * in program_register. etc..
> > > +	 */
> > > +	switch (nfc_op.cmnds[0]) {
> > > +	case NAND_CMD_SEQIN:
> > > +		addrcycles = achip->raddr_cycles + achip->caddr_cycles;
> > > +
> > > +		anfc_prepare_cmd(nfc, nfc_op.cmnds[0], NAND_CMD_PAGEPROG, 1,
> > > +				 mtd->writesize, addrcycles);
> > > +		anfc_setpagecoladdr(nfc, nfc_op.row, nfc_op.col);
> > > +		break;
> > > +	case NAND_CMD_READOOB:
> > > +		nfc_op.col += 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, nfc_op.row, nfc_op.col);
> > > +		if (!nfc_op.data_instr)
> > > +			return 0;
> > > +
> > > +		anfc_read_data_op(mtd, instr->ctx.data.buf.in, len);
> > > +		break;
> > > +	case NAND_CMD_RNDOUT:
> > > +		anfc_prepare_cmd(nfc, nfc_op.cmnds[0], NAND_CMD_RNDOUTSTART, 1,
> > > +				 mtd->writesize, 2);
> > > +		anfc_setpagecoladdr(nfc, nfc_op.row, nfc_op.col);
> > > +		nfc->prog = PROG_PGRD;
> > > +		break;
> > > +	case NAND_CMD_PARAM:
> > > +		anfc_prepare_cmd(nfc, nfc_op.cmnds[0], 0, 0, 0, 1);
> > > +		anfc_setpagecoladdr(nfc, nfc_op.row, nfc_op.col);
> > > +		nfc->prog = PROG_RDPARAM;
> > > +		break;
> > > +	case NAND_CMD_READID:
> > > +		anfc_prepare_cmd(nfc, nfc_op.cmnds[0], 0, 0, 0, 1);
> > > +		anfc_setpagecoladdr(nfc, nfc_op.row, nfc_op.col);
> > > +		nfc->prog = PROG_RDID;
> > > +		break;
> > > +	case NAND_CMD_GET_FEATURES:
> > > +		anfc_prepare_cmd(nfc, nfc_op.cmnds[0], 0, 0, 0, 1);
> > > +		anfc_setpagecoladdr(nfc, nfc_op.row, nfc_op.col);
> > > +		nfc->prog = PROG_GET_FEATURE;
> > > +		break;
> > > +	case NAND_CMD_SET_FEATURES:
> > > +		anfc_prepare_cmd(nfc, nfc_op.cmnds[0], 0, 0, 0, 1);
> > > +		anfc_setpagecoladdr(nfc, nfc_op.row, nfc_op.col);
> > > +		nfc->prog = PROG_SET_FEATURE;
> > > +		break;
> > > +	case NAND_CMD_ERASE1:
> > > +		anfc_erase_function(chip, nfc_op);
> > > +		break;
> > > +	default:
> > > +		break;
> > > +	}  
> > 
> > Looks like you have one of these smart controllers where everything is hardcoded and new
> > commands (like vendor specific commands) can't be supported, and we're back to abusing -  
> > >exec_op(), just like ->cmdfunc() was abused.  
> Actually hardcoding commands with ->exec_op() interface in the driver is really looking weird.
> I agree with that.
> But as per the spec, for every command, we need to set respective bit in PROG_REG and because
> Of this, we need to track the commands for each exec_op() call.
> > 
> > Don't you have a way to send raw CMD/ADDR/DATA cycles? If not, then we'll have to
> > consider other options, because I don't want to go back to the situation we are in with -  
> > >cmdfunc().  
> As I said above, for each command we need to set a bit in PROG_REG, to initiate the operation.
> The only conflicting thing is that, setting a respective bit in PROG_REG based on the command
> Needs command tracking. 
> > 
> > Maybe I already asked, but is there a public spec for this IP?  
> I didn't find any public spec for this IP, but you can find the register data base at below link
> https://www.xilinx.com/html_docs/registers/ug1087/ug1087-zynq-ultrascale-registers.html
> and click on NAND.
> There we have Command Reg(Off: 0x0C) and Program Reg(Off: 0x10), which describes the usage.
> 
> Also not in depth but at least something is documented in TRM
> https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf
> we can find the programming model in chapter 25.
> Please let me know if I missed anything.

I had a closer look at those documents (which I remember reading a
while back BTW), and the PROG bits are indeed not described in detail.

Still, I think it's close to what we have on Marvell SoC, where those
'modes' actually encode a specific sequence of timing that is used for
well-known commands (like ERASE, RESET, READID) but could be used the
same way with other opcodes assuming the sequence is the same.

So, you should use the pattern approach, but not like you do: you
should have a dedicated hook for each pattern which would set PROG bit,
and then a generic helper to fill all other information (opcode,
address cycles, ...).