Re: [Qemu-devel] [PATCH] m25p80.c Added support for N25Q256 and N25Q512

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

> -----Original Message-----
> From: EXT Peter Crosthwaite [mailto:address@hidden
> Sent: Sunday, November 29, 2015 8:19 PM
> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> Cc: address@hidden; address@hidden; 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) <address@hidden> wrote:
> >
> >
> >> -----Original Message-----
> >> From: EXT Peter Crosthwaite [mailto:address@hidden
> >> Sent: Saturday, November 28, 2015 7:50 PM
> >> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> >> Cc: address@hidden; address@hidden; 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) <address@hidden> 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 <address@hidden>
> >> > ---
> >>
> >> ... 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
> >> >