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: 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.
Why didn't xilinks merge it with mainline?
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.
> > +        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.

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
> >