| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CABGWkvruAWcOxX99U0jcK1sz+AoGKKJwHWbtn_kSO=SJsCO5Wg@mail.gmail.com>
Date: Mon, 17 Jan 2022 15:08:30 +0100
From: Dario Binacchi <dario.binacchi@...rulasolutions.com>
To: Sascha Hauer <sha@...gutronix.de>
Cc: linux-kernel@...r.kernel.org,
Michael Trimarchi <michael@...rulasolutions.com>,
Han Xu <han.xu@....com>,
Miquel Raynal <miquel.raynal@...tlin.com>,
Richard Weinberger <richard@....at>,
Vignesh Raghavendra <vigneshr@...com>,
linux-mtd@...ts.infradead.org
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@...gutronix.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@...rulasolutions.com
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info@...rulasolutions.com
www.amarulasolutions.com
Powered by blists - more mailing lists