Re: [Qemu-devel] [PATCH v3 3/3] i.MX: Add an i.MX25 specific CCM class/instance.

[Peter Crosthwaite]

On Thu, Nov 19, 2015 at 12:40 PM, Jean-Christophe Dubois
<address@hidden> wrote:
> Signed-off-by: Jean-Christophe Dubois <address@hidden>
> ---
>
> Changes since v1:
>  * rework loging to match other i.MX drivers
>
> Changes since v2:
>  * We moved to an inheritance QOM scheme
>
>  hw/arm/fsl-imx25.c          |   2 +-
>  hw/misc/Makefile.objs       |   1 +
>  hw/misc/imx25_ccm.c         | 243 
> ++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/arm/fsl-imx25.h  |   4 +-
>  include/hw/misc/imx25_ccm.h |  59 +++++++++++
>  5 files changed, 306 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 5526c22..0aacc91 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 c77f3e3..8a235df 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..a6c9bff
> --- /dev/null
> +++ b/hw/misc/imx25_ccm.c
> @@ -0,0 +1,243 @@
> +/*
> + * 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 */
> +
> +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_END_OF_LIST()
> +    },
> +};
> +
> +static uint32_t imx25_ccm_get_mpll_clk(DeviceState *dev)
> +{
> +    IMX25CCMState *s = IMX25_CCM(dev);
> +
> +    DPRINTF("()\n");
> +
> +    if (EXTRACT(s->cctl, MPLL_BYPASS)) {
> +        return CKIH_FREQ;
> +    } else {
> +        return imx_ccm_calc_pll(s->mpctl, CKIH_FREQ);
> +    }
> +}
> +
> +static uint32_t imx25_ccm_get_upll_clk(DeviceState *dev)
> +{
> +    IMX25CCMState *s = IMX25_CCM(dev);
> +
> +    DPRINTF("()\n");
> +
> +    return imx_ccm_calc_pll(s->upctl, CKIH_FREQ);
> +}
> +
> +static uint32_t imx25_ccm_get_mcu_clk(DeviceState *dev)
> +{
> +    IMX25CCMState *s = IMX25_CCM(dev);
> +
> +    DPRINTF("()\n");
> +
> +    if (EXTRACT(s->cctl, ARM_SRC)) {
> +        return (imx25_ccm_get_mpll_clk(dev) * 3 / 4) /
> +               (1 + EXTRACT(s->cctl, ARM_CLK_DIV));
> +    } else {
> +        return imx25_ccm_get_mpll_clk(dev) /
> +               (1 + EXTRACT(s->cctl, ARM_CLK_DIV));
> +    }
> +}
> +
> +static uint32_t imx25_ccm_get_ahb_clk(DeviceState *dev)
> +{
> +    IMX25CCMState *s = IMX25_CCM(dev);
> +
> +    DPRINTF("()\n");
> +
> +    return imx25_ccm_get_mcu_clk(dev) / (1 + EXTRACT(s->cctl, AHB_CLK_DIV));
> +}
> +
> +static uint32_t imx25_ccm_get_ipg_clk(DeviceState *dev)
> +{
> +    DPRINTF("()\n");
> +
> +    return imx25_ccm_get_ahb_clk(dev) / 2;
> +}
> +
> +static uint32_t imx25_ccm_get_clock_frequency(DeviceState *dev, IMXClk clock)
> +{
> +    DPRINTF("Clock = %d)\n", clock);

This would be more useful if it was at the end and grabbed the return
value too (similar to what's commonly done for an MMIO read function).

> +
> +    switch (clock) {
> +    case NOCLK:
> +        return 0;
> +    case CLK_MPLL:
> +        return imx25_ccm_get_mpll_clk(dev);
> +    case CLK_UPLL:
> +        return imx25_ccm_get_upll_clk(dev);
> +    case CLK_MCU:
> +        return imx25_ccm_get_mcu_clk(dev);
> +    case CLK_AHB:
> +        return imx25_ccm_get_ahb_clk(dev);
> +    case CLK_IPG:
> +        return imx25_ccm_get_ipg_clk(dev);
> +    case CLK_32k:
> +        return CKIL_FREQ;
> +    default:
> +        return 0;

Is this condition guest creatable? If so should it be LOG_GUEST_ERROR?
If it is supposed to be unreachable even by guest error, then it
should g_assert_not_reached().

> +    }
> +}
> +
> +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;


With the union-struct approach (see below), you can memset 0 the whole
thing then set the non-zeros after for a shorter result.

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

If you want the best of both worlds for the register indexing scheme
(e.g. array + set of names) an anonymous union-struct would work
better. There are some in tree precedents. E.g. from hcd-ehci.h:

    union {
        uint32_t opreg[0x44/sizeof(uint32_t)];
        struct {
            uint32_t usbcmd;
            uint32_t usbsts;
            uint32_t usbintr;
            uint32_t frindex;
            uint32_t ctrldssegment;
            uint32_t periodiclistbase;
            uint32_t asynclistaddr;
            uint32_t notused[9];
            uint32_t configflag;
        };
    };


> +
> +    DPRINTF("(offset=0x%" HWADDR_PRIx ")\n", offset);
> +

Delay to give return value as well.

> +    if (offset < 0x70) {
> +        return reg[offset >> 2];
> +    } else {
> +        return 0;
> +    }
> +}
> +
> +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);

The cast of value should be to an IP meaningful type. As reg's are 32
bit, it should be a uint32_t with PRIx32.

> +
> +    if (offset < 0x70) {
> +        /*
> +         * We will do a better implementation later. In particular some bits
> +         * cannot be writen to.

"written"

> +         */
> +        reg[offset >> 2] = value;
> +    }
> +}
> +
> +static const struct MemoryRegionOps imx25_ccm_ops = {
> +    .read = imx25_ccm_read,
> +    .write = imx25_ccm_write,

Your IP doesn't handle sub-word access, so should the size restrictions be here?

We have a new sub-mailing list for ARM related work.
address@hidden You can CC to that (as well as qemu-devel) for
the i.MX stuff to get a wider ARM review community.

Thanks.

Regards,
Peter

> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +
> +static void imx25_ccm_init(Object *obj)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +    SysBusDevice *sd = SYS_BUS_DEVICE(obj);
> +    IMX25CCMState *s = IMX25_CCM(obj);
> +
> +    memory_region_init_io(&s->iomem, OBJECT(dev), &imx25_ccm_ops, s,
> +                          TYPE_IMX25_CCM, 0x1000);
> +    sysbus_init_mmio(sd, &s->iomem);
> +}
> +