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

[Schrempf Frieder]

Hi Boris,

On 28.11.18 13:53, Boris Brezillon wrote:
> On Tue, 27 Nov 2018 16:41:56 +0000
> Schrempf Frieder <frieder.schrempf@xxxxxxxxxx> 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.

Thank you for clarifying this. I will send a v2 of my patch and revert 
this change.

Thanks,
Frieder

> 
>>
>>>    
>>>>> +
>>>>> +
>>>>> +     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.
>