[PATCH v3 2/3] mtd: cfi_cmdset_0002: Move and rename chip_check/chip_ready/chip_good_for_write

[Miquel Raynal]

Hi Tokunori,

ikegami.t at gmail.com wrote on Wed, 16 Mar 2022 01:56:06 +0900:

> This is a preparation patch for the functinal change to fix the issue.

				      functional

Commits are independent "to fix the issue" does not make any sense
here, please give more information like "in preparation to a change
expected to fix the buffered writes on S29GL..."

> 
> Signed-off-by: Tokunori Ikegami <ikegami.t at gmail.com>
> Cc: Miquel Raynal <miquel.raynal at bootlin.com>
> Cc: Richard Weinberger <richard at nod.at>
> Cc: Vignesh Raghavendra <vigneshr at ti.com>
> Cc: linux-mtd at lists.infradead.org
> ---
>  drivers/mtd/chips/cfi_cmdset_0002.c | 82 +++++++++++++----------------
>  1 file changed, 38 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c
> index 0125658a1d30..8f3f0309dc03 100644
> --- a/drivers/mtd/chips/cfi_cmdset_0002.c
> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c
> @@ -802,22 +802,12 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd)
>  	return NULL;
>  }
>  
> -/*
> - * Return true if the chip is ready.
> - *
> - * Ready is one of: read mode, query mode, erase-suspend-read mode (in any
> - * non-suspended sector) and is indicated by no toggle bits toggling.
> - *
> - * Note that anything more complicated than checking if no bits are toggling
> - * (including checking DQ5 for an error status) is tricky to get working
> - * correctly and is therefore not done	(particularly with interleaved chips
> - * as each chip must be checked independently of the others).
> - */
> -static int __xipram chip_ready(struct map_info *map, struct flchip *chip,
> -			       unsigned long addr)
> +static int __xipram chip_check(struct map_info *map, struct flchip *chip,
> +			       unsigned long addr, map_word *expected)
>  {
>  	struct cfi_private *cfi = map->fldrv_priv;
> -	map_word d, t;
> +	map_word oldd, curd;
> +	int ret;
>  
>  	if (cfi_use_status_reg(cfi)) {
>  		map_word ready = CMD(CFI_SR_DRB);
> @@ -827,17 +817,35 @@ static int __xipram chip_ready(struct map_info *map, struct flchip *chip,
>  		 */
>  		cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi,
>  				 cfi->device_type, NULL);
> -		d = map_read(map, addr);
> +		curd = map_read(map, addr);
>  
> -		return map_word_andequal(map, d, ready, ready);
> +		return map_word_andequal(map, curd, ready, ready);
>  	}
>  
> -	d = map_read(map, addr);
> -	t = map_read(map, addr);
> +	oldd = map_read(map, addr);
> +	curd = map_read(map, addr);
> +
> +	ret = map_word_equal(map, oldd, curd);
>  
> -	return map_word_equal(map, d, t);
> +	if (!ret || !expected)
> +		return ret;
> +
> +	return map_word_equal(map, curd, *expected);
>  }
>  
> +/*
> + * Return true if the chip is ready.
> + *
> + * Ready is one of: read mode, query mode, erase-suspend-read mode (in any
> + * non-suspended sector) and is indicated by no toggle bits toggling.
> + *
> + * Note that anything more complicated than checking if no bits are toggling
> + * (including checking DQ5 for an error status) is tricky to get working
> + * correctly and is therefore not done	(particularly with interleaved chips
> + * as each chip must be checked independently of the others).
> + */
> +#define chip_ready(map, chip, addr) chip_check(map, chip, addr, NULL)
> +
>  /*
>   * Return true if the chip is ready and has the correct value.
>   *
> @@ -856,28 +864,14 @@ static int __xipram chip_ready(struct map_info *map, struct flchip *chip,
>  static int __xipram chip_good(struct map_info *map, struct flchip *chip,
>  			      unsigned long addr, map_word expected)
>  {
> -	struct cfi_private *cfi = map->fldrv_priv;
> -	map_word oldd, curd;
> -
> -	if (cfi_use_status_reg(cfi)) {
> -		map_word ready = CMD(CFI_SR_DRB);
> -
> -		/*
> -		 * For chips that support status register, check device
> -		 * ready bit
> -		 */
> -		cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi,
> -				 cfi->device_type, NULL);
> -		curd = map_read(map, addr);
> -
> -		return map_word_andequal(map, curd, ready, ready);
> -	}
> -
> -	oldd = map_read(map, addr);
> -	curd = map_read(map, addr);
> +	return chip_check(map, chip, addr, &expected);

I must admit that chip_check, chip_good(), chip_ready() are rather poor
names, at least they don't bring a lot of information. Is this part of a
revert? If yes maybe it would be better to actually revert the patch if
it not too complicated? Otherwise I don't really see the point of
having chip_good() calling just chip_check()? And creating a macro for
chip_ready()?

> +}
>  
> -	return	map_word_equal(map, oldd, curd) &&
> -		map_word_equal(map, curd, expected);
> +static int __xipram chip_good_for_write(struct map_info *map,
> +					struct flchip *chip, unsigned long addr,
> +					map_word expected)
> +{
> +	return chip_good(map, chip, addr, expected);
>  }
>  
>  static int get_chip(struct map_info *map, struct flchip *chip, unsigned long adr, int mode)
> @@ -1700,7 +1694,7 @@ static int __xipram do_write_oneword_once(struct map_info *map,
>  		 * "chip_good" to avoid the failure due to scheduling.
>  		 */
>  		if (time_after(jiffies, timeo) &&
> -		    !chip_good(map, chip, adr, datum)) {
> +		    !chip_good_for_write(map, chip, adr, datum)) {
>  			xip_enable(map, chip, adr);
>  			printk(KERN_WARNING "MTD %s(): software timeout\n", __func__);
>  			xip_disable(map, chip, adr);
> @@ -1708,7 +1702,7 @@ static int __xipram do_write_oneword_once(struct map_info *map,
>  			break;
>  		}
>  
> -		if (chip_good(map, chip, adr, datum)) {
> +		if (chip_good_for_write(map, chip, adr, datum)) {
>  			if (cfi_check_err_status(map, chip, adr))
>  				ret = -EIO;
>  			break;
> @@ -1980,14 +1974,14 @@ static int __xipram do_write_buffer_wait(struct map_info *map,
>  		 * "chip_good" to avoid the failure due to scheduling.
>  		 */
>  		if (time_after(jiffies, timeo) &&
> -		    !chip_good(map, chip, adr, datum)) {
> +		    !chip_good_for_write(map, chip, adr, datum)) {
>  			pr_err("MTD %s(): software timeout, address:0x%.8lx.\n",
>  			       __func__, adr);
>  			ret = -EIO;
>  			break;
>  		}
>  
> -		if (chip_good(map, chip, adr, datum)) {
> +		if (chip_good_for_write(map, chip, adr, datum)) {
>  			if (cfi_check_err_status(map, chip, adr))
>  				ret = -EIO;
>  			break;


Thanks,
Miquèl