[PATCH 1/2] mtd: spi-nor: core: move Spansion SR ready codes out of core
Pratyush Yadav
p.yadav at ti.com
Mon Mar 1 19:08:29 GMT 2021
On 01/03/21 10:28PM, yaliang.wang at windriver.com wrote:
> From: Yaliang Wang <Yaliang.Wang at windriver.com>
>
> To move manufacture specific checking SR ready codes out of core, set up
> an extra method "sr_ready" in struct spi_nor_fixups, by doing this, the
> manufactures can fill in and initialize the method it in their own
> files, and leaves the core a relatively clean place.
>
> Spansion's checking flash chip ready codes are moved out by applying
> forementioned "sr_ready" method. There will be several following-up
> patches for moving "xsr" and "fsr" related codes out of core.
>
> Signed-off-by: Yaliang Wang <Yaliang.Wang at windriver.com>
> ---
> drivers/mtd/spi-nor/core.c | 84 ++++++++++------------------------
> drivers/mtd/spi-nor/core.h | 8 ++++
> drivers/mtd/spi-nor/spansion.c | 70 ++++++++++++++++++++++++++++
> include/linux/mtd/spi-nor.h | 1 -
> 4 files changed, 101 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 20df44b753da..74729eb89947 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -157,7 +157,7 @@ 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,
> +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))
> @@ -166,7 +166,7 @@ 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,
> +int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
> const u8 *buf, size_t len)
The second line is not aligned with the open parenthesis in the above 3
functions. You can pass "--strict" to checkpatch.pl to check for this.
> {
> if (spi_nor_protocol_is_dtr(nor->reg_proto))
> @@ -175,7 +175,7 @@ static int spi_nor_controller_ops_write_reg(struct spi_nor *nor, u8 opcode,
> return nor->controller_ops->write_reg(nor, opcode, buf, len);
> }
>
> -static int spi_nor_controller_ops_erase(struct spi_nor *nor, loff_t offs)
> +int spi_nor_controller_ops_erase(struct spi_nor *nor, loff_t offs)
> {
> if (spi_nor_protocol_is_dtr(nor->write_proto))
> return -EOPNOTSUPP;
> @@ -649,32 +649,7 @@ static int spi_nor_xsr_ready(struct spi_nor *nor)
> return !!(nor->bouncebuf[0] & XSR_RDY);
> }
>
> -/**
> - * 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);
> -}
> +static const struct flash_info *spi_nor_read_id(struct spi_nor *nor);
This forward declaration is not needed. More on this below.
>
> /**
> * spi_nor_sr_ready() - Query the Status Register to see if the flash is ready
> @@ -690,28 +665,6 @@ static int spi_nor_sr_ready(struct spi_nor *nor)
> 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);
> }
>
> @@ -792,18 +745,27 @@ static int spi_nor_fsr_ready(struct spi_nor *nor)
> */
> static int spi_nor_ready(struct spi_nor *nor)
> {
> - int sr, fsr;
> + const struct flash_info *info = nor->info ? nor->info : spi_nor_read_id(nor);
> +
> + if (IS_ERR_OR_NULL(info))
> + return -ENOENT;
nor->info can't be NULL or invalid. The probe would fail in that case
and this function would never get executed. No need to check anything
before using nor->info.
> +
> + /*Chip specific method for querying whether the flash is ready*/
Nitpick: Please add a space after the '*' when starting a comment and
before it when ending a comment. Same for all other comments.
> + if (info->fixups && info->fixups->sr_ready)
> + return info->fixups->sr_ready(nor);
> +
> + /*Manufacture specific method for querying whether the flash is ready*/
s/Manufacture/Manufacturer/
> + if (nor->manufacturer && nor->manufacturer->fixups &&
> + nor->manufacturer->fixups->sr_ready)
> + return nor->manufacturer->fixups->sr_ready(nor);
I don't think nor->info->fixups is the correct place for this hook.
Those should be about fixing up incorrect or missing information about
the flash. It should instead be placed in nor->params.
This eliminates the need for the call to
nor->manufacturer->fixups->sr_ready. Now you can simply call
nor->params->sr_ready(). The fixup hooks will take care of populating it
with the correct function based on the flash or manufacturer.
>
> if (nor->flags & SNOR_F_READY_XSR_RDY)
> - sr = spi_nor_xsr_ready(nor);
> - else
> - 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_xsr_ready(nor);
> + if (nor->flags & SNOR_F_USE_FSR)
> + return spi_nor_fsr_ready(nor);
> +
> + /*Common method for querying whether the flash is ready*/
> + return spi_nor_sr_ready(nor);
You are no longer taking into account the FSR status correctly. If a
flash specifies the USE_FSR flag, then spi_nor_fsr_ready() directly
returns and spi_nor_sr_ready() is not consulted at all. But if the flash
then populates the sr_ready() hook then it directly the result of
sr_ready() and spi_nor_fsr_ready() is not consulted at all.
> }
>
> /**
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index d631ee299de3..36356f27ee92 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -253,6 +253,7 @@ struct spi_nor_flash_parameter {
> * parameters that could not be extracted by other means (i.e.
> * when information provided by the SFDP/flash_info tables are
> * incomplete or wrong).
> + * @sr_ready: special method for querying whether a flash chip is ready.
There is nothing "special" about this. Simply saying "queries whether a
flash chip is ready or not" should be fine.
> *
> * Those hooks can be used to tweak the SPI NOR configuration when the SFDP
> * table is broken or not available.
> @@ -264,6 +265,7 @@ struct spi_nor_fixups {
> const struct sfdp_bfpt *bfpt,
> struct spi_nor_flash_parameter *params);
> void (*post_sfdp)(struct spi_nor *nor);
> + int (*sr_ready)(struct spi_nor *nor);
> };
>
> struct flash_info {
> @@ -471,6 +473,12 @@ int spi_nor_post_bfpt_fixups(struct spi_nor *nor,
> const struct sfdp_bfpt *bfpt,
> struct spi_nor_flash_parameter *params);
>
> +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);
> +int spi_nor_controller_ops_erase(struct spi_nor *nor, loff_t offs);
> +
> static struct spi_nor __maybe_unused *mtd_to_spi_nor(struct mtd_info *mtd)
> {
> return mtd->priv;
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index b0c5521c1e27..67619b64c148 100644
> --- a/drivers/mtd/spi-nor/spansion.c
> +++ b/drivers/mtd/spi-nor/spansion.c
> @@ -18,6 +18,7 @@
> #define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN 0x3
> #define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS 0
> #define SPINOR_OP_CYPRESS_RD_FAST 0xee
> +#define SPINOR_OP_SPANSION_CLSR 0x30
>
> /**
> * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
> @@ -175,6 +176,74 @@ static struct spi_nor_fixups s28hs512t_fixups = {
> .post_bfpt = s28hs512t_post_bfpt_fixup,
> };
>
> +/**
> + * spansion_clear_sr() - Clear the Status Register.
> + * @nor: pointer to 'struct spi_nor'.
> + */
> +static void spansion_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_SPANSION_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_SPANSION_CLSR,
> + NULL, 0);
> + }
> +
> + if (ret)
> + dev_dbg(nor->dev, "error %d clearing SR\n", ret);
> +}
Ok.
> +
> +/**
> + * spansion_sr_ready() - Spansion specific method for querying the flash to
> + * see if it is ready for new commands.
> + * @nor: pointer to 'struct spi_nor'.
> + *
> + * Return: 1 if ready, 0 if not ready, -errno on errors.
> + */
> +static int spansion_sr_ready(struct spi_nor *nor)
> +{
> + u8 *sr = nor->bouncebuf;
> + int ret;
> +
> + ret = spi_nor_read_sr(nor, sr);
> + 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");
> +
> + spansion_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 !(sr[0] & SR_WIP);
> +}
> +
Ok.
> static int
> s25fs_s_post_bfpt_fixups(struct spi_nor *nor,
> const struct sfdp_parameter_header *bfpt_header,
> @@ -291,6 +360,7 @@ static void spansion_post_sfdp_fixups(struct spi_nor *nor)
>
> static const struct spi_nor_fixups spansion_fixups = {
> .post_sfdp = spansion_post_sfdp_fixups,
> + .sr_ready = spansion_sr_ready,
> };
>
> const struct spi_nor_manufacturer spi_nor_spansion = {
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index d13958de6d8a..43bd66204fdf 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -100,7 +100,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 */
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
More information about the linux-mtd
mailing list