Re: [Qemu-devel] [PATCH 3/6] i.MX: Add i.MX6 CCM and ANALOG device.

[Peter Maydell]

On 26 January 2016 at 21:45, Jean-Christophe Dubois <address@hidden> wrote:
> Signed-off-by: Jean-Christophe Dubois <address@hidden>

> +static uint32_t imx6_analog_get_pll2_clk(IMX6CCMState *dev)
> +{
> +    uint32_t freq = 24000000;
> +
> +    if (EXTRACT(dev->analog[CCM_ANALOG_PLL_SYS], DIV_SELECT)) {
> +        freq *= 22;
> +    } else {
> +        freq *= 20;
> +    }
> +
> +    DPRINTF("freq = %d\n", freq);
> +
> +    return freq;
> +}
> +
> +static uint32_t imx6_analog_get_pll2_pfd0_clk(IMX6CCMState *dev)
> +{
> +    uint32_t freq = 0;
> +
> +    freq = imx6_analog_get_pll2_clk(dev) * 18
> +           / EXTRACT(dev->analog[CCM_ANALOG_PFD_528], PFD0_FRAC);

I think this multiplication can overflow. Are you sure you want
to do it at 32 bits?

(24000000 == 0x16E3600. 24000000 * 22 * 18 == 0x2367B8800.)

> +
> +    DPRINTF("freq = %d\n", freq);
> +
> +    return freq;
> +}
> +
> +static uint32_t imx6_analog_get_pll2_pfd2_clk(IMX6CCMState *dev)
> +{
> +    uint32_t freq = 0;
> +
> +    freq = imx6_analog_get_pll2_clk(dev) * 18
> +           / EXTRACT(dev->analog[CCM_ANALOG_PFD_528], PFD2_FRAC);

Ditto.

> +
> +    DPRINTF("freq = %d\n", freq);

All these debug printfs are identical, which seems to me like
it would make them not terribly useful, but I'm not the one that
would be debugging this :-)

> +
> +    return freq;
> +}
> +
> +static uint32_t imx6_analog_get_periph_clk(IMX6CCMState *dev)
> +{
> +    uint32_t freq = 0;
> +
> +    switch (EXTRACT(dev->ccm[CCM_CBCMR], PRE_PERIPH_CLK_SEL)) {
> +    case 0:
> +        freq = imx6_analog_get_pll2_clk(dev);
> +        break;
> +    case 1:
> +        freq = imx6_analog_get_pll2_pfd2_clk(dev);
> +        break;
> +    case 2:
> +        freq = imx6_analog_get_pll2_pfd0_clk(dev);
> +        break;
> +    case 3:
> +        freq = imx6_analog_get_pll2_pfd2_clk(dev) / 2;
> +        break;
> +    default:
> +        /* We should never get there */
> +        qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: unexpected clock selector\n",
> +                      TYPE_IMX6_CCM, __func__);

PRE_PERIPH_CLK_SEL is a two bit field, right? So this is actually
unreachable, not just "only reachable if guest misprograms a register".
That should be g_assert_not_reached().

> +        break;
> +    }
> +
> +    DPRINTF("freq = %d\n", freq);
> +
> +    return freq;
> +}
> +
> +static uint32_t imx6_ccm_get_ahb_clk(IMX6CCMState *dev)
> +{
> +    uint32_t freq = 0;
> +
> +    freq = imx6_analog_get_periph_clk(dev)
> +           / (1 + EXTRACT(dev->ccm[CCM_CBCDR], AHB_PODF));;

Stray extra ';'.

> +
> +    DPRINTF("freq = %d\n", freq);
> +
> +    return freq;
> +}
> +
> +static uint32_t imx6_ccm_get_ipg_clk(IMX6CCMState *dev)
> +{
> +    uint32_t freq = 0;
> +
> +    freq = imx6_ccm_get_ahb_clk(dev)
> +           / (1 + EXTRACT(dev->ccm[CCM_CBCDR], IPG_PODF));;
> +
> +    DPRINTF("freq = %d\n", freq);
> +
> +    return freq;
> +}
> +
> +static uint32_t imx6_ccm_get_per_clk(IMX6CCMState *dev)
> +{
> +    uint32_t freq = 0;
> +
> +    freq = imx6_ccm_get_ipg_clk(dev)
> +           / (1 + EXTRACT(dev->ccm[CCM_CSCMR1], PERCLK_PODF));
> +
> +    DPRINTF("freq = %d\n", freq);
> +
> +    return freq;
> +}
> +
> +static uint32_t imx6_ccm_get_clock_frequency(IMXCCMState *dev, IMXClk clock)
> +{
> +    uint32_t freq = 0;
> +    IMX6CCMState *s = IMX6_CCM(dev);
> +
> +    switch (clock) {
> +    case CLK_NONE:
> +        break;
> +    case CLK_IPG:
> +        freq = imx6_ccm_get_ipg_clk(s);
> +        break;
> +    case CLK_IPG_HIGH:
> +        freq = imx6_ccm_get_per_clk(s);
> +        break;
> +    case CLK_32k:
> +        freq = CKIL_FREQ;
> +        break;
> +    default:
> +        qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: unsupported clock %d\n",
> +                      TYPE_IMX6_CCM, __func__, clock);
> +        break;
> +    }
> +
> +    DPRINTF("Clock = %d) = %d\n", clock, freq);
> +
> +    return freq;
> +}
> +
> +static void imx6_ccm_reset(DeviceState *dev)
> +{
> +    IMX6CCMState *s = IMX6_CCM(dev);
> +
> +    DPRINTF("\n");

What's this for?

> +
> +    s->ccm[CCM_CCR] = 0x040116FF;
> +    s->ccm[CCM_CCDR] = 0x00000000;
> +    s->ccm[CCM_CSR] = 0x00000010;
> +    s->ccm[CCM_CCSR] = 0x00000100;
> +    s->ccm[CCM_CACRR] = 0x00000000;
> +    s->ccm[CCM_CBCDR] = 0x00018D40;
> +    s->ccm[CCM_CBCMR] = 0x00022324;
> +    s->ccm[CCM_CSCMR1] = 0x00F00000;
> +    s->ccm[CCM_CSCMR2] = 0x02B92F06;
> +    s->ccm[CCM_CSCDR1] = 0x00490B00;
> +    s->ccm[CCM_CS1CDR] = 0x0EC102C1;
> +    s->ccm[CCM_CS2CDR] = 0x000736C1;
> +    s->ccm[CCM_CDCDR] = 0x33F71F92;
> +    s->ccm[CCM_CHSCCDR] = 0x0002A150;
> +    s->ccm[CCM_CSCDR2] = 0x0002A150;
> +    s->ccm[CCM_CSCDR3] = 0x00014841;
> +    s->ccm[CCM_CDHIPR] = 0x00000000;
> +    s->ccm[CCM_CTOR] = 0x00000000;
> +    s->ccm[CCM_CLPCR] = 0x00000079;
> +    s->ccm[CCM_CISR] = 0x00000000;
> +    s->ccm[CCM_CIMR] = 0xFFFFFFFF;
> +    s->ccm[CCM_CCOSR] = 0x000A0001;
> +    s->ccm[CCM_CGPR] = 0x0000FE62;
> +    s->ccm[CCM_CCGR0] = 0xFFFFFFFF;
> +    s->ccm[CCM_CCGR1] = 0xFFFFFFFF;
> +    s->ccm[CCM_CCGR2] = 0xFC3FFFFF;
> +    s->ccm[CCM_CCGR3] = 0xFFFFFFFF;
> +    s->ccm[CCM_CCGR4] = 0xFFFFFFFF;
> +    s->ccm[CCM_CCGR5] = 0xFFFFFFFF;
> +    s->ccm[CCM_CCGR6] = 0xFFFFFFFF;
> +    s->ccm[CCM_CMEOR] = 0xFFFFFFFF;
> +
> +    s->analog[CCM_ANALOG_PLL_ARM] = 0x00013042;
> +    s->analog[CCM_ANALOG_PLL_USB1] = 0x00012000;
> +    s->analog[CCM_ANALOG_PLL_USB2] = 0x00012000;
> +    s->analog[CCM_ANALOG_PLL_SYS] = 0x00013001;
> +    s->analog[CCM_ANALOG_PLL_SYS_SS] = 0x00000000;
> +    s->analog[CCM_ANALOG_PLL_SYS_NUM] = 0x00000000;
> +    s->analog[CCM_ANALOG_PLL_SYS_DENOM] = 0x00000012;
> +    s->analog[CCM_ANALOG_PLL_AUDIO] = 0x00011006;
> +    s->analog[CCM_ANALOG_PLL_AUDIO_NUM] = 0x05F5E100;
> +    s->analog[CCM_ANALOG_PLL_AUDIO_DENOM] = 0x2964619C;
> +    s->analog[CCM_ANALOG_PLL_VIDEO] = 0x0001100C;
> +    s->analog[CCM_ANALOG_PLL_VIDEO_NUM] = 0x05F5E100;
> +    s->analog[CCM_ANALOG_PLL_VIDEO_DENOM] = 0x10A24447;
> +    s->analog[CCM_ANALOG_PLL_MLB] = 0x00010000;
> +    s->analog[CCM_ANALOG_PLL_ENET] = 0x00011001;
> +    s->analog[CCM_ANALOG_PFD_480] = 0x1311100C;
> +    s->analog[CCM_ANALOG_PFD_528] = 0x1018101B;
> +    /*
> +     * This value is overrided by the PMU value
> +    s->analog[CCM_ANALOG_MISC0] = 0x02000000;
> +     */

"overridden", but please don't leave code in but commented out
like this.

> +    s->analog[CCM_ANALOG_MISC2] = 0x00272727;
> +
> +    s->analog[PMU_REG_1P1] = 0x00001073;
> +    s->analog[PMU_REG_3P0] = 0x00000F74;
> +    s->analog[PMU_REG_2P5] = 0x00005071;
> +    s->analog[PMU_REG_CORE] = 0x00402010;
> +    s->analog[PMU_MISC0] = 0x04000000;
> +    s->analog[PMU_MISC1] = 0x00000000;
> +    /*
> +     * Same value as CCM_ANALOG_MISC2
> +    s->analog[PMU_MISC2] = 0x00272727;
> +     */

Ditto.

> +
> +    s->analog[USB_ANALOG_DIGPROG] = 0x00000000;
> +
> +    /* all PLL need to be locked */

"PLLs"

> +    s->analog[CCM_ANALOG_PLL_ARM]   |= CCM_ANALOG_PLL_LOCK;
> +    s->analog[CCM_ANALOG_PLL_USB1]  |= CCM_ANALOG_PLL_LOCK;
> +    s->analog[CCM_ANALOG_PLL_USB2]  |= CCM_ANALOG_PLL_LOCK;
> +    s->analog[CCM_ANALOG_PLL_SYS]   |= CCM_ANALOG_PLL_LOCK;
> +    s->analog[CCM_ANALOG_PLL_AUDIO] |= CCM_ANALOG_PLL_LOCK;
> +    s->analog[CCM_ANALOG_PLL_VIDEO] |= CCM_ANALOG_PLL_LOCK;
> +    s->analog[CCM_ANALOG_PLL_MLB]   |= CCM_ANALOG_PLL_LOCK;
> +    s->analog[CCM_ANALOG_PLL_ENET]  |= CCM_ANALOG_PLL_LOCK;
> +}
> +
> +static uint64_t imx6_ccm_read(void *opaque, uint32 index, unsigned size)

These are only called from within this driver code, so there's
no need to leave the first argument as a void*, you can make
it an IMX6CCMState *.

> +{
> +    uint32 value = 0;
> +    IMX6CCMState *s = (IMX6CCMState *)opaque;
> +
> +    value = s->ccm[index];
> +
> +    DPRINTF("reg[%s] => 0x%" PRIx32 "\n", imx6_ccm_reg_name(index), value);
> +
> +    return (uint64_t)value;
> +}
> +
> +static void imx6_ccm_write(void *opaque, uint32 index, uint64_t value,
> +                           unsigned size)
> +{
> +    IMX6CCMState *s = (IMX6CCMState *)opaque;
> +
> +    DPRINTF("reg[%s] <= 0x%" PRIx32 "\n", imx6_ccm_reg_name(index),
> +            (uint32_t)value);
> +
> +    /*
> +     * We will do a better implementation later. In particular some bits
> +     * cannot be writen to.

"written"

> +     */
> +    s->ccm[index] = value;
> +}
> +
> +static uint64_t imx6_analog_read(void *opaque, uint32_t index, unsigned size)
> +{
> +    uint32_t value;
> +    IMX6CCMState *s = (IMX6CCMState *)opaque;
> +
> +    switch (index) {
> +    case CCM_ANALOG_PLL_ARM_SET:
> +    case CCM_ANALOG_PLL_USB1_SET:
> +    case CCM_ANALOG_PLL_USB2_SET:
> +    case CCM_ANALOG_PLL_SYS_SET:
> +    case CCM_ANALOG_PLL_AUDIO_SET:
> +    case CCM_ANALOG_PLL_VIDEO_SET:
> +    case CCM_ANALOG_PLL_MLB_SET:
> +    case CCM_ANALOG_PLL_ENET_SET:
> +    case CCM_ANALOG_PFD_480_SET:
> +    case CCM_ANALOG_PFD_528_SET:
> +    case CCM_ANALOG_MISC0_SET:
> +    case PMU_MISC1_SET:
> +    case CCM_ANALOG_MISC2_SET:
> +    case USB_ANALOG_USB1_VBUS_DETECT_SET:
> +    case USB_ANALOG_USB1_CHRG_DETECT_SET:
> +    case USB_ANALOG_USB1_MISC_SET:
> +    case USB_ANALOG_USB2_VBUS_DETECT_SET:
> +    case USB_ANALOG_USB2_CHRG_DETECT_SET:
> +    case USB_ANALOG_USB2_MISC_SET:
> +        value = s->analog[index - 1];

I don't understand why these are referencing index -1 and index - 2;
could we have some explanation, please?

> +        break;
> +    case CCM_ANALOG_PLL_ARM_CLR:
> +    case CCM_ANALOG_PLL_USB1_CLR:
> +    case CCM_ANALOG_PLL_USB2_CLR:
> +    case CCM_ANALOG_PLL_SYS_CLR:
> +    case CCM_ANALOG_PLL_AUDIO_CLR:
> +    case CCM_ANALOG_PLL_VIDEO_CLR:
> +    case CCM_ANALOG_PLL_MLB_CLR:
> +    case CCM_ANALOG_PLL_ENET_CLR:
> +    case CCM_ANALOG_PFD_480_CLR:
> +    case CCM_ANALOG_PFD_528_CLR:
> +    case CCM_ANALOG_MISC0_CLR:
> +    case PMU_MISC1_CLR:
> +    case CCM_ANALOG_MISC2_CLR:
> +    case USB_ANALOG_USB1_VBUS_DETECT_CLR:
> +    case USB_ANALOG_USB1_CHRG_DETECT_CLR:
> +    case USB_ANALOG_USB1_MISC_CLR:
> +    case USB_ANALOG_USB2_VBUS_DETECT_CLR:
> +    case USB_ANALOG_USB2_CHRG_DETECT_CLR:
> +    case USB_ANALOG_USB2_MISC_CLR:
> +        value = s->analog[index - 2];
> +        break;
> +    case CCM_ANALOG_PLL_ARM_TOG:
> +    case CCM_ANALOG_PLL_USB1_TOG:
> +    case CCM_ANALOG_PLL_USB2_TOG:
> +    case CCM_ANALOG_PLL_SYS_TOG:
> +    case CCM_ANALOG_PLL_AUDIO_TOG:
> +    case CCM_ANALOG_PLL_VIDEO_TOG:
> +    case CCM_ANALOG_PLL_MLB_TOG:
> +    case CCM_ANALOG_PLL_ENET_TOG:
> +    case CCM_ANALOG_PFD_480_TOG:
> +    case CCM_ANALOG_PFD_528_TOG:
> +    case CCM_ANALOG_MISC0_TOG:
> +    case PMU_MISC1_TOG:
> +    case CCM_ANALOG_MISC2_TOG:
> +    case USB_ANALOG_USB1_VBUS_DETECT_TOG:
> +    case USB_ANALOG_USB1_CHRG_DETECT_TOG:
> +    case USB_ANALOG_USB1_MISC_TOG:
> +    case USB_ANALOG_USB2_VBUS_DETECT_TOG:
> +    case USB_ANALOG_USB2_CHRG_DETECT_TOG:
> +    case USB_ANALOG_USB2_MISC_TOG:
> +        value = s->analog[index - 3];
> +        break;
> +    default:
> +        value = s->analog[index];
> +        break;
> +    }
> +
> +    DPRINTF("reg[%s] => 0x%" PRIx32 "\n", imx6_analog_reg_name(index), 
> value);
> +
> +    return (uint64_t)value;
> +}
> +
> +static void imx6_analog_write(void *opaque, uint32_t index, uint64_t value,
> +                              unsigned size)
> +{
> +    IMX6CCMState *s = (IMX6CCMState *)opaque;
> +
> +    DPRINTF("reg[%s] <= 0x%" PRIx32 "\n", imx6_analog_reg_name(index),
> +            (uint32_t)value);
> +
> +    switch (index) {
> +    case CCM_ANALOG_PLL_ARM_SET:
> +    case CCM_ANALOG_PLL_USB1_SET:
> +    case CCM_ANALOG_PLL_USB2_SET:
> +    case CCM_ANALOG_PLL_SYS_SET:
> +    case CCM_ANALOG_PLL_AUDIO_SET:
> +    case CCM_ANALOG_PLL_VIDEO_SET:
> +    case CCM_ANALOG_PLL_MLB_SET:
> +    case CCM_ANALOG_PLL_ENET_SET:
> +    case CCM_ANALOG_PFD_480_SET:
> +    case CCM_ANALOG_PFD_528_SET:
> +    case CCM_ANALOG_MISC0_SET:
> +    case PMU_MISC1_SET:
> +    case CCM_ANALOG_MISC2_SET:
> +    case USB_ANALOG_USB1_VBUS_DETECT_SET:
> +    case USB_ANALOG_USB1_CHRG_DETECT_SET:
> +    case USB_ANALOG_USB1_MISC_SET:
> +    case USB_ANALOG_USB2_VBUS_DETECT_SET:
> +    case USB_ANALOG_USB2_CHRG_DETECT_SET:
> +    case USB_ANALOG_USB2_MISC_SET:
> +        /* Set bits in ref register */
> +        s->analog[index - 1] |= value;
> +        break;
> +    case CCM_ANALOG_PLL_ARM_CLR:
> +    case CCM_ANALOG_PLL_USB1_CLR:
> +    case CCM_ANALOG_PLL_USB2_CLR:
> +    case CCM_ANALOG_PLL_SYS_CLR:
> +    case CCM_ANALOG_PLL_AUDIO_CLR:
> +    case CCM_ANALOG_PLL_VIDEO_CLR:
> +    case CCM_ANALOG_PLL_MLB_CLR:
> +    case CCM_ANALOG_PLL_ENET_CLR:
> +    case CCM_ANALOG_PFD_480_CLR:
> +    case CCM_ANALOG_PFD_528_CLR:
> +    case CCM_ANALOG_MISC0_CLR:
> +    case PMU_MISC1_CLR:
> +    case CCM_ANALOG_MISC2_CLR:
> +    case USB_ANALOG_USB1_VBUS_DETECT_CLR:
> +    case USB_ANALOG_USB1_CHRG_DETECT_CLR:
> +    case USB_ANALOG_USB1_MISC_CLR:
> +    case USB_ANALOG_USB2_VBUS_DETECT_CLR:
> +    case USB_ANALOG_USB2_CHRG_DETECT_CLR:
> +    case USB_ANALOG_USB2_MISC_CLR:
> +        /* clear bits in ref register */
> +        s->analog[index - 2] &= ~value;
> +        break;
> +    case CCM_ANALOG_PLL_ARM_TOG:
> +    case CCM_ANALOG_PLL_USB1_TOG:
> +    case CCM_ANALOG_PLL_USB2_TOG:
> +    case CCM_ANALOG_PLL_SYS_TOG:
> +    case CCM_ANALOG_PLL_AUDIO_TOG:
> +    case CCM_ANALOG_PLL_VIDEO_TOG:
> +    case CCM_ANALOG_PLL_MLB_TOG:
> +    case CCM_ANALOG_PLL_ENET_TOG:
> +    case CCM_ANALOG_PFD_480_TOG:
> +    case CCM_ANALOG_PFD_528_TOG:
> +    case CCM_ANALOG_MISC0_TOG:
> +    case PMU_MISC1_TOG:
> +    case CCM_ANALOG_MISC2_TOG:
> +    case USB_ANALOG_USB1_VBUS_DETECT_TOG:
> +    case USB_ANALOG_USB1_CHRG_DETECT_TOG:
> +    case USB_ANALOG_USB1_MISC_TOG:
> +    case USB_ANALOG_USB2_VBUS_DETECT_TOG:
> +    case USB_ANALOG_USB2_CHRG_DETECT_TOG:
> +    case USB_ANALOG_USB2_MISC_TOG:
> +        /* toggle bits in ref register */
> +        s->analog[index - 3] ^= value;
> +        break;
> +    default:
> +        /*
> +         * We will do a better implementation later. In particular some bits
> +         * cannot be writen to.
> +         */
> +        s->analog[index] = value;
> +        break;
> +    }
> +}
> +
> +static uint64_t imx6_main_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +    uint64_t value = 0;
> +    uint32_t index = offset >> 2;
> +
> +    if (offset > (0x4000 + CCM_ANALOG_MAX * sizeof(uint32_t))) {
> +        /* This is an unsupported area */
> +        qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad register at offset 0x%"
> +                      HWADDR_PRIx "\n", TYPE_IMX6_CCM, __func__, offset);
> +        DPRINTF("Bad register at offset 0x%" HWADDR_PRIx "\n", offset);
> +    } else if (offset >= 0x4000) {
> +        /* This is an analog register */
> +        value = imx6_analog_read(opaque, index - 0x1000, size);
> +    } else if (offset < (CCM_MAX * sizeof(uint32_t))) {
> +        /* This is a ccm register */
> +        value = imx6_ccm_read(opaque, index, size);

Consider doing this with a set of subregions inside a container,
rather than an if-elseif chain.

> +    } else {
> +        /* This is an unsupported area */
> +        qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad register at offset 0x%"
> +                      HWADDR_PRIx "\n", TYPE_IMX6_CCM, __func__, offset);
> +        DPRINTF("Bad register at offset 0x%" HWADDR_PRIx "\n", offset);

You can reshuffle the if..elseif chain to not have this and the first
if be duplicated code.

> +    }
> +
> +    return value;
> +}

> +#define EXTRACT(value, name) (((value) >> name##_SHIFT) & name##_MASK)
> +#define INSERT(value, name) (((value) & name##_MASK) << name##_SHIFT)

Can we not reinvent the extract32/deposit32 functions, please?
Just use the standard ones in bitops.h.

thanks
-- PMM