Re: [Qemu-devel] [PATCH v2 2/2] i.MX: Add i.MX25 CCM driver

[Peter Crosthwaite]

On Wed, Oct 28, 2015 at 4:02 PM, Jean-Christophe Dubois
<address@hidden> wrote:
> The i.MX25 CCM device is different from the i.MX31 one.
>
> So for now the emulation was not correct even if linux was working OK
> on top of the i.MX25 emulation.
>
> We add an i.MX25 specific CCM driver and use it in the i.MX25 SOC
> emulation.
>

s/driver/device here and in subject.

> Signed-off-by: Jean-Christophe Dubois <address@hidden>
> ---
>
> Changes since v1:
>  * rework loging to match other i.MX drivers
>
>  hw/arm/fsl-imx25.c          |   2 +-
>  hw/misc/Makefile.objs       |   1 +
>  hw/misc/imx25_ccm.c         | 228 
> ++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/arm/fsl-imx25.h  |   4 +-
>  include/hw/misc/imx25_ccm.h |  61 ++++++++++++
>  5 files changed, 293 insertions(+), 3 deletions(-)
>  create mode 100644 hw/misc/imx25_ccm.c
>  create mode 100644 include/hw/misc/imx25_ccm.h
>
> diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
> index 620c5c6..5301c1c 100644
> --- a/hw/arm/fsl-imx25.c
> +++ b/hw/arm/fsl-imx25.c
> @@ -38,7 +38,7 @@ static void fsl_imx25_init(Object *obj)
>      object_initialize(&s->avic, sizeof(s->avic), TYPE_IMX_AVIC);
>      qdev_set_parent_bus(DEVICE(&s->avic), sysbus_get_default());
>
> -    object_initialize(&s->ccm, sizeof(s->ccm), TYPE_IMX31_CCM);
> +    object_initialize(&s->ccm, sizeof(s->ccm), TYPE_IMX25_CCM);
>      qdev_set_parent_bus(DEVICE(&s->ccm), sysbus_get_default());
>
>      for (i = 0; i < FSL_IMX25_NUM_UARTS; i++) {
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index 79b3487..e8dc22d 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -27,6 +27,7 @@ obj-$(CONFIG_ECCMEMCTL) += eccmemctl.o
>  obj-$(CONFIG_EXYNOS4) += exynos4210_pmu.o
>  obj-$(CONFIG_IMX) += imx_ccm.o
>  obj-$(CONFIG_IMX) += imx31_ccm.o
> +obj-$(CONFIG_IMX) += imx25_ccm.o
>  obj-$(CONFIG_MILKYMIST) += milkymist-hpdmc.o
>  obj-$(CONFIG_MILKYMIST) += milkymist-pfpu.o
>  obj-$(CONFIG_MAINSTONE) += mst_fpga.o
> diff --git a/hw/misc/imx25_ccm.c b/hw/misc/imx25_ccm.c
> new file mode 100644
> index 0000000..e76292d
> --- /dev/null
> +++ b/hw/misc/imx25_ccm.c
> @@ -0,0 +1,228 @@
> +/*
> + * IMX25 Clock Control Module
> + *
> + * Copyright (C) 2012 NICTA
> + * Updated by Jean-Christophe Dubois <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + * To get the timer frequencies right, we need to emulate at least part of
> + * the CCM.
> + */
> +
> +#include "hw/misc/imx25_ccm.h"
> +
> +#ifndef DEBUG_IMX25_CCM
> +#define DEBUG_IMX25_CCM 0
> +#endif
> +
> +#define DPRINTF(fmt, args...) \
> +    do { \
> +        if (DEBUG_IMX25_CCM) { \
> +            fprintf(stderr, "[%s]%s: " fmt , TYPE_IMX25_CCM, \
> +                                             __func__, ##args); \
> +        } \
> +    } while (0)
> +
> +#define CKIH_FREQ 24000000 /* 24MHz crystal input */
> +

Is the crystal freq really decided by the controller? Should this be
on the board level instead? Same applies to prev patch.

> +static int imx25_ccm_post_load(void *opaque, int version_id);
> +

Can you just reorder to remove the forward reference?

> +static const VMStateDescription vmstate_imx25_ccm = {
> +    .name = TYPE_IMX25_CCM,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT32(mpctl, IMX25CCMState),
> +        VMSTATE_UINT32(upctl, IMX25CCMState),
> +        VMSTATE_UINT32(cctl, IMX25CCMState),
> +        VMSTATE_UINT32_ARRAY(cgcr, IMX25CCMState, 3),
> +        VMSTATE_UINT32_ARRAY(pcdr, IMX25CCMState, 4),
> +        VMSTATE_UINT32(rcsr, IMX25CCMState),
> +        VMSTATE_UINT32(crdr, IMX25CCMState),
> +        VMSTATE_UINT32_ARRAY(dcvr, IMX25CCMState, 4),
> +        VMSTATE_UINT32_ARRAY(ltr, IMX25CCMState, 4),
> +        VMSTATE_UINT32_ARRAY(ltbr, IMX25CCMState, 2),
> +        VMSTATE_UINT32_ARRAY(pmcr, IMX25CCMState, 3),
> +        VMSTATE_UINT32(mcr, IMX25CCMState),
> +        VMSTATE_UINT32_ARRAY(lpimr, IMX25CCMState, 2),
> +        VMSTATE_UINT32(pll_refclk_freq, IMX25CCMState),
> +        VMSTATE_END_OF_LIST()
> +    },
> +    .post_load = imx25_ccm_post_load,
> +};
> +
> +static void imx25_ccm_update_clocks(IMX25CCMState *s)
> +{
> +    DPRINTF("\n");
> +    /*
> +     * If we ever emulate more clocks, this should switch to a data-driven
> +     * approach
> +     */
> +
> +    /* Our input CLK */
> +    s->pll_refclk_freq = CKIH_FREQ;
> +
> +    /* Set MCU PLL CLK */
> +    if (EXTRACT(s->cctl, MPLL_BYPASS)) {
> +        imx_ccm_set_clock_frequency(CLK_MPLL, s->pll_refclk_freq);
> +    } else {
> +        imx_ccm_set_clock_frequency(CLK_MPLL,
> +                                    imx_ccm_calc_pll(s->mpctl,
> +                                                     s->pll_refclk_freq));
> +    }
> +
> +    /* Set USB PLL CLK */
> +    imx_ccm_set_clock_frequency(CLK_UPLL,
> +                                imx_ccm_calc_pll(s->upctl,
> +                                                 s->pll_refclk_freq));
> +
> +    /* Set Processor clock */
> +    if (EXTRACT(s->cctl, ARM_SRC)) {
> +        imx_ccm_set_clock_frequency(CLK_MCU,
> +                                    (imx_ccm_get_clock_frequency(CLK_MPLL) * 
> 3
> +                                     / 4)
> +                                     / (1 + EXTRACT(s->cctl, ARM_CLK_DIV)));
> +    } else {
> +        imx_ccm_set_clock_frequency(CLK_MCU,
> +                                    imx_ccm_get_clock_frequency(CLK_MPLL) /
> +                                    (1 + EXTRACT(s->cctl, ARM_CLK_DIV)));
> +    }
> +
> +    /* Set AHB CLK */
> +    imx_ccm_set_clock_frequency(CLK_AHB, 
> imx_ccm_get_clock_frequency(CLK_MCU) /
> +                                         (1 + EXTRACT(s->cctl, 
> AHB_CLK_DIV)));
> +
> +    /* Set IPG CLK */
> +    imx_ccm_set_clock_frequency(CLK_IPG, 
> imx_ccm_get_clock_frequency(CLK_AHB) /
> +                                         2);
> +
> +    /* Set 32K cristal CLK */
> +    imx_ccm_set_clock_frequency(CLK_32k, CKIL_FREQ);
> +
> +}
> +
> +static void imx25_ccm_reset(DeviceState *dev)
> +{
> +    IMX25CCMState *s = IMX25_CCM(dev);
> +
> +    DPRINTF("\n");
> +
> +    s->mpctl = 0x800b2c01;
> +    s->upctl = 0x84002800;
> +    s->cctl = 0x40030000;
> +    s->cgcr[0] = 0x028A0100;
> +    s->cgcr[1] = 0x04008100;
> +    s->cgcr[2] = 0x00000438;
> +    s->pcdr[0] = 0x01010101;
> +    s->pcdr[1] = 0x01010101;
> +    s->pcdr[2] = 0x01010101;
> +    s->pcdr[3] = 0x01010101;
> +    s->rcsr = 0;
> +    s->crdr = 0;
> +    s->dcvr[0] = 0;
> +    s->dcvr[1] = 0;
> +    s->dcvr[2] = 0;
> +    s->dcvr[3] = 0;
> +    s->ltr[0] = 0;
> +    s->ltr[1] = 0;
> +    s->ltr[2] = 0;
> +    s->ltr[3] = 0;
> +    s->ltbr[0] = 0;
> +    s->ltbr[1] = 0;
> +    s->pmcr[0] = 0x00A00000;
> +    s->pmcr[1] = 0x0000A030;
> +    s->pmcr[2] = 0x0000A030;
> +    s->mcr = 0x43000000;
> +    s->lpimr[0] = 0;
> +    s->lpimr[1] = 0;
> +
> +    /* default ROM Boot will change the reset values */

"boot"

> +    s->cctl |= INSERT(1, ARM_SRC);
> +    s->cctl |= INSERT(1, AHB_CLK_DIV);
> +
> +    imx25_ccm_update_clocks(s);
> +}
> +
> +static uint64_t imx25_ccm_read(void *opaque, hwaddr offset,
> +                                unsigned size)
> +{
> +    IMX25CCMState *s = (IMX25CCMState *)opaque;
> +    uint32_t *reg = &s->mpctl;

I think it is just easier to just make it an array in state, and index
into it with macros for the non-array uses. Then you can also memset-0
in reset and just set the non-zeros afterwards.

> +
> +    DPRINTF("(offset=0x%" HWADDR_PRIx ")\n", offset);
> +
> +    if (offset >= 0x70) {
> +        return 0;
> +    } else {
> +        return reg[offset >> 2];
> +    }
> +}
> +
> +static void imx25_ccm_write(void *opaque, hwaddr offset,
> +                          uint64_t value, unsigned size)
> +{
> +    IMX25CCMState *s = (IMX25CCMState *)opaque;
> +    uint32_t *reg = &s->mpctl;
> +
> +    DPRINTF("(offset=0x%" HWADDR_PRIx ", value = %x)\n",
> +            offset, (unsigned int)value);
> +
> +    if (offset < 0x70) {
> +        reg[offset >> 2] = value;
> +    }
> +
> +    imx25_ccm_update_clocks(s);
> +}
> +
> +static const struct MemoryRegionOps imx25_ccm_ops = {
> +    .read = imx25_ccm_read,
> +    .write = imx25_ccm_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static int imx25_ccm_init(SysBusDevice *dev)
> +{
> +    IMX25CCMState *s = IMX25_CCM(dev);
> +
> +    memory_region_init_io(&s->iomem, OBJECT(dev), &imx25_ccm_ops, s,
> +                          TYPE_IMX25_CCM, 0x1000);
> +    sysbus_init_mmio(dev, &s->iomem);
> +
> +    return 0;
> +}
> +
> +static int imx25_ccm_post_load(void *opaque, int version_id)
> +{
> +    IMX25CCMState *s = (IMX25CCMState *)opaque;
> +
> +    imx25_ccm_update_clocks(s);
> +
> +    return 0;
> +}
> +
> +static void imx25_ccm_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    SysBusDeviceClass *sbc = SYS_BUS_DEVICE_CLASS(klass);
> +
> +    sbc->init = imx25_ccm_init;

Sysbus device init is deprecated, use object instance_init and device
realize instead. I think this is hangover from the i.MX31 that is
unconverted. A possible follow up patch is to do the conversion for 31
based on this change. You have just mr_init_io and sb_init_mmio with
constant sizes, so that can be done with just an instance_init I
think.

> +    dc->reset = imx25_ccm_reset;
> +    dc->vmsd = &vmstate_imx25_ccm;
> +    dc->desc = "i.MX25 Clock Control Module";
> +}
> +
> +static const TypeInfo imx25_ccm_info = {
> +    .name = TYPE_IMX25_CCM,
> +    .parent = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(IMX25CCMState),
> +    .class_init = imx25_ccm_class_init,
> +};
> +
> +static void imx25_ccm_register_types(void)
> +{
> +    type_register_static(&imx25_ccm_info);
> +}
> +
> +type_init(imx25_ccm_register_types)
> diff --git a/include/hw/arm/fsl-imx25.h b/include/hw/arm/fsl-imx25.h
> index 5c62fde..d0e8e9d 100644
> --- a/include/hw/arm/fsl-imx25.h
> +++ b/include/hw/arm/fsl-imx25.h
> @@ -19,7 +19,7 @@
>
>  #include "hw/arm/arm.h"
>  #include "hw/intc/imx_avic.h"
> -#include "hw/misc/imx31_ccm.h"
> +#include "hw/misc/imx25_ccm.h"
>  #include "hw/char/imx_serial.h"
>  #include "hw/timer/imx_gpt.h"
>  #include "hw/timer/imx_epit.h"
> @@ -44,7 +44,7 @@ typedef struct FslIMX25State {
>      /*< public >*/
>      ARMCPU         cpu;
>      IMXAVICState   avic;
> -    IMX31CCMState  ccm;
> +    IMX25CCMState  ccm;
>      IMXSerialState uart[FSL_IMX25_NUM_UARTS];
>      IMXGPTState    gpt[FSL_IMX25_NUM_GPTS];
>      IMXEPITState   epit[FSL_IMX25_NUM_EPITS];
> diff --git a/include/hw/misc/imx25_ccm.h b/include/hw/misc/imx25_ccm.h
> new file mode 100644
> index 0000000..20100ca
> --- /dev/null
> +++ b/include/hw/misc/imx25_ccm.h
> @@ -0,0 +1,61 @@
> +/*
> + * IMX25 Clock Control Module
> + *
> + * Copyright (C) 2012 NICTA
> + * Updated by Jean-Christophe Dubois <address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#ifndef IMX25_CCM_H
> +#define IMX25_CCM_H
> +
> +#include "hw/sysbus.h"
> +#include "hw/misc/imx_ccm.h"
> +
> +/* CCTL */
> +#define CCTL_ARM_CLK_DIV_SHIFT (30)
> +#define CCTL_ARM_CLK_DIV_MASK  (0x3)
> +#define CCTL_AHB_CLK_DIV_SHIFT (28)
> +#define CCTL_AHB_CLK_DIV_MASK  (0x3)
> +#define CCTL_MPLL_BYPASS_SHIFT (22)
> +#define CCTL_MPLL_BYPASS_MASK  (0x1)
> +#define CCTL_USB_DIV_SHIFT (16)
> +#define CCTL_USB_DIV_MASK  (0x3F)
> +#define CCTL_ARM_SRC_SHIFT (13)
> +#define CCTL_ARM_SRC_MASK  (0x1)
> +
> +#define EXTRACT(value, name) (((value) >> CCTL_##name##_SHIFT) \
> +                              & CCTL_##name##_MASK)
> +#define INSERT(value, name) (((value) & CCTL_##name##_MASK) << \
> +                             CCTL_##name##_SHIFT)
> +

I have used this approach before, but wanted to make it globally
available, this might be relevant:

https://lists.nongnu.org/archive/html/qemu-devel/2015-04/msg03614.html

We could split off just the macros from that patch into a new
register.h (basically make that P4 a patch in its own right).

Regards,
Peter

> +#define TYPE_IMX25_CCM "imx25.ccm"
> +#define IMX25_CCM(obj) OBJECT_CHECK(IMX25CCMState, (obj), TYPE_IMX25_CCM)
> +
> +typedef struct IMX25CCMState {
> +    /* <private> */
> +    SysBusDevice parent_obj;
> +
> +    /* <public> */
> +    MemoryRegion iomem;
> +
> +    uint32_t mpctl;
> +    uint32_t upctl;
> +    uint32_t cctl;
> +    uint32_t cgcr[3];
> +    uint32_t pcdr[4];
> +    uint32_t rcsr;
> +    uint32_t crdr;
> +    uint32_t dcvr[4];
> +    uint32_t ltr[4];
> +    uint32_t ltbr[2];
> +    uint32_t pmcr[3];
> +    uint32_t mcr;
> +    uint32_t lpimr[2];
> +
> +    uint32_t pll_refclk_freq;
> +} IMX25CCMState;
> +
> +#endif /* IMX25_CCM_H */
> --
> 2.5.0
>