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

[Peter Crosthwaite]

On Wed, Oct 28, 2015 at 4:02 PM, Jean-Christophe Dubois
<address@hidden> wrote:
> The CCM drive is in fact specific to the i.MX31. We need to add an i.MX25
> CCM driver.
>
> As a first step, we split the CCM driver into 2 parts:
> 1) A common/utility part that allow to compute an manipulate clock freq for
>    any CCM driver
> 2) The i.MX31 CCM specifc driver.
>
> We also remove EPIT/GPT timer reference to CCM. These objects now use the
> utility function we added in 1) instead of direct reference to CCM object.
>
> Signed-off-by: Jean-Christophe Dubois <address@hidden>
> ---
>
> Changes since v1:
>  * None
>
>  hw/arm/fsl-imx25.c          |   6 +-
>  hw/arm/fsl-imx31.c          |   6 +-
>  hw/misc/Makefile.objs       |   1 +
>  hw/misc/imx31_ccm.c         | 240 ++++++++++++++++++++++++++++++++++++++++++
>  hw/misc/imx_ccm.c           | 249 
> +++-----------------------------------------

This is tricky to review as there is a combination of refactoring as
well as major code motion. To get a diff on this, I split the patch in
two, first with a pure mechanical patch that renames everything "IMX
CCM" related to "IMX31 CCM". The diff on that is then easily reviewed
if the patch is formatted with rename detection on. To setup rename
detection:

git config diff.renames true; git config diff.algorithm patience

With that, the diff on the rest (second patch) is:

$ git diff --stat HEAD..77f8cde33bebe057d668c6e6e2c420ee4e37dbae
 hw/arm/fsl-imx25.c          |  4 ---
 hw/arm/fsl-imx31.c          |  4 ---
 hw/misc/Makefile.objs       |  1 +
 hw/misc/imx31_ccm.c         | 73 +++++++++++++--------------------------------
 hw/misc/imx_ccm.c           | 50 +++++++++++++++++++++++++++++++
 hw/timer/imx_epit.c         | 10 +++----
 hw/timer/imx_gpt.c          | 14 ++++-----
 include/hw/misc/imx31_ccm.h | 25 +---------------
 include/hw/misc/imx_ccm.h   | 48 +++++++++++++++++++++++++++++
 include/hw/timer/imx_epit.h |  1 -
 include/hw/timer/imx_gpt.h  |  1 -
 11 files changed, 133 insertions(+), 98 deletions(-)

I'm reviewing based on what I can see in this new diff. You can get
the split version here:

https://github.com/pcrost/qemu/commits/imx-ccm.next

>  hw/timer/imx_epit.c         |   8 +-
>  hw/timer/imx_gpt.c          |  12 +--
>  include/hw/arm/fsl-imx25.h  |   4 +-
>  include/hw/arm/fsl-imx31.h  |   4 +-
>  include/hw/misc/imx31_ccm.h |  68 ++++++++++++
>  include/hw/misc/imx_ccm.h   |  73 +++----------
>  include/hw/timer/imx_epit.h |   1 -
>  include/hw/timer/imx_gpt.h  |   1 -
>  13 files changed, 354 insertions(+), 319 deletions(-)
>  create mode 100644 hw/misc/imx31_ccm.c
>  create mode 100644 include/hw/misc/imx31_ccm.h
>
> diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
> index e1cadac..620c5c6 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_IMX_CCM);
> +    object_initialize(&s->ccm, sizeof(s->ccm), TYPE_IMX31_CCM);
>      qdev_set_parent_bus(DEVICE(&s->ccm), sysbus_get_default());
>
>      for (i = 0; i < FSL_IMX25_NUM_UARTS; i++) {
> @@ -150,8 +150,6 @@ static void fsl_imx25_realize(DeviceState *dev, Error 
> **errp)
>              { FSL_IMX25_GPT4_ADDR, FSL_IMX25_GPT4_IRQ }
>          };
>
> -        s->gpt[i].ccm = DEVICE(&s->ccm);
> -
>          object_property_set_bool(OBJECT(&s->gpt[i]), true, "realized", &err);
>          if (err) {
>              error_propagate(errp, err);

> +            imx_ccm_get_clock_frequency(CLK_AHB) / 1000000,
> +            imx_ccm_get_clock_frequency(CLK_IPG));
> +}
> +
> +static void imx31_ccm_reset(DeviceState *dev)
> +{
> +    IMX31CCMState *s = IMX31_CCM(dev);
> +
> +    s->ccmr = 0x074b0b7b;
> +    s->pdr0 = 0xff870b48;
> +    s->pdr1 = 0x49fcfe7f;
> +    s->mpctl = PLL_PD(1) | PLL_MFD(0) | PLL_MFI(6) | PLL_MFN(0);
> +    s->cgr[0] = s->cgr[1] = s->cgr[2] = 0xffffffff;
> +    s->spctl = PLL_PD(1) | PLL_MFD(4) | PLL_MFI(0xc) | PLL_MFN(1);
> +    s->pmcr0 = 0x80209828;
> +
> +    update_clocks(s);
> +}
> +
> +static uint64_t imx31_ccm_read(void *opaque, hwaddr offset,
> +                                unsigned size)

Indentation of continued arg list slightly out. Here and for write.

> +{
> +    IMX31CCMState *s = (IMX31CCMState *)opaque;
> +
> +    DPRINTF("(offset=0x%" HWADDR_PRIx ")\n", offset);
> +
> +    switch (offset >> 2) {
> +    case 0: /* CCMR */
> +        DPRINTF(" ccmr = 0x%x\n", s->ccmr);
> +        return s->ccmr;
> +    case 1:
> +        DPRINTF(" pdr0 = 0x%x\n", s->pdr0);
> +        return s->pdr0;
> +    case 2:
> +        DPRINTF(" pdr1 = 0x%x\n", s->pdr1);
> +        return s->pdr1;
> +    case 4:
> +        DPRINTF(" mpctl = 0x%x\n", s->mpctl);
> +        return s->mpctl;
> +    case 6:
> +        DPRINTF(" spctl = 0x%x\n", s->spctl);
> +        return s->spctl;
> +    case 8:
> +        DPRINTF(" cgr0 = 0x%x\n", s->cgr[0]);

> -#define CKIL_FREQ    32768 /* nominal 32khz clock */
> -
> -#ifndef DEBUG_IMX_CCM
> -#define DEBUG_IMX_CCM 0
> -#endif
> -
> -#define DPRINTF(fmt, args...) \
> -    do { \
> -        if (DEBUG_IMX_CCM) { \
> -            fprintf(stderr, "[%s]%s: " fmt , TYPE_IMX_CCM, \
> -                                             __func__, ##args); \
> -        } \
> -    } while (0)
> -
> -static int imx_ccm_post_load(void *opaque, int version_id);
> +static uint32_t imx_ccm_freq_table[CLK_32k+1];

I don't think this can really be a global variable. The SoC as a
non-global object can't assume that it can set this via
ccm_set_clock_frequency. I'm guessing the real issue is that when you
go implement the CCM25, the peripherals don't know which CCM device to
ref. That has a few solutions

1: QOM inheritence. There is an abstract device TYPE_IMX_CCM of which
both TYPE_IMX25_CCM and TYPE_IMX31_CCM inherit. The peripherals then
ref the abstraction, which provides the functionality.

2: QOM interface. There is an interface for the same, which implements
the imx_ccm_get_clock_frequency and set_clock_frequency.
TYPE_ARM_LINUX_BOOT_IF or TYPE_STREAM_SLAVE are two examples of
devices implementing interfaces.

3: The GPIO approach. You can (hackishly) use the full numeric value
of a GPIO (as set by qemu_set_irq) at the sink. This means you could
propagate clock frequencies around a system via GPIOs. I have done
this a fair bit in out-of-tree work.

A formalisation of number 3 would be ideal. This means existing GPIO
support should be either generalised or replicated, or something
in-between; create an abstraction for "signal" which then has
concretes GPIO and clock. Then we have a qemu_set_clock or a qemu_irq
like object.

If you don't want to take that on however (which is reasonable becuase
its a big new core feature), solution #1 or #2 would be ok. #2 feels
right by gut to me.

>
> -static const VMStateDescription vmstate_imx_ccm = {
> -    .name = TYPE_IMX_CCM,
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> -    .fields = (VMStateField[]) {
> -        VMSTATE_UINT32(ccmr, IMXCCMState),
> -        VMSTATE_UINT32(pdr0, IMXCCMState),
> -        VMSTATE_UINT32(pdr1, IMXCCMState),
> -        VMSTATE_UINT32(mpctl, IMXCCMState),
> -        VMSTATE_UINT32(spctl, IMXCCMState),
> -        VMSTATE_UINT32_ARRAY(cgr, IMXCCMState, 3),
> -        VMSTATE_UINT32(pmcr0, IMXCCMState),
> -        VMSTATE_UINT32(pmcr1, IMXCCMState),

> +    uint32_t pdr0;
> +    uint32_t pdr1;
> +    uint32_t mpctl;
> +    uint32_t spctl;
> +    uint32_t cgr[3];
> +    uint32_t pmcr0;
> +    uint32_t pmcr1;
> +
> +    /* Frequencies precalculated on register changes */
> +    uint32_t pll_refclk_freq;
> +} IMX31CCMState;
> +
> +#endif /* IMX31_CCM_H */
> diff --git a/include/hw/misc/imx_ccm.h b/include/hw/misc/imx_ccm.h
> index 0f2e469..d25e450 100644
> --- a/include/hw/misc/imx_ccm.h
> +++ b/include/hw/misc/imx_ccm.h
> @@ -1,5 +1,5 @@
>  /*
> - * IMX31 Clock Control Module
> + * IMX common functions for Clock Control Module
>   *
>   * Copyright (C) 2012 NICTA
>   * Updated by Jean-Christophe Dubois <address@hidden>
> @@ -13,33 +13,7 @@
>
>  #include "hw/sysbus.h"

I think this header can be dropped.

Regards,
Peter

>
> -/* CCMR */
> -#define CCMR_FPME (1<<0)
> -#define CCMR_MPE  (1<<3)
> -#define CCMR_MDS  (1<<7)
> -#define CCMR_FPMF (1<<26)
> -#define CCMR_PRCS (3<<1)