From mboxrd@z Thu Jan 1 00:00:00 1970 From: Cyrille Pitchen Subject: Re: [PATCH linux-next v2 02/14] mtd: spi-nor: properly detect the memory when it boots in Quad or Dual mode Date: Mon, 11 Jan 2016 14:56:10 +0100 Message-ID: <5693B47A.9050705@atmel.com> References: <20160111110830.605b2d54@bbrezillon> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: In-Reply-To: <20160111110830.605b2d54@bbrezillon> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Boris Brezillon Cc: computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org, marex-ynQEQJNshbs@public.gmane.org, vigneshr-l0cyMroinI0@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, pawel.moll-5wv7dgnIgG8@public.gmane.org, mark.rutland-5wv7dgnIgG8@public.gmane.org, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org, galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org List-Id: devicetree@vger.kernel.org Hi Boris, Le 11/01/2016 11:08, Boris Brezillon a =E9crit : > Hi Cyrille, >=20 > Sorry for this pretty useless review, but I think you know more about > SPI-NOR than I do. So, consider my comments as nitpicks, which should= n't > prevent your series from being applied. >=20 > On Fri, 8 Jan 2016 17:02:14 +0100 > Cyrille Pitchen wrote: >=20 >> The quad (or dual) mode of a spi-nor memory may be enabled at boot t= ime by >> non-volatile bits in some setting register. Also such a mode may hav= e >> already been enabled at early stage by some boot loader. >> >> Hence, we should not guess the spi-nor memory is always configured f= or the >> regular SPI 1-1-1 protocol. >> >> Micron and Macronix memories, once their Quad (or dual for Micron) m= ode >> enabled, no longer process the regular JEDEC Read ID (0x9f) command = but >> instead reply to a new command: JEDEC Read ID Multiple I/O (0xaf). >> Besides, in Quad mode both memory manufacturers expect ALL commands = to >> use the SPI 4-4-4 protocol. For Micron memories, enabling their Dual= mode >> implies to use the SPI 2-2-2 protocol for ALL commands. >> >> Winbond memories, once their Quad mode enabled, expect ALL commands = to use >> the SPI 4-4-4 protocol. Unlike Micron and Macronix memories, they st= ill >> reply to the regular JEDEC Read ID (0x9f) command. >> >> Signed-off-by: Cyrille Pitchen >> --- >> drivers/mtd/spi-nor/spi-nor.c | 83 ++++++++++++++++++++++++++++++++= ++++++++--- >> include/linux/mtd/spi-nor.h | 50 ++++++++++++++++++++++++-- >> 2 files changed, 127 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi= -nor.c >> index 3028c06547c1..8967319ea7da 100644 >> --- a/drivers/mtd/spi-nor/spi-nor.c >> +++ b/drivers/mtd/spi-nor/spi-nor.c >> @@ -73,6 +73,12 @@ struct flash_info { >> =20 >> #define JEDEC_MFR(info) ((info)->id[0]) >> =20 >> +struct read_id_config { >> + enum read_mode mode; >> + enum spi_nor_protocol proto; >> + u8 opcode; >> +}; >> + >> static const struct flash_info *spi_nor_match_id(const char *name); >> =20 >> /* >> @@ -879,11 +885,22 @@ static const struct flash_info spi_nor_ids[] =3D= { >> { }, >> }; >> =20 >> -static const struct flash_info *spi_nor_read_id(struct spi_nor *nor= ) >> +static const struct flash_info *spi_nor_read_id(struct spi_nor *nor= , >> + enum read_mode mode) >> { >> - int tmp; >> + int i, tmp; >> u8 id[SPI_NOR_MAX_ID_LEN]; >> const struct flash_info *info; >> + static const struct read_id_config configs[] =3D { >> + /* Winbond QPI mode */ >> + {SPI_NOR_QUAD, SNOR_PROTO_4_4_4, SPINOR_OP_RDID}, >> + >> + /* Micron Quad mode & Macronix QPI mode */ >> + {SPI_NOR_QUAD, SNOR_PROTO_4_4_4, SPINOR_OP_MIO_RDID}, >> + >> + /* Micron Dual mode */ >> + {SPI_NOR_DUAL, SNOR_PROTO_2_2_2, SPINOR_OP_MIO_RDID} >> + }; >> =20 >> tmp =3D nor->read_reg(nor, SPINOR_OP_RDID, id, SPI_NOR_MAX_ID_LEN)= ; >> if (tmp < 0) { >> @@ -891,6 +908,58 @@ static const struct flash_info *spi_nor_read_id= (struct spi_nor *nor) >> return ERR_PTR(tmp); >> } >> =20 >> + /* >> + * Check whether the SPI NOR memory has already been configured (a= t >> + * reset or by some bootloader) to use a protocol other than SPI 1= -1-1. >> + */ >> + for (i =3D 0; i < ARRAY_SIZE(configs); ++i) { >> + int len =3D SPI_NOR_MAX_ID_LEN; >> + bool is_multi =3D false; >> + >> + /* >> + * Check the latest read Manufacturer ID + Device ID (3 bytes): >> + * if they are different from both 0x000000 and 0xffffff, we >> + * assume that we succeeded in reading a valid JEDEC ID so we >> + * don't need to try other SPI protocols. >> + * Indeed when either the protocol or the op code are not valid, >> + * the SPI NOR memory should not reply to the command. Hence the >> + * SPI I/O lines remain in their default state: 1 when connected >> + * to pull-up resistors or 0 with pull-down. >> + */ >> + if (!((id[0] =3D=3D 0xff && id[1] =3D=3D 0xff && id[2] =3D=3D 0xf= f) || >> + (id[0] =3D=3D 0x00 && id[1] =3D=3D 0x00 && id[2] =3D=3D 0x0= 0))) >> + break; >=20 > Hopefully, you'll have all the pins configured with pull-up or > pull-down resistors, but what if one of them is configured differentl= y? >=20 > Shouldn't we try to find a valid chip definition by iterating over th= e > spi_nor_ids[] table after each mode change? >=20 Honestly I don't know and I didn't test what would happen if no resisto= rs were used. So maybe you're right, it would be safer to look in the ID table = up after every read attempt. >> + >> + /* Only try protocols supported by the user. */ >> + if (configs[i].mode !=3D mode) >> + continue; >> + >> + /* Set this protocol for all commands. */ >> + nor->reg_proto =3D configs[i].proto; >> + nor->read_proto =3D configs[i].proto; >> + nor->write_proto =3D configs[i].proto; >> + nor->erase_proto =3D configs[i].proto; >> + >> + /* >> + * Multiple I/O Read ID only returns the Manufacturer ID >> + * (1 byte) and the Device ID (2 bytes). So we reset the >> + * remaining bytes. >> + */ >> + if (configs[i].opcode =3D=3D SPINOR_OP_MIO_RDID) { >> + is_multi =3D true; >> + len =3D 3; >> + memset(id + len, 0, sizeof(id) - len); >> + } >> + >> + tmp =3D nor->read_reg(nor, configs[i].opcode, id, len); >> + if (tmp < 0) { >> + dev_dbg(nor->dev, >> + "error %d reading JEDEC ID%s\n", >> + tmp, (is_multi ? " Multi I/O" : "")); >> + return ERR_PTR(tmp); >> + } >> + } >> + >> for (tmp =3D 0; tmp < ARRAY_SIZE(spi_nor_ids) - 1; tmp++) { >> info =3D &spi_nor_ids[tmp]; >> if (info->id_len) { >> @@ -1148,11 +1217,17 @@ int spi_nor_scan(struct spi_nor *nor, const = char *name, enum read_mode mode) >> if (ret) >> return ret; >> =20 >> + /* Reset SPI protocol for all commands */ >> + nor->erase_proto =3D SNOR_PROTO_1_1_1; >> + nor->read_proto =3D SNOR_PROTO_1_1_1; >> + nor->write_proto =3D SNOR_PROTO_1_1_1; >> + nor->reg_proto =3D SNOR_PROTO_1_1_1; >> + >> if (name) >> info =3D spi_nor_match_id(name); >> /* Try to auto-detect if chip name wasn't specified or not found *= / >> if (!info) >> - info =3D spi_nor_read_id(nor); >> + info =3D spi_nor_read_id(nor, mode); >> if (IS_ERR_OR_NULL(info)) >> return -ENOENT; >> =20 >> @@ -1163,7 +1238,7 @@ int spi_nor_scan(struct spi_nor *nor, const ch= ar *name, enum read_mode mode) >> if (name && info->id_len) { >> const struct flash_info *jinfo; >> =20 >> - jinfo =3D spi_nor_read_id(nor); >> + jinfo =3D spi_nor_read_id(nor, mode); >> if (IS_ERR(jinfo)) { >> return PTR_ERR(jinfo); >> } else if (jinfo !=3D info) { >> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor= =2Eh >> index 62356d50815b..53932c87bcf2 100644 >> --- a/include/linux/mtd/spi-nor.h >> +++ b/include/linux/mtd/spi-nor.h >> @@ -75,8 +75,9 @@ >> #define SPINOR_OP_BRWR 0x17 /* Bank register write */ >> =20 >> /* Used for Micron flashes only. */ >> -#define SPINOR_OP_RD_EVCR 0x65 /* Read EVCR register */ >> -#define SPINOR_OP_WD_EVCR 0x61 /* Write EVCR register */ >> +#define SPINOR_OP_MIO_RDID 0xaf /* Multiple I/O Read JEDEC ID */ >> +#define SPINOR_OP_RD_EVCR 0x65 /* Read EVCR register */ >> +#define SPINOR_OP_WD_EVCR 0x61 /* Write EVCR register */ >> =20 >> /* Status Register bits. */ >> #define SR_WIP BIT(0) /* Write in progress */ >> @@ -105,6 +106,43 @@ enum read_mode { >> SPI_NOR_QUAD, >> }; >> =20 >> + >=20 > Extra blank line here ^ >=20 >> +#define SNOR_PROTO_CMD_OFF 8 >> +#define SNOR_PROTO_CMD_MASK GENMASK(11, 8) >> +#define SNOR_PROTO_CMD_TO_PROTO(cmd) \ >> + (((cmd) << SNOR_PROTO_CMD_OFF) & SNOR_PROTO_CMD_MASK) >> +#define SNOR_PROTO_CMD_FROM_PROTO(proto) \ >> + ((((u32)(proto)) & SNOR_PROTO_CMD_MASK) >> SNOR_PROTO_CMD_OFF) >> + >> +#define SNOR_PROTO_ADDR_OFF 4 >> +#define SNOR_PROTO_ADDR_MASK GENMASK(7, 4) >> +#define SNOR_PROTO_ADDR_TO_PROTO(addr) \ >> + (((addr) << SNOR_PROTO_ADDR_OFF) & SNOR_PROTO_ADDR_MASK) >> +#define SNOR_PROTO_ADDR_FROM_PROTO(proto) \ >> + ((((u32)(proto)) & SNOR_PROTO_ADDR_MASK) >> SNOR_PROTO_ADDR_OFF) >> + >> +#define SNOR_PROTO_DATA_OFF 0 >> +#define SNOR_PROTO_DATA_MASK GENMASK(3, 0) >=20 > Why not directly reserving 8 bits per cycle type. Your end result is > stored on a 32 bit integer anyway, and since you'll always have only = 3 > cycle types (ADDR, CMD and DATA). This way, you'll be ready for > SPI-8-8-8 :-). >=20 I've reserved 4 bits per cycle type so the possible values range into [= 0 - 15]. I thought it would be enough to anticipate future evolution like SPI 8-= 8-8. Also it leaves more bits available for future usage. =46inally, I've chosen 4 bits and this order so when debugging and/or p= rinting the value in hex format, you can easily decode the corresponding protoc= ol: SPI 1-1-4 -> 0x00000114 SPI 1-2-2 -> 0x00000122 SPI 4-4-4 -> 0x00000444 Anyway I don't mind using 8 bits instead if it is preferred. >> +#define SNOR_PROTO_DATA_TO_PROTO(data) \ >> + (((data) << SNOR_PROTO_DATA_OFF) & SNOR_PROTO_DATA_MASK) >> +#define SNOR_PROTO_DATA_FROM_PROTO(proto) \ >> + ((((u32)(proto)) & SNOR_PROTO_DATA_MASK) >> SNOR_PROTO_DATA_OFF) >> + >> +#define SNOR_PROTO(cmd, addr, data) \ >> + (SNOR_PROTO_CMD_TO_PROTO(cmd) | \ >> + SNOR_PROTO_ADDR_TO_PROTO(addr) | \ >> + SNOR_PROTO_DATA_TO_PROTO(data)) >> + >> +enum spi_nor_protocol { >> + SNOR_PROTO_1_1_1 =3D SNOR_PROTO(1, 1, 1), /* SPI */ >> + SNOR_PROTO_1_1_2 =3D SNOR_PROTO(1, 1, 2), /* Dual Output */ >> + SNOR_PROTO_1_1_4 =3D SNOR_PROTO(1, 1, 4), /* Quad Output */ >> + SNOR_PROTO_1_2_2 =3D SNOR_PROTO(1, 2, 2), /* Dual IO */ >> + SNOR_PROTO_1_4_4 =3D SNOR_PROTO(1, 4, 4), /* Quad IO */ >> + SNOR_PROTO_2_2_2 =3D SNOR_PROTO(2, 2, 2), /* Dual Command */ >> + SNOR_PROTO_4_4_4 =3D SNOR_PROTO(4, 4, 4), /* Quad Command */ >> +}; >=20 > Do you really need this enum definition? I mean, directly using > SNOR_PROTO(X, Y, Z) is just as easy as using the SNOR_PROTO_X_Y_Z enu= m > definitions, and you won't have to define new ones if spi-nor vendors > decide to support new combinations. >=20 This enum was created only for stronger type checking; Except for that,= there is no strong reason to keep the enum. So I don't mind removing it if ot= her people agree with you. >> + >> #define SPI_NOR_MAX_CMD_SIZE 8 >> enum spi_nor_ops { >> SPI_NOR_OPS_READ =3D 0, >> @@ -132,6 +170,10 @@ enum spi_nor_option_flags { >> * @flash_read: the mode of the read >> * @sst_write_second: used by the SST write operation >> * @flags: flag options for the current SPI-NOR (SNOR_F_*) >> + * @erase_proto: the SPI protocol used by erase operations >> + * @read_proto: the SPI protocol used by read operations >> + * @write_proto: the SPI protocol used by write operations >> + * @reg_proto the SPI protocol used by read_reg/write_reg operatio= ns >> * @cmd_buf: used by the write_reg >> * @prepare: [OPTIONAL] do some preparations for the >> * read/write/erase/lock/unlock operations >> @@ -160,6 +202,10 @@ struct spi_nor { >> u8 read_opcode; >> u8 read_dummy; >> u8 program_opcode; >> + enum spi_nor_protocol erase_proto; >> + enum spi_nor_protocol read_proto; >> + enum spi_nor_protocol write_proto; >> + enum spi_nor_protocol reg_proto; >=20 > Should be u32 fields if you decide to drop the enum definition. >=20 >> enum read_mode flash_read; >> bool sst_write_second; >> u32 flags; >=20 >=20 >=20 Thanks for your review :) Best regards, Cyrille -- To unsubscribe from this list: send the line "unsubscribe devicetree" i= n the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html