> -----Original Message-----
> From: EXT Peter Crosthwaite [mailto:crosthwaitepe...@gmail.com]
> Sent: Saturday, November 28, 2015 7:50 PM
> To: Krzeminski, Marcin (Nokia - PL/Wroclaw)
> Cc: qemu-devel@nongnu.org; g...@xilinx.com; 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) <marcin.krzemin...@nokia.com> 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 <marcin.krzemin...@nokia.com>
> > ---
>
> ... 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
> >
