diff mbox

[v2,4/4] ARM: exynos4210_pmu: Add software reset support

Message ID 1342112068-23345-5-git-send-email-m.kozlov@samsung.com
State New
Headers show

Commit Message

Maksim E. Kozlov July 12, 2012, 4:54 p.m. UTC 
Signed-off-by: Maksim Kozlov <m.kozlov@samsung.com>
---
 hw/exynos4210_pmu.c |   40 +++++++++++++++++++++++++++++++++-------
 1 files changed, 33 insertions(+), 7 deletions(-)

Comments

Peter Maydell July 20, 2012, 2:32 p.m. UTC | #1 
On 12 July 2012 17:54, Maksim Kozlov <m.kozlov@samsung.com> wrote:
> Signed-off-by: Maksim Kozlov <m.kozlov@samsung.com>
> ---
>  hw/exynos4210_pmu.c |   40 +++++++++++++++++++++++++++++++++-------
>  1 files changed, 33 insertions(+), 7 deletions(-)
>
> diff --git a/hw/exynos4210_pmu.c b/hw/exynos4210_pmu.c
> index 7f09c79..96588d9 100644
> --- a/hw/exynos4210_pmu.c
> +++ b/hw/exynos4210_pmu.c
> @@ -18,13 +18,8 @@
>   *  with this program; if not, see <http://www.gnu.org/licenses/>.
>   */
>
> -/*
> - * This model implements PMU registers just as a bulk of memory. Currently,
> - * the only reason this device exists is that secondary CPU boot loader
> - * uses PMU INFORM5 register as a holding pen.
> - */
> -
>  #include "sysbus.h"
> +#include "sysemu.h"
>
>  #ifndef DEBUG_PMU
>  #define DEBUG_PMU           0
> @@ -230,6 +225,8 @@
>
>  #define EXYNOS4210_PMU_REGS_MEM_SIZE 0x3d0c
>
> +#define SWRESET_SYSTEM_MASK     0x00000001
> +
>  typedef struct Exynos4210PmuReg {
>      const char  *name; /* for debug only */
>      uint32_t     offset;
> @@ -458,7 +455,17 @@ static void exynos4210_pmu_write(void *opaque, target_phys_addr_t offset,
>      PRINT_DEBUG_EXTEND("%s [0x%04x] <- 0x%04x\n",
>               exynos4210_pmu_regs[index].name, (uint32_t)offset, (uint32_t)val);
>
> -    s->reg[index] = val;
> +    switch (offset) {
> +    case SWRESET:
> +        if (val & SWRESET_SYSTEM_MASK) {
> +            s->reg[index] = val;
> +            qemu_system_reset_request();
> +        }
> +        break;
> +    default:
> +        s->reg[index] = val;
> +        break;
> +    }
>  }
>
>  static const MemoryRegionOps exynos4210_pmu_ops = {
> @@ -477,9 +484,28 @@ static void exynos4210_pmu_reset(DeviceState *dev)
>      Exynos4210PmuState *s =
>              container_of(dev, Exynos4210PmuState, busdev.qdev);
>      unsigned i;
> +    uint32_t index = exynos4210_pmu_get_register_index(s, SWRESET);
> +    uint32_t swreset = s->reg[index];
>
>      /* Set default values for registers */
>      for (i = 0; i < PMU_NUM_OF_REGISTERS; i++) {
> +        if (swreset) {
> +            switch (exynos4210_pmu_regs[i].offset) {
> +            case INFORM0:
> +            case INFORM1:
> +            case INFORM2:
> +            case INFORM3:
> +            case INFORM4:
> +            case INFORM5:
> +            case INFORM6:
> +            case INFORM7:
> +            case PS_HOLD_CONTROL:
> +                /* keep these registers during SW reset */
> +                continue;
> +            default:
> +                break;
> +            }
> +        }
>          s->reg[i] = exynos4210_pmu_regs[i].reset_value;
>      }
>  }

This patch seems to be trying to make a distinction that QEMU doesn't
support, ie between system wide "software reset" and system wide
"hard reset". I'm not convinced about the wisdom of trying to paper
over this lack with a single-device workaround.

-- PMM
Maksim E. Kozlov July 23, 2012, 6:38 a.m. UTC | #2 
20.07.2012 18:32, Peter Maydell пишет:
> On 12 July 2012 17:54, Maksim Kozlov<m.kozlov@samsung.com>  wrote:
>> Signed-off-by: Maksim Kozlov<m.kozlov@samsung.com>
>> ---
>>   hw/exynos4210_pmu.c |   40 +++++++++++++++++++++++++++++++++-------
>>   1 files changed, 33 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/exynos4210_pmu.c b/hw/exynos4210_pmu.c
>> index 7f09c79..96588d9 100644
>> --- a/hw/exynos4210_pmu.c
>> +++ b/hw/exynos4210_pmu.c
>> @@ -18,13 +18,8 @@
>>    *  with this program; if not, see<http://www.gnu.org/licenses/>.
>>    */
>>
>> -/*
>> - * This model implements PMU registers just as a bulk of memory. Currently,
>> - * the only reason this device exists is that secondary CPU boot loader
>> - * uses PMU INFORM5 register as a holding pen.
>> - */
>> -
>>   #include "sysbus.h"
>> +#include "sysemu.h"
>>
>>   #ifndef DEBUG_PMU
>>   #define DEBUG_PMU           0
>> @@ -230,6 +225,8 @@
>>
>>   #define EXYNOS4210_PMU_REGS_MEM_SIZE 0x3d0c
>>
>> +#define SWRESET_SYSTEM_MASK     0x00000001
>> +
>>   typedef struct Exynos4210PmuReg {
>>       const char  *name; /* for debug only */
>>       uint32_t     offset;
>> @@ -458,7 +455,17 @@ static void exynos4210_pmu_write(void *opaque, target_phys_addr_t offset,
>>       PRINT_DEBUG_EXTEND("%s [0x%04x]<- 0x%04x\n",
>>                exynos4210_pmu_regs[index].name, (uint32_t)offset, (uint32_t)val);
>>
>> -    s->reg[index] = val;
>> +    switch (offset) {
>> +    case SWRESET:
>> +        if (val&  SWRESET_SYSTEM_MASK) {
>> +            s->reg[index] = val;
>> +            qemu_system_reset_request();
>> +        }
>> +        break;
>> +    default:
>> +        s->reg[index] = val;
>> +        break;
>> +    }
>>   }
>>
>>   static const MemoryRegionOps exynos4210_pmu_ops = {
>> @@ -477,9 +484,28 @@ static void exynos4210_pmu_reset(DeviceState *dev)
>>       Exynos4210PmuState *s =
>>               container_of(dev, Exynos4210PmuState, busdev.qdev);
>>       unsigned i;
>> +    uint32_t index = exynos4210_pmu_get_register_index(s, SWRESET);
>> +    uint32_t swreset = s->reg[index];
>>
>>       /* Set default values for registers */
>>       for (i = 0; i<  PMU_NUM_OF_REGISTERS; i++) {
>> +        if (swreset) {
>> +            switch (exynos4210_pmu_regs[i].offset) {
>> +            case INFORM0:
>> +            case INFORM1:
>> +            case INFORM2:
>> +            case INFORM3:
>> +            case INFORM4:
>> +            case INFORM5:
>> +            case INFORM6:
>> +            case INFORM7:
>> +            case PS_HOLD_CONTROL:
>> +                /* keep these registers during SW reset */
>> +                continue;
>> +            default:
>> +                break;
>> +            }
>> +        }
>>           s->reg[i] = exynos4210_pmu_regs[i].reset_value;
>>       }
>>   }
>
> This patch seems to be trying to make a distinction that QEMU doesn't
> support, ie between system wide "software reset" and system wide
> "hard reset". I'm not convinced about the wisdom of trying to paper
> over this lack with a single-device workaround.

Ok. Thank for review

And I found out that this patch contains a mistake, therefore it should 
be rejected in any case

>
> -- PMM
>
>
Maksim E. Kozlov July 23, 2012, 11:01 a.m. UTC | #3 
20.07.2012 18:32, Peter Maydell пишет:
> On 12 July 2012 17:54, Maksim Kozlov<m.kozlov@samsung.com>  wrote:
>> Signed-off-by: Maksim Kozlov<m.kozlov@samsung.com>
>> ---
>>   hw/exynos4210_pmu.c |   40 +++++++++++++++++++++++++++++++++-------
>>   1 files changed, 33 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/exynos4210_pmu.c b/hw/exynos4210_pmu.c
>> index 7f09c79..96588d9 100644
>> --- a/hw/exynos4210_pmu.c
>> +++ b/hw/exynos4210_pmu.c
>> @@ -18,13 +18,8 @@
>>    *  with this program; if not, see<http://www.gnu.org/licenses/>.
>>    */
>>
>> -/*
>> - * This model implements PMU registers just as a bulk of memory. Currently,
>> - * the only reason this device exists is that secondary CPU boot loader
>> - * uses PMU INFORM5 register as a holding pen.
>> - */
>> -
>>   #include "sysbus.h"
>> +#include "sysemu.h"
>>
>>   #ifndef DEBUG_PMU
>>   #define DEBUG_PMU           0
>> @@ -230,6 +225,8 @@
>>
>>   #define EXYNOS4210_PMU_REGS_MEM_SIZE 0x3d0c
>>
>> +#define SWRESET_SYSTEM_MASK     0x00000001
>> +
>>   typedef struct Exynos4210PmuReg {
>>       const char  *name; /* for debug only */
>>       uint32_t     offset;
>> @@ -458,7 +455,17 @@ static void exynos4210_pmu_write(void *opaque, target_phys_addr_t offset,
>>       PRINT_DEBUG_EXTEND("%s [0x%04x]<- 0x%04x\n",
>>                exynos4210_pmu_regs[index].name, (uint32_t)offset, (uint32_t)val);
>>
>> -    s->reg[index] = val;
>> +    switch (offset) {
>> +    case SWRESET:
>> +        if (val&  SWRESET_SYSTEM_MASK) {
>> +            s->reg[index] = val;
>> +            qemu_system_reset_request();
>> +        }
>> +        break;
>> +    default:
>> +        s->reg[index] = val;
>> +        break;
>> +    }
>>   }
>>
>>   static const MemoryRegionOps exynos4210_pmu_ops = {
>> @@ -477,9 +484,28 @@ static void exynos4210_pmu_reset(DeviceState *dev)
>>       Exynos4210PmuState *s =
>>               container_of(dev, Exynos4210PmuState, busdev.qdev);
>>       unsigned i;
>> +    uint32_t index = exynos4210_pmu_get_register_index(s, SWRESET);
>> +    uint32_t swreset = s->reg[index];
>>
>>       /* Set default values for registers */
>>       for (i = 0; i<  PMU_NUM_OF_REGISTERS; i++) {
>> +        if (swreset) {
>> +            switch (exynos4210_pmu_regs[i].offset) {
>> +            case INFORM0:
>> +            case INFORM1:
>> +            case INFORM2:
>> +            case INFORM3:
>> +            case INFORM4:
>> +            case INFORM5:
>> +            case INFORM6:
>> +            case INFORM7:
>> +            case PS_HOLD_CONTROL:
>> +                /* keep these registers during SW reset */
>> +                continue;
>> +            default:
>> +                break;
>> +            }
>> +        }
>>           s->reg[i] = exynos4210_pmu_regs[i].reset_value;
>>       }
>>   }
> This patch seems to be trying to make a distinction that QEMU doesn't
> support, ie between system wide "software reset" and system wide
> "hard reset". I'm not convinced about the wisdom of trying to paper
> over this lack with a single-device workaround.
Peter, if we add callback (*swreset)() into DeviceClass, whether it will 
be an acceptable solution? And devices which have different behavior 
during swreset can register different function for that. What do you 
think about this?
>
> -- PMM
>
Peter Maydell July 23, 2012, 11:05 a.m. UTC | #4 
On 23 July 2012 12:01, Maksim Kozlov <m.kozlov@samsung.com> wrote:
> 20.07.2012 18:32, Peter Maydell пишет:
>> This patch seems to be trying to make a distinction that QEMU doesn't
>> support, ie between system wide "software reset" and system wide
>> "hard reset". I'm not convinced about the wisdom of trying to paper
>> over this lack with a single-device workaround.
>
> Peter, if we add callback (*swreset)() into DeviceClass, whether it will be
> an acceptable solution? And devices which have different behavior during
> swreset can register different function for that. What do you think about
> this?

I think reset is a key part of device lifecycle and adding support
for 'warm reset' or whatever needs general discussion so we can get
the semantics right... cc'ing a couple of people who might have opinions.

-- PMM
diff mbox

Patch

diff --git a/hw/exynos4210_pmu.c b/hw/exynos4210_pmu.c
index 7f09c79..96588d9 100644
--- a/hw/exynos4210_pmu.c
+++ b/hw/exynos4210_pmu.c
@@ -18,13 +18,8 @@ 
  *  with this program; if not, see <http://www.gnu.org/licenses/>.
  */
 
-/*
- * This model implements PMU registers just as a bulk of memory. Currently,
- * the only reason this device exists is that secondary CPU boot loader
- * uses PMU INFORM5 register as a holding pen.
- */
-
 #include "sysbus.h"
+#include "sysemu.h"
 
 #ifndef DEBUG_PMU
 #define DEBUG_PMU           0
@@ -230,6 +225,8 @@ 
 
 #define EXYNOS4210_PMU_REGS_MEM_SIZE 0x3d0c
 
+#define SWRESET_SYSTEM_MASK     0x00000001
+
 typedef struct Exynos4210PmuReg {
     const char  *name; /* for debug only */
     uint32_t     offset;
@@ -458,7 +455,17 @@  static void exynos4210_pmu_write(void *opaque, target_phys_addr_t offset,
     PRINT_DEBUG_EXTEND("%s [0x%04x] <- 0x%04x\n",
              exynos4210_pmu_regs[index].name, (uint32_t)offset, (uint32_t)val);
 
-    s->reg[index] = val;
+    switch (offset) {
+    case SWRESET:
+        if (val & SWRESET_SYSTEM_MASK) {
+            s->reg[index] = val;
+            qemu_system_reset_request();
+        }
+        break;
+    default:
+        s->reg[index] = val;
+        break;
+    }
 }
 
 static const MemoryRegionOps exynos4210_pmu_ops = {
@@ -477,9 +484,28 @@  static void exynos4210_pmu_reset(DeviceState *dev)
     Exynos4210PmuState *s =
             container_of(dev, Exynos4210PmuState, busdev.qdev);
     unsigned i;
+    uint32_t index = exynos4210_pmu_get_register_index(s, SWRESET);
+    uint32_t swreset = s->reg[index];
 
     /* Set default values for registers */
     for (i = 0; i < PMU_NUM_OF_REGISTERS; i++) {
+        if (swreset) {
+            switch (exynos4210_pmu_regs[i].offset) {
+            case INFORM0:
+            case INFORM1:
+            case INFORM2:
+            case INFORM3:
+            case INFORM4:
+            case INFORM5:
+            case INFORM6:
+            case INFORM7:
+            case PS_HOLD_CONTROL:
+                /* keep these registers during SW reset */
+                continue;
+            default:
+                break;
+            }
+        }
         s->reg[i] = exynos4210_pmu_regs[i].reset_value;
     }
 }