[PATCH v2 0/7] mtd: spi-nor: move manuf out of the core - batch 1

[PATCH v2 0/7] mtd: spi-nor: move manuf out of the core - batch 1

[Tudor.Ambarus]

From: Tudor Ambarus <tudor.ambarus@microchip.com>

Depends on 'mtd: spi-nor: move manuf out of the core - batch 0' series:
https://patchwork.ozlabs.org/project/linux-mtd/list/?series=127030

v2:
- addressed all the comments
- all flash parameters and settings are now set in 'struct
  spi_nor_flash_parameter', for a clearer separation between the SPI NOR
  layer and the flash params.

The scope of the "mtd: spi-nor: move manuf out of the core" batches,
is to move all manufacturer specific code out of the spi-nor core.

In the quest of removing the manufacturer specific code from the spi-nor
core, we want to impose a timeline/priority on how the flash parameters
are updated. As of now. the flash parameters initialization logic is as
following:

    a/ default flash parameters init in spi_nor_init_params()
    b/ manufacturer specific flash parameters updates, split across entire
       spi-nor core code
    c/ flash parameters updates based on SFDP tables
    d/ post BFPT flash parameter updates

With the "mtd: spi-nor: move manuf out of the core" batches, we want to
impose the following sequence of calls:

    1/ spi-nor core legacy flash parameters init:
            spi_nor_default_init_params()

    2/ MFR-based manufacturer flash parameters init:
            nor->manufacturer->fixups->default_init()

    3/ specific flash_info tweeks done when decisions can not be done just
       on MFR:
            nor->info->fixups->default_init()

    4/ SFDP tables flash parameters init - SFDP knows better:
            spi_nor_sfdp_init_params()

    5/ post SFDP tables flash parameters updates - in case manufacturers
       get the serial flash tables wrong or incomplete.
            nor->info->fixups->post_sfdp()
       The later can be extended to nor->manufacturer->fixups->post_sfdp()
       if needed.

Setting of flash parameters will no longer be spread interleaved across
the spi-nor core, there will be a clear separation on who and when will
update the flash parameters.

Tested on sst26vf064b with atmel-quadspi SPIMEM driver.

Boris Brezillon (3):
  mtd: spi-nor: Add a default_init() fixup hook for gd25q256
  mtd: spi-nor: Create a ->set_4byte() method
  mtd: spi-nor: Rework the SPI NOR lock/unlock logic

Tudor Ambarus (4):
  mtd: spi-nor: Add default_init() hook to tweak flash parameters
  mtd: spi_nor: Move manufacturer quad_enable() in ->default_init()
  mtd: spi-nor: Split spi_nor_init_params()
  mtd: spi-nor: Rework the disabling of block write protection

 drivers/mtd/spi-nor/spi-nor.c | 320 ++++++++++++++++++++++++++++--------------
 include/linux/mtd/spi-nor.h   |  25 +++-
 2 files changed, 233 insertions(+), 112 deletions(-)

-- 
2.9.5


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

[PATCH v2 1/7] mtd: spi-nor: Add default_init() hook to tweak flash parameters

[Tudor.Ambarus]

From: Tudor Ambarus <tudor.ambarus@microchip.com>

As of now, the flash parameters initialization logic is as following:

a/ default flash parameters init in spi_nor_init_params()
b/ manufacturer specific flash parameters updates, split across entire
   spi-nor core code
c/ flash parameters updates based on SFDP tables
d/ post BFPT flash parameter updates

In the quest of removing the manufacturer specific code from the spi-nor
core, we want to impose a timeline/priority on how the flash parameters
are updated. The following sequence of calls is pursued:

1/ spi-nor core legacy flash parameters init:
	spi_nor_default_init_params()

2/ MFR-based manufacturer flash parameters init:
	nor->manufacturer->fixups->default_init()

3/ specific flash_info tweeks done when decisions can not be done just on
   MFR:
	nor->info->fixups->default_init()

4/ SFDP tables flash parameters init - SFDP knows better:
	spi_nor_sfdp_init_params()

5/ post SFDP tables flash parameters updates - in case manufacturers get
   the serial flash tables wrong or incomplete.
	nor->info->fixups->post_sfdp()
   The later can be extended to nor->manufacturer->fixups->post_sfdp() if
   needed.

This patch opens doors for steps 2/ and 3/.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 7c02eddad9fd..016bfe2fb592 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -154,12 +154,16 @@ struct sfdp_bfpt {
 
 /**
  * struct spi_nor_fixups - SPI NOR fixup hooks
+ * @default_init: called after default flash parameters init. Used to tweak
+ *                flash parameters when information provided by the flash_info
+ *                table is incomplete or wrong.
  * @post_bfpt: called after the BFPT table has been parsed
  *
  * Those hooks can be used to tweak the SPI NOR configuration when the SFDP
  * table is broken or not available.
  */
 struct spi_nor_fixups {
+	void (*default_init)(struct spi_nor *nor);
 	int (*post_bfpt)(struct spi_nor *nor,
 			 const struct sfdp_parameter_header *bfpt_header,
 			 const struct sfdp_bfpt *bfpt,
@@ -4133,6 +4137,17 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
 	return err;
 }
 
+/**
+ * spi_nor_manufacturer_init_params() - Initialize the flash's parameters and
+ * settings based on ->default_init() hook.
+ * @nor:	pointer to a 'struct spi-nor'.
+ */
+static void spi_nor_manufacturer_init_params(struct spi_nor *nor)
+{
+	if (nor->info->fixups && nor->info->fixups->default_init)
+		nor->info->fixups->default_init(nor);
+}
+
 static int spi_nor_init_params(struct spi_nor *nor)
 {
 	struct spi_nor_flash_parameter *params = &nor->params;
@@ -4233,6 +4248,8 @@ static int spi_nor_init_params(struct spi_nor *nor)
 			params->quad_enable = info->quad_enable;
 	}
 
+	spi_nor_manufacturer_init_params(nor);
+
 	if ((info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
 	    !(info->flags & SPI_NOR_SKIP_SFDP)) {
 		struct spi_nor_flash_parameter sfdp_params;
-- 
2.9.5


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

[PATCH v2 2/7] mtd: spi-nor: Add a default_init() fixup hook for gd25q256

[Tudor.Ambarus]

From: Boris Brezillon <boris.brezillon@bootlin.com>

gd25q256 needs to tweak the ->quad_enable() implementation and the
->default_init() fixup hook is the perfect place to do that. This way,
if we ever need to tweak more things for this flash, we won't have to
add new fields in flash_info.

We can get rid of the flash_info->quad_enable field as gd25q256 was
the only user.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
[tudor.ambarus@microchip.com: use ->default_init() hook instead of
->post_sfdp()]
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 016bfe2fb592..27951e5a01e2 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -222,8 +222,6 @@ struct flash_info {
 
 	/* Part specific fixup hooks. */
 	const struct spi_nor_fixups *fixups;
-
-	int	(*quad_enable)(struct spi_nor *nor);
 };
 
 #define JEDEC_MFR(info)	((info)->id[0])
@@ -2126,6 +2124,21 @@ static struct spi_nor_fixups mx25l25635_fixups = {
 	.post_bfpt = mx25l25635_post_bfpt_fixups,
 };
 
+static void gd25q256_default_init(struct spi_nor *nor)
+{
+	/*
+	 * Some manufacturer like GigaDevice may use different
+	 * bit to set QE on different memories, so the MFR can't
+	 * indicate the quad_enable method for this case, we need
+	 * to set it in the default_init fixup hook.
+	 */
+	nor->params.quad_enable = macronix_quad_enable;
+}
+
+static struct spi_nor_fixups gd25q256_fixups = {
+	.default_init = gd25q256_default_init,
+};
+
 /* NOTE: double check command sets and memory organization when you add
  * more nor chips.  This current list focusses on newer chips, which
  * have been converging on command sets which including JEDEC ID.
@@ -2218,7 +2231,7 @@ static const struct flash_info spi_nor_ids[] = {
 		"gd25q256", INFO(0xc84019, 0, 64 * 1024, 512,
 			SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
 			SPI_NOR_4B_OPCODES | SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB)
-			.quad_enable = macronix_quad_enable,
+			.fixups = &gd25q256_fixups,
 	},
 
 	/* Intel/Numonyx -- xxxs33b */
@@ -4237,15 +4250,6 @@ static int spi_nor_init_params(struct spi_nor *nor)
 			params->quad_enable = spansion_quad_enable;
 			break;
 		}
-
-		/*
-		 * Some manufacturer like GigaDevice may use different
-		 * bit to set QE on different memories, so the MFR can't
-		 * indicate the quad_enable method for this case, we need
-		 * set it in flash info list.
-		 */
-		if (info->quad_enable)
-			params->quad_enable = info->quad_enable;
 	}
 
 	spi_nor_manufacturer_init_params(nor);
-- 
2.9.5


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

[PATCH v2 3/7] mtd: spi_nor: Move manufacturer quad_enable() in ->default_init()

[Tudor.Ambarus]

From: Tudor Ambarus <tudor.ambarus@microchip.com>

The goal is to move the quad_enable manufacturer specific init in the
nor->manufacturer->fixups->default_init()

The legacy quad_enable() implementation is spansion_quad_enable(),
select this method by default.

Set specific manufacturer fixups->default_init() hooks to overwrite
the default quad_enable() implementation when needed.

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

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 27951e5a01e2..c9514dfd7d6d 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -4150,13 +4150,38 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
 	return err;
 }
 
+static void macronix_set_default_init(struct spi_nor *nor)
+{
+	nor->params.quad_enable = macronix_quad_enable;
+}
+
+static void st_micron_set_default_init(struct spi_nor *nor)
+{
+	nor->params.quad_enable = NULL;
+}
+
 /**
  * spi_nor_manufacturer_init_params() - Initialize the flash's parameters and
- * settings based on ->default_init() hook.
+ * settings based on MFR register and ->default_init() hook.
  * @nor:	pointer to a 'struct spi-nor'.
  */
 static void spi_nor_manufacturer_init_params(struct spi_nor *nor)
 {
+	/* Init flash parameters based on MFR */
+	switch (JEDEC_MFR(nor->info)) {
+	case SNOR_MFR_MACRONIX:
+		macronix_set_default_init(nor);
+		break;
+
+	case SNOR_MFR_ST:
+	case SNOR_MFR_MICRON:
+		st_micron_set_default_init(nor);
+		break;
+
+	default:
+		break;
+	}
+
 	if (nor->info->fixups && nor->info->fixups->default_init)
 		nor->info->fixups->default_init(nor);
 }
@@ -4168,6 +4193,9 @@ static int spi_nor_init_params(struct spi_nor *nor)
 	const struct flash_info *info = nor->info;
 	u8 i, erase_mask;
 
+	/* Initialize legacy flash parameters and settings. */
+	params->quad_enable = spansion_quad_enable;
+
 	/* Set SPI NOR sizes. */
 	params->size = (u64)info->sector_size * info->n_sectors;
 	params->page_size = info->page_size;
@@ -4233,24 +4261,6 @@ static int spi_nor_init_params(struct spi_nor *nor)
 			       SPINOR_OP_SE);
 	spi_nor_init_uniform_erase_map(map, erase_mask, params->size);
 
-	/* Select the procedure to set the Quad Enable bit. */
-	if (params->hwcaps.mask & (SNOR_HWCAPS_READ_QUAD |
-				   SNOR_HWCAPS_PP_QUAD)) {
-		switch (JEDEC_MFR(info)) {
-		case SNOR_MFR_MACRONIX:
-			params->quad_enable = macronix_quad_enable;
-			break;
-
-		case SNOR_MFR_ST:
-		case SNOR_MFR_MICRON:
-			break;
-
-		default:
-			/* Kept only for backward compatibility purpose. */
-			params->quad_enable = spansion_quad_enable;
-			break;
-		}
-	}
 
 	spi_nor_manufacturer_init_params(nor);
 
-- 
2.9.5


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

Re: [PATCH v2 3/7] mtd: spi_nor: Move manufacturer quad_enable() in ->default_init()

[Boris Brezillon]

On Sat, 24 Aug 2019 12:00:41 +0000
<Tudor.Ambarus@microchip.com> wrote:

> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> The goal is to move the quad_enable manufacturer specific init in the
> nor->manufacturer->fixups->default_init()
> 
> The legacy quad_enable() implementation is spansion_quad_enable(),
> select this method by default.
> 
> Set specific manufacturer fixups->default_init() hooks to overwrite
> the default quad_enable() implementation when needed.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 48 ++++++++++++++++++++++++++-----------------
>  1 file changed, 29 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 27951e5a01e2..c9514dfd7d6d 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -4150,13 +4150,38 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
>  	return err;
>  }
>  
> +static void macronix_set_default_init(struct spi_nor *nor)
> +{
> +	nor->params.quad_enable = macronix_quad_enable;

Since it's now supposed to be the default QE implementation I'd
recommend renaming the function into default_quad_enable() (this can be
done in a separate patch).

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> +}
> +
> +static void st_micron_set_default_init(struct spi_nor *nor)
> +{
> +	nor->params.quad_enable = NULL;
> +}
> +
>  /**
>   * spi_nor_manufacturer_init_params() - Initialize the flash's parameters and
> - * settings based on ->default_init() hook.
> + * settings based on MFR register and ->default_init() hook.
>   * @nor:	pointer to a 'struct spi-nor'.
>   */
>  static void spi_nor_manufacturer_init_params(struct spi_nor *nor)
>  {
> +	/* Init flash parameters based on MFR */
> +	switch (JEDEC_MFR(nor->info)) {
> +	case SNOR_MFR_MACRONIX:
> +		macronix_set_default_init(nor);
> +		break;
> +
> +	case SNOR_MFR_ST:
> +	case SNOR_MFR_MICRON:
> +		st_micron_set_default_init(nor);
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
>  	if (nor->info->fixups && nor->info->fixups->default_init)
>  		nor->info->fixups->default_init(nor);
>  }
> @@ -4168,6 +4193,9 @@ static int spi_nor_init_params(struct spi_nor *nor)
>  	const struct flash_info *info = nor->info;
>  	u8 i, erase_mask;
>  
> +	/* Initialize legacy flash parameters and settings. */
> +	params->quad_enable = spansion_quad_enable;
> +
>  	/* Set SPI NOR sizes. */
>  	params->size = (u64)info->sector_size * info->n_sectors;
>  	params->page_size = info->page_size;
> @@ -4233,24 +4261,6 @@ static int spi_nor_init_params(struct spi_nor *nor)
>  			       SPINOR_OP_SE);
>  	spi_nor_init_uniform_erase_map(map, erase_mask, params->size);
>  
> -	/* Select the procedure to set the Quad Enable bit. */
> -	if (params->hwcaps.mask & (SNOR_HWCAPS_READ_QUAD |
> -				   SNOR_HWCAPS_PP_QUAD)) {
> -		switch (JEDEC_MFR(info)) {
> -		case SNOR_MFR_MACRONIX:
> -			params->quad_enable = macronix_quad_enable;
> -			break;
> -
> -		case SNOR_MFR_ST:
> -		case SNOR_MFR_MICRON:
> -			break;
> -
> -		default:
> -			/* Kept only for backward compatibility purpose. */
> -			params->quad_enable = spansion_quad_enable;
> -			break;
> -		}
> -	}
>  
>  	spi_nor_manufacturer_init_params(nor);
>  


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

Re: [PATCH v2 3/7] mtd: spi_nor: Move manufacturer quad_enable() in ->default_init()

[Tudor.Ambarus]

On 08/25/2019 02:47 PM, Boris Brezillon wrote:
> External E-Mail
> 
> 
> On Sat, 24 Aug 2019 12:00:41 +0000
> <Tudor.Ambarus@microchip.com> wrote:
> 
>> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>>
>> The goal is to move the quad_enable manufacturer specific init in the
>> nor->manufacturer->fixups->default_init()
>>
>> The legacy quad_enable() implementation is spansion_quad_enable(),
>> select this method by default.
>>
>> Set specific manufacturer fixups->default_init() hooks to overwrite
>> the default quad_enable() implementation when needed.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 48 ++++++++++++++++++++++++++-----------------
>>  1 file changed, 29 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index 27951e5a01e2..c9514dfd7d6d 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -4150,13 +4150,38 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
>>  	return err;
>>  }
>>  
>> +static void macronix_set_default_init(struct spi_nor *nor)
>> +{
>> +	nor->params.quad_enable = macronix_quad_enable;
> 
> Since it's now supposed to be the default QE implementation I'd
> recommend renaming the function into default_quad_enable() (this can be
> done in a separate patch).

You are referring to spansion_quad_enable. Yes, you made a patch that stops
prefixing generic functions with a manufacturer name, will follow.
https://patchwork.ozlabs.org/patch/1009264/

> 
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> 
>> +}
>> +
>> +static void st_micron_set_default_init(struct spi_nor *nor)
>> +{
>> +	nor->params.quad_enable = NULL;
>> +}
>> +
>>  /**
>>   * spi_nor_manufacturer_init_params() - Initialize the flash's parameters and
>> - * settings based on ->default_init() hook.
>> + * settings based on MFR register and ->default_init() hook.
>>   * @nor:	pointer to a 'struct spi-nor'.
>>   */
>>  static void spi_nor_manufacturer_init_params(struct spi_nor *nor)
>>  {
>> +	/* Init flash parameters based on MFR */
>> +	switch (JEDEC_MFR(nor->info)) {
>> +	case SNOR_MFR_MACRONIX:
>> +		macronix_set_default_init(nor);
>> +		break;
>> +
>> +	case SNOR_MFR_ST:
>> +	case SNOR_MFR_MICRON:
>> +		st_micron_set_default_init(nor);
>> +		break;
>> +
>> +	default:
>> +		break;
>> +	}
>> +
>>  	if (nor->info->fixups && nor->info->fixups->default_init)
>>  		nor->info->fixups->default_init(nor);
>>  }
>> @@ -4168,6 +4193,9 @@ static int spi_nor_init_params(struct spi_nor *nor)
>>  	const struct flash_info *info = nor->info;
>>  	u8 i, erase_mask;
>>  
>> +	/* Initialize legacy flash parameters and settings. */
>> +	params->quad_enable = spansion_quad_enable;
>> +
>>  	/* Set SPI NOR sizes. */
>>  	params->size = (u64)info->sector_size * info->n_sectors;
>>  	params->page_size = info->page_size;
>> @@ -4233,24 +4261,6 @@ static int spi_nor_init_params(struct spi_nor *nor)
>>  			       SPINOR_OP_SE);
>>  	spi_nor_init_uniform_erase_map(map, erase_mask, params->size);
>>  
>> -	/* Select the procedure to set the Quad Enable bit. */
>> -	if (params->hwcaps.mask & (SNOR_HWCAPS_READ_QUAD |
>> -				   SNOR_HWCAPS_PP_QUAD)) {
>> -		switch (JEDEC_MFR(info)) {
>> -		case SNOR_MFR_MACRONIX:
>> -			params->quad_enable = macronix_quad_enable;
>> -			break;
>> -
>> -		case SNOR_MFR_ST:
>> -		case SNOR_MFR_MICRON:
>> -			break;
>> -
>> -		default:
>> -			/* Kept only for backward compatibility purpose. */
>> -			params->quad_enable = spansion_quad_enable;
>> -			break;
>> -		}
>> -	}
>>  
>>  	spi_nor_manufacturer_init_params(nor);
>>  
> 
> 
> 
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 4/7] mtd: spi-nor: Split spi_nor_init_params()

[Tudor.Ambarus]

From: Tudor Ambarus <tudor.ambarus@microchip.com>

Add functions to delimit what the chunks of code do:

static void spi_nor_init_params()
{
	spi_nor_legacy_init_params()
	spi_nor_manufacturer_init_params()
	spi_nor_sfdp_init_params()
}

Add descriptions to all methods.

spi_nor_init_params() becomes of type void, as all its children
return void.

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

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index c9514dfd7d6d..93424f914159 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -4186,7 +4186,34 @@ static void spi_nor_manufacturer_init_params(struct spi_nor *nor)
 		nor->info->fixups->default_init(nor);
 }
 
-static int spi_nor_init_params(struct spi_nor *nor)
+/**
+ * spi_nor_sfdp_init_params() - Initialize the flash's parameters and settings
+ * based on JESD216 SFDP standard.
+ * @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.
+ */
+static void spi_nor_sfdp_init_params(struct spi_nor *nor)
+{
+	struct spi_nor_flash_parameter sfdp_params;
+
+	memcpy(&sfdp_params, &nor->params, sizeof(sfdp_params));
+
+	if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
+		nor->addr_width = 0;
+		nor->flags &= ~SNOR_F_4B_OPCODES;
+	} else {
+		memcpy(&nor->params, &sfdp_params, sizeof(nor->params));
+	}
+}
+
+/**
+ * spi_nor_legacy_init_params() - Initialize the flash's parameters and settings
+ * based on nor->info data.
+ * @nor:	pointer to a 'struct spi-nor'.
+ */
+static void spi_nor_legacy_init_params(struct spi_nor *nor)
 {
 	struct spi_nor_flash_parameter *params = &nor->params;
 	struct spi_nor_erase_map *map = &params->erase_map;
@@ -4260,25 +4287,43 @@ static int spi_nor_init_params(struct spi_nor *nor)
 	spi_nor_set_erase_type(&map->erase_type[i], info->sector_size,
 			       SPINOR_OP_SE);
 	spi_nor_init_uniform_erase_map(map, erase_mask, params->size);
+}
 
+/**
+ * spi_nor_init_params() - Initialize the flash's parameters and settings.
+ * @nor:	pointer to a 'struct spi-nor'.
+ *
+ * The flash parameters and settings are initialized based on a sequence of
+ * calls that are ordered by priority:
+ *
+ * 1/ Legacy flash parameters initialization. The initializations are done
+ *    based on nor->info data:
+ *		spi_nor_legacy_init_params()
+ *
+ * which can be overwritten by:
+ * 2/ Manufacturer flash parameters initialization. The initializations are
+ *    done based on MFR register, or when the decisions can not be done solely
+ *    based on MFR, by using specific flash_info tweeks, ->default_init():
+ *		spi_nor_manufacturer_init_params()
+ *
+ * which can be overwritten by:
+ * 3/ SFDP flash parameters initialization. JESD216 SFDP is a standard and
+ *    should be more accurate that the above.
+ *		spi_nor_sfdp_init_params()
+ *
+ *    Please not that there is a ->post_bfpt() fixup hook that can overwrite the
+ *    flash parameters and settings imediately after parsing the Basic Flash
+ *    Parameter Table.
+ */
+static void spi_nor_init_params(struct spi_nor *nor)
+{
+	spi_nor_legacy_init_params(nor);
 
 	spi_nor_manufacturer_init_params(nor);
 
-	if ((info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
-	    !(info->flags & SPI_NOR_SKIP_SFDP)) {
-		struct spi_nor_flash_parameter sfdp_params;
-
-		memcpy(&sfdp_params, params, sizeof(sfdp_params));
-
-		if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
-			nor->addr_width = 0;
-			nor->flags &= ~SNOR_F_4B_OPCODES;
-		} else {
-			memcpy(params, &sfdp_params, sizeof(*params));
-		}
-	}
-
-	return 0;
+	if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
+	    !(nor->info->flags & SPI_NOR_SKIP_SFDP))
+		spi_nor_sfdp_init_params(nor);
 }
 
 static int spi_nor_select_read(struct spi_nor *nor,
@@ -4693,10 +4738,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	    nor->info->flags & SPI_NOR_HAS_LOCK)
 		nor->params.disable_block_protection = spi_nor_clear_sr_bp;
 
-	/* Parse the Serial Flash Discoverable Parameters table. */
-	ret = spi_nor_init_params(nor);
-	if (ret)
-		return ret;
+	/* Init flash parameters based on flash_info struct and SFDP */
+	spi_nor_init_params(nor);
 
 	if (!mtd->name)
 		mtd->name = dev_name(dev);
-- 
2.9.5


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

Re: [PATCH v2 4/7] mtd: spi-nor: Split spi_nor_init_params()

[Boris Brezillon]

On Sat, 24 Aug 2019 12:00:43 +0000
<Tudor.Ambarus@microchip.com> wrote:

> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> Add functions to delimit what the chunks of code do:
> 
> static void spi_nor_init_params()
> {
> 	spi_nor_legacy_init_params()
> 	spi_nor_manufacturer_init_params()
> 	spi_nor_sfdp_init_params()
> }
> 
> Add descriptions to all methods.
> 
> spi_nor_init_params() becomes of type void, as all its children
> return void.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 83 ++++++++++++++++++++++++++++++++-----------
>  1 file changed, 63 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index c9514dfd7d6d..93424f914159 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -4186,7 +4186,34 @@ static void spi_nor_manufacturer_init_params(struct spi_nor *nor)
>  		nor->info->fixups->default_init(nor);
>  }
>  
> -static int spi_nor_init_params(struct spi_nor *nor)
> +/**
> + * spi_nor_sfdp_init_params() - Initialize the flash's parameters and settings
> + * based on JESD216 SFDP standard.
> + * @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.
> + */
> +static void spi_nor_sfdp_init_params(struct spi_nor *nor)
> +{
> +	struct spi_nor_flash_parameter sfdp_params;
> +
> +	memcpy(&sfdp_params, &nor->params, sizeof(sfdp_params));
> +
> +	if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
> +		nor->addr_width = 0;
> +		nor->flags &= ~SNOR_F_4B_OPCODES;
> +	} else {
> +		memcpy(&nor->params, &sfdp_params, sizeof(nor->params));
> +	}
> +}
> +
> +/**
> + * spi_nor_legacy_init_params() - Initialize the flash's parameters and settings
> + * based on nor->info data.
> + * @nor:	pointer to a 'struct spi-nor'.
> + */
> +static void spi_nor_legacy_init_params(struct spi_nor *nor)

Nitpick: hm, I'm not a big fan of the 'legacy' term here as I'm not sure
it reflects the reality. I guess this function will stay around, and
even new NORs are not guaranteed to provide SFDP tables. How about
spi_nor_set_default_params() or spi_nor_info_init_params()?

That's just a suggestion, so here is my

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

in case you want to ignore it.

>  {
>  	struct spi_nor_flash_parameter *params = &nor->params;
>  	struct spi_nor_erase_map *map = &params->erase_map;
> @@ -4260,25 +4287,43 @@ static int spi_nor_init_params(struct spi_nor *nor)
>  	spi_nor_set_erase_type(&map->erase_type[i], info->sector_size,
>  			       SPINOR_OP_SE);
>  	spi_nor_init_uniform_erase_map(map, erase_mask, params->size);
> +}
>  
> +/**
> + * spi_nor_init_params() - Initialize the flash's parameters and settings.
> + * @nor:	pointer to a 'struct spi-nor'.
> + *
> + * The flash parameters and settings are initialized based on a sequence of
> + * calls that are ordered by priority:
> + *
> + * 1/ Legacy flash parameters initialization. The initializations are done
> + *    based on nor->info data:
> + *		spi_nor_legacy_init_params()
> + *
> + * which can be overwritten by:
> + * 2/ Manufacturer flash parameters initialization. The initializations are
> + *    done based on MFR register, or when the decisions can not be done solely
> + *    based on MFR, by using specific flash_info tweeks, ->default_init():
> + *		spi_nor_manufacturer_init_params()
> + *
> + * which can be overwritten by:
> + * 3/ SFDP flash parameters initialization. JESD216 SFDP is a standard and
> + *    should be more accurate that the above.
> + *		spi_nor_sfdp_init_params()
> + *
> + *    Please not that there is a ->post_bfpt() fixup hook that can overwrite the
> + *    flash parameters and settings imediately after parsing the Basic Flash
> + *    Parameter Table.
> + */
> +static void spi_nor_init_params(struct spi_nor *nor)
> +{
> +	spi_nor_legacy_init_params(nor);
>  
>  	spi_nor_manufacturer_init_params(nor);
>  
> -	if ((info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
> -	    !(info->flags & SPI_NOR_SKIP_SFDP)) {
> -		struct spi_nor_flash_parameter sfdp_params;
> -
> -		memcpy(&sfdp_params, params, sizeof(sfdp_params));
> -
> -		if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
> -			nor->addr_width = 0;
> -			nor->flags &= ~SNOR_F_4B_OPCODES;
> -		} else {
> -			memcpy(params, &sfdp_params, sizeof(*params));
> -		}
> -	}
> -
> -	return 0;
> +	if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
> +	    !(nor->info->flags & SPI_NOR_SKIP_SFDP))
> +		spi_nor_sfdp_init_params(nor);
>  }
>  
>  static int spi_nor_select_read(struct spi_nor *nor,
> @@ -4693,10 +4738,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	    nor->info->flags & SPI_NOR_HAS_LOCK)
>  		nor->params.disable_block_protection = spi_nor_clear_sr_bp;
>  
> -	/* Parse the Serial Flash Discoverable Parameters table. */
> -	ret = spi_nor_init_params(nor);
> -	if (ret)
> -		return ret;
> +	/* Init flash parameters based on flash_info struct and SFDP */
> +	spi_nor_init_params(nor);
>  
>  	if (!mtd->name)
>  		mtd->name = dev_name(dev);


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

Re: [PATCH v2 4/7] mtd: spi-nor: Split spi_nor_init_params()

[Tudor.Ambarus]

On 08/25/2019 03:03 PM, Boris Brezillon wrote:
> External E-Mail
> 
> 
> On Sat, 24 Aug 2019 12:00:43 +0000
> <Tudor.Ambarus@microchip.com> wrote:
> 
>> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>>
>> Add functions to delimit what the chunks of code do:
>>
>> static void spi_nor_init_params()
>> {
>> 	spi_nor_legacy_init_params()
>> 	spi_nor_manufacturer_init_params()
>> 	spi_nor_sfdp_init_params()
>> }
>>
>> Add descriptions to all methods.
>>
>> spi_nor_init_params() becomes of type void, as all its children
>> return void.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 83 ++++++++++++++++++++++++++++++++-----------
>>  1 file changed, 63 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index c9514dfd7d6d..93424f914159 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -4186,7 +4186,34 @@ static void spi_nor_manufacturer_init_params(struct spi_nor *nor)
>>  		nor->info->fixups->default_init(nor);
>>  }
>>  
>> -static int spi_nor_init_params(struct spi_nor *nor)
>> +/**
>> + * spi_nor_sfdp_init_params() - Initialize the flash's parameters and settings
>> + * based on JESD216 SFDP standard.
>> + * @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.
>> + */
>> +static void spi_nor_sfdp_init_params(struct spi_nor *nor)
>> +{
>> +	struct spi_nor_flash_parameter sfdp_params;
>> +
>> +	memcpy(&sfdp_params, &nor->params, sizeof(sfdp_params));
>> +
>> +	if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
>> +		nor->addr_width = 0;
>> +		nor->flags &= ~SNOR_F_4B_OPCODES;
>> +	} else {
>> +		memcpy(&nor->params, &sfdp_params, sizeof(nor->params));
>> +	}
>> +}
>> +
>> +/**
>> + * spi_nor_legacy_init_params() - Initialize the flash's parameters and settings
>> + * based on nor->info data.
>> + * @nor:	pointer to a 'struct spi-nor'.
>> + */
>> +static void spi_nor_legacy_init_params(struct spi_nor *nor)
> 
> Nitpick: hm, I'm not a big fan of the 'legacy' term here as I'm not sure
> it reflects the reality. I guess this function will stay around, and
> even new NORs are not guaranteed to provide SFDP tables. How about
> spi_nor_set_default_params() or spi_nor_info_init_params()?

I can rename it to spi_nor_info_init_params() to be in sync with
                   spi_nor_manufacturer_init_params() and
                   spi_nor_sfdp_init_params()

or I can rename all to:
spi_nor_set_params()
spi_nor_set_default_params()
spi_nor_set_manufacturer_params()
spi_nor_set_sfdp_params()

Both are ok, but the second option seems better. What would you choose?

> 
> That's just a suggestion, so here is my
> 
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> 
> in case you want to ignore it.
> 
>>  {
>>  	struct spi_nor_flash_parameter *params = &nor->params;
>>  	struct spi_nor_erase_map *map = &params->erase_map;
>> @@ -4260,25 +4287,43 @@ static int spi_nor_init_params(struct spi_nor *nor)
>>  	spi_nor_set_erase_type(&map->erase_type[i], info->sector_size,
>>  			       SPINOR_OP_SE);
>>  	spi_nor_init_uniform_erase_map(map, erase_mask, params->size);
>> +}
>>  
>> +/**
>> + * spi_nor_init_params() - Initialize the flash's parameters and settings.
>> + * @nor:	pointer to a 'struct spi-nor'.
>> + *
>> + * The flash parameters and settings are initialized based on a sequence of
>> + * calls that are ordered by priority:
>> + *
>> + * 1/ Legacy flash parameters initialization. The initializations are done
>> + *    based on nor->info data:
>> + *		spi_nor_legacy_init_params()
>> + *
>> + * which can be overwritten by:
>> + * 2/ Manufacturer flash parameters initialization. The initializations are
>> + *    done based on MFR register, or when the decisions can not be done solely
>> + *    based on MFR, by using specific flash_info tweeks, ->default_init():
>> + *		spi_nor_manufacturer_init_params()
>> + *
>> + * which can be overwritten by:
>> + * 3/ SFDP flash parameters initialization. JESD216 SFDP is a standard and
>> + *    should be more accurate that the above.
>> + *		spi_nor_sfdp_init_params()
>> + *
>> + *    Please not that there is a ->post_bfpt() fixup hook that can overwrite the
>> + *    flash parameters and settings imediately after parsing the Basic Flash
>> + *    Parameter Table.
>> + */
>> +static void spi_nor_init_params(struct spi_nor *nor)
>> +{
>> +	spi_nor_legacy_init_params(nor);
>>  
>>  	spi_nor_manufacturer_init_params(nor);
>>  
>> -	if ((info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
>> -	    !(info->flags & SPI_NOR_SKIP_SFDP)) {
>> -		struct spi_nor_flash_parameter sfdp_params;
>> -
>> -		memcpy(&sfdp_params, params, sizeof(sfdp_params));
>> -
>> -		if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
>> -			nor->addr_width = 0;
>> -			nor->flags &= ~SNOR_F_4B_OPCODES;
>> -		} else {
>> -			memcpy(params, &sfdp_params, sizeof(*params));
>> -		}
>> -	}
>> -
>> -	return 0;
>> +	if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
>> +	    !(nor->info->flags & SPI_NOR_SKIP_SFDP))
>> +		spi_nor_sfdp_init_params(nor);
>>  }
>>  
>>  static int spi_nor_select_read(struct spi_nor *nor,
>> @@ -4693,10 +4738,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>>  	    nor->info->flags & SPI_NOR_HAS_LOCK)
>>  		nor->params.disable_block_protection = spi_nor_clear_sr_bp;
>>  
>> -	/* Parse the Serial Flash Discoverable Parameters table. */
>> -	ret = spi_nor_init_params(nor);
>> -	if (ret)
>> -		return ret;
>> +	/* Init flash parameters based on flash_info struct and SFDP */
>> +	spi_nor_init_params(nor);
>>  
>>  	if (!mtd->name)
>>  		mtd->name = dev_name(dev);
> 
> 
> 
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 4/7] mtd: spi-nor: Split spi_nor_init_params()

[Boris Brezillon]

On Sun, 25 Aug 2019 12:23:45 +0000
<Tudor.Ambarus@microchip.com> wrote:

> On 08/25/2019 03:03 PM, Boris Brezillon wrote:
> > External E-Mail
> > 
> > 
> > On Sat, 24 Aug 2019 12:00:43 +0000
> > <Tudor.Ambarus@microchip.com> wrote:
> >   
> >> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> >>
> >> Add functions to delimit what the chunks of code do:
> >>
> >> static void spi_nor_init_params()
> >> {
> >> 	spi_nor_legacy_init_params()
> >> 	spi_nor_manufacturer_init_params()
> >> 	spi_nor_sfdp_init_params()
> >> }
> >>
> >> Add descriptions to all methods.
> >>
> >> spi_nor_init_params() becomes of type void, as all its children
> >> return void.
> >>
> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> >> ---
> >>  drivers/mtd/spi-nor/spi-nor.c | 83 ++++++++++++++++++++++++++++++++-----------
> >>  1 file changed, 63 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> >> index c9514dfd7d6d..93424f914159 100644
> >> --- a/drivers/mtd/spi-nor/spi-nor.c
> >> +++ b/drivers/mtd/spi-nor/spi-nor.c
> >> @@ -4186,7 +4186,34 @@ static void spi_nor_manufacturer_init_params(struct spi_nor *nor)
> >>  		nor->info->fixups->default_init(nor);
> >>  }
> >>  
> >> -static int spi_nor_init_params(struct spi_nor *nor)
> >> +/**
> >> + * spi_nor_sfdp_init_params() - Initialize the flash's parameters and settings
> >> + * based on JESD216 SFDP standard.
> >> + * @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.
> >> + */
> >> +static void spi_nor_sfdp_init_params(struct spi_nor *nor)
> >> +{
> >> +	struct spi_nor_flash_parameter sfdp_params;
> >> +
> >> +	memcpy(&sfdp_params, &nor->params, sizeof(sfdp_params));
> >> +
> >> +	if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
> >> +		nor->addr_width = 0;
> >> +		nor->flags &= ~SNOR_F_4B_OPCODES;
> >> +	} else {
> >> +		memcpy(&nor->params, &sfdp_params, sizeof(nor->params));
> >> +	}
> >> +}
> >> +
> >> +/**
> >> + * spi_nor_legacy_init_params() - Initialize the flash's parameters and settings
> >> + * based on nor->info data.
> >> + * @nor:	pointer to a 'struct spi-nor'.
> >> + */
> >> +static void spi_nor_legacy_init_params(struct spi_nor *nor)  
> > 
> > Nitpick: hm, I'm not a big fan of the 'legacy' term here as I'm not sure
> > it reflects the reality. I guess this function will stay around, and
> > even new NORs are not guaranteed to provide SFDP tables. How about
> > spi_nor_set_default_params() or spi_nor_info_init_params()?  
> 
> I can rename it to spi_nor_info_init_params() to be in sync with
>                    spi_nor_manufacturer_init_params() and
>                    spi_nor_sfdp_init_params()
> 
> or I can rename all to:
> spi_nor_set_params()
> spi_nor_set_default_params()
> spi_nor_set_manufacturer_params()
> spi_nor_set_sfdp_params()
> 
> Both are ok, but the second option seems better. What would you choose?

Both sound good, pick the one you prefer.

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

[PATCH v2 5/7] mtd: spi-nor: Create a ->set_4byte() method

[Tudor.Ambarus]

From: Boris Brezillon <boris.brezillon@bootlin.com>

The procedure used to enable 4 byte addressing mode depends on the NOR
device, so let's provide a hook so that manufacturer specific handling
can be implemented in a sane way.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
[tudor.ambarus@microchip.com: use nor->params.set_4byte() instead of
nor->set_4byte()]
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 76 ++++++++++++++++++++++---------------------
 include/linux/mtd/spi-nor.h   |  2 ++
 2 files changed, 41 insertions(+), 37 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 93424f914159..1629584be30e 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -633,6 +633,17 @@ static int macronix_set_4byte(struct spi_nor *nor, bool enable)
 			      NULL, 0);
 }
 
+static int st_micron_set_4byte(struct spi_nor *nor, bool enable)
+{
+	int ret;
+
+	write_enable(nor);
+	ret = macronix_set_4byte(nor, enable);
+	write_disable(nor);
+
+	return ret;
+}
+
 static int spansion_set_4byte(struct spi_nor *nor, bool enable)
 {
 	nor->bouncebuf[0] = enable << 7;
@@ -667,45 +678,24 @@ static int spi_nor_write_ear(struct spi_nor *nor, u8 ear)
 	return nor->write_reg(nor, SPINOR_OP_WREAR, nor->bouncebuf, 1);
 }
 
-/* Enable/disable 4-byte addressing mode. */
-static int set_4byte(struct spi_nor *nor, bool enable)
+static int winbond_set_4byte(struct spi_nor *nor, bool enable)
 {
-	int status;
-	bool need_wren = false;
-
-	switch (JEDEC_MFR(nor->info)) {
-	case SNOR_MFR_ST:
-	case SNOR_MFR_MICRON:
-		/* Some Micron need WREN command; all will accept it */
-		need_wren = true;
-		/* fall through */
-	case SNOR_MFR_MACRONIX:
-	case SNOR_MFR_WINBOND:
-		if (need_wren)
-			write_enable(nor);
+	int ret;
 
-		status = macronix_set_4byte(nor, enable);
-		if (need_wren)
-			write_disable(nor);
+	ret = macronix_set_4byte(nor, enable);
+	if (ret || enable)
+		return ret;
 
-		if (!status && !enable &&
-		    JEDEC_MFR(nor->info) == SNOR_MFR_WINBOND) {
-			/*
-			 * On Winbond W25Q256FV, leaving 4byte mode causes
-			 * the Extended Address Register to be set to 1, so all
-			 * 3-byte-address reads come from the second 16M.
-			 * We must clear the register to enable normal behavior.
-			 */
-			write_enable(nor);
-			spi_nor_write_ear(nor, 0);
-			write_disable(nor);
-		}
+	/*
+	 * On Winbond W25Q256FV, leaving 4byte mode causes the Extended Address
+	 * Register to be set to 1, so all 3-byte-address reads come from the
+	 * second 16M. We must clear the register to enable normal behavior.
+	 */
+	write_enable(nor);
+	ret = spi_nor_write_ear(nor, 0);
+	write_disable(nor);
 
-		return status;
-	default:
-		/* Spansion style */
-		return spansion_set_4byte(nor, enable);
-	}
+	return ret;
 }
 
 static int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)
@@ -4153,11 +4143,18 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
 static void macronix_set_default_init(struct spi_nor *nor)
 {
 	nor->params.quad_enable = macronix_quad_enable;
+	nor->params.set_4byte = macronix_set_4byte;
 }
 
 static void st_micron_set_default_init(struct spi_nor *nor)
 {
 	nor->params.quad_enable = NULL;
+	nor->params.set_4byte = st_micron_set_4byte;
+}
+
+static void winbond_set_default_init(struct spi_nor *nor)
+{
+	nor->params.set_4byte = winbond_set_4byte;
 }
 
 /**
@@ -4178,6 +4175,10 @@ static void spi_nor_manufacturer_init_params(struct spi_nor *nor)
 		st_micron_set_default_init(nor);
 		break;
 
+	case SNOR_MFR_WINBOND:
+		winbond_set_default_init(nor);
+		break;
+
 	default:
 		break;
 	}
@@ -4222,6 +4223,7 @@ static void spi_nor_legacy_init_params(struct spi_nor *nor)
 
 	/* Initialize legacy flash parameters and settings. */
 	params->quad_enable = spansion_quad_enable;
+	params->set_4byte = spansion_set_4byte;
 
 	/* Set SPI NOR sizes. */
 	params->size = (u64)info->sector_size * info->n_sectors;
@@ -4610,7 +4612,7 @@ static int spi_nor_init(struct spi_nor *nor)
 		 */
 		WARN_ONCE(nor->flags & SNOR_F_BROKEN_RESET,
 			  "enabling reset hack; may not recover from unexpected reboots\n");
-		set_4byte(nor, true);
+		nor->params.set_4byte(nor, true);
 	}
 
 	return 0;
@@ -4634,7 +4636,7 @@ void spi_nor_restore(struct spi_nor *nor)
 	/* restore the addressing mode */
 	if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES) &&
 	    nor->flags & SNOR_F_BROKEN_RESET)
-		set_4byte(nor, false);
+		nor->params.set_4byte(nor, false);
 }
 EXPORT_SYMBOL_GPL(spi_nor_restore);
 
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index a3a765c21edc..012731ad339f 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -482,6 +482,7 @@ struct spi_nor;
  * @erase_map:		the erase map parsed from the SFDP Sector Map Parameter
  *                      Table.
  * @quad_enable:	enables SPI NOR quad mode.
+ * @set_4byte:		puts the SPI NOR in 4 byte addressing mode.
  * @disable_block_protection: disables block protection during power-up.
  */
 struct spi_nor_flash_parameter {
@@ -495,6 +496,7 @@ struct spi_nor_flash_parameter {
 	struct spi_nor_erase_map        erase_map;
 
 	int (*quad_enable)(struct spi_nor *nor);
+	int (*set_4byte)(struct spi_nor *nor, bool enable);
 	int (*disable_block_protection)(struct spi_nor *nor);
 };
 
-- 
2.9.5


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

[PATCH v2 6/7] mtd: spi-nor: Rework the SPI NOR lock/unlock logic

[Tudor.Ambarus]

From: Boris Brezillon <boris.brezillon@bootlin.com>

Add the SNOR_F_HAS_LOCK flag and set it when SPI_NOR_HAS_LOCK is set
in the flash_info entry or when it's a Micron or ST flash.

Move the locking hooks in a separate struct so that we have just
one field to update when we change the locking implementation.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
[tudor.ambarus@microchip.com: use ->default_init() hook, introduce
spi_nor_late_init_params(), set ops in nor->params]
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 50 ++++++++++++++++++++++++++++++++-----------
 include/linux/mtd/spi-nor.h   | 23 ++++++++++++++------
 2 files changed, 53 insertions(+), 20 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 1629584be30e..fc9e14777212 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1598,6 +1598,12 @@ static int stm_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len)
 	return stm_is_locked_sr(nor, ofs, len, status);
 }
 
+static const struct spi_nor_locking_ops stm_locking_ops = {
+	.lock = stm_lock,
+	.unlock = stm_unlock,
+	.is_locked = stm_is_locked,
+};
+
 static int spi_nor_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 {
 	struct spi_nor *nor = mtd_to_spi_nor(mtd);
@@ -1607,7 +1613,7 @@ static int spi_nor_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 	if (ret)
 		return ret;
 
-	ret = nor->flash_lock(nor, ofs, len);
+	ret = nor->params.locking_ops->lock(nor, ofs, len);
 
 	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_UNLOCK);
 	return ret;
@@ -1622,7 +1628,7 @@ static int spi_nor_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 	if (ret)
 		return ret;
 
-	ret = nor->flash_unlock(nor, ofs, len);
+	ret = nor->params.locking_ops->unlock(nor, ofs, len);
 
 	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_LOCK);
 	return ret;
@@ -1637,7 +1643,7 @@ static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 	if (ret)
 		return ret;
 
-	ret = nor->flash_is_locked(nor, ofs, len);
+	ret = nor->params.locking_ops->is_locked(nor, ofs, len);
 
 	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_LOCK);
 	return ret;
@@ -4148,6 +4154,7 @@ static void macronix_set_default_init(struct spi_nor *nor)
 
 static void st_micron_set_default_init(struct spi_nor *nor)
 {
+	nor->flags = SNOR_F_HAS_LOCK;
 	nor->params.quad_enable = NULL;
 	nor->params.set_4byte = st_micron_set_4byte;
 }
@@ -4292,6 +4299,23 @@ static void spi_nor_legacy_init_params(struct spi_nor *nor)
 }
 
 /**
+ * spi_nor_late_init_params() - Late initialization of legacy flash parameters.
+ * @nor:	pointer to a 'struct spi_nor'
+ *
+ * Used to set legacy flash parameters and settings when the ->default_init()
+ * hook or the SFDP parser let voids.
+ */
+static void spi_nor_late_init_params(struct spi_nor *nor)
+{
+	/*
+	 * NOR protection support. When locking_ops are not provided, we pick
+	 * the default ones.
+	 */
+	if (nor->flags & SNOR_F_HAS_LOCK && !nor->params.locking_ops)
+		nor->params.locking_ops = &stm_locking_ops;
+}
+
+/**
  * spi_nor_init_params() - Initialize the flash's parameters and settings.
  * @nor:	pointer to a 'struct spi-nor'.
  *
@@ -4316,6 +4340,10 @@ static void spi_nor_legacy_init_params(struct spi_nor *nor)
  *    Please not that there is a ->post_bfpt() fixup hook that can overwrite the
  *    flash parameters and settings imediately after parsing the Basic Flash
  *    Parameter Table.
+ *
+ * 4/ Late legacy flash parameters initialization, used when the
+ * ->default_init() hook or the SFDP parser do not set specific params.
+ *		spi_nor_late_init_params()
  */
 static void spi_nor_init_params(struct spi_nor *nor)
 {
@@ -4326,6 +4354,8 @@ static void spi_nor_init_params(struct spi_nor *nor)
 	if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
 	    !(nor->info->flags & SPI_NOR_SKIP_SFDP))
 		spi_nor_sfdp_init_params(nor);
+
+	spi_nor_late_init_params(nor);
 }
 
 static int spi_nor_select_read(struct spi_nor *nor,
@@ -4730,6 +4760,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	if (info->flags & SPI_S3AN)
 		nor->flags |=  SNOR_F_READY_XSR_RDY;
 
+	if (info->flags & SPI_NOR_HAS_LOCK)
+		nor->flags |= SNOR_F_HAS_LOCK;
+
 	/*
 	 * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
 	 * with the software protection bits set.
@@ -4754,16 +4787,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	mtd->_read = spi_nor_read;
 	mtd->_resume = spi_nor_resume;
 
-	/* NOR protection support for STmicro/Micron chips and similar */
-	if (JEDEC_MFR(info) == SNOR_MFR_ST ||
-	    JEDEC_MFR(info) == SNOR_MFR_MICRON ||
-	    info->flags & SPI_NOR_HAS_LOCK) {
-		nor->flash_lock = stm_lock;
-		nor->flash_unlock = stm_unlock;
-		nor->flash_is_locked = stm_is_locked;
-	}
-
-	if (nor->flash_lock && nor->flash_unlock && nor->flash_is_locked) {
+	if (nor->params.locking_ops) {
 		mtd->_lock = spi_nor_lock;
 		mtd->_unlock = spi_nor_unlock;
 		mtd->_is_locked = spi_nor_is_locked;
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 012731ad339f..6c5eaf607b50 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -243,6 +243,7 @@ enum spi_nor_option_flags {
 	SNOR_F_BROKEN_RESET	= BIT(6),
 	SNOR_F_4B_OPCODES	= BIT(7),
 	SNOR_F_HAS_4BAIT	= BIT(8),
+	SNOR_F_HAS_LOCK		= BIT(9),
 };
 
 /**
@@ -466,6 +467,18 @@ enum spi_nor_pp_command_index {
 struct spi_nor;
 
 /**
+ * struct spi_nor_locking_ops - SPI NOR locking methods
+ * @lock:	lock a region of the SPI NOR.
+ * @unlock:	unlock a region of the SPI NOR.
+ * @is_locked:	check if a region of the SPI NOR is completely locked
+ */
+struct spi_nor_locking_ops {
+	int (*lock)(struct spi_nor *nor, loff_t ofs, uint64_t len);
+	int (*unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len);
+	int (*is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len);
+};
+
+/**
  * struct spi_nor_flash_parameter - SPI NOR flash parameters and settings.
  * Includes legacy flash parameters and settings that can be overwritten
  * by the spi_nor_fixups hooks, or dynamically when parsing the JESD216
@@ -484,6 +497,7 @@ struct spi_nor;
  * @quad_enable:	enables SPI NOR quad mode.
  * @set_4byte:		puts the SPI NOR in 4 byte addressing mode.
  * @disable_block_protection: disables block protection during power-up.
+ * @locking_ops:	SPI NOR locking methods.
  */
 struct spi_nor_flash_parameter {
 	u64				size;
@@ -498,6 +512,8 @@ struct spi_nor_flash_parameter {
 	int (*quad_enable)(struct spi_nor *nor);
 	int (*set_4byte)(struct spi_nor *nor, bool enable);
 	int (*disable_block_protection)(struct spi_nor *nor);
+
+	const struct spi_nor_locking_ops *locking_ops;
 };
 
 /**
@@ -538,10 +554,6 @@ struct flash_info;
  * @erase:		[DRIVER-SPECIFIC] erase a sector of the SPI NOR
  *			at the offset @offs; if not provided by the driver,
  *			spi-nor will send the erase opcode via write_reg()
- * @flash_lock:		[FLASH-SPECIFIC] lock a region of the SPI NOR
- * @flash_unlock:	[FLASH-SPECIFIC] unlock a region of the SPI NOR
- * @flash_is_locked:	[FLASH-SPECIFIC] check if a region of the SPI NOR is
- *			completely locked
  * @params:		[FLASH-SPECIFIC] SPI-NOR flash parameters and settings.
  *                      The structure includes legacy flash parameters and
  *                      settings that can be overwritten by the spi_nor_fixups
@@ -579,9 +591,6 @@ struct spi_nor {
 			size_t len, const u_char *write_buf);
 	int (*erase)(struct spi_nor *nor, loff_t offs);
 
-	int (*flash_lock)(struct spi_nor *nor, loff_t ofs, uint64_t len);
-	int (*flash_unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len);
-	int (*flash_is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len);
 	struct spi_nor_flash_parameter params;
 
 	void *priv;
-- 
2.9.5


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

Re: [PATCH v2 6/7] mtd: spi-nor: Rework the SPI NOR lock/unlock logic

[Boris Brezillon]

On Sat, 24 Aug 2019 12:00:47 +0000
<Tudor.Ambarus@microchip.com> wrote:

> From: Boris Brezillon <boris.brezillon@bootlin.com>
> 
> Add the SNOR_F_HAS_LOCK flag and set it when SPI_NOR_HAS_LOCK is set
> in the flash_info entry or when it's a Micron or ST flash.
> 
> Move the locking hooks in a separate struct so that we have just
> one field to update when we change the locking implementation.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> [tudor.ambarus@microchip.com: use ->default_init() hook, introduce
> spi_nor_late_init_params(), set ops in nor->params]
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 50 ++++++++++++++++++++++++++++++++-----------
>  include/linux/mtd/spi-nor.h   | 23 ++++++++++++++------
>  2 files changed, 53 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 1629584be30e..fc9e14777212 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -1598,6 +1598,12 @@ static int stm_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len)
>  	return stm_is_locked_sr(nor, ofs, len, status);
>  }
>  
> +static const struct spi_nor_locking_ops stm_locking_ops = {
> +	.lock = stm_lock,
> +	.unlock = stm_unlock,
> +	.is_locked = stm_is_locked,
> +};
> +
>  static int spi_nor_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  {
>  	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> @@ -1607,7 +1613,7 @@ static int spi_nor_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  	if (ret)
>  		return ret;
>  
> -	ret = nor->flash_lock(nor, ofs, len);
> +	ret = nor->params.locking_ops->lock(nor, ofs, len);
>  
>  	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_UNLOCK);
>  	return ret;
> @@ -1622,7 +1628,7 @@ static int spi_nor_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  	if (ret)
>  		return ret;
>  
> -	ret = nor->flash_unlock(nor, ofs, len);
> +	ret = nor->params.locking_ops->unlock(nor, ofs, len);
>  
>  	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_LOCK);
>  	return ret;
> @@ -1637,7 +1643,7 @@ static int spi_nor_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  	if (ret)
>  		return ret;
>  
> -	ret = nor->flash_is_locked(nor, ofs, len);
> +	ret = nor->params.locking_ops->is_locked(nor, ofs, len);
>  
>  	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_LOCK);
>  	return ret;
> @@ -4148,6 +4154,7 @@ static void macronix_set_default_init(struct spi_nor *nor)
>  
>  static void st_micron_set_default_init(struct spi_nor *nor)
>  {
> +	nor->flags = SNOR_F_HAS_LOCK;
>  	nor->params.quad_enable = NULL;
>  	nor->params.set_4byte = st_micron_set_4byte;
>  }
> @@ -4292,6 +4299,23 @@ static void spi_nor_legacy_init_params(struct spi_nor *nor)
>  }
>  
>  /**
> + * spi_nor_late_init_params() - Late initialization of legacy flash parameters.
> + * @nor:	pointer to a 'struct spi_nor'
> + *
> + * Used to set legacy flash parameters and settings when the ->default_init()
> + * hook or the SFDP parser let voids.
> + */
> +static void spi_nor_late_init_params(struct spi_nor *nor)
> +{
> +	/*
> +	 * NOR protection support. When locking_ops are not provided, we pick
> +	 * the default ones.
> +	 */
> +	if (nor->flags & SNOR_F_HAS_LOCK && !nor->params.locking_ops)
> +		nor->params.locking_ops = &stm_locking_ops;
> +}
> +
> +/**
>   * spi_nor_init_params() - Initialize the flash's parameters and settings.
>   * @nor:	pointer to a 'struct spi-nor'.
>   *
> @@ -4316,6 +4340,10 @@ static void spi_nor_legacy_init_params(struct spi_nor *nor)
>   *    Please not that there is a ->post_bfpt() fixup hook that can overwrite the
>   *    flash parameters and settings imediately after parsing the Basic Flash
>   *    Parameter Table.
> + *
> + * 4/ Late legacy flash parameters initialization, used when the
> + * ->default_init() hook or the SFDP parser do not set specific params.
> + *		spi_nor_late_init_params()
>   */
>  static void spi_nor_init_params(struct spi_nor *nor)
>  {
> @@ -4326,6 +4354,8 @@ static void spi_nor_init_params(struct spi_nor *nor)
>  	if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
>  	    !(nor->info->flags & SPI_NOR_SKIP_SFDP))
>  		spi_nor_sfdp_init_params(nor);
> +
> +	spi_nor_late_init_params(nor);
>  }
>  
>  static int spi_nor_select_read(struct spi_nor *nor,
> @@ -4730,6 +4760,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	if (info->flags & SPI_S3AN)
>  		nor->flags |=  SNOR_F_READY_XSR_RDY;
>  
> +	if (info->flags & SPI_NOR_HAS_LOCK)
> +		nor->flags |= SNOR_F_HAS_LOCK;
> +
>  	/*
>  	 * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
>  	 * with the software protection bits set.
> @@ -4754,16 +4787,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	mtd->_read = spi_nor_read;
>  	mtd->_resume = spi_nor_resume;
>  
> -	/* NOR protection support for STmicro/Micron chips and similar */
> -	if (JEDEC_MFR(info) == SNOR_MFR_ST ||
> -	    JEDEC_MFR(info) == SNOR_MFR_MICRON ||
> -	    info->flags & SPI_NOR_HAS_LOCK) {
> -		nor->flash_lock = stm_lock;
> -		nor->flash_unlock = stm_unlock;
> -		nor->flash_is_locked = stm_is_locked;
> -	}
> -
> -	if (nor->flash_lock && nor->flash_unlock && nor->flash_is_locked) {
> +	if (nor->params.locking_ops) {
>  		mtd->_lock = spi_nor_lock;
>  		mtd->_unlock = spi_nor_unlock;
>  		mtd->_is_locked = spi_nor_is_locked;
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 012731ad339f..6c5eaf607b50 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -243,6 +243,7 @@ enum spi_nor_option_flags {
>  	SNOR_F_BROKEN_RESET	= BIT(6),
>  	SNOR_F_4B_OPCODES	= BIT(7),
>  	SNOR_F_HAS_4BAIT	= BIT(8),
> +	SNOR_F_HAS_LOCK		= BIT(9),
>  };
>  
>  /**
> @@ -466,6 +467,18 @@ enum spi_nor_pp_command_index {
>  struct spi_nor;
>  
>  /**
> + * struct spi_nor_locking_ops - SPI NOR locking methods
> + * @lock:	lock a region of the SPI NOR.
> + * @unlock:	unlock a region of the SPI NOR.
> + * @is_locked:	check if a region of the SPI NOR is completely locked
> + */
> +struct spi_nor_locking_ops {
> +	int (*lock)(struct spi_nor *nor, loff_t ofs, uint64_t len);
> +	int (*unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len);
> +	int (*is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len);
> +};
> +
> +/**
>   * struct spi_nor_flash_parameter - SPI NOR flash parameters and settings.
>   * Includes legacy flash parameters and settings that can be overwritten
>   * by the spi_nor_fixups hooks, or dynamically when parsing the JESD216
> @@ -484,6 +497,7 @@ struct spi_nor;
>   * @quad_enable:	enables SPI NOR quad mode.
>   * @set_4byte:		puts the SPI NOR in 4 byte addressing mode.
>   * @disable_block_protection: disables block protection during power-up.
> + * @locking_ops:	SPI NOR locking methods.
>   */
>  struct spi_nor_flash_parameter {
>  	u64				size;
> @@ -498,6 +512,8 @@ struct spi_nor_flash_parameter {
>  	int (*quad_enable)(struct spi_nor *nor);
>  	int (*set_4byte)(struct spi_nor *nor, bool enable);
>  	int (*disable_block_protection)(struct spi_nor *nor);

Should we move this ->disable_block_protection() hook to
spi_nor_locking_ops? 

> +
> +	const struct spi_nor_locking_ops *locking_ops;
>  };
>  
>  /**
> @@ -538,10 +554,6 @@ struct flash_info;
>   * @erase:		[DRIVER-SPECIFIC] erase a sector of the SPI NOR
>   *			at the offset @offs; if not provided by the driver,
>   *			spi-nor will send the erase opcode via write_reg()
> - * @flash_lock:		[FLASH-SPECIFIC] lock a region of the SPI NOR
> - * @flash_unlock:	[FLASH-SPECIFIC] unlock a region of the SPI NOR
> - * @flash_is_locked:	[FLASH-SPECIFIC] check if a region of the SPI NOR is
> - *			completely locked
>   * @params:		[FLASH-SPECIFIC] SPI-NOR flash parameters and settings.
>   *                      The structure includes legacy flash parameters and
>   *                      settings that can be overwritten by the spi_nor_fixups
> @@ -579,9 +591,6 @@ struct spi_nor {
>  			size_t len, const u_char *write_buf);
>  	int (*erase)(struct spi_nor *nor, loff_t offs);
>  
> -	int (*flash_lock)(struct spi_nor *nor, loff_t ofs, uint64_t len);
> -	int (*flash_unlock)(struct spi_nor *nor, loff_t ofs, uint64_t len);
> -	int (*flash_is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len);
>  	struct spi_nor_flash_parameter params;
>  
>  	void *priv;


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

Re: [PATCH v2 6/7] mtd: spi-nor: Rework the SPI NOR lock/unlock logic

[Tudor.Ambarus]

On 08/25/2019 03:26 PM, Boris Brezillon wrote:
>>  	int (*disable_block_protection)(struct spi_nor *nor);
> Should we move this ->disable_block_protection() hook to
> spi_nor_locking_ops? 
> 

yes, will do. thanks!
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v2 7/7] mtd: spi-nor: Rework the disabling of block write protection

[Tudor.Ambarus]

From: Tudor Ambarus <tudor.ambarus@microchip.com>

Get rid of MFR handling and implement specific manufacturer
default_init() fixup hooks.

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

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index fc9e14777212..f4e9fcca619f 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -4146,6 +4146,16 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
 	return err;
 }
 
+static void atmel_set_default_init(struct spi_nor *nor)
+{
+	nor->params.disable_block_protection = spi_nor_clear_sr_bp;
+}
+
+static void intel_set_default_init(struct spi_nor *nor)
+{
+	nor->params.disable_block_protection = spi_nor_clear_sr_bp;
+}
+
 static void macronix_set_default_init(struct spi_nor *nor)
 {
 	nor->params.quad_enable = macronix_quad_enable;
@@ -4173,6 +4183,14 @@ static void spi_nor_manufacturer_init_params(struct spi_nor *nor)
 {
 	/* Init flash parameters based on MFR */
 	switch (JEDEC_MFR(nor->info)) {
+	case SNOR_MFR_ATMEL:
+		atmel_set_default_init(nor);
+		break;
+
+	case SNOR_MFR_INTEL:
+		intel_set_default_init(nor);
+		break;
+
 	case SNOR_MFR_MACRONIX:
 		macronix_set_default_init(nor);
 		break;
@@ -4760,18 +4778,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	if (info->flags & SPI_S3AN)
 		nor->flags |=  SNOR_F_READY_XSR_RDY;
 
-	if (info->flags & SPI_NOR_HAS_LOCK)
+	if (info->flags & SPI_NOR_HAS_LOCK) {
 		nor->flags |= SNOR_F_HAS_LOCK;
-
-	/*
-	 * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
-	 * with the software protection bits set.
-	 */
-	if (JEDEC_MFR(nor->info) == SNOR_MFR_ATMEL ||
-	    JEDEC_MFR(nor->info) == SNOR_MFR_INTEL ||
-	    JEDEC_MFR(nor->info) == SNOR_MFR_SST ||
-	    nor->info->flags & SPI_NOR_HAS_LOCK)
 		nor->params.disable_block_protection = spi_nor_clear_sr_bp;
+	}
 
 	/* Init flash parameters based on flash_info struct and SFDP */
 	spi_nor_init_params(nor);
-- 
2.9.5


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

Re: [PATCH v2 7/7] mtd: spi-nor: Rework the disabling of block write protection

[Boris Brezillon]

On Sat, 24 Aug 2019 12:00:48 +0000
<Tudor.Ambarus@microchip.com> wrote:

> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> Get rid of MFR handling and implement specific manufacturer
> default_init() fixup hooks.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 30 ++++++++++++++++++++----------
>  1 file changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index fc9e14777212..f4e9fcca619f 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -4146,6 +4146,16 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
>  	return err;
>  }
>  
> +static void atmel_set_default_init(struct spi_nor *nor)
> +{
> +	nor->params.disable_block_protection = spi_nor_clear_sr_bp;
> +}
> +
> +static void intel_set_default_init(struct spi_nor *nor)
> +{
> +	nor->params.disable_block_protection = spi_nor_clear_sr_bp;

That's weird: you can unlock blocks but locking is not
explicitly flagged as supported (SNOR_F_HAS_LOCK is not set). Is that
expected?

> +}
> +
>  static void macronix_set_default_init(struct spi_nor *nor)
>  {
>  	nor->params.quad_enable = macronix_quad_enable;
> @@ -4173,6 +4183,14 @@ static void spi_nor_manufacturer_init_params(struct spi_nor *nor)
>  {
>  	/* Init flash parameters based on MFR */
>  	switch (JEDEC_MFR(nor->info)) {
> +	case SNOR_MFR_ATMEL:
> +		atmel_set_default_init(nor);
> +		break;
> +
> +	case SNOR_MFR_INTEL:
> +		intel_set_default_init(nor);
> +		break;
> +
>  	case SNOR_MFR_MACRONIX:
>  		macronix_set_default_init(nor);
>  		break;
> @@ -4760,18 +4778,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	if (info->flags & SPI_S3AN)
>  		nor->flags |=  SNOR_F_READY_XSR_RDY;
>  
> -	if (info->flags & SPI_NOR_HAS_LOCK)
> +	if (info->flags & SPI_NOR_HAS_LOCK) {

If this flag implies SR_BP-based locking we should really rename it into
SPI_NOR_HAS_SR_BP_LOCK to avoid any confusion.

>  		nor->flags |= SNOR_F_HAS_LOCK;
> -
> -	/*
> -	 * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
> -	 * with the software protection bits set.
> -	 */
> -	if (JEDEC_MFR(nor->info) == SNOR_MFR_ATMEL ||
> -	    JEDEC_MFR(nor->info) == SNOR_MFR_INTEL ||
> -	    JEDEC_MFR(nor->info) == SNOR_MFR_SST ||
> -	    nor->info->flags & SPI_NOR_HAS_LOCK)
>  		nor->params.disable_block_protection = spi_nor_clear_sr_bp;
> +	}
>  
>  	/* Init flash parameters based on flash_info struct and SFDP */
>  	spi_nor_init_params(nor);


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

Re: [PATCH v2 7/7] mtd: spi-nor: Rework the disabling of block write protection

[Tudor.Ambarus]

On 08/25/2019 03:24 PM, Boris Brezillon wrote:
> On Sat, 24 Aug 2019 12:00:48 +0000
> <Tudor.Ambarus@microchip.com> wrote:
> 
>> From: Tudor Ambarus <tudor.ambarus@microchip.com>
>>
>> Get rid of MFR handling and implement specific manufacturer
>> default_init() fixup hooks.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 30 ++++++++++++++++++++----------
>>  1 file changed, 20 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index fc9e14777212..f4e9fcca619f 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -4146,6 +4146,16 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
>>  	return err;
>>  }
>>  
>> +static void atmel_set_default_init(struct spi_nor *nor)
>> +{
>> +	nor->params.disable_block_protection = spi_nor_clear_sr_bp;
>> +}
>> +
>> +static void intel_set_default_init(struct spi_nor *nor)
>> +{
>> +	nor->params.disable_block_protection = spi_nor_clear_sr_bp;
> 
> That's weird: you can unlock blocks but locking is not
> explicitly flagged as supported (SNOR_F_HAS_LOCK is not set). Is that
> expected?

Yes. Manufacturers have different methods for locking/unlocking blocks of
memory. Right now we support just the stm/sr locking operations. sst26vf064b for
example, uses dedicated registers for reading/writing which blocks are
protected, and not the Status Register.

The reason for having disable_block_protection(), is that some spi-nor flashes
are write protected by default after a power-on reset cycle, in order to avoid
inadvertent writes during power-up. Backward compatibility imposes to disable
the write block protection at power-up by default, so that you can erase/write
the memory without having to send an unlock-all command. Which is bad in my
opinion and that's why I proposed https://patchwork.ozlabs.org/patch/1133278/.

Even if sst26vf064b does not yet have the lock ops implemented (SNOR_F_HAS_LOCK
is not set), I would like to be able to interact with it, so to disable the
block protection at power-up. This flash, and others, support a Global Unlock
Command which unlocks the entire memory array in a single cycle. We can't
determine who supports this command purely by manufacturer type, and it's not
discoverable through SFDP, so we'll have to add a nor->info flag for it:
UNLOCK_GLOBAL_BLOCK (see https://patchwork.ozlabs.org/patch/1152606/).

In conclusion, even if SNOR_F_HAS_LOCK is not set (the locking ops are not
implemented), we can still have disable_block_protection() mechanisms to unlock
the entire flash at power-up.

> 
>> +}
>> +
>>  static void macronix_set_default_init(struct spi_nor *nor)
>>  {
>>  	nor->params.quad_enable = macronix_quad_enable;
>> @@ -4173,6 +4183,14 @@ static void spi_nor_manufacturer_init_params(struct spi_nor *nor)
>>  {
>>  	/* Init flash parameters based on MFR */
>>  	switch (JEDEC_MFR(nor->info)) {
>> +	case SNOR_MFR_ATMEL:
>> +		atmel_set_default_init(nor);
>> +		break;
>> +
>> +	case SNOR_MFR_INTEL:
>> +		intel_set_default_init(nor);
>> +		break;
>> +
>>  	case SNOR_MFR_MACRONIX:
>>  		macronix_set_default_init(nor);
>>  		break;
>> @@ -4760,18 +4778,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>>  	if (info->flags & SPI_S3AN)
>>  		nor->flags |=  SNOR_F_READY_XSR_RDY;
>>  
>> -	if (info->flags & SPI_NOR_HAS_LOCK)
>> +	if (info->flags & SPI_NOR_HAS_LOCK) {
> 
> If this flag implies SR_BP-based locking we should really rename it into
> SPI_NOR_HAS_SR_BP_LOCK to avoid any confusion.

Not only SR-based locking, should be a general flag that indicates that locking
ops are supported whichever they are. I would keep the SPI_NOR_HAS_LOCK and let
the manufacturer set its locking ops using the ->default_init() hook.

> 
>>  		nor->flags |= SNOR_F_HAS_LOCK;
>> -
>> -	/*
>> -	 * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
>> -	 * with the software protection bits set.
>> -	 */
>> -	if (JEDEC_MFR(nor->info) == SNOR_MFR_ATMEL ||
>> -	    JEDEC_MFR(nor->info) == SNOR_MFR_INTEL ||
>> -	    JEDEC_MFR(nor->info) == SNOR_MFR_SST ||
>> -	    nor->info->flags & SPI_NOR_HAS_LOCK)
>>  		nor->params.disable_block_protection = spi_nor_clear_sr_bp;
>> +	}
>>  
>>  	/* Init flash parameters based on flash_info struct and SFDP */
>>  	spi_nor_init_params(nor);
> 
> 
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH v2 7/7] mtd: spi-nor: Rework the disabling of block write protection

[Boris Brezillon]

On Sun, 25 Aug 2019 12:57:35 +0000
<Tudor.Ambarus@microchip.com> wrote:

> On 08/25/2019 03:24 PM, Boris Brezillon wrote:
> > On Sat, 24 Aug 2019 12:00:48 +0000
> > <Tudor.Ambarus@microchip.com> wrote:
> >   
> >> From: Tudor Ambarus <tudor.ambarus@microchip.com>
> >>
> >> Get rid of MFR handling and implement specific manufacturer
> >> default_init() fixup hooks.
> >>
> >> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> >> ---
> >>  drivers/mtd/spi-nor/spi-nor.c | 30 ++++++++++++++++++++----------
> >>  1 file changed, 20 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> >> index fc9e14777212..f4e9fcca619f 100644
> >> --- a/drivers/mtd/spi-nor/spi-nor.c
> >> +++ b/drivers/mtd/spi-nor/spi-nor.c
> >> @@ -4146,6 +4146,16 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
> >>  	return err;
> >>  }
> >>  
> >> +static void atmel_set_default_init(struct spi_nor *nor)
> >> +{
> >> +	nor->params.disable_block_protection = spi_nor_clear_sr_bp;
> >> +}
> >> +
> >> +static void intel_set_default_init(struct spi_nor *nor)
> >> +{
> >> +	nor->params.disable_block_protection = spi_nor_clear_sr_bp;  
> > 
> > That's weird: you can unlock blocks but locking is not
> > explicitly flagged as supported (SNOR_F_HAS_LOCK is not set). Is that
> > expected?  
> 
> Yes. Manufacturers have different methods for locking/unlocking blocks of
> memory. Right now we support just the stm/sr locking operations. sst26vf064b for
> example, uses dedicated registers for reading/writing which blocks are
> protected, and not the Status Register.
> 
> The reason for having disable_block_protection(), is that some spi-nor flashes
> are write protected by default after a power-on reset cycle, in order to avoid
> inadvertent writes during power-up. Backward compatibility imposes to disable
> the write block protection at power-up by default, so that you can erase/write
> the memory without having to send an unlock-all command. Which is bad in my
> opinion and that's why I proposed https://patchwork.ozlabs.org/patch/1133278/.
> 
> Even if sst26vf064b does not yet have the lock ops implemented (SNOR_F_HAS_LOCK
> is not set), I would like to be able to interact with it, so to disable the
> block protection at power-up. This flash, and others, support a Global Unlock
> Command which unlocks the entire memory array in a single cycle. We can't
> determine who supports this command purely by manufacturer type, and it's not
> discoverable through SFDP, so we'll have to add a nor->info flag for it:
> UNLOCK_GLOBAL_BLOCK (see https://patchwork.ozlabs.org/patch/1152606/).
> 
> In conclusion, even if SNOR_F_HAS_LOCK is not set (the locking ops are not
> implemented), we can still have disable_block_protection() mechanisms to unlock
> the entire flash at power-up.

Hm, okay, but what about those atmel/intel chips that support
SR_BP-based global unlock? Shouldn't they also support SR_BP-based
locking/unlocking?

> 
> >   
> >> +}
> >> +
> >>  static void macronix_set_default_init(struct spi_nor *nor)
> >>  {
> >>  	nor->params.quad_enable = macronix_quad_enable;
> >> @@ -4173,6 +4183,14 @@ static void spi_nor_manufacturer_init_params(struct spi_nor *nor)
> >>  {
> >>  	/* Init flash parameters based on MFR */
> >>  	switch (JEDEC_MFR(nor->info)) {
> >> +	case SNOR_MFR_ATMEL:
> >> +		atmel_set_default_init(nor);
> >> +		break;
> >> +
> >> +	case SNOR_MFR_INTEL:
> >> +		intel_set_default_init(nor);
> >> +		break;
> >> +
> >>  	case SNOR_MFR_MACRONIX:
> >>  		macronix_set_default_init(nor);
> >>  		break;
> >> @@ -4760,18 +4778,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
> >>  	if (info->flags & SPI_S3AN)
> >>  		nor->flags |=  SNOR_F_READY_XSR_RDY;
> >>  
> >> -	if (info->flags & SPI_NOR_HAS_LOCK)
> >> +	if (info->flags & SPI_NOR_HAS_LOCK) {  
> > 
> > If this flag implies SR_BP-based locking we should really rename it into
> > SPI_NOR_HAS_SR_BP_LOCK to avoid any confusion.  
> 
> Not only SR-based locking, should be a general flag that indicates that locking
> ops are supported whichever they are. I would keep the SPI_NOR_HAS_LOCK and let
> the manufacturer set its locking ops using the ->default_init() hook.

Okay, sounds good as long as the locking scheme is selected on a
per-manufacturer basis, not a per-chip basis.

> 
> >   
> >>  		nor->flags |= SNOR_F_HAS_LOCK;
> >> -
> >> -	/*
> >> -	 * Atmel, SST, Intel/Numonyx, and others serial NOR tend to power up
> >> -	 * with the software protection bits set.
> >> -	 */
> >> -	if (JEDEC_MFR(nor->info) == SNOR_MFR_ATMEL ||
> >> -	    JEDEC_MFR(nor->info) == SNOR_MFR_INTEL ||
> >> -	    JEDEC_MFR(nor->info) == SNOR_MFR_SST ||
> >> -	    nor->info->flags & SPI_NOR_HAS_LOCK)
> >>  		nor->params.disable_block_protection = spi_nor_clear_sr_bp;
> >> +	}
> >>  
> >>  	/* Init flash parameters based on flash_info struct and SFDP */
> >>  	spi_nor_init_params(nor);  
> > 
> >   


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