> -----Original Message-----
> From: EXT Peter Crosthwaite [mailto:crosthwaitepe...@gmail.com]
> Sent: Sunday, November 29, 2015 8:19 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
>
> On Sun, Nov 29, 2015 at 5:12 AM, Krzeminski, Marcin (Nokia -
> PL/Wroclaw) <marcin.krzemin...@nokia.com> wrote:
> >
> >
> >> -----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.
>
> 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) <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.
>
> 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
> >> >
