Re: [U-Boot] [PATCH V2 04/23] imx: mx8m: add clock driver

[Fabio Estevam]

On Mon, Dec 4, 2017 at 2:31 AM, Peng Fan  wrote:

> +   switch (frac_pll) {
> +   case ARM_PLL_CLK:
> +   pll_cfg0 = readl(_pll->arm_pll_cfg0);
> +   pll_cfg1 = readl(_pll->arm_pll_cfg1);
> +   pllout_div_shift = HW_FRAC_ARM_PLL_DIV_SHIFT;
> +   pllout_div_mask = HW_FRAC_ARM_PLL_DIV_MASK;
> +   break;
> +   default:
> +   printf("Not supported\n");

Please improve the error message. "Not supported"  is simply too vague.

Also, you use this same error message in several parts of the driver.

Please distinguish between them so that it can make it easier for
people debugging issues to know where exactly the error message comes
from.

> +
> +static u32 decode_sscg_pll(enum clk_root_src sscg_pll)
> +{
> +   u32 pll_cfg0, pll_cfg1, pll_cfg2;
> +   u32 pll_refclk_sel, pll_refclk;
> +   u32 divr1, divr2, divf1, divf2, divq, div;
> +   u32 sse;
> +   u32 pll_clke;
> +   u32 pllout_div_shift, pllout_div_mask, pllout_div;
> +   u32 pllout;
> +
> +   switch (sscg_pll) {
> +   case SYSTEM_PLL1_800M_CLK:
> +   case SYSTEM_PLL1_400M_CLK:
> +   case SYSTEM_PLL1_266M_CLK:
> +   case SYSTEM_PLL1_200M_CLK:
> +   case SYSTEM_PLL1_160M_CLK:
> +   case SYSTEM_PLL1_133M_CLK:
> +   case SYSTEM_PLL1_100M_CLK:
> +   case SYSTEM_PLL1_80M_CLK:
> +   case SYSTEM_PLL1_40M_CLK:
> +   pll_cfg0 = readl(_pll->sys_pll1_cfg0);
> +   pll_cfg1 = readl(_pll->sys_pll1_cfg1);
> +   pll_cfg2 = readl(_pll->sys_pll1_cfg2);
> +   pllout_div_shift = HW_SSCG_SYSTEM_PLL1_DIV_SHIFT;
> +   pllout_div_mask = HW_SSCG_SYSTEM_PLL1_DIV_MASK;
> +   break;
> +   case SYSTEM_PLL2_1000M_CLK:
> +   case SYSTEM_PLL2_500M_CLK:
> +   case SYSTEM_PLL2_333M_CLK:
> +   case SYSTEM_PLL2_250M_CLK:
> +   case SYSTEM_PLL2_200M_CLK:
> +   case SYSTEM_PLL2_166M_CLK:
> +   case SYSTEM_PLL2_125M_CLK:
> +   case SYSTEM_PLL2_100M_CLK:
> +   case SYSTEM_PLL2_50M_CLK:
> +   pll_cfg0 = readl(_pll->sys_pll2_cfg0);
> +   pll_cfg1 = readl(_pll->sys_pll2_cfg1);
> +   pll_cfg2 = readl(_pll->sys_pll2_cfg2);
> +   pllout_div_shift = HW_SSCG_SYSTEM_PLL2_DIV_SHIFT;
> +   pllout_div_mask = HW_SSCG_SYSTEM_PLL2_DIV_MASK;
> +   break;
> +   case SYSTEM_PLL3_CLK:
> +   pll_cfg0 = readl(_pll->sys_pll3_cfg0);
> +   pll_cfg1 = readl(_pll->sys_pll3_cfg1);
> +   pll_cfg2 = readl(_pll->sys_pll3_cfg2);
> +   pllout_div_shift = HW_SSCG_SYSTEM_PLL3_DIV_SHIFT;
> +   pllout_div_mask = HW_SSCG_SYSTEM_PLL3_DIV_MASK;
> +   break;
> +   case DRAM_PLL1_CLK:
> +   pll_cfg0 = readl(_pll->dram_pll_cfg0);
> +   pll_cfg1 = readl(_pll->dram_pll_cfg1);
> +   pll_cfg2 = readl(_pll->dram_pll_cfg2);
> +   pllout_div_shift = HW_SSCG_DRAM_PLL_DIV_SHIFT;
> +   pllout_div_mask = HW_SSCG_DRAM_PLL_DIV_MASK;
> +   break;
> +   default:
> +   printf("Not supported\n");

Ditto.

> +   case SYSTEM_PLL2_50M_CLK:
> +   case SYSTEM_PLL1_40M_CLK:
> +   pll_clke = SSCG_PLL_DIV20_CLKE_MASK;
> +   div = 20;
> +   break;
> +   default:
> +   printf("Not supported\n");

Ditto.

> +static u32 get_root_src_clk(enum clk_root_src root_src)
> +{
> +   switch (root_src) {
> +   case OSC_25M_CLK:
> +   return 2500u;

Is this 'u' in the end really needed? It doesn't seem so.


> +#ifdef CONFIG_MXC_OCOTP
> +void enable_ocotp_clk(unsigned char enable)
> +{
> +   clock_enable(CCGR_OCOTP, !!enable);

Not a big fan of a function called clock_enabled() that can be used to
enable or disable a clock.

I prefer the kernel style: clock_enable() and clock_disable().



> +}
> +#endif
> +
> +int enable_i2c_clk(unsigned char enable, unsigned int i2c_num)
> +{
> +   /* 0 - 3 is valid i2c num */
> +   if (i2c_num > 3)
> +   return -EINVAL;
> +
> +   clock_enable(CCGR_I2C1 + i2c_num, !!enable);
> +
> +   return 0;
> +}
> +
> +unsigned int mxc_get_clock(enum clk_root_index clk)
> +{
> +   u32 val;
> +
> +   if (clk >= CLK_ROOT_MAX)
> +   return 0;
> +
> +   if (clk == MXC_ARM_CLK)
> +   return get_root_clk(ARM_A53_CLK_ROOT);
> +
> +   if (clk == MXC_IPG_CLK) {
> +   clock_get_target_val(IPG_CLK_ROOT, );
> +   val = val & 0x3;
> +   return get_root_clk(AHB_CLK_ROOT) / (val + 1);
> +   }
> +
> +   return get_root_clk(clk);
> +}
> +
> +u32 imx_get_uartclk(void)
> +{
> +   return mxc_get_clock(UART1_CLK_ROOT);
> +}
> +
> +void mxs_set_lcdclk(u32 base_addr, u32 freq)
> +{
> +   /*
> +* LCDIF_PIXEL_CLK: select 800MHz root clock,
> +* select 

[U-Boot] [PATCH V2 04/23] imx: mx8m: add clock driver

[Peng Fan]

Add clock driver to support i.MX8M.

There are two kind PLLs, FRAC pll and SSCG pll. ROM already
configured SYS PLL1/2, we only need to configure the output.
ocotp/i2c/pll decoding and configuration/usdhc/lcdif/dram pll/
enet clock are configured in the code.

Signed-off-by: Peng Fan 
---
 arch/arm/include/asm/arch-mx8m/clock.h | 657 +++
 arch/arm/mach-imx/mx8m/Makefile|   7 +
 arch/arm/mach-imx/mx8m/clock.c | 788 +
 arch/arm/mach-imx/mx8m/clock_slice.c   | 742 +++
 4 files changed, 2194 insertions(+)
 create mode 100644 arch/arm/include/asm/arch-mx8m/clock.h
 create mode 100644 arch/arm/mach-imx/mx8m/Makefile
 create mode 100644 arch/arm/mach-imx/mx8m/clock.c
 create mode 100644 arch/arm/mach-imx/mx8m/clock_slice.c

diff --git a/arch/arm/include/asm/arch-mx8m/clock.h 
b/arch/arm/include/asm/arch-mx8m/clock.h
new file mode 100644
index 00..12b453
--- /dev/null
+++ b/arch/arm/include/asm/arch-mx8m/clock.h
@@ -0,0 +1,657 @@
+/*
+ * Copyright 2017 NXP
+ *
+ * Peng Fan 
+ *
+ * SPDX-License-Identifier:GPL-2.0+
+ */
+
+#ifndef _ASM_ARCH_IMX8M_CLOCK_H
+#define _ASM_ARCH_IMX8M_CLOCK_H
+
+#include 
+
+enum pll_clocks {
+   ANATOP_ARM_PLL,
+   ANATOP_GPU_PLL,
+   ANATOP_SYSTEM_PLL1,
+   ANATOP_SYSTEM_PLL2,
+   ANATOP_SYSTEM_PLL3,
+   ANATOP_AUDIO_PLL1,
+   ANATOP_AUDIO_PLL2,
+   ANATOP_VIDEO_PLL1,
+   ANATOP_VIDEO_PLL2,
+   ANATOP_DRAM_PLL,
+};
+
+enum clk_slice_type {
+   CORE_CLOCK_SLICE,
+   BUS_CLOCK_SLICE,
+   IP_CLOCK_SLICE,
+   AHB_CLOCK_SLICE,
+   IPG_CLOCK_SLICE,
+   CORE_SEL_CLOCK_SLICE,
+   DRAM_SEL_CLOCK_SLICE,
+};
+
+enum clk_root_index {
+   MXC_ARM_CLK = 0,
+   ARM_A53_CLK_ROOT= 0,
+   ARM_M4_CLK_ROOT = 1,
+   VPU_A53_CLK_ROOT= 2,
+   GPU_CORE_CLK_ROOT   = 3,
+   GPU_SHADER_CLK_ROOT = 4,
+   MAIN_AXI_CLK_ROOT   = 16,
+   ENET_AXI_CLK_ROOT   = 17,
+   NAND_USDHC_BUS_CLK_ROOT = 18,
+   VPU_BUS_CLK_ROOT= 19,
+   DISPLAY_AXI_CLK_ROOT= 20,
+   DISPLAY_APB_CLK_ROOT= 21,
+   DISPLAY_RTRM_CLK_ROOT   = 22,
+   USB_BUS_CLK_ROOT= 23,
+   GPU_AXI_CLK_ROOT= 24,
+   GPU_AHB_CLK_ROOT= 25,
+   NOC_CLK_ROOT= 26,
+   NOC_APB_CLK_ROOT= 27,
+   AHB_CLK_ROOT= 32,
+   IPG_CLK_ROOT= 33,
+   MXC_IPG_CLK = 33,
+   AUDIO_AHB_CLK_ROOT  = 34,
+   MIPI_DSI_ESC_RX_CLK_ROOT= 36,
+   DRAM_SEL_CFG= 48,
+   CORE_SEL_CFG= 49,
+   DRAM_ALT_CLK_ROOT   = 64,
+   DRAM_APB_CLK_ROOT   = 65,
+   VPU_G1_CLK_ROOT = 66,
+   VPU_G2_CLK_ROOT = 67,
+   DISPLAY_DTRC_CLK_ROOT   = 68,
+   DISPLAY_DC8000_CLK_ROOT = 69,
+   PCIE1_CTRL_CLK_ROOT = 70,
+   PCIE1_PHY_CLK_ROOT  = 71,
+   PCIE1_AUX_CLK_ROOT  = 72,
+   DC_PIXEL_CLK_ROOT   = 73,
+   LCDIF_PIXEL_CLK_ROOT= 74,
+   SAI1_CLK_ROOT   = 75,
+   SAI2_CLK_ROOT   = 76,
+   SAI3_CLK_ROOT   = 77,
+   SAI4_CLK_ROOT   = 78,
+   SAI5_CLK_ROOT   = 79,
+   SAI6_CLK_ROOT   = 80,
+   SPDIF1_CLK_ROOT = 81,
+   SPDIF2_CLK_ROOT = 82,
+   ENET_REF_CLK_ROOT   = 83,
+   ENET_TIMER_CLK_ROOT = 84,
+   ENET_PHY_REF_CLK_ROOT   = 85,
+   NAND_CLK_ROOT   = 86,
+   QSPI_CLK_ROOT   = 87,
+   MXC_ESDHC_CLK   = 88,
+   USDHC1_CLK_ROOT = 88,
+   MXC_ESDHC2_CLK  = 89,
+   USDHC2_CLK_ROOT = 89,
+   I2C1_CLK_ROOT   = 90,
+   MXC_I2C_CLK = 90,
+   I2C2_CLK_ROOT   = 91,
+   I2C3_CLK_ROOT   = 92,
+   I2C4_CLK_ROOT   = 93,
+   UART1_CLK_ROOT  = 94,
+   UART2_CLK_ROOT  = 95,
+   UART3_CLK_ROOT  = 96,
+   UART4_CLK_ROOT  = 97,
+   USB_CORE_REF_CLK_ROOT   = 98,
+   USB_PHY_REF_CLK_ROOT= 99,
+   GIC_CLK_ROOT= 100,
+   ECSPI1_CLK_ROOT = 101,
+   ECSPI2_CLK_ROOT = 102,
+   PWM1_CLK_ROOT   = 103,
+   PWM2_CLK_ROOT   = 104,
+   PWM3_CLK_ROOT