Re: [PATCH v4 5/6] mtd: spi-nor: Introduce Manufacturer ID collisions driver

[George Brooke <figgyc@figgyc.uk>]

Hi Tudor,

Tudor Ambarus <tudor.ambarus@microchip.com> writes:

> 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                |  3 +++
>  drivers/mtd/spi-nor/core.h                |  1 +
>  drivers/mtd/spi-nor/manuf-id-collisions.c | 32 
>  +++++++++++++++++++++++
>  drivers/mtd/spi-nor/sysfs.c               |  2 +-
>  include/linux/mtd/spi-nor.h               |  6 ++++-
>  6 files changed, 43 insertions(+), 2 deletions(-)
>  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 aef00151c116..80d6ce41122a 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1610,6 +1610,7 @@ int spi_nor_sr2_bit7_quad_enable(struct 
> spi_nor *nor)
>  }
>
>  static const struct spi_nor_manufacturer *manufacturers[] = {
> +	&spi_nor_manuf_id_collisions,
>   &spi_nor_atmel,
>   &spi_nor_catalyst,
>   &spi_nor_eon,
> @@ -3037,6 +3038,8 @@ int spi_nor_scan(struct spi_nor *nor, 
> const char *name,
>
>   if (!nor->name)
>       nor->name = info->name;
> +	if (!nor->manufacturer_name)
> +		nor->manufacturer_name = nor->manufacturer->name;
>
>   dev_info(dev, "%s (%lld Kbytes)\n", nor->name,
>           (long long)mtd->size >> 10);
> diff --git a/drivers/mtd/spi-nor/core.h 
> b/drivers/mtd/spi-nor/core.h
> index b7fd760e3b47..f727e632c0ee 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -500,6 +500,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..75c5ad6480ee
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/manuf-id-collisions.c
> @@ -0,0 +1,32 @@
> +// 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 void boya_nor_late_init(struct spi_nor *nor)
> +{
> +	nor->manufacturer_name = "boya";
> +}
> +
> +static const struct spi_nor_fixups boya_nor_fixups = {
> +	.late_init = boya_nor_late_init,
> +};
> +
> +static const struct flash_info id_collision_parts[] = {
> +	/* Boya */
> +	{ "by25q128as", INFO(0x684018, 0, 64 * 1024, 256)
> +		FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
> +		NO_SFDP_FLAGS(SPI_NOR_SKIP_SFDP | SECT_4K | 
> SPI_NOR_DUAL_READ |
> +               SPI_NOR_QUAD_READ)
> +		.fixups = &boya_nor_fixups },
> +};
> +
Finally got around to testing v4, and it looks good to me.
Sorry for the delay, I was struggling a bit with device tree
overlays because I lost my old one for this Raspberry Pi.
For v5+ I should be able to test a lot quicker if needed. Thanks
for working on this again.

Tested-by: George Brooke <figgyc@figgyc.uk>

# cat /sys/bus/spi/devices/spi0.0/spi-nor/jedec_id
684018
# cat /sys/bus/spi/devices/spi0.0/spi-nor/manufacturer
boya
# cat /sys/bus/spi/devices/spi0.0/spi-nor/partname
by25q128as
# xxd -p /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
xxd: /sys/bus/spi/devices/spi0.0/spi-nor/sfdp: No such file or 
directory

# dd bs=1M count=6 if=/dev/urandom of=./nor_test
6+0 records in
6+0 records out
6291456 bytes (6.3 MB, 6.0 MiB) copied, 0.158377 s, 39.7 MB/s

# time mtd_debug erase /dev/mtd0 0 6291456
Erased 6291456 bytes from address 0x00000000 in flash

real	1m25.420s
user	0m0.000s
sys	0m56.700s

# time mtd_debug read /dev/mtd0 0 6291456 nor_read
Copied 6291456 bytes from address 0x00000000 in flash to nor_read

real	0m2.472s
user	0m0.001s
sys	0m0.050s

# hexdump nor_read
0000000 ffff ffff ffff ffff ffff ffff ffff ffff
*
0600000

# time mtd_debug write /dev/mtd0 0 6291456 nor_test
Copied 6291456 bytes from nor_test to address 0x00000000 in flash

real	0m14.151s
user	0m0.001s
sys	0m7.880s

# time mtd_debug read /dev/mtd0 0 6291456 nor_read
Copied 6291456 bytes from address 0x00000000 in flash to nor_read

real	0m2.580s
user	0m0.001s
sys	0m0.059s

# sha1sum nor_test nor_read
6a4ecd64a21335ade4dd8e329718df6666a8c2e8  nor_test
6a4ecd64a21335ade4dd8e329718df6666a8c2e8  nor_read

> +const struct spi_nor_manufacturer spi_nor_manuf_id_collisions = 
> {
> +	.parts = id_collision_parts,
> +	.nparts = ARRAY_SIZE(id_collision_parts),
> +};
> diff --git a/drivers/mtd/spi-nor/sysfs.c 
> b/drivers/mtd/spi-nor/sysfs.c
> index 017119768f32..fa0cf1a96797 100644
> --- a/drivers/mtd/spi-nor/sysfs.c
> +++ b/drivers/mtd/spi-nor/sysfs.c
> @@ -14,7 +14,7 @@ static ssize_t manufacturer_show(struct device 
> *dev,
>   struct spi_mem *spimem = spi_get_drvdata(spi);
>   struct spi_nor *nor = spi_mem_get_drvdata(spimem);
>
> -	return sysfs_emit(buf, "%s\n", nor->manufacturer->name);
> +	return sysfs_emit(buf, "%s\n", nor->manufacturer_name);
>  }
>  static DEVICE_ATTR_RO(manufacturer);
>
> diff --git a/include/linux/mtd/spi-nor.h 
> b/include/linux/mtd/spi-nor.h
> index 449496b57acb..3087589d01ac 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -351,7 +351,10 @@ struct spi_nor_flash_parameter;
>   * @bouncebuf:		bounce buffer used when the buffer passed 
>   by the MTD
>   *                      layer is not DMA-able
>   * @bouncebuf_size:	size of the bounce buffer
> - * @name:		used to point to correct name in case of ID 
> collisions.
> + * @name:		used to point to correct flash name in case of 
> ID
> + *                      collisions.
> + * @manufacturer_name:	used to point to correct manufacturer 
> name in case of
> + *                      ID collisions.
>   * @info:		SPI NOR part JEDEC MFR ID and other info
>   * @manufacturer:	SPI NOR manufacturer
>   * @addr_width:		number of address bytes
> @@ -382,6 +385,7 @@ struct spi_nor {
>   u8			*bouncebuf;
>   size_t			bouncebuf_size;
>   const char *name;
> +	const char *manufacturer_name;
>   const struct flash_info	*info;
>   const struct spi_nor_manufacturer *manufacturer;
>   u8			addr_width;

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/