qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v9 06/20] i.MX: Split CCM emulator in a header f


From: Peter Crosthwaite
Subject: Re: [Qemu-devel] [PATCH v9 06/20] i.MX: Split CCM emulator in a header file and a source file
Date: Sat, 4 Jul 2015 21:17:13 -0700

You need a commit message body. In trival ones like this I usally just
repeat the what with some why.

"Split CCM state struct and register defs into a header file to
prepare support for use in a SoC device"

On Sat, Jul 4, 2015 at 7:34 AM, Jean-Christophe Dubois
<address@hidden> wrote:
> Signed-off-by: Jean-Christophe Dubois <address@hidden>
> ---
>
> Changes since v1:
>     * not present on v1
>
> Changes since v2:
>     * not present on v2
>
> Changes since v3:
>     * not present on v3
>
> Changes since v4:
>     * not present on v4
>
> Changes since v5:
>     * not present on v5
>
> Changes since v6:
>     * not present on v6
>
> Changes since v7:
>     * Splited the i.MX CCM emulator into a header file and a source file
>
> Changes since v8:
>     * no change
>
>  hw/misc/imx_ccm.c         | 70 ++----------------------------------
>  include/hw/arm/imx.h      | 10 ------
>  include/hw/misc/imx_ccm.h | 91 
> +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 94 insertions(+), 77 deletions(-)
>  create mode 100644 include/hw/misc/imx_ccm.h
>
> diff --git a/hw/misc/imx_ccm.c b/hw/misc/imx_ccm.c
> index 0920288..2e9bd9c 100644
> --- a/hw/misc/imx_ccm.c
> +++ b/hw/misc/imx_ccm.c
> @@ -2,6 +2,7 @@
>   * IMX31 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.
> @@ -10,10 +11,7 @@
>   * the CCM.
>   */
>
> -#include "hw/hw.h"
> -#include "hw/sysbus.h"
> -#include "sysemu/sysemu.h"
> -#include "hw/arm/imx.h"
> +#include "hw/misc/imx_ccm.h"
>
>  #define CKIH_FREQ 26000000 /* 26MHz crystal input */
>  #define CKIL_FREQ    32768 /* nominal 32khz clock */
> @@ -29,30 +27,6 @@ do { printf("imx_ccm: " fmt , ##args); } while (0)
>
>  static int imx_ccm_post_load(void *opaque, int version_id);
>
> -#define TYPE_IMX_CCM "imx_ccm"
> -#define IMX_CCM(obj) OBJECT_CHECK(IMXCCMState, (obj), TYPE_IMX_CCM)
> -
> -typedef struct IMXCCMState {
> -    SysBusDevice parent_obj;
> -
> -    MemoryRegion iomem;
> -
> -    uint32_t ccmr;
> -    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;
> -    uint32_t mcu_clk_freq;
> -    uint32_t hsp_clk_freq;
> -    uint32_t ipg_clk_freq;
> -} IMXCCMState;
> -
>  static const VMStateDescription vmstate_imx_ccm = {
>      .name = "imx-ccm",
>      .version_id = 1,
> @@ -72,44 +46,6 @@ static const VMStateDescription vmstate_imx_ccm = {
>      .post_load = imx_ccm_post_load,
>  };
>
> -/* 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)
> -
> -/* PDR0 */
> -#define PDR0_MCU_PODF_SHIFT (0)
> -#define PDR0_MCU_PODF_MASK (0x7)
> -#define PDR0_MAX_PODF_SHIFT (3)
> -#define PDR0_MAX_PODF_MASK (0x7)
> -#define PDR0_IPG_PODF_SHIFT (6)
> -#define PDR0_IPG_PODF_MASK (0x3)
> -#define PDR0_NFC_PODF_SHIFT (8)
> -#define PDR0_NFC_PODF_MASK (0x7)
> -#define PDR0_HSP_PODF_SHIFT (11)
> -#define PDR0_HSP_PODF_MASK (0x7)
> -#define PDR0_PER_PODF_SHIFT (16)
> -#define PDR0_PER_PODF_MASK (0x1f)
> -#define PDR0_CSI_PODF_SHIFT (23)
> -#define PDR0_CSI_PODF_MASK (0x1ff)
> -
> -#define EXTRACT(value, name) (((value) >> PDR0_##name##_PODF_SHIFT) \
> -                              & PDR0_##name##_PODF_MASK)
> -#define INSERT(value, name) (((value) & PDR0_##name##_PODF_MASK) << \
> -                             PDR0_##name##_PODF_SHIFT)
> -/* PLL control registers */
> -#define PD(v) (((v) >> 26) & 0xf)
> -#define MFD(v) (((v) >> 16) & 0x3ff)
> -#define MFI(v) (((v) >> 10) & 0xf);
> -#define MFN(v) ((v) & 0x3ff)
> -
> -#define PLL_PD(x)               (((x) & 0xf) << 26)
> -#define PLL_MFD(x)              (((x) & 0x3ff) << 16)
> -#define PLL_MFI(x)              (((x) & 0xf) << 10)
> -#define PLL_MFN(x)              (((x) & 0x3ff) << 0)
> -
>  uint32_t imx_clock_frequency(DeviceState *dev, IMXClk clock)
>  {
>      IMXCCMState *s = IMX_CCM(dev);
> @@ -286,7 +222,7 @@ static int imx_ccm_init(SysBusDevice *dev)
>      IMXCCMState *s = IMX_CCM(dev);
>
>      memory_region_init_io(&s->iomem, OBJECT(dev), &imx_ccm_ops, s,
> -                          "imx_ccm", 0x1000);
> +                          TYPE_IMX_CCM, 0x1000);
>      sysbus_init_mmio(dev, &s->iomem);
>
>      return 0;
> diff --git a/include/hw/arm/imx.h b/include/hw/arm/imx.h
> index c39f112..6cb90cc 100644
> --- a/include/hw/arm/imx.h
> +++ b/include/hw/arm/imx.h
> @@ -11,16 +11,6 @@
>  #ifndef IMX_H
>  #define IMX_H
>
> -typedef enum  {
> -    NOCLK,
> -    MCU,
> -    HSP,
> -    IPG,
> -    CLK_32k
> -} IMXClk;
> -
> -uint32_t imx_clock_frequency(DeviceState *s, IMXClk clock);
> -
>  void imx_timerp_create(const hwaddr addr,
>                        qemu_irq irq,
>                        DeviceState *ccm);
> diff --git a/include/hw/misc/imx_ccm.h b/include/hw/misc/imx_ccm.h
> new file mode 100644
> index 0000000..7febafd
> --- /dev/null
> +++ b/include/hw/misc/imx_ccm.h
> @@ -0,0 +1,91 @@
> +/*
> + * IMX31 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 IMX_CCM_H
> +#define IMX_CCM_H
> +
> +#include "hw/sysbus.h"
> +
> +/* 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)
> +
> +/* PDR0 */
> +#define PDR0_MCU_PODF_SHIFT (0)
> +#define PDR0_MCU_PODF_MASK (0x7)
> +#define PDR0_MAX_PODF_SHIFT (3)
> +#define PDR0_MAX_PODF_MASK (0x7)
> +#define PDR0_IPG_PODF_SHIFT (6)
> +#define PDR0_IPG_PODF_MASK (0x3)
> +#define PDR0_NFC_PODF_SHIFT (8)
> +#define PDR0_NFC_PODF_MASK (0x7)
> +#define PDR0_HSP_PODF_SHIFT (11)
> +#define PDR0_HSP_PODF_MASK (0x7)
> +#define PDR0_PER_PODF_SHIFT (16)
> +#define PDR0_PER_PODF_MASK (0x1f)
> +#define PDR0_CSI_PODF_SHIFT (23)
> +#define PDR0_CSI_PODF_MASK (0x1ff)
> +
> +#define EXTRACT(value, name) (((value) >> PDR0_##name##_PODF_SHIFT) \
> +                              & PDR0_##name##_PODF_MASK)
> +#define INSERT(value, name) (((value) & PDR0_##name##_PODF_MASK) << \
> +                             PDR0_##name##_PODF_SHIFT)
> +
> +/* PLL control registers */
> +#define PD(v) (((v) >> 26) & 0xf)
> +#define MFD(v) (((v) >> 16) & 0x3ff)
> +#define MFI(v) (((v) >> 10) & 0xf);
> +#define MFN(v) ((v) & 0x3ff)
> +
> +#define PLL_PD(x)               (((x) & 0xf) << 26)
> +#define PLL_MFD(x)              (((x) & 0x3ff) << 16)
> +#define PLL_MFI(x)              (((x) & 0xf) << 10)
> +#define PLL_MFN(x)              (((x) & 0x3ff) << 0)
> +
> +#define TYPE_IMX_CCM "imx.ccm"

You should mention this addition in commit message.

> +#define IMX_CCM(obj) OBJECT_CHECK(IMXCCMState, (obj), TYPE_IMX_CCM)
> +
> +typedef struct {

Keep the IMXCCMState from original code. Most typedef struct defs do
the double define.

"Add missing macro for QOM type name in new header".

Otherwise:

Reviewed-by: Peter Crosthwaite <address@hidden>

Regards,
Peter
> +    /* <private> */
> +    SysBusDevice parent_obj;
> +
> +    /* <public> */
> +    MemoryRegion iomem;
> +
> +    uint32_t ccmr;
> +    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;
> +    uint32_t mcu_clk_freq;
> +    uint32_t hsp_clk_freq;
> +    uint32_t ipg_clk_freq;
> +} IMXCCMState;
> +
> +typedef enum  {
> +    NOCLK,
> +    MCU,
> +    HSP,
> +    IPG,
> +    CLK_32k
> +} IMXClk;
> +
> +uint32_t imx_clock_frequency(DeviceState *s, IMXClk clock);
> +
> +#endif /* IMX_CCM_H */
> --
> 2.1.4
>
>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]