[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 03/11] hw/gpio/pl061: Clean up read/write offset handling log

From: Richard Henderson
Subject: Re: [PATCH 03/11] hw/gpio/pl061: Clean up read/write offset handling logic
Date: Tue, 6 Jul 2021 18:25:29 -0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0

On 7/2/21 4:45 AM, Peter Maydell wrote:
On Fri, 2 Jul 2021 at 12:02, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

Hi Peter,

On 7/2/21 12:40 PM, Peter Maydell wrote:
Currently the pl061_read() and pl061_write() functions handle offsets
using a combination of three if() statements and a switch().  Clean
this up to use just a switch, using case ranges.

This requires that instead of catching accesses to the luminary-only
registers on a stock PL061 via a check on s->rsvd_start we use
an "is this luminary?" check in the cases for each luminary-only

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
  hw/gpio/pl061.c | 106 ++++++++++++++++++++++++++++++++++++------------
  1 file changed, 81 insertions(+), 25 deletions(-)

diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
index a6ace88895d..0f5d12e6d5a 100644
--- a/hw/gpio/pl061.c
+++ b/hw/gpio/pl061.c
@@ -55,7 +55,6 @@ struct PL061State {
      qemu_irq irq;
      qemu_irq out[N_GPIOS];
      const unsigned char *id;
-    uint32_t rsvd_start; /* reserved area: [rsvd_start, 0xfcc] */

  static const VMStateDescription vmstate_pl061 = {
@@ -151,16 +150,9 @@ static uint64_t pl061_read(void *opaque, hwaddr offset,
      PL061State *s = (PL061State *)opaque;

-    if (offset < 0x400) {
-        return s->data & (offset >> 2);
-    }
-    if (offset >= s->rsvd_start && offset <= 0xfcc) {
-        goto err_out;
-    }
-    if (offset >= 0xfd0 && offset < 0x1000) {
-        return s->id[(offset - 0xfd0) >> 2];
-    }
      switch (offset) {
+    case 0x0 ... 0x3fc: /* Data */
+        return s->data & (offset >> 2);

Don't we need to set pl061_ops.impl.min/max_access_size = 4
to keep the same logic?

I think the hardware intends to permit accesses of any width, but only
at 4-byte boundaries. There is a slight behaviour change here:
accesses to 0x3fd, 0x3fe, 0x3ff now fall into the default case (ie error)
rather than being treated like 0x3fc, and similarly accesses to 0xfdd,
0xfde, 0xfdf are errors rather than treated like 0xfdc. But I think
that it's probably more correct to consider those to be errors.

(We could explicitly check and goto err_out if (offset & 3)
right at the top, I suppose.)

Perhaps just better to retain current behaviour with this patch by extending the case to the ends. If you want to check oddness of offset, use a separate patch.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]