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

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

Hi Rickard,

Rickard X Andersson <Rickard.Andersson@xxxxxxxx> wrote on Tue, 26 May
2020 10:10:58 +0000:

> On Tue, May 26, 2020 at 11:46 AM Miquel Raynal <miquel.raynal@xxxxxxxxxxx> 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

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