1. Patchew
  2. QEMU
  3. View series

[PATCH] hw/timer/renesas_tmr.c cleanup read operation.

Yoshinori Sato posted 1 patch 3 years, 9 months ago
Test checkpatch passed
Test docker-mingw@fedora passed
Test FreeBSD passed
Test docker-quick@centos7 passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200711154242.41222-1-ysato@users.sourceforge.jp
Maintainers: Magnus Damm <magnus.damm@gmail.com>, Yoshinori Sato <ysato@users.sourceforge.jp>
hw/timer/renesas_tmr.c | 106 ++++++++++++++++++++++-------------------
1 file changed, 57 insertions(+), 49 deletions(-)
[PATCH] hw/timer/renesas_tmr.c cleanup read operation.
Posted by Yoshinori Sato 3 years, 9 months ago 
Cleanup read operation.
This module different return of access size.

Signed-off-by: Yoshinori Sato <ysato@users.sourceforge.jp>
---
 hw/timer/renesas_tmr.c | 106 ++++++++++++++++++++++-------------------
 1 file changed, 57 insertions(+), 49 deletions(-)

diff --git a/hw/timer/renesas_tmr.c b/hw/timer/renesas_tmr.c
index 446f2eacdd..d7b21edf39 100644
--- a/hw/timer/renesas_tmr.c
+++ b/hw/timer/renesas_tmr.c
@@ -187,59 +187,67 @@ static uint64_t tmr_read(void *opaque, hwaddr addr, unsigned size)
                       addr);
         return UINT64_MAX;
     }
-    switch (addr & 0x0e) {
-    case A_TCR:
-        ret = 0;
-        ret = FIELD_DP8(ret, TCR, CCLR,
-                        FIELD_EX8(tmr->tcr[ch], TCR, CCLR));
-        ret = FIELD_DP8(ret, TCR, OVIE,
-                        FIELD_EX8(tmr->tcr[ch], TCR, OVIE));
-        ret = FIELD_DP8(ret, TCR, CMIEA,
-                        FIELD_EX8(tmr->tcr[ch], TCR, CMIEA));
-        ret = FIELD_DP8(ret, TCR, CMIEB,
-                        FIELD_EX8(tmr->tcr[ch], TCR, CMIEB));
-        return ret;
-    case A_TCSR:
-        ret = 0;
-        ret = FIELD_DP8(ret, TCSR, OSA,
-                        FIELD_EX8(tmr->tcsr[ch], TCSR, OSA));
-        ret = FIELD_DP8(ret, TCSR, OSB,
-                        FIELD_EX8(tmr->tcsr[ch], TCSR, OSB));
-        switch (ch) {
-        case 0:
-            ret = FIELD_DP8(ret, TCSR, ADTE,
-                            FIELD_EX8(tmr->tcsr[ch], TCSR, ADTE));
-            break;
-        case 1: /* CH1 ADTE unimplement always 1 */
-            ret = FIELD_DP8(ret, TCSR, ADTE, 1);
-            break;
-        }
-        return ret;
-    case A_TCORA:
-        if (size == 1) {
+    switch (size) {
+    case 1:
+        switch (addr & 0x0e) {
+        case A_TCR:
+            ret = 0;
+            ret = FIELD_DP8(ret, TCR, CCLR,
+                            FIELD_EX8(tmr->tcr[ch], TCR, CCLR));
+            ret = FIELD_DP8(ret, TCR, OVIE,
+                            FIELD_EX8(tmr->tcr[ch], TCR, OVIE));
+            ret = FIELD_DP8(ret, TCR, CMIEA,
+                            FIELD_EX8(tmr->tcr[ch], TCR, CMIEA));
+            ret = FIELD_DP8(ret, TCR, CMIEB,
+                            FIELD_EX8(tmr->tcr[ch], TCR, CMIEB));
+            return ret;
+        case A_TCSR:
+            ret = 0;
+            ret = FIELD_DP8(ret, TCSR, OSA,
+                            FIELD_EX8(tmr->tcsr[ch], TCSR, OSA));
+            ret = FIELD_DP8(ret, TCSR, OSB,
+                            FIELD_EX8(tmr->tcsr[ch], TCSR, OSB));
+            switch (ch) {
+            case 0:
+                ret = FIELD_DP8(ret, TCSR, ADTE,
+                                FIELD_EX8(tmr->tcsr[ch], TCSR, ADTE));
+                break;
+            case 1: /* CH1 ADTE unimplement always 1 */
+                ret = FIELD_DP8(ret, TCSR, ADTE, 1);
+                break;
+            }
+            return ret;
+        case A_TCORA:
             return tmr->tcora[ch];
-        } else if (ch == 0) {
-            return concat_reg(tmr->tcora);
-        }
-    case A_TCORB:
-        if (size == 1) {
+        case A_TCORB:
             return tmr->tcorb[ch];
-        } else {
-            return concat_reg(tmr->tcorb);
-        }
-    case A_TCNT:
-        return read_tcnt(tmr, size, ch);
-    case A_TCCR:
-        if (size == 1) {
+        case A_TCNT:
+            return read_tcnt(tmr, size, ch);
+        case A_TCCR:
             return read_tccr(tmr->tccr[ch]);
-        } else {
-            return read_tccr(tmr->tccr[0]) << 8 | read_tccr(tmr->tccr[1]);
+        default:
+            qemu_log_mask(LOG_UNIMP, "renesas_tmr: Register 0x%" HWADDR_PRIX
+                          " not implemented\n",
+                          addr);
+            break;
+        }
+    case 2:
+        switch (addr) {
+        case A_TCORA:
+            return concat_reg(tmr->tcora);
+        case A_TCORB:
+            return concat_reg(tmr->tcora);
+        case A_TCNT:
+            return read_tcnt(tmr, size, ch);
+        case A_TCCR:
+            return read_tccr(tmr->tccr[ch]) << 8 | read_tccr(tmr->tccr[1]);
+        default:
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "renesas_tmr: Register 0x%" HWADDR_PRIX
+                          " invalid access size\n",
+                          addr);
+            break;
         }
-    default:
-        qemu_log_mask(LOG_UNIMP, "renesas_tmr: Register 0x%" HWADDR_PRIX
-                                 " not implemented\n",
-                      addr);
-        break;
     }
     return UINT64_MAX;
 }
-- 
2.20.1
Re: [PATCH] hw/timer/renesas_tmr.c cleanup read operation.
Posted by Thomas Huth 3 years, 5 months ago 
On 11/07/2020 17.42, Yoshinori Sato wrote:
> Cleanup read operation.
> This module different return of access size.

The last sentence is hard to read, could you maybe rephrase it?

> -    case A_TCORB:
> -        if (size == 1) {
> +        case A_TCORB:
>              return tmr->tcorb[ch];
> -        } else {
> -            return concat_reg(tmr->tcorb);

So with size == 2 and addr == TCORB you were returning tmr->tcorb here...

> -        }
> -    case A_TCNT:
> -        return read_tcnt(tmr, size, ch);
> -    case A_TCCR:
> -        if (size == 1) {
> +        case A_TCNT:
> +            return read_tcnt(tmr, size, ch);
> +        case A_TCCR:
>              return read_tccr(tmr->tccr[ch]);
> -        } else {
> -            return read_tccr(tmr->tccr[0]) << 8 | read_tccr(tmr->tccr[1]);
> +        default:
> +            qemu_log_mask(LOG_UNIMP, "renesas_tmr: Register 0x%" HWADDR_PRIX
> +                          " not implemented\n",
> +                          addr);
> +            break;
> +        }
> +    case 2:
> +        switch (addr) {
> +        case A_TCORA:
> +            return concat_reg(tmr->tcora);
> +        case A_TCORB:
> +            return concat_reg(tmr->tcora);

... but in the new code, you are now returning tmr->tcora ... copy-n-paste
error?

> +        case A_TCNT:
> +            return read_tcnt(tmr, size, ch);
> +        case A_TCCR:
> +            return read_tccr(tmr->tccr[ch]) << 8 | read_tccr(tmr->tccr[1]);
> +        default:
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "renesas_tmr: Register 0x%" HWADDR_PRIX
> +                          " invalid access size\n",
> +                          addr);
> +            break;
>          }
> -    default:
> -        qemu_log_mask(LOG_UNIMP, "renesas_tmr: Register 0x%" HWADDR_PRIX
> -                                 " not implemented\n",
> -                      addr);
> -        break;
>      }
>      return UINT64_MAX;
>  }
> 

 Thomas

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.