diff mbox

[v4,02/11] block: m25p80: RESET_ENABLE and RESET_MEMORY commnads

Message ID 1456128205-5092-3-git-send-email-marcin.krzeminski@nokia.com (mailing list archive)
State New, archived
Headers show

Commit Message

marcin.krzeminski@nokia.com Feb. 22, 2016, 8:03 a.m. UTC 
From: Marcin Krzeminski <marcin.krzeminski@nokia.com>

Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
---
 hw/block/m25p80.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

Comments

Peter Crosthwaite March 17, 2016, 5:42 p.m. UTC | #1 
"commands" in commit msg

On Mon, Feb 22, 2016 at 12:03 AM,  <marcin.krzeminski@nokia.com> wrote:
> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
>
> Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
> ---
>  hw/block/m25p80.c | 35 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 2222124..06b0af3 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -233,6 +233,9 @@ typedef enum {
>      ERASE_4K = 0x20,
>      ERASE_32K = 0x52,
>      ERASE_SECTOR = 0xd8,
> +
> +    RESET_ENABLE = 0x66,
> +    RESET_MEMORY = 0x99,
>  } FlashCMD;
>
>  typedef enum {
> @@ -260,6 +263,7 @@ typedef struct Flash {
>      uint8_t cmd_in_progress;
>      uint64_t cur_addr;
>      bool write_enable;
> +    bool reset_enable;
>
>      int64_t dirty_page;
>
> @@ -432,11 +436,29 @@ static void complete_collecting_data(Flash *s)
>      }
>  }
>
> +static void reset_memory(Flash *s)
> +{
> +    s->cmd_in_progress = NOP;
> +    s->cur_addr = 0;
> +    s->len = 0;
> +    s->needed_bytes = 0;
> +    s->pos = 0;
> +    s->state = STATE_IDLE;
> +    s->write_enable = false;
> +    s->reset_enable = false;
> +
> +    DB_PRINT_L(0, "Reset done.\n");
> +}
> +
>  static void decode_new_cmd(Flash *s, uint32_t value)
>  {
>      s->cmd_in_progress = value;
>      DB_PRINT_L(0, "decoded new command:%x\n", value);
>
> +    if (value != RESET_MEMORY) {
> +        s->reset_enable = false;
> +    }
> +
>      switch (value) {
>
>      case ERASE_4K:
> @@ -541,6 +563,14 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>          break;
>      case NOP:
>          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 +652,8 @@ static int m25p80_init(SSISlave *ss)
>      s->size = s->pi->sector_size * s->pi->n_sectors;
>      s->dirty_page = -1;
>
> +    reset_memory(s);
> +

This shouldn't be here, you need to add a Device::reset function. Your
use case of persisting data through a warn system reset (that we
discussed previously) is difficult to support with QEMUs current reset
semantics and your board is out-of-tree. So I think this should be in
the Device::reset and we need to revisit your unique use case along
with the addition of your board.

So with the move to device::reset,

Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>

>      /* FIXME use a qdev drive property instead of drive_get_next() */
>      dinfo = drive_get_next(IF_MTD);
>
> @@ -654,7 +686,7 @@ static void m25p80_pre_save(void *opaque)
>
>  static const VMStateDescription vmstate_m25p80 = {
>      .name = "xilinx_spi",
> -    .version_id = 1,
> +    .version_id = 2,
>      .minimum_version_id = 1,
>      .pre_save = m25p80_pre_save,
>      .fields = (VMStateField[]) {
> @@ -666,6 +698,7 @@ static const VMStateDescription vmstate_m25p80 = {
>          VMSTATE_UINT8(cmd_in_progress, Flash),
>          VMSTATE_UINT64(cur_addr, Flash),
>          VMSTATE_BOOL(write_enable, Flash),
> +        VMSTATE_BOOL(reset_enable, Flash),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> --
> 2.5.0
>
marcin.krzeminski@nokia.com March 17, 2016, 9:54 p.m. UTC | #2 
W dniu 17.03.2016 o 18:42, Peter Crosthwaite pisze:
> "commands" in commit msg
>
> On Mon, Feb 22, 2016 at 12:03 AM,  <marcin.krzeminski@nokia.com> wrote:
>> From: Marcin Krzeminski <marcin.krzeminski@nokia.com>
>>
>> Signed-off-by: Marcin Krzeminski <marcin.krzeminski@nokia.com>
>> ---
>>  hw/block/m25p80.c | 35 ++++++++++++++++++++++++++++++++++-
>>  1 file changed, 34 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 2222124..06b0af3 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -233,6 +233,9 @@ typedef enum {
>>      ERASE_4K = 0x20,
>>      ERASE_32K = 0x52,
>>      ERASE_SECTOR = 0xd8,
>> +
>> +    RESET_ENABLE = 0x66,
>> +    RESET_MEMORY = 0x99,
>>  } FlashCMD;
>>
>>  typedef enum {
>> @@ -260,6 +263,7 @@ typedef struct Flash {
>>      uint8_t cmd_in_progress;
>>      uint64_t cur_addr;
>>      bool write_enable;
>> +    bool reset_enable;
>>
>>      int64_t dirty_page;
>>
>> @@ -432,11 +436,29 @@ static void complete_collecting_data(Flash *s)
>>      }
>>  }
>>
>> +static void reset_memory(Flash *s)
>> +{
>> +    s->cmd_in_progress = NOP;
>> +    s->cur_addr = 0;
>> +    s->len = 0;
>> +    s->needed_bytes = 0;
>> +    s->pos = 0;
>> +    s->state = STATE_IDLE;
>> +    s->write_enable = false;
>> +    s->reset_enable = false;
>> +
>> +    DB_PRINT_L(0, "Reset done.\n");
>> +}
>> +
>>  static void decode_new_cmd(Flash *s, uint32_t value)
>>  {
>>      s->cmd_in_progress = value;
>>      DB_PRINT_L(0, "decoded new command:%x\n", value);
>>
>> +    if (value != RESET_MEMORY) {
>> +        s->reset_enable = false;
>> +    }
>> +
>>      switch (value) {
>>
>>      case ERASE_4K:
>> @@ -541,6 +563,14 @@ static void decode_new_cmd(Flash *s, uint32_t value)
>>          break;
>>      case NOP:
>>          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 +652,8 @@ static int m25p80_init(SSISlave *ss)
>>      s->size = s->pi->sector_size * s->pi->n_sectors;
>>      s->dirty_page = -1;
>>
>> +    reset_memory(s);
>> +
>
> This shouldn't be here, you need to add a Device::reset function. Your
> use case of persisting data through a warn system reset (that we
> discussed previously) is difficult to support with QEMUs current reset
> semantics and your board is out-of-tree. So I think this should be in
> the Device::reset and we need to revisit your unique use case along
> with the addition of your board.
My impression was, that I should remove all "warm reset" additions I made
(including dc::reset) and move reset_memory call to init that is why it is here.
I will move it to dc:reset in v5.

Thanks,
Marcin
>
>
> So with the move to device::reset,
>
> Reviewed-by: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>
>>      /* FIXME use a qdev drive property instead of drive_get_next() */
>>      dinfo = drive_get_next(IF_MTD);
>>
>> @@ -654,7 +686,7 @@ static void m25p80_pre_save(void *opaque)
>>
>>  static const VMStateDescription vmstate_m25p80 = {
>>      .name = "xilinx_spi",
>> -    .version_id = 1,
>> +    .version_id = 2,
>>      .minimum_version_id = 1,
>>      .pre_save = m25p80_pre_save,
>>      .fields = (VMStateField[]) {
>> @@ -666,6 +698,7 @@ static const VMStateDescription vmstate_m25p80 = {
>>          VMSTATE_UINT8(cmd_in_progress, Flash),
>>          VMSTATE_UINT64(cur_addr, Flash),
>>          VMSTATE_BOOL(write_enable, Flash),
>> +        VMSTATE_BOOL(reset_enable, Flash),
>>          VMSTATE_END_OF_LIST()
>>      }
>>  };
>> --
>> 2.5.0
>>
>
>
diff mbox

Patch

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 2222124..06b0af3 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -233,6 +233,9 @@  typedef enum {
     ERASE_4K = 0x20,
     ERASE_32K = 0x52,
     ERASE_SECTOR = 0xd8,
+
+    RESET_ENABLE = 0x66,
+    RESET_MEMORY = 0x99,
 } FlashCMD;
 
 typedef enum {
@@ -260,6 +263,7 @@  typedef struct Flash {
     uint8_t cmd_in_progress;
     uint64_t cur_addr;
     bool write_enable;
+    bool reset_enable;
 
     int64_t dirty_page;
 
@@ -432,11 +436,29 @@  static void complete_collecting_data(Flash *s)
     }
 }
 
+static void reset_memory(Flash *s)
+{
+    s->cmd_in_progress = NOP;
+    s->cur_addr = 0;
+    s->len = 0;
+    s->needed_bytes = 0;
+    s->pos = 0;
+    s->state = STATE_IDLE;
+    s->write_enable = false;
+    s->reset_enable = false;
+
+    DB_PRINT_L(0, "Reset done.\n");
+}
+
 static void decode_new_cmd(Flash *s, uint32_t value)
 {
     s->cmd_in_progress = value;
     DB_PRINT_L(0, "decoded new command:%x\n", value);
 
+    if (value != RESET_MEMORY) {
+        s->reset_enable = false;
+    }
+
     switch (value) {
 
     case ERASE_4K:
@@ -541,6 +563,14 @@  static void decode_new_cmd(Flash *s, uint32_t value)
         break;
     case NOP:
         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 +652,8 @@  static int m25p80_init(SSISlave *ss)
     s->size = s->pi->sector_size * s->pi->n_sectors;
     s->dirty_page = -1;
 
+    reset_memory(s);
+
     /* FIXME use a qdev drive property instead of drive_get_next() */
     dinfo = drive_get_next(IF_MTD);
 
@@ -654,7 +686,7 @@  static void m25p80_pre_save(void *opaque)
 
 static const VMStateDescription vmstate_m25p80 = {
     .name = "xilinx_spi",
-    .version_id = 1,
+    .version_id = 2,
     .minimum_version_id = 1,
     .pre_save = m25p80_pre_save,
     .fields = (VMStateField[]) {
@@ -666,6 +698,7 @@  static const VMStateDescription vmstate_m25p80 = {
         VMSTATE_UINT8(cmd_in_progress, Flash),
         VMSTATE_UINT64(cur_addr, Flash),
         VMSTATE_BOOL(write_enable, Flash),
+        VMSTATE_BOOL(reset_enable, Flash),
         VMSTATE_END_OF_LIST()
     }
 };