[PATCH v4 0/6] mtd: spi-nor: Handle ID collisions

[PATCH v4 0/6] mtd: spi-nor: Handle ID collisions

[Tudor Ambarus]

Tested with MX25L3233F. I need sysfs dumps where there aren't.

v4:
- adapt core to handle collisions between SFDP and non-SFDP flashes
- rebase on latest spi-nor/next

v3:
- report correct manufacturer name in ID collisions driver
- use fixup name of the first flash, not of the one that collide
- use xxd to dump sfdp (where I had the flash)
- drop locking support on xt25f128b
- constify flash_info fixups hook

Tudor Ambarus (6):
  mtd: spi-nor: core: Report correct name in case of ID collisions
  mtd: spi-nor: core: Handle ID collisions between SFDP & non-SFDP
    flashes
  mtd: spi-nor: macronix: Handle ID collision b/w MX25L3233F and
    MX25L3205D
  mtd: spi-nor: macronix: Handle ID collision b/w MX25L12805D and
    MX25L12835F
  mtd: spi-nor: Introduce Manufacturer ID collisions driver
  mtd: spi-nor: manuf-id-collisions: Add support for xt25f128b

 drivers/mtd/spi-nor/Makefile              |  1 +
 drivers/mtd/spi-nor/core.c                | 21 +++++++++--
 drivers/mtd/spi-nor/core.h                |  1 +
 drivers/mtd/spi-nor/macronix.c            | 46 ++++++++++++++++++++++-
 drivers/mtd/spi-nor/manuf-id-collisions.c | 46 +++++++++++++++++++++++
 drivers/mtd/spi-nor/sysfs.c               |  4 +-
 include/linux/mtd/spi-nor.h               |  6 +++
 7 files changed, 118 insertions(+), 7 deletions(-)
 create mode 100644 drivers/mtd/spi-nor/manuf-id-collisions.c

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 1/6] mtd: spi-nor: core: Report correct name in case of ID collisions

[Tudor Ambarus]

Provide a way to report the correct flash name in case of ID collisions.
There will be a single flash_info entry when flash IDs collide, and the
differentiation between the flash types will be made at runtime
if possible.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/core.c  | 5 ++++-
 drivers/mtd/spi-nor/sysfs.c | 2 +-
 include/linux/mtd/spi-nor.h | 2 ++
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 9014008e60b3..fbf3278ba29a 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3026,7 +3026,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	/* No mtd_info fields should be used up to this point. */
 	spi_nor_set_mtd_info(nor);
 
-	dev_info(dev, "%s (%lld Kbytes)\n", info->name,
+	if (!nor->name)
+		nor->name = info->name;
+
+	dev_info(dev, "%s (%lld Kbytes)\n", nor->name,
 			(long long)mtd->size >> 10);
 
 	dev_dbg(dev,
diff --git a/drivers/mtd/spi-nor/sysfs.c b/drivers/mtd/spi-nor/sysfs.c
index 9aec9d8a98ad..017119768f32 100644
--- a/drivers/mtd/spi-nor/sysfs.c
+++ b/drivers/mtd/spi-nor/sysfs.c
@@ -25,7 +25,7 @@ static ssize_t partname_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->info->name);
+	return sysfs_emit(buf, "%s\n", nor->name);
 }
 static DEVICE_ATTR_RO(partname);
 
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 5e25a7b75ae2..449496b57acb 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -351,6 +351,7 @@ 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.
  * @info:		SPI NOR part JEDEC MFR ID and other info
  * @manufacturer:	SPI NOR manufacturer
  * @addr_width:		number of address bytes
@@ -380,6 +381,7 @@ struct spi_nor {
 	struct spi_mem		*spimem;
 	u8			*bouncebuf;
 	size_t			bouncebuf_size;
+	const char *name;
 	const struct flash_info	*info;
 	const struct spi_nor_manufacturer *manufacturer;
 	u8			addr_width;
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 1/6] mtd: spi-nor: core: Report correct name in case of ID collisions

[Michael Walle]

Am 2022-02-28 14:45, schrieb Tudor Ambarus:
> Provide a way to report the correct flash name in case of ID 
> collisions.
> There will be a single flash_info entry when flash IDs collide, and the
> differentiation between the flash types will be made at runtime
> if possible.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Reviewed-by: Michael Walle <michael@walle.cc>

-michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 1/6] mtd: spi-nor: core: Report correct name in case of ID collisions

[Pratyush Yadav]

On 28/02/22 03:45PM, Tudor Ambarus wrote:
> Provide a way to report the correct flash name in case of ID collisions.
> There will be a single flash_info entry when flash IDs collide, and the
> differentiation between the flash types will be made at runtime
> if possible.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 2/6] mtd: spi-nor: core: Handle ID collisions between SFDP & non-SFDP flashes

[Tudor Ambarus]

A typical differentiator between flashes whose ID collide is whether they
support SFDP or not. For such a collision there will be a single
flash_info entry where the developer should specify:
1/ PARSE_SFDP - so that the flash that supports SFDP to initialize its
   parameters by parsing the SFDP tables
2/ any of the no_sfdp_flags less SPI_NOR_SKIP_SFDP, to initialize the
   flash parameters via the static no_sfdp_flags for the flash that
   doesn't support SFDP.
Use the logic the above to handle ID collisions between SFDP & non-SFDP
flashes.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/core.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index fbf3278ba29a..aef00151c116 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2639,8 +2639,17 @@ static int spi_nor_init_params(struct spi_nor *nor)
 	if (nor->info->parse_sfdp) {
 		ret = spi_nor_parse_sfdp(nor);
 		if (ret) {
-			dev_err(nor->dev, "BFPT parsing failed. Please consider using SPI_NOR_SKIP_SFDP when declaring the flash\n");
-			return ret;
+			/*
+			 * Handle ID collisions between flashes that support
+			 * SFDP and flashes that don't. Initialize parameters
+			 * for the flash that doesn't support SFDP.
+			 */
+			if (nor->info->no_sfdp_flags & ~SPI_NOR_SKIP_SFDP) {
+				spi_nor_no_sfdp_init_params(nor);
+			} else {
+				dev_err(nor->dev, "BFPT parsing failed. Please consider using SPI_NOR_SKIP_SFDP when declaring the flash\n");
+				return ret;
+			}
 		}
 	} else if (nor->info->no_sfdp_flags & SPI_NOR_SKIP_SFDP) {
 		spi_nor_no_sfdp_init_params(nor);
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/6] mtd: spi-nor: core: Handle ID collisions between SFDP & non-SFDP flashes

[Michael Walle]

Am 2022-02-28 14:45, schrieb Tudor Ambarus:
> A typical differentiator between flashes whose ID collide is whether 
> they
> support SFDP or not. For such a collision there will be a single
> flash_info entry where the developer should specify:
> 1/ PARSE_SFDP - so that the flash that supports SFDP to initialize its
>    parameters by parsing the SFDP tables
> 2/ any of the no_sfdp_flags less SPI_NOR_SKIP_SFDP, to initialize the
>    flash parameters via the static no_sfdp_flags for the flash that
>    doesn't support SFDP.
> Use the logic the above to handle ID collisions between SFDP & non-SFDP
> flashes.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/core.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index fbf3278ba29a..aef00151c116 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2639,8 +2639,17 @@ static int spi_nor_init_params(struct spi_nor 
> *nor)
>  	if (nor->info->parse_sfdp) {
>  		ret = spi_nor_parse_sfdp(nor);

Can we return -ENOENT here if sfdp isn't supported, so we
can differentiate between "no sfdp present" and other errors?

>  		if (ret) {
> -			dev_err(nor->dev, "BFPT parsing failed. Please consider using
> SPI_NOR_SKIP_SFDP when declaring the flash\n");
> -			return ret;
> +			/*
> +			 * Handle ID collisions between flashes that support
> +			 * SFDP and flashes that don't. Initialize parameters
> +			 * for the flash that doesn't support SFDP.
> +			 */
> +			if (nor->info->no_sfdp_flags & ~SPI_NOR_SKIP_SFDP) {

Shouldn't this be
if (!(nor->info->no_sfdp_flags & SPI_NOR_SKIP_SFDP))

> +				spi_nor_no_sfdp_init_params(nor);
> +			} else {
> +				dev_err(nor->dev, "BFPT parsing failed. Please consider using
> SPI_NOR_SKIP_SFDP when declaring the flash\n");
> +				return ret;
> +			}
>  		}
>  	} else if (nor->info->no_sfdp_flags & SPI_NOR_SKIP_SFDP) {
>  		spi_nor_no_sfdp_init_params(nor);

-michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/6] mtd: spi-nor: core: Handle ID collisions between SFDP & non-SFDP flashes

[Tudor.Ambarus]

On 3/1/22 23:52, 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:
>> A typical differentiator between flashes whose ID collide is whether
>> they
>> support SFDP or not. For such a collision there will be a single
>> flash_info entry where the developer should specify:
>> 1/ PARSE_SFDP - so that the flash that supports SFDP to initialize its
>>    parameters by parsing the SFDP tables
>> 2/ any of the no_sfdp_flags less SPI_NOR_SKIP_SFDP, to initialize the
>>    flash parameters via the static no_sfdp_flags for the flash that
>>    doesn't support SFDP.
>> Use the logic the above to handle ID collisions between SFDP & non-SFDP
>> flashes.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>>  drivers/mtd/spi-nor/core.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index fbf3278ba29a..aef00151c116 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -2639,8 +2639,17 @@ static int spi_nor_init_params(struct spi_nor
>> *nor)
>>       if (nor->info->parse_sfdp) {
>>               ret = spi_nor_parse_sfdp(nor);
> 
> Can we return -ENOENT here if sfdp isn't supported, so we
> can differentiate between "no sfdp present" and other errors?

I'll take a look.

> 
>>               if (ret) {
>> -                     dev_err(nor->dev, "BFPT parsing failed. Please consider using
>> SPI_NOR_SKIP_SFDP when declaring the flash\n");
>> -                     return ret;
>> +                     /*
>> +                      * Handle ID collisions between flashes that support
>> +                      * SFDP and flashes that don't. Initialize parameters
>> +                      * for the flash that doesn't support SFDP.
>> +                      */
>> +                     if (nor->info->no_sfdp_flags & ~SPI_NOR_SKIP_SFDP) {
> 
> Shouldn't this be
> if (!(nor->info->no_sfdp_flags & SPI_NOR_SKIP_SFDP))

No, because this will be true when no_sfdp_flags is zero, and the method
from below will be called. I would like to call it when any of the
no_sfdp_flags are defined, less the SPI_NOR_SKIP_SFDP flag. So when one
declares a flash like:
+      { "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64)
+             /* ID collision with mx25l3233f. */
+             PARSE_SFDP
+             NO_SFDP_FLAGS(SECT_4K) 

First we will try to retrieve the flash params from SFDP. If SFDP fails,
then we'll init the flash based on the no_sfdp_flags. If SFDP succeeds
the no_sfdp_flags is ignored.

Cheers,
ta
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/6] mtd: spi-nor: core: Handle ID collisions between SFDP & non-SFDP flashes

[Michael Walle]

Am 2022-03-03 15:41, schrieb Tudor.Ambarus@microchip.com:
> On 3/1/22 23:52, 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:
>>> A typical differentiator between flashes whose ID collide is whether
>>> they
>>> support SFDP or not. For such a collision there will be a single
>>> flash_info entry where the developer should specify:
>>> 1/ PARSE_SFDP - so that the flash that supports SFDP to initialize 
>>> its
>>>    parameters by parsing the SFDP tables
>>> 2/ any of the no_sfdp_flags less SPI_NOR_SKIP_SFDP, to initialize the
>>>    flash parameters via the static no_sfdp_flags for the flash that
>>>    doesn't support SFDP.
>>> Use the logic the above to handle ID collisions between SFDP & 
>>> non-SFDP
>>> flashes.
>>> 
>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>>> ---
>>>  drivers/mtd/spi-nor/core.c | 13 +++++++++++--
>>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>> index fbf3278ba29a..aef00151c116 100644
>>> --- a/drivers/mtd/spi-nor/core.c
>>> +++ b/drivers/mtd/spi-nor/core.c
>>> @@ -2639,8 +2639,17 @@ static int spi_nor_init_params(struct spi_nor
>>> *nor)
>>>       if (nor->info->parse_sfdp) {
>>>               ret = spi_nor_parse_sfdp(nor);
>> 
>> Can we return -ENOENT here if sfdp isn't supported, so we
>> can differentiate between "no sfdp present" and other errors?
> 
> I'll take a look.
> 
>> 
>>>               if (ret) {
>>> -                     dev_err(nor->dev, "BFPT parsing failed. Please 
>>> consider using
>>> SPI_NOR_SKIP_SFDP when declaring the flash\n");
>>> -                     return ret;
>>> +                     /*
>>> +                      * Handle ID collisions between flashes that 
>>> support
>>> +                      * SFDP and flashes that don't. Initialize 
>>> parameters
>>> +                      * for the flash that doesn't support SFDP.
>>> +                      */
>>> +                     if (nor->info->no_sfdp_flags & 
>>> ~SPI_NOR_SKIP_SFDP) {
>> 
>> Shouldn't this be
>> if (!(nor->info->no_sfdp_flags & SPI_NOR_SKIP_SFDP))

Ahh I misread that for the "skip no sftp handling". But doesn't render
my point below invalid.

> No, because this will be true when no_sfdp_flags is zero, and the 
> method
> from below will be called. I would like to call it when any of the
> no_sfdp_flags are defined, less the SPI_NOR_SKIP_SFDP flag.

You should add a comment then.

> So when one
> declares a flash like:
> +      { "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64)
> +             /* ID collision with mx25l3233f. */
> +             PARSE_SFDP
> +             NO_SFDP_FLAGS(SECT_4K)

But what about
+      { "differentflash",  INFO(0xc22016, 0, 64 * 1024,  64)
+             /* ID collision with mx25l3233f. */
+             PARSE_SFDP

Thats also valid, no? Why is having 4k sectors special? FWIW, the
function not only handles no_sfdp_flags but also erase related
things.

So in summary, the nosfdp handling is always called when parsing
fails (that is when there is no SFDP, not due to read errors or
similar). I don't see why that shouldn't be the case.

-michael

> First we will try to retrieve the flash params from SFDP. If SFDP 
> fails,
> then we'll init the flash based on the no_sfdp_flags. If SFDP succeeds
> the no_sfdp_flags is ignored.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/6] mtd: spi-nor: core: Handle ID collisions between SFDP & non-SFDP flashes

[Tudor.Ambarus]

On 3/3/22 16:51, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-03-03 15:41, schrieb Tudor.Ambarus@microchip.com:
>> On 3/1/22 23:52, 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:
>>>> A typical differentiator between flashes whose ID collide is whether
>>>> they
>>>> support SFDP or not. For such a collision there will be a single
>>>> flash_info entry where the developer should specify:
>>>> 1/ PARSE_SFDP - so that the flash that supports SFDP to initialize
>>>> its
>>>>    parameters by parsing the SFDP tables
>>>> 2/ any of the no_sfdp_flags less SPI_NOR_SKIP_SFDP, to initialize the
>>>>    flash parameters via the static no_sfdp_flags for the flash that
>>>>    doesn't support SFDP.
>>>> Use the logic the above to handle ID collisions between SFDP &
>>>> non-SFDP
>>>> flashes.
>>>>
>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>>>> ---
>>>>  drivers/mtd/spi-nor/core.c | 13 +++++++++++--
>>>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>>>> index fbf3278ba29a..aef00151c116 100644
>>>> --- a/drivers/mtd/spi-nor/core.c
>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>> @@ -2639,8 +2639,17 @@ static int spi_nor_init_params(struct spi_nor
>>>> *nor)
>>>>       if (nor->info->parse_sfdp) {
>>>>               ret = spi_nor_parse_sfdp(nor);
>>>
>>> Can we return -ENOENT here if sfdp isn't supported, so we
>>> can differentiate between "no sfdp present" and other errors?
>>
>> I'll take a look.
>>
>>>
>>>>               if (ret) {
>>>> -                     dev_err(nor->dev, "BFPT parsing failed. Please
>>>> consider using
>>>> SPI_NOR_SKIP_SFDP when declaring the flash\n");
>>>> -                     return ret;
>>>> +                     /*
>>>> +                      * Handle ID collisions between flashes that
>>>> support
>>>> +                      * SFDP and flashes that don't. Initialize
>>>> parameters
>>>> +                      * for the flash that doesn't support SFDP.
>>>> +                      */
>>>> +                     if (nor->info->no_sfdp_flags &
>>>> ~SPI_NOR_SKIP_SFDP) {
>>>
>>> Shouldn't this be
>>> if (!(nor->info->no_sfdp_flags & SPI_NOR_SKIP_SFDP))
> 
> Ahh I misread that for the "skip no sftp handling". But doesn't render
> my point below invalid.
> 
>> No, because this will be true when no_sfdp_flags is zero, and the
>> method
>> from below will be called. I would like to call it when any of the
>> no_sfdp_flags are defined, less the SPI_NOR_SKIP_SFDP flag.
> 
> You should add a comment then.
> 
>> So when one
>> declares a flash like:
>> +      { "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64)
>> +             /* ID collision with mx25l3233f. */
>> +             PARSE_SFDP
>> +             NO_SFDP_FLAGS(SECT_4K)
> 
> But what about
> +      { "differentflash",  INFO(0xc22016, 0, 64 * 1024,  64)
> +             /* ID collision with mx25l3233f. */
> +             PARSE_SFDP
> 
> Thats also valid, no? Why is having 4k sectors special? FWIW, the

no, because just the first entry with the 0xc22016 ID will be hit, the
second one will be ignored. We use a single flash entry for all the
flashes that collide.

PARSE_SFDP together with any of the:
#define SECT_4K                         BIT(1)                                  
#define SECT_4K_PMC                     BIT(2)                                  
#define SPI_NOR_DUAL_READ               BIT(3)                                  
#define SPI_NOR_QUAD_READ               BIT(4)                                  
#define SPI_NOR_OCTAL_READ              BIT(5)                                  
#define SPI_NOR_OCTAL_DTR_READ          BIT(6)                                  
#define SPI_NOR_OCTAL_DTR_PP            BIT(7)   

suggests that we'd like to differentiate between a flash that supports
SFDP and can discover its params via SFDP (no_sfdp_flags will be ignored),
and a flash that doesn't support SFDP and it's forced to initialize the
params via the no_sfdp_flags.

> function not only handles no_sfdp_flags but also erase related
> things.
> 
> So in summary, the nosfdp handling is always called when parsing
> fails (that is when there is no SFDP, not due to read errors or
> similar). I don't see why that shouldn't be the case.
> 
> -michael
> 
>> First we will try to retrieve the flash params from SFDP. If SFDP
>> fails,
>> then we'll init the flash based on the no_sfdp_flags. If SFDP succeeds
>> the no_sfdp_flags is ignored.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/6] mtd: spi-nor: core: Handle ID collisions between SFDP & non-SFDP flashes

[Michael Walle]

Am 2022-03-03 16:25, schrieb Tudor.Ambarus@microchip.com:
> On 3/3/22 16:51, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Am 2022-03-03 15:41, schrieb Tudor.Ambarus@microchip.com:
>>> On 3/1/22 23:52, 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:
>>>>> A typical differentiator between flashes whose ID collide is 
>>>>> whether
>>>>> they
>>>>> support SFDP or not. For such a collision there will be a single
>>>>> flash_info entry where the developer should specify:
>>>>> 1/ PARSE_SFDP - so that the flash that supports SFDP to initialize
>>>>> its
>>>>>    parameters by parsing the SFDP tables
>>>>> 2/ any of the no_sfdp_flags less SPI_NOR_SKIP_SFDP, to initialize 
>>>>> the
>>>>>    flash parameters via the static no_sfdp_flags for the flash that
>>>>>    doesn't support SFDP.
>>>>> Use the logic the above to handle ID collisions between SFDP &
>>>>> non-SFDP
>>>>> flashes.
>>>>> 
>>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>>>>> ---
>>>>>  drivers/mtd/spi-nor/core.c | 13 +++++++++++--
>>>>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>>>> 
>>>>> diff --git a/drivers/mtd/spi-nor/core.c 
>>>>> b/drivers/mtd/spi-nor/core.c
>>>>> index fbf3278ba29a..aef00151c116 100644
>>>>> --- a/drivers/mtd/spi-nor/core.c
>>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>>> @@ -2639,8 +2639,17 @@ static int spi_nor_init_params(struct 
>>>>> spi_nor
>>>>> *nor)
>>>>>       if (nor->info->parse_sfdp) {
>>>>>               ret = spi_nor_parse_sfdp(nor);
>>>> 
>>>> Can we return -ENOENT here if sfdp isn't supported, so we
>>>> can differentiate between "no sfdp present" and other errors?
>>> 
>>> I'll take a look.
>>> 
>>>> 
>>>>>               if (ret) {
>>>>> -                     dev_err(nor->dev, "BFPT parsing failed. 
>>>>> Please
>>>>> consider using
>>>>> SPI_NOR_SKIP_SFDP when declaring the flash\n");
>>>>> -                     return ret;
>>>>> +                     /*
>>>>> +                      * Handle ID collisions between flashes that
>>>>> support
>>>>> +                      * SFDP and flashes that don't. Initialize
>>>>> parameters
>>>>> +                      * for the flash that doesn't support SFDP.
>>>>> +                      */
>>>>> +                     if (nor->info->no_sfdp_flags &
>>>>> ~SPI_NOR_SKIP_SFDP) {
>>>> 
>>>> Shouldn't this be
>>>> if (!(nor->info->no_sfdp_flags & SPI_NOR_SKIP_SFDP))
>> 
>> Ahh I misread that for the "skip no sftp handling". But doesn't render
>> my point below invalid.
>> 
>>> No, because this will be true when no_sfdp_flags is zero, and the
>>> method
>>> from below will be called. I would like to call it when any of the
>>> no_sfdp_flags are defined, less the SPI_NOR_SKIP_SFDP flag.
>> 
>> You should add a comment then.
>> 
>>> So when one
>>> declares a flash like:
>>> +      { "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64)
>>> +             /* ID collision with mx25l3233f. */
>>> +             PARSE_SFDP
>>> +             NO_SFDP_FLAGS(SECT_4K)
>> 
>> But what about
>> +      { "differentflash",  INFO(0xc22016, 0, 64 * 1024,  64)
>> +             /* ID collision with mx25l3233f. */
>> +             PARSE_SFDP
>> 
>> Thats also valid, no? Why is having 4k sectors special? FWIW, the
> 
> no, because just the first entry with the 0xc22016 ID will be hit, the
> second one will be ignored. We use a single flash entry for all the
> flashes that collide.

Not in addition but instead of yours, of course.

> PARSE_SFDP together with any of the:
> #define SECT_4K                         BIT(1)
> #define SECT_4K_PMC                     BIT(2)
> #define SPI_NOR_DUAL_READ               BIT(3)
> #define SPI_NOR_QUAD_READ               BIT(4)
> #define SPI_NOR_OCTAL_READ              BIT(5)
> #define SPI_NOR_OCTAL_DTR_READ          BIT(6)
> #define SPI_NOR_OCTAL_DTR_PP            BIT(7)
> 
> suggests that we'd like to differentiate between a flash that supports
> SFDP and can discover its params via SFDP (no_sfdp_flags will be 
> ignored),
> and a flash that doesn't support SFDP and it's forced to initialize the
> params via the no_sfdp_flags.

I get that, but what if I have a flash, which doesn't have 4k
sectors but ordinary 64k sectors (to stick to your example). In
general, what if I have a flash where none of the above flags
are set. You only call that function if there are any no_sfdp flags
set, but they are all optional, no? Who is setting the erase opcode
for flashes with that ID but without SFDP, then?

>> function not only handles no_sfdp_flags but also erase related
>> things.
>> 
>> So in summary, the nosfdp handling is always called when parsing
>> fails (that is when there is no SFDP, not due to read errors or
>> similar). I don't see why that shouldn't be the case.
>> 
>> -michael
>> 
>>> First we will try to retrieve the flash params from SFDP. If SFDP
>>> fails,
>>> then we'll init the flash based on the no_sfdp_flags. If SFDP 
>>> succeeds
>>> the no_sfdp_flags is ignored.

-- 
-michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/6] mtd: spi-nor: core: Handle ID collisions between SFDP & non-SFDP flashes

[Tudor.Ambarus]

On 3/3/22 17:42, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-03-03 16:25, schrieb Tudor.Ambarus@microchip.com:
>> On 3/3/22 16:51, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Am 2022-03-03 15:41, schrieb Tudor.Ambarus@microchip.com:
>>>> On 3/1/22 23:52, 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:
>>>>>> A typical differentiator between flashes whose ID collide is
>>>>>> whether
>>>>>> they
>>>>>> support SFDP or not. For such a collision there will be a single
>>>>>> flash_info entry where the developer should specify:
>>>>>> 1/ PARSE_SFDP - so that the flash that supports SFDP to initialize
>>>>>> its
>>>>>>    parameters by parsing the SFDP tables
>>>>>> 2/ any of the no_sfdp_flags less SPI_NOR_SKIP_SFDP, to initialize
>>>>>> the
>>>>>>    flash parameters via the static no_sfdp_flags for the flash that
>>>>>>    doesn't support SFDP.
>>>>>> Use the logic the above to handle ID collisions between SFDP &
>>>>>> non-SFDP
>>>>>> flashes.
>>>>>>
>>>>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>>>>>> ---
>>>>>>  drivers/mtd/spi-nor/core.c | 13 +++++++++++--
>>>>>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mtd/spi-nor/core.c
>>>>>> b/drivers/mtd/spi-nor/core.c
>>>>>> index fbf3278ba29a..aef00151c116 100644
>>>>>> --- a/drivers/mtd/spi-nor/core.c
>>>>>> +++ b/drivers/mtd/spi-nor/core.c
>>>>>> @@ -2639,8 +2639,17 @@ static int spi_nor_init_params(struct
>>>>>> spi_nor
>>>>>> *nor)
>>>>>>       if (nor->info->parse_sfdp) {
>>>>>>               ret = spi_nor_parse_sfdp(nor);
>>>>>
>>>>> Can we return -ENOENT here if sfdp isn't supported, so we
>>>>> can differentiate between "no sfdp present" and other errors?
>>>>
>>>> I'll take a look.
>>>>
>>>>>
>>>>>>               if (ret) {
>>>>>> -                     dev_err(nor->dev, "BFPT parsing failed.
>>>>>> Please
>>>>>> consider using
>>>>>> SPI_NOR_SKIP_SFDP when declaring the flash\n");
>>>>>> -                     return ret;
>>>>>> +                     /*
>>>>>> +                      * Handle ID collisions between flashes that
>>>>>> support
>>>>>> +                      * SFDP and flashes that don't. Initialize
>>>>>> parameters
>>>>>> +                      * for the flash that doesn't support SFDP.
>>>>>> +                      */
>>>>>> +                     if (nor->info->no_sfdp_flags &
>>>>>> ~SPI_NOR_SKIP_SFDP) {
>>>>>
>>>>> Shouldn't this be
>>>>> if (!(nor->info->no_sfdp_flags & SPI_NOR_SKIP_SFDP))
>>>
>>> Ahh I misread that for the "skip no sftp handling". But doesn't render
>>> my point below invalid.
>>>
>>>> No, because this will be true when no_sfdp_flags is zero, and the
>>>> method
>>>> from below will be called. I would like to call it when any of the
>>>> no_sfdp_flags are defined, less the SPI_NOR_SKIP_SFDP flag.
>>>
>>> You should add a comment then.
>>>
>>>> So when one
>>>> declares a flash like:
>>>> +      { "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64)
>>>> +             /* ID collision with mx25l3233f. */
>>>> +             PARSE_SFDP
>>>> +             NO_SFDP_FLAGS(SECT_4K)
>>>
>>> But what about
>>> +      { "differentflash",  INFO(0xc22016, 0, 64 * 1024,  64)
>>> +             /* ID collision with mx25l3233f. */
>>> +             PARSE_SFDP
>>>
>>> Thats also valid, no? Why is having 4k sectors special? FWIW, the
>>
>> no, because just the first entry with the 0xc22016 ID will be hit, the
>> second one will be ignored. We use a single flash entry for all the
>> flashes that collide.
> 
> Not in addition but instead of yours, of course.
> 
>> PARSE_SFDP together with any of the:
>> #define SECT_4K                         BIT(1)
>> #define SECT_4K_PMC                     BIT(2)
>> #define SPI_NOR_DUAL_READ               BIT(3)
>> #define SPI_NOR_QUAD_READ               BIT(4)
>> #define SPI_NOR_OCTAL_READ              BIT(5)
>> #define SPI_NOR_OCTAL_DTR_READ          BIT(6)
>> #define SPI_NOR_OCTAL_DTR_PP            BIT(7)
>>
>> suggests that we'd like to differentiate between a flash that supports
>> SFDP and can discover its params via SFDP (no_sfdp_flags will be
>> ignored),
>> and a flash that doesn't support SFDP and it's forced to initialize the
>> params via the no_sfdp_flags.
> 
> I get that, but what if I have a flash, which doesn't have 4k
> sectors but ordinary 64k sectors (to stick to your example). In
> general, what if I have a flash where none of the above flags
> are set. You only call that function if there are any no_sfdp flags
> set, but they are all optional, no? Who is setting the erase opcode
> for flashes with that ID but without SFDP, then?

this use case is not handled in this proposal, indeed. I'm not sure
if there are such flashes though. Can't think of an example. We search
for a flash that operates only in 1-1-1 and can't do 4k erase.
Is that a NOR? But if you feel we should care about this case too,
I'll address it, probably I will be forced to introduce a new flag or
a bool, meh.

> 
>>> function not only handles no_sfdp_flags but also erase related
>>> things.
>>>
>>> So in summary, the nosfdp handling is always called when parsing
>>> fails (that is when there is no SFDP, not due to read errors or
>>> similar). I don't see why that shouldn't be the case.
>>>
>>> -michael
>>>
>>>> First we will try to retrieve the flash params from SFDP. If SFDP
>>>> fails,
>>>> then we'll init the flash based on the no_sfdp_flags. If SFDP
>>>> succeeds
>>>> the no_sfdp_flags is ignored.
> 
> -- 
> -michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/6] mtd: spi-nor: core: Handle ID collisions between SFDP & non-SFDP flashes

[Michael Walle]

Am 2022-03-03 17:03, schrieb Tudor.Ambarus@microchip.com:
..
>>>>> So when one
>>>>> declares a flash like:
>>>>> +      { "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64)
>>>>> +             /* ID collision with mx25l3233f. */
>>>>> +             PARSE_SFDP
>>>>> +             NO_SFDP_FLAGS(SECT_4K)
>>>> 
>>>> But what about
>>>> +      { "differentflash",  INFO(0xc22016, 0, 64 * 1024,  64)
>>>> +             /* ID collision with mx25l3233f. */
>>>> +             PARSE_SFDP
>>>> 
>>>> Thats also valid, no? Why is having 4k sectors special? FWIW, the
>>> 
>>> no, because just the first entry with the 0xc22016 ID will be hit, 
>>> the
>>> second one will be ignored. We use a single flash entry for all the
>>> flashes that collide.
>> 
>> Not in addition but instead of yours, of course.
>> 
>>> PARSE_SFDP together with any of the:
>>> #define SECT_4K                         BIT(1)
>>> #define SECT_4K_PMC                     BIT(2)
>>> #define SPI_NOR_DUAL_READ               BIT(3)
>>> #define SPI_NOR_QUAD_READ               BIT(4)
>>> #define SPI_NOR_OCTAL_READ              BIT(5)
>>> #define SPI_NOR_OCTAL_DTR_READ          BIT(6)
>>> #define SPI_NOR_OCTAL_DTR_PP            BIT(7)
>>> 
>>> suggests that we'd like to differentiate between a flash that 
>>> supports
>>> SFDP and can discover its params via SFDP (no_sfdp_flags will be
>>> ignored),
>>> and a flash that doesn't support SFDP and it's forced to initialize 
>>> the
>>> params via the no_sfdp_flags.
>> 
>> I get that, but what if I have a flash, which doesn't have 4k
>> sectors but ordinary 64k sectors (to stick to your example). In
>> general, what if I have a flash where none of the above flags
>> are set. You only call that function if there are any no_sfdp flags
>> set, but they are all optional, no? Who is setting the erase opcode
>> for flashes with that ID but without SFDP, then?
> 
> this use case is not handled in this proposal, indeed. I'm not sure
> if there are such flashes though. Can't think of an example. We search
> for a flash that operates only in 1-1-1 and can't do 4k erase.
> Is that a NOR? But if you feel we should care about this case too,
> I'll address it, probably I will be forced to introduce a new flag or
> a bool, meh.

1-1-1 with 64k sectors would be the very basic spi nor flash no?
In any case, no I don't care that much about this, it was just not
that easy to understand why you did it this way. So we should
really add a comment that we assume here that at least one
nosfdp flag is set.

-michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 3/6] mtd: spi-nor: macronix: Handle ID collision b/w MX25L3233F and MX25L3205D

[Tudor Ambarus]

Macronix has a bad habbit of reusing flash IDs. While MX25L3233F supports
RDSFDP opcode, MX25L3205D does not support it and does not recommend
issuing opcodes that are not supported ("It is not recommended to adopt
any other code not in the command definition table, which will potentially
enter the hidden mode.").

We tested the RDSFDP on the MX25L3205D and the conclusion is that the
flash didn't reply anything. Given that it is unlikely that RDSFDP will
cause any problems for the old MX25L3205D, differentiate between the two
flashes by parsing SFDP.

Tested MX25L3233F. Generated a 256 Kbyte random data and did an erase,
write, read back and compare test. The flash uses for reads
SPINOR_OP_READ_1_4_4 0xeb, for erases SPINOR_OP_BE_4K 0x20, and for writes
SPINOR_OP_PP 0x02.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
Acked-by: Pratyush Yadav <p.yadav@ti.com>
---
#  cat /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/jedec_id
c22016
# cat /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/manufacturer
macronix
# cat /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/partname
mx25l3233f
# cat /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/sfdp > mx25l3233f-sfdp


# xxd -p mx25l3233f-sfdp
53464450000101ff00000109300000ffc2000104600000ffffffffffffff
ffffffffffffffffffffffffffffffffffffe520f1ffffffff0144eb086b
083b04bbeeffffffffff00ffffff00ff0c200f5210d800ffffffffffffff
ffffffffffff003650269cf97764fecfffffffffffff

# sha1sum mx25l3233f-sfdp 
1b6e0f75b4a6d08d570082992455affa72b2dc81  mx25l3233f-sfdp

 drivers/mtd/spi-nor/macronix.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
index d81a4cb2812b..2754bbef3d2e 100644
--- a/drivers/mtd/spi-nor/macronix.c
+++ b/drivers/mtd/spi-nor/macronix.c
@@ -8,6 +8,24 @@
 
 #include "core.h"
 
+static int mx25l3205d_post_bfpt_fixups(struct spi_nor *nor,
+				const struct sfdp_parameter_header *bfpt_header,
+				const struct sfdp_bfpt *bfpt)
+{
+	/*
+	 * Macronix has a bad habit of reusing flash IDs: MX25L3233F collides
+	 * with MX25L3205D. MX25L3233F defines SFDP tables, while the older
+	 * variant does not.
+	 */
+	nor->name = "mx25l3233f";
+
+	return 0;
+}
+
+static const struct spi_nor_fixups mx25l3205d_fixups = {
+	.post_bfpt = mx25l3205d_post_bfpt_fixups,
+};
+
 static int
 mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
 			    const struct sfdp_parameter_header *bfpt_header,
@@ -44,7 +62,10 @@ static const struct flash_info macronix_nor_parts[] = {
 	{ "mx25l1606e",  INFO(0xc22015, 0, 64 * 1024,  32)
 		NO_SFDP_FLAGS(SECT_4K) },
 	{ "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64)
-		NO_SFDP_FLAGS(SECT_4K) },
+		/* ID collision with mx25l3233f. */
+		PARSE_SFDP
+		NO_SFDP_FLAGS(SECT_4K)
+		.fixups = &mx25l3205d_fixups },
 	{ "mx25l3255e",  INFO(0xc29e16, 0, 64 * 1024,  64)
 		NO_SFDP_FLAGS(SECT_4K) },
 	{ "mx25l6405d",  INFO(0xc22017, 0, 64 * 1024, 128)
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 3/6] mtd: spi-nor: macronix: Handle ID collision b/w MX25L3233F and MX25L3205D

[Michael Walle]

Am 2022-02-28 14:45, schrieb Tudor Ambarus:
> Macronix has a bad habbit of reusing flash IDs. While MX25L3233F 
> supports
> RDSFDP opcode, MX25L3205D does not support it and does not recommend
> issuing opcodes that are not supported ("It is not recommended to adopt
> any other code not in the command definition table, which will 
> potentially
> enter the hidden mode.").
> 
> We tested the RDSFDP on the MX25L3205D and the conclusion is that the
> flash didn't reply anything. Given that it is unlikely that RDSFDP will
> cause any problems for the old MX25L3205D, differentiate between the 
> two
> flashes by parsing SFDP.
> 
> Tested MX25L3233F. Generated a 256 Kbyte random data and did an erase,
> write, read back and compare test. The flash uses for reads
> SPINOR_OP_READ_1_4_4 0xeb, for erases SPINOR_OP_BE_4K 0x20, and for 
> writes
> SPINOR_OP_PP 0x02.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> Acked-by: Pratyush Yadav <p.yadav@ti.com>
> ---
> #  cat
> /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/jedec_id
> c22016
> # cat
> /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/manufacturer
> macronix
> # cat
> /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/partname
> mx25l3233f
> # cat
> /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/sfdp
> > mx25l3233f-sfdp
> 
> 
> # xxd -p mx25l3233f-sfdp
> 53464450000101ff00000109300000ffc2000104600000ffffffffffffff
> ffffffffffffffffffffffffffffffffffffe520f1ffffffff0144eb086b
> 083b04bbeeffffffffff00ffffff00ff0c200f5210d800ffffffffffffff
> ffffffffffff003650269cf97764fecfffffffffffff

Is quad enable working or has this the same problem as
the macronix flash in patch 4? Judging by the length of the SFDP
this also lacks the required information to select an
appropriate enable method. I haven't had closer look though.

> 
> # sha1sum mx25l3233f-sfdp
> 1b6e0f75b4a6d08d570082992455affa72b2dc81  mx25l3233f-sfdp
> 
>  drivers/mtd/spi-nor/macronix.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/macronix.c 
> b/drivers/mtd/spi-nor/macronix.c
> index d81a4cb2812b..2754bbef3d2e 100644
> --- a/drivers/mtd/spi-nor/macronix.c
> +++ b/drivers/mtd/spi-nor/macronix.c
> @@ -8,6 +8,24 @@
> 
>  #include "core.h"
> 
> +static int mx25l3205d_post_bfpt_fixups(struct spi_nor *nor,
> +				const struct sfdp_parameter_header *bfpt_header,
> +				const struct sfdp_bfpt *bfpt)
> +{
> +	/*
> +	 * Macronix has a bad habit of reusing flash IDs: MX25L3233F collides
> +	 * with MX25L3205D. MX25L3233F defines SFDP tables, while the older
> +	 * variant does not.
> +	 */
> +	nor->name = "mx25l3233f";
> +
> +	return 0;
> +}
> +
> +static const struct spi_nor_fixups mx25l3205d_fixups = {
> +	.post_bfpt = mx25l3205d_post_bfpt_fixups,
> +};
> +
>  static int
>  mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
>  			    const struct sfdp_parameter_header *bfpt_header,
> @@ -44,7 +62,10 @@ static const struct flash_info macronix_nor_parts[] 
> = {
>  	{ "mx25l1606e",  INFO(0xc22015, 0, 64 * 1024,  32)
>  		NO_SFDP_FLAGS(SECT_4K) },
>  	{ "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64)
> -		NO_SFDP_FLAGS(SECT_4K) },
> +		/* ID collision with mx25l3233f. */
> +		PARSE_SFDP
> +		NO_SFDP_FLAGS(SECT_4K)
> +		.fixups = &mx25l3205d_fixups },
>  	{ "mx25l3255e",  INFO(0xc29e16, 0, 64 * 1024,  64)
>  		NO_SFDP_FLAGS(SECT_4K) },
>  	{ "mx25l6405d",  INFO(0xc22017, 0, 64 * 1024, 128)

-michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 3/6] mtd: spi-nor: macronix: Handle ID collision b/w MX25L3233F and MX25L3205D

[Tudor.Ambarus]

On 3/1/22 23:57, 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:
>> Macronix has a bad habbit of reusing flash IDs. While MX25L3233F
>> supports
>> RDSFDP opcode, MX25L3205D does not support it and does not recommend
>> issuing opcodes that are not supported ("It is not recommended to adopt
>> any other code not in the command definition table, which will
>> potentially
>> enter the hidden mode.").
>>
>> We tested the RDSFDP on the MX25L3205D and the conclusion is that the
>> flash didn't reply anything. Given that it is unlikely that RDSFDP will
>> cause any problems for the old MX25L3205D, differentiate between the
>> two
>> flashes by parsing SFDP.
>>
>> Tested MX25L3233F. Generated a 256 Kbyte random data and did an erase,
>> write, read back and compare test. The flash uses for reads
>> SPINOR_OP_READ_1_4_4 0xeb, for erases SPINOR_OP_BE_4K 0x20, and for
>> writes
>> SPINOR_OP_PP 0x02.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> Acked-by: Pratyush Yadav <p.yadav@ti.com>
>> ---
>> #  cat
>> /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/jedec_id
>> c22016
>> # cat
>> /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/manufacturer
>> macronix
>> # cat
>> /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/partname
>> mx25l3233f
>> # cat
>> /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/sfdp
>> > mx25l3233f-sfdp
>>
>>
>> # xxd -p mx25l3233f-sfdp
>> 53464450000101ff00000109300000ffc2000104600000ffffffffffffff
>> ffffffffffffffffffffffffffffffffffffe520f1ffffffff0144eb086b
>> 083b04bbeeffffffffff00ffffff00ff0c200f5210d800ffffffffffffff
>> ffffffffffff003650269cf97764fecfffffffffffff
> 
> Is quad enable working or has this the same problem as
> the macronix flash in patch 4? Judging by the length of the SFDP
> this also lacks the required information to select an
> appropriate enable method. I haven't had closer look though.

it worked, yes. As I specified in the commit message, I tested it and it used
SPINOR_OP_READ_1_4_4 0xeb opcode for reads.

> 
>>
>> # sha1sum mx25l3233f-sfdp
>> 1b6e0f75b4a6d08d570082992455affa72b2dc81  mx25l3233f-sfdp
>>
>>  drivers/mtd/spi-nor/macronix.c | 23 ++++++++++++++++++++++-
>>  1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/spi-nor/macronix.c
>> b/drivers/mtd/spi-nor/macronix.c
>> index d81a4cb2812b..2754bbef3d2e 100644
>> --- a/drivers/mtd/spi-nor/macronix.c
>> +++ b/drivers/mtd/spi-nor/macronix.c
>> @@ -8,6 +8,24 @@
>>
>>  #include "core.h"
>>
>> +static int mx25l3205d_post_bfpt_fixups(struct spi_nor *nor,
>> +                             const struct sfdp_parameter_header *bfpt_header,
>> +                             const struct sfdp_bfpt *bfpt)
>> +{
>> +     /*
>> +      * Macronix has a bad habit of reusing flash IDs: MX25L3233F collides
>> +      * with MX25L3205D. MX25L3233F defines SFDP tables, while the older
>> +      * variant does not.
>> +      */
>> +     nor->name = "mx25l3233f";
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct spi_nor_fixups mx25l3205d_fixups = {
>> +     .post_bfpt = mx25l3205d_post_bfpt_fixups,
>> +};
>> +
>>  static int
>>  mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
>>                           const struct sfdp_parameter_header *bfpt_header,
>> @@ -44,7 +62,10 @@ static const struct flash_info macronix_nor_parts[]
>> = {
>>       { "mx25l1606e",  INFO(0xc22015, 0, 64 * 1024,  32)
>>               NO_SFDP_FLAGS(SECT_4K) },
>>       { "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64)
>> -             NO_SFDP_FLAGS(SECT_4K) },
>> +             /* ID collision with mx25l3233f. */
>> +             PARSE_SFDP
>> +             NO_SFDP_FLAGS(SECT_4K)
>> +             .fixups = &mx25l3205d_fixups },
>>       { "mx25l3255e",  INFO(0xc29e16, 0, 64 * 1024,  64)
>>               NO_SFDP_FLAGS(SECT_4K) },
>>       { "mx25l6405d",  INFO(0xc22017, 0, 64 * 1024, 128)
> 
> -michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 3/6] mtd: spi-nor: macronix: Handle ID collision b/w MX25L3233F and MX25L3205D

[Michael Walle]

Am 2022-03-03 16:28, schrieb Tudor.Ambarus@microchip.com:
> On 3/1/22 23:57, 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:
>>> Macronix has a bad habbit of reusing flash IDs. While MX25L3233F
>>> supports
>>> RDSFDP opcode, MX25L3205D does not support it and does not recommend
>>> issuing opcodes that are not supported ("It is not recommended to 
>>> adopt
>>> any other code not in the command definition table, which will
>>> potentially
>>> enter the hidden mode.").
>>> 
>>> We tested the RDSFDP on the MX25L3205D and the conclusion is that the
>>> flash didn't reply anything. Given that it is unlikely that RDSFDP 
>>> will
>>> cause any problems for the old MX25L3205D, differentiate between the
>>> two
>>> flashes by parsing SFDP.
>>> 
>>> Tested MX25L3233F. Generated a 256 Kbyte random data and did an 
>>> erase,
>>> write, read back and compare test. The flash uses for reads
>>> SPINOR_OP_READ_1_4_4 0xeb, for erases SPINOR_OP_BE_4K 0x20, and for
>>> writes
>>> SPINOR_OP_PP 0x02.
>>> 
>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>>> Acked-by: Pratyush Yadav <p.yadav@ti.com>
>>> ---
>>> #  cat
>>> /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/jedec_id
>>> c22016
>>> # cat
>>> /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/manufacturer
>>> macronix
>>> # cat
>>> /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/partname
>>> mx25l3233f
>>> # cat
>>> /sys/devices/platform/ahb/ahb:apb/f0020000.spi/spi_master/spi1/spi1.0/spi-nor/sfdp
>>> > mx25l3233f-sfdp
>>> 
>>> 
>>> # xxd -p mx25l3233f-sfdp
>>> 53464450000101ff00000109300000ffc2000104600000ffffffffffffff
>>> ffffffffffffffffffffffffffffffffffffe520f1ffffffff0144eb086b
>>> 083b04bbeeffffffffff00ffffff00ff0c200f5210d800ffffffffffffff
>>> ffffffffffff003650269cf97764fecfffffffffffff
>> 
>> Is quad enable working or has this the same problem as
>> the macronix flash in patch 4? Judging by the length of the SFDP
>> this also lacks the required information to select an
>> appropriate enable method. I haven't had closer look though.
> 
> it worked, yes. As I specified in the commit message, I tested it and 
> it used
> SPINOR_OP_READ_1_4_4 0xeb opcode for reads.

I'm confused, why is Heiko reporting that the CR/SR writing isn't
working because a wrong quad_enable method is chosen, but here it
will work. What am I missing?

-michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 3/6] mtd: spi-nor: macronix: Handle ID collision b/w MX25L3233F and MX25L3205D

[Michael Walle]

Am 2022-03-03 17:31, schrieb Heiko Thiery:
..

>>>>> # xxd -p mx25l3233f-sfdp
>>>>> 53464450000101ff00000109300000ffc2000104600000ffffffffffffff
>>>>> ffffffffffffffffffffffffffffffffffffe520f1ffffffff0144eb086b
>>>>> 083b04bbeeffffffffff00ffffff00ff0c200f5210d800ffffffffffffff
>>>>> ffffffffffff003650269cf97764fecfffffffffffff
>>>> 
>>>> Is quad enable working or has this the same problem as
>>>> the macronix flash in patch 4? Judging by the length of the SFDP
>>>> this also lacks the required information to select an
>>>> appropriate enable method. I haven't had closer look though.
>>> 
>>> it worked, yes. As I specified in the commit message, I tested it
>> and
>>> it used
>>> SPINOR_OP_READ_1_4_4 0xeb opcode for reads.
>> 
>> I'm confused, why is Heiko reporting that the CR/SR writing isn't
>> working because a wrong quad_enable method is chosen, but here it
>> will work. What am I missing?
> 
> I suppose that the flash that supports the RSSFDP is JEDES216B
> compatible including DWORD[15]. The flash that I have is only JEDES216
> compatible and has not the DWORD[15] defined.

That was why I wrote "Judging by the length of the SFDP". I've
converted both the mx25l12835f and mx25l3233f to binary and both
are 112 bytes long. Both seem to have the short BFPT table, ie.
no DWORD(15). Both seem to have a second table at offset 60h.

-michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 3/6] mtd: spi-nor: macronix: Handle ID collision b/w MX25L3233F and MX25L3205D

[Tudor.Ambarus]

On 3/3/22 18:45, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-03-03 17:31, schrieb Heiko Thiery:
> ..
> 
>>>>>> # xxd -p mx25l3233f-sfdp
>>>>>> 53464450000101ff00000109300000ffc2000104600000ffffffffffffff
>>>>>> ffffffffffffffffffffffffffffffffffffe520f1ffffffff0144eb086b
>>>>>> 083b04bbeeffffffffff00ffffff00ff0c200f5210d800ffffffffffffff
>>>>>> ffffffffffff003650269cf97764fecfffffffffffff
>>>>>
>>>>> Is quad enable working or has this the same problem as
>>>>> the macronix flash in patch 4? Judging by the length of the SFDP
>>>>> this also lacks the required information to select an
>>>>> appropriate enable method. I haven't had closer look though.
>>>>
>>>> it worked, yes. As I specified in the commit message, I tested it
>>> and
>>>> it used
>>>> SPINOR_OP_READ_1_4_4 0xeb opcode for reads.
>>>
>>> I'm confused, why is Heiko reporting that the CR/SR writing isn't
>>> working because a wrong quad_enable method is chosen, but here it
>>> will work. What am I missing?
>>
>> I suppose that the flash that supports the RSSFDP is JEDES216B
>> compatible including DWORD[15]. The flash that I have is only JEDES216
>> compatible and has not the DWORD[15] defined.
> 
> That was why I wrote "Judging by the length of the SFDP". I've
> converted both the mx25l12835f and mx25l3233f to binary and both
> are 112 bytes long. Both seem to have the short BFPT table, ie.
> no DWORD(15). Both seem to have a second table at offset 60h.
> 

I've just redone the test, I see:
root@sama5d2-xplained:~# mtd_debug read /dev/mtd1 0 65536 read
atmel_qspi f0020000.spi: op->cmd.opcode = 00eb, so SPINOR_OP_READ_1_4_4 as I said.

Michael, you have the eyes of an eagle, only the first 9 BFPT dwords are defined:
spi-nor spi1.0: bfpt_header->length = 9
spi-nor spi1.0: BFPT[DWORD(1)] = fff120e5
spi-nor spi1.0: BFPT[DWORD(2)] = 01ffffff
spi-nor spi1.0: BFPT[DWORD(3)] = 6b08eb44
spi-nor spi1.0: BFPT[DWORD(4)] = bb043b08
spi-nor spi1.0: BFPT[DWORD(5)] = ffffffee
spi-nor spi1.0: BFPT[DWORD(6)] = ff00ffff
spi-nor spi1.0: BFPT[DWORD(7)] = ff00ffff
spi-nor spi1.0: BFPT[DWORD(8)] = 520f200c
spi-nor spi1.0: BFPT[DWORD(9)] = ff00d810

What happens is that the QE bit is non volatile and it's already set.

spi-nor spi1.0: spi_nor_quad_enable
spi-nor spi1.0: spi_nor_sr2_bit1_quad_enable
atmel_qspi f0020000.spi: op->cmd.opcode = 0035
spi-nor spi1.0: spi_nor_sr2_bit1_quad_enable cr = ff
atmel_qspi f0020000.spi: op->cmd.opcode = 0005
spi-nor spi1.0: sr = 40

spi_nor_sr2_bit1_quad_enable is called, RDCR is ignored so 0xff,
but I did a read of the SR and surprise, it's value is 0x40, so QE set.
This is a new kind of bug :). So yes, this patch has the same problem
as Heiko's, I will update it. Thanks for the heads up!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 3/6] mtd: spi-nor: macronix: Handle ID collision b/w MX25L3233F and MX25L3205D

[Michael Walle]

Am 2022-03-04 01:36, schrieb Tudor.Ambarus@microchip.com:
> On 3/3/22 18:45, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Am 2022-03-03 17:31, schrieb Heiko Thiery:
>> ..
>> 
>>>>>>> # xxd -p mx25l3233f-sfdp
>>>>>>> 53464450000101ff00000109300000ffc2000104600000ffffffffffffff
>>>>>>> ffffffffffffffffffffffffffffffffffffe520f1ffffffff0144eb086b
>>>>>>> 083b04bbeeffffffffff00ffffff00ff0c200f5210d800ffffffffffffff
>>>>>>> ffffffffffff003650269cf97764fecfffffffffffff
>>>>>> 
>>>>>> Is quad enable working or has this the same problem as
>>>>>> the macronix flash in patch 4? Judging by the length of the SFDP
>>>>>> this also lacks the required information to select an
>>>>>> appropriate enable method. I haven't had closer look though.
>>>>> 
>>>>> it worked, yes. As I specified in the commit message, I tested it
>>>> and
>>>>> it used
>>>>> SPINOR_OP_READ_1_4_4 0xeb opcode for reads.
>>>> 
>>>> I'm confused, why is Heiko reporting that the CR/SR writing isn't
>>>> working because a wrong quad_enable method is chosen, but here it
>>>> will work. What am I missing?
>>> 
>>> I suppose that the flash that supports the RSSFDP is JEDES216B
>>> compatible including DWORD[15]. The flash that I have is only 
>>> JEDES216
>>> compatible and has not the DWORD[15] defined.
>> 
>> That was why I wrote "Judging by the length of the SFDP". I've
>> converted both the mx25l12835f and mx25l3233f to binary and both
>> are 112 bytes long. Both seem to have the short BFPT table, ie.
>> no DWORD(15). Both seem to have a second table at offset 60h.
>> 
> 
> I've just redone the test, I see:
> root@sama5d2-xplained:~# mtd_debug read /dev/mtd1 0 65536 read
> atmel_qspi f0020000.spi: op->cmd.opcode = 00eb, so
> SPINOR_OP_READ_1_4_4 as I said.
> 
> Michael, you have the eyes of an eagle, only the first 9 BFPT dwords
> are defined:
> spi-nor spi1.0: bfpt_header->length = 9
> spi-nor spi1.0: BFPT[DWORD(1)] = fff120e5
> spi-nor spi1.0: BFPT[DWORD(2)] = 01ffffff
> spi-nor spi1.0: BFPT[DWORD(3)] = 6b08eb44
> spi-nor spi1.0: BFPT[DWORD(4)] = bb043b08
> spi-nor spi1.0: BFPT[DWORD(5)] = ffffffee
> spi-nor spi1.0: BFPT[DWORD(6)] = ff00ffff
> spi-nor spi1.0: BFPT[DWORD(7)] = ff00ffff
> spi-nor spi1.0: BFPT[DWORD(8)] = 520f200c
> spi-nor spi1.0: BFPT[DWORD(9)] = ff00d810
> 
> What happens is that the QE bit is non volatile and it's already set.
> 
> spi-nor spi1.0: spi_nor_quad_enable
> spi-nor spi1.0: spi_nor_sr2_bit1_quad_enable
> atmel_qspi f0020000.spi: op->cmd.opcode = 0035
> spi-nor spi1.0: spi_nor_sr2_bit1_quad_enable cr = ff
> atmel_qspi f0020000.spi: op->cmd.opcode = 0005
> spi-nor spi1.0: sr = 40
> 
> spi_nor_sr2_bit1_quad_enable is called, RDCR is ignored so 0xff,
> but I did a read of the SR and surprise, it's value is 0x40, so QE set.
> This is a new kind of bug :). So yes, this patch has the same problem
> as Heiko's, I will update it. Thanks for the heads up!

I've given this a little bit more thought. I'm pretty sure
the QE bit is non-volatile on most flashes which share IO2
IO3 with hold#/reset#/wp#. Why is the QE bit needed in the
first place? Because it will determine the function of the
pins. For example, all our products will have WP# pin enabled
and pulled low to make sure (parts of) the flash is
write-protected. Needless to say, we cannot use quad mode.

There might be edge cases where we accidentally disable the
write protection pin. I'm not saying it is likely, though.
E.g. on our boards we have a jumper to disable the write
protection, now if linux decides for whatever reason to fiddle
with the QE bit and set it to 1 that jumper might very well
be useless after you booted linux once. Now that does ring a
bell with "linux will just disable the write protection for
no good reason on any flash", if you still remember ;)

Anyways, just to let you know. I don't have any better
idea.

-michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 3/6] mtd: spi-nor: macronix: Handle ID collision b/w MX25L3233F and MX25L3205D

[Pratyush Yadav]

On 04/03/22 03:36PM, Michael Walle wrote:
> Am 2022-03-04 01:36, schrieb Tudor.Ambarus@microchip.com:
> > On 3/3/22 18:45, Michael Walle wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe
> > > 
> > > Am 2022-03-03 17:31, schrieb Heiko Thiery:
> > > ..
> > > 
> > > > > > > > # xxd -p mx25l3233f-sfdp
> > > > > > > > 53464450000101ff00000109300000ffc2000104600000ffffffffffffff
> > > > > > > > ffffffffffffffffffffffffffffffffffffe520f1ffffffff0144eb086b
> > > > > > > > 083b04bbeeffffffffff00ffffff00ff0c200f5210d800ffffffffffffff
> > > > > > > > ffffffffffff003650269cf97764fecfffffffffffff
> > > > > > > 
> > > > > > > Is quad enable working or has this the same problem as
> > > > > > > the macronix flash in patch 4? Judging by the length of the SFDP
> > > > > > > this also lacks the required information to select an
> > > > > > > appropriate enable method. I haven't had closer look though.
> > > > > > 
> > > > > > it worked, yes. As I specified in the commit message, I tested it
> > > > > and
> > > > > > it used
> > > > > > SPINOR_OP_READ_1_4_4 0xeb opcode for reads.
> > > > > 
> > > > > I'm confused, why is Heiko reporting that the CR/SR writing isn't
> > > > > working because a wrong quad_enable method is chosen, but here it
> > > > > will work. What am I missing?
> > > > 
> > > > I suppose that the flash that supports the RSSFDP is JEDES216B
> > > > compatible including DWORD[15]. The flash that I have is only
> > > > JEDES216
> > > > compatible and has not the DWORD[15] defined.
> > > 
> > > That was why I wrote "Judging by the length of the SFDP". I've
> > > converted both the mx25l12835f and mx25l3233f to binary and both
> > > are 112 bytes long. Both seem to have the short BFPT table, ie.
> > > no DWORD(15). Both seem to have a second table at offset 60h.
> > > 
> > 
> > I've just redone the test, I see:
> > root@sama5d2-xplained:~# mtd_debug read /dev/mtd1 0 65536 read
> > atmel_qspi f0020000.spi: op->cmd.opcode = 00eb, so
> > SPINOR_OP_READ_1_4_4 as I said.
> > 
> > Michael, you have the eyes of an eagle, only the first 9 BFPT dwords
> > are defined:
> > spi-nor spi1.0: bfpt_header->length = 9
> > spi-nor spi1.0: BFPT[DWORD(1)] = fff120e5
> > spi-nor spi1.0: BFPT[DWORD(2)] = 01ffffff
> > spi-nor spi1.0: BFPT[DWORD(3)] = 6b08eb44
> > spi-nor spi1.0: BFPT[DWORD(4)] = bb043b08
> > spi-nor spi1.0: BFPT[DWORD(5)] = ffffffee
> > spi-nor spi1.0: BFPT[DWORD(6)] = ff00ffff
> > spi-nor spi1.0: BFPT[DWORD(7)] = ff00ffff
> > spi-nor spi1.0: BFPT[DWORD(8)] = 520f200c
> > spi-nor spi1.0: BFPT[DWORD(9)] = ff00d810
> > 
> > What happens is that the QE bit is non volatile and it's already set.
> > 
> > spi-nor spi1.0: spi_nor_quad_enable
> > spi-nor spi1.0: spi_nor_sr2_bit1_quad_enable
> > atmel_qspi f0020000.spi: op->cmd.opcode = 0035
> > spi-nor spi1.0: spi_nor_sr2_bit1_quad_enable cr = ff
> > atmel_qspi f0020000.spi: op->cmd.opcode = 0005
> > spi-nor spi1.0: sr = 40
> > 
> > spi_nor_sr2_bit1_quad_enable is called, RDCR is ignored so 0xff,
> > but I did a read of the SR and surprise, it's value is 0x40, so QE set.
> > This is a new kind of bug :). So yes, this patch has the same problem
> > as Heiko's, I will update it. Thanks for the heads up!
> 
> I've given this a little bit more thought. I'm pretty sure
> the QE bit is non-volatile on most flashes which share IO2
> IO3 with hold#/reset#/wp#. Why is the QE bit needed in the
> first place? Because it will determine the function of the
> pins. For example, all our products will have WP# pin enabled
> and pulled low to make sure (parts of) the flash is
> write-protected. Needless to say, we cannot use quad mode.

For those cases you should set spi-{rx,tx}-bus-width to 1 or 2 in device 
tree so SPI NOR does not attempt quad mode. If it does despite that, 
this is a bug which we should fix.

> 
> There might be edge cases where we accidentally disable the
> write protection pin. I'm not saying it is likely, though.
> E.g. on our boards we have a jumper to disable the write
> protection, now if linux decides for whatever reason to fiddle
> with the QE bit and set it to 1 that jumper might very well
> be useless after you booted linux once. Now that does ring a
> bell with "linux will just disable the write protection for
> no good reason on any flash", if you still remember ;)
> 
> Anyways, just to let you know. I don't have any better
> idea.
> 
> -michael

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 4/6] mtd: spi-nor: macronix: Handle ID collision b/w MX25L12805D and MX25L12835F

[Tudor Ambarus]

Macronix has a bad habbit of reusing flash IDs. While MX25L12835F supports
RDSFDP opcode, MX25L12805D does not. Given that it is unlikely that RDSFDP
will cause any problems for the old MX25L12805D, differentiate between the
two flashes by parsing SFDP.

cc: Heiko Thiery <heiko.thiery@gmail.com>
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
Acked-by: Pratyush Yadav <p.yadav@ti.com>
---
# cat /sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0
.0/spi-nor/sfdp | xxd -p
53464450000101ff00000109300000ffc2000104600000ffffffffffffff
ffffffffffffffffffffffffffffffffffffe520f1ffffffff0744eb086b
083b04bbfeffffffffff00ffffff44eb0c200f5210d800ffffffffffffff
ffffffffffff003600279df9c06485cbffffffffffff

 drivers/mtd/spi-nor/macronix.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
index 2754bbef3d2e..45c2f2c50e56 100644
--- a/drivers/mtd/spi-nor/macronix.c
+++ b/drivers/mtd/spi-nor/macronix.c
@@ -26,6 +26,24 @@ static const struct spi_nor_fixups mx25l3205d_fixups = {
 	.post_bfpt = mx25l3205d_post_bfpt_fixups,
 };
 
+static int mx25l12805d_post_bfpt_fixups(struct spi_nor *nor,
+				const struct sfdp_parameter_header *bfpt_header,
+				const struct sfdp_bfpt *bfpt)
+{
+	/*
+	 * Macronix has a bad habit of reusing flash IDs: MX25L12835F collides
+	 * with MX25L12805D. MX25L12835F defines SFDP tables, while the older
+	 * variant does not.
+	 */
+	nor->name = "mx25l12835f";
+
+	return 0;
+}
+
+static const struct spi_nor_fixups mx25l12805d_fixups = {
+	.post_bfpt = mx25l12805d_post_bfpt_fixups,
+};
+
 static int
 mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
 			    const struct sfdp_parameter_header *bfpt_header,
@@ -82,8 +100,11 @@ static const struct flash_info macronix_nor_parts[] = {
 	{ "mx25u6435f",  INFO(0xc22537, 0, 64 * 1024, 128)
 		NO_SFDP_FLAGS(SECT_4K) },
 	{ "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256)
+		/* ID collision with mx25l12835f. */
+		PARSE_SFDP
 		FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_4BIT_BP)
-		NO_SFDP_FLAGS(SECT_4K) },
+		NO_SFDP_FLAGS(SECT_4K)
+		.fixups = &mx25l12805d_fixups },
 	{ "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256) },
 	{ "mx25r1635f",  INFO(0xc22815, 0, 64 * 1024,  32)
 		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ |
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/6] mtd: spi-nor: macronix: Handle ID collision b/w MX25L12805D and MX25L12835F

[Heiko Thiery]

Hi Tudor,

Am Mo., 28. Feb. 2022 um 14:45 Uhr schrieb Tudor Ambarus
<tudor.ambarus@microchip.com>:
>
> Macronix has a bad habbit of reusing flash IDs. While MX25L12835F supports
> RDSFDP opcode, MX25L12805D does not. Given that it is unlikely that RDSFDP
> will cause any problems for the old MX25L12805D, differentiate between the
> two flashes by parsing SFDP.
>
> cc: Heiko Thiery <heiko.thiery@gmail.com>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> Acked-by: Pratyush Yadav <p.yadav@ti.com>
> ---
> # cat /sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0
> .0/spi-nor/sfdp | xxd -p
> 53464450000101ff00000109300000ffc2000104600000ffffffffffffff
> ffffffffffffffffffffffffffffffffffffe520f1ffffffff0744eb086b
> 083b04bbfeffffffffff00ffffff44eb0c200f5210d800ffffffffffffff
> ffffffffffff003600279df9c06485cbffffffffffff
>
>  drivers/mtd/spi-nor/macronix.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> index 2754bbef3d2e..45c2f2c50e56 100644
> --- a/drivers/mtd/spi-nor/macronix.c
> +++ b/drivers/mtd/spi-nor/macronix.c
> @@ -26,6 +26,24 @@ static const struct spi_nor_fixups mx25l3205d_fixups = {
>         .post_bfpt = mx25l3205d_post_bfpt_fixups,
>  };
>
> +static int mx25l12805d_post_bfpt_fixups(struct spi_nor *nor,
> +                               const struct sfdp_parameter_header *bfpt_header,
> +                               const struct sfdp_bfpt *bfpt)
> +{
> +       /*
> +        * Macronix has a bad habit of reusing flash IDs: MX25L12835F collides
> +        * with MX25L12805D. MX25L12835F defines SFDP tables, while the older
> +        * variant does not.
> +        */
> +       nor->name = "mx25l12835f";
> +
> +       return 0;
> +}
> +
> +static const struct spi_nor_fixups mx25l12805d_fixups = {
> +       .post_bfpt = mx25l12805d_post_bfpt_fixups,
> +};
> +
>  static int
>  mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
>                             const struct sfdp_parameter_header *bfpt_header,
> @@ -82,8 +100,11 @@ static const struct flash_info macronix_nor_parts[] = {
>         { "mx25u6435f",  INFO(0xc22537, 0, 64 * 1024, 128)
>                 NO_SFDP_FLAGS(SECT_4K) },
>         { "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256)
> +               /* ID collision with mx25l12835f. */
> +               PARSE_SFDP
>                 FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_4BIT_BP)
> -               NO_SFDP_FLAGS(SECT_4K) },
> +               NO_SFDP_FLAGS(SECT_4K)
> +               .fixups = &mx25l12805d_fixups },
>         { "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256) },
>         { "mx25r1635f",  INFO(0xc22815, 0, 64 * 1024,  32)
>                 NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ |
> --
> 2.25.1
>

I tried this patch and saw that the flash is no longer detected. I did
some debugging and see now that the correct function to set the quad
mode (spi_nor_sr1_bit6_quad_enable) is not called. Instead the
spi_nor_sr2_bit1_quad_enable() is invoked. Further debbuging showed me
that the macronix specific fixup is not called.

For the flash that does support SFDP parsing the
spi_nor_manufacturer_init_params() is not called. Is that expected to
be?

--
Heiko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/6] mtd: spi-nor: macronix: Handle ID collision b/w MX25L12805D and MX25L12835F

[Tudor.Ambarus]

On 3/1/22 09:55, Heiko Thiery wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Tudor,

Hi!

> 
> Am Mo., 28. Feb. 2022 um 14:45 Uhr schrieb Tudor Ambarus
> <tudor.ambarus@microchip.com>:
>>
>> Macronix has a bad habbit of reusing flash IDs. While MX25L12835F supports
>> RDSFDP opcode, MX25L12805D does not. Given that it is unlikely that RDSFDP
>> will cause any problems for the old MX25L12805D, differentiate between the
>> two flashes by parsing SFDP.
>>
>> cc: Heiko Thiery <heiko.thiery@gmail.com>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> Acked-by: Pratyush Yadav <p.yadav@ti.com>
>> ---
>> # cat /sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0
>> .0/spi-nor/sfdp | xxd -p
>> 53464450000101ff00000109300000ffc2000104600000ffffffffffffff
>> ffffffffffffffffffffffffffffffffffffe520f1ffffffff0744eb086b
>> 083b04bbfeffffffffff00ffffff44eb0c200f5210d800ffffffffffffff
>> ffffffffffff003600279df9c06485cbffffffffffff
>>
>>  drivers/mtd/spi-nor/macronix.c | 23 ++++++++++++++++++++++-
>>  1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
>> index 2754bbef3d2e..45c2f2c50e56 100644
>> --- a/drivers/mtd/spi-nor/macronix.c
>> +++ b/drivers/mtd/spi-nor/macronix.c
>> @@ -26,6 +26,24 @@ static const struct spi_nor_fixups mx25l3205d_fixups = {
>>         .post_bfpt = mx25l3205d_post_bfpt_fixups,
>>  };
>>
>> +static int mx25l12805d_post_bfpt_fixups(struct spi_nor *nor,
>> +                               const struct sfdp_parameter_header *bfpt_header,
>> +                               const struct sfdp_bfpt *bfpt)
>> +{
>> +       /*
>> +        * Macronix has a bad habit of reusing flash IDs: MX25L12835F collides
>> +        * with MX25L12805D. MX25L12835F defines SFDP tables, while the older
>> +        * variant does not.
>> +        */
>> +       nor->name = "mx25l12835f";
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct spi_nor_fixups mx25l12805d_fixups = {
>> +       .post_bfpt = mx25l12805d_post_bfpt_fixups,
>> +};
>> +
>>  static int
>>  mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
>>                             const struct sfdp_parameter_header *bfpt_header,
>> @@ -82,8 +100,11 @@ static const struct flash_info macronix_nor_parts[] = {
>>         { "mx25u6435f",  INFO(0xc22537, 0, 64 * 1024, 128)
>>                 NO_SFDP_FLAGS(SECT_4K) },
>>         { "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256)
>> +               /* ID collision with mx25l12835f. */
>> +               PARSE_SFDP
>>                 FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_4BIT_BP)
>> -               NO_SFDP_FLAGS(SECT_4K) },
>> +               NO_SFDP_FLAGS(SECT_4K)
>> +               .fixups = &mx25l12805d_fixups },
>>         { "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256) },
>>         { "mx25r1635f",  INFO(0xc22815, 0, 64 * 1024,  32)
>>                 NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ |
>> --
>> 2.25.1
>>
> 
> I tried this patch and saw that the flash is no longer detected. I did

Thanks! No longer detected? ReadID fails? What do you get on the console?

> some debugging and see now that the correct function to set the quad
> mode (spi_nor_sr1_bit6_quad_enable) is not called. Instead the
> spi_nor_sr2_bit1_quad_enable() is invoked. Further debbuging showed me
> that the macronix specific fixup is not called.
> 

ok, this bit is clear

> For the flash that does support SFDP parsing the
> spi_nor_manufacturer_init_params() is not called. Is that expected to
> be?

Yes, I would like to get rid of the default_init() hooks for new code.
I'm addressing it right now. Would be good if you can help testing it.
Will add you in to:

Cheers,
ta


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 4/6] mtd: spi-nor: macronix: Handle ID collision b/w MX25L12805D and MX25L12835F

[Heiko Thiery]

Hi Tudor,

Am Di., 1. März 2022 um 09:52 Uhr schrieb <Tudor.Ambarus@microchip.com>:
>
> On 3/1/22 09:55, Heiko Thiery wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >
> > Hi Tudor,
>
> Hi!
>
> >
> > Am Mo., 28. Feb. 2022 um 14:45 Uhr schrieb Tudor Ambarus
> > <tudor.ambarus@microchip.com>:
> >>
> >> Macronix has a bad habbit of reusing flash IDs. While MX25L12835F supports
> >> RDSFDP opcode, MX25L12805D does not. Given that it is unlikely that RDSFDP
> >> will cause any problems for the old MX25L12805D, differentiate between the
> >> two flashes by parsing SFDP.
> >>
> >> cc: Heiko Thiery <heiko.thiery@gmail.com>
> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> >> Acked-by: Pratyush Yadav <p.yadav@ti.com>
> >> ---
> >> # cat /sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0
> >> .0/spi-nor/sfdp | xxd -p
> >> 53464450000101ff00000109300000ffc2000104600000ffffffffffffff
> >> ffffffffffffffffffffffffffffffffffffe520f1ffffffff0744eb086b
> >> 083b04bbfeffffffffff00ffffff44eb0c200f5210d800ffffffffffffff
> >> ffffffffffff003600279df9c06485cbffffffffffff
> >>
> >>  drivers/mtd/spi-nor/macronix.c | 23 ++++++++++++++++++++++-
> >>  1 file changed, 22 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> >> index 2754bbef3d2e..45c2f2c50e56 100644
> >> --- a/drivers/mtd/spi-nor/macronix.c
> >> +++ b/drivers/mtd/spi-nor/macronix.c
> >> @@ -26,6 +26,24 @@ static const struct spi_nor_fixups mx25l3205d_fixups = {
> >>         .post_bfpt = mx25l3205d_post_bfpt_fixups,
> >>  };
> >>
> >> +static int mx25l12805d_post_bfpt_fixups(struct spi_nor *nor,
> >> +                               const struct sfdp_parameter_header *bfpt_header,
> >> +                               const struct sfdp_bfpt *bfpt)
> >> +{
> >> +       /*
> >> +        * Macronix has a bad habit of reusing flash IDs: MX25L12835F collides
> >> +        * with MX25L12805D. MX25L12835F defines SFDP tables, while the older
> >> +        * variant does not.
> >> +        */
> >> +       nor->name = "mx25l12835f";
> >> +
> >> +       return 0;
> >> +}
> >> +
> >> +static const struct spi_nor_fixups mx25l12805d_fixups = {
> >> +       .post_bfpt = mx25l12805d_post_bfpt_fixups,
> >> +};
> >> +
> >>  static int
> >>  mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
> >>                             const struct sfdp_parameter_header *bfpt_header,
> >> @@ -82,8 +100,11 @@ static const struct flash_info macronix_nor_parts[] = {
> >>         { "mx25u6435f",  INFO(0xc22537, 0, 64 * 1024, 128)
> >>                 NO_SFDP_FLAGS(SECT_4K) },
> >>         { "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256)
> >> +               /* ID collision with mx25l12835f. */
> >> +               PARSE_SFDP
> >>                 FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_4BIT_BP)
> >> -               NO_SFDP_FLAGS(SECT_4K) },
> >> +               NO_SFDP_FLAGS(SECT_4K)
> >> +               .fixups = &mx25l12805d_fixups },
> >>         { "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256) },
> >>         { "mx25r1635f",  INFO(0xc22815, 0, 64 * 1024,  32)
> >>                 NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ |
> >> --
> >> 2.25.1
> >>
> >
> > I tried this patch and saw that the flash is no longer detected. I did
>
> Thanks! No longer detected? ReadID fails? What do you get on the console?

[    1.442225] spi-nor spi0.0: CR: read back test failed
[    1.445979] spi-nor spi0.0: quad mode not supported
[    1.445989] spi-nor: probe of spi0.0 failed with error -5

> > some debugging and see now that the correct function to set the quad
> > mode (spi_nor_sr1_bit6_quad_enable) is not called. Instead the
> > spi_nor_sr2_bit1_quad_enable() is invoked. Further debbuging showed me
> > that the macronix specific fixup is not called.
> >
>
> ok, this bit is clear
>
> > For the flash that does support SFDP parsing the
> > spi_nor_manufacturer_init_params() is not called. Is that expected to
> > be?
>
> Yes, I would like to get rid of the default_init() hooks for new code.
> I'm addressing it right now. Would be good if you can help testing it.
> Will add you in to:

Yes, of course. Meanwhile I figured out that the SFDP of this flash
does not contain the required table (DWORD[15]) to figure out what
function should be used for quad_enable.

-- 
Heiko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[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,
 	&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;
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Michael Walle]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Tudor.Ambarus]

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?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Michael Walle]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Tudor.Ambarus]

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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Michael Walle]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[George Brooke]

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Tudor.Ambarus]

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 6/6] mtd: spi-nor: manuf-id-collisions: Add support for xt25f128b

[Tudor Ambarus]

Flash does not support continuation codes and may collide with a flash
of other manufacturer, Intersil being an example. Add support for
xt25f128b.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
0000000 4653 5044 0100 ff01 0000 0901 0030 ff00
0000010 000b 0301 0060 ff00 ffff ffff ffff ffff
0000020 ffff ffff ffff ffff ffff ffff ffff ffff
0000030 20e5 fff1 ffff 07ff eb44 6b08 3b08 bb42
0000040 ffee ffff ffff ff00 ffff ff00 200c 520f
0000050 d810 ff00 ffff ffff ffff ffff ffff ffff
0000060 3600 2700 f99f 6477 e8d9 ffff

 drivers/mtd/spi-nor/manuf-id-collisions.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/mtd/spi-nor/manuf-id-collisions.c b/drivers/mtd/spi-nor/manuf-id-collisions.c
index 75c5ad6480ee..0447e245f4b1 100644
--- a/drivers/mtd/spi-nor/manuf-id-collisions.c
+++ b/drivers/mtd/spi-nor/manuf-id-collisions.c
@@ -17,6 +17,15 @@ static const struct spi_nor_fixups boya_nor_fixups = {
 	.late_init = boya_nor_late_init,
 };
 
+static void xtx_nor_late_init(struct spi_nor *nor)
+{
+	nor->manufacturer_name = "xtx";
+}
+
+static const struct spi_nor_fixups xtx_nor_fixups = {
+	.late_init = xtx_nor_late_init,
+};
+
 static const struct flash_info id_collision_parts[] = {
 	/* Boya */
 	{ "by25q128as", INFO(0x684018, 0, 64 * 1024, 256)
@@ -24,6 +33,11 @@ static const struct flash_info id_collision_parts[] = {
 		NO_SFDP_FLAGS(SPI_NOR_SKIP_SFDP | SECT_4K | SPI_NOR_DUAL_READ |
 			      SPI_NOR_QUAD_READ)
 		.fixups = &boya_nor_fixups },
+
+	/* XTX (XTX Technology Limited) */
+	{ "xt25f128b", INFO(0x0b4018, 0, 64 * 1024, 256)
+		PARSE_SFDP
+		.fixups = &xtx_nor_fixups },
 };
 
 const struct spi_nor_manufacturer spi_nor_manuf_id_collisions = {
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 6/6] mtd: spi-nor: manuf-id-collisions: Add support for xt25f128b

[Michael Walle]

Am 2022-02-28 14:45, schrieb Tudor Ambarus:
> Flash does not support continuation codes and may collide with a flash
> of other manufacturer, Intersil being an example. Add support for
> xt25f128b.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
> 0000000 4653 5044 0100 ff01 0000 0901 0030 ff00
> 0000010 000b 0301 0060 ff00 ffff ffff ffff ffff
> 0000020 ffff ffff ffff ffff ffff ffff ffff ffff
> 0000030 20e5 fff1 ffff 07ff eb44 6b08 3b08 bb42
> 0000040 ffee ffff ffff ff00 ffff ff00 200c 520f
> 0000050 d810 ff00 ffff ffff ffff ffff ffff ffff
> 0000060 3600 2700 f99f 6477 e8d9 ffff

You don't have this flash no? because the md5sum is
missing.

> 
>  drivers/mtd/spi-nor/manuf-id-collisions.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/manuf-id-collisions.c
> b/drivers/mtd/spi-nor/manuf-id-collisions.c
> index 75c5ad6480ee..0447e245f4b1 100644
> --- a/drivers/mtd/spi-nor/manuf-id-collisions.c
> +++ b/drivers/mtd/spi-nor/manuf-id-collisions.c
> @@ -17,6 +17,15 @@ static const struct spi_nor_fixups boya_nor_fixups = 
> {
>  	.late_init = boya_nor_late_init,
>  };
> 
> +static void xtx_nor_late_init(struct spi_nor *nor)
> +{
> +	nor->manufacturer_name = "xtx";
> +}
> +
> +static const struct spi_nor_fixups xtx_nor_fixups = {
> +	.late_init = xtx_nor_late_init,
> +};
> +
>  static const struct flash_info id_collision_parts[] = {
>  	/* Boya */
>  	{ "by25q128as", INFO(0x684018, 0, 64 * 1024, 256)
> @@ -24,6 +33,11 @@ static const struct flash_info id_collision_parts[] 
> = {
>  		NO_SFDP_FLAGS(SPI_NOR_SKIP_SFDP | SECT_4K | SPI_NOR_DUAL_READ |
>  			      SPI_NOR_QUAD_READ)
>  		.fixups = &boya_nor_fixups },
> +
> +	/* XTX (XTX Technology Limited) */
> +	{ "xt25f128b", INFO(0x0b4018, 0, 64 * 1024, 256)
> +		PARSE_SFDP
> +		.fixups = &xtx_nor_fixups },

I'd suggest to order the entries by the id to make it
easier to spot collisions.

-michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 6/6] mtd: spi-nor: manuf-id-collisions: Add support for xt25f128b

[Chris Morgan]

On Tue, Mar 01, 2022 at 11:23:24PM +0100, Michael Walle wrote:
> Am 2022-02-28 14:45, schrieb Tudor Ambarus:
> > Flash does not support continuation codes and may collide with a flash
> > of other manufacturer, Intersil being an example. Add support for
> > xt25f128b.
> > 
> > Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> > ---
> > 0000000 4653 5044 0100 ff01 0000 0901 0030 ff00
> > 0000010 000b 0301 0060 ff00 ffff ffff ffff ffff
> > 0000020 ffff ffff ffff ffff ffff ffff ffff ffff
> > 0000030 20e5 fff1 ffff 07ff eb44 6b08 3b08 bb42
> > 0000040 ffee ffff ffff ff00 ffff ff00 200c 520f
> > 0000050 d810 ff00 ffff ffff ffff ffff ffff ffff
> > 0000060 3600 2700 f99f 6477 e8d9 ffff
> 
> You don't have this flash no? because the md5sum is
> missing.

I have this flash, what do you need from me?

I haven't had a chance to test this just yet, but I'm about to. I'll
report back with my status.

Chris

> 
> > 
> >  drivers/mtd/spi-nor/manuf-id-collisions.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/drivers/mtd/spi-nor/manuf-id-collisions.c
> > b/drivers/mtd/spi-nor/manuf-id-collisions.c
> > index 75c5ad6480ee..0447e245f4b1 100644
> > --- a/drivers/mtd/spi-nor/manuf-id-collisions.c
> > +++ b/drivers/mtd/spi-nor/manuf-id-collisions.c
> > @@ -17,6 +17,15 @@ static const struct spi_nor_fixups boya_nor_fixups =
> > {
> >  	.late_init = boya_nor_late_init,
> >  };
> > 
> > +static void xtx_nor_late_init(struct spi_nor *nor)
> > +{
> > +	nor->manufacturer_name = "xtx";
> > +}
> > +
> > +static const struct spi_nor_fixups xtx_nor_fixups = {
> > +	.late_init = xtx_nor_late_init,
> > +};
> > +
> >  static const struct flash_info id_collision_parts[] = {
> >  	/* Boya */
> >  	{ "by25q128as", INFO(0x684018, 0, 64 * 1024, 256)
> > @@ -24,6 +33,11 @@ static const struct flash_info id_collision_parts[] =
> > {
> >  		NO_SFDP_FLAGS(SPI_NOR_SKIP_SFDP | SECT_4K | SPI_NOR_DUAL_READ |
> >  			      SPI_NOR_QUAD_READ)
> >  		.fixups = &boya_nor_fixups },
> > +
> > +	/* XTX (XTX Technology Limited) */
> > +	{ "xt25f128b", INFO(0x0b4018, 0, 64 * 1024, 256)
> > +		PARSE_SFDP
> > +		.fixups = &xtx_nor_fixups },
> 
> I'd suggest to order the entries by the id to make it
> easier to spot collisions.
> 
> -michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 6/6] mtd: spi-nor: manuf-id-collisions: Add support for xt25f128b

[Tudor.Ambarus]

On 3/3/22 23:04, Chris Morgan wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Tue, Mar 01, 2022 at 11:23:24PM +0100, Michael Walle wrote:
>> Am 2022-02-28 14:45, schrieb Tudor Ambarus:
>>> Flash does not support continuation codes and may collide with a flash
>>> of other manufacturer, Intersil being an example. Add support for
>>> xt25f128b.
>>>
>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>>> ---
>>> 0000000 4653 5044 0100 ff01 0000 0901 0030 ff00
>>> 0000010 000b 0301 0060 ff00 ffff ffff ffff ffff
>>> 0000020 ffff ffff ffff ffff ffff ffff ffff ffff
>>> 0000030 20e5 fff1 ffff 07ff eb44 6b08 3b08 bb42
>>> 0000040 ffee ffff ffff ff00 ffff ff00 200c 520f
>>> 0000050 d810 ff00 ffff ffff ffff ffff ffff ffff
>>> 0000060 3600 2700 f99f 6477 e8d9 ffff
>>
>> You don't have this flash no? because the md5sum is
>> missing.
> 
> I have this flash, what do you need from me?

Hi, Chris,

Thanks for getting back!

I would need you to dump all the SPI NOR sysfs entries and then do a little
test. Here's an example:
# cat /sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0.0/spi-nor/jedec_id
c22018
# cat /sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0.0/spi-nor/manufacturer
macronix
# cat /sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0.0/spi-nor/partname
mx25l12835f
# xxd -p /sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0.0/spi-nor/sfdp
53464450000101ff00000109300000ffc2000104600000ffffffffffffff
ffffffffffffffffffffffffffffffffffffe520f1ffffffff0744eb086b
083b04bbfeffffffffff00ffffff44eb0c200f5210d800ffffffffffffff
ffffffffffff003600279df9c06485cbffffffffffff
# sha1sum  /sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0.0/spi-nor/sfdp
c5e5abe6c5650a9d9c448690b1eeebdf5bfe57d4
/sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0.0/spi-nor/sfdp

And then an erase, write, read back and compare test.
1/ generate a 6M file:
dd if=/dev/urandom of=./nor_test bs=1M count=6
2/ read first 6MB from flash and check if is already erased:
time mtd_debug read /dev/mtd5 0 6291456 nor_read
hexdump nor_read
	If at the hexdump you'll see anything that just 0xff, the flash has
something written to it and you can go to step 3. If you'll see just 0xff
data, then jump at 4/

3/ verify if the erase is successful, then do a write, read-back and compare test.
time mtd_debug erase /dev/mtd5 0 6291456
time mtd_debug read /dev/mtd5 0 6291456 nor_read
hexdump nor_read
time mtd_debug write /dev/mtd5 0 6291456 nor_test
time mtd_debug read /dev/mtd5 0 6291456 nor_read
sha1sum nor_test nor_read

If you reached here the test ends, you can ignore 4/.

4/ if you reached here the first 6MB of the flash is already erased. Do a write
read back and compare test, followed by an erase test:
time mtd_debug write /dev/mtd5 0 6291456 nor_test
time mtd_debug read /dev/mtd5 0 6291456 nor_read
sha1sum nor_test nor_read
time mtd_debug erase /dev/mtd5 0 6291456
time mtd_debug read /dev/mtd5 0 6291456 nor_read
hexdump nor_read
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 6/6] mtd: spi-nor: manuf-id-collisions: Add support for xt25f128b

[Chris Morgan]

On Thu, Mar 03, 2022 at 11:50:34PM +0000, Tudor.Ambarus@microchip.com wrote:
> On 3/3/22 23:04, Chris Morgan wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Tue, Mar 01, 2022 at 11:23:24PM +0100, Michael Walle wrote:
> >> Am 2022-02-28 14:45, schrieb Tudor Ambarus:
> >>> Flash does not support continuation codes and may collide with a flash
> >>> of other manufacturer, Intersil being an example. Add support for
> >>> xt25f128b.
> >>>
> >>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> >>> ---
> >>> 0000000 4653 5044 0100 ff01 0000 0901 0030 ff00
> >>> 0000010 000b 0301 0060 ff00 ffff ffff ffff ffff
> >>> 0000020 ffff ffff ffff ffff ffff ffff ffff ffff
> >>> 0000030 20e5 fff1 ffff 07ff eb44 6b08 3b08 bb42
> >>> 0000040 ffee ffff ffff ff00 ffff ff00 200c 520f
> >>> 0000050 d810 ff00 ffff ffff ffff ffff ffff ffff
> >>> 0000060 3600 2700 f99f 6477 e8d9 ffff
> >>
> >> You don't have this flash no? because the md5sum is
> >> missing.
> > 
> > I have this flash, what do you need from me?
> 
> Hi, Chris,
> 
> Thanks for getting back!
> 
> I would need you to dump all the SPI NOR sysfs entries and then do a little
> test. Here's an example:
> # cat /sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0.0/spi-nor/jedec_id
> c22018
> # cat /sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0.0/spi-nor/manufacturer
> macronix
> # cat /sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0.0/spi-nor/partname
> mx25l12835f
> # xxd -p /sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0.0/spi-nor/sfdp
> 53464450000101ff00000109300000ffc2000104600000ffffffffffffff
> ffffffffffffffffffffffffffffffffffffe520f1ffffffff0744eb086b
> 083b04bbfeffffffffff00ffffff44eb0c200f5210d800ffffffffffffff
> ffffffffffff003600279df9c06485cbffffffffffff
> # sha1sum  /sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0.0/spi-nor/sfdp
> c5e5abe6c5650a9d9c448690b1eeebdf5bfe57d4
> /sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0.0/spi-nor/sfdp
> 
> And then an erase, write, read back and compare test.
> 1/ generate a 6M file:
> dd if=/dev/urandom of=./nor_test bs=1M count=6
> 2/ read first 6MB from flash and check if is already erased:
> time mtd_debug read /dev/mtd5 0 6291456 nor_read
> hexdump nor_read
> 	If at the hexdump you'll see anything that just 0xff, the flash has
> something written to it and you can go to step 3. If you'll see just 0xff
> data, then jump at 4/
> 
> 3/ verify if the erase is successful, then do a write, read-back and compare test.
> time mtd_debug erase /dev/mtd5 0 6291456
> time mtd_debug read /dev/mtd5 0 6291456 nor_read
> hexdump nor_read
> time mtd_debug write /dev/mtd5 0 6291456 nor_test
> time mtd_debug read /dev/mtd5 0 6291456 nor_read
> sha1sum nor_test nor_read
> 
> If you reached here the test ends, you can ignore 4/.
> 
> 4/ if you reached here the first 6MB of the flash is already erased. Do a write
> read back and compare test, followed by an erase test:
> time mtd_debug write /dev/mtd5 0 6291456 nor_test
> time mtd_debug read /dev/mtd5 0 6291456 nor_read
> sha1sum nor_test nor_read
> time mtd_debug erase /dev/mtd5 0 6291456
> time mtd_debug read /dev/mtd5 0 6291456 nor_read
> hexdump nor_read

Looks good to me. See below for test outputs. Thank you for all your hard work
on this.

Tested-by: Chris Morgan <macromorgan@hotmail.com>

# cat /sys/bus/spi/devices/spi2.0/spi-nor/jedec_id
0b4018

# cat /sys/bus/spi/devices/spi2.0/spi-nor/manufacturer
xtx

# cat /sys/bus/spi/devices/spi2.0/spi-nor/partname
xt25f128b

# xxd -p /sys/bus/spi/devices/spi2.0/spi-nor/sfdp
53464450000101ff00000109300000ff0b000103600000ffffffffffffff
ffffffffffffffffffffffffffffffffffffe520f1ffffffff0744eb086b
083b42bbeeffffffffff00ffffff00ff0c200f5210d800ffffffffffffff
ffffffffffff003600279ff97764d9e8ffff

# sha1sum /sys/bus/spi/devices/spi2.0/spi-nor/sfdp
191346c29bddd9ea16bc59727c390df50687313f  /sys/bus/spi/devices/spi2.0/spi-nor/sfdp

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

real    2m7.957s
user    0m0.001s
sys     2m7.868s

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

real    0m0.785s
user    0m0.000s
sys     0m0.259s

# 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    0m8.851s
user    0m0.000s
sys     0m6.020s

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

real    0m0.811s
user    0m0.000s
sys     0m0.285s

# sha1sum nor_test nor_read
1404362fff3b2f8a79f8090901a5276e66598b72  nor_test
1404362fff3b2f8a79f8090901a5276e66598b72  nor_read

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 0/6] mtd: spi-nor: Handle ID collisions

[Michael Walle]

Am 2022-02-28 14:44, schrieb Tudor Ambarus:
> Tested with MX25L3233F. I need sysfs dumps where there aren't.
> 
> v4:
> - adapt core to handle collisions between SFDP and non-SFDP flashes
> - rebase on latest spi-nor/next
> 
> v3:
> - report correct manufacturer name in ID collisions driver
> - use fixup name of the first flash, not of the one that collide
> - use xxd to dump sfdp (where I had the flash)
> - drop locking support on xt25f128b
> - constify flash_info fixups hook
> 
> Tudor Ambarus (6):
>   mtd: spi-nor: core: Report correct name in case of ID collisions
>   mtd: spi-nor: core: Handle ID collisions between SFDP & non-SFDP
>     flashes
>   mtd: spi-nor: macronix: Handle ID collision b/w MX25L3233F and
>     MX25L3205D
>   mtd: spi-nor: macronix: Handle ID collision b/w MX25L12805D and
>     MX25L12835F
>   mtd: spi-nor: Introduce Manufacturer ID collisions driver
>   mtd: spi-nor: manuf-id-collisions: Add support for xt25f128b
> 
>  drivers/mtd/spi-nor/Makefile              |  1 +
>  drivers/mtd/spi-nor/core.c                | 21 +++++++++--
>  drivers/mtd/spi-nor/core.h                |  1 +
>  drivers/mtd/spi-nor/macronix.c            | 46 ++++++++++++++++++++++-
>  drivers/mtd/spi-nor/manuf-id-collisions.c | 46 +++++++++++++++++++++++
>  drivers/mtd/spi-nor/sysfs.c               |  4 +-
>  include/linux/mtd/spi-nor.h               |  6 +++
>  7 files changed, 118 insertions(+), 7 deletions(-)
>  create mode 100644 drivers/mtd/spi-nor/manuf-id-collisions.c

drivers/mtd/spi-nor/xmc.c should probably be moved into
manuf-id-collisions as it actually reusing the micron/st
vendor id (?!).

-michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel