[PATCH v5 0/5] Add support for SPI-NOR Macronix OTP

[PATCH v5 0/5] Add support for SPI-NOR Macronix OTP

[Erez Geva]

From: Erez Geva <ErezGeva2@gmail.com>

Add support for SPI-NOR Macronix OTP.
And add MX25L12833F with OTP.

v2:
Improve description of mx25l12833f.
Add note about mx25l12833f using the same JEDEC ID as mx25l12805d.

v3:
Improve description.
Rename _nor_send_cmd() to spi_nor_send_cmd_internal().
Remove MX25L12833F specific changes.
Add reading SFDP to all Macronix chips.
Add support of reading OTP parameters from device tree.
Reorgenize patches to 2 SPI-NOR patches and 2 Macronix patches.
Testing with MX25L3233F using BeagleBone Black.
Test results are in "mtd: spi-nor: macronix: add manufacturer flags" patch.

v4:
Add DT bindings of new OTP parameters.
Test OTP with 'flash_otp_write' and 'flash_otp_lock'.
Fix macronix_nor_otp_lock() and macronix_nor_otp_is_locked(),
Macronix uses a single flag for all regions.

v5:
Fix DT binding errors.
Fix functions description of Macronix OTP functions.

Erez Geva (5):
  mtd: spi-nor: core: add manufacturer flags
  mtd: spi-nor: core: add generic functions
  dt-bindings: mtd: spi-nor: add OTP parameters
  mtd: spi-nor: macronix: add support for OTP
  mtd: spi-nor: macronix: add manufacturer flags

 .../bindings/mtd/jedec,spi-nor.yaml           |  39 ++++
 drivers/mtd/spi-nor/core.c                    | 166 +++++++++++++----
 drivers/mtd/spi-nor/core.h                    |  34 +---
 drivers/mtd/spi-nor/macronix.c                | 168 ++++++++++++++++++
 drivers/mtd/spi-nor/otp.c                     |   6 +-
 drivers/mtd/spi-nor/winbond.c                 |   2 +-
 include/linux/mtd/spi-nor.h                   |   9 +
 7 files changed, 356 insertions(+), 68 deletions(-)

-- 
2.39.5

[PATCH v5 1/5] mtd: spi-nor: core: add manufacturer flags

[Erez Geva]

From: Erez Geva <ErezGeva2@gmail.com>

Add flag for always trying reading SFDP:
Some vendors reuse all JEDEC IDs on manufacture table
 with new chips that support SFDP.

Add flag for reading OTP parameters from device tree.
Some vendors reuse JEDEC IDs
 with several chips with different OTP parameters.
Alternatively we read parameters from SFDP.
But the OTP parameters are absent from the SFDP.
So there is not other way but to add the OTP parameters in the device tree.

In this patch series we use the new flags with Macronix.

Signed-off-by: Erez Geva <ErezGeva2@gmail.com>
---
 drivers/mtd/spi-nor/core.c    | 36 ++++++++++++++++++++++++++++++-----
 drivers/mtd/spi-nor/core.h    |  7 ++++++-
 drivers/mtd/spi-nor/otp.c     |  6 +++---
 drivers/mtd/spi-nor/winbond.c |  2 +-
 4 files changed, 41 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 9d6e85bf227b..0238475059f0 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2891,10 +2891,11 @@ static void spi_nor_init_params_deprecated(struct spi_nor *nor)
 
 	spi_nor_manufacturer_init_params(nor);
 
-	if (nor->info->no_sfdp_flags & (SPI_NOR_DUAL_READ |
-					SPI_NOR_QUAD_READ |
-					SPI_NOR_OCTAL_READ |
-					SPI_NOR_OCTAL_DTR_READ))
+	if ((nor->info->no_sfdp_flags & (SPI_NOR_DUAL_READ |
+					 SPI_NOR_QUAD_READ |
+					 SPI_NOR_OCTAL_READ |
+					 SPI_NOR_OCTAL_DTR_READ)) ||
+	     nor->manufacturer->flags & SPI_NOR_MANUFACT_TRY_SFDP)
 		spi_nor_sfdp_init_params_deprecated(nor);
 }
 
@@ -2911,7 +2912,32 @@ static void spi_nor_init_default_params(struct spi_nor *nor)
 	struct device_node *np = spi_nor_get_flash_node(nor);
 
 	params->quad_enable = spi_nor_sr2_bit1_quad_enable;
-	params->otp.org = info->otp;
+	memset(&params->otp.org, 0, sizeof(struct spi_nor_otp_organization));
+	if (info->otp) {
+		memcpy(&params->otp.org, info->otp, sizeof(struct spi_nor_otp_organization));
+	} else if (nor->manufacturer->flags & SPI_NOR_MANUFACT_DT_OTP) {
+		/* Check for OTP information on device tree */
+		u32 n_regions, len;
+
+		if (!of_property_read_u32(np, "otp-n-regions", &n_regions) &&
+		   n_regions > 0 &&
+		   !of_property_read_u32(np, "otp-len", &len) &&
+		   len > 0) {
+			u32 base, offset = 0;
+
+			if (n_regions > 1) {
+				/* If offset is not defined use length as offset */
+				if (of_property_read_u32(np, "otp-offset", &offset))
+					offset = len;
+			}
+			if (of_property_read_u32(np, "otp-base", &base))
+				base = 0;
+			params->otp.org.n_regions = n_regions;
+			params->otp.org.offset = offset;
+			params->otp.org.base = base;
+			params->otp.org.len = len;
+		}
+	}
 
 	/* Default to 16-bit Write Status (01h) Command */
 	nor->flags |= SNOR_F_HAS_16BIT_SR;
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 1516b6d0dc37..c862e42c844f 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -326,7 +326,7 @@ struct spi_nor_otp_ops {
  * @ops:	OTP access ops
  */
 struct spi_nor_otp {
-	const struct spi_nor_otp_organization *org;
+	struct spi_nor_otp_organization org;
 	const struct spi_nor_otp_ops *ops;
 };
 
@@ -560,12 +560,17 @@ struct flash_info {
  * @parts: array of parts supported by this manufacturer
  * @nparts: number of entries in the parts array
  * @fixups: hooks called at various points in time during spi_nor_scan()
+ * @flags: manufacturer flags
  */
 struct spi_nor_manufacturer {
 	const char *name;
 	const struct flash_info *parts;
 	unsigned int nparts;
 	const struct spi_nor_fixups *fixups;
+
+	u8 flags;
+#define SPI_NOR_MANUFACT_TRY_SFDP	BIT(0)
+#define SPI_NOR_MANUFACT_DT_OTP		BIT(0)
 };
 
 /**
diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
index 9a729aa3452d..ffb7ffeb9030 100644
--- a/drivers/mtd/spi-nor/otp.c
+++ b/drivers/mtd/spi-nor/otp.c
@@ -11,8 +11,8 @@
 
 #include "core.h"
 
-#define spi_nor_otp_region_len(nor) ((nor)->params->otp.org->len)
-#define spi_nor_otp_n_regions(nor) ((nor)->params->otp.org->n_regions)
+#define spi_nor_otp_region_len(nor) ((nor)->params->otp.org.len)
+#define spi_nor_otp_n_regions(nor) ((nor)->params->otp.org.n_regions)
 
 /**
  * spi_nor_otp_read_secr() - read security register
@@ -222,7 +222,7 @@ int spi_nor_otp_is_locked_sr2(struct spi_nor *nor, unsigned int region)
 
 static loff_t spi_nor_otp_region_start(const struct spi_nor *nor, unsigned int region)
 {
-	const struct spi_nor_otp_organization *org = nor->params->otp.org;
+	const struct spi_nor_otp_organization *org = &nor->params->otp.org;
 
 	return org->base + region * org->offset;
 }
diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
index 9f7ce5763e71..b60df00e43b9 100644
--- a/drivers/mtd/spi-nor/winbond.c
+++ b/drivers/mtd/spi-nor/winbond.c
@@ -333,7 +333,7 @@ static int winbond_nor_late_init(struct spi_nor *nor)
 {
 	struct spi_nor_flash_parameter *params = nor->params;
 
-	if (params->otp.org)
+	if (params->otp.org.n_regions)
 		params->otp.ops = &winbond_nor_otp_ops;
 
 	/*
-- 
2.39.5

Re: [PATCH v5 1/5] mtd: spi-nor: core: add manufacturer flags

[Tudor Ambarus]

Hi,

On 9/20/24 7:12 PM, Erez Geva wrote:
> From: Erez Geva <ErezGeva2@gmail.com>
> 
> Add flag for always trying reading SFDP:
> Some vendors reuse all JEDEC IDs on manufacture table
>  with new chips that support SFDP.
> 
> Add flag for reading OTP parameters from device tree.
> Some vendors reuse JEDEC IDs
>  with several chips with different OTP parameters.
> Alternatively we read parameters from SFDP.
> But the OTP parameters are absent from the SFDP.

Do you have some specific flashes that you try to identify? Why can't
they be differentiated at runtime?

> So there is not other way but to add the OTP parameters in the device tree.
> 

If there isn't any way to distinguish the flashes at runtime (which I
doubt/challenge btw), then as a last resort we introduce a dedicated
compatible for the flash in cause and specify all needed parameters in a
dedicated flash entry. This shall be more generic as further flash
parameters can be statically specified in the dedicated flash entry,
less invasive for dt, and less confusing for people when they decide
whether to use OTP or not. OTP params in device tree is a no-go.

But again, you have to prove why you can't distinguish the flash at
runtime before introducing a new flash compatible. So don't go this path
before sharing with us what you're trying to achieve.

Cheers,
ta

Re: [PATCH v5 1/5] mtd: spi-nor: core: add manufacturer flags

[Erez]

On Mon, 23 Sept 2024 at 08:04, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Hi,
>
> On 9/20/24 7:12 PM, Erez Geva wrote:
> > From: Erez Geva <ErezGeva2@gmail.com>
> >
> > Add flag for always trying reading SFDP:
> > Some vendors reuse all JEDEC IDs on manufacture table
> >  with new chips that support SFDP.
> >
> > Add flag for reading OTP parameters from device tree.
> > Some vendors reuse JEDEC IDs
> >  with several chips with different OTP parameters.
> > Alternatively we read parameters from SFDP.
> > But the OTP parameters are absent from the SFDP.
>
> Do you have some specific flashes that you try to identify? Why can't
> they be differentiated at runtime?

You can not figure OTP parameters based on  JEDEC ID and SFDP existence.
I did send a few examples.

One of them:
"How?

When using mx25l12805d, we do not read SFDP.
As it uses the no-SFDP flags.
When using mx25l12833f hardware with mx25l12805d driver, it did not
try to read the SFDP.
Yet mx25l12833f does have SFDP, when I remove the no-SFDP flags, the
driver fetch the SFDP.

Secondly SFDP does not contain OTP information.

mx25l12805d has two OTP regions of 128 KiB and 384 KiB (yes asymmetric).
While mx25l12833f has two OTP regions of 512 KiB.

How do we handle it?
I would gladly remove the obsolete mx25l12805d.
And skp compatibles all together."


>
> > So there is not other way but to add the OTP parameters in the device tree.
> >
>
> If there isn't any way to distinguish the flashes at runtime (which I
> doubt/challenge btw), then as a last resort we introduce a dedicated
> compatible for the flash in cause and specify all needed parameters in a
> dedicated flash entry. This shall be more generic as further flash
> parameters can be statically specified in the dedicated flash entry,
> less invasive for dt, and less confusing for people when they decide
> whether to use OTP or not. OTP params in device tree is a no-go.
>
> But again, you have to prove why you can't distinguish the flash at
> runtime before introducing a new flash compatible. So don't go this path
> before sharing with us what you're trying to achieve.

You keep sending me contradictory messages.

I told you we can not "guess" OTP settings based on JEDEC ID and SFDP existence.
It may be partial and Macronix may add new chips in the future.
They reuse JEDEC ID only retaining flash size and blocks.
This is why compatibilities work with new Macronix chips . Although by
reading the SFDP, we can use higher speeds.
We can use SFDP parameters to read  flash size, blocks and speed.
But it does not contain any OTP parameters.
I found only one Macronix chip with an enterprise SFDP table with a
boolean flag for OTP, this does not help us much.
Macronix technical support was explicit on OTP settings. You can not
deduce them. You must know what chip you use.
As far as I can see, Macronix does not reuse module names (god thanks for that).

I do not mind using flash compatible.
Just clarify that point.
And I will send the patches accordingly.

Thanks
Erez

>
> Cheers,
> ta

Re: [PATCH v5 1/5] mtd: spi-nor: core: add manufacturer flags

[Michael Walle]

Hi,

> I would gladly remove the obsolete mx25l12805d.

Why? I don't see any need for that.

> > If there isn't any way to distinguish the flashes at runtime (which I
> > doubt/challenge btw), then as a last resort we introduce a dedicated
> > compatible for the flash in cause and specify all needed parameters in a
> > dedicated flash entry. This shall be more generic as further flash
> > parameters can be statically specified in the dedicated flash entry,
> > less invasive for dt, and less confusing for people when they decide
> > whether to use OTP or not. OTP params in device tree is a no-go.
> >
> > But again, you have to prove why you can't distinguish the flash at
> > runtime before introducing a new flash compatible. So don't go this path
> > before sharing with us what you're trying to achieve.
>
> You keep sending me contradictory messages.
>
> I told you we can not "guess" OTP settings based on JEDEC ID and
> SFDP existence.

What are you trying to achieve here? I've told you we are trying
hard to figure out everything out at runtime. I'd suggest you start
with one particular device where you want OTP support for. If the
flash id is already in our database, find a way to distinguish
between the old and the new one; probably by looking at some SFDP
parameters. No need for any new compatible. Don't try to solve the
problem for all the chips out there.

Again, the reason why we are trying hard to determine that at
runtime is that these flashes are usually second source devices and
a manufacturer might just replace it with a (more or less)
compatible one. Therefore, the less information we put into the
devicetree the better. So before you are sending a new version with
the flash compatibles, you actually have to convince us that there
is no other way of knowing what kind of flash there is on your
board except for providing the name by the firmware.

-michael

Re: [PATCH v5 1/5] mtd: spi-nor: core: add manufacturer flags

[Erez]

On Mon, 23 Sept 2024 at 12:46, Michael Walle <mwalle@kernel.org> wrote:
>
> Hi,
>
> > I would gladly remove the obsolete mx25l12805d.
>
> Why? I don't see any need for that.

Maybe because we do not want compatibility table?

>
> > > If there isn't any way to distinguish the flashes at runtime (which I
> > > doubt/challenge btw), then as a last resort we introduce a dedicated
> > > compatible for the flash in cause and specify all needed parameters in a
> > > dedicated flash entry. This shall be more generic as further flash
> > > parameters can be statically specified in the dedicated flash entry,
> > > less invasive for dt, and less confusing for people when they decide
> > > whether to use OTP or not. OTP params in device tree is a no-go.
> > >
> > > But again, you have to prove why you can't distinguish the flash at
> > > runtime before introducing a new flash compatible. So don't go this path
> > > before sharing with us what you're trying to achieve.
> >
> > You keep sending me contradictory messages.
> >
> > I told you we can not "guess" OTP settings based on JEDEC ID and
> > SFDP existence.
>
> What are you trying to achieve here? I've told you we are trying
> hard to figure out everything out at runtime. I'd suggest you start
> with one particular device where you want OTP support for. If the
> flash id is already in our database, find a way to distinguish
> between the old and the new one; probably by looking at some SFDP
> parameters. No need for any new compatibility. Don't try to solve the
> problem for all the chips out there.

I start with "Add support for SPI-NOR Macronix OTP".
With one flash chip and move to another one chip.
I never suggested adding multiple.

Yet, after some research I find that all Macronix chips in the last 15
years have SFDP.
So I added a second patch to always read the SFDP of Macronix chips.
Perhaps I should send it separately,as it seems to confuse you.

And no, I do not try to support all chips, just to remove the
restriction that if
Macronix reuses an old chip JEDEC ID, we skip the SFDP of the new
chip, because we have the old chip in the compatibility table,
although there are two distinct chips. They use different model names.
There is no reason to differentiate chips in that way, at least not
with Macronix chips.

>
> Again, the reason why we are trying hard to determine that at
> runtime is that these flashes are usually second source devices and
> a manufacturer might just replace it with a (more or less)
> compatible one. Therefore, the less information we put into the
> devicetree the better. So before you are sending a new version with
> the flash compatibles, you actually have to convince us that there
> is no other way of knowing what kind of flash there is on your
> board except for providing the name by the firmware.

No, reading the SFDP is great.
Except for OTP parameters/configuration.
As there is not way to find OTP parameters using JEDEC ID/SFDP
So as I understand there are only 2 ways to set the OTP parameters:
* Use a compatibility.
* Use dynamic OTP configuration, through DT, sysfs,

Erez


>
> -michael

Re: [PATCH v5 1/5] mtd: spi-nor: core: add manufacturer flags

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 829 bytes --]

Hi,

> > > I would gladly remove the obsolete mx25l12805d.
> > Why? I don't see any need for that.
> Maybe because we do not want compatibility table?

I don't get this? Anyway, we do not remove support for older
flashes for no reason.

> No, reading the SFDP is great.
> Except for OTP parameters/configuration.
> As there is not way to find OTP parameters using JEDEC ID/SFDP
> So as I understand there are only 2 ways to set the OTP parameters:
> * Use a compatibility.
> * Use dynamic OTP configuration, through DT, sysfs,

* Use the in-kernel database to look up the parameters as it is done
  with any other flash device. If the id is reused, look for
  differences in the SFDP to figure out the correct flash device,
  then add some .fixups to manually add the OTP flags and
  parameters.

-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

Re: [PATCH v5 1/5] mtd: spi-nor: core: add manufacturer flags

[Erez]

On Mon, 23 Sept 2024 at 18:19, Michael Walle <mwalle@kernel.org> wrote:
>
> Hi,
>
> > > > I would gladly remove the obsolete mx25l12805d.
> > > Why? I don't see any need for that.
> > Maybe because we do not want compatibility table?
>
> I don't get this? Anyway, we do not remove support for older
> flashes for no reason.

I did not insist, you asked.
Macronix stopped selling these chips 15 year ago.
How long do you want to support old chips?

>
> > No, reading the SFDP is great.
> > Except for OTP parameters/configuration.
> > As there is not way to find OTP parameters using JEDEC ID/SFDP
> > So as I understand there are only 2 ways to set the OTP parameters:
> > * Use a compatibility.
> > * Use dynamic OTP configuration, through DT, sysfs,
>
> * Use the in-kernel database to look up the parameters as it is done
>   with any other flash device. If the id is reused, look for

All IDs in table are reused.
The change I suggest is to read SFDP to all Macronix chips.
As today, we skip the RDSFDP. "to avoid no-op".

>   differences in the SFDP to figure out the correct flash device,

You can not do that unless you actually read the SFDP.

>   then add some .fixups to manually add the OTP flags and
>   parameters.

I am in favour.
As we can not use JEDEC ID or SFDP for OTP parameters.
How do you suggest manually OTP parameters?
Tudor Ambarus rejected my proposal.
I am open to implementing your suggestion.

I will split the two patches to avoid confusion.
So I will submit 2 patches:
* Always read Macronix chips SFDP, as Macronix replaced all old chips
in the Manufacture table.
* Support Macronix OTP.

Thanks for the feedback
Erez



>
> -michael

Re: [PATCH v5 1/5] mtd: spi-nor: core: add manufacturer flags

[Tudor Ambarus]

On 9/23/24 5:31 PM, Erez wrote:
> On Mon, 23 Sept 2024 at 18:19, Michael Walle <mwalle@kernel.org> wrote:
>>
>> Hi,
>>
>>>>> I would gladly remove the obsolete mx25l12805d.
>>>> Why? I don't see any need for that.
>>> Maybe because we do not want compatibility table?
>>
>> I don't get this? Anyway, we do not remove support for older
>> flashes for no reason.
> 
> I did not insist, you asked.
> Macronix stopped selling these chips 15 year ago.
> How long do you want to support old chips?
> 

as long as there's at least a user in the kernel.
>>
>>> No, reading the SFDP is great.
>>> Except for OTP parameters/configuration.
>>> As there is not way to find OTP parameters using JEDEC ID/SFDP
>>> So as I understand there are only 2 ways to set the OTP parameters:
>>> * Use a compatibility.
>>> * Use dynamic OTP configuration, through DT, sysfs,
>>
>> * Use the in-kernel database to look up the parameters as it is done
>>   with any other flash device. If the id is reused, look for
> 
> All IDs in table are reused.
> The change I suggest is to read SFDP to all Macronix chips.
> As today, we skip the RDSFDP. "to avoid no-op".
> 
>>   differences in the SFDP to figure out the correct flash device,
> 
> You can not do that unless you actually read the SFDP.
> 
>>   then add some .fixups to manually add the OTP flags and
>>   parameters.
> 
> I am in favour.
> As we can not use JEDEC ID or SFDP for OTP parameters.
> How do you suggest manually OTP parameters?

Michael literally just said how to do it. Here it is again:
```
* Use the in-kernel database to look up the parameters as it is done
  with any other flash device. If the id is reused, look for
  differences in the SFDP to figure out the correct flash device,
  then add some .fixups to manually add the OTP flags and
  parameters.
```
> Tudor Ambarus rejected my proposal.
> I am open to implementing your suggestion.
> 
> I will split the two patches to avoid confusion.
> So I will submit 2 patches:
> * Always read Macronix chips SFDP, as Macronix replaced all old chips
> in the Manufacture table.

I'll NACK it unless you prove that the old flavors of flashes are not
used anymore in the kernel.
> * Support Macronix OTP.

Hopefully this time you describe your problem in the commit message and
convince us it is worth fixing and that it makes sense for us to read
past the first paragraph.

Re: [PATCH v5 1/5] mtd: spi-nor: core: add manufacturer flags

[Tudor Ambarus]

On 9/23/24 7:04 PM, Tudor Ambarus wrote:
>> * Always read Macronix chips SFDP, as Macronix replaced all old chips
>> in the Manufacture table.
> I'll NACK it unless you prove that the old flavors of flashes are not
> used anymore in the kernel.

Even if you can prove that the older flashes are not used in the kernel
anymore, we can't just switch to parsing SFDP, because we have seen in
the past flashes with wrong SFDP data that made the flashes misbehave.
The recommended way is to update just the flashes that you can test.

Re: [PATCH v5 1/5] mtd: spi-nor: core: add manufacturer flags

[Esben Haabendal]

Erez <erezgeva2@gmail.com> writes:

> On Mon, 23 Sept 2024 at 18:19, Michael Walle <mwalle@kernel.org> wrote:
>>
>> > > > I would gladly remove the obsolete mx25l12805d.
>> > > Why? I don't see any need for that.
>> > Maybe because we do not want compatibility table?
>>
>> I don't get this? Anyway, we do not remove support for older
>> flashes for no reason.
>
> I did not insist, you asked.
> Macronix stopped selling these chips 15 year ago.
> How long do you want to support old chips?

It is not unusual for embedded products to have a support span of more
than 20 years. And chips such as these flashes might not be entirely new
when the product is introduced. So dropping support for SPI-NOR flashes
that are newer than 25-30 years is definitely a risk. Somebody out there
might not be able to upgrade to latest kernel versions anymore, which is
not a position we should put anyone in. With the increasing pressure to
upgrade product for better security, we definitely should not make it
more difficult to run newer kernel versions than absolutely necessary.

/Esben

Re: [PATCH v5 1/5] mtd: spi-nor: core: add manufacturer flags

[Erez]

On Thu, 26 Sept 2024 at 09:46, Esben Haabendal <esben@geanix.com> wrote:
>
> Erez <erezgeva2@gmail.com> writes:
>
> > On Mon, 23 Sept 2024 at 18:19, Michael Walle <mwalle@kernel.org> wrote:
> >>
> >> > > > I would gladly remove the obsolete mx25l12805d.
> >> > > Why? I don't see any need for that.
> >> > Maybe because we do not want compatibility table?
> >>
> >> I don't get this? Anyway, we do not remove support for older
> >> flashes for no reason.
> >
> > I did not insist, you asked.
> > Macronix stopped selling these chips 15 year ago.
> > How long do you want to support old chips?
>
> It is not unusual for embedded products to have a support span of more
> than 20 years. And chips such as these flashes might not be entirely new
> when the product is introduced. So dropping support for SPI-NOR flashes
> that are newer than 25-30 years is definitely a risk. Somebody out there
> might not be able to upgrade to latest kernel versions anymore, which is
> not a position we should put anyone in. With the increasing pressure to
> upgrade product for better security, we definitely should not make it
> more difficult to run newer kernel versions than absolutely necessary.

I do not insist. Nor send any patch in this direction.

Each project can define the extent of backward compatibility.
In terms of compilers, linkers and tools, i.e. build environment.
In terms of standards like the C standard we use.
In terms of network protocols.
And also what Hardware do we support.

There is no harm in asking where the boundaries are.
All projects move their boundaries all the time.
The Linux kernel is no exception.

Erez

>
> /Esben

Re: [PATCH v5 1/5] mtd: spi-nor: core: add manufacturer flags

[Esben Haabendal]

Erez <erezgeva2@gmail.com> writes:

> On Thu, 26 Sept 2024 at 09:46, Esben Haabendal <esben@geanix.com> wrote:
>>
>> Erez <erezgeva2@gmail.com> writes:
>>
>> > On Mon, 23 Sept 2024 at 18:19, Michael Walle <mwalle@kernel.org> wrote:
>> >>
>> >> > > > I would gladly remove the obsolete mx25l12805d.
>> >> > > Why? I don't see any need for that.
>> >> > Maybe because we do not want compatibility table?
>> >>
>> >> I don't get this? Anyway, we do not remove support for older
>> >> flashes for no reason.
>> >
>> > I did not insist, you asked.
>> > Macronix stopped selling these chips 15 year ago.
>> > How long do you want to support old chips?
>>
>> It is not unusual for embedded products to have a support span of more
>> than 20 years. And chips such as these flashes might not be entirely new
>> when the product is introduced. So dropping support for SPI-NOR flashes
>> that are newer than 25-30 years is definitely a risk. Somebody out there
>> might not be able to upgrade to latest kernel versions anymore, which is
>> not a position we should put anyone in. With the increasing pressure to
>> upgrade product for better security, we definitely should not make it
>> more difficult to run newer kernel versions than absolutely necessary.
>
> I do not insist. Nor send any patch in this direction.

I did not say or imply that you did any such thing.

You asked an open question, and I gave my response. Nothing more,
nothing less.

> Each project can define the extent of backward compatibility.
> In terms of compilers, linkers and tools, i.e. build environment.
> In terms of standards like the C standard we use.
> In terms of network protocols.
> And also what Hardware do we support.
>
> There is no harm in asking where the boundaries are.
> All projects move their boundaries all the time.
> The Linux kernel is no exception.

Re: [PATCH v5 1/5] mtd: spi-nor: core: add manufacturer flags

[Erez]

On Thu, 26 Sept 2024 at 13:37, Esben Haabendal <esben@geanix.com> wrote:
>
> Erez <erezgeva2@gmail.com> writes:
>
> > On Thu, 26 Sept 2024 at 09:46, Esben Haabendal <esben@geanix.com> wrote:
> >>
> >> Erez <erezgeva2@gmail.com> writes:
> >>
> >> > On Mon, 23 Sept 2024 at 18:19, Michael Walle <mwalle@kernel.org> wrote:
> >> >>
> >> >> > > > I would gladly remove the obsolete mx25l12805d.
> >> >> > > Why? I don't see any need for that.
> >> >> > Maybe because we do not want compatibility table?
> >> >>
> >> >> I don't get this? Anyway, we do not remove support for older
> >> >> flashes for no reason.
> >> >
> >> > I did not insist, you asked.
> >> > Macronix stopped selling these chips 15 year ago.
> >> > How long do you want to support old chips?
> >>
> >> It is not unusual for embedded products to have a support span of more
> >> than 20 years. And chips such as these flashes might not be entirely new
> >> when the product is introduced. So dropping support for SPI-NOR flashes
> >> that are newer than 25-30 years is definitely a risk. Somebody out there
> >> might not be able to upgrade to latest kernel versions anymore, which is
> >> not a position we should put anyone in. With the increasing pressure to
> >> upgrade product for better security, we definitely should not make it
> >> more difficult to run newer kernel versions than absolutely necessary.
> >
> > I do not insist. Nor send any patch in this direction.
>
> I did not say or imply that you did any such thing.
>
> You asked an open question, and I gave my response. Nothing more,
> nothing less.

+

>
> > Each project can define the extent of backward compatibility.
> > In terms of compilers, linkers and tools, i.e. build environment.
> > In terms of standards like the C standard we use.
> > In terms of network protocols.
> > And also what Hardware do we support.
> >
> > There is no harm in asking where the boundaries are.
> > All projects move their boundaries all the time.
> > The Linux kernel is no exception.

Re: [PATCH v5 1/5] mtd: spi-nor: core: add manufacturer flags

[Tudor Ambarus]

On 9/23/24 11:31 AM, Erez wrote:
> On Mon, 23 Sept 2024 at 08:04, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>> Hi,
>>

Hi!

>> On 9/20/24 7:12 PM, Erez Geva wrote:
>>> From: Erez Geva <ErezGeva2@gmail.com>
>>>
>>> Add flag for always trying reading SFDP:
>>> Some vendors reuse all JEDEC IDs on manufacture table
>>>  with new chips that support SFDP.
>>>
>>> Add flag for reading OTP parameters from device tree.
>>> Some vendors reuse JEDEC IDs
>>>  with several chips with different OTP parameters.
>>> Alternatively we read parameters from SFDP.
>>> But the OTP parameters are absent from the SFDP.
>>
>> Do you have some specific flashes that you try to identify? Why can't
>> they be differentiated at runtime?
> 
> You can not figure OTP parameters based on  JEDEC ID and SFDP existence.
> I did send a few examples.
> 
> One of them:
> "How?
> 
> When using mx25l12805d, we do not read SFDP.
> As it uses the no-SFDP flags.
> When using mx25l12833f hardware with mx25l12805d driver, it did not
> try to read the SFDP.
> Yet mx25l12833f does have SFDP, when I remove the no-SFDP flags, the
> driver fetch the SFDP.
> 
> Secondly SFDP does not contain OTP information.
> 
> mx25l12805d has two OTP regions of 128 KiB and 384 KiB (yes asymmetric).
> While mx25l12833f has two OTP regions of 512 KiB.

Ok, so you want to add support for mx25l12833f which shares the same ID
as mx25l12805d and has different OTP settings. Is that correct?

Which flash do you have at hand, both, none, just one of them?
> 
> How do we handle it?
> I would gladly remove the obsolete mx25l12805d.
> And skp compatibles all together."

I need to understand first what you're trying to do. Don't assume that I
remember what we discussed one month ago. Describe the why in the commit
message.
> 
> 
>>
>>> So there is not other way but to add the OTP parameters in the device tree.
>>>
>>
>> If there isn't any way to distinguish the flashes at runtime (which I
>> doubt/challenge btw), then as a last resort we introduce a dedicated
>> compatible for the flash in cause and specify all needed parameters in a
>> dedicated flash entry. This shall be more generic as further flash
>> parameters can be statically specified in the dedicated flash entry,
>> less invasive for dt, and less confusing for people when they decide
>> whether to use OTP or not. OTP params in device tree is a no-go.
>>
>> But again, you have to prove why you can't distinguish the flash at
>> runtime before introducing a new flash compatible. So don't go this path
>> before sharing with us what you're trying to achieve.
> 
> You keep sending me contradictory messages.

when? Please accept my apologies if that's the case, it's not in my
intention. Provide better commit message, help me help you.

> 
> I told you we can not "guess" OTP settings based on JEDEC ID and SFDP existence.

When? And more importantly, why?

> It may be partial and Macronix may add new chips in the future.

I don't understand what you mean by partial, please elaborate.

And we don't add support for what we assume new chips will look like.

> They reuse JEDEC ID only retaining flash size and blocks.

Yes, I know macronix shares flash IDs among flavors of flashes or new
chips altogether.

> This is why compatibilities work with new Macronix chips . Although by

In the last 7 years we haven't add any new compatible for SPI NOR, I
don't understand what are you referring to.

> reading the SFDP, we can use higher speeds.

I don't see what's your point with this sentence.

> We can use SFDP parameters to read  flash size, blocks and speed.
> But it does not contain any OTP parameters.
> I found only one Macronix chip with an enterprise SFDP table with a
> boolean flag for OTP, this does not help us much.

So you say that there's a specific vendor SFDP table that contains a bit
indicating whether OTP is supported or not? Use that then.

> Macronix technical support was explicit on OTP settings. You can not

Provide us the answer for your specific flash. I don't care yet about
all their flashes.

> deduce them. You must know what chip you use.

And I think I already said that you can differentiate between the two
based on SFDP presence. mx25l12833f has SFDP, thus when SFDP present use
the mx25l12833f-OTP configuration. When SFDP is not presence one may add
support for the mx25l12805d-OTP configuration.

Is there any case that I miss?

> As far as I can see, Macronix does not reuse module names (god thanks for that).
> 
> I do not mind using flash compatible.
> Just clarify that point.
> And I will send the patches accordingly.
> 

I'm afraid I haven't understood yet what you're trying to achieve to
provide some guidance.

Re: [PATCH v5 1/5] mtd: spi-nor: core: add manufacturer flags

[Erez]

On Mon, 23 Sept 2024 at 14:07, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
>
>
> On 9/23/24 11:31 AM, Erez wrote:
> > On Mon, 23 Sept 2024 at 08:04, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> >>
> >> Hi,
> >>
>
> Hi!
>
> >> On 9/20/24 7:12 PM, Erez Geva wrote:
> >>> From: Erez Geva <ErezGeva2@gmail.com>
> >>>
> >>> Add flag for always trying reading SFDP:
> >>> Some vendors reuse all JEDEC IDs on manufacture table
> >>>  with new chips that support SFDP.
> >>>
> >>> Add flag for reading OTP parameters from device tree.
> >>> Some vendors reuse JEDEC IDs
> >>>  with several chips with different OTP parameters.
> >>> Alternatively we read parameters from SFDP.
> >>> But the OTP parameters are absent from the SFDP.
> >>
> >> Do you have some specific flashes that you try to identify? Why can't
> >> they be differentiated at runtime?
> >
> > You can not figure OTP parameters based on  JEDEC ID and SFDP existence.
> > I did send a few examples.
> >
> > One of them:
> > "How?
> >
> > When using mx25l12805d, we do not read SFDP.
> > As it uses the no-SFDP flags.
> > When using mx25l12833f hardware with mx25l12805d driver, it did not
> > try to read the SFDP.
> > Yet mx25l12833f does have SFDP, when I remove the no-SFDP flags, the
> > driver fetch the SFDP.
> >
> > Secondly SFDP does not contain OTP information.
> >
> > mx25l12805d has two OTP regions of 128 KiB and 384 KiB (yes asymmetric).
> > While mx25l12833f has two OTP regions of 512 KiB.
>
> Ok, so you want to add support for mx25l12833f which shares the same ID
> as mx25l12805d and has different OTP settings. Is that correct?

My patch purpose was initially adding Mactronix OTP.
After reading a lot of Mactronix datasheets, I suggest also reading
all SFDP to all  Mactronix chips.

I add compatibility as I follow Kernel rule, that new code must be used!

>
> Which flash do you have at hand, both, none, just one of them?

When I started working on the OTP code, I used MX25L12833F.
But later I left the company.
So I use my beaglebone black and connect it to a MX25L3233F.

> >
> > How do we handle it?
> > I would gladly remove the obsolete mx25l12805d.
> > And skp compatibles all together."
>
> I need to understand first what you're trying to do. Don't assume that I
> remember what we discussed one month ago. Describe the why in the commit
> message.

"Add support for SPI-NOR Macronix OTP."
I wrote in the cover letter.
No, I do not expect you to remember everything.
I did write my intention in the cover letter.



> >
> >
> >>
> >>> So there is not other way but to add the OTP parameters in the device tree.
> >>>
> >>
> >> If there isn't any way to distinguish the flashes at runtime (which I
> >> doubt/challenge btw), then as a last resort we introduce a dedicated
> >> compatible for the flash in cause and specify all needed parameters in a
> >> dedicated flash entry. This shall be more generic as further flash
> >> parameters can be statically specified in the dedicated flash entry,
> >> less invasive for dt, and less confusing for people when they decide
> >> whether to use OTP or not. OTP params in device tree is a no-go.
> >>
> >> But again, you have to prove why you can't distinguish the flash at
> >> runtime before introducing a new flash compatible. So don't go this path
> >> before sharing with us what you're trying to achieve.
> >
> > You keep sending me contradictory messages.
>
> when? Please accept my apologies if that's the case, it's not in my
> intention. Provide better commit message, help me help you.

I tried adding a new compatibility.
You say you do not want new compatibility.
You ask if it is possible to deduce it from JEDEC ID and SFDP,
I answer that this is not possible, at least in some cases..
I try to add OTP parameters in DT. You do not like it, fair enough.
What am I to do?
Seems like a dead end.

Erez


>
> >
> > I told you we can not "guess" OTP settings based on JEDEC ID and SFDP existence.
>
> When? And more importantly, why?

I send you example of 3/4 chips that using JEDEC ID and SFDP existence
is not enough.

>
> > It may be partial and Macronix may add new chips in the future.
>
> I don't understand what you mean by partial, please elaborate.

I think Kernel like using bullet proof methods.
methods that will produce a working results.
If I find one example we can not probe the OTP parameters this way, it
means this method is NOT bullet proof.

>
> And we don't add support for what we assume new chips will look like.

This is not what I ask for.
Just trying to guess OTP parameters in current chips will break with new chips.


>
> > They reuse JEDEC ID only retaining flash size and blocks.
>
> Yes, I know macronix shares flash IDs among flavors of flashes or new
> chips altogether.

I am glad you know.

>
> > This is why compatibilities work with new Macronix chips . Although by
>
> In the last 7 years we haven't add any new compatible for SPI NOR, I
> don't understand what are you referring to.

The fact we can not deduce OTP parameters with current methods.
I do not mind in what way we do it.
But it seems there is no way according to your suggestions.

>
> > reading the SFDP, we can use higher speeds.
>
> I don't see what's your point with this sentence.

SFDP specifies different methods of reading and writing. Double, block.
But it does not contain any information on OTP.


>
> > We can use SFDP parameters to read  flash size, blocks and speed.
> > But it does not contain any OTP parameters.
> > I found only one Macronix chip with an enterprise SFDP table with a
> > boolean flag for OTP, this does not help us much.
>
> So you say that there's a specific vendor SFDP table that contains a bit
> indicating whether OTP is supported or not? Use that then.

I found only one chip like that.
All the others do not have it.
Nor any information on OTP size and number of regions.
So, no it does not help much.
I do not hold any information.

>
> > Macronix technical support was explicit on OTP settings. You can not
>
> Provide us the answer for your specific flash. I don't care yet about
> all their flashes.

There are two parts:

SFDP - all Mactronix chips whether they are in the compatibilities or
not, have SFDP.
 There is no logic to skip SFDP just because we have an old chip entry
in the compatibilities table.

OTP - I want to add only one chip with OTP, the one I test.
 But I can not rely on JEDEC ID or SFDP to deduce the OTP parameters.
 So as I understand we have two options:Add compatibility for this
chip, or add dynamic OTP parameters (in DT or sysfs).


>
> > deduce them. You must know what chip you use.
>
> And I think I already said that you can differentiate between the two
> based on SFDP presence. mx25l12833f has SFDP, thus when SFDP present use
> the mx25l12833f-OTP configuration. When SFDP is not presence one may add
> support for the mx25l12805d-OTP configuration.

No, we have 3 chips.
2 are using the same JEDEC ID and both using SFDP, yet they use a different OTP.
So, the problem is here, and probably be bigger in the future, despite
you "do not care".


>
> Is there any case that I miss?

According to your reply, I would say pretty much most.

>
> > As far as I can see, Macronix does not reuse module names (god thanks for that).
> >
> > I do not mind using flash compatible.
> > Just clarify that point.
> > And I will send the patches accordingly.
> >
>
> I'm afraid I haven't understood yet what you're trying to achieve to
> provide some guidance.

I do not understand you either.
But I try to work on my communication skills.

Thank you for your feedback

Erez

Re: [PATCH v5 1/5] mtd: spi-nor: core: add manufacturer flags

[Tudor Ambarus]

On 9/23/24 2:01 PM, Erez wrote:

cut

>>>
>>> mx25l12805d has two OTP regions of 128 KiB and 384 KiB (yes asymmetric).
>>> While mx25l12833f has two OTP regions of 512 KiB.
>>
>> Ok, so you want to add support for mx25l12833f which shares the same ID
>> as mx25l12805d and has different OTP settings. Is that correct?
> 
> My patch purpose was initially adding Mactronix OTP.

support needs to be added per flash, not per manufacturer.

> After reading a lot of Mactronix datasheets, I suggest also reading
> all SFDP to all  Mactronix chips.

Why? It seems too invasive. Generally it is not recommended to issue
unsupported commands to flashes. Change just the flash that you use and
can test.

cut

>> Which flash do you have at hand, both, none, just one of them?
> 
> When I started working on the OTP code, I used MX25L12833F.
> But later I left the company.
> So I use my beaglebone black and connect it to a MX25L3233F.

I understand mx25l12805d and mx25l12833f share the same ID. How is
mx25l3233f related, does it share the same ID as the previous two?

> 
>>>
>>> How do we handle it?
>>> I would gladly remove the obsolete mx25l12805d.
>>> And skp compatibles all together."
>>
>> I need to understand first what you're trying to do. Don't assume that I
>> remember what we discussed one month ago. Describe the why in the commit
>> message.
> 
> "Add support for SPI-NOR Macronix OTP."
> I wrote in the cover letter.
> No, I do not expect you to remember everything.
> I did write my intention in the cover letter.
> 

I already read your cover letter and it didn't explain the challenges
that you're facing.

cut

>>>
>>> You keep sending me contradictory messages.
>>
>> when? Please accept my apologies if that's the case, it's not in my
>> intention. Provide better commit message, help me help you.
> 
> I tried adding a new compatibility.
> You say you do not want new compatibility.

I said new compatibility will be introduced as a last resort only if we
can't differentiate the flashes at run-time. You haven't proved me yet
that this is the case.

> You ask if it is possible to deduce it from JEDEC ID and SFDP,
> I answer that this is not possible, at least in some cases..

I'm interested just about your case, not all the possible cases.

> I try to add OTP parameters in DT. You do not like it, fair enough.
> What am I to do?
> Seems like a dead end.
> 

You can better explain what are the challenges you have and answer the
questions that we're asking ...
> Erez
> 
> 
>>
>>>
>>> I told you we can not "guess" OTP settings based on JEDEC ID and SFDP existence.
>>
>> When? And more importantly, why?
> 
> I send you example of 3/4 chips that using JEDEC ID and SFDP existence
> is not enough.

Why? Do they have the exact SFDP tables? Prove me, drop them all.
Any bit that differs in the SFDP tables is enough to differentiate the
flavors of flashes. Vendor tables included ;)
> 
>>
>>> It may be partial and Macronix may add new chips in the future.
>>
>> I don't understand what you mean by partial, please elaborate.
> 
> I think Kernel like using bullet proof methods.
> methods that will produce a working results.
> If I find one example we can not probe the OTP parameters this way, it
> means this method is NOT bullet proof.
> 
>>
>> And we don't add support for what we assume new chips will look like.
> 
> This is not what I ask for.
> Just trying to guess OTP parameters in current chips will break with new chips.
> 

Again, I'm interested just in the flashes that you use and can test. I'm
not interested in addressing theoretical problems.
> 
>>
>>> They reuse JEDEC ID only retaining flash size and blocks.
>>
>> Yes, I know macronix shares flash IDs among flavors of flashes or new
>> chips altogether.
> 
> I am glad you know.

I sense some rage and I find this inappropriate. I'm just trying to help.

cut

>>
>> And I think I already said that you can differentiate between the two
>> based on SFDP presence. mx25l12833f has SFDP, thus when SFDP present use
>> the mx25l12833f-OTP configuration. When SFDP is not presence one may add
>> support for the mx25l12805d-OTP configuration.
> 
> No, we have 3 chips.
> 2 are using the same JEDEC ID and both using SFDP, yet they use a different OTP.

Which ones are these?

I guess mx25l12805d is non-SFDP. Then mx25l12833f and mx25l3233f define
some SFDP tables. Do mx25l12833f and mx25l3233f have the same OTP
organization?

> So, the problem is here, and probably be bigger in the future, despite
> you "do not care".
> 

I hear about your problem just here, after 3 emails exchanged and at
least one hour wasted of my time.

I want to address problems one step at a time. We can address the bigger
theoretical problems in the future, if they'll ever become real.
> 
>>
>> Is there any case that I miss?
> 
> According to your reply, I would say pretty much most.

This is again inappropriate ... I will read your next email as well, but
I'm not going to tolerate such replies anymore.

Re: [PATCH v5 1/5] mtd: spi-nor: core: add manufacturer flags

[Erez]

On Mon, 23 Sept 2024 at 16:18, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
>
>
> On 9/23/24 2:01 PM, Erez wrote:
>
> cut
>
> >>>
> >>> mx25l12805d has two OTP regions of 128 KiB and 384 KiB (yes asymmetric).
> >>> While mx25l12833f has two OTP regions of 512 KiB.
> >>
> >> Ok, so you want to add support for mx25l12833f which shares the same ID
> >> as mx25l12805d and has different OTP settings. Is that correct?
> >
> > My patch purpose was initially adding Mactronix OTP.
>
> support needs to be added per flash, not per manufacturer.

The OTP code is per manufacturer.
Not my inventions, See drivers/mtd/spi-nor/winbond.c OTP callbacks.
My patch adds support after a single chip flash.


>
> > After reading a lot of Mactronix datasheets, I suggest also reading
> > all SFDP to all  Mactronix chips.
>
> Why? It seems too invasive. Generally it is not recommended to issue
> unsupported commands to flashes. Change just the flash that you use and
> can test.

It is not, All Mactronix chips in the last 15 years have SFDP.
The chips in the manufacturer compatibility table are all obsolete.
Mactronix reuse the JEDEC IDs. There is no point pretending these are
the same chips.



>
> cut
>
> >> Which flash do you have at hand, both, none, just one of them?
> >
> > When I started working on the OTP code, I used MX25L12833F.
> > But later I left the company.
> > So I use my beaglebone black and connect it to a MX25L3233F.
>
> I understand mx25l12805d and mx25l12833f share the same ID. How is
> mx25l3233f related, does it share the same ID as the previous two?

They are not. I just replaced the company hardware with a different one.
You ask me to report the hardware I use for testing.

The patch covers the one I use with beaglebone black.
I just mentioned the OTP callbacks are per manufacturer.
But if a new chip in the future would require different callbacks,
then just add them.
My patch is using a single chip, the one I send the testing with.
beaglebone black + MX25L3233F.

>
> >
> >>>
> >>> How do we handle it?
> >>> I would gladly remove the obsolete mx25l12805d.
> >>> And skp compatibles all together."
> >>
> >> I need to understand first what you're trying to do. Don't assume that I
> >> remember what we discussed one month ago. Describe the why in the commit
> >> message.
> >
> > "Add support for SPI-NOR Macronix OTP."
> > I wrote in the cover letter.
> > No, I do not expect you to remember everything.
> > I did write my intention in the cover letter.
> >
>
> I already read your cover letter and it didn't explain the challenges
> that you're facing.

Sorry, I will improve my cover letter.

>
> cut
>
> >>>
> >>> You keep sending me contradictory messages.
> >>
> >> when? Please accept my apologies if that's the case, it's not in my
> >> intention. Provide better commit message, help me help you.
> >
> > I tried adding a new compatibility.
> > You say you do not want new compatibility.
>
> I said new compatibility will be introduced as a last resort only if we
> can't differentiate the flashes at run-time. You haven't proved me yet
> that this is the case.

Then you do not read my explanation.
Do you wish me to send the Macronix datasheet of the 4 chips?

>
> > You ask if it is possible to deduce it from JEDEC ID and SFDP,
> > I answer that this is not possible, at least in some cases..
>
> I'm interested just about your case, not all the possible cases.

Actually it is the MX25L3233F and its previous chips.
Anyway, I will not submit a broken solution.
Whether you like the idea or not.

>
> > I try to add OTP parameters in DT. You do not like it, fair enough.
> > What am I to do?
> > Seems like a dead end.
> >
>
> You can better explain what are the challenges you have and answer the
> questions that we're asking ...

I try.
But seems you are lock in a loop.
You want to eat the cake and leave it as is.
You do not want to add compatibility and reject using dynamic OTP.
You are welcome to find a third way.
And no you can not use JEDEC ID and SFDP for that.



> > Erez
> >
> >
> >>
> >>>
> >>> I told you we can not "guess" OTP settings based on JEDEC ID and SFDP existence.
> >>
> >> When? And more importantly, why?
> >
> > I send you example of 3/4 chips that using JEDEC ID and SFDP existence
> > is not enough.
>
> Why? Do they have the exact SFDP tables? Prove me, drop them all.
> Any bit that differs in the SFDP tables is enough to differentiate the
> flavors of flashes. Vendor tables included ;)

Because the SFDP is not related to OTP in any way.
You are planning to decide OTP parameters on non relevant information.
If you wish to implement such a broken feature, you are welcomed.
I'll pass.

> >
> >>
> >>> It may be partial and Macronix may add new chips in the future.
> >>
> >> I don't understand what you mean by partial, please elaborate.
> >
> > I think Kernel like using bullet proof methods.
> > methods that will produce a working results.
> > If I find one example we can not probe the OTP parameters this way, it
> > means this method is NOT bullet proof.
> >
> >>
> >> And we don't add support for what we assume new chips will look like.
> >
> > This is not what I ask for.
> > Just trying to guess OTP parameters in current chips will break with new chips.
> >
>
> Again, I'm interested just in the flashes that you use and can test. I'm
> not interested in addressing theoretical problems.
> >
> >>
> >>> They reuse JEDEC ID only retaining flash size and blocks.
> >>
> >> Yes, I know macronix shares flash IDs among flavors of flashes or new
> >> chips altogether.
> >
> > I am glad you know.
>
> I sense some rage and I find this inappropriate. I'm just trying to help.

Because I repeat myself over and over.
And I find myself in a loop with you.

>
> cut
>
> >>
> >> And I think I already said that you can differentiate between the two
> >> based on SFDP presence. mx25l12833f has SFDP, thus when SFDP present use
> >> the mx25l12833f-OTP configuration. When SFDP is not presence one may add
> >> support for the mx25l12805d-OTP configuration.
> >
> > No, we have 3 chips.
> > 2 are using the same JEDEC ID and both using SFDP, yet they use a different OTP.
>
> Which ones are these?
>
> I guess mx25l12805d is non-SFDP. Then mx25l12833f and mx25l3233f define
> some SFDP tables. Do mx25l12833f and mx25l3233f have the same OTP
> organization?

No, that is the point.


>
> > So, the problem is here, and probably be bigger in the future, despite
> > you "do not care".
> >
>
> I hear about your problem just here, after 3 emails exchanged and at
> least one hour wasted of my time.

Sorry for wasting your time.
I am also wasting mine, I hope you appreciate mine as yourself.

>
> I want to address problems one step at a time. We can address the bigger
> theoretical problems in the future, if they'll ever become real.

As I read more than 15 datasheets of Macronix flash chips, and
specifically ask the Macronix technical staff.
This is not theoretical. But actually.
Having said that, does not mean I plan to support in this patch more
than one chip with OTP.
Only that your suggestion will lead to a broken probing of OTP
parameters, if not in the present, then in the future.
See, until today OTP was not that appealing. I seem to be the first
one who wrote OTP code for Macronix.
So it is not like you have a real experience with it.
I just start the surface of it.





> >
> >>
> >> Is there any case that I miss?
> >
> > According to your reply, I would say pretty much most.
>
> This is again inappropriate ... I will read your next email as well, but
> I'm not going to tolerate such replies anymore.

I agree on this one.
Seems you are looking for something I do not agree on.
This is not because I oppose probing,
 this is because you ask for indirect probing, against Macronix own
recommendation.

Sorry for bothering you

Yours
   Erez

Re: [PATCH v5 1/5] mtd: spi-nor: core: add manufacturer flags

[Tudor Ambarus]

On 9/23/24 3:51 PM, Erez wrote:
> On Mon, 23 Sept 2024 at 16:18, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:

cut

>>> After reading a lot of Mactronix datasheets, I suggest also reading
>>> all SFDP to all  Mactronix chips.
>>
>> Why? It seems too invasive. Generally it is not recommended to issue
>> unsupported commands to flashes. Change just the flash that you use and
>> can test.
> 
> It is not, All Mactronix chips in the last 15 years have SFDP.

This is false. I worked with an octal macronix flash that didn't define
SFDP tables, mx66something.

> The chips in the manufacturer compatibility table are all obsolete.
> Mactronix reuse the JEDEC IDs. There is no point pretending these are
> the same chips.
> 

If macronix doesn't care about backward compatibility, we/I do, and we
can't break it.

>> cut
>>
>>>> Which flash do you have at hand, both, none, just one of them?
>>>
>>> When I started working on the OTP code, I used MX25L12833F.
>>> But later I left the company.
>>> So I use my beaglebone black and connect it to a MX25L3233F.
>>
>> I understand mx25l12805d and mx25l12833f share the same ID. How is
>> mx25l3233f related, does it share the same ID as the previous two?
> 
> They are not. I just replaced the company hardware with a different one.
> You ask me to report the hardware I use for testing.

So MX25L3233F does not share the same ID as MX25L12833F and mx25l12805d?
Then why do we talk about ID reuse?
> 
> The patch covers the one I use with beaglebone black.
> I just mentioned the OTP callbacks are per manufacturer.
> But if a new chip in the future would require different callbacks,
> then just add them.
> My patch is using a single chip, the one I send the testing with.
> beaglebone black + MX25L3233F.

Sounds good.

cut

>> I said new compatibility will be introduced as a last resort only if we
>> can't differentiate the flashes at run-time. You haven't proved me yet
>> that this is the case.
> 
> Then you do not read my explanation.

What explanation? I've read your cover letter, commit message and I
didn't understood what you're trying to achieve.

> Do you wish me to send the Macronix datasheet of the 4 chips?

No, I need just a paragraph where you explain what are the challenges
and how you want to address them.
> 
>>
>>> You ask if it is possible to deduce it from JEDEC ID and SFDP,
>>> I answer that this is not possible, at least in some cases..
>>
>> I'm interested just about your case, not all the possible cases.
> 
> Actually it is the MX25L3233F and its previous chips.

Which previous chips? Do you have any such chip at hand? If not, why are
we talking about collisions?

> Anyway, I will not submit a broken solution.
> Whether you like the idea or not.
> 

Fine by me.

cut

>>>>> I told you we can not "guess" OTP settings based on JEDEC ID and SFDP existence.
>>>>
>>>> When? And more importantly, why?
>>>
>>> I send you example of 3/4 chips that using JEDEC ID and SFDP existence
>>> is not enough.
>>
>> Why? Do they have the exact SFDP tables? Prove me, drop them all.
>> Any bit that differs in the SFDP tables is enough to differentiate the
>> flavors of flashes. Vendor tables included ;)
> 
> Because the SFDP is not related to OTP in any way.
> You are planning to decide OTP parameters on non relevant information.
> If you wish to implement such a broken feature, you are welcomed.
> I'll pass.

Ideally we have a single jedec,spi-nor compatible. If there are flash ID
collisions we try very hard to differentiate the flashes at run-time.
New compatibles are introduced only if we can't differentiate the flavor
at runtime (be it by parsing SFDP or some other way).

cut

>>>> And I think I already said that you can differentiate between the two
>>>> based on SFDP presence. mx25l12833f has SFDP, thus when SFDP present use
>>>> the mx25l12833f-OTP configuration. When SFDP is not presence one may add
>>>> support for the mx25l12805d-OTP configuration.
>>>
>>> No, we have 3 chips.
>>> 2 are using the same JEDEC ID and both using SFDP, yet they use a different OTP.
>>
>> Which ones are these?
>>
>> I guess mx25l12805d is non-SFDP. Then mx25l12833f and mx25l3233f define
>> some SFDP tables. Do mx25l12833f and mx25l3233f have the same OTP
>> organization?
> 
> No, that is the point.
> 
? Do you care to explain?

cut

> 
>>>
>>>>
>>>> Is there any case that I miss?
>>>
>>> According to your reply, I would say pretty much most.
>>
>> This is again inappropriate ... I will read your next email as well, but
>> I'm not going to tolerate such replies anymore.
> 
> I agree on this one.
> Seems you are looking for something I do not agree on.

Michael disagrees with OTP being specified in DT too. We both already
suggested how to deal with flash ID collisions but you keep ignoring us ...

> This is not because I oppose probing,
>  this is because you ask for indirect probing, against Macronix own
> recommendation.

What did macronix exactly recommend? Did they say that we shouldn't
interrogate the SFDP data in order to differentiate the flashes at
run-time? If yes, why is that?

Re: [PATCH v5 1/5] mtd: spi-nor: core: add manufacturer flags

[Erez]

On Mon, 23 Sept 2024 at 19:53, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
>
>
> On 9/23/24 3:51 PM, Erez wrote:
> > On Mon, 23 Sept 2024 at 16:18, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> cut
>
> >>> After reading a lot of Mactronix datasheets, I suggest also reading
> >>> all SFDP to all  Mactronix chips.
> >>
> >> Why? It seems too invasive. Generally it is not recommended to issue
> >> unsupported commands to flashes. Change just the flash that you use and
> >> can test.
> >
> > It is not, All Mactronix chips in the last 15 years have SFDP.
>
> This is false. I worked with an octal macronix flash that didn't define
> SFDP tables, mx66something.

I did not use the mx66XXX. Is it an SPI-NOR?
I would guess that mx66something you use is already obsolete.
Or marked by Mactronix as 'not recommended for new HW'.
It might be used by a big customer using the HW for a long time.


>
> > The chips in the manufacturer compatibility table are all obsolete.
> > Mactronix reuse the JEDEC IDs. There is no point pretending these are
> > the same chips.
> >
>
> If macronix doesn't care about backward compatibility, we/I do, and we
> can't break it.

As I said, I do not ask you to do that.

I do not represent Macronix, so I can not speak in their name.
My conclusions are based on examining their datasheet.
I did ask their technical representor.
The only straight answer I got from the technical support is
that you can not derive OTP configuration based on JEDEC ID/SFDP,
and you must know what chip you use.

>
> >> cut
> >>
> >>>> Which flash do you have at hand, both, none, just one of them?
> >>>
> >>> When I started working on the OTP code, I used MX25L12833F.
> >>> But later I left the company.
> >>> So I use my beaglebone black and connect it to a MX25L3233F.
> >>
> >> I understand mx25l12805d and mx25l12833f share the same ID. How is
> >> mx25l3233f related, does it share the same ID as the previous two?
> >
> > They are not. I just replaced the company hardware with a different one.
> > You ask me to report the hardware I use for testing.
>
> So MX25L3233F does not share the same ID as MX25L12833F and mx25l12805d?
> Then why do we talk about ID reuse?

I replaced the hardware I use.
Most of the reused IDs are of old chips, usually obsolete, not selled
or not recommended for new HW.
So the chance to use 2 new chips with the same ID is limited.

I respect the fact you want to keep backward compatibility.
I just extend the approach to OTP parameters.
If an old chip with the same JEDEC ID uses different OTP parameters.
We will break backward compatibility with this old chip.

> >
> > The patch covers the one I use with beaglebone black.
> > I just mentioned the OTP callbacks are per manufacturer.
> > But if a new chip in the future would require different callbacks,
> > then just add them.
> > My patch is using a single chip, the one I send the testing with.
> > beaglebone black + MX25L3233F.
>
> Sounds good.

+

>
> cut
>
> >> I said new compatibility will be introduced as a last resort only if we
> >> can't differentiate the flashes at run-time. You haven't proved me yet
> >> that this is the case.
> >
> > Then you do not read my explanation.
>
> What explanation? I've read your cover letter, commit message and I
> didn't understood what you're trying to achieve.

This is on me.
I'll try harder with the cover letter.
I apologize.

>
> > Do you wish me to send the Macronix datasheet of the 4 chips?
>
> No, I need just a paragraph where you explain what are the challenges
> and how you want to address them.

+

I try to explain that we can not based on JEDEC ID + SFDP to figure
out the correct OTP parameters.
This is also the only straight answer from Maxtronix I got.



> >
> >>
> >>> You ask if it is possible to deduce it from JEDEC ID and SFDP,
> >>> I answer that this is not possible, at least in some cases..
> >>
> >> I'm interested just about your case, not all the possible cases.
> >
> > Actually it is the MX25L3233F and its previous chips.
>
> Which previous chips? Do you have any such chip at hand? If not, why are
> we talking about collisions?

JEDEC ID 0xc22016
MX25L3205D - No SFDP, 2 OTP  regions of 128-bit, 384-bit, Status:End of Life,
Recommended Product MX25L3206E
MX25L3206E - support SFDP, 2 OTP  regions of 128-bit, 384-bit, Status:
Not recommend for new design Recommended Product MX25L3233F
MX25L3233F - support SFDP, 1 OTP region of 4096-bit, Status Production

JEDEC ID 0xc22017
MX25L6405D - No SFDP,  2 OTP  regions of 128-bit, 384-bit, Status: End
of Life, recommend Product MX25L6406E.
MX25L6406E - support SFDP, 2 OTP  regions of 128-bit, 384-bit,
Status:Not recommended for new design,
Recommended Product MX25L6433F.
MX25L6433F - support SFDP, 2 OTP regions of 4096-bit, 4096-bit, Status
Production.

JEDEC ID 0xc22015
MX25L1606E - support SFDP, 2 OTP  regions of 128-bit, 384-bit,Status:
Not recommend for new design,
Recommended Product MX25V16066
MX25V16066 - support SFDP, No OTP. Status Production.

The older chips with the same JEDEC IDs are at end of life or not
commended for new design.
But if we talk about backward compatibility, we can have them on old Hardware.

I think that I miss a chip in the list, I remember finding 4 chips
with the same JEDEC ID.

>
> > Anyway, I will not submit a broken solution.
> > Whether you like the idea or not.
> >
>
> Fine by me.

+

>
> cut
>
> >>>>> I told you we can not "guess" OTP settings based on JEDEC ID and SFDP existence.
> >>>>
> >>>> When? And more importantly, why?
> >>>
> >>> I send you example of 3/4 chips that using JEDEC ID and SFDP existence
> >>> is not enough.
> >>
> >> Why? Do they have the exact SFDP tables? Prove me, drop them all.
> >> Any bit that differs in the SFDP tables is enough to differentiate the
> >> flavors of flashes. Vendor tables included ;)
> >
> > Because the SFDP is not related to OTP in any way.
> > You are planning to decide OTP parameters on non relevant information.
> > If you wish to implement such a broken feature, you are welcomed.
> > I'll pass.
>
> Ideally we have a single jedec,spi-nor compatible. If there are flash ID
> collisions we try very hard to differentiate the flashes at run-time.
> New compatibles are introduced only if we can't differentiate the flavor
> at runtime (be it by parsing SFDP or some other way).
>

All I say is that it is a dangerous approach to deduce in this way.
Macronix does not care about breaking, they might introduce new chips
with different SFDP.
They usually do not sell new chips with the same JEDEC IDs. but apart
from that, we can not rely on any assumption.

I can understand if you say that you do not wish to go into this mess.
But indirect probing based on JEDEC ID + SFDP is a risk, I don't think
we should take with Macronix.


> cut
>
> >>>> And I think I already said that you can differentiate between the two
> >>>> based on SFDP presence. mx25l12833f has SFDP, thus when SFDP present use
> >>>> the mx25l12833f-OTP configuration. When SFDP is not presence one may add
> >>>> support for the mx25l12805d-OTP configuration.
> >>>
> >>> No, we have 3 chips.
> >>> 2 are using the same JEDEC ID and both using SFDP, yet they use a different OTP.
> >>
> >> Which ones are these?
> >>
> >> I guess mx25l12805d is non-SFDP. Then mx25l12833f and mx25l3233f define
> >> some SFDP tables. Do mx25l12833f and mx25l3233f have the same OTP
> >> organization?
> >
> > No, that is the point.
> >
> ? Do you care to explain?

The point is that you can not predict OTP based on JEDEC ID + SFDP.
It seems as if Macronix does the OTP in parallel.
See the list above.

>
> cut
>
> >
> >>>
> >>>>
> >>>> Is there any case that I miss?
> >>>
> >>> According to your reply, I would say pretty much most.
> >>
> >> This is again inappropriate ... I will read your next email as well, but
> >> I'm not going to tolerate such replies anymore.
> >
> > I agree on this one.
> > Seems you are looking for something I do not agree on.
>
> Michael disagrees with OTP being specified in DT too. We both already
> suggested how to deal with flash ID collisions but you keep ignoring us ...

I apologize, but I do not insist on DT.
Any solution that configures OTP regardless of JEDEC ID + SFDP is OK by me,
I am open to testing and submit a patch accordingly.


>
> > This is not because I oppose probing,
> >  this is because you ask for indirect probing, against Macronix own
> > recommendation.
>
> What did macronix exactly recommend? Did they say that we shouldn't
> interrogate the SFDP data in order to differentiate the flashes at
> run-time? If yes, why is that?


I forward the reply from Holger Schulze, Field Application Engineer,
Macronix Europe N.V. I received on the 3 Jul.

I ask:
"But the OTP setting is not in the SFDP.
How can we know which OTP size and number of regions to set?"

And the reply:
"You are correct, the OTP setting is not defined in the JEDEC spec for
the SFDP table. The only way is to refer to the data sheet."

Thanks for your feedback
Erez Geva

Re: [PATCH v5 1/5] mtd: spi-nor: core: add manufacturer flags

[Erez]

On Mon, 23 Sept 2024 at 21:30, Erez <erezgeva2@gmail.com> wrote:
>
> On Mon, 23 Sept 2024 at 19:53, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> >
> >
> >
> > On 9/23/24 3:51 PM, Erez wrote:
> > > On Mon, 23 Sept 2024 at 16:18, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
> >
> > cut
> >
> > >>> After reading a lot of Mactronix datasheets, I suggest also reading
> > >>> all SFDP to all  Mactronix chips.
> > >>
> > >> Why? It seems too invasive. Generally it is not recommended to issue
> > >> unsupported commands to flashes. Change just the flash that you use and
> > >> can test.
> > >
> > > It is not, All Mactronix chips in the last 15 years have SFDP.
> >
> > This is false. I worked with an octal macronix flash that didn't define
> > SFDP tables, mx66something.
>
> I did not use the mx66XXX. Is it an SPI-NOR?
> I would guess that mx66something you use is already obsolete.
> Or marked by Mactronix as 'not recommended for new HW'.
> It might be used by a big customer using the HW for a long time.
>
>
> >
> > > The chips in the manufacturer compatibility table are all obsolete.
> > > Mactronix reuse the JEDEC IDs. There is no point pretending these are
> > > the same chips.
> > >
> >
> > If macronix doesn't care about backward compatibility, we/I do, and we
> > can't break it.
>
> As I said, I do not ask you to do that.
>
> I do not represent Macronix, so I can not speak in their name.
> My conclusions are based on examining their datasheet.
> I did ask their technical representor.
> The only straight answer I got from the technical support is
> that you can not derive OTP configuration based on JEDEC ID/SFDP,
> and you must know what chip you use.
>
> >
> > >> cut
> > >>
> > >>>> Which flash do you have at hand, both, none, just one of them?
> > >>>
> > >>> When I started working on the OTP code, I used MX25L12833F.
> > >>> But later I left the company.
> > >>> So I use my beaglebone black and connect it to a MX25L3233F.
> > >>
> > >> I understand mx25l12805d and mx25l12833f share the same ID. How is
> > >> mx25l3233f related, does it share the same ID as the previous two?
> > >
> > > They are not. I just replaced the company hardware with a different one.
> > > You ask me to report the hardware I use for testing.
> >
> > So MX25L3233F does not share the same ID as MX25L12833F and mx25l12805d?
> > Then why do we talk about ID reuse?
>
> I replaced the hardware I use.
> Most of the reused IDs are of old chips, usually obsolete, not selled
> or not recommended for new HW.
> So the chance to use 2 new chips with the same ID is limited.
>
> I respect the fact you want to keep backward compatibility.
> I just extend the approach to OTP parameters.
> If an old chip with the same JEDEC ID uses different OTP parameters.
> We will break backward compatibility with this old chip.
>
> > >
> > > The patch covers the one I use with beaglebone black.
> > > I just mentioned the OTP callbacks are per manufacturer.
> > > But if a new chip in the future would require different callbacks,
> > > then just add them.
> > > My patch is using a single chip, the one I send the testing with.
> > > beaglebone black + MX25L3233F.
> >
> > Sounds good.
>
> +
>
> >
> > cut
> >
> > >> I said new compatibility will be introduced as a last resort only if we
> > >> can't differentiate the flashes at run-time. You haven't proved me yet
> > >> that this is the case.
> > >
> > > Then you do not read my explanation.
> >
> > What explanation? I've read your cover letter, commit message and I
> > didn't understood what you're trying to achieve.
>
> This is on me.
> I'll try harder with the cover letter.
> I apologize.
>
> >
> > > Do you wish me to send the Macronix datasheet of the 4 chips?
> >
> > No, I need just a paragraph where you explain what are the challenges
> > and how you want to address them.
>
> +
>
> I try to explain that we can not based on JEDEC ID + SFDP to figure
> out the correct OTP parameters.
> This is also the only straight answer from Maxtronix I got.
>
>
>
> > >
> > >>
> > >>> You ask if it is possible to deduce it from JEDEC ID and SFDP,
> > >>> I answer that this is not possible, at least in some cases..
> > >>
> > >> I'm interested just about your case, not all the possible cases.
> > >
> > > Actually it is the MX25L3233F and its previous chips.
> >
> > Which previous chips? Do you have any such chip at hand? If not, why are
> > we talking about collisions?
>
> JEDEC ID 0xc22016
> MX25L3205D - No SFDP, 2 OTP  regions of 128-bit, 384-bit, Status:End of Life,
> Recommended Product MX25L3206E
> MX25L3206E - support SFDP, 2 OTP  regions of 128-bit, 384-bit, Status:
> Not recommend for new design Recommended Product MX25L3233F
> MX25L3233F - support SFDP, 1 OTP region of 4096-bit, Status Production
>
> JEDEC ID 0xc22017
> MX25L6405D - No SFDP,  2 OTP  regions of 128-bit, 384-bit, Status: End
> of Life, recommend Product MX25L6406E.
> MX25L6406E - support SFDP, 2 OTP  regions of 128-bit, 384-bit,
> Status:Not recommended for new design,
> Recommended Product MX25L6433F.
> MX25L6433F - support SFDP, 2 OTP regions of 4096-bit, 4096-bit, Status
> Production.
>
> JEDEC ID 0xc22015
> MX25L1606E - support SFDP, 2 OTP  regions of 128-bit, 384-bit,Status:
> Not recommend for new design,
> Recommended Product MX25V16066
> MX25V16066 - support SFDP, No OTP. Status Production.
>
> The older chips with the same JEDEC IDs are at end of life or not
> commended for new design.
> But if we talk about backward compatibility, we can have them on old Hardware.
>
> I think that I miss a chip in the list, I remember finding 4 chips
> with the same JEDEC ID.
>
> >
> > > Anyway, I will not submit a broken solution.
> > > Whether you like the idea or not.
> > >
> >
> > Fine by me.
>
> +
>
> >
> > cut
> >
> > >>>>> I told you we can not "guess" OTP settings based on JEDEC ID and SFDP existence.
> > >>>>
> > >>>> When? And more importantly, why?
> > >>>
> > >>> I send you example of 3/4 chips that using JEDEC ID and SFDP existence
> > >>> is not enough.
> > >>
> > >> Why? Do they have the exact SFDP tables? Prove me, drop them all.
> > >> Any bit that differs in the SFDP tables is enough to differentiate the
> > >> flavors of flashes. Vendor tables included ;)
> > >
> > > Because the SFDP is not related to OTP in any way.
> > > You are planning to decide OTP parameters on non relevant information.
> > > If you wish to implement such a broken feature, you are welcomed.
> > > I'll pass.
> >
> > Ideally we have a single jedec,spi-nor compatible. If there are flash ID
> > collisions we try very hard to differentiate the flashes at run-time.
> > New compatibles are introduced only if we can't differentiate the flavor
> > at runtime (be it by parsing SFDP or some other way).
> >
>
> All I say is that it is a dangerous approach to deduce in this way.
> Macronix does not care about breaking, they might introduce new chips
> with different SFDP.
> They usually do not sell new chips with the same JEDEC IDs. but apart
> from that, we can not rely on any assumption.
>
> I can understand if you say that you do not wish to go into this mess.
> But indirect probing based on JEDEC ID + SFDP is a risk, I don't think
> we should take with Macronix.
>
>
> > cut
> >
> > >>>> And I think I already said that you can differentiate between the two
> > >>>> based on SFDP presence. mx25l12833f has SFDP, thus when SFDP present use
> > >>>> the mx25l12833f-OTP configuration. When SFDP is not presence one may add
> > >>>> support for the mx25l12805d-OTP configuration.
> > >>>
> > >>> No, we have 3 chips.
> > >>> 2 are using the same JEDEC ID and both using SFDP, yet they use a different OTP.
> > >>
> > >> Which ones are these?
> > >>
> > >> I guess mx25l12805d is non-SFDP. Then mx25l12833f and mx25l3233f define
> > >> some SFDP tables. Do mx25l12833f and mx25l3233f have the same OTP
> > >> organization?
> > >
> > > No, that is the point.
> > >
> > ? Do you care to explain?
>
> The point is that you can not predict OTP based on JEDEC ID + SFDP.
> It seems as if Macronix does the OTP in parallel.
> See the list above.
>
> >
> > cut
> >
> > >
> > >>>
> > >>>>
> > >>>> Is there any case that I miss?
> > >>>
> > >>> According to your reply, I would say pretty much most.
> > >>
> > >> This is again inappropriate ... I will read your next email as well, but
> > >> I'm not going to tolerate such replies anymore.
> > >
> > > I agree on this one.
> > > Seems you are looking for something I do not agree on.
> >
> > Michael disagrees with OTP being specified in DT too. We both already
> > suggested how to deal with flash ID collisions but you keep ignoring us ...
>
> I apologize, but I do not insist on DT.
> Any solution that configures OTP regardless of JEDEC ID + SFDP is OK by me,
> I am open to testing and submit a patch accordingly.

Just a thought.
What if we put a JEDEC ID + SFDP Macronix OTP probing code under a
kernel configuration with a poorer warning?

Erez

>
>
> >
> > > This is not because I oppose probing,
> > >  this is because you ask for indirect probing, against Macronix own
> > > recommendation.
> >
> > What did macronix exactly recommend? Did they say that we shouldn't
> > interrogate the SFDP data in order to differentiate the flashes at
> > run-time? If yes, why is that?
>
>
> I forward the reply from Holger Schulze, Field Application Engineer,
> Macronix Europe N.V. I received on the 3 Jul.
>
> I ask:
> "But the OTP setting is not in the SFDP.
> How can we know which OTP size and number of regions to set?"
>
> And the reply:
> "You are correct, the OTP setting is not defined in the JEDEC spec for
> the SFDP table. The only way is to refer to the data sheet."
>
> Thanks for your feedback
> Erez Geva

Re: [PATCH v5 1/5] mtd: spi-nor: core: add manufacturer flags

[Tudor Ambarus]

On 9/23/24 10:41 PM, Erez wrote:
> What if we put a JEDEC ID + SFDP Macronix OTP probing code under a
> kernel configuration with a poorer warning?

No, we don't add kernel configs for such things.

Re: [PATCH v5 1/5] mtd: spi-nor: core: add manufacturer flags

[Tudor Ambarus]

On 9/23/24 8:30 PM, Erez wrote:
> On Mon, 23 Sept 2024 at 19:53, Tudor Ambarus <tudor.ambarus@linaro.org> wrote:

cut

>>>>> You ask if it is possible to deduce it from JEDEC ID and SFDP,
>>>>> I answer that this is not possible, at least in some cases..
>>>>
>>>> I'm interested just about your case, not all the possible cases.
>>>
>>> Actually it is the MX25L3233F and its previous chips.
>>
>> Which previous chips? Do you have any such chip at hand? If not, why are
>> we talking about collisions?
> 
> JEDEC ID 0xc22016
> MX25L3205D - No SFDP, 2 OTP  regions of 128-bit, 384-bit, Status:End of Life,
> Recommended Product MX25L3206E
> MX25L3206E - support SFDP, 2 OTP  regions of 128-bit, 384-bit, Status:
> Not recommend for new design Recommended Product MX25L3233F
> MX25L3233F - support SFDP, 1 OTP region of 4096-bit, Status Production
> 

Okay. So you have just MX25L3233F on your table and you are worried
about possible ID collisions with MX25L3206E and MX25L3205D. But you
don't have access to MX25L3206E and MX25L3205D. Is my understanding correct?

If it's a yes, then don't worry about ID collisions at all, you can't
test the other flashes anyway. OTP is not used by the older flashes as
there's no support right now, so you don't break anything. Just add your
OTP organization to your flash and I (or other SPI NOR maintainer) will
decide on how to handle the ID collisions when/if they appear.

> JEDEC ID 0xc22017
> MX25L6405D - No SFDP,  2 OTP  regions of 128-bit, 384-bit, Status: End
> of Life, recommend Product MX25L6406E.
> MX25L6406E - support SFDP, 2 OTP  regions of 128-bit, 384-bit,
> Status:Not recommended for new design,
> Recommended Product MX25L6433F.
> MX25L6433F - support SFDP, 2 OTP regions of 4096-bit, 4096-bit, Status
> Production.
> 
> JEDEC ID 0xc22015
> MX25L1606E - support SFDP, 2 OTP  regions of 128-bit, 384-bit,Status:
> Not recommend for new design,
> Recommended Product MX25V16066
> MX25V16066 - support SFDP, No OTP. Status Production.

Thanks for the dive. We can ignore these because you can't test any of
them. I'll worry about them when someone adds OTP support for them.

cut

>>
>>> This is not because I oppose probing,
>>>  this is because you ask for indirect probing, against Macronix own
>>> recommendation.
>>
>> What did macronix exactly recommend? Did they say that we shouldn't
>> interrogate the SFDP data in order to differentiate the flashes at
>> run-time? If yes, why is that?
> 
> 
> I forward the reply from Holger Schulze, Field Application Engineer,
> Macronix Europe N.V. I received on the 3 Jul.
> 
> I ask:
> "But the OTP setting is not in the SFDP.
> How can we know which OTP size and number of regions to set?"
> 
> And the reply:
> "You are correct, the OTP setting is not defined in the JEDEC spec for
> the SFDP table. The only way is to refer to the data sheet."

We already know that the OTP is not defined in any SFDP table. This
doesn't mean that we can't compare SFDP tables to determine which flavor
of flash is present. That is the reason why we ask for SFDP dumps when
someone proposes a flash update or addition. So that we can compare the
SFDP dumps and correctly identify the flash or assure backward
compatibility for it.

[PATCH v5 2/5] mtd: spi-nor: core: add generic functions

[Erez Geva]

From: Erez Geva <ErezGeva2@gmail.com>

Create a new internal function, spi_nor_send_cmd_internal()
 to reduce duplicate code.

Add generic functions for use by vendor callbacks.
The functions contain dispatching for using SPIMEM
 or using an SPI controller.
It is better to leave this kind of dispatcher,
 out side of vendor specific code.

In this patch series we use the new functions in
 Macronix new OTP callbacks.

The new added functions:

 - Send an opcode without address or data.

 - Read a register value.

 - Write a register value.

Signed-off-by: Erez Geva <ErezGeva2@gmail.com>
---
 drivers/mtd/spi-nor/core.c | 130 +++++++++++++++++++++++++++----------
 drivers/mtd/spi-nor/core.h |  27 +-------
 2 files changed, 99 insertions(+), 58 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 0238475059f0..e486596fc70a 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -354,53 +354,134 @@ int spi_nor_write_any_volatile_reg(struct spi_nor *nor, struct spi_mem_op *op,
 }
 
 /**
- * spi_nor_write_enable() - Set write enable latch with Write Enable command.
+ * spi_nor_send_cmd_internal() - Send instruction without address or data to the chip.
  * @nor:	pointer to 'struct spi_nor'.
+ * @opcode:	Command to send
  *
  * Return: 0 on success, -errno otherwise.
  */
-int spi_nor_write_enable(struct spi_nor *nor)
+static inline int spi_nor_send_cmd_internal(struct spi_nor *nor, u8 opcode)
 {
 	int ret;
 
 	if (nor->spimem) {
-		struct spi_mem_op op = SPI_NOR_WREN_OP;
+		struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(opcode, 0),
+					   SPI_MEM_OP_NO_ADDR,
+					   SPI_MEM_OP_NO_DUMMY,
+					   SPI_MEM_OP_NO_DATA);
 
 		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
 
 		ret = spi_mem_exec_op(nor->spimem, &op);
 	} else {
-		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_WREN,
-						       NULL, 0);
+		ret = spi_nor_controller_ops_write_reg(nor, opcode, NULL, 0);
 	}
 
-	if (ret)
-		dev_dbg(nor->dev, "error %d on Write Enable\n", ret);
+	return ret;
+}
+
+/**
+ * spi_nor_send_cmd() - Send instruction without address or data to the chip.
+ * @nor:	pointer to 'struct spi_nor'.
+ * @opcode:	Command to send
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+int spi_nor_send_cmd(struct spi_nor *nor, u8 opcode)
+{
+	int ret;
+
+	ret = spi_nor_send_cmd_internal(nor, opcode);
 
 	return ret;
 }
 
 /**
- * spi_nor_write_disable() - Send Write Disable instruction to the chip.
+ * spi_nor_read_reg() - Send instruction without address or data to the chip.
  * @nor:	pointer to 'struct spi_nor'.
+ * @opcode:	Command to send
+ * @len:	register value length
  *
  * Return: 0 on success, -errno otherwise.
  */
-int spi_nor_write_disable(struct spi_nor *nor)
+int spi_nor_read_reg(struct spi_nor *nor, u8 opcode, size_t len)
 {
 	int ret;
 
 	if (nor->spimem) {
-		struct spi_mem_op op = SPI_NOR_WRDI_OP;
+		struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(opcode, 0),
+					   SPI_MEM_OP_NO_ADDR,
+					   SPI_MEM_OP_NO_DUMMY,
+					   SPI_MEM_OP_DATA_IN(len, nor->bouncebuf, 0));
 
 		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
 
 		ret = spi_mem_exec_op(nor->spimem, &op);
 	} else {
-		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_WRDI,
-						       NULL, 0);
+		ret = spi_nor_controller_ops_read_reg(nor, opcode, nor->bouncebuf, len);
 	}
 
+	return ret;
+}
+
+/*
+ * spi_nor_write_reg() - Send instruction without address or data to the chip.
+ * @nor:	pointer to 'struct spi_nor'.
+ * @opcode:	Command to send
+ * @len:	register value length
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+int spi_nor_write_reg(struct spi_nor *nor, u8 opcode, size_t len)
+{
+	int ret;
+
+	if (nor->spimem) {
+		struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(opcode, 0),
+					   SPI_MEM_OP_NO_ADDR,
+					   SPI_MEM_OP_NO_DUMMY,
+					   SPI_MEM_OP_DATA_OUT(len, nor->bouncebuf, 0));
+
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
+		ret = spi_mem_exec_op(nor->spimem, &op);
+	} else {
+		ret = spi_nor_controller_ops_write_reg(nor, opcode, nor->bouncebuf, len);
+	}
+
+	return ret;
+}
+
+/**
+ * spi_nor_write_enable() - Set write enable latch with Write Enable command.
+ * @nor:	pointer to 'struct spi_nor'.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+int spi_nor_write_enable(struct spi_nor *nor)
+{
+	int ret;
+
+	ret = spi_nor_send_cmd_internal(nor, SPINOR_OP_WREN);
+
+	if (ret)
+		dev_dbg(nor->dev, "error %d on Write Enable\n", ret);
+
+	return ret;
+}
+
+/**
+ * spi_nor_write_disable() - Send Write Disable instruction to the chip.
+ * @nor:	pointer to 'struct spi_nor'.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+int spi_nor_write_disable(struct spi_nor *nor)
+{
+	int ret;
+
+	ret = spi_nor_send_cmd_internal(nor, SPINOR_OP_WRDI);
+
 	if (ret)
 		dev_dbg(nor->dev, "error %d on Write Disable\n", ret);
 
@@ -521,18 +602,8 @@ int spi_nor_set_4byte_addr_mode_en4b_ex4b(struct spi_nor *nor, bool enable)
 {
 	int ret;
 
-	if (nor->spimem) {
-		struct spi_mem_op op = SPI_NOR_EN4B_EX4B_OP(enable);
-
-		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
-
-		ret = spi_mem_exec_op(nor->spimem, &op);
-	} else {
-		ret = spi_nor_controller_ops_write_reg(nor,
-						       enable ? SPINOR_OP_EN4B :
-								SPINOR_OP_EX4B,
-						       NULL, 0);
-	}
+	ret = spi_nor_send_cmd_internal(nor, enable ? SPINOR_OP_EN4B :
+						SPINOR_OP_EX4B);
 
 	if (ret)
 		dev_dbg(nor->dev, "error %d setting 4-byte mode\n", ret);
@@ -765,16 +836,7 @@ int spi_nor_global_block_unlock(struct spi_nor *nor)
 	if (ret)
 		return ret;
 
-	if (nor->spimem) {
-		struct spi_mem_op op = SPI_NOR_GBULK_OP;
-
-		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
-
-		ret = spi_mem_exec_op(nor->spimem, &op);
-	} else {
-		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_GBULK,
-						       NULL, 0);
-	}
+	ret = spi_nor_send_cmd_internal(nor, SPINOR_OP_GBULK);
 
 	if (ret) {
 		dev_dbg(nor->dev, "error %d on Global Block Unlock\n", ret);
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index c862e42c844f..615c399dfb6e 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -25,18 +25,6 @@
 		   SPI_MEM_OP_DUMMY(ndummy, 0),				\
 		   SPI_MEM_OP_DATA_IN(len, buf, 0))
 
-#define SPI_NOR_WREN_OP							\
-	SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WREN, 0),			\
-		   SPI_MEM_OP_NO_ADDR,					\
-		   SPI_MEM_OP_NO_DUMMY,					\
-		   SPI_MEM_OP_NO_DATA)
-
-#define SPI_NOR_WRDI_OP							\
-	SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WRDI, 0),			\
-		   SPI_MEM_OP_NO_ADDR,					\
-		   SPI_MEM_OP_NO_DUMMY,					\
-		   SPI_MEM_OP_NO_DATA)
-
 #define SPI_NOR_RDSR_OP(buf)						\
 	SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDSR, 0),			\
 		   SPI_MEM_OP_NO_ADDR,					\
@@ -67,24 +55,12 @@
 		   SPI_MEM_OP_NO_DUMMY,					\
 		   SPI_MEM_OP_DATA_IN(1, buf, 0))
 
-#define SPI_NOR_EN4B_EX4B_OP(enable)					\
-	SPI_MEM_OP(SPI_MEM_OP_CMD(enable ? SPINOR_OP_EN4B : SPINOR_OP_EX4B, 0),	\
-		   SPI_MEM_OP_NO_ADDR,					\
-		   SPI_MEM_OP_NO_DUMMY,					\
-		   SPI_MEM_OP_NO_DATA)
-
 #define SPI_NOR_BRWR_OP(buf)						\
 	SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_BRWR, 0),			\
 		   SPI_MEM_OP_NO_ADDR,					\
 		   SPI_MEM_OP_NO_DUMMY,					\
 		   SPI_MEM_OP_DATA_OUT(1, buf, 0))
 
-#define SPI_NOR_GBULK_OP						\
-	SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_GBULK, 0),			\
-		   SPI_MEM_OP_NO_ADDR,					\
-		   SPI_MEM_OP_NO_DUMMY,					\
-		   SPI_MEM_OP_NO_DATA)
-
 #define SPI_NOR_DIE_ERASE_OP(opcode, addr_nbytes, addr, dice)		\
 	SPI_MEM_OP(SPI_MEM_OP_CMD(opcode, 0),				\
 		   SPI_MEM_OP_ADDR(dice ? addr_nbytes : 0, addr, 0),	\
@@ -604,6 +580,9 @@ extern const struct attribute_group *spi_nor_sysfs_groups[];
 void spi_nor_spimem_setup_op(const struct spi_nor *nor,
 			     struct spi_mem_op *op,
 			     const enum spi_nor_protocol proto);
+int spi_nor_send_cmd(struct spi_nor *nor, u8 opcode);
+int spi_nor_read_reg(struct spi_nor *nor, u8 opcode, size_t len);
+int spi_nor_write_reg(struct spi_nor *nor, u8 opcode, size_t len);
 int spi_nor_write_enable(struct spi_nor *nor);
 int spi_nor_write_disable(struct spi_nor *nor);
 int spi_nor_set_4byte_addr_mode_en4b_ex4b(struct spi_nor *nor, bool enable);
-- 
2.39.5

[PATCH v5 3/5] dt-bindings: mtd: spi-nor: add OTP parameters

[Erez Geva]

From: Erez Geva <ErezGeva2@gmail.com>

Some flash devices need OTP parameters in device tree.
As we can not deduce the parameters based on JEDEC ID or SFDP.

Signed-off-by: Erez Geva <ErezGeva2@gmail.com>
---
 .../bindings/mtd/jedec,spi-nor.yaml           | 39 +++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
index 6e3afb42926e..4f7bb3f41cb1 100644
--- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
+++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
@@ -90,6 +90,43 @@ properties:
       the SRWD bit while writing the status register. WP# signal hard strapped to GND
       can be a valid use case.
 
+  otp-n-regions:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Some flash devices need OTP parameters in the device tree.
+      As we can not deduce the parameters based on JEDEC ID or SFDP.
+      This parameter indicates the number of OTP regions.
+      The value must be larger or equal to 1 and mandatory for OTP.
+
+  otp-len:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Some flash devices need OTP parameters in the device tree.
+      As we can not deduce the parameters based on JEDEC ID or SFDP.
+      This parameter indicates the size (length) in bytes of an OTP region.
+      Currently the driver supports symmetric OTP,
+       which means all regions must use the same size.
+      The value must be positive and mandatory for OTP.
+
+  otp-offset:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Some flash devices need OTP parameters in the device tree.
+      As we can not deduce the parameters based on JEDEC ID or SFDP.
+      This parameter indicates the offset in bytes of
+       an OTP region relative to its previous.
+      User can omit it if the offset equals the length.
+      Or in case we have a single OTP region.
+
+  otp-base:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Some flash devices need OTP parameters in the device tree.
+      As we can not deduce the parameters based on JEDEC ID or SFDP.
+      This parameter indicates the base in bytes of the first OTP region.
+      User can omit it if the base is zero.
+      I.e. the address of the first OTP region starts from 0.
+
   reset-gpios:
     description:
       A GPIO line connected to the RESET (active low) signal of the device.
@@ -118,6 +155,8 @@ examples:
             spi-max-frequency = <40000000>;
             m25p,fast-read;
             reset-gpios = <&gpio 12 GPIO_ACTIVE_LOW>;
+            otp-len = <512>;
+            otp-n-regions = <1>;
         };
     };
 ...
-- 
2.39.5

Re: [PATCH v5 3/5] dt-bindings: mtd: spi-nor: add OTP parameters

[Krzysztof Kozlowski]

On Fri, Sep 20, 2024 at 08:12:29PM +0200, Erez Geva wrote:
> From: Erez Geva <ErezGeva2@gmail.com>
> 
> Some flash devices need OTP parameters in device tree.
> As we can not deduce the parameters based on JEDEC ID or SFDP.
> 
> Signed-off-by: Erez Geva <ErezGeva2@gmail.com>
> ---
>  .../bindings/mtd/jedec,spi-nor.yaml           | 39 +++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
> index 6e3afb42926e..4f7bb3f41cb1 100644
> --- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
> +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
> @@ -90,6 +90,43 @@ properties:
>        the SRWD bit while writing the status register. WP# signal hard strapped to GND
>        can be a valid use case.
>  
> +  otp-n-regions:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Some flash devices need OTP parameters in the device tree.
> +      As we can not deduce the parameters based on JEDEC ID or SFDP.
> +      This parameter indicates the number of OTP regions.

OTP regions where? In DTS? On flash itself?

> +      The value must be larger or equal to 1 and mandatory for OTP.

Don't repeat constraints in free form text. Add proper minimum and
default, although it is confusing - property is not required but it is
mandatory for OTP?



> +
> +  otp-len:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Some flash devices need OTP parameters in the device tree.
> +      As we can not deduce the parameters based on JEDEC ID or SFDP.

Don't repeat the same.

> +      This parameter indicates the size (length) in bytes of an OTP region.

What if each region has different length? Is it possible?

> +      Currently the driver supports symmetric OTP,
> +       which means all regions must use the same size.

Drop unneeded space in front.

> +      The value must be positive and mandatory for OTP.

Same comments as before.

> +
> +  otp-offset:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Some flash devices need OTP parameters in the device tree.
> +      As we can not deduce the parameters based on JEDEC ID or SFDP.
> +      This parameter indicates the offset in bytes of
> +       an OTP region relative to its previous.
> +      User can omit it if the offset equals the length.
> +      Or in case we have a single OTP region.
> +
> +  otp-base:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      Some flash devices need OTP parameters in the device tree.
> +      As we can not deduce the parameters based on JEDEC ID or SFDP.
> +      This parameter indicates the base in bytes of the first OTP region.
> +      User can omit it if the base is zero.

so default: 0?

I am not sure, but all this looks like duplicated description of nvmem
cells or some sort of partitions.

Considering that this is first time the binding mentions OTP, I really
do not get what you are doing it. If the properties are to stay, please
describe better the problem (and hardware!) in commit msg.

Best regards,
Krzysztof

Re: [PATCH v5 3/5] dt-bindings: mtd: spi-nor: add OTP parameters

[Erez]

On Sun, 22 Sept 2024 at 22:40, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Fri, Sep 20, 2024 at 08:12:29PM +0200, Erez Geva wrote:
> > From: Erez Geva <ErezGeva2@gmail.com>
> >
> > Some flash devices need OTP parameters in device tree.
> > As we can not deduce the parameters based on JEDEC ID or SFDP.
> >
> > Signed-off-by: Erez Geva <ErezGeva2@gmail.com>
> > ---
> >  .../bindings/mtd/jedec,spi-nor.yaml           | 39 +++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
> > index 6e3afb42926e..4f7bb3f41cb1 100644
> > --- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
> > +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
> > @@ -90,6 +90,43 @@ properties:
> >        the SRWD bit while writing the status register. WP# signal hard strapped to GND
> >        can be a valid use case.
> >
> > +  otp-n-regions:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      Some flash devices need OTP parameters in the device tree.
> > +      As we can not deduce the parameters based on JEDEC ID or SFDP.
> > +      This parameter indicates the number of OTP regions.
>
> OTP regions where? In DTS? On flash itself?

Where can OTP regions be?
Can you please be serious?
If you have any suggestions, I am happy to hear.
I did ask before.

>
> > +      The value must be larger or equal to 1 and mandatory for OTP.
>
> Don't repeat constraints in free form text. Add proper minimum and

Sure, I will add a minimum.

> default, although it is confusing - property is not required but it is
> mandatory for OTP?

You are welcome to suggest a better rephrase.
Using OTP settings is optional.
If you set OTP then the number of regions and region length are mandatory.
While offset and base are optional for OTP settings.


>
>
>
> > +
> > +  otp-len:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      Some flash devices need OTP parameters in the device tree.
> > +      As we can not deduce the parameters based on JEDEC ID or SFDP.
>
> Don't repeat the same.

Is there a grouping description?

>
> > +      This parameter indicates the size (length) in bytes of an OTP region.
>
> What if each region has different length? Is it possible?

Yes, there are. Old Mactronix have chips with the first region bigger
than the second region.
As these are old chips, we may skip the support of them.

>
> > +      Currently the driver supports symmetric OTP,
> > +       which means all regions must use the same size.
>
> Drop unneeded space in front.

OK

>
> > +      The value must be positive and mandatory for OTP.
>
> Same comments as before.
>
> > +
> > +  otp-offset:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      Some flash devices need OTP parameters in the device tree.
> > +      As we can not deduce the parameters based on JEDEC ID or SFDP.
> > +      This parameter indicates the offset in bytes of
> > +       an OTP region relative to its previous.
> > +      User can omit it if the offset equals the length.
> > +      Or in case we have a single OTP region.
> > +
> > +  otp-base:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      Some flash devices need OTP parameters in the device tree.
> > +      As we can not deduce the parameters based on JEDEC ID or SFDP.
> > +      This parameter indicates the base in bytes of the first OTP region.
> > +      User can omit it if the base is zero.
>
> so default: 0?

Sure.

>
> I am not sure, but all this looks like duplicated description of nvmem
> cells or some sort of partitions.
>
> Considering that this is first time the binding mentions OTP, I really
> do not get what you are doing it. If the properties are to stay, please
> describe better the problem (and hardware!) in commit msg.

OK, good point.

Thanks for the feedback.

>
> Best regards,
> Krzysztof
>

Re: [PATCH v5 3/5] dt-bindings: mtd: spi-nor: add OTP parameters

[Krzysztof Kozlowski]

On 23/09/2024 11:21, Erez wrote:
> On Sun, 22 Sept 2024 at 22:40, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On Fri, Sep 20, 2024 at 08:12:29PM +0200, Erez Geva wrote:
>>> From: Erez Geva <ErezGeva2@gmail.com>
>>>
>>> Some flash devices need OTP parameters in device tree.
>>> As we can not deduce the parameters based on JEDEC ID or SFDP.
>>>
>>> Signed-off-by: Erez Geva <ErezGeva2@gmail.com>
>>> ---
>>>  .../bindings/mtd/jedec,spi-nor.yaml           | 39 +++++++++++++++++++
>>>  1 file changed, 39 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
>>> index 6e3afb42926e..4f7bb3f41cb1 100644
>>> --- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
>>> +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
>>> @@ -90,6 +90,43 @@ properties:
>>>        the SRWD bit while writing the status register. WP# signal hard strapped to GND
>>>        can be a valid use case.
>>>
>>> +  otp-n-regions:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description:
>>> +      Some flash devices need OTP parameters in the device tree.
>>> +      As we can not deduce the parameters based on JEDEC ID or SFDP.
>>> +      This parameter indicates the number of OTP regions.
>>
>> OTP regions where? In DTS? On flash itself?
> 
> Where can OTP regions be?
> Can you please be serious?
> If you have any suggestions, I am happy to hear.
> I did ask before.

Yes, I am serious, imagine that we do not know what you wanted to say.
At first this just sounded like you mix nvmem-cells here.

Out of blue this binding starts mentioning OTP and you add bunch of
generic properties not really matching anything so far. Instead of being
sarcastic about reviewers confusion, rather improve your description.

Otherwise, good luck.



> 
>>
>>> +      The value must be larger or equal to 1 and mandatory for OTP.
>>
>> Don't repeat constraints in free form text. Add proper minimum and
> 
> Sure, I will add a minimum.
> 
>> default, although it is confusing - property is not required but it is
>> mandatory for OTP?
> 
> You are welcome to suggest a better rephrase.
> Using OTP settings is optional.
> If you set OTP then the number of regions and region length are mandatory.
> While offset and base are optional for OTP settings.

So properties should be required?

> 
> 
>>
>>
>>
>>> +
>>> +  otp-len:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    description:
>>> +      Some flash devices need OTP parameters in the device tree.
>>> +      As we can not deduce the parameters based on JEDEC ID or SFDP.
>>
>> Don't repeat the same.
> 
> Is there a grouping description?
> 
>>
>>> +      This parameter indicates the size (length) in bytes of an OTP region.
>>
>> What if each region has different length? Is it possible?
> 
> Yes, there are. Old Mactronix have chips with the first region bigger
> than the second region.
> As these are old chips, we may skip the support of them.

Other devices can come later re-introducing this approach.



Best regards,
Krzysztof

Re: [PATCH v5 3/5] dt-bindings: mtd: spi-nor: add OTP parameters

[Erez]

On Mon, 23 Sept 2024 at 17:42, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 23/09/2024 11:21, Erez wrote:
> > On Sun, 22 Sept 2024 at 22:40, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >>
> >> On Fri, Sep 20, 2024 at 08:12:29PM +0200, Erez Geva wrote:
> >>> From: Erez Geva <ErezGeva2@gmail.com>
> >>>
> >>> Some flash devices need OTP parameters in device tree.
> >>> As we can not deduce the parameters based on JEDEC ID or SFDP.
> >>>
> >>> Signed-off-by: Erez Geva <ErezGeva2@gmail.com>
> >>> ---
> >>>  .../bindings/mtd/jedec,spi-nor.yaml           | 39 +++++++++++++++++++
> >>>  1 file changed, 39 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
> >>> index 6e3afb42926e..4f7bb3f41cb1 100644
> >>> --- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
> >>> +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
> >>> @@ -90,6 +90,43 @@ properties:
> >>>        the SRWD bit while writing the status register. WP# signal hard strapped to GND
> >>>        can be a valid use case.
> >>>
> >>> +  otp-n-regions:
> >>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>> +    description:
> >>> +      Some flash devices need OTP parameters in the device tree.
> >>> +      As we can not deduce the parameters based on JEDEC ID or SFDP.
> >>> +      This parameter indicates the number of OTP regions.
> >>
> >> OTP regions where? In DTS? On flash itself?
> >
> > Where can OTP regions be?
> > Can you please be serious?
> > If you have any suggestions, I am happy to hear.
> > I did ask before.
>
> Yes, I am serious, imagine that we do not know what you wanted to say.
> At first this just sounded like you mix nvmem-cells here.
>
> Out of blue this binding starts mentioning OTP and you add bunch of
> generic properties not really matching anything so far. Instead of being
> sarcastic about reviewers confusion, rather improve your description

Sorry, no disrespect.
I just prefer to be explicit, same as you ask me to be.

The patch seria is under review in the MTD-SPI-NOR.
This patch is under hold at the moment.

Thanks for the feedback and your time
Erez
.
>
> Otherwise, good luck.
>
>
>
> >
> >>
> >>> +      The value must be larger or equal to 1 and mandatory for OTP.
> >>
> >> Don't repeat constraints in free form text. Add proper minimum and
> >
> > Sure, I will add a minimum.
> >
> >> default, although it is confusing - property is not required but it is
> >> mandatory for OTP?
> >
> > You are welcome to suggest a better rephrase.
> > Using OTP settings is optional.
> > If you set OTP then the number of regions and region length are mandatory.
> > While offset and base are optional for OTP settings.
>
> So properties should be required?
>
> >
> >
> >>
> >>
> >>
> >>> +
> >>> +  otp-len:
> >>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>> +    description:
> >>> +      Some flash devices need OTP parameters in the device tree.
> >>> +      As we can not deduce the parameters based on JEDEC ID or SFDP.
> >>
> >> Don't repeat the same.
> >
> > Is there a grouping description?
> >
> >>
> >>> +      This parameter indicates the size (length) in bytes of an OTP region.
> >>
> >> What if each region has different length? Is it possible?
> >
> > Yes, there are. Old Mactronix have chips with the first region bigger
> > than the second region.
> > As these are old chips, we may skip the support of them.
>
> Other devices can come later re-introducing this approach.
>
>
>
> Best regards,
> Krzysztof
>

[PATCH v5 4/5] mtd: spi-nor: macronix: add support for OTP

[Erez Geva]

From: Erez Geva <ErezGeva2@gmail.com>

Macronix SPI-NOR support OTP.
Add callbacks to read, write and lock the OTP.

Notice Macronix OTP do not support erase.
Every bit written with '0', can not be changed further.

Notice Macronix OTP do not support single region lock!
The locking includes all regions at once!

Signed-off-by: Erez Geva <ErezGeva2@gmail.com>
---
 drivers/mtd/spi-nor/macronix.c | 167 +++++++++++++++++++++++++++++++++
 include/linux/mtd/spi-nor.h    |   9 ++
 2 files changed, 176 insertions(+)

diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
index ea6be95e75a5..bee62e9051d6 100644
--- a/drivers/mtd/spi-nor/macronix.c
+++ b/drivers/mtd/spi-nor/macronix.c
@@ -8,6 +8,162 @@
 
 #include "core.h"
 
+/**
+ * macronix_nor_otp_enter() - Send Enter Secured OTP instruction to the chip.
+ * @nor:	pointer to 'struct spi_nor'.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int macronix_nor_otp_enter(struct spi_nor *nor)
+{
+	int error;
+
+	error = spi_nor_send_cmd(nor, SPINOR_OP_ENSO);
+	if (error)
+		dev_dbg(nor->dev, "error %d on Macronix Enter Secured OTP\n", error);
+
+	return error;
+}
+
+/**
+ * macronix_nor_otp_exit() - Send Exit Secured OTP instruction to the chip.
+ * @nor:	pointer to 'struct spi_nor'.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int macronix_nor_otp_exit(struct spi_nor *nor)
+{
+	int error;
+
+	error = spi_nor_send_cmd(nor, SPINOR_OP_EXSO);
+	if (error)
+		dev_dbg(nor->dev, "error %d on Macronix Exit Secured OTP\n", error);
+
+	return error;
+}
+
+/**
+ * macronix_nor_otp_read() - read OTP data
+ * @nor:	pointer to 'struct spi_nor'
+ * @addr:       offset to read from
+ * @len:        number of bytes to read
+ * @buf:        pointer to dst buffer
+ *
+ * Return: number of bytes read successfully, -errno otherwise
+ */
+static int macronix_nor_otp_read(struct spi_nor *nor, loff_t addr, size_t len, u8 *buf)
+{
+	int ret, error;
+
+	error = macronix_nor_otp_enter(nor);
+	if (error)
+		return error;
+
+	ret = spi_nor_read_data(nor, addr, len, buf);
+
+	error = macronix_nor_otp_exit(nor);
+
+	if (ret < 0)
+		dev_dbg(nor->dev, "error %d on Macronix read OTP data\n", ret);
+	else if (error)
+		return error;
+
+	return ret;
+}
+
+/**
+ * macronix_nor_otp_write() - write data to OTP
+ * @nor:        pointer to 'struct spi_nor'
+ * @addr:       offset to write to
+ * @len:        number of bytes to write
+ * @buf:        pointer to src buffer
+ *
+ * Return: number of bytes written successfully, -errno otherwise
+ */
+static int macronix_nor_otp_write(struct spi_nor *nor, loff_t addr, size_t len, const u8 *buf)
+{
+	int error, ret = 0;
+
+	error = macronix_nor_otp_enter(nor);
+	if (error)
+		return error;
+
+	error = spi_nor_write_enable(nor);
+	if (error)
+		goto otp_write_err;
+
+	ret = spi_nor_write_data(nor, addr, len, buf);
+	if (ret < 0) {
+		dev_dbg(nor->dev, "error %d on Macronix write OTP data\n", ret);
+		goto otp_write_err;
+	}
+
+	error = spi_nor_wait_till_ready(nor);
+	if (error)
+		dev_dbg(nor->dev, "error %d on Macronix waiting write OTP finish\n", error);
+
+otp_write_err:
+
+	error = macronix_nor_otp_exit(nor);
+
+	return ret;
+}
+
+/**
+ * macronix_nor_otp_lock() - lock the OTP
+ * @nor:        pointer to 'struct spi_nor'
+ * @region:     OTP region
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int macronix_nor_otp_lock(struct spi_nor *nor, unsigned int region)
+{
+	int error;
+	u8 *rdscur = nor->bouncebuf;
+
+	error = spi_nor_read_reg(nor, SPINOR_OP_RDSCUR, 1);
+	if (error) {
+		dev_dbg(nor->dev, "error %d on read security register\n", error);
+		return error;
+	}
+
+	if (rdscur[0] & SEC_REG_LDSO)
+		return 0;
+
+	error = spi_nor_write_enable(nor);
+	if (error) {
+		dev_dbg(nor->dev, "error %d on enable write before update security register\n",
+			error);
+		return error;
+	}
+
+	error = spi_nor_send_cmd(nor, SPINOR_OP_WRSCUR);
+	if (error)
+		dev_dbg(nor->dev, "error %d on update security register\n", error);
+
+	return error;
+}
+
+/**
+ * macronix_nor_otp_is_locked() - get the OTP lock status
+ * @nor:        pointer to 'struct spi_nor'
+ * @region:     OTP region
+ *
+ * Return: 1 on lock, 0 on not locked, -errno otherwise.
+ */
+static int macronix_nor_otp_is_locked(struct spi_nor *nor, unsigned int region)
+{
+	int error;
+	u8 *rdscur = nor->bouncebuf;
+
+	error = spi_nor_read_reg(nor, SPINOR_OP_RDSCUR, 1);
+	if (error) {
+		dev_dbg(nor->dev, "error %d on read security register\n", error);
+		return error;
+	}
+	return rdscur[0] & SEC_REG_LDSO ? 1 : 0;
+}
+
 static int
 mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
 			    const struct sfdp_parameter_header *bfpt_header,
@@ -190,8 +346,19 @@ static void macronix_nor_default_init(struct spi_nor *nor)
 	nor->params->quad_enable = spi_nor_sr1_bit6_quad_enable;
 }
 
+static const struct spi_nor_otp_ops macronix_nor_otp_ops = {
+	.read = macronix_nor_otp_read,
+	.write = macronix_nor_otp_write,
+	/* .erase = Macronix OTP do not support erase, */
+	.lock = macronix_nor_otp_lock,
+	.is_locked = macronix_nor_otp_is_locked,
+};
+
 static int macronix_nor_late_init(struct spi_nor *nor)
 {
+	if (nor->params->otp.org.n_regions)
+		nor->params->otp.ops = &macronix_nor_otp_ops;
+
 	if (!nor->params->set_4byte_addr_mode)
 		nor->params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode_en4b_ex4b;
 
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index cdcfe0fd2e7d..ef834e7fc0ac 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -81,6 +81,15 @@
 #define SPINOR_OP_BP		0x02	/* Byte program */
 #define SPINOR_OP_AAI_WP	0xad	/* Auto address increment word program */
 
+/* Macronix OTP registers. */
+#define SPINOR_OP_RDSCUR	0x2b	/* read security register */
+#define SPINOR_OP_WRSCUR	0x2f	/* write security register */
+#define SPINOR_OP_ENSO		0xb1	/* enter secured OTP */
+#define SPINOR_OP_EXSO		0xc1	/* exit secured OTP */
+
+/* Macronix security register values */
+#define SEC_REG_LDSO		BIT(1)  /* Lock-down Secured OTP */
+
 /* Used for Macronix and Winbond flashes. */
 #define SPINOR_OP_EN4B		0xb7	/* Enter 4-byte mode */
 #define SPINOR_OP_EX4B		0xe9	/* Exit 4-byte mode */
-- 
2.39.5

[PATCH v5 5/5] mtd: spi-nor: macronix: add manufacturer flags

[Erez Geva]

From: Erez Geva <ErezGeva2@gmail.com>

Add flag for always trying reading SFDP:
All new chips from Macronix support SFDP.
All old chips in the IDs table were reused by new chips.

Add flag for reading OTP parameters from device tree.
As Macronix reuse JEDEC IDs, there is no way to determine OTP parameters.
Allow users to define the OTP parameters in device tree.

Signed-off-by: Erez Geva <ErezGeva2@gmail.com>
---

Notes:
    * My initial Macronix OTP code was tested with MX25l12833F.
    * As I no longer have that hardware.
    
    * I now testing with MX25L3233F connected to my BeagleBone Black
    *  through an 8-PIN SOP (200mil).
    * The BeagleBone Black runs with Debian GNU/Linux 12.
    * And use Kernel 6.6.32-ti-arm32-r5 build with
    *  arm-linux-gnueabihf-gcc gcc version 12.2.0 (Debian 12.2.0-14).
    
    $ cat /sys/bus/spi/devices/spi0.0/spi-nor/jedec_id
    c22016
    $ cat /sys/bus/spi/devices/spi0.0/spi-nor/manufacturer
    macronix
    $ cat /sys/bus/spi/devices/spi0.0/spi-nor/partname
    mx25l3205d
    $ xxd -p /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
    53464450000101ff00000109300000ffc2000104600000ffffffffffffff
    ffffffffffffffffffffffffffffffffffffe520f1ffffffff0144eb086b
    083b04bbeeffffffffff00ffffff00ff0c200f5210d800ffffffffffffff
    ffffffffffff003650269ef97764fecfffffffffffff
    $ sha256sum /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
    22d5d34af77c3628300056a0fc4bfbeafa027f544998852cf27f7cebf7881196  /sys/bus/spi/devices/spi0.0/spi-nor/sfdp
    
    $ cat /sys/kernel/debug/spi-nor/spi0.0/capabilities
    Supported read modes by the flash
     1S-1S-1S
      opcode        0x03
      mode cycles   0
      dummy cycles  0
     1S-1S-2S
      opcode        0x3b
      mode cycles   0
      dummy cycles  8
     1S-2S-2S
      opcode        0xbb
      mode cycles   0
      dummy cycles  4
     1S-1S-4S
      opcode        0x6b
      mode cycles   0
      dummy cycles  8
     1S-4S-4S
      opcode        0xeb
      mode cycles   2
      dummy cycles  4
    
    Supported page program modes by the flash
     1S-1S-1S
      opcode        0x02
    
    $ cat /sys/kernel/debug/spi-nor/spi0.0/params
    name            mx25l3205d
    id              c2 20 16 c2 20 16
    size            4.00 MiB
    write size      1
    page size       256
    address nbytes  3
    flags           HAS_16BIT_SR
    
    opcodes
     read           0x03
      dummy cycles  0
     erase          0x20
     program        0x02
     8D extension   none
    
    protocols
     read           1S-1S-1S
     write          1S-1S-1S
     register       1S-1S-1S
    
    erase commands
     20 (4.00 KiB) [1]
     52 (32.0 KiB) [2]
     d8 (64.0 KiB) [3]
     c7 (4.00 MiB)
    
    sector map
     region (in hex)   | erase mask | flags
     ------------------+------------+----------
     00000000-003fffff |     [ 123] |
    
    # mtd_debug info /dev/mtd0
    mtd.type = MTD_NORFLASH
    mtd.flags = MTD_CAP_NORFLASH
    mtd.size = 4194304 (4M)
    mtd.erasesize = 4096 (4K)
    mtd.writesize = 1
    mtd.oobsize = 0
    regions = 0
    
    * The BeagleBone Black SPI is very slow, Tests are slow.
    
    # dd if=/dev/urandom of=./spi_test bs=1M count=2
    2+0 records in
    2+0 records out
    2097152 bytes (2.1 MB, 2.0 MiB) copied, 0.0682607 s, 30.7 MB/s
    
    # time mtd_debug erase /dev/mtd0 0 2097152
    Erased 2097152 bytes from address 0x00000000 in flash
    
    real    0m12.703s
    user    0m0.000s
    sys     0m12.692s
    
    # time mtd_debug read /dev/mtd0 0 2097152 spi_read
    Copied 2097152 bytes from address 0x00000000 in flash to spi_read
    
    real    0m1.942s
    user    0m0.000s
    sys     0m0.053s
    
    # hexdump spi_read
    0000000 ffff ffff ffff ffff ffff ffff ffff ffff
    *
    0200000
    
    # sha256sum spi_read
    4bda3a28f4ffe603c0ec1258c0034d65a1a0d35ab7bd523a834608adabf03cc5  spi_read
    
    # time mtd_debug write /dev/mtd0 0 2097152 spi_test
    Copied 2097152 bytes from spi_test to address 0x00000000 in flash
    
    real    0m5.883s
    user    0m0.006s
    sys     0m3.970s
    
    # time mtd_debug read /dev/mtd0 0 2097152 spi_read
    Copied 2097152 bytes from address 0x00000000 in flash to spi_read
    
    real    0m2.208s
    user    0m0.003s
    sys     0m0.063s
    
    # sha256sum spi*
    f4f5d1d0a4fef487037cdb3f1be0f9aab68ca32f2dbe8782c927f03adf623ec3  spi_read
    f4f5d1d0a4fef487037cdb3f1be0f9aab68ca32f2dbe8782c927f03adf623ec3  spi_test
    
    # time mtd_debug erase /dev/mtd0 0 2097152
    Erased 2097152 bytes from address 0x00000000 in flash
    
    real    0m12.126s
    user    0m0.001s
    sys     0m12.115s
    
    # time mtd_debug read /dev/mtd0 0 2097152 spi_read
    Copied 2097152 bytes from address 0x00000000 in flash to spi_read
    
    real    0m2.611s
    user    0m0.000s
    sys     0m0.064s
    
    # sha256sum spi*
    4bda3a28f4ffe603c0ec1258c0034d65a1a0d35ab7bd523a834608adabf03cc5  spi_read
    f4f5d1d0a4fef487037cdb3f1be0f9aab68ca32f2dbe8782c927f03adf623ec3  spi_test
    
    * MX25L3233F OTP uses 1 region of size of 4096 bits
    
    $ git diff -U1
    diff --git a/arch/arm/boot/dts/ti/omap/BB-SPINOR-00A0.dtso b/arch/arm/boot/dts/ti/omap/BB-SPINOR-00A0.dtso
    index 333a6b9556ed..adaddfaa8091 100644
    --- a/arch/arm/boot/dts/ti/omap/BB-SPINOR-00A0.dtso
    +++ b/arch/arm/boot/dts/ti/omap/BB-SPINOR-00A0.dtso
    @@ -63,6 +63,8 @@ channel@0 {
                    compatible = "jedec,spi-nor";
    +               otp-len = <512>;
    +               otp-n-regions = <1>;
    
    # flash_otp_info -u /dev/mtd0
    Number of OTP user blocks on /dev/mtd0: 1
    block  0:  offset = 0x0000  size = 512 bytes  [unlocked]
    
    # flash_otp_dump -u /dev/mtd0
    OTP user data for /dev/mtd0
    0x0000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x0010: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x0020: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x0030: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x0040: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x0050: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x0060: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x0070: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x0080: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x0090: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x00a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x00b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x00c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x00d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x00e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x00f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x0100: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x0110: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x0120: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x0130: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x0140: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x0150: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x0160: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x0170: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x0180: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x0190: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x01a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x01b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x01c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x01d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x01e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    0x01f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
    
    * Macronix OTP does not support erase!
    * flash_otp_erase
    
    # printf '\xde\xad%.0s' {1..256} | flash_otp_write -u /dev/mtd0 0
    Writing OTP user data on /dev/mtd0 at offset 0x0
    Wrote 512 bytes of OTP user data
    
    # flash_otp_dump -u /dev/mtd0
    OTP user data for /dev/mtd0
    0x0000: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0010: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0020: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0030: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0040: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0050: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0060: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0070: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0080: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0090: de ad de ad de ad de ad de ad de ad de ad de ad
    0x00a0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x00b0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x00c0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x00d0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x00e0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x00f0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0100: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0110: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0120: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0130: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0140: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0150: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0160: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0170: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0180: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0190: de ad de ad de ad de ad de ad de ad de ad de ad
    0x01a0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x01b0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x01c0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x01d0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x01e0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x01f0: de ad de ad de ad de ad de ad de ad de ad de ad
    
    # flash_otp_info -u /dev/mtd0
    Number of OTP user blocks on /dev/mtd0: 1
    block  0:  offset = 0x0000  size = 512 bytes  [unlocked]
    
    # printf '\xff\xff%.0s' {1..256} | flash_otp_write -u /dev/mtd0 0
    Writing OTP user data on /dev/mtd0 at offset 0x0
    Wrote 512 bytes of OTP user data
    
    # We cannot overwrite '0' with '1'!
    
    # flash_otp_dump -u /dev/mtd0
    OTP user data for /dev/mtd0
    0x0000: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0010: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0020: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0030: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0040: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0050: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0060: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0070: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0080: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0090: de ad de ad de ad de ad de ad de ad de ad de ad
    0x00a0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x00b0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x00c0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x00d0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x00e0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x00f0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0100: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0110: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0120: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0130: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0140: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0150: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0160: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0170: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0180: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0190: de ad de ad de ad de ad de ad de ad de ad de ad
    0x01a0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x01b0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x01c0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x01d0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x01e0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x01f0: de ad de ad de ad de ad de ad de ad de ad de ad
    
    # flash_otp_lock -u /dev/mtd0 0 512
    About to lock OTP user data on /dev/mtd0 from 0x0 to 0x200
    flash_otp_lock: Are you sure? (y/N) y
    Done.
    
    # flash_otp_info -u /dev/mtd0
    Number of OTP user blocks on /dev/mtd0: 1
    block  0:  offset = 0x0000  size = 512 bytes  [locked]
    
    # printf '\x00\x00%.0s' {1..256} | flash_otp_write -u /dev/mtd0 0
    Writing OTP user data on /dev/mtd0 at offset 0x0
    write(): Read-only file system
    
    # flash_otp_dump -u /dev/mtd0
    OTP user data for /dev/mtd0
    0x0000: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0010: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0020: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0030: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0040: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0050: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0060: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0070: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0080: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0090: de ad de ad de ad de ad de ad de ad de ad de ad
    0x00a0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x00b0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x00c0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x00d0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x00e0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x00f0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0100: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0110: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0120: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0130: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0140: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0150: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0160: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0170: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0180: de ad de ad de ad de ad de ad de ad de ad de ad
    0x0190: de ad de ad de ad de ad de ad de ad de ad de ad
    0x01a0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x01b0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x01c0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x01d0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x01e0: de ad de ad de ad de ad de ad de ad de ad de ad
    0x01f0: de ad de ad de ad de ad de ad de ad de ad de ad
    
    * Test that mimic old Macronix chip lack SFDP.
    * In order to check how will the driver cope with an old Macronix chip.
    * As we do not posses such an old chip, we will change RDSFDP to an unused opcode.
    
    $ git diff -U0
    diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
    index 4ebc527aadc1..784cba9b2d0d 100644
    --- a/include/linux/mtd/spi-nor.h
    +++ b/include/linux/mtd/spi-nor.h
    @@ -47 +47 @@
    -#define SPINOR_OP_RDSFDP       0x5a    /* Read SFDP */
    +#define SPINOR_OP_RDSFDP       0x57    /* Read SFDP */
    
    # dmesg | grep spi
    [   42.436974] spi-nor spi0.0: mx25l3205d (4096 Kbytes)
    
    * No error in kernel log!
    
    # ls /sys/bus/spi/devices/spi0.0/spi-nor/
    jedec_id  manufacturer  partname
    
    * No SFDP, as expected!
    
    $ cat /sys/kernel/debug/spi-nor/spi0.0/capabilities
    Supported read modes by the flash
     1S-1S-1S
      opcode        0x03
      mode cycles   0
      dummy cycles  0
    
    Supported page program modes by the flash
     1S-1S-1S
      opcode        0x02
    
    $ cat /sys/kernel/debug/spi-nor/spi0.0/params
    name            mx25l3205d
    id              c2 20 16 c2 20 16
    size            4.00 MiB
    write size      1
    page size       256
    address nbytes  3
    flags           HAS_16BIT_SR
    
    opcodes
     read           0x03
      dummy cycles  0
     erase          0x20
     program        0x02
     8D extension   none
    
    protocols
     read           1S-1S-1S
     write          1S-1S-1S
     register       1S-1S-1S
    
    erase commands
     20 (4.00 KiB) [0]
     d8 (64.0 KiB) [1]
     c7 (4.00 MiB)
    
    sector map
     region (in hex)   | erase mask | flags
     ------------------+------------+----------
     00000000-003fffff |     [01  ] |

 drivers/mtd/spi-nor/macronix.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
index bee62e9051d6..5a69d5a48033 100644
--- a/drivers/mtd/spi-nor/macronix.c
+++ b/drivers/mtd/spi-nor/macronix.c
@@ -375,4 +375,5 @@ const struct spi_nor_manufacturer spi_nor_macronix = {
 	.parts = macronix_nor_parts,
 	.nparts = ARRAY_SIZE(macronix_nor_parts),
 	.fixups = &macronix_nor_fixups,
+	.flags = SPI_NOR_MANUFACT_TRY_SFDP | SPI_NOR_MANUFACT_DT_OTP,
 };
-- 
2.39.5