Re: [Qemu-devel] [PATCH 2/6] i.MX: simplify CCM to only handle clock required by timers.

[Peter Maydell]

On 26 January 2016 at 21:44, Jean-Christophe Dubois <address@hidden> wrote:
> Various i.MX timers (GPT, EPIT, PWM, ...) are only requesting 4 clocks
> from the system.
> * CLK_NONE,
> * CLK_IPG,
> * CLK_IPG_HIGH,
> * CLK_32k
>
> Other "clocks" are not required by the qemu framework so far.

This patch confuses me. You don't say why we're removing a bunch
of code or why it's OK to do that.

> Signed-off-by: Jean-Christophe Dubois <address@hidden>
> ---
>  hw/misc/imx25_ccm.c       | 35 +++++------------------------------
>  hw/misc/imx31_ccm.c       | 38 ++++----------------------------------
>  hw/timer/imx_epit.c       |  8 ++++----
>  hw/timer/imx_gpt.c        | 16 ++++++++--------
>  include/hw/misc/imx_ccm.h | 10 ++--------
>  5 files changed, 23 insertions(+), 84 deletions(-)
>
> diff --git a/hw/misc/imx25_ccm.c b/hw/misc/imx25_ccm.c
> index 23759ca..38599f2 100644
> --- a/hw/misc/imx25_ccm.c
> +++ b/hw/misc/imx25_ccm.c
> @@ -119,20 +119,6 @@ static uint32_t imx25_ccm_get_mpll_clk(IMXCCMState *dev)
>      return freq;
>  }
>
> -static uint32_t imx25_ccm_get_upll_clk(IMXCCMState *dev)
> -{
> -    uint32_t freq = 0;
> -    IMX25CCMState *s = IMX25_CCM(dev);
> -
> -    if (!EXTRACT(s->reg[IMX25_CCM_CCTL_REG], UPLL_DIS)) {
> -        freq = imx_ccm_calc_pll(s->reg[IMX25_CCM_UPCTL_REG], CKIH_FREQ);
> -    }
> -
> -    DPRINTF("freq = %d\n", freq);
> -
> -    return freq;
> -}
> -
>  static uint32_t imx25_ccm_get_mcu_clk(IMXCCMState *dev)
>  {
>      uint32_t freq;
> @@ -181,21 +167,10 @@ static uint32_t 
> imx25_ccm_get_clock_frequency(IMXCCMState *dev, IMXClk clock)
>      DPRINTF("Clock = %d)\n", clock);
>
>      switch (clock) {
> -    case NOCLK:
> -        break;
> -    case CLK_MPLL:
> -        freq = imx25_ccm_get_mpll_clk(dev);
> -        break;
> -    case CLK_UPLL:
> -        freq = imx25_ccm_get_upll_clk(dev);
> -        break;
> -    case CLK_MCU:
> -        freq = imx25_ccm_get_mcu_clk(dev);
> -        break;
> -    case CLK_AHB:
> -        freq = imx25_ccm_get_ahb_clk(dev);
> +    case CLK_NONE:
>          break;
>      case CLK_IPG:
> +    case CLK_IPG_HIGH:
>          freq = imx25_ccm_get_ipg_clk(dev);
>          break;
>      case CLK_32k:
> @@ -221,9 +196,9 @@ static void imx25_ccm_reset(DeviceState *dev)
>      memset(s->reg, 0, IMX25_CCM_MAX_REG * sizeof(uint32_t));
>      s->reg[IMX25_CCM_MPCTL_REG] = 0x800b2c01;
>      s->reg[IMX25_CCM_UPCTL_REG] = 0x84042800;
> -    /*
> +    /*
>       * The value below gives:
> -     * CPU = 133 MHz, AHB = 66,5 MHz, IPG = 33 MHz.
> +     * CPU = 133 MHz, AHB = 66,5 MHz, IPG = 33 MHz.
>       */
>      s->reg[IMX25_CCM_CCTL_REG]  = 0xd0030000;
>      s->reg[IMX25_CCM_CGCR0_REG] = 0x028A0100;
> @@ -240,7 +215,7 @@ static void imx25_ccm_reset(DeviceState *dev)
>
>      /*
>       * default boot will change the reset values to allow:
> -     * CPU = 399 MHz, AHB = 133 MHz, IPG = 66,5 MHz.
> +     * CPU = 399 MHz, AHB = 133 MHz, IPG = 66,5 MHz.
>       * For some reason, this doesn't work. With the value below, linux
>       * detects a 88 MHz IPG CLK instead of 66,5 MHz.
>      s->reg[IMX25_CCM_CCTL_REG]  = 0x20032000;
> diff --git a/hw/misc/imx31_ccm.c b/hw/misc/imx31_ccm.c
> index 47d6ead..3876cc2 100644
> --- a/hw/misc/imx31_ccm.c
> +++ b/hw/misc/imx31_ccm.c
> @@ -111,7 +111,7 @@ static uint32_t imx31_ccm_get_pll_ref_clk(IMXCCMState 
> *dev)
>              if (s->reg[IMX31_CCM_CCMR_REG] & CCMR_FPMF) {
>                  freq *= 1024;
>              }
> -        }
> +        }


What are all these whitespace only changes? If you need to do a
whitespace cleanup please put it in its own patch.

>      case CLK_32k:
> diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c
> index 50bf83c..a67bcb5 100644
> --- a/hw/timer/imx_epit.c
> +++ b/hw/timer/imx_epit.c
> @@ -51,10 +51,10 @@ static char const *imx_epit_reg_name(uint32_t reg)
>   * These are typical.
>   */
>  static const IMXClk imx_epit_clocks[] =  {
> -    NOCLK,    /* 00 disabled */
> -    CLK_IPG,  /* 01 ipg_clk, ~532MHz */
> -    CLK_IPG,  /* 10 ipg_clk_highfreq */
> -    CLK_32k,  /* 11 ipg_clk_32k -- ~32kHz */
> +    CLK_NONE,     /* 00 disabled */
> +    CLK_IPG,      /* 01 ipg_clk, ~532MHz */
> +    CLK_IPG_HIGH, /* 10 ipg_clk_highfreq */
> +    CLK_32k,      /* 11 ipg_clk_32k -- ~32kHz */
>  };
>
>  /*
> diff --git a/hw/timer/imx_gpt.c b/hw/timer/imx_gpt.c
> index b227256..b2f03b0 100644
> --- a/hw/timer/imx_gpt.c
> +++ b/hw/timer/imx_gpt.c
> @@ -80,14 +80,14 @@ static const VMStateDescription vmstate_imx_timer_gpt = {
>  };
>
>  static const IMXClk imx_gpt_clocks[] = {
> -    NOCLK,    /* 000 No clock source */
> -    CLK_IPG,  /* 001 ipg_clk, 532MHz*/
> -    CLK_IPG,  /* 010 ipg_clk_highfreq */
> -    NOCLK,    /* 011 not defined */
> -    CLK_32k,  /* 100 ipg_clk_32k */
> -    NOCLK,    /* 101 not defined */
> -    NOCLK,    /* 110 not defined */
> -    NOCLK,    /* 111 not defined */
> +    CLK_NONE,     /* 000 No clock source */
> +    CLK_IPG,      /* 001 ipg_clk, 532MHz*/
> +    CLK_IPG_HIGH, /* 010 ipg_clk_highfreq */
> +    CLK_NONE,     /* 011 not defined */
> +    CLK_32k,      /* 100 ipg_clk_32k */
> +    CLK_NONE,     /* 101 not defined */
> +    CLK_NONE,     /* 110 not defined */
> +    CLK_NONE,     /* 111 not defined */
>  };

These are just renaming NOCLK to CLK_NONE and fixing formatting?
Again, please don't put that in the same patch as substantive
code changes.

thanks
-- PMM