Re: [PATCH v2 05/35] mtd: spi-nor: Introduce Manufacturer ID collisions driver

Michael Walle
2021-08-23T22:48:39Z

Am 2021-07-27 06:51, schrieb Tudor Ambarus:
> Some manufacturers completely ignore the manufacturer's identification
> code
> standard (JEP106) and do not define the manufacturer ID continuation
> scheme. This will result in manufacturer ID collisions.
>
> An an example, JEP106BA requires Boya that it's manufacturer ID to be
> preceded by 8 continuation codes. Boya's identification code must be:
> 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x68. But Boya ignores
> the
> continuation scheme and its ID collides with the manufacturer defined
> in
> bank one: Convex Computer.
>
> Introduce the manuf-id-collisions driver in order to address ID
> collisions
> between manufacturers. flash_info entries will be added in a first
> come,
> first served manner. Differentiation between flashes will be done at
> runtime if possible. Where runtime differentiation is not possible, new
> compatibles will be introduced, but this will be done as a last resort.
> Every new flash addition that define the SFDP tables, should dump its
> SFDP
> tables in the patch's comment section below the --- line, so that we
> can
> reference it in case of collisions.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
> drivers/mtd/spi-nor/Makefile | 1 +
> drivers/mtd/spi-nor/core.c | 1 +
> drivers/mtd/spi-nor/core.h | 1 +
> drivers/mtd/spi-nor/manuf-id-collisions.c | 22 ++++++++++++++++++++++
> 4 files changed, 25 insertions(+)
> create mode 100644 drivers/mtd/spi-nor/manuf-id-collisions.c
>
> diff --git a/drivers/mtd/spi-nor/Makefile
> b/drivers/mtd/spi-nor/Makefile
> index 6b904e439372..48763d10daad 100644
> --- a/drivers/mtd/spi-nor/Makefile
> +++ b/drivers/mtd/spi-nor/Makefile
> @@ -1,6 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0
>
> spi-nor-objs := core.o sfdp.o swp.o otp.o sysfs.o
> +spi-nor-objs += manuf-id-collisions.o
> spi-nor-objs += atmel.o
> spi-nor-objs += catalyst.o
> spi-nor-objs += eon.o
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 1a278d33b02d..d30c8f350dc1 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1829,6 +1829,7 @@ int spi_nor_sr2_bit7_quad_enable(struct spi_nor
> *nor)
> }
>
> static const struct spi_nor_manufacturer *manufacturers[] = {
> + &spi_nor_manuf_id_collisions,

Intentionally at the beginning of all the flashes? So these
might take precedence over "normal" ones? Shouldn't it be
the other way around?

> &spi_nor_atmel,
> &spi_nor_catalyst,
> &spi_nor_eon,
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 55fceb0ec894..e9896cd60695 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -476,6 +476,7 @@ struct sfdp {
> };
>
> /* Manufacturer drivers. */
> +extern const struct spi_nor_manufacturer spi_nor_manuf_id_collisions;
> extern const struct spi_nor_manufacturer spi_nor_atmel;
> extern const struct spi_nor_manufacturer spi_nor_catalyst;
> extern const struct spi_nor_manufacturer spi_nor_eon;
> diff --git a/drivers/mtd/spi-nor/manuf-id-collisions.c
> b/drivers/mtd/spi-nor/manuf-id-collisions.c
> new file mode 100644
> index 000000000000..bf7dba34f018
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/manuf-id-collisions.c
> @@ -0,0 +1,22 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Used to handle collisions between manufacturers, where
> manufacturers are
> + * ignorant enough to not implement the ID continuation scheme
> described in the
> + * JEP106 JEDEC standard.
> + */
> +
> +#include <linux/mtd/spi-nor.h>
> +#include "core.h"
> +
> +static const struct flash_info id_collision_parts[] = {
> + /* Boya */
> + { "by25q128as", INFO(0x684018, 0, 64 * 1024, 256, SPI_NOR_SKIP_SFDP |
> + SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> + SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
> +};
> +
> +const struct spi_nor_manufacturer spi_nor_manuf_id_collisions = {
> + .name = "manufacturer ID collisions",

mhh, so we loose the manufacturer name. Doh. Can we do better?

-michael

> + .parts = id_collision_parts,
> + .nparts = ARRAY_SIZE(id_collision_parts),
> +};