| Message ID | 20190702001301.4768-3-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/block/pflash_cfi01: Add DeviceReset() handler | expand |
On 7/1/19 8:12 PM, Philippe Mathieu-Daudé wrote: > In the "Read Array Flowchart" the command has a value of 0xFF. > > In the document [*] the "Read Array Flowchart", the READ_ARRAY > command has a value of 0xff. > > Use the correct value in the pflash model. > > There is no change of behavior in the guest, because: > - when the guest were sending 0xFF, the reset_flash label > was setting the command value as 0x00 > - 0x00 was used internally for READ_ARRAY > > [*] "Common Flash Interface (CFI) and Command Sets" > (Intel Application Note 646) > Appendix B "Basic Command Set" > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> I'm assuming you tested a guest and saw it sending 0xFF; and based entirely on the assumption that you tested this and it is not now worse: Reviewed-by: John Snow <jsnow@redhat.com> > --- > hw/block/pflash_cfi01.c | 15 ++++++--------- > 1 file changed, 6 insertions(+), 9 deletions(-) > > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c > index dcc9885bf0..743b5d5794 100644 > --- a/hw/block/pflash_cfi01.c > +++ b/hw/block/pflash_cfi01.c > @@ -280,10 +280,9 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset, > /* This should never happen : reset state & treat it as a read */ > DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd); > pfl->wcycle = 0; > - pfl->cmd = 0; > + pfl->cmd = 0xff; > /* fall through to read code */ > - case 0x00: > - /* Flash area read */ > + case 0xff: /* Read Array */ > ret = pflash_data_read(pfl, offset, width, be); > break; > case 0x10: /* Single byte program */ > @@ -449,8 +448,6 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset, > case 0: > /* read mode */ > switch (cmd) { > - case 0x00: /* ??? */ > - goto reset_flash; > case 0x10: /* Single Byte Program */ > case 0x40: /* Single Byte Program */ > DPRINTF("%s: Single Byte Program\n", __func__); > @@ -527,7 +524,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset, > if (cmd == 0xd0) { /* confirm */ > pfl->wcycle = 0; > pfl->status |= 0x80; > - } else if (cmd == 0xff) { /* read array mode */ > + } else if (cmd == 0xff) { /* Read Array */ > goto reset_flash; > } else > goto error_flash; > @@ -554,7 +551,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset, > } else if (cmd == 0x01) { > pfl->wcycle = 0; > pfl->status |= 0x80; > - } else if (cmd == 0xff) { > + } else if (cmd == 0xff) { /* read array mode */ > goto reset_flash; > } else { > DPRINTF("%s: Unknown (un)locking command\n", __func__); > @@ -646,7 +643,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset, > trace_pflash_reset(); > memory_region_rom_device_set_romd(&pfl->mem, true); > pfl->wcycle = 0; > - pfl->cmd = 0; > + pfl->cmd = 0xff; > } > > > @@ -762,7 +759,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) > } > > pfl->wcycle = 0; > - pfl->cmd = 0; > + pfl->cmd = 0xff; > pfl->status = 0; > /* Hardcoded CFI table */ > /* Standard "QRY" string */ >
On Mon, Jul 1, 2019 at 5:14 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > In the "Read Array Flowchart" the command has a value of 0xFF. > > In the document [*] the "Read Array Flowchart", the READ_ARRAY > command has a value of 0xff. > > Use the correct value in the pflash model. > > There is no change of behavior in the guest, because: > - when the guest were sending 0xFF, the reset_flash label > was setting the command value as 0x00 > - 0x00 was used internally for READ_ARRAY > > [*] "Common Flash Interface (CFI) and Command Sets" > (Intel Application Note 646) > Appendix B "Basic Command Set" > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > hw/block/pflash_cfi01.c | 15 ++++++--------- > 1 file changed, 6 insertions(+), 9 deletions(-) > > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c > index dcc9885bf0..743b5d5794 100644 > --- a/hw/block/pflash_cfi01.c > +++ b/hw/block/pflash_cfi01.c > @@ -280,10 +280,9 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset, > /* This should never happen : reset state & treat it as a read */ > DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd); > pfl->wcycle = 0; > - pfl->cmd = 0; > + pfl->cmd = 0xff; > /* fall through to read code */ > - case 0x00: > - /* Flash area read */ > + case 0xff: /* Read Array */ > ret = pflash_data_read(pfl, offset, width, be); > break; > case 0x10: /* Single byte program */ > @@ -449,8 +448,6 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset, > case 0: > /* read mode */ > switch (cmd) { > - case 0x00: /* ??? */ > - goto reset_flash; > case 0x10: /* Single Byte Program */ > case 0x40: /* Single Byte Program */ > DPRINTF("%s: Single Byte Program\n", __func__); > @@ -527,7 +524,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset, > if (cmd == 0xd0) { /* confirm */ > pfl->wcycle = 0; > pfl->status |= 0x80; > - } else if (cmd == 0xff) { /* read array mode */ > + } else if (cmd == 0xff) { /* Read Array */ > goto reset_flash; > } else > goto error_flash; > @@ -554,7 +551,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset, > } else if (cmd == 0x01) { > pfl->wcycle = 0; > pfl->status |= 0x80; > - } else if (cmd == 0xff) { > + } else if (cmd == 0xff) { /* read array mode */ > goto reset_flash; > } else { > DPRINTF("%s: Unknown (un)locking command\n", __func__); > @@ -646,7 +643,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset, > trace_pflash_reset(); > memory_region_rom_device_set_romd(&pfl->mem, true); > pfl->wcycle = 0; > - pfl->cmd = 0; > + pfl->cmd = 0xff; > } > > > @@ -762,7 +759,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) > } > > pfl->wcycle = 0; > - pfl->cmd = 0; > + pfl->cmd = 0xff; > pfl->status = 0; > /* Hardcoded CFI table */ > /* Standard "QRY" string */ > -- > 2.20.1 >
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c index dcc9885bf0..743b5d5794 100644 --- a/hw/block/pflash_cfi01.c +++ b/hw/block/pflash_cfi01.c @@ -280,10 +280,9 @@ static uint32_t pflash_read(PFlashCFI01 *pfl, hwaddr offset, /* This should never happen : reset state & treat it as a read */ DPRINTF("%s: unknown command state: %x\n", __func__, pfl->cmd); pfl->wcycle = 0; - pfl->cmd = 0; + pfl->cmd = 0xff; /* fall through to read code */ - case 0x00: - /* Flash area read */ + case 0xff: /* Read Array */ ret = pflash_data_read(pfl, offset, width, be); break; case 0x10: /* Single byte program */ @@ -449,8 +448,6 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset, case 0: /* read mode */ switch (cmd) { - case 0x00: /* ??? */ - goto reset_flash; case 0x10: /* Single Byte Program */ case 0x40: /* Single Byte Program */ DPRINTF("%s: Single Byte Program\n", __func__); @@ -527,7 +524,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset, if (cmd == 0xd0) { /* confirm */ pfl->wcycle = 0; pfl->status |= 0x80; - } else if (cmd == 0xff) { /* read array mode */ + } else if (cmd == 0xff) { /* Read Array */ goto reset_flash; } else goto error_flash; @@ -554,7 +551,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset, } else if (cmd == 0x01) { pfl->wcycle = 0; pfl->status |= 0x80; - } else if (cmd == 0xff) { + } else if (cmd == 0xff) { /* read array mode */ goto reset_flash; } else { DPRINTF("%s: Unknown (un)locking command\n", __func__); @@ -646,7 +643,7 @@ static void pflash_write(PFlashCFI01 *pfl, hwaddr offset, trace_pflash_reset(); memory_region_rom_device_set_romd(&pfl->mem, true); pfl->wcycle = 0; - pfl->cmd = 0; + pfl->cmd = 0xff; } @@ -762,7 +759,7 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp) } pfl->wcycle = 0; - pfl->cmd = 0; + pfl->cmd = 0xff; pfl->status = 0; /* Hardcoded CFI table */ /* Standard "QRY" string */
In the "Read Array Flowchart" the command has a value of 0xFF. In the document [*] the "Read Array Flowchart", the READ_ARRAY command has a value of 0xff. Use the correct value in the pflash model. There is no change of behavior in the guest, because: - when the guest were sending 0xFF, the reset_flash label was setting the command value as 0x00 - 0x00 was used internally for READ_ARRAY [*] "Common Flash Interface (CFI) and Command Sets" (Intel Application Note 646) Appendix B "Basic Command Set" Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- hw/block/pflash_cfi01.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)