Re: [Qemu-devel] [PATCH 10/12] Support for quad commands.

[Lenkow, Pawel (Nokia - PL/Wroclaw)]

> -----Original Message-----
> From: EXT Peter Crosthwaite [mailto:address@hidden
> Sent: Wednesday, December 23, 2015 3:25 AM
> To: Krzeminski, Marcin (Nokia - PL/Wroclaw); Cédric Le Goater
> Cc: address@hidden Developers; Lenkow, Pawel (Nokia -
> PL/Wroclaw)
> Subject: Re: [Qemu-devel] [PATCH 10/12] Support for quad commands.
> 
> On Tue, Dec 22, 2015 at 1:40 PM, Peter Crosthwaite
> <address@hidden> wrote:
> > On Wed, Dec 16, 2015 at 4:57 AM,  <address@hidden> wrote:
> >> From: Marcin Krzeminski <address@hidden>
> >>
> >> Signed-off-by: Pawel Lenkow <address@hidden>
> >> ---
> >>  hw/block/m25p80.c | 38 ++++++++++++++++++++++++++++++++++++-
> -
> >>  1 file changed, 36 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> >> index 6fc55a3..25ec666 100644
> >> --- a/hw/block/m25p80.c
> >> +++ b/hw/block/m25p80.c
> >> @@ -255,19 +255,24 @@ typedef enum {
> >>      BULK_ERASE = 0xc7,
> >>
> >>      READ = 0x3,
> >> +    READ4 = 0x13,
> >>      FAST_READ = 0xb,
> >>      DOR = 0x3b,
> >>      QOR = 0x6b,
> >>      DIOR = 0xbb,
> >>      QIOR = 0xeb,
> >> +    QIOR4 = 0xec,
> >>
> >>      PP = 0x2,
> >> +    PP4 = 0x12,
> >>      DPP = 0xa2,
> >>      QPP = 0x32,
> >>
> >>      ERASE_4K = 0x20,
> >> +    ERASE4_4K = 0x21,
> >>      ERASE_32K = 0x52,
> >>      ERASE_SECTOR = 0xd8,
> >> +    ERASE4_SECTOR = 0xdc,
> >>
> >>      EN_4BYTE_ADDR = 0xB7,
> >>      EX_4BYTE_ADDR = 0xE9,
> >> @@ -307,6 +312,7 @@ typedef struct Flash {
> >>      bool write_enable;
> >>      bool four_bytes_address_mode;
> >>      bool reset_enable;
> >> +    bool quad_enable;
> >>      bool initialized;
> >>      uint8_t reset_pin;
> >>      uint8_t ear;
> >> @@ -381,6 +387,7 @@ static void flash_erase(Flash *s, int offset,
> FlashCMD cmd)
> >>
> >>      switch (cmd) {
> >>      case ERASE_4K:
> >> +    case ERASE4_4K:
> >>          len = 4 << 10;
> >>          capa_to_assert = ER_4K;
> >>          break;
> >> @@ -389,6 +396,7 @@ static void flash_erase(Flash *s, int offset,
> FlashCMD cmd)
> >>          capa_to_assert = ER_32K;
> >>          break;
> >>      case ERASE_SECTOR:
> >> +    case ERASE4_SECTOR:
> >>          len = s->pi->sector_size;
> >>          break;
> >>      case BULK_ERASE:
> >> @@ -447,6 +455,14 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t
> data)
> >>
> >>  static inline int is_4bytes(Flash *s)
> >>  {
> >> +   switch (s->cmd_in_progress) {
> >> +   case PP4:
> >> +   case READ4:
> >> +   case QIOR4:
> >> +   case ERASE4_4K:
> >> +   case ERASE4_SECTOR:
> >> +       return 1;
> >> +   default:
> >>         return s->four_bytes_address_mode;
> >>     }
> >>  }
> >> @@ -472,19 +488,24 @@ static void complete_collecting_data(Flash *s)
> >>      case DPP:
> >>      case QPP:
> >>      case PP:
> >> +    case PP4:
> >>          s->state = STATE_PAGE_PROGRAM;
> >>          break;
> >>      case READ:
> >> +    case READ4:
> >>      case FAST_READ:
> >>      case DOR:
> >>      case QOR:
> >>      case DIOR:
> >>      case QIOR:
> >> +    case QIOR4:
> >>          s->state = STATE_READ;
> >>          break;
> >>      case ERASE_4K:
> >> +    case ERASE4_4K:
> >>      case ERASE_32K:
> >>      case ERASE_SECTOR:
> >> +    case ERASE4_SECTOR:
> >>          flash_erase(s, s->cur_addr, s->cmd_in_progress);
> >>          break;
> >>      case WRSR:
> >> @@ -512,6 +533,7 @@ static void reset_memory(Flash *s)
> >>      s->state = STATE_IDLE;
> >>      s->write_enable = false;
> >>      s->reset_enable = false;
> >> +    s->quad_enable = false;
> >>
> >>      DB_PRINT_L(0, "Reset done.\n");
> >>  }
> >> @@ -519,18 +541,23 @@ static void reset_memory(Flash *s)
> >>  static void decode_new_cmd(Flash *s, uint32_t value)
> >>  {
> >>      s->cmd_in_progress = value;
> >> +    int i;
> >>      DB_PRINT_L(0, "decoded new command:%x\n", value);
> >>
> >>      switch (value) {
> >>
> >>      case ERASE_4K:
> >> +    case ERASE4_4K:
> >>      case ERASE_32K:
> >>      case ERASE_SECTOR:
> >> +    case ERASE4_SECTOR:
> >>      case READ:
> >> +    case READ4:
> >>      case DPP:
> >>      case QPP:
> >>      case PP:
> >> -        s->needed_bytes = 3;
> >> +    case PP4:
> >> +        s->needed_bytes = is_4bytes(s) ? 4 : 3;
> >>          s->pos = 0;
> >>          s->len = 0;
> >>          s->state = STATE_COLLECTING_DATA;
> >> @@ -574,7 +601,13 @@ static void decode_new_cmd(Flash *s, uint32_t
> value)
> >>          s->len = 0;
> >>          s->state = STATE_COLLECTING_DATA;
> >>          break;
> >> -
> >> +    case QIOR4:
> >> +        /* 4 address + 1 dummy */
> >> +        s->needed_bytes = 5;
> >
> > So looking at the current QIOR handling, the number of "dummy bytes"
> > is calculated as:
> >
> > num_dummy_cycles X 4.
> >
> > This makes sense, as both the address and data-phase on either side of
> > the dummy phase is done on four-wires, so it is cleaner that the dummy
> > phase is also on 4 wires. Ideally we implement Dual and Quad mode
> > awareness and cycle accuracy on the SSI layer itself to handle these
> > cases, but until then we stuck with this strange fixed policy.
> >
> > For numonyx, this gives 1 command + 3 address + 4 dummies ( == 8
> > cycles) = 8 needed_bytes.
> >
> 
> Correction, there is no command byte in the needed bytes, rather there
> a 5 dummies (for 10 cycles).
> 
> Regards,
> Peter
> 

Yes you are right, I will fixed it.
Micron has by default 10 cycles for QIOR so it will be 5 bytes extra.
I need to check to Spansion datasheets, because there is no clear for me how
many dummy cycles are required. 


> > So this would be 9 for the extra address byte in 4b mode.
> >
> >> +        s->pos = 0;
> >> +        s->len = 0;
> >> +        s->state = STATE_COLLECTING_DATA;
> >> +        break;
> >
> > Following on from, Cedric's spot, should this be a fallthrough to
> > regular QIOR, which then uses is_4bytes to implement the +1 on needed
> > bytes?


Yes, good idea.
Br,

Pawel Lenkow


> >
> > Regards,
> > Peter
> >
> >>      case WRSR:
> >>          if (s->write_enable) {
> >>              s->needed_bytes = 1;
> >> @@ -792,6 +825,7 @@ static const VMStateDescription vmstate_m25p80
> = {
> >>          VMSTATE_BOOL(four_bytes_address_mode, Flash),
> >>          VMSTATE_UINT8(ear, Flash),
> >>          VMSTATE_BOOL(reset_enable, Flash),
> >> +        VMSTATE_BOOL(quad_enable, Flash),
> >>          VMSTATE_BOOL(initialized, Flash),
> >>          VMSTATE_UINT8(reset_pin, Flash),
> >>          VMSTATE_END_OF_LIST()
> >> --
> >> 2.5.0
> >>
> >>