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

Schrempf Frieder
2018-11-28T13:02:00Z

Hi Boris,

On 28.11.18 13:53, Boris Brezillon wrote:
> 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.

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