[PATCH] spi-nor: sfdp: Allow configuring unknown flashes using SFDP

[PATCH] spi-nor: sfdp: Allow configuring unknown flashes using SFDP

[Petr Malat]

This change allows adding a support for flashes with correct SFDP
without recompilation of the kernel by setting sfdp-compatible property
in their node. Alternatively, sfdp_compatible module option can be used
to list JEDEC IDs of flashes, whose SFDP can be trusted. Star "*" can
be used to match all JEDEC IDs.

Signed-off-by: Petr Malat <oss@malat.biz>
---
 .../devicetree/bindings/mtd/jedec,spi-nor.txt |  3 +
 drivers/mtd/spi-nor/core.c                    | 86 +++++++++++--------
 drivers/mtd/spi-nor/core.h                    |  4 +
 drivers/mtd/spi-nor/sfdp.c                    | 66 ++++++++++++++
 drivers/mtd/spi-nor/sfdp.h                    |  2 +
 5 files changed, 126 insertions(+), 35 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
index f03be904d3c2..b641b3e24a07 100644
--- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
+++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt
@@ -78,6 +78,9 @@ Optional properties:
 		   cannot reboot properly if the flash is left in the "wrong"
 		   state. This boolean flag can be used on such systems, to
 		   denote the absence of a reliable reset mechanism.
+- sfdp-compatible : Trust information in SFDP and instantiate the device even if
+		   its JEDEC ID isn't known to the driver. All the configuration
+		   is read from SFDP and no workarounds are applied.
 
 Example:
 
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 20df44b753da..47f8108e971e 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2193,7 +2193,8 @@ spi_nor_search_part_by_id(const struct flash_info *parts, unsigned int nparts,
 	return NULL;
 }
 
-static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
+static const struct flash_info *spi_nor_read_id(struct spi_nor *nor,
+						bool autodetect)
 {
 	const struct flash_info *info;
 	u8 *id = nor->bouncebuf;
@@ -2227,8 +2228,15 @@ static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
 		}
 	}
 
-	dev_err(nor->dev, "unrecognized JEDEC id bytes: %*ph\n",
-		SPI_NOR_MAX_ID_LEN, id);
+	if (autodetect) {
+		info = spi_nor_sfdp_generic_info(nor, id);
+		if (!IS_ERR_OR_NULL(info))
+			return info;
+	}
+
+	dev_warn(nor->dev, "unrecognized JEDEC id bytes: %*ph\n",
+		 SPI_NOR_MAX_ID_LEN, id);
+
 	return ERR_PTR(-ENODEV);
 }
 
@@ -2849,19 +2857,25 @@ static void spi_nor_manufacturer_init_params(struct spi_nor *nor)
  * @nor:	pointer to a 'struct spi_nor'.
  *
  * The method has a roll-back mechanism: in case the SFDP parsing fails, the
- * legacy flash parameters and settings will be restored.
+ * legacy flash parameters and settings will be restored unless configuration
+ * based solely on SFDP was requested.
  */
-static void spi_nor_sfdp_init_params(struct spi_nor *nor)
+static int spi_nor_sfdp_init_params(struct spi_nor *nor)
 {
 	struct spi_nor_flash_parameter sfdp_params;
+	int rtn;
 
 	memcpy(&sfdp_params, nor->params, sizeof(sfdp_params));
 
-	if (spi_nor_parse_sfdp(nor, nor->params)) {
+	rtn = spi_nor_parse_sfdp(nor, nor->params);
+	if (rtn && !(nor->info->flags & SPI_NOR_SFDP_AUTODETECT)) {
 		memcpy(nor->params, &sfdp_params, sizeof(*nor->params));
 		nor->addr_width = 0;
 		nor->flags &= ~SNOR_F_4B_OPCODES;
+		return 0;
 	}
+
+	return rtn;
 }
 
 /**
@@ -3051,6 +3065,8 @@ static void spi_nor_late_init_params(struct spi_nor *nor)
  */
 static int spi_nor_init_params(struct spi_nor *nor)
 {
+	int rtn;
+
 	nor->params = devm_kzalloc(nor->dev, sizeof(*nor->params), GFP_KERNEL);
 	if (!nor->params)
 		return -ENOMEM;
@@ -3060,9 +3076,13 @@ static int spi_nor_init_params(struct spi_nor *nor)
 	spi_nor_manufacturer_init_params(nor);
 
 	if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
-				 SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) &&
-	    !(nor->info->flags & SPI_NOR_SKIP_SFDP))
-		spi_nor_sfdp_init_params(nor);
+				 SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ |
+				 SPI_NOR_SFDP_AUTODETECT)) &&
+	    !(nor->info->flags & SPI_NOR_SKIP_SFDP)) {
+		rtn = spi_nor_sfdp_init_params(nor);
+		if (rtn)
+			return rtn;
+	}
 
 	spi_nor_post_sfdp_fixups(nor);
 
@@ -3349,39 +3369,35 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
 {
 	const struct flash_info *info = NULL;
 
-	if (name)
+	if (name) {
 		info = spi_nor_match_id(nor, name);
-	/* Try to auto-detect if chip name wasn't specified or not found */
-	if (!info)
-		info = spi_nor_read_id(nor);
-	if (IS_ERR_OR_NULL(info))
-		return ERR_PTR(-ENOENT);
-
-	/*
-	 * If caller has specified name of flash model that can normally be
-	 * detected using JEDEC, let's verify it.
-	 */
-	if (name && info->id_len) {
-		const struct flash_info *jinfo;
-
-		jinfo = spi_nor_read_id(nor);
-		if (IS_ERR(jinfo)) {
-			return jinfo;
-		} else if (jinfo != info) {
+		if (info) {
 			/*
-			 * JEDEC knows better, so overwrite platform ID. We
-			 * can't trust partitions any longer, but we'll let
-			 * mtd apply them anyway, since some partitions may be
-			 * marked read-only, and we don't want to lose that
+			 * If caller has specified name of flash model that can
+			 * normally be detected using JEDEC, we revert to JEDEC
+			 * detection. If different flash is detected, we can't
+			 * trust partitions any longer, but we'll let mtd apply
+			 * them anyway, since some partitions may be marked
+			 * read-only, and we don't want to lose that
 			 * information, even if it's not 100% accurate.
 			 */
-			dev_warn(nor->dev, "found %s, expected %s\n",
-				 jinfo->name, info->name);
-			info = jinfo;
+			if (info->id_len) {
+				const struct flash_info *jinfo;
+
+				jinfo = spi_nor_read_id(nor, false);
+				if (jinfo != info) {
+					dev_warn(nor->dev,
+						 "found %s, expected %s\n",
+						 jinfo->name, info->name);
+					info = jinfo;
+				}
+			}
+			return info;
 		}
 	}
 
-	return info;
+	/* Try to auto-detect if chip name wasn't specified or not found */
+	return spi_nor_read_id(nor, true);
 }
 
 int spi_nor_scan(struct spi_nor *nor, const char *name,
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index d631ee299de3..96742dc6a91e 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -338,6 +338,10 @@ struct flash_info {
 					 * protection bits. Usually these will
 					 * power-up in a write-protected state.
 					 */
+#define SPI_NOR_SFDP_AUTODETECT	BIT(23)	/*
+					 * Flashinfo was dynamically created and
+					 * all parameters come from SFDP
+					 */
 
 	/* Part specific fixup hooks. */
 	const struct spi_nor_fixups *fixups;
diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index 6ee7719e5903..3e5c138b0143 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -7,6 +7,7 @@
 #include <linux/bitfield.h>
 #include <linux/slab.h>
 #include <linux/sort.h>
+#include <linux/moduleparam.h>
 #include <linux/mtd/spi-nor.h>
 
 #include "core.h"
@@ -1373,3 +1374,68 @@ int spi_nor_parse_sfdp(struct spi_nor *nor,
 	kfree(param_headers);
 	return err;
 }
+
+static char *sfdp_compatible[8];
+static int num_sfdp_compatible;
+module_param_array(sfdp_compatible, charp, &num_sfdp_compatible, 0444);
+MODULE_PARM_DESC(sfdp_compatible, "JEDEC IDs of devices with trusted SFDP which"
+		" the kernel instantiates without them being explicitly listed "
+		"in the driver. Use * to trust SFDP of all devices.");
+
+static bool spi_nor_is_sfdp_trusted(struct spi_nor *nor, const u8 *id)
+{
+	struct device_node *np = spi_nor_get_flash_node(nor);
+	char strid[SPI_NOR_MAX_ID_LEN * 2 + 1];
+	int i;
+
+	if (np && of_property_read_bool(np, "sfdp-compatible"))
+		return 1;
+
+	snprintf(strid, sizeof(strid), "%*phN", SPI_NOR_MAX_ID_LEN, id);
+
+	for (i = 0; i < num_sfdp_compatible; i++) {
+		if (!strcmp("*", sfdp_compatible[i]))
+			return 1;
+		if (!strcmp(strid, sfdp_compatible[i]))
+			return 1;
+	}
+
+	return 0;
+}
+
+struct flash_info *spi_nor_sfdp_generic_info(struct spi_nor *nor, const u8 *id)
+{
+	struct flash_info *info;
+	__le32 sfdp_sig = 0;
+	int err;
+
+	if (!spi_nor_is_sfdp_trusted(nor, id)) {
+		dev_info(nor->dev, "SFDP configuration of JEDEC id %*ph is not"
+				" enabled\n", SPI_NOR_MAX_ID_LEN, id);
+		return ERR_PTR(-ENODEV);
+	}
+
+	/* Sanity check - read SFDP_SIGNATURE */
+	err = spi_nor_read_sfdp_dma_unsafe(nor, 0, sizeof(sfdp_sig), &sfdp_sig);
+	if (err || le32_to_cpu(sfdp_sig) != SFDP_SIGNATURE) {
+		dev_err(nor->dev, "Failed to read SFDP signature: %d\n", err);
+		return ERR_PTR(err ?: -ENODEV);
+	}
+
+	/* Allocate info */
+	info = devm_kzalloc(nor->dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return ERR_PTR(-ENOMEM);
+
+	memcpy(info->id, id, SPI_NOR_MAX_ID_LEN);
+	info->id_len = SPI_NOR_MAX_ID_LEN;
+	info->name = "sfdp-compatible";
+	info->flags = SPI_NOR_SFDP_AUTODETECT;
+	info->page_size = 256;
+	nor->info = info;
+
+	dev_info(nor->dev, "Using SFDP to configure JEDEC id %*ph\n",
+		 SPI_NOR_MAX_ID_LEN, id);
+
+	return info;
+}
diff --git a/drivers/mtd/spi-nor/sfdp.h b/drivers/mtd/spi-nor/sfdp.h
index 89152ae1cf3e..e9f6c4abd0c3 100644
--- a/drivers/mtd/spi-nor/sfdp.h
+++ b/drivers/mtd/spi-nor/sfdp.h
@@ -110,4 +110,6 @@ struct sfdp_parameter_header {
 int spi_nor_parse_sfdp(struct spi_nor *nor,
 		       struct spi_nor_flash_parameter *params);
 
+struct flash_info *spi_nor_sfdp_generic_info(struct spi_nor *nor, const u8 *id);
+
 #endif /* __LINUX_MTD_SFDP_H */
-- 
2.20.1


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

Re: [PATCH] spi-nor: sfdp: Allow configuring unknown flashes using SFDP

[Pratyush Yadav]

On 20/05/21 06:07PM, Petr Malat wrote:
> This change allows adding a support for flashes with correct SFDP
> without recompilation of the kernel by setting sfdp-compatible property
> in their node. Alternatively, sfdp_compatible module option can be used
> to list JEDEC IDs of flashes, whose SFDP can be trusted. Star "*" can
> be used to match all JEDEC IDs.

I have skimmed through the patch. Before I look at it more closely, I 
want to understand the use case for this patch. Why would you not want 
to recompile the kernel when adding support for new hardware? Do you 
want the ability to support flashes on devices that have already been 
deployed in the field? Is it something that comes up frequently?

Then comes the question of how do you get the flash to probe if it is 
not present in device tree? And if you are able to update the device 
tree then you should also be able to update the kernel.

Have you tested this with any flash? If so, with which one?

> 
> Signed-off-by: Petr Malat <oss@malat.biz>
> ---
>  .../devicetree/bindings/mtd/jedec,spi-nor.txt |  3 +

This file has been converted to yaml since at least 5.12-rc1. Please 
update your kernel sources and rebase your patches.

>  drivers/mtd/spi-nor/core.c                    | 86 +++++++++++--------
>  drivers/mtd/spi-nor/core.h                    |  4 +
>  drivers/mtd/spi-nor/sfdp.c                    | 66 ++++++++++++++
>  drivers/mtd/spi-nor/sfdp.h                    |  2 +
>  5 files changed, 126 insertions(+), 35 deletions(-)

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

Re: [PATCH] spi-nor: sfdp: Allow configuring unknown flashes using SFDP

[Petr Malat]

Hi!
On Fri, May 21, 2021 at 03:25:05PM +0530, Pratyush Yadav wrote:
> On 20/05/21 06:07PM, Petr Malat wrote:
> > This change allows adding a support for flashes with correct SFDP
> > without recompilation of the kernel by setting sfdp-compatible property
> > in their node. Alternatively, sfdp_compatible module option can be used
> > to list JEDEC IDs of flashes, whose SFDP can be trusted. Star "*" can
> > be used to match all JEDEC IDs.
> 
> I have skimmed through the patch. Before I look at it more closely, I 
> want to understand the use case for this patch. Why would you not want 
> to recompile the kernel when adding support for new hardware? Do you 
> want the ability to support flashes on devices that have already been 
> deployed in the field? Is it something that comes up frequently?
In my case the kernel is loaded from a USB mass storage device, which
can be preproduced and on stock (with the kernel already on it). With
my patch I can change the flash vendor without the need of updating
the image on already existing USB mass storage devices.

The patch is also useful for people who use distribution kernel as they
will not have to wait until (and if) the distribution updates it.

> Then comes the question of how do you get the flash to probe if it is 
> not present in device tree? And if you are able to update the device 
> tree then you should also be able to update the kernel.
If one sets sfdp-compatible flag in the device tree and replaces one
flash with another, he doesn't need to update the device tree. It
extends what is already possible now - if I make "jedec,spi-nor" node
and the JEDEC ID is known to the driver I can replace the flash with
a different one whose JEDEC ID is known as well and it will work
without touching the device tree.

> Have you tested this with any flash? If so, with which one?
I have tested it with n25q128a11 by removing it from the driver.

> > 
> > Signed-off-by: Petr Malat <oss@malat.biz>
> > ---
> >  .../devicetree/bindings/mtd/jedec,spi-nor.txt |  3 +
> 
> This file has been converted to yaml since at least 5.12-rc1. Please 
> update your kernel sources and rebase your patches.
OK, I will rebase.
  Petr

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

Re: [PATCH] spi-nor: sfdp: Allow configuring unknown flashes using SFDP

[Vignesh Raghavendra]

On 5/21/21 5:23 PM, Petr Malat wrote:
> Hi!
> On Fri, May 21, 2021 at 03:25:05PM +0530, Pratyush Yadav wrote:
>> On 20/05/21 06:07PM, Petr Malat wrote:
>>> This change allows adding a support for flashes with correct SFDP
>>> without recompilation of the kernel by setting sfdp-compatible property
>>> in their node. Alternatively, sfdp_compatible module option can be used
>>> to list JEDEC IDs of flashes, whose SFDP can be trusted. Star "*" can
>>> be used to match all JEDEC IDs.
>>
>> I have skimmed through the patch. Before I look at it more closely, I 
>> want to understand the use case for this patch. Why would you not want 
>> to recompile the kernel when adding support for new hardware? Do you 
>> want the ability to support flashes on devices that have already been 
>> deployed in the field? Is it something that comes up frequently?
> In my case the kernel is loaded from a USB mass storage device, which
> can be preproduced and on stock (with the kernel already on it). With
> my patch I can change the flash vendor without the need of updating
> the image on already existing USB mass storage devices.
> 
> The patch is also useful for people who use distribution kernel as they
> will not have to wait until (and if) the distribution updates it.

Don't need separate DT property or cmdline argument. There is no way to
know if the SFDP tables are 100% valid when kernel is working with a
"generic flash".
Flashes are often replaced with newer revisions of the part that may/may
not have valid SFDP tables.

So just rely on SFDP, when no valid entry is found in the manufacturer's
flash_info[] while printing out a warning to user that this is just best
effort support.

Regards
Vignesh



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

Re: [PATCH] spi-nor: sfdp: Allow configuring unknown flashes using SFDP

[Pratyush Yadav]

On 27/05/21 07:22PM, Vignesh Raghavendra wrote:
> 
> 
> On 5/21/21 5:23 PM, Petr Malat wrote:
> > Hi!
> > On Fri, May 21, 2021 at 03:25:05PM +0530, Pratyush Yadav wrote:
> >> On 20/05/21 06:07PM, Petr Malat wrote:
> >>> This change allows adding a support for flashes with correct SFDP
> >>> without recompilation of the kernel by setting sfdp-compatible property
> >>> in their node. Alternatively, sfdp_compatible module option can be used
> >>> to list JEDEC IDs of flashes, whose SFDP can be trusted. Star "*" can
> >>> be used to match all JEDEC IDs.
> >>
> >> I have skimmed through the patch. Before I look at it more closely, I 
> >> want to understand the use case for this patch. Why would you not want 
> >> to recompile the kernel when adding support for new hardware? Do you 
> >> want the ability to support flashes on devices that have already been 
> >> deployed in the field? Is it something that comes up frequently?
> > In my case the kernel is loaded from a USB mass storage device, which
> > can be preproduced and on stock (with the kernel already on it). With
> > my patch I can change the flash vendor without the need of updating
> > the image on already existing USB mass storage devices.
> > 
> > The patch is also useful for people who use distribution kernel as they
> > will not have to wait until (and if) the distribution updates it.
> 
> Don't need separate DT property or cmdline argument. There is no way to
> know if the SFDP tables are 100% valid when kernel is working with a
> "generic flash".
> Flashes are often replaced with newer revisions of the part that may/may
> not have valid SFDP tables.
> 
> So just rely on SFDP, when no valid entry is found in the manufacturer's
> flash_info[] while printing out a warning to user that this is just best
> effort support.

+1

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

Re: [PATCH] spi-nor: sfdp: Allow configuring unknown flashes using SFDP

[Petr Malat]

On Thu, May 27, 2021 at 07:22:19PM +0530, Vignesh Raghavendra wrote:
> >>> This change allows adding a support for flashes with correct SFDP
> >>> without recompilation of the kernel by setting sfdp-compatible property
> >>> in their node. Alternatively, sfdp_compatible module option can be used
> >>> to list JEDEC IDs of flashes, whose SFDP can be trusted. Star "*" can
> >>> be used to match all JEDEC IDs.
> >>
> >> I have skimmed through the patch. Before I look at it more closely, I 
> >> want to understand the use case for this patch. Why would you not want 
> >> to recompile the kernel when adding support for new hardware? Do you 
> >> want the ability to support flashes on devices that have already been 
> >> deployed in the field? Is it something that comes up frequently?
> > In my case the kernel is loaded from a USB mass storage device, which
> > can be preproduced and on stock (with the kernel already on it). With
> > my patch I can change the flash vendor without the need of updating
> > the image on already existing USB mass storage devices.
> > 
> > The patch is also useful for people who use distribution kernel as they
> > will not have to wait until (and if) the distribution updates it.
> 
> Don't need separate DT property or cmdline argument. There is no way to
> know if the SFDP tables are 100% valid when kernel is working with a
> "generic flash".
> Flashes are often replaced with newer revisions of the part that may/may
> not have valid SFDP tables.
> 
> So just rely on SFDP, when no valid entry is found in the manufacturer's
> flash_info[] while printing out a warning to user that this is just best
> effort support.

OK, I will rework it to fallback to SFDP by default. I put the safeguard
there to avoid a case when one has a flash requiring workarounds and then
does a kernel downgrade to a release where the flash is not known and uses
it. Should I make an opposite flag sfdp-incompatible or do we consider this
a theoretical scenario not worth of addressing in the code?
  Petr

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

Re: [PATCH] spi-nor: sfdp: Allow configuring unknown flashes using SFDP

[Vignesh Raghavendra]

On 5/27/21 9:15 PM, Petr Malat wrote:
> On Thu, May 27, 2021 at 07:22:19PM +0530, Vignesh Raghavendra wrote:
>>>>> This change allows adding a support for flashes with correct SFDP
>>>>> without recompilation of the kernel by setting sfdp-compatible property
>>>>> in their node. Alternatively, sfdp_compatible module option can be used
>>>>> to list JEDEC IDs of flashes, whose SFDP can be trusted. Star "*" can
>>>>> be used to match all JEDEC IDs.
>>>>
>>>> I have skimmed through the patch. Before I look at it more closely, I 
>>>> want to understand the use case for this patch. Why would you not want 
>>>> to recompile the kernel when adding support for new hardware? Do you 
>>>> want the ability to support flashes on devices that have already been 
>>>> deployed in the field? Is it something that comes up frequently?
>>> In my case the kernel is loaded from a USB mass storage device, which
>>> can be preproduced and on stock (with the kernel already on it). With
>>> my patch I can change the flash vendor without the need of updating
>>> the image on already existing USB mass storage devices.
>>>
>>> The patch is also useful for people who use distribution kernel as they
>>> will not have to wait until (and if) the distribution updates it.
>>
>> Don't need separate DT property or cmdline argument. There is no way to
>> know if the SFDP tables are 100% valid when kernel is working with a
>> "generic flash".
>> Flashes are often replaced with newer revisions of the part that may/may
>> not have valid SFDP tables.
>>
>> So just rely on SFDP, when no valid entry is found in the manufacturer's
>> flash_info[] while printing out a warning to user that this is just best
>> effort support.
> 
> OK, I will rework it to fallback to SFDP by default. I put the safeguard
> there to avoid a case when one has a flash requiring workarounds and then
> does a kernel downgrade to a release where the flash is not known and uses
> it. Should I make an opposite flag sfdp-incompatible or do we consider this
> a theoretical scenario not worth of addressing in the code?
>  

We can guarantee new kernels don't regress wrt older version on
upgrading. But anyone downgrading kernel should expect some HW support
to be missing (not just SPI flash but many other things in general).
I don't see a need to work about such scenario

Regards
Vignesh

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