Re: [PATCH v5 17/41] Implement BCM2838 thermal sensor

[Peter Maydell]

On Mon, 19 Feb 2024 at 01:21, Sergey Kambalin <serg.oker@gmail.com> wrote:
>
> Signed-off-by: Sergey Kambalin <sergey.kambalin@auriga.com>
> ---
>  hw/arm/bcm2838_peripherals.c         | 27 ++++++--
>  hw/arm/raspi4b.c                     |  1 -
>  hw/misc/bcm2838_thermal.c            | 98 ++++++++++++++++++++++++++++
>  hw/misc/meson.build                  |  3 +-
>  include/hw/arm/bcm2838_peripherals.h |  2 +
>  include/hw/misc/bcm2838_thermal.h    | 24 +++++++
>  6 files changed, 147 insertions(+), 8 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 6b4c840c5b..48c5fd5978 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);
> @@ -78,6 +81,9 @@ static void bcm2838_peripherals_realize(DeviceState *dev, 
> Error **errp)
>      BCMSocPeripheralBaseState *s_base = BCM_SOC_PERIPHERALS_BASE(dev);
>      MemoryRegion *regs_mr;
>      MemoryRegion *mmio_mr;
> +    MemoryRegion *rng200_mr;
> +    MemoryRegion *thermal_mr;
> +    qemu_irq rng_200_irq;
>
>      int n;
>
> @@ -95,12 +101,20 @@ static void bcm2838_peripherals_realize(DeviceState 
> *dev, Error **errp)
>      if (!sysbus_realize(SYS_BUS_DEVICE(&s->rng200), errp)) {
>          return;
>      }
> -    memory_region_add_subregion(
> -        &s_base->peri_mr, RNG_OFFSET,
> -        sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->rng200), 0));
> -    sysbus_connect_irq(SYS_BUS_DEVICE(&s->rng200), 0,
> -        qdev_get_gpio_in_named(DEVICE(&s_base->ic), BCM2835_IC_GPU_IRQ,
> -                               INTERRUPT_RNG));
> +    rng200_mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->rng200), 0);
> +    memory_region_add_subregion(&s_base->peri_mr, RNG_OFFSET, rng200_mr);
> +
> +    rng_200_irq = qdev_get_gpio_in_named(DEVICE(&s_base->ic),
> +                                         BCM2835_IC_GPU_IRQ, INTERRUPT_RNG);
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->rng200), 0, rng_200_irq);

These are RNG200 related and look like they should be squashed into
the patch that added the RNG200 to the SoC.

> +
> +
> +    /* THERMAL */
> +    if (!sysbus_realize(SYS_BUS_DEVICE(&s->thermal), errp)) {
> +        return;
> +    }
> +    thermal_mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->thermal), 0);
> +    memory_region_add_subregion( &s->peri_low_mr, 0x15D2000, thermal_mr);
>
>      /* Extended Mass Media Controller 2 */
>      object_property_set_uint(OBJECT(&s->emmc2), "sd-spec-version", 3,
> @@ -201,6 +215,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);
> +

Stray whitespace change: should be in some other patch, I suspect
(maybe the one adding the pcie to the SoC?)

>      /* PCIe Root Complex */
>      if (!sysbus_realize(SYS_BUS_DEVICE(&s->pcie_host), errp)) {
>          return;

> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> index 7c769fd2a4..8a8c5ce659 100644
> --- a/hw/misc/meson.build
> +++ b/hw/misc/meson.build
> @@ -91,7 +91,8 @@ system_ss.add(when: 'CONFIG_RASPI', if_true: files(
>    'bcm2835_thermal.c',
>    'bcm2835_cprman.c',
>    'bcm2835_powermgt.c',
> -  'bcm2838_rng200.c'
> +  'bcm2838_rng200.c',
> +  'bcm2838_thermal.c'

If you always keep a trailing comma on the end of this kind of list,
then you don't have to modify the preceding line when you add a new one.

>  ))
>  system_ss.add(when: 'CONFIG_SLAVIO', if_true: files('slavio_misc.c'))
>  system_ss.add(when: 'CONFIG_ZYNQ', if_true: files('zynq_slcr.c'))

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM