| Message ID | 1385727565-25794-9-git-send-email-lee.jones@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Nov 29, 2013 at 12:18:57PM +0000, Lee Jones wrote: > JEDEC have helped to standardise a great deal of the commands which > can be issued to a Serial Flash devices. Many of the OPCODEs and all > of the Serial Flash Discoverable Parameters (SFDP) commands are > generic across devices. This patch provides a shared point where > these commands can be defined. I think this is probably a good start for a shared header. In the near term, we need to settle on one header that can store commands, at least. I think both you and Huang are submitting patches that do similar opcode relocation. Let's make sure we get the separation right, so that this header contains exactly as much of the opcode-related stuff that can be easily shared and reused. > Suggested-by: Mark Brown <broonie@kernel.org> > Signed-off-by: Lee Jones <lee.jones@linaro.org> > --- > drivers/mtd/devices/serial_flash_cmds.h | 91 +++++++++++++++++++++++++++++++++ > drivers/mtd/devices/st_spi_fsm.c | 1 + > drivers/mtd/devices/st_spi_fsm.h | 2 + > 3 files changed, 94 insertions(+) > create mode 100644 drivers/mtd/devices/serial_flash_cmds.h > > diff --git a/drivers/mtd/devices/serial_flash_cmds.h b/drivers/mtd/devices/serial_flash_cmds.h > new file mode 100644 > index 0000000..62aa420 > --- /dev/null > +++ b/drivers/mtd/devices/serial_flash_cmds.h > @@ -0,0 +1,91 @@ > +/* > + * Generic/SFDP Flash Commands and Device Capabilities > + * > + * Copyright (C) 2013 Lee Jones <lee.jones@lianro.org> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA BTW, I think checkpatch.pl complains about putting the FSF address here. > +/* JEDEC Standard - Serial Flash Discoverable Parmeters (SFDP) Commands */ > +#define FLASH_CMD_READ 0x03 /* READ */ > +#define FLASH_CMD_READ_FAST 0x0b /* FAST READ */ > +#define FLASH_CMD_READ_1_1_2 0x3b /* DUAL OUTPUT READ */ > +#define FLASH_CMD_READ_1_2_2 0xbb /* DUAL I/O READ */ > +#define FLASH_CMD_READ_1_1_4 0x6b /* QUAD OUTPUT READ */ > +#define FLASH_CMD_READ_1_4_4 0xeb /* QUAD I/O READ */ All of these {1,2,4}_{1,2,4}_{1,2,4} suffixes are a little cryptic; I ended up referring to the SFDP spec to figure out what the first number meant. Can you document them briefly at the top of this SFDP list? > +#define FLASH_CMD_WRITE 0x02 /* PAGE PROGRAM */ > +#define FLASH_CMD_WRITE_1_1_2 0xa2 /* DUAL INPUT PROGRAM */ > +#define FLASH_CMD_WRITE_1_2_2 0xd2 /* DUAL INPUT EXT PROGRAM */ > +#define FLASH_CMD_WRITE_1_1_4 0x32 /* QUAD INPUT PROGRAM */ > +#define FLASH_CMD_WRITE_1_4_4 0x12 /* QUAD INPUT EXT PROGRAM */ > + > +#define FLASH_CMD_EN4B_ADDR 0xb7 /* Enter 4-byte address mode */ > +#define FLASH_CMD_EX4B_ADDR 0xe9 /* Exit 4-byte address mode */ > + > +/* READ commands with 32-bit addressing */ > +#define FLASH_CMD_READ4 0x13 > +#define FLASH_CMD_READ4_FAST 0x0c > +#define FLASH_CMD_READ4_1_1_2 0x3c > +#define FLASH_CMD_READ4_1_2_2 0xbc > +#define FLASH_CMD_READ4_1_1_4 0x6c > +#define FLASH_CMD_READ4_1_4_4 0xec Since you list these, does SFDP even describe the 32-bit addressing commands? It seems like it assumes the device is switched (statefully) between 24-bit and 32-bit address modes (or kept permanently in one or the other). > +/* Configuration flags */ > +#define FLASH_FLAG_SINGLE 0x000000ff > +#define FLASH_FLAG_READ_WRITE 0x00000001 > +#define FLASH_FLAG_READ_FAST 0x00000002 > +#define FLASH_FLAG_SE_4K 0x00000004 > +#define FLASH_FLAG_SE_32K 0x00000008 > +#define FLASH_FLAG_CE 0x00000010 > +#define FLASH_FLAG_32BIT_ADDR 0x00000020 > +#define FLASH_FLAG_RESET 0x00000040 > +#define FLASH_FLAG_DYB_LOCKING 0x00000080 > + > +#define FLASH_FLAG_DUAL 0x0000ff00 > +#define FLASH_FLAG_READ_1_1_2 0x00000100 > +#define FLASH_FLAG_READ_1_2_2 0x00000200 > +#define FLASH_FLAG_READ_2_2_2 0x00000400 > +#define FLASH_FLAG_WRITE_1_1_2 0x00001000 > +#define FLASH_FLAG_WRITE_1_2_2 0x00002000 > +#define FLASH_FLAG_WRITE_2_2_2 0x00004000 > + > +#define FLASH_FLAG_QUAD 0x00ff0000 > +#define FLASH_FLAG_READ_1_1_4 0x00010000 > +#define FLASH_FLAG_READ_1_4_4 0x00020000 > +#define FLASH_FLAG_READ_4_4_4 0x00040000 > +#define FLASH_FLAG_WRITE_1_1_4 0x00100000 > +#define FLASH_FLAG_WRITE_1_4_4 0x00200000 > +#define FLASH_FLAG_WRITE_4_4_4 0x00400000 It doesn't look to me like the dual/quad bitfields are laid out very usefully. Can't they be divided so that their bit position is more meaningful? Like reserve bits [8:23] for dual/quad descriptions, then give the following: [8:11] number of pins for the opcode (1=single; 2=dual; 4=quad) [12:15] number of pins for the address (1=single; 2=dual; ...) [16:19] number of pins for the data [20] read or write opcode (0=read; 1=write) (you could easily pack these differently, but you get the idea) Then: #define FLASH_FLAG_DUAL_MASK 0x00022200 #define FLASH_FLAG_QUAD_MASK 0x00044400 And we could do things like: #define FLASH_FLAG_OPCODE_PINS(x) (((x) & 0x00000f00) >> 8) #define FLASH_FLAG_ADDR_PINS(x) (((x) & 0x0000f000) >> 12) #define FLASH_FLAG_DATA_PINS(x) (((x) & 0x000f0000) >> 16) (and maybe replace those mask/shifts with macro names) This could eliminate some redundancy in your tables, I think, so that you can extract (from this flag) the number of "addr_pads" and "data_pads" needed for the opcode. And I think this would be useful to other controllers eventually. For instance, my quad-read driver might support 4 data pins but it can't put the opcode or address out on more than 1 pin. Brian
On Tue, Dec 10, 2013 at 01:48:49PM -0800, Brian Norris wrote: > It doesn't look to me like the dual/quad bitfields are laid out very > usefully. Can't they be divided so that their bit position is more > meaningful? Like reserve bits [8:23] for dual/quad descriptions, then > give the following: > > [8:11] number of pins for the opcode (1=single; 2=dual; 4=quad) > [12:15] number of pins for the address (1=single; 2=dual; ...) > [16:19] number of pins for the data > [20] read or write opcode (0=read; 1=write) > > (you could easily pack these differently, but you get the idea) > > Then: > > #define FLASH_FLAG_DUAL_MASK 0x00022200 > #define FLASH_FLAG_QUAD_MASK 0x00044400 > > And we could do things like: > > #define FLASH_FLAG_OPCODE_PINS(x) (((x) & 0x00000f00) >> 8) > #define FLASH_FLAG_ADDR_PINS(x) (((x) & 0x0000f000) >> 12) > #define FLASH_FLAG_DATA_PINS(x) (((x) & 0x000f0000) >> 16) > > (and maybe replace those mask/shifts with macro names) I realized you are packing these tight because you're using them as combinable (bitwise OR) flags (hence the name FLASH_FLAG_*, duh!). So while my scheme is nicer for representing a single opcode w.r.t. controller requirements, it is not able to represent the exact opcodes supported by a particular flash. Hence, this is not the right place for it. But I don't want to imitate your flags in any generic framework; we need a method (that doesn't involve too many extra tables like in your driver; or that supports these tables generically, to share with other drivers) to map the "supported opcodes" flags to useful properties that the controller/driver can examine. Is it possible, for instance, to describe a range of opcodes supported using patterns? Like "this flash supports opcodes for 1-4 data pins and 1-2 address pins"? Or am I being too idealistic, and most flash really support a totally arbitrary combination of opcodes? Brian
On 12/10/2013 09:48 PM, Brian Norris wrote: > On Fri, Nov 29, 2013 at 12:18:57PM +0000, Lee Jones wrote: >> JEDEC have helped to standardise a great deal of the commands which >> can be issued to a Serial Flash devices. Many of the OPCODEs and all >> of the Serial Flash Discoverable Parameters (SFDP) commands are >> generic across devices. This patch provides a shared point where >> these commands can be defined. This comment isn't entirely correct. The SFDP standard (JESD216A) does not go as far as standardising the OPCODES, but merely a way of determining at runtime which operations are supported (e.g. READ_1_1_4, READ_1_4_4), and the associated opcodes; vendors are still free to use a different opcode for a particular operation. >> +/* JEDEC Standard - Serial Flash Discoverable Parmeters (SFDP) Commands */ >> +#define FLASH_CMD_READ 0x03 /* READ */ >> +#define FLASH_CMD_READ_FAST 0x0b /* FAST READ */ >> +#define FLASH_CMD_READ_1_1_2 0x3b /* DUAL OUTPUT READ */ >> +#define FLASH_CMD_READ_1_2_2 0xbb /* DUAL I/O READ */ >> +#define FLASH_CMD_READ_1_1_4 0x6b /* QUAD OUTPUT READ */ >> +#define FLASH_CMD_READ_1_4_4 0xeb /* QUAD I/O READ */ > > All of these {1,2,4}_{1,2,4}_{1,2,4} suffixes are a little cryptic; I > ended up referring to the SFDP spec to figure out what the first number > meant. Can you document them briefly at the top of this SFDP list? I think the {x,y,z} descriptions offer the simplest way of representing the myriad of operations supported by various devices -- it was one thing SFDP did get right. However, I agree, the nomenclature does need explained. We also need a way of representing 32-bit address commands, and SDR vs DDR commands -- this has not yet been addresses by SFDP. > Since you list these, does SFDP even describe the 32-bit addressing > commands? Not yet, but I believe "JESD216B" will include a parameter table related to 32-bit address instructions. > It seems like it assumes the device is switched (statefully) > between 24-bit and 32-bit address modes (or kept permanently in one or > the other). And the complexity increases! The st_spi_fsm H/W also supports a memory-mapped mode for booting. However, the boot controller is hard-coded to issue 24-bit address cycles. If the Serial Flash device includes a dedicated REST pin, and this is appropriately routed to the system reset, then we have no issue -- the device will reset to its power-on state following a warm reset. If a reset pin is not available, then certain precautions can be followed to minimise problems with warm resets. If the Serial Flash device supports a 32-bit instruction set, then this should be used in preference, and we can avoid any statefull transitions. Failing that, we can minimise the window in which a reset would fail by issuing EN4B/EX4B before/after each operation. Of course, if we know the system is not subject to these constraints, then we just need to issue EN4B at the start, and stay in 4B mode. All this knowledge needs to be in the spi-nor layer...
On 12/10/2013 10:23 PM, Brian Norris wrote: > On Tue, Dec 10, 2013 at 01:48:49PM -0800, Brian Norris wrote: >> It doesn't look to me like the dual/quad bitfields are laid out very >> usefully. Can't they be divided so that their bit position is more >> meaningful? [...] > I realized you are packing these tight because you're using them as > combinable (bitwise OR) flags (hence the name FLASH_FLAG_*, duh!). So > while my scheme is nicer for representing a single opcode w.r.t. > controller requirements, it is not able to represent the exact opcodes > supported by a particular flash. Hence, this is not the right place for > it. There is certainly scope to compact the opcode representation... > But I don't want to imitate your flags in any generic framework; we need > a method (that doesn't involve too many extra tables like in your > driver; or that supports these tables generically, to share with other > drivers) to map the "supported opcodes" flags to useful properties that > the controller/driver can examine. I believe a key function of any spi-nor framework should be to determine which fundamental modes of operation are supported by the device (e.g. READ_1_1_2, READ_1_2_2, READ_1_1_4, READ_1_4_4 etc) and how they are driven (i.e. opcode, number of mode bits, number of dummy cycles). A second stage would be to configure the best read, write, erase configurations based on the combined capabilities of the device and the H/W controller. However, it is not obvious how best to achieve this functionality. Given the amount of information that needs to be represented, a static table is not going to be popular. The current approach in st_spi_fsm assumes some default configurations for supported modes, with the option to override with device/family-specific configurations. To be honest, it is rather ugly, and the result of historic evolutions rather than a clean design! > Is it possible, for instance, to describe a range of opcodes supported > using patterns? Like "this flash supports opcodes for 1-4 data pins and > 1-2 address pins"? Or am I being too idealistic, and most flash really > support a totally arbitrary combination of opcodes? Yes, I am afraid so. For example, the mx25l12836e supports READ_1_1_2(0x3b) and READ_1_1_4(0x6b), whereas the mx25l12845e supports READ_1_2_2(0xbb) and READ_1_4_4(0xeb). Cheers, Angus
diff --git a/drivers/mtd/devices/serial_flash_cmds.h b/drivers/mtd/devices/serial_flash_cmds.h new file mode 100644 index 0000000..62aa420 --- /dev/null +++ b/drivers/mtd/devices/serial_flash_cmds.h @@ -0,0 +1,91 @@ +/* + * Generic/SFDP Flash Commands and Device Capabilities + * + * Copyright (C) 2013 Lee Jones <lee.jones@lianro.org> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + */ + +#ifndef _MTD_SERIAL_FLASH_CMDS_H +#define _MTD_SERIAL_FLASH_CMDS_H + +/* Generic Flash Commands/OPCODEs */ +#define FLASH_CMD_WREN 0x06 +#define FLASH_CMD_WRDI 0x04 +#define FLASH_CMD_RDID 0x9f +#define FLASH_CMD_RDSR 0x05 +#define FLASH_CMD_RDSR2 0x35 +#define FLASH_CMD_WRSR 0x01 +#define FLASH_CMD_SE_4K 0x20 +#define FLASH_CMD_SE_32K 0x52 +#define FLASH_CMD_SE 0xd8 +#define FLASH_CMD_CHIPERASE 0xc7 +#define FLASH_CMD_WRVCR 0x81 +#define FLASH_CMD_RDVCR 0x85 + +/* JEDEC Standard - Serial Flash Discoverable Parmeters (SFDP) Commands */ +#define FLASH_CMD_READ 0x03 /* READ */ +#define FLASH_CMD_READ_FAST 0x0b /* FAST READ */ +#define FLASH_CMD_READ_1_1_2 0x3b /* DUAL OUTPUT READ */ +#define FLASH_CMD_READ_1_2_2 0xbb /* DUAL I/O READ */ +#define FLASH_CMD_READ_1_1_4 0x6b /* QUAD OUTPUT READ */ +#define FLASH_CMD_READ_1_4_4 0xeb /* QUAD I/O READ */ + +#define FLASH_CMD_WRITE 0x02 /* PAGE PROGRAM */ +#define FLASH_CMD_WRITE_1_1_2 0xa2 /* DUAL INPUT PROGRAM */ +#define FLASH_CMD_WRITE_1_2_2 0xd2 /* DUAL INPUT EXT PROGRAM */ +#define FLASH_CMD_WRITE_1_1_4 0x32 /* QUAD INPUT PROGRAM */ +#define FLASH_CMD_WRITE_1_4_4 0x12 /* QUAD INPUT EXT PROGRAM */ + +#define FLASH_CMD_EN4B_ADDR 0xb7 /* Enter 4-byte address mode */ +#define FLASH_CMD_EX4B_ADDR 0xe9 /* Exit 4-byte address mode */ + +/* READ commands with 32-bit addressing */ +#define FLASH_CMD_READ4 0x13 +#define FLASH_CMD_READ4_FAST 0x0c +#define FLASH_CMD_READ4_1_1_2 0x3c +#define FLASH_CMD_READ4_1_2_2 0xbc +#define FLASH_CMD_READ4_1_1_4 0x6c +#define FLASH_CMD_READ4_1_4_4 0xec + +/* Configuration flags */ +#define FLASH_FLAG_SINGLE 0x000000ff +#define FLASH_FLAG_READ_WRITE 0x00000001 +#define FLASH_FLAG_READ_FAST 0x00000002 +#define FLASH_FLAG_SE_4K 0x00000004 +#define FLASH_FLAG_SE_32K 0x00000008 +#define FLASH_FLAG_CE 0x00000010 +#define FLASH_FLAG_32BIT_ADDR 0x00000020 +#define FLASH_FLAG_RESET 0x00000040 +#define FLASH_FLAG_DYB_LOCKING 0x00000080 + +#define FLASH_FLAG_DUAL 0x0000ff00 +#define FLASH_FLAG_READ_1_1_2 0x00000100 +#define FLASH_FLAG_READ_1_2_2 0x00000200 +#define FLASH_FLAG_READ_2_2_2 0x00000400 +#define FLASH_FLAG_WRITE_1_1_2 0x00001000 +#define FLASH_FLAG_WRITE_1_2_2 0x00002000 +#define FLASH_FLAG_WRITE_2_2_2 0x00004000 + +#define FLASH_FLAG_QUAD 0x00ff0000 +#define FLASH_FLAG_READ_1_1_4 0x00010000 +#define FLASH_FLAG_READ_1_4_4 0x00020000 +#define FLASH_FLAG_READ_4_4_4 0x00040000 +#define FLASH_FLAG_WRITE_1_1_4 0x00100000 +#define FLASH_FLAG_WRITE_1_4_4 0x00200000 +#define FLASH_FLAG_WRITE_4_4_4 0x00400000 + +#endif /* _MTD_SERIAL_FLASH_CMDS_H */ diff --git a/drivers/mtd/devices/st_spi_fsm.c b/drivers/mtd/devices/st_spi_fsm.c index fe19ec3..71b6188 100644 --- a/drivers/mtd/devices/st_spi_fsm.c +++ b/drivers/mtd/devices/st_spi_fsm.c @@ -24,6 +24,7 @@ #include <asm/io.h> #include "st_spi_fsm.h" +#include "serial_flash_cmds.h" static struct stfsm_seq stfsm_seq_read_jedec = { .data_size = TRANSFER_SIZE(8), diff --git a/drivers/mtd/devices/st_spi_fsm.h b/drivers/mtd/devices/st_spi_fsm.h index 97b4868..5c6c55e 100644 --- a/drivers/mtd/devices/st_spi_fsm.h +++ b/drivers/mtd/devices/st_spi_fsm.h @@ -16,6 +16,8 @@ #ifndef ST_SPI_FSM_H #define ST_SPI_FSM_H +#include "serial_flash_cmds.h" + /* * FSM SPI Controller Registers */
JEDEC have helped to standardise a great deal of the commands which can be issued to a Serial Flash devices. Many of the OPCODEs and all of the Serial Flash Discoverable Parameters (SFDP) commands are generic across devices. This patch provides a shared point where these commands can be defined. Suggested-by: Mark Brown <broonie@kernel.org> Signed-off-by: Lee Jones <lee.jones@linaro.org> --- drivers/mtd/devices/serial_flash_cmds.h | 91 +++++++++++++++++++++++++++++++++ drivers/mtd/devices/st_spi_fsm.c | 1 + drivers/mtd/devices/st_spi_fsm.h | 2 + 3 files changed, 94 insertions(+) create mode 100644 drivers/mtd/devices/serial_flash_cmds.h