SV: [PATCH v4 17/19] mtd: rawnand: Introduce nand_choose_best_vendor_sdr_iface()

[Rickard X Andersson <Rickard.Andersson@axis.com>]

On Tue, May 26, 2020 at 12:44 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> > > > > +/**
> > > > > + * nand_choose_best_sdr_iface - given a data interface, find the closest
> > > > > + *                              mode/timings set for this interface supported
> > > > > + *                              by both the NAND controller and the NAND chip
> > > > > + * @chip: the NAND chip
> > > > > + * @best_iface: the best data interface (can eventually be updated)
> > > > > + */
> > > > > +static int nand_choose_best_sdr_iface(struct nand_chip *chip,
> > > > > +                                     struct nand_data_interface *best_iface)
> > > > > +{
> > > > > +       const struct nand_controller_ops *ops = chip->controller->ops;
> > > > > +       int mode, ret;
> > > > > +
> > > > > +       /* Verify the controller supports the requested interface */
> > > > > +       ret = ops->setup_data_interface(chip, NAND_DATA_IFACE_CHECK_ONLY,
> > > > > +                                       best_iface);
> > > > > +       if (!ret)
> > > > > +               return ret;
> > > > > +
> > > > > +       /* Fallback to slower modes */
> > > > > +       for (mode = best_iface->timings.mode - 1; mode >= 0; mode--) {
> > > > > +               ret = onfi_fill_data_interface(chip, best_iface,
> > > > > +                                              NAND_SDR_IFACE, mode);
> > > > > +               if (ret)
> > > > > +                       continue;
> > > > > +
> > > > > +               ret = ops->setup_data_interface(chip,
> > > > > +                                               NAND_DATA_IFACE_CHECK_ONLY,
> > > > > +                                               best_iface);
> > > > > +               if (!ret)
> > > > > +                       break;
> > > > > +       }
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > > +   
> > > >
> > > > Should we not start looping from "mode = best_iface->timings.mode" ? The first setup_data_interface call in the above function tests the specific timings or am I missing something? 
> > >
> > > Indeed, we assume that the controller driver will not support the
> > > "official" ONFI timings mode X if it did not support the specific
> > > timings close to mode X.
> > >
> > > This is an assumption, but I don't think we are far from the reality
> > > here.
> > >
> > > Miquèl 
> >
> > It could be that the "corresponding onfi mode" is quite low due to deviation of some parameter that are actually not checked/used by the controller drivers.
>
> That might happen. I'll change it.
>
> > Another thing, should we not check in order to be sure that mode does not become negative? I.e if best_iface->timings.mode is zero before executing the loop.
>
> I think the for-loop "mode >= 0" condition ensures this will never
> happen, right?
>
> Thanks,
> Miquèl

If best_iface->timings.mode is zero before entering the loop we will not call onfi_fill_data_interface at all.

BR,
Rickard
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/