| Message ID | CA0E6F9BA6AED7458C23277BD87075E51015AD1C@DEMUMBX005.nsn-intra.net |
|---|---|
| State | New |
| Headers | show |
These features are also available in Xilinx QEMU if you want to compare implementation: https://github.com/Xilinx/qemu/blob/pub/2015.2.plnx/hw/block/m25p80.c That work also handles the larger and newer Spansion flash parts, as well as the quad and dual mode commands for QSPI (also features of n25qXXX). On Sat, Nov 28, 2015 at 9:06 AM, Krzeminski, Marcin (Nokia - PL/Wroclaw) <marcin.krzeminski@nokia.com> wrote: > It is my first patch, so any comment are really welcome. > Check MAINTAINERS file for relevant people to CC. To make informal comments on your patches, you but them below the line ... > Changes: > * Removed unused variable > * Added support for n25q256a and n25q512a > * Added support for 4bytes address mode 4 byte addressing is a feature common to more than just n25qXXX. It should be done as a separate prepended patch. > * Added support for banked read mode > * Added support for sw reset flash commands > * Added Read Flag Status register command support > Basically these bullets should indicate separate patches to ease review. > Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com> > --- ... here. So when the maintainers apply the patch they are not committed to the git logs. > hw/block/m25p80.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 88 insertions(+), 6 deletions(-) > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c > index efc43dd..c8b92d8 100644 > --- a/hw/block/m25p80.c > +++ b/hw/block/m25p80.c > @@ -47,6 +47,9 @@ > */ > #define WR_1 0x100 > > +/* 16 MiB max in 3 byte address mode */ > +#define MAX_3BYTES_SIZE 0x1000000 > + > typedef struct FlashPartInfo { > const char *part_name; > /* jedec code. (jedec >> 16) & 0xff is the 1st byte, >> 8 the 2nd etc */ > @@ -206,6 +209,8 @@ static const FlashPartInfo known_devices[] = { > > /* Numonyx -- n25q128 */ > { INFO("n25q128", 0x20ba18, 0, 64 << 10, 256, 0) }, > + { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512, ER_4K) }, > + { INFO("n25q512a", 0x20ba20, 0, 64 << 10, 1024, ER_4K) }, > }; > > typedef enum { > @@ -216,6 +221,7 @@ typedef enum { > WREN = 0x6, > JEDEC_READ = 0x9f, > BULK_ERASE = 0xc7, > + READ_FSL = 0x70, Where does "FSL" come from? I am looking at an n25q256 datasheet here that has this is "RFSR". http://datasheet.octopart.com/N25Q256A13E1241F-Micron-datasheet-11552757.pdf Admittedly, the vendors do tend to rename this stuff from part-to-part. To keep consistent with surrounding code, this would be READ_FSR. > > READ = 0x3, > FAST_READ = 0xb, > @@ -231,6 +237,15 @@ typedef enum { > ERASE_4K = 0x20, > ERASE_32K = 0x52, > ERASE_SECTOR = 0xd8, > + > + ENTER_4BYTE_ADDR_MODE = 0xB7, > + LEAVE_4BYTE_ADDR_MODE = 0xE9, > + > + EXTEND_ADDR_READ = 0xC8, > + EXTEND_ADDR_WRITE = 0xC5, > + Same data sheet has: WREAR, RDEAR, EN4BYTEADDR ... but Xilinx code has something different again. In both cases, it is shorter, so I think this should just be something shorter. > + RESET_ENABLE = 0x66, > + RESET_MEMORY = 0x99, > } FlashCMD; > > typedef enum { > @@ -244,8 +259,6 @@ typedef enum { > typedef struct Flash { > SSISlave parent_obj; > > - uint32_t r; > - Even the trivial cleanup can be a separate patch. > BlockBackend *blk; > > uint8_t *storage; > @@ -260,6 +273,9 @@ typedef struct Flash { > uint8_t cmd_in_progress; > uint64_t cur_addr; > bool write_enable; > + bool four_bytes_address_mode; > + bool reset_enable; > + uint8_t extended_addr_reg; The datasheets abbreviate this to "EAR" so I think the same should be done in code. > > int64_t dirty_page; > > @@ -397,9 +413,17 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t data) > > static void complete_collecting_data(Flash *s) > { > - s->cur_addr = s->data[0] << 16; > - s->cur_addr |= s->data[1] << 8; > - s->cur_addr |= s->data[2]; > + if (s->four_bytes_address_mode) { > + s->cur_addr = s->data[0] << 24; > + s->cur_addr |= s->data[1] << 16; > + s->cur_addr |= s->data[2] << 8; > + s->cur_addr |= s->data[3]; > + } else { > + s->cur_addr = s->data[0] << 16; > + s->cur_addr |= s->data[1] << 8; > + s->cur_addr |= s->data[2]; > + s->cur_addr += (s->extended_addr_reg&0x3)*MAX_3BYTES_SIZE; > + } This can share implementation between 3 byte and 4 byte mode. From the Xilinx work: static inline void do_4_byte_address(Flash *s) { s->cur_addr <<= 8; s->cur_addr |= s->data[3]; } #define BAR_7_4_BYTE_ADDR (1<<7) static inline void check_4_byte_address(Flash *s) { /* Allow 4byte address if MSB of bar register is set to 1 * Or if 4byte addressing is allowed. */ if ((s->bar & BAR_7_4_BYTE_ADDR) || s->addr_4b) { do_4_byte_address(s); } else { s->cur_addr |= s->bar << 24; } } Which also handles case instructions where the 4 byte addresses comes as data on the wire. For your feature set it would be more minimal than this. > > > s->state = STATE_IDLE; > > @@ -427,11 +452,28 @@ static void complete_collecting_data(Flash *s) > s->write_enable = false; > } > break; > + case EXTEND_ADDR_WRITE: > + s->extended_addr_reg = s->data[0]; > + break; > default: > break; > } > } > > +static void reset_memory(Flash *s) > +{ > + s->cmd_in_progress = NOP; > + s->cur_addr = 0; > + s->extended_addr_reg = 0; > + s->four_bytes_address_mode = false; > + s->len = 0; > + s->needed_bytes = 0; > + s->pos = 0; > + s->state = STATE_IDLE; > + s->write_enable = false; > + s->reset_enable = false; > +} > + > static void decode_new_cmd(Flash *s, uint32_t value) > { > s->cmd_in_progress = value; > @@ -446,7 +488,7 @@ static void decode_new_cmd(Flash *s, uint32_t value) > case DPP: > case QPP: > case PP: > - s->needed_bytes = 3; > + s->needed_bytes = s->four_bytes_address_mode ? 4 : 3; > s->pos = 0; > s->len = 0; > s->state = STATE_COLLECTING_DATA; > @@ -514,6 +556,13 @@ static void decode_new_cmd(Flash *s, uint32_t value) > s->state = STATE_READING_DATA; > break; > > + case READ_FSL: > + s->data[0] = (1<<7); /*Indicates flash is ready */ > + s->pos = 0; > + s->len = 1; IIRC, this command will continue to read out the same data byte continuously until a new command is issued. For this reason, the Xilinx work has a feature where commands can be marked looping. This confused some drivers we had. > + s->state = STATE_READING_DATA; > + break; > + > case JEDEC_READ: > DB_PRINT_L(0, "populated jedec code\n"); > s->data[0] = (s->pi->jedec >> 16) & 0xff; > @@ -541,6 +590,34 @@ static void decode_new_cmd(Flash *s, uint32_t value) > break; > case NOP: > break; > + case ENTER_4BYTE_ADDR_MODE: > + s->four_bytes_address_mode = true; > + break; > + case LEAVE_4BYTE_ADDR_MODE: > + s->four_bytes_address_mode = false; > + break; > + case EXTEND_ADDR_READ: > + s->data[0] = s->extended_addr_reg; > + s->pos = 0; > + s->len = 1; > + s->state = STATE_READING_DATA; > + break; > + case EXTEND_ADDR_WRITE: > + if (s->write_enable) { > + s->needed_bytes = 1; > + s->pos = 0; > + s->len = 0; > + s->state = STATE_COLLECTING_DATA; > + } > + break; > + case RESET_ENABLE: > + s->reset_enable = true; > + break; > + case RESET_MEMORY: > + if (s->reset_enable) { > + reset_memory(s); > + } > + break; > default: > qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Unknown cmd %x\n", value); > break; > @@ -622,6 +699,8 @@ static int m25p80_init(SSISlave *ss) > s->size = s->pi->sector_size * s->pi->n_sectors; > s->dirty_page = -1; > > + reset_memory(s); > + Resets should be handled in the device->reset callback rather than the init() function. Regards, Peter > /* FIXME use a qdev drive property instead of drive_get_next() */ > dinfo = drive_get_next(IF_MTD); > > @@ -666,6 +745,9 @@ static const VMStateDescription vmstate_m25p80 = { > VMSTATE_UINT8(cmd_in_progress, Flash), > VMSTATE_UINT64(cur_addr, Flash), > VMSTATE_BOOL(write_enable, Flash), > + VMSTATE_BOOL(four_bytes_address_mode, Flash), > + VMSTATE_UINT8(extended_addr_reg, Flash), > + VMSTATE_BOOL(reset_enable, Flash), > VMSTATE_END_OF_LIST() > } > }; > -- > 1.9.1 > > Regards, > Marcin Krzeminski >
> -----Original Message----- > From: EXT Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com] > Sent: Saturday, November 28, 2015 7:50 PM > To: Krzeminski, Marcin (Nokia - PL/Wroclaw) > Cc: qemu-devel@nongnu.org; git@xilinx.com; Sai Pavan Boddu > Subject: Re: [Qemu-devel] [PATCH] m25p80.c Added support for N25Q256 > and N25Q512 > > These features are also available in Xilinx QEMU if you want to compare > implementation: > > https://github.com/Xilinx/qemu/blob/pub/2015.2.plnx/hw/block/m25p80.c > > That work also handles the larger and newer Spansion flash parts, as well as > the quad and dual mode commands for QSPI (also features of n25qXXX). > Too bad I did not checked xilix fork, so V2 of this patch does not make sense since all is already implemented. Why didn't xilinks merge it with mainline? Anyway, I do not like not commented reviews, so lets play the game... > On Sat, Nov 28, 2015 at 9:06 AM, Krzeminski, Marcin (Nokia - > PL/Wroclaw) <marcin.krzeminski@nokia.com> wrote: > > It is my first patch, so any comment are really welcome. > > > Check MAINTAINERS file for relevant people to CC. > > To make informal comments on your patches, you but them below the line ... > > > Changes: > > * Removed unused variable > > * Added support for n25q256a and n25q512a > > * Added support for 4bytes address mode > > 4 byte addressing is a feature common to more than just n25qXXX. It should > be done as a separate prepended patch. > > > * Added support for banked read mode > > * Added support for sw reset flash commands > > * Added Read Flag Status register command support > > > > Basically these bullets should indicate separate patches to ease review. > > > Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com> > > --- > > ... here. So when the maintainers apply the patch they are not committed to > the git logs. > Thanks. > > hw/block/m25p80.c | 94 > > +++++++++++++++++++++++++++++++++++++++++++++++++++---- > > 1 file changed, 88 insertions(+), 6 deletions(-) > > > > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index > > efc43dd..c8b92d8 100644 > > --- a/hw/block/m25p80.c > > +++ b/hw/block/m25p80.c > > @@ -47,6 +47,9 @@ > > */ > > #define WR_1 0x100 > > > > +/* 16 MiB max in 3 byte address mode */ #define MAX_3BYTES_SIZE > > +0x1000000 > > + > > typedef struct FlashPartInfo { > > const char *part_name; > > /* jedec code. (jedec >> 16) & 0xff is the 1st byte, >> 8 the 2nd > > etc */ @@ -206,6 +209,8 @@ static const FlashPartInfo known_devices[] > > = { > > > > /* Numonyx -- n25q128 */ > > { INFO("n25q128", 0x20ba18, 0, 64 << 10, 256, 0) }, > > + { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512, ER_4K) }, > > + { INFO("n25q512a", 0x20ba20, 0, 64 << 10, 1024, ER_4K) }, > > }; > > > > typedef enum { > > @@ -216,6 +221,7 @@ typedef enum { > > WREN = 0x6, > > JEDEC_READ = 0x9f, > > BULK_ERASE = 0xc7, > > + READ_FSL = 0x70, > > Where does "FSL" come from? I am looking at an n25q256 datasheet here > that has this is "RFSR". > > http://datasheet.octopart.com/N25Q256A13E1241F-Micron-datasheet- > 11552757.pdf > > Admittedly, the vendors do tend to rename this stuff from part-to-part. To > keep consistent with surrounding code, this would be READ_FSR. I seem it is a typo, it should be READ_FSR. > > > > > READ = 0x3, > > FAST_READ = 0xb, > > @@ -231,6 +237,15 @@ typedef enum { > > ERASE_4K = 0x20, > > ERASE_32K = 0x52, > > ERASE_SECTOR = 0xd8, > > + > > + ENTER_4BYTE_ADDR_MODE = 0xB7, > > + LEAVE_4BYTE_ADDR_MODE = 0xE9, > > + > > + EXTEND_ADDR_READ = 0xC8, > > + EXTEND_ADDR_WRITE = 0xC5, > > + > > Same data sheet has: WREAR, RDEAR, EN4BYTEADDR ... but Xilinx code has > something different again. In both cases, it is shorter, so I think this should > just be something shorter. > True. > > + RESET_ENABLE = 0x66, > > + RESET_MEMORY = 0x99, > > } FlashCMD; > > > > typedef enum { > > @@ -244,8 +259,6 @@ typedef enum { > > typedef struct Flash { > > SSISlave parent_obj; > > > > - uint32_t r; > > - > > Even the trivial cleanup can be a separate patch. > > > BlockBackend *blk; > > > > uint8_t *storage; > > @@ -260,6 +273,9 @@ typedef struct Flash { > > uint8_t cmd_in_progress; > > uint64_t cur_addr; > > bool write_enable; > > + bool four_bytes_address_mode; > > + bool reset_enable; > > + uint8_t extended_addr_reg; > > The datasheets abbreviate this to "EAR" so I think the same should be done > in code. > > > > > int64_t dirty_page; > > > > @@ -397,9 +413,17 @@ void flash_write8(Flash *s, uint64_t addr, > > uint8_t data) > > > > static void complete_collecting_data(Flash *s) { > > - s->cur_addr = s->data[0] << 16; > > - s->cur_addr |= s->data[1] << 8; > > - s->cur_addr |= s->data[2]; > > + if (s->four_bytes_address_mode) { > > + s->cur_addr = s->data[0] << 24; > > + s->cur_addr |= s->data[1] << 16; > > + s->cur_addr |= s->data[2] << 8; > > + s->cur_addr |= s->data[3]; > > + } else { > > + s->cur_addr = s->data[0] << 16; > > + s->cur_addr |= s->data[1] << 8; > > + s->cur_addr |= s->data[2]; > > + s->cur_addr += (s->extended_addr_reg&0x3)*MAX_3BYTES_SIZE; > > + } > > This can share implementation between 3 byte and 4 byte mode. From the > Xilinx work: > > static inline void do_4_byte_address(Flash *s) { > s->cur_addr <<= 8; > s->cur_addr |= s->data[3]; > } > > #define BAR_7_4_BYTE_ADDR (1<<7) > > static inline void check_4_byte_address(Flash *s) { > /* Allow 4byte address if MSB of bar register is set to 1 > * Or if 4byte addressing is allowed. > */ > if ((s->bar & BAR_7_4_BYTE_ADDR) || s->addr_4b) { > do_4_byte_address(s); > } else { > s->cur_addr |= s->bar << 24; > } > } > > Which also handles case instructions where the 4 byte addresses comes as > data on the wire. For your feature set it would be more minimal than this. > > > > > > > s->state = STATE_IDLE; > > > > @@ -427,11 +452,28 @@ static void complete_collecting_data(Flash *s) > > s->write_enable = false; > > } > > break; > > + case EXTEND_ADDR_WRITE: > > + s->extended_addr_reg = s->data[0]; > > + break; > > default: > > break; > > } > > } > > > > +static void reset_memory(Flash *s) > > +{ > > + s->cmd_in_progress = NOP; > > + s->cur_addr = 0; > > + s->extended_addr_reg = 0; > > + s->four_bytes_address_mode = false; > > + s->len = 0; > > + s->needed_bytes = 0; > > + s->pos = 0; > > + s->state = STATE_IDLE; > > + s->write_enable = false; > > + s->reset_enable = false; > > +} > > + > > static void decode_new_cmd(Flash *s, uint32_t value) { > > s->cmd_in_progress = value; > > @@ -446,7 +488,7 @@ static void decode_new_cmd(Flash *s, uint32_t > value) > > case DPP: > > case QPP: > > case PP: > > - s->needed_bytes = 3; > > + s->needed_bytes = s->four_bytes_address_mode ? 4 : 3; > > s->pos = 0; > > s->len = 0; > > s->state = STATE_COLLECTING_DATA; @@ -514,6 +556,13 @@ static > > void decode_new_cmd(Flash *s, uint32_t value) > > s->state = STATE_READING_DATA; > > break; > > > > + case READ_FSL: > > + s->data[0] = (1<<7); /*Indicates flash is ready */ > > + s->pos = 0; > > + s->len = 1; > > IIRC, this command will continue to read out the same data byte continuously > until a new command is issued. For this reason, the Xilinx work has a feature > where commands can be marked looping. This confused some drivers we > had. > Haven't observed that problem, but it is possible. > > + s->state = STATE_READING_DATA; > > + break; > > + > > case JEDEC_READ: > > DB_PRINT_L(0, "populated jedec code\n"); > > s->data[0] = (s->pi->jedec >> 16) & 0xff; @@ -541,6 +590,34 > > @@ static void decode_new_cmd(Flash *s, uint32_t value) > > break; > > case NOP: > > break; > > + case ENTER_4BYTE_ADDR_MODE: > > + s->four_bytes_address_mode = true; > > + break; > > + case LEAVE_4BYTE_ADDR_MODE: > > + s->four_bytes_address_mode = false; > > + break; > > + case EXTEND_ADDR_READ: > > + s->data[0] = s->extended_addr_reg; > > + s->pos = 0; > > + s->len = 1; > > + s->state = STATE_READING_DATA; > > + break; > > + case EXTEND_ADDR_WRITE: > > + if (s->write_enable) { > > + s->needed_bytes = 1; > > + s->pos = 0; > > + s->len = 0; > > + s->state = STATE_COLLECTING_DATA; > > + } > > + break; > > + case RESET_ENABLE: > > + s->reset_enable = true; > > + break; > > + case RESET_MEMORY: > > + if (s->reset_enable) { > > + reset_memory(s); > > + } > > + break; > > default: > > qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Unknown cmd > %x\n", value); > > break; > > @@ -622,6 +699,8 @@ static int m25p80_init(SSISlave *ss) > > s->size = s->pi->sector_size * s->pi->n_sectors; > > s->dirty_page = -1; > > > > + reset_memory(s); > > + > > Resets should be handled in the device->reset callback rather than the > init() function. > That is intentionally - I want to emulated case where guest reboots does no trigger flash reset. For final solution I thought about property that tells if reset pin is used or not. Thanks, Marcin > Regards, > Peter > > > /* FIXME use a qdev drive property instead of drive_get_next() */ > > dinfo = drive_get_next(IF_MTD); > > > > @@ -666,6 +745,9 @@ static const VMStateDescription vmstate_m25p80 = > { > > VMSTATE_UINT8(cmd_in_progress, Flash), > > VMSTATE_UINT64(cur_addr, Flash), > > VMSTATE_BOOL(write_enable, Flash), > > + VMSTATE_BOOL(four_bytes_address_mode, Flash), > > + VMSTATE_UINT8(extended_addr_reg, Flash), > > + VMSTATE_BOOL(reset_enable, Flash), > > VMSTATE_END_OF_LIST() > > } > > }; > > -- > > 1.9.1 > > > > Regards, > > Marcin Krzeminski > >
On Sun, Nov 29, 2015 at 5:12 AM, Krzeminski, Marcin (Nokia - PL/Wroclaw) <marcin.krzeminski@nokia.com> wrote: > > >> -----Original Message----- >> From: EXT Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com] >> Sent: Saturday, November 28, 2015 7:50 PM >> To: Krzeminski, Marcin (Nokia - PL/Wroclaw) >> Cc: qemu-devel@nongnu.org; git@xilinx.com; Sai Pavan Boddu >> Subject: Re: [Qemu-devel] [PATCH] m25p80.c Added support for N25Q256 >> and N25Q512 >> >> These features are also available in Xilinx QEMU if you want to compare >> implementation: >> >> https://github.com/Xilinx/qemu/blob/pub/2015.2.plnx/hw/block/m25p80.c >> >> That work also handles the larger and newer Spansion flash parts, as well as >> the quad and dual mode commands for QSPI (also features of n25qXXX). >> > Too bad I did not checked xilix fork, so V2 of this patch does not make sense since all is already implemented. V2 still makes sense. My V1 comments are pretty minor and that Xilinx work will need cleanup before upstreaming. It is not a competing implementaiton and the diff there will go down when it is rebased on yours. > Why didn't xilinks merge it with mainline? There's a lot in that tree and Xilinx hasn't gotten around to it yet. > Anyway, I do not like not commented reviews, so lets play the game... > >> On Sat, Nov 28, 2015 at 9:06 AM, Krzeminski, Marcin (Nokia - >> PL/Wroclaw) <marcin.krzeminski@nokia.com> wrote: >> > It is my first patch, so any comment are really welcome. >> > >> Check MAINTAINERS file for relevant people to CC. >> >> To make informal comments on your patches, you but them below the line ... >> >> > Changes: >> > * Removed unused variable >> > * Added support for n25q256a and n25q512a >> > * Added support for 4bytes address mode >> >> 4 byte addressing is a feature common to more than just n25qXXX. It should >> be done as a separate prepended patch. >> >> > * Added support for banked read mode >> > * Added support for sw reset flash commands >> > * Added Read Flag Status register command support >> > >> >> Basically these bullets should indicate separate patches to ease review. >> >> > Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com> >> > --- >> >> ... here. So when the maintainers apply the patch they are not committed to >> the git logs. >> > Thanks. >> > hw/block/m25p80.c | 94 >> > +++++++++++++++++++++++++++++++++++++++++++++++++++---- >> > 1 file changed, 88 insertions(+), 6 deletions(-) >> > >> > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index >> > efc43dd..c8b92d8 100644 >> > --- a/hw/block/m25p80.c >> > +++ b/hw/block/m25p80.c >> > @@ -47,6 +47,9 @@ >> > */ >> > #define WR_1 0x100 >> > >> > +/* 16 MiB max in 3 byte address mode */ #define MAX_3BYTES_SIZE >> > +0x1000000 >> > + >> > typedef struct FlashPartInfo { >> > const char *part_name; >> > /* jedec code. (jedec >> 16) & 0xff is the 1st byte, >> 8 the 2nd >> > etc */ @@ -206,6 +209,8 @@ static const FlashPartInfo known_devices[] >> > = { >> > >> > /* Numonyx -- n25q128 */ >> > { INFO("n25q128", 0x20ba18, 0, 64 << 10, 256, 0) }, >> > + { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512, ER_4K) }, >> > + { INFO("n25q512a", 0x20ba20, 0, 64 << 10, 1024, ER_4K) }, >> > }; >> > >> > typedef enum { >> > @@ -216,6 +221,7 @@ typedef enum { >> > WREN = 0x6, >> > JEDEC_READ = 0x9f, >> > BULK_ERASE = 0xc7, >> > + READ_FSL = 0x70, >> >> Where does "FSL" come from? I am looking at an n25q256 datasheet here >> that has this is "RFSR". >> >> http://datasheet.octopart.com/N25Q256A13E1241F-Micron-datasheet- >> 11552757.pdf >> >> Admittedly, the vendors do tend to rename this stuff from part-to-part. To >> keep consistent with surrounding code, this would be READ_FSR. > I seem it is a typo, it should be READ_FSR. >> >> > >> > READ = 0x3, >> > FAST_READ = 0xb, >> > @@ -231,6 +237,15 @@ typedef enum { >> > ERASE_4K = 0x20, >> > ERASE_32K = 0x52, >> > ERASE_SECTOR = 0xd8, >> > + >> > + ENTER_4BYTE_ADDR_MODE = 0xB7, >> > + LEAVE_4BYTE_ADDR_MODE = 0xE9, >> > + >> > + EXTEND_ADDR_READ = 0xC8, >> > + EXTEND_ADDR_WRITE = 0xC5, >> > + >> >> Same data sheet has: WREAR, RDEAR, EN4BYTEADDR ... but Xilinx code has >> something different again. In both cases, it is shorter, so I think this should >> just be something shorter. >> > True. >> > + RESET_ENABLE = 0x66, >> > + RESET_MEMORY = 0x99, >> > } FlashCMD; >> > >> > typedef enum { >> > @@ -244,8 +259,6 @@ typedef enum { >> > typedef struct Flash { >> > SSISlave parent_obj; >> > >> > - uint32_t r; >> > - >> >> Even the trivial cleanup can be a separate patch. >> >> > BlockBackend *blk; >> > >> > uint8_t *storage; >> > @@ -260,6 +273,9 @@ typedef struct Flash { >> > uint8_t cmd_in_progress; >> > uint64_t cur_addr; >> > bool write_enable; >> > + bool four_bytes_address_mode; >> > + bool reset_enable; >> > + uint8_t extended_addr_reg; >> >> The datasheets abbreviate this to "EAR" so I think the same should be done >> in code. >> >> > >> > int64_t dirty_page; >> > >> > @@ -397,9 +413,17 @@ void flash_write8(Flash *s, uint64_t addr, >> > uint8_t data) >> > >> > static void complete_collecting_data(Flash *s) { >> > - s->cur_addr = s->data[0] << 16; >> > - s->cur_addr |= s->data[1] << 8; >> > - s->cur_addr |= s->data[2]; >> > + if (s->four_bytes_address_mode) { >> > + s->cur_addr = s->data[0] << 24; >> > + s->cur_addr |= s->data[1] << 16; >> > + s->cur_addr |= s->data[2] << 8; >> > + s->cur_addr |= s->data[3]; >> > + } else { >> > + s->cur_addr = s->data[0] << 16; >> > + s->cur_addr |= s->data[1] << 8; >> > + s->cur_addr |= s->data[2]; >> > + s->cur_addr += (s->extended_addr_reg&0x3)*MAX_3BYTES_SIZE; >> > + } >> >> This can share implementation between 3 byte and 4 byte mode. From the >> Xilinx work: >> >> static inline void do_4_byte_address(Flash *s) { >> s->cur_addr <<= 8; >> s->cur_addr |= s->data[3]; >> } >> >> #define BAR_7_4_BYTE_ADDR (1<<7) >> >> static inline void check_4_byte_address(Flash *s) { >> /* Allow 4byte address if MSB of bar register is set to 1 >> * Or if 4byte addressing is allowed. >> */ >> if ((s->bar & BAR_7_4_BYTE_ADDR) || s->addr_4b) { >> do_4_byte_address(s); >> } else { >> s->cur_addr |= s->bar << 24; >> } >> } >> >> Which also handles case instructions where the 4 byte addresses comes as >> data on the wire. For your feature set it would be more minimal than this. >> >> > >> > >> > s->state = STATE_IDLE; >> > >> > @@ -427,11 +452,28 @@ static void complete_collecting_data(Flash *s) >> > s->write_enable = false; >> > } >> > break; >> > + case EXTEND_ADDR_WRITE: >> > + s->extended_addr_reg = s->data[0]; >> > + break; >> > default: >> > break; >> > } >> > } >> > >> > +static void reset_memory(Flash *s) >> > +{ >> > + s->cmd_in_progress = NOP; >> > + s->cur_addr = 0; >> > + s->extended_addr_reg = 0; >> > + s->four_bytes_address_mode = false; >> > + s->len = 0; >> > + s->needed_bytes = 0; >> > + s->pos = 0; >> > + s->state = STATE_IDLE; >> > + s->write_enable = false; >> > + s->reset_enable = false; >> > +} >> > + >> > static void decode_new_cmd(Flash *s, uint32_t value) { >> > s->cmd_in_progress = value; >> > @@ -446,7 +488,7 @@ static void decode_new_cmd(Flash *s, uint32_t >> value) >> > case DPP: >> > case QPP: >> > case PP: >> > - s->needed_bytes = 3; >> > + s->needed_bytes = s->four_bytes_address_mode ? 4 : 3; >> > s->pos = 0; >> > s->len = 0; >> > s->state = STATE_COLLECTING_DATA; @@ -514,6 +556,13 @@ static >> > void decode_new_cmd(Flash *s, uint32_t value) >> > s->state = STATE_READING_DATA; >> > break; >> > >> > + case READ_FSL: >> > + s->data[0] = (1<<7); /*Indicates flash is ready */ >> > + s->pos = 0; >> > + s->len = 1; >> >> IIRC, this command will continue to read out the same data byte continuously >> until a new command is issued. For this reason, the Xilinx work has a feature >> where commands can be marked looping. This confused some drivers we >> had. >> > Haven't observed that problem, but it is possible. Yeh you don't need this for V2, just a heads up. >> > + s->state = STATE_READING_DATA; >> > + break; >> > + >> > case JEDEC_READ: >> > DB_PRINT_L(0, "populated jedec code\n"); >> > s->data[0] = (s->pi->jedec >> 16) & 0xff; @@ -541,6 +590,34 >> > @@ static void decode_new_cmd(Flash *s, uint32_t value) >> > break; >> > case NOP: >> > break; >> > + case ENTER_4BYTE_ADDR_MODE: >> > + s->four_bytes_address_mode = true; >> > + break; >> > + case LEAVE_4BYTE_ADDR_MODE: >> > + s->four_bytes_address_mode = false; >> > + break; >> > + case EXTEND_ADDR_READ: >> > + s->data[0] = s->extended_addr_reg; >> > + s->pos = 0; >> > + s->len = 1; >> > + s->state = STATE_READING_DATA; >> > + break; >> > + case EXTEND_ADDR_WRITE: >> > + if (s->write_enable) { >> > + s->needed_bytes = 1; >> > + s->pos = 0; >> > + s->len = 0; >> > + s->state = STATE_COLLECTING_DATA; >> > + } >> > + break; >> > + case RESET_ENABLE: >> > + s->reset_enable = true; >> > + break; >> > + case RESET_MEMORY: >> > + if (s->reset_enable) { >> > + reset_memory(s); >> > + } >> > + break; >> > default: >> > qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Unknown cmd >> %x\n", value); >> > break; >> > @@ -622,6 +699,8 @@ static int m25p80_init(SSISlave *ss) >> > s->size = s->pi->sector_size * s->pi->n_sectors; >> > s->dirty_page = -1; >> > >> > + reset_memory(s); >> > + >> >> Resets should be handled in the device->reset callback rather than the >> init() function. >> > That is intentionally - I want to emulated case where guest reboots does no trigger flash reset. > For final solution I thought about property that tells if reset pin is used or not. > This means that the device->reset as-is needs rework to handle your case as the semantics of the device->reset is a power-on reset. init() should never implement any form of reset. If the current functionality there contains components that are a combination of soft reset and hard reset it should be split to two functions. The hard reset stuff is called as device->reset. The soft components are then triggered but the IO events that happen on your guest reboot. Hard reset can call soft reset if it is a functional superset. Regards, Peter > Thanks, > Marcin >> Regards, >> Peter >> >> > /* FIXME use a qdev drive property instead of drive_get_next() */ >> > dinfo = drive_get_next(IF_MTD); >> > >> > @@ -666,6 +745,9 @@ static const VMStateDescription vmstate_m25p80 = >> { >> > VMSTATE_UINT8(cmd_in_progress, Flash), >> > VMSTATE_UINT64(cur_addr, Flash), >> > VMSTATE_BOOL(write_enable, Flash), >> > + VMSTATE_BOOL(four_bytes_address_mode, Flash), >> > + VMSTATE_UINT8(extended_addr_reg, Flash), >> > + VMSTATE_BOOL(reset_enable, Flash), >> > VMSTATE_END_OF_LIST() >> > } >> > }; >> > -- >> > 1.9.1 >> > >> > Regards, >> > Marcin Krzeminski >> >
> -----Original Message----- > From: EXT Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com] > Sent: Sunday, November 29, 2015 8:19 PM > To: Krzeminski, Marcin (Nokia - PL/Wroclaw) > Cc: qemu-devel@nongnu.org; git@xilinx.com; Sai Pavan Boddu > Subject: Re: [Qemu-devel] [PATCH] m25p80.c Added support for N25Q256 > and N25Q512 > > On Sun, Nov 29, 2015 at 5:12 AM, Krzeminski, Marcin (Nokia - > PL/Wroclaw) <marcin.krzeminski@nokia.com> wrote: > > > > > >> -----Original Message----- > >> From: EXT Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com] > >> Sent: Saturday, November 28, 2015 7:50 PM > >> To: Krzeminski, Marcin (Nokia - PL/Wroclaw) > >> Cc: qemu-devel@nongnu.org; git@xilinx.com; Sai Pavan Boddu > >> Subject: Re: [Qemu-devel] [PATCH] m25p80.c Added support for > N25Q256 > >> and N25Q512 > >> > >> These features are also available in Xilinx QEMU if you want to > >> compare > >> implementation: > >> > >> > https://github.com/Xilinx/qemu/blob/pub/2015.2.plnx/hw/block/m25p80.c > >> > >> That work also handles the larger and newer Spansion flash parts, as > >> well as the quad and dual mode commands for QSPI (also features of > n25qXXX). > >> > > Too bad I did not checked xilix fork, so V2 of this patch does not make > sense since all is already implemented. > > V2 still makes sense. My V1 comments are pretty minor and that Xilinx work > will need cleanup before upstreaming. It is not a competing implementaiton > and the diff there will go down when it is rebased on yours. > Since this is start with open source, making this feature at least ready to pull seem to be worth to try. I'll prepare v2 when I got some free time. > > Why didn't xilinks merge it with mainline? > > There's a lot in that tree and Xilinx hasn't gotten around to it yet. > Yes, I noticed one interesting feature. > > Anyway, I do not like not commented reviews, so lets play the game... > > > >> On Sat, Nov 28, 2015 at 9:06 AM, Krzeminski, Marcin (Nokia - > >> PL/Wroclaw) <marcin.krzeminski@nokia.com> wrote: > >> > It is my first patch, so any comment are really welcome. > >> > > >> Check MAINTAINERS file for relevant people to CC. > >> > >> To make informal comments on your patches, you but them below the > line ... > >> > >> > Changes: > >> > * Removed unused variable > >> > * Added support for n25q256a and n25q512a > >> > * Added support for 4bytes address mode > >> > >> 4 byte addressing is a feature common to more than just n25qXXX. It > >> should be done as a separate prepended patch. > >> > >> > * Added support for banked read mode > >> > * Added support for sw reset flash commands > >> > * Added Read Flag Status register command support > >> > > >> > >> Basically these bullets should indicate separate patches to ease review. > >> > >> > Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com> > >> > --- > >> > >> ... here. So when the maintainers apply the patch they are not > >> committed to the git logs. > >> > > Thanks. > >> > hw/block/m25p80.c | 94 > >> > +++++++++++++++++++++++++++++++++++++++++++++++++++---- > >> > 1 file changed, 88 insertions(+), 6 deletions(-) > >> > > >> > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index > >> > efc43dd..c8b92d8 100644 > >> > --- a/hw/block/m25p80.c > >> > +++ b/hw/block/m25p80.c > >> > @@ -47,6 +47,9 @@ > >> > */ > >> > #define WR_1 0x100 > >> > > >> > +/* 16 MiB max in 3 byte address mode */ #define MAX_3BYTES_SIZE > >> > +0x1000000 > >> > + > >> > typedef struct FlashPartInfo { > >> > const char *part_name; > >> > /* jedec code. (jedec >> 16) & 0xff is the 1st byte, >> 8 the > >> > 2nd etc */ @@ -206,6 +209,8 @@ static const FlashPartInfo > >> > known_devices[] = { > >> > > >> > /* Numonyx -- n25q128 */ > >> > { INFO("n25q128", 0x20ba18, 0, 64 << 10, 256, 0) }, > >> > + { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512, ER_4K) }, > >> > + { INFO("n25q512a", 0x20ba20, 0, 64 << 10, 1024, ER_4K) }, > >> > }; > >> > > >> > typedef enum { > >> > @@ -216,6 +221,7 @@ typedef enum { > >> > WREN = 0x6, > >> > JEDEC_READ = 0x9f, > >> > BULK_ERASE = 0xc7, > >> > + READ_FSL = 0x70, > >> > >> Where does "FSL" come from? I am looking at an n25q256 datasheet here > >> that has this is "RFSR". > >> > >> http://datasheet.octopart.com/N25Q256A13E1241F-Micron-datasheet- > >> 11552757.pdf > >> > >> Admittedly, the vendors do tend to rename this stuff from > >> part-to-part. To keep consistent with surrounding code, this would be > READ_FSR. > > I seem it is a typo, it should be READ_FSR. > >> > >> > > >> > READ = 0x3, > >> > FAST_READ = 0xb, > >> > @@ -231,6 +237,15 @@ typedef enum { > >> > ERASE_4K = 0x20, > >> > ERASE_32K = 0x52, > >> > ERASE_SECTOR = 0xd8, > >> > + > >> > + ENTER_4BYTE_ADDR_MODE = 0xB7, > >> > + LEAVE_4BYTE_ADDR_MODE = 0xE9, > >> > + > >> > + EXTEND_ADDR_READ = 0xC8, > >> > + EXTEND_ADDR_WRITE = 0xC5, > >> > + > >> > >> Same data sheet has: WREAR, RDEAR, EN4BYTEADDR ... but Xilinx code > >> has something different again. In both cases, it is shorter, so I > >> think this should just be something shorter. > >> > > True. > >> > + RESET_ENABLE = 0x66, > >> > + RESET_MEMORY = 0x99, > >> > } FlashCMD; > >> > > >> > typedef enum { > >> > @@ -244,8 +259,6 @@ typedef enum { > >> > typedef struct Flash { > >> > SSISlave parent_obj; > >> > > >> > - uint32_t r; > >> > - > >> > >> Even the trivial cleanup can be a separate patch. > >> > >> > BlockBackend *blk; > >> > > >> > uint8_t *storage; > >> > @@ -260,6 +273,9 @@ typedef struct Flash { > >> > uint8_t cmd_in_progress; > >> > uint64_t cur_addr; > >> > bool write_enable; > >> > + bool four_bytes_address_mode; > >> > + bool reset_enable; > >> > + uint8_t extended_addr_reg; > >> > >> The datasheets abbreviate this to "EAR" so I think the same should be > >> done in code. > >> > >> > > >> > int64_t dirty_page; > >> > > >> > @@ -397,9 +413,17 @@ void flash_write8(Flash *s, uint64_t addr, > >> > uint8_t data) > >> > > >> > static void complete_collecting_data(Flash *s) { > >> > - s->cur_addr = s->data[0] << 16; > >> > - s->cur_addr |= s->data[1] << 8; > >> > - s->cur_addr |= s->data[2]; > >> > + if (s->four_bytes_address_mode) { > >> > + s->cur_addr = s->data[0] << 24; > >> > + s->cur_addr |= s->data[1] << 16; > >> > + s->cur_addr |= s->data[2] << 8; > >> > + s->cur_addr |= s->data[3]; > >> > + } else { > >> > + s->cur_addr = s->data[0] << 16; > >> > + s->cur_addr |= s->data[1] << 8; > >> > + s->cur_addr |= s->data[2]; > >> > + s->cur_addr += (s->extended_addr_reg&0x3)*MAX_3BYTES_SIZE; > >> > + } > >> > >> This can share implementation between 3 byte and 4 byte mode. From > >> the Xilinx work: > >> > >> static inline void do_4_byte_address(Flash *s) { > >> s->cur_addr <<= 8; > >> s->cur_addr |= s->data[3]; > >> } > >> > >> #define BAR_7_4_BYTE_ADDR (1<<7) > >> > >> static inline void check_4_byte_address(Flash *s) { > >> /* Allow 4byte address if MSB of bar register is set to 1 > >> * Or if 4byte addressing is allowed. > >> */ > >> if ((s->bar & BAR_7_4_BYTE_ADDR) || s->addr_4b) { > >> do_4_byte_address(s); > >> } else { > >> s->cur_addr |= s->bar << 24; > >> } > >> } > >> > >> Which also handles case instructions where the 4 byte addresses comes > >> as data on the wire. For your feature set it would be more minimal than > this. > >> > >> > > >> > > >> > s->state = STATE_IDLE; > >> > > >> > @@ -427,11 +452,28 @@ static void complete_collecting_data(Flash *s) > >> > s->write_enable = false; > >> > } > >> > break; > >> > + case EXTEND_ADDR_WRITE: > >> > + s->extended_addr_reg = s->data[0]; > >> > + break; > >> > default: > >> > break; > >> > } > >> > } > >> > > >> > +static void reset_memory(Flash *s) { > >> > + s->cmd_in_progress = NOP; > >> > + s->cur_addr = 0; > >> > + s->extended_addr_reg = 0; > >> > + s->four_bytes_address_mode = false; > >> > + s->len = 0; > >> > + s->needed_bytes = 0; > >> > + s->pos = 0; > >> > + s->state = STATE_IDLE; > >> > + s->write_enable = false; > >> > + s->reset_enable = false; > >> > +} > >> > + > >> > static void decode_new_cmd(Flash *s, uint32_t value) { > >> > s->cmd_in_progress = value; > >> > @@ -446,7 +488,7 @@ static void decode_new_cmd(Flash *s, uint32_t > >> value) > >> > case DPP: > >> > case QPP: > >> > case PP: > >> > - s->needed_bytes = 3; > >> > + s->needed_bytes = s->four_bytes_address_mode ? 4 : 3; > >> > s->pos = 0; > >> > s->len = 0; > >> > s->state = STATE_COLLECTING_DATA; @@ -514,6 +556,13 @@ > >> > static void decode_new_cmd(Flash *s, uint32_t value) > >> > s->state = STATE_READING_DATA; > >> > break; > >> > > >> > + case READ_FSL: > >> > + s->data[0] = (1<<7); /*Indicates flash is ready */ > >> > + s->pos = 0; > >> > + s->len = 1; > >> > >> IIRC, this command will continue to read out the same data byte > >> continuously until a new command is issued. For this reason, the > >> Xilinx work has a feature where commands can be marked looping. This > >> confused some drivers we had. > >> > > Haven't observed that problem, but it is possible. > > Yeh you don't need this for V2, just a heads up. > > >> > + s->state = STATE_READING_DATA; > >> > + break; > >> > + > >> > case JEDEC_READ: > >> > DB_PRINT_L(0, "populated jedec code\n"); > >> > s->data[0] = (s->pi->jedec >> 16) & 0xff; @@ -541,6 > >> > +590,34 @@ static void decode_new_cmd(Flash *s, uint32_t value) > >> > break; > >> > case NOP: > >> > break; > >> > + case ENTER_4BYTE_ADDR_MODE: > >> > + s->four_bytes_address_mode = true; > >> > + break; > >> > + case LEAVE_4BYTE_ADDR_MODE: > >> > + s->four_bytes_address_mode = false; > >> > + break; > >> > + case EXTEND_ADDR_READ: > >> > + s->data[0] = s->extended_addr_reg; > >> > + s->pos = 0; > >> > + s->len = 1; > >> > + s->state = STATE_READING_DATA; > >> > + break; > >> > + case EXTEND_ADDR_WRITE: > >> > + if (s->write_enable) { > >> > + s->needed_bytes = 1; > >> > + s->pos = 0; > >> > + s->len = 0; > >> > + s->state = STATE_COLLECTING_DATA; > >> > + } > >> > + break; > >> > + case RESET_ENABLE: > >> > + s->reset_enable = true; > >> > + break; > >> > + case RESET_MEMORY: > >> > + if (s->reset_enable) { > >> > + reset_memory(s); > >> > + } > >> > + break; > >> > default: > >> > qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Unknown cmd > >> %x\n", value); > >> > break; > >> > @@ -622,6 +699,8 @@ static int m25p80_init(SSISlave *ss) > >> > s->size = s->pi->sector_size * s->pi->n_sectors; > >> > s->dirty_page = -1; > >> > > >> > + reset_memory(s); > >> > + > >> > >> Resets should be handled in the device->reset callback rather than > >> the > >> init() function. > >> > > That is intentionally - I want to emulated case where guest reboots does no > trigger flash reset. > > For final solution I thought about property that tells if reset pin is used or > not. > > > > This means that the device->reset as-is needs rework to handle your case as > the semantics of the device->reset is a power-on reset. init() should never > implement any form of reset. If the current functionality there contains > components that are a combination of soft reset and hard reset it should be > split to two functions. The hard reset stuff is called as device->reset. The soft > components are then triggered but the IO events that happen on your guest > reboot. Hard reset can call soft reset if it is a functional superset. > Yes, that is the idea. Thanks, Marcin > Regards, > Peter > > > Thanks, > > Marcin > >> Regards, > >> Peter > >> > >> > /* FIXME use a qdev drive property instead of drive_get_next() */ > >> > dinfo = drive_get_next(IF_MTD); > >> > > >> > @@ -666,6 +745,9 @@ static const VMStateDescription > vmstate_m25p80 > >> > = > >> { > >> > VMSTATE_UINT8(cmd_in_progress, Flash), > >> > VMSTATE_UINT64(cur_addr, Flash), > >> > VMSTATE_BOOL(write_enable, Flash), > >> > + VMSTATE_BOOL(four_bytes_address_mode, Flash), > >> > + VMSTATE_UINT8(extended_addr_reg, Flash), > >> > + VMSTATE_BOOL(reset_enable, Flash), > >> > VMSTATE_END_OF_LIST() > >> > } > >> > }; > >> > -- > >> > 1.9.1 > >> > > >> > Regards, > >> > Marcin Krzeminski > >> >
On Mon, Nov 30, 2015 at 11:51 AM, Krzeminski, Marcin (Nokia - PL/Wroclaw) <marcin.krzeminski@nokia.com> wrote: > > >> -----Original Message----- >> From: EXT Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com] >> Sent: Sunday, November 29, 2015 8:19 PM >> To: Krzeminski, Marcin (Nokia - PL/Wroclaw) >> Cc: qemu-devel@nongnu.org; git@xilinx.com; Sai Pavan Boddu >> Subject: Re: [Qemu-devel] [PATCH] m25p80.c Added support for N25Q256 >> and N25Q512 >> >> On Sun, Nov 29, 2015 at 5:12 AM, Krzeminski, Marcin (Nokia - >> PL/Wroclaw) <marcin.krzeminski@nokia.com> wrote: >> > >> > >> >> -----Original Message----- >> >> From: EXT Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com] >> >> Sent: Saturday, November 28, 2015 7:50 PM >> >> To: Krzeminski, Marcin (Nokia - PL/Wroclaw) >> >> Cc: qemu-devel@nongnu.org; git@xilinx.com; Sai Pavan Boddu >> >> Subject: Re: [Qemu-devel] [PATCH] m25p80.c Added support for >> N25Q256 >> >> and N25Q512 >> >> >> >> These features are also available in Xilinx QEMU if you want to >> >> compare >> >> implementation: >> >> >> >> >> https://github.com/Xilinx/qemu/blob/pub/2015.2.plnx/hw/block/m25p80.c >> >> >> >> That work also handles the larger and newer Spansion flash parts, as >> >> well as the quad and dual mode commands for QSPI (also features of >> n25qXXX). >> >> >> > Too bad I did not checked xilix fork, so V2 of this patch does not make >> sense since all is already implemented. >> >> V2 still makes sense. My V1 comments are pretty minor and that Xilinx work >> will need cleanup before upstreaming. It is not a competing implementaiton >> and the diff there will go down when it is rebased on yours. >> > Since this is start with open source, making this feature at least ready to pull seem to be > worth to try. I'll prepare v2 when I got some free time. There is a learning curve for some people on getting into patch respinning. You need some features of git that let you rewrite history. Specifically you want to look at: git rebase -i git add -p git commit --amend If you are unfamiliar. >> > Why didn't xilinks merge it with mainline? >> >> There's a lot in that tree and Xilinx hasn't gotten around to it yet. >> > Yes, I noticed one interesting feature. Are you able to share more? :) If there is known community interest in specific features it helps upstreaming. Regards, Peter
> -----Original Message----- > From: EXT Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com] > Sent: Monday, November 30, 2015 9:55 PM > To: Krzeminski, Marcin (Nokia - PL/Wroclaw) > Cc: qemu-devel@nongnu.org; git@xilinx.com; Sai Pavan Boddu > Subject: Re: [Qemu-devel] [PATCH] m25p80.c Added support for N25Q256 > and N25Q512 > > On Mon, Nov 30, 2015 at 11:51 AM, Krzeminski, Marcin (Nokia - > PL/Wroclaw) <marcin.krzeminski@nokia.com> wrote: > > > > > >> -----Original Message----- > >> From: EXT Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com] > >> Sent: Sunday, November 29, 2015 8:19 PM > >> To: Krzeminski, Marcin (Nokia - PL/Wroclaw) > >> Cc: qemu-devel@nongnu.org; git@xilinx.com; Sai Pavan Boddu > >> Subject: Re: [Qemu-devel] [PATCH] m25p80.c Added support for > N25Q256 > >> and N25Q512 > >> > >> On Sun, Nov 29, 2015 at 5:12 AM, Krzeminski, Marcin (Nokia - > >> PL/Wroclaw) <marcin.krzeminski@nokia.com> wrote: > >> > > >> > > >> >> -----Original Message----- > >> >> From: EXT Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com] > >> >> Sent: Saturday, November 28, 2015 7:50 PM > >> >> To: Krzeminski, Marcin (Nokia - PL/Wroclaw) > >> >> Cc: qemu-devel@nongnu.org; git@xilinx.com; Sai Pavan Boddu > >> >> Subject: Re: [Qemu-devel] [PATCH] m25p80.c Added support for > >> N25Q256 > >> >> and N25Q512 > >> >> > >> >> These features are also available in Xilinx QEMU if you want to > >> >> compare > >> >> implementation: > >> >> > >> >> > >> > https://github.com/Xilinx/qemu/blob/pub/2015.2.plnx/hw/block/m25p80.c > >> >> > >> >> That work also handles the larger and newer Spansion flash parts, > >> >> as well as the quad and dual mode commands for QSPI (also features > >> >> of > >> n25qXXX). > >> >> > >> > Too bad I did not checked xilix fork, so V2 of this patch does not > >> > make > >> sense since all is already implemented. > >> > >> V2 still makes sense. My V1 comments are pretty minor and that Xilinx > >> work will need cleanup before upstreaming. It is not a competing > >> implementaiton and the diff there will go down when it is rebased on > yours. > >> > > Since this is start with open source, making this feature at least > > ready to pull seem to be worth to try. I'll prepare v2 when I got some free > time. > > There is a learning curve for some people on getting into patch respinning. > You need some features of git that let you rewrite history. Specifically you > want to look at: > > git rebase -i > git add -p > git commit --amend > > If you are unfamiliar. > For me it is much more complicated. I added support for those flash devices in one commit, then made one or two fixes and that's it. It is really hard to get patch set from that.. The way of working is the main issue here... > >> > Why didn't xilinks merge it with mainline? > >> > >> There's a lot in that tree and Xilinx hasn't gotten around to it yet. > >> > > Yes, I noticed one interesting feature. > > Are you able to share more? :) If there is known community interest in > specific features it helps upstreaming. > Yes, most interesting thing is the I2C eeproms (m24cxx.c file). There is smbus eeprom, but this is not exactly I2C. At least from my point of view it is interesting to have it in upstream :) Regards, Marcin > Regards, > Peter
On Mon, Nov 30, 2015 at 1:33 PM, Krzeminski, Marcin (Nokia - PL/Wroclaw) <marcin.krzeminski@nokia.com> wrote: > > >> -----Original Message----- >> From: EXT Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com] >> Sent: Monday, November 30, 2015 9:55 PM >> To: Krzeminski, Marcin (Nokia - PL/Wroclaw) >> Cc: qemu-devel@nongnu.org; git@xilinx.com; Sai Pavan Boddu >> Subject: Re: [Qemu-devel] [PATCH] m25p80.c Added support for N25Q256 >> and N25Q512 >> >> On Mon, Nov 30, 2015 at 11:51 AM, Krzeminski, Marcin (Nokia - >> PL/Wroclaw) <marcin.krzeminski@nokia.com> wrote: >> > >> > >> >> -----Original Message----- >> >> From: EXT Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com] >> >> Sent: Sunday, November 29, 2015 8:19 PM >> >> To: Krzeminski, Marcin (Nokia - PL/Wroclaw) >> >> Cc: qemu-devel@nongnu.org; git@xilinx.com; Sai Pavan Boddu >> >> Subject: Re: [Qemu-devel] [PATCH] m25p80.c Added support for >> N25Q256 >> >> and N25Q512 >> >> >> >> On Sun, Nov 29, 2015 at 5:12 AM, Krzeminski, Marcin (Nokia - >> >> PL/Wroclaw) <marcin.krzeminski@nokia.com> wrote: >> >> > >> >> > >> >> >> -----Original Message----- >> >> >> From: EXT Peter Crosthwaite [mailto:crosthwaitepeter@gmail.com] >> >> >> Sent: Saturday, November 28, 2015 7:50 PM >> >> >> To: Krzeminski, Marcin (Nokia - PL/Wroclaw) >> >> >> Cc: qemu-devel@nongnu.org; git@xilinx.com; Sai Pavan Boddu >> >> >> Subject: Re: [Qemu-devel] [PATCH] m25p80.c Added support for >> >> N25Q256 >> >> >> and N25Q512 >> >> >> >> >> >> These features are also available in Xilinx QEMU if you want to >> >> >> compare >> >> >> implementation: >> >> >> >> >> >> >> >> >> https://github.com/Xilinx/qemu/blob/pub/2015.2.plnx/hw/block/m25p80.c >> >> >> >> >> >> That work also handles the larger and newer Spansion flash parts, >> >> >> as well as the quad and dual mode commands for QSPI (also features >> >> >> of >> >> n25qXXX). >> >> >> >> >> > Too bad I did not checked xilix fork, so V2 of this patch does not >> >> > make >> >> sense since all is already implemented. >> >> >> >> V2 still makes sense. My V1 comments are pretty minor and that Xilinx >> >> work will need cleanup before upstreaming. It is not a competing >> >> implementaiton and the diff there will go down when it is rebased on >> yours. >> >> >> > Since this is start with open source, making this feature at least >> > ready to pull seem to be worth to try. I'll prepare v2 when I got some free >> time. >> >> There is a learning curve for some people on getting into patch respinning. >> You need some features of git that let you rewrite history. Specifically you >> want to look at: >> >> git rebase -i >> git add -p >> git commit --amend >> >> If you are unfamiliar. >> > For me it is much more complicated. I added support for those flash devices in one commit, > then made one or two fixes and that's it. It is really hard to get patch set from that.. > The way of working is the main issue here... > git reset HEAD^ then git add -p. It lets you interactively select hunks to commit. Then when you git commit only the ones you selected are committed. Use "e" mode if you have a single hunk with multiple changes. Keep adding and commiting until you have no diff. Regards, Peter >> >> > Why didn't xilinks merge it with mainline? >> >> >> >> There's a lot in that tree and Xilinx hasn't gotten around to it yet. >> >> >> > Yes, I noticed one interesting feature. >> >> Are you able to share more? :) If there is known community interest in >> specific features it helps upstreaming. >> > Yes, most interesting thing is the I2C eeproms (m24cxx.c file). There is smbus eeprom, > but this is not exactly I2C. At least from my point of view it is interesting to have it in upstream :) > > Regards, > Marcin > >> Regards, >> Peter
diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index efc43dd..c8b92d8 100644 --- a/hw/block/m25p80.c +++ b/hw/block/m25p80.c @@ -47,6 +47,9 @@ */ #define WR_1 0x100 +/* 16 MiB max in 3 byte address mode */ +#define MAX_3BYTES_SIZE 0x1000000 + typedef struct FlashPartInfo { const char *part_name; /* jedec code. (jedec >> 16) & 0xff is the 1st byte, >> 8 the 2nd etc */ @@ -206,6 +209,8 @@ static const FlashPartInfo known_devices[] = { /* Numonyx -- n25q128 */ { INFO("n25q128", 0x20ba18, 0, 64 << 10, 256, 0) }, + { INFO("n25q256a", 0x20ba19, 0, 64 << 10, 512, ER_4K) }, + { INFO("n25q512a", 0x20ba20, 0, 64 << 10, 1024, ER_4K) }, }; typedef enum { @@ -216,6 +221,7 @@ typedef enum { WREN = 0x6, JEDEC_READ = 0x9f, BULK_ERASE = 0xc7, + READ_FSL = 0x70, READ = 0x3, FAST_READ = 0xb, @@ -231,6 +237,15 @@ typedef enum { ERASE_4K = 0x20, ERASE_32K = 0x52, ERASE_SECTOR = 0xd8, + + ENTER_4BYTE_ADDR_MODE = 0xB7, + LEAVE_4BYTE_ADDR_MODE = 0xE9, + + EXTEND_ADDR_READ = 0xC8, + EXTEND_ADDR_WRITE = 0xC5, + + RESET_ENABLE = 0x66, + RESET_MEMORY = 0x99, } FlashCMD; typedef enum { @@ -244,8 +259,6 @@ typedef enum { typedef struct Flash { SSISlave parent_obj; - uint32_t r; - BlockBackend *blk; uint8_t *storage; @@ -260,6 +273,9 @@ typedef struct Flash { uint8_t cmd_in_progress; uint64_t cur_addr; bool write_enable; + bool four_bytes_address_mode; + bool reset_enable; + uint8_t extended_addr_reg; int64_t dirty_page; @@ -397,9 +413,17 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t data) static void complete_collecting_data(Flash *s) { - s->cur_addr = s->data[0] << 16; - s->cur_addr |= s->data[1] << 8; - s->cur_addr |= s->data[2]; + if (s->four_bytes_address_mode) { + s->cur_addr = s->data[0] << 24; + s->cur_addr |= s->data[1] << 16; + s->cur_addr |= s->data[2] << 8; + s->cur_addr |= s->data[3]; + } else { + s->cur_addr = s->data[0] << 16; + s->cur_addr |= s->data[1] << 8; + s->cur_addr |= s->data[2]; + s->cur_addr += (s->extended_addr_reg&0x3)*MAX_3BYTES_SIZE; + } s->state = STATE_IDLE;
It is my first patch, so any comment are really welcome. Changes: * Removed unused variable * Added support for n25q256a and n25q512a * Added support for 4bytes address mode * Added support for banked read mode * Added support for sw reset flash commands * Added Read Flag Status register command support Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com> --- hw/block/m25p80.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 88 insertions(+), 6 deletions(-) @@ -427,11 +452,28 @@ static void complete_collecting_data(Flash *s) s->write_enable = false; } break; + case EXTEND_ADDR_WRITE: + s->extended_addr_reg = s->data[0]; + break; default: break; } } +static void reset_memory(Flash *s) +{ + s->cmd_in_progress = NOP; + s->cur_addr = 0; + s->extended_addr_reg = 0; + s->four_bytes_address_mode = false; + s->len = 0; + s->needed_bytes = 0; + s->pos = 0; + s->state = STATE_IDLE; + s->write_enable = false; + s->reset_enable = false; +} + static void decode_new_cmd(Flash *s, uint32_t value) { s->cmd_in_progress = value; @@ -446,7 +488,7 @@ static void decode_new_cmd(Flash *s, uint32_t value) case DPP: case QPP: case PP: - s->needed_bytes = 3; + s->needed_bytes = s->four_bytes_address_mode ? 4 : 3; s->pos = 0; s->len = 0; s->state = STATE_COLLECTING_DATA; @@ -514,6 +556,13 @@ static void decode_new_cmd(Flash *s, uint32_t value) s->state = STATE_READING_DATA; break; + case READ_FSL: + s->data[0] = (1<<7); /*Indicates flash is ready */ + s->pos = 0; + s->len = 1; + s->state = STATE_READING_DATA; + break; + case JEDEC_READ: DB_PRINT_L(0, "populated jedec code\n"); s->data[0] = (s->pi->jedec >> 16) & 0xff; @@ -541,6 +590,34 @@ static void decode_new_cmd(Flash *s, uint32_t value) break; case NOP: break; + case ENTER_4BYTE_ADDR_MODE: + s->four_bytes_address_mode = true; + break; + case LEAVE_4BYTE_ADDR_MODE: + s->four_bytes_address_mode = false; + break; + case EXTEND_ADDR_READ: + s->data[0] = s->extended_addr_reg; + s->pos = 0; + s->len = 1; + s->state = STATE_READING_DATA; + break; + case EXTEND_ADDR_WRITE: + if (s->write_enable) { + s->needed_bytes = 1; + s->pos = 0; + s->len = 0; + s->state = STATE_COLLECTING_DATA; + } + break; + case RESET_ENABLE: + s->reset_enable = true; + break; + case RESET_MEMORY: + if (s->reset_enable) { + reset_memory(s); + } + break; default: qemu_log_mask(LOG_GUEST_ERROR, "M25P80: Unknown cmd %x\n", value); break; @@ -622,6 +699,8 @@ static int m25p80_init(SSISlave *ss) s->size = s->pi->sector_size * s->pi->n_sectors; s->dirty_page = -1; + reset_memory(s); + /* FIXME use a qdev drive property instead of drive_get_next() */ dinfo = drive_get_next(IF_MTD); @@ -666,6 +745,9 @@ static const VMStateDescription vmstate_m25p80 = { VMSTATE_UINT8(cmd_in_progress, Flash), VMSTATE_UINT64(cur_addr, Flash), VMSTATE_BOOL(write_enable, Flash), + VMSTATE_BOOL(four_bytes_address_mode, Flash), + VMSTATE_UINT8(extended_addr_reg, Flash), + VMSTATE_BOOL(reset_enable, Flash), VMSTATE_END_OF_LIST() } };