Re: [PATCH v2] mtd: spinand: Add support for AllianceMemory AS5F34G04SND

Miquel Raynal
2023-01-25T14:27:59Z

Hi Mario,

dev@kicherer.org wrote on Wed, 25 Jan 2023 13:13:13 +0100:

> Hello Miquel,
>
> On 2023-01-23 11:33, Miquel Raynal wrote:
> >> +static int as5f34g04snd_ooblayout_ecc(struct mtd_info *mtd, int >> section,
> >> + struct mtd_oob_region *region)
> >> +{
> >> + if (section > 3)
> >> + return -ERANGE;
> >> +
> >> + region->offset = 0x48;
> >> + region->length = 0x38;
> >
> > Why would you have three sections if you have static offsets and
> > length? That does not look correct.
>
> Hm, I do not remember. I guess this is a leftover I forgot to remove.
>
> >> + /*
> >> + * It is unclear how many bytes are used for the bad block marker. >> We
> >> + * reserve one byte here.
> >
> > So far we reserved two bytes in Linux to mimic the raw NAND BBM.
>
> Ok, changed.
>
> >> +static int am_ecc_get_status(struct spinand_device *spinand, u8 >> status)
> >> +{
> >> + switch (status & AM_STATUS_ECC_BITMASK) {
> >> + case AM_STATUS_ECC_NONE_DETECTED:
> >> + return 0;
> >> +
> >> + case AM_STATUS_ECC_1_CORRECTED:
> >> + return 1;
> >> +
> >> + case AM_STATUS_ECC_MAX_CORRECTED:
> >> + return 8;
> >> +
> >> + case AM_STATUS_ECC_1_DETECTED:
> >
> > What does this mean "1 detected"?
>
> According to the manual, the chip can report that a bit error was detected
> but could not be corrected.

That does not make a lot of sense. Either you use a Hamming algorithm,
you will be able to correct 1 bit error and to detect 2 bit errors, or
you use another algorithm (BCH, usually) with a strength of X (X > 1)
and you'll be able to correct up to X errors and detect X+1 errors.

But only being able to detect a single bit flip without being able to
correct it is strange (even useless?). So I don't understand how it
should be used.

>
> >> +static const struct spinand_manufacturer_ops >> alliancememory_spinand_manuf_ops = {
> >> + .init = alliancememory_spinand_init,
> >
> > I don't think .init is mandatory, so if you do nothing in it, you
> > can just drop it.
>
> Done.
>
> Thanks for the review!
> Mario