Re: [PATCH v4 2/7] mtd: spi-nor: sfdp: parse xSPI Profile 1.0 table

[<Tudor.Ambarus@xxxxxxxxxxxxx>]

Hi, Mason, YC Lin,

On 5/29/20 10:36 AM, Mason Yang wrote:
> JESD251, xSPI profile 1.0 table supports octal DTR mode.
> Extract information like the fast read opcode, dummy cycles for various
> frequencies, the number of dummy cycles needed for a Read Status
> Register command, the number of address bytes needed for a Read
> Status Register command, read volatile register command and write
> volatile register command.
> 
> According to BFPT 20th DWORD of octal maximum speed, driver get it's
> specific dummy cycles from profile 1.0 table and then could update
> it to device by their fixup hooks.
> 
> Since driver get octal DTR read opcode and then set read settings,
> expose spi_nor_set_read_settings() in core.h.
> 
> Signed-off-by: Mason Yang <masonccyang@xxxxxxxxxxx>
> ---
>  drivers/mtd/spi-nor/core.c |   2 +-
>  drivers/mtd/spi-nor/core.h |  16 +++++++
>  drivers/mtd/spi-nor/sfdp.c | 106 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 123 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 1ab4386..3799417 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2204,7 +2204,7 @@ static int spi_nor_check(struct spi_nor *nor)
>  	return 0;
>  }
>  
> -static void
> +void
>  spi_nor_set_read_settings(struct spi_nor_read_command *read,
>  			  u8 num_mode_clocks,
>  			  u8 num_wait_states,
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 7a36b22..a33f807 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -191,6 +191,12 @@ struct spi_nor_locking_ops {
>   * @size:		the flash memory density in bytes.
>   * @page_size:		the page size of the SPI NOR flash memory.
>   * @octal_max_speed:	maximum operation speed of device in octal mode.
> + * @rdsr_dummy:		dummy cycles needed for Read Status Register command.
> + * @rdsr_addr_nbytes:	dummy address bytes needed for Read Status Register
> + *			command.
> + * @rd_reg_cmd:		read volatile register command for xSPI device.
> + * @wr_reg_cmd:		write volatile register command for xSPI device.

Can all the volatile registers be accessed with these commands?
The profile1.0 table lacks description.

> + * @dummy_cycles:	dummy cycles used for various frequencies
>   * @hwcaps:		describes the read and page program hardware
>   *			capabilities.
>   * @reads:		read capabilities ordered by priority: the higher index
> @@ -214,6 +220,11 @@ struct spi_nor_flash_parameter {
>  	u64				size;
>  	u32				page_size;
>  	u16				octal_max_speed;
> +	u8				rdsr_dummy;
> +	u8				rdsr_addr_nbytes;
> +	u8				rd_reg_cmd;
> +	u8				wr_reg_cmd;
> +	u8				dummy_cycles;
>  
>  	struct spi_nor_hwcaps		hwcaps;
>  	struct spi_nor_read_command	reads[SNOR_CMD_READ_MAX];
> @@ -420,6 +431,11 @@ ssize_t spi_nor_write_data(struct spi_nor *nor, loff_t to, size_t len,
>  
>  int spi_nor_hwcaps_read2cmd(u32 hwcaps);
>  u8 spi_nor_convert_3to4_read(u8 opcode);
> +void spi_nor_set_read_settings(struct spi_nor_read_command *read,
> +			       u8 num_mode_clocks,
> +			       u8 num_wait_states,
> +			       u8 opcode,
> +			       enum spi_nor_protocol proto);
>  void spi_nor_set_pp_settings(struct spi_nor_pp_command *pp, u8 opcode,
>  			     enum spi_nor_protocol proto);
>  
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index 4d13f66..27a4de4 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -20,6 +20,7 @@
>  #define SFDP_BFPT_ID		0xff00	/* Basic Flash Parameter Table */
>  #define SFDP_SECTOR_MAP_ID	0xff81	/* Sector Map Table */
>  #define SFDP_4BAIT_ID		0xff84  /* 4-byte Address Instruction Table */
> +#define SFDP_PROFILE1_ID	0xff05	/* xSPI Profile 1.0 table. */
>  
>  #define SFDP_SIGNATURE		0x50444653U
>  #define SFDP_JESD216_MAJOR	1
> @@ -27,6 +28,27 @@
>  #define SFDP_JESD216A_MINOR	5
>  #define SFDP_JESD216B_MINOR	6
>  
> +/* xSPI Profile 1.0 table (from JESD216D.01). */
> +#define XSPI_PF1_DWORD1_RD_CMD                  GENMASK(15, 8)
> +#define XSPI_PF1_DWORD1_RDSR_ADDR_BYTES         BIT(29)
> +#define XSPI_PF1_DWORD1_RDSR_DUMMY_CYCLES       BIT(28)
> +#define XSPI_PF1_DWORD2_RD_REG_CMD              GENMASK(31, 24)
> +#define XSPI_PF1_DWORD2_WR_REG_CMD              GENMASK(15, 8)
> +#define XSPI_PF1_DUMMY_CYCLES_DEFAULT		20
> +#define XSPI_DWORD(x)   ((x) - 1)
> +#define XSPI_DWORD_MAX  5
> +
> +struct sfdp_xspi {
> +	u32 dwords[XSPI_DWORD_MAX];
> +};
> +
> +struct xspi_dummy_cycles {
> +	u16 speed_hz;	/* Speed MHz */
> +	u8 dwords;	/* Dwords index */
> +	u32 mask;	/* Mask */
> +	u8 shift;	/* Bit shift */
> +};
> +
>  /* Basic Flash Parameter Table 20th DWORD, Max operation speed of device */
>  struct octal_max_speed {
>  	u8 idx; /* Bits value */
> @@ -1117,6 +1139,86 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
>  }
>  
>  /**
> + * spi_nor_parse_profile1() - parse the xSPI Profile 1.0 table
> + * @nor:		pointer to a 'struct spi_nor'
> + * @param_header:	xspi profile 1 parameter table header
> + * @params:		pointer to the 'struct spi_nor_flash_parameter' to be.
> + *
> + * Return: 0 on success, -errno otherwise.
> + */
> +static int spi_nor_parse_profile1(struct spi_nor *nor,
> +				  const struct sfdp_parameter_header *header,
> +				  struct spi_nor_flash_parameter *params)
> +{
> +	struct sfdp_xspi pfile1;
> +	u8 opcode;
> +	u32 i, addr;
> +	size_t len;
> +	int ret;
> +	static const struct xspi_dummy_cycles dummy[] = {
> +		/* {MHz, Dwords index, Mask, Bit shift} */
> +		{ 200, 4, GENMASK(11, 7),   7},
> +		{ 166, 5, GENMASK(31, 27), 27},
> +		{ 133, 5, GENMASK(21, 17), 17},
> +		{ 100, 5, GENMASK(11, 7),   7},
> +	};
> +
> +	if (header->major != SFDP_JESD216_MAJOR ||
> +	    header->length < XSPI_DWORD_MAX)
> +		return -EINVAL;
> +
> +	len = min_t(size_t, sizeof(pfile1),
> +		    header->length * sizeof(u32));
> +
> +	memset(&pfile1, 0, sizeof(pfile1));
> +
> +	addr = SFDP_PARAM_HEADER_PTP(header);
> +	ret = spi_nor_read_sfdp(nor, addr, len, &pfile1);
> +	if (ret)
> +		goto out;
> +
> +	/* Fix endianness of the xSPI 1.0 DWORDs. */
> +	le32_to_cpu_array(pfile1.dwords, XSPI_DWORD_MAX);
> +
> +	/* Get 8D-8D-8D fast read opcode and dummy cycles. */
> +	opcode = FIELD_GET(XSPI_PF1_DWORD1_RD_CMD,
> +			   pfile1.dwords[XSPI_DWORD(1)]);
> +
> +	if (pfile1.dwords[XSPI_DWORD(1)] & XSPI_PF1_DWORD1_RDSR_ADDR_BYTES)
> +		params->rdsr_addr_nbytes = 4;
> +	else
> +		params->rdsr_addr_nbytes = 0;
> +
> +	if (pfile1.dwords[XSPI_DWORD(1)] & XSPI_PF1_DWORD1_RDSR_DUMMY_CYCLES)
> +		params->rdsr_dummy = 8;
> +	else
> +		params->rdsr_dummy = 4;
> +
> +	params->rd_reg_cmd = FIELD_GET(XSPI_PF1_DWORD2_RD_REG_CMD,
> +				       pfile1.dwords[XSPI_DWORD(2)]);
> +	params->wr_reg_cmd = FIELD_GET(XSPI_PF1_DWORD2_WR_REG_CMD,
> +				       pfile1.dwords[XSPI_DWORD(2)]);
> +
> +	/* according to BFPT 20th DOWRD to get devices dummy cycles */
> +	for (i = 0; i < ARRAY_SIZE(dummy); i++) {
> +		if (params->octal_max_speed == dummy[i].speed_hz) {

Why do you introduced this restriction? The dummy cycles fields themselves
can be used to determine if a speed is supported or not. A value different
than zero for the dummy cycles fields means that the corresponding frequency
is supported.

Cheers,
ta


> +			params->dummy_cycles = (dummy[i].mask &
> +				pfile1.dwords[XSPI_DWORD(dummy[i].dwords)]) >>
> +				dummy[i].shift;
> +			break;
> +		}
> +	}
> +	if (i == ARRAY_SIZE(dummy))
> +		params->dummy_cycles = XSPI_PF1_DUMMY_CYCLES_DEFAULT;
> +
> +	spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_8_8_8_DTR],
> +				  0, params->dummy_cycles,
> +				  opcode, SNOR_PROTO_8_8_8_DTR);
> +out:
> +	return ret;
> +}
> +
> +/**
>   * spi_nor_parse_sfdp() - parse the Serial Flash Discoverable Parameters.
>   * @nor:		pointer to a 'struct spi_nor'
>   * @params:		pointer to the 'struct spi_nor_flash_parameter' to be
> @@ -1217,6 +1319,10 @@ int spi_nor_parse_sfdp(struct spi_nor *nor,
>  			err = spi_nor_parse_4bait(nor, param_header, params);
>  			break;
>  
> +		case SFDP_PROFILE1_ID:
> +			err = spi_nor_parse_profile1(nor, param_header, params);
> +			break;
> +
>  		default:
>  			break;
>  		}
>