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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YeVsnQpOkyXaBk0+@pengutronix.de>
Date:   Mon, 17 Jan 2022 14:18:21 +0100
From:   Sascha Hauer <sha@...gutronix.de>
To:     Dario Binacchi <dario.binacchi@...rulasolutions.com>
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 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:

	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.

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 |