qemu-devel
[Top][All Lists]
Advanced

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

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


From: Peter Crosthwaite
Subject: Re: [Qemu-devel] [PATCH] m25p80.c Added support for N25Q256 and N25Q512
Date: Sat, 28 Nov 2015 10:50:25 -0800

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

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.

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

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

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

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

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
>



reply via email to

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