All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH qemu] Add basic power management to raspi.
@ 2021-05-18 20:24 ~nolanl
  2021-05-31 11:23 ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 5+ messages in thread
From: ~nolanl @ 2021-05-18 20:24 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, qemu-arm, Philippe Mathieu-Daudé, Andrew Baumann

From: Nolan Leake <nolan@sigbus.net>

This is just enough to make reboot and poweroff work. Notably, the
watchdog timer functionality is not yet implemented.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/64
Signed-off-by: Nolan Leake <nolan@sigbus.net>
---
 hw/arm/bcm2835_peripherals.c         |  13 ++-
 hw/misc/bcm2835_powermgt.c           | 157 +++++++++++++++++++++++++++
 hw/misc/meson.build                  |   1 +
 include/hw/arm/bcm2835_peripherals.h |   3 +-
 include/hw/misc/bcm2835_powermgt.h   |  29 +++++
 5 files changed, 201 insertions(+), 2 deletions(-)
 create mode 100644 hw/misc/bcm2835_powermgt.c
 create mode 100644 include/hw/misc/bcm2835_powermgt.h

diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
index dcff13433e..48538c9360 100644
--- a/hw/arm/bcm2835_peripherals.c
+++ b/hw/arm/bcm2835_peripherals.c
@@ -126,6 +126,10 @@ static void bcm2835_peripherals_init(Object *obj)
 
     object_property_add_const_link(OBJECT(&s->dwc2), "dma-mr",
                                    OBJECT(&s->gpu_bus_mr));
+
+    /* Power Management */
+    object_initialize_child(obj, "powermgt", &s->powermgt,
+                            TYPE_BCM2835_POWERMGT);
 }
 
 static void bcm2835_peripherals_realize(DeviceState *dev, Error **errp)
@@ -364,9 +368,16 @@ static void bcm2835_peripherals_realize(DeviceState *dev, Error **errp)
         qdev_get_gpio_in_named(DEVICE(&s->ic), BCM2835_IC_GPU_IRQ,
                                INTERRUPT_USB));
 
+    /* Power Management */
+    if (!sysbus_realize(SYS_BUS_DEVICE(&s->powermgt), errp)) {
+        return;
+    }
+
+    memory_region_add_subregion(&s->peri_mr, PM_OFFSET,
+                sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->powermgt), 0));
+
     create_unimp(s, &s->txp, "bcm2835-txp", TXP_OFFSET, 0x1000);
     create_unimp(s, &s->armtmr, "bcm2835-sp804", ARMCTRL_TIMER0_1_OFFSET, 0x40);
-    create_unimp(s, &s->powermgt, "bcm2835-powermgt", PM_OFFSET, 0x114);
     create_unimp(s, &s->i2s, "bcm2835-i2s", I2S_OFFSET, 0x100);
     create_unimp(s, &s->smi, "bcm2835-smi", SMI_OFFSET, 0x100);
     create_unimp(s, &s->spi[0], "bcm2835-spi0", SPI0_OFFSET, 0x20);
diff --git a/hw/misc/bcm2835_powermgt.c b/hw/misc/bcm2835_powermgt.c
new file mode 100644
index 0000000000..81107ecc8f
--- /dev/null
+++ b/hw/misc/bcm2835_powermgt.c
@@ -0,0 +1,157 @@
+/*
+ * BCM2835 Power Management emulation
+ *
+ * Copyright (C) 2017 Marcin Chojnacki <marcinch7@gmail.com>
+ * Copyright (C) 2021 Nolan Leake <nolan@sigbus.net>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "hw/misc/bcm2835_powermgt.h"
+#include "migration/vmstate.h"
+#include "sysemu/runstate.h"
+
+#define PASSWORD 0x5a000000
+#define PASSWORD_MASK 0xff000000
+
+#define R_RSTC 0x1c
+#define V_RSTC_RESET 0x20
+#define R_RSTS 0x20
+#define V_RSTS_POWEROFF 0x555
+#define R_WDOG 0x24
+
+static uint64_t bcm2835_powermgt_read(void *opaque, hwaddr offset,
+                                      unsigned size)
+{
+    BCM2835PowerMgtState *s = (BCM2835PowerMgtState *)opaque;
+    uint32_t res = 0;
+
+    assert(size == 4);
+
+    switch (offset) {
+    case R_RSTC:
+        res = s->rstc;
+        break;
+    case R_RSTS:
+        res = s->rsts;
+        break;
+    case R_WDOG:
+        res = s->wdog;
+        break;
+
+    default:
+        qemu_log_mask(LOG_UNIMP,
+                      "bcm2835_powermgt_read: Unknown offset %x\n",
+                      (int)offset);
+        res = 0;
+        break;
+    }
+
+    return res;
+}
+
+static void bcm2835_powermgt_write(void *opaque, hwaddr offset,
+                                   uint64_t value, unsigned size)
+{
+    BCM2835PowerMgtState *s = (BCM2835PowerMgtState *)opaque;
+
+    assert(size == 4);
+
+    if ((value & PASSWORD_MASK) != PASSWORD) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "bcm2835_powermgt_write: Bad password %x at offset %x\n",
+                      (uint32_t)value, (int)offset);
+        return;
+    }
+
+    value = value & ~PASSWORD_MASK;
+
+    switch (offset) {
+    case R_RSTC:
+        s->rstc = value;
+        if (value & V_RSTC_RESET) {
+            if ((s->rsts & 0xfff) == V_RSTS_POWEROFF) {
+                /* Linux uses partition 63 to indicate halt. */
+                qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+            } else {
+                qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+            }
+        }
+        break;
+    case R_RSTS:
+        s->rsts = value;
+        break;
+    case R_WDOG:
+        s->wdog = value;
+        break;
+
+    default:
+        qemu_log_mask(LOG_UNIMP,
+                      "bcm2835_powermgt_write: Unknown offset %x\n",
+                      (int)offset);
+        break;
+    }
+}
+
+static const MemoryRegionOps bcm2835_powermgt_ops = {
+    .read = bcm2835_powermgt_read,
+    .write = bcm2835_powermgt_write,
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static const VMStateDescription vmstate_bcm2835_powermgt = {
+    .name = TYPE_BCM2835_POWERMGT,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32(rstc, BCM2835PowerMgtState),
+        VMSTATE_UINT32(rsts, BCM2835PowerMgtState),
+        VMSTATE_UINT32(wdog, BCM2835PowerMgtState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void bcm2835_powermgt_init(Object *obj)
+{
+    BCM2835PowerMgtState *s = BCM2835_POWERMGT(obj);
+
+    memory_region_init_io(&s->iomem, obj, &bcm2835_powermgt_ops, s,
+                          TYPE_BCM2835_POWERMGT, 0x114);
+    sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
+}
+
+static void bcm2835_powermgt_reset(DeviceState *dev)
+{
+    BCM2835PowerMgtState *s = BCM2835_POWERMGT(dev);
+
+    s->rstc = 0x00000102;
+    s->rsts = 0x00001000;
+    s->wdog = 0x00000000;
+}
+
+static void bcm2835_powermgt_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->reset = bcm2835_powermgt_reset;
+    dc->vmsd = &vmstate_bcm2835_powermgt;
+}
+
+static TypeInfo bcm2835_powermgt_info = {
+    .name          = TYPE_BCM2835_POWERMGT,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(BCM2835PowerMgtState),
+    .class_init    = bcm2835_powermgt_class_init,
+    .instance_init = bcm2835_powermgt_init,
+};
+
+static void bcm2835_powermgt_register_types(void)
+{
+    type_register_static(&bcm2835_powermgt_info);
+}
+
+type_init(bcm2835_powermgt_register_types)
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 66e1648533..f89b5c1643 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -82,6 +82,7 @@ softmmu_ss.add(when: 'CONFIG_RASPI', if_true: files(
   'bcm2835_rng.c',
   'bcm2835_thermal.c',
   'bcm2835_cprman.c',
+  'bcm2835_powermgt.c',
 ))
 softmmu_ss.add(when: 'CONFIG_SLAVIO', if_true: files('slavio_misc.c'))
 softmmu_ss.add(when: 'CONFIG_ZYNQ', if_true: files('zynq_slcr.c', 'zynq-xadc.c'))
diff --git a/include/hw/arm/bcm2835_peripherals.h b/include/hw/arm/bcm2835_peripherals.h
index 479e2346e8..d864879421 100644
--- a/include/hw/arm/bcm2835_peripherals.h
+++ b/include/hw/arm/bcm2835_peripherals.h
@@ -24,6 +24,7 @@
 #include "hw/misc/bcm2835_mphi.h"
 #include "hw/misc/bcm2835_thermal.h"
 #include "hw/misc/bcm2835_cprman.h"
+#include "hw/misc/bcm2835_powermgt.h"
 #include "hw/sd/sdhci.h"
 #include "hw/sd/bcm2835_sdhost.h"
 #include "hw/gpio/bcm2835_gpio.h"
@@ -48,7 +49,7 @@ struct BCM2835PeripheralState {
     BCM2835MphiState mphi;
     UnimplementedDeviceState txp;
     UnimplementedDeviceState armtmr;
-    UnimplementedDeviceState powermgt;
+    BCM2835PowerMgtState powermgt;
     BCM2835CprmanState cprman;
     PL011State uart0;
     BCM2835AuxState aux;
diff --git a/include/hw/misc/bcm2835_powermgt.h b/include/hw/misc/bcm2835_powermgt.h
new file mode 100644
index 0000000000..303b9a6f68
--- /dev/null
+++ b/include/hw/misc/bcm2835_powermgt.h
@@ -0,0 +1,29 @@
+/*
+ * BCM2835 Power Management emulation
+ *
+ * Copyright (C) 2017 Marcin Chojnacki <marcinch7@gmail.com>
+ * Copyright (C) 2021 Nolan Leake <nolan@sigbus.net>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef BCM2835_POWERMGT_H
+#define BCM2835_POWERMGT_H
+
+#include "hw/sysbus.h"
+#include "qom/object.h"
+
+#define TYPE_BCM2835_POWERMGT "bcm2835-powermgt"
+OBJECT_DECLARE_SIMPLE_TYPE(BCM2835PowerMgtState, BCM2835_POWERMGT)
+
+struct BCM2835PowerMgtState {
+    SysBusDevice busdev;
+    MemoryRegion iomem;
+
+    uint32_t rstc;
+    uint32_t rsts;
+    uint32_t wdog;
+};
+
+#endif
-- 
2.30.2


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH qemu] Add basic power management to raspi.
  2021-05-18 20:24 [PATCH qemu] Add basic power management to raspi ~nolanl
@ 2021-05-31 11:23 ` Philippe Mathieu-Daudé
  2021-06-02 21:33   ` Nolan
  0 siblings, 1 reply; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-31 11:23 UTC (permalink / raw)
  To: qemu-devel, Nolan Leake; +Cc: Peter Maydell, qemu-arm, Andrew Baumann

Hi Nolan,

Thanks for your patch!

There is something odd with your email address, which apparently
became sourcehut@sigbus.net instead of nolan@sigbus.net.

On 5/18/21 10:24 PM, ~nolanl wrote:
> From: Nolan Leake <nolan@sigbus.net>
> 
> This is just enough to make reboot and poweroff work.

Please precise "for Linux kernels", since this doesn't work
with firmwares (ideally we should implement the FIRMWARE_NOTIFY_REBOOT
property - which Linux sends - to handle the machine reboot/halt
via the videocore firmware model).

> Notably, the
> watchdog timer functionality is not yet implemented.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/64
> Signed-off-by: Nolan Leake <nolan@sigbus.net>
> ---
>  hw/arm/bcm2835_peripherals.c         |  13 ++-
>  hw/misc/bcm2835_powermgt.c           | 157 +++++++++++++++++++++++++++
>  hw/misc/meson.build                  |   1 +
>  include/hw/arm/bcm2835_peripherals.h |   3 +-
>  include/hw/misc/bcm2835_powermgt.h   |  29 +++++
>  5 files changed, 201 insertions(+), 2 deletions(-)
>  create mode 100644 hw/misc/bcm2835_powermgt.c
>  create mode 100644 include/hw/misc/bcm2835_powermgt.h
> 
> diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
> index dcff13433e..48538c9360 100644
> --- a/hw/arm/bcm2835_peripherals.c
> +++ b/hw/arm/bcm2835_peripherals.c
> @@ -126,6 +126,10 @@ static void bcm2835_peripherals_init(Object *obj)
>  
>      object_property_add_const_link(OBJECT(&s->dwc2), "dma-mr",
>                                     OBJECT(&s->gpu_bus_mr));
> +
> +    /* Power Management */
> +    object_initialize_child(obj, "powermgt", &s->powermgt,
> +                            TYPE_BCM2835_POWERMGT);
>  }
>  
>  static void bcm2835_peripherals_realize(DeviceState *dev, Error **errp)
> @@ -364,9 +368,16 @@ static void bcm2835_peripherals_realize(DeviceState *dev, Error **errp)
>          qdev_get_gpio_in_named(DEVICE(&s->ic), BCM2835_IC_GPU_IRQ,
>                                 INTERRUPT_USB));
>  
> +    /* Power Management */
> +    if (!sysbus_realize(SYS_BUS_DEVICE(&s->powermgt), errp)) {
> +        return;
> +    }
> +
> +    memory_region_add_subregion(&s->peri_mr, PM_OFFSET,
> +                sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->powermgt), 0));
> +
>      create_unimp(s, &s->txp, "bcm2835-txp", TXP_OFFSET, 0x1000);
>      create_unimp(s, &s->armtmr, "bcm2835-sp804", ARMCTRL_TIMER0_1_OFFSET, 0x40);
> -    create_unimp(s, &s->powermgt, "bcm2835-powermgt", PM_OFFSET, 0x114);
>      create_unimp(s, &s->i2s, "bcm2835-i2s", I2S_OFFSET, 0x100);
>      create_unimp(s, &s->smi, "bcm2835-smi", SMI_OFFSET, 0x100);
>      create_unimp(s, &s->spi[0], "bcm2835-spi0", SPI0_OFFSET, 0x20);
> diff --git a/hw/misc/bcm2835_powermgt.c b/hw/misc/bcm2835_powermgt.c
> new file mode 100644
> index 0000000000..81107ecc8f
> --- /dev/null
> +++ b/hw/misc/bcm2835_powermgt.c
> @@ -0,0 +1,157 @@
> +/*
> + * BCM2835 Power Management emulation
> + *
> + * Copyright (C) 2017 Marcin Chojnacki <marcinch7@gmail.com>
> + * Copyright (C) 2021 Nolan Leake <nolan@sigbus.net>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "hw/misc/bcm2835_powermgt.h"
> +#include "migration/vmstate.h"
> +#include "sysemu/runstate.h"
> +
> +#define PASSWORD 0x5a000000
> +#define PASSWORD_MASK 0xff000000
> +
> +#define R_RSTC 0x1c
> +#define V_RSTC_RESET 0x20
> +#define R_RSTS 0x20
> +#define V_RSTS_POWEROFF 0x555
> +#define R_WDOG 0x24
> +
> +static uint64_t bcm2835_powermgt_read(void *opaque, hwaddr offset,
> +                                      unsigned size)
> +{
> +    BCM2835PowerMgtState *s = (BCM2835PowerMgtState *)opaque;
> +    uint32_t res = 0;
> +
> +    assert(size == 4);

Instead of this assert, add in bcm2835_powermgt_ops:

  .impl.min_access_size = 4,
  .impl.max_access_size = 4,

> +
> +    switch (offset) {
> +    case R_RSTC:
> +        res = s->rstc;
> +        break;
> +    case R_RSTS:
> +        res = s->rsts;
> +        break;
> +    case R_WDOG:
> +        res = s->wdog;
> +        break;
> +
> +    default:
> +        qemu_log_mask(LOG_UNIMP,
> +                      "bcm2835_powermgt_read: Unknown offset %x\n",
> +                      (int)offset);
> +        res = 0;
> +        break;
> +    }
> +
> +    return res;
> +}
> +
> +static void bcm2835_powermgt_write(void *opaque, hwaddr offset,
> +                                   uint64_t value, unsigned size)
> +{
> +    BCM2835PowerMgtState *s = (BCM2835PowerMgtState *)opaque;
> +
> +    assert(size == 4);

(remove this assert too).

> +    if ((value & PASSWORD_MASK) != PASSWORD) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "bcm2835_powermgt_write: Bad password %x at offset %x\n",

Please prefix hexadecimal formats with 0x,

> +                      (uint32_t)value, (int)offset);

and do not cast.

> +        return;
> +    }
> +
> +    value = value & ~PASSWORD_MASK;
> +
> +    switch (offset) {
> +    case R_RSTC:
> +        s->rstc = value;
> +        if (value & V_RSTC_RESET) {
> +            if ((s->rsts & 0xfff) == V_RSTS_POWEROFF) {
> +                /* Linux uses partition 63 to indicate halt. */

I'd move this comment with the V_RSTS_POWEROFF definition.

> +                qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> +            } else {
> +                qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> +            }
> +        }

Shouldn't we log the unsupported bits?

> +        break;
> +    case R_RSTS:
> +        s->rsts = value;
> +        break;
> +    case R_WDOG:
> +        s->wdog = value;
> +        break;
> +
> +    default:
> +        qemu_log_mask(LOG_UNIMP,
> +                      "bcm2835_powermgt_write: Unknown offset %x\n",
> +                      (int)offset);

Please do not cast, use the proper format.

> +        break;
> +    }
> +}
> +
> +static const MemoryRegionOps bcm2835_powermgt_ops = {
> +    .read = bcm2835_powermgt_read,
> +    .write = bcm2835_powermgt_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static const VMStateDescription vmstate_bcm2835_powermgt = {
> +    .name = TYPE_BCM2835_POWERMGT,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(rstc, BCM2835PowerMgtState),
> +        VMSTATE_UINT32(rsts, BCM2835PowerMgtState),
> +        VMSTATE_UINT32(wdog, BCM2835PowerMgtState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static void bcm2835_powermgt_init(Object *obj)
> +{
> +    BCM2835PowerMgtState *s = BCM2835_POWERMGT(obj);
> +
> +    memory_region_init_io(&s->iomem, obj, &bcm2835_powermgt_ops, s,
> +                          TYPE_BCM2835_POWERMGT, 0x114);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
> +}
> +
> +static void bcm2835_powermgt_reset(DeviceState *dev)
> +{
> +    BCM2835PowerMgtState *s = BCM2835_POWERMGT(dev);
> +
> +    s->rstc = 0x00000102;
> +    s->rsts = 0x00001000;
> +    s->wdog = 0x00000000;

Where these bits come from?

> +}
> +
> +static void bcm2835_powermgt_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->reset = bcm2835_powermgt_reset;
> +    dc->vmsd = &vmstate_bcm2835_powermgt;
> +}
> +
> +static TypeInfo bcm2835_powermgt_info = {
> +    .name          = TYPE_BCM2835_POWERMGT,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(BCM2835PowerMgtState),
> +    .class_init    = bcm2835_powermgt_class_init,
> +    .instance_init = bcm2835_powermgt_init,
> +};

Minor comments, otherwise:

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH qemu] Add basic power management to raspi.
  2021-05-31 11:23 ` Philippe Mathieu-Daudé
@ 2021-06-02 21:33   ` Nolan
  2021-06-02 23:22     ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 5+ messages in thread
From: Nolan @ 2021-06-02 21:33 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Peter Maydell, qemu-arm, Andrew Baumann

On 5/31/21 4:23 AM, Philippe Mathieu-Daudé wrote:
> Hi Nolan,
> 
> Thanks for your patch!
> 
> There is something odd with your email address, which apparently
> became sourcehut@sigbus.net instead of nolan@sigbus.net.

Ugh, oops. I was trying out sourcehut for this, and reflexively gave 
them a marker email. I'm pretty sure sourcehut won't sell my email 
address, so I'll just change it.

> On 5/18/21 10:24 PM, ~nolanl wrote:
>> From: Nolan Leake <nolan@sigbus.net>
>>
>> This is just enough to make reboot and poweroff work.
> 
> Please precise "for Linux kernels", since this doesn't work
> with firmwares (ideally we should implement the FIRMWARE_NOTIFY_REBOOT
> property - which Linux sends - to handle the machine reboot/halt
> via the videocore firmware model).

Thanks, good point re: this being tuned to what Linux (and u-boot) do. 
Poking around a bit, it looks like 
"trusted-firmware-a"/"arm-trusted-firmware" uses the same method, as do 
a couple of bare-metal/hobby OSes. Couldn't immediately figure out what 
FreeBSD does.

I'm not sure I understand your point about FIRMWARE_NOTIFY_REBOOT, my 
understanding is that message is there to tell the GPU firmware that 
we're about to reboot, so it can do things like reload the PCIe USB 
chip's firmware. In my testing without the watchdog module loaded, my 
physical pi4 does not reboot, so it appears that sending 
FIRMWARE_NOTIFY_REBOOT is not enough on its own.

>> Notably, the
>> watchdog timer functionality is not yet implemented.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/64
>> Signed-off-by: Nolan Leake <nolan@sigbus.net>
>> ---
>>   hw/arm/bcm2835_peripherals.c         |  13 ++-
>>   hw/misc/bcm2835_powermgt.c           | 157 +++++++++++++++++++++++++++
>>   hw/misc/meson.build                  |   1 +
>>   include/hw/arm/bcm2835_peripherals.h |   3 +-
>>   include/hw/misc/bcm2835_powermgt.h   |  29 +++++
>>   5 files changed, 201 insertions(+), 2 deletions(-)
>>   create mode 100644 hw/misc/bcm2835_powermgt.c
>>   create mode 100644 include/hw/misc/bcm2835_powermgt.h
>>
>> diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
>> index dcff13433e..48538c9360 100644
>> --- a/hw/arm/bcm2835_peripherals.c
>> +++ b/hw/arm/bcm2835_peripherals.c
>> @@ -126,6 +126,10 @@ static void bcm2835_peripherals_init(Object *obj)
>>   
>>       object_property_add_const_link(OBJECT(&s->dwc2), "dma-mr",
>>                                      OBJECT(&s->gpu_bus_mr));
>> +
>> +    /* Power Management */
>> +    object_initialize_child(obj, "powermgt", &s->powermgt,
>> +                            TYPE_BCM2835_POWERMGT);
>>   }
>>   
>>   static void bcm2835_peripherals_realize(DeviceState *dev, Error **errp)
>> @@ -364,9 +368,16 @@ static void bcm2835_peripherals_realize(DeviceState *dev, Error **errp)
>>           qdev_get_gpio_in_named(DEVICE(&s->ic), BCM2835_IC_GPU_IRQ,
>>                                  INTERRUPT_USB));
>>   
>> +    /* Power Management */
>> +    if (!sysbus_realize(SYS_BUS_DEVICE(&s->powermgt), errp)) {
>> +        return;
>> +    }
>> +
>> +    memory_region_add_subregion(&s->peri_mr, PM_OFFSET,
>> +                sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->powermgt), 0));
>> +
>>       create_unimp(s, &s->txp, "bcm2835-txp", TXP_OFFSET, 0x1000);
>>       create_unimp(s, &s->armtmr, "bcm2835-sp804", ARMCTRL_TIMER0_1_OFFSET, 0x40);
>> -    create_unimp(s, &s->powermgt, "bcm2835-powermgt", PM_OFFSET, 0x114);
>>       create_unimp(s, &s->i2s, "bcm2835-i2s", I2S_OFFSET, 0x100);
>>       create_unimp(s, &s->smi, "bcm2835-smi", SMI_OFFSET, 0x100);
>>       create_unimp(s, &s->spi[0], "bcm2835-spi0", SPI0_OFFSET, 0x20);
>> diff --git a/hw/misc/bcm2835_powermgt.c b/hw/misc/bcm2835_powermgt.c
>> new file mode 100644
>> index 0000000000..81107ecc8f
>> --- /dev/null
>> +++ b/hw/misc/bcm2835_powermgt.c
>> @@ -0,0 +1,157 @@
>> +/*
>> + * BCM2835 Power Management emulation
>> + *
>> + * Copyright (C) 2017 Marcin Chojnacki <marcinch7@gmail.com>
>> + * Copyright (C) 2021 Nolan Leake <nolan@sigbus.net>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "qemu/log.h"
>> +#include "qemu/module.h"
>> +#include "hw/misc/bcm2835_powermgt.h"
>> +#include "migration/vmstate.h"
>> +#include "sysemu/runstate.h"
>> +
>> +#define PASSWORD 0x5a000000
>> +#define PASSWORD_MASK 0xff000000
>> +
>> +#define R_RSTC 0x1c
>> +#define V_RSTC_RESET 0x20
>> +#define R_RSTS 0x20
>> +#define V_RSTS_POWEROFF 0x555
>> +#define R_WDOG 0x24
>> +
>> +static uint64_t bcm2835_powermgt_read(void *opaque, hwaddr offset,
>> +                                      unsigned size)
>> +{
>> +    BCM2835PowerMgtState *s = (BCM2835PowerMgtState *)opaque;
>> +    uint32_t res = 0;
>> +
>> +    assert(size == 4);
> 
> Instead of this assert, add in bcm2835_powermgt_ops:
> 
>    .impl.min_access_size = 4,
>    .impl.max_access_size = 4,

Will do.

>> +
>> +    switch (offset) {
>> +    case R_RSTC:
>> +        res = s->rstc;
>> +        break;
>> +    case R_RSTS:
>> +        res = s->rsts;
>> +        break;
>> +    case R_WDOG:
>> +        res = s->wdog;
>> +        break;
>> +
>> +    default:
>> +        qemu_log_mask(LOG_UNIMP,
>> +                      "bcm2835_powermgt_read: Unknown offset %x\n",
>> +                      (int)offset);
>> +        res = 0;
>> +        break;
>> +    }
>> +
>> +    return res;
>> +}
>> +
>> +static void bcm2835_powermgt_write(void *opaque, hwaddr offset,
>> +                                   uint64_t value, unsigned size)
>> +{
>> +    BCM2835PowerMgtState *s = (BCM2835PowerMgtState *)opaque;
>> +
>> +    assert(size == 4);
> 
> (remove this assert too).

OK.

>> +    if ((value & PASSWORD_MASK) != PASSWORD) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "bcm2835_powermgt_write: Bad password %x at offset %x\n",
> 
> Please prefix hexadecimal formats with 0x,
> 
>> +                      (uint32_t)value, (int)offset);
> 
> and do not cast.

OK.

>> +        return;
>> +    }
>> +
>> +    value = value & ~PASSWORD_MASK;
>> +
>> +    switch (offset) {
>> +    case R_RSTC:
>> +        s->rstc = value;
>> +        if (value & V_RSTC_RESET) {
>> +            if ((s->rsts & 0xfff) == V_RSTS_POWEROFF) {
>> +                /* Linux uses partition 63 to indicate halt. */
> 
> I'd move this comment with the V_RSTS_POWEROFF definition.

OK.

>> +                qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
>> +            } else {
>> +                qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>> +            }
>> +        }
> 
> Shouldn't we log the unsupported bits?

I can add that, I didn't originally because the unsupported writes are 
expected.

>> +        break;
>> +    case R_RSTS:
>> +        s->rsts = value;
>> +        break;
>> +    case R_WDOG:
>> +        s->wdog = value;
>> +        break;
>> +
>> +    default:
>> +        qemu_log_mask(LOG_UNIMP,
>> +                      "bcm2835_powermgt_write: Unknown offset %x\n",
>> +                      (int)offset);
> 
> Please do not cast, use the proper format.

OK.

>> +        break;
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps bcm2835_powermgt_ops = {
>> +    .read = bcm2835_powermgt_read,
>> +    .write = bcm2835_powermgt_write,
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +};
>> +
>> +static const VMStateDescription vmstate_bcm2835_powermgt = {
>> +    .name = TYPE_BCM2835_POWERMGT,
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT32(rstc, BCM2835PowerMgtState),
>> +        VMSTATE_UINT32(rsts, BCM2835PowerMgtState),
>> +        VMSTATE_UINT32(wdog, BCM2835PowerMgtState),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static void bcm2835_powermgt_init(Object *obj)
>> +{
>> +    BCM2835PowerMgtState *s = BCM2835_POWERMGT(obj);
>> +
>> +    memory_region_init_io(&s->iomem, obj, &bcm2835_powermgt_ops, s,
>> +                          TYPE_BCM2835_POWERMGT, 0x114);
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
>> +}
>> +
>> +static void bcm2835_powermgt_reset(DeviceState *dev)
>> +{
>> +    BCM2835PowerMgtState *s = BCM2835_POWERMGT(dev);
>> +
>> +    s->rstc = 0x00000102;
>> +    s->rsts = 0x00001000;
>> +    s->wdog = 0x00000000;
> 
> Where these bits come from?

 From my pi4. https://elinux.org/BCM2835_registers agrees (processed 
from Broadcom source code).

>> +}
>> +
>> +static void bcm2835_powermgt_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->reset = bcm2835_powermgt_reset;
>> +    dc->vmsd = &vmstate_bcm2835_powermgt;
>> +}
>> +
>> +static TypeInfo bcm2835_powermgt_info = {
>> +    .name          = TYPE_BCM2835_POWERMGT,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(BCM2835PowerMgtState),
>> +    .class_init    = bcm2835_powermgt_class_init,
>> +    .instance_init = bcm2835_powermgt_init,
>> +};
> 
> Minor comments, otherwise:
> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> 

I'll make the changes and send a v2.

- nolan


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH qemu] Add basic power management to raspi.
  2021-06-02 21:33   ` Nolan
@ 2021-06-02 23:22     ` Philippe Mathieu-Daudé
  2021-06-02 23:32       ` Nolan
  0 siblings, 1 reply; 5+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-06-02 23:22 UTC (permalink / raw)
  To: Nolan, qemu-devel; +Cc: Peter Maydell, qemu-arm, Andrew Baumann

On 6/2/21 11:33 PM, Nolan wrote:
> On 5/31/21 4:23 AM, Philippe Mathieu-Daudé wrote:
>> Hi Nolan,
>>
>> Thanks for your patch!
>>
>> There is something odd with your email address, which apparently
>> became sourcehut@sigbus.net instead of nolan@sigbus.net.
> 
> Ugh, oops. I was trying out sourcehut for this, and reflexively gave
> them a marker email. I'm pretty sure sourcehut won't sell my email
> address, so I'll just change it.
> 
>> On 5/18/21 10:24 PM, ~nolanl wrote:
>>> From: Nolan Leake <nolan@sigbus.net>
>>>
>>> This is just enough to make reboot and poweroff work.
>>
>> Please precise "for Linux kernels", since this doesn't work
>> with firmwares (ideally we should implement the FIRMWARE_NOTIFY_REBOOT
>> property - which Linux sends - to handle the machine reboot/halt
>> via the videocore firmware model).
> 
> Thanks, good point re: this being tuned to what Linux (and u-boot) do.
> Poking around a bit, it looks like
> "trusted-firmware-a"/"arm-trusted-firmware" uses the same method, as do
> a couple of bare-metal/hobby OSes. Couldn't immediately figure out what
> FreeBSD does.
> 
> I'm not sure I understand your point about FIRMWARE_NOTIFY_REBOOT, my
> understanding is that message is there to tell the GPU firmware that
> we're about to reboot, so it can do things like reload the PCIe USB
> chip's firmware. In my testing without the watchdog module loaded, my
> physical pi4 does not reboot, so it appears that sending
> FIRMWARE_NOTIFY_REBOOT is not enough on its own.

From the ARM core point of view, once it sent the FIRMWARE_NOTIFY_REBOOT
message, it can't really power-off itself, it waits in a busy loop for
the VC to disable its power domain.

hw/misc/bcm2835_property.c is our model of the VC behavior. IMO this
should be where QEMU shuts down. How I'd model it is:

- ARM: sends FIRMWARE_NOTIFY_REBOOT and loops

- VC emulated via property: delays (200ms?) then calls
  SHUTDOWN_CAUSE_GUEST_RESET.

(it helps to see hw/misc/bcm2835_property.c as an external component).

>>> qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
>>> +            } else {
>>> +                qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>>> +            }
>>> +        }
>>
>> Shouldn't we log the unsupported bits?
> 
> I can add that, I didn't originally because the unsupported writes are
> expected.

I'd prefer we log them, even if unsupported, so in case something
behaves oddly we could look at those bits.

>>> +static void bcm2835_powermgt_reset(DeviceState *dev)
>>> +{
>>> +    BCM2835PowerMgtState *s = BCM2835_POWERMGT(dev);
>>> +
>>> +    s->rstc = 0x00000102;
>>> +    s->rsts = 0x00001000;
>>> +    s->wdog = 0x00000000;
>>
>> Where these bits come from?
> 
> From my pi4. https://elinux.org/BCM2835_registers agrees (processed from
> Broadcom source code).

OK, so please add a comment referring to
https://elinux.org/BCM2835_registers#PM.

Looking forward for v2 :)

Phil.


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH qemu] Add basic power management to raspi.
  2021-06-02 23:22     ` Philippe Mathieu-Daudé
@ 2021-06-02 23:32       ` Nolan
  0 siblings, 0 replies; 5+ messages in thread
From: Nolan @ 2021-06-02 23:32 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Peter Maydell, qemu-arm, Andrew Baumann

On 6/2/21 4:22 PM, Philippe Mathieu-Daudé wrote:
> On 6/2/21 11:33 PM, Nolan wrote:
>> On 5/31/21 4:23 AM, Philippe Mathieu-Daudé wrote:
>>> Hi Nolan,
>>>
>>> Thanks for your patch!
>>>
>>> There is something odd with your email address, which apparently
>>> became sourcehut@sigbus.net instead of nolan@sigbus.net.
>>
>> Ugh, oops. I was trying out sourcehut for this, and reflexively gave
>> them a marker email. I'm pretty sure sourcehut won't sell my email
>> address, so I'll just change it.
>>
>>> On 5/18/21 10:24 PM, ~nolanl wrote:
>>>> From: Nolan Leake <nolan@sigbus.net>
>>>>
>>>> This is just enough to make reboot and poweroff work.
>>>
>>> Please precise "for Linux kernels", since this doesn't work
>>> with firmwares (ideally we should implement the FIRMWARE_NOTIFY_REBOOT
>>> property - which Linux sends - to handle the machine reboot/halt
>>> via the videocore firmware model).
>>
>> Thanks, good point re: this being tuned to what Linux (and u-boot) do.
>> Poking around a bit, it looks like
>> "trusted-firmware-a"/"arm-trusted-firmware" uses the same method, as do
>> a couple of bare-metal/hobby OSes. Couldn't immediately figure out what
>> FreeBSD does.
>>
>> I'm not sure I understand your point about FIRMWARE_NOTIFY_REBOOT, my
>> understanding is that message is there to tell the GPU firmware that
>> we're about to reboot, so it can do things like reload the PCIe USB
>> chip's firmware. In my testing without the watchdog module loaded, my
>> physical pi4 does not reboot, so it appears that sending
>> FIRMWARE_NOTIFY_REBOOT is not enough on its own.
> 
>  From the ARM core point of view, once it sent the FIRMWARE_NOTIFY_REBOOT
> message, it can't really power-off itself, it waits in a busy loop for
> the VC to disable its power domain.
> 
> hw/misc/bcm2835_property.c is our model of the VC behavior. IMO this
> should be where QEMU shuts down. How I'd model it is:
> 
> - ARM: sends FIRMWARE_NOTIFY_REBOOT and loops
> 
> - VC emulated via property: delays (200ms?) then calls
>    SHUTDOWN_CAUSE_GUEST_RESET.
> 
> (it helps to see hw/misc/bcm2835_property.c as an external component).

This is not what I see on my hardware pi4. With the following kernel config:
...
CONFIG_RASPBERRYPI_FIRMWARE=y
...
CONFIG_BCM2835_WDT=m
...

if I reboot the machine without the WDT module (but with the firmware 
module), I get this:
#  echo b > /proc/sysrq-trigger
[   54.498768] sysrq: Resetting
[   54.501713] SMP: stopping secondary CPUs
[   54.505701] Reboot failed -- System halted

and it hangs forever there.

If I load the WDT module, it reboots successfully.

>>>> qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
>>>> +            } else {
>>>> +                qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>>>> +            }
>>>> +        }
>>>
>>> Shouldn't we log the unsupported bits?
>>
>> I can add that, I didn't originally because the unsupported writes are
>> expected.
> 
> I'd prefer we log them, even if unsupported, so in case something
> behaves oddly we could look at those bits.

In v2, I log the writes, since any side effects they have (notably the 
watchdog register) are unimplemented. I didn't log the reads, since they 
actually behave exactly as the real hardware does...

>>>> +static void bcm2835_powermgt_reset(DeviceState *dev)
>>>> +{
>>>> +    BCM2835PowerMgtState *s = BCM2835_POWERMGT(dev);
>>>> +
>>>> +    s->rstc = 0x00000102;
>>>> +    s->rsts = 0x00001000;
>>>> +    s->wdog = 0x00000000;
>>>
>>> Where these bits come from?
>>
>>  From my pi4. https://elinux.org/BCM2835_registers agrees (processed from
>> Broadcom source code).
> 
> OK, so please add a comment referring to
> https://elinux.org/BCM2835_registers#PM.
> 
> Looking forward for v2 :)

Already sent. Is the link comment here worth a v3?



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-06-02 23:32 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-18 20:24 [PATCH qemu] Add basic power management to raspi ~nolanl
2021-05-31 11:23 ` Philippe Mathieu-Daudé
2021-06-02 21:33   ` Nolan
2021-06-02 23:22     ` Philippe Mathieu-Daudé
2021-06-02 23:32       ` Nolan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.