linux-kernel - Re: [PATCH v4 28/32] mtd: spi-nor: move all spansion specifics into spansion.c

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220223115923.rxvpuuaj4dkdrxdl@ti.com>
Date:   Wed, 23 Feb 2022 17:29:23 +0530
From:   Pratyush Yadav <p.yadav@...com>
To:     Michael Walle <michael@...le.cc>
CC:     <linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        Tudor Ambarus <tudor.ambarus@...rochip.com>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Richard Weinberger <richard@....at>,
        Vignesh Raghavendra <vigneshr@...com>,
        <yaliang.wang@...driver.com>
Subject: Re: [PATCH v4 28/32] mtd: spi-nor: move all spansion specifics into
 spansion.c

On 21/02/22 01:08PM, Michael Walle wrote:
> 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@...le.cc>
> ---
>  drivers/mtd/spi-nor/core.c     | 49 ------------------------
>  drivers/mtd/spi-nor/spansion.c | 70 ++++++++++++++++++++++++++++++++++
>  include/linux/mtd/spi-nor.h    |  1 -
>  3 files changed, 70 insertions(+), 50 deletions(-)
> 
[...]
> diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
> index 1a0e7214d9e5..38eeb0913ab2 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_nor_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.

Nitpick: "... and clear it if there are any errors."

Looks good otherwise.

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

> + * @nor:	pointer to 'struct spi_nor'.
> + *
> + * Return: 1 if ready, 0 if not ready, -errno on errors.
> + */
> +static 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_nor_late_init(struct spi_nor *nor)
>  {
>  	if (nor->params->size > SZ_16M) {
> @@ -302,6 +369,9 @@ static void spansion_nor_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_nor_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
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.