Re: [PATCH -next] mtd: nand: Add support for Toshiba BENAND (Built-in ECC NAND)

[Boris Brezillon]

On Mon, 5 Jun 2017 14:36:23 +0900
KOBAYASHI Yoshitake <yoshitake.kobayashi@xxxxxxxxxxxxx> wrote:

> Hi Boris,
> 
> Thank you very much for your detailed review.
> I just comming back from busy week for conference and started to look it.
> 
> I have a question regarding to the following comment.
> 
> >>  static int toshiba_nand_init(struct nand_chip *chip)
> >>  {
> >> +	struct mtd_info *mtd = nand_to_mtd(chip);
> >> +
> >>  	if (nand_is_slc(chip))
> >>  		chip->bbt_options |= NAND_BBT_SCAN2NDPAGE;
> >>  
> >> +	if (chip->ecc.mode == NAND_ECC_BENAND) {
> >> +		chip->ecc.options = NAND_ECC_CUSTOM_PAGE_ACCESS;
> >> +		chip->ecc.bytes = 0;  
> > 
> > It should be set to 16 according to the datasheet. But I guess this is
> > not exactly true. I'm pretty sure we can use some of these bytes to
> > store real data. Assuming you're using BCH, only 13bytes are needed for
> > 8bits/512bytes strength, and I guess the BBM region is also left
> > untouched (first 2 bytes of the OOB region).  
> 
> On BENAND, all OOB reginon can be used by user (driver). The calculated
> ECC data stored into other isolated area which is ubable to access from user.
> This is why chip->ecc.bytes = 0.

Oh, nice!

> To make sure this specification, I also checked the BENAND datasheet
> but unfortunatelly it was undocumented. Sorry for this confusion.

No problem. Do you think you can update the datasheet (or ask someone
who can) to clarify this aspect? As you said, it's really not clear
that these ECC bytes are actually stored in a dedicated region that is
invisible to users.

> 
> I think I can add comment here or add definition somewhere to describe this
> specification.

Yes please, add a comment in the code, just above the ->ecc.bytes = 0
assignment.

> If you have any preference or suggestion to describe this, please let me know.

Nope. Just put the explanation you give here.