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

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

On Thu, 22 Feb 2018 00:23:15 +0100
Stefan Agner <stefan@agner.ch> wrote:

> On 21.02.2018 09:28, Boris Brezillon 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.  
> 
> Have implemented those callbacks and the driver mounts UBI just fine.
> However, when trying to use the NAND tests this currently fails:
> 
> [   27.458447] =================================================
> [   27.464427] mtd_oobtest: MTD device: 3
> [   27.481006] mtd_oobtest: MTD device size 16777216, eraseblock size
> 131072, page size 2048, count of eraseblocks 128, pages per eraseblock
> 64, OOB size 64
> [   27.495154] mtd_test: scanning for bad eraseblocks
> [   27.500002] mtd_test: block 12 is bad
> [   27.505482] mtd_test: scanned 128 eraseblocks, 1 are bad
> [   27.510940] mtd_oobtest: test 1 of 5
> [   27.587528] mtd_oobtest: writing OOBs of whole device
> [   27.593113] mtd_oobtest: error: writeoob failed at 0x0
> [   27.598274] mtd_oobtest: error: use_len 2, use_offset 0
> [   27.605190] mtd_oobtest: error -5 occurred
> [   27.609320] =================================================
> insmod: ERROR: could not insert module mtd_oobtest.ko: Input/output
> error
> root@colibri-vf:~# insmod mtd_nandbiterrs.ko dev=3
> [   30.075103]
> [   30.076653] ==================================================
> [   30.082770] mtd_nandbiterrs: MTD device: 3
> [   30.100602] mtd_nandbiterrs: MTD device size 16777216,
> eraseblock=131072, page=2048, oob=64
> [   30.109019] mtd_nandbiterrs: Device uses 1 subpages of 2048 bytes
> [   30.115260] mtd_nandbiterrs: Using page=0, offset=0, eraseblock=0
> [   30.123679] mtd_nandbiterrs: incremental biterrors test
> [   30.129080] mtd_nandbiterrs: write_page
> [   30.134474] mtd_nandbiterrs: rewrite page
> [   30.138702] mtd_nandbiterrs: error: write_oob failed (-5)
> 
> 
> I did overwrite ->{read,write}_{page,oob}[_raw]() and set ->page_access
> in all of those, but still...
> 
> Do you have an idea what that could be? Otherwise will have to look
> closer what might cause this.

Look like your ->write_oob() method is returning -EIO, but I can't help
you without the code. Can you push it on a public git repo?


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