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 <[email protected]> > --- > 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
