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

[Stefan Agner]

On 21.02.2018 09:35, Boris Brezillon wrote:
> On Wed, 21 Feb 2018 09:28:21 +0100
> Boris Brezillon <boris.brezillon at bootlin.com> wrote:
> 
>> On Wed, 21 Feb 2018 00:15:18 +0100
>> Stefan Agner <stefan at 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.

That sounds scary. I think bad block scan is done through oob reads
only. I guess if we only implement raw read/write and ecc read/write,
the stack will actually read OOB with proper ending when implemented as
you suggested.

--
Stefan