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

[<Tudor.Ambarus@microchip.com>]

On 3/4/22 23:20, George Brooke wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> 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>

Thanks, George! Will let you know when v5 is out.

Cheers,
ta
> 
> # 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/