Hello!

i.MX6 RM says that setting software reset bit in CR register of GPT 
(general purpose timers) should resets all of the registers of GPT to 
their default reset values, except for the CLKSRC, EN, ENMOD, STOPEN, 
WAITEN, and DBGEN bits in CR. But current implementation does the 
opposite for CR register (it clears CLKSRC and friends bits and 
preserves the others).

Most importantly this leads to that software reset bit doesn't clears 
automatically.

I have a look at git history and found that software reset bit was being 
cleared before 462566fc5e3 commit.

I have doubts about the correct fixing of this problem. I don't really 
understand the nature of the "/Soft reset doesn't touch some bits; hard 
reset clears them/" comment in /imx_gpt_reset/ function, does it mean 
that /imx_gpt_reset/ performs a hard reset or soft reset? I see two 
possible fixings:

1. If /imx_gpt_reset/ purpose is to do a software reset of device, then 
we should fix this function. My patch at the end of email fixes this 
function.

2. If /imx_gpt_reset/ purpose is to do a hard reset of device? then 
there should be another function to software reset of device. If so I 
can create a new patch.


---

 From e12f689a2f41d18a29714771d83e343ca5195b61 Mon Sep 17 00:00:00 2001

From: Kurban Mallachiev <mallachiev@ispras.ru>

Date: Fri, 17 Feb 2017 20:30:49 +0300

Subject: [PATCH] i.MX timers: fix software reset

Software reset function clears CR bits that should not clear and

preserves bits that should clear.

Signed-off-by: Kurban Mallachiev <mallachiev@ispras.ru>

---

  hw/timer/imx_gpt.c | 4 ++--

  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/timer/imx_gpt.c b/hw/timer/imx_gpt.c

index 010ccbf207..9777160f49 100644

--- a/hw/timer/imx_gpt.c

+++ b/hw/timer/imx_gpt.c

@@ -306,8 +306,8 @@ static void imx_gpt_reset(DeviceState *dev)

      /*

       * Soft reset doesn't touch some bits; hard reset clears them

       */

-    s->cr &= ~(GPT_CR_EN|GPT_CR_ENMOD|GPT_CR_STOPEN|GPT_CR_DOZEN|

-               GPT_CR_WAITEN|GPT_CR_DBGEN);

+    s->cr &= GPT_CR_EN|GPT_CR_ENMOD|GPT_CR_STOPEN|GPT_CR_DOZEN|

+               GPT_CR_WAITEN|GPT_CR_DBGEN;

      s->sr = 0;

      s->pr = 0;

      s->ir = 0;

-- 

2.11.1

On 17 February 2017 at 18:18, Kurban Mallachiev <mallachiev@ispras.ru> wrote:
> Hello!
>
> i.MX6 RM says that setting software reset bit in CR register of GPT (general
> purpose timers) should resets all of the registers of GPT to their default
> reset values, except for the CLKSRC, EN, ENMOD, STOPEN, WAITEN, and DBGEN
> bits in CR. But current implementation does the opposite for CR register (it
> clears CLKSRC and friends bits and preserves the others).
>
> Most importantly this leads to that software reset bit doesn't clears
> automatically.
>
> I have a look at git history and found that software reset bit was being
> cleared before 462566fc5e3 commit.
>
> I have doubts about the correct fixing of this problem. I don't really
> understand the nature of the "Soft reset doesn't touch some bits; hard reset
> clears them" comment in imx_gpt_reset function, does it mean that
> imx_gpt_reset performs a hard reset or soft reset? I see two possible
> fixings:
>
> 1. If imx_gpt_reset purpose is to do a software reset of device, then we
> should fix this function. My patch at the end of email fixes this function.
>
> 2. If imx_gpt_reset purpose is to do a hard reset of device? then there
> should be another function to software reset of device. If so I can create a
> new patch.

As the function registered via the DeviceState's dc->reset
pointer, imx_gpt_reset() has to behave as a "device was
powercycled" level reset. It looks like we also call it
from imx_gpt_write() when the guest does a write to a
particular register. If that guest-requested reset has
to have different behaviour from "device was powered off
and on again" then it needs to use a different function
and can't just call imx_gpt_reset().

thanks
-- PMM

Ok, thank you for reply.

Here is another patch. I have created different function for software 
reset (imx_gpt_soft_reset), and to prevent duplication extracted common 
code to imx_gpt_reset_common.

Take a look, please.

---
Subject: [PATCH] i.MX timers: fix software reset

Software reset function clears CR bits that should not clear and
preserve bits that should clear.

Signed-off-by: Kurban Mallachiev <mallachiev@ispras.ru>
---
  hw/timer/imx_gpt.c | 26 ++++++++++++++++++++------
  1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/hw/timer/imx_gpt.c b/hw/timer/imx_gpt.c
index 010ccbf207..f07ae3d9a4 100644
--- a/hw/timer/imx_gpt.c
+++ b/hw/timer/imx_gpt.c
@@ -296,18 +296,20 @@ static uint64_t imx_gpt_read(void *opaque, hwaddr 
offset, unsigned size)
      return reg_value;
  }

-static void imx_gpt_reset(DeviceState *dev)
+static void imx_gpt_reset_common(IMXGPTState *s, int is_soft_reset)
  {
-    IMXGPTState *s = IMX_GPT(dev);
-
      /* stop timer */
      ptimer_stop(s->timer);

      /*
       * Soft reset doesn't touch some bits; hard reset clears them
       */
-    s->cr &= ~(GPT_CR_EN|GPT_CR_ENMOD|GPT_CR_STOPEN|GPT_CR_DOZEN|
-               GPT_CR_WAITEN|GPT_CR_DBGEN);
+    if (is_soft_reset) {
+        s->cr &= GPT_CR_EN|GPT_CR_ENMOD|GPT_CR_STOPEN|GPT_CR_DOZEN|
+                   GPT_CR_WAITEN|GPT_CR_DBGEN;
+    } else {
+        s->cr = 0;
+    }
      s->sr = 0;
      s->pr = 0;
      s->ir = 0;
@@ -333,6 +335,18 @@ static void imx_gpt_reset(DeviceState *dev)
      }
  }

+static void imx_gpt_soft_reset(DeviceState *dev)
+{
+    IMXGPTState *s = IMX_GPT(dev);
+    imx_gpt_reset_common(s, 1);
+}
+
+static void imx_gpt_reset(DeviceState *dev)
+{
+    IMXGPTState *s = IMX_GPT(dev);
+    imx_gpt_reset_common(s, 0);
+}
+
  static void imx_gpt_write(void *opaque, hwaddr offset, uint64_t value,
                            unsigned size)
  {
@@ -348,7 +362,7 @@ static void imx_gpt_write(void *opaque, hwaddr 
offset, uint64_t value,
          s->cr = value & ~0x7c14;
          if (s->cr & GPT_CR_SWR) { /* force reset */
              /* handle the reset */
-            imx_gpt_reset(DEVICE(s));
+            imx_gpt_soft_reset(DEVICE(s));
          } else {
              /* set our freq, as the source might have changed */
              imx_gpt_set_freq(s);
-- 
2.11.1

On 18.02.2017 18:46, Peter Maydell wrote:
> On 17 February 2017 at 18:18, Kurban Mallachiev <mallachiev@ispras.ru> wrote:
>> Hello!
>>
>> i.MX6 RM says that setting software reset bit in CR register of GPT (general
>> purpose timers) should resets all of the registers of GPT to their default
>> reset values, except for the CLKSRC, EN, ENMOD, STOPEN, WAITEN, and DBGEN
>> bits in CR. But current implementation does the opposite for CR register (it
>> clears CLKSRC and friends bits and preserves the others).
>>
>> Most importantly this leads to that software reset bit doesn't clears
>> automatically.
>>
>> I have a look at git history and found that software reset bit was being
>> cleared before 462566fc5e3 commit.
>>
>> I have doubts about the correct fixing of this problem. I don't really
>> understand the nature of the "Soft reset doesn't touch some bits; hard reset
>> clears them" comment in imx_gpt_reset function, does it mean that
>> imx_gpt_reset performs a hard reset or soft reset? I see two possible
>> fixings:
>>
>> 1. If imx_gpt_reset purpose is to do a software reset of device, then we
>> should fix this function. My patch at the end of email fixes this function.
>>
>> 2. If imx_gpt_reset purpose is to do a hard reset of device? then there
>> should be another function to software reset of device. If so I can create a
>> new patch.
> As the function registered via the DeviceState's dc->reset
> pointer, imx_gpt_reset() has to behave as a "device was
> powercycled" level reset. It looks like we also call it
> from imx_gpt_write() when the guest does a write to a
> particular register. If that guest-requested reset has
> to have different behaviour from "device was powered off
> and on again" then it needs to use a different function
> and can't just call imx_gpt_reset().
>
> thanks
> -- PMM

Hello!

Problem: function imx_gpt_reset is used for soft (requested by guest) 
and hard resets. But soft and hard resets should have different 
behaviour (hard reset should clear all registers, while soft reset 
should preserve some bits).

Patch changelog:
v1 -> v2: use different approach, patch completely rewritten
v2 -> v3: in software reset add preserving of CLKSRC bit as manual says

---
Software reset function clears CR bits that should not be cleared and
preserve bits that should be cleared.

Signed-off-by: Kurban Mallachiev <mallachiev@ispras.ru>
---
  hw/timer/imx_gpt.c | 27 +++++++++++++++++++++------
  1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/hw/timer/imx_gpt.c b/hw/timer/imx_gpt.c
index 010ccbf207..3ea18ff5de 100644
--- a/hw/timer/imx_gpt.c
+++ b/hw/timer/imx_gpt.c
@@ -296,18 +296,21 @@ static uint64_t imx_gpt_read(void *opaque, hwaddr 
offset, unsigned size)
      return reg_value;
  }

-static void imx_gpt_reset(DeviceState *dev)
+static void imx_gpt_reset_common(IMXGPTState *s, int is_soft_reset)
  {
-    IMXGPTState *s = IMX_GPT(dev);
-
      /* stop timer */
      ptimer_stop(s->timer);

      /*
       * Soft reset doesn't touch some bits; hard reset clears them
       */
-    s->cr &= ~(GPT_CR_EN|GPT_CR_ENMOD|GPT_CR_STOPEN|GPT_CR_DOZEN|
-               GPT_CR_WAITEN|GPT_CR_DBGEN);
+    if (is_soft_reset) {
+        s->cr &= GPT_CR_EN|GPT_CR_ENMOD|GPT_CR_STOPEN|GPT_CR_DOZEN|
+                   GPT_CR_WAITEN|GPT_CR_DBGEN|
+                   GPT_CR_CLKSRC_MASK<<GPT_CR_CLKSRC_SHIFT;
+    } else {
+        s->cr = 0;
+    }
      s->sr = 0;
      s->pr = 0;
      s->ir = 0;
@@ -333,6 +336,18 @@ static void imx_gpt_reset(DeviceState *dev)
      }
  }

+static void imx_gpt_soft_reset(DeviceState *dev)
+{
+    IMXGPTState *s = IMX_GPT(dev);
+    imx_gpt_reset_common(s, 1);
+}
+
+static void imx_gpt_reset(DeviceState *dev)
+{
+    IMXGPTState *s = IMX_GPT(dev);
+    imx_gpt_reset_common(s, 0);
+}
+
  static void imx_gpt_write(void *opaque, hwaddr offset, uint64_t value,
                            unsigned size)
  {
@@ -348,7 +363,7 @@ static void imx_gpt_write(void *opaque, hwaddr 
offset, uint64_t value,
          s->cr = value & ~0x7c14;
          if (s->cr & GPT_CR_SWR) { /* force reset */
              /* handle the reset */
-            imx_gpt_reset(DEVICE(s));
+            imx_gpt_soft_reset(DEVICE(s));
          } else {
              /* set our freq, as the source might have changed */
              imx_gpt_set_freq(s);
-- 
2.11.1

On 19 February 2017 at 19:00, Kurban Mallachiev <mallachiev@ispras.ru> wrote:
> Hello!
>
> Problem: function imx_gpt_reset is used for soft (requested by guest) and
> hard resets. But soft and hard resets should have different behaviour (hard
> reset should clear all registers, while soft reset should preserve some
> bits).
>
> Patch changelog:
> v1 -> v2: use different approach, patch completely rewritten
> v2 -> v3: in software reset add preserving of CLKSRC bit as manual says
>
> ---
> Software reset function clears CR bits that should not be cleared and
> preserve bits that should be cleared.
>
> Signed-off-by: Kurban Mallachiev <mallachiev@ispras.ru>

Thanks for this patch -- I have added it to my target-arm queue
(with a few tweaks).

Unfortunately your mail client mangled the patch a bit, and I had
to apply it manually. I'm happy to do that for occasional contributions,
but if you're planning to submit a lot of patches in the near future
you might like to look at your patch submission workflow (we usually
recommend 'git send-email').

thanks
-- PMM