RE: [PATCH v5 3/7] mtd: spi-nor: add opcodes for octal Read/Write commands

From: Yogesh Narayan Gaur
Date: Mon Dec 10 2018 - 23:55:44 EST

Hi Tudor,

> -----Original Message-----
> From: Tudor.Ambarus@xxxxxxxxxxxxx [mailto:Tudor.Ambarus@xxxxxxxxxxxxx]
> Sent: Monday, December 10, 2018 4:15 PM
> To: Yogesh Narayan Gaur <yogeshnarayan.gaur@xxxxxxx>; linux-
> mtd@xxxxxxxxxxxxxxxxxxx; boris.brezillon@xxxxxxxxxxx; broonie@xxxxxxxxxx;
> marek.vasut@xxxxxxxxx; vigneshr@xxxxxx; linux-spi@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx
> Cc: robh@xxxxxxxxxx; mark.rutland@xxxxxxx; shawnguo@xxxxxxxxxx; linux-
> arm-kernel@xxxxxxxxxxxxxxxxxxx; computersforpeace@xxxxxxxxx;
> frieder.schrempf@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v5 3/7] mtd: spi-nor: add opcodes for octal Read/Write
> commands
>
> Hi, Yogesh,
>
> On 12/03/2018 10:39 AM, Yogesh Narayan Gaur wrote:
> > - Add opcodes for octal I/O commands
> > * Read : 1-1-8 and 1-8-8 protocol
> > * Write : 1-1-8 and 1-8-8 protocol
> > * opcodes for 4-byte address mode command
> >
> > - Entry of macros in _convert_3to4_xxx function
> >
> > - Add flag specifying flash support octal read commands.
> >
> > Signed-off-by: Vignesh R <vigneshr@xxxxxx>
> > Signed-off-by: Yogesh Gaur <yogeshnarayan.gaur@xxxxxxx>
> > ---
> > Changes for v5:
> > - Modified string 'octo' with 'octal'.
> > Changes for v4:
> > - None
> > Changes for v3:
> > - Modified string 'octal' with 'octo'.
> > Changes for v2:
> > - Incorporated review comments of Boris and Vignesh
> >
> > drivers/mtd/spi-nor/spi-nor.c | 16 ++++++++++++++--
> > include/linux/mtd/spi-nor.h | 8 ++++++++
> > 2 files changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c
> > b/drivers/mtd/spi-nor/spi-nor.c index 398d273..7a2176d 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -90,6 +90,7 @@ struct flash_info {
> > #define NO_CHIP_ERASE BIT(12) /* Chip does not support chip
> erase */
> > #define SPI_NOR_SKIP_SFDP BIT(13) /* Skip parsing of SFDP tables */
> > #define USE_CLSR BIT(14) /* use CLSR command */
> > +#define SPI_NOR_OCTAL_READ BIT(15) /* Flash supports Octal Read */
> >
> > int (*quad_enable)(struct spi_nor *nor);
> > };
> > @@ -209,6 +210,8 @@ 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_8, SPINOR_OP_READ_1_1_8_4B },
> > + { SPINOR_OP_READ_1_8_8, SPINOR_OP_READ_1_8_8_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 },
> > @@ -225,6 +228,8 @@ static inline u8 spi_nor_convert_3to4_program(u8
> opcode)
> > { SPINOR_OP_PP, SPINOR_OP_PP_4B },
> > { SPINOR_OP_PP_1_1_4, SPINOR_OP_PP_1_1_4_4B },
> > { SPINOR_OP_PP_1_4_4, SPINOR_OP_PP_1_4_4_4B },
> > + { SPINOR_OP_PP_1_1_8, SPINOR_OP_PP_1_1_8_4B },
> > + { SPINOR_OP_PP_1_8_8, SPINOR_OP_PP_1_8_8_4B },
> > };
> >
> > return spi_nor_convert_opcode(opcode, spi_nor_3to4_program, @@
> > -2093,7 +2098,7 @@ enum spi_nor_read_command_index {
> > SNOR_CMD_READ_4_4_4,
> > SNOR_CMD_READ_1_4_4_DTR,
> >
> > - /* Octo SPI */
> > + /* Octal SPI */
> > SNOR_CMD_READ_1_1_8,
> > SNOR_CMD_READ_1_8_8,
> > SNOR_CMD_READ_8_8_8,
> > @@ -2110,7 +2115,7 @@ enum spi_nor_pp_command_index {
> > SNOR_CMD_PP_1_4_4,
> > SNOR_CMD_PP_4_4_4,
> >
> > - /* Octo SPI */
> > + /* Octal SPI */
> > SNOR_CMD_PP_1_1_8,
> > SNOR_CMD_PP_1_8_8,
> > SNOR_CMD_PP_8_8_8,
> > @@ -3195,6 +3200,13 @@ static int spi_nor_init_params(struct spi_nor *nor,
> > SNOR_PROTO_1_1_4);
> > }
> >
> > + if (info->flags & SPI_NOR_OCTAL_READ) {
> > + params->hwcaps.mask |= SNOR_HWCAPS_READ_1_1_8;
> > + spi_nor_set_read_settings(&params-
> >reads[SNOR_CMD_READ_1_1_8],
> > + 0, 8, SPINOR_OP_READ_1_1_8,
> > + SNOR_PROTO_1_1_8);
> > + }
> > +> /* Page Program settings. */
> > params->hwcaps.mask |= SNOR_HWCAPS_PP;
> > spi_nor_set_pp_settings(&params->page_programs[SNOR_CMD_PP],
>
> At the end of spi_nor_init_params we check the conditions for parsing the sfdp.
> Shouldn't SPI_NOR_OCTAL_READ trigger the sfdp parsing when
> SPI_NOR_SKIP_SFDP is not set?

I guess for now setting only SPI_NOR_OCTAL_READ didn't trigger SFDP parsing as SFDP parsing would start only when we have both of the below condition are true
if ((info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)) &&
!(info->flags & SPI_NOR_SKIP_SFDP)) {
But for case when we have only set SPI_NOR_OCTAL_READ then first condition check would be false and hence SFDP parsing won't occur.

I guess, we needn't to have SFDP parsing only when SPI_NOR_SKIP_SFDP is set.
Thus would going to modify the condition check as below, so that SFDP parsing would also be performed when SPI_NOR_OCTAL_READ is set. Correct?

if ((info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
SPI_NOR_OCTAL_READ)) && !(info->flags & SPI_NOR_SKIP_SFDP)) {

--
Regards
Yogesh Gaur

>
> > diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> > index 8b1acf6..019f534 100644
> > --- a/include/linux/mtd/spi-nor.h
> > +++ b/include/linux/mtd/spi-nor.h
> > @@ -50,9 +50,13 @@
>
> Can you please s/octo/octal on these as well:
> $ grep -ni octo include/linux/mtd/spi-nor.h
> 471: * As a matter of performances, it is relevant to use Octo SPI protocols first,
> 492:#define SNOR_HWCPAS_READ_OCTO GENMASK(14, 11)
> 501: * Like (Fast) Read capabilities, Octo/Quad SPI protocols are preferred to
> the
> 515:#define SNOR_HWCAPS_PP_OCTO GENMASK(22, 20)
>
> thanks,
> ta