[PATCH v4 1/6] mtd: spi-nor: Add manufacturer read id function

[Tudor Ambarus]

Hi, Jaime,

Thanks for the patch! I think we are getting closer to have this
integrated, but I have some concerns that hopefully with your help we'll
address and move forward.

On 08.09.2023 09:42, Jaime Liao wrote:
> From: JaimeLiao <jaimeliao at mxic.com.tw>
> 
> Add manufacturer read id function because of some flash
> may change data format when read id in octal dtr mode.

I'm not convinced such a method is really needed, would you please
elaborate the explanation why it's needed?

I'm looking at the mx66lm1g45g datasheet. From what I see in "Figure 13.
Read Identification (RDID) Sequence (DTR-OPI Mode)" looks like even if
the flash is operated in 8d-8d-8d mode, what the flash actually uses is
a 8d-8d-8s mode for the read id. Each ID byte is sent twice on both
rising and falling edge of the clock, thus behaving like a 8d-8d-8s
protocol.

I see this flash supports 1-1-1, 8-8-8, and 8d-8d-8d, there are no mixed
modes supported, thus a 8d-8d-8s mode seems just like a hardware bug to
me. So my proposal is to leave the core away and to handle the read id
hack just in the macronix driver.

> 
> Signed-off-by: JaimeLiao <jaimeliao at mxic.com.tw>
> ---
>  drivers/mtd/spi-nor/core.c     | 30 ++++++++++++++++++++++++++++--
>  drivers/mtd/spi-nor/core.h     |  6 ++++++
>  drivers/mtd/spi-nor/macronix.c | 18 ++++++++++++++++++
>  3 files changed, 52 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 1b0c6770c14e..7ee624b16e17 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -408,7 +408,7 @@ int spi_nor_write_disable(struct spi_nor *nor)
>  }
>  
>  /**
> - * spi_nor_read_id() - Read the JEDEC ID.
> + * spi_nor_default_read_id() - Read the JEDEC ID.
>   * @nor:	pointer to 'struct spi_nor'.
>   * @naddr:	number of address bytes to send. Can be zero if the operation
>   *		does not need to send an address.
> @@ -420,7 +420,7 @@ int spi_nor_write_disable(struct spi_nor *nor)
>   *
>   * Return: 0 on success, -errno otherwise.
>   */
> -int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id,
> +int spi_nor_default_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id,
>  		    enum spi_nor_protocol proto)
>  {
>  	int ret;
> @@ -438,6 +438,32 @@ int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id,
>  	return ret;
>  }
>  
> +/**
> + * spi_nor_read_id() - Read ID by manufacturer read id function.
> + * @nor:	pointer to 'struct spi_nor'.
> + * @naddr:	number of address bytes to send. Can be zero if the operation
> + *		does not need to send an address.
> + * @ndummy:	number of dummy bytes to send after an opcode or address. Can
> + *		be zero if the operation does not require dummy bytes.
> + * @id:		pointer to a DMA-able buffer where the value of the JEDEC ID
> + *		will be written.
> + * @proto:	the SPI protocol for register operation.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id,
> +		    enum spi_nor_protocol proto)
> +{
> +	int ret;
> +
> +	if (nor->manufacturer && nor->manufacturer->fixups && nor->manufacturer->fixups->read_id)
> +		ret = nor->manufacturer->fixups->read_id(nor, naddr, ndummy, id, proto);
> +	else
> +		ret = spi_nor_default_read_id(nor, naddr, ndummy, id, proto);
> +
> +	return ret;
> +}
> +
>  /**
>   * spi_nor_read_sr() - Read the Status Register.
>   * @nor:	pointer to 'struct spi_nor'.
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 9217379b9cfe..92cbc2d3f7fe 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -424,6 +424,8 @@ struct spi_nor_flash_parameter {
>   * @late_init: used to initialize flash parameters that are not declared in the
>   *             JESD216 SFDP standard, or where SFDP tables not defined at all.
>   *             Will replace the default_init() hook.
> + * @read_id:   used to read id which may change format after enter into
> +	       octal dtr mode.
>   *
>   * Those hooks can be used to tweak the SPI NOR configuration when the SFDP
>   * table is broken or not available.
> @@ -435,6 +437,8 @@ struct spi_nor_fixups {
>  			 const struct sfdp_bfpt *bfpt);
>  	int (*post_sfdp)(struct spi_nor *nor);
>  	int (*late_init)(struct spi_nor *nor);
> +	int (*read_id)(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id,
> +		       enum spi_nor_protocol reg_proto);
>  };
>  
>  /**
> @@ -667,6 +671,8 @@ void spi_nor_unlock_and_unprep(struct spi_nor *nor);
>  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_default_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id,
> +			    enum spi_nor_protocol reg_proto);
>  int spi_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id,
>  		    enum spi_nor_protocol reg_proto);
>  int spi_nor_read_sr(struct spi_nor *nor, u8 *sr);
> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> index eb149e517c1f..8ab47691dfbb 100644
> --- a/drivers/mtd/spi-nor/macronix.c
> +++ b/drivers/mtd/spi-nor/macronix.c
> @@ -118,9 +118,27 @@ static int macronix_nor_late_init(struct spi_nor *nor)
>  	return 0;
>  }
>  
> +static int macronix_nor_read_id(struct spi_nor *nor, u8 naddr, u8 ndummy, u8 *id,
> +				enum spi_nor_protocol proto)
> +{
> +	int i, ret;
> +
> +	ret = spi_nor_default_read_id(nor, naddr, ndummy, id, proto);
> +	/* Retrieve odd array and re-sort id because of read id format will be A-A-B-B-C-C
> +	 * after enter into octal dtr mode for Macronix flashes.
> +	 */
> +	if (proto == SNOR_PROTO_8_8_8_DTR) {
> +		for (i = 0; i < nor->info->id_len; i++)
> +			id[i] = id[i*2];

why do you overwrite the id? How about just checking that
id[i] == id[i + 1]? why do you care if you print an a-a-b-b-c-c id?
> +	}
> +
> +	return ret;
> +}
> +
>  static const struct spi_nor_fixups macronix_nor_fixups = {
>  	.default_init = macronix_nor_default_init,
>  	.late_init = macronix_nor_late_init,
> +	.read_id = macronix_nor_read_id,
>  };
>  
>  const struct spi_nor_manufacturer spi_nor_macronix = {