> -----Original Message----- > From: EXT Peter Crosthwaite [mailto:crosthwaitepe...@gmail.com] > Sent: Tuesday, December 22, 2015 10:29 PM > To: Cédric Le Goater; g...@xilinx.com > Cc: Krzeminski, Marcin (Nokia - PL/Wroclaw); qemu-devel@nongnu.org > Developers; Lenkow, Pawel (Nokia - PL/Wroclaw) > Subject: Re: [Qemu-devel] [PATCH 06/12] 4byte address mode support > added. > > On Tue, Dec 22, 2015 at 10:41 AM, Cédric Le Goater <c...@fr.ibm.com> wrote: > > Hello Marcin, > > > > > > On 12/16/2015 01:57 PM, marcin.krzemin...@nokia.com wrote: > >> From: Marcin Krzeminski <marcin.krzemin...@nokia.com> > >> > >> Signed-off-by: Marcin Krzeminski <marcin.krzemin...@nokia.com> > >> --- > >> hw/block/m25p80.c | 31 ++++++++++++++++++++++++++++--- > >> 1 file changed, 28 insertions(+), 3 deletions(-) > >> > >> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index > >> 1a547ae..6d5d90d 100644 > >> --- a/hw/block/m25p80.c > >> +++ b/hw/block/m25p80.c > >> @@ -237,6 +237,9 @@ typedef enum { > >> ERASE_32K = 0x52, > >> ERASE_SECTOR = 0xd8, > >> > >> + EN_4BYTE_ADDR = 0xB7, > >> + EX_4BYTE_ADDR = 0xE9, > >> + > >> RESET_ENABLE = 0x66, > >> RESET_MEMORY = 0x99, > >> > >> @@ -267,6 +270,7 @@ typedef struct Flash { > >> uint8_t cmd_in_progress; > >> uint64_t cur_addr; > >> bool write_enable; > >> + bool four_bytes_address_mode; > >> bool reset_enable; > >> bool initialized; > >> uint8_t reset_pin; > >> @@ -405,11 +409,24 @@ void flash_write8(Flash *s, uint64_t addr, > uint8_t data) > >> s->dirty_page = page; > >> } > >> > >> +static inline int is_4bytes(Flash *s) { > >> + return s->four_bytes_address_mode; > >> + } > >> +} > >> + > >> 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 (is_4bytes(s)) { > >> + 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->state = STATE_IDLE; > > > > > > Don't we need to also change 'needed_bytes' in the decode_new_cmd() > > routine to increase the number of bytes expected by some commands ? > > > > I think you are right, and it may be solved later in the series, from patch > 10: > > case QPP: > case PP: > - s->needed_bytes = 3; > + s->needed_bytes = is_4bytes(s) ? 4 : 3; > s->pos = 0; > s->len = 0; > s->state = STATE_COLLECTING_DATA; > > This hunk should be brought forward to this patch. > > > If so, we could add a width attribute to 'struct Flash' and to something > > like : > > > > @@ -260,6 +263,7 @@ typedef struct Flash { > > uint8_t cmd_in_progress; > > uint64_t cur_addr; > > bool write_enable; > > + uint8_t width; > > > > int64_t dirty_page; > > > > @@ -401,6 +405,10 @@ static void complete_collecting_data(Fla > > s->cur_addr |= s->data[1] << 8; > > s->cur_addr |= s->data[2]; > > > > + if (s->width == 4) { > > + s->cur_addr = s->cur_addr << 8 | s->data[4]; > > + } > > + > > s->state = STATE_IDLE; > > > > switch (s->cmd_in_progress) { > > @@ -446,7 +454,7 @@ static void decode_new_cmd(Flash *s, uin > > case DPP: > > case QPP: > > case PP: > > - s->needed_bytes = 3; > > + s->needed_bytes = s->width; > > s->pos = 0; > > s->len = 0; > > s->state = STATE_COLLECTING_DATA; > > @@ -644,6 +658,7 @@ static int m25p80_init(SSISlave *ss) > > memset(s->storage, 0xFF, s->size); > > } > > > > + s->width = 3; > > return 0; > > } > > > > > > > > QOR, DIOR, QIOR command also need a check. I suppose an address and > > some number of dummy bytes are expected before the fast read > command is done. > > I would need to check the datasheets. > > > > I just checked an n25q256 datasheet, and yes you are right. The same logic as > in the hunk above needs to apply to these commands. CC Xilinx, this bug is in > their tree as well I think. > > https://github.com/Xilinx/qemu/blob/pub/2015.2.plnx/hw/block/m25p80.c > > Where PP, READ and friends have the 4 byte correction logic based on > addr_4b but QIOR does not. > > Nice catch :) > > Regards, > Peter >
Hello Cedric, Sorry for late response. As peter has responded, needed bytes for 4bytes address mode/cmd length is handled partially (not for all commands). Dummy cycles are not handled since my QSPI controller model had a bug so I missed this feature. Thanks for finding - it will be covered in v2. Regards, Marcin > > Cheers, > > > > C. > > > >> @@ -446,6 +463,7 @@ static void reset_memory(Flash *s) { > >> s->cmd_in_progress = NOP; > >> s->cur_addr = 0; > >> + s->four_bytes_address_mode = false; > >> s->len = 0; > >> s->needed_bytes = 0; > >> s->pos = 0; > >> @@ -565,6 +583,12 @@ static void decode_new_cmd(Flash *s, uint32_t > value) > >> break; > >> case NOP: > >> break; > >> + case EN_4BYTE_ADDR: > >> + s->four_bytes_address_mode = true; > >> + break; > >> + case EX_4BYTE_ADDR: > >> + s->four_bytes_address_mode = false; > >> + break; > >> case RESET_ENABLE: > >> s->reset_enable = true; > >> break; > >> @@ -715,6 +739,7 @@ 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_BOOL(reset_enable, Flash), > >> VMSTATE_BOOL(initialized, Flash), > >> VMSTATE_UINT8(reset_pin, Flash), > >> > > > > > >