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

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

[Tudor.Ambarus]

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

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 paramters will no longer be spread interleaved accross
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.

These batches are based of Boris Brezillon's work at:
https://patchwork.ozlabs.org/project/linux-mtd/list/?series=80353

These batches depend on m25p80 code move and a recent fix:
https://patchwork.ozlabs.org/project/linux-mtd/list/?series=120475
https://patchwork.ozlabs.org/project/linux-mtd/list/?series=122416

You can find my developing branch (not stable) at:
https://github.com/ambarus/linux-0day/tree/spi-nor/manuf-drv

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: Rework quad_enable()
  mtd: spi-nor: Split spi_nor_init_params()
  mtd: spi-nor: Rework the disabling of write protection at init

 drivers/mtd/spi-nor/spi-nor.c | 417 ++++++++++++++++++++++++------------------
 include/linux/mtd/spi-nor.h   |  23 ++-
 2 files changed, 260 insertions(+), 180 deletions(-)

-- 
2.9.5


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

[PATCH 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>
---
 drivers/mtd/spi-nor/spi-nor.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 28a64dbdaea9..ac00f90ebaa9 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -219,12 +219,17 @@ 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,
+			     struct spi_nor_flash_parameter *params);
 	int (*post_bfpt)(struct spi_nor *nor,
 			 const struct sfdp_parameter_header *bfpt_header,
 			 const struct sfdp_bfpt *bfpt,
@@ -4267,6 +4272,14 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
 	return err;
 }
 
+static void
+spi_nor_manufacturer_init_params(struct spi_nor *nor,
+				 struct spi_nor_flash_parameter *params)
+{
+	if (nor->info->fixups && nor->info->fixups->default_init)
+		return nor->info->fixups->default_init(nor, params);
+}
+
 static int spi_nor_init_params(struct spi_nor *nor,
 			       struct spi_nor_flash_parameter *params)
 {
@@ -4370,6 +4383,8 @@ static int spi_nor_init_params(struct spi_nor *nor,
 			params->quad_enable = info->quad_enable;
 	}
 
+	spi_nor_manufacturer_init_params(nor, params);
+
 	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/

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

[Boris Brezillon]

On Wed, 31 Jul 2019 09:03:27 +0000
<Tudor.Ambarus@microchip.com> wrote:

> 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 | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 28a64dbdaea9..ac00f90ebaa9 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -219,12 +219,17 @@ 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,
> +			     struct spi_nor_flash_parameter *params);
>  	int (*post_bfpt)(struct spi_nor *nor,
>  			 const struct sfdp_parameter_header *bfpt_header,
>  			 const struct sfdp_bfpt *bfpt,
> @@ -4267,6 +4272,14 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
>  	return err;
>  }
>  
> +static void
> +spi_nor_manufacturer_init_params(struct spi_nor *nor,
> +				 struct spi_nor_flash_parameter *params)
> +{
> +	if (nor->info->fixups && nor->info->fixups->default_init)
> +		return nor->info->fixups->default_init(nor, params);
> +}
> +
>  static int spi_nor_init_params(struct spi_nor *nor,
>  			       struct spi_nor_flash_parameter *params)
>  {
> @@ -4370,6 +4383,8 @@ static int spi_nor_init_params(struct spi_nor *nor,
>  			params->quad_enable = info->quad_enable;
>  	}
>  
> +	spi_nor_manufacturer_init_params(nor, params);
> +
>  	if ((info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
>  	    !(info->flags & SPI_NOR_SKIP_SFDP)) {
>  		struct spi_nor_flash_parameter sfdp_params;


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

[PATCH 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 | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index ac00f90ebaa9..94aba5ce1462 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -288,8 +288,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])
@@ -2268,6 +2266,22 @@ static struct spi_nor_fixups mx25l25635_fixups = {
 	.post_bfpt = mx25l25635_post_bfpt_fixups,
 };
 
+static void gd25q256_default_init(struct spi_nor *nor,
+				  struct spi_nor_flash_parameter *params)
+{
+	/*
+	 * 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 the default_init fixup hook.
+	 */
+	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.
@@ -2360,7 +2374,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 */
@@ -4372,15 +4386,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, params);
-- 
2.9.5


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

[PATCH 3/7] mtd: spi_nor: Rework quad_enable()

[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/core 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.

Get rid of the spi_nor_flash_parameter int (*quad_enable)() pointer to
function, as we always choose to overwrite the nor->quad_enable,
if needed.

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

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 94aba5ce1462..a906c36260c8 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -101,8 +101,6 @@ struct spi_nor_flash_parameter {
 	struct spi_nor_hwcaps		hwcaps;
 	struct spi_nor_read_command	reads[SNOR_CMD_READ_MAX];
 	struct spi_nor_pp_command	page_programs[SNOR_CMD_PP_MAX];
-
-	int (*quad_enable)(struct spi_nor *nor);
 };
 
 struct sfdp_parameter_header {
@@ -2275,7 +2273,7 @@ static void gd25q256_default_init(struct spi_nor *nor,
 	 * indicate the quad_enable method for this case, we need
 	 * set it in the default_init fixup hook.
 	 */
-	params->quad_enable = macronix_quad_enable;
+	nor->quad_enable = macronix_quad_enable;
 }
 
 static struct spi_nor_fixups gd25q256_fixups = {
@@ -3618,24 +3616,24 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
 	/* Quad Enable Requirements. */
 	switch (bfpt.dwords[BFPT_DWORD(15)] & BFPT_DWORD15_QER_MASK) {
 	case BFPT_DWORD15_QER_NONE:
-		params->quad_enable = NULL;
+		nor->quad_enable = NULL;
 		break;
 
 	case BFPT_DWORD15_QER_SR2_BIT1_BUGGY:
 	case BFPT_DWORD15_QER_SR2_BIT1_NO_RD:
-		params->quad_enable = spansion_no_read_cr_quad_enable;
+		nor->quad_enable = spansion_no_read_cr_quad_enable;
 		break;
 
 	case BFPT_DWORD15_QER_SR1_BIT6:
-		params->quad_enable = macronix_quad_enable;
+		nor->quad_enable = macronix_quad_enable;
 		break;
 
 	case BFPT_DWORD15_QER_SR2_BIT7:
-		params->quad_enable = sr2_bit7_quad_enable;
+		nor->quad_enable = sr2_bit7_quad_enable;
 		break;
 
 	case BFPT_DWORD15_QER_SR2_BIT1:
-		params->quad_enable = spansion_read_cr_quad_enable;
+		nor->quad_enable = spansion_read_cr_quad_enable;
 		break;
 
 	default:
@@ -4286,10 +4284,41 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
 	return err;
 }
 
+static void macronix_set_default_init(struct spi_nor *nor)
+{
+	nor->quad_enable = macronix_quad_enable;
+}
+
+static void st_micron_set_default_init(struct spi_nor *nor)
+{
+	nor->quad_enable = NULL;
+}
+
+static void spi_nor_mfr_init_params(struct spi_nor *nor,
+				    struct spi_nor_flash_parameter *params)
+{
+	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;
+	}
+}
+
 static void
 spi_nor_manufacturer_init_params(struct spi_nor *nor,
 				 struct spi_nor_flash_parameter *params)
 {
+	/* Init flash parameters based on MFR */
+	spi_nor_mfr_init_params(nor, params);
+
 	if (nor->info->fixups && nor->info->fixups->default_init)
 		return nor->info->fixups->default_init(nor, params);
 }
@@ -4369,25 +4398,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, params);
 
 	if ((info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
@@ -4569,7 +4579,6 @@ static int spi_nor_setup(struct spi_nor *nor,
 			 const struct spi_nor_hwcaps *hwcaps)
 {
 	u32 ignored_mask, shared_mask;
-	bool enable_quad_io;
 	int err;
 
 	/*
@@ -4617,21 +4626,23 @@ static int spi_nor_setup(struct spi_nor *nor,
 
 	/* Select the Sector Erase command. */
 	err = spi_nor_select_erase(nor, nor->info->sector_size);
-	if (err) {
+	if (err)
 		dev_err(nor->dev,
 			"can't select erase settings supported by both the SPI controller and memory.\n");
-		return err;
-	}
 
-	/* Enable Quad I/O if needed. */
-	enable_quad_io = (spi_nor_get_protocol_width(nor->read_proto) == 4 ||
-			  spi_nor_get_protocol_width(nor->write_proto) == 4);
-	if (enable_quad_io && params->quad_enable)
-		nor->quad_enable = params->quad_enable;
-	else
-		nor->quad_enable = NULL;
+	return err;
+}
 
-	return 0;
+static int spi_nor_quad_enable(struct spi_nor *nor)
+{
+	if (!nor->quad_enable)
+		return 0;
+
+	if (!(spi_nor_get_protocol_width(nor->read_proto) == 4 ||
+	      spi_nor_get_protocol_width(nor->write_proto) == 4))
+		return 0;
+
+	return nor->quad_enable(nor);
 }
 
 static int spi_nor_init(struct spi_nor *nor)
@@ -4650,12 +4661,10 @@ static int spi_nor_init(struct spi_nor *nor)
 		}
 	}
 
-	if (nor->quad_enable) {
-		err = nor->quad_enable(nor);
-		if (err) {
-			dev_err(nor->dev, "quad mode not supported\n");
-			return err;
-		}
+	err = spi_nor_quad_enable(nor);
+	if (err) {
+		dev_err(nor->dev, "quad mode not supported\n");
+		return err;
 	}
 
 	if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) {
@@ -4782,6 +4791,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	    nor->info->flags & SPI_NOR_HAS_LOCK)
 		nor->clear_sr_bp = spi_nor_clear_sr_bp;
 
+	/* Kept only for backward compatibility purpose. */
+	nor->quad_enable = spansion_quad_enable;
+
 	/* Parse the Serial Flash Discoverable Parameters table. */
 	ret = spi_nor_init_params(nor, &params);
 	if (ret)
@@ -4858,7 +4870,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	 * - select op codes for (Fast) Read, Page Program and Sector Erase.
 	 * - set the number of dummy cycles (mode cycles + wait states).
 	 * - set the SPI protocols for register and memory accesses.
-	 * - set the Quad Enable bit if needed (required by SPI x-y-4 protos).
 	 */
 	ret = spi_nor_setup(nor, &params, hwcaps);
 	if (ret)
-- 
2.9.5


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

Re: [PATCH 3/7] mtd: spi_nor: Rework quad_enable()

[Boris Brezillon]

On Wed, 31 Jul 2019 09:03:31 +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/core 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.
> 
> Get rid of the spi_nor_flash_parameter int (*quad_enable)() pointer to
> function, as we always choose to overwrite the nor->quad_enable,
> if needed.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 103 +++++++++++++++++++++++-------------------
>  1 file changed, 57 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 94aba5ce1462..a906c36260c8 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -101,8 +101,6 @@ struct spi_nor_flash_parameter {
>  	struct spi_nor_hwcaps		hwcaps;
>  	struct spi_nor_read_command	reads[SNOR_CMD_READ_MAX];
>  	struct spi_nor_pp_command	page_programs[SNOR_CMD_PP_MAX];
> -
> -	int (*quad_enable)(struct spi_nor *nor);
>  };
>  
>  struct sfdp_parameter_header {
> @@ -2275,7 +2273,7 @@ static void gd25q256_default_init(struct spi_nor *nor,
>  	 * indicate the quad_enable method for this case, we need
>  	 * set it in the default_init fixup hook.
>  	 */
> -	params->quad_enable = macronix_quad_enable;
> +	nor->quad_enable = macronix_quad_enable;
>  }
>  
>  static struct spi_nor_fixups gd25q256_fixups = {
> @@ -3618,24 +3616,24 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>  	/* Quad Enable Requirements. */
>  	switch (bfpt.dwords[BFPT_DWORD(15)] & BFPT_DWORD15_QER_MASK) {
>  	case BFPT_DWORD15_QER_NONE:
> -		params->quad_enable = NULL;
> +		nor->quad_enable = NULL;
>  		break;
>  
>  	case BFPT_DWORD15_QER_SR2_BIT1_BUGGY:
>  	case BFPT_DWORD15_QER_SR2_BIT1_NO_RD:
> -		params->quad_enable = spansion_no_read_cr_quad_enable;
> +		nor->quad_enable = spansion_no_read_cr_quad_enable;
>  		break;
>  
>  	case BFPT_DWORD15_QER_SR1_BIT6:
> -		params->quad_enable = macronix_quad_enable;
> +		nor->quad_enable = macronix_quad_enable;
>  		break;
>  
>  	case BFPT_DWORD15_QER_SR2_BIT7:
> -		params->quad_enable = sr2_bit7_quad_enable;
> +		nor->quad_enable = sr2_bit7_quad_enable;
>  		break;
>  
>  	case BFPT_DWORD15_QER_SR2_BIT1:
> -		params->quad_enable = spansion_read_cr_quad_enable;
> +		nor->quad_enable = spansion_read_cr_quad_enable;
>  		break;
>  
>  	default:
> @@ -4286,10 +4284,41 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
>  	return err;
>  }
>  
> +static void macronix_set_default_init(struct spi_nor *nor)
> +{
> +	nor->quad_enable = macronix_quad_enable;
> +}
> +
> +static void st_micron_set_default_init(struct spi_nor *nor)
> +{
> +	nor->quad_enable = NULL;
> +}
> +
> +static void spi_nor_mfr_init_params(struct spi_nor *nor,
> +				    struct spi_nor_flash_parameter *params)

So now we have spi_nor_mfr_init_params() and
spi_nor_manufacturer_init_params(), that's a bit confusing. Can't we
just inline the below code in the spi_nor_manufacturer_init_params()
func? I guess this func will be removed anyway, so maybe it's not
such a big deal.

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

> +{
> +	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;
> +	}
> +}
> +
>  static void
>  spi_nor_manufacturer_init_params(struct spi_nor *nor,
>  				 struct spi_nor_flash_parameter *params)
>  {
> +	/* Init flash parameters based on MFR */
> +	spi_nor_mfr_init_params(nor, params);
> +
>  	if (nor->info->fixups && nor->info->fixups->default_init)
>  		return nor->info->fixups->default_init(nor, params);
>  }
> @@ -4369,25 +4398,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, params);
>  
>  	if ((info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
> @@ -4569,7 +4579,6 @@ static int spi_nor_setup(struct spi_nor *nor,
>  			 const struct spi_nor_hwcaps *hwcaps)
>  {
>  	u32 ignored_mask, shared_mask;
> -	bool enable_quad_io;
>  	int err;
>  
>  	/*
> @@ -4617,21 +4626,23 @@ static int spi_nor_setup(struct spi_nor *nor,
>  
>  	/* Select the Sector Erase command. */
>  	err = spi_nor_select_erase(nor, nor->info->sector_size);
> -	if (err) {
> +	if (err)
>  		dev_err(nor->dev,
>  			"can't select erase settings supported by both the SPI controller and memory.\n");
> -		return err;
> -	}
>  
> -	/* Enable Quad I/O if needed. */
> -	enable_quad_io = (spi_nor_get_protocol_width(nor->read_proto) == 4 ||
> -			  spi_nor_get_protocol_width(nor->write_proto) == 4);
> -	if (enable_quad_io && params->quad_enable)
> -		nor->quad_enable = params->quad_enable;
> -	else
> -		nor->quad_enable = NULL;
> +	return err;
> +}
>  
> -	return 0;
> +static int spi_nor_quad_enable(struct spi_nor *nor)
> +{
> +	if (!nor->quad_enable)
> +		return 0;
> +
> +	if (!(spi_nor_get_protocol_width(nor->read_proto) == 4 ||
> +	      spi_nor_get_protocol_width(nor->write_proto) == 4))
> +		return 0;
> +
> +	return nor->quad_enable(nor);
>  }
>  
>  static int spi_nor_init(struct spi_nor *nor)
> @@ -4650,12 +4661,10 @@ static int spi_nor_init(struct spi_nor *nor)
>  		}
>  	}
>  
> -	if (nor->quad_enable) {
> -		err = nor->quad_enable(nor);
> -		if (err) {
> -			dev_err(nor->dev, "quad mode not supported\n");
> -			return err;
> -		}
> +	err = spi_nor_quad_enable(nor);
> +	if (err) {
> +		dev_err(nor->dev, "quad mode not supported\n");
> +		return err;
>  	}
>  
>  	if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) {
> @@ -4782,6 +4791,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	    nor->info->flags & SPI_NOR_HAS_LOCK)
>  		nor->clear_sr_bp = spi_nor_clear_sr_bp;
>  
> +	/* Kept only for backward compatibility purpose. */
> +	nor->quad_enable = spansion_quad_enable;
> +
>  	/* Parse the Serial Flash Discoverable Parameters table. */
>  	ret = spi_nor_init_params(nor, &params);
>  	if (ret)
> @@ -4858,7 +4870,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	 * - select op codes for (Fast) Read, Page Program and Sector Erase.
>  	 * - set the number of dummy cycles (mode cycles + wait states).
>  	 * - set the SPI protocols for register and memory accesses.
> -	 * - set the Quad Enable bit if needed (required by SPI x-y-4 protos).
>  	 */
>  	ret = spi_nor_setup(nor, &params, hwcaps);
>  	if (ret)


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

Re: [PATCH 3/7] mtd: spi_nor: Rework quad_enable()

[Tudor.Ambarus]

On 08/01/2019 09:29 AM, Boris Brezillon wrote:
> External E-Mail
> 
> 
> On Wed, 31 Jul 2019 09:03:31 +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/core 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.
>>
>> Get rid of the spi_nor_flash_parameter int (*quad_enable)() pointer to
>> function, as we always choose to overwrite the nor->quad_enable,
>> if needed.
>>
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>> ---
>>  drivers/mtd/spi-nor/spi-nor.c | 103 +++++++++++++++++++++++-------------------
>>  1 file changed, 57 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
>> index 94aba5ce1462..a906c36260c8 100644
>> --- a/drivers/mtd/spi-nor/spi-nor.c
>> +++ b/drivers/mtd/spi-nor/spi-nor.c
>> @@ -101,8 +101,6 @@ struct spi_nor_flash_parameter {
>>  	struct spi_nor_hwcaps		hwcaps;
>>  	struct spi_nor_read_command	reads[SNOR_CMD_READ_MAX];
>>  	struct spi_nor_pp_command	page_programs[SNOR_CMD_PP_MAX];
>> -
>> -	int (*quad_enable)(struct spi_nor *nor);
>>  };
>>  
>>  struct sfdp_parameter_header {
>> @@ -2275,7 +2273,7 @@ static void gd25q256_default_init(struct spi_nor *nor,
>>  	 * indicate the quad_enable method for this case, we need
>>  	 * set it in the default_init fixup hook.
>>  	 */
>> -	params->quad_enable = macronix_quad_enable;
>> +	nor->quad_enable = macronix_quad_enable;
>>  }
>>  
>>  static struct spi_nor_fixups gd25q256_fixups = {
>> @@ -3618,24 +3616,24 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>>  	/* Quad Enable Requirements. */
>>  	switch (bfpt.dwords[BFPT_DWORD(15)] & BFPT_DWORD15_QER_MASK) {
>>  	case BFPT_DWORD15_QER_NONE:
>> -		params->quad_enable = NULL;
>> +		nor->quad_enable = NULL;
>>  		break;
>>  
>>  	case BFPT_DWORD15_QER_SR2_BIT1_BUGGY:
>>  	case BFPT_DWORD15_QER_SR2_BIT1_NO_RD:
>> -		params->quad_enable = spansion_no_read_cr_quad_enable;
>> +		nor->quad_enable = spansion_no_read_cr_quad_enable;
>>  		break;
>>  
>>  	case BFPT_DWORD15_QER_SR1_BIT6:
>> -		params->quad_enable = macronix_quad_enable;
>> +		nor->quad_enable = macronix_quad_enable;
>>  		break;
>>  
>>  	case BFPT_DWORD15_QER_SR2_BIT7:
>> -		params->quad_enable = sr2_bit7_quad_enable;
>> +		nor->quad_enable = sr2_bit7_quad_enable;
>>  		break;
>>  
>>  	case BFPT_DWORD15_QER_SR2_BIT1:
>> -		params->quad_enable = spansion_read_cr_quad_enable;
>> +		nor->quad_enable = spansion_read_cr_quad_enable;
>>  		break;
>>  
>>  	default:
>> @@ -4286,10 +4284,41 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
>>  	return err;
>>  }
>>  
>> +static void macronix_set_default_init(struct spi_nor *nor)
>> +{
>> +	nor->quad_enable = macronix_quad_enable;
>> +}
>> +
>> +static void st_micron_set_default_init(struct spi_nor *nor)
>> +{
>> +	nor->quad_enable = NULL;
>> +}
>> +
>> +static void spi_nor_mfr_init_params(struct spi_nor *nor,
>> +				    struct spi_nor_flash_parameter *params)
> 
> So now we have spi_nor_mfr_init_params() and
> spi_nor_manufacturer_init_params(), that's a bit confusing. Can't we
> just inline the below code in the spi_nor_manufacturer_init_params()
> func? I guess this func will be removed anyway, so maybe it's not
> such a big deal.
> 

Will do. spi_nor_mfr_init_params() would have been removed anyway when moving
the manufacturer specific code out of the core. I chose separate function to
underline that these selects are done solely based on JEDEC_MFR, while the
default_init() sets data that can't be deduced from JEDEC_MFR. But might be
confusing, so will remove it.

> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> 
>> +{
>> +	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;
>> +	}
>> +}
>> +
>>  static void
>>  spi_nor_manufacturer_init_params(struct spi_nor *nor,
>>  				 struct spi_nor_flash_parameter *params)
>>  {
>> +	/* Init flash parameters based on MFR */
>> +	spi_nor_mfr_init_params(nor, params);
>> +
>>  	if (nor->info->fixups && nor->info->fixups->default_init)
>>  		return nor->info->fixups->default_init(nor, params);
>>  }
>> @@ -4369,25 +4398,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, params);
>>  
>>  	if ((info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
>> @@ -4569,7 +4579,6 @@ static int spi_nor_setup(struct spi_nor *nor,
>>  			 const struct spi_nor_hwcaps *hwcaps)
>>  {
>>  	u32 ignored_mask, shared_mask;
>> -	bool enable_quad_io;
>>  	int err;
>>  
>>  	/*
>> @@ -4617,21 +4626,23 @@ static int spi_nor_setup(struct spi_nor *nor,
>>  
>>  	/* Select the Sector Erase command. */
>>  	err = spi_nor_select_erase(nor, nor->info->sector_size);
>> -	if (err) {
>> +	if (err)
>>  		dev_err(nor->dev,
>>  			"can't select erase settings supported by both the SPI controller and memory.\n");
>> -		return err;
>> -	}
>>  
>> -	/* Enable Quad I/O if needed. */
>> -	enable_quad_io = (spi_nor_get_protocol_width(nor->read_proto) == 4 ||
>> -			  spi_nor_get_protocol_width(nor->write_proto) == 4);
>> -	if (enable_quad_io && params->quad_enable)
>> -		nor->quad_enable = params->quad_enable;
>> -	else
>> -		nor->quad_enable = NULL;
>> +	return err;
>> +}
>>  
>> -	return 0;
>> +static int spi_nor_quad_enable(struct spi_nor *nor)
>> +{
>> +	if (!nor->quad_enable)
>> +		return 0;
>> +
>> +	if (!(spi_nor_get_protocol_width(nor->read_proto) == 4 ||
>> +	      spi_nor_get_protocol_width(nor->write_proto) == 4))
>> +		return 0;
>> +
>> +	return nor->quad_enable(nor);
>>  }
>>  
>>  static int spi_nor_init(struct spi_nor *nor)
>> @@ -4650,12 +4661,10 @@ static int spi_nor_init(struct spi_nor *nor)
>>  		}
>>  	}
>>  
>> -	if (nor->quad_enable) {
>> -		err = nor->quad_enable(nor);
>> -		if (err) {
>> -			dev_err(nor->dev, "quad mode not supported\n");
>> -			return err;
>> -		}
>> +	err = spi_nor_quad_enable(nor);
>> +	if (err) {
>> +		dev_err(nor->dev, "quad mode not supported\n");
>> +		return err;
>>  	}
>>  
>>  	if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) {
>> @@ -4782,6 +4791,9 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>>  	    nor->info->flags & SPI_NOR_HAS_LOCK)
>>  		nor->clear_sr_bp = spi_nor_clear_sr_bp;
>>  
>> +	/* Kept only for backward compatibility purpose. */
>> +	nor->quad_enable = spansion_quad_enable;
>> +
>>  	/* Parse the Serial Flash Discoverable Parameters table. */
>>  	ret = spi_nor_init_params(nor, &params);
>>  	if (ret)
>> @@ -4858,7 +4870,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>>  	 * - select op codes for (Fast) Read, Page Program and Sector Erase.
>>  	 * - set the number of dummy cycles (mode cycles + wait states).
>>  	 * - set the SPI protocols for register and memory accesses.
>> -	 * - set the Quad Enable bit if needed (required by SPI x-y-4 protos).
>>  	 */
>>  	ret = spi_nor_setup(nor, &params, hwcaps);
>>  	if (ret)
> 
> 
> 
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 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_default_init_params()
	spi_nor_manufacturer_init_params()
	spi_nor_sfdp_init_params()
}

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 | 58 ++++++++++++++++++++++++-------------------
 1 file changed, 32 insertions(+), 26 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index a906c36260c8..b2e72668e7ab 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -4312,6 +4312,25 @@ static void spi_nor_mfr_init_params(struct spi_nor *nor,
 	}
 }
 
+static void spi_nor_sfdp_init_params(struct spi_nor *nor,
+				     struct spi_nor_flash_parameter *params)
+{
+	struct spi_nor_flash_parameter sfdp_params;
+	struct spi_nor_erase_map prev_map;
+
+	memcpy(&sfdp_params, params, sizeof(sfdp_params));
+	memcpy(&prev_map, &nor->erase_map, sizeof(prev_map));
+
+	if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
+		nor->addr_width = 0;
+		nor->flags &= ~SNOR_F_4B_OPCODES;
+		/* restore previous erase map */
+		memcpy(&nor->erase_map, &prev_map, sizeof(nor->erase_map));
+	} else {
+		memcpy(params, &sfdp_params, sizeof(*params));
+	}
+}
+
 static void
 spi_nor_manufacturer_init_params(struct spi_nor *nor,
 				 struct spi_nor_flash_parameter *params)
@@ -4323,8 +4342,8 @@ spi_nor_manufacturer_init_params(struct spi_nor *nor,
 		return nor->info->fixups->default_init(nor, params);
 }
 
-static int spi_nor_init_params(struct spi_nor *nor,
-			       struct spi_nor_flash_parameter *params)
+static void spi_nor_default_init_params(struct spi_nor *nor,
+					struct spi_nor_flash_parameter *params)
 {
 	struct spi_nor_erase_map *map = &nor->erase_map;
 	const struct flash_info *info = nor->info;
@@ -4397,29 +4416,18 @@ 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_manufacturer_init_params(nor, params);
-
-	if ((info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
-	    !(info->flags & SPI_NOR_SKIP_SFDP)) {
-		struct spi_nor_flash_parameter sfdp_params;
-		struct spi_nor_erase_map prev_map;
-
-		memcpy(&sfdp_params, params, sizeof(sfdp_params));
-		memcpy(&prev_map, &nor->erase_map, sizeof(prev_map));
+static void spi_nor_init_params(struct spi_nor *nor,
+				struct spi_nor_flash_parameter *params)
+{
+	spi_nor_default_init_params(nor, params);
 
-		if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
-			nor->addr_width = 0;
-			nor->flags &= ~SNOR_F_4B_OPCODES;
-			/* restore previous erase map */
-			memcpy(&nor->erase_map, &prev_map,
-			       sizeof(nor->erase_map));
-		} else {
-			memcpy(params, &sfdp_params, sizeof(*params));
-		}
-	}
+	spi_nor_manufacturer_init_params(nor, 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, params);
 }
 
 static int spi_nor_select_read(struct spi_nor *nor,
@@ -4794,10 +4802,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	/* Kept only for backward compatibility purpose. */
 	nor->quad_enable = spansion_quad_enable;
 
-	/* Parse the Serial Flash Discoverable Parameters table. */
-	ret = spi_nor_init_params(nor, &params);
-	if (ret)
-		return ret;
+	/* Init flash parameters based on flash_info struct and SFDP */
+	spi_nor_init_params(nor, &params);
 
 	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 4/7] mtd: spi-nor: Split spi_nor_init_params()

[Boris Brezillon]

On Wed, 31 Jul 2019 09:03:33 +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_default_init_params()
> 	spi_nor_manufacturer_init_params()
> 	spi_nor_sfdp_init_params()
> }
> 
> spi_nor_init_params() becomes of type void, as all its children
> return void.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

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

> ---
>  drivers/mtd/spi-nor/spi-nor.c | 58 ++++++++++++++++++++++++-------------------
>  1 file changed, 32 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index a906c36260c8..b2e72668e7ab 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -4312,6 +4312,25 @@ static void spi_nor_mfr_init_params(struct spi_nor *nor,
>  	}
>  }
>  
> +static void spi_nor_sfdp_init_params(struct spi_nor *nor,
> +				     struct spi_nor_flash_parameter *params)
> +{
> +	struct spi_nor_flash_parameter sfdp_params;
> +	struct spi_nor_erase_map prev_map;
> +
> +	memcpy(&sfdp_params, params, sizeof(sfdp_params));
> +	memcpy(&prev_map, &nor->erase_map, sizeof(prev_map));
> +
> +	if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
> +		nor->addr_width = 0;
> +		nor->flags &= ~SNOR_F_4B_OPCODES;
> +		/* restore previous erase map */
> +		memcpy(&nor->erase_map, &prev_map, sizeof(nor->erase_map));
> +	} else {
> +		memcpy(params, &sfdp_params, sizeof(*params));
> +	}
> +}
> +
>  static void
>  spi_nor_manufacturer_init_params(struct spi_nor *nor,
>  				 struct spi_nor_flash_parameter *params)
> @@ -4323,8 +4342,8 @@ spi_nor_manufacturer_init_params(struct spi_nor *nor,
>  		return nor->info->fixups->default_init(nor, params);
>  }
>  
> -static int spi_nor_init_params(struct spi_nor *nor,
> -			       struct spi_nor_flash_parameter *params)
> +static void spi_nor_default_init_params(struct spi_nor *nor,
> +					struct spi_nor_flash_parameter *params)
>  {
>  	struct spi_nor_erase_map *map = &nor->erase_map;
>  	const struct flash_info *info = nor->info;
> @@ -4397,29 +4416,18 @@ 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_manufacturer_init_params(nor, params);
> -
> -	if ((info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
> -	    !(info->flags & SPI_NOR_SKIP_SFDP)) {
> -		struct spi_nor_flash_parameter sfdp_params;
> -		struct spi_nor_erase_map prev_map;
> -
> -		memcpy(&sfdp_params, params, sizeof(sfdp_params));
> -		memcpy(&prev_map, &nor->erase_map, sizeof(prev_map));
> +static void spi_nor_init_params(struct spi_nor *nor,
> +				struct spi_nor_flash_parameter *params)
> +{
> +	spi_nor_default_init_params(nor, params);
>  
> -		if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
> -			nor->addr_width = 0;
> -			nor->flags &= ~SNOR_F_4B_OPCODES;
> -			/* restore previous erase map */
> -			memcpy(&nor->erase_map, &prev_map,
> -			       sizeof(nor->erase_map));
> -		} else {
> -			memcpy(params, &sfdp_params, sizeof(*params));
> -		}
> -	}
> +	spi_nor_manufacturer_init_params(nor, 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, params);
>  }
>  
>  static int spi_nor_select_read(struct spi_nor *nor,
> @@ -4794,10 +4802,8 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	/* Kept only for backward compatibility purpose. */
>  	nor->quad_enable = spansion_quad_enable;
>  
> -	/* Parse the Serial Flash Discoverable Parameters table. */
> -	ret = spi_nor_init_params(nor, &params);
> -	if (ret)
> -		return ret;
> +	/* Init flash parameters based on flash_info struct and SFDP */
> +	spi_nor_init_params(nor, &params);
>  
>  	if (!mtd->name)
>  		mtd->name = dev_name(dev);


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

[PATCH 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.

set_4byte methods can be amended when parsing BFPT.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 119 ++++++++++++++++++++++--------------------
 include/linux/mtd/spi-nor.h   |   3 ++
 2 files changed, 64 insertions(+), 58 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index b2e72668e7ab..e35aae88d38b 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -758,6 +758,21 @@ static void spi_nor_set_4byte_opcodes(struct spi_nor *nor)
 	}
 }
 
+static int spi_nor_write_ear(struct spi_nor *nor, u8 ear)
+{
+	if (nor->spimem) {
+		struct spi_mem_op op =
+			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WREAR, 1),
+				   SPI_MEM_OP_NO_ADDR,
+				   SPI_MEM_OP_NO_DUMMY,
+				   SPI_MEM_OP_DATA_OUT(0, NULL, 1));
+
+		return spi_nor_data_op(nor, &op, &ear, 1);
+	}
+
+	return nor->write_reg(nor, SPINOR_OP_WREAR, &ear, 1);
+}
+
 static int macronix_set_4byte(struct spi_nor *nor, bool enable)
 {
 	if (nor->spimem) {
@@ -777,6 +792,39 @@ 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 winbond_set_4byte(struct spi_nor *nor, bool enable)
+{
+	int ret;
+
+	ret = macronix_set_4byte(nor, enable);
+	if (ret || enable)
+		return ret;
+
+	/*
+	 * 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);
+	nor->cmd_buf[0] = 0;
+	ret = spi_nor_write_ear(nor, nor->cmd_buf[0]);
+	write_disable(nor);
+
+	return ret;
+}
+
 static int spansion_set_4byte(struct spi_nor *nor, bool enable)
 {
 	u8 quad_en = enable << 7;
@@ -794,62 +842,6 @@ static int spansion_set_4byte(struct spi_nor *nor, bool enable)
 	return nor->write_reg(nor, SPINOR_OP_BRWR, &quad_en, 1);
 }
 
-static int spi_nor_write_ear(struct spi_nor *nor, u8 ear)
-{
-	if (nor->spimem) {
-		struct spi_mem_op op =
-			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_WREAR, 1),
-				   SPI_MEM_OP_NO_ADDR,
-				   SPI_MEM_OP_NO_DUMMY,
-				   SPI_MEM_OP_DATA_OUT(0, NULL, 1));
-
-		return spi_nor_data_op(nor, &op, &ear, 1);
-	}
-
-	return nor->write_reg(nor, SPINOR_OP_WREAR, &ear, 1);
-}
-
-/* Enable/disable 4-byte addressing mode. */
-static int 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);
-
-		status = macronix_set_4byte(nor, enable);
-		if (need_wren)
-			write_disable(nor);
-
-		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);
-		}
-
-		return status;
-	default:
-		/* Spansion style */
-		return spansion_set_4byte(nor, enable);
-	}
-}
-
 static int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)
 {
 	if (nor->spimem) {
@@ -4287,11 +4279,18 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
 static void macronix_set_default_init(struct spi_nor *nor)
 {
 	nor->quad_enable = macronix_quad_enable;
+	nor->set_4byte = macronix_set_4byte;
 }
 
 static void st_micron_set_default_init(struct spi_nor *nor)
 {
 	nor->quad_enable = NULL;
+	nor->set_4byte = st_micron_set_4byte;
+}
+
+static void winbond_set_default_init(struct spi_nor *nor)
+{
+	nor->set_4byte = winbond_set_4byte;
 }
 
 static void spi_nor_mfr_init_params(struct spi_nor *nor,
@@ -4307,6 +4306,9 @@ static void spi_nor_mfr_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;
 	}
@@ -4685,7 +4687,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->set_4byte(nor, true);
 	}
 
 	return 0;
@@ -4709,7 +4711,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->set_4byte(nor, false);
 }
 EXPORT_SYMBOL_GPL(spi_nor_restore);
 
@@ -4801,6 +4803,7 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 
 	/* Kept only for backward compatibility purpose. */
 	nor->quad_enable = spansion_quad_enable;
+	nor->set_4byte = spansion_set_4byte;
 
 	/* Init flash parameters based on flash_info struct and SFDP */
 	spi_nor_init_params(nor, &params);
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 4521a38452d6..a434ab7a53e6 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -378,6 +378,8 @@ struct flash_info;
  * @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
  * @quad_enable:	[FLASH-SPECIFIC] enables SPI NOR quad mode
+ * @set_4byte:		[FLASH-SPECIFIC] puts the SPI NOR in 4 byte addressing
+ *                      mode
  * @clear_sr_bp:	[FLASH-SPECIFIC] clears the Block Protection Bits from
  *			the SPI NOR Status Register.
  *			completely locked
@@ -420,6 +422,7 @@ struct spi_nor {
 	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);
 	int (*quad_enable)(struct spi_nor *nor);
+	int (*set_4byte)(struct spi_nor *nor, bool enable);
 	int (*clear_sr_bp)(struct spi_nor *nor);
 
 	void *priv;
-- 
2.9.5


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

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

[Tudor.Ambarus]

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

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

stm_locking_ops, the legacy locking operations, can be overwritten
later on by implementing manufacturer specific default_init() hooks.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
[tudor.ambarus@microchip.com: use ->default_init() hook]
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 37 +++++++++++++++++++------------------
 include/linux/mtd/spi-nor.h   | 14 ++++++++++++++
 2 files changed, 33 insertions(+), 18 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index e35aae88d38b..95ca5dd96403 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1743,6 +1743,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);
@@ -1752,7 +1758,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->locking_ops->lock(nor, ofs, len);
 
 	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_UNLOCK);
 	return ret;
@@ -1767,7 +1773,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->locking_ops->unlock(nor, ofs, len);
 
 	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_LOCK);
 	return ret;
@@ -1782,7 +1788,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->locking_ops->is_locked(nor, ofs, len);
 
 	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_LOCK);
 	return ret;
@@ -4805,6 +4811,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	nor->quad_enable = spansion_quad_enable;
 	nor->set_4byte = spansion_set_4byte;
 
+	/* Default locking operations. */
+	if (info->flags & SPI_NOR_HAS_LOCK)
+		nor->locking_ops = &stm_locking_ops;
+
 	/* Init flash parameters based on flash_info struct and SFDP */
 	spi_nor_init_params(nor, &params);
 
@@ -4819,21 +4829,6 @@ 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) {
-		mtd->_lock = spi_nor_lock;
-		mtd->_unlock = spi_nor_unlock;
-		mtd->_is_locked = spi_nor_is_locked;
-	}
-
 	/* sst nor chips use AAI word program */
 	if (info->flags & SST_WRITE)
 		mtd->_write = sst_write;
@@ -4874,6 +4869,12 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	if (info->flags & SPI_NOR_NO_FR)
 		params.hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
 
+	if (nor->locking_ops) {
+		mtd->_lock = spi_nor_lock;
+		mtd->_unlock = spi_nor_unlock;
+		mtd->_is_locked = spi_nor_is_locked;
+	}
+
 	/*
 	 * Configure the SPI memory:
 	 * - select op codes for (Fast) Read, Page Program and Sector Erase.
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index a434ab7a53e6..bd68ec5a10e7 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -425,9 +425,23 @@ struct spi_nor {
 	int (*set_4byte)(struct spi_nor *nor, bool enable);
 	int (*clear_sr_bp)(struct spi_nor *nor);
 
+	const struct spi_nor_locking_ops *locking_ops;
+
 	void *priv;
 };
 
+/**
+ * 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);
+};
+
 static u64 __maybe_unused
 spi_nor_region_is_last(const struct spi_nor_erase_region *region)
 {
-- 
2.9.5


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

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

[Vignesh Raghavendra]

Hi Tudor,

On 31-Jul-19 2:33 PM, Tudor.Ambarus@microchip.com wrote:
> From: Boris Brezillon <boris.brezillon@bootlin.com>
> 
> Move the locking hooks in a separate struct so that we have just
> one field to update when we change the locking implementation.
> 
> stm_locking_ops, the legacy locking operations, can be overwritten
> later on by implementing manufacturer specific default_init() hooks.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> [tudor.ambarus@microchip.com: use ->default_init() hook]
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

[...]

> @@ -1782,7 +1788,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->locking_ops->is_locked(nor, ofs, len);
>  
>  	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_LOCK);
>  	return ret;
> @@ -4805,6 +4811,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	nor->quad_enable = spansion_quad_enable;
>  	nor->set_4byte = spansion_set_4byte;
>  
> +	/* Default locking operations. */
> +	if (info->flags & SPI_NOR_HAS_LOCK)
> +		nor->locking_ops = &stm_locking_ops;
> +

This condition is different than how lock/unlock ops are populated
today. We would need to add SPI_NOR_HAS_LOCK to all SNOR_MFR_ST and
SNOR_MFR_MICRON entries to be backward compatible or keep the condition
as is.

>  	/* Init flash parameters based on flash_info struct and SFDP */
>  	spi_nor_init_params(nor, &params);
>  
> @@ -4819,21 +4829,6 @@ 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;
> -	}
> -

[...]

> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index a434ab7a53e6..bd68ec5a10e7 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -425,9 +425,23 @@ struct spi_nor {
>  	int (*set_4byte)(struct spi_nor *nor, bool enable);
>  	int (*clear_sr_bp)(struct spi_nor *nor);
>  
> +	const struct spi_nor_locking_ops *locking_ops;
> +

Also, to be consistent, document this new member.


>  	void *priv;
>  };
>  
> +/**
> + * 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);

checkpatch does not like uint64_t. Please changes these to size_t

Regards
Vignesh


> +};
> +
>  static u64 __maybe_unused
>  spi_nor_region_is_last(const struct spi_nor_erase_region *region)
>  {
> 

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

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

[Tudor.Ambarus]

On 08/04/2019 05:36 PM, Vignesh Raghavendra wrote:
> External E-Mail
> 
> 
> Hi Tudor,
> 
> On 31-Jul-19 2:33 PM, Tudor.Ambarus@microchip.com wrote:
>> From: Boris Brezillon <boris.brezillon@bootlin.com>
>>
>> Move the locking hooks in a separate struct so that we have just
>> one field to update when we change the locking implementation.
>>
>> stm_locking_ops, the legacy locking operations, can be overwritten
>> later on by implementing manufacturer specific default_init() hooks.
>>
>> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
>> [tudor.ambarus@microchip.com: use ->default_init() hook]
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> [...]
> 
>> @@ -1782,7 +1788,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->locking_ops->is_locked(nor, ofs, len);
>>  
>>  	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_LOCK);
>>  	return ret;
>> @@ -4805,6 +4811,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>>  	nor->quad_enable = spansion_quad_enable;
>>  	nor->set_4byte = spansion_set_4byte;
>>  
>> +	/* Default locking operations. */
>> +	if (info->flags & SPI_NOR_HAS_LOCK)
>> +		nor->locking_ops = &stm_locking_ops;
>> +
> 
> This condition is different than how lock/unlock ops are populated
> today. We would need to add SPI_NOR_HAS_LOCK to all SNOR_MFR_ST and
> SNOR_MFR_MICRON entries to be backward compatible or keep the condition
> as is.

Will do, thanks!

> 
>>  	/* Init flash parameters based on flash_info struct and SFDP */
>>  	spi_nor_init_params(nor, &params);
>>  
>> @@ -4819,21 +4829,6 @@ 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;
>> -	}
>> -
> 
> [...]
> 
>> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
>> index a434ab7a53e6..bd68ec5a10e7 100644
>> --- a/include/linux/mtd/spi-nor.h
>> +++ b/include/linux/mtd/spi-nor.h
>> @@ -425,9 +425,23 @@ struct spi_nor {
>>  	int (*set_4byte)(struct spi_nor *nor, bool enable);
>>  	int (*clear_sr_bp)(struct spi_nor *nor);
>>  
>> +	const struct spi_nor_locking_ops *locking_ops;
>> +
> 
> Also, to be consistent, document this new member.

Will do.
> 
> 
>>  	void *priv;
>>  };
>>  
>> +/**
>> + * 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);
> 
> checkpatch does not like uint64_t. Please changes these to size_t

This respects what struct mtd_info is expecting:

        int (*_lock) (struct mtd_info *mtd, loff_t ofs, uint64_t len);
        int (*_unlock) (struct mtd_info *mtd, loff_t ofs, uint64_t len);
        int (*_is_locked) (struct mtd_info *mtd, loff_t ofs, uint64_t len);

I haven't seen the warnings, would you mind pasting them?
./scripts/checkpatch.pl --strict 6-7-mtd-spi-nor-Rework-the-SPI-NOR-lock-unlock-logic.patch
total: 0 errors, 0 warnings, 0 checks, 102 lines checked

6-7-mtd-spi-nor-Rework-the-SPI-NOR-lock-unlock-logic.patch has no obvious style problems and is ready for submission.

Cheers,
ta

> 
> Regards
> Vignesh
> 
> 
>> +};
>> +
>>  static u64 __maybe_unused
>>  spi_nor_region_is_last(const struct spi_nor_erase_region *region)
>>  {
>>
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

[Vignesh Raghavendra]

On 05/08/19 1:30 PM, Tudor.Ambarus@microchip.com wrote:
>>
>> On 31-Jul-19 2:33 PM, Tudor.Ambarus@microchip.com wrote:
>>> From: Boris Brezillon <boris.brezillon@bootlin.com>
>>>
>>> Move the locking hooks in a separate struct so that we have just
>>> one field to update when we change the locking implementation.
>>>
>>> stm_locking_ops, the legacy locking operations, can be overwritten
>>> later on by implementing manufacturer specific default_init() hooks.
>>>
>>> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
>>> [tudor.ambarus@microchip.com: use ->default_init() hook]
>>> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
>>
>> [...]
[...]
>>>  
>>> +/**
>>> + * 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);
>>
>> checkpatch does not like uint64_t. Please changes these to size_t
> 
> This respects what struct mtd_info is expecting:
> 
>         int (*_lock) (struct mtd_info *mtd, loff_t ofs, uint64_t len);
>         int (*_unlock) (struct mtd_info *mtd, loff_t ofs, uint64_t len);
>         int (*_is_locked) (struct mtd_info *mtd, loff_t ofs, uint64_t len);
> 
> I haven't seen the warnings, would you mind pasting them?
> ./scripts/checkpatch.pl --strict 6-7-mtd-spi-nor-Rework-the-SPI-NOR-lock-unlock-logic.patch
> total: 0 errors, 0 warnings, 0 checks, 102 lines checked
> 
> 6-7-mtd-spi-nor-Rework-the-SPI-NOR-lock-unlock-logic.patch has no obvious style problems and is ready for submission.
> 

Hmm, seems to be emitted only for certain type of declarations. Not sure 
whats the pattern here. Warning is something like:

CHECK: Prefer kernel type 'u64' over 'uint64_t'

from: https://elixir.bootlin.com/linux/latest/source/scripts/checkpatch.pl#L5906


-- 
Regards
Vignesh

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

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

[Tudor.Ambarus]

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

Rename clear_sr_bp()/disable_write_protection() to better indicate
what the function does.

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 | 64 +++++++++++++++++++++++++++++--------------
 include/linux/mtd/spi-nor.h   |  6 ++--
 2 files changed, 46 insertions(+), 24 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 95ca5dd96403..9b9f9b530207 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -4282,6 +4282,16 @@ static int spi_nor_parse_sfdp(struct spi_nor *nor,
 	return err;
 }
 
+static void atmel_set_default_init(struct spi_nor *nor)
+{
+	nor->disable_write_protection = spi_nor_clear_sr_bp;
+}
+
+static void intel_set_default_init(struct spi_nor *nor)
+{
+	nor->disable_write_protection = spi_nor_clear_sr_bp;
+}
+
 static void macronix_set_default_init(struct spi_nor *nor)
 {
 	nor->quad_enable = macronix_quad_enable;
@@ -4303,6 +4313,14 @@ static void spi_nor_mfr_init_params(struct spi_nor *nor,
 				    struct spi_nor_flash_parameter *params)
 {
 	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;
@@ -4649,6 +4667,23 @@ static int spi_nor_setup(struct spi_nor *nor,
 	return err;
 }
 
+static int spi_nor_disable_write_protection(struct spi_nor *nor)
+{
+	if (!nor->disable_write_protection)
+		return 0;
+
+	/*
+	 * In case of the legacy quad enable requirements are set, if the
+	 * configuration register Quad Enable bit is one, only the the
+	 * Write Status (01h) command with two data bytes may be used to clear
+	 * the block protection bits.
+	 */
+	if (nor->quad_enable == spansion_quad_enable)
+		nor->disable_write_protection = spi_nor_spansion_clear_sr_bp;
+
+	return nor->disable_write_protection(nor);
+}
+
 static int spi_nor_quad_enable(struct spi_nor *nor)
 {
 	if (!nor->quad_enable)
@@ -4665,16 +4700,11 @@ static int spi_nor_init(struct spi_nor *nor)
 {
 	int err;
 
-	if (nor->clear_sr_bp) {
-		if (nor->quad_enable == spansion_quad_enable)
-			nor->clear_sr_bp = spi_nor_spansion_clear_sr_bp;
-
-		err = nor->clear_sr_bp(nor);
-		if (err) {
-			dev_err(nor->dev,
-				"fail to clear block protection bits\n");
-			return err;
-		}
+	err = spi_nor_disable_write_protection(nor);
+	if (err) {
+		dev_err(nor->dev,
+			"fail to unlock the flash at init (err = %d)\n", err);
+		return err;
 	}
 
 	err = spi_nor_quad_enable(nor);
@@ -4797,23 +4827,15 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	if (info->flags & SPI_S3AN)
 		nor->flags |=  SNOR_F_READY_XSR_RDY;
 
-	/*
-	 * 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->clear_sr_bp = spi_nor_clear_sr_bp;
-
 	/* Kept only for backward compatibility purpose. */
 	nor->quad_enable = spansion_quad_enable;
 	nor->set_4byte = spansion_set_4byte;
 
 	/* Default locking operations. */
-	if (info->flags & SPI_NOR_HAS_LOCK)
+	if (info->flags & SPI_NOR_HAS_LOCK) {
 		nor->locking_ops = &stm_locking_ops;
+		nor->disable_write_protection = spi_nor_clear_sr_bp;
+	}
 
 	/* Init flash parameters based on flash_info struct and SFDP */
 	spi_nor_init_params(nor, &params);
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index bd68ec5a10e7..185ca11bfb63 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -380,8 +380,8 @@ struct flash_info;
  * @quad_enable:	[FLASH-SPECIFIC] enables SPI NOR quad mode
  * @set_4byte:		[FLASH-SPECIFIC] puts the SPI NOR in 4 byte addressing
  *                      mode
- * @clear_sr_bp:	[FLASH-SPECIFIC] clears the Block Protection Bits from
- *			the SPI NOR Status Register.
+ * @disable_write_protection: [FLASH-SPECIFIC] disable write protection during
+ *                            power-up
  *			completely locked
  * @priv:		the private data
  */
@@ -423,7 +423,7 @@ struct spi_nor {
 	int (*flash_is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len);
 	int (*quad_enable)(struct spi_nor *nor);
 	int (*set_4byte)(struct spi_nor *nor, bool enable);
-	int (*clear_sr_bp)(struct spi_nor *nor);
+	int (*disable_write_protection)(struct spi_nor *nor);
 
 	const struct spi_nor_locking_ops *locking_ops;
 
-- 
2.9.5


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