[PATCH v1 00/14] mtd: spi-nor: move vendor specific code into vendor modules

[PATCH v1 00/14] mtd: spi-nor: move vendor specific code into vendor modules

[Michael Walle]

It turns out that most of the special status register handling is
specific for a particular vendor. I.e. Xilinx has some different
opcodes for the status register read, Micron has an additional FSR
register and Spansion has these flags integrated into the SR.

Create a callback to ready() where a flash chip can register its
own function. This will let us move all the vendor specific stuff
out of the core into the vendor modules.

Please note that this is only compile-time tested.

For sake of consistency and better readability of the code flow,
I also converted the setup() callback to be optional.

Michael Walle (14):
  mtd: spi-nor: export more function to be used in vendor modules
  mtd: spi-nor: slightly refactor the spi_nor_setup()
  mtd: spi-nor: allow a flash to define its own ready() function
  mtd: spi-nor: move all xilinx specifics into xilinx.c
  mtd: spi-nor: xilinx: rename vendor specific functions and defines
  mtd: spi-nor: xilinx: correct the debug message
  mtd: spi-nor: move all micron-st specifics into micron-st.c
  mtd: spi-nor: micron-st: convert USE_FSR to a manufacturer flag
  mtd: spi-nor: micron-st: fix micron_st prefix
  mtd: spi-nor: micron-st: rename vendor specific functions and defines
  mtd: spi-nor: spansion: slightly rework control flow in late_init()
  mtd: spi-nor: move all spansion specifics into spansion.c
  mtd: spi-nor: spansion: convert USE_CLSR to a manufacturer flag
  mtd: spi-nor: renumber flags

 drivers/mtd/spi-nor/core.c      | 265 ++------------------------------
 drivers/mtd/spi-nor/core.h      |  70 ++++-----
 drivers/mtd/spi-nor/micron-st.c | 225 ++++++++++++++++++++++-----
 drivers/mtd/spi-nor/spansion.c  | 133 ++++++++++++----
 drivers/mtd/spi-nor/xilinx.c    |  79 +++++++++-
 include/linux/mtd/spi-nor.h     |  18 ---
 6 files changed, 417 insertions(+), 373 deletions(-)

-- 
2.30.2

[PATCH v1 01/14] mtd: spi-nor: export more function to be used in vendor modules

[Michael Walle]

We will move vendor specific code into the vendor modules and thus we
will have to export these functions so they can be called.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/core.c | 10 +++++-----
 drivers/mtd/spi-nor/core.h |  6 ++++++
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 04ea180118e3..f05ece6018dc 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -157,8 +157,8 @@ static int spi_nor_spimem_exec_op(struct spi_nor *nor, struct spi_mem_op *op)
 	return spi_mem_exec_op(nor->spimem, op);
 }
 
-static int spi_nor_controller_ops_read_reg(struct spi_nor *nor, u8 opcode,
-					   u8 *buf, size_t len)
+int spi_nor_controller_ops_read_reg(struct spi_nor *nor, u8 opcode,
+				    u8 *buf, size_t len)
 {
 	if (spi_nor_protocol_is_dtr(nor->reg_proto))
 		return -EOPNOTSUPP;
@@ -166,8 +166,8 @@ static int spi_nor_controller_ops_read_reg(struct spi_nor *nor, u8 opcode,
 	return nor->controller_ops->read_reg(nor, opcode, buf, len);
 }
 
-static int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
-					    const u8 *buf, size_t len)
+int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
+				     const u8 *buf, size_t len)
 {
 	if (spi_nor_protocol_is_dtr(nor->reg_proto))
 		return -EOPNOTSUPP;
@@ -683,7 +683,7 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
  *
  * Return: 1 if ready, 0 if not ready, -errno on errors.
  */
-static int spi_nor_sr_ready(struct spi_nor *nor)
+int spi_nor_sr_ready(struct spi_nor *nor)
 {
 	int ret = spi_nor_read_sr(nor, nor->bouncebuf);
 
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 2afb610853a9..c6578d3f598b 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -554,6 +554,7 @@ int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor);
 int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor);
 int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor);
 int spi_nor_read_sr(struct spi_nor *nor, u8 *sr);
+int spi_nor_sr_ready(struct spi_nor *nor);
 int spi_nor_read_cr(struct spi_nor *nor, u8 *cr);
 int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len);
 int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1);
@@ -599,6 +600,11 @@ void spi_nor_try_unlock_all(struct spi_nor *nor);
 void spi_nor_set_mtd_locking_ops(struct spi_nor *nor);
 void spi_nor_set_mtd_otp_ops(struct spi_nor *nor);
 
+int spi_nor_controller_ops_read_reg(struct spi_nor *nor, u8 opcode,
+				    u8 *buf, size_t len);
+int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
+				     const u8 *buf, size_t len);
+
 static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
 {
 	return container_of(mtd, struct spi_nor, mtd);
-- 
2.30.2

Re: [PATCH v1 01/14] mtd: spi-nor: export more function to be used in vendor modules

[Tudor.Ambarus]

On 2/2/22 16:58, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> We will move vendor specific code into the vendor modules and thus we
> will have to export these functions so they can be called.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

please move this patch closer to where the vendors actually use the methods.
With that:

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

> ---
>  drivers/mtd/spi-nor/core.c | 10 +++++-----
>  drivers/mtd/spi-nor/core.h |  6 ++++++
>  2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 04ea180118e3..f05ece6018dc 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -157,8 +157,8 @@ static int spi_nor_spimem_exec_op(struct spi_nor *nor, struct spi_mem_op *op)
>         return spi_mem_exec_op(nor->spimem, op);
>  }
> 
> -static int spi_nor_controller_ops_read_reg(struct spi_nor *nor, u8 opcode,
> -                                          u8 *buf, size_t len)
> +int spi_nor_controller_ops_read_reg(struct spi_nor *nor, u8 opcode,
> +                                   u8 *buf, size_t len)
>  {
>         if (spi_nor_protocol_is_dtr(nor->reg_proto))
>                 return -EOPNOTSUPP;
> @@ -166,8 +166,8 @@ static int spi_nor_controller_ops_read_reg(struct spi_nor *nor, u8 opcode,
>         return nor->controller_ops->read_reg(nor, opcode, buf, len);
>  }
> 
> -static int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
> -                                           const u8 *buf, size_t len)
> +int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
> +                                    const u8 *buf, size_t len)
>  {
>         if (spi_nor_protocol_is_dtr(nor->reg_proto))
>                 return -EOPNOTSUPP;
> @@ -683,7 +683,7 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
>   *
>   * Return: 1 if ready, 0 if not ready, -errno on errors.
>   */
> -static int spi_nor_sr_ready(struct spi_nor *nor)
> +int spi_nor_sr_ready(struct spi_nor *nor)
>  {
>         int ret = spi_nor_read_sr(nor, nor->bouncebuf);
> 
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 2afb610853a9..c6578d3f598b 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -554,6 +554,7 @@ int spi_nor_sr1_bit6_quad_enable(struct spi_nor *nor);
>  int spi_nor_sr2_bit1_quad_enable(struct spi_nor *nor);
>  int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor);
>  int spi_nor_read_sr(struct spi_nor *nor, u8 *sr);
> +int spi_nor_sr_ready(struct spi_nor *nor);
>  int spi_nor_read_cr(struct spi_nor *nor, u8 *cr);
>  int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len);
>  int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1);
> @@ -599,6 +600,11 @@ void spi_nor_try_unlock_all(struct spi_nor *nor);
>  void spi_nor_set_mtd_locking_ops(struct spi_nor *nor);
>  void spi_nor_set_mtd_otp_ops(struct spi_nor *nor);
> 
> +int spi_nor_controller_ops_read_reg(struct spi_nor *nor, u8 opcode,
> +                                   u8 *buf, size_t len);
> +int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
> +                                    const u8 *buf, size_t len);
> +
>  static inline struct spi_nor *mtd_to_spi_nor(struct mtd_info *mtd)
>  {
>         return container_of(mtd, struct spi_nor, mtd);
> --
> 2.30.2
> 

Re: [PATCH v1 01/14] mtd: spi-nor: export more function to be used in vendor modules

[Pratyush Yadav]

On 10/02/22 03:13AM, Tudor.Ambarus@microchip.com wrote:
> On 2/2/22 16:58, Michael Walle wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > We will move vendor specific code into the vendor modules and thus we
> > will have to export these functions so they can be called.
> > 
> > Signed-off-by: Michael Walle <michael@walle.cc>
> 
> please move this patch closer to where the vendors actually use the methods.
> With that:
> 
> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v1 02/14] mtd: spi-nor: slightly refactor the spi_nor_setup()

[Michael Walle]

Instead of always using a function pointer (and initializing it to our
default), just call the default function if the flash didn't set its own
one. That will make the call flow easier to follow.

Also mark the parameter as optional now.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/core.c | 10 +++++-----
 drivers/mtd/spi-nor/core.h |  8 ++++----
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index f05ece6018dc..c8cc906cf771 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2532,11 +2532,12 @@ static int spi_nor_setup(struct spi_nor *nor,
 {
 	int ret;
 
-	if (nor->params->setup) {
+	if (nor->params->setup)
 		ret = nor->params->setup(nor, hwcaps);
-		if (ret)
-			return ret;
-	}
+	else
+		ret = spi_nor_default_setup(nor, hwcaps);
+	if (ret)
+		return ret;
 
 	return spi_nor_set_addr_width(nor);
 }
@@ -2786,7 +2787,6 @@ static void spi_nor_init_default_params(struct spi_nor *nor)
 
 	params->quad_enable = spi_nor_sr2_bit1_quad_enable;
 	params->set_4byte_addr_mode = spansion_set_4byte_addr_mode;
-	params->setup = spi_nor_default_setup;
 	params->otp.org = &info->otp_org;
 
 	/* Default to 16-bit Write Status (01h) Command */
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index c6578d3f598b..10f478547dc2 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -257,10 +257,10 @@ struct spi_nor_otp {
  * @convert_addr:	converts an absolute address into something the flash
  *                      will understand. Particularly useful when pagesize is
  *                      not a power-of-2.
- * @setup:              configures the SPI NOR memory. Useful for SPI NOR
- *                      flashes that have peculiarities to the SPI NOR standard
- *                      e.g. different opcodes, specific address calculation,
- *                      page size, etc.
+ * @setup:		(optional) configures the SPI NOR memory. Useful for
+ *			SPI NOR flashes that have peculiarities to the SPI NOR
+ *			standard e.g. different opcodes, specific address
+ *			calculation, page size, etc.
  * @locking_ops:	SPI NOR locking methods.
  */
 struct spi_nor_flash_parameter {
-- 
2.30.2

Re: [PATCH v1 02/14] mtd: spi-nor: slightly refactor the spi_nor_setup()

[Tudor.Ambarus]

On 2/2/22 16:58, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Instead of always using a function pointer (and initializing it to our
> default), just call the default function if the flash didn't set its own
> one. That will make the call flow easier to follow.
> 
> Also mark the parameter as optional now.

what should be done in the future?

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

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

> ---
>  drivers/mtd/spi-nor/core.c | 10 +++++-----
>  drivers/mtd/spi-nor/core.h |  8 ++++----
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index f05ece6018dc..c8cc906cf771 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2532,11 +2532,12 @@ static int spi_nor_setup(struct spi_nor *nor,
>  {
>         int ret;
> 
> -       if (nor->params->setup) {
> +       if (nor->params->setup)
>                 ret = nor->params->setup(nor, hwcaps);
> -               if (ret)
> -                       return ret;
> -       }
> +       else
> +               ret = spi_nor_default_setup(nor, hwcaps);
> +       if (ret)
> +               return ret;
> 
>         return spi_nor_set_addr_width(nor);
>  }
> @@ -2786,7 +2787,6 @@ static void spi_nor_init_default_params(struct spi_nor *nor)
> 
>         params->quad_enable = spi_nor_sr2_bit1_quad_enable;
>         params->set_4byte_addr_mode = spansion_set_4byte_addr_mode;
> -       params->setup = spi_nor_default_setup;
>         params->otp.org = &info->otp_org;
> 
>         /* Default to 16-bit Write Status (01h) Command */
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index c6578d3f598b..10f478547dc2 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -257,10 +257,10 @@ struct spi_nor_otp {
>   * @convert_addr:      converts an absolute address into something the flash
>   *                      will understand. Particularly useful when pagesize is
>   *                      not a power-of-2.
> - * @setup:              configures the SPI NOR memory. Useful for SPI NOR
> - *                      flashes that have peculiarities to the SPI NOR standard
> - *                      e.g. different opcodes, specific address calculation,
> - *                      page size, etc.
> + * @setup:             (optional) configures the SPI NOR memory. Useful for
> + *                     SPI NOR flashes that have peculiarities to the SPI NOR
> + *                     standard e.g. different opcodes, specific address
> + *                     calculation, page size, etc.
>   * @locking_ops:       SPI NOR locking methods.
>   */
>  struct spi_nor_flash_parameter {
> --
> 2.30.2
> 

Re: [PATCH v1 02/14] mtd: spi-nor: slightly refactor the spi_nor_setup()

[Michael Walle]

Am 2022-02-10 04:00, schrieb Tudor.Ambarus@microchip.com:
> On 2/2/22 16:58, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Instead of always using a function pointer (and initializing it to our
>> default), just call the default function if the flash didn't set its 
>> own
>> one. That will make the call flow easier to follow.
>> 
>> Also mark the parameter as optional now.
> 
> what should be done in the future?

What do you mean? IMHO the default path should be visible in the
function. Eg.

int spi_nor_bla(nor) {
     if (nor->some_exceptional_cb)
         return nor->some_exceptional_cb(nor);

    return usual_cb(nor);
}

-michael

Re: [PATCH v1 02/14] mtd: spi-nor: slightly refactor the spi_nor_setup()

[Tudor.Ambarus]

On 2/10/22 10:01, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-02-10 04:00, schrieb Tudor.Ambarus@microchip.com:
>> On 2/2/22 16:58, Michael Walle wrote:

cut

>>> Also mark the parameter as optional now.

Oh, I misread, in my head I read "for now". That's why I asked what should
be done next :).
>>
>> what should be done in the future?
> 
> What do you mean? IMHO the default path should be visible in the
> function. Eg.
> 
> int spi_nor_bla(nor) {
>     if (nor->some_exceptional_cb)
>         return nor->some_exceptional_cb(nor);
> 
>    return usual_cb(nor);
> }
> 
> -michael

Re: [PATCH v1 02/14] mtd: spi-nor: slightly refactor the spi_nor_setup()

[Pratyush Yadav]

On 02/02/22 03:58PM, Michael Walle wrote:
> Instead of always using a function pointer (and initializing it to our
> default), just call the default function if the flash didn't set its own
> one. That will make the call flow easier to follow.
> 
> Also mark the parameter as optional now.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v1 03/14] mtd: spi-nor: allow a flash to define its own ready() function

[Michael Walle]

Xilinx and Micron flashes have their own implementation of the
spi_nor_ready() function. At the moment, the core will figure out
which one to call according to some flags. Lay the foundation to
make it possible that a flash can register its own ready()
function.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/core.c | 4 ++++
 drivers/mtd/spi-nor/core.h | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index c8cc906cf771..c181f2680df2 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -794,6 +794,10 @@ static int spi_nor_ready(struct spi_nor *nor)
 {
 	int sr, fsr;
 
+	/* flashes might override our standard routine */
+	if (nor->params->ready)
+		return nor->params->ready(nor);
+
 	if (nor->flags & SNOR_F_READY_XSR_RDY)
 		sr = spi_nor_xsr_ready(nor);
 	else
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 10f478547dc2..446218b0e017 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -261,6 +261,9 @@ struct spi_nor_otp {
  *			SPI NOR flashes that have peculiarities to the SPI NOR
  *			standard e.g. different opcodes, specific address
  *			calculation, page size, etc.
+ * @ready:		(optional) flashes might use a different mechanism
+ *			than reading the status register to indicate they
+ *			are ready for a new command
  * @locking_ops:	SPI NOR locking methods.
  */
 struct spi_nor_flash_parameter {
@@ -282,6 +285,7 @@ struct spi_nor_flash_parameter {
 	int (*set_4byte_addr_mode)(struct spi_nor *nor, bool enable);
 	u32 (*convert_addr)(struct spi_nor *nor, u32 addr);
 	int (*setup)(struct spi_nor *nor, const struct spi_nor_hwcaps *hwcaps);
+	int (*ready)(struct spi_nor *nor);
 
 	const struct spi_nor_locking_ops *locking_ops;
 };
-- 
2.30.2

Re: [PATCH v1 03/14] mtd: spi-nor: allow a flash to define its own ready() function

[Tudor.Ambarus]

On 2/2/22 16:58, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Xilinx and Micron flashes have their own implementation of the
> spi_nor_ready() function. At the moment, the core will figure out
> which one to call according to some flags. Lay the foundation to
> make it possible that a flash can register its own ready()
> function.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/core.c | 4 ++++
>  drivers/mtd/spi-nor/core.h | 4 ++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index c8cc906cf771..c181f2680df2 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -794,6 +794,10 @@ static int spi_nor_ready(struct spi_nor *nor)
>  {
>         int sr, fsr;
> 
> +       /* flashes might override our standard routine */

let's start comments with capital letter and put a dot at the end of
the sentence. s/our/the

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> +       if (nor->params->ready)
> +               return nor->params->ready(nor);
> +
>         if (nor->flags & SNOR_F_READY_XSR_RDY)
>                 sr = spi_nor_xsr_ready(nor);
>         else
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 10f478547dc2..446218b0e017 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -261,6 +261,9 @@ struct spi_nor_otp {
>   *                     SPI NOR flashes that have peculiarities to the SPI NOR
>   *                     standard e.g. different opcodes, specific address
>   *                     calculation, page size, etc.
> + * @ready:             (optional) flashes might use a different mechanism
> + *                     than reading the status register to indicate they
> + *                     are ready for a new command
>   * @locking_ops:       SPI NOR locking methods.
>   */
>  struct spi_nor_flash_parameter {
> @@ -282,6 +285,7 @@ struct spi_nor_flash_parameter {
>         int (*set_4byte_addr_mode)(struct spi_nor *nor, bool enable);
>         u32 (*convert_addr)(struct spi_nor *nor, u32 addr);
>         int (*setup)(struct spi_nor *nor, const struct spi_nor_hwcaps *hwcaps);
> +       int (*ready)(struct spi_nor *nor);
> 
>         const struct spi_nor_locking_ops *locking_ops;
>  };
> --
> 2.30.2
> 

Re: [PATCH v1 03/14] mtd: spi-nor: allow a flash to define its own ready() function

[Pratyush Yadav]

On 10/02/22 03:05AM, Tudor.Ambarus@microchip.com wrote:
> On 2/2/22 16:58, Michael Walle wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > Xilinx and Micron flashes have their own implementation of the
> > spi_nor_ready() function. At the moment, the core will figure out
> > which one to call according to some flags. Lay the foundation to
> > make it possible that a flash can register its own ready()
> > function.
> > 
> > Signed-off-by: Michael Walle <michael@walle.cc>
> > ---
> >  drivers/mtd/spi-nor/core.c | 4 ++++
> >  drivers/mtd/spi-nor/core.h | 4 ++++
> >  2 files changed, 8 insertions(+)
> > 
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index c8cc906cf771..c181f2680df2 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -794,6 +794,10 @@ static int spi_nor_ready(struct spi_nor *nor)
> >  {
> >         int sr, fsr;
> > 
> > +       /* flashes might override our standard routine */
> 
> let's start comments with capital letter and put a dot at the end of
> the sentence. s/our/the

+1

> 
> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

With that fixed,

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

> > +       if (nor->params->ready)
> > +               return nor->params->ready(nor);
> > +
> >         if (nor->flags & SNOR_F_READY_XSR_RDY)
> >                 sr = spi_nor_xsr_ready(nor);
> >         else
> > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> > index 10f478547dc2..446218b0e017 100644
> > --- a/drivers/mtd/spi-nor/core.h
> > +++ b/drivers/mtd/spi-nor/core.h
> > @@ -261,6 +261,9 @@ struct spi_nor_otp {
> >   *                     SPI NOR flashes that have peculiarities to the SPI NOR
> >   *                     standard e.g. different opcodes, specific address
> >   *                     calculation, page size, etc.
> > + * @ready:             (optional) flashes might use a different mechanism
> > + *                     than reading the status register to indicate they
> > + *                     are ready for a new command
> >   * @locking_ops:       SPI NOR locking methods.
> >   */
> >  struct spi_nor_flash_parameter {
> > @@ -282,6 +285,7 @@ struct spi_nor_flash_parameter {
> >         int (*set_4byte_addr_mode)(struct spi_nor *nor, bool enable);
> >         u32 (*convert_addr)(struct spi_nor *nor, u32 addr);
> >         int (*setup)(struct spi_nor *nor, const struct spi_nor_hwcaps *hwcaps);
> > +       int (*ready)(struct spi_nor *nor);
> > 
> >         const struct spi_nor_locking_ops *locking_ops;
> >  };
> > --
> > 2.30.2
> > 
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v1 04/14] mtd: spi-nor: move all xilinx specifics into xilinx.c

[Michael Walle]

Mechanically move all the xilinx functions to its own module.

Then register the new flash specific ready() function.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/core.c   | 64 +------------------------------
 drivers/mtd/spi-nor/core.h   | 18 ---------
 drivers/mtd/spi-nor/xilinx.c | 73 ++++++++++++++++++++++++++++++++++++
 include/linux/mtd/spi-nor.h  |  9 -----
 4 files changed, 74 insertions(+), 90 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index c181f2680df2..fdc8ef623254 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -598,57 +598,6 @@ int spi_nor_write_ear(struct spi_nor *nor, u8 ear)
 	return ret;
 }
 
-/**
- * spi_nor_xread_sr() - Read the Status Register on S3AN flashes.
- * @nor:	pointer to 'struct spi_nor'.
- * @sr:		pointer to a DMA-able buffer where the value of the
- *              Status Register will be written.
- *
- * Return: 0 on success, -errno otherwise.
- */
-int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)
-{
-	int ret;
-
-	if (nor->spimem) {
-		struct spi_mem_op op =
-			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_XRDSR, 0),
-				   SPI_MEM_OP_NO_ADDR,
-				   SPI_MEM_OP_NO_DUMMY,
-				   SPI_MEM_OP_DATA_IN(1, sr, 0));
-
-		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
-
-		ret = spi_mem_exec_op(nor->spimem, &op);
-	} else {
-		ret = spi_nor_controller_ops_read_reg(nor, SPINOR_OP_XRDSR, sr,
-						      1);
-	}
-
-	if (ret)
-		dev_dbg(nor->dev, "error %d reading XRDSR\n", ret);
-
-	return ret;
-}
-
-/**
- * spi_nor_xsr_ready() - Query the Status Register of the S3AN flash to see if
- * the flash is ready for new commands.
- * @nor:	pointer to 'struct spi_nor'.
- *
- * Return: 1 if ready, 0 if not ready, -errno on errors.
- */
-static int spi_nor_xsr_ready(struct spi_nor *nor)
-{
-	int ret;
-
-	ret = spi_nor_xread_sr(nor, nor->bouncebuf);
-	if (ret)
-		return ret;
-
-	return !!(nor->bouncebuf[0] & XSR_RDY);
-}
-
 /**
  * spi_nor_clear_sr() - Clear the Status Register.
  * @nor:	pointer to 'struct spi_nor'.
@@ -798,10 +747,7 @@ static int spi_nor_ready(struct spi_nor *nor)
 	if (nor->params->ready)
 		return nor->params->ready(nor);
 
-	if (nor->flags & SNOR_F_READY_XSR_RDY)
-		sr = spi_nor_xsr_ready(nor);
-	else
-		sr = spi_nor_sr_ready(nor);
+	sr = spi_nor_sr_ready(nor);
 	if (sr < 0)
 		return sr;
 	fsr = nor->flags & SNOR_F_USE_FSR ? spi_nor_fsr_ready(nor) : 1;
@@ -2677,14 +2623,6 @@ static void spi_nor_init_flags(struct spi_nor *nor)
 
 	if (flags & USE_FSR)
 		nor->flags |= SNOR_F_USE_FSR;
-
-	/*
-	 * Make sure the XSR_RDY flag is set before calling
-	 * spi_nor_wait_till_ready(). Xilinx S3AN share MFR
-	 * with Atmel SPI NOR.
-	 */
-	if (flags & SPI_NOR_XSR_RDY)
-		nor->flags |=  SNOR_F_READY_XSR_RDY;
 }
 
 /**
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 446218b0e017..fabc01ae9a81 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -15,7 +15,6 @@ enum spi_nor_option_flags {
 	SNOR_F_USE_FSR		= BIT(0),
 	SNOR_F_HAS_SR_TB	= BIT(1),
 	SNOR_F_NO_OP_CHIP_ERASE	= BIT(2),
-	SNOR_F_READY_XSR_RDY	= BIT(3),
 	SNOR_F_USE_CLSR		= BIT(4),
 	SNOR_F_BROKEN_RESET	= BIT(5),
 	SNOR_F_4B_OPCODES	= BIT(6),
@@ -351,8 +350,6 @@ struct spi_nor_fixups {
  *   SPI_NOR_NO_FR:           can't do fastread.
  *   USE_CLSR:                use CLSR command.
  *   USE_FSR:                 use flag status register
- *   SPI_NOR_XSR_RDY:         S3AN flashes have specific opcode to read the
- *                            status register.
  *
  * @no_sfdp_flags:  flags that indicate support that can be discovered via SFDP.
  *                  Used when SFDP tables are not defined in the flash. These
@@ -405,7 +402,6 @@ struct flash_info {
 #define SPI_NOR_NO_FR			BIT(8)
 #define USE_CLSR			BIT(9)
 #define USE_FSR				BIT(10)
-#define SPI_NOR_XSR_RDY			BIT(11)
 
 	u8 no_sfdp_flags;
 #define SPI_NOR_SKIP_SFDP		BIT(0)
@@ -462,19 +458,6 @@ struct flash_info {
 		.addr_width = (_addr_width),				\
 		.flags = SPI_NOR_NO_ERASE | SPI_NOR_NO_FR,		\
 
-#define S3AN_INFO(_jedec_id, _n_sectors, _page_size)			\
-		.id = {							\
-			((_jedec_id) >> 16) & 0xff,			\
-			((_jedec_id) >> 8) & 0xff,			\
-			(_jedec_id) & 0xff				\
-			},						\
-		.id_len = 3,						\
-		.sector_size = (8*_page_size),				\
-		.n_sectors = (_n_sectors),				\
-		.page_size = _page_size,				\
-		.addr_width = 3,					\
-		.flags = SPI_NOR_NO_FR | SPI_NOR_XSR_RDY,
-
 #define OTP_INFO(_len, _n_regions, _base, _offset)			\
 		.otp_org = {						\
 			.len = (_len),					\
@@ -564,7 +547,6 @@ int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len);
 int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1);
 int spi_nor_write_16bit_cr_and_check(struct spi_nor *nor, u8 cr);
 
-int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr);
 ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
 			  u8 *buf);
 ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
diff --git a/drivers/mtd/spi-nor/xilinx.c b/drivers/mtd/spi-nor/xilinx.c
index 580562bc1e45..a865dadc2e5d 100644
--- a/drivers/mtd/spi-nor/xilinx.c
+++ b/drivers/mtd/spi-nor/xilinx.c
@@ -8,6 +8,27 @@
 
 #include "core.h"
 
+#define SPINOR_OP_XSE		0x50	/* Sector erase */
+#define SPINOR_OP_XPP		0x82	/* Page program */
+#define SPINOR_OP_XRDSR		0xd7	/* Read status register */
+
+#define XSR_PAGESIZE		BIT(0)	/* Page size in Po2 or Linear */
+#define XSR_RDY			BIT(7)	/* Ready */
+
+#define S3AN_INFO(_jedec_id, _n_sectors, _page_size)			\
+		.id = {							\
+			((_jedec_id) >> 16) & 0xff,			\
+			((_jedec_id) >> 8) & 0xff,			\
+			(_jedec_id) & 0xff				\
+			},						\
+		.id_len = 3,						\
+		.sector_size = (8*_page_size),				\
+		.n_sectors = (_n_sectors),				\
+		.page_size = _page_size,				\
+		.addr_width = 3,					\
+		.flags = SPI_NOR_NO_FR
+
+/* Xilinx S3AN share MFR with Atmel SPI NOR */
 static const struct flash_info xilinx_parts[] = {
 	/* Xilinx S3AN Internal Flash */
 	{ "3S50AN", S3AN_INFO(0x1f2200, 64, 264) },
@@ -38,6 +59,57 @@ static u32 s3an_convert_addr(struct spi_nor *nor, u32 addr)
 	return page | offset;
 }
 
+/**
+ * spi_nor_xread_sr() - Read the Status Register on S3AN flashes.
+ * @nor:	pointer to 'struct spi_nor'.
+ * @sr:		pointer to a DMA-able buffer where the value of the
+ *              Status Register will be written.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)
+{
+	int ret;
+
+	if (nor->spimem) {
+		struct spi_mem_op op =
+			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_XRDSR, 0),
+				   SPI_MEM_OP_NO_ADDR,
+				   SPI_MEM_OP_NO_DUMMY,
+				   SPI_MEM_OP_DATA_IN(1, sr, 0));
+
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
+		ret = spi_mem_exec_op(nor->spimem, &op);
+	} else {
+		ret = spi_nor_controller_ops_read_reg(nor, SPINOR_OP_XRDSR, sr,
+						      1);
+	}
+
+	if (ret)
+		dev_dbg(nor->dev, "error %d reading XRDSR\n", ret);
+
+	return ret;
+}
+
+/**
+ * spi_nor_xsr_ready() - Query the Status Register of the S3AN flash to see if
+ * the flash is ready for new commands.
+ * @nor:	pointer to 'struct spi_nor'.
+ *
+ * Return: 1 if ready, 0 if not ready, -errno on errors.
+ */
+static int spi_nor_xsr_ready(struct spi_nor *nor)
+{
+	int ret;
+
+	ret = spi_nor_xread_sr(nor, nor->bouncebuf);
+	if (ret)
+		return ret;
+
+	return !!(nor->bouncebuf[0] & XSR_RDY);
+}
+
 static int xilinx_nor_setup(struct spi_nor *nor,
 			    const struct spi_nor_hwcaps *hwcaps)
 {
@@ -83,6 +155,7 @@ static int xilinx_nor_setup(struct spi_nor *nor,
 static void xilinx_late_init(struct spi_nor *nor)
 {
 	nor->params->setup = xilinx_nor_setup;
+	nor->params->ready = spi_nor_xsr_ready;
 }
 
 static const struct spi_nor_fixups xilinx_fixups = {
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index fc90fce26e33..b44b05a6f934 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -86,15 +86,6 @@
 #define SPINOR_OP_BP		0x02	/* Byte program */
 #define SPINOR_OP_AAI_WP	0xad	/* Auto address increment word program */
 
-/* Used for S3AN flashes only */
-#define SPINOR_OP_XSE		0x50	/* Sector erase */
-#define SPINOR_OP_XPP		0x82	/* Page program */
-#define SPINOR_OP_XRDSR		0xd7	/* Read status register */
-
-#define XSR_PAGESIZE		BIT(0)	/* Page size in Po2 or Linear */
-#define XSR_RDY			BIT(7)	/* Ready */
-
-
 /* Used for Macronix and Winbond flashes. */
 #define SPINOR_OP_EN4B		0xb7	/* Enter 4-byte mode */
 #define SPINOR_OP_EX4B		0xe9	/* Exit 4-byte mode */
-- 
2.30.2

Re: [PATCH v1 04/14] mtd: spi-nor: move all xilinx specifics into xilinx.c

[Tudor.Ambarus]

On 2/2/22 16:58, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Mechanically move all the xilinx functions to its own module.
> 
> Then register the new flash specific ready() function.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

excellent!
Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

> ---
>  drivers/mtd/spi-nor/core.c   | 64 +------------------------------
>  drivers/mtd/spi-nor/core.h   | 18 ---------
>  drivers/mtd/spi-nor/xilinx.c | 73 ++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/spi-nor.h  |  9 -----
>  4 files changed, 74 insertions(+), 90 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index c181f2680df2..fdc8ef623254 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -598,57 +598,6 @@ int spi_nor_write_ear(struct spi_nor *nor, u8 ear)
>         return ret;
>  }
> 
> -/**
> - * spi_nor_xread_sr() - Read the Status Register on S3AN flashes.
> - * @nor:       pointer to 'struct spi_nor'.
> - * @sr:                pointer to a DMA-able buffer where the value of the
> - *              Status Register will be written.
> - *
> - * Return: 0 on success, -errno otherwise.
> - */
> -int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)
> -{
> -       int ret;
> -
> -       if (nor->spimem) {
> -               struct spi_mem_op op =
> -                       SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_XRDSR, 0),
> -                                  SPI_MEM_OP_NO_ADDR,
> -                                  SPI_MEM_OP_NO_DUMMY,
> -                                  SPI_MEM_OP_DATA_IN(1, sr, 0));
> -
> -               spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> -
> -               ret = spi_mem_exec_op(nor->spimem, &op);
> -       } else {
> -               ret = spi_nor_controller_ops_read_reg(nor, SPINOR_OP_XRDSR, sr,
> -                                                     1);
> -       }
> -
> -       if (ret)
> -               dev_dbg(nor->dev, "error %d reading XRDSR\n", ret);
> -
> -       return ret;
> -}
> -
> -/**
> - * spi_nor_xsr_ready() - Query the Status Register of the S3AN flash to see if
> - * the flash is ready for new commands.
> - * @nor:       pointer to 'struct spi_nor'.
> - *
> - * Return: 1 if ready, 0 if not ready, -errno on errors.
> - */
> -static int spi_nor_xsr_ready(struct spi_nor *nor)
> -{
> -       int ret;
> -
> -       ret = spi_nor_xread_sr(nor, nor->bouncebuf);
> -       if (ret)
> -               return ret;
> -
> -       return !!(nor->bouncebuf[0] & XSR_RDY);
> -}
> -
>  /**
>   * spi_nor_clear_sr() - Clear the Status Register.
>   * @nor:       pointer to 'struct spi_nor'.
> @@ -798,10 +747,7 @@ static int spi_nor_ready(struct spi_nor *nor)
>         if (nor->params->ready)
>                 return nor->params->ready(nor);
> 
> -       if (nor->flags & SNOR_F_READY_XSR_RDY)
> -               sr = spi_nor_xsr_ready(nor);
> -       else
> -               sr = spi_nor_sr_ready(nor);
> +       sr = spi_nor_sr_ready(nor);
>         if (sr < 0)
>                 return sr;
>         fsr = nor->flags & SNOR_F_USE_FSR ? spi_nor_fsr_ready(nor) : 1;
> @@ -2677,14 +2623,6 @@ static void spi_nor_init_flags(struct spi_nor *nor)
> 
>         if (flags & USE_FSR)
>                 nor->flags |= SNOR_F_USE_FSR;
> -
> -       /*
> -        * Make sure the XSR_RDY flag is set before calling
> -        * spi_nor_wait_till_ready(). Xilinx S3AN share MFR
> -        * with Atmel SPI NOR.
> -        */
> -       if (flags & SPI_NOR_XSR_RDY)
> -               nor->flags |=  SNOR_F_READY_XSR_RDY;
>  }
> 
>  /**
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 446218b0e017..fabc01ae9a81 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -15,7 +15,6 @@ enum spi_nor_option_flags {
>         SNOR_F_USE_FSR          = BIT(0),
>         SNOR_F_HAS_SR_TB        = BIT(1),
>         SNOR_F_NO_OP_CHIP_ERASE = BIT(2),
> -       SNOR_F_READY_XSR_RDY    = BIT(3),
>         SNOR_F_USE_CLSR         = BIT(4),
>         SNOR_F_BROKEN_RESET     = BIT(5),
>         SNOR_F_4B_OPCODES       = BIT(6),
> @@ -351,8 +350,6 @@ struct spi_nor_fixups {
>   *   SPI_NOR_NO_FR:           can't do fastread.
>   *   USE_CLSR:                use CLSR command.
>   *   USE_FSR:                 use flag status register
> - *   SPI_NOR_XSR_RDY:         S3AN flashes have specific opcode to read the
> - *                            status register.
>   *
>   * @no_sfdp_flags:  flags that indicate support that can be discovered via SFDP.
>   *                  Used when SFDP tables are not defined in the flash. These
> @@ -405,7 +402,6 @@ struct flash_info {
>  #define SPI_NOR_NO_FR                  BIT(8)
>  #define USE_CLSR                       BIT(9)
>  #define USE_FSR                                BIT(10)
> -#define SPI_NOR_XSR_RDY                        BIT(11)
> 
>         u8 no_sfdp_flags;
>  #define SPI_NOR_SKIP_SFDP              BIT(0)
> @@ -462,19 +458,6 @@ struct flash_info {
>                 .addr_width = (_addr_width),                            \
>                 .flags = SPI_NOR_NO_ERASE | SPI_NOR_NO_FR,              \
> 
> -#define S3AN_INFO(_jedec_id, _n_sectors, _page_size)                   \
> -               .id = {                                                 \
> -                       ((_jedec_id) >> 16) & 0xff,                     \
> -                       ((_jedec_id) >> 8) & 0xff,                      \
> -                       (_jedec_id) & 0xff                              \
> -                       },                                              \
> -               .id_len = 3,                                            \
> -               .sector_size = (8*_page_size),                          \
> -               .n_sectors = (_n_sectors),                              \
> -               .page_size = _page_size,                                \
> -               .addr_width = 3,                                        \
> -               .flags = SPI_NOR_NO_FR | SPI_NOR_XSR_RDY,
> -
>  #define OTP_INFO(_len, _n_regions, _base, _offset)                     \
>                 .otp_org = {                                            \
>                         .len = (_len),                                  \
> @@ -564,7 +547,6 @@ int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len);
>  int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1);
>  int spi_nor_write_16bit_cr_and_check(struct spi_nor *nor, u8 cr);
> 
> -int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr);
>  ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
>                           u8 *buf);
>  ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
> diff --git a/drivers/mtd/spi-nor/xilinx.c b/drivers/mtd/spi-nor/xilinx.c
> index 580562bc1e45..a865dadc2e5d 100644
> --- a/drivers/mtd/spi-nor/xilinx.c
> +++ b/drivers/mtd/spi-nor/xilinx.c
> @@ -8,6 +8,27 @@
> 
>  #include "core.h"
> 
> +#define SPINOR_OP_XSE          0x50    /* Sector erase */
> +#define SPINOR_OP_XPP          0x82    /* Page program */
> +#define SPINOR_OP_XRDSR                0xd7    /* Read status register */
> +
> +#define XSR_PAGESIZE           BIT(0)  /* Page size in Po2 or Linear */
> +#define XSR_RDY                        BIT(7)  /* Ready */
> +
> +#define S3AN_INFO(_jedec_id, _n_sectors, _page_size)                   \
> +               .id = {                                                 \
> +                       ((_jedec_id) >> 16) & 0xff,                     \
> +                       ((_jedec_id) >> 8) & 0xff,                      \
> +                       (_jedec_id) & 0xff                              \
> +                       },                                              \
> +               .id_len = 3,                                            \
> +               .sector_size = (8*_page_size),                          \
> +               .n_sectors = (_n_sectors),                              \
> +               .page_size = _page_size,                                \
> +               .addr_width = 3,                                        \
> +               .flags = SPI_NOR_NO_FR
> +
> +/* Xilinx S3AN share MFR with Atmel SPI NOR */
>  static const struct flash_info xilinx_parts[] = {
>         /* Xilinx S3AN Internal Flash */
>         { "3S50AN", S3AN_INFO(0x1f2200, 64, 264) },
> @@ -38,6 +59,57 @@ static u32 s3an_convert_addr(struct spi_nor *nor, u32 addr)
>         return page | offset;
>  }
> 
> +/**
> + * spi_nor_xread_sr() - Read the Status Register on S3AN flashes.
> + * @nor:       pointer to 'struct spi_nor'.
> + * @sr:                pointer to a DMA-able buffer where the value of the
> + *              Status Register will be written.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)
> +{
> +       int ret;
> +
> +       if (nor->spimem) {
> +               struct spi_mem_op op =
> +                       SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_XRDSR, 0),
> +                                  SPI_MEM_OP_NO_ADDR,
> +                                  SPI_MEM_OP_NO_DUMMY,
> +                                  SPI_MEM_OP_DATA_IN(1, sr, 0));
> +
> +               spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> +
> +               ret = spi_mem_exec_op(nor->spimem, &op);
> +       } else {
> +               ret = spi_nor_controller_ops_read_reg(nor, SPINOR_OP_XRDSR, sr,
> +                                                     1);
> +       }
> +
> +       if (ret)
> +               dev_dbg(nor->dev, "error %d reading XRDSR\n", ret);
> +
> +       return ret;
> +}
> +
> +/**
> + * spi_nor_xsr_ready() - Query the Status Register of the S3AN flash to see if
> + * the flash is ready for new commands.
> + * @nor:       pointer to 'struct spi_nor'.
> + *
> + * Return: 1 if ready, 0 if not ready, -errno on errors.
> + */
> +static int spi_nor_xsr_ready(struct spi_nor *nor)
> +{
> +       int ret;
> +
> +       ret = spi_nor_xread_sr(nor, nor->bouncebuf);
> +       if (ret)
> +               return ret;
> +
> +       return !!(nor->bouncebuf[0] & XSR_RDY);
> +}
> +
>  static int xilinx_nor_setup(struct spi_nor *nor,
>                             const struct spi_nor_hwcaps *hwcaps)
>  {
> @@ -83,6 +155,7 @@ static int xilinx_nor_setup(struct spi_nor *nor,
>  static void xilinx_late_init(struct spi_nor *nor)
>  {
>         nor->params->setup = xilinx_nor_setup;
> +       nor->params->ready = spi_nor_xsr_ready;
>  }
> 
>  static const struct spi_nor_fixups xilinx_fixups = {
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index fc90fce26e33..b44b05a6f934 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -86,15 +86,6 @@
>  #define SPINOR_OP_BP           0x02    /* Byte program */
>  #define SPINOR_OP_AAI_WP       0xad    /* Auto address increment word program */
> 
> -/* Used for S3AN flashes only */
> -#define SPINOR_OP_XSE          0x50    /* Sector erase */
> -#define SPINOR_OP_XPP          0x82    /* Page program */
> -#define SPINOR_OP_XRDSR                0xd7    /* Read status register */
> -
> -#define XSR_PAGESIZE           BIT(0)  /* Page size in Po2 or Linear */
> -#define XSR_RDY                        BIT(7)  /* Ready */
> -
> -
>  /* Used for Macronix and Winbond flashes. */
>  #define SPINOR_OP_EN4B         0xb7    /* Enter 4-byte mode */
>  #define SPINOR_OP_EX4B         0xe9    /* Exit 4-byte mode */
> --
> 2.30.2
> 

Re: [PATCH v1 04/14] mtd: spi-nor: move all xilinx specifics into xilinx.c

[Pratyush Yadav]

On 02/02/22 03:58PM, Michael Walle wrote:
> Mechanically move all the xilinx functions to its own module.

If you do some code changes mechanically you should also paste the 
semantic patch for future reference.

> 
> Then register the new flash specific ready() function.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/core.c   | 64 +------------------------------
>  drivers/mtd/spi-nor/core.h   | 18 ---------
>  drivers/mtd/spi-nor/xilinx.c | 73 ++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/spi-nor.h  |  9 -----
>  4 files changed, 74 insertions(+), 90 deletions(-)
> 
[...]
>  
> +/**
> + * spi_nor_xread_sr() - Read the Status Register on S3AN flashes.
> + * @nor:	pointer to 'struct spi_nor'.
> + * @sr:		pointer to a DMA-able buffer where the value of the
> + *              Status Register will be written.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)

Nitpick: rename to xilinx_read_sr()?

> +{
> +	int ret;
> +
> +	if (nor->spimem) {
> +		struct spi_mem_op op =
> +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_XRDSR, 0),
> +				   SPI_MEM_OP_NO_ADDR,
> +				   SPI_MEM_OP_NO_DUMMY,
> +				   SPI_MEM_OP_DATA_IN(1, sr, 0));
> +
> +		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> +
> +		ret = spi_mem_exec_op(nor->spimem, &op);
> +	} else {
> +		ret = spi_nor_controller_ops_read_reg(nor, SPINOR_OP_XRDSR, sr,
> +						      1);
> +	}
> +
> +	if (ret)
> +		dev_dbg(nor->dev, "error %d reading XRDSR\n", ret);
> +
> +	return ret;
> +}
> +
> +/**
> + * spi_nor_xsr_ready() - Query the Status Register of the S3AN flash to see if
> + * the flash is ready for new commands.
> + * @nor:	pointer to 'struct spi_nor'.
> + *
> + * Return: 1 if ready, 0 if not ready, -errno on errors.
> + */
> +static int spi_nor_xsr_ready(struct spi_nor *nor)

Nitpick: rename to xilinx_sr_ready()?

Either way,

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


-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v1 05/14] mtd: spi-nor: xilinx: rename vendor specific functions and defines

[Michael Walle]

Drop the generic spi_nor prefix for all the xilinx functions.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/xilinx.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/mtd/spi-nor/xilinx.c b/drivers/mtd/spi-nor/xilinx.c
index a865dadc2e5d..3e85530df1e4 100644
--- a/drivers/mtd/spi-nor/xilinx.c
+++ b/drivers/mtd/spi-nor/xilinx.c
@@ -8,9 +8,9 @@
 
 #include "core.h"
 
-#define SPINOR_OP_XSE		0x50	/* Sector erase */
-#define SPINOR_OP_XPP		0x82	/* Page program */
-#define SPINOR_OP_XRDSR		0xd7	/* Read status register */
+#define XILINX_OP_SE		0x50	/* Sector erase */
+#define XILINX_OP_PP		0x82	/* Page program */
+#define XILINX_OP_RDSR		0xd7	/* Read status register */
 
 #define XSR_PAGESIZE		BIT(0)	/* Page size in Po2 or Linear */
 #define XSR_RDY			BIT(7)	/* Ready */
@@ -60,20 +60,20 @@ static u32 s3an_convert_addr(struct spi_nor *nor, u32 addr)
 }
 
 /**
- * spi_nor_xread_sr() - Read the Status Register on S3AN flashes.
+ * xilinx_read_sr() - Read the Status Register on S3AN flashes.
  * @nor:	pointer to 'struct spi_nor'.
  * @sr:		pointer to a DMA-able buffer where the value of the
  *              Status Register will be written.
  *
  * Return: 0 on success, -errno otherwise.
  */
-static int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)
+static int xilinx_read_sr(struct spi_nor *nor, u8 *sr)
 {
 	int ret;
 
 	if (nor->spimem) {
 		struct spi_mem_op op =
-			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_XRDSR, 0),
+			SPI_MEM_OP(SPI_MEM_OP_CMD(XILINX_OP_RDSR, 0),
 				   SPI_MEM_OP_NO_ADDR,
 				   SPI_MEM_OP_NO_DUMMY,
 				   SPI_MEM_OP_DATA_IN(1, sr, 0));
@@ -82,7 +82,7 @@ static int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)
 
 		ret = spi_mem_exec_op(nor->spimem, &op);
 	} else {
-		ret = spi_nor_controller_ops_read_reg(nor, SPINOR_OP_XRDSR, sr,
+		ret = spi_nor_controller_ops_read_reg(nor, XILINX_OP_RDSR, sr,
 						      1);
 	}
 
@@ -99,11 +99,11 @@ static int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)
  *
  * Return: 1 if ready, 0 if not ready, -errno on errors.
  */
-static int spi_nor_xsr_ready(struct spi_nor *nor)
+static int xilinx_sr_ready(struct spi_nor *nor)
 {
 	int ret;
 
-	ret = spi_nor_xread_sr(nor, nor->bouncebuf);
+	ret = xilinx_read_sr(nor, nor->bouncebuf);
 	if (ret)
 		return ret;
 
@@ -116,12 +116,12 @@ static int xilinx_nor_setup(struct spi_nor *nor,
 	u32 page_size;
 	int ret;
 
-	ret = spi_nor_xread_sr(nor, nor->bouncebuf);
+	ret = xilinx_read_sr(nor, nor->bouncebuf);
 	if (ret)
 		return ret;
 
-	nor->erase_opcode = SPINOR_OP_XSE;
-	nor->program_opcode = SPINOR_OP_XPP;
+	nor->erase_opcode = XILINX_OP_SE;
+	nor->program_opcode = XILINX_OP_PP;
 	nor->read_opcode = SPINOR_OP_READ;
 	nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
 
@@ -155,7 +155,7 @@ static int xilinx_nor_setup(struct spi_nor *nor,
 static void xilinx_late_init(struct spi_nor *nor)
 {
 	nor->params->setup = xilinx_nor_setup;
-	nor->params->ready = spi_nor_xsr_ready;
+	nor->params->ready = xilinx_sr_ready;
 }
 
 static const struct spi_nor_fixups xilinx_fixups = {
-- 
2.30.2

Re: [PATCH v1 05/14] mtd: spi-nor: xilinx: rename vendor specific functions and defines

[kernel test robot]

Hi Michael,

I love your patch! Perhaps something to improve:

[auto build test WARNING on mtd/spi-nor/next]
[also build test WARNING on linux/master linus/master v5.17-rc2 next-20220202]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Michael-Walle/mtd-spi-nor-move-vendor-specific-code-into-vendor-modules/20220202-230139
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git spi-nor/next
config: alpha-allmodconfig (https://download.01.org/0day-ci/archive/20220203/202202030130.meVKrEGw-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/6527d5f32d65d5695ddcc0bcf3ac31928e64a935
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Michael-Walle/mtd-spi-nor-move-vendor-specific-code-into-vendor-modules/20220202-230139
        git checkout 6527d5f32d65d5695ddcc0bcf3ac31928e64a935
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=alpha SHELL=/bin/bash drivers/mtd/spi-nor/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/mtd/spi-nor/xilinx.c:103: warning: expecting prototype for spi_nor_xsr_ready(). Prototype was for xilinx_sr_ready() instead


vim +103 drivers/mtd/spi-nor/xilinx.c

37ccf5a41e1528 Michael Walle 2022-02-02   94  
37ccf5a41e1528 Michael Walle 2022-02-02   95  /**
37ccf5a41e1528 Michael Walle 2022-02-02   96   * spi_nor_xsr_ready() - Query the Status Register of the S3AN flash to see if
37ccf5a41e1528 Michael Walle 2022-02-02   97   * the flash is ready for new commands.
37ccf5a41e1528 Michael Walle 2022-02-02   98   * @nor:	pointer to 'struct spi_nor'.
37ccf5a41e1528 Michael Walle 2022-02-02   99   *
37ccf5a41e1528 Michael Walle 2022-02-02  100   * Return: 1 if ready, 0 if not ready, -errno on errors.
37ccf5a41e1528 Michael Walle 2022-02-02  101   */
6527d5f32d65d5 Michael Walle 2022-02-02  102  static int xilinx_sr_ready(struct spi_nor *nor)
37ccf5a41e1528 Michael Walle 2022-02-02 @103  {
37ccf5a41e1528 Michael Walle 2022-02-02  104  	int ret;
37ccf5a41e1528 Michael Walle 2022-02-02  105  
6527d5f32d65d5 Michael Walle 2022-02-02  106  	ret = xilinx_read_sr(nor, nor->bouncebuf);
37ccf5a41e1528 Michael Walle 2022-02-02  107  	if (ret)
37ccf5a41e1528 Michael Walle 2022-02-02  108  		return ret;
37ccf5a41e1528 Michael Walle 2022-02-02  109  
37ccf5a41e1528 Michael Walle 2022-02-02  110  	return !!(nor->bouncebuf[0] & XSR_RDY);
37ccf5a41e1528 Michael Walle 2022-02-02  111  }
37ccf5a41e1528 Michael Walle 2022-02-02  112  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH v1 05/14] mtd: spi-nor: xilinx: rename vendor specific functions and defines

[kernel test robot]

Hi Michael,

I love your patch! Perhaps something to improve:

[auto build test WARNING on mtd/spi-nor/next]
[also build test WARNING on linux/master linus/master v5.17-rc2 next-20220202]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Michael-Walle/mtd-spi-nor-move-vendor-specific-code-into-vendor-modules/20220202-230139
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git spi-nor/next
config: riscv-randconfig-r042-20220131 (https://download.01.org/0day-ci/archive/20220203/202202030313.BlFmGeWH-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 6b1e844b69f15bb7dffaf9365cd2b355d2eb7579)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/0day-ci/linux/commit/6527d5f32d65d5695ddcc0bcf3ac31928e64a935
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Michael-Walle/mtd-spi-nor-move-vendor-specific-code-into-vendor-modules/20220202-230139
        git checkout 6527d5f32d65d5695ddcc0bcf3ac31928e64a935
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/mtd/spi-nor/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/mtd/spi-nor/xilinx.c:103: warning: expecting prototype for spi_nor_xsr_ready(). Prototype was for xilinx_sr_ready() instead


vim +103 drivers/mtd/spi-nor/xilinx.c

37ccf5a41e1528e Michael Walle 2022-02-02   94  
37ccf5a41e1528e Michael Walle 2022-02-02   95  /**
37ccf5a41e1528e Michael Walle 2022-02-02   96   * spi_nor_xsr_ready() - Query the Status Register of the S3AN flash to see if
37ccf5a41e1528e Michael Walle 2022-02-02   97   * the flash is ready for new commands.
37ccf5a41e1528e Michael Walle 2022-02-02   98   * @nor:	pointer to 'struct spi_nor'.
37ccf5a41e1528e Michael Walle 2022-02-02   99   *
37ccf5a41e1528e Michael Walle 2022-02-02  100   * Return: 1 if ready, 0 if not ready, -errno on errors.
37ccf5a41e1528e Michael Walle 2022-02-02  101   */
6527d5f32d65d56 Michael Walle 2022-02-02  102  static int xilinx_sr_ready(struct spi_nor *nor)
37ccf5a41e1528e Michael Walle 2022-02-02 @103  {
37ccf5a41e1528e Michael Walle 2022-02-02  104  	int ret;
37ccf5a41e1528e Michael Walle 2022-02-02  105  
6527d5f32d65d56 Michael Walle 2022-02-02  106  	ret = xilinx_read_sr(nor, nor->bouncebuf);
37ccf5a41e1528e Michael Walle 2022-02-02  107  	if (ret)
37ccf5a41e1528e Michael Walle 2022-02-02  108  		return ret;
37ccf5a41e1528e Michael Walle 2022-02-02  109  
37ccf5a41e1528e Michael Walle 2022-02-02  110  	return !!(nor->bouncebuf[0] & XSR_RDY);
37ccf5a41e1528e Michael Walle 2022-02-02  111  }
37ccf5a41e1528e Michael Walle 2022-02-02  112  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH v1 05/14] mtd: spi-nor: xilinx: rename vendor specific functions and defines

[Tudor.Ambarus]

On 2/2/22 16:58, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Drop the generic spi_nor prefix for all the xilinx functions.

mm, no, I would keep the spi_nor prefix because xilinx_sr_ready is too
generic and can conflict with methods from other subsystems.

> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/xilinx.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/xilinx.c b/drivers/mtd/spi-nor/xilinx.c
> index a865dadc2e5d..3e85530df1e4 100644
> --- a/drivers/mtd/spi-nor/xilinx.c
> +++ b/drivers/mtd/spi-nor/xilinx.c
> @@ -8,9 +8,9 @@
> 
>  #include "core.h"
> 
> -#define SPINOR_OP_XSE          0x50    /* Sector erase */
> -#define SPINOR_OP_XPP          0x82    /* Page program */
> -#define SPINOR_OP_XRDSR                0xd7    /* Read status register */
> +#define XILINX_OP_SE           0x50    /* Sector erase */
> +#define XILINX_OP_PP           0x82    /* Page program */
> +#define XILINX_OP_RDSR         0xd7    /* Read status register */
> 
>  #define XSR_PAGESIZE           BIT(0)  /* Page size in Po2 or Linear */
>  #define XSR_RDY                        BIT(7)  /* Ready */
> @@ -60,20 +60,20 @@ static u32 s3an_convert_addr(struct spi_nor *nor, u32 addr)
>  }
> 
>  /**
> - * spi_nor_xread_sr() - Read the Status Register on S3AN flashes.
> + * xilinx_read_sr() - Read the Status Register on S3AN flashes.
>   * @nor:       pointer to 'struct spi_nor'.
>   * @sr:                pointer to a DMA-able buffer where the value of the
>   *              Status Register will be written.
>   *
>   * Return: 0 on success, -errno otherwise.
>   */
> -static int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)
> +static int xilinx_read_sr(struct spi_nor *nor, u8 *sr)
>  {
>         int ret;
> 
>         if (nor->spimem) {
>                 struct spi_mem_op op =
> -                       SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_XRDSR, 0),
> +                       SPI_MEM_OP(SPI_MEM_OP_CMD(XILINX_OP_RDSR, 0),
>                                    SPI_MEM_OP_NO_ADDR,
>                                    SPI_MEM_OP_NO_DUMMY,
>                                    SPI_MEM_OP_DATA_IN(1, sr, 0));
> @@ -82,7 +82,7 @@ static int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)
> 
>                 ret = spi_mem_exec_op(nor->spimem, &op);
>         } else {
> -               ret = spi_nor_controller_ops_read_reg(nor, SPINOR_OP_XRDSR, sr,
> +               ret = spi_nor_controller_ops_read_reg(nor, XILINX_OP_RDSR, sr,
>                                                       1);
>         }
> 
> @@ -99,11 +99,11 @@ static int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)
>   *
>   * Return: 1 if ready, 0 if not ready, -errno on errors.
>   */
> -static int spi_nor_xsr_ready(struct spi_nor *nor)
> +static int xilinx_sr_ready(struct spi_nor *nor)
>  {
>         int ret;
> 
> -       ret = spi_nor_xread_sr(nor, nor->bouncebuf);
> +       ret = xilinx_read_sr(nor, nor->bouncebuf);
>         if (ret)
>                 return ret;
> 
> @@ -116,12 +116,12 @@ static int xilinx_nor_setup(struct spi_nor *nor,
>         u32 page_size;
>         int ret;
> 
> -       ret = spi_nor_xread_sr(nor, nor->bouncebuf);
> +       ret = xilinx_read_sr(nor, nor->bouncebuf);
>         if (ret)
>                 return ret;
> 
> -       nor->erase_opcode = SPINOR_OP_XSE;
> -       nor->program_opcode = SPINOR_OP_XPP;
> +       nor->erase_opcode = XILINX_OP_SE;
> +       nor->program_opcode = XILINX_OP_PP;
>         nor->read_opcode = SPINOR_OP_READ;
>         nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
> 
> @@ -155,7 +155,7 @@ static int xilinx_nor_setup(struct spi_nor *nor,
>  static void xilinx_late_init(struct spi_nor *nor)
>  {
>         nor->params->setup = xilinx_nor_setup;
> -       nor->params->ready = spi_nor_xsr_ready;
> +       nor->params->ready = xilinx_sr_ready;
>  }
> 
>  static const struct spi_nor_fixups xilinx_fixups = {
> --
> 2.30.2
> 

Re: [PATCH v1 05/14] mtd: spi-nor: xilinx: rename vendor specific functions and defines

[Michael Walle]

Am 2022-02-10 04:08, schrieb Tudor.Ambarus@microchip.com:
> On 2/2/22 16:58, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Drop the generic spi_nor prefix for all the xilinx functions.
> 
> mm, no, I would keep the spi_nor prefix because xilinx_sr_ready is too
> generic and can conflict with methods from other subsystems.

But all the other functions in this file start with xilinx_ ;)

I don't have a strong opinion here, other than it shouldn't
be called spi_nor_read_blaba() because that looks like a
standard spi nor function belonging in core.c

-michael

Re: [PATCH v1 05/14] mtd: spi-nor: xilinx: rename vendor specific functions and defines

[Tudor.Ambarus]

On 2/10/22 10:04, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-02-10 04:08, schrieb Tudor.Ambarus@microchip.com:
>> On 2/2/22 16:58, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Drop the generic spi_nor prefix for all the xilinx functions.
>>
>> mm, no, I would keep the spi_nor prefix because xilinx_sr_ready is too
>> generic and can conflict with methods from other subsystems.
> 
> But all the other functions in this file start with xilinx_ ;)
> 
> I don't have a strong opinion here, other than it shouldn't
> be called spi_nor_read_blaba() because that looks like a
> standard spi nor function belonging in core.c
> 

then let's prepend all with spi_nor_xilinx_*()?

Re: [PATCH v1 05/14] mtd: spi-nor: xilinx: rename vendor specific functions and defines

[Michael Walle]

Am 2022-02-10 09:06, schrieb Tudor.Ambarus@microchip.com:
> On 2/10/22 10:04, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Am 2022-02-10 04:08, schrieb Tudor.Ambarus@microchip.com:
>>> On 2/2/22 16:58, Michael Walle wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you 
>>>> know
>>>> the content is safe
>>>> 
>>>> Drop the generic spi_nor prefix for all the xilinx functions.
>>> 
>>> mm, no, I would keep the spi_nor prefix because xilinx_sr_ready is 
>>> too
>>> generic and can conflict with methods from other subsystems.
>> 
>> But all the other functions in this file start with xilinx_ ;)
>> 
>> I don't have a strong opinion here, other than it shouldn't
>> be called spi_nor_read_blaba() because that looks like a
>> standard spi nor function belonging in core.c
>> 
> 
> then let's prepend all with spi_nor_xilinx_*()?

I'm still not sure what to do here. Have a look at all the other
vendor modules in spi-nor. they are all prefixed with the vendor
name? E.g. there is a sst_write() which is far more likely to
cause a conflict. So should we rename all these functions? Or
do we just take our chance that it might have a conflict in
the future (with an easy fix to rename the function then). TBH
I doubt there will be a global symbol "xilinx_read_sr()".

But I care for consistency, so having some named xilinx_, sst_,
st_micron_ and some spi_nor_read_xsr sounds and looks awful.

-michael

Re: [PATCH v1 05/14] mtd: spi-nor: xilinx: rename vendor specific functions and defines

[Tudor.Ambarus]

Miquel in To:

On 2/15/22 10:25, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-02-10 09:06, schrieb Tudor.Ambarus@microchip.com:
>> On 2/10/22 10:04, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Am 2022-02-10 04:08, schrieb Tudor.Ambarus@microchip.com:
>>>> On 2/2/22 16:58, Michael Walle wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>> know
>>>>> the content is safe
>>>>>
>>>>> Drop the generic spi_nor prefix for all the xilinx functions.
>>>>
>>>> mm, no, I would keep the spi_nor prefix because xilinx_sr_ready is
>>>> too
>>>> generic and can conflict with methods from other subsystems.
>>>
>>> But all the other functions in this file start with xilinx_ ;)
>>>
>>> I don't have a strong opinion here, other than it shouldn't
>>> be called spi_nor_read_blaba() because that looks like a
>>> standard spi nor function belonging in core.c
>>>
>>
>> then let's prepend all with spi_nor_xilinx_*()?
> 
> I'm still not sure what to do here. Have a look at all the other
> vendor modules in spi-nor. they are all prefixed with the vendor
> name? E.g. there is a sst_write() which is far more likely to
> cause a conflict. So should we rename all these functions? Or
> do we just take our chance that it might have a conflict in
> the future (with an easy fix to rename the function then). TBH
> I doubt there will be a global symbol "xilinx_read_sr()".

I doubt it will not be a conflict.

> 
> But I care for consistency, so having some named xilinx_, sst_,
> st_micron_ and some spi_nor_read_xsr sounds and looks awful.

yes, I agree. Take a look on what's happening in NAND. They prepend
the name with vendor_nand_*(). Or in SPI NAND they use flash family
names which should be unique. So how about aligning with NAND and
use vendor_nor_*()?

Re: [PATCH v1 05/14] mtd: spi-nor: xilinx: rename vendor specific functions and defines

[Michael Walle]

Am 2022-02-15 09:52, schrieb Tudor.Ambarus@microchip.com:
> Miquel in To:
> 
> On 2/15/22 10:25, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Am 2022-02-10 09:06, schrieb Tudor.Ambarus@microchip.com:
>>> On 2/10/22 10:04, Michael Walle wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you 
>>>> know
>>>> the content is safe
>>>> 
>>>> Am 2022-02-10 04:08, schrieb Tudor.Ambarus@microchip.com:
>>>>> On 2/2/22 16:58, Michael Walle wrote:
>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>>> know
>>>>>> the content is safe
>>>>>> 
>>>>>> Drop the generic spi_nor prefix for all the xilinx functions.
>>>>> 
>>>>> mm, no, I would keep the spi_nor prefix because xilinx_sr_ready is
>>>>> too
>>>>> generic and can conflict with methods from other subsystems.
>>>> 
>>>> But all the other functions in this file start with xilinx_ ;)
>>>> 
>>>> I don't have a strong opinion here, other than it shouldn't
>>>> be called spi_nor_read_blaba() because that looks like a
>>>> standard spi nor function belonging in core.c
>>>> 
>>> 
>>> then let's prepend all with spi_nor_xilinx_*()?
>> 
>> I'm still not sure what to do here. Have a look at all the other
>> vendor modules in spi-nor. they are all prefixed with the vendor
>> name? E.g. there is a sst_write() which is far more likely to
>> cause a conflict. So should we rename all these functions? Or
>> do we just take our chance that it might have a conflict in
>> the future (with an easy fix to rename the function then). TBH
>> I doubt there will be a global symbol "xilinx_read_sr()".
> 
> I doubt it will not be a conflict.
> 
>> 
>> But I care for consistency, so having some named xilinx_, sst_,
>> st_micron_ and some spi_nor_read_xsr sounds and looks awful.
> 
> yes, I agree. Take a look on what's happening in NAND. They prepend
> the name with vendor_nand_*(). Or in SPI NAND they use flash family
> names which should be unique. So how about aligning with NAND and
> use vendor_nor_*()?

Sounds good. Regarding the flash family.. take a look at Winbond W25M
which can either be NAND or NOR depending on the size ;)

But the main question was rather whether we rename all the function
names at once or bit by bit. To proceed here with this series, I'd
use the vendor_nor_ prefix for the moved functions (but still keep
the micron_st_ st_micron_ rename patch).

-michael

Re: [PATCH v1 05/14] mtd: spi-nor: xilinx: rename vendor specific functions and defines

[Tudor.Ambarus]

On 2/15/22 11:58, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-02-15 09:52, schrieb Tudor.Ambarus@microchip.com:
>> Miquel in To:
>>
>> On 2/15/22 10:25, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Am 2022-02-10 09:06, schrieb Tudor.Ambarus@microchip.com:
>>>> On 2/10/22 10:04, Michael Walle wrote:
>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>> know
>>>>> the content is safe
>>>>>
>>>>> Am 2022-02-10 04:08, schrieb Tudor.Ambarus@microchip.com:
>>>>>> On 2/2/22 16:58, Michael Walle wrote:
>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you
>>>>>>> know
>>>>>>> the content is safe
>>>>>>>
>>>>>>> Drop the generic spi_nor prefix for all the xilinx functions.
>>>>>>
>>>>>> mm, no, I would keep the spi_nor prefix because xilinx_sr_ready is
>>>>>> too
>>>>>> generic and can conflict with methods from other subsystems.
>>>>>
>>>>> But all the other functions in this file start with xilinx_ ;)
>>>>>
>>>>> I don't have a strong opinion here, other than it shouldn't
>>>>> be called spi_nor_read_blaba() because that looks like a
>>>>> standard spi nor function belonging in core.c
>>>>>
>>>>
>>>> then let's prepend all with spi_nor_xilinx_*()?
>>>
>>> I'm still not sure what to do here. Have a look at all the other
>>> vendor modules in spi-nor. they are all prefixed with the vendor
>>> name? E.g. there is a sst_write() which is far more likely to
>>> cause a conflict. So should we rename all these functions? Or
>>> do we just take our chance that it might have a conflict in
>>> the future (with an easy fix to rename the function then). TBH
>>> I doubt there will be a global symbol "xilinx_read_sr()".
>>
>> I doubt it will not be a conflict.
>>
>>>
>>> But I care for consistency, so having some named xilinx_, sst_,
>>> st_micron_ and some spi_nor_read_xsr sounds and looks awful.
>>
>> yes, I agree. Take a look on what's happening in NAND. They prepend
>> the name with vendor_nand_*(). Or in SPI NAND they use flash family
>> names which should be unique. So how about aligning with NAND and
>> use vendor_nor_*()?
> 
> Sounds good. Regarding the flash family.. take a look at Winbond W25M
> which can either be NAND or NOR depending on the size ;)

right, vendor_nor_*() should be just fine

> 
> But the main question was rather whether we rename all the function
> names at once or bit by bit. To proceed here with this series, I'd
> use the vendor_nor_ prefix for the moved functions (but still keep
> the micron_st_ st_micron_ rename patch).
> 

let's rename them all in a single patch before moving the vendor code
out of the core, and then you can fit with the moved bits in an
elegant way ;)

Re: [PATCH v1 05/14] mtd: spi-nor: xilinx: rename vendor specific functions and defines

[Pratyush Yadav]

On 02/02/22 03:58PM, Michael Walle wrote:
> Drop the generic spi_nor prefix for all the xilinx functions.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/xilinx.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/xilinx.c b/drivers/mtd/spi-nor/xilinx.c
> index a865dadc2e5d..3e85530df1e4 100644
> --- a/drivers/mtd/spi-nor/xilinx.c
> +++ b/drivers/mtd/spi-nor/xilinx.c
> @@ -8,9 +8,9 @@
>  
>  #include "core.h"
>  
> -#define SPINOR_OP_XSE		0x50	/* Sector erase */
> -#define SPINOR_OP_XPP		0x82	/* Page program */
> -#define SPINOR_OP_XRDSR		0xd7	/* Read status register */
> +#define XILINX_OP_SE		0x50	/* Sector erase */
> +#define XILINX_OP_PP		0x82	/* Page program */
> +#define XILINX_OP_RDSR		0xd7	/* Read status register */
>  
>  #define XSR_PAGESIZE		BIT(0)	/* Page size in Po2 or Linear */
>  #define XSR_RDY			BIT(7)	/* Ready */
> @@ -60,20 +60,20 @@ static u32 s3an_convert_addr(struct spi_nor *nor, u32 addr)
>  }
>  
>  /**
> - * spi_nor_xread_sr() - Read the Status Register on S3AN flashes.
> + * xilinx_read_sr() - Read the Status Register on S3AN flashes.
>   * @nor:	pointer to 'struct spi_nor'.
>   * @sr:		pointer to a DMA-able buffer where the value of the
>   *              Status Register will be written.
>   *
>   * Return: 0 on success, -errno otherwise.
>   */
> -static int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr)
> +static int xilinx_read_sr(struct spi_nor *nor, u8 *sr)

Great minds think alike huh ;-)

But I agree with Tudor. vendor_nor_* or spi_nor_vendor_* would be 
better. I don't have much preference for any though. Please also 
remember to update the name in the doc comment, as the kernel test robot 
has already pointed out for this patch.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v1 06/14] mtd: spi-nor: xilinx: correct the debug message

[Michael Walle]

We are reading the status register, there is no XRDSR register.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/xilinx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/xilinx.c b/drivers/mtd/spi-nor/xilinx.c
index 3e85530df1e4..9e3ed9609250 100644
--- a/drivers/mtd/spi-nor/xilinx.c
+++ b/drivers/mtd/spi-nor/xilinx.c
@@ -87,7 +87,7 @@ static int xilinx_read_sr(struct spi_nor *nor, u8 *sr)
 	}
 
 	if (ret)
-		dev_dbg(nor->dev, "error %d reading XRDSR\n", ret);
+		dev_dbg(nor->dev, "error %d reading SR\n", ret);
 
 	return ret;
 }
-- 
2.30.2

Re: [PATCH v1 06/14] mtd: spi-nor: xilinx: correct the debug message

[Tudor.Ambarus]

On 2/2/22 16:58, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> We are reading the status register, there is no XRDSR register.

don't use the commit message as a continuation of the subject. Tell in the
commit message what you're doing and why it is worth taking it.

with that fixed:
Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/xilinx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/xilinx.c b/drivers/mtd/spi-nor/xilinx.c
> index 3e85530df1e4..9e3ed9609250 100644
> --- a/drivers/mtd/spi-nor/xilinx.c
> +++ b/drivers/mtd/spi-nor/xilinx.c
> @@ -87,7 +87,7 @@ static int xilinx_read_sr(struct spi_nor *nor, u8 *sr)
>         }
> 
>         if (ret)
> -               dev_dbg(nor->dev, "error %d reading XRDSR\n", ret);
> +               dev_dbg(nor->dev, "error %d reading SR\n", ret);
> 
>         return ret;
>  }
> --
> 2.30.2
> 

Re: [PATCH v1 06/14] mtd: spi-nor: xilinx: correct the debug message

[Pratyush Yadav]

On 10/02/22 03:11AM, Tudor.Ambarus@microchip.com wrote:
> On 2/2/22 16:58, Michael Walle wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > We are reading the status register, there is no XRDSR register.
> 
> don't use the commit message as a continuation of the subject. Tell in the
> commit message what you're doing and why it is worth taking it.
> 
> with that fixed:
> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v1 07/14] mtd: spi-nor: move all micron-st specifics into micron-st.c

[Michael Walle]

The flag status register is only available on micron flashes. Move all
the functions around that into the micron module.

This is almost a mechanical move except for the spi_nor_fsr_ready()
which now also checks the normal status register. Previously, this was
done in spi_nor_ready().

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/core.c      | 123 +-----------------------------
 drivers/mtd/spi-nor/micron-st.c | 129 ++++++++++++++++++++++++++++++++
 include/linux/mtd/spi-nor.h     |   8 --
 3 files changed, 130 insertions(+), 130 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index fdc8ef623254..e9d9880149d2 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -412,50 +412,6 @@ int spi_nor_read_sr(struct spi_nor *nor, u8 *sr)
 	return ret;
 }
 
-/**
- * spi_nor_read_fsr() - Read the Flag Status Register.
- * @nor:	pointer to 'struct spi_nor'
- * @fsr:	pointer to a DMA-able buffer where the value of the
- *              Flag Status Register will be written. Should be at least 2
- *              bytes.
- *
- * Return: 0 on success, -errno otherwise.
- */
-static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr)
-{
-	int ret;
-
-	if (nor->spimem) {
-		struct spi_mem_op op =
-			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDFSR, 0),
-				   SPI_MEM_OP_NO_ADDR,
-				   SPI_MEM_OP_NO_DUMMY,
-				   SPI_MEM_OP_DATA_IN(1, fsr, 0));
-
-		if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR) {
-			op.addr.nbytes = nor->params->rdsr_addr_nbytes;
-			op.dummy.nbytes = nor->params->rdsr_dummy;
-			/*
-			 * We don't want to read only one byte in DTR mode. So,
-			 * read 2 and then discard the second byte.
-			 */
-			op.data.nbytes = 2;
-		}
-
-		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
-
-		ret = spi_mem_exec_op(nor->spimem, &op);
-	} else {
-		ret = spi_nor_controller_ops_read_reg(nor, SPINOR_OP_RDFSR, fsr,
-						      1);
-	}
-
-	if (ret)
-		dev_dbg(nor->dev, "error %d reading FSR\n", ret);
-
-	return ret;
-}
-
 /**
  * spi_nor_read_cr() - Read the Configuration Register using the
  * SPINOR_OP_RDCR (35h) command.
@@ -664,75 +620,6 @@ int spi_nor_sr_ready(struct spi_nor *nor)
 	return !(nor->bouncebuf[0] & SR_WIP);
 }
 
-/**
- * spi_nor_clear_fsr() - Clear the Flag Status Register.
- * @nor:	pointer to 'struct spi_nor'.
- */
-static void spi_nor_clear_fsr(struct spi_nor *nor)
-{
-	int ret;
-
-	if (nor->spimem) {
-		struct spi_mem_op op =
-			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLFSR, 0),
-				   SPI_MEM_OP_NO_ADDR,
-				   SPI_MEM_OP_NO_DUMMY,
-				   SPI_MEM_OP_NO_DATA);
-
-		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
-
-		ret = spi_mem_exec_op(nor->spimem, &op);
-	} else {
-		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLFSR,
-						       NULL, 0);
-	}
-
-	if (ret)
-		dev_dbg(nor->dev, "error %d clearing FSR\n", ret);
-}
-
-/**
- * spi_nor_fsr_ready() - Query the Flag Status Register to see if the flash is
- * ready for new commands.
- * @nor:	pointer to 'struct spi_nor'.
- *
- * Return: 1 if ready, 0 if not ready, -errno on errors.
- */
-static int spi_nor_fsr_ready(struct spi_nor *nor)
-{
-	int ret = spi_nor_read_fsr(nor, nor->bouncebuf);
-
-	if (ret)
-		return ret;
-
-	if (nor->bouncebuf[0] & (FSR_E_ERR | FSR_P_ERR)) {
-		if (nor->bouncebuf[0] & FSR_E_ERR)
-			dev_err(nor->dev, "Erase operation failed.\n");
-		else
-			dev_err(nor->dev, "Program operation failed.\n");
-
-		if (nor->bouncebuf[0] & FSR_PT_ERR)
-			dev_err(nor->dev,
-			"Attempted to modify a protected sector.\n");
-
-		spi_nor_clear_fsr(nor);
-
-		/*
-		 * WEL bit remains set to one when an erase or page program
-		 * error occurs. Issue a Write Disable command to protect
-		 * against inadvertent writes that can possibly corrupt the
-		 * contents of the memory.
-		 */
-		ret = spi_nor_write_disable(nor);
-		if (ret)
-			return ret;
-
-		return -EIO;
-	}
-
-	return !!(nor->bouncebuf[0] & FSR_READY);
-}
-
 /**
  * spi_nor_ready() - Query the flash to see if it is ready for new commands.
  * @nor:	pointer to 'struct spi_nor'.
@@ -741,19 +628,11 @@ static int spi_nor_fsr_ready(struct spi_nor *nor)
  */
 static int spi_nor_ready(struct spi_nor *nor)
 {
-	int sr, fsr;
-
 	/* flashes might override our standard routine */
 	if (nor->params->ready)
 		return nor->params->ready(nor);
 
-	sr = spi_nor_sr_ready(nor);
-	if (sr < 0)
-		return sr;
-	fsr = nor->flags & SNOR_F_USE_FSR ? spi_nor_fsr_ready(nor) : 1;
-	if (fsr < 0)
-		return fsr;
-	return sr && fsr;
+	return spi_nor_sr_ready(nor);
 }
 
 /**
diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
index bb95b1aabf74..c66580e8aa00 100644
--- a/drivers/mtd/spi-nor/micron-st.c
+++ b/drivers/mtd/spi-nor/micron-st.c
@@ -8,6 +8,8 @@
 
 #include "core.h"
 
+#define SPINOR_OP_RDFSR		0x70	/* Read flag status register */
+#define SPINOR_OP_CLFSR		0x50	/* Clear flag status register */
 #define SPINOR_OP_MT_DTR_RD	0xfd	/* Fast Read opcode in DTR mode */
 #define SPINOR_OP_MT_RD_ANY_REG	0x85	/* Read volatile register */
 #define SPINOR_OP_MT_WR_ANY_REG	0x81	/* Write volatile register */
@@ -17,6 +19,12 @@
 #define SPINOR_MT_OCT_DTR	0xe7	/* Enable Octal DTR. */
 #define SPINOR_MT_EXSPI		0xff	/* Enable Extended SPI (default) */
 
+/* Flag Status Register bits */
+#define FSR_READY		BIT(7)	/* Device status, 0 = Busy, 1 = Ready */
+#define FSR_E_ERR		BIT(5)	/* Erase operation status */
+#define FSR_P_ERR		BIT(4)	/* Program operation status */
+#define FSR_PT_ERR		BIT(1)	/* Protection error bit */
+
 static int spi_nor_micron_octal_dtr_enable(struct spi_nor *nor, bool enable)
 {
 	struct spi_mem_op op;
@@ -273,12 +281,133 @@ static int st_micron_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
 	return spi_nor_write_disable(nor);
 }
 
+/**
+ * spi_nor_read_fsr() - Read the Flag Status Register.
+ * @nor:	pointer to 'struct spi_nor'
+ * @fsr:	pointer to a DMA-able buffer where the value of the
+ *              Flag Status Register will be written. Should be at least 2
+ *              bytes.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr)
+{
+	int ret;
+
+	if (nor->spimem) {
+		struct spi_mem_op op =
+			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDFSR, 0),
+				   SPI_MEM_OP_NO_ADDR,
+				   SPI_MEM_OP_NO_DUMMY,
+				   SPI_MEM_OP_DATA_IN(1, fsr, 0));
+
+		if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR) {
+			op.addr.nbytes = nor->params->rdsr_addr_nbytes;
+			op.dummy.nbytes = nor->params->rdsr_dummy;
+			/*
+			 * We don't want to read only one byte in DTR mode. So,
+			 * read 2 and then discard the second byte.
+			 */
+			op.data.nbytes = 2;
+		}
+
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
+		ret = spi_mem_exec_op(nor->spimem, &op);
+	} else {
+		ret = spi_nor_controller_ops_read_reg(nor, SPINOR_OP_RDFSR, fsr,
+						      1);
+	}
+
+	if (ret)
+		dev_dbg(nor->dev, "error %d reading FSR\n", ret);
+
+	return ret;
+}
+
+/**
+ * spi_nor_clear_fsr() - Clear the Flag Status Register.
+ * @nor:	pointer to 'struct spi_nor'.
+ */
+static void spi_nor_clear_fsr(struct spi_nor *nor)
+{
+	int ret;
+
+	if (nor->spimem) {
+		struct spi_mem_op op =
+			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLFSR, 0),
+				   SPI_MEM_OP_NO_ADDR,
+				   SPI_MEM_OP_NO_DUMMY,
+				   SPI_MEM_OP_NO_DATA);
+
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
+		ret = spi_mem_exec_op(nor->spimem, &op);
+	} else {
+		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLFSR,
+						       NULL, 0);
+	}
+
+	if (ret)
+		dev_dbg(nor->dev, "error %d clearing FSR\n", ret);
+}
+
+/**
+ * spi_nor_fsr_ready() - Query the Flag Status Register to see if the flash is
+ * ready for new commands.
+ * @nor:	pointer to 'struct spi_nor'.
+ *
+ * Return: 1 if ready, 0 if not ready, -errno on errors.
+ */
+static int spi_nor_fsr_ready(struct spi_nor *nor)
+{
+	int sr_ready, ret;
+
+	sr_ready = spi_nor_sr_ready(nor);
+	if (sr_ready < 0)
+		return sr_ready;
+
+	ret = spi_nor_read_fsr(nor, nor->bouncebuf);
+	if (ret)
+		return ret;
+
+	if (nor->bouncebuf[0] & (FSR_E_ERR | FSR_P_ERR)) {
+		if (nor->bouncebuf[0] & FSR_E_ERR)
+			dev_err(nor->dev, "Erase operation failed.\n");
+		else
+			dev_err(nor->dev, "Program operation failed.\n");
+
+		if (nor->bouncebuf[0] & FSR_PT_ERR)
+			dev_err(nor->dev,
+			"Attempted to modify a protected sector.\n");
+
+		spi_nor_clear_fsr(nor);
+
+		/*
+		 * WEL bit remains set to one when an erase or page program
+		 * error occurs. Issue a Write Disable command to protect
+		 * against inadvertent writes that can possibly corrupt the
+		 * contents of the memory.
+		 */
+		ret = spi_nor_write_disable(nor);
+		if (ret)
+			return ret;
+
+		return -EIO;
+	}
+
+	return sr_ready && !!(nor->bouncebuf[0] & FSR_READY);
+}
+
 static void micron_st_default_init(struct spi_nor *nor)
 {
 	nor->flags |= SNOR_F_HAS_LOCK;
 	nor->flags &= ~SNOR_F_HAS_16BIT_SR;
 	nor->params->quad_enable = NULL;
 	nor->params->set_4byte_addr_mode = st_micron_set_4byte_addr_mode;
+
+	if (nor->flags & SNOR_F_USE_FSR)
+		nor->params->ready = spi_nor_fsr_ready;
 }
 
 static const struct spi_nor_fixups micron_st_fixups = {
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index b44b05a6f934..4622251a79ff 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -47,8 +47,6 @@
 #define SPINOR_OP_RDID		0x9f	/* Read JEDEC ID */
 #define SPINOR_OP_RDSFDP	0x5a	/* Read SFDP */
 #define SPINOR_OP_RDCR		0x35	/* Read configuration register */
-#define SPINOR_OP_RDFSR		0x70	/* Read flag status register */
-#define SPINOR_OP_CLFSR		0x50	/* Clear flag status register */
 #define SPINOR_OP_RDEAR		0xc8	/* Read Extended Address Register */
 #define SPINOR_OP_WREAR		0xc5	/* Write Extended Address Register */
 #define SPINOR_OP_SRSTEN	0x66	/* Software Reset Enable */
@@ -126,12 +124,6 @@
 /* Enhanced Volatile Configuration Register bits */
 #define EVCR_QUAD_EN_MICRON	BIT(7)	/* Micron Quad I/O */
 
-/* Flag Status Register bits */
-#define FSR_READY		BIT(7)	/* Device status, 0 = Busy, 1 = Ready */
-#define FSR_E_ERR		BIT(5)	/* Erase operation status */
-#define FSR_P_ERR		BIT(4)	/* Program operation status */
-#define FSR_PT_ERR		BIT(1)	/* Protection error bit */
-
 /* Status Register 2 bits. */
 #define SR2_QUAD_EN_BIT1	BIT(1)
 #define SR2_LB1			BIT(3)	/* Security Register Lock Bit 1 */
-- 
2.30.2

Re: [PATCH v1 07/14] mtd: spi-nor: move all micron-st specifics into micron-st.c

[Tudor.Ambarus]

On 2/2/22 16:58, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> The flag status register is only available on micron flashes. Move all
> the functions around that into the micron module.
> 
> This is almost a mechanical move except for the spi_nor_fsr_ready()
> which now also checks the normal status register. Previously, this was
> done in spi_nor_ready().
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/core.c      | 123 +-----------------------------
>  drivers/mtd/spi-nor/micron-st.c | 129 ++++++++++++++++++++++++++++++++
>  include/linux/mtd/spi-nor.h     |   8 --
>  3 files changed, 130 insertions(+), 130 deletions(-)
> 

cut

> diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
> index bb95b1aabf74..c66580e8aa00 100644
> --- a/drivers/mtd/spi-nor/micron-st.c
> +++ b/drivers/mtd/spi-nor/micron-st.c

>  static void micron_st_default_init(struct spi_nor *nor)
>  {
>         nor->flags |= SNOR_F_HAS_LOCK;
>         nor->flags &= ~SNOR_F_HAS_16BIT_SR;
>         nor->params->quad_enable = NULL;
>         nor->params->set_4byte_addr_mode = st_micron_set_4byte_addr_mode;
> +
> +       if (nor->flags & SNOR_F_USE_FSR)
> +               nor->params->ready = spi_nor_fsr_ready;

I would like to get rid of the default_init hooks. Can't we use late_init for
setting the ready method?

Re: [PATCH v1 07/14] mtd: spi-nor: move all micron-st specifics into micron-st.c

[Pratyush Yadav]

On 02/02/22 03:58PM, Michael Walle wrote:
> The flag status register is only available on micron flashes. Move all
> the functions around that into the micron module.
> 
> This is almost a mechanical move except for the spi_nor_fsr_ready()
> which now also checks the normal status register. Previously, this was
> done in spi_nor_ready().
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/core.c      | 123 +-----------------------------
>  drivers/mtd/spi-nor/micron-st.c | 129 ++++++++++++++++++++++++++++++++
>  include/linux/mtd/spi-nor.h     |   8 --
>  3 files changed, 130 insertions(+), 130 deletions(-)
> 
[...]
> diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
> index bb95b1aabf74..c66580e8aa00 100644
> --- a/drivers/mtd/spi-nor/micron-st.c
> +++ b/drivers/mtd/spi-nor/micron-st.c
> @@ -8,6 +8,8 @@
>  
>  #include "core.h"
>  
> +#define SPINOR_OP_RDFSR		0x70	/* Read flag status register */
> +#define SPINOR_OP_CLFSR		0x50	/* Clear flag status register */
>  #define SPINOR_OP_MT_DTR_RD	0xfd	/* Fast Read opcode in DTR mode */
>  #define SPINOR_OP_MT_RD_ANY_REG	0x85	/* Read volatile register */
>  #define SPINOR_OP_MT_WR_ANY_REG	0x81	/* Write volatile register */
> @@ -17,6 +19,12 @@
>  #define SPINOR_MT_OCT_DTR	0xe7	/* Enable Octal DTR. */
>  #define SPINOR_MT_EXSPI		0xff	/* Enable Extended SPI (default) */
>  
> +/* Flag Status Register bits */
> +#define FSR_READY		BIT(7)	/* Device status, 0 = Busy, 1 = Ready */
> +#define FSR_E_ERR		BIT(5)	/* Erase operation status */
> +#define FSR_P_ERR		BIT(4)	/* Program operation status */
> +#define FSR_PT_ERR		BIT(1)	/* Protection error bit */
> +
>  static int spi_nor_micron_octal_dtr_enable(struct spi_nor *nor, bool enable)
>  {
>  	struct spi_mem_op op;
> @@ -273,12 +281,133 @@ static int st_micron_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
>  	return spi_nor_write_disable(nor);
>  }
>  
> +/**
> + * spi_nor_read_fsr() - Read the Flag Status Register.
> + * @nor:	pointer to 'struct spi_nor'
> + * @fsr:	pointer to a DMA-able buffer where the value of the
> + *              Flag Status Register will be written. Should be at least 2
> + *              bytes.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr)
> +{
> +	int ret;
> +
> +	if (nor->spimem) {
> +		struct spi_mem_op op =
> +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDFSR, 0),
> +				   SPI_MEM_OP_NO_ADDR,
> +				   SPI_MEM_OP_NO_DUMMY,
> +				   SPI_MEM_OP_DATA_IN(1, fsr, 0));
> +
> +		if (nor->reg_proto == SNOR_PROTO_8_8_8_DTR) {
> +			op.addr.nbytes = nor->params->rdsr_addr_nbytes;
> +			op.dummy.nbytes = nor->params->rdsr_dummy;
> +			/*
> +			 * We don't want to read only one byte in DTR mode. So,
> +			 * read 2 and then discard the second byte.
> +			 */
> +			op.data.nbytes = 2;
> +		}
> +
> +		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> +
> +		ret = spi_mem_exec_op(nor->spimem, &op);
> +	} else {
> +		ret = spi_nor_controller_ops_read_reg(nor, SPINOR_OP_RDFSR, fsr,
> +						      1);
> +	}
> +
> +	if (ret)
> +		dev_dbg(nor->dev, "error %d reading FSR\n", ret);
> +
> +	return ret;
> +}
> +
> +/**
> + * spi_nor_clear_fsr() - Clear the Flag Status Register.
> + * @nor:	pointer to 'struct spi_nor'.
> + */
> +static void spi_nor_clear_fsr(struct spi_nor *nor)
> +{
> +	int ret;
> +
> +	if (nor->spimem) {
> +		struct spi_mem_op op =
> +			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLFSR, 0),
> +				   SPI_MEM_OP_NO_ADDR,
> +				   SPI_MEM_OP_NO_DUMMY,
> +				   SPI_MEM_OP_NO_DATA);
> +
> +		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> +
> +		ret = spi_mem_exec_op(nor->spimem, &op);
> +	} else {
> +		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLFSR,
> +						       NULL, 0);
> +	}
> +
> +	if (ret)
> +		dev_dbg(nor->dev, "error %d clearing FSR\n", ret);
> +}
> +
> +/**
> + * spi_nor_fsr_ready() - Query the Flag Status Register to see if the flash is
> + * ready for new commands.
> + * @nor:	pointer to 'struct spi_nor'.
> + *
> + * Return: 1 if ready, 0 if not ready, -errno on errors.
> + */
> +static int spi_nor_fsr_ready(struct spi_nor *nor)

Nitpick: At this point this function is not just spi_nor_fsr_ready(). I 
think it should be renamed to something more accurate like 
micron_st_ready() (with whatever prefix scheme that was decided upon).

Looks good otherwise.

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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v1 08/14] mtd: spi-nor: micron-st: convert USE_FSR to a manufacturer flag

[Michael Walle]

Now that all functions using that flag are local to the micron module,
we can convert the flag to a manufacturer one.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/core.c      |  3 --
 drivers/mtd/spi-nor/core.h      |  3 --
 drivers/mtd/spi-nor/micron-st.c | 92 +++++++++++++++++++++------------
 3 files changed, 59 insertions(+), 39 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index e9d9880149d2..be65aaa954ca 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2499,9 +2499,6 @@ static void spi_nor_init_flags(struct spi_nor *nor)
 
 	if (flags & USE_CLSR)
 		nor->flags |= SNOR_F_USE_CLSR;
-
-	if (flags & USE_FSR)
-		nor->flags |= SNOR_F_USE_FSR;
 }
 
 /**
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index fabc01ae9a81..a02bf54289fb 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -12,7 +12,6 @@
 #define SPI_NOR_MAX_ID_LEN	6
 
 enum spi_nor_option_flags {
-	SNOR_F_USE_FSR		= BIT(0),
 	SNOR_F_HAS_SR_TB	= BIT(1),
 	SNOR_F_NO_OP_CHIP_ERASE	= BIT(2),
 	SNOR_F_USE_CLSR		= BIT(4),
@@ -349,7 +348,6 @@ struct spi_nor_fixups {
  *   NO_CHIP_ERASE:           chip does not support chip erase.
  *   SPI_NOR_NO_FR:           can't do fastread.
  *   USE_CLSR:                use CLSR command.
- *   USE_FSR:                 use flag status register
  *
  * @no_sfdp_flags:  flags that indicate support that can be discovered via SFDP.
  *                  Used when SFDP tables are not defined in the flash. These
@@ -401,7 +399,6 @@ struct flash_info {
 #define NO_CHIP_ERASE			BIT(7)
 #define SPI_NOR_NO_FR			BIT(8)
 #define USE_CLSR			BIT(9)
-#define USE_FSR				BIT(10)
 
 	u8 no_sfdp_flags;
 #define SPI_NOR_SKIP_SFDP		BIT(0)
diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
index c66580e8aa00..33531c101ccb 100644
--- a/drivers/mtd/spi-nor/micron-st.c
+++ b/drivers/mtd/spi-nor/micron-st.c
@@ -8,6 +8,8 @@
 
 #include "core.h"
 
+#define USE_FSR		BIT(0)
+
 #define SPINOR_OP_RDFSR		0x70	/* Read flag status register */
 #define SPINOR_OP_CLFSR		0x50	/* Clear flag status register */
 #define SPINOR_OP_MT_DTR_RD	0xfd	/* Fast Read opcode in DTR mode */
@@ -140,15 +142,17 @@ static const struct spi_nor_fixups mt35xu512aba_fixups = {
 
 static const struct flash_info micron_parts[] = {
 	{ "mt35xu512aba", INFO(0x2c5b1a, 0, 128 * 1024, 512)
-		FLAGS(USE_FSR)
 		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_OCTAL_READ |
 			   SPI_NOR_OCTAL_DTR_READ | SPI_NOR_OCTAL_DTR_PP)
 		FIXUP_FLAGS(SPI_NOR_4B_OPCODES | SPI_NOR_IO_MODE_EN_VOLATILE)
-		.fixups = &mt35xu512aba_fixups},
+		MFR_FLAGS(USE_FSR)
+		.fixups = &mt35xu512aba_fixups
+	},
 	{ "mt35xu02g", INFO(0x2c5b1c, 0, 128 * 1024, 2048)
-		FLAGS(USE_FSR)
 		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_OCTAL_READ)
-		FIXUP_FLAGS(SPI_NOR_4B_OPCODES) },
+		FIXUP_FLAGS(SPI_NOR_4B_OPCODES)
+		MFR_FLAGS(USE_FSR)
+	},
 };
 
 static const struct flash_info st_parts[] = {
@@ -164,57 +168,79 @@ static const struct flash_info st_parts[] = {
 		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ) },
 	{ "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256)
 		FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_4BIT_BP |
-		      SPI_NOR_BP3_SR_BIT6 | USE_FSR)
-		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ) },
+		      SPI_NOR_BP3_SR_BIT6)
+		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ)
+		MFR_FLAGS(USE_FSR)
+	},
 	{ "n25q128a13",  INFO(0x20ba18, 0, 64 * 1024,  256)
 		FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_4BIT_BP |
-		      SPI_NOR_BP3_SR_BIT6 | USE_FSR)
-		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ) },
+		      SPI_NOR_BP3_SR_BIT6)
+		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ)
+		MFR_FLAGS(USE_FSR)
+	},
 	{ "mt25ql256a",  INFO6(0x20ba19, 0x104400, 64 * 1024,  512)
-		FLAGS(USE_FSR)
 		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
-		FIXUP_FLAGS(SPI_NOR_4B_OPCODES) },
+		FIXUP_FLAGS(SPI_NOR_4B_OPCODES)
+		MFR_FLAGS(USE_FSR)
+	},
 	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512)
-		FLAGS(USE_FSR)
 		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ |
-			      SPI_NOR_QUAD_READ) },
+			      SPI_NOR_QUAD_READ)
+		MFR_FLAGS(USE_FSR)
+	},
 	{ "mt25qu256a",  INFO6(0x20bb19, 0x104400, 64 * 1024,  512)
-		FLAGS(USE_FSR)
 		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
-		FIXUP_FLAGS(SPI_NOR_4B_OPCODES) },
+		FIXUP_FLAGS(SPI_NOR_4B_OPCODES)
+		MFR_FLAGS(USE_FSR)
+	},
 	{ "n25q256ax1",  INFO(0x20bb19, 0, 64 * 1024,  512)
-		FLAGS(USE_FSR)
-		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ) },
+		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ)
+		MFR_FLAGS(USE_FSR)
+	},
 	{ "mt25ql512a",  INFO6(0x20ba20, 0x104400, 64 * 1024, 1024)
-		FLAGS(USE_FSR)
 		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
-		FIXUP_FLAGS(SPI_NOR_4B_OPCODES) },
+		FIXUP_FLAGS(SPI_NOR_4B_OPCODES)
+		MFR_FLAGS(USE_FSR)
+	},
 	{ "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024)
 		FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_4BIT_BP |
-		      SPI_NOR_BP3_SR_BIT6 | USE_FSR)
-		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ) },
+		      SPI_NOR_BP3_SR_BIT6)
+		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ)
+		MFR_FLAGS(USE_FSR)
+	},
 	{ "mt25qu512a",  INFO6(0x20bb20, 0x104400, 64 * 1024, 1024)
-		FLAGS(USE_FSR)
 		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
-		FIXUP_FLAGS(SPI_NOR_4B_OPCODES) },
+		FIXUP_FLAGS(SPI_NOR_4B_OPCODES)
+		MFR_FLAGS(USE_FSR)
+	},
 	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024)
 		FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_4BIT_BP |
-		      SPI_NOR_BP3_SR_BIT6 | USE_FSR)
-		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ) },
+		      SPI_NOR_BP3_SR_BIT6)
+		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ)
+		MFR_FLAGS(USE_FSR)
+	},
 	{ "n25q00",      INFO(0x20ba21, 0, 64 * 1024, 2048)
 		FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_4BIT_BP |
-		      SPI_NOR_BP3_SR_BIT6 | NO_CHIP_ERASE | USE_FSR)
-		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ) },
+		      SPI_NOR_BP3_SR_BIT6 | NO_CHIP_ERASE)
+		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ)
+		MFR_FLAGS(USE_FSR)
+	},
 	{ "n25q00a",     INFO(0x20bb21, 0, 64 * 1024, 2048)
-		FLAGS(NO_CHIP_ERASE | USE_FSR)
-		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ) },
+		FLAGS(NO_CHIP_ERASE)
+		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ)
+		MFR_FLAGS(USE_FSR)
+	},
 	{ "mt25ql02g",   INFO(0x20ba22, 0, 64 * 1024, 4096)
-		FLAGS(NO_CHIP_ERASE | USE_FSR)
-		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ) },
+		FLAGS(NO_CHIP_ERASE)
+		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ)
+		MFR_FLAGS(USE_FSR)
+	},
 	{ "mt25qu02g",   INFO(0x20bb22, 0, 64 * 1024, 4096)
-		FLAGS(NO_CHIP_ERASE | USE_FSR)
+		FLAGS(NO_CHIP_ERASE)
 		NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ |
-			      SPI_NOR_QUAD_READ) },
+			      SPI_NOR_QUAD_READ)
+		MFR_FLAGS(USE_FSR)
+	},
 
 	{ "m25p05",  INFO(0x202010,  0,  32 * 1024,   2) },
 	{ "m25p10",  INFO(0x202011,  0,  32 * 1024,   4) },
@@ -406,7 +432,7 @@ static void micron_st_default_init(struct spi_nor *nor)
 	nor->params->quad_enable = NULL;
 	nor->params->set_4byte_addr_mode = st_micron_set_4byte_addr_mode;
 
-	if (nor->flags & SNOR_F_USE_FSR)
+	if (nor->info->mfr_flags & USE_FSR)
 		nor->params->ready = spi_nor_fsr_ready;
 }
 
-- 
2.30.2

Re: [PATCH v1 08/14] mtd: spi-nor: micron-st: convert USE_FSR to a manufacturer flag

[Tudor.Ambarus]

On 2/2/22 16:58, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Now that all functions using that flag are local to the micron module,
> we can convert the flag to a manufacturer one.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/core.c      |  3 --
>  drivers/mtd/spi-nor/core.h      |  3 --
>  drivers/mtd/spi-nor/micron-st.c | 92 +++++++++++++++++++++------------
>  3 files changed, 59 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index e9d9880149d2..be65aaa954ca 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2499,9 +2499,6 @@ static void spi_nor_init_flags(struct spi_nor *nor)
> 
>         if (flags & USE_CLSR)
>                 nor->flags |= SNOR_F_USE_CLSR;
> -
> -       if (flags & USE_FSR)
> -               nor->flags |= SNOR_F_USE_FSR;
>  }
> 
>  /**
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index fabc01ae9a81..a02bf54289fb 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -12,7 +12,6 @@
>  #define SPI_NOR_MAX_ID_LEN     6
> 
>  enum spi_nor_option_flags {
> -       SNOR_F_USE_FSR          = BIT(0),
>         SNOR_F_HAS_SR_TB        = BIT(1),
>         SNOR_F_NO_OP_CHIP_ERASE = BIT(2),
>         SNOR_F_USE_CLSR         = BIT(4),
> @@ -349,7 +348,6 @@ struct spi_nor_fixups {
>   *   NO_CHIP_ERASE:           chip does not support chip erase.
>   *   SPI_NOR_NO_FR:           can't do fastread.
>   *   USE_CLSR:                use CLSR command.
> - *   USE_FSR:                 use flag status register
>   *
>   * @no_sfdp_flags:  flags that indicate support that can be discovered via SFDP.
>   *                  Used when SFDP tables are not defined in the flash. These
> @@ -401,7 +399,6 @@ struct flash_info {
>  #define NO_CHIP_ERASE                  BIT(7)
>  #define SPI_NOR_NO_FR                  BIT(8)
>  #define USE_CLSR                       BIT(9)
> -#define USE_FSR                                BIT(10)
> 
>         u8 no_sfdp_flags;
>  #define SPI_NOR_SKIP_SFDP              BIT(0)
> diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
> index c66580e8aa00..33531c101ccb 100644
> --- a/drivers/mtd/spi-nor/micron-st.c
> +++ b/drivers/mtd/spi-nor/micron-st.c
> @@ -8,6 +8,8 @@
> 
>  #include "core.h"
> 
> +#define USE_FSR                BIT(0)

please add a description and inform the reader that this is a manufacturer specific
flash_info flag.
Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

> +
>  #define SPINOR_OP_RDFSR                0x70    /* Read flag status register */
>  #define SPINOR_OP_CLFSR                0x50    /* Clear flag status register */
>  #define SPINOR_OP_MT_DTR_RD    0xfd    /* Fast Read opcode in DTR mode */
> @@ -140,15 +142,17 @@ static const struct spi_nor_fixups mt35xu512aba_fixups = {
> 
>  static const struct flash_info micron_parts[] = {
>         { "mt35xu512aba", INFO(0x2c5b1a, 0, 128 * 1024, 512)
> -               FLAGS(USE_FSR)
>                 NO_SFDP_FLAGS(SECT_4K | SPI_NOR_OCTAL_READ |
>                            SPI_NOR_OCTAL_DTR_READ | SPI_NOR_OCTAL_DTR_PP)
>                 FIXUP_FLAGS(SPI_NOR_4B_OPCODES | SPI_NOR_IO_MODE_EN_VOLATILE)
> -               .fixups = &mt35xu512aba_fixups},
> +               MFR_FLAGS(USE_FSR)
> +               .fixups = &mt35xu512aba_fixups
> +       },
>         { "mt35xu02g", INFO(0x2c5b1c, 0, 128 * 1024, 2048)
> -               FLAGS(USE_FSR)
>                 NO_SFDP_FLAGS(SECT_4K | SPI_NOR_OCTAL_READ)
> -               FIXUP_FLAGS(SPI_NOR_4B_OPCODES) },
> +               FIXUP_FLAGS(SPI_NOR_4B_OPCODES)
> +               MFR_FLAGS(USE_FSR)
> +       },
>  };
> 
>  static const struct flash_info st_parts[] = {
> @@ -164,57 +168,79 @@ static const struct flash_info st_parts[] = {
>                 NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ) },
>         { "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256)
>                 FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_4BIT_BP |
> -                     SPI_NOR_BP3_SR_BIT6 | USE_FSR)
> -               NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ) },
> +                     SPI_NOR_BP3_SR_BIT6)
> +               NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ)
> +               MFR_FLAGS(USE_FSR)
> +       },
>         { "n25q128a13",  INFO(0x20ba18, 0, 64 * 1024,  256)
>                 FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_4BIT_BP |
> -                     SPI_NOR_BP3_SR_BIT6 | USE_FSR)
> -               NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ) },
> +                     SPI_NOR_BP3_SR_BIT6)
> +               NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ)
> +               MFR_FLAGS(USE_FSR)
> +       },
>         { "mt25ql256a",  INFO6(0x20ba19, 0x104400, 64 * 1024,  512)
> -               FLAGS(USE_FSR)
>                 NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> -               FIXUP_FLAGS(SPI_NOR_4B_OPCODES) },
> +               FIXUP_FLAGS(SPI_NOR_4B_OPCODES)
> +               MFR_FLAGS(USE_FSR)
> +       },
>         { "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512)
> -               FLAGS(USE_FSR)
>                 NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ |
> -                             SPI_NOR_QUAD_READ) },
> +                             SPI_NOR_QUAD_READ)
> +               MFR_FLAGS(USE_FSR)
> +       },
>         { "mt25qu256a",  INFO6(0x20bb19, 0x104400, 64 * 1024,  512)
> -               FLAGS(USE_FSR)
>                 NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> -               FIXUP_FLAGS(SPI_NOR_4B_OPCODES) },
> +               FIXUP_FLAGS(SPI_NOR_4B_OPCODES)
> +               MFR_FLAGS(USE_FSR)
> +       },
>         { "n25q256ax1",  INFO(0x20bb19, 0, 64 * 1024,  512)
> -               FLAGS(USE_FSR)
> -               NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ) },
> +               NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ)
> +               MFR_FLAGS(USE_FSR)
> +       },
>         { "mt25ql512a",  INFO6(0x20ba20, 0x104400, 64 * 1024, 1024)
> -               FLAGS(USE_FSR)
>                 NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> -               FIXUP_FLAGS(SPI_NOR_4B_OPCODES) },
> +               FIXUP_FLAGS(SPI_NOR_4B_OPCODES)
> +               MFR_FLAGS(USE_FSR)
> +       },
>         { "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024)
>                 FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_4BIT_BP |
> -                     SPI_NOR_BP3_SR_BIT6 | USE_FSR)
> -               NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ) },
> +                     SPI_NOR_BP3_SR_BIT6)
> +               NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ)
> +               MFR_FLAGS(USE_FSR)
> +       },
>         { "mt25qu512a",  INFO6(0x20bb20, 0x104400, 64 * 1024, 1024)
> -               FLAGS(USE_FSR)
>                 NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> -               FIXUP_FLAGS(SPI_NOR_4B_OPCODES) },
> +               FIXUP_FLAGS(SPI_NOR_4B_OPCODES)
> +               MFR_FLAGS(USE_FSR)
> +       },
>         { "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024)
>                 FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_4BIT_BP |
> -                     SPI_NOR_BP3_SR_BIT6 | USE_FSR)
> -               NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ) },
> +                     SPI_NOR_BP3_SR_BIT6)
> +               NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ)
> +               MFR_FLAGS(USE_FSR)
> +       },
>         { "n25q00",      INFO(0x20ba21, 0, 64 * 1024, 2048)
>                 FLAGS(SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB | SPI_NOR_4BIT_BP |
> -                     SPI_NOR_BP3_SR_BIT6 | NO_CHIP_ERASE | USE_FSR)
> -               NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ) },
> +                     SPI_NOR_BP3_SR_BIT6 | NO_CHIP_ERASE)
> +               NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ)
> +               MFR_FLAGS(USE_FSR)
> +       },
>         { "n25q00a",     INFO(0x20bb21, 0, 64 * 1024, 2048)
> -               FLAGS(NO_CHIP_ERASE | USE_FSR)
> -               NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ) },
> +               FLAGS(NO_CHIP_ERASE)
> +               NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ)
> +               MFR_FLAGS(USE_FSR)
> +       },
>         { "mt25ql02g",   INFO(0x20ba22, 0, 64 * 1024, 4096)
> -               FLAGS(NO_CHIP_ERASE | USE_FSR)
> -               NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ) },
> +               FLAGS(NO_CHIP_ERASE)
> +               NO_SFDP_FLAGS(SECT_4K | SPI_NOR_QUAD_READ)
> +               MFR_FLAGS(USE_FSR)
> +       },
>         { "mt25qu02g",   INFO(0x20bb22, 0, 64 * 1024, 4096)
> -               FLAGS(NO_CHIP_ERASE | USE_FSR)
> +               FLAGS(NO_CHIP_ERASE)
>                 NO_SFDP_FLAGS(SECT_4K | SPI_NOR_DUAL_READ |
> -                             SPI_NOR_QUAD_READ) },
> +                             SPI_NOR_QUAD_READ)
> +               MFR_FLAGS(USE_FSR)
> +       },
> 
>         { "m25p05",  INFO(0x202010,  0,  32 * 1024,   2) },
>         { "m25p10",  INFO(0x202011,  0,  32 * 1024,   4) },
> @@ -406,7 +432,7 @@ static void micron_st_default_init(struct spi_nor *nor)
>         nor->params->quad_enable = NULL;
>         nor->params->set_4byte_addr_mode = st_micron_set_4byte_addr_mode;
> 
> -       if (nor->flags & SNOR_F_USE_FSR)
> +       if (nor->info->mfr_flags & USE_FSR)
>                 nor->params->ready = spi_nor_fsr_ready;
>  }
> 
> --
> 2.30.2
> 

Re: [PATCH v1 08/14] mtd: spi-nor: micron-st: convert USE_FSR to a manufacturer flag

[Pratyush Yadav]

On 10/02/22 03:37AM, Tudor.Ambarus@microchip.com wrote:
> On 2/2/22 16:58, Michael Walle wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > Now that all functions using that flag are local to the micron module,
> > we can convert the flag to a manufacturer one.
> > 
> > Signed-off-by: Michael Walle <michael@walle.cc>
> > ---
> >  drivers/mtd/spi-nor/core.c      |  3 --
> >  drivers/mtd/spi-nor/core.h      |  3 --
> >  drivers/mtd/spi-nor/micron-st.c | 92 +++++++++++++++++++++------------
> >  3 files changed, 59 insertions(+), 39 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index e9d9880149d2..be65aaa954ca 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -2499,9 +2499,6 @@ static void spi_nor_init_flags(struct spi_nor *nor)
> > 
> >         if (flags & USE_CLSR)
> >                 nor->flags |= SNOR_F_USE_CLSR;
> > -
> > -       if (flags & USE_FSR)
> > -               nor->flags |= SNOR_F_USE_FSR;
> >  }
> > 
> >  /**
> > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> > index fabc01ae9a81..a02bf54289fb 100644
> > --- a/drivers/mtd/spi-nor/core.h
> > +++ b/drivers/mtd/spi-nor/core.h
> > @@ -12,7 +12,6 @@
> >  #define SPI_NOR_MAX_ID_LEN     6
> > 
> >  enum spi_nor_option_flags {
> > -       SNOR_F_USE_FSR          = BIT(0),
> >         SNOR_F_HAS_SR_TB        = BIT(1),
> >         SNOR_F_NO_OP_CHIP_ERASE = BIT(2),
> >         SNOR_F_USE_CLSR         = BIT(4),
> > @@ -349,7 +348,6 @@ struct spi_nor_fixups {
> >   *   NO_CHIP_ERASE:           chip does not support chip erase.
> >   *   SPI_NOR_NO_FR:           can't do fastread.
> >   *   USE_CLSR:                use CLSR command.
> > - *   USE_FSR:                 use flag status register
> >   *
> >   * @no_sfdp_flags:  flags that indicate support that can be discovered via SFDP.
> >   *                  Used when SFDP tables are not defined in the flash. These
> > @@ -401,7 +399,6 @@ struct flash_info {
> >  #define NO_CHIP_ERASE                  BIT(7)
> >  #define SPI_NOR_NO_FR                  BIT(8)
> >  #define USE_CLSR                       BIT(9)
> > -#define USE_FSR                                BIT(10)
> > 
> >         u8 no_sfdp_flags;
> >  #define SPI_NOR_SKIP_SFDP              BIT(0)
> > diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
> > index c66580e8aa00..33531c101ccb 100644
> > --- a/drivers/mtd/spi-nor/micron-st.c
> > +++ b/drivers/mtd/spi-nor/micron-st.c
> > @@ -8,6 +8,8 @@
> > 
> >  #include "core.h"
> > 
> > +#define USE_FSR                BIT(0)
> 
> please add a description and inform the reader that this is a manufacturer specific
> flash_info flag.

+1

> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

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


-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v1 09/14] mtd: spi-nor: micron-st: fix micron_st prefix

[Michael Walle]

The usual prefix is micron_st, so use that instead of st_micron.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/micron-st.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
index 33531c101ccb..ca368b48bcb0 100644
--- a/drivers/mtd/spi-nor/micron-st.c
+++ b/drivers/mtd/spi-nor/micron-st.c
@@ -284,7 +284,7 @@ static const struct flash_info st_parts[] = {
 };
 
 /**
- * st_micron_set_4byte_addr_mode() - Set 4-byte address mode for ST and Micron
+ * micron_st_set_4byte_addr_mode() - Set 4-byte address mode for ST and Micron
  * flashes.
  * @nor:	pointer to 'struct spi_nor'.
  * @enable:	true to enter the 4-byte address mode, false to exit the 4-byte
@@ -292,7 +292,7 @@ static const struct flash_info st_parts[] = {
  *
  * Return: 0 on success, -errno otherwise.
  */
-static int st_micron_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
+static int micron_st_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
 {
 	int ret;
 
@@ -430,7 +430,7 @@ static void micron_st_default_init(struct spi_nor *nor)
 	nor->flags |= SNOR_F_HAS_LOCK;
 	nor->flags &= ~SNOR_F_HAS_16BIT_SR;
 	nor->params->quad_enable = NULL;
-	nor->params->set_4byte_addr_mode = st_micron_set_4byte_addr_mode;
+	nor->params->set_4byte_addr_mode = micron_st_set_4byte_addr_mode;
 
 	if (nor->info->mfr_flags & USE_FSR)
 		nor->params->ready = spi_nor_fsr_ready;
-- 
2.30.2

Re: [PATCH v1 09/14] mtd: spi-nor: micron-st: fix micron_st prefix

[Tudor.Ambarus]

On 2/2/22 16:58, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> The usual prefix is micron_st, so use that instead of st_micron.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

> ---
>  drivers/mtd/spi-nor/micron-st.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
> index 33531c101ccb..ca368b48bcb0 100644
> --- a/drivers/mtd/spi-nor/micron-st.c
> +++ b/drivers/mtd/spi-nor/micron-st.c
> @@ -284,7 +284,7 @@ static const struct flash_info st_parts[] = {
>  };
> 
>  /**
> - * st_micron_set_4byte_addr_mode() - Set 4-byte address mode for ST and Micron
> + * micron_st_set_4byte_addr_mode() - Set 4-byte address mode for ST and Micron
>   * flashes.
>   * @nor:       pointer to 'struct spi_nor'.
>   * @enable:    true to enter the 4-byte address mode, false to exit the 4-byte
> @@ -292,7 +292,7 @@ static const struct flash_info st_parts[] = {
>   *
>   * Return: 0 on success, -errno otherwise.
>   */
> -static int st_micron_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
> +static int micron_st_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
>  {
>         int ret;
> 
> @@ -430,7 +430,7 @@ static void micron_st_default_init(struct spi_nor *nor)
>         nor->flags |= SNOR_F_HAS_LOCK;
>         nor->flags &= ~SNOR_F_HAS_16BIT_SR;
>         nor->params->quad_enable = NULL;
> -       nor->params->set_4byte_addr_mode = st_micron_set_4byte_addr_mode;
> +       nor->params->set_4byte_addr_mode = micron_st_set_4byte_addr_mode;
> 
>         if (nor->info->mfr_flags & USE_FSR)
>                 nor->params->ready = spi_nor_fsr_ready;
> --
> 2.30.2
> 

Re: [PATCH v1 09/14] mtd: spi-nor: micron-st: fix micron_st prefix

[Pratyush Yadav]

On 02/02/22 03:58PM, Michael Walle wrote:
> The usual prefix is micron_st, so use that instead of st_micron.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v1 10/14] mtd: spi-nor: micron-st: rename vendor specific functions and defines

[Michael Walle]

Drop the generic spi_nor prefix for all the micron-st functions.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/micron-st.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
index ca368b48bcb0..988ecbffdc36 100644
--- a/drivers/mtd/spi-nor/micron-st.c
+++ b/drivers/mtd/spi-nor/micron-st.c
@@ -308,7 +308,7 @@ static int micron_st_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
 }
 
 /**
- * spi_nor_read_fsr() - Read the Flag Status Register.
+ * micron_st_read_fsr() - Read the Flag Status Register.
  * @nor:	pointer to 'struct spi_nor'
  * @fsr:	pointer to a DMA-able buffer where the value of the
  *              Flag Status Register will be written. Should be at least 2
@@ -316,7 +316,7 @@ static int micron_st_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
  *
  * Return: 0 on success, -errno otherwise.
  */
-static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr)
+static int micron_st_read_fsr(struct spi_nor *nor, u8 *fsr)
 {
 	int ret;
 
@@ -352,10 +352,10 @@ static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr)
 }
 
 /**
- * spi_nor_clear_fsr() - Clear the Flag Status Register.
+ * micron_st_clear_fsr() - Clear the Flag Status Register.
  * @nor:	pointer to 'struct spi_nor'.
  */
-static void spi_nor_clear_fsr(struct spi_nor *nor)
+static void micron_st_clear_fsr(struct spi_nor *nor)
 {
 	int ret;
 
@@ -379,13 +379,13 @@ static void spi_nor_clear_fsr(struct spi_nor *nor)
 }
 
 /**
- * spi_nor_fsr_ready() - Query the Flag Status Register to see if the flash is
+ * micron_st_fsr_ready() - Query the Flag Status Register to see if the flash is
  * ready for new commands.
  * @nor:	pointer to 'struct spi_nor'.
  *
  * Return: 1 if ready, 0 if not ready, -errno on errors.
  */
-static int spi_nor_fsr_ready(struct spi_nor *nor)
+static int micron_st_fsr_ready(struct spi_nor *nor)
 {
 	int sr_ready, ret;
 
@@ -393,7 +393,7 @@ static int spi_nor_fsr_ready(struct spi_nor *nor)
 	if (sr_ready < 0)
 		return sr_ready;
 
-	ret = spi_nor_read_fsr(nor, nor->bouncebuf);
+	ret = micron_st_read_fsr(nor, nor->bouncebuf);
 	if (ret)
 		return ret;
 
@@ -407,7 +407,7 @@ static int spi_nor_fsr_ready(struct spi_nor *nor)
 			dev_err(nor->dev,
 			"Attempted to modify a protected sector.\n");
 
-		spi_nor_clear_fsr(nor);
+		micron_st_clear_fsr(nor);
 
 		/*
 		 * WEL bit remains set to one when an erase or page program
@@ -433,7 +433,7 @@ static void micron_st_default_init(struct spi_nor *nor)
 	nor->params->set_4byte_addr_mode = micron_st_set_4byte_addr_mode;
 
 	if (nor->info->mfr_flags & USE_FSR)
-		nor->params->ready = spi_nor_fsr_ready;
+		nor->params->ready = micron_st_fsr_ready;
 }
 
 static const struct spi_nor_fixups micron_st_fixups = {
-- 
2.30.2

Re: [PATCH v1 10/14] mtd: spi-nor: micron-st: rename vendor specific functions and defines

[Tudor.Ambarus]

On 2/2/22 16:58, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Drop the generic spi_nor prefix for all the micron-st functions.

please, no.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/micron-st.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
> index ca368b48bcb0..988ecbffdc36 100644
> --- a/drivers/mtd/spi-nor/micron-st.c
> +++ b/drivers/mtd/spi-nor/micron-st.c
> @@ -308,7 +308,7 @@ static int micron_st_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
>  }
> 
>  /**
> - * spi_nor_read_fsr() - Read the Flag Status Register.
> + * micron_st_read_fsr() - Read the Flag Status Register.
>   * @nor:       pointer to 'struct spi_nor'
>   * @fsr:       pointer to a DMA-able buffer where the value of the
>   *              Flag Status Register will be written. Should be at least 2
> @@ -316,7 +316,7 @@ static int micron_st_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
>   *
>   * Return: 0 on success, -errno otherwise.
>   */
> -static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr)
> +static int micron_st_read_fsr(struct spi_nor *nor, u8 *fsr)
>  {
>         int ret;
> 
> @@ -352,10 +352,10 @@ static int spi_nor_read_fsr(struct spi_nor *nor, u8 *fsr)
>  }
> 
>  /**
> - * spi_nor_clear_fsr() - Clear the Flag Status Register.
> + * micron_st_clear_fsr() - Clear the Flag Status Register.
>   * @nor:       pointer to 'struct spi_nor'.
>   */
> -static void spi_nor_clear_fsr(struct spi_nor *nor)
> +static void micron_st_clear_fsr(struct spi_nor *nor)
>  {
>         int ret;
> 
> @@ -379,13 +379,13 @@ static void spi_nor_clear_fsr(struct spi_nor *nor)
>  }
> 
>  /**
> - * spi_nor_fsr_ready() - Query the Flag Status Register to see if the flash is
> + * micron_st_fsr_ready() - Query the Flag Status Register to see if the flash is
>   * ready for new commands.
>   * @nor:       pointer to 'struct spi_nor'.
>   *
>   * Return: 1 if ready, 0 if not ready, -errno on errors.
>   */
> -static int spi_nor_fsr_ready(struct spi_nor *nor)
> +static int micron_st_fsr_ready(struct spi_nor *nor)
>  {
>         int sr_ready, ret;
> 
> @@ -393,7 +393,7 @@ static int spi_nor_fsr_ready(struct spi_nor *nor)
>         if (sr_ready < 0)
>                 return sr_ready;
> 
> -       ret = spi_nor_read_fsr(nor, nor->bouncebuf);
> +       ret = micron_st_read_fsr(nor, nor->bouncebuf);
>         if (ret)
>                 return ret;
> 
> @@ -407,7 +407,7 @@ static int spi_nor_fsr_ready(struct spi_nor *nor)
>                         dev_err(nor->dev,
>                         "Attempted to modify a protected sector.\n");
> 
> -               spi_nor_clear_fsr(nor);
> +               micron_st_clear_fsr(nor);
> 
>                 /*
>                  * WEL bit remains set to one when an erase or page program
> @@ -433,7 +433,7 @@ static void micron_st_default_init(struct spi_nor *nor)
>         nor->params->set_4byte_addr_mode = micron_st_set_4byte_addr_mode;
> 
>         if (nor->info->mfr_flags & USE_FSR)
> -               nor->params->ready = spi_nor_fsr_ready;
> +               nor->params->ready = micron_st_fsr_ready;
>  }
> 
>  static const struct spi_nor_fixups micron_st_fixups = {
> --
> 2.30.2
> 

[PATCH v1 11/14] mtd: spi-nor: spansion: slightly rework control flow in late_init()

[Michael Walle]

Increase readability of the code. Instead of returning early if the
flash size is smaller or equal than 16MiB and then do the fixups for
larger flashes, do it within the condition.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/spansion.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index 534196b1d3e7..dedc2de90cb8 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -296,13 +296,12 @@ static const struct flash_info spansion_parts[] = {
 
 static void spansion_late_init(struct spi_nor *nor)
 {
-	if (nor->params->size <= SZ_16M)
-		return;
-
-	nor->flags |= SNOR_F_4B_OPCODES;
-	/* No small sector erase for 4-byte command set */
-	nor->erase_opcode = SPINOR_OP_SE;
-	nor->mtd.erasesize = nor->info->sector_size;
+	if (nor->params->size > SZ_16M) {
+		nor->flags |= SNOR_F_4B_OPCODES;
+		/* No small sector erase for 4-byte command set */
+		nor->erase_opcode = SPINOR_OP_SE;
+		nor->mtd.erasesize = nor->info->sector_size;
+	}
 }
 
 static const struct spi_nor_fixups spansion_fixups = {
-- 
2.30.2

Re: [PATCH v1 11/14] mtd: spi-nor: spansion: slightly rework control flow in late_init()

[Tudor.Ambarus]

On 2/2/22 16:58, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Increase readability of the code. Instead of returning early if the
> flash size is smaller or equal than 16MiB and then do the fixups for
> larger flashes, do it within the condition.
> 

mm, no, I'm not sure this improves readability, I see the two equivalent.
The original version has the benefit of no indentation. Pratyush?

> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/spansion.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index 534196b1d3e7..dedc2de90cb8 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -296,13 +296,12 @@ static const struct flash_info spansion_parts[] = {
> 
>  static void spansion_late_init(struct spi_nor *nor)
>  {
> -       if (nor->params->size <= SZ_16M)
> -               return;
> -
> -       nor->flags |= SNOR_F_4B_OPCODES;
> -       /* No small sector erase for 4-byte command set */
> -       nor->erase_opcode = SPINOR_OP_SE;
> -       nor->mtd.erasesize = nor->info->sector_size;
> +       if (nor->params->size > SZ_16M) {
> +               nor->flags |= SNOR_F_4B_OPCODES;
> +               /* No small sector erase for 4-byte command set */
> +               nor->erase_opcode = SPINOR_OP_SE;
> +               nor->mtd.erasesize = nor->info->sector_size;
> +       }
>  }
> 
>  static const struct spi_nor_fixups spansion_fixups = {
> --
> 2.30.2
> 

Re: [PATCH v1 11/14] mtd: spi-nor: spansion: slightly rework control flow in late_init()

[Michael Walle]

Am 2022-02-10 04:26, schrieb Tudor.Ambarus@microchip.com:
> On 2/2/22 16:58, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>> Increase readability of the code. Instead of returning early if the
>> flash size is smaller or equal than 16MiB and then do the fixups for
>> larger flashes, do it within the condition.
>> 
> 
> mm, no, I'm not sure this improves readability, I see the two 
> equivalent.
> The original version has the benefit of no indentation. Pratyush?

This is a preparation patch for 12/14, where the current version isn't
working anyway. If that is not enough reason why this is bad IMHO, I'll
give you two more.

I'd agree with you if that function was called
spansion_late_init_smaller_flashes() or something like that. But it is
a generic function valid for all flashes. And if you read it you might
get the impression there are only flashes smaller or equal than 16MiB.
You have to look twice to notice it was the intention that the
assignment afterwards are just for the smaller flashes (and you will
need to notice that there aren't any assignments for all spansion
flashes). There is no direct connection between the assignment and
the condition. Whereas with
   if (condition) {
     some_action();
   }
It is clear that some_action() was intended to only execute if
condition is true.

Also - and that is worse IMHO - it might easily be missed as someone
just add stuff to the end of the function which might goes unnoticed
but it won't work for flashes >16MiB.

-michael

Re: [PATCH v1 11/14] mtd: spi-nor: spansion: slightly rework control flow in late_init()

[Tudor.Ambarus]

On 2/10/22 10:16, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-02-10 04:26, schrieb Tudor.Ambarus@microchip.com:
>> On 2/2/22 16:58, Michael Walle wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know
>>> the content is safe
>>>
>>> Increase readability of the code. Instead of returning early if the
>>> flash size is smaller or equal than 16MiB and then do the fixups for
>>> larger flashes, do it within the condition.
>>>
>>
>> mm, no, I'm not sure this improves readability, I see the two
>> equivalent.
>> The original version has the benefit of no indentation. Pratyush?
> 
> This is a preparation patch for 12/14, where the current version isn't
> working anyway. If that is not enough reason why this is bad IMHO, I'll
> give you two more.

you can put the 
+       if (nor->flags & SNOR_F_USE_CLSR)
+               nor->params->ready = spi_nor_sr_ready_and_clear;

above the size check and get rid of the prerequisite requirement, no?
But it will look ugly indeed.

If these two are so tightly related, how about squashing them?

> 
> I'd agree with you if that function was called
> spansion_late_init_smaller_flashes() or something like that. But it is
> a generic function valid for all flashes. And if you read it you might
> get the impression there are only flashes smaller or equal than 16MiB.
> You have to look twice to notice it was the intention that the
> assignment afterwards are just for the smaller flashes (and you will
> need to notice that there aren't any assignments for all spansion
> flashes). There is no direct connection between the assignment and
> the condition. Whereas with
>   if (condition) {
>     some_action();
>   }
> It is clear that some_action() was intended to only execute if
> condition is true.
> 
> Also - and that is worse IMHO - it might easily be missed as someone
> just add stuff to the end of the function which might goes unnoticed
> but it won't work for flashes >16MiB.
> 

You definitely care about it if you wrote such a long email :). I find
the first argument the strongest, these two are biased IMO. I'm waiting
for v2 with this change included! :)

Cheers,
ta

Re: [PATCH v1 11/14] mtd: spi-nor: spansion: slightly rework control flow in late_init()

[Pratyush Yadav]

On 10/02/22 03:26AM, Tudor.Ambarus@microchip.com wrote:
> On 2/2/22 16:58, Michael Walle wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > Increase readability of the code. Instead of returning early if the
> > flash size is smaller or equal than 16MiB and then do the fixups for
> > larger flashes, do it within the condition.
> > 
> 
> mm, no, I'm not sure this improves readability, I see the two equivalent.
> The original version has the benefit of no indentation. Pratyush?

I am fine with both to be honest. But Michael's reasoning does make some 
sense to me. So,

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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v1 12/14] mtd: spi-nor: move all spansion specifics into spansion.c

[Michael Walle]

The clear status register flags is only available on spansion flashes.
Move all the functions around that into the spanion module.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/core.c     | 52 +------------------------
 drivers/mtd/spi-nor/spansion.c | 70 ++++++++++++++++++++++++++++++++++
 include/linux/mtd/spi-nor.h    |  1 -
 3 files changed, 72 insertions(+), 51 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index be65aaa954ca..5b00dfab77a6 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -554,33 +554,6 @@ int spi_nor_write_ear(struct spi_nor *nor, u8 ear)
 	return ret;
 }
 
-/**
- * spi_nor_clear_sr() - Clear the Status Register.
- * @nor:	pointer to 'struct spi_nor'.
- */
-static void spi_nor_clear_sr(struct spi_nor *nor)
-{
-	int ret;
-
-	if (nor->spimem) {
-		struct spi_mem_op op =
-			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLSR, 0),
-				   SPI_MEM_OP_NO_ADDR,
-				   SPI_MEM_OP_NO_DUMMY,
-				   SPI_MEM_OP_NO_DATA);
-
-		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
-
-		ret = spi_mem_exec_op(nor->spimem, &op);
-	} else {
-		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLSR,
-						       NULL, 0);
-	}
-
-	if (ret)
-		dev_dbg(nor->dev, "error %d clearing SR\n", ret);
-}
-
 /**
  * spi_nor_sr_ready() - Query the Status Register to see if the flash is ready
  * for new commands.
@@ -590,33 +563,12 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
  */
 int spi_nor_sr_ready(struct spi_nor *nor)
 {
-	int ret = spi_nor_read_sr(nor, nor->bouncebuf);
+	int ret;
 
+	ret = spi_nor_read_sr(nor, nor->bouncebuf);
 	if (ret)
 		return ret;
 
-	if (nor->flags & SNOR_F_USE_CLSR &&
-	    nor->bouncebuf[0] & (SR_E_ERR | SR_P_ERR)) {
-		if (nor->bouncebuf[0] & SR_E_ERR)
-			dev_err(nor->dev, "Erase Error occurred\n");
-		else
-			dev_err(nor->dev, "Programming Error occurred\n");
-
-		spi_nor_clear_sr(nor);
-
-		/*
-		 * WEL bit remains set to one when an erase or page program
-		 * error occurs. Issue a Write Disable command to protect
-		 * against inadvertent writes that can possibly corrupt the
-		 * contents of the memory.
-		 */
-		ret = spi_nor_write_disable(nor);
-		if (ret)
-			return ret;
-
-		return -EIO;
-	}
-
 	return !(nor->bouncebuf[0] & SR_WIP);
 }
 
diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index dedc2de90cb8..4756fb88eab2 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -8,6 +8,7 @@
 
 #include "core.h"
 
+#define SPINOR_OP_CLSR		0x30	/* Clear status register 1 */
 #define SPINOR_OP_RD_ANY_REG			0x65	/* Read any register */
 #define SPINOR_OP_WR_ANY_REG			0x71	/* Write any register */
 #define SPINOR_REG_CYPRESS_CFR2V		0x00800003
@@ -294,6 +295,72 @@ static const struct flash_info spansion_parts[] = {
 	},
 };
 
+/**
+ * spi_nor_clear_sr() - Clear the Status Register.
+ * @nor:	pointer to 'struct spi_nor'.
+ */
+static void spi_nor_clear_sr(struct spi_nor *nor)
+{
+	int ret;
+
+	if (nor->spimem) {
+		struct spi_mem_op op =
+			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLSR, 0),
+				   SPI_MEM_OP_NO_ADDR,
+				   SPI_MEM_OP_NO_DUMMY,
+				   SPI_MEM_OP_NO_DATA);
+
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
+		ret = spi_mem_exec_op(nor->spimem, &op);
+	} else {
+		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLSR,
+						       NULL, 0);
+	}
+
+	if (ret)
+		dev_dbg(nor->dev, "error %d clearing SR\n", ret);
+}
+
+/**
+ * spi_nor_sr_ready_and_clear() - Query the Status Register to see if the flash
+ * is ready for new commands and clear it.
+ * @nor:	pointer to 'struct spi_nor'.
+ *
+ * Return: 1 if ready, 0 if not ready, -errno on errors.
+ */
+int spi_nor_sr_ready_and_clear(struct spi_nor *nor)
+{
+	int ret;
+
+	ret = spi_nor_read_sr(nor, nor->bouncebuf);
+	if (ret)
+		return ret;
+
+	if (nor->bouncebuf[0] & (SR_E_ERR | SR_P_ERR)) {
+		if (nor->bouncebuf[0] & SR_E_ERR)
+			dev_err(nor->dev, "Erase Error occurred\n");
+		else
+			dev_err(nor->dev, "Programming Error occurred\n");
+
+		spi_nor_clear_sr(nor);
+
+		/*
+		 * WEL bit remains set to one when an erase or page program
+		 * error occurs. Issue a Write Disable command to protect
+		 * against inadvertent writes that can possibly corrupt the
+		 * contents of the memory.
+		 */
+		ret = spi_nor_write_disable(nor);
+		if (ret)
+			return ret;
+
+		return -EIO;
+	}
+
+	return !(nor->bouncebuf[0] & SR_WIP);
+}
+
 static void spansion_late_init(struct spi_nor *nor)
 {
 	if (nor->params->size > SZ_16M) {
@@ -302,6 +369,9 @@ static void spansion_late_init(struct spi_nor *nor)
 		nor->erase_opcode = SPINOR_OP_SE;
 		nor->mtd.erasesize = nor->info->sector_size;
 	}
+
+	if (nor->flags & SNOR_F_USE_CLSR)
+		nor->params->ready = spi_nor_sr_ready_and_clear;
 }
 
 static const struct spi_nor_fixups spansion_fixups = {
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 4622251a79ff..5e25a7b75ae2 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -90,7 +90,6 @@
 
 /* Used for Spansion flashes only. */
 #define SPINOR_OP_BRWR		0x17	/* Bank register write */
-#define SPINOR_OP_CLSR		0x30	/* Clear status register 1 */
 
 /* Used for Micron flashes only. */
 #define SPINOR_OP_RD_EVCR      0x65    /* Read EVCR register */
-- 
2.30.2

Re: [PATCH v1 12/14] mtd: spi-nor: move all spansion specifics into spansion.c

[kernel test robot]

Hi Michael,

I love your patch! Perhaps something to improve:

[auto build test WARNING on mtd/spi-nor/next]
[also build test WARNING on linux/master linus/master v5.17-rc2 next-20220202]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Michael-Walle/mtd-spi-nor-move-vendor-specific-code-into-vendor-modules/20220202-230139
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git spi-nor/next
config: alpha-allmodconfig (https://download.01.org/0day-ci/archive/20220203/202202030228.8WUETAc8-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/01f9808a20c96553c43f929e10f3fb448cd8bd93
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Michael-Walle/mtd-spi-nor-move-vendor-specific-code-into-vendor-modules/20220202-230139
        git checkout 01f9808a20c96553c43f929e10f3fb448cd8bd93
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=alpha SHELL=/bin/bash drivers/mtd/spi-nor/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/mtd/spi-nor/spansion.c:332:5: warning: no previous prototype for 'spi_nor_sr_ready_and_clear' [-Wmissing-prototypes]
     332 | int spi_nor_sr_ready_and_clear(struct spi_nor *nor)
         |     ^~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/spi_nor_sr_ready_and_clear +332 drivers/mtd/spi-nor/spansion.c

   324	
   325	/**
   326	 * spi_nor_sr_ready_and_clear() - Query the Status Register to see if the flash
   327	 * is ready for new commands and clear it.
   328	 * @nor:	pointer to 'struct spi_nor'.
   329	 *
   330	 * Return: 1 if ready, 0 if not ready, -errno on errors.
   331	 */
 > 332	int spi_nor_sr_ready_and_clear(struct spi_nor *nor)
   333	{
   334		int ret;
   335	
   336		ret = spi_nor_read_sr(nor, nor->bouncebuf);
   337		if (ret)
   338			return ret;
   339	
   340		if (nor->bouncebuf[0] & (SR_E_ERR | SR_P_ERR)) {
   341			if (nor->bouncebuf[0] & SR_E_ERR)
   342				dev_err(nor->dev, "Erase Error occurred\n");
   343			else
   344				dev_err(nor->dev, "Programming Error occurred\n");
   345	
   346			spi_nor_clear_sr(nor);
   347	
   348			/*
   349			 * WEL bit remains set to one when an erase or page program
   350			 * error occurs. Issue a Write Disable command to protect
   351			 * against inadvertent writes that can possibly corrupt the
   352			 * contents of the memory.
   353			 */
   354			ret = spi_nor_write_disable(nor);
   355			if (ret)
   356				return ret;
   357	
   358			return -EIO;
   359		}
   360	
   361		return !(nor->bouncebuf[0] & SR_WIP);
   362	}
   363	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH v1 12/14] mtd: spi-nor: move all spansion specifics into spansion.c

[kernel test robot]

Hi Michael,

I love your patch! Perhaps something to improve:

[auto build test WARNING on mtd/spi-nor/next]
[also build test WARNING on linux/master linus/master v5.17-rc2 next-20220202]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Michael-Walle/mtd-spi-nor-move-vendor-specific-code-into-vendor-modules/20220202-230139
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git spi-nor/next
config: x86_64-randconfig-a001 (https://download.01.org/0day-ci/archive/20220203/202202030449.IQfj2Y5O-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 6b1e844b69f15bb7dffaf9365cd2b355d2eb7579)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/01f9808a20c96553c43f929e10f3fb448cd8bd93
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Michael-Walle/mtd-spi-nor-move-vendor-specific-code-into-vendor-modules/20220202-230139
        git checkout 01f9808a20c96553c43f929e10f3fb448cd8bd93
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/mtd/spi-nor/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/mtd/spi-nor/spansion.c:332:5: warning: no previous prototype for function 'spi_nor_sr_ready_and_clear' [-Wmissing-prototypes]
   int spi_nor_sr_ready_and_clear(struct spi_nor *nor)
       ^
   drivers/mtd/spi-nor/spansion.c:332:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int spi_nor_sr_ready_and_clear(struct spi_nor *nor)
   ^
   static 
   1 warning generated.


vim +/spi_nor_sr_ready_and_clear +332 drivers/mtd/spi-nor/spansion.c

   324	
   325	/**
   326	 * spi_nor_sr_ready_and_clear() - Query the Status Register to see if the flash
   327	 * is ready for new commands and clear it.
   328	 * @nor:	pointer to 'struct spi_nor'.
   329	 *
   330	 * Return: 1 if ready, 0 if not ready, -errno on errors.
   331	 */
 > 332	int spi_nor_sr_ready_and_clear(struct spi_nor *nor)
   333	{
   334		int ret;
   335	
   336		ret = spi_nor_read_sr(nor, nor->bouncebuf);
   337		if (ret)
   338			return ret;
   339	
   340		if (nor->bouncebuf[0] & (SR_E_ERR | SR_P_ERR)) {
   341			if (nor->bouncebuf[0] & SR_E_ERR)
   342				dev_err(nor->dev, "Erase Error occurred\n");
   343			else
   344				dev_err(nor->dev, "Programming Error occurred\n");
   345	
   346			spi_nor_clear_sr(nor);
   347	
   348			/*
   349			 * WEL bit remains set to one when an erase or page program
   350			 * error occurs. Issue a Write Disable command to protect
   351			 * against inadvertent writes that can possibly corrupt the
   352			 * contents of the memory.
   353			 */
   354			ret = spi_nor_write_disable(nor);
   355			if (ret)
   356				return ret;
   357	
   358			return -EIO;
   359		}
   360	
   361		return !(nor->bouncebuf[0] & SR_WIP);
   362	}
   363	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[RFC PATCH] mtd: spi-nor: spi_nor_sr_ready_and_clear() can be static

[kernel test robot]

drivers/mtd/spi-nor/spansion.c:332:5: warning: symbol 'spi_nor_sr_ready_and_clear' was not declared. Should it be static?

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: kernel test robot <lkp@intel.com>
---
 spansion.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index 4756fb88eab232..242d55e043f08d 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -329,7 +329,7 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
  *
  * Return: 1 if ready, 0 if not ready, -errno on errors.
  */
-int spi_nor_sr_ready_and_clear(struct spi_nor *nor)
+static int spi_nor_sr_ready_and_clear(struct spi_nor *nor)
 {
 	int ret;
 

Re: [PATCH v1 12/14] mtd: spi-nor: move all spansion specifics into spansion.c

[kernel test robot]

Hi Michael,

I love your patch! Perhaps something to improve:

[auto build test WARNING on mtd/spi-nor/next]
[also build test WARNING on linux/master linus/master v5.17-rc2 next-20220202]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Michael-Walle/mtd-spi-nor-move-vendor-specific-code-into-vendor-modules/20220202-230139
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git spi-nor/next
config: x86_64-randconfig-s021 (https://download.01.org/0day-ci/archive/20220203/202202030556.9u7o7D18-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-dirty
        # https://github.com/0day-ci/linux/commit/01f9808a20c96553c43f929e10f3fb448cd8bd93
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Michael-Walle/mtd-spi-nor-move-vendor-specific-code-into-vendor-modules/20220202-230139
        git checkout 01f9808a20c96553c43f929e10f3fb448cd8bd93
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/mtd/spi-nor/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> drivers/mtd/spi-nor/spansion.c:332:5: sparse: sparse: symbol 'spi_nor_sr_ready_and_clear' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH v1 12/14] mtd: spi-nor: move all spansion specifics into spansion.c

[Tudor.Ambarus]

On 2/2/22 16:58, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> The clear status register flags is only available on spansion flashes.
> Move all the functions around that into the spanion module.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/core.c     | 52 +------------------------
>  drivers/mtd/spi-nor/spansion.c | 70 ++++++++++++++++++++++++++++++++++
>  include/linux/mtd/spi-nor.h    |  1 -
>  3 files changed, 72 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index be65aaa954ca..5b00dfab77a6 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -554,33 +554,6 @@ int spi_nor_write_ear(struct spi_nor *nor, u8 ear)
>         return ret;
>  }
> 
> -/**
> - * spi_nor_clear_sr() - Clear the Status Register.
> - * @nor:       pointer to 'struct spi_nor'.
> - */
> -static void spi_nor_clear_sr(struct spi_nor *nor)
> -{
> -       int ret;
> -
> -       if (nor->spimem) {
> -               struct spi_mem_op op =
> -                       SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLSR, 0),
> -                                  SPI_MEM_OP_NO_ADDR,
> -                                  SPI_MEM_OP_NO_DUMMY,
> -                                  SPI_MEM_OP_NO_DATA);
> -
> -               spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> -
> -               ret = spi_mem_exec_op(nor->spimem, &op);
> -       } else {
> -               ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLSR,
> -                                                      NULL, 0);
> -       }
> -
> -       if (ret)
> -               dev_dbg(nor->dev, "error %d clearing SR\n", ret);
> -}
> -
>  /**
>   * spi_nor_sr_ready() - Query the Status Register to see if the flash is ready
>   * for new commands.
> @@ -590,33 +563,12 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
>   */
>  int spi_nor_sr_ready(struct spi_nor *nor)
>  {
> -       int ret = spi_nor_read_sr(nor, nor->bouncebuf);
> +       int ret;
> 
> +       ret = spi_nor_read_sr(nor, nor->bouncebuf);
>         if (ret)
>                 return ret;

:) don't change style for no reason. What's wrong with the previous version?

Anyway, with the reports fixed and no hidden style changes, it looks good to me.

> 
> -       if (nor->flags & SNOR_F_USE_CLSR &&
> -           nor->bouncebuf[0] & (SR_E_ERR | SR_P_ERR)) {
> -               if (nor->bouncebuf[0] & SR_E_ERR)
> -                       dev_err(nor->dev, "Erase Error occurred\n");
> -               else
> -                       dev_err(nor->dev, "Programming Error occurred\n");
> -
> -               spi_nor_clear_sr(nor);
> -
> -               /*
> -                * WEL bit remains set to one when an erase or page program
> -                * error occurs. Issue a Write Disable command to protect
> -                * against inadvertent writes that can possibly corrupt the
> -                * contents of the memory.
> -                */
> -               ret = spi_nor_write_disable(nor);
> -               if (ret)
> -                       return ret;
> -
> -               return -EIO;
> -       }
> -
>         return !(nor->bouncebuf[0] & SR_WIP);
>  }
> 
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index dedc2de90cb8..4756fb88eab2 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -8,6 +8,7 @@
> 
>  #include "core.h"
> 
> +#define SPINOR_OP_CLSR         0x30    /* Clear status register 1 */
>  #define SPINOR_OP_RD_ANY_REG                   0x65    /* Read any register */
>  #define SPINOR_OP_WR_ANY_REG                   0x71    /* Write any register */
>  #define SPINOR_REG_CYPRESS_CFR2V               0x00800003
> @@ -294,6 +295,72 @@ static const struct flash_info spansion_parts[] = {
>         },
>  };
> 
> +/**
> + * spi_nor_clear_sr() - Clear the Status Register.
> + * @nor:       pointer to 'struct spi_nor'.
> + */
> +static void spi_nor_clear_sr(struct spi_nor *nor)
> +{
> +       int ret;
> +
> +       if (nor->spimem) {
> +               struct spi_mem_op op =
> +                       SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLSR, 0),
> +                                  SPI_MEM_OP_NO_ADDR,
> +                                  SPI_MEM_OP_NO_DUMMY,
> +                                  SPI_MEM_OP_NO_DATA);
> +
> +               spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> +
> +               ret = spi_mem_exec_op(nor->spimem, &op);
> +       } else {
> +               ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLSR,
> +                                                      NULL, 0);
> +       }
> +
> +       if (ret)
> +               dev_dbg(nor->dev, "error %d clearing SR\n", ret);
> +}
> +
> +/**
> + * spi_nor_sr_ready_and_clear() - Query the Status Register to see if the flash
> + * is ready for new commands and clear it.
> + * @nor:       pointer to 'struct spi_nor'.
> + *
> + * Return: 1 if ready, 0 if not ready, -errno on errors.
> + */
> +int spi_nor_sr_ready_and_clear(struct spi_nor *nor)
> +{
> +       int ret;
> +
> +       ret = spi_nor_read_sr(nor, nor->bouncebuf);
> +       if (ret)
> +               return ret;
> +
> +       if (nor->bouncebuf[0] & (SR_E_ERR | SR_P_ERR)) {
> +               if (nor->bouncebuf[0] & SR_E_ERR)
> +                       dev_err(nor->dev, "Erase Error occurred\n");
> +               else
> +                       dev_err(nor->dev, "Programming Error occurred\n");
> +
> +               spi_nor_clear_sr(nor);
> +
> +               /*
> +                * WEL bit remains set to one when an erase or page program
> +                * error occurs. Issue a Write Disable command to protect
> +                * against inadvertent writes that can possibly corrupt the
> +                * contents of the memory.
> +                */
> +               ret = spi_nor_write_disable(nor);
> +               if (ret)
> +                       return ret;
> +
> +               return -EIO;
> +       }
> +
> +       return !(nor->bouncebuf[0] & SR_WIP);
> +}
> +
>  static void spansion_late_init(struct spi_nor *nor)
>  {
>         if (nor->params->size > SZ_16M) {
> @@ -302,6 +369,9 @@ static void spansion_late_init(struct spi_nor *nor)
>                 nor->erase_opcode = SPINOR_OP_SE;
>                 nor->mtd.erasesize = nor->info->sector_size;
>         }
> +
> +       if (nor->flags & SNOR_F_USE_CLSR)
> +               nor->params->ready = spi_nor_sr_ready_and_clear;
>  }
> 
>  static const struct spi_nor_fixups spansion_fixups = {
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 4622251a79ff..5e25a7b75ae2 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -90,7 +90,6 @@
> 
>  /* Used for Spansion flashes only. */
>  #define SPINOR_OP_BRWR         0x17    /* Bank register write */
> -#define SPINOR_OP_CLSR         0x30    /* Clear status register 1 */
> 
>  /* Used for Micron flashes only. */
>  #define SPINOR_OP_RD_EVCR      0x65    /* Read EVCR register */
> --
> 2.30.2
> 

Re: [PATCH v1 12/14] mtd: spi-nor: move all spansion specifics into spansion.c

[Pratyush Yadav]

On 10/02/22 03:32AM, Tudor.Ambarus@microchip.com wrote:
> On 2/2/22 16:58, Michael Walle wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > The clear status register flags is only available on spansion flashes.
> > Move all the functions around that into the spanion module.
> > 
> > Signed-off-by: Michael Walle <michael@walle.cc>
> > ---
> >  drivers/mtd/spi-nor/core.c     | 52 +------------------------
> >  drivers/mtd/spi-nor/spansion.c | 70 ++++++++++++++++++++++++++++++++++
> >  include/linux/mtd/spi-nor.h    |  1 -
> >  3 files changed, 72 insertions(+), 51 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index be65aaa954ca..5b00dfab77a6 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -554,33 +554,6 @@ int spi_nor_write_ear(struct spi_nor *nor, u8 ear)
> >         return ret;
> >  }
> > 
> > -/**
> > - * spi_nor_clear_sr() - Clear the Status Register.
> > - * @nor:       pointer to 'struct spi_nor'.
> > - */
> > -static void spi_nor_clear_sr(struct spi_nor *nor)
> > -{
> > -       int ret;
> > -
> > -       if (nor->spimem) {
> > -               struct spi_mem_op op =
> > -                       SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLSR, 0),
> > -                                  SPI_MEM_OP_NO_ADDR,
> > -                                  SPI_MEM_OP_NO_DUMMY,
> > -                                  SPI_MEM_OP_NO_DATA);
> > -
> > -               spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> > -
> > -               ret = spi_mem_exec_op(nor->spimem, &op);
> > -       } else {
> > -               ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLSR,
> > -                                                      NULL, 0);
> > -       }
> > -
> > -       if (ret)
> > -               dev_dbg(nor->dev, "error %d clearing SR\n", ret);
> > -}
> > -
> >  /**
> >   * spi_nor_sr_ready() - Query the Status Register to see if the flash is ready
> >   * for new commands.
> > @@ -590,33 +563,12 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
> >   */
> >  int spi_nor_sr_ready(struct spi_nor *nor)
> >  {
> > -       int ret = spi_nor_read_sr(nor, nor->bouncebuf);
> > +       int ret;
> > 
> > +       ret = spi_nor_read_sr(nor, nor->bouncebuf);
> >         if (ret)
> >                 return ret;
> 
> :) don't change style for no reason. What's wrong with the previous version?

FWIW, I like the newer style better. But that should come in a separate 
patch in either case.

> 
> Anyway, with the reports fixed and no hidden style changes, it looks good to me.
> 
> > 
> > -       if (nor->flags & SNOR_F_USE_CLSR &&
> > -           nor->bouncebuf[0] & (SR_E_ERR | SR_P_ERR)) {
> > -               if (nor->bouncebuf[0] & SR_E_ERR)
> > -                       dev_err(nor->dev, "Erase Error occurred\n");
> > -               else
> > -                       dev_err(nor->dev, "Programming Error occurred\n");
> > -
> > -               spi_nor_clear_sr(nor);
> > -
> > -               /*
> > -                * WEL bit remains set to one when an erase or page program
> > -                * error occurs. Issue a Write Disable command to protect
> > -                * against inadvertent writes that can possibly corrupt the
> > -                * contents of the memory.
> > -                */
> > -               ret = spi_nor_write_disable(nor);
> > -               if (ret)
> > -                       return ret;
> > -
> > -               return -EIO;
> > -       }
> > -
> >         return !(nor->bouncebuf[0] & SR_WIP);
> >  }
> > 
> > diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> > index dedc2de90cb8..4756fb88eab2 100644
> > --- a/drivers/mtd/spi-nor/spansion.c
> > +++ b/drivers/mtd/spi-nor/spansion.c
> > @@ -8,6 +8,7 @@
> > 
> >  #include "core.h"
> > 
> > +#define SPINOR_OP_CLSR         0x30    /* Clear status register 1 */
> >  #define SPINOR_OP_RD_ANY_REG                   0x65    /* Read any register */
> >  #define SPINOR_OP_WR_ANY_REG                   0x71    /* Write any register */
> >  #define SPINOR_REG_CYPRESS_CFR2V               0x00800003
> > @@ -294,6 +295,72 @@ static const struct flash_info spansion_parts[] = {
> >         },
> >  };
> > 
> > +/**
> > + * spi_nor_clear_sr() - Clear the Status Register.
> > + * @nor:       pointer to 'struct spi_nor'.
> > + */
> > +static void spi_nor_clear_sr(struct spi_nor *nor)
> > +{
> > +       int ret;
> > +
> > +       if (nor->spimem) {
> > +               struct spi_mem_op op =
> > +                       SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLSR, 0),
> > +                                  SPI_MEM_OP_NO_ADDR,
> > +                                  SPI_MEM_OP_NO_DUMMY,
> > +                                  SPI_MEM_OP_NO_DATA);
> > +
> > +               spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
> > +
> > +               ret = spi_mem_exec_op(nor->spimem, &op);
> > +       } else {
> > +               ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLSR,
> > +                                                      NULL, 0);
> > +       }
> > +
> > +       if (ret)
> > +               dev_dbg(nor->dev, "error %d clearing SR\n", ret);
> > +}
> > +
> > +/**
> > + * spi_nor_sr_ready_and_clear() - Query the Status Register to see if the flash
> > + * is ready for new commands and clear it.
> > + * @nor:       pointer to 'struct spi_nor'.
> > + *
> > + * Return: 1 if ready, 0 if not ready, -errno on errors.
> > + */
> > +int spi_nor_sr_ready_and_clear(struct spi_nor *nor)

Make it static.

[...]

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v1 13/14] mtd: spi-nor: spansion: convert USE_CLSR to a manufacturer flag

[Michael Walle]

Now that all functions using that flag are local to the spanion module,
we can convert the flag to a manufacturer one.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/core.c     |  3 --
 drivers/mtd/spi-nor/core.h     |  3 --
 drivers/mtd/spi-nor/spansion.c | 54 +++++++++++++++++++++-------------
 3 files changed, 33 insertions(+), 27 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 5b00dfab77a6..2d5517b3db96 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2448,9 +2448,6 @@ static void spi_nor_init_flags(struct spi_nor *nor)
 
 	if (flags & NO_CHIP_ERASE)
 		nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
-
-	if (flags & USE_CLSR)
-		nor->flags |= SNOR_F_USE_CLSR;
 }
 
 /**
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index a02bf54289fb..2130a96e2044 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -14,7 +14,6 @@
 enum spi_nor_option_flags {
 	SNOR_F_HAS_SR_TB	= BIT(1),
 	SNOR_F_NO_OP_CHIP_ERASE	= BIT(2),
-	SNOR_F_USE_CLSR		= BIT(4),
 	SNOR_F_BROKEN_RESET	= BIT(5),
 	SNOR_F_4B_OPCODES	= BIT(6),
 	SNOR_F_HAS_4BAIT	= BIT(7),
@@ -347,7 +346,6 @@ struct spi_nor_fixups {
  *   SPI_NOR_NO_ERASE:        no erase command needed.
  *   NO_CHIP_ERASE:           chip does not support chip erase.
  *   SPI_NOR_NO_FR:           can't do fastread.
- *   USE_CLSR:                use CLSR command.
  *
  * @no_sfdp_flags:  flags that indicate support that can be discovered via SFDP.
  *                  Used when SFDP tables are not defined in the flash. These
@@ -398,7 +396,6 @@ struct flash_info {
 #define SPI_NOR_NO_ERASE		BIT(6)
 #define NO_CHIP_ERASE			BIT(7)
 #define SPI_NOR_NO_FR			BIT(8)
-#define USE_CLSR			BIT(9)
 
 	u8 no_sfdp_flags;
 #define SPI_NOR_SKIP_SFDP		BIT(0)
diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index 4756fb88eab2..c31ea11f71f2 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -8,6 +8,8 @@
 
 #include "core.h"
 
+#define USE_CLSR	BIT(0)
+
 #define SPINOR_OP_CLSR		0x30	/* Clear status register 1 */
 #define SPINOR_OP_RD_ANY_REG			0x65	/* Read any register */
 #define SPINOR_OP_WR_ANY_REG			0x71	/* Write any register */
@@ -212,43 +214,53 @@ static const struct flash_info spansion_parts[] = {
 	{ "s25sl064p",  INFO(0x010216, 0x4d00,  64 * 1024, 128)
 		NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
 	{ "s25fl128s0", INFO6(0x012018, 0x4d0080, 256 * 1024, 64)
-		FLAGS(USE_CLSR)
-		NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+		NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
+		MFR_FLAGS(USE_CLSR)
+	},
 	{ "s25fl128s1", INFO6(0x012018, 0x4d0180, 64 * 1024, 256)
-		FLAGS(USE_CLSR)
-		NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+		NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
+		MFR_FLAGS(USE_CLSR)
+	},
 	{ "s25fl256s0", INFO6(0x010219, 0x4d0080, 256 * 1024, 128)
-		FLAGS(USE_CLSR)
 		NO_SFDP_FLAGS(SPI_NOR_SKIP_SFDP | SPI_NOR_DUAL_READ |
-			      SPI_NOR_QUAD_READ) },
+			      SPI_NOR_QUAD_READ)
+		MFR_FLAGS(USE_CLSR)
+	},
 	{ "s25fl256s1", INFO6(0x010219, 0x4d0180, 64 * 1024, 512)
-		FLAGS(USE_CLSR)
-		NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+		NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
+		MFR_FLAGS(USE_CLSR)
+	},
 	{ "s25fl512s",  INFO6(0x010220, 0x4d0080, 256 * 1024, 256)
-		FLAGS(SPI_NOR_HAS_LOCK | USE_CLSR)
-		NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+		FLAGS(SPI_NOR_HAS_LOCK)
+		NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
+		MFR_FLAGS(USE_CLSR)
+	},
 	{ "s25fs128s1", INFO6(0x012018, 0x4d0181, 64 * 1024, 256)
-		FLAGS(USE_CLSR)
 		NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
+		MFR_FLAGS(USE_CLSR)
 		.fixups = &s25fs_s_fixups, },
 	{ "s25fs256s0", INFO6(0x010219, 0x4d0081, 256 * 1024, 128)
-		FLAGS(USE_CLSR)
-		NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+		NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
+		MFR_FLAGS(USE_CLSR)
+	},
 	{ "s25fs256s1", INFO6(0x010219, 0x4d0181, 64 * 1024, 512)
-		FLAGS(USE_CLSR)
-		NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+		NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
+		MFR_FLAGS(USE_CLSR)
+	},
 	{ "s25fs512s",  INFO6(0x010220, 0x4d0081, 256 * 1024, 256)
-		FLAGS(USE_CLSR)
 		NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
+		MFR_FLAGS(USE_CLSR)
 		.fixups = &s25fs_s_fixups, },
 	{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64) },
 	{ "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256) },
 	{ "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64)
-		FLAGS(USE_CLSR)
-		NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+		NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
+		MFR_FLAGS(USE_CLSR)
+	},
 	{ "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256)
-		FLAGS(USE_CLSR)
-		NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+		NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
+		MFR_FLAGS(USE_CLSR)
+	},
 	{ "s25sl004a",  INFO(0x010212,      0,  64 * 1024,   8) },
 	{ "s25sl008a",  INFO(0x010213,      0,  64 * 1024,  16) },
 	{ "s25sl016a",  INFO(0x010214,      0,  64 * 1024,  32) },
@@ -370,7 +382,7 @@ static void spansion_late_init(struct spi_nor *nor)
 		nor->mtd.erasesize = nor->info->sector_size;
 	}
 
-	if (nor->flags & SNOR_F_USE_CLSR)
+	if (nor->info->mfr_flags & USE_CLSR)
 		nor->params->ready = spi_nor_sr_ready_and_clear;
 }
 
-- 
2.30.2

Re: [PATCH v1 13/14] mtd: spi-nor: spansion: convert USE_CLSR to a manufacturer flag

[Tudor.Ambarus]

On 2/2/22 16:58, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Now that all functions using that flag are local to the spanion module,
> we can convert the flag to a manufacturer one.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/mtd/spi-nor/core.c     |  3 --
>  drivers/mtd/spi-nor/core.h     |  3 --
>  drivers/mtd/spi-nor/spansion.c | 54 +++++++++++++++++++++-------------
>  3 files changed, 33 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 5b00dfab77a6..2d5517b3db96 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2448,9 +2448,6 @@ static void spi_nor_init_flags(struct spi_nor *nor)
> 
>         if (flags & NO_CHIP_ERASE)
>                 nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
> -
> -       if (flags & USE_CLSR)
> -               nor->flags |= SNOR_F_USE_CLSR;
>  }
> 
>  /**
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index a02bf54289fb..2130a96e2044 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -14,7 +14,6 @@
>  enum spi_nor_option_flags {
>         SNOR_F_HAS_SR_TB        = BIT(1),
>         SNOR_F_NO_OP_CHIP_ERASE = BIT(2),
> -       SNOR_F_USE_CLSR         = BIT(4),
>         SNOR_F_BROKEN_RESET     = BIT(5),
>         SNOR_F_4B_OPCODES       = BIT(6),
>         SNOR_F_HAS_4BAIT        = BIT(7),
> @@ -347,7 +346,6 @@ struct spi_nor_fixups {
>   *   SPI_NOR_NO_ERASE:        no erase command needed.
>   *   NO_CHIP_ERASE:           chip does not support chip erase.
>   *   SPI_NOR_NO_FR:           can't do fastread.
> - *   USE_CLSR:                use CLSR command.
>   *
>   * @no_sfdp_flags:  flags that indicate support that can be discovered via SFDP.
>   *                  Used when SFDP tables are not defined in the flash. These
> @@ -398,7 +396,6 @@ struct flash_info {
>  #define SPI_NOR_NO_ERASE               BIT(6)
>  #define NO_CHIP_ERASE                  BIT(7)
>  #define SPI_NOR_NO_FR                  BIT(8)
> -#define USE_CLSR                       BIT(9)
> 
>         u8 no_sfdp_flags;
>  #define SPI_NOR_SKIP_SFDP              BIT(0)
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index 4756fb88eab2..c31ea11f71f2 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -8,6 +8,8 @@
> 
>  #include "core.h"
> 
> +#define USE_CLSR       BIT(0)

add a description, tell the reader this is a manufacturer specific flag.
excellent work:

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

> +
>  #define SPINOR_OP_CLSR         0x30    /* Clear status register 1 */
>  #define SPINOR_OP_RD_ANY_REG                   0x65    /* Read any register */
>  #define SPINOR_OP_WR_ANY_REG                   0x71    /* Write any register */
> @@ -212,43 +214,53 @@ static const struct flash_info spansion_parts[] = {
>         { "s25sl064p",  INFO(0x010216, 0x4d00,  64 * 1024, 128)
>                 NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>         { "s25fl128s0", INFO6(0x012018, 0x4d0080, 256 * 1024, 64)
> -               FLAGS(USE_CLSR)
> -               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> +               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> +               MFR_FLAGS(USE_CLSR)
> +       },
>         { "s25fl128s1", INFO6(0x012018, 0x4d0180, 64 * 1024, 256)
> -               FLAGS(USE_CLSR)
> -               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> +               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> +               MFR_FLAGS(USE_CLSR)
> +       },
>         { "s25fl256s0", INFO6(0x010219, 0x4d0080, 256 * 1024, 128)
> -               FLAGS(USE_CLSR)
>                 NO_SFDP_FLAGS(SPI_NOR_SKIP_SFDP | SPI_NOR_DUAL_READ |
> -                             SPI_NOR_QUAD_READ) },
> +                             SPI_NOR_QUAD_READ)
> +               MFR_FLAGS(USE_CLSR)
> +       },
>         { "s25fl256s1", INFO6(0x010219, 0x4d0180, 64 * 1024, 512)
> -               FLAGS(USE_CLSR)
> -               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> +               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> +               MFR_FLAGS(USE_CLSR)
> +       },
>         { "s25fl512s",  INFO6(0x010220, 0x4d0080, 256 * 1024, 256)
> -               FLAGS(SPI_NOR_HAS_LOCK | USE_CLSR)
> -               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> +               FLAGS(SPI_NOR_HAS_LOCK)
> +               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> +               MFR_FLAGS(USE_CLSR)
> +       },
>         { "s25fs128s1", INFO6(0x012018, 0x4d0181, 64 * 1024, 256)
> -               FLAGS(USE_CLSR)
>                 NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> +               MFR_FLAGS(USE_CLSR)
>                 .fixups = &s25fs_s_fixups, },
>         { "s25fs256s0", INFO6(0x010219, 0x4d0081, 256 * 1024, 128)
> -               FLAGS(USE_CLSR)
> -               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> +               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> +               MFR_FLAGS(USE_CLSR)
> +       },
>         { "s25fs256s1", INFO6(0x010219, 0x4d0181, 64 * 1024, 512)
> -               FLAGS(USE_CLSR)
> -               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> +               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> +               MFR_FLAGS(USE_CLSR)
> +       },
>         { "s25fs512s",  INFO6(0x010220, 0x4d0081, 256 * 1024, 256)
> -               FLAGS(USE_CLSR)
>                 NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> +               MFR_FLAGS(USE_CLSR)
>                 .fixups = &s25fs_s_fixups, },
>         { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64) },
>         { "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256) },
>         { "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64)
> -               FLAGS(USE_CLSR)
> -               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> +               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> +               MFR_FLAGS(USE_CLSR)
> +       },
>         { "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256)
> -               FLAGS(USE_CLSR)
> -               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> +               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> +               MFR_FLAGS(USE_CLSR)
> +       },
>         { "s25sl004a",  INFO(0x010212,      0,  64 * 1024,   8) },
>         { "s25sl008a",  INFO(0x010213,      0,  64 * 1024,  16) },
>         { "s25sl016a",  INFO(0x010214,      0,  64 * 1024,  32) },
> @@ -370,7 +382,7 @@ static void spansion_late_init(struct spi_nor *nor)
>                 nor->mtd.erasesize = nor->info->sector_size;
>         }
> 
> -       if (nor->flags & SNOR_F_USE_CLSR)
> +       if (nor->info->mfr_flags & USE_CLSR)
>                 nor->params->ready = spi_nor_sr_ready_and_clear;
>  }
> 
> --
> 2.30.2
> 

Re: [PATCH v1 13/14] mtd: spi-nor: spansion: convert USE_CLSR to a manufacturer flag

[Pratyush Yadav]

On 10/02/22 03:34AM, Tudor.Ambarus@microchip.com wrote:
> On 2/2/22 16:58, Michael Walle wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > Now that all functions using that flag are local to the spanion module,

s/spanion/spansion/

> > we can convert the flag to a manufacturer one.
> > 
> > Signed-off-by: Michael Walle <michael@walle.cc>
> > ---
> >  drivers/mtd/spi-nor/core.c     |  3 --
> >  drivers/mtd/spi-nor/core.h     |  3 --
> >  drivers/mtd/spi-nor/spansion.c | 54 +++++++++++++++++++++-------------
> >  3 files changed, 33 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > index 5b00dfab77a6..2d5517b3db96 100644
> > --- a/drivers/mtd/spi-nor/core.c
> > +++ b/drivers/mtd/spi-nor/core.c
> > @@ -2448,9 +2448,6 @@ static void spi_nor_init_flags(struct spi_nor *nor)
> > 
> >         if (flags & NO_CHIP_ERASE)
> >                 nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
> > -
> > -       if (flags & USE_CLSR)
> > -               nor->flags |= SNOR_F_USE_CLSR;
> >  }
> > 
> >  /**
> > diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> > index a02bf54289fb..2130a96e2044 100644
> > --- a/drivers/mtd/spi-nor/core.h
> > +++ b/drivers/mtd/spi-nor/core.h
> > @@ -14,7 +14,6 @@
> >  enum spi_nor_option_flags {
> >         SNOR_F_HAS_SR_TB        = BIT(1),
> >         SNOR_F_NO_OP_CHIP_ERASE = BIT(2),
> > -       SNOR_F_USE_CLSR         = BIT(4),
> >         SNOR_F_BROKEN_RESET     = BIT(5),
> >         SNOR_F_4B_OPCODES       = BIT(6),
> >         SNOR_F_HAS_4BAIT        = BIT(7),
> > @@ -347,7 +346,6 @@ struct spi_nor_fixups {
> >   *   SPI_NOR_NO_ERASE:        no erase command needed.
> >   *   NO_CHIP_ERASE:           chip does not support chip erase.
> >   *   SPI_NOR_NO_FR:           can't do fastread.
> > - *   USE_CLSR:                use CLSR command.
> >   *
> >   * @no_sfdp_flags:  flags that indicate support that can be discovered via SFDP.
> >   *                  Used when SFDP tables are not defined in the flash. These
> > @@ -398,7 +396,6 @@ struct flash_info {
> >  #define SPI_NOR_NO_ERASE               BIT(6)
> >  #define NO_CHIP_ERASE                  BIT(7)
> >  #define SPI_NOR_NO_FR                  BIT(8)
> > -#define USE_CLSR                       BIT(9)
> > 
> >         u8 no_sfdp_flags;
> >  #define SPI_NOR_SKIP_SFDP              BIT(0)
> > diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> > index 4756fb88eab2..c31ea11f71f2 100644
> > --- a/drivers/mtd/spi-nor/spansion.c
> > +++ b/drivers/mtd/spi-nor/spansion.c
> > @@ -8,6 +8,8 @@
> > 
> >  #include "core.h"
> > 
> > +#define USE_CLSR       BIT(0)
> 
> add a description, tell the reader this is a manufacturer specific flag.

+1

> excellent work:

+1

> 
> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

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

> 
> > +
> >  #define SPINOR_OP_CLSR         0x30    /* Clear status register 1 */
> >  #define SPINOR_OP_RD_ANY_REG                   0x65    /* Read any register */
> >  #define SPINOR_OP_WR_ANY_REG                   0x71    /* Write any register */
> > @@ -212,43 +214,53 @@ static const struct flash_info spansion_parts[] = {
> >         { "s25sl064p",  INFO(0x010216, 0x4d00,  64 * 1024, 128)
> >                 NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> >         { "s25fl128s0", INFO6(0x012018, 0x4d0080, 256 * 1024, 64)
> > -               FLAGS(USE_CLSR)
> > -               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> > +               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> > +               MFR_FLAGS(USE_CLSR)
> > +       },
> >         { "s25fl128s1", INFO6(0x012018, 0x4d0180, 64 * 1024, 256)
> > -               FLAGS(USE_CLSR)
> > -               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> > +               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> > +               MFR_FLAGS(USE_CLSR)
> > +       },
> >         { "s25fl256s0", INFO6(0x010219, 0x4d0080, 256 * 1024, 128)
> > -               FLAGS(USE_CLSR)
> >                 NO_SFDP_FLAGS(SPI_NOR_SKIP_SFDP | SPI_NOR_DUAL_READ |
> > -                             SPI_NOR_QUAD_READ) },
> > +                             SPI_NOR_QUAD_READ)
> > +               MFR_FLAGS(USE_CLSR)
> > +       },
> >         { "s25fl256s1", INFO6(0x010219, 0x4d0180, 64 * 1024, 512)
> > -               FLAGS(USE_CLSR)
> > -               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> > +               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> > +               MFR_FLAGS(USE_CLSR)
> > +       },
> >         { "s25fl512s",  INFO6(0x010220, 0x4d0080, 256 * 1024, 256)
> > -               FLAGS(SPI_NOR_HAS_LOCK | USE_CLSR)
> > -               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> > +               FLAGS(SPI_NOR_HAS_LOCK)
> > +               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> > +               MFR_FLAGS(USE_CLSR)
> > +       },
> >         { "s25fs128s1", INFO6(0x012018, 0x4d0181, 64 * 1024, 256)
> > -               FLAGS(USE_CLSR)
> >                 NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> > +               MFR_FLAGS(USE_CLSR)
> >                 .fixups = &s25fs_s_fixups, },
> >         { "s25fs256s0", INFO6(0x010219, 0x4d0081, 256 * 1024, 128)
> > -               FLAGS(USE_CLSR)
> > -               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> > +               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> > +               MFR_FLAGS(USE_CLSR)
> > +       },
> >         { "s25fs256s1", INFO6(0x010219, 0x4d0181, 64 * 1024, 512)
> > -               FLAGS(USE_CLSR)
> > -               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> > +               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> > +               MFR_FLAGS(USE_CLSR)
> > +       },
> >         { "s25fs512s",  INFO6(0x010220, 0x4d0081, 256 * 1024, 256)
> > -               FLAGS(USE_CLSR)
> >                 NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> > +               MFR_FLAGS(USE_CLSR)
> >                 .fixups = &s25fs_s_fixups, },
> >         { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64) },
> >         { "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256) },
> >         { "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64)
> > -               FLAGS(USE_CLSR)
> > -               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> > +               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> > +               MFR_FLAGS(USE_CLSR)
> > +       },
> >         { "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256)
> > -               FLAGS(USE_CLSR)
> > -               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> > +               NO_SFDP_FLAGS(SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
> > +               MFR_FLAGS(USE_CLSR)
> > +       },
> >         { "s25sl004a",  INFO(0x010212,      0,  64 * 1024,   8) },
> >         { "s25sl008a",  INFO(0x010213,      0,  64 * 1024,  16) },
> >         { "s25sl016a",  INFO(0x010214,      0,  64 * 1024,  32) },
> > @@ -370,7 +382,7 @@ static void spansion_late_init(struct spi_nor *nor)
> >                 nor->mtd.erasesize = nor->info->sector_size;
> >         }
> > 
> > -       if (nor->flags & SNOR_F_USE_CLSR)
> > +       if (nor->info->mfr_flags & USE_CLSR)
> >                 nor->params->ready = spi_nor_sr_ready_and_clear;
> >  }
> > 
> > --
> > 2.30.2
> > 
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v1 14/14] mtd: spi-nor: renumber flags

[Michael Walle]

As we have deleted some flag, lets renumber them so there are no holes.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mtd/spi-nor/core.h | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 2130a96e2044..b7fd760e3b47 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -12,20 +12,20 @@
 #define SPI_NOR_MAX_ID_LEN	6
 
 enum spi_nor_option_flags {
-	SNOR_F_HAS_SR_TB	= BIT(1),
-	SNOR_F_NO_OP_CHIP_ERASE	= BIT(2),
-	SNOR_F_BROKEN_RESET	= BIT(5),
-	SNOR_F_4B_OPCODES	= BIT(6),
-	SNOR_F_HAS_4BAIT	= BIT(7),
-	SNOR_F_HAS_LOCK		= BIT(8),
-	SNOR_F_HAS_16BIT_SR	= BIT(9),
-	SNOR_F_NO_READ_CR	= BIT(10),
-	SNOR_F_HAS_SR_TB_BIT6	= BIT(11),
-	SNOR_F_HAS_4BIT_BP      = BIT(12),
-	SNOR_F_HAS_SR_BP3_BIT6  = BIT(13),
-	SNOR_F_IO_MODE_EN_VOLATILE = BIT(14),
-	SNOR_F_SOFT_RESET	= BIT(15),
-	SNOR_F_SWP_IS_VOLATILE	= BIT(16),
+	SNOR_F_HAS_SR_TB	= BIT(0),
+	SNOR_F_NO_OP_CHIP_ERASE	= BIT(1),
+	SNOR_F_BROKEN_RESET	= BIT(2),
+	SNOR_F_4B_OPCODES	= BIT(3),
+	SNOR_F_HAS_4BAIT	= BIT(4),
+	SNOR_F_HAS_LOCK		= BIT(5),
+	SNOR_F_HAS_16BIT_SR	= BIT(6),
+	SNOR_F_NO_READ_CR	= BIT(7),
+	SNOR_F_HAS_SR_TB_BIT6	= BIT(8),
+	SNOR_F_HAS_4BIT_BP      = BIT(9),
+	SNOR_F_HAS_SR_BP3_BIT6  = BIT(10),
+	SNOR_F_IO_MODE_EN_VOLATILE = BIT(11),
+	SNOR_F_SOFT_RESET	= BIT(12),
+	SNOR_F_SWP_IS_VOLATILE	= BIT(13),
 };
 
 struct spi_nor_read_command {
-- 
2.30.2

Re: [PATCH v1 14/14] mtd: spi-nor: renumber flags

[Tudor.Ambarus]

On 2/2/22 16:58, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> As we have deleted some flag, lets renumber them so there are no holes.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>

Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

> ---
>  drivers/mtd/spi-nor/core.h | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 2130a96e2044..b7fd760e3b47 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -12,20 +12,20 @@
>  #define SPI_NOR_MAX_ID_LEN     6
> 
>  enum spi_nor_option_flags {
> -       SNOR_F_HAS_SR_TB        = BIT(1),
> -       SNOR_F_NO_OP_CHIP_ERASE = BIT(2),
> -       SNOR_F_BROKEN_RESET     = BIT(5),
> -       SNOR_F_4B_OPCODES       = BIT(6),
> -       SNOR_F_HAS_4BAIT        = BIT(7),
> -       SNOR_F_HAS_LOCK         = BIT(8),
> -       SNOR_F_HAS_16BIT_SR     = BIT(9),
> -       SNOR_F_NO_READ_CR       = BIT(10),
> -       SNOR_F_HAS_SR_TB_BIT6   = BIT(11),
> -       SNOR_F_HAS_4BIT_BP      = BIT(12),
> -       SNOR_F_HAS_SR_BP3_BIT6  = BIT(13),
> -       SNOR_F_IO_MODE_EN_VOLATILE = BIT(14),
> -       SNOR_F_SOFT_RESET       = BIT(15),
> -       SNOR_F_SWP_IS_VOLATILE  = BIT(16),
> +       SNOR_F_HAS_SR_TB        = BIT(0),
> +       SNOR_F_NO_OP_CHIP_ERASE = BIT(1),
> +       SNOR_F_BROKEN_RESET     = BIT(2),
> +       SNOR_F_4B_OPCODES       = BIT(3),
> +       SNOR_F_HAS_4BAIT        = BIT(4),
> +       SNOR_F_HAS_LOCK         = BIT(5),
> +       SNOR_F_HAS_16BIT_SR     = BIT(6),
> +       SNOR_F_NO_READ_CR       = BIT(7),
> +       SNOR_F_HAS_SR_TB_BIT6   = BIT(8),
> +       SNOR_F_HAS_4BIT_BP      = BIT(9),
> +       SNOR_F_HAS_SR_BP3_BIT6  = BIT(10),
> +       SNOR_F_IO_MODE_EN_VOLATILE = BIT(11),
> +       SNOR_F_SOFT_RESET       = BIT(12),
> +       SNOR_F_SWP_IS_VOLATILE  = BIT(13),
>  };
> 
>  struct spi_nor_read_command {
> --
> 2.30.2
> 

Re: [PATCH v1 14/14] mtd: spi-nor: renumber flags

[Pratyush Yadav]

On 02/02/22 03:58PM, Michael Walle wrote:
> As we have deleted some flag, lets renumber them so there are no holes.

I was expecting you to forget about this and was about to comment this 
in a previous patch. Good thing I looked ahead to double check :-)

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

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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v1 00/14] mtd: spi-nor: move vendor specific code into vendor modules

[Tudor.Ambarus]

On 2/2/22 16:58, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> It turns out that most of the special status register handling is
> specific for a particular vendor. I.e. Xilinx has some different
> opcodes for the status register read, Micron has an additional FSR
> register and Spansion has these flags integrated into the SR.
> 
> Create a callback to ready() where a flash chip can register its
> own function. This will let us move all the vendor specific stuff
> out of the core into the vendor modules.
> 
> Please note that this is only compile-time tested.
> 
> For sake of consistency and better readability of the code flow,
> I also converted the setup() callback to be optional.
> 
> Michael Walle (14):
>   mtd: spi-nor: export more function to be used in vendor modules
>   mtd: spi-nor: slightly refactor the spi_nor_setup()
>   mtd: spi-nor: allow a flash to define its own ready() function
>   mtd: spi-nor: move all xilinx specifics into xilinx.c
>   mtd: spi-nor: xilinx: rename vendor specific functions and defines
>   mtd: spi-nor: xilinx: correct the debug message
>   mtd: spi-nor: move all micron-st specifics into micron-st.c
>   mtd: spi-nor: micron-st: convert USE_FSR to a manufacturer flag
>   mtd: spi-nor: micron-st: fix micron_st prefix
>   mtd: spi-nor: micron-st: rename vendor specific functions and defines
>   mtd: spi-nor: spansion: slightly rework control flow in late_init()
>   mtd: spi-nor: move all spansion specifics into spansion.c
>   mtd: spi-nor: spansion: convert USE_CLSR to a manufacturer flag
>   mtd: spi-nor: renumber flags
> 
>  drivers/mtd/spi-nor/core.c      | 265 ++------------------------------
>  drivers/mtd/spi-nor/core.h      |  70 ++++-----
>  drivers/mtd/spi-nor/micron-st.c | 225 ++++++++++++++++++++++-----
>  drivers/mtd/spi-nor/spansion.c  | 133 ++++++++++++----
>  drivers/mtd/spi-nor/xilinx.c    |  79 +++++++++-
>  include/linux/mtd/spi-nor.h     |  18 ---
>  6 files changed, 417 insertions(+), 373 deletions(-)
> 

Good job, Michael! I added R-b to some patches to inform you that they
look fine to me. You don't need to add the tags on the next version, as
I'll be the one that will apply them. I think I have a micron and a spansion
flash somewhere, will try to test your series once v2 is out.

Cheers,
ta

Re: [PATCH v1 00/14] mtd: spi-nor: move vendor specific code into vendor modules

[Tudor.Ambarus]

+ Yaliang,

might be interested in reviewing/testing this, as there were some similar
patches submitted a while ago:

https://patchwork.ozlabs.org/project/linux-mtd/list/?series=237043

Cheers,
ta

On 2/2/22 16:58, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> It turns out that most of the special status register handling is
> specific for a particular vendor. I.e. Xilinx has some different
> opcodes for the status register read, Micron has an additional FSR
> register and Spansion has these flags integrated into the SR.
> 
> Create a callback to ready() where a flash chip can register its
> own function. This will let us move all the vendor specific stuff
> out of the core into the vendor modules.
> 
> Please note that this is only compile-time tested.
> 
> For sake of consistency and better readability of the code flow,
> I also converted the setup() callback to be optional.
> 
> Michael Walle (14):
>   mtd: spi-nor: export more function to be used in vendor modules
>   mtd: spi-nor: slightly refactor the spi_nor_setup()
>   mtd: spi-nor: allow a flash to define its own ready() function
>   mtd: spi-nor: move all xilinx specifics into xilinx.c
>   mtd: spi-nor: xilinx: rename vendor specific functions and defines
>   mtd: spi-nor: xilinx: correct the debug message
>   mtd: spi-nor: move all micron-st specifics into micron-st.c
>   mtd: spi-nor: micron-st: convert USE_FSR to a manufacturer flag
>   mtd: spi-nor: micron-st: fix micron_st prefix
>   mtd: spi-nor: micron-st: rename vendor specific functions and defines
>   mtd: spi-nor: spansion: slightly rework control flow in late_init()
>   mtd: spi-nor: move all spansion specifics into spansion.c
>   mtd: spi-nor: spansion: convert USE_CLSR to a manufacturer flag
>   mtd: spi-nor: renumber flags
> 
>  drivers/mtd/spi-nor/core.c      | 265 ++------------------------------
>  drivers/mtd/spi-nor/core.h      |  70 ++++-----
>  drivers/mtd/spi-nor/micron-st.c | 225 ++++++++++++++++++++++-----
>  drivers/mtd/spi-nor/spansion.c  | 133 ++++++++++++----
>  drivers/mtd/spi-nor/xilinx.c    |  79 +++++++++-
>  include/linux/mtd/spi-nor.h     |  18 ---
>  6 files changed, 417 insertions(+), 373 deletions(-)
> 
> --
> 2.30.2
>