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 11:46 AM 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. 

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.

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