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

[Peter Crosthwaite]

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.

> Why didn't xilinks merge it with mainline?

There's a lot in that tree and Xilinx hasn't gotten around to it yet.

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

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