Re: [RFC PATCH v3 2/3] mtd: nand: vf610_nfc: make use of ->exec_op()

[Boris Brezillon <boris.brezillon@bootlin.com>]

On Wed, 21 Feb 2018 09:28:21 +0100
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> On Wed, 21 Feb 2018 00:15:18 +0100
> Stefan Agner <stefan@agner.ch> wrote:
> 
> > >> +		}
> > >> +
> > >> +		row = ROW_ADDR(0, instr->ctx.addr.addrs[addr++]);
> > >> +		code |= COMMAND_RAR_BYTE1;
> > >> +		if (addr < instr->ctx.addr.naddrs) {
> > >> +			row |= ROW_ADDR(1, instr->ctx.addr.addrs[addr++]);
> > >> +			code |= COMMAND_RAR_BYTE2;
> > >> +		}
> > >> +		if (addr < instr->ctx.addr.naddrs) {
> > >> +			row |= ROW_ADDR(2, instr->ctx.addr.addrs[addr++]);
> > >> +			code |= COMMAND_RAR_BYTE3;
> > >> +		}
> > >> +
> > >> +		dev_dbg(nfc->dev, "OP_ADDR: col %d, row %d\n", col, row);
> > >> +
> > >> +		instr = vf610_get_next_instr(subop, &op_id);
> > >> +	}
> > >> +
> > >> +	if (instr && instr->type == NAND_OP_DATA_OUT_INSTR) {
> > >> +		int len = nand_subop_get_data_len(subop, op_id);
> > >> +		int offset = nand_subop_get_data_start_off(subop, op_id);
> > >> +
> > >> +		dev_dbg(nfc->dev, "OP_DATA_OUT: len %d, offset %d\n", len, offset);
> > >> +
> > >> +		vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + offset,
> > >> +				 instr->ctx.data.buf.in + offset,
> > >> +				 len);    
> > > 
> > > I think you have the same endianness problem you have for the READ
> > > path. For example, I doubt SET_FEATURES will work properly if you're
> > > in LE. So I repeat my initial suggestion: always do the byte swapping
> > > when you're transfering data to/from the SRAM from vf610_nfc_cmd()
> > > and use vf610_nfc_memcpy() only in the ->read/write_page()
> > > implementations. 
> > >     
> > 
> > Hm, but doesn't that leads to wrong order of data when using e.g. raw
> > read/write page...?  
> 
> Yep you'll have to implement ->{read,write}_{page,oob}[_raw](), but I
> prefer that to having an ->exec_op() implementation that tries to guess
> what the core is trying to do.
> 

One more thing, in its current state the driver may fail to detect
blocks marked bad by the manufacturer because of this endianness issue.
That should work most of the time because manufacturers tend to fill
the whole block with zeros when they want to mark it bad, but they're
not required to do that.

-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com