Re: [PATCH 20/44] Implement BCM2838 thermal sensor

[Peter Maydell]

On Wed, 26 Jul 2023 at 14:48, Sergey Kambalin <serg.oker@gmail.com> wrote:
>
> Signed-off-by: Sergey Kambalin <sergey.kambalin@auriga.com>
> ---
>  hw/arm/bcm2838_peripherals.c         | 12 ++++
>  hw/arm/raspi4b.c                     |  1 -
>  hw/misc/bcm2838_thermal.c            | 96 ++++++++++++++++++++++++++++
>  hw/misc/meson.build                  |  3 +-
>  include/hw/arm/bcm2838_peripherals.h |  2 +
>  include/hw/misc/bcm2838_thermal.h    | 24 +++++++
>  6 files changed, 136 insertions(+), 2 deletions(-)
>  create mode 100644 hw/misc/bcm2838_thermal.c
>  create mode 100644 include/hw/misc/bcm2838_thermal.h
>
> diff --git a/hw/arm/bcm2838_peripherals.c b/hw/arm/bcm2838_peripherals.c
> index df97953bbe..e7a9db97ab 100644
> --- a/hw/arm/bcm2838_peripherals.c
> +++ b/hw/arm/bcm2838_peripherals.c
> @@ -37,6 +37,9 @@ static void bcm2838_peripherals_init(Object *obj)
>      /* Random Number Generator */
>      object_initialize_child(obj, "rng200", &s->rng200, TYPE_BCM2838_RNG200);
>
> +    /* Thermal */
> +    object_initialize_child(obj, "thermal", &s->thermal, 
> TYPE_BCM2838_THERMAL);
> +
>      /* PCIe Host Bridge */
>      object_initialize_child(obj, "pcie-host", &s->pcie_host,
>                              TYPE_BCM2838_PCIE_HOST);
> @@ -98,6 +101,14 @@ static void bcm2838_peripherals_realize(DeviceState *dev, 
> Error **errp)
>          qdev_get_gpio_in_named(DEVICE(&s_base->ic), BCM2835_IC_GPU_IRQ,
>                                 INTERRUPT_RNG));
>
> +    /* THERMAL */
> +    if (!sysbus_realize(SYS_BUS_DEVICE(&s->thermal), errp)) {
> +        return;
> +    }
> +    memory_region_add_subregion(
> +        &s->peri_low_mr, 0x15D2000,
> +        sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->thermal), 0));

(more odd indentation here)

> +
>      /* Extended Mass Media Controller 2 */
>      object_property_set_uint(OBJECT(&s->emmc2), "sd-spec-version", 3,
>                               &error_abort);
> @@ -197,6 +208,7 @@ static void bcm2838_peripherals_realize(DeviceState *dev, 
> Error **errp)
>                               BCM2838_MPHI_SIZE);
>      memory_region_add_subregion(&s_base->peri_mr, BCM2838_MPHI_OFFSET,
>                                  &s->mphi_mr_alias);
> +
>      /* PCIe Root Complex */
>      if (!sysbus_realize(SYS_BUS_DEVICE(&s->pcie_host), errp)) {
>          return;
> diff --git a/hw/arm/raspi4b.c b/hw/arm/raspi4b.c
> index 82b6b48d3c..c7fdf791d5 100644
> --- a/hw/arm/raspi4b.c
> +++ b/hw/arm/raspi4b.c
> @@ -66,7 +66,6 @@ static void raspi4_modify_dtb(const struct arm_boot_info 
> *info, void *fdt)
>
>      /* Temporary disable following devices until they are implemented*/
>      const char *to_be_removed_from_dt_as_wa[] = {
> -        "brcm,bcm2711-thermal",
>          "brcm,bcm2711-genet-v5",
>      };
>
> diff --git a/hw/misc/bcm2838_thermal.c b/hw/misc/bcm2838_thermal.c
> new file mode 100644
> index 0000000000..a488dc3da1
> --- /dev/null
> +++ b/hw/misc/bcm2838_thermal.c
> @@ -0,0 +1,96 @@
> +/*
> + * BCM2838 dummy thermal sensor
> + *
> + * Copyright (C) 2022 Maksim Kopusov <maksim.kopusov@auriga.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qapi/error.h"
> +#include "hw/misc/bcm2838_thermal.h"
> +#include "hw/registerfields.h"
> +#include "migration/vmstate.h"
> +#include "qemu/error-report.h"
> +
> +REG32(STAT, 0x200)
> +FIELD(STAT, DATA, 0, 10)
> +FIELD(STAT, VALID_1, 10, 1)
> +FIELD(STAT, VALID_2, 16, 1)
> +
> +#define BCM2838_THERMAL_SIZE 0xf00
> +
> +#define THERMAL_OFFSET_C 410040
> +#define THERMAL_COEFF  (-487.0f)
> +#define MILLIDEGREE_COEFF 1000
> +
> +static uint16_t bcm2838_thermal_temp2adc(int temp_C)
> +{
> +    return (temp_C * MILLIDEGREE_COEFF - THERMAL_OFFSET_C) / THERMAL_COEFF;
> +}
> +
> +static uint64_t bcm2838_thermal_read(void *opaque, hwaddr addr, unsigned 
> size)
> +{
> +    uint32_t val = 0;
> +
> +    switch (addr) {
> +    case A_STAT:
> +        /* Temperature is always 25°C */
> +         val = FIELD_DP32(val, STAT, DATA, bcm2838_thermal_temp2adc(25));
> +         val = FIELD_DP32(val, STAT, VALID_1, 1);
> +         val = FIELD_DP32(val, STAT, VALID_2, 1);
> +
> +         break;
> +    default:
> +         /* MemoryRegionOps are aligned, so this can not happen. */

This comment doesn't seem right. BCM2838_THERMAL_SIZE is larger
than 4, so we can have more than one possible address accessed,
even if the address must be aligned. So the warning is
OK, but the g_assert_reached() and the comment are not.

> +         warn_report("%s can't access addr: 0x%"PRIx64,
> +                     TYPE_BCM2838_THERMAL, addr);

We usually use a LOG_GUEST_ERROR for this kind of "access
with an address that doesn't correspond to a register".

> +         g_assert_not_reached();
> +    }
> +    return val;
> +}
> +
> +static const MemoryRegionOps bcm2838_thermal_ops = {
> +    .read = bcm2838_thermal_read,

You need to provide a .write, even if it just does a
LOG_GUEST_ERROR for every access. Otherwise if the guest
tries to write then QEMU will crash trying to call the NULL
pointer.

> +    .impl.max_access_size = 4,
> +    .valid.min_access_size = 4,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void bcm2838_thermal_realize(DeviceState *dev, Error **errp)
> +{
> +    Bcm2838ThermalState *s = BCM2838_THERMAL(dev);
> +
> +    memory_region_init_io(&s->iomem, OBJECT(s), &bcm2838_thermal_ops,
> +                          s, TYPE_BCM2838_THERMAL, BCM2838_THERMAL_SIZE);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
> +}
> +
> +static const VMStateDescription bcm2838_thermal_vmstate = {
> +    .name = TYPE_BCM2838_THERMAL,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +};

You don't need to set up a vmstate if it has no data.

> +
> +static void bcm2838_thermal_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = bcm2838_thermal_realize;
> +    dc->vmsd = &bcm2838_thermal_vmstate;

You can add a comment here:
 /* This device has no state: no need for vmstate or reset */

to tell the reader you haven't forgotten it.

> +}
> +
> +static const TypeInfo bcm2838_thermal_info = {
> +    .name = TYPE_BCM2838_THERMAL,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(Bcm2838ThermalState),
> +    .class_init = bcm2838_thermal_class_init,
> +};
> +
> +static void bcm2838_thermal_register_types(void)
> +{
> +    type_register_static(&bcm2838_thermal_info);
> +}
> +

thanks
-- PMM