Re: [PATCH v10 07/17] mtd: spi-nor: sfdp: parse xSPI Profile 1.0 table

From: Tudor.Ambarus
Date: Wed Jul 08 2020 - 12:01:35 EST

On 6/23/20 9:30 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> This table is indication that the flash is xSPI compliant and hence
> supports octal DTR mode. Extract information like the fast read opcode,
> dummy cycles, the number of dummy cycles needed for a Read Status
> Register command, and the number of address bytes needed for a Read
> Status Register command.
>
> We don't know what speed the controller is running at. Find the fast
> read dummy cycles for the fastest frequency the flash can run at to be
> sure we are never short of dummy cycles. If nothing is available,
> default to 20. Flashes that use a different value should update it in
> their fixup hooks.
>
> Since we want to set read settings, expose spi_nor_set_read_settings()
> in core.h.
>
> Signed-off-by: Pratyush Yadav <p.yadav@xxxxxx>
> ---
> drivers/mtd/spi-nor/core.c | 2 +-
> drivers/mtd/spi-nor/core.h | 10 ++++
> drivers/mtd/spi-nor/sfdp.c | 98 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 109 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 22a3832b83a6..7d24e63fcca8 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2355,7 +2355,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 de1e3917889f..7e6df8322da0 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -192,6 +192,9 @@ struct spi_nor_locking_ops {
> *
> * @size: the flash memory density in bytes.
> * @page_size: the page size of the SPI NOR flash memory.
> + * @rdsr_dummy: dummy cycles needed for Read Status Register command.
> + * @rdsr_addr_nbytes: dummy address bytes needed for Read Status Register
> + * command.
> * @hwcaps: describes the read and page program hardware
> * capabilities.
> * @reads: read capabilities ordered by priority: the higher index
> @@ -214,6 +217,8 @@ struct spi_nor_locking_ops {
> struct spi_nor_flash_parameter {
> u64 size;
> u32 page_size;
> + u8 rdsr_dummy;
> + u8 rdsr_addr_nbytes;
>
> struct spi_nor_hwcaps hwcaps;
> struct spi_nor_read_command reads[SNOR_CMD_READ_MAX];
> @@ -424,6 +429,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 3f709de5ea67..d5a24e61813c 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -4,12 +4,15 @@
> * Copyright (C) 2014, Freescale Semiconductor, Inc.
> */
>
> +#include <linux/bitfield.h>
> #include <linux/slab.h>
> #include <linux/sort.h>
> #include <linux/mtd/spi-nor.h>
>
> #include "core.h"
>
> +#define ROUND_UP_TO(x, y) (((x) + (y) - 1) / (y) * (y))

you can use round_up()

> +
> #define SFDP_PARAM_HEADER_ID(p) (((p)->id_msb << 8) | (p)->id_lsb)
> #define SFDP_PARAM_HEADER_PTP(p) \
> (((p)->parameter_table_pointer[2] << 16) | \
> @@ -19,6 +22,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
>
> @@ -66,6 +70,16 @@ struct sfdp_bfpt_erase {
> u32 shift;
> };
>
> +/* xSPI Profile 1.0 table (from JESD216D.01). */
> +#define PROFILE1_DWORD1_RD_FAST_CMD GENMASK(15, 8)
> +#define PROFILE1_DWORD1_RDSR_DUMMY BIT(28)
> +#define PROFILE1_DWORD1_RDSR_ADDR_BYTES BIT(29)
> +#define PROFILE1_DWORD4_DUMMY_200MHZ GENMASK(11, 7)
> +#define PROFILE1_DWORD5_DUMMY_166MHZ GENMASK(31, 27)
> +#define PROFILE1_DWORD5_DUMMY_133MHZ GENMASK(21, 17)
> +#define PROFILE1_DWORD5_DUMMY_100MHZ GENMASK(11, 7)

we should order these macros in a consistent way. I see that previous macros
are declared in order starting from MSB to LSB.

> +#define PROFILE1_DUMMY_DEFAULT 20

we need to explain why the default dummy value is 20.

How about declaring all these macros immediately above of spi_nor_parse_profile1()?

> +
> #define SMPT_CMD_ADDRESS_LEN_MASK GENMASK(23, 22)
> #define SMPT_CMD_ADDRESS_LEN_0 (0x0UL << 22)
> #define SMPT_CMD_ADDRESS_LEN_3 (0x1UL << 22)
> @@ -1106,6 +1120,86 @@ static int spi_nor_parse_4bait(struct spi_nor *nor,
> return ret;
> }
>
> +/**
> + * spi_nor_parse_profile1() - parse the xSPI Profile 1.0 table
> + * @nor: pointer to a 'struct spi_nor'
> + * @param_header: pointer to the 'struct sfdp_parameter_header' describing
> + * the 4-Byte Address Instruction Table length and version.
> + * @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 *profile1_header,
> + struct spi_nor_flash_parameter *params)
> +{
> + u32 *table, opcode, addr;

s/table/dwords?

u8 opcode?

> + size_t len;
> + int ret, i;
> + u8 dummy;
> +
> + len = profile1_header->length * sizeof(*table);
> + table = kmalloc(len, GFP_KERNEL);
> + if (!table)
> + return -ENOMEM;
> +
> + addr = SFDP_PARAM_HEADER_PTP(profile1_header);
> + ret = spi_nor_read_sfdp(nor, addr, len, table);
> + if (ret)
> + goto out;
> +
> + /* Fix endianness of the table DWORDs. */
> + for (i = 0; i < profile1_header->length; i++)
> + table[i] = le32_to_cpu(table[i]);

le32_to_cpu_array(table, profile1_header->length);

> +
> + /* Get 8D-8D-8D fast read opcode and dummy cycles. */
> + opcode = FIELD_GET(PROFILE1_DWORD1_RD_FAST_CMD, table[0]);
> +
> + /*
> + * We don't know what speed the controller is running at. Find the
> + * dummy cycles for the fastest frequency the flash can run at to be
> + * sure we are never short of dummy cycles. A value of 0 means the
> + * frequency is not supported.
> + *
> + * Default to PROFILE1_DUMMY_DEFAULT if we don't find anything, and let
> + * flashes set the correct value if needed in their fixup hooks.
> + */
> + dummy = FIELD_GET(PROFILE1_DWORD4_DUMMY_200MHZ, table[3]);
> + if (!dummy)
> + dummy = FIELD_GET(PROFILE1_DWORD5_DUMMY_166MHZ, table[4]);
> + if (!dummy)
> + dummy = FIELD_GET(PROFILE1_DWORD5_DUMMY_133MHZ, table[4]);
> + if (!dummy)
> + dummy = FIELD_GET(PROFILE1_DWORD5_DUMMY_100MHZ, table[4]);
> + if (!dummy)
> + dummy = PROFILE1_DUMMY_DEFAULT;
> +
> + /* Round up to an even value to avoid tripping controllers up. */
> + dummy = ROUND_UP_TO(dummy, 2);
> +
> + /* Update the fast read settings. */
> + spi_nor_set_read_settings(&params->reads[SNOR_CMD_READ_8_8_8_DTR],
> + 0, dummy, opcode,
> + SNOR_PROTO_8_8_8_DTR);
> +
> + /*
> + * Set the Read Status Register dummy cycles and dummy address bytes.
> + */

the comment can fit in a single line

> + if (table[0] & PROFILE1_DWORD1_RDSR_DUMMY)

I would move this above, where opcode is parsed from the same dword

> + params->rdsr_dummy = 8;
> + else
> + params->rdsr_dummy = 4;
> +
> + if (table[0] & PROFILE1_DWORD1_RDSR_ADDR_BYTES)
> + params->rdsr_addr_nbytes = 4;
> + else
> + params->rdsr_addr_nbytes = 0;
> +
> +out:
> + kfree(table);
> + return ret;
> +}
> +
> /**
> * spi_nor_parse_sfdp() - parse the Serial Flash Discoverable Parameters.
> * @nor: pointer to a 'struct spi_nor'
> @@ -1207,6 +1301,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;
> }
> --
> 2.27.0
>