Messages in this thread | | | From | Dario Binacchi <> | Date | Mon, 17 Jan 2022 15:08:30 +0100 | Subject | Re: [RFC PATCH v2 3/5] mtd: rawnand: gpmi: use a table to get EDO mode setup |
| |
Hi Sascha,
On Mon, Jan 17, 2022 at 2:18 PM Sascha Hauer <sha@pengutronix.de> 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@amarulasolutions.com
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310 info@amarulasolutions.com
www.amarulasolutions.com
| |