[PATCH 2/3] mtd: rawnand: Check the CHANGE_READ_COLUMN from nand_read_subpage() is supported
Boris Brezillon
boris.brezillon at collabora.com
Mon Sep 6 10:59:20 PDT 2021
On Mon, 6 Sep 2021 19:32:55 +0200
Miquel Raynal <miquel.raynal at bootlin.com> wrote:
> > > > > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> > > > > index 7f29f27bb6fd..8175635b9802 100644
> > > > > --- a/drivers/mtd/nand/raw/nand_base.c
> > > > > +++ b/drivers/mtd/nand/raw/nand_base.c
> > > > > @@ -3065,10 +3065,19 @@ static int nand_read_subpage(struct nand_chip *chip, uint32_t data_offs,
> > > > > (busw - 1))
> > > > > aligned_len++;
> > > > >
> > > > > - ret = nand_change_read_column_op(chip,
> > > > > - mtd->writesize + aligned_pos,
> > > > > - &chip->oob_poi[aligned_pos],
> > > > > - aligned_len, false);
> > > > > + ret = nand_check_change_read_column_op(chip,
> > > > > + mtd->writesize + aligned_pos,
> > > > > + &chip->oob_poi[aligned_pos],
> > > > > + aligned_len, false, true);
> > > > > + if (!ret)
> > > > > + ret = nand_change_read_column_op(chip,
> > > > > + mtd->writesize + aligned_pos,
> > > > > + &chip->oob_poi[aligned_pos],
> > > > > + aligned_len, false);
> > > > > + else
> > > > > + ret = nand_change_read_column_op(chip, mtd->writesize,
> > > > > + chip->oob_poi,
> > > > > + mtd->oobsize, false);
> > > > > if (ret)
> > > > > return ret;
> > > >
> > >
> > >
> > > The previous behavior was:
> > >
> > > /*
> > > * gaps == true means we need to request the entire OOB area and we
> > > * will call an OOB layout helper to extract the ECC bytes.
> > > * gaps == false means there is no data interleaved, we can just read
> > > * the number of ECC bytes by starting at the right offset and no
> > > * extracting operation will be needed.
> > > */
> > > gaps = ...;
> > > if (!gaps)
> > > nand_change_read_column(at a specific offset and a specific
> > > length just to match the ECC bytes);
> > > else
> > > nand_change_read_column(the entire OOB);
> > >
> > > While the new behavior is:
> > >
> > > if (!gaps) {
> > > op_supported = nand_check_change_read_column();
> > > if (!op_supported)
> > > // same operation as if gaps == true
> > > nand_change_read_column(the entire OOB);
> >
> > What if this one is not supported either?
>
> If this one is not supported either I guess the entire software ECC
> support cannot be used. But what I am trying to achieve here is more
> complicated: the controller does not support one thing: reading the ECC
> bytes until the OOB end minus 2. Only a call to
> nand_change_read_column with these particular parameters would fail
> because it depends on the actual offset and length that are requested.
I suspect those parameters can be extracted from the NAND page size/ECC
config, which are known before the ECC initialization IIRC.
>
> > > else
> > > nand_change_read_column(at a specific offset);
> > > } else {
> > > nand_change_read_column(the entire OOB)
> > > }
> > >
> > > > So you just fail if the CHANGE column op is not supported? Looks like
> > > > this check should be done before we assign ecc->read_subpage to
> > > > nand_read_subpage in
> > > > nand_set_ecc_on_host_ops()/nand_set_ecc_soft_ops()...
> > >
> > > So I actually don't understand the above comment as the code is not
> > > simply failing, it's actually falling back to second method which is
> > > maybe slightly slower but still valid. Do you think it's wrong?
> >
> > Well, it's falling back to a different method that might be unsupported
> > too, so it might still fail on other controllers.
>
> Well, yes it may fail on other controllers but as I said, if
> controllers do not support any type of nand_change_read_column() then
> they cannot use software ECC at all (and are close to be almost
> useless).
Which would be good to detect at probe time, with a probe failure when
this happens.
>
> > Moreover, I really
> > think this sort of things should be checked at probe time so you don't
> > spend time checking it every time you do a read.
>
> Agreed. I can certainly make something like this in theory, but what
> would be the condition? I am describing it in patch 3/3 : there is
> really a tiny set of conditions where this will fail so in the end I
> would end-up writing a condition that precisely matches the Arasan
> limitation.
I see. Maybe it's simpler to add NAND_NO_SUBPAGE_READ flag until you
find a better way to describe such limitations.
>
> Not mentioning that it only fails with NV-DDR enabled (also something
> that I missed to capture in patch 3/3). The data interface being
> initialized after the read_subpage() hook it means I couldn't use this
> information.
Duh, this is nasty, indeed.
>
> > Something like:
> >
> > if (nand_check_change_read_column(gaps=false))
> > ecc->read_subpage = nand_read_subpage_nogaps;
> > else if (nand_check_change_read_column(gaps=true))
> > ecc->read_subpage = nand_read_subpage_gaps;
> > else
> > return -EOPNOTSUP;
>
More information about the linux-mtd
mailing list