| Message ID | 20220228134505.203270-6-tudor.ambarus@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mtd: spi-nor: Handle ID collisions | expand |
Am 2022-02-28 14:45, 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 | 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, I'm still not convinced it should be the first entry here. We will put other vendors at a disadvantage who play fair. I doubt we will always checking any new IDs for duplications - or some might slip through. Putting it as the last entry will make sure, legitimate users will always come first. Esp. because xmc reuses vendor id whose flashes we also support making a collision very likely. Unlike boya who reuses "convex computers" where we will probably never see an SPI flash from. That being said. I'd also suggest to only allow flashes with SFDP here, so we have at least some clue to differentiate between flashes. If there will ever be a flash without SFDP and which is using a non-legitimate vendor id, then we'll need to either deny support for it or specify it by a name (i.e. device tree compatible or similar). But these should go into a seperate list then. > &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 }, No PARSE_SFDP nor SKIP_SFDP? > +}; > + > +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; -michael
On 3/2/22 00:19, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Am 2022-02-28 14:45, 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 | 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, > > I'm still not convinced it should be the first entry here. We will > put other vendors at a disadvantage who play fair. I doubt we will > always checking any new IDs for duplications - or some might slip > through. Putting it as the last entry will make sure, legitimate > users will always come first. > > Esp. because xmc reuses vendor id whose flashes we also support > making a collision very likely. Unlike boya who reuses "convex > computers" where we will probably never see an SPI flash from. Yes, being the first was intentional. The rationale is that if someone adds a micron and sees an XMC name it's clear that it's a collision, so we get the chance to fix it from the first day. Better test coverage, easier to identify the collisions, thus easier work for maintainers. But at the same time ID collisions for new flash additions can be identified by a simple grep, so I will not insist here given that it is the second time you mention putting the collisions driver the last in the array. > > That being said. I'd also suggest to only allow flashes with > SFDP here, so we have at least some clue to differentiate > between flashes. If there will ever be a flash without SFDP > and which is using a non-legitimate vendor id, then we'll > need to either deny support for it or specify it by a name we can't deny support for this reason, we'll be forced to use dt to get the flash name. > (i.e. device tree compatible or similar). But these should > go into a seperate list then. > How you will differentiate between two flashes of different manufacturers that collide, one that supports SFDP and one that doesn't? You'll have to have a single flash entry in one of the drivers, where will you put it?
Am 2022-03-03 17:12, schrieb Tudor.Ambarus@microchip.com: > On 3/2/22 00:19, Michael Walle wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the content is safe >> >> Am 2022-02-28 14:45, 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 | 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, >> >> I'm still not convinced it should be the first entry here. We will >> put other vendors at a disadvantage who play fair. I doubt we will >> always checking any new IDs for duplications - or some might slip >> through. Putting it as the last entry will make sure, legitimate >> users will always come first. >> >> Esp. because xmc reuses vendor id whose flashes we also support >> making a collision very likely. Unlike boya who reuses "convex >> computers" where we will probably never see an SPI flash from. > > Yes, being the first was intentional. The rationale is that if someone > adds a micron and sees an XMC name it's clear that it's a collision, > so we get the chance to fix it from the first day. Better test > coverage, > easier to identify the collisions, thus easier work for maintainers. > But at the same time ID collisions for new flash additions can be > identified by a simple grep, so I will not insist here given that it is > the second time you mention putting the collisions driver the last in > the > array. > >> >> That being said. I'd also suggest to only allow flashes with >> SFDP here, so we have at least some clue to differentiate >> between flashes. If there will ever be a flash without SFDP >> and which is using a non-legitimate vendor id, then we'll >> need to either deny support for it or specify it by a name > > we can't deny support for this reason, we'll be forced to use dt to get > the flash name. > >> (i.e. device tree compatible or similar). But these should >> go into a seperate list then. >> > How you will differentiate between two flashes of different > manufacturers that > collide, one that supports SFDP and one that doesn't? You'll have to > have a > single flash entry in one of the drivers, where will you put it? Hm, I see. But it doesn't end there. Imagine one would need function from a different (vendor) module. So we have to export it again which formerly was just private to this module. All of this makes me wonder if we can't just add one device id multiple times in our lists in different vendor modules. To distinguish between entries with the same id, we provide another callback: bool is_match(nor, sfdp, ..) That would solve the following: (1) we can have the proper flags per flash instead of having to change them in fixups later (2) vendor functions can be left private in the corresponding module, because all entries will be held in a per vendor list (3) we can provide some sanity checks (enabled by a Kconfig) to walk the list and watch for invalid duplicate entries. See more below. (4) sane fallback. I.e. if there is a duplicate in the future, we just have to add a new entry with a is_match(). If it doesn't match, we just continue and will finally fall back to the original entry. (5) if necessary, compatible strings matches should be easy to add. Think of something like: bool is_match(nor, sfdp) { return of_device_is_compatible(nor, "macronix,mx.."); } is_match() is optional, but if given, both the flash id has to match as well as is_match() has to return true. I.e. one sanity could be: walk the list and see if there are two entries with the same id, but both without an is_match() function. This would mean an invalid duplicate entry. What do you think? -michael
On 3/3/22 23:38, Michael Walle wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Am 2022-03-03 17:12, schrieb Tudor.Ambarus@microchip.com: >> On 3/2/22 00:19, Michael Walle wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know >>> the content is safe >>> >>> Am 2022-02-28 14:45, 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 | 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, >>> >>> I'm still not convinced it should be the first entry here. We will >>> put other vendors at a disadvantage who play fair. I doubt we will >>> always checking any new IDs for duplications - or some might slip >>> through. Putting it as the last entry will make sure, legitimate >>> users will always come first. >>> >>> Esp. because xmc reuses vendor id whose flashes we also support >>> making a collision very likely. Unlike boya who reuses "convex >>> computers" where we will probably never see an SPI flash from. >> >> Yes, being the first was intentional. The rationale is that if someone >> adds a micron and sees an XMC name it's clear that it's a collision, >> so we get the chance to fix it from the first day. Better test >> coverage, >> easier to identify the collisions, thus easier work for maintainers. >> But at the same time ID collisions for new flash additions can be >> identified by a simple grep, so I will not insist here given that it is >> the second time you mention putting the collisions driver the last in >> the >> array. >> >>> >>> That being said. I'd also suggest to only allow flashes with >>> SFDP here, so we have at least some clue to differentiate >>> between flashes. If there will ever be a flash without SFDP >>> and which is using a non-legitimate vendor id, then we'll >>> need to either deny support for it or specify it by a name >> >> we can't deny support for this reason, we'll be forced to use dt to get >> the flash name. >> >>> (i.e. device tree compatible or similar). But these should >>> go into a seperate list then. >>> >> How you will differentiate between two flashes of different >> manufacturers that >> collide, one that supports SFDP and one that doesn't? You'll have to >> have a >> single flash entry in one of the drivers, where will you put it? > > Hm, I see. But it doesn't end there. Imagine one would need > function from a different (vendor) module. So we have to export it > again which formerly was just private to this module. > > All of this makes me wonder if we can't just add one device id > multiple times in our lists in different vendor modules. To > distinguish between entries with the same id, we provide another > callback: > bool is_match(nor, sfdp, ..) > > That would solve the following: > (1) we can have the proper flags per flash instead of having > to change them in fixups later > (2) vendor functions can be left private in the corresponding > module, because all entries will be held in a per vendor list > (3) we can provide some sanity checks (enabled by a Kconfig) > to walk the list and watch for invalid duplicate entries. > See more below. we already have to go through the entire list of flash info entries to check for collisions, we can just add a dev_info or dev_warn for invalid duplicate entries. No need for a kconfig. > (4) sane fallback. I.e. if there is a duplicate in the future, > we just have to add a new entry with a is_match(). If it > doesn't match, we just continue and will finally fall back > to the original entry. > (5) if necessary, compatible strings matches should be easy to > add. Think of something like: > bool is_match(nor, sfdp) { > return of_device_is_compatible(nor, "macronix,mx.."); > } > > is_match() is optional, but if given, both the flash id has to > match as well as is_match() has to return true. > > I.e. one sanity could be: walk the list and see if there are > two entries with the same id, but both without an is_match() > function. This would mean an invalid duplicate entry. > > What do you think? I find the idea good and would like to give it a try. The downside that I see is that we'll always have to go through the entire list of flash_info entries to determine the collisions and to handle them all, so a little delay in finding the flash, but I think we can live with this. Can I implement your suggestion and add Suggestion-by tags, or do you want to handle it yourself? Cheers, ta
Am 2022-03-04 08:07, schrieb Tudor.Ambarus@microchip.com: > On 3/3/22 23:38, Michael Walle wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know >> the content is safe >> >> Am 2022-03-03 17:12, schrieb Tudor.Ambarus@microchip.com: >>> On 3/2/22 00:19, Michael Walle wrote: >>>> EXTERNAL EMAIL: Do not click links or open attachments unless you >>>> know >>>> the content is safe >>>> >>>> Am 2022-02-28 14:45, 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 | 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, >>>> >>>> I'm still not convinced it should be the first entry here. We will >>>> put other vendors at a disadvantage who play fair. I doubt we will >>>> always checking any new IDs for duplications - or some might slip >>>> through. Putting it as the last entry will make sure, legitimate >>>> users will always come first. >>>> >>>> Esp. because xmc reuses vendor id whose flashes we also support >>>> making a collision very likely. Unlike boya who reuses "convex >>>> computers" where we will probably never see an SPI flash from. >>> >>> Yes, being the first was intentional. The rationale is that if >>> someone >>> adds a micron and sees an XMC name it's clear that it's a collision, >>> so we get the chance to fix it from the first day. Better test >>> coverage, >>> easier to identify the collisions, thus easier work for maintainers. >>> But at the same time ID collisions for new flash additions can be >>> identified by a simple grep, so I will not insist here given that it >>> is >>> the second time you mention putting the collisions driver the last in >>> the >>> array. >>> >>>> >>>> That being said. I'd also suggest to only allow flashes with >>>> SFDP here, so we have at least some clue to differentiate >>>> between flashes. If there will ever be a flash without SFDP >>>> and which is using a non-legitimate vendor id, then we'll >>>> need to either deny support for it or specify it by a name >>> >>> we can't deny support for this reason, we'll be forced to use dt to >>> get >>> the flash name. >>> >>>> (i.e. device tree compatible or similar). But these should >>>> go into a seperate list then. >>>> >>> How you will differentiate between two flashes of different >>> manufacturers that >>> collide, one that supports SFDP and one that doesn't? You'll have to >>> have a >>> single flash entry in one of the drivers, where will you put it? >> >> Hm, I see. But it doesn't end there. Imagine one would need >> function from a different (vendor) module. So we have to export it >> again which formerly was just private to this module. >> >> All of this makes me wonder if we can't just add one device id >> multiple times in our lists in different vendor modules. To >> distinguish between entries with the same id, we provide another >> callback: >> bool is_match(nor, sfdp, ..) >> >> That would solve the following: >> (1) we can have the proper flags per flash instead of having >> to change them in fixups later >> (2) vendor functions can be left private in the corresponding >> module, because all entries will be held in a per vendor list >> (3) we can provide some sanity checks (enabled by a Kconfig) >> to walk the list and watch for invalid duplicate entries. >> See more below. > > we already have to go through the entire list of flash info entries > to check for collisions, we can just add a dev_info or dev_warn for > invalid duplicate entries. No need for a kconfig. It's some kind of selftest. I'm not just talking about the current flash being probed. But always iterate over any flashes and look duplicates, so the naive implementation would be O(N^2). Which is a runtime overhead and only really needed by ourselves when we add a new flash. It doesn't bring any good otherwise, so it should be off. >> (4) sane fallback. I.e. if there is a duplicate in the future, >> we just have to add a new entry with a is_match(). If it >> doesn't match, we just continue and will finally fall back >> to the original entry. >> (5) if necessary, compatible strings matches should be easy to >> add. Think of something like: >> bool is_match(nor, sfdp) { >> return of_device_is_compatible(nor, "macronix,mx.."); >> } >> >> is_match() is optional, but if given, both the flash id has to >> match as well as is_match() has to return true. >> >> I.e. one sanity could be: walk the list and see if there are >> two entries with the same id, but both without an is_match() >> function. This would mean an invalid duplicate entry. >> >> What do you think? > > I find the idea good and would like to give it a try. The downside that > I see is that we'll always have to go through the entire list of > flash_info entries to determine the collisions and to handle them all, > so a > little delay in finding the flash, but I think we can live with this. If the flash is the last one in the list you have the same delay ;) > Can I implement your suggestion and add Suggestion-by tags, or do you > want > to handle it yourself? Sure, go ahead! -michael
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;
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;
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 }, +}; + +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;
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