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

[Dario Binacchi]

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 elements 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.

Thanks and regards,
Dario

>
> Sascha
>
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |



--

Dario Binacchi

Embedded Linux Developer

dario.binacchi@xxxxxxxxxxxxxxxxxxxx

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info@xxxxxxxxxxxxxxxxxxxx

www.amarulasolutions.com