| 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
| ||
|
Message-ID: <0c730ece-cb37-1ce2-dc3c-8fe2f1a63abb@gmail.com>
Date: Mon, 17 Apr 2017 00:26:04 +0200
From: Marek Vasut <marek.vasut@...il.com>
To: Cyrille Pitchen <cyrille.pitchen@...el.com>,
linux-mtd@...ts.infradead.org, kdasu.kdev@...il.com
Cc: computersforpeace@...il.com, dwmw2@...radead.org,
boris.brezillon@...e-electrons.com, richard@....at,
linux-kernel@...r.kernel.org, jartur@...ence.com,
mar.krzeminski@...il.com
Subject: Re: [PATCH v6 1/4] mtd: spi-nor: introduce more SPI protocols and the
Dual Transfer Mode
On 04/16/2017 11:41 PM, Cyrille Pitchen wrote:
> This patch changes the prototype of spi_nor_scan(): its 3rd parameter
> is replaced by a 'struct spi_nor_hwcaps' pointer, which tells the spi-nor
> framework about the actual hardware capabilities supported by the SPI
> controller and its driver.
>
> Besides, this patch also introduces a new 'struct spi_nor_flash_parameter'
> telling the spi-nor framework about the hardware capabilities supported by
> the SPI flash memory and the associated settings required to use those
> hardware caps.
>
> Currently the 'struct spi_nor_flash_parameter' is filled with legacy
> values but a later patch will allow to fill it dynamically by reading the
> JESD216 Serial Flash Discoverable Parameter (SFDP) tables from the SPI
> memory.
>
> With both structures, the spi-nor framework can now compute the best
> match between hardware caps supported by both the (Q)SPI memory and
> controller hence selecting the relevant SPI protocols and op codes for
> (Fast) Read and Page Program operations.
>
> The 'struct spi_nor_flash_parameter' also provides the spi-nor framework
> with the number of dummy cycles to be used with each Fast Read commands.
>
> Finally the 'struct spi_nor_flash_parameter', through the optional
> .quad_enable() hook, tells the spi-nor framework how to set the Quad
> Enable (QE) bit of the QSPI memory to enable its Quad SPI features.
> For now the .quad_enable() hook is set with the exact same functions as
> already used before this patch: the right function was and is still
> chosen only based on the memory manufacturer. In further patches, this
> choice will be made also based on the memory part. Indeed, we already
> know that some manufacturers, like Spansion, have updated their procedure
> to set the QE bit with their latest QSPI memories. The new procedure is
> safer but not supported by the oldest memory parts.
> The SFDP table could be one solution to select the relevant procedure but
> this issue will be addressed in further dedicated patches.
This IMO could've been factored out into separate small patch, so that
this patch isn't such a blob of multiple changes.
Otherwise, minor nits below ...
> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@...el.com>
> ---
> drivers/mtd/devices/m25p80.c | 18 +-
> drivers/mtd/spi-nor/aspeed-smc.c | 23 +-
> drivers/mtd/spi-nor/atmel-quadspi.c | 83 ++++--
> drivers/mtd/spi-nor/cadence-quadspi.c | 18 +-
> drivers/mtd/spi-nor/fsl-quadspi.c | 6 +-
> drivers/mtd/spi-nor/hisi-sfc.c | 31 ++-
> drivers/mtd/spi-nor/intel-spi.c | 7 +-
> drivers/mtd/spi-nor/mtk-quadspi.c | 15 +-
> drivers/mtd/spi-nor/nxp-spifi.c | 22 +-
> drivers/mtd/spi-nor/spi-nor.c | 472 +++++++++++++++++++++++++++-------
> drivers/mtd/spi-nor/stm32-quadspi.c | 27 +-
> include/linux/mtd/spi-nor.h | 158 +++++++++++-
> 12 files changed, 686 insertions(+), 194 deletions(-)
>
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index c4df3b1bded0..34f33f6b88b4 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -111,10 +111,10 @@ static ssize_t m25p80_write(struct spi_nor *nor, loff_t to, size_t len,
>
> static inline unsigned int m25p80_rx_nbits(struct spi_nor *nor)
> {
> - switch (nor->flash_read) {
> - case SPI_NOR_DUAL:
> + switch (nor->read_proto) {
> + case SNOR_PROTO_1_1_2:
> return 2;
> - case SPI_NOR_QUAD:
> + case SNOR_PROTO_1_1_4:
> return 4;
Won't this become something like:
rx_bits = (nor->read_proto >> SNOR_PROTO_INST_SHIFT) & SNOR_PROTO_INST_MASK;
return rx_bits >= 1 ? rx_bits : 0;
And then you don't have to ever touch it once you add new SNOR_PROTO_1_1_x ?
Why do we return 0 by default though ? SPI_NBITS_SINGLE = 1 ... hmmm.
> default:
> return 0;
> @@ -196,7 +196,11 @@ static int m25p_probe(struct spi_device *spi)
> struct flash_platform_data *data;
> struct m25p *flash;
> struct spi_nor *nor;
> - enum read_mode mode = SPI_NOR_NORMAL;
> + struct spi_nor_hwcaps hwcaps = {
> + .mask = SNOR_HWCAPS_READ |
> + SNOR_HWCAPS_READ_FAST |
> + SNOR_HWCAPS_PP,
> + };
> char *flash_name;
> int ret;
[...]
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 36684ca7aa24..3bb2adff306b 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -150,24 +150,6 @@ static int read_cr(struct spi_nor *nor)
> }
>
> /*
> - * Dummy Cycle calculation for different type of read.
> - * It can be used to support more commands with
> - * different dummy cycle requirements.
> - */
> -static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor)
> -{
> - switch (nor->flash_read) {
> - case SPI_NOR_FAST:
> - case SPI_NOR_DUAL:
> - case SPI_NOR_QUAD:
> - return 8;
> - case SPI_NOR_NORMAL:
> - return 0;
> - }
> - return 0;
> -}
> -
> -/*
> * Write status register 1 byte
> * Returns negative if error occurred.
> */
> @@ -221,6 +203,10 @@ static inline u8 spi_nor_convert_3to4_read(u8 opcode)
> { SPINOR_OP_READ_1_2_2, SPINOR_OP_READ_1_2_2_4B },
> { SPINOR_OP_READ_1_1_4, SPINOR_OP_READ_1_1_4_4B },
> { SPINOR_OP_READ_1_4_4, SPINOR_OP_READ_1_4_4_4B },
> +
> + { SPINOR_OP_READ_1_1_1_DTR, SPINOR_OP_READ_1_1_1_DTR_4B },
> + { SPINOR_OP_READ_1_2_2_DTR, SPINOR_OP_READ_1_2_2_DTR_4B },
> + { SPINOR_OP_READ_1_4_4_DTR, SPINOR_OP_READ_1_4_4_DTR_4B },
If you moved this into separate patch, that patch would be a nice
example for future generations on how to add new protocols ...
> };
>
> return spi_nor_convert_opcode(opcode, spi_nor_3to4_read,
[...]
> +struct spi_nor_flash_parameter {
> + u64 size;
> + u32 page_size;
> +
> + struct spi_nor_hwcaps hwcaps;
> + struct spi_nor_read_command reads[SNOR_CMD_READ_MAX];
> + struct spi_nor_pp_command page_programs[SNOR_CMD_PP_MAX];
> +
> + int (*quad_enable)(struct spi_nor *nor);
> +};
> +
> +
One newline too many here :)
> +static 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)
> +{
> + read->num_mode_clocks = num_mode_clocks;
> + read->num_wait_states = num_wait_states;
> + read->opcode = opcode;
> + read->proto = proto;
> +}
[...]
--
Best regards,
Marek Vasut
Powered by blists - more mailing lists