[U-Boot] [PATCH v2 4/8] imx: mx6: ccm: Change the clock settings for i.MX6QP

[Stefano Babic <sbabic@denx.de>]

Hi Peng,

On 11/06/2015 12:30, Peng Fan wrote:
> Since i.MX6QP changes some CCM registers, so modify the clocks settings to
> follow the hardware changes.
> 
> In c files, use runtime check and discard #ifdef.
> 
> A new CONFIG_MX6QP is introduced here and is used for the CCM difference,
> only used in header files for different bits.
> At default CONFIG_MX6Q is enabled along with the CONFIG_MX6QP.
> 
> Signed-off-by: Ye.Li <B37916@freescale.com>
> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
> ---
> 
> Changes v2:
>  1. Remove #ifdef, but use runtime check
>  2. A few bit definitions are introduced in c files, because to other platforms
>     the macro will make compilation fail, also there are no other places refer
>     the bit macro definitions.
> 
>  arch/arm/cpu/armv7/mx6/clock.c           | 33 ++++++++++++++++++++++----------
>  arch/arm/cpu/armv7/mx6/soc.c             |  5 ++++-
>  arch/arm/include/asm/arch-mx6/crm_regs.h | 33 +++++++++++++++++---------------
>  include/configs/mx6_common.h             |  3 +++
>  4 files changed, 48 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/mx6/clock.c b/arch/arm/cpu/armv7/mx6/clock.c
> index ae99945..0d862b2 100644
> --- a/arch/arm/cpu/armv7/mx6/clock.c
> +++ b/arch/arm/cpu/armv7/mx6/clock.c
> @@ -323,10 +323,13 @@ static u32 get_ipg_per_clk(void)
>  	u32 reg, perclk_podf;
>  
>  	reg = __raw_readl(&imx_ccm->cscmr1);
> -#if (defined(CONFIG_MX6SL) || defined(CONFIG_MX6SX))
> -	if (reg & MXC_CCM_CSCMR1_PER_CLK_SEL_MASK)
> -		return MXC_HCLK; /* OSC 24Mhz */
> -#endif
> +	if (is_cpu_type(MXC_CPU_MX6SL) || is_cpu_type(MXC_CPU_MX6SX) ||
> +	    is_mx6dqp()) {
> +#define MXC_CCM_CSCMR1_PER_CLK_SEL_MASK (1 << 6)

I am missing why the define is set here and dropped from crm_regs.h.
This makes the defines split between code (clock.c) and header
(crm_regs.h), and make harder to read and to find them.

I am also missing if the goal to have runtime checked is really reached.
The MX6QuadPlus is (please correct me if I am wrong) pin compatible with
Quad. If it is, we lose the possibility to have a single binary for all
SOC variants that a board can mount.

This is more evident for the code you surround with #ifdef CONFIG_MX6QP
- I mean, it is fully ok if you add new defines to crm_regs.h: they do
not conflict with the old ones. But if you redefine some of them, the
SOC must be decided@compile time. Having a single binary (of course,
for SOC variants where it is possible) is a feture we get with big
efforts and we should not remove it.



> +		if (reg & MXC_CCM_CSCMR1_PER_CLK_SEL_MASK)
> +			return MXC_HCLK; /* OSC 24Mhz */
> +	}
> +
>  	perclk_podf = reg & MXC_CCM_CSCMR1_PERCLK_PODF_MASK;
>  
>  	return get_ipg_clk() / (perclk_podf + 1);
> @@ -337,10 +340,14 @@ static u32 get_uart_clk(void)
>  	u32 reg, uart_podf;
>  	u32 freq = decode_pll(PLL_USBOTG, MXC_HCLK) / 6; /* static divider */
>  	reg = __raw_readl(&imx_ccm->cscdr1);
> -#if (defined(CONFIG_MX6SL) || defined(CONFIG_MX6SX))
> -	if (reg & MXC_CCM_CSCDR1_UART_CLK_SEL)
> -		freq = MXC_HCLK;
> -#endif
> +
> +	if (is_cpu_type(MXC_CPU_MX6SL) || is_cpu_type(MXC_CPU_MX6SX) ||
> +	    is_mx6dqp()) {
> +#define MXC_CCM_CSCDR1_UART_CLK_SEL (1 << 6)
> +		if (reg & MXC_CCM_CSCDR1_UART_CLK_SEL)
> +			freq = MXC_HCLK;
> +	}
> +
>  	reg &= MXC_CCM_CSCDR1_UART_CLK_PODF_MASK;
>  	uart_podf = reg >> MXC_CCM_CSCDR1_UART_CLK_PODF_OFFSET;
>  
> @@ -352,8 +359,14 @@ static u32 get_cspi_clk(void)
>  	u32 reg, cspi_podf;
>  
>  	reg = __raw_readl(&imx_ccm->cscdr2);
> -	reg &= MXC_CCM_CSCDR2_ECSPI_CLK_PODF_MASK;
> -	cspi_podf = reg >> MXC_CCM_CSCDR2_ECSPI_CLK_PODF_OFFSET;
> +	cspi_podf = (reg & MXC_CCM_CSCDR2_ECSPI_CLK_PODF_MASK) >>
> +		     MXC_CCM_CSCDR2_ECSPI_CLK_PODF_OFFSET;
> +
> +	if (is_mx6dqp()) {
> +#define MXC_CCM_CSCDR2_ECSPI_CLK_SEL_MASK (0x1 << 18)
> +		if (reg & MXC_CCM_CSCDR2_ECSPI_CLK_SEL_MASK)
> +			return MXC_HCLK / (cspi_podf + 1);
> +	}
>  
>  	return	decode_pll(PLL_USBOTG, MXC_HCLK) / (8 * (cspi_podf + 1));
>  }
> diff --git a/arch/arm/cpu/armv7/mx6/soc.c b/arch/arm/cpu/armv7/mx6/soc.c
> index 29de624..bcfa2f6 100644
> --- a/arch/arm/cpu/armv7/mx6/soc.c
> +++ b/arch/arm/cpu/armv7/mx6/soc.c
> @@ -335,9 +335,12 @@ static void set_ahb_rate(u32 val)
>  static void clear_mmdc_ch_mask(void)
>  {
>  	struct mxc_ccm_reg *mxc_ccm = (struct mxc_ccm_reg *)CCM_BASE_ADDR;
> +	u32 reg;
> +	reg = readl(&mxc_ccm->ccdr);
>  
>  	/* Clear MMDC channel mask */
> -	writel(0, &mxc_ccm->ccdr);
> +	reg &= ~(MXC_CCM_CCDR_MMDC_CH1_HS_MASK | MXC_CCM_CCDR_MMDC_CH0_HS_MASK);
> +	writel(reg, &mxc_ccm->ccdr);
>  }
>  
>  static void init_bandgap(void)
> diff --git a/arch/arm/include/asm/arch-mx6/crm_regs.h b/arch/arm/include/asm/arch-mx6/crm_regs.h
> index 887d048..2ff1005 100644
> --- a/arch/arm/include/asm/arch-mx6/crm_regs.h
> +++ b/arch/arm/include/asm/arch-mx6/crm_regs.h
> @@ -113,7 +113,7 @@ struct mxc_ccm_reg {
>  #define MXC_CCM_CCR_WB_COUNT_MASK			0x7
>  #define MXC_CCM_CCR_WB_COUNT_OFFSET			(1 << 16)
>  #define MXC_CCM_CCR_COSC_EN				(1 << 12)
> -#ifdef CONFIG_MX6SX
> +#if (defined(CONFIG_MX6SL) || defined(CONFIG_MX6QP))
>  #define MXC_CCM_CCR_OSCNT_MASK				0x7F
>  #else
>  #define MXC_CCM_CCR_OSCNT_MASK				0xFF
> @@ -123,6 +123,9 @@ struct mxc_ccm_reg {
>  /* Define the bits in register CCDR */
>  #define MXC_CCM_CCDR_MMDC_CH1_HS_MASK			(1 << 16)
>  #define MXC_CCM_CCDR_MMDC_CH0_HS_MASK			(1 << 17)
> +#ifdef CONFIG_MX6QP
> +#define MXC_CCM_CCDR_MMDC_CH1_AXI_ROOT_CG	(1 << 18)
> +#endif
>  
>  /* Define the bits in register CSR */
>  #define MXC_CCM_CSR_COSC_READY				(1 << 5)
> @@ -196,7 +199,11 @@ struct mxc_ccm_reg {
>  #define MXC_CCM_CBCMR_GPU3D_CORE_CLK_SEL_MASK		(0x3 << 4)
>  #define MXC_CCM_CBCMR_GPU3D_CORE_CLK_SEL_OFFSET		4
>  #ifndef CONFIG_MX6SX
> +#ifdef CONFIG_MX6QP
> +#define MXC_CCM_CBCMR_PRE_CLK_SEL			(1 << 1)
> +#else
>  #define MXC_CCM_CBCMR_GPU3D_AXI_CLK_SEL			(1 << 1)
> +#endif
>  #define MXC_CCM_CBCMR_GPU2D_AXI_CLK_SEL			(1 << 0)
>  #endif
>  
> @@ -230,7 +237,6 @@ struct mxc_ccm_reg {
>  #define MXC_CCM_CSCMR1_QSPI1_CLK_SEL_OFFSET		7
>  #endif
>  #if (defined(CONFIG_MX6SL) || defined(CONFIG_MX6SX))
> -#define MXC_CCM_CSCMR1_PER_CLK_SEL_MASK			(1 << 6)
>  #define MXC_CCM_CSCMR1_PER_CLK_SEL_OFFSET		6
>  #endif
>  #define MXC_CCM_CSCMR1_PERCLK_PODF_MASK			0x3F
> @@ -244,15 +250,12 @@ struct mxc_ccm_reg {
>  #define MXC_CCM_CSCMR2_ESAI_PRE_SEL_OFFSET		19
>  #define MXC_CCM_CSCMR2_LDB_DI1_IPU_DIV			(1 << 11)
>  #define MXC_CCM_CSCMR2_LDB_DI0_IPU_DIV			(1 << 10)
> -#ifdef CONFIG_MX6SX
> +#if (defined(CONFIG_MX6SX) || defined(CONFIG_MX6QP))
>  #define MXC_CCM_CSCMR2_CAN_CLK_SEL_MASK			(0x3 << 8)
>  #define MXC_CCM_CSCMR2_CAN_CLK_SEL_OFFSET		8
> +#endif
>  #define MXC_CCM_CSCMR2_CAN_CLK_PODF_MASK		(0x3F << 2)
>  #define MXC_CCM_CSCMR2_CAN_CLK_PODF_OFFSET		2
> -#else
> -#define MXC_CCM_CSCMR2_CAN_CLK_SEL_MASK			(0x3F << 2)
> -#define MXC_CCM_CSCMR2_CAN_CLK_SEL_OFFSET		2
> -#endif
>  
>  /* Define the bits in register CSCDR1 */
>  #ifndef CONFIG_MX6SX
> @@ -273,15 +276,7 @@ struct mxc_ccm_reg {
>  #define MXC_CCM_CSCDR1_USBOH3_CLK_PODF_OFFSET		6
>  #define MXC_CCM_CSCDR1_USBOH3_CLK_PODF_MASK		(0x3 << 6)
>  #endif
> -#ifdef CONFIG_MX6SL
> -#define MXC_CCM_CSCDR1_UART_CLK_PODF_MASK		0x1F
> -#define MXC_CCM_CSCDR1_UART_CLK_SEL			(1 << 6)
> -#else
>  #define MXC_CCM_CSCDR1_UART_CLK_PODF_MASK		0x3F
> -#ifdef CONFIG_MX6SX
> -#define MXC_CCM_CSCDR1_UART_CLK_SEL			(1 << 6)
> -#endif
> -#endif
>  #define MXC_CCM_CSCDR1_UART_CLK_PODF_OFFSET		0
>  
>  /* Define the bits in register CS1CDR */
> @@ -316,10 +311,17 @@ struct mxc_ccm_reg {
>  #define MXC_CCM_CS2CDR_ENFC_CLK_PRED_MASK		(0x7 << 18)
>  #define MXC_CCM_CS2CDR_ENFC_CLK_PRED_OFFSET		18
>  #define MXC_CCM_CS2CDR_ENFC_CLK_PRED(v)			(((v) & 0x7) << 18)
> +
> +#ifdef CONFIG_MX6QP
> +#define MXC_CCM_CS2CDR_ENFC_CLK_SEL_MASK		(0x7 << 15)
> +#define MXC_CCM_CS2CDR_ENFC_CLK_SEL_OFFSET		15
> +#define MXC_CCM_CS2CDR_ENFC_CLK_SEL(v)			(((v) & 0x7) << 15)
> +#else
>  #define MXC_CCM_CS2CDR_ENFC_CLK_SEL_MASK		(0x3 << 16)
>  #define MXC_CCM_CS2CDR_ENFC_CLK_SEL_OFFSET		16
>  #define MXC_CCM_CS2CDR_ENFC_CLK_SEL(v)			(((v) & 0x3) << 16)
>  #endif
> +#endif

This is an example about my concerns I tried to explain above.

Best regards,
Stefano Babic



-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================