Received-SPF: pass (google.com: domain of linux-kernel+bounces-198711-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Content-Type: multipart/signed;
 boundary=62dfc1cd91faea84d32d705095127aab3d83287f351da8bd6655d6784688;
 micalg=pgp-sha384; protocol="application/pgp-signature"
Date: Mon, 03 Jun 2024 09:25:42 +0200
Message-Id: <D1Q7BU6PJ356.1CTXPUZE8U6XX@kernel.org>
Subject: Re: [PATCH] mtd: spi-nor: macronix: workaround for device id re-use
Cc: <linux-mtd@lists.infradead.org>, <linux-kernel@vger.kernel.org>, "Rasmus
 Villemoes" <rasmus.villemoes@prevas.dk>
From: "Michael Walle" <mwalle@kernel.org>
To: "Esben Haabendal" <esben@geanix.com>, "Tudor Ambarus"
 <tudor.ambarus@linaro.org>, "Pratyush Yadav" <pratyush@kernel.org>, "Miquel
 Raynal" <miquel.raynal@bootlin.com>, "Richard Weinberger" <richard@nod.at>,
 "Vignesh Raghavendra" <vigneshr@ti.com>
References: <20240524-macronix-mx25l3205d-fixups-v1-1-ee152e56afb3@geanix.com>
In-Reply-To: <20240524-macronix-mx25l3205d-fixups-v1-1-ee152e56afb3@geanix.com>
Precedence: bulk

--62dfc1cd91faea84d32d705095127aab3d83287f351da8bd6655d6784688
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8

Hi,

On Fri May 24, 2024 at 12:48 PM CEST, Esben Haabendal wrote:
> Macronix engineers apparantly do not understand the purpose of having
> an ID actually identify the chip and its capabilities. Sigh.
>
> The original Macronix SPI NOR flash that identifies itself as 0xC22016
> with RDID was MX25L3205D. This chip does not support SFDP, but does
> support the 2READ command (1-2-2).
>
> When Macronix announced EoL for MX25L3205D, the recommended
> replacement part was MX25L3206E, which conveniently also identifies
> itself as 0xC22016. It does not support 2READ, but supports DREAD
> (1-1-2) instead, and supports SFDP for discovering this.
>
> When Macronix announced EoL for MX25L3206E, the recommended
> replacement part was MX25L3233F, which also identifies itself as
> 0xC22016. It supports DREAD, 2READ, and the quad modes QREAD (1-1-4)
> and 4READ (1-4-4). This also support SFDP.

Thanks for collecting all this info!

> So far, all of these chips have been handled the same way by the Linux
> driver. The SFDP information have not been read, and no dual and quad
> read modes have been enabled.
>
> The trouble begins when we want to enable the faster read modes. The
> RDID command only return the same 3 bytes for all 3 chips, so that
> doesn't really help.
>
> But we can take advantage of the fact that only the old MX25L3205D
> chip does not support SFDP, so by triggering the old initialization
> mechanism where we try to read and parse SFDP, but has a fall-back
> configuration in place, we can configure all 3 chips to their optimal
> configurations.

You are (mis)using the quad info bits to trigger an sfdp read,
correct? In that case, I'd rather see a new flag in .no_sfdp_flags
to explicitly trigger the SFDP read. Then your new flash would only
need this flag and doesn't require the shenanigans with the fixup,
right?

> With this, MX25L3205D will get the faster 2READ command enabled,
> speading up reads. This should be safe.
>
> MX25L3206E will get the faster DREAD command enabled. This should also
> be safe.
>
> MX25L3233F will get all of DREAD, 2READ, QREAD and 4READ enabled. In
> order for this to actually work, the WP#/SIO2 and HOLD#/SIO3 pins must
> be correctly wired to the SPI controller.

That should already be taken care of with the spi-{tx,rx}-bus-width.

-michael

> Signed-off-by: Esben Haabendal <esben@geanix.com>
> ---
> I only have access to boards with MX25L3233F flashes, so haven't been
> able to test the backwards compatibility. If anybody has boards with
> MX25L3205D and/or MX25L3206E, please help test this patch. Keep an eye
> for read performance regression.
>
> It is worth nothing that both MX25L3205D and MX25L3206E are
> end-of-life, and is unavailable from Macronix, so any new boards
> featuring a Macronix flash with this ID will likely be using
> MX25L3233F.
> ---
>  drivers/mtd/spi-nor/macronix.c | 60 ++++++++++++++++++++++++++++++++++++=
+++++-
>  1 file changed, 59 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macroni=
x.c
> index ea6be95e75a5..c1e64ee3baa3 100644
> --- a/drivers/mtd/spi-nor/macronix.c
> +++ b/drivers/mtd/spi-nor/macronix.c
> @@ -8,6 +8,63 @@
> =20
>  #include "core.h"
> =20
> +/*
> + * There is a whole sequence of chips from Macronix that uses the same d=
evice
> + * id. These are recommended as EoL replacement parts by Macronix, altho=
ugh they
> + * are only partly software compatible.
> + *
> + * Recommended replacement for MX25L3205D was MX25L3206E.
> + * Recommended replacement for MX25L3206E was MX25L3233F.
> + *
> + * MX25L3205D does not support RDSFDP. The other two does.
> + *
> + * MX25L3205D supports 1-2-2 (2READ) command.
> + * MX25L3206E supports 1-1-2 (DREAD) command.
> + * MX25L3233F supports 1-1-2 (DREAD), 1-2-2 (2READ), 1-1-4 (QREAD), and =
1-4-4
> + * (4READ) commands.
> + *
> + * In order to trigger reading optional SFDP configuration, the
> + * SPI_NOR_DUAL_READ|SPI_NOR_QUAD_READ flags are set, seemingly enabling=
 1-1-2
> + * and 1-1-4 for MX25L3205D. The other chips supporting RDSFDP will have=
 the
> + * correct read commands configured based on SFDP information.
> + *
> + * As none of the other will enable 1-1-4 and NOT 1-4-4, so we identify
> + * MX25L3205D when we see that.
> + */
> +static int
> +mx25l3205d_late_init(struct spi_nor *nor)
> +{
> +	struct spi_nor_flash_parameter *params =3D nor->params;
> +
> +	/*                          DREAD  2READ  QREAD  4READ
> +	 *                          1-1-2  1-2-2  1-1-4  1-4-4
> +	 * Before SFDP parse          1      0      1      0
> +	 * 3206e after SFDP parse     1      0      0      0
> +	 * 3233f after SFDP parse     1      1      1      1
> +	 * 3205d after this func      0      1      0      0
> +	 */
> +	if ((params->hwcaps.mask & SNOR_HWCAPS_READ_1_1_4) &&
> +	    !(params->hwcaps.mask & SNOR_HWCAPS_READ_1_4_4)) {
> +		/* Should be MX25L3205D */
> +		params->hwcaps.mask &=3D ~SNOR_HWCAPS_READ_1_1_2;
> +		spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_1_1_2],
> +					  0, 0, 0, 0);
> +		params->hwcaps.mask &=3D ~SNOR_HWCAPS_READ_1_1_4;
> +		spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_1_1_4],
> +					  0, 0, 0, 0);
> +		params->hwcaps.mask |=3D SNOR_HWCAPS_READ_1_2_2;
> +		spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_1_2_2],
> +					  0, 4, SPINOR_OP_READ_1_2_2,
> +					  SNOR_PROTO_1_2_2);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct spi_nor_fixups mx25l3205d_fixups =3D {
> +	.late_init =3D mx25l3205d_late_init,
> +};
> +
>  static int
>  mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
>  			    const struct sfdp_parameter_header *bfpt_header,
> @@ -61,7 +118,8 @@ static const struct flash_info macronix_nor_parts[] =
=3D {
>  		.id =3D SNOR_ID(0xc2, 0x20, 0x16),
>  		.name =3D "mx25l3205d",
>  		.size =3D SZ_4M,
> -		.no_sfdp_flags =3D SECT_4K,
> +		.no_sfdp_flags =3D SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ,
> +		.fixups =3D &mx25l3205d_fixups
>  	}, {
>  		.id =3D SNOR_ID(0xc2, 0x20, 0x17),
>  		.name =3D "mx25l6405d",
>
> ---
> base-commit: a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6
> change-id: 20240524-macronix-mx25l3205d-fixups-882e92eed7d7
>
> Best regards,


--62dfc1cd91faea84d32d705095127aab3d83287f351da8bd6655d6784688
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iKgEABMJADAWIQTIVZIcOo5wfU/AngkSJzzuPgIf+AUCZl1v9xIcbXdhbGxlQGtl
cm5lbC5vcmcACgkQEic87j4CH/gMxwGAvrjWMLnmjxWYUHiaiERNrQqk9PERyYF8
ZttkIypS4wKL0tvNNNDWENLL9KwJu/yJAYCbHkiwVlZGfD+dI9XKXdXXu/SMT363
c1pHCkOvDZ82nN21GLn+EMMOYVTEswg+beg=
=u1Gm
-----END PGP SIGNATURE-----

--62dfc1cd91faea84d32d705095127aab3d83287f351da8bd6655d6784688--