Re: [PATCH] mtd: nand: spi: Add initial support for Toshiba TC58CVG2S0H

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

On Tue, 27 Nov 2018 16:41:56 +0000
Schrempf Frieder <frieder.schrempf@kontron.de> wrote:

> >>> +static int tc58cvg2s0h_ooblayout_ecc(struct mtd_info *mtd, int section,
> >>> +                                  struct mtd_oob_region *region)
> >>> +{
> >>> +     if (section > 7)
> >>> +             return -ERANGE;
> >>> +
> >>> +     region->offset = 128 + 16 * section;
> >>> +     region->length = 16;  
> > 
> > Here you expose the ECC bits has 8 sections of 16 bytes.
> > But regarding the datasheet this should not be accessed page 32.
> > "The ECC parity code generated by internal ECC is stored in column
> > addresses 4224-4351 and the users cannot access to these specific
> > addresses when internal ECC is enabled."  

'when internal ECC is enabled' means that those bytes can be accessed
when it's disabled. We should keep exposing the ECC byte sections. Note
that even if ECC sections are not exposed, the core will read those
bytes. They're probably filled with garbage in this case, but we don't
care since we won't use them.

> 
> This is just to let the other layers know, where the bytes used for ECC 
> are. As long as the driver uses the on-chip ECC it won't write to this 
> area. So this is correct unless I misunderstood this concept. All the 
> other supported SPI NAND chips use the same approach.

Yes, and that's the right thing to do. We want to know where the ECC
bytes are, especially when doing raw accesses.

> 
> >   
> >>> +
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +
> >>> +static int tc58cvg2s0h_ooblayout_free(struct mtd_info *mtd, int section,
> >>> +                                   struct mtd_oob_region *region)
> >>> +{
> >>> +     if (section > 7)
> >>> +             return -ERANGE;
> >>> +
> >>> +     region->offset = 2 + 16 * section;
> >>> +     region->length = 14;  
> > 
> > This reserved 2 bytes for BBM for each section.
> > But maybe we could declare this as 1 section of 128bytes:
> > 
> > if (section)
> >           return -ERANGE;
> > 
> > region->offset = 2;
> > region->length = 126;  

I agree with this suggestion: if the free bytes are contiguous, it's
better to expose them as a single section.