[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: |
Sun, 29 Nov 2015 11:19:28 -0800 |
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
>> >
- [Qemu-devel] [PATCH] m25p80.c Added support for N25Q256 and N25Q512, Krzeminski, Marcin (Nokia - PL/Wroclaw), 2015/11/28
- Re: [Qemu-devel] [PATCH] m25p80.c Added support for N25Q256 and N25Q512, Peter Crosthwaite, 2015/11/28
- Re: [Qemu-devel] [PATCH] m25p80.c Added support for N25Q256 and N25Q512, Krzeminski, Marcin (Nokia - PL/Wroclaw), 2015/11/29
- Re: [Qemu-devel] [PATCH] m25p80.c Added support for N25Q256 and N25Q512,
Peter Crosthwaite <=
- Re: [Qemu-devel] [PATCH] m25p80.c Added support for N25Q256 and N25Q512, Krzeminski, Marcin (Nokia - PL/Wroclaw), 2015/11/30
- Re: [Qemu-devel] [PATCH] m25p80.c Added support for N25Q256 and N25Q512, Peter Crosthwaite, 2015/11/30
- Re: [Qemu-devel] [PATCH] m25p80.c Added support for N25Q256 and N25Q512, Krzeminski, Marcin (Nokia - PL/Wroclaw), 2015/11/30
- Re: [Qemu-devel] [PATCH] m25p80.c Added support for N25Q256 and N25Q512, Peter Crosthwaite, 2015/11/30