diff mbox series

[v8,02/14] hw/misc: Add NPCM7xx Clock Controller device model

Message ID 20200825001711.1340443-3-hskinnemoen@google.com (mailing list archive)
State New, archived
Headers show
Series Add Nuvoton NPCM730/NPCM750 SoCs and two BMC machines | expand

Commit Message

Zhijian Li (Fujitsu)" via Aug. 25, 2020, 12:16 a.m. UTC
Enough functionality to boot the Linux kernel has been implemented. This
includes:

  - Correct power-on reset values so the various clock rates can be
    accurately calculated.
  - Clock enables stick around when written.

In addition, a best effort attempt to implement SECCNT and CNTR25M was
made even though I don't think the kernel needs them.

Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
Reviewed-by: Joel Stanley <joel@jms.id.au>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
---
 include/hw/misc/npcm7xx_clk.h |  48 ++++++
 hw/misc/npcm7xx_clk.c         | 266 ++++++++++++++++++++++++++++++++++
 hw/misc/meson.build           |   1 +
 hw/misc/trace-events          |   4 +
 4 files changed, 319 insertions(+)
 create mode 100644 include/hw/misc/npcm7xx_clk.h
 create mode 100644 hw/misc/npcm7xx_clk.c

Comments

Philippe Mathieu-Daudé Sept. 4, 2020, 9:32 a.m. UTC | #1
On 8/25/20 2:16 AM, Havard Skinnemoen via wrote:
> Enough functionality to boot the Linux kernel has been implemented. This
> includes:
> 
>   - Correct power-on reset values so the various clock rates can be
>     accurately calculated.
>   - Clock enables stick around when written.
> 
> In addition, a best effort attempt to implement SECCNT and CNTR25M was
> made even though I don't think the kernel needs them.
> 
> Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
> ---
>  include/hw/misc/npcm7xx_clk.h |  48 ++++++
>  hw/misc/npcm7xx_clk.c         | 266 ++++++++++++++++++++++++++++++++++
>  hw/misc/meson.build           |   1 +
>  hw/misc/trace-events          |   4 +
>  4 files changed, 319 insertions(+)
>  create mode 100644 include/hw/misc/npcm7xx_clk.h
>  create mode 100644 hw/misc/npcm7xx_clk.c
> 
> diff --git a/include/hw/misc/npcm7xx_clk.h b/include/hw/misc/npcm7xx_clk.h
> new file mode 100644
> index 0000000000..cdcc9e8534
> --- /dev/null
> +++ b/include/hw/misc/npcm7xx_clk.h
> @@ -0,0 +1,48 @@
> +/*
> + * Nuvoton NPCM7xx Clock Control Registers.
> + *
> + * Copyright 2020 Google LLC
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that 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.
> + */
> +#ifndef NPCM7XX_CLK_H
> +#define NPCM7XX_CLK_H
> +
> +#include "exec/memory.h"
> +#include "hw/sysbus.h"
> +
> +/*
> + * The reference clock frequency for the timer modules, and the SECCNT and
> + * CNTR25M registers in this module, is always 25 MHz.
> + */
> +#define NPCM7XX_TIMER_REF_HZ            (25000000)
> +
> +/*
> + * Number of registers in our device state structure. Don't change this without
> + * incrementing the version_id in the vmstate.
> + */
> +#define NPCM7XX_CLK_NR_REGS             (0x70 / sizeof(uint32_t))
> +
> +typedef struct NPCM7xxCLKState {
> +    SysBusDevice parent;
> +
> +    MemoryRegion iomem;
> +
> +    uint32_t regs[NPCM7XX_CLK_NR_REGS];
> +
> +    /* Time reference for SECCNT and CNTR25M, initialized by power on reset */
> +    int64_t ref_ns;
> +} NPCM7xxCLKState;
> +
> +#define TYPE_NPCM7XX_CLK "npcm7xx-clk"
> +#define NPCM7XX_CLK(obj) OBJECT_CHECK(NPCM7xxCLKState, (obj), TYPE_NPCM7XX_CLK)
> +
> +#endif /* NPCM7XX_CLK_H */
> diff --git a/hw/misc/npcm7xx_clk.c b/hw/misc/npcm7xx_clk.c
> new file mode 100644
> index 0000000000..21ab4200d1
> --- /dev/null
> +++ b/hw/misc/npcm7xx_clk.c
> @@ -0,0 +1,266 @@
> +/*
> + * Nuvoton NPCM7xx Clock Control Registers.
> + *
> + * Copyright 2020 Google LLC
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that 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.
> + */
> +
> +#include "qemu/osdep.h"
> +
> +#include "hw/misc/npcm7xx_clk.h"
> +#include "migration/vmstate.h"
> +#include "qemu/error-report.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "qemu/timer.h"
> +#include "qemu/units.h"
> +#include "trace.h"
> +
> +#define PLLCON_LOKI     BIT(31)
> +#define PLLCON_LOKS     BIT(30)
> +#define PLLCON_PWDEN    BIT(12)
> +
> +enum NPCM7xxCLKRegisters {
> +    NPCM7XX_CLK_CLKEN1,
> +    NPCM7XX_CLK_CLKSEL,
> +    NPCM7XX_CLK_CLKDIV1,
> +    NPCM7XX_CLK_PLLCON0,
> +    NPCM7XX_CLK_PLLCON1,
> +    NPCM7XX_CLK_SWRSTR,
> +    NPCM7XX_CLK_IPSRST1         = 0x20 / sizeof(uint32_t),
> +    NPCM7XX_CLK_IPSRST2,
> +    NPCM7XX_CLK_CLKEN2,
> +    NPCM7XX_CLK_CLKDIV2,
> +    NPCM7XX_CLK_CLKEN3,
> +    NPCM7XX_CLK_IPSRST3,
> +    NPCM7XX_CLK_WD0RCR,
> +    NPCM7XX_CLK_WD1RCR,
> +    NPCM7XX_CLK_WD2RCR,
> +    NPCM7XX_CLK_SWRSTC1,
> +    NPCM7XX_CLK_SWRSTC2,
> +    NPCM7XX_CLK_SWRSTC3,
> +    NPCM7XX_CLK_SWRSTC4,
> +    NPCM7XX_CLK_PLLCON2,
> +    NPCM7XX_CLK_CLKDIV3,
> +    NPCM7XX_CLK_CORSTC,
> +    NPCM7XX_CLK_PLLCONG,
> +    NPCM7XX_CLK_AHBCKFI,
> +    NPCM7XX_CLK_SECCNT,
> +    NPCM7XX_CLK_CNTR25M,
> +    NPCM7XX_CLK_REGS_END,
> +};
> +
> +/*
> + * These reset values were taken from version 0.91 of the NPCM750R data sheet.
> + *
> + * All are loaded on power-up reset. CLKENx and SWRSTR should also be loaded on
> + * core domain reset, but this reset type is not yet supported by QEMU.
> + */
> +static const uint32_t cold_reset_values[NPCM7XX_CLK_NR_REGS] = {
> +    [NPCM7XX_CLK_CLKEN1]        = 0xffffffff,
> +    [NPCM7XX_CLK_CLKSEL]        = 0x004aaaaa,
> +    [NPCM7XX_CLK_CLKDIV1]       = 0x5413f855,
> +    [NPCM7XX_CLK_PLLCON0]       = 0x00222101 | PLLCON_LOKI,
> +    [NPCM7XX_CLK_PLLCON1]       = 0x00202101 | PLLCON_LOKI,
> +    [NPCM7XX_CLK_IPSRST1]       = 0x00001000,
> +    [NPCM7XX_CLK_IPSRST2]       = 0x80000000,
> +    [NPCM7XX_CLK_CLKEN2]        = 0xffffffff,
> +    [NPCM7XX_CLK_CLKDIV2]       = 0xaa4f8f9f,
> +    [NPCM7XX_CLK_CLKEN3]        = 0xffffffff,
> +    [NPCM7XX_CLK_IPSRST3]       = 0x03000000,
> +    [NPCM7XX_CLK_WD0RCR]        = 0xffffffff,
> +    [NPCM7XX_CLK_WD1RCR]        = 0xffffffff,
> +    [NPCM7XX_CLK_WD2RCR]        = 0xffffffff,
> +    [NPCM7XX_CLK_SWRSTC1]       = 0x00000003,
> +    [NPCM7XX_CLK_PLLCON2]       = 0x00c02105 | PLLCON_LOKI,
> +    [NPCM7XX_CLK_CORSTC]        = 0x04000003,
> +    [NPCM7XX_CLK_PLLCONG]       = 0x01228606 | PLLCON_LOKI,
> +    [NPCM7XX_CLK_AHBCKFI]       = 0x000000c8,
> +};
> +
> +static uint64_t npcm7xx_clk_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +    uint32_t reg = offset / sizeof(uint32_t);
> +    NPCM7xxCLKState *s = opaque;
> +    int64_t now_ns;
> +    uint32_t value = 0;
> +
> +    if (reg >= NPCM7XX_CLK_NR_REGS) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: offset 0x%04" HWADDR_PRIx " out of range\n",

I find "out of range" not clear, but this might be the correct
terminology.

> +                      __func__, offset);
> +        return 0;
> +    }
> +
> +    switch (reg) {
> +    case NPCM7XX_CLK_SWRSTR:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: register @ 0x%04" HWADDR_PRIx " is write-only\n",
> +                      __func__, offset);
> +        break;
> +
> +    case NPCM7XX_CLK_SECCNT:
> +        now_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +        value = (now_ns - s->ref_ns) / NANOSECONDS_PER_SECOND;
> +        break;
> +
> +    case NPCM7XX_CLK_CNTR25M:
> +        now_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +        /*
> +         * This register counts 25 MHz cycles, updating every 640 ns. It rolls
> +         * over to zero every second.
> +         *
> +         * The 4 LSBs are always zero: (1e9 / 640) << 4 = 25000000.
> +         */
> +        value = (((now_ns - s->ref_ns) / 640) << 4) % NPCM7XX_TIMER_REF_HZ;
> +        break;
> +
> +    default:
> +        value = s->regs[reg];
> +        break;
> +    };
> +
> +    trace_npcm7xx_clk_read(offset, value);
> +
> +    return value;
> +}
> +
> +static void npcm7xx_clk_write(void *opaque, hwaddr offset,
> +                              uint64_t v, unsigned size)
> +{
> +    uint32_t reg = offset / sizeof(uint32_t);
> +    NPCM7xxCLKState *s = opaque;
> +    uint32_t value = v;
> +
> +    trace_npcm7xx_clk_write(offset, value);
> +
> +    if (reg >= NPCM7XX_CLK_NR_REGS) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: offset 0x%04" HWADDR_PRIx " out of range\n",
> +                      __func__, offset);
> +        return;
> +    }
> +
> +    switch (reg) {
> +    case NPCM7XX_CLK_SWRSTR:
> +        qemu_log_mask(LOG_UNIMP, "%s: SW reset not implemented: 0x%02x\n",
> +                      __func__, value);

Isn't this sufficient?

           qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);

> +        value = 0;
> +        break;
> +
> +    case NPCM7XX_CLK_PLLCON0:
> +    case NPCM7XX_CLK_PLLCON1:
> +    case NPCM7XX_CLK_PLLCON2:
> +    case NPCM7XX_CLK_PLLCONG:
> +        if (value & PLLCON_PWDEN) {
> +            /* Power down -- clear lock and indicate loss of lock */
> +            value &= ~PLLCON_LOKI;
> +            value |= PLLCON_LOKS;
> +        } else {
> +            /* Normal mode -- assume always locked */
> +            value |= PLLCON_LOKI;
> +            /* Keep LOKS unchanged unless cleared by writing 1 */
> +            if (value & PLLCON_LOKS) {
> +                value &= ~PLLCON_LOKS;
> +            } else {
> +                value |= (value & PLLCON_LOKS);
> +            }
> +        }
> +        break;
> +
> +    case NPCM7XX_CLK_CNTR25M:
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: register @ 0x%04" HWADDR_PRIx " is read-only\n",
> +                      __func__, offset);
> +        return;
> +    }
> +
> +    s->regs[reg] = value;
> +}
> +
> +static const struct MemoryRegionOps npcm7xx_clk_ops = {
> +    .read       = npcm7xx_clk_read,
> +    .write      = npcm7xx_clk_write,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +    .valid      = {
> +        .min_access_size        = 4,
> +        .max_access_size        = 4,
> +        .unaligned              = false,
> +    },
> +};
> +
> +static void npcm7xx_clk_enter_reset(Object *obj, ResetType type)
> +{
> +    NPCM7xxCLKState *s = NPCM7XX_CLK(obj);
> +
> +    QEMU_BUILD_BUG_ON(sizeof(s->regs) != sizeof(cold_reset_values));
> +
> +    switch (type) {
> +    case RESET_TYPE_COLD:
> +        memcpy(s->regs, cold_reset_values, sizeof(cold_reset_values));
> +        s->ref_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +        return;
> +    }
> +
> +    /*
> +     * A small number of registers need to be reset on a core domain reset,
> +     * but no such reset type exists yet.
> +     */
> +    qemu_log_mask(LOG_UNIMP, "%s: reset type %d not implemented.",
> +                  __func__, type);

Clean :)

> +}
> +
> +static void npcm7xx_clk_init(Object *obj)
> +{
> +    NPCM7xxCLKState *s = NPCM7XX_CLK(obj);
> +
> +    memory_region_init_io(&s->iomem, obj, &npcm7xx_clk_ops, s,
> +                          TYPE_NPCM7XX_CLK, 4 * KiB);
> +    sysbus_init_mmio(&s->parent, &s->iomem);
> +}
> +
> +static const VMStateDescription vmstate_npcm7xx_clk = {
> +    .name = "npcm7xx-clk",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32_ARRAY(regs, NPCM7xxCLKState, NPCM7XX_CLK_NR_REGS),

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> +        VMSTATE_INT64(ref_ns, NPCM7xxCLKState),
> +        VMSTATE_END_OF_LIST(),
> +    },
> +};
> +
> +static void npcm7xx_clk_class_init(ObjectClass *klass, void *data)
> +{
> +    ResettableClass *rc = RESETTABLE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    QEMU_BUILD_BUG_ON(NPCM7XX_CLK_REGS_END > NPCM7XX_CLK_NR_REGS);
> +
> +    dc->desc = "NPCM7xx Clock Control Registers";
> +    dc->vmsd = &vmstate_npcm7xx_clk;
> +    rc->phases.enter = npcm7xx_clk_enter_reset;
> +}
> +
> +static const TypeInfo npcm7xx_clk_info = {
> +    .name               = TYPE_NPCM7XX_CLK,
> +    .parent             = TYPE_SYS_BUS_DEVICE,
> +    .instance_size      = sizeof(NPCM7xxCLKState),
> +    .instance_init      = npcm7xx_clk_init,
> +    .class_init         = npcm7xx_clk_class_init,
> +};
> +
> +static void npcm7xx_clk_register_type(void)
> +{
> +    type_register_static(&npcm7xx_clk_info);
> +}
> +type_init(npcm7xx_clk_register_type);
> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> index 99ffb9f9e8..531cc31dcc 100644
> --- a/hw/misc/meson.build
> +++ b/hw/misc/meson.build
> @@ -51,6 +51,7 @@ softmmu_ss.add(when: 'CONFIG_IMX', if_true: files(
>  softmmu_ss.add(when: 'CONFIG_MILKYMIST', if_true: files('milkymist-hpdmc.c', 'milkymist-pfpu.c'))
>  softmmu_ss.add(when: 'CONFIG_MAINSTONE', if_true: files('mst_fpga.c'))
>  softmmu_ss.add(when: 'CONFIG_NPCM7XX', if_true: files(
> +  'npcm7xx_clk.c',
>    'npcm7xx_gcr.c',
>  ))
>  softmmu_ss.add(when: 'CONFIG_OMAP', if_true: files(
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index dbf90db8ed..a010699b4f 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -111,6 +111,10 @@ mos6522_set_sr_int(void) "set sr_int"
>  mos6522_write(uint64_t addr, uint64_t val) "reg=0x%"PRIx64 " val=0x%"PRIx64
>  mos6522_read(uint64_t addr, unsigned val) "reg=0x%"PRIx64 " val=0x%x"
>  
> +# npcm7xx_clk.c
> +npcm7xx_clk_read(uint64_t offset, uint32_t value) " offset: 0x%04" PRIx64 " value: 0x%08" PRIx32
> +npcm7xx_clk_write(uint64_t offset, uint32_t value) "offset: 0x%04" PRIx64 " value: 0x%08" PRIx32
> +
>  # npcm7xx_gcr.c
>  npcm7xx_gcr_read(uint64_t offset, uint32_t value) " offset: 0x%04" PRIx64 " value: 0x%08" PRIx32
>  npcm7xx_gcr_write(uint64_t offset, uint32_t value) "offset: 0x%04" PRIx64 " value: 0x%08" PRIx32
>
Havard Skinnemoen Sept. 4, 2020, 10:02 p.m. UTC | #2
On Fri, Sep 4, 2020 at 2:32 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 8/25/20 2:16 AM, Havard Skinnemoen via wrote:
> > Enough functionality to boot the Linux kernel has been implemented. This
> > includes:
> >
> >   - Correct power-on reset values so the various clock rates can be
> >     accurately calculated.
> >   - Clock enables stick around when written.
> >
> > In addition, a best effort attempt to implement SECCNT and CNTR25M was
> > made even though I don't think the kernel needs them.
> >
> > Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
> > Reviewed-by: Joel Stanley <joel@jms.id.au>
> > Reviewed-by: Cédric Le Goater <clg@kaod.org>
> > Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
> > ---
> >  include/hw/misc/npcm7xx_clk.h |  48 ++++++
> >  hw/misc/npcm7xx_clk.c         | 266 ++++++++++++++++++++++++++++++++++
> >  hw/misc/meson.build           |   1 +
> >  hw/misc/trace-events          |   4 +
> >  4 files changed, 319 insertions(+)
> >  create mode 100644 include/hw/misc/npcm7xx_clk.h
> >  create mode 100644 hw/misc/npcm7xx_clk.c
> >
> > diff --git a/include/hw/misc/npcm7xx_clk.h b/include/hw/misc/npcm7xx_clk.h
> > new file mode 100644
> > index 0000000000..cdcc9e8534
> > --- /dev/null
> > +++ b/include/hw/misc/npcm7xx_clk.h
> > @@ -0,0 +1,48 @@
> > +/*
> > + * Nuvoton NPCM7xx Clock Control Registers.
> > + *
> > + * Copyright 2020 Google LLC
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the
> > + * Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that 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.
> > + */
> > +#ifndef NPCM7XX_CLK_H
> > +#define NPCM7XX_CLK_H
> > +
> > +#include "exec/memory.h"
> > +#include "hw/sysbus.h"
> > +
> > +/*
> > + * The reference clock frequency for the timer modules, and the SECCNT and
> > + * CNTR25M registers in this module, is always 25 MHz.
> > + */
> > +#define NPCM7XX_TIMER_REF_HZ            (25000000)
> > +
> > +/*
> > + * Number of registers in our device state structure. Don't change this without
> > + * incrementing the version_id in the vmstate.
> > + */
> > +#define NPCM7XX_CLK_NR_REGS             (0x70 / sizeof(uint32_t))
> > +
> > +typedef struct NPCM7xxCLKState {
> > +    SysBusDevice parent;
> > +
> > +    MemoryRegion iomem;
> > +
> > +    uint32_t regs[NPCM7XX_CLK_NR_REGS];
> > +
> > +    /* Time reference for SECCNT and CNTR25M, initialized by power on reset */
> > +    int64_t ref_ns;
> > +} NPCM7xxCLKState;
> > +
> > +#define TYPE_NPCM7XX_CLK "npcm7xx-clk"
> > +#define NPCM7XX_CLK(obj) OBJECT_CHECK(NPCM7xxCLKState, (obj), TYPE_NPCM7XX_CLK)
> > +
> > +#endif /* NPCM7XX_CLK_H */
> > diff --git a/hw/misc/npcm7xx_clk.c b/hw/misc/npcm7xx_clk.c
> > new file mode 100644
> > index 0000000000..21ab4200d1
> > --- /dev/null
> > +++ b/hw/misc/npcm7xx_clk.c
> > @@ -0,0 +1,266 @@
> > +/*
> > + * Nuvoton NPCM7xx Clock Control Registers.
> > + *
> > + * Copyright 2020 Google LLC
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the
> > + * Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that 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.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +
> > +#include "hw/misc/npcm7xx_clk.h"
> > +#include "migration/vmstate.h"
> > +#include "qemu/error-report.h"
> > +#include "qemu/log.h"
> > +#include "qemu/module.h"
> > +#include "qemu/timer.h"
> > +#include "qemu/units.h"
> > +#include "trace.h"
> > +
> > +#define PLLCON_LOKI     BIT(31)
> > +#define PLLCON_LOKS     BIT(30)
> > +#define PLLCON_PWDEN    BIT(12)
> > +
> > +enum NPCM7xxCLKRegisters {
> > +    NPCM7XX_CLK_CLKEN1,
> > +    NPCM7XX_CLK_CLKSEL,
> > +    NPCM7XX_CLK_CLKDIV1,
> > +    NPCM7XX_CLK_PLLCON0,
> > +    NPCM7XX_CLK_PLLCON1,
> > +    NPCM7XX_CLK_SWRSTR,
> > +    NPCM7XX_CLK_IPSRST1         = 0x20 / sizeof(uint32_t),
> > +    NPCM7XX_CLK_IPSRST2,
> > +    NPCM7XX_CLK_CLKEN2,
> > +    NPCM7XX_CLK_CLKDIV2,
> > +    NPCM7XX_CLK_CLKEN3,
> > +    NPCM7XX_CLK_IPSRST3,
> > +    NPCM7XX_CLK_WD0RCR,
> > +    NPCM7XX_CLK_WD1RCR,
> > +    NPCM7XX_CLK_WD2RCR,
> > +    NPCM7XX_CLK_SWRSTC1,
> > +    NPCM7XX_CLK_SWRSTC2,
> > +    NPCM7XX_CLK_SWRSTC3,
> > +    NPCM7XX_CLK_SWRSTC4,
> > +    NPCM7XX_CLK_PLLCON2,
> > +    NPCM7XX_CLK_CLKDIV3,
> > +    NPCM7XX_CLK_CORSTC,
> > +    NPCM7XX_CLK_PLLCONG,
> > +    NPCM7XX_CLK_AHBCKFI,
> > +    NPCM7XX_CLK_SECCNT,
> > +    NPCM7XX_CLK_CNTR25M,
> > +    NPCM7XX_CLK_REGS_END,
> > +};
> > +
> > +/*
> > + * These reset values were taken from version 0.91 of the NPCM750R data sheet.
> > + *
> > + * All are loaded on power-up reset. CLKENx and SWRSTR should also be loaded on
> > + * core domain reset, but this reset type is not yet supported by QEMU.
> > + */
> > +static const uint32_t cold_reset_values[NPCM7XX_CLK_NR_REGS] = {
> > +    [NPCM7XX_CLK_CLKEN1]        = 0xffffffff,
> > +    [NPCM7XX_CLK_CLKSEL]        = 0x004aaaaa,
> > +    [NPCM7XX_CLK_CLKDIV1]       = 0x5413f855,
> > +    [NPCM7XX_CLK_PLLCON0]       = 0x00222101 | PLLCON_LOKI,
> > +    [NPCM7XX_CLK_PLLCON1]       = 0x00202101 | PLLCON_LOKI,
> > +    [NPCM7XX_CLK_IPSRST1]       = 0x00001000,
> > +    [NPCM7XX_CLK_IPSRST2]       = 0x80000000,
> > +    [NPCM7XX_CLK_CLKEN2]        = 0xffffffff,
> > +    [NPCM7XX_CLK_CLKDIV2]       = 0xaa4f8f9f,
> > +    [NPCM7XX_CLK_CLKEN3]        = 0xffffffff,
> > +    [NPCM7XX_CLK_IPSRST3]       = 0x03000000,
> > +    [NPCM7XX_CLK_WD0RCR]        = 0xffffffff,
> > +    [NPCM7XX_CLK_WD1RCR]        = 0xffffffff,
> > +    [NPCM7XX_CLK_WD2RCR]        = 0xffffffff,
> > +    [NPCM7XX_CLK_SWRSTC1]       = 0x00000003,
> > +    [NPCM7XX_CLK_PLLCON2]       = 0x00c02105 | PLLCON_LOKI,
> > +    [NPCM7XX_CLK_CORSTC]        = 0x04000003,
> > +    [NPCM7XX_CLK_PLLCONG]       = 0x01228606 | PLLCON_LOKI,
> > +    [NPCM7XX_CLK_AHBCKFI]       = 0x000000c8,
> > +};
> > +
> > +static uint64_t npcm7xx_clk_read(void *opaque, hwaddr offset, unsigned size)
> > +{
> > +    uint32_t reg = offset / sizeof(uint32_t);
> > +    NPCM7xxCLKState *s = opaque;
> > +    int64_t now_ns;
> > +    uint32_t value = 0;
> > +
> > +    if (reg >= NPCM7XX_CLK_NR_REGS) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: offset 0x%04" HWADDR_PRIx " out of range\n",
>
> I find "out of range" not clear, but this might be the correct
> terminology.
>
> > +                      __func__, offset);
> > +        return 0;
> > +    }
> > +
> > +    switch (reg) {
> > +    case NPCM7XX_CLK_SWRSTR:
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: register @ 0x%04" HWADDR_PRIx " is write-only\n",
> > +                      __func__, offset);
> > +        break;
> > +
> > +    case NPCM7XX_CLK_SECCNT:
> > +        now_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > +        value = (now_ns - s->ref_ns) / NANOSECONDS_PER_SECOND;
> > +        break;
> > +
> > +    case NPCM7XX_CLK_CNTR25M:
> > +        now_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > +        /*
> > +         * This register counts 25 MHz cycles, updating every 640 ns. It rolls
> > +         * over to zero every second.
> > +         *
> > +         * The 4 LSBs are always zero: (1e9 / 640) << 4 = 25000000.
> > +         */
> > +        value = (((now_ns - s->ref_ns) / 640) << 4) % NPCM7XX_TIMER_REF_HZ;
> > +        break;
> > +
> > +    default:
> > +        value = s->regs[reg];
> > +        break;
> > +    };
> > +
> > +    trace_npcm7xx_clk_read(offset, value);
> > +
> > +    return value;
> > +}
> > +
> > +static void npcm7xx_clk_write(void *opaque, hwaddr offset,
> > +                              uint64_t v, unsigned size)
> > +{
> > +    uint32_t reg = offset / sizeof(uint32_t);
> > +    NPCM7xxCLKState *s = opaque;
> > +    uint32_t value = v;
> > +
> > +    trace_npcm7xx_clk_write(offset, value);
> > +
> > +    if (reg >= NPCM7XX_CLK_NR_REGS) {
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: offset 0x%04" HWADDR_PRIx " out of range\n",
> > +                      __func__, offset);
> > +        return;
> > +    }
> > +
> > +    switch (reg) {
> > +    case NPCM7XX_CLK_SWRSTR:
> > +        qemu_log_mask(LOG_UNIMP, "%s: SW reset not implemented: 0x%02x\n",
> > +                      __func__, value);
>
> Isn't this sufficient?
>
>            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);

It's not quite that easy; this register holds 4 bits, each of which
maps to a separate register which defines the modules to reset. It's
not that hard, but a little more than I'd like to add to the series at
this point. I'll send a followup patch once the initial series is in.

>
> > +        value = 0;
> > +        break;
> > +
> > +    case NPCM7XX_CLK_PLLCON0:
> > +    case NPCM7XX_CLK_PLLCON1:
> > +    case NPCM7XX_CLK_PLLCON2:
> > +    case NPCM7XX_CLK_PLLCONG:
> > +        if (value & PLLCON_PWDEN) {
> > +            /* Power down -- clear lock and indicate loss of lock */
> > +            value &= ~PLLCON_LOKI;
> > +            value |= PLLCON_LOKS;
> > +        } else {
> > +            /* Normal mode -- assume always locked */
> > +            value |= PLLCON_LOKI;
> > +            /* Keep LOKS unchanged unless cleared by writing 1 */
> > +            if (value & PLLCON_LOKS) {
> > +                value &= ~PLLCON_LOKS;
> > +            } else {
> > +                value |= (value & PLLCON_LOKS);
> > +            }
> > +        }
> > +        break;
> > +
> > +    case NPCM7XX_CLK_CNTR25M:
> > +        qemu_log_mask(LOG_GUEST_ERROR,
> > +                      "%s: register @ 0x%04" HWADDR_PRIx " is read-only\n",
> > +                      __func__, offset);
> > +        return;
> > +    }
> > +
> > +    s->regs[reg] = value;
> > +}
> > +
> > +static const struct MemoryRegionOps npcm7xx_clk_ops = {
> > +    .read       = npcm7xx_clk_read,
> > +    .write      = npcm7xx_clk_write,
> > +    .endianness = DEVICE_LITTLE_ENDIAN,
> > +    .valid      = {
> > +        .min_access_size        = 4,
> > +        .max_access_size        = 4,
> > +        .unaligned              = false,
> > +    },
> > +};
> > +
> > +static void npcm7xx_clk_enter_reset(Object *obj, ResetType type)
> > +{
> > +    NPCM7xxCLKState *s = NPCM7XX_CLK(obj);
> > +
> > +    QEMU_BUILD_BUG_ON(sizeof(s->regs) != sizeof(cold_reset_values));
> > +
> > +    switch (type) {
> > +    case RESET_TYPE_COLD:
> > +        memcpy(s->regs, cold_reset_values, sizeof(cold_reset_values));
> > +        s->ref_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > +        return;
> > +    }
> > +
> > +    /*
> > +     * A small number of registers need to be reset on a core domain reset,
> > +     * but no such reset type exists yet.
> > +     */
> > +    qemu_log_mask(LOG_UNIMP, "%s: reset type %d not implemented.",
> > +                  __func__, type);
>
> Clean :)
>
> > +}
> > +
> > +static void npcm7xx_clk_init(Object *obj)
> > +{
> > +    NPCM7xxCLKState *s = NPCM7XX_CLK(obj);
> > +
> > +    memory_region_init_io(&s->iomem, obj, &npcm7xx_clk_ops, s,
> > +                          TYPE_NPCM7XX_CLK, 4 * KiB);
> > +    sysbus_init_mmio(&s->parent, &s->iomem);
> > +}
> > +
> > +static const VMStateDescription vmstate_npcm7xx_clk = {
> > +    .name = "npcm7xx-clk",
> > +    .version_id = 0,
> > +    .minimum_version_id = 0,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT32_ARRAY(regs, NPCM7xxCLKState, NPCM7XX_CLK_NR_REGS),
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thank you!

> > +        VMSTATE_INT64(ref_ns, NPCM7xxCLKState),
> > +        VMSTATE_END_OF_LIST(),
> > +    },
> > +};
> > +
> > +static void npcm7xx_clk_class_init(ObjectClass *klass, void *data)
> > +{
> > +    ResettableClass *rc = RESETTABLE_CLASS(klass);
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +
> > +    QEMU_BUILD_BUG_ON(NPCM7XX_CLK_REGS_END > NPCM7XX_CLK_NR_REGS);
> > +
> > +    dc->desc = "NPCM7xx Clock Control Registers";
> > +    dc->vmsd = &vmstate_npcm7xx_clk;
> > +    rc->phases.enter = npcm7xx_clk_enter_reset;
> > +}
> > +
> > +static const TypeInfo npcm7xx_clk_info = {
> > +    .name               = TYPE_NPCM7XX_CLK,
> > +    .parent             = TYPE_SYS_BUS_DEVICE,
> > +    .instance_size      = sizeof(NPCM7xxCLKState),
> > +    .instance_init      = npcm7xx_clk_init,
> > +    .class_init         = npcm7xx_clk_class_init,
> > +};
> > +
> > +static void npcm7xx_clk_register_type(void)
> > +{
> > +    type_register_static(&npcm7xx_clk_info);
> > +}
> > +type_init(npcm7xx_clk_register_type);
> > diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> > index 99ffb9f9e8..531cc31dcc 100644
> > --- a/hw/misc/meson.build
> > +++ b/hw/misc/meson.build
> > @@ -51,6 +51,7 @@ softmmu_ss.add(when: 'CONFIG_IMX', if_true: files(
> >  softmmu_ss.add(when: 'CONFIG_MILKYMIST', if_true: files('milkymist-hpdmc.c', 'milkymist-pfpu.c'))
> >  softmmu_ss.add(when: 'CONFIG_MAINSTONE', if_true: files('mst_fpga.c'))
> >  softmmu_ss.add(when: 'CONFIG_NPCM7XX', if_true: files(
> > +  'npcm7xx_clk.c',
> >    'npcm7xx_gcr.c',
> >  ))
> >  softmmu_ss.add(when: 'CONFIG_OMAP', if_true: files(
> > diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> > index dbf90db8ed..a010699b4f 100644
> > --- a/hw/misc/trace-events
> > +++ b/hw/misc/trace-events
> > @@ -111,6 +111,10 @@ mos6522_set_sr_int(void) "set sr_int"
> >  mos6522_write(uint64_t addr, uint64_t val) "reg=0x%"PRIx64 " val=0x%"PRIx64
> >  mos6522_read(uint64_t addr, unsigned val) "reg=0x%"PRIx64 " val=0x%x"
> >
> > +# npcm7xx_clk.c
> > +npcm7xx_clk_read(uint64_t offset, uint32_t value) " offset: 0x%04" PRIx64 " value: 0x%08" PRIx32
> > +npcm7xx_clk_write(uint64_t offset, uint32_t value) "offset: 0x%04" PRIx64 " value: 0x%08" PRIx32
> > +
> >  # npcm7xx_gcr.c
> >  npcm7xx_gcr_read(uint64_t offset, uint32_t value) " offset: 0x%04" PRIx64 " value: 0x%08" PRIx32
> >  npcm7xx_gcr_write(uint64_t offset, uint32_t value) "offset: 0x%04" PRIx64 " value: 0x%08" PRIx32
> >
Havard Skinnemoen Sept. 4, 2020, 10:38 p.m. UTC | #3
On Fri, Sep 4, 2020 at 3:02 PM Havard Skinnemoen <hskinnemoen@google.com> wrote:
>
> On Fri, Sep 4, 2020 at 2:32 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >
> > On 8/25/20 2:16 AM, Havard Skinnemoen via wrote:
> > > Enough functionality to boot the Linux kernel has been implemented. This
> > > includes:
> > >
> > >   - Correct power-on reset values so the various clock rates can be
> > >     accurately calculated.
> > >   - Clock enables stick around when written.
> > >
> > > In addition, a best effort attempt to implement SECCNT and CNTR25M was
> > > made even though I don't think the kernel needs them.
> > >
> > > Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
> > > Reviewed-by: Joel Stanley <joel@jms.id.au>
> > > Reviewed-by: Cédric Le Goater <clg@kaod.org>
> > > Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
> > > ---
> > >  include/hw/misc/npcm7xx_clk.h |  48 ++++++
> > >  hw/misc/npcm7xx_clk.c         | 266 ++++++++++++++++++++++++++++++++++
> > >  hw/misc/meson.build           |   1 +
> > >  hw/misc/trace-events          |   4 +
> > >  4 files changed, 319 insertions(+)
> > >  create mode 100644 include/hw/misc/npcm7xx_clk.h
> > >  create mode 100644 hw/misc/npcm7xx_clk.c
> > >
> > > diff --git a/include/hw/misc/npcm7xx_clk.h b/include/hw/misc/npcm7xx_clk.h
> > > new file mode 100644
> > > index 0000000000..cdcc9e8534
> > > --- /dev/null
> > > +++ b/include/hw/misc/npcm7xx_clk.h
> > > @@ -0,0 +1,48 @@
> > > +/*
> > > + * Nuvoton NPCM7xx Clock Control Registers.
> > > + *
> > > + * Copyright 2020 Google LLC
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify it
> > > + * under the terms of the GNU General Public License as published by the
> > > + * Free Software Foundation; either version 2 of the License, or
> > > + * (at your option) any later version.
> > > + *
> > > + * This program is distributed in the hope that 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.
> > > + */
> > > +#ifndef NPCM7XX_CLK_H
> > > +#define NPCM7XX_CLK_H
> > > +
> > > +#include "exec/memory.h"
> > > +#include "hw/sysbus.h"
> > > +
> > > +/*
> > > + * The reference clock frequency for the timer modules, and the SECCNT and
> > > + * CNTR25M registers in this module, is always 25 MHz.
> > > + */
> > > +#define NPCM7XX_TIMER_REF_HZ            (25000000)
> > > +
> > > +/*
> > > + * Number of registers in our device state structure. Don't change this without
> > > + * incrementing the version_id in the vmstate.
> > > + */
> > > +#define NPCM7XX_CLK_NR_REGS             (0x70 / sizeof(uint32_t))
> > > +
> > > +typedef struct NPCM7xxCLKState {
> > > +    SysBusDevice parent;
> > > +
> > > +    MemoryRegion iomem;
> > > +
> > > +    uint32_t regs[NPCM7XX_CLK_NR_REGS];
> > > +
> > > +    /* Time reference for SECCNT and CNTR25M, initialized by power on reset */
> > > +    int64_t ref_ns;
> > > +} NPCM7xxCLKState;
> > > +
> > > +#define TYPE_NPCM7XX_CLK "npcm7xx-clk"
> > > +#define NPCM7XX_CLK(obj) OBJECT_CHECK(NPCM7xxCLKState, (obj), TYPE_NPCM7XX_CLK)
> > > +
> > > +#endif /* NPCM7XX_CLK_H */
> > > diff --git a/hw/misc/npcm7xx_clk.c b/hw/misc/npcm7xx_clk.c
> > > new file mode 100644
> > > index 0000000000..21ab4200d1
> > > --- /dev/null
> > > +++ b/hw/misc/npcm7xx_clk.c
> > > @@ -0,0 +1,266 @@
> > > +/*
> > > + * Nuvoton NPCM7xx Clock Control Registers.
> > > + *
> > > + * Copyright 2020 Google LLC
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify it
> > > + * under the terms of the GNU General Public License as published by the
> > > + * Free Software Foundation; either version 2 of the License, or
> > > + * (at your option) any later version.
> > > + *
> > > + * This program is distributed in the hope that 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.
> > > + */
> > > +
> > > +#include "qemu/osdep.h"
> > > +
> > > +#include "hw/misc/npcm7xx_clk.h"
> > > +#include "migration/vmstate.h"
> > > +#include "qemu/error-report.h"
> > > +#include "qemu/log.h"
> > > +#include "qemu/module.h"
> > > +#include "qemu/timer.h"
> > > +#include "qemu/units.h"
> > > +#include "trace.h"
> > > +
> > > +#define PLLCON_LOKI     BIT(31)
> > > +#define PLLCON_LOKS     BIT(30)
> > > +#define PLLCON_PWDEN    BIT(12)
> > > +
> > > +enum NPCM7xxCLKRegisters {
> > > +    NPCM7XX_CLK_CLKEN1,
> > > +    NPCM7XX_CLK_CLKSEL,
> > > +    NPCM7XX_CLK_CLKDIV1,
> > > +    NPCM7XX_CLK_PLLCON0,
> > > +    NPCM7XX_CLK_PLLCON1,
> > > +    NPCM7XX_CLK_SWRSTR,
> > > +    NPCM7XX_CLK_IPSRST1         = 0x20 / sizeof(uint32_t),
> > > +    NPCM7XX_CLK_IPSRST2,
> > > +    NPCM7XX_CLK_CLKEN2,
> > > +    NPCM7XX_CLK_CLKDIV2,
> > > +    NPCM7XX_CLK_CLKEN3,
> > > +    NPCM7XX_CLK_IPSRST3,
> > > +    NPCM7XX_CLK_WD0RCR,
> > > +    NPCM7XX_CLK_WD1RCR,
> > > +    NPCM7XX_CLK_WD2RCR,
> > > +    NPCM7XX_CLK_SWRSTC1,
> > > +    NPCM7XX_CLK_SWRSTC2,
> > > +    NPCM7XX_CLK_SWRSTC3,
> > > +    NPCM7XX_CLK_SWRSTC4,
> > > +    NPCM7XX_CLK_PLLCON2,
> > > +    NPCM7XX_CLK_CLKDIV3,
> > > +    NPCM7XX_CLK_CORSTC,
> > > +    NPCM7XX_CLK_PLLCONG,
> > > +    NPCM7XX_CLK_AHBCKFI,
> > > +    NPCM7XX_CLK_SECCNT,
> > > +    NPCM7XX_CLK_CNTR25M,
> > > +    NPCM7XX_CLK_REGS_END,
> > > +};
> > > +
> > > +/*
> > > + * These reset values were taken from version 0.91 of the NPCM750R data sheet.
> > > + *
> > > + * All are loaded on power-up reset. CLKENx and SWRSTR should also be loaded on
> > > + * core domain reset, but this reset type is not yet supported by QEMU.
> > > + */
> > > +static const uint32_t cold_reset_values[NPCM7XX_CLK_NR_REGS] = {
> > > +    [NPCM7XX_CLK_CLKEN1]        = 0xffffffff,
> > > +    [NPCM7XX_CLK_CLKSEL]        = 0x004aaaaa,
> > > +    [NPCM7XX_CLK_CLKDIV1]       = 0x5413f855,
> > > +    [NPCM7XX_CLK_PLLCON0]       = 0x00222101 | PLLCON_LOKI,
> > > +    [NPCM7XX_CLK_PLLCON1]       = 0x00202101 | PLLCON_LOKI,
> > > +    [NPCM7XX_CLK_IPSRST1]       = 0x00001000,
> > > +    [NPCM7XX_CLK_IPSRST2]       = 0x80000000,
> > > +    [NPCM7XX_CLK_CLKEN2]        = 0xffffffff,
> > > +    [NPCM7XX_CLK_CLKDIV2]       = 0xaa4f8f9f,
> > > +    [NPCM7XX_CLK_CLKEN3]        = 0xffffffff,
> > > +    [NPCM7XX_CLK_IPSRST3]       = 0x03000000,
> > > +    [NPCM7XX_CLK_WD0RCR]        = 0xffffffff,
> > > +    [NPCM7XX_CLK_WD1RCR]        = 0xffffffff,
> > > +    [NPCM7XX_CLK_WD2RCR]        = 0xffffffff,
> > > +    [NPCM7XX_CLK_SWRSTC1]       = 0x00000003,
> > > +    [NPCM7XX_CLK_PLLCON2]       = 0x00c02105 | PLLCON_LOKI,
> > > +    [NPCM7XX_CLK_CORSTC]        = 0x04000003,
> > > +    [NPCM7XX_CLK_PLLCONG]       = 0x01228606 | PLLCON_LOKI,
> > > +    [NPCM7XX_CLK_AHBCKFI]       = 0x000000c8,
> > > +};
> > > +
> > > +static uint64_t npcm7xx_clk_read(void *opaque, hwaddr offset, unsigned size)
> > > +{
> > > +    uint32_t reg = offset / sizeof(uint32_t);
> > > +    NPCM7xxCLKState *s = opaque;
> > > +    int64_t now_ns;
> > > +    uint32_t value = 0;
> > > +
> > > +    if (reg >= NPCM7XX_CLK_NR_REGS) {
> > > +        qemu_log_mask(LOG_GUEST_ERROR,
> > > +                      "%s: offset 0x%04" HWADDR_PRIx " out of range\n",
> >
> > I find "out of range" not clear, but this might be the correct
> > terminology.
> >
> > > +                      __func__, offset);
> > > +        return 0;
> > > +    }
> > > +
> > > +    switch (reg) {
> > > +    case NPCM7XX_CLK_SWRSTR:
> > > +        qemu_log_mask(LOG_GUEST_ERROR,
> > > +                      "%s: register @ 0x%04" HWADDR_PRIx " is write-only\n",
> > > +                      __func__, offset);
> > > +        break;
> > > +
> > > +    case NPCM7XX_CLK_SECCNT:
> > > +        now_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > > +        value = (now_ns - s->ref_ns) / NANOSECONDS_PER_SECOND;
> > > +        break;
> > > +
> > > +    case NPCM7XX_CLK_CNTR25M:
> > > +        now_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > > +        /*
> > > +         * This register counts 25 MHz cycles, updating every 640 ns. It rolls
> > > +         * over to zero every second.
> > > +         *
> > > +         * The 4 LSBs are always zero: (1e9 / 640) << 4 = 25000000.
> > > +         */
> > > +        value = (((now_ns - s->ref_ns) / 640) << 4) % NPCM7XX_TIMER_REF_HZ;
> > > +        break;
> > > +
> > > +    default:
> > > +        value = s->regs[reg];
> > > +        break;
> > > +    };
> > > +
> > > +    trace_npcm7xx_clk_read(offset, value);
> > > +
> > > +    return value;
> > > +}
> > > +
> > > +static void npcm7xx_clk_write(void *opaque, hwaddr offset,
> > > +                              uint64_t v, unsigned size)
> > > +{
> > > +    uint32_t reg = offset / sizeof(uint32_t);
> > > +    NPCM7xxCLKState *s = opaque;
> > > +    uint32_t value = v;
> > > +
> > > +    trace_npcm7xx_clk_write(offset, value);
> > > +
> > > +    if (reg >= NPCM7XX_CLK_NR_REGS) {
> > > +        qemu_log_mask(LOG_GUEST_ERROR,
> > > +                      "%s: offset 0x%04" HWADDR_PRIx " out of range\n",
> > > +                      __func__, offset);
> > > +        return;
> > > +    }
> > > +
> > > +    switch (reg) {
> > > +    case NPCM7XX_CLK_SWRSTR:
> > > +        qemu_log_mask(LOG_UNIMP, "%s: SW reset not implemented: 0x%02x\n",
> > > +                      __func__, value);
> >
> > Isn't this sufficient?
> >
> >            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>
> It's not quite that easy; this register holds 4 bits, each of which
> maps to a separate register which defines the modules to reset. It's
> not that hard, but a little more than I'd like to add to the series at
> this point. I'll send a followup patch once the initial series is in.

Actually, I'm not sure if this would have any effect on being able to
reboot. Running with -d unimp shows:

reboot: Restarting system
npcm7xx_timer_write: WTCR write not implemented: 0x00000083
Reboot failed -- System halted

So we need to implement watchdog support, which is something we were
planning to do fairly soon.

> >
> > > +        value = 0;
> > > +        break;
> > > +
> > > +    case NPCM7XX_CLK_PLLCON0:
> > > +    case NPCM7XX_CLK_PLLCON1:
> > > +    case NPCM7XX_CLK_PLLCON2:
> > > +    case NPCM7XX_CLK_PLLCONG:
> > > +        if (value & PLLCON_PWDEN) {
> > > +            /* Power down -- clear lock and indicate loss of lock */
> > > +            value &= ~PLLCON_LOKI;
> > > +            value |= PLLCON_LOKS;
> > > +        } else {
> > > +            /* Normal mode -- assume always locked */
> > > +            value |= PLLCON_LOKI;
> > > +            /* Keep LOKS unchanged unless cleared by writing 1 */
> > > +            if (value & PLLCON_LOKS) {
> > > +                value &= ~PLLCON_LOKS;
> > > +            } else {
> > > +                value |= (value & PLLCON_LOKS);
> > > +            }
> > > +        }
> > > +        break;
> > > +
> > > +    case NPCM7XX_CLK_CNTR25M:
> > > +        qemu_log_mask(LOG_GUEST_ERROR,
> > > +                      "%s: register @ 0x%04" HWADDR_PRIx " is read-only\n",
> > > +                      __func__, offset);
> > > +        return;
> > > +    }
> > > +
> > > +    s->regs[reg] = value;
> > > +}
> > > +
> > > +static const struct MemoryRegionOps npcm7xx_clk_ops = {
> > > +    .read       = npcm7xx_clk_read,
> > > +    .write      = npcm7xx_clk_write,
> > > +    .endianness = DEVICE_LITTLE_ENDIAN,
> > > +    .valid      = {
> > > +        .min_access_size        = 4,
> > > +        .max_access_size        = 4,
> > > +        .unaligned              = false,
> > > +    },
> > > +};
> > > +
> > > +static void npcm7xx_clk_enter_reset(Object *obj, ResetType type)
> > > +{
> > > +    NPCM7xxCLKState *s = NPCM7XX_CLK(obj);
> > > +
> > > +    QEMU_BUILD_BUG_ON(sizeof(s->regs) != sizeof(cold_reset_values));
> > > +
> > > +    switch (type) {
> > > +    case RESET_TYPE_COLD:
> > > +        memcpy(s->regs, cold_reset_values, sizeof(cold_reset_values));
> > > +        s->ref_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > > +        return;
> > > +    }
> > > +
> > > +    /*
> > > +     * A small number of registers need to be reset on a core domain reset,
> > > +     * but no such reset type exists yet.
> > > +     */
> > > +    qemu_log_mask(LOG_UNIMP, "%s: reset type %d not implemented.",
> > > +                  __func__, type);
> >
> > Clean :)
> >
> > > +}
> > > +
> > > +static void npcm7xx_clk_init(Object *obj)
> > > +{
> > > +    NPCM7xxCLKState *s = NPCM7XX_CLK(obj);
> > > +
> > > +    memory_region_init_io(&s->iomem, obj, &npcm7xx_clk_ops, s,
> > > +                          TYPE_NPCM7XX_CLK, 4 * KiB);
> > > +    sysbus_init_mmio(&s->parent, &s->iomem);
> > > +}
> > > +
> > > +static const VMStateDescription vmstate_npcm7xx_clk = {
> > > +    .name = "npcm7xx-clk",
> > > +    .version_id = 0,
> > > +    .minimum_version_id = 0,
> > > +    .fields = (VMStateField[]) {
> > > +        VMSTATE_UINT32_ARRAY(regs, NPCM7xxCLKState, NPCM7XX_CLK_NR_REGS),
> >
> > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> Thank you!
>
> > > +        VMSTATE_INT64(ref_ns, NPCM7xxCLKState),
> > > +        VMSTATE_END_OF_LIST(),
> > > +    },
> > > +};
> > > +
> > > +static void npcm7xx_clk_class_init(ObjectClass *klass, void *data)
> > > +{
> > > +    ResettableClass *rc = RESETTABLE_CLASS(klass);
> > > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > > +
> > > +    QEMU_BUILD_BUG_ON(NPCM7XX_CLK_REGS_END > NPCM7XX_CLK_NR_REGS);
> > > +
> > > +    dc->desc = "NPCM7xx Clock Control Registers";
> > > +    dc->vmsd = &vmstate_npcm7xx_clk;
> > > +    rc->phases.enter = npcm7xx_clk_enter_reset;
> > > +}
> > > +
> > > +static const TypeInfo npcm7xx_clk_info = {
> > > +    .name               = TYPE_NPCM7XX_CLK,
> > > +    .parent             = TYPE_SYS_BUS_DEVICE,
> > > +    .instance_size      = sizeof(NPCM7xxCLKState),
> > > +    .instance_init      = npcm7xx_clk_init,
> > > +    .class_init         = npcm7xx_clk_class_init,
> > > +};
> > > +
> > > +static void npcm7xx_clk_register_type(void)
> > > +{
> > > +    type_register_static(&npcm7xx_clk_info);
> > > +}
> > > +type_init(npcm7xx_clk_register_type);
> > > diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> > > index 99ffb9f9e8..531cc31dcc 100644
> > > --- a/hw/misc/meson.build
> > > +++ b/hw/misc/meson.build
> > > @@ -51,6 +51,7 @@ softmmu_ss.add(when: 'CONFIG_IMX', if_true: files(
> > >  softmmu_ss.add(when: 'CONFIG_MILKYMIST', if_true: files('milkymist-hpdmc.c', 'milkymist-pfpu.c'))
> > >  softmmu_ss.add(when: 'CONFIG_MAINSTONE', if_true: files('mst_fpga.c'))
> > >  softmmu_ss.add(when: 'CONFIG_NPCM7XX', if_true: files(
> > > +  'npcm7xx_clk.c',
> > >    'npcm7xx_gcr.c',
> > >  ))
> > >  softmmu_ss.add(when: 'CONFIG_OMAP', if_true: files(
> > > diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> > > index dbf90db8ed..a010699b4f 100644
> > > --- a/hw/misc/trace-events
> > > +++ b/hw/misc/trace-events
> > > @@ -111,6 +111,10 @@ mos6522_set_sr_int(void) "set sr_int"
> > >  mos6522_write(uint64_t addr, uint64_t val) "reg=0x%"PRIx64 " val=0x%"PRIx64
> > >  mos6522_read(uint64_t addr, unsigned val) "reg=0x%"PRIx64 " val=0x%x"
> > >
> > > +# npcm7xx_clk.c
> > > +npcm7xx_clk_read(uint64_t offset, uint32_t value) " offset: 0x%04" PRIx64 " value: 0x%08" PRIx32
> > > +npcm7xx_clk_write(uint64_t offset, uint32_t value) "offset: 0x%04" PRIx64 " value: 0x%08" PRIx32
> > > +
> > >  # npcm7xx_gcr.c
> > >  npcm7xx_gcr_read(uint64_t offset, uint32_t value) " offset: 0x%04" PRIx64 " value: 0x%08" PRIx32
> > >  npcm7xx_gcr_write(uint64_t offset, uint32_t value) "offset: 0x%04" PRIx64 " value: 0x%08" PRIx32
> > >
Philippe Mathieu-Daudé Sept. 7, 2020, 1:40 p.m. UTC | #4
On 9/5/20 12:38 AM, Havard Skinnemoen wrote:
> On Fri, Sep 4, 2020 at 3:02 PM Havard Skinnemoen <hskinnemoen@google.com> wrote:
>>
>> On Fri, Sep 4, 2020 at 2:32 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>
>>> On 8/25/20 2:16 AM, Havard Skinnemoen via wrote:
>>>> Enough functionality to boot the Linux kernel has been implemented. This
>>>> includes:
>>>>
>>>>   - Correct power-on reset values so the various clock rates can be
>>>>     accurately calculated.
>>>>   - Clock enables stick around when written.
>>>>
>>>> In addition, a best effort attempt to implement SECCNT and CNTR25M was
>>>> made even though I don't think the kernel needs them.
>>>>
>>>> Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
>>>> Reviewed-by: Joel Stanley <joel@jms.id.au>
>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>>> Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
>>>> ---
[...]
>>>> +static void npcm7xx_clk_write(void *opaque, hwaddr offset,
>>>> +                              uint64_t v, unsigned size)
>>>> +{
>>>> +    uint32_t reg = offset / sizeof(uint32_t);
>>>> +    NPCM7xxCLKState *s = opaque;
>>>> +    uint32_t value = v;
>>>> +
>>>> +    trace_npcm7xx_clk_write(offset, value);
>>>> +
>>>> +    if (reg >= NPCM7XX_CLK_NR_REGS) {
>>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>>> +                      "%s: offset 0x%04" HWADDR_PRIx " out of range\n",
>>>> +                      __func__, offset);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    switch (reg) {
>>>> +    case NPCM7XX_CLK_SWRSTR:
>>>> +        qemu_log_mask(LOG_UNIMP, "%s: SW reset not implemented: 0x%02x\n",
>>>> +                      __func__, value);
>>>
>>> Isn't this sufficient?
>>>
>>>            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>>
>> It's not quite that easy; this register holds 4 bits, each of which
>> maps to a separate register which defines the modules to reset. It's
>> not that hard, but a little more than I'd like to add to the series at
>> this point. I'll send a followup patch once the initial series is in.
> 
> Actually, I'm not sure if this would have any effect on being able to
> reboot. Running with -d unimp shows:
> 
> reboot: Restarting system
> npcm7xx_timer_write: WTCR write not implemented: 0x00000083
> Reboot failed -- System halted
> 
> So we need to implement watchdog support, which is something we were
> planning to do fairly soon.

Well this seems a guest implementation decision to prefer
wait the watchdog to kick (hard reset?) rather than doing
a soft reset.

Two different issues IMO. Anyway this is certainly not
blocking your series to get merged.

Regards,

Phil.
Havard Skinnemoen Sept. 7, 2020, 5:58 p.m. UTC | #5
On Mon, Sep 7, 2020 at 6:40 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 9/5/20 12:38 AM, Havard Skinnemoen wrote:
> > On Fri, Sep 4, 2020 at 3:02 PM Havard Skinnemoen <hskinnemoen@google.com> wrote:
> >>
> >> On Fri, Sep 4, 2020 at 2:32 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>>
> >>> On 8/25/20 2:16 AM, Havard Skinnemoen via wrote:
> >>>> Enough functionality to boot the Linux kernel has been implemented. This
> >>>> includes:
> >>>>
> >>>>   - Correct power-on reset values so the various clock rates can be
> >>>>     accurately calculated.
> >>>>   - Clock enables stick around when written.
> >>>>
> >>>> In addition, a best effort attempt to implement SECCNT and CNTR25M was
> >>>> made even though I don't think the kernel needs them.
> >>>>
> >>>> Reviewed-by: Tyrone Ting <kfting@nuvoton.com>
> >>>> Reviewed-by: Joel Stanley <joel@jms.id.au>
> >>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> >>>> Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
> >>>> ---
> [...]
> >>>> +static void npcm7xx_clk_write(void *opaque, hwaddr offset,
> >>>> +                              uint64_t v, unsigned size)
> >>>> +{
> >>>> +    uint32_t reg = offset / sizeof(uint32_t);
> >>>> +    NPCM7xxCLKState *s = opaque;
> >>>> +    uint32_t value = v;
> >>>> +
> >>>> +    trace_npcm7xx_clk_write(offset, value);
> >>>> +
> >>>> +    if (reg >= NPCM7XX_CLK_NR_REGS) {
> >>>> +        qemu_log_mask(LOG_GUEST_ERROR,
> >>>> +                      "%s: offset 0x%04" HWADDR_PRIx " out of range\n",
> >>>> +                      __func__, offset);
> >>>> +        return;
> >>>> +    }
> >>>> +
> >>>> +    switch (reg) {
> >>>> +    case NPCM7XX_CLK_SWRSTR:
> >>>> +        qemu_log_mask(LOG_UNIMP, "%s: SW reset not implemented: 0x%02x\n",
> >>>> +                      __func__, value);
> >>>
> >>> Isn't this sufficient?
> >>>
> >>>            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> >>
> >> It's not quite that easy; this register holds 4 bits, each of which
> >> maps to a separate register which defines the modules to reset. It's
> >> not that hard, but a little more than I'd like to add to the series at
> >> this point. I'll send a followup patch once the initial series is in.
> >
> > Actually, I'm not sure if this would have any effect on being able to
> > reboot. Running with -d unimp shows:
> >
> > reboot: Restarting system
> > npcm7xx_timer_write: WTCR write not implemented: 0x00000083
> > Reboot failed -- System halted
> >
> > So we need to implement watchdog support, which is something we were
> > planning to do fairly soon.
>
> Well this seems a guest implementation decision to prefer
> wait the watchdog to kick (hard reset?) rather than doing
> a soft reset.
>
> Two different issues IMO. Anyway this is certainly not
> blocking your series to get merged.

I agree, we need to implement both. I just wanted to note that
implementing SWRST alone may not be enough to make reboots work with
the current image. The SW reset registers are actually very similar to
the WD reset registers, so implementing WD reset should make it almost
trivial to add SW reset as well.

It looks like the kernel has a driver that can use SW reset to reboot,
but the DT is missing a property needed to set up the restart handler.

https://elixir.bootlin.com/linux/v5.8.7/source/drivers/reset/reset-npcm.c#L269

Havard
diff mbox series

Patch

diff --git a/include/hw/misc/npcm7xx_clk.h b/include/hw/misc/npcm7xx_clk.h
new file mode 100644
index 0000000000..cdcc9e8534
--- /dev/null
+++ b/include/hw/misc/npcm7xx_clk.h
@@ -0,0 +1,48 @@ 
+/*
+ * Nuvoton NPCM7xx Clock Control Registers.
+ *
+ * Copyright 2020 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that 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.
+ */
+#ifndef NPCM7XX_CLK_H
+#define NPCM7XX_CLK_H
+
+#include "exec/memory.h"
+#include "hw/sysbus.h"
+
+/*
+ * The reference clock frequency for the timer modules, and the SECCNT and
+ * CNTR25M registers in this module, is always 25 MHz.
+ */
+#define NPCM7XX_TIMER_REF_HZ            (25000000)
+
+/*
+ * Number of registers in our device state structure. Don't change this without
+ * incrementing the version_id in the vmstate.
+ */
+#define NPCM7XX_CLK_NR_REGS             (0x70 / sizeof(uint32_t))
+
+typedef struct NPCM7xxCLKState {
+    SysBusDevice parent;
+
+    MemoryRegion iomem;
+
+    uint32_t regs[NPCM7XX_CLK_NR_REGS];
+
+    /* Time reference for SECCNT and CNTR25M, initialized by power on reset */
+    int64_t ref_ns;
+} NPCM7xxCLKState;
+
+#define TYPE_NPCM7XX_CLK "npcm7xx-clk"
+#define NPCM7XX_CLK(obj) OBJECT_CHECK(NPCM7xxCLKState, (obj), TYPE_NPCM7XX_CLK)
+
+#endif /* NPCM7XX_CLK_H */
diff --git a/hw/misc/npcm7xx_clk.c b/hw/misc/npcm7xx_clk.c
new file mode 100644
index 0000000000..21ab4200d1
--- /dev/null
+++ b/hw/misc/npcm7xx_clk.c
@@ -0,0 +1,266 @@ 
+/*
+ * Nuvoton NPCM7xx Clock Control Registers.
+ *
+ * Copyright 2020 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that 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.
+ */
+
+#include "qemu/osdep.h"
+
+#include "hw/misc/npcm7xx_clk.h"
+#include "migration/vmstate.h"
+#include "qemu/error-report.h"
+#include "qemu/log.h"
+#include "qemu/module.h"
+#include "qemu/timer.h"
+#include "qemu/units.h"
+#include "trace.h"
+
+#define PLLCON_LOKI     BIT(31)
+#define PLLCON_LOKS     BIT(30)
+#define PLLCON_PWDEN    BIT(12)
+
+enum NPCM7xxCLKRegisters {
+    NPCM7XX_CLK_CLKEN1,
+    NPCM7XX_CLK_CLKSEL,
+    NPCM7XX_CLK_CLKDIV1,
+    NPCM7XX_CLK_PLLCON0,
+    NPCM7XX_CLK_PLLCON1,
+    NPCM7XX_CLK_SWRSTR,
+    NPCM7XX_CLK_IPSRST1         = 0x20 / sizeof(uint32_t),
+    NPCM7XX_CLK_IPSRST2,
+    NPCM7XX_CLK_CLKEN2,
+    NPCM7XX_CLK_CLKDIV2,
+    NPCM7XX_CLK_CLKEN3,
+    NPCM7XX_CLK_IPSRST3,
+    NPCM7XX_CLK_WD0RCR,
+    NPCM7XX_CLK_WD1RCR,
+    NPCM7XX_CLK_WD2RCR,
+    NPCM7XX_CLK_SWRSTC1,
+    NPCM7XX_CLK_SWRSTC2,
+    NPCM7XX_CLK_SWRSTC3,
+    NPCM7XX_CLK_SWRSTC4,
+    NPCM7XX_CLK_PLLCON2,
+    NPCM7XX_CLK_CLKDIV3,
+    NPCM7XX_CLK_CORSTC,
+    NPCM7XX_CLK_PLLCONG,
+    NPCM7XX_CLK_AHBCKFI,
+    NPCM7XX_CLK_SECCNT,
+    NPCM7XX_CLK_CNTR25M,
+    NPCM7XX_CLK_REGS_END,
+};
+
+/*
+ * These reset values were taken from version 0.91 of the NPCM750R data sheet.
+ *
+ * All are loaded on power-up reset. CLKENx and SWRSTR should also be loaded on
+ * core domain reset, but this reset type is not yet supported by QEMU.
+ */
+static const uint32_t cold_reset_values[NPCM7XX_CLK_NR_REGS] = {
+    [NPCM7XX_CLK_CLKEN1]        = 0xffffffff,
+    [NPCM7XX_CLK_CLKSEL]        = 0x004aaaaa,
+    [NPCM7XX_CLK_CLKDIV1]       = 0x5413f855,
+    [NPCM7XX_CLK_PLLCON0]       = 0x00222101 | PLLCON_LOKI,
+    [NPCM7XX_CLK_PLLCON1]       = 0x00202101 | PLLCON_LOKI,
+    [NPCM7XX_CLK_IPSRST1]       = 0x00001000,
+    [NPCM7XX_CLK_IPSRST2]       = 0x80000000,
+    [NPCM7XX_CLK_CLKEN2]        = 0xffffffff,
+    [NPCM7XX_CLK_CLKDIV2]       = 0xaa4f8f9f,
+    [NPCM7XX_CLK_CLKEN3]        = 0xffffffff,
+    [NPCM7XX_CLK_IPSRST3]       = 0x03000000,
+    [NPCM7XX_CLK_WD0RCR]        = 0xffffffff,
+    [NPCM7XX_CLK_WD1RCR]        = 0xffffffff,
+    [NPCM7XX_CLK_WD2RCR]        = 0xffffffff,
+    [NPCM7XX_CLK_SWRSTC1]       = 0x00000003,
+    [NPCM7XX_CLK_PLLCON2]       = 0x00c02105 | PLLCON_LOKI,
+    [NPCM7XX_CLK_CORSTC]        = 0x04000003,
+    [NPCM7XX_CLK_PLLCONG]       = 0x01228606 | PLLCON_LOKI,
+    [NPCM7XX_CLK_AHBCKFI]       = 0x000000c8,
+};
+
+static uint64_t npcm7xx_clk_read(void *opaque, hwaddr offset, unsigned size)
+{
+    uint32_t reg = offset / sizeof(uint32_t);
+    NPCM7xxCLKState *s = opaque;
+    int64_t now_ns;
+    uint32_t value = 0;
+
+    if (reg >= NPCM7XX_CLK_NR_REGS) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: offset 0x%04" HWADDR_PRIx " out of range\n",
+                      __func__, offset);
+        return 0;
+    }
+
+    switch (reg) {
+    case NPCM7XX_CLK_SWRSTR:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: register @ 0x%04" HWADDR_PRIx " is write-only\n",
+                      __func__, offset);
+        break;
+
+    case NPCM7XX_CLK_SECCNT:
+        now_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+        value = (now_ns - s->ref_ns) / NANOSECONDS_PER_SECOND;
+        break;
+
+    case NPCM7XX_CLK_CNTR25M:
+        now_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+        /*
+         * This register counts 25 MHz cycles, updating every 640 ns. It rolls
+         * over to zero every second.
+         *
+         * The 4 LSBs are always zero: (1e9 / 640) << 4 = 25000000.
+         */
+        value = (((now_ns - s->ref_ns) / 640) << 4) % NPCM7XX_TIMER_REF_HZ;
+        break;
+
+    default:
+        value = s->regs[reg];
+        break;
+    };
+
+    trace_npcm7xx_clk_read(offset, value);
+
+    return value;
+}
+
+static void npcm7xx_clk_write(void *opaque, hwaddr offset,
+                              uint64_t v, unsigned size)
+{
+    uint32_t reg = offset / sizeof(uint32_t);
+    NPCM7xxCLKState *s = opaque;
+    uint32_t value = v;
+
+    trace_npcm7xx_clk_write(offset, value);
+
+    if (reg >= NPCM7XX_CLK_NR_REGS) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: offset 0x%04" HWADDR_PRIx " out of range\n",
+                      __func__, offset);
+        return;
+    }
+
+    switch (reg) {
+    case NPCM7XX_CLK_SWRSTR:
+        qemu_log_mask(LOG_UNIMP, "%s: SW reset not implemented: 0x%02x\n",
+                      __func__, value);
+        value = 0;
+        break;
+
+    case NPCM7XX_CLK_PLLCON0:
+    case NPCM7XX_CLK_PLLCON1:
+    case NPCM7XX_CLK_PLLCON2:
+    case NPCM7XX_CLK_PLLCONG:
+        if (value & PLLCON_PWDEN) {
+            /* Power down -- clear lock and indicate loss of lock */
+            value &= ~PLLCON_LOKI;
+            value |= PLLCON_LOKS;
+        } else {
+            /* Normal mode -- assume always locked */
+            value |= PLLCON_LOKI;
+            /* Keep LOKS unchanged unless cleared by writing 1 */
+            if (value & PLLCON_LOKS) {
+                value &= ~PLLCON_LOKS;
+            } else {
+                value |= (value & PLLCON_LOKS);
+            }
+        }
+        break;
+
+    case NPCM7XX_CLK_CNTR25M:
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: register @ 0x%04" HWADDR_PRIx " is read-only\n",
+                      __func__, offset);
+        return;
+    }
+
+    s->regs[reg] = value;
+}
+
+static const struct MemoryRegionOps npcm7xx_clk_ops = {
+    .read       = npcm7xx_clk_read,
+    .write      = npcm7xx_clk_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+    .valid      = {
+        .min_access_size        = 4,
+        .max_access_size        = 4,
+        .unaligned              = false,
+    },
+};
+
+static void npcm7xx_clk_enter_reset(Object *obj, ResetType type)
+{
+    NPCM7xxCLKState *s = NPCM7XX_CLK(obj);
+
+    QEMU_BUILD_BUG_ON(sizeof(s->regs) != sizeof(cold_reset_values));
+
+    switch (type) {
+    case RESET_TYPE_COLD:
+        memcpy(s->regs, cold_reset_values, sizeof(cold_reset_values));
+        s->ref_ns = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+        return;
+    }
+
+    /*
+     * A small number of registers need to be reset on a core domain reset,
+     * but no such reset type exists yet.
+     */
+    qemu_log_mask(LOG_UNIMP, "%s: reset type %d not implemented.",
+                  __func__, type);
+}
+
+static void npcm7xx_clk_init(Object *obj)
+{
+    NPCM7xxCLKState *s = NPCM7XX_CLK(obj);
+
+    memory_region_init_io(&s->iomem, obj, &npcm7xx_clk_ops, s,
+                          TYPE_NPCM7XX_CLK, 4 * KiB);
+    sysbus_init_mmio(&s->parent, &s->iomem);
+}
+
+static const VMStateDescription vmstate_npcm7xx_clk = {
+    .name = "npcm7xx-clk",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT32_ARRAY(regs, NPCM7xxCLKState, NPCM7XX_CLK_NR_REGS),
+        VMSTATE_INT64(ref_ns, NPCM7xxCLKState),
+        VMSTATE_END_OF_LIST(),
+    },
+};
+
+static void npcm7xx_clk_class_init(ObjectClass *klass, void *data)
+{
+    ResettableClass *rc = RESETTABLE_CLASS(klass);
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    QEMU_BUILD_BUG_ON(NPCM7XX_CLK_REGS_END > NPCM7XX_CLK_NR_REGS);
+
+    dc->desc = "NPCM7xx Clock Control Registers";
+    dc->vmsd = &vmstate_npcm7xx_clk;
+    rc->phases.enter = npcm7xx_clk_enter_reset;
+}
+
+static const TypeInfo npcm7xx_clk_info = {
+    .name               = TYPE_NPCM7XX_CLK,
+    .parent             = TYPE_SYS_BUS_DEVICE,
+    .instance_size      = sizeof(NPCM7xxCLKState),
+    .instance_init      = npcm7xx_clk_init,
+    .class_init         = npcm7xx_clk_class_init,
+};
+
+static void npcm7xx_clk_register_type(void)
+{
+    type_register_static(&npcm7xx_clk_info);
+}
+type_init(npcm7xx_clk_register_type);
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 99ffb9f9e8..531cc31dcc 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -51,6 +51,7 @@  softmmu_ss.add(when: 'CONFIG_IMX', if_true: files(
 softmmu_ss.add(when: 'CONFIG_MILKYMIST', if_true: files('milkymist-hpdmc.c', 'milkymist-pfpu.c'))
 softmmu_ss.add(when: 'CONFIG_MAINSTONE', if_true: files('mst_fpga.c'))
 softmmu_ss.add(when: 'CONFIG_NPCM7XX', if_true: files(
+  'npcm7xx_clk.c',
   'npcm7xx_gcr.c',
 ))
 softmmu_ss.add(when: 'CONFIG_OMAP', if_true: files(
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index dbf90db8ed..a010699b4f 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -111,6 +111,10 @@  mos6522_set_sr_int(void) "set sr_int"
 mos6522_write(uint64_t addr, uint64_t val) "reg=0x%"PRIx64 " val=0x%"PRIx64
 mos6522_read(uint64_t addr, unsigned val) "reg=0x%"PRIx64 " val=0x%x"
 
+# npcm7xx_clk.c
+npcm7xx_clk_read(uint64_t offset, uint32_t value) " offset: 0x%04" PRIx64 " value: 0x%08" PRIx32
+npcm7xx_clk_write(uint64_t offset, uint32_t value) "offset: 0x%04" PRIx64 " value: 0x%08" PRIx32
+
 # npcm7xx_gcr.c
 npcm7xx_gcr_read(uint64_t offset, uint32_t value) " offset: 0x%04" PRIx64 " value: 0x%08" PRIx32
 npcm7xx_gcr_write(uint64_t offset, uint32_t value) "offset: 0x%04" PRIx64 " value: 0x%08" PRIx32