Re: [PATCH RFC v2 09/30] hw/misc: add RP2040 PLL and clock controller

["Alex Bennée" <alex.bennee@linaro.org>]

Gilles Grimaud <gilles.grimaud@univ-lille.fr> writes:

> From: gilles grimaud <gilles.grimaud@univ-lille.fr>
>
> Model the RP2040 system and USB PLL register blocks and their clock outputs. Add the clock mux, divider, selected-source and frequency-counter registers, then route clk-sys to the Cortex-M0+ and clk-peri to both UARTs. Include focused qtests for PLL configuration, clock switching and frequency measurement.
>
> Signed-off-by: gilles grimaud <gilles.grimaud@univ-lille.fr>
> ---
>  hw/arm/Kconfig                   |   2 +
<snip>
> +
> +#define ATOMIC_ALIAS_MASK    0x3000
> +#define ATOMIC_XOR           0x1000
> +#define ATOMIC_SET           0x2000
> +#define ATOMIC_CLR           0x3000

These seem to be common defines - why not have a common 2040 header that
they all use.

<snip>
> diff --git a/hw/misc/rp2040_pll.c b/hw/misc/rp2040_pll.c
> new file mode 100644
> index 0000000000..6fe2be6608
> --- /dev/null
> +++ b/hw/misc/rp2040_pll.c
> @@ -0,0 +1,234 @@
<snip>
> +
> +#define ATOMIC_ALIAS_MASK   0x3000
> +#define ATOMIC_XOR          0x1000
> +#define ATOMIC_SET          0x2000
> +#define ATOMIC_CLR          0x3000
> +
> +static uint32_t rp2040_pll_apply_alias(uint32_t old, uint32_t value,
> +                                       hwaddr alias)
> +{
> +    switch (alias) {
> +    case ATOMIC_XOR:
> +        return old ^ value;
> +    case ATOMIC_SET:
> +        return old | value;
> +    case ATOMIC_CLR:
> +        return old & ~value;
> +    default:
> +        return value;
> +    }

This is repeated throughout the code, sometimes inline sometimes not.
This screams for a common inline helper in the headers.

> +}
> +
> +static bool rp2040_pll_locked(RP2040PllState *s)
> +{
> +    return !(s->pwr & (PLL_PWR_PD | PLL_PWR_VCOPD));
> +}
> +
> +static unsigned rp2040_pll_output_hz(RP2040PllState *s)
> +{
> +    uint32_t refdiv = s->cs & PLL_CS_REFDIV_MASK;
> +    uint32_t fbdiv = s->fbdiv_int & PLL_FBDIV_MASK;
> +    uint32_t postdiv1 = extract32(s->prim, 16, 3);
> +    uint32_t postdiv2 = extract32(s->prim, 12, 3);
> +    uint64_t hz;
> +
> +    if (!rp2040_pll_locked(s) || (s->pwr & PLL_PWR_POSTDIVPD)) {
> +        return 0;
> +    }
> +
> +    refdiv = refdiv ? refdiv : 1;
> +    if (s->cs & PLL_CS_BYPASS) {
> +        return XOSC_HZ / refdiv;
> +    }
> +
> +    if (!fbdiv || !postdiv1 || !postdiv2) {
> +        return s->fallback_hz;
> +    }
> +
> +    hz = XOSC_HZ;
> +    hz = hz * fbdiv / refdiv / postdiv1 / postdiv2;
> +    return hz;
> +}
> +
> +static void rp2040_pll_update_clock(RP2040PllState *s)
> +{
> +    clock_update_hz(s->clk, rp2040_pll_output_hz(s));
> +}
> +
> +static uint64_t rp2040_pll_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    RP2040PllState *s = opaque;
> +    hwaddr offset = addr & 0xfff;
> +    const char *name = s->trace_name ? s->trace_name : "rp2040.pll";
> +    uint64_t value;
> +
> +    switch (offset) {
> +    case PLL_CS:
> +        value = s->cs & ~PLL_CS_LOCK;
> +        if (rp2040_pll_locked(s)) {
> +            value |= PLL_CS_LOCK;
> +        }
> +        break;
> +    case PLL_PWR:
> +        value = s->pwr;
> +        break;
> +    case PLL_FBDIV_INT:
> +        value = s->fbdiv_int;
> +        break;
> +    case PLL_PRIM:
> +        value = s->prim;
> +        break;
> +    default:
> +        value = 0;
> +        rp2040_log_unimplemented_read(name, size, s->base + addr, offset,
> +                                      value);
> +        break;
> +    }
> +
> +    return value;
> +}
> +
> +static void rp2040_pll_write(void *opaque, hwaddr addr,
> +                             uint64_t value64, unsigned size)
> +{
> +    RP2040PllState *s = opaque;
> +    hwaddr alias = addr & ATOMIC_ALIAS_MASK;
> +    hwaddr offset = addr & 0xfff;
> +    const char *name = s->trace_name ? s->trace_name : "rp2040.pll";
> +    uint32_t value = value64;
> +
> +    switch (offset) {
> +    case PLL_CS:
> +        s->cs = rp2040_pll_apply_alias(s->cs, value, alias) &
> +                (PLL_CS_BYPASS | PLL_CS_REFDIV_MASK);
> +        break;
> +    case PLL_PWR:
> +        s->pwr = rp2040_pll_apply_alias(s->pwr, value, alias) & PLL_PWR_MASK;
> +        break;
> +    case PLL_FBDIV_INT:
> +        s->fbdiv_int = rp2040_pll_apply_alias(s->fbdiv_int, value, alias) &
> +                       PLL_FBDIV_MASK;
> +        break;
> +    case PLL_PRIM:
> +        s->prim = rp2040_pll_apply_alias(s->prim, value, alias) &
> +                  PLL_PRIM_MASK;
> +        break;
> +    default:
> +        rp2040_log_unimplemented_write(name, size, s->base + addr, offset,
> +                                       value64);
> +        break;
> +    }
> +
> +    rp2040_pll_update_clock(s);
> +}
> +
> +static const MemoryRegionOps rp2040_pll_ops = {
> +    .read = rp2040_pll_read,
> +    .write = rp2040_pll_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
> +};
> +
> +static void rp2040_pll_reset(DeviceState *dev)
> +{
> +    RP2040PllState *s = RP2040_PLL(dev);
> +
> +    s->cs = 1;
> +    s->pwr = PLL_PWR_MASK;
> +    s->fbdiv_int = 0;
> +    s->prim = (7 << 16) | (7 << 12);

#defines for magic numbers please.

> +
> +    rp2040_pll_update_clock(s);
> +}
> +
> +static void rp2040_pll_init(Object *obj)
> +{
> +    RP2040PllState *s = RP2040_PLL(obj);
> +    DeviceState *dev = DEVICE(obj);
> +
> +    s->clk = qdev_init_clock_out(dev, "clk");
> +    memory_region_init_io(&s->iomem, obj, &rp2040_pll_ops, s,
> +                          "rp2040.pll", RP2040_PLL_SIZE);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
> +}
> +
> +static const VMStateDescription rp2040_pll_vmstate = {
> +    .name = TYPE_RP2040_PLL,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_UINT32(cs, RP2040PllState),
> +        VMSTATE_UINT32(pwr, RP2040PllState),
> +        VMSTATE_UINT32(fbdiv_int, RP2040PllState),
> +        VMSTATE_UINT32(prim, RP2040PllState),
> +        VMSTATE_CLOCK(clk, RP2040PllState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static const Property rp2040_pll_properties[] = {
> +    DEFINE_PROP_STRING("trace-name", RP2040PllState, trace_name),
> +    DEFINE_PROP_UINT32("base", RP2040PllState, base, 0),
> +    DEFINE_PROP_UINT32("fallback-hz", RP2040PllState, fallback_hz, 0),
> +};
> +
> +static void rp2040_pll_class_init(ObjectClass *klass, const void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    device_class_set_legacy_reset(dc, rp2040_pll_reset);
> +    device_class_set_props(dc, rp2040_pll_properties);
> +    dc->vmsd = &rp2040_pll_vmstate;
> +}
> +
> +static const TypeInfo rp2040_pll_info = {
> +    .name          = TYPE_RP2040_PLL,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(RP2040PllState),
> +    .instance_init = rp2040_pll_init,
> +    .class_init    = rp2040_pll_class_init,
> +};
> +
> +static void rp2040_pll_register_types(void)
> +{
> +    type_register_static(&rp2040_pll_info);
> +}
> +type_init(rp2040_pll_register_types)
> diff --git a/include/hw/arm/rp2040.h b/include/hw/arm/rp2040.h
> index 47a8cb2236..abdd2f006c 100644
> --- a/include/hw/arm/rp2040.h
> +++ b/include/hw/arm/rp2040.h
> @@ -13,6 +13,8 @@
>  #include "hw/char/pl011.h"
>  #include "hw/core/clock.h"
>  #include "hw/core/sysbus.h"
> +#include "hw/misc/rp2040_clocks.h"
> +#include "hw/misc/rp2040_pll.h"
>  #include "hw/misc/rp2040_rosc.h"
>  #include "hw/misc/rp2040_syscfg.h"
>  #include "hw/misc/rp2040_sysinfo.h"
> @@ -40,6 +42,9 @@ struct RP2040State {
>  
>      ARMv7MState armv7m;
>      PL011State uart[2];
> +    RP2040ClocksState clocks;
> +    RP2040PllState pll_sys;
> +    RP2040PllState pll_usb;
>      RP2040SysCfgState syscfg;
>      RP2040SysInfoState sysinfo;
>      RP2040RoscState rosc;
> diff --git a/include/hw/misc/rp2040_clocks.h b/include/hw/misc/rp2040_clocks.h
> new file mode 100644
> index 0000000000..ebbb14e3fa
> --- /dev/null
> +++ b/include/hw/misc/rp2040_clocks.h
> @@ -0,0 +1,36 @@
> +/*
> + * RP2040 clocks emulation
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef HW_MISC_RP2040_CLOCKS_H
> +#define HW_MISC_RP2040_CLOCKS_H
> +
> +#include "hw/core/clock.h"
> +#include "hw/core/sysbus.h"
> +#include "qom/object.h"
> +
> +#define TYPE_RP2040_CLOCKS "rp2040-clocks"
> +OBJECT_DECLARE_SIMPLE_TYPE(RP2040ClocksState, RP2040_CLOCKS)
> +
> +#define RP2040_CLOCKS_BASE 0x40008000
> +#define RP2040_CLOCKS_SIZE 0x4000
> +
> +struct RP2040ClocksState {
> +    SysBusDevice parent_obj;
> +
> +    MemoryRegion iomem;
> +    Clock *clk_ref;
> +    Clock *clk_sys;
> +    Clock *clk_peri;
> +    Clock *clk_usb;
> +    Clock *clk_adc;
> +    Clock *clk_rtc;
> +    Clock *pll_sys;
> +    Clock *pll_usb;
> +
> +    uint32_t regs[0x100 / 4];
> +};
> +
> +#endif
> diff --git a/include/hw/misc/rp2040_pll.h b/include/hw/misc/rp2040_pll.h
> new file mode 100644
> index 0000000000..ad188aa46e
> --- /dev/null
> +++ b/include/hw/misc/rp2040_pll.h
> @@ -0,0 +1,37 @@
> +/*
> + * RP2040 PLL emulation
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef HW_MISC_RP2040_PLL_H
> +#define HW_MISC_RP2040_PLL_H
> +
> +#include "hw/core/clock.h"
> +#include "hw/core/sysbus.h"
> +#include "qom/object.h"
> +
> +#define TYPE_RP2040_PLL "rp2040-pll"
> +OBJECT_DECLARE_SIMPLE_TYPE(RP2040PllState, RP2040_PLL)
> +
> +#define RP2040_PLL_SYS_BASE 0x40028000
> +#define RP2040_PLL_USB_BASE 0x4002c000
> +#define RP2040_PLL_SIZE     0x4000
> +
> +struct RP2040PllState {
> +    SysBusDevice parent_obj;
> +
> +    MemoryRegion iomem;
> +    Clock *clk;
> +
> +    char *trace_name;
> +    uint32_t base;
> +    uint32_t fallback_hz;
> +
> +    uint32_t cs;
> +    uint32_t pwr;
> +    uint32_t fbdiv_int;
> +    uint32_t prim;
> +};
> +
> +#endif
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
> index 441f6f294a..416d90ae94 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -256,6 +256,7 @@ qtests_arm = \
>    (config_all_devices.has_key('CONFIG_VEXPRESS') ? ['test-arm-mptimer'] : []) + \
>    (config_all_devices.has_key('CONFIG_MICROBIT') ? ['microbit-test'] : []) + \
>    (config_all_devices.has_key('CONFIG_RASPI_PICO') ? ['rp2040-sysinfo-syscfg-test',
> +                                                       'rp2040-clocks-test',
>                                                         'rp2040-rosc-test',
>                                                         'rp2040-tbman-test',
>                                                         'rp2040-vreg-test'] : []) + \
> diff --git a/tests/qtest/rp2040-clocks-test.c b/tests/qtest/rp2040-clocks-test.c
> new file mode 100644
> index 0000000000..39e49eb57f
> --- /dev/null
> +++ b/tests/qtest/rp2040-clocks-test.c
> @@ -0,0 +1,107 @@
> +/*
> + * QTest testcase for the RP2040 clocks block.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +#include "qemu/bitops.h"
> +
> +#define CLOCKS_BASE             0x40008000
> +#define CLK_SYS_CTRL            0x3c
> +#define CLK_SYS_DIV             0x40
> +#define CLK_PERI_CTRL           0x48
> +#define CLK_PERI_DIV            0x4c
> +#define CLK_USB_CTRL            0x54
> +#define CLK_USB_DIV             0x58
> +#define CLK_ADC_CTRL            0x60
> +#define CLK_ADC_DIV             0x64
> +#define CLK_RTC_CTRL            0x6c
> +#define CLK_RTC_DIV             0x70
> +#define FC0_SRC                 0x94
> +#define FC0_STATUS              0x98
> +#define FC0_RESULT              0x9c
> +
> +#define PLL_SYS_BASE            0x40028000
> +#define PLL_USB_BASE            0x4002c000
> +#define PLL_CS                  0x00
> +#define PLL_PWR                 0x04
> +#define PLL_FBDIV_INT           0x08
> +#define PLL_PRIM                0x0c
> +
> +#define PLL_PWR_BITS            (BIT(5) | BIT(3) | BIT(2) | BIT(0))
> +#define CLK_CTRL_ENABLE         BIT(11)
> +#define FC0_STATUS_DONE         BIT(4)

Again you can re-use include definitions.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro