Re: [PATCH v6 00/15] A SPI NAND framework under generic NAND framework

[Boris Brezillon <boris.brezillon@free-electrons.com>]

Hi Frieder,

On Thu, 21 Dec 2017 12:48:16 +0100
Frieder Schrempf <frieder.schrempf@exceet.de> wrote:

> Hello Boris,
> 
> >>>> So shouldn't there be an spinand_die_select_op in the SPI NAND core that
> >>>> is executed when necessary and skipped if there's only one die.  
> >>>
> >>> Sure, I was arguing against a ->select_chip() at the generic NAND
> >>> level. That's something you can add in the SPI NAND framework.  
> 
> I added an op to send the die selection command and call it, where I 
> think it is needed: [1]
> 
> Accessing both dies on the Winbond SPI NAND works fine like this.

Okay, I'll have a look.

> 
> While running tests I came across some problems:
> 
> * On accessing the BBT in RAM via nanddev_bbt_[set/get]_block_status, 
> the calculation of position and offset seems to be wrong.
> My fix is here: [2]

Indeed.

> 
> * On calculating the row address for read/program/erase via 
> nanddev_pos_to_row, it seems like the eraseblock_addr_shift is wrong.

My version was incorrect, but yours is not good either :-), should
be:

	nand->rowconv.eraseblock_addr_shift =
				fls(memorg->pages_per_eraseblock - 1);

otherwise it doesn't work when the number of pages per block is not a
power of 2, and that can happen :-/.

> 
> * Also, I'm not sure if the LUN should be taken into account when 
> calculating the row address. At least when you select the LUN by issuing 
> a separate command, the row address sent to the chip should only contain 
> the page address.

The LUN id is part of the row address on parallel ONFI NANDs. Are you
sure what you're trying to access is actually a LUN?

The ONFI spec says:
"
LUN (logical unit number)
The minimum unit that can independently execute commands and report
status. There are one or more LUNs per NAND Target.
"

I suspect what you're trying to expose is a chip with 2 targets
(target is a synonym for die). If this is the case, then you should
have:

	luns_per_target = 1;
	ntargets = 2;	/* ntargets <=> number of dies */

> But I'm not sure if that's the case in general, or if some code is 
> needed to differentiate.
> 
> See my changes of nanddev_pos_to_row here: [3]
> 
> * I run into a mutex deadlock, when spinand_write_page fails (e.g. 
> because of a bad block) as the lock is not released in such cases. See 
> my fix here: [4]

This is a valid fix.

> 
> With these fixes applied and as far as I can tell everything seems to 
> work fine. I'll do some tests with UBI next and look into the ECC topic.

Okay cool.

I'll squash your fixes in the original commits and push an updated
version on my repo.

Thanks,

Boris