qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 06/12] 4byte address mode support added.


From: Krzeminski, Marcin (Nokia - PL/Wroclaw)
Subject: Re: [Qemu-devel] [PATCH 06/12] 4byte address mode support added.
Date: Thu, 4 Feb 2016 11:58:30 +0000


> -----Original Message-----
> From: EXT Peter Crosthwaite [mailto:address@hidden
> Sent: Tuesday, December 22, 2015 10:29 PM
> To: Cédric Le Goater; address@hidden
> Cc: Krzeminski, Marcin (Nokia - PL/Wroclaw); address@hidden
> 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 <address@hidden> wrote:
> > Hello Marcin,
> >
> >
> > On 12/16/2015 01:57 PM, address@hidden wrote:
> >> From: Marcin Krzeminski <address@hidden>
> >>
> >> Signed-off-by: Marcin Krzeminski <address@hidden>
> >> ---
> >>  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),
> >>
> >
> >
> >

reply via email to

[Prev in Thread] Current Thread [Next in Thread]