Re: [PATCH 4/8] tiva c sysctl implementation

[Peter Maydell]

On Wed, 17 May 2023 at 09:13, Mohamed ElSayed <m.elsayed4420@gmail.com> wrote:
>
> Signed-off-by: Mohamed ElSayed <m.elsayed4420@gmail.com>
> ---
>  hw/misc/tm4c123_sysctl.c         | 989 +++++++++++++++++++++++++++++++
>  hw/misc/trace-events             |   5 +
>  include/hw/misc/tm4c123_sysctl.h | 307 ++++++++++
>  3 files changed, 1301 insertions(+)
>  create mode 100644 hw/misc/tm4c123_sysctl.c
>  create mode 100644 include/hw/misc/tm4c123_sysctl.h
>
> diff --git a/hw/misc/tm4c123_sysctl.c b/hw/misc/tm4c123_sysctl.c
> new file mode 100644
> index 0000000000..c996609fc7
> --- /dev/null
> +++ b/hw/misc/tm4c123_sysctl.c
> @@ -0,0 +1,989 @@
> +/*
> + * TM4C123 SYSCTL
> + *
> + * Copyright (c) 2023 Mohamed ElSayed <m.elsayed4420@gmail.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a 
> copy
> + * of this software and associated documentation files (the "Software"), to 
> deal
> + * in the Software without restriction, including without limitation the 
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/misc/tm4c123_sysctl.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "trace.h"
> +
> +#define LOG(mask, fmt, args...) qemu_log_mask(mask, "%s: " fmt, __func__, ## 
> args)
> +#define READONLY LOG(LOG_GUEST_ERROR, "0x%"HWADDR_PRIx" is a readonly 
> field\n.", addr)

See the remarks in an earlier patch about these macros.

> +
> +static void tm4c123_sysctl_update_system_clock(void *opaque)
> +{
> +    TM4C123SysCtlState *s = opaque;
> +
> +    uint32_t RCC_Val = s->sysctl_rcc;
> +    uint32_t RCC2_Val = s->sysctl_rcc2;
> +
> +    uint32_t __CORE_CLK_PRE;
> +    uint32_t __CORE_CLK;

Please don't use double-underscore prefixes or all-caps for
variables.

> +
> +    if (RCC2_Val & (1UL << 31)) {  /* is rcc2 used? */
> +        if (RCC2_Val & (1UL << 11)) {  /* check BYPASS */
> +            if (((RCC2_Val >> 4) & 0x07) == 0x0) {
> +                if (((RCC_Val >> 6) & 0x1F) == 0x0) {
> +                    __CORE_CLK_PRE = 1000000UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0x1) {
> +                    __CORE_CLK_PRE = 1843200UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0x2) {
> +                    __CORE_CLK_PRE = 2000000UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0x3) {
> +                    __CORE_CLK_PRE = 2457600UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0x4) {
> +                    __CORE_CLK_PRE = 3579545UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0x5) {
> +                    __CORE_CLK_PRE = 3686400UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0x6) {
> +                    __CORE_CLK_PRE = 4000000UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0x7) {
> +                    __CORE_CLK_PRE = 4096000UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0x8) {
> +                    __CORE_CLK_PRE = 4915200UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0x9) {
> +                    __CORE_CLK_PRE = 5000000UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0xA) {
> +                    __CORE_CLK_PRE = 5120000UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0xB) {
> +                    __CORE_CLK_PRE = 6000000UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0xC) {
> +                    __CORE_CLK_PRE = 6144000UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0xD) {
> +                    __CORE_CLK_PRE = 7372800UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0xE) {
> +                    __CORE_CLK_PRE = 8000000UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0xF) {
> +                    __CORE_CLK_PRE = 8192000UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0x10) {
> +                    __CORE_CLK_PRE = 10000000UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0x11) {
> +                    __CORE_CLK_PRE = 12000000UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0x12) {
> +                    __CORE_CLK_PRE = 12288000UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0x13) {
> +                    __CORE_CLK_PRE = 13560000UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0x14) {
> +                    __CORE_CLK_PRE = 14318180UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0x15) {
> +                    __CORE_CLK_PRE = 16000000UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0x16) {
> +                    __CORE_CLK_PRE = 16384000UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0x17) {
> +                    __CORE_CLK_PRE = 18000000UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0x18) {
> +                    __CORE_CLK_PRE = 20000000UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0x19) {
> +                    __CORE_CLK_PRE = 24000000UL;
> +                } else if (((RCC_Val >> 6) & 0x1F) == 0x1A) {
> +                    __CORE_CLK_PRE = 25000000UL;
> +                } else {
> +                    __CORE_CLK_PRE = 0UL;
> +                }
> +                __CORE_CLK = __CORE_CLK_PRE / 2UL;  /* divide by 2 since 
> BYPASS is set */
> +            } else {  /* PLL is used */
> +                uint32_t __PLL_MULT = ((RCC2_Val >> 4) & 0x1F) + 2;
> +                uint32_t __PLL_DIV = ((RCC2_Val >> 0) & 0x3F) + 1;
> +                uint32_t __PLL_SOURCE = ((RCC2_Val >> 13) & 0x01);
> +                if (__PLL_SOURCE == 0) {  /* source is XTAL */
> +                    __CORE_CLK_PRE = (XTALI * __PLL_MULT) / __PLL_DIV;
> +                } else {  /* source is internal oscillator */
> +                    __CORE_CLK_PRE = (16000000UL * __PLL_MULT) / __PLL_DIV;  
> /* internal oscillator frequency is 16MHz */
> +                }
> +                __CORE_CLK = __CORE_CLK_PRE / 2UL;  /* divide by 2 since 
> BYPASS is set */
> +            }
> +        } else {  /* BYPASS is not set */
> +            uint32_t __SYS_DIV = ((RCC2_Val >> 22) & 0x7F) + 1;
> +            uint32_t __PLL_MULT = ((RCC2_Val >> 4) & 0x1F) + 2;
> +            uint32_t __PLL_DIV = ((RCC2_Val >> 0) & 0x3F) + 1;
> +            uint32_t __PLL_SOURCE = ((RCC2_Val >> 13) & 0x01);
> +            if (__PLL_SOURCE == 0) {  /* source is XTAL */
> +                __CORE_CLK_PRE = (XTALI * __PLL_MULT) / __PLL_DIV;
> +            } else {  /* source is internal oscillator */
> +                __CORE_CLK_PRE = (16000000UL * __PLL_MULT) / __PLL_DIV;  /* 
> internal oscillator frequency is 16MHz */
> +            }
> +            __CORE_CLK = __CORE_CLK_PRE / __SYS_DIV;
> +        }
> +    } else {  /* rcc2 is not used */
> +        if (((RCC_Val >> 16) & 0x01) == 0x01) {  /* check USESYSCLK */
> +            if (((RCC_Val >> 23) & 0x01) == 0x01) {  /* check BYPASS */
> +                __CORE_CLK_PRE = XTALI;
> +            } else {  /* PLL is used */
> +                uint32_t __PLL_MULT = ((RCC_Val >> 18) & 0x1F) + 2;
> +                uint32_t __PLL_DIV = ((RCC_Val >> 12) & 0x3F) + 1;
> +                uint32_t __PLL_SOURCE = ((RCC_Val >> 16) & 0x01);
> +                if (__PLL_SOURCE == 0) {  /* source is XTAL */
> +                    __CORE_CLK_PRE = (XTALI * __PLL_MULT) / __PLL_DIV;
> +                } else {  /* source is internal oscillator */
> +                    __CORE_CLK_PRE = (16000000UL * __PLL_MULT) / __PLL_DIV;  
> /* internal oscillator frequency is 16MHz */
> +                }
> +            }
> +        } else {  /* USESYSCLK bit is not set */
> +            __CORE_CLK_PRE = 16000000UL;  /* default to internal oscillator 
> frequency */
> +        }
> +        __CORE_CLK = __CORE_CLK_PRE / 1UL;  /* no division needed since 
> BYPASS is not set */
> +    }
> +    trace_tm4c123_sysctl_update_system_clock(__CORE_CLK);
> +    clock_update_hz(s->mainclk, __CORE_CLK);
> +}

> +static void tm4c123_sysctl_write(void *opaque, hwaddr addr, uint64_t val64, 
> unsigned int size)
> +{
> +    TM4C123SysCtlState *s = opaque;
> +    uint32_t val32 = val64;
> +
> +    trace_tm4c123_sysctl_write(addr, val32);
> +
> +    switch (addr) {
> +        case SYSCTL_DID0:
> +            READONLY;
> +            break;
> +        case SYSCTL_DID1:
> +            READONLY;
> +            break;
> +        case SYSCTL_PBORCTL:
> +            s->sysctl_pborctl = val32;
> +            break;
> +        case SYSCTL_RIS:
> +            READONLY;
> +            break;
> +        case SYSCTL_IMC:
> +            s->sysctl_imc = val32;
> +            /*
> +             * setting the MISC
> +             */
> +            s->sysctl_misc = val32;
> +            break;

What's this for? The spec does not say anything about the MISC
register being written by the IMC. In fact the MISC is just
the masked status of the current interrupts, so it shouldn't
have a status field at all. Reading the MISC register ought
to return s->sysctl_ris & s->sysctl_imc.

> +        case SYSCTL_MISC:
> +            s->sysctl_misc = val32;

Writing to MISC should change bits in RIS on a write-on-to-clear
basis, so this isn't right. It should be
   s->sysctl_ris &= ~val32; /* W1C */

> +            break;

> +        case SYSCTL_RESC:
> +            s->sysctl_resc = val32;
> +            break;
> +        case SYSCTL_RCC:
> +            s->sysctl_rcc = val32;
> +            /*
> +             * Setting the SYSCTL_RIS manually for now.
> +             */
> +            if (s->sysctl_rcc & SYSCTL_RCC_PWRDN && !(s->sysctl_rcc2 & 
> SYSCTL_RCC2_USERCC2)) {
> +                s->sysctl_ris |= SYSCTL_RIS_PLLRIS;
> +            }

I don't entirely understand the comment here. My guess is that we're
opting for "report the PLL as locked immediately rather than
emulating the real hardware's timed delay before reporting it"
(which is fine, though I think the comment could be clearer
about what it's doing).

However should we really be testing PWRDN == 1 here ? That means
"PLL powered down", not "powered up"; it doesn't match the logic
you have below for the similar RCC2 bits either.

> +            tm4c123_sysctl_update_system_clock(s);
> +            break;
> +        case SYSCTL_GPIOHBCTL:
> +            s->sysctl_gpiohbctl = val32;
> +            break;
> +        case SYSCTL_RCC2:
> +            s->sysctl_rcc2 = val32;
> +            /*
> +             * Setting the SYSCTL_RIS manually for now.
> +             */
> +            if (s->sysctl_rcc2 & SYSCTL_RCC2_USERCC2 && !(s->sysctl_rcc2 & 
> SYSCTL_RCC2_PWRDN2)) {
> +                s->sysctl_ris |= SYSCTL_RIS_PLLRIS;
> +            }
> +            tm4c123_sysctl_update_system_clock(s);

Because the logic for "do we set the PLLRIS bit?" depends
on both the rcc and rcc2 values, you need to check everything
in both places. Otherwise if the guest writes RCC.PWRDN to
0 first and RCC2.USERCC2 to 0 second then you won't notice.
So I think you should move this logic to the
tm4c123_sysctl_update_system_clock() function.

> +            break;

> +
> +static void tm4c123_sysctl_class_init(ObjectClass *kclass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(kclass);
> +    dc->reset = tm4c123_sysctl_reset;
> +    dc->realize = tm4c123_sysctl_realize;

You also need to set dc->vmsd. (Every device with internal
state needs to set up a VMStateDescription, which describes
that internal state for the purposes of migration and
for VM state save/restore.)

thanks
-- PMM