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

[<Tudor.Ambarus@microchip.com>]

On 8/24/21 1:47 AM, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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

yes

> might take precedence over "normal" ones? Shouldn't it be

yes

> the other way around?

It doesn't really matter, either way we should correctly identify the flash.
I thought of putting the collisions driver first for better test coverage.
In case we miss a collision between a (future) entry from the manufacturer
collisions driver and one from a dedicated manufacturer driver, to hit the
flash entry from the collisions driver, which will report a wrong name for
the flash in the dedicated manufacturer driver. People will report that
something's wrong and we can fix the things sooner.

> 
>>       &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?

yes, we can introduce a const char *manufacturer_name in struct spi_nor and
update it with the correct manufacturer name. Will do.

cheers,
ta

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

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