[PATCH 00/11] hw/arm: Make virt board secure powerdown/reset work

[PATCH 00/11] hw/arm: Make virt board secure powerdown/reset work

[Peter Maydell]

This series fixes a bug reported by Maxim where the virt board's
functionality intended to allow a guest in the Secure world to
shutdown or reset the system was broken. Patch 1 from Maxim (seen
already on-list) fixes a silly cut-n-paste error in the gpio-pwr
device. The rest of the series fixes the trickier part of the bug.

The bug is caused because the gpio-pwr device and the virt board
wiring assume that the GPIO outputs used to trigger shutdown and reset
are active-high. However, for the PL061 GPIO lines default to inputs,
and our PL061 implementation currently hardcodes "assume lines have
pullup resistors so that when configured for input they act like a
logical 1 output". The only reason this doesn't mean that we
immediately shutdown/reset on startup is a second PL061 bug where we
don't assert the GPIO lines to the active-1-pullup behaviour on reset,
but instead do this the first time that pl061_active() is called,
which is typically when the guest touches the PL061. It happens that
OVMF doesn't do that until it decides it actually wants to do a
shutdown, so the bug was manifesting as "we got a reset instead of
shutdown".

I did an audit of our PL061 users in-tree:

 * highbank, versatilepb create PL061 devices but don't connect any of
   the GPIO inputs or outputs
 * realview, sbsa_ref connect only inputs, never outputs
 * virt has an NS PL061 which is used only for inputs, and an S PL061
   which is used for outputs (and doesn't work correctly, as described
   above)
 * stellaris uses its PL061s for both inputs and outputs; the output
   handling is for the SSI device chip selects (and is not really
   correctly modelling what the hardware does).  These PL061s are the
   "Luminary" variant.

The Realview and Versatile boards really do want pull-up behaviour (as
documented in their TRMs). For 'virt' and 'sbsa_ref' we can define
whatever behaviour we like; 'virt' needs pull-down unless we want to
redesign the gpio-pwr device, as noted above.  The Luminary PL061s in
the stellaris board have extra registers not provided in the stock
PL061 which let the guest control whether to enable pullup or pulldown
or to leave the line truly floating. I don't know what highbank
hardware needs, but since it doesn't connect any inputs or outputs it
doesn't really matter.

These patches make the implementation honour the Luminary PL061 PUR
and PDR registers, and add QOM properties so that boards using the
stock PL061 can configure whether the lines are pullup, pulldown, or
floating. We default to pullup to retain existing behaviour, and make
the virt board explicitly configure the pulldown it wants. Once that
is all in place, we can make the PL061 assert the GPIO lines to the
pullup/pulldown state on reset (by converting it to 3-phase reset).

A few patches early in the series do some cleanup of the PL061:
converting it to tracepoints, some minor refactoring, and a
documentation patch. The last two patches are comments only, and
document a couple of issues with our stellaris board model which I
noticed while I was making sure I hadn't broken it with my PL061
changes. Given that the stellaris board is old and not very useful
these days, I don't want to put in a lot of effort trying to "fix"
things which aren't causing any issues with guest images we know
about, but I did want to record what I'd figured out from various data
sheets so it doesn't get forgotten...

thanks
-- PMM

Maxim Uvarov (1):
  hw/gpio/gpio_pwr: use shutdown function for reboot

Peter Maydell (10):
  hw/gpio/pl061: Convert DPRINTF to tracepoints
  hw/gpio/pl061: Clean up read/write offset handling logic
  hw/gpio/pl061: Add tracepoints for register read and write
  hw/gpio/pl061: Document the interface of this device
  hw/gpio/pl061: Honour Luminary PL061 PUR and PDR registers
  hw/gpio/pl061: Make pullup/pulldown of outputs configurable
  hw/arm/virt: Make PL061 GPIO lines pulled low, not high
  hw/gpio/pl061: Convert to 3-phase reset and assert GPIO lines
    correctly on reset
  hw/gpio/pl061: Document a shortcoming in our implementation
  hw/arm/stellaris: Expand comment about handling of OLED chipselect

 hw/arm/stellaris.c   |  56 ++++++-
 hw/arm/virt.c        |   3 +
 hw/gpio/gpio_pwr.c   |   2 +-
 hw/gpio/pl061.c      | 343 ++++++++++++++++++++++++++++++++++---------
 hw/gpio/trace-events |   9 ++
 5 files changed, 341 insertions(+), 72 deletions(-)

-- 
2.20.1

[PATCH 01/11] hw/gpio/gpio_pwr: use shutdown function for reboot

[Peter Maydell]

From: Maxim Uvarov <maxim.uvarov@linaro.org>

qemu has 2 type of functions: shutdown and reboot. Shutdown
function has to be used for machine shutdown. Otherwise we cause
a reset with a bogus "cause" value, when we intended a shutdown.

Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Message-id: 20210625111842.3790-3-maxim.uvarov@linaro.org
[PMM: tweaked commit message]
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/gpio/gpio_pwr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/gpio/gpio_pwr.c b/hw/gpio/gpio_pwr.c
index 7714fa0dc4d..dbaf1c70c88 100644
--- a/hw/gpio/gpio_pwr.c
+++ b/hw/gpio/gpio_pwr.c
@@ -43,7 +43,7 @@ static void gpio_pwr_reset(void *opaque, int n, int level)
 static void gpio_pwr_shutdown(void *opaque, int n, int level)
 {
     if (level) {
-        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
     }
 }
 
-- 
2.20.1

[PATCH 02/11] hw/gpio/pl061: Convert DPRINTF to tracepoints

[Peter Maydell]

Convert the use of the DPRINTF debug macro in the PL061 model to
use tracepoints.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/gpio/pl061.c      | 27 +++++++++------------------
 hw/gpio/trace-events |  6 ++++++
 2 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
index e72e77572a0..a6ace88895d 100644
--- a/hw/gpio/pl061.c
+++ b/hw/gpio/pl061.c
@@ -15,19 +15,7 @@
 #include "qemu/log.h"
 #include "qemu/module.h"
 #include "qom/object.h"
-
-//#define DEBUG_PL061 1
-
-#ifdef DEBUG_PL061
-#define DPRINTF(fmt, ...) \
-do { printf("pl061: " fmt , ## __VA_ARGS__); } while (0)
-#define BADF(fmt, ...) \
-do { fprintf(stderr, "pl061: error: " fmt , ## __VA_ARGS__); exit(1);} while (0)
-#else
-#define DPRINTF(fmt, ...) do {} while(0)
-#define BADF(fmt, ...) \
-do { fprintf(stderr, "pl061: error: " fmt , ## __VA_ARGS__);} while (0)
-#endif
+#include "trace.h"
 
 static const uint8_t pl061_id[12] =
   { 0x00, 0x00, 0x00, 0x00, 0x61, 0x10, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1 };
@@ -107,7 +95,7 @@ static void pl061_update(PL061State *s)
     uint8_t out;
     int i;
 
-    DPRINTF("dir = %d, data = %d\n", s->dir, s->data);
+    trace_pl061_update(DEVICE(s)->canonical_path, s->dir, s->data);
 
     /* Outputs float high.  */
     /* FIXME: This is board dependent.  */
@@ -118,8 +106,9 @@ static void pl061_update(PL061State *s)
         for (i = 0; i < N_GPIOS; i++) {
             mask = 1 << i;
             if (changed & mask) {
-                DPRINTF("Set output %d = %d\n", i, (out & mask) != 0);
-                qemu_set_irq(s->out[i], (out & mask) != 0);
+                int level = (out & mask) != 0;
+                trace_pl061_set_output(DEVICE(s)->canonical_path, i, level);
+                qemu_set_irq(s->out[i], level);
             }
         }
     }
@@ -131,7 +120,8 @@ static void pl061_update(PL061State *s)
         for (i = 0; i < N_GPIOS; i++) {
             mask = 1 << i;
             if (changed & mask) {
-                DPRINTF("Changed input %d = %d\n", i, (s->data & mask) != 0);
+                trace_pl061_input_change(DEVICE(s)->canonical_path, i,
+                                         (s->data & mask) != 0);
 
                 if (!(s->isense & mask)) {
                     /* Edge interrupt */
@@ -150,7 +140,8 @@ static void pl061_update(PL061State *s)
     /* Level interrupt */
     s->istate |= ~(s->data ^ s->iev) & s->isense;
 
-    DPRINTF("istate = %02X\n", s->istate);
+    trace_pl061_update_istate(DEVICE(s)->canonical_path,
+                              s->istate, s->im, (s->istate & s->im) != 0);
 
     qemu_set_irq(s->irq, (s->istate & s->im) != 0);
 }
diff --git a/hw/gpio/trace-events b/hw/gpio/trace-events
index f0b664158e2..48ccbb183cc 100644
--- a/hw/gpio/trace-events
+++ b/hw/gpio/trace-events
@@ -13,6 +13,12 @@ nrf51_gpio_write(uint64_t offset, uint64_t value) "offset 0x%" PRIx64 " value 0x
 nrf51_gpio_set(int64_t line, int64_t value) "line %" PRIi64 " value %" PRIi64
 nrf51_gpio_update_output_irq(int64_t line, int64_t value) "line %" PRIi64 " value %" PRIi64
 
+# pl061.c
+pl061_update(const char *id, uint32_t dir, uint32_t data) "%s GPIODIR 0x%x GPIODATA 0x%x"
+pl061_set_output(const char *id, int gpio, int level) "%s setting output %d to %d"
+pl061_input_change(const char *id, int gpio, int level) "%s input %d changed to %d"
+pl061_update_istate(const char *id, uint32_t istate, uint32_t im, int level) "%s GPIORIS 0x%x GPIOIE 0x%x interrupt level %d"
+
 # sifive_gpio.c
 sifive_gpio_read(uint64_t offset, uint64_t r) "offset 0x%" PRIx64 " value 0x%" PRIx64
 sifive_gpio_write(uint64_t offset, uint64_t value) "offset 0x%" PRIx64 " value 0x%" PRIx64
-- 
2.20.1

Re: [PATCH 02/11] hw/gpio/pl061: Convert DPRINTF to tracepoints

[Philippe Mathieu-Daudé]

On 7/2/21 12:40 PM, Peter Maydell wrote:
> Convert the use of the DPRINTF debug macro in the PL061 model to
> use tracepoints.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/gpio/pl061.c      | 27 +++++++++------------------
>  hw/gpio/trace-events |  6 ++++++
>  2 files changed, 15 insertions(+), 18 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 02/11] hw/gpio/pl061: Convert DPRINTF to tracepoints

[Richard Henderson]

On 7/2/21 3:40 AM, Peter Maydell wrote:
> Convert the use of the DPRINTF debug macro in the PL061 model to
> use tracepoints.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/gpio/pl061.c      | 27 +++++++++------------------
>   hw/gpio/trace-events |  6 ++++++
>   2 files changed, 15 insertions(+), 18 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

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

[Peter Maydell]

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
register.

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);
     case 0x400: /* Direction */
         return s->dir;
     case 0x404: /* Interrupt sense */
@@ -178,33 +170,70 @@ static uint64_t pl061_read(void *opaque, hwaddr offset,
     case 0x420: /* Alternate function select */
         return s->afsel;
     case 0x500: /* 2mA drive */
+        if (s->id != pl061_id_luminary) {
+            goto bad_offset;
+        }
         return s->dr2r;
     case 0x504: /* 4mA drive */
+        if (s->id != pl061_id_luminary) {
+            goto bad_offset;
+        }
         return s->dr4r;
     case 0x508: /* 8mA drive */
+        if (s->id != pl061_id_luminary) {
+            goto bad_offset;
+        }
         return s->dr8r;
     case 0x50c: /* Open drain */
+        if (s->id != pl061_id_luminary) {
+            goto bad_offset;
+        }
         return s->odr;
     case 0x510: /* Pull-up */
+        if (s->id != pl061_id_luminary) {
+            goto bad_offset;
+        }
         return s->pur;
     case 0x514: /* Pull-down */
+        if (s->id != pl061_id_luminary) {
+            goto bad_offset;
+        }
         return s->pdr;
     case 0x518: /* Slew rate control */
+        if (s->id != pl061_id_luminary) {
+            goto bad_offset;
+        }
         return s->slr;
     case 0x51c: /* Digital enable */
+        if (s->id != pl061_id_luminary) {
+            goto bad_offset;
+        }
         return s->den;
     case 0x520: /* Lock */
+        if (s->id != pl061_id_luminary) {
+            goto bad_offset;
+        }
         return s->locked;
     case 0x524: /* Commit */
+        if (s->id != pl061_id_luminary) {
+            goto bad_offset;
+        }
         return s->cr;
     case 0x528: /* Analog mode select */
+        if (s->id != pl061_id_luminary) {
+            goto bad_offset;
+        }
         return s->amsel;
+    case 0x52c ... 0xfcc: /* Reserved */
+        goto bad_offset;
+    case 0xfd0 ... 0xffc: /* ID registers */
+        return s->id[(offset - 0xfd0) >> 2];
     default:
+    bad_offset:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "pl061_read: Bad offset %x\n", (int)offset);
         break;
     }
-err_out:
-    qemu_log_mask(LOG_GUEST_ERROR,
-                  "pl061_read: Bad offset %x\n", (int)offset);
     return 0;
 }
 
@@ -214,16 +243,12 @@ static void pl061_write(void *opaque, hwaddr offset,
     PL061State *s = (PL061State *)opaque;
     uint8_t mask;
 
-    if (offset < 0x400) {
+    switch (offset) {
+    case 0 ... 0x3fc:
         mask = (offset >> 2) & s->dir;
         s->data = (s->data & ~mask) | (value & mask);
         pl061_update(s);
         return;
-    }
-    if (offset >= s->rsvd_start) {
-        goto err_out;
-    }
-    switch (offset) {
     case 0x400: /* Direction */
         s->dir = value & 0xff;
         break;
@@ -247,47 +272,80 @@ static void pl061_write(void *opaque, hwaddr offset,
         s->afsel = (s->afsel & ~mask) | (value & mask);
         break;
     case 0x500: /* 2mA drive */
+        if (s->id != pl061_id_luminary) {
+            goto bad_offset;
+        }
         s->dr2r = value & 0xff;
         break;
     case 0x504: /* 4mA drive */
+        if (s->id != pl061_id_luminary) {
+            goto bad_offset;
+        }
         s->dr4r = value & 0xff;
         break;
     case 0x508: /* 8mA drive */
+        if (s->id != pl061_id_luminary) {
+            goto bad_offset;
+        }
         s->dr8r = value & 0xff;
         break;
     case 0x50c: /* Open drain */
+        if (s->id != pl061_id_luminary) {
+            goto bad_offset;
+        }
         s->odr = value & 0xff;
         break;
     case 0x510: /* Pull-up */
+        if (s->id != pl061_id_luminary) {
+            goto bad_offset;
+        }
         s->pur = value & 0xff;
         break;
     case 0x514: /* Pull-down */
+        if (s->id != pl061_id_luminary) {
+            goto bad_offset;
+        }
         s->pdr = value & 0xff;
         break;
     case 0x518: /* Slew rate control */
+        if (s->id != pl061_id_luminary) {
+            goto bad_offset;
+        }
         s->slr = value & 0xff;
         break;
     case 0x51c: /* Digital enable */
+        if (s->id != pl061_id_luminary) {
+            goto bad_offset;
+        }
         s->den = value & 0xff;
         break;
     case 0x520: /* Lock */
+        if (s->id != pl061_id_luminary) {
+            goto bad_offset;
+        }
         s->locked = (value != 0xacce551);
         break;
     case 0x524: /* Commit */
+        if (s->id != pl061_id_luminary) {
+            goto bad_offset;
+        }
         if (!s->locked)
             s->cr = value & 0xff;
         break;
     case 0x528:
+        if (s->id != pl061_id_luminary) {
+            goto bad_offset;
+        }
         s->amsel = value & 0xff;
         break;
     default:
-        goto err_out;
+    bad_offset:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "pl061_write: Bad offset %x\n", (int)offset);
+        return;
     }
     pl061_update(s);
     return;
-err_out:
-    qemu_log_mask(LOG_GUEST_ERROR,
-                  "pl061_write: Bad offset %x\n", (int)offset);
 }
 
 static void pl061_reset(DeviceState *dev)
@@ -343,7 +401,6 @@ static void pl061_luminary_init(Object *obj)
     PL061State *s = PL061(obj);
 
     s->id = pl061_id_luminary;
-    s->rsvd_start = 0x52c;
 }
 
 static void pl061_init(Object *obj)
@@ -353,7 +410,6 @@ static void pl061_init(Object *obj)
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
 
     s->id = pl061_id;
-    s->rsvd_start = 0x424;
 
     memory_region_init_io(&s->iomem, obj, &pl061_ops, s, "pl061", 0x1000);
     sysbus_init_mmio(sbd, &s->iomem);
-- 
2.20.1

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

[Philippe Mathieu-Daudé]

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
> register.
> 
> 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?

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

[Peter Maydell]

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
> > register.
> >
> > 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.)

-- PMM

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

[Richard Henderson]

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
>>> register.
>>>
>>> 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.


r~

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

[Peter Maydell]

On Wed, 7 Jul 2021 at 02:25, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> 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:
> >> On 7/2/21 12:40 PM, Peter Maydell wrote:
> >>>   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.

Makes sense. I propose to squash this diff into this patch, which
should make it a no-behaviour-change refactor, and then queue the
series to target-arm.next, since it's now all reviewed except for
patch 11 which is a comment-only change. (If you think this is a bit
fast I'm happy to post a v2 instead -- as a bugfix patchset this is
still OK post-softfreeze, so it's just a matter of my personal convenience
to be able to put it into the arm pull I'm going to do either this
afternoon or tomorrow.)

diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
index 0f5d12e6d5a..b21b230402f 100644
--- a/hw/gpio/pl061.c
+++ b/hw/gpio/pl061.c
@@ -151,7 +151,7 @@ static uint64_t pl061_read(void *opaque, hwaddr offset,
     PL061State *s = (PL061State *)opaque;

     switch (offset) {
-    case 0x0 ... 0x3fc: /* Data */
+    case 0x0 ... 0x3ff: /* Data */
         return s->data & (offset >> 2);
     case 0x400: /* Direction */
         return s->dir;
@@ -224,9 +224,7 @@ static uint64_t pl061_read(void *opaque, hwaddr offset,
             goto bad_offset;
         }
         return s->amsel;
-    case 0x52c ... 0xfcc: /* Reserved */
-        goto bad_offset;
-    case 0xfd0 ... 0xffc: /* ID registers */
+    case 0xfd0 ... 0xfff: /* ID registers */
         return s->id[(offset - 0xfd0) >> 2];
     default:
     bad_offset:
@@ -244,7 +242,7 @@ static void pl061_write(void *opaque, hwaddr offset,
     uint8_t mask;

     switch (offset) {
-    case 0 ... 0x3fc:
+    case 0 ... 0x3ff:
         mask = (offset >> 2) & s->dir;
         s->data = (s->data & ~mask) | (value & mask);
         pl061_update(s);

thanks
-- PMM

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

[Richard Henderson]

On 7/8/21 2:39 AM, Peter Maydell wrote:
> Makes sense. I propose to squash this diff into this patch, which
> should make it a no-behaviour-change refactor, and then queue the
> series to target-arm.next, since it's now all reviewed except for
> patch 11 which is a comment-only change.

Sounds good.


r~

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

[Richard Henderson]

On 7/2/21 3:40 AM, Peter Maydell wrote:
> +    case 0x52c ... 0xfcc: /* Reserved */
> +        goto bad_offset;

Any reason to not just use default for these?

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

[PATCH 04/11] hw/gpio/pl061: Add tracepoints for register read and write

[Peter Maydell]

Add tracepoints for reads and writes to the PL061 registers. This requires
restructuring pl061_read() to only return after the tracepoint, rather
than having lots of early-returns.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/gpio/pl061.c      | 70 ++++++++++++++++++++++++++++++--------------
 hw/gpio/trace-events |  2 ++
 2 files changed, 50 insertions(+), 22 deletions(-)

diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
index 0f5d12e6d5a..f3b80c7776f 100644
--- a/hw/gpio/pl061.c
+++ b/hw/gpio/pl061.c
@@ -149,92 +149,116 @@ static uint64_t pl061_read(void *opaque, hwaddr offset,
                            unsigned size)
 {
     PL061State *s = (PL061State *)opaque;
+    uint64_t r = 0;
 
     switch (offset) {
     case 0x0 ... 0x3fc: /* Data */
-        return s->data & (offset >> 2);
+        r = s->data & (offset >> 2);
+        break;
     case 0x400: /* Direction */
-        return s->dir;
+        r = s->dir;
+        break;
     case 0x404: /* Interrupt sense */
-        return s->isense;
+        r = s->isense;
+        break;
     case 0x408: /* Interrupt both edges */
-        return s->ibe;
+        r = s->ibe;
+        break;
     case 0x40c: /* Interrupt event */
-        return s->iev;
+        r = s->iev;
+        break;
     case 0x410: /* Interrupt mask */
-        return s->im;
+        r = s->im;
+        break;
     case 0x414: /* Raw interrupt status */
-        return s->istate;
+        r = s->istate;
+        break;
     case 0x418: /* Masked interrupt status */
-        return s->istate & s->im;
+        r = s->istate & s->im;
+        break;
     case 0x420: /* Alternate function select */
-        return s->afsel;
+        r = s->afsel;
+        break;
     case 0x500: /* 2mA drive */
         if (s->id != pl061_id_luminary) {
             goto bad_offset;
         }
-        return s->dr2r;
+        r = s->dr2r;
+        break;
     case 0x504: /* 4mA drive */
         if (s->id != pl061_id_luminary) {
             goto bad_offset;
         }
-        return s->dr4r;
+        r = s->dr4r;
+        break;
     case 0x508: /* 8mA drive */
         if (s->id != pl061_id_luminary) {
             goto bad_offset;
         }
-        return s->dr8r;
+        r = s->dr8r;
+        break;
     case 0x50c: /* Open drain */
         if (s->id != pl061_id_luminary) {
             goto bad_offset;
         }
-        return s->odr;
+        r = s->odr;
+        break;
     case 0x510: /* Pull-up */
         if (s->id != pl061_id_luminary) {
             goto bad_offset;
         }
-        return s->pur;
+        r = s->pur;
+        break;
     case 0x514: /* Pull-down */
         if (s->id != pl061_id_luminary) {
             goto bad_offset;
         }
-        return s->pdr;
+        r = s->pdr;
+        break;
     case 0x518: /* Slew rate control */
         if (s->id != pl061_id_luminary) {
             goto bad_offset;
         }
-        return s->slr;
+        r = s->slr;
+        break;
     case 0x51c: /* Digital enable */
         if (s->id != pl061_id_luminary) {
             goto bad_offset;
         }
-        return s->den;
+        r = s->den;
+        break;
     case 0x520: /* Lock */
         if (s->id != pl061_id_luminary) {
             goto bad_offset;
         }
-        return s->locked;
+        r = s->locked;
+        break;
     case 0x524: /* Commit */
         if (s->id != pl061_id_luminary) {
             goto bad_offset;
         }
-        return s->cr;
+        r = s->cr;
+        break;
     case 0x528: /* Analog mode select */
         if (s->id != pl061_id_luminary) {
             goto bad_offset;
         }
-        return s->amsel;
+        r = s->amsel;
+        break;
     case 0x52c ... 0xfcc: /* Reserved */
         goto bad_offset;
     case 0xfd0 ... 0xffc: /* ID registers */
-        return s->id[(offset - 0xfd0) >> 2];
+        r = s->id[(offset - 0xfd0) >> 2];
+        break;
     default:
     bad_offset:
         qemu_log_mask(LOG_GUEST_ERROR,
                       "pl061_read: Bad offset %x\n", (int)offset);
         break;
     }
-    return 0;
+
+    trace_pl061_read(DEVICE(s)->canonical_path, offset, r);
+    return r;
 }
 
 static void pl061_write(void *opaque, hwaddr offset,
@@ -243,6 +267,8 @@ static void pl061_write(void *opaque, hwaddr offset,
     PL061State *s = (PL061State *)opaque;
     uint8_t mask;
 
+    trace_pl061_write(DEVICE(s)->canonical_path, offset, value);
+
     switch (offset) {
     case 0 ... 0x3fc:
         mask = (offset >> 2) & s->dir;
diff --git a/hw/gpio/trace-events b/hw/gpio/trace-events
index 48ccbb183cc..442be9406f5 100644
--- a/hw/gpio/trace-events
+++ b/hw/gpio/trace-events
@@ -18,6 +18,8 @@ pl061_update(const char *id, uint32_t dir, uint32_t data) "%s GPIODIR 0x%x GPIOD
 pl061_set_output(const char *id, int gpio, int level) "%s setting output %d to %d"
 pl061_input_change(const char *id, int gpio, int level) "%s input %d changed to %d"
 pl061_update_istate(const char *id, uint32_t istate, uint32_t im, int level) "%s GPIORIS 0x%x GPIOIE 0x%x interrupt level %d"
+pl061_read(const char *id, uint64_t offset, uint64_t r) "%s offset 0x%" PRIx64 " value 0x%" PRIx64
+pl061_write(const char *id, uint64_t offset, uint64_t value) "%s offset 0x%" PRIx64 " value 0x%" PRIx64
 
 # sifive_gpio.c
 sifive_gpio_read(uint64_t offset, uint64_t r) "offset 0x%" PRIx64 " value 0x%" PRIx64
-- 
2.20.1

Re: [PATCH 04/11] hw/gpio/pl061: Add tracepoints for register read and write

[Philippe Mathieu-Daudé]

On 7/2/21 12:40 PM, Peter Maydell wrote:
> Add tracepoints for reads and writes to the PL061 registers. This requires
> restructuring pl061_read() to only return after the tracepoint, rather
> than having lots of early-returns.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/gpio/pl061.c      | 70 ++++++++++++++++++++++++++++++--------------
>  hw/gpio/trace-events |  2 ++
>  2 files changed, 50 insertions(+), 22 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 04/11] hw/gpio/pl061: Add tracepoints for register read and write

[Richard Henderson]

On 7/2/21 3:40 AM, Peter Maydell wrote:
> Add tracepoints for reads and writes to the PL061 registers. This requires
> restructuring pl061_read() to only return after the tracepoint, rather
> than having lots of early-returns.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/gpio/pl061.c      | 70 ++++++++++++++++++++++++++++++--------------
>   hw/gpio/trace-events |  2 ++
>   2 files changed, 50 insertions(+), 22 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 05/11] hw/gpio/pl061: Document the interface of this device

[Peter Maydell]

Add a comment documenting the "QEMU interface" of this device:
which MMIO regions, IRQ lines, GPIO lines, etc it exposes.

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

diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
index f3b80c7776f..06a1b82a503 100644
--- a/hw/gpio/pl061.c
+++ b/hw/gpio/pl061.c
@@ -6,6 +6,13 @@
  * Written by Paul Brook
  *
  * This code is licensed under the GPL.
+ *
+ * QEMU interface:
+ *  + sysbus MMIO region 0: the device registers
+ *  + sysbus IRQ: the GPIOINTR interrupt line
+ *  + unnamed GPIO inputs 0..7: inputs to connect to the emulated GPIO lines
+ *  + unnamed GPIO outputs 0..7: the emulated GPIO lines, considered as
+ *    outputs
  */
 
 #include "qemu/osdep.h"
-- 
2.20.1

Re: [PATCH 05/11] hw/gpio/pl061: Document the interface of this device

[Richard Henderson]

On 7/2/21 3:40 AM, Peter Maydell wrote:
> Add a comment documenting the "QEMU interface" of this device:
> which MMIO regions, IRQ lines, GPIO lines, etc it exposes.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/gpio/pl061.c | 7 +++++++
>   1 file changed, 7 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 06/11] hw/gpio/pl061: Honour Luminary PL061 PUR and PDR registers

[Peter Maydell]

The Luminary variant of the PL061 has registers GPIOPUR and GPIOPDR
which lets the guest configure whether the GPIO lines are pull-up,
pull-down, or truly floating. Instead of assuming all lines are pulled
high, honour the PUR and PDR registers.

For the plain PL061, continue to assume that lines have an external
pull-up resistor, as we did before.

The stellaris board actually relies on this behaviour -- the CD line
of the ssd0323 display device is connected to GPIO output C7, and it
is only because of a different bug which we're about to fix that we
weren't incorrectly driving this line high on reset and putting the
ssd0323 into data mode.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/gpio/pl061.c      | 58 +++++++++++++++++++++++++++++++++++++++++---
 hw/gpio/trace-events |  2 +-
 2 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
index 06a1b82a503..44bed56fef0 100644
--- a/hw/gpio/pl061.c
+++ b/hw/gpio/pl061.c
@@ -94,18 +94,68 @@ static const VMStateDescription vmstate_pl061 = {
     }
 };
 
+static uint8_t pl061_floating(PL061State *s)
+{
+    /*
+     * Return mask of bits which correspond to pins configured as inputs
+     * and which are floating (neither pulled up to 1 nor down to 0).
+     */
+    uint8_t floating;
+
+    if (s->id == pl061_id_luminary) {
+        /*
+         * If both PUR and PDR bits are clear, there is neither a pullup
+         * nor a pulldown in place, and the output truly floats.
+         */
+        floating = ~(s->pur | s->pdr);
+    } else {
+        /* Assume outputs are pulled high. FIXME: this is board dependent. */
+        floating = 0;
+    }
+    return floating & ~s->dir;
+}
+
+static uint8_t pl061_pullups(PL061State *s)
+{
+    /*
+     * Return mask of bits which correspond to pins configured as inputs
+     * and which are pulled up to 1.
+     */
+    uint8_t pullups;
+
+    if (s->id == pl061_id_luminary) {
+        /*
+         * The Luminary variant of the PL061 has an extra registers which
+         * the guest can use to configure whether lines should be pullup
+         * or pulldown.
+         */
+        pullups = s->pur;
+    } else {
+        /* Assume outputs are pulled high. FIXME: this is board dependent. */
+        pullups = 0xff;
+    }
+    return pullups & ~s->dir;
+}
+
 static void pl061_update(PL061State *s)
 {
     uint8_t changed;
     uint8_t mask;
     uint8_t out;
     int i;
+    uint8_t pullups = pl061_pullups(s);
+    uint8_t floating = pl061_floating(s);
 
-    trace_pl061_update(DEVICE(s)->canonical_path, s->dir, s->data);
+    trace_pl061_update(DEVICE(s)->canonical_path, s->dir, s->data,
+                       pullups, floating);
 
-    /* Outputs float high.  */
-    /* FIXME: This is board dependent.  */
-    out = (s->data & s->dir) | ~s->dir;
+    /*
+     * Pins configured as output are driven from the data register;
+     * otherwise if they're pulled up they're 1, and if they're floating
+     * then we give them the same value they had previously, so we don't
+     * report any change to the other end.
+     */
+    out = (s->data & s->dir) | pullups | (s->old_out_data & floating);
     changed = s->old_out_data ^ out;
     if (changed) {
         s->old_out_data = out;
diff --git a/hw/gpio/trace-events b/hw/gpio/trace-events
index 442be9406f5..eb5fb4701c6 100644
--- a/hw/gpio/trace-events
+++ b/hw/gpio/trace-events
@@ -14,7 +14,7 @@ nrf51_gpio_set(int64_t line, int64_t value) "line %" PRIi64 " value %" PRIi64
 nrf51_gpio_update_output_irq(int64_t line, int64_t value) "line %" PRIi64 " value %" PRIi64
 
 # pl061.c
-pl061_update(const char *id, uint32_t dir, uint32_t data) "%s GPIODIR 0x%x GPIODATA 0x%x"
+pl061_update(const char *id, uint32_t dir, uint32_t data, uint32_t pullups, uint32_t floating) "%s GPIODIR 0x%x GPIODATA 0x%x pullups 0x%x floating 0x%x"
 pl061_set_output(const char *id, int gpio, int level) "%s setting output %d to %d"
 pl061_input_change(const char *id, int gpio, int level) "%s input %d changed to %d"
 pl061_update_istate(const char *id, uint32_t istate, uint32_t im, int level) "%s GPIORIS 0x%x GPIOIE 0x%x interrupt level %d"
-- 
2.20.1

Re: [PATCH 06/11] hw/gpio/pl061: Honour Luminary PL061 PUR and PDR registers

[Richard Henderson]

On 7/2/21 3:40 AM, Peter Maydell wrote:
> The Luminary variant of the PL061 has registers GPIOPUR and GPIOPDR
> which lets the guest configure whether the GPIO lines are pull-up,
> pull-down, or truly floating. Instead of assuming all lines are pulled
> high, honour the PUR and PDR registers.
> 
> For the plain PL061, continue to assume that lines have an external
> pull-up resistor, as we did before.
> 
> The stellaris board actually relies on this behaviour -- the CD line
> of the ssd0323 display device is connected to GPIO output C7, and it
> is only because of a different bug which we're about to fix that we
> weren't incorrectly driving this line high on reset and putting the
> ssd0323 into data mode.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/gpio/pl061.c      | 58 +++++++++++++++++++++++++++++++++++++++++---
>   hw/gpio/trace-events |  2 +-
>   2 files changed, 55 insertions(+), 5 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 07/11] hw/gpio/pl061: Make pullup/pulldown of outputs configurable

[Peter Maydell]

The PL061 GPIO does not itself include pullup or pulldown resistors
to set the value of a GPIO line treated as an output when it is
configured as an input (ie when the PL061 itself is not driving it).
In real hardware it is up to the board to add suitable pullups or
pulldowns.  Currently our implementation hardwires this to "outputs
pulled high", which is correct for some boards (eg the realview ones:
see figure 3-29 in the "RealView Platform Baseboard for ARM926EJ-S
User Guide" DUI0224I), but wrong for others.

In particular, the wiring in the 'virt' board and the gpio-pwr device
assumes that wires should be pulled low, because otherwise the
pull-to-high will trigger a shutdown or reset action.  (The only
reason this doesn't happen immediately on startup is due to another
bug in the PL061, where we don't assert the GPIOs to the correct
value on reset, but will do so as soon as the guest touches a
register and pl061_update() gets called.)

Add properties to the pl061 so the board can configure whether it
wants GPIO lines to have pullup, pulldown, or neither.

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

diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
index 44bed56fef0..bb496a19ade 100644
--- a/hw/gpio/pl061.c
+++ b/hw/gpio/pl061.c
@@ -13,12 +13,28 @@
  *  + unnamed GPIO inputs 0..7: inputs to connect to the emulated GPIO lines
  *  + unnamed GPIO outputs 0..7: the emulated GPIO lines, considered as
  *    outputs
+ *  + QOM property "pullups": an integer defining whether non-floating lines
+ *    configured as inputs should be pulled up to logical 1 (ie whether in
+ *    real hardware they have a pullup resistor on the line out of the PL061).
+ *    This should be an 8-bit value, where bit 0 is 1 if GPIO line 0 should
+ *    be pulled high, bit 1 configures line 1, and so on. The default is 0xff,
+ *    indicating that all GPIO lines are pulled up to logical 1.
+ *  + QOM property "pulldowns": an integer defining whether non-floating lines
+ *    configured as inputs should be pulled down to logical 0 (ie whether in
+ *    real hardware they have a pulldown resistor on the line out of the PL061).
+ *    This should be an 8-bit value, where bit 0 is 1 if GPIO line 0 should
+ *    be pulled low, bit 1 configures line 1, and so on. The default is 0x0.
+ *    It is an error to set a bit in both "pullups" and "pulldowns". If a bit
+ *    is 0 in both, then the line is considered to be floating, and it will
+ *    not have qemu_set_irq() called on it when it is configured as an input.
  */
 
 #include "qemu/osdep.h"
 #include "hw/irq.h"
 #include "hw/sysbus.h"
+#include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
+#include "qapi/error.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
 #include "qom/object.h"
@@ -62,6 +78,9 @@ struct PL061State {
     qemu_irq irq;
     qemu_irq out[N_GPIOS];
     const unsigned char *id;
+    /* Properties, for non-Luminary PL061 */
+    uint32_t pullups;
+    uint32_t pulldowns;
 };
 
 static const VMStateDescription vmstate_pl061 = {
@@ -109,8 +128,7 @@ static uint8_t pl061_floating(PL061State *s)
          */
         floating = ~(s->pur | s->pdr);
     } else {
-        /* Assume outputs are pulled high. FIXME: this is board dependent. */
-        floating = 0;
+        floating = ~(s->pullups | s->pulldowns);
     }
     return floating & ~s->dir;
 }
@@ -131,8 +149,7 @@ static uint8_t pl061_pullups(PL061State *s)
          */
         pullups = s->pur;
     } else {
-        /* Assume outputs are pulled high. FIXME: this is board dependent. */
-        pullups = 0xff;
+        pullups = s->pullups;
     }
     return pullups & ~s->dir;
 }
@@ -501,12 +518,38 @@ static void pl061_init(Object *obj)
     qdev_init_gpio_out(dev, s->out, N_GPIOS);
 }
 
+static void pl061_realize(DeviceState *dev, Error **errp)
+{
+    PL061State *s = PL061(dev);
+
+    if (s->pullups > 0xff) {
+        error_setg(errp, "pullups property must be between 0 and 0xff");
+        return;
+    }
+    if (s->pulldowns > 0xff) {
+        error_setg(errp, "pulldowns property must be between 0 and 0xff");
+        return;
+    }
+    if (s->pullups & s->pulldowns) {
+        error_setg(errp, "no bit may be set both in pullups and pulldowns");
+        return;
+    }
+}
+
+static Property pl061_props[] = {
+    DEFINE_PROP_UINT32("pullups", PL061State, pullups, 0xff),
+    DEFINE_PROP_UINT32("pulldowns", PL061State, pulldowns, 0x0),
+    DEFINE_PROP_END_OF_LIST()
+};
+
 static void pl061_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->vmsd = &vmstate_pl061;
     dc->reset = &pl061_reset;
+    dc->realize = pl061_realize;
+    device_class_set_props(dc, pl061_props);
 }
 
 static const TypeInfo pl061_info = {
-- 
2.20.1

Re: [PATCH 07/11] hw/gpio/pl061: Make pullup/pulldown of outputs configurable

[Richard Henderson]

On 7/2/21 3:40 AM, Peter Maydell wrote:
> The PL061 GPIO does not itself include pullup or pulldown resistors
> to set the value of a GPIO line treated as an output when it is
> configured as an input (ie when the PL061 itself is not driving it).
> In real hardware it is up to the board to add suitable pullups or
> pulldowns.  Currently our implementation hardwires this to "outputs
> pulled high", which is correct for some boards (eg the realview ones:
> see figure 3-29 in the "RealView Platform Baseboard for ARM926EJ-S
> User Guide" DUI0224I), but wrong for others.
> 
> In particular, the wiring in the 'virt' board and the gpio-pwr device
> assumes that wires should be pulled low, because otherwise the
> pull-to-high will trigger a shutdown or reset action.  (The only
> reason this doesn't happen immediately on startup is due to another
> bug in the PL061, where we don't assert the GPIOs to the correct
> value on reset, but will do so as soon as the guest touches a
> register and pl061_update() gets called.)
> 
> Add properties to the pl061 so the board can configure whether it
> wants GPIO lines to have pullup, pulldown, or neither.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/gpio/pl061.c | 51 +++++++++++++++++++++++++++++++++++++++++++++----
>   1 file changed, 47 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 08/11] hw/arm/virt: Make PL061 GPIO lines pulled low, not high

[Peter Maydell]

For the virt board we have two PL061 devices -- one for NonSecure which
is inputs only, and one for Secure which is outputs only. For the former,
we don't care whether its outputs are pulled low or high when the line is
configured as an input, because we don't connect them. For the latter,
we do care, because we wire the lines up to the gpio-pwr device, which
assumes that level 1 means "do the action" and 1 means "do nothing".
For consistency in case we add more outputs in future, configure both
PL061s to pull GPIO lines down to 0.

Reported-by: Maxim Uvarov <maxim.uvarov@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 4b96f060140..93ab9d21ea0 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -895,6 +895,9 @@ static void create_gpio_devices(const VirtMachineState *vms, int gpio,
     MachineState *ms = MACHINE(vms);
 
     pl061_dev = qdev_new("pl061");
+    /* Pull lines down to 0 if not driven by the PL061 */
+    qdev_prop_set_uint32(pl061_dev, "pullups", 0);
+    qdev_prop_set_uint32(pl061_dev, "pulldowns", 0xff);
     s = SYS_BUS_DEVICE(pl061_dev);
     sysbus_realize_and_unref(s, &error_fatal);
     memory_region_add_subregion(mem, base, sysbus_mmio_get_region(s, 0));
-- 
2.20.1

Re: [PATCH 08/11] hw/arm/virt: Make PL061 GPIO lines pulled low, not high

[Richard Henderson]

On 7/2/21 3:40 AM, Peter Maydell wrote:
> For the virt board we have two PL061 devices -- one for NonSecure which
> is inputs only, and one for Secure which is outputs only. For the former,
> we don't care whether its outputs are pulled low or high when the line is
> configured as an input, because we don't connect them. For the latter,
> we do care, because we wire the lines up to the gpio-pwr device, which
> assumes that level 1 means "do the action" and 1 means "do nothing".
> For consistency in case we add more outputs in future, configure both
> PL061s to pull GPIO lines down to 0.
> 
> Reported-by: Maxim Uvarov<maxim.uvarov@linaro.org>
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/arm/virt.c | 3 +++
>   1 file changed, 3 insertions(+)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 09/11] hw/gpio/pl061: Convert to 3-phase reset and assert GPIO lines correctly on reset

[Peter Maydell]

The PL061 comes out of reset with all its lines configured as input,
which means they might need to be pulled to 0 or 1 depending on the
'pullups' and 'pulldowns' properties.  Currently we do not assert
these lines on reset; they will only be set whenever the guest first
touches a register that triggers a call to pl061_update().

Convert the device to three-phase reset so we have a place where we
can safely call qemu_set_irq() to set the floating lines to their
correct values.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/gpio/pl061.c      | 29 +++++++++++++++++++++++++----
 hw/gpio/trace-events |  1 +
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
index bb496a19ade..8d12b2d6b97 100644
--- a/hw/gpio/pl061.c
+++ b/hw/gpio/pl061.c
@@ -448,13 +448,14 @@ static void pl061_write(void *opaque, hwaddr offset,
     return;
 }
 
-static void pl061_reset(DeviceState *dev)
+static void pl061_enter_reset(Object *obj, ResetType type)
 {
-    PL061State *s = PL061(dev);
+    PL061State *s = PL061(obj);
+
+    trace_pl061_reset(DEVICE(s)->canonical_path);
 
     /* reset values from PL061 TRM, Stellaris LM3S5P31 & LM3S8962 Data Sheet */
     s->data = 0;
-    s->old_out_data = 0;
     s->old_in_data = 0;
     s->dir = 0;
     s->isense = 0;
@@ -476,6 +477,24 @@ static void pl061_reset(DeviceState *dev)
     s->amsel = 0;
 }
 
+static void pl061_hold_reset(Object *obj)
+{
+    PL061State *s = PL061(obj);
+    int i, level;
+    uint8_t floating = pl061_floating(s);
+    uint8_t pullups = pl061_pullups(s);
+
+    for (i = 0; i < N_GPIOS; i++) {
+        if (extract32(floating, i, 1)) {
+            continue;
+        }
+        level = extract32(pullups, i, 1);
+        trace_pl061_set_output(DEVICE(s)->canonical_path, i, level);
+        qemu_set_irq(s->out[i], level);
+    }
+    s->old_out_data = pullups;
+}
+
 static void pl061_set_irq(void * opaque, int irq, int level)
 {
     PL061State *s = (PL061State *)opaque;
@@ -545,11 +564,13 @@ static Property pl061_props[] = {
 static void pl061_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    ResettableClass *rc = RESETTABLE_CLASS(klass);
 
     dc->vmsd = &vmstate_pl061;
-    dc->reset = &pl061_reset;
     dc->realize = pl061_realize;
     device_class_set_props(dc, pl061_props);
+    rc->phases.enter = pl061_enter_reset;
+    rc->phases.hold = pl061_hold_reset;
 }
 
 static const TypeInfo pl061_info = {
diff --git a/hw/gpio/trace-events b/hw/gpio/trace-events
index eb5fb4701c6..1dab99c5604 100644
--- a/hw/gpio/trace-events
+++ b/hw/gpio/trace-events
@@ -20,6 +20,7 @@ pl061_input_change(const char *id, int gpio, int level) "%s input %d changed to
 pl061_update_istate(const char *id, uint32_t istate, uint32_t im, int level) "%s GPIORIS 0x%x GPIOIE 0x%x interrupt level %d"
 pl061_read(const char *id, uint64_t offset, uint64_t r) "%s offset 0x%" PRIx64 " value 0x%" PRIx64
 pl061_write(const char *id, uint64_t offset, uint64_t value) "%s offset 0x%" PRIx64 " value 0x%" PRIx64
+pl061_reset(const char *id) "%s reset"
 
 # sifive_gpio.c
 sifive_gpio_read(uint64_t offset, uint64_t r) "offset 0x%" PRIx64 " value 0x%" PRIx64
-- 
2.20.1

Re: [PATCH 09/11] hw/gpio/pl061: Convert to 3-phase reset and assert GPIO lines correctly on reset

[Philippe Mathieu-Daudé]

On 7/2/21 12:40 PM, Peter Maydell wrote:
> The PL061 comes out of reset with all its lines configured as input,
> which means they might need to be pulled to 0 or 1 depending on the
> 'pullups' and 'pulldowns' properties.  Currently we do not assert
> these lines on reset; they will only be set whenever the guest first
> touches a register that triggers a call to pl061_update().
> 
> Convert the device to three-phase reset so we have a place where we
> can safely call qemu_set_irq() to set the floating lines to their
> correct values.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/gpio/pl061.c      | 29 +++++++++++++++++++++++++----
>  hw/gpio/trace-events |  1 +
>  2 files changed, 26 insertions(+), 4 deletions(-)

Nice.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 09/11] hw/gpio/pl061: Convert to 3-phase reset and assert GPIO lines correctly on reset

[Richard Henderson]

On 7/2/21 3:40 AM, Peter Maydell wrote:
> The PL061 comes out of reset with all its lines configured as input,
> which means they might need to be pulled to 0 or 1 depending on the
> 'pullups' and 'pulldowns' properties.  Currently we do not assert
> these lines on reset; they will only be set whenever the guest first
> touches a register that triggers a call to pl061_update().
> 
> Convert the device to three-phase reset so we have a place where we
> can safely call qemu_set_irq() to set the floating lines to their
> correct values.
> 
> Signed-off-by: Peter Maydell<peter.maydell@linaro.org>
> ---
>   hw/gpio/pl061.c      | 29 +++++++++++++++++++++++++----
>   hw/gpio/trace-events |  1 +
>   2 files changed, 26 insertions(+), 4 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 10/11] hw/gpio/pl061: Document a shortcoming in our implementation

[Peter Maydell]

The Luminary PL061s in the Stellaris LM3S9695 don't all have the same
reset value for GPIOPUR.  We can get away with not letting the board
configure the PUR reset value because we don't actually wire anything
up to the lines which should reset to pull-up.  Add a comment noting
this omission.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Not worth actually fixing, but I wanted a note since I spotted this
while I was reading the datasheet anyway.
---
 hw/gpio/pl061.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
index 8d12b2d6b97..2cb3a231b43 100644
--- a/hw/gpio/pl061.c
+++ b/hw/gpio/pl061.c
@@ -455,6 +455,15 @@ static void pl061_enter_reset(Object *obj, ResetType type)
     trace_pl061_reset(DEVICE(s)->canonical_path);
 
     /* reset values from PL061 TRM, Stellaris LM3S5P31 & LM3S8962 Data Sheet */
+
+    /*
+     * FIXME: For the LM3S6965, not all of the PL061 instances have the
+     * same reset values for GPIOPUR, GPIOAFSEL and GPIODEN, so in theory
+     * we should allow the board to configure these via properties.
+     * In practice, we don't wire anything up to the affected GPIO lines
+     * (PB7, PC0, PC1, PC2, PC3 -- they're used for JTAG), so we can
+     * get away with this inaccuracy.
+     */
     s->data = 0;
     s->old_in_data = 0;
     s->dir = 0;
-- 
2.20.1

Re: [PATCH 10/11] hw/gpio/pl061: Document a shortcoming in our implementation

[Philippe Mathieu-Daudé]

On 7/2/21 12:40 PM, Peter Maydell wrote:
> The Luminary PL061s in the Stellaris LM3S9695 don't all have the same
> reset value for GPIOPUR.  We can get away with not letting the board
> configure the PUR reset value because we don't actually wire anything
> up to the lines which should reset to pull-up.  Add a comment noting
> this omission.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Not worth actually fixing, but I wanted a note since I spotted this
> while I was reading the datasheet anyway.
> ---
>  hw/gpio/pl061.c | 9 +++++++++
>  1 file changed, 9 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH 11/11] hw/arm/stellaris: Expand comment about handling of OLED chipselect

[Peter Maydell]

The stellaris board doesn't emulate the handling of the OLED
chipselect line correctly.  Expand the comment describing this,
including a sketch of the theoretical correct way to do it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Given the stellaris board is old and not very useful these days,
I didn't think it worth the effort of actually implementing the
correct behaviour, but I wanted to record what I figured out
from various data sheets while I was looking at PL061 stuff...
---
 hw/arm/stellaris.c | 56 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index 8b4dab9b79f..ad48cf26058 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -1453,13 +1453,67 @@ static void stellaris_init(MachineState *ms, stellaris_board_info *board)
             DeviceState *sddev;
             DeviceState *ssddev;
 
-            /* Some boards have both an OLED controller and SD card connected to
+            /*
+             * Some boards have both an OLED controller and SD card connected to
              * the same SSI port, with the SD card chip select connected to a
              * GPIO pin.  Technically the OLED chip select is connected to the
              * SSI Fss pin.  We do not bother emulating that as both devices
              * should never be selected simultaneously, and our OLED controller
              * ignores stray 0xff commands that occur when deselecting the SD
              * card.
+             *
+             * The h/w wiring is:
+             *  - GPIO pin D0 is wired to the active-low SD card chip select
+             *  - GPIO pin A3 is wired to the active-low OLED chip select
+             *  - The SoC wiring of the PL061 "auxiliary function" for A3 is
+             *    SSI0Fss ("frame signal"), which is an output from the SoC's
+             *    SSI controller. The SSI controller takes SSI0Fss low when it
+             *    transmits a frame, so it can work as a chip-select signal.
+             *  - GPIO A4 is aux-function SSI0Rx, and wired to the SD card Tx
+             *    (the OLED never sends data to the CPU, so no wiring needed)
+             *  - GPIO A5 is aux-function SSI0Tx, and wired to the SD card Rx
+             *    and the OLED display-data-in
+             *  - GPIO A2 is aux-function SSI0Clk, wired to SD card and OLED
+             *    serial-clock input
+             * So a guest that wants to use the OLED can configure the PL061
+             * to make pins A2, A3, A5 aux-function, so they are connected
+             * directly to the SSI controller. When the SSI controller sends
+             * data it asserts SSI0Fss which selects the OLED.
+             * A guest that wants to use the SD card configures A2, A4 and A5
+             * as aux-function, but leaves A3 as a software-controlled GPIO
+             * line. It asserts the SD card chip-select by using the PL061
+             * to control pin D0, and lets the SSI controller handle Clk, Tx
+             * and Rx. (The SSI controller asserts Fss during tx cycles as
+             * usual, but because A3 is not set to aux-function this is not
+             * forwarded to the OLED, and so the OLED stays unselected.)
+             *
+             * The QEMU implementation instead is:
+             *  - GPIO pin D0 is wired to the active-low SD card chip select,
+             *    and also to the OLED chip-select which is implemented
+             *    as *active-high*
+             *  - SSI controller signals go to the devices regardless of
+             *    whether the guest programs A2, A4, A5 as aux-function or not
+             *
+             * The problem with this implementation is if the guest doesn't
+             * care about the SD card and only uses the OLED. In that case it
+             * may choose never to do anything with D0 (leaving it in its
+             * default floating state, which reliably leaves the card disabled
+             * because an SD card has a pullup on CS within the card itself),
+             * and only set up A2, A3, A5. This for us would mean the OLED
+             * never gets the chip-select assert it needs. We work around
+             * this with a manual raise of D0 here (despite board creation
+             * code being the wrong place to raise IRQ lines) to put the OLED
+             * into an initially selected state.
+             *
+             * In theory the right way to model this would be:
+             *  - Implement aux-function support in the PL061, with an
+             *    extra set of AFIN and AFOUT GPIO lines (set up so that
+             *    if a GPIO line is in auxfn mode the main GPIO in and out
+             *    track the AFIN and AFOUT lines)
+             *  - Wire the AFOUT for D0 up to either a line from the
+             *    SSI controller that's pulled low around every transmit,
+             *    or at least to an always-0 line here on the board
+             *  - Make the ssd0323 OLED controller chipselect active-low
              */
             bus = qdev_get_child_bus(dev, "ssi");
 
-- 
2.20.1