Re: [Qemu-devel] [PATCH v4 17/28] riscv: sifive: Implement PRCI model for FU540

[Alistair Francis <alistair23@gmail.com>]

 On Sun, Aug 18, 2019 at 10:30 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> This adds a simple PRCI model for FU540 (sifive_u). It has different
> register layout from the existing PRCI model for FE310 (sifive_e).
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>
> ---
>
> Changes in v4:
> - prefix all macros/variables/functions with SIFIVE_U/sifive_u
>   in the sifive_u_prci driver
>
> Changes in v3: None
> Changes in v2: None
>
>  hw/riscv/Makefile.objs           |   1 +
>  hw/riscv/sifive_u_prci.c         | 165 +++++++++++++++++++++++++++++++++++++++
>  include/hw/riscv/sifive_u_prci.h |  90 +++++++++++++++++++++
>  3 files changed, 256 insertions(+)
>  create mode 100644 hw/riscv/sifive_u_prci.c
>  create mode 100644 include/hw/riscv/sifive_u_prci.h
>
> diff --git a/hw/riscv/Makefile.objs b/hw/riscv/Makefile.objs
> index c859697..b95bbd5 100644
> --- a/hw/riscv/Makefile.objs
> +++ b/hw/riscv/Makefile.objs
> @@ -8,6 +8,7 @@ obj-$(CONFIG_SIFIVE) += sifive_gpio.o
>  obj-$(CONFIG_SIFIVE) += sifive_plic.o
>  obj-$(CONFIG_SIFIVE) += sifive_test.o
>  obj-$(CONFIG_SIFIVE_U) += sifive_u.o
> +obj-$(CONFIG_SIFIVE_U) += sifive_u_prci.o
>  obj-$(CONFIG_SIFIVE) += sifive_uart.o
>  obj-$(CONFIG_SPIKE) += spike.o
>  obj-$(CONFIG_RISCV_VIRT) += virt.o
> diff --git a/hw/riscv/sifive_u_prci.c b/hw/riscv/sifive_u_prci.c
> new file mode 100644
> index 0000000..578c643
> --- /dev/null
> +++ b/hw/riscv/sifive_u_prci.c
> @@ -0,0 +1,165 @@
> +/*
> + * QEMU SiFive U PRCI (Power, Reset, Clock, Interrupt)
> + *
> + * Copyright (c) 2019 Bin Meng <bmeng.cn@gmail.com>
> + *
> + * Simple model of the PRCI to emulate register reads made by the SDK BSP
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2 or later, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/sysbus.h"
> +#include "qemu/module.h"
> +#include "target/riscv/cpu.h"
> +#include "hw/riscv/sifive_u_prci.h"
> +
> +static uint64_t sifive_u_prci_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> +    SiFiveUPRCIState *s = opaque;
> +
> +    switch (addr) {
> +    case SIFIVE_U_PRCI_HFXOSCCFG:
> +        return s->hfxosccfg;
> +    case SIFIVE_U_PRCI_COREPLLCFG0:
> +        return s->corepllcfg0;
> +    case SIFIVE_U_PRCI_DDRPLLCFG0:
> +        return s->ddrpllcfg0;
> +    case SIFIVE_U_PRCI_DDRPLLCFG1:
> +        return s->ddrpllcfg1;
> +    case SIFIVE_U_PRCI_GEMGXLPLLCFG0:
> +        return s->gemgxlpllcfg0;
> +    case SIFIVE_U_PRCI_GEMGXLPLLCFG1:
> +        return s->gemgxlpllcfg1;
> +    case SIFIVE_U_PRCI_CORECLKSEL:
> +        return s->coreclksel;
> +    case SIFIVE_U_PRCI_DEVICESRESET:
> +        return s->devicesreset;
> +    case SIFIVE_U_PRCI_CLKMUXSTATUS:
> +        return s->clkmuxstatus;
> +    }
> +
> +    hw_error("%s: read: addr=0x%x\n", __func__, (int)addr);

This should be: qemu_log_mask(LOG_GUEST_ERROR,... instead.

> +    return 0;
> +}
> +
> +static void sifive_u_prci_write(void *opaque, hwaddr addr,
> +                                uint64_t val64, unsigned int size)
> +{
> +    SiFiveUPRCIState *s = opaque;
> +
> +    switch (addr) {
> +    case SIFIVE_U_PRCI_HFXOSCCFG:
> +        s->hfxosccfg = (uint32_t) val64;
> +        /* OSC stays ready */
> +        s->hfxosccfg |= SIFIVE_U_PRCI_HFXOSCCFG_RDY;
> +        break;
> +    case SIFIVE_U_PRCI_COREPLLCFG0:
> +        s->corepllcfg0 = (uint32_t) val64;
> +        /* internal feedback */
> +        s->corepllcfg0 |= SIFIVE_U_PRCI_PLLCFG0_FSE;
> +        /* PLL stays locked */
> +        s->corepllcfg0 |= SIFIVE_U_PRCI_PLLCFG0_LOCK;
> +        break;
> +    case SIFIVE_U_PRCI_DDRPLLCFG0:
> +        s->ddrpllcfg0 = (uint32_t) val64;
> +        /* internal feedback */
> +        s->ddrpllcfg0 |= SIFIVE_U_PRCI_PLLCFG0_FSE;
> +        /* PLL stays locked */
> +        s->ddrpllcfg0 |= SIFIVE_U_PRCI_PLLCFG0_LOCK;
> +        break;
> +    case SIFIVE_U_PRCI_DDRPLLCFG1:
> +        s->ddrpllcfg1 = (uint32_t) val64;
> +        break;
> +    case SIFIVE_U_PRCI_GEMGXLPLLCFG0:
> +        s->gemgxlpllcfg0 = (uint32_t) val64;
> +         /* internal feedback */
> +        s->gemgxlpllcfg0 |= SIFIVE_U_PRCI_PLLCFG0_FSE;
> +       /* PLL stays locked */
> +        s->gemgxlpllcfg0 |= SIFIVE_U_PRCI_PLLCFG0_LOCK;
> +        break;
> +    case SIFIVE_U_PRCI_GEMGXLPLLCFG1:
> +        s->gemgxlpllcfg1 = (uint32_t) val64;
> +        break;
> +    case SIFIVE_U_PRCI_CORECLKSEL:
> +        s->coreclksel = (uint32_t) val64;
> +        break;
> +    case SIFIVE_U_PRCI_DEVICESRESET:
> +        s->devicesreset = (uint32_t) val64;
> +        break;
> +    case SIFIVE_U_PRCI_CLKMUXSTATUS:
> +        s->clkmuxstatus = (uint32_t) val64;

Maybe it's worth creating a 32-bit val variable and using that instead
of casting everywhere.

> +        break;
> +    default:
> +        hw_error("%s: bad write: addr=0x%x v=0x%x\n",
> +                 __func__, (int)addr, (int)val64);

Same comment about: qemu_log_mask(LOG_GUEST_ERROR,

> +    }
> +}
> +
> +static const MemoryRegionOps sifive_u_prci_ops = {
> +    .read = sifive_u_prci_read,
> +    .write = sifive_u_prci_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4
> +    }
> +};
> +
> +static void sifive_u_prci_init(Object *obj)
> +{
> +    SiFiveUPRCIState *s = SIFIVE_U_PRCI(obj);
> +
> +    memory_region_init_io(&s->mmio, obj, &sifive_u_prci_ops, s,
> +                          TYPE_SIFIVE_U_PRCI, SIFIVE_U_PRCI_REG_SIZE);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
> +
> +    /* Initialize register to power-on-reset values */
> +    s->hfxosccfg = (SIFIVE_U_PRCI_HFXOSCCFG_RDY | SIFIVE_U_PRCI_HFXOSCCFG_EN);
> +    s->corepllcfg0 = (SIFIVE_U_PRCI_PLLCFG0_DIVR | SIFIVE_U_PRCI_PLLCFG0_DIVF |
> +                      SIFIVE_U_PRCI_PLLCFG0_DIVQ | SIFIVE_U_PRCI_PLLCFG0_FSE |
> +                      SIFIVE_U_PRCI_PLLCFG0_LOCK);
> +    s->ddrpllcfg0 = (SIFIVE_U_PRCI_PLLCFG0_DIVR | SIFIVE_U_PRCI_PLLCFG0_DIVF |
> +                     SIFIVE_U_PRCI_PLLCFG0_DIVQ | SIFIVE_U_PRCI_PLLCFG0_FSE |
> +                     SIFIVE_U_PRCI_PLLCFG0_LOCK);
> +    s->gemgxlpllcfg0 = (SIFIVE_U_PRCI_PLLCFG0_DIVR |
> +                        SIFIVE_U_PRCI_PLLCFG0_DIVF |
> +                        SIFIVE_U_PRCI_PLLCFG0_DIVQ |
> +                        SIFIVE_U_PRCI_PLLCFG0_FSE |
> +                        SIFIVE_U_PRCI_PLLCFG0_LOCK);
> +    s->coreclksel = SIFIVE_U_PRCI_CORECLKSEL_HFCLK;

These should be in the reset functino shouldn't they?

> +}
> +
> +static const TypeInfo sifive_u_prci_info = {
> +    .name          = TYPE_SIFIVE_U_PRCI,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(SiFiveUPRCIState),
> +    .instance_init = sifive_u_prci_init,
> +};
> +
> +static void sifive_u_prci_register_types(void)
> +{
> +    type_register_static(&sifive_u_prci_info);
> +}
> +
> +type_init(sifive_u_prci_register_types)
> +
> +
> +/* Create PRCI device */
> +DeviceState *sifive_u_prci_create(hwaddr addr)
> +{
> +    DeviceState *dev = qdev_create(NULL, TYPE_SIFIVE_U_PRCI);
> +    qdev_init_nofail(dev);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, addr);
> +    return dev;
> +}

Don't do this, just create it in the machine.

> diff --git a/include/hw/riscv/sifive_u_prci.h b/include/hw/riscv/sifive_u_prci.h
> new file mode 100644
> index 0000000..66eacb5
> --- /dev/null
> +++ b/include/hw/riscv/sifive_u_prci.h
> @@ -0,0 +1,90 @@
> +/*
> + * QEMU SiFive U PRCI (Power, Reset, Clock, Interrupt) interface
> + *
> + * Copyright (c) 2019 Bin Meng <bmeng.cn@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2 or later, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef HW_SIFIVE_U_PRCI_H
> +#define HW_SIFIVE_U_PRCI_H
> +
> +enum {
> +    SIFIVE_U_PRCI_HFXOSCCFG     = 0x00,
> +    SIFIVE_U_PRCI_COREPLLCFG0   = 0x04,
> +    SIFIVE_U_PRCI_DDRPLLCFG0    = 0x0C,
> +    SIFIVE_U_PRCI_DDRPLLCFG1    = 0x10,
> +    SIFIVE_U_PRCI_GEMGXLPLLCFG0 = 0x1C,
> +    SIFIVE_U_PRCI_GEMGXLPLLCFG1 = 0x20,
> +    SIFIVE_U_PRCI_CORECLKSEL    = 0x24,
> +    SIFIVE_U_PRCI_DEVICESRESET  = 0x28,
> +    SIFIVE_U_PRCI_CLKMUXSTATUS  = 0x2C
> +};
> +
> +/*
> + * Current FU540-C000 manual says ready bit is at bit 29, but
> + * freedom-u540-c000-bootloader codes (ux00prci.h) says it is at bit 31.
> + * We have to trust the actual codes that worked.

s/codes that worked/code that works/g

> + *
> + * see https://github.com/sifive/freedom-u540-c000-bootloader
> + */
> +enum {
> +    SIFIVE_U_PRCI_HFXOSCCFG_EN  = (1 << 30),
> +    SIFIVE_U_PRCI_HFXOSCCFG_RDY = (1 << 31),
> +};
> +
> +/* xxxPLLCFG0 register bits */
> +enum {
> +    SIFIVE_U_PRCI_PLLCFG0_DIVR  = (1 << 0),
> +    SIFIVE_U_PRCI_PLLCFG0_DIVF  = (31 << 6),
> +    SIFIVE_U_PRCI_PLLCFG0_DIVQ  = (3 << 15),
> +    SIFIVE_U_PRCI_PLLCFG0_FSE   = (1 << 25),
> +    SIFIVE_U_PRCI_PLLCFG0_LOCK  = (1 << 31)
> +};
> +
> +/* xxxPLLCFG1 register bits */
> +enum {
> +    SIFIVE_U_PRCI_PLLCFG1_CKE   = (1 << 24)
> +};
> +
> +enum {
> +    SIFIVE_U_PRCI_CORECLKSEL_HFCLK = (1 << 0)
> +};

Why use enums for eveythign instead of #defines?

Alistair

> +
> +#define SIFIVE_U_PRCI_REG_SIZE  0x1000
> +
> +#define TYPE_SIFIVE_U_PRCI      "riscv.sifive.u.prci"
> +
> +#define SIFIVE_U_PRCI(obj) \
> +    OBJECT_CHECK(SiFiveUPRCIState, (obj), TYPE_SIFIVE_U_PRCI)
> +
> +typedef struct SiFiveUPRCIState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +
> +    /*< public >*/
> +    MemoryRegion mmio;
> +    uint32_t hfxosccfg;
> +    uint32_t corepllcfg0;
> +    uint32_t ddrpllcfg0;
> +    uint32_t ddrpllcfg1;
> +    uint32_t gemgxlpllcfg0;
> +    uint32_t gemgxlpllcfg1;
> +    uint32_t coreclksel;
> +    uint32_t devicesreset;
> +    uint32_t clkmuxstatus;
> +} SiFiveUPRCIState;
> +
> +DeviceState *sifive_u_prci_create(hwaddr addr);
> +
> +#endif /* HW_SIFIVE_U_PRCI_H */
> --
> 2.7.4
>
>