On Sun, Nov 29, 2015 at 5:12 AM, Krzeminski, Marcin (Nokia - PL/Wroclaw) <marcin.krzemin...@nokia.com> wrote: > > >> -----Original Message----- >> From: EXT Peter Crosthwaite [mailto:crosthwaitepe...@gmail.com] >> Sent: Saturday, November 28, 2015 7:50 PM >> To: Krzeminski, Marcin (Nokia - PL/Wroclaw) >> Cc: qemu-devel@nongnu.org; g...@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.krzemin...@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.krzemin...@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 >> >