Re: [RFC PATCH v2 3/5] mtd: rawnand: gpmi: use a table to get EDO mode setup

[Sascha Hauer]

On Mon, Jan 17, 2022 at 03:08:30PM +0100, Dario Binacchi wrote:
> Hi Sascha,
> 
> On Mon, Jan 17, 2022 at 2:18 PM Sascha Hauer <sha@xxxxxxxxxxxxxx> wrote:
> >
> > Hi Dario,
> >
> > On Mon, Jan 17, 2022 at 12:18:27PM +0100, Dario Binacchi wrote:
> > > +struct edo_mode {
> > > +     u32 tRC_min;
> > > +     long clk_rate;
> > > +     u8 wrn_dly_sel;
> > > +};
> > > +
> > > +static const struct edo_mode edo_modes[] = {
> > > +     {.tRC_min = 30000, .clk_rate = 22000000,
> > > +      .wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_4_TO_8NS},
> > > +     {.tRC_min = 30000, .clk_rate = 22000000,
> > > +      .wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_4_TO_8NS},
> > > +     {.tRC_min = 30000, .clk_rate = 22000000,
> > > +      .wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_4_TO_8NS},
> > > +     {.tRC_min = 30000, .clk_rate = 22000000,
> > > +      .wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_4_TO_8NS},
> > > +     {.tRC_min = 25000, .clk_rate = 80000000,
> > > +      .wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY},
> > > +     {.tRC_min = 20000, .clk_rate = 100000000,
> > > +      .wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY},
> > > +};
> > > +
> > >  /*
> > >   * <1> Firstly, we should know what's the GPMI-clock means.
> > >   *     The GPMI-clock is the internal clock in the gpmi nand controller.
> > > @@ -657,22 +678,18 @@ static void gpmi_nfc_compute_timings(struct gpmi_nand_data *this,
> > >       int sample_delay_ps, sample_delay_factor;
> > >       u16 busy_timeout_cycles;
> > >       u8 wrn_dly_sel;
> > > +     int i, emode = ARRAY_SIZE(edo_modes) - 1;
> > >
> > > -     if (sdr->tRC_min >= 30000) {
> > > -             /* ONFI non-EDO modes [0-3] */
> > > -             hw->clk_rate = 22000000;
> > > -             wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_4_TO_8NS;
> > > -     } else if (sdr->tRC_min >= 25000) {
> > > -             /* ONFI EDO mode 4 */
> > > -             hw->clk_rate = 80000000;
> > > -             wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY;
> > > -     } else {
> > > -             /* ONFI EDO mode 5 */
> > > -             hw->clk_rate = 100000000;
> > > -             wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY;
> > > +     /* Search the required EDO mode */
> > > +     for (i = 0; i < ARRAY_SIZE(edo_modes); i++) {
> > > +             if (sdr->tRC_min >= edo_modes[i].tRC_min) {
> > > +                     emode = i;
> > > +                     break;
> > > +             }
> >
> > The first four entries of edo_modes[] all have the same value, so this loop
> > will never end on the second, third or fourth element. These eleqments are just
> > there to match 'emode' with the existing ONFI mode numbers, but then 'emode' is
> > never used as an ONFI mode number, instead it's only used as an index to the
> > array. You could equally well remove the second till fourth array entries.
> >
> > Then with only three entries left in the array I wonder if you're not better
> > off with the original code and change it to something like:
> 
> This implementation is justified by the next patch in the series ([RFC
> PATCH v2 4/5] mtd: rawnand: gpmi: validate controller clock rate).
> I thought that clock rate validation could be better implemented by
> indexing a table. The replication of the second, third and fourth
> elements
> makes the index also the edo mode (used in the debug printout). Using
> a less redundant table (three elements) requires the edo mode
> to be stored in it if you want to keep the debug printing or remove
> the debug message.
> 
> >
> >         if (sdr->tRC_min >= 30000) {
> >                 /* ONFI non-EDO modes [0-3] */
> >                 hw->clk_rate = 22000000;
> >                 min_rate = 0;
> >                 wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_4_TO_8NS;
> >         } else if (sdr->tRC_min >= 25000) {
> >                 /* ONFI EDO mode 4 */
> >                 hw->clk_rate = 80000000;
> >                 min_rate = 22000000;
> >                 wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY;
> >         } else {
> >                 /* ONFI EDO mode 5 */
> >                 hw->clk_rate = 100000000;
> >                 min_rate = 80000000;
> >                 wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY;
> >         }
> >
> >         hw->clk_rate = clk_round_rate(r->clock[0], hw->clk_rate);
> >         if (hw->clk_rate < min_rate)
> >                 return -EINVAL;
> >
> > I think this would be easier to follow.
> 
> I think the key point is to decide if the patch "[RFC PATCH v2 4/5]
> mtd: rawnand: gpmi: validate controller clock rate" makes sense.
> I thought of this patch to facilitate that implementation, since it
> compares the real GPMI clock rate to that of the previous edo mode.
> I thought it wasn't elegant to implement another switch case.

You don't have to implement another switch/case. It's already there.

To be more clear my suggestion was to replace 3/5 and 4/5 with the
following.

Sascha

---------------------------8<--------------------------------