[2/2] clk: imx: imx7d: remove clks_init_on array

Commit Message

Anson Huang Aug. 8, 2018, 4:39 a.m. UTC

Clock framework will enable those clocks registered
with CLK_IS_CRITICAL flag, so no need to have
clks_init_on array during clock initialization now.

Signed-off-by: Anson Huang <[e-mail]>
---
 drivers/clk/imx/clk-imx7d.c | 27 ++++++++-------------------
 drivers/clk/imx/clk.h       |  7 +++++++
 2 files changed, 15 insertions(+), 19 deletions(-)

Comments

Peng Fan Aug. 8, 2018, 8:48 a.m. UTC | #1

> -----Original Message-----
> From: Anson Huang
> Sent: 2018年8月8日 12:39
> To: [e-mail]; [e-mail]; [e-mail];
> Fabio Estevam <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Cc: dl-linux-imx <[e-mail]>
> Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
> 
> Clock framework will enable those clocks registered with CLK_IS_CRITICAL flag,
> so no need to have clks_init_on array during clock initialization now.

Will it be more flexible to parse dts saying "critical-clocks = <xxx>" or "init-on-arrary=<xxx>"
and enable those clocks?

Regards,
Peng.

> 
> Signed-off-by: Anson Huang <[e-mail]>
> ---
>  drivers/clk/imx/clk-imx7d.c | 27 ++++++++-------------------
>  drivers/clk/imx/clk.h       |  7 +++++++
>  2 files changed, 15 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-imx7d.c b/drivers/clk/imx/clk-imx7d.c index
> c4518d7..076460b 100644
> --- a/drivers/clk/imx/clk-imx7d.c
> +++ b/drivers/clk/imx/clk-imx7d.c
> @@ -379,13 +379,6 @@ static const char *pll_enet_bypass_sel[] =
> { "pll_enet_main", "pll_enet_main_src  static const char
> *pll_audio_bypass_sel[] = { "pll_audio_main", "pll_audio_main_src", };  static
> const char *pll_video_bypass_sel[] = { "pll_video_main", "pll_video_main_src", };
> 
> -static int const clks_init_on[] __initconst = {
> -	IMX7D_ARM_A7_ROOT_CLK, IMX7D_MAIN_AXI_ROOT_CLK,
> -	IMX7D_PLL_SYS_MAIN_480M_CLK, IMX7D_IPG_ROOT_CLK,
> -	IMX7D_DRAM_PHYM_ROOT_CLK, IMX7D_DRAM_ROOT_CLK,
> -	IMX7D_DRAM_PHYM_ALT_ROOT_CLK, IMX7D_DRAM_ALT_ROOT_CLK,
> -};
> -
>  static struct clk_onecell_data clk_data;
> 
>  static struct clk ** const uart_clks[] __initconst = { @@ -403,7 +396,6 @@
> static void __init imx7d_clocks_init(struct device_node *ccm_node)  {
>  	struct device_node *np;
>  	void __iomem *base;
> -	int i;
> 
>  	clks[IMX7D_CLK_DUMMY] = imx_clk_fixed("dummy", 0);
>  	clks[IMX7D_OSC_24M_CLK] = of_clk_get_by_name(ccm_node, "osc"); @@
> -466,7 +458,7 @@ static void __init imx7d_clocks_init(struct device_node
> *ccm_node)
>  	clks[IMX7D_PLL_SYS_MAIN_120M] =
> imx_clk_fixed_factor("pll_sys_main_120m", "pll_sys_main_clk", 1, 4);
>  	clks[IMX7D_PLL_DRAM_MAIN_533M] =
> imx_clk_fixed_factor("pll_dram_533m", "pll_dram_main_clk", 1, 2);
> 
> -	clks[IMX7D_PLL_SYS_MAIN_480M_CLK] =
> imx_clk_gate_dis("pll_sys_main_480m_clk", "pll_sys_main_480m", base + 0xb0,
> 4);
> +	clks[IMX7D_PLL_SYS_MAIN_480M_CLK] =
> +imx_clk_gate_dis_flags("pll_sys_main_480m_clk", "pll_sys_main_480m",
> +base + 0xb0, 4, CLK_IS_CRITICAL);
>  	clks[IMX7D_PLL_SYS_MAIN_240M_CLK] =
> imx_clk_gate_dis("pll_sys_main_240m_clk", "pll_sys_main_240m", base + 0xb0,
> 5);
>  	clks[IMX7D_PLL_SYS_MAIN_120M_CLK] =
> imx_clk_gate_dis("pll_sys_main_120m_clk", "pll_sys_main_120m", base + 0xb0,
> 6);
>  	clks[IMX7D_PLL_DRAM_MAIN_533M_CLK] =
> imx_clk_gate("pll_dram_533m_clk", "pll_dram_533m", base + 0x70, 12); @@
> -719,7 +711,7 @@ static void __init imx7d_clocks_init(struct device_node
> *ccm_node)
>  	clks[IMX7D_ENET_AXI_ROOT_DIV] =
> imx_clk_divider2("enet_axi_post_div", "enet_axi_pre_div", base + 0x8900, 0, 6);
>  	clks[IMX7D_NAND_USDHC_BUS_ROOT_CLK] =
> imx_clk_divider2("nand_usdhc_root_clk", "nand_usdhc_pre_div", base +
> 0x8980, 0, 6);
>  	clks[IMX7D_AHB_CHANNEL_ROOT_DIV] =
> imx_clk_divider2("ahb_root_clk", "ahb_pre_div", base + 0x9000, 0, 6);
> -	clks[IMX7D_IPG_ROOT_CLK] = imx_clk_divider2("ipg_root_clk",
> "ahb_root_clk", base + 0x9080, 0, 2);
> +	clks[IMX7D_IPG_ROOT_CLK] = imx_clk_divider_flags("ipg_root_clk",
> +"ahb_root_clk", base + 0x9080, 0, 2, CLK_IS_CRITICAL |
> +CLK_OPS_PARENT_ENABLE | CLK_SET_RATE_PARENT);
>  	clks[IMX7D_DRAM_ROOT_DIV] = imx_clk_divider2("dram_post_div",
> "dram_cg", base + 0x9880, 0, 3);
>  	clks[IMX7D_DRAM_PHYM_ALT_ROOT_DIV] =
> imx_clk_divider2("dram_phym_alt_post_div", "dram_phym_alt_pre_div", base
> + 0xa000, 0, 3);
>  	clks[IMX7D_DRAM_ALT_ROOT_DIV] =
> imx_clk_divider2("dram_alt_post_div", "dram_alt_pre_div", base + 0xa080, 0,
> 3); @@ -783,17 +775,17 @@ static void __init imx7d_clocks_init(struct
> device_node *ccm_node)
>  	clks[IMX7D_CLKO1_ROOT_DIV] = imx_clk_divider2("clko1_post_div",
> "clko1_pre_div", base + 0xbd80, 0, 6);
>  	clks[IMX7D_CLKO2_ROOT_DIV] = imx_clk_divider2("clko2_post_div",
> "clko2_pre_div", base + 0xbe00, 0, 6);
> 
> -	clks[IMX7D_ARM_A7_ROOT_CLK] = imx_clk_gate4("arm_a7_root_clk",
> "arm_a7_div", base + 0x4000, 0);
> +	clks[IMX7D_ARM_A7_ROOT_CLK] =
> imx_clk_gate2_flags("arm_a7_root_clk",
> +"arm_a7_div", base + 0x4000, 0, CLK_IS_CRITICAL |
> +CLK_OPS_PARENT_ENABLE);
>  	clks[IMX7D_ARM_M4_ROOT_CLK] = imx_clk_gate4("arm_m4_root_clk",
> "arm_m4_div", base + 0x4010, 0);
> -	clks[IMX7D_MAIN_AXI_ROOT_CLK] = imx_clk_gate4("main_axi_root_clk",
> "axi_post_div", base + 0x4040, 0);
> +	clks[IMX7D_MAIN_AXI_ROOT_CLK] =
> +imx_clk_gate2_flags("main_axi_root_clk", "axi_post_div", base + 0x4040,
> +0, CLK_IS_CRITICAL | CLK_OPS_PARENT_ENABLE);
>  	clks[IMX7D_DISP_AXI_ROOT_CLK] = imx_clk_gate4("disp_axi_root_clk",
> "disp_axi_post_div", base + 0x4050, 0);
>  	clks[IMX7D_ENET_AXI_ROOT_CLK] = imx_clk_gate4("enet_axi_root_clk",
> "enet_axi_post_div", base + 0x4060, 0);
>  	clks[IMX7D_OCRAM_CLK] = imx_clk_gate4("ocram_clk",
> "main_axi_root_clk", base + 0x4110, 0);
>  	clks[IMX7D_OCRAM_S_CLK] = imx_clk_gate4("ocram_s_clk",
> "ahb_root_clk", base + 0x4120, 0);
> -	clks[IMX7D_DRAM_ROOT_CLK] = imx_clk_gate4("dram_root_clk",
> "dram_post_div", base + 0x4130, 0);
> -	clks[IMX7D_DRAM_PHYM_ROOT_CLK] =
> imx_clk_gate4("dram_phym_root_clk", "dram_phym_cg", base + 0x4130, 0);
> -	clks[IMX7D_DRAM_PHYM_ALT_ROOT_CLK] =
> imx_clk_gate4("dram_phym_alt_root_clk", "dram_phym_alt_post_div", base +
> 0x4130, 0);
> -	clks[IMX7D_DRAM_ALT_ROOT_CLK] = imx_clk_gate4("dram_alt_root_clk",
> "dram_alt_post_div", base + 0x4130, 0);
> +	clks[IMX7D_DRAM_ROOT_CLK] = imx_clk_gate2_flags("dram_root_clk",
> "dram_post_div", base + 0x4130, 0, CLK_IS_CRITICAL |
> CLK_OPS_PARENT_ENABLE);
> +	clks[IMX7D_DRAM_PHYM_ROOT_CLK] =
> imx_clk_gate2_flags("dram_phym_root_clk", "dram_phym_cg", base + 0x4130,
> 0, CLK_IS_CRITICAL | CLK_OPS_PARENT_ENABLE);
> +	clks[IMX7D_DRAM_PHYM_ALT_ROOT_CLK] =
> imx_clk_gate2_flags("dram_phym_alt_root_clk", "dram_phym_alt_post_div",
> base + 0x4130, 0, CLK_IS_CRITICAL | CLK_OPS_PARENT_ENABLE);
> +	clks[IMX7D_DRAM_ALT_ROOT_CLK] =
> +imx_clk_gate2_flags("dram_alt_root_clk", "dram_alt_post_div", base +
> +0x4130, 0, CLK_IS_CRITICAL | CLK_OPS_PARENT_ENABLE);
>  	clks[IMX7D_OCOTP_CLK] = imx_clk_gate4("ocotp_clk", "ipg_root_clk",
> base + 0x4230, 0);
>  	clks[IMX7D_SNVS_CLK] = imx_clk_gate4("snvs_clk", "ipg_root_clk", base +
> 0x4250, 0);
>  	clks[IMX7D_MU_ROOT_CLK] = imx_clk_gate4("mu_root_clk",
> "ipg_root_clk", base + 0x4270, 0); @@ -882,9 +874,6 @@ static void __init
> imx7d_clocks_init(struct device_node *ccm_node)
>  	clk_data.clk_num = ARRAY_SIZE(clks);
>  	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
> 
> -	for (i = 0; i < ARRAY_SIZE(clks_init_on); i++)
> -		clk_prepare_enable(clks[clks_init_on[i]]);
> -
>  	clk_set_parent(clks[IMX7D_PLL_ARM_MAIN_BYPASS],
> clks[IMX7D_PLL_ARM_MAIN]);
>  	clk_set_parent(clks[IMX7D_PLL_DRAM_MAIN_BYPASS],
> clks[IMX7D_PLL_DRAM_MAIN]);
>  	clk_set_parent(clks[IMX7D_PLL_SYS_MAIN_BYPASS],
> clks[IMX7D_PLL_SYS_MAIN]); diff --git a/drivers/clk/imx/clk.h
> b/drivers/clk/imx/clk.h index 8076ec0..5895e223 100644
> --- a/drivers/clk/imx/clk.h
> +++ b/drivers/clk/imx/clk.h
> @@ -137,6 +137,13 @@ static inline struct clk *imx_clk_gate_dis(const char
> *name, const char *parent,
>  			shift, CLK_GATE_SET_TO_DISABLE, &imx_ccm_lock);  }
> 
> +static inline struct clk *imx_clk_gate_dis_flags(const char *name, const char
> *parent,
> +		void __iomem *reg, u8 shift, unsigned long flags) {
> +	return clk_register_gate(NULL, name, parent, flags |
> CLK_SET_RATE_PARENT, reg,
> +			shift, CLK_GATE_SET_TO_DISABLE, &imx_ccm_lock); }
> +
>  static inline struct clk *imx_clk_gate2(const char *name, const char *parent,
>  		void __iomem *reg, u8 shift)
>  {
> --
> 2.7.4

Anson Huang Aug. 8, 2018, 9 a.m. UTC | #2

Anson Huang
Best Regards!


> -----Original Message-----
> From: Peng Fan
> Sent: Wednesday, August 8, 2018 4:49 PM
> To: Anson Huang <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; Fabio Estevam
> <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Rob Herring <[e-mail]>
> Cc: dl-linux-imx <[e-mail]>
> Subject: RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
> 
> 
> 
> > -----Original Message-----
> > From: Anson Huang
> > Sent: 2018年8月8日 12:39
> > To: [e-mail]; [e-mail];
> > [e-mail]; Fabio Estevam <[e-mail]>;
> > [e-mail]; [e-mail];
> > [e-mail];
> > [e-mail]; [e-mail]
> > Cc: dl-linux-imx <[e-mail]>
> > Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
> >
> > Clock framework will enable those clocks registered with
> > CLK_IS_CRITICAL flag, so no need to have clks_init_on array during clock
> initialization now.
> 
> Will it be more flexible to parse dts saying "critical-clocks = <xxx>" or
> "init-on-arrary=<xxx>"
> and enable those clocks?
 
Parsing the clocks arrays from dtb is another way of enabling critical clocks, but
for current i.MX6/7 platforms, we implement it in same way as most of other SoCs,
currently I did NOT see any necessity of putting them in dtb,
just adding flag during clock registering is more simple, if there is any special requirement
for different clocks set to be enabled, then we can add support to enable the method of
parsing critical-clocks from dtb. Just my two cents.

Anson.

> 
> Regards,
> Peng.
> 
> >
> > Signed-off-by: Anson Huang <[e-mail]>
> > ---
> >  drivers/clk/imx/clk-imx7d.c | 27 ++++++++-------------------
> >  drivers/clk/imx/clk.h       |  7 +++++++
> >  2 files changed, 15 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/clk/imx/clk-imx7d.c b/drivers/clk/imx/clk-imx7d.c
> > index c4518d7..076460b 100644
> > --- a/drivers/clk/imx/clk-imx7d.c
> > +++ b/drivers/clk/imx/clk-imx7d.c
> > @@ -379,13 +379,6 @@ static const char *pll_enet_bypass_sel[] = {
> > "pll_enet_main", "pll_enet_main_src  static const char
> > *pll_audio_bypass_sel[] = { "pll_audio_main", "pll_audio_main_src", };
> > static const char *pll_video_bypass_sel[] = { "pll_video_main",
> > "pll_video_main_src", };
> >
> > -static int const clks_init_on[] __initconst = {
> > -	IMX7D_ARM_A7_ROOT_CLK, IMX7D_MAIN_AXI_ROOT_CLK,
> > -	IMX7D_PLL_SYS_MAIN_480M_CLK, IMX7D_IPG_ROOT_CLK,
> > -	IMX7D_DRAM_PHYM_ROOT_CLK, IMX7D_DRAM_ROOT_CLK,
> > -	IMX7D_DRAM_PHYM_ALT_ROOT_CLK, IMX7D_DRAM_ALT_ROOT_CLK,
> > -};
> > -
> >  static struct clk_onecell_data clk_data;
> >
> >  static struct clk ** const uart_clks[] __initconst = { @@ -403,7
> > +396,6 @@ static void __init imx7d_clocks_init(struct device_node
> *ccm_node)  {
> >  	struct device_node *np;
> >  	void __iomem *base;
> > -	int i;
> >
> >  	clks[IMX7D_CLK_DUMMY] = imx_clk_fixed("dummy", 0);
> >  	clks[IMX7D_OSC_24M_CLK] = of_clk_get_by_name(ccm_node, "osc");
> @@
> > -466,7 +458,7 @@ static void __init imx7d_clocks_init(struct
> > device_node
> > *ccm_node)
> >  	clks[IMX7D_PLL_SYS_MAIN_120M] =
> > imx_clk_fixed_factor("pll_sys_main_120m", "pll_sys_main_clk", 1, 4);
> >  	clks[IMX7D_PLL_DRAM_MAIN_533M] =
> > imx_clk_fixed_factor("pll_dram_533m", "pll_dram_main_clk", 1, 2);
> >
> > -	clks[IMX7D_PLL_SYS_MAIN_480M_CLK] =
> > imx_clk_gate_dis("pll_sys_main_480m_clk", "pll_sys_main_480m", base +
> > 0xb0, 4);
> > +	clks[IMX7D_PLL_SYS_MAIN_480M_CLK] =
> > +imx_clk_gate_dis_flags("pll_sys_main_480m_clk", "pll_sys_main_480m",
> > +base + 0xb0, 4, CLK_IS_CRITICAL);
> >  	clks[IMX7D_PLL_SYS_MAIN_240M_CLK] =
> > imx_clk_gate_dis("pll_sys_main_240m_clk", "pll_sys_main_240m", base +
> > 0xb0, 5);
> >  	clks[IMX7D_PLL_SYS_MAIN_120M_CLK] =
> > imx_clk_gate_dis("pll_sys_main_120m_clk", "pll_sys_main_120m", base +
> > 0xb0, 6);
> >  	clks[IMX7D_PLL_DRAM_MAIN_533M_CLK] =
> > imx_clk_gate("pll_dram_533m_clk", "pll_dram_533m", base + 0x70, 12);
> > @@
> > -719,7 +711,7 @@ static void __init imx7d_clocks_init(struct
> > device_node
> > *ccm_node)
> >  	clks[IMX7D_ENET_AXI_ROOT_DIV] =
> > imx_clk_divider2("enet_axi_post_div", "enet_axi_pre_div", base + 0x8900, 0,
> 6);
> >  	clks[IMX7D_NAND_USDHC_BUS_ROOT_CLK] =
> > imx_clk_divider2("nand_usdhc_root_clk", "nand_usdhc_pre_div", base +
> > 0x8980, 0, 6);
> >  	clks[IMX7D_AHB_CHANNEL_ROOT_DIV] =
> > imx_clk_divider2("ahb_root_clk", "ahb_pre_div", base + 0x9000, 0, 6);
> > -	clks[IMX7D_IPG_ROOT_CLK] = imx_clk_divider2("ipg_root_clk",
> > "ahb_root_clk", base + 0x9080, 0, 2);
> > +	clks[IMX7D_IPG_ROOT_CLK] = imx_clk_divider_flags("ipg_root_clk",
> > +"ahb_root_clk", base + 0x9080, 0, 2, CLK_IS_CRITICAL |
> > +CLK_OPS_PARENT_ENABLE | CLK_SET_RATE_PARENT);
> >  	clks[IMX7D_DRAM_ROOT_DIV] = imx_clk_divider2("dram_post_div",
> > "dram_cg", base + 0x9880, 0, 3);
> >  	clks[IMX7D_DRAM_PHYM_ALT_ROOT_DIV] =
> > imx_clk_divider2("dram_phym_alt_post_div", "dram_phym_alt_pre_div",
> > base
> > + 0xa000, 0, 3);
> >  	clks[IMX7D_DRAM_ALT_ROOT_DIV] =
> > imx_clk_divider2("dram_alt_post_div", "dram_alt_pre_div", base +
> > 0xa080, 0, 3); @@ -783,17 +775,17 @@ static void __init
> > imx7d_clocks_init(struct device_node *ccm_node)
> >  	clks[IMX7D_CLKO1_ROOT_DIV] = imx_clk_divider2("clko1_post_div",
> > "clko1_pre_div", base + 0xbd80, 0, 6);
> >  	clks[IMX7D_CLKO2_ROOT_DIV] = imx_clk_divider2("clko2_post_div",
> > "clko2_pre_div", base + 0xbe00, 0, 6);
> >
> > -	clks[IMX7D_ARM_A7_ROOT_CLK] = imx_clk_gate4("arm_a7_root_clk",
> > "arm_a7_div", base + 0x4000, 0);
> > +	clks[IMX7D_ARM_A7_ROOT_CLK] =
> > imx_clk_gate2_flags("arm_a7_root_clk",
> > +"arm_a7_div", base + 0x4000, 0, CLK_IS_CRITICAL |
> > +CLK_OPS_PARENT_ENABLE);
> >  	clks[IMX7D_ARM_M4_ROOT_CLK] = imx_clk_gate4("arm_m4_root_clk",
> > "arm_m4_div", base + 0x4010, 0);
> > -	clks[IMX7D_MAIN_AXI_ROOT_CLK] = imx_clk_gate4("main_axi_root_clk",
> > "axi_post_div", base + 0x4040, 0);
> > +	clks[IMX7D_MAIN_AXI_ROOT_CLK] =
> > +imx_clk_gate2_flags("main_axi_root_clk", "axi_post_div", base +
> > +0x4040, 0, CLK_IS_CRITICAL | CLK_OPS_PARENT_ENABLE);
> >  	clks[IMX7D_DISP_AXI_ROOT_CLK] = imx_clk_gate4("disp_axi_root_clk",
> > "disp_axi_post_div", base + 0x4050, 0);
> >  	clks[IMX7D_ENET_AXI_ROOT_CLK] = imx_clk_gate4("enet_axi_root_clk",
> > "enet_axi_post_div", base + 0x4060, 0);
> >  	clks[IMX7D_OCRAM_CLK] = imx_clk_gate4("ocram_clk",
> > "main_axi_root_clk", base + 0x4110, 0);
> >  	clks[IMX7D_OCRAM_S_CLK] = imx_clk_gate4("ocram_s_clk",
> > "ahb_root_clk", base + 0x4120, 0);
> > -	clks[IMX7D_DRAM_ROOT_CLK] = imx_clk_gate4("dram_root_clk",
> > "dram_post_div", base + 0x4130, 0);
> > -	clks[IMX7D_DRAM_PHYM_ROOT_CLK] =
> > imx_clk_gate4("dram_phym_root_clk", "dram_phym_cg", base + 0x4130, 0);
> > -	clks[IMX7D_DRAM_PHYM_ALT_ROOT_CLK] =
> > imx_clk_gate4("dram_phym_alt_root_clk", "dram_phym_alt_post_div", base
> > + 0x4130, 0);
> > -	clks[IMX7D_DRAM_ALT_ROOT_CLK] =
> imx_clk_gate4("dram_alt_root_clk",
> > "dram_alt_post_div", base + 0x4130, 0);
> > +	clks[IMX7D_DRAM_ROOT_CLK] = imx_clk_gate2_flags("dram_root_clk",
> > "dram_post_div", base + 0x4130, 0, CLK_IS_CRITICAL |
> > CLK_OPS_PARENT_ENABLE);
> > +	clks[IMX7D_DRAM_PHYM_ROOT_CLK] =
> > imx_clk_gate2_flags("dram_phym_root_clk", "dram_phym_cg", base +
> > 0x4130, 0, CLK_IS_CRITICAL | CLK_OPS_PARENT_ENABLE);
> > +	clks[IMX7D_DRAM_PHYM_ALT_ROOT_CLK] =
> > imx_clk_gate2_flags("dram_phym_alt_root_clk",
> > "dram_phym_alt_post_div", base + 0x4130, 0, CLK_IS_CRITICAL |
> > CLK_OPS_PARENT_ENABLE);
> > +	clks[IMX7D_DRAM_ALT_ROOT_CLK] =
> > +imx_clk_gate2_flags("dram_alt_root_clk", "dram_alt_post_div", base +
> > +0x4130, 0, CLK_IS_CRITICAL | CLK_OPS_PARENT_ENABLE);
> >  	clks[IMX7D_OCOTP_CLK] = imx_clk_gate4("ocotp_clk", "ipg_root_clk",
> > base + 0x4230, 0);
> >  	clks[IMX7D_SNVS_CLK] = imx_clk_gate4("snvs_clk", "ipg_root_clk",
> > base + 0x4250, 0);
> >  	clks[IMX7D_MU_ROOT_CLK] = imx_clk_gate4("mu_root_clk",
> > "ipg_root_clk", base + 0x4270, 0); @@ -882,9 +874,6 @@ static void
> > __init imx7d_clocks_init(struct device_node *ccm_node)
> >  	clk_data.clk_num = ARRAY_SIZE(clks);
> >  	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
> >
> > -	for (i = 0; i < ARRAY_SIZE(clks_init_on); i++)
> > -		clk_prepare_enable(clks[clks_init_on[i]]);
> > -
> >  	clk_set_parent(clks[IMX7D_PLL_ARM_MAIN_BYPASS],
> > clks[IMX7D_PLL_ARM_MAIN]);
> >  	clk_set_parent(clks[IMX7D_PLL_DRAM_MAIN_BYPASS],
> > clks[IMX7D_PLL_DRAM_MAIN]);
> >  	clk_set_parent(clks[IMX7D_PLL_SYS_MAIN_BYPASS],
> > clks[IMX7D_PLL_SYS_MAIN]); diff --git a/drivers/clk/imx/clk.h
> > b/drivers/clk/imx/clk.h index 8076ec0..5895e223 100644
> > --- a/drivers/clk/imx/clk.h
> > +++ b/drivers/clk/imx/clk.h
> > @@ -137,6 +137,13 @@ static inline struct clk *imx_clk_gate_dis(const
> > char *name, const char *parent,
> >  			shift, CLK_GATE_SET_TO_DISABLE, &imx_ccm_lock);  }
> >
> > +static inline struct clk *imx_clk_gate_dis_flags(const char *name,
> > +const char
> > *parent,
> > +		void __iomem *reg, u8 shift, unsigned long flags) {
> > +	return clk_register_gate(NULL, name, parent, flags |
> > CLK_SET_RATE_PARENT, reg,
> > +			shift, CLK_GATE_SET_TO_DISABLE, &imx_ccm_lock); }
> > +
> >  static inline struct clk *imx_clk_gate2(const char *name, const char
> *parent,
> >  		void __iomem *reg, u8 shift)
> >  {
> > --
> > 2.7.4

Peng Fan Aug. 13, 2018, 1:15 a.m. UTC | #3

Hi Anson,

> > > -----Original Message-----
> > > From: Anson Huang
> > > Sent: 2018年8月8日 12:39
> > > To: [e-mail]; [e-mail];
> > > [e-mail]; Fabio Estevam <[e-mail]>;
> > > [e-mail]; [e-mail];
> > > [e-mail];
> > > [e-mail]; [e-mail]
> > > Cc: dl-linux-imx <[e-mail]>
> > > Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
> > >
> > > Clock framework will enable those clocks registered with
> > > CLK_IS_CRITICAL flag, so no need to have clks_init_on array during
> > > clock
> > initialization now.
> >
> > Will it be more flexible to parse dts saying "critical-clocks = <xxx>"
> > or "init-on-arrary=<xxx>"
> > and enable those clocks?
> 
> Parsing the clocks arrays from dtb is another way of enabling critical clocks, but
> for current i.MX6/7 platforms, we implement it in same way as most of other
> SoCs, currently I did NOT see any necessity of putting them in dtb, just adding
> flag during clock registering is more simple, if there is any special requirement
> for different clocks set to be enabled, then we can add support to enable the
> method of parsing critical-clocks from dtb. Just my two cents.

Thinking about OP-TEE want to use one device, but it's clocks are registered
by Linux, because there is no module in Linux side use it, it will shutdown the clock,
which cause OP-TEE could not access the device.

Then people have to modify clk code to add CLK_IS_CRITICAL flag to make sure
the clocks are not shutdown by Linux.

However adding a new property in clk node and let driver code parse the dts,
there is no need to modify clk driver code when OP-TEE needs another device clock.

Regards,
Peng.

> 
> Anson.
> 
> >
> > Regards,
> > Peng.
> >
> > >
> > > Signed-off-by: Anson Huang <[e-mail]>
> > > ---
> > >  drivers/clk/imx/clk-imx7d.c | 27 ++++++++-------------------
> > >  drivers/clk/imx/clk.h       |  7 +++++++
> > >  2 files changed, 15 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/clk/imx/clk-imx7d.c
> > > b/drivers/clk/imx/clk-imx7d.c index c4518d7..076460b 100644
> > > --- a/drivers/clk/imx/clk-imx7d.c
> > > +++ b/drivers/clk/imx/clk-imx7d.c
> > > @@ -379,13 +379,6 @@ static const char *pll_enet_bypass_sel[] = {
> > > "pll_enet_main", "pll_enet_main_src  static const char
> > > *pll_audio_bypass_sel[] = { "pll_audio_main", "pll_audio_main_src",
> > > }; static const char *pll_video_bypass_sel[] = { "pll_video_main",
> > > "pll_video_main_src", };
> > >
> > > -static int const clks_init_on[] __initconst = {
> > > -	IMX7D_ARM_A7_ROOT_CLK, IMX7D_MAIN_AXI_ROOT_CLK,
> > > -	IMX7D_PLL_SYS_MAIN_480M_CLK, IMX7D_IPG_ROOT_CLK,
> > > -	IMX7D_DRAM_PHYM_ROOT_CLK, IMX7D_DRAM_ROOT_CLK,
> > > -	IMX7D_DRAM_PHYM_ALT_ROOT_CLK, IMX7D_DRAM_ALT_ROOT_CLK,
> > > -};
> > > -
> > >  static struct clk_onecell_data clk_data;
> > >
> > >  static struct clk ** const uart_clks[] __initconst = { @@ -403,7
> > > +396,6 @@ static void __init imx7d_clocks_init(struct device_node
> > *ccm_node)  {
> > >  	struct device_node *np;
> > >  	void __iomem *base;
> > > -	int i;
> > >
> > >  	clks[IMX7D_CLK_DUMMY] = imx_clk_fixed("dummy", 0);
> > >  	clks[IMX7D_OSC_24M_CLK] = of_clk_get_by_name(ccm_node, "osc");
> > @@
> > > -466,7 +458,7 @@ static void __init imx7d_clocks_init(struct
> > > device_node
> > > *ccm_node)
> > >  	clks[IMX7D_PLL_SYS_MAIN_120M] =
> > > imx_clk_fixed_factor("pll_sys_main_120m", "pll_sys_main_clk", 1, 4);
> > >  	clks[IMX7D_PLL_DRAM_MAIN_533M] =
> > > imx_clk_fixed_factor("pll_dram_533m", "pll_dram_main_clk", 1, 2);
> > >
> > > -	clks[IMX7D_PLL_SYS_MAIN_480M_CLK] =
> > > imx_clk_gate_dis("pll_sys_main_480m_clk", "pll_sys_main_480m", base
> > > + 0xb0, 4);
> > > +	clks[IMX7D_PLL_SYS_MAIN_480M_CLK] =
> > > +imx_clk_gate_dis_flags("pll_sys_main_480m_clk",
> > > +"pll_sys_main_480m", base + 0xb0, 4, CLK_IS_CRITICAL);
> > >  	clks[IMX7D_PLL_SYS_MAIN_240M_CLK] =
> > > imx_clk_gate_dis("pll_sys_main_240m_clk", "pll_sys_main_240m", base
> > > + 0xb0, 5);
> > >  	clks[IMX7D_PLL_SYS_MAIN_120M_CLK] =
> > > imx_clk_gate_dis("pll_sys_main_120m_clk", "pll_sys_main_120m", base
> > > + 0xb0, 6);
> > >  	clks[IMX7D_PLL_DRAM_MAIN_533M_CLK] =
> > > imx_clk_gate("pll_dram_533m_clk", "pll_dram_533m", base + 0x70, 12);
> > > @@
> > > -719,7 +711,7 @@ static void __init imx7d_clocks_init(struct
> > > device_node
> > > *ccm_node)
> > >  	clks[IMX7D_ENET_AXI_ROOT_DIV] =
> > > imx_clk_divider2("enet_axi_post_div", "enet_axi_pre_div", base +
> > > 0x8900, 0,
> > 6);
> > >  	clks[IMX7D_NAND_USDHC_BUS_ROOT_CLK] =
> > > imx_clk_divider2("nand_usdhc_root_clk", "nand_usdhc_pre_div", base +
> > > 0x8980, 0, 6);
> > >  	clks[IMX7D_AHB_CHANNEL_ROOT_DIV] =
> > > imx_clk_divider2("ahb_root_clk", "ahb_pre_div", base + 0x9000, 0, 6);
> > > -	clks[IMX7D_IPG_ROOT_CLK] = imx_clk_divider2("ipg_root_clk",
> > > "ahb_root_clk", base + 0x9080, 0, 2);
> > > +	clks[IMX7D_IPG_ROOT_CLK] = imx_clk_divider_flags("ipg_root_clk",
> > > +"ahb_root_clk", base + 0x9080, 0, 2, CLK_IS_CRITICAL |
> > > +CLK_OPS_PARENT_ENABLE | CLK_SET_RATE_PARENT);
> > >  	clks[IMX7D_DRAM_ROOT_DIV] = imx_clk_divider2("dram_post_div",
> > > "dram_cg", base + 0x9880, 0, 3);
> > >  	clks[IMX7D_DRAM_PHYM_ALT_ROOT_DIV] =
> > > imx_clk_divider2("dram_phym_alt_post_div", "dram_phym_alt_pre_div",
> > > base
> > > + 0xa000, 0, 3);
> > >  	clks[IMX7D_DRAM_ALT_ROOT_DIV] =
> > > imx_clk_divider2("dram_alt_post_div", "dram_alt_pre_div", base +
> > > 0xa080, 0, 3); @@ -783,17 +775,17 @@ static void __init
> > > imx7d_clocks_init(struct device_node *ccm_node)
> > >  	clks[IMX7D_CLKO1_ROOT_DIV] = imx_clk_divider2("clko1_post_div",
> > > "clko1_pre_div", base + 0xbd80, 0, 6);
> > >  	clks[IMX7D_CLKO2_ROOT_DIV] = imx_clk_divider2("clko2_post_div",
> > > "clko2_pre_div", base + 0xbe00, 0, 6);
> > >
> > > -	clks[IMX7D_ARM_A7_ROOT_CLK] = imx_clk_gate4("arm_a7_root_clk",
> > > "arm_a7_div", base + 0x4000, 0);
> > > +	clks[IMX7D_ARM_A7_ROOT_CLK] =
> > > imx_clk_gate2_flags("arm_a7_root_clk",
> > > +"arm_a7_div", base + 0x4000, 0, CLK_IS_CRITICAL |
> > > +CLK_OPS_PARENT_ENABLE);
> > >  	clks[IMX7D_ARM_M4_ROOT_CLK] =
> imx_clk_gate4("arm_m4_root_clk",
> > > "arm_m4_div", base + 0x4010, 0);
> > > -	clks[IMX7D_MAIN_AXI_ROOT_CLK] = imx_clk_gate4("main_axi_root_clk",
> > > "axi_post_div", base + 0x4040, 0);
> > > +	clks[IMX7D_MAIN_AXI_ROOT_CLK] =
> > > +imx_clk_gate2_flags("main_axi_root_clk", "axi_post_div", base +
> > > +0x4040, 0, CLK_IS_CRITICAL | CLK_OPS_PARENT_ENABLE);
> > >  	clks[IMX7D_DISP_AXI_ROOT_CLK] =
> imx_clk_gate4("disp_axi_root_clk",
> > > "disp_axi_post_div", base + 0x4050, 0);
> > >  	clks[IMX7D_ENET_AXI_ROOT_CLK] =
> imx_clk_gate4("enet_axi_root_clk",
> > > "enet_axi_post_div", base + 0x4060, 0);
> > >  	clks[IMX7D_OCRAM_CLK] = imx_clk_gate4("ocram_clk",
> > > "main_axi_root_clk", base + 0x4110, 0);
> > >  	clks[IMX7D_OCRAM_S_CLK] = imx_clk_gate4("ocram_s_clk",
> > > "ahb_root_clk", base + 0x4120, 0);
> > > -	clks[IMX7D_DRAM_ROOT_CLK] = imx_clk_gate4("dram_root_clk",
> > > "dram_post_div", base + 0x4130, 0);
> > > -	clks[IMX7D_DRAM_PHYM_ROOT_CLK] =
> > > imx_clk_gate4("dram_phym_root_clk", "dram_phym_cg", base + 0x4130,
> 0);
> > > -	clks[IMX7D_DRAM_PHYM_ALT_ROOT_CLK] =
> > > imx_clk_gate4("dram_phym_alt_root_clk", "dram_phym_alt_post_div",
> > > base
> > > + 0x4130, 0);
> > > -	clks[IMX7D_DRAM_ALT_ROOT_CLK] =
> > imx_clk_gate4("dram_alt_root_clk",
> > > "dram_alt_post_div", base + 0x4130, 0);
> > > +	clks[IMX7D_DRAM_ROOT_CLK] =
> imx_clk_gate2_flags("dram_root_clk",
> > > "dram_post_div", base + 0x4130, 0, CLK_IS_CRITICAL |
> > > CLK_OPS_PARENT_ENABLE);
> > > +	clks[IMX7D_DRAM_PHYM_ROOT_CLK] =
> > > imx_clk_gate2_flags("dram_phym_root_clk", "dram_phym_cg", base +
> > > 0x4130, 0, CLK_IS_CRITICAL | CLK_OPS_PARENT_ENABLE);
> > > +	clks[IMX7D_DRAM_PHYM_ALT_ROOT_CLK] =
> > > imx_clk_gate2_flags("dram_phym_alt_root_clk",
> > > "dram_phym_alt_post_div", base + 0x4130, 0, CLK_IS_CRITICAL |
> > > CLK_OPS_PARENT_ENABLE);
> > > +	clks[IMX7D_DRAM_ALT_ROOT_CLK] =
> > > +imx_clk_gate2_flags("dram_alt_root_clk", "dram_alt_post_div", base
> > > ++ 0x4130, 0, CLK_IS_CRITICAL | CLK_OPS_PARENT_ENABLE);
> > >  	clks[IMX7D_OCOTP_CLK] = imx_clk_gate4("ocotp_clk",
> "ipg_root_clk",
> > > base + 0x4230, 0);
> > >  	clks[IMX7D_SNVS_CLK] = imx_clk_gate4("snvs_clk", "ipg_root_clk",
> > > base + 0x4250, 0);
> > >  	clks[IMX7D_MU_ROOT_CLK] = imx_clk_gate4("mu_root_clk",
> > > "ipg_root_clk", base + 0x4270, 0); @@ -882,9 +874,6 @@ static void
> > > __init imx7d_clocks_init(struct device_node *ccm_node)
> > >  	clk_data.clk_num = ARRAY_SIZE(clks);
> > >  	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
> > >
> > > -	for (i = 0; i < ARRAY_SIZE(clks_init_on); i++)
> > > -		clk_prepare_enable(clks[clks_init_on[i]]);
> > > -
> > >  	clk_set_parent(clks[IMX7D_PLL_ARM_MAIN_BYPASS],
> > > clks[IMX7D_PLL_ARM_MAIN]);
> > >  	clk_set_parent(clks[IMX7D_PLL_DRAM_MAIN_BYPASS],
> > > clks[IMX7D_PLL_DRAM_MAIN]);
> > >  	clk_set_parent(clks[IMX7D_PLL_SYS_MAIN_BYPASS],
> > > clks[IMX7D_PLL_SYS_MAIN]); diff --git a/drivers/clk/imx/clk.h
> > > b/drivers/clk/imx/clk.h index 8076ec0..5895e223 100644
> > > --- a/drivers/clk/imx/clk.h
> > > +++ b/drivers/clk/imx/clk.h
> > > @@ -137,6 +137,13 @@ static inline struct clk
> > > *imx_clk_gate_dis(const char *name, const char *parent,
> > >  			shift, CLK_GATE_SET_TO_DISABLE, &imx_ccm_lock);  }
> > >
> > > +static inline struct clk *imx_clk_gate_dis_flags(const char *name,
> > > +const char
> > > *parent,
> > > +		void __iomem *reg, u8 shift, unsigned long flags) {
> > > +	return clk_register_gate(NULL, name, parent, flags |
> > > CLK_SET_RATE_PARENT, reg,
> > > +			shift, CLK_GATE_SET_TO_DISABLE, &imx_ccm_lock); }
> > > +
> > >  static inline struct clk *imx_clk_gate2(const char *name, const
> > > char
> > *parent,
> > >  		void __iomem *reg, u8 shift)
> > >  {
> > > --
> > > 2.7.4

Anson Huang Aug. 14, 2018, 7:31 a.m. UTC | #4

Hi, Peng

Anson Huang
Best Regards!


> -----Original Message-----
> From: Peng Fan
> Sent: Monday, August 13, 2018 9:16 AM
> To: Anson Huang <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; Fabio Estevam
> <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Rob Herring <[e-mail]>
> Cc: dl-linux-imx <[e-mail]>
> Subject: RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
> 
> Hi Anson,
> 
> > > > -----Original Message-----
> > > > From: Anson Huang
> > > > Sent: 2018年8月8日 12:39
> > > > To: [e-mail]; [e-mail];
> > > > [e-mail]; Fabio Estevam <[e-mail]>;
> > > > [e-mail]; [e-mail];
> > > > [e-mail];
> > > > [e-mail]; [e-mail]
> > > > Cc: dl-linux-imx <[e-mail]>
> > > > Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
> > > >
> > > > Clock framework will enable those clocks registered with
> > > > CLK_IS_CRITICAL flag, so no need to have clks_init_on array during
> > > > clock
> > > initialization now.
> > >
> > > Will it be more flexible to parse dts saying "critical-clocks = <xxx>"
> > > or "init-on-arrary=<xxx>"
> > > and enable those clocks?
> >
> > Parsing the clocks arrays from dtb is another way of enabling critical
> > clocks, but for current i.MX6/7 platforms, we implement it in same way
> > as most of other SoCs, currently I did NOT see any necessity of
> > putting them in dtb, just adding flag during clock registering is more
> > simple, if there is any special requirement for different clocks set
> > to be enabled, then we can add support to enable the method of parsing
> critical-clocks from dtb. Just my two cents.
> 
> Thinking about OP-TEE want to use one device, but it's clocks are registered by
> Linux, because there is no module in Linux side use it, it will shutdown the clock,
> which cause OP-TEE could not access the device.
> 
> Then people have to modify clk code to add CLK_IS_CRITICAL flag to make sure
> the clocks are not shutdown by Linux.
> 
> However adding a new property in clk node and let driver code parse the dts,
> there is no need to modify clk driver code when OP-TEE needs another device
> clock.
For supporting OP-TEE or other features which has special requirement about clock
settings, I think we can have another patch to add parsing critical clocks from dtb in
common place of i.MX clock drivers. 

Anson.

> 
> Regards,
> Peng.
> 
> >
> > Anson.
> >
> > >
> > > Regards,
> > > Peng.
> > >
> > > >
> > > > Signed-off-by: Anson Huang <[e-mail]>
> > > > ---
> > > >  drivers/clk/imx/clk-imx7d.c | 27 ++++++++-------------------
> > > >  drivers/clk/imx/clk.h       |  7 +++++++
> > > >  2 files changed, 15 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/imx/clk-imx7d.c
> > > > b/drivers/clk/imx/clk-imx7d.c index c4518d7..076460b 100644
> > > > --- a/drivers/clk/imx/clk-imx7d.c
> > > > +++ b/drivers/clk/imx/clk-imx7d.c
> > > > @@ -379,13 +379,6 @@ static const char *pll_enet_bypass_sel[] = {
> > > > "pll_enet_main", "pll_enet_main_src  static const char
> > > > *pll_audio_bypass_sel[] = { "pll_audio_main",
> > > > "pll_audio_main_src", }; static const char *pll_video_bypass_sel[]
> > > > = { "pll_video_main", "pll_video_main_src", };
> > > >
> > > > -static int const clks_init_on[] __initconst = {
> > > > -	IMX7D_ARM_A7_ROOT_CLK, IMX7D_MAIN_AXI_ROOT_CLK,
> > > > -	IMX7D_PLL_SYS_MAIN_480M_CLK, IMX7D_IPG_ROOT_CLK,
> > > > -	IMX7D_DRAM_PHYM_ROOT_CLK, IMX7D_DRAM_ROOT_CLK,
> > > > -	IMX7D_DRAM_PHYM_ALT_ROOT_CLK,
> IMX7D_DRAM_ALT_ROOT_CLK,
> > > > -};
> > > > -
> > > >  static struct clk_onecell_data clk_data;
> > > >
> > > >  static struct clk ** const uart_clks[] __initconst = { @@ -403,7
> > > > +396,6 @@ static void __init imx7d_clocks_init(struct device_node
> > > *ccm_node)  {
> > > >  	struct device_node *np;
> > > >  	void __iomem *base;
> > > > -	int i;
> > > >
> > > >  	clks[IMX7D_CLK_DUMMY] = imx_clk_fixed("dummy", 0);
> > > >  	clks[IMX7D_OSC_24M_CLK] = of_clk_get_by_name(ccm_node,
> "osc");
> > > @@
> > > > -466,7 +458,7 @@ static void __init imx7d_clocks_init(struct
> > > > device_node
> > > > *ccm_node)
> > > >  	clks[IMX7D_PLL_SYS_MAIN_120M] =
> > > > imx_clk_fixed_factor("pll_sys_main_120m", "pll_sys_main_clk", 1, 4);
> > > >  	clks[IMX7D_PLL_DRAM_MAIN_533M] =
> > > > imx_clk_fixed_factor("pll_dram_533m", "pll_dram_main_clk", 1, 2);
> > > >
> > > > -	clks[IMX7D_PLL_SYS_MAIN_480M_CLK] =
> > > > imx_clk_gate_dis("pll_sys_main_480m_clk", "pll_sys_main_480m",
> > > > base
> > > > + 0xb0, 4);
> > > > +	clks[IMX7D_PLL_SYS_MAIN_480M_CLK] =
> > > > +imx_clk_gate_dis_flags("pll_sys_main_480m_clk",
> > > > +"pll_sys_main_480m", base + 0xb0, 4, CLK_IS_CRITICAL);
> > > >  	clks[IMX7D_PLL_SYS_MAIN_240M_CLK] =
> > > > imx_clk_gate_dis("pll_sys_main_240m_clk", "pll_sys_main_240m",
> > > > base
> > > > + 0xb0, 5);
> > > >  	clks[IMX7D_PLL_SYS_MAIN_120M_CLK] =
> > > > imx_clk_gate_dis("pll_sys_main_120m_clk", "pll_sys_main_120m",
> > > > base
> > > > + 0xb0, 6);
> > > >  	clks[IMX7D_PLL_DRAM_MAIN_533M_CLK] =
> > > > imx_clk_gate("pll_dram_533m_clk", "pll_dram_533m", base + 0x70,
> > > > 12); @@
> > > > -719,7 +711,7 @@ static void __init imx7d_clocks_init(struct
> > > > device_node
> > > > *ccm_node)
> > > >  	clks[IMX7D_ENET_AXI_ROOT_DIV] =
> > > > imx_clk_divider2("enet_axi_post_div", "enet_axi_pre_div", base +
> > > > 0x8900, 0,
> > > 6);
> > > >  	clks[IMX7D_NAND_USDHC_BUS_ROOT_CLK] =
> > > > imx_clk_divider2("nand_usdhc_root_clk", "nand_usdhc_pre_div", base
> > > > + 0x8980, 0, 6);
> > > >  	clks[IMX7D_AHB_CHANNEL_ROOT_DIV] =
> > > > imx_clk_divider2("ahb_root_clk", "ahb_pre_div", base + 0x9000, 0, 6);
> > > > -	clks[IMX7D_IPG_ROOT_CLK] = imx_clk_divider2("ipg_root_clk",
> > > > "ahb_root_clk", base + 0x9080, 0, 2);
> > > > +	clks[IMX7D_IPG_ROOT_CLK] = imx_clk_divider_flags("ipg_root_clk",
> > > > +"ahb_root_clk", base + 0x9080, 0, 2, CLK_IS_CRITICAL |
> > > > +CLK_OPS_PARENT_ENABLE | CLK_SET_RATE_PARENT);
> > > >  	clks[IMX7D_DRAM_ROOT_DIV] = imx_clk_divider2("dram_post_div",
> > > > "dram_cg", base + 0x9880, 0, 3);
> > > >  	clks[IMX7D_DRAM_PHYM_ALT_ROOT_DIV] =
> > > > imx_clk_divider2("dram_phym_alt_post_div",
> > > > "dram_phym_alt_pre_div", base
> > > > + 0xa000, 0, 3);
> > > >  	clks[IMX7D_DRAM_ALT_ROOT_DIV] =
> > > > imx_clk_divider2("dram_alt_post_div", "dram_alt_pre_div", base +
> > > > 0xa080, 0, 3); @@ -783,17 +775,17 @@ static void __init
> > > > imx7d_clocks_init(struct device_node *ccm_node)
> > > >  	clks[IMX7D_CLKO1_ROOT_DIV] = imx_clk_divider2("clko1_post_div",
> > > > "clko1_pre_div", base + 0xbd80, 0, 6);
> > > >  	clks[IMX7D_CLKO2_ROOT_DIV] = imx_clk_divider2("clko2_post_div",
> > > > "clko2_pre_div", base + 0xbe00, 0, 6);
> > > >
> > > > -	clks[IMX7D_ARM_A7_ROOT_CLK] =
> imx_clk_gate4("arm_a7_root_clk",
> > > > "arm_a7_div", base + 0x4000, 0);
> > > > +	clks[IMX7D_ARM_A7_ROOT_CLK] =
> > > > imx_clk_gate2_flags("arm_a7_root_clk",
> > > > +"arm_a7_div", base + 0x4000, 0, CLK_IS_CRITICAL |
> > > > +CLK_OPS_PARENT_ENABLE);
> > > >  	clks[IMX7D_ARM_M4_ROOT_CLK] =
> > imx_clk_gate4("arm_m4_root_clk",
> > > > "arm_m4_div", base + 0x4010, 0);
> > > > -	clks[IMX7D_MAIN_AXI_ROOT_CLK] =
> imx_clk_gate4("main_axi_root_clk",
> > > > "axi_post_div", base + 0x4040, 0);
> > > > +	clks[IMX7D_MAIN_AXI_ROOT_CLK] =
> > > > +imx_clk_gate2_flags("main_axi_root_clk", "axi_post_div", base +
> > > > +0x4040, 0, CLK_IS_CRITICAL | CLK_OPS_PARENT_ENABLE);
> > > >  	clks[IMX7D_DISP_AXI_ROOT_CLK] =
> > imx_clk_gate4("disp_axi_root_clk",
> > > > "disp_axi_post_div", base + 0x4050, 0);
> > > >  	clks[IMX7D_ENET_AXI_ROOT_CLK] =
> > imx_clk_gate4("enet_axi_root_clk",
> > > > "enet_axi_post_div", base + 0x4060, 0);
> > > >  	clks[IMX7D_OCRAM_CLK] = imx_clk_gate4("ocram_clk",
> > > > "main_axi_root_clk", base + 0x4110, 0);
> > > >  	clks[IMX7D_OCRAM_S_CLK] = imx_clk_gate4("ocram_s_clk",
> > > > "ahb_root_clk", base + 0x4120, 0);
> > > > -	clks[IMX7D_DRAM_ROOT_CLK] = imx_clk_gate4("dram_root_clk",
> > > > "dram_post_div", base + 0x4130, 0);
> > > > -	clks[IMX7D_DRAM_PHYM_ROOT_CLK] =
> > > > imx_clk_gate4("dram_phym_root_clk", "dram_phym_cg", base + 0x4130,
> > 0);
> > > > -	clks[IMX7D_DRAM_PHYM_ALT_ROOT_CLK] =
> > > > imx_clk_gate4("dram_phym_alt_root_clk", "dram_phym_alt_post_div",
> > > > base
> > > > + 0x4130, 0);
> > > > -	clks[IMX7D_DRAM_ALT_ROOT_CLK] =
> > > imx_clk_gate4("dram_alt_root_clk",
> > > > "dram_alt_post_div", base + 0x4130, 0);
> > > > +	clks[IMX7D_DRAM_ROOT_CLK] =
> > imx_clk_gate2_flags("dram_root_clk",
> > > > "dram_post_div", base + 0x4130, 0, CLK_IS_CRITICAL |
> > > > CLK_OPS_PARENT_ENABLE);
> > > > +	clks[IMX7D_DRAM_PHYM_ROOT_CLK] =
> > > > imx_clk_gate2_flags("dram_phym_root_clk", "dram_phym_cg", base +
> > > > 0x4130, 0, CLK_IS_CRITICAL | CLK_OPS_PARENT_ENABLE);
> > > > +	clks[IMX7D_DRAM_PHYM_ALT_ROOT_CLK] =
> > > > imx_clk_gate2_flags("dram_phym_alt_root_clk",
> > > > "dram_phym_alt_post_div", base + 0x4130, 0, CLK_IS_CRITICAL |
> > > > CLK_OPS_PARENT_ENABLE);
> > > > +	clks[IMX7D_DRAM_ALT_ROOT_CLK] =
> > > > +imx_clk_gate2_flags("dram_alt_root_clk", "dram_alt_post_div",
> > > > +base
> > > > ++ 0x4130, 0, CLK_IS_CRITICAL | CLK_OPS_PARENT_ENABLE);
> > > >  	clks[IMX7D_OCOTP_CLK] = imx_clk_gate4("ocotp_clk",
> > "ipg_root_clk",
> > > > base + 0x4230, 0);
> > > >  	clks[IMX7D_SNVS_CLK] = imx_clk_gate4("snvs_clk", "ipg_root_clk",
> > > > base + 0x4250, 0);
> > > >  	clks[IMX7D_MU_ROOT_CLK] = imx_clk_gate4("mu_root_clk",
> > > > "ipg_root_clk", base + 0x4270, 0); @@ -882,9 +874,6 @@ static void
> > > > __init imx7d_clocks_init(struct device_node *ccm_node)
> > > >  	clk_data.clk_num = ARRAY_SIZE(clks);
> > > >  	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
> > > >
> > > > -	for (i = 0; i < ARRAY_SIZE(clks_init_on); i++)
> > > > -		clk_prepare_enable(clks[clks_init_on[i]]);
> > > > -
> > > >  	clk_set_parent(clks[IMX7D_PLL_ARM_MAIN_BYPASS],
> > > > clks[IMX7D_PLL_ARM_MAIN]);
> > > >  	clk_set_parent(clks[IMX7D_PLL_DRAM_MAIN_BYPASS],
> > > > clks[IMX7D_PLL_DRAM_MAIN]);
> > > >  	clk_set_parent(clks[IMX7D_PLL_SYS_MAIN_BYPASS],
> > > > clks[IMX7D_PLL_SYS_MAIN]); diff --git a/drivers/clk/imx/clk.h
> > > > b/drivers/clk/imx/clk.h index 8076ec0..5895e223 100644
> > > > --- a/drivers/clk/imx/clk.h
> > > > +++ b/drivers/clk/imx/clk.h
> > > > @@ -137,6 +137,13 @@ static inline struct clk
> > > > *imx_clk_gate_dis(const char *name, const char *parent,
> > > >  			shift, CLK_GATE_SET_TO_DISABLE, &imx_ccm_lock);  }
> > > >
> > > > +static inline struct clk *imx_clk_gate_dis_flags(const char
> > > > +*name, const char
> > > > *parent,
> > > > +		void __iomem *reg, u8 shift, unsigned long flags) {
> > > > +	return clk_register_gate(NULL, name, parent, flags |
> > > > CLK_SET_RATE_PARENT, reg,
> > > > +			shift, CLK_GATE_SET_TO_DISABLE, &imx_ccm_lock); }
> > > > +
> > > >  static inline struct clk *imx_clk_gate2(const char *name, const
> > > > char
> > > *parent,
> > > >  		void __iomem *reg, u8 shift)
> > > >  {
> > > > --
> > > > 2.7.4

Stephen Boyd Aug. 31, 2018, 1:29 a.m. UTC | #5

Quoting Peng Fan (2018-08-12 18:15:41)
> Hi Anson,
> 
> > > > -----Original Message-----
> > > > From: Anson Huang
> > > > Sent: 2018年8月8日 12:39
> > > > To: [e-mail]; [e-mail];
> > > > [e-mail]; Fabio Estevam <[e-mail]>;
> > > > [e-mail]; [e-mail];
> > > > [e-mail];
> > > > [e-mail]; [e-mail]
> > > > Cc: dl-linux-imx <[e-mail]>
> > > > Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
> > > >
> > > > Clock framework will enable those clocks registered with
> > > > CLK_IS_CRITICAL flag, so no need to have clks_init_on array during
> > > > clock
> > > initialization now.
> > >
> > > Will it be more flexible to parse dts saying "critical-clocks = <xxx>"
> > > or "init-on-arrary=<xxx>"
> > > and enable those clocks?
> > 
> > Parsing the clocks arrays from dtb is another way of enabling critical clocks, but
> > for current i.MX6/7 platforms, we implement it in same way as most of other
> > SoCs, currently I did NOT see any necessity of putting them in dtb, just adding
> > flag during clock registering is more simple, if there is any special requirement
> > for different clocks set to be enabled, then we can add support to enable the
> > method of parsing critical-clocks from dtb. Just my two cents.
> 
> Thinking about OP-TEE want to use one device, but it's clocks are registered
> by Linux, because there is no module in Linux side use it, it will shutdown the clock,
> which cause OP-TEE could not access the device.
> 
> Then people have to modify clk code to add CLK_IS_CRITICAL flag to make sure
> the clocks are not shutdown by Linux.
> 
> However adding a new property in clk node and let driver code parse the dts,
> there is no need to modify clk driver code when OP-TEE needs another device clock.
> 

If OP-TEE needs linux to keep things on then why can't the OP-TEE driver
in Linux probe, acquire clocks, and keep the clks enabled forever?

Anson Huang Aug. 31, 2018, 1:40 a.m. UTC | #6

Hi, Stephen

Anson Huang
Best Regards!


> -----Original Message-----
> From: Stephen Boyd <[e-mail]>
> Sent: Friday, August 31, 2018 9:29 AM
> To: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> Anson Huang <[e-mail]>; Fabio Estevam
> <[e-mail]>; Peng Fan <[e-mail]>; Rob Herring
> <[e-mail]>
> Cc: dl-linux-imx <[e-mail]>
> Subject: RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
> 
> Quoting Peng Fan (2018-08-12 18:15:41)
> > Hi Anson,
> >
> > > > > -----Original Message-----
> > > > > From: Anson Huang
> > > > > Sent: 2018年8月8日 12:39
> > > > > To: [e-mail]; [e-mail];
> > > > > [e-mail]; Fabio Estevam <[e-mail]>;
> > > > > [e-mail]; [e-mail];
> > > > > [e-mail];
> > > > > [e-mail]; [e-mail]
> > > > > Cc: dl-linux-imx <[e-mail]>
> > > > > Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
> > > > >
> > > > > Clock framework will enable those clocks registered with
> > > > > CLK_IS_CRITICAL flag, so no need to have clks_init_on array
> > > > > during clock
> > > > initialization now.
> > > >
> > > > Will it be more flexible to parse dts saying "critical-clocks = <xxx>"
> > > > or "init-on-arrary=<xxx>"
> > > > and enable those clocks?
> > >
> > > Parsing the clocks arrays from dtb is another way of enabling
> > > critical clocks, but for current i.MX6/7 platforms, we implement it
> > > in same way as most of other SoCs, currently I did NOT see any
> > > necessity of putting them in dtb, just adding flag during clock
> > > registering is more simple, if there is any special requirement for
> > > different clocks set to be enabled, then we can add support to enable the
> method of parsing critical-clocks from dtb. Just my two cents.
> >
> > Thinking about OP-TEE want to use one device, but it's clocks are
> > registered by Linux, because there is no module in Linux side use it,
> > it will shutdown the clock, which cause OP-TEE could not access the device.
> >
> > Then people have to modify clk code to add CLK_IS_CRITICAL flag to
> > make sure the clocks are not shutdown by Linux.
> >
> > However adding a new property in clk node and let driver code parse
> > the dts, there is no need to modify clk driver code when OP-TEE needs
> another device clock.
> >
> 
> If OP-TEE needs linux to keep things on then why can't the OP-TEE driver in
> Linux probe, acquire clocks, and keep the clks enabled forever?
 
Yes, I agree with you, OP-TEE should maintain its clocks when OP-TEE is enabled.

This is ONLY a concern raised by Peng, all platforms' current clock driver does NOT consider
this case, so I think this patch should be fine for now? Thanks.

Anson.

Jerome Forissier Aug. 31, 2018, 8:01 a.m. UTC | #7

On 08/31/2018 03:29 AM, Stephen Boyd wrote:
> Quoting Peng Fan (2018-08-12 18:15:41)
>> Hi Anson,
>>
>>>>> -----Original Message-----
>>>>> From: Anson Huang
>>>>> Sent: 2018年8月8日 12:39
>>>>> To: [e-mail]; [e-mail];
>>>>> [e-mail]; Fabio Estevam <[e-mail]>;
>>>>> [e-mail]; [e-mail];
>>>>> [e-mail];
>>>>> [e-mail]; [e-mail]
>>>>> Cc: dl-linux-imx <[e-mail]>
>>>>> Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
>>>>>
>>>>> Clock framework will enable those clocks registered with
>>>>> CLK_IS_CRITICAL flag, so no need to have clks_init_on array during
>>>>> clock
>>>> initialization now.
>>>>
>>>> Will it be more flexible to parse dts saying "critical-clocks = <xxx>"
>>>> or "init-on-arrary=<xxx>"
>>>> and enable those clocks?
>>>
>>> Parsing the clocks arrays from dtb is another way of enabling critical clocks, but
>>> for current i.MX6/7 platforms, we implement it in same way as most of other
>>> SoCs, currently I did NOT see any necessity of putting them in dtb, just adding
>>> flag during clock registering is more simple, if there is any special requirement
>>> for different clocks set to be enabled, then we can add support to enable the
>>> method of parsing critical-clocks from dtb. Just my two cents.
>>
>> Thinking about OP-TEE want to use one device, but it's clocks are registered
>> by Linux, because there is no module in Linux side use it, it will shutdown the clock,
>> which cause OP-TEE could not access the device.
>>
>> Then people have to modify clk code to add CLK_IS_CRITICAL flag to make sure
>> the clocks are not shutdown by Linux.
>>
>> However adding a new property in clk node and let driver code parse the dts,
>> there is no need to modify clk driver code when OP-TEE needs another device clock.
>>
> 
> If OP-TEE needs linux to keep things on then why can't the OP-TEE driver
> in Linux probe, acquire clocks, and keep the clks enabled forever?

Sounds reasonable, but how could this be done without introducing
platform-specific stuff in the OP-TEE driver?

> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

Stephen Boyd Aug. 31, 2018, 5:57 p.m. UTC | #8

Quoting Jerome Forissier (2018-08-31 01:01:44)
> 
> 
> On 08/31/2018 03:29 AM, Stephen Boyd wrote:
> > Quoting Peng Fan (2018-08-12 18:15:41)
> >> Hi Anson,
> >>
> >>>>> -----Original Message-----
> >>>>> From: Anson Huang
> >>>>> Sent: 2018年8月8日 12:39
> >>>>> To: [e-mail]; [e-mail];
> >>>>> [e-mail]; Fabio Estevam <[e-mail]>;
> >>>>> [e-mail]; [e-mail];
> >>>>> [e-mail];
> >>>>> [e-mail]; [e-mail]
> >>>>> Cc: dl-linux-imx <[e-mail]>
> >>>>> Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
> >>>>>
> >>>>> Clock framework will enable those clocks registered with
> >>>>> CLK_IS_CRITICAL flag, so no need to have clks_init_on array during
> >>>>> clock
> >>>> initialization now.
> >>>>
> >>>> Will it be more flexible to parse dts saying "critical-clocks = <xxx>"
> >>>> or "init-on-arrary=<xxx>"
> >>>> and enable those clocks?
> >>>
> >>> Parsing the clocks arrays from dtb is another way of enabling critical clocks, but
> >>> for current i.MX6/7 platforms, we implement it in same way as most of other
> >>> SoCs, currently I did NOT see any necessity of putting them in dtb, just adding
> >>> flag during clock registering is more simple, if there is any special requirement
> >>> for different clocks set to be enabled, then we can add support to enable the
> >>> method of parsing critical-clocks from dtb. Just my two cents.
> >>
> >> Thinking about OP-TEE want to use one device, but it's clocks are registered
> >> by Linux, because there is no module in Linux side use it, it will shutdown the clock,
> >> which cause OP-TEE could not access the device.
> >>
> >> Then people have to modify clk code to add CLK_IS_CRITICAL flag to make sure
> >> the clocks are not shutdown by Linux.
> >>
> >> However adding a new property in clk node and let driver code parse the dts,
> >> there is no need to modify clk driver code when OP-TEE needs another device clock.
> >>
> > 
> > If OP-TEE needs linux to keep things on then why can't the OP-TEE driver
> > in Linux probe, acquire clocks, and keep the clks enabled forever?
> 
> Sounds reasonable, but how could this be done without introducing
> platform-specific stuff in the OP-TEE driver?
> 

Why is that a goal?

Anson Huang Sept. 3, 2018, 7:20 a.m. UTC | #9

Anson Huang
Best Regards!


> -----Original Message-----
> From: Stephen Boyd <[e-mail]>
> Sent: Saturday, September 1, 2018 1:58 AM
> To: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> Anson Huang <[e-mail]>; Fabio Estevam
> <[e-mail]>; Jerome Forissier <[e-mail]>;
> Peng Fan <[e-mail]>; Rob Herring <[e-mail]>
> Cc: dl-linux-imx <[e-mail]>
> Subject: Re: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
> 
> Quoting Jerome Forissier (2018-08-31 01:01:44)
> >
> >
> > On 08/31/2018 03:29 AM, Stephen Boyd wrote:
> > > Quoting Peng Fan (2018-08-12 18:15:41)
> > >> Hi Anson,
> > >>
> > >>>>> -----Original Message-----
> > >>>>> From: Anson Huang
> > >>>>> Sent: 2018年8月8日 12:39
> > >>>>> To: [e-mail]; [e-mail];
> > >>>>> [e-mail]; Fabio Estevam <[e-mail]>;
> > >>>>> [e-mail]; [e-mail];
> > >>>>> [e-mail];
> > >>>>> [e-mail]; [e-mail]
> > >>>>> Cc: dl-linux-imx <[e-mail]>
> > >>>>> Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
> > >>>>>
> > >>>>> Clock framework will enable those clocks registered with
> > >>>>> CLK_IS_CRITICAL flag, so no need to have clks_init_on array
> > >>>>> during clock
> > >>>> initialization now.
> > >>>>
> > >>>> Will it be more flexible to parse dts saying "critical-clocks = <xxx>"
> > >>>> or "init-on-arrary=<xxx>"
> > >>>> and enable those clocks?
> > >>>
> > >>> Parsing the clocks arrays from dtb is another way of enabling
> > >>> critical clocks, but for current i.MX6/7 platforms, we implement
> > >>> it in same way as most of other SoCs, currently I did NOT see any
> > >>> necessity of putting them in dtb, just adding flag during clock
> > >>> registering is more simple, if there is any special requirement
> > >>> for different clocks set to be enabled, then we can add support to enable
> the method of parsing critical-clocks from dtb. Just my two cents.
> > >>
> > >> Thinking about OP-TEE want to use one device, but it's clocks are
> > >> registered by Linux, because there is no module in Linux side use
> > >> it, it will shutdown the clock, which cause OP-TEE could not access the
> device.
> > >>
> > >> Then people have to modify clk code to add CLK_IS_CRITICAL flag to
> > >> make sure the clocks are not shutdown by Linux.
> > >>
> > >> However adding a new property in clk node and let driver code parse
> > >> the dts, there is no need to modify clk driver code when OP-TEE needs
> another device clock.
> > >>
> > >
> > > If OP-TEE needs linux to keep things on then why can't the OP-TEE
> > > driver in Linux probe, acquire clocks, and keep the clks enabled forever?
> >
> > Sounds reasonable, but how could this be done without introducing
> > platform-specific stuff in the OP-TEE driver?
> >
> 
> Why is that a goal?
 
I do NOT think we should consider such case in this patch series, whatever OP-TEE needs for its own
feature, it should do necessary operations either in its driver or somewhere else by adding new patch.

Anson.

Anson Huang Sept. 10, 2018, 9:18 a.m. UTC | #10

Gentle Ping...

Anson Huang
Best Regards!


> -----Original Message-----
> From: Anson Huang
> Sent: Monday, September 3, 2018 3:21 PM
> To: Stephen Boyd <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Fabio Estevam
> <[e-mail]>; Jerome Forissier <[e-mail]>;
> Peng Fan <[e-mail]>; Rob Herring <[e-mail]>
> Cc: dl-linux-imx <[e-mail]>
> Subject: RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
> 
> 
> 
> Anson Huang
> Best Regards!
> 
> 
> > -----Original Message-----
> > From: Stephen Boyd <[e-mail]>
> > Sent: Saturday, September 1, 2018 1:58 AM
> > To: [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> [e-mail];
> > Anson Huang <[e-mail]>; Fabio Estevam
> > <[e-mail]>; Jerome Forissier
> > <[e-mail]>; Peng Fan <[e-mail]>; Rob
> > Herring <[e-mail]>
> > Cc: dl-linux-imx <[e-mail]>
> > Subject: Re: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
> >
> > Quoting Jerome Forissier (2018-08-31 01:01:44)
> > >
> > >
> > > On 08/31/2018 03:29 AM, Stephen Boyd wrote:
> > > > Quoting Peng Fan (2018-08-12 18:15:41)
> > > >> Hi Anson,
> > > >>
> > > >>>>> -----Original Message-----
> > > >>>>> From: Anson Huang
> > > >>>>> Sent: 2018年8月8日 12:39
> > > >>>>> To: [e-mail]; [e-mail];
> > > >>>>> [e-mail]; Fabio Estevam <[e-mail]>;
> > > >>>>> [e-mail]; [e-mail];
> > > >>>>> [e-mail];
> > > >>>>> [e-mail]; [e-mail]
> > > >>>>> Cc: dl-linux-imx <[e-mail]>
> > > >>>>> Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on
> > > >>>>> array
> > > >>>>>
> > > >>>>> Clock framework will enable those clocks registered with
> > > >>>>> CLK_IS_CRITICAL flag, so no need to have clks_init_on array
> > > >>>>> during clock
> > > >>>> initialization now.
> > > >>>>
> > > >>>> Will it be more flexible to parse dts saying "critical-clocks = <xxx>"
> > > >>>> or "init-on-arrary=<xxx>"
> > > >>>> and enable those clocks?
> > > >>>
> > > >>> Parsing the clocks arrays from dtb is another way of enabling
> > > >>> critical clocks, but for current i.MX6/7 platforms, we implement
> > > >>> it in same way as most of other SoCs, currently I did NOT see
> > > >>> any necessity of putting them in dtb, just adding flag during
> > > >>> clock registering is more simple, if there is any special
> > > >>> requirement for different clocks set to be enabled, then we can
> > > >>> add support to enable
> > the method of parsing critical-clocks from dtb. Just my two cents.
> > > >>
> > > >> Thinking about OP-TEE want to use one device, but it's clocks are
> > > >> registered by Linux, because there is no module in Linux side use
> > > >> it, it will shutdown the clock, which cause OP-TEE could not
> > > >> access the
> > device.
> > > >>
> > > >> Then people have to modify clk code to add CLK_IS_CRITICAL flag
> > > >> to make sure the clocks are not shutdown by Linux.
> > > >>
> > > >> However adding a new property in clk node and let driver code
> > > >> parse the dts, there is no need to modify clk driver code when
> > > >> OP-TEE needs
> > another device clock.
> > > >>
> > > >
> > > > If OP-TEE needs linux to keep things on then why can't the OP-TEE
> > > > driver in Linux probe, acquire clocks, and keep the clks enabled forever?
> > >
> > > Sounds reasonable, but how could this be done without introducing
> > > platform-specific stuff in the OP-TEE driver?
> > >
> >
> > Why is that a goal?
> 
> I do NOT think we should consider such case in this patch series, whatever
> OP-TEE needs for its own feature, it should do necessary operations either in its
> driver or somewhere else by adding new patch.
> 
> Anson.

Stephen Boyd Oct. 8, 2018, 7:40 a.m. UTC | #11

Quoting Anson Huang (2018-09-03 00:20:53)
> > > On 08/31/2018 03:29 AM, Stephen Boyd wrote:
> > > > Quoting Peng Fan (2018-08-12 18:15:41)
> > > >> Hi Anson,
> > > >>
> > > >>>>> -----Original Message-----
> > > >>>>> From: Anson Huang
> > > >>>>> Sent: 2018年8月8日 12:39
> > > >>>>> To: [e-mail]; [e-mail];
> > > >>>>> [e-mail]; Fabio Estevam <[e-mail]>;
> > > >>>>> [e-mail]; [e-mail];
> > > >>>>> [e-mail];
> > > >>>>> [e-mail]; [e-mail]
> > > >>>>> Cc: dl-linux-imx <[e-mail]>
> > > >>>>> Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
> > > >>>>>
> > > >>>>> Clock framework will enable those clocks registered with
> > > >>>>> CLK_IS_CRITICAL flag, so no need to have clks_init_on array
> > > >>>>> during clock
> > > >>>> initialization now.
> > > >>>>
> > > >>>> Will it be more flexible to parse dts saying "critical-clocks = <xxx>"
> > > >>>> or "init-on-arrary=<xxx>"
> > > >>>> and enable those clocks?
> > > >>>
> > > >>> Parsing the clocks arrays from dtb is another way of enabling
> > > >>> critical clocks, but for current i.MX6/7 platforms, we implement
> > > >>> it in same way as most of other SoCs, currently I did NOT see any
> > > >>> necessity of putting them in dtb, just adding flag during clock
> > > >>> registering is more simple, if there is any special requirement
> > > >>> for different clocks set to be enabled, then we can add support to enable
> > the method of parsing critical-clocks from dtb. Just my two cents.
> > > >>
> > > >> Thinking about OP-TEE want to use one device, but it's clocks are
> > > >> registered by Linux, because there is no module in Linux side use
> > > >> it, it will shutdown the clock, which cause OP-TEE could not access the
> > device.
> > > >>
> > > >> Then people have to modify clk code to add CLK_IS_CRITICAL flag to
> > > >> make sure the clocks are not shutdown by Linux.
> > > >>
> > > >> However adding a new property in clk node and let driver code parse
> > > >> the dts, there is no need to modify clk driver code when OP-TEE needs
> > another device clock.
> > > >>
> > > >
> > > > If OP-TEE needs linux to keep things on then why can't the OP-TEE
> > > > driver in Linux probe, acquire clocks, and keep the clks enabled forever?
> > >
> > > Sounds reasonable, but how could this be done without introducing
> > > platform-specific stuff in the OP-TEE driver?
> > >
> > 
> > Why is that a goal?
>  
> I do NOT think we should consider such case in this patch series, whatever OP-TEE needs for its own
> feature, it should do necessary operations either in its driver or somewhere else by adding new patch.
> 

Why can't we add clks to the op-tee node in DT's /firmware container?
Then any clks in there can be turned on forever and left enabled by the
linux driver?

Anson Huang Oct. 8, 2018, 8:34 a.m. UTC | #12

Anson Huang
Best Regards!


> -----Original Message-----
> From: Stephen Boyd <[e-mail]>
> Sent: Monday, October 8, 2018 3:41 PM
> To: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> Anson Huang <[e-mail]>; Fabio Estevam
> <[e-mail]>; Jerome Forissier <[e-mail]>;
> Peng Fan <[e-mail]>; Rob Herring <[e-mail]>
> Cc: dl-linux-imx <[e-mail]>
> Subject: RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
> 
> Quoting Anson Huang (2018-09-03 00:20:53)
> > > > On 08/31/2018 03:29 AM, Stephen Boyd wrote:
> > > > > Quoting Peng Fan (2018-08-12 18:15:41)
> > > > >> Hi Anson,
> > > > >>
> > > > >>>>> -----Original Message-----
> > > > >>>>> From: Anson Huang
> > > > >>>>> Sent: 2018年8月8日 12:39
> > > > >>>>> To: [e-mail]; [e-mail];
> > > > >>>>> [e-mail]; Fabio Estevam
> > > > >>>>> <[e-mail]>; [e-mail];
> > > > >>>>> [e-mail]; [e-mail];
> > > > >>>>> [e-mail]; [e-mail]
> > > > >>>>> Cc: dl-linux-imx <[e-mail]>
> > > > >>>>> Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on
> > > > >>>>> array
> > > > >>>>>
> > > > >>>>> Clock framework will enable those clocks registered with
> > > > >>>>> CLK_IS_CRITICAL flag, so no need to have clks_init_on array
> > > > >>>>> during clock
> > > > >>>> initialization now.
> > > > >>>>
> > > > >>>> Will it be more flexible to parse dts saying "critical-clocks = <xxx>"
> > > > >>>> or "init-on-arrary=<xxx>"
> > > > >>>> and enable those clocks?
> > > > >>>
> > > > >>> Parsing the clocks arrays from dtb is another way of enabling
> > > > >>> critical clocks, but for current i.MX6/7 platforms, we
> > > > >>> implement it in same way as most of other SoCs, currently I
> > > > >>> did NOT see any necessity of putting them in dtb, just adding
> > > > >>> flag during clock registering is more simple, if there is any
> > > > >>> special requirement for different clocks set to be enabled,
> > > > >>> then we can add support to enable
> > > the method of parsing critical-clocks from dtb. Just my two cents.
> > > > >>
> > > > >> Thinking about OP-TEE want to use one device, but it's clocks
> > > > >> are registered by Linux, because there is no module in Linux
> > > > >> side use it, it will shutdown the clock, which cause OP-TEE
> > > > >> could not access the
> > > device.
> > > > >>
> > > > >> Then people have to modify clk code to add CLK_IS_CRITICAL flag
> > > > >> to make sure the clocks are not shutdown by Linux.
> > > > >>
> > > > >> However adding a new property in clk node and let driver code
> > > > >> parse the dts, there is no need to modify clk driver code when
> > > > >> OP-TEE needs
> > > another device clock.
> > > > >>
> > > > >
> > > > > If OP-TEE needs linux to keep things on then why can't the
> > > > > OP-TEE driver in Linux probe, acquire clocks, and keep the clks enabled
> forever?
> > > >
> > > > Sounds reasonable, but how could this be done without introducing
> > > > platform-specific stuff in the OP-TEE driver?
> > > >
> > >
> > > Why is that a goal?
> >
> > I do NOT think we should consider such case in this patch series,
> > whatever OP-TEE needs for its own feature, it should do necessary operations
> either in its driver or somewhere else by adding new patch.
> >
> 
> Why can't we add clks to the op-tee node in DT's /firmware container?
> Then any clks in there can be turned on forever and left enabled by the linux
> driver?
 
I did NOT run op-tee with Linux-next kernel before, can you advise more? And I think if op-tee has such requirement,
can we have another patch to cover it? I believe all other i.MX platforms also have same
requirements if considering op-tee support, so I think it should be another topic, what do you think?

Anson.

Stephen Boyd Oct. 12, 2018, 7:48 p.m. UTC | #13

Quoting Anson Huang (2018-10-08 01:34:59)
> > Quoting Anson Huang (2018-09-03 00:20:53)
> > > > > On 08/31/2018 03:29 AM, Stephen Boyd wrote:
> > > > > > Quoting Peng Fan (2018-08-12 18:15:41)
> > > > > >> Hi Anson,
> > > > > >>
> > > > > >>>>> -----Original Message-----
> > > > > >>>>> From: Anson Huang
> > > > > >>>>> Sent: 2018年8月8日 12:39
> > > > > >>>>> To: [e-mail]; [e-mail];
> > > > > >>>>> [e-mail]; Fabio Estevam
> > > > > >>>>> <[e-mail]>; [e-mail];
> > > > > >>>>> [e-mail]; [e-mail];
> > > > > >>>>> [e-mail]; [e-mail]
> > > > > >>>>> Cc: dl-linux-imx <[e-mail]>
> > > > > >>>>> Subject: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on
> > > > > >>>>> array
> > > > > >>>>>
> > > > > >>>>> Clock framework will enable those clocks registered with
> > > > > >>>>> CLK_IS_CRITICAL flag, so no need to have clks_init_on array
> > > > > >>>>> during clock
> > > > > >>>> initialization now.
> > > > > >>>>
> > > > > >>>> Will it be more flexible to parse dts saying "critical-clocks = <xxx>"
> > > > > >>>> or "init-on-arrary=<xxx>"
> > > > > >>>> and enable those clocks?
> > > > > >>>
> > > > > >>> Parsing the clocks arrays from dtb is another way of enabling
> > > > > >>> critical clocks, but for current i.MX6/7 platforms, we
> > > > > >>> implement it in same way as most of other SoCs, currently I
> > > > > >>> did NOT see any necessity of putting them in dtb, just adding
> > > > > >>> flag during clock registering is more simple, if there is any
> > > > > >>> special requirement for different clocks set to be enabled,
> > > > > >>> then we can add support to enable
> > > > the method of parsing critical-clocks from dtb. Just my two cents.
> > > > > >>
> > > > > >> Thinking about OP-TEE want to use one device, but it's clocks
> > > > > >> are registered by Linux, because there is no module in Linux
> > > > > >> side use it, it will shutdown the clock, which cause OP-TEE
> > > > > >> could not access the
> > > > device.
> > > > > >>
> > > > > >> Then people have to modify clk code to add CLK_IS_CRITICAL flag
> > > > > >> to make sure the clocks are not shutdown by Linux.
> > > > > >>
> > > > > >> However adding a new property in clk node and let driver code
> > > > > >> parse the dts, there is no need to modify clk driver code when
> > > > > >> OP-TEE needs
> > > > another device clock.
> > > > > >>
> > > > > >
> > > > > > If OP-TEE needs linux to keep things on then why can't the
> > > > > > OP-TEE driver in Linux probe, acquire clocks, and keep the clks enabled
> > forever?
> > > > >
> > > > > Sounds reasonable, but how could this be done without introducing
> > > > > platform-specific stuff in the OP-TEE driver?
> > > > >
> > > >
> > > > Why is that a goal?
> > >
> > > I do NOT think we should consider such case in this patch series,
> > > whatever OP-TEE needs for its own feature, it should do necessary operations
> > either in its driver or somewhere else by adding new patch.
> > >
> > 
> > Why can't we add clks to the op-tee node in DT's /firmware container?
> > Then any clks in there can be turned on forever and left enabled by the linux
> > driver?
>  
> I did NOT run op-tee with Linux-next kernel before, can you advise more?

Neither have I, so I can't advise more.

> And I think if op-tee has such requirement,
> can we have another patch to cover it?

Yes.


> I believe all other i.MX platforms also have same
> requirements if considering op-tee support, so I think it should be another topic, what do you think?
> 

I'm going to drop these patches from my review queue. Please resend them
and please include the op-tee patches too.

Anson Huang Oct. 15, 2018, 9:33 a.m. UTC | #14

Hi, Stephen

Anson Huang
Best Regards!


> -----Original Message-----
> From: Stephen Boyd <[e-mail]>
> Sent: Saturday, October 13, 2018 3:48 AM
> To: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> Anson Huang <[e-mail]>; Fabio Estevam
> <[e-mail]>; Jerome Forissier <[e-mail]>;
> Peng Fan <[e-mail]>; Rob Herring <[e-mail]>
> Cc: dl-linux-imx <[e-mail]>
> Subject: RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
> 
> Quoting Anson Huang (2018-10-08 01:34:59)
> > > Quoting Anson Huang (2018-09-03 00:20:53)
> > > > > > On 08/31/2018 03:29 AM, Stephen Boyd wrote:
> > > > > > > Quoting Peng Fan (2018-08-12 18:15:41)
> > > > > > >> Hi Anson,
> > > > > > >>
> > > > > > >>>>> -----Original Message-----
> > > > > > >>>>> From: Anson Huang
> > > > > > >>>>> Sent: 2018年8月8日 12:39
> > > > > > >>>>> To: [e-mail]; [e-mail];
> > > > > > >>>>> [e-mail]; Fabio Estevam
> > > > > > >>>>> <[e-mail]>; [e-mail];
> > > > > > >>>>> [e-mail]; [e-mail];
> > > > > > >>>>> [e-mail]; [e-mail]
> > > > > > >>>>> Cc: dl-linux-imx <[e-mail]>
> > > > > > >>>>> Subject: [PATCH 2/2] clk: imx: imx7d: remove
> > > > > > >>>>> clks_init_on array
> > > > > > >>>>>
> > > > > > >>>>> Clock framework will enable those clocks registered with
> > > > > > >>>>> CLK_IS_CRITICAL flag, so no need to have clks_init_on
> > > > > > >>>>> array during clock
> > > > > > >>>> initialization now.
> > > > > > >>>>
> > > > > > >>>> Will it be more flexible to parse dts saying "critical-clocks =
> <xxx>"
> > > > > > >>>> or "init-on-arrary=<xxx>"
> > > > > > >>>> and enable those clocks?
> > > > > > >>>
> > > > > > >>> Parsing the clocks arrays from dtb is another way of
> > > > > > >>> enabling critical clocks, but for current i.MX6/7
> > > > > > >>> platforms, we implement it in same way as most of other
> > > > > > >>> SoCs, currently I did NOT see any necessity of putting
> > > > > > >>> them in dtb, just adding flag during clock registering is
> > > > > > >>> more simple, if there is any special requirement for
> > > > > > >>> different clocks set to be enabled, then we can add
> > > > > > >>> support to enable
> > > > > the method of parsing critical-clocks from dtb. Just my two cents.
> > > > > > >>
> > > > > > >> Thinking about OP-TEE want to use one device, but it's
> > > > > > >> clocks are registered by Linux, because there is no module
> > > > > > >> in Linux side use it, it will shutdown the clock, which
> > > > > > >> cause OP-TEE could not access the
> > > > > device.
> > > > > > >>
> > > > > > >> Then people have to modify clk code to add CLK_IS_CRITICAL
> > > > > > >> flag to make sure the clocks are not shutdown by Linux.
> > > > > > >>
> > > > > > >> However adding a new property in clk node and let driver
> > > > > > >> code parse the dts, there is no need to modify clk driver
> > > > > > >> code when OP-TEE needs
> > > > > another device clock.
> > > > > > >>
> > > > > > >
> > > > > > > If OP-TEE needs linux to keep things on then why can't the
> > > > > > > OP-TEE driver in Linux probe, acquire clocks, and keep the
> > > > > > > clks enabled
> > > forever?
> > > > > >
> > > > > > Sounds reasonable, but how could this be done without
> > > > > > introducing platform-specific stuff in the OP-TEE driver?
> > > > > >
> > > > >
> > > > > Why is that a goal?
> > > >
> > > > I do NOT think we should consider such case in this patch series,
> > > > whatever OP-TEE needs for its own feature, it should do necessary
> > > > operations
> > > either in its driver or somewhere else by adding new patch.
> > > >
> > >
> > > Why can't we add clks to the op-tee node in DT's /firmware container?
> > > Then any clks in there can be turned on forever and left enabled by
> > > the linux driver?
> >
> > I did NOT run op-tee with Linux-next kernel before, can you advise more?
> 
> Neither have I, so I can't advise more.
> 
> > And I think if op-tee has such requirement, can we have another patch
> > to cover it?
> 
> Yes.
> 
> 
> > I believe all other i.MX platforms also have same requirements if
> > considering op-tee support, so I think it should be another topic, what do you
> think?
> >
> 
> I'm going to drop these patches from my review queue. Please resend them
> and please include the op-tee patches too.


I do NOT know how to include the op-tee patch to meet special requirement, should
the op-tee related patch be added later when someone actually add the op-tee support for i.MX7?
It should NOT block this patch set, do you think we can add this patch set first?

Anson.

Stephen Boyd Oct. 15, 2018, 4:45 p.m. UTC | #15

Quoting Anson Huang (2018-10-15 02:33:35)
> > > > >
> > > >
> > > > Why can't we add clks to the op-tee node in DT's /firmware container?
> > > > Then any clks in there can be turned on forever and left enabled by
> > > > the linux driver?
> > >
> > > I did NOT run op-tee with Linux-next kernel before, can you advise more?
> > 
> > Neither have I, so I can't advise more.
> > 
> > > And I think if op-tee has such requirement, can we have another patch
> > > to cover it?
> > 
> > Yes.
> > 
> > 
> > > I believe all other i.MX platforms also have same requirements if
> > > considering op-tee support, so I think it should be another topic, what do you
> > think?
> > >
> > 
> > I'm going to drop these patches from my review queue. Please resend them
> > and please include the op-tee patches too.
> 
> 
> I do NOT know how to include the op-tee patch to meet special requirement, should
> the op-tee related patch be added later when someone actually add the op-tee support for i.MX7?
> It should NOT block this patch set, do you think we can add this patch set first?
> 

Please resend the two patches. In the commit text for the second patch,
describe the plan to remove CLK_IS_CRITICAL from these clks by adding an
OP-TEE device/driver to the kernel to keep these clks enabled. My
understanding is that there isn't an OP-TEE driver right now, but these
clks are used by the firmware and can't be turned off in Linux. If in
the future we want to be able to turn them on and off, we'll need to add
them to an OP-TEE device node and have that driver manage the clks.

How that will work when a system doesn't enable the OP-TEE driver I'm
not sure. We may need to develop some system whereby clks like this are
handed from the clk controller to the consumer driver when it's enabled
for further power managment, but if they're never handed off, they're
kept on forever like is done here. Anyway, please resend with a note
about why these are marked CLK_IS_CRITICAL.

Anson Huang Oct. 16, 2018, 4:37 a.m. UTC | #16

Hi, Stephen

Anson Huang
Best Regards!


> -----Original Message-----
> From: Stephen Boyd <[e-mail]>
> Sent: Tuesday, October 16, 2018 12:46 AM
> To: [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> Anson Huang <[e-mail]>; Fabio Estevam
> <[e-mail]>; Jerome Forissier <[e-mail]>;
> Peng Fan <[e-mail]>; Rob Herring <[e-mail]>
> Cc: dl-linux-imx <[e-mail]>
> Subject: RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
> 
> Quoting Anson Huang (2018-10-15 02:33:35)
> > > > > >
> > > > >
> > > > > Why can't we add clks to the op-tee node in DT's /firmware container?
> > > > > Then any clks in there can be turned on forever and left enabled
> > > > > by the linux driver?
> > > >
> > > > I did NOT run op-tee with Linux-next kernel before, can you advise more?
> > >
> > > Neither have I, so I can't advise more.
> > >
> > > > And I think if op-tee has such requirement, can we have another
> > > > patch to cover it?
> > >
> > > Yes.
> > >
> > >
> > > > I believe all other i.MX platforms also have same requirements if
> > > > considering op-tee support, so I think it should be another topic,
> > > > what do you
> > > think?
> > > >
> > >
> > > I'm going to drop these patches from my review queue. Please resend
> > > them and please include the op-tee patches too.
> >
> >
> > I do NOT know how to include the op-tee patch to meet special
> > requirement, should the op-tee related patch be added later when someone
> actually add the op-tee support for i.MX7?
> > It should NOT block this patch set, do you think we can add this patch set
> first?
> >
> 
> Please resend the two patches. In the commit text for the second patch,
> describe the plan to remove CLK_IS_CRITICAL from these clks by adding an
> OP-TEE device/driver to the kernel to keep these clks enabled. My
> understanding is that there isn't an OP-TEE driver right now, but these clks are
> used by the firmware and can't be turned off in Linux. If in the future we want
> to be able to turn them on and off, we'll need to add them to an OP-TEE device
> node and have that driver manage the clks.
> 
> How that will work when a system doesn't enable the OP-TEE driver I'm not
> sure. We may need to develop some system whereby clks like this are handed
> from the clk controller to the consumer driver when it's enabled for further
> power managment, but if they're never handed off, they're kept on forever like
> is done here. Anyway, please resend with a note about why these are marked
> CLK_IS_CRITICAL.
 
I think there is some misunderstanding here, this patch sets those critical clocks
with CLK_IS_CRITICAL flag to let clock driver keep them always ON, as they are
critical clocks and system (Linux kernel, NOT include op-tee) can NOT run normally
if anyone of them is OFF.

The question about op-tee is, there might be some special clocks needed by op-tee,
for example, currently clk A, B and C are kept always ON for Linux kernel to run normally,
but when op-tee is executing, it might need clk D to be ON, so I think the clk D is needed
ONLY for op-tee, op-tee should have a driver or firmware to runtime ON/OFF clk D as needed.
Since I do NOT know what clocks op-tee needs, so I will leave them to be added by op-tee
owner, whoever enables op-tee, he should consider where to manage its special clocks,
either in clk driver or op-tee driver/firmware.

So do we still need to mention the op-tee related info in commit text? Actually, this patch
is just to replace those always-ON clocks in clks_init_on array with CLK_IS_CRITICAL flag.
Then no need to explicitly enable clocks in clks_init_on array and just rely on common clk
framework which will enable all clocks with CLK_IS_CRITICAL flag set, it will align with all
other i.MX SoCs clk driver.

Anson.

Stephen Boyd Oct. 16, 2018, 10:24 p.m. UTC | #17

Quoting Anson Huang (2018-10-15 21:37:31)
> Hi, Stephen
> 
> Anson Huang
> Best Regards!
> 
> 
> > -----Original Message-----
> > From: Stephen Boyd <[e-mail]>
> > Sent: Tuesday, October 16, 2018 12:46 AM
> > To: [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > Anson Huang <[e-mail]>; Fabio Estevam
> > <[e-mail]>; Jerome Forissier <[e-mail]>;
> > Peng Fan <[e-mail]>; Rob Herring <[e-mail]>
> > Cc: dl-linux-imx <[e-mail]>
> > Subject: RE: [PATCH 2/2] clk: imx: imx7d: remove clks_init_on array
> > 
> > Quoting Anson Huang (2018-10-15 02:33:35)
> > > > > > >
> > > > > >
> > > > > > Why can't we add clks to the op-tee node in DT's /firmware container?
> > > > > > Then any clks in there can be turned on forever and left enabled
> > > > > > by the linux driver?
> > > > >
> > > > > I did NOT run op-tee with Linux-next kernel before, can you advise more?
> > > >
> > > > Neither have I, so I can't advise more.
> > > >
> > > > > And I think if op-tee has such requirement, can we have another
> > > > > patch to cover it?
> > > >
> > > > Yes.
> > > >
> > > >
> > > > > I believe all other i.MX platforms also have same requirements if
> > > > > considering op-tee support, so I think it should be another topic,
> > > > > what do you
> > > > think?
> > > > >
> > > >
> > > > I'm going to drop these patches from my review queue. Please resend
> > > > them and please include the op-tee patches too.
> > >
> > >
> > > I do NOT know how to include the op-tee patch to meet special
> > > requirement, should the op-tee related patch be added later when someone
> > actually add the op-tee support for i.MX7?
> > > It should NOT block this patch set, do you think we can add this patch set
> > first?
> > >
> > 
> > Please resend the two patches. In the commit text for the second patch,
> > describe the plan to remove CLK_IS_CRITICAL from these clks by adding an
> > OP-TEE device/driver to the kernel to keep these clks enabled. My
> > understanding is that there isn't an OP-TEE driver right now, but these clks are
> > used by the firmware and can't be turned off in Linux. If in the future we want
> > to be able to turn them on and off, we'll need to add them to an OP-TEE device
> > node and have that driver manage the clks.
> > 
> > How that will work when a system doesn't enable the OP-TEE driver I'm not
> > sure. We may need to develop some system whereby clks like this are handed
> > from the clk controller to the consumer driver when it's enabled for further
> > power managment, but if they're never handed off, they're kept on forever like
> > is done here. Anyway, please resend with a note about why these are marked
> > CLK_IS_CRITICAL.
>  
> I think there is some misunderstanding here, this patch sets those critical clocks
> with CLK_IS_CRITICAL flag to let clock driver keep them always ON, as they are
> critical clocks and system (Linux kernel, NOT include op-tee) can NOT run normally
> if anyone of them is OFF.

Ok it sounds like OP-TEE usage completely orthogonal here? I'll go apply
these refactorings then.

Patch

diff --git a/drivers/clk/imx/clk-imx7d.c b/drivers/clk/imx/clk-imx7d.c
index c4518d7..076460b 100644
--- a/drivers/clk/imx/clk-imx7d.c
+++ b/drivers/clk/imx/clk-imx7d.c
@@ -379,13 +379,6 @@  static const char *pll_enet_bypass_sel[] = { "pll_enet_main", "pll_enet_main_src
 static const char *pll_audio_bypass_sel[] = { "pll_audio_main", "pll_audio_main_src", };
 static const char *pll_video_bypass_sel[] = { "pll_video_main", "pll_video_main_src", };
 
-static int const clks_init_on[] __initconst = {
-	IMX7D_ARM_A7_ROOT_CLK, IMX7D_MAIN_AXI_ROOT_CLK,
-	IMX7D_PLL_SYS_MAIN_480M_CLK, IMX7D_IPG_ROOT_CLK,
-	IMX7D_DRAM_PHYM_ROOT_CLK, IMX7D_DRAM_ROOT_CLK,
-	IMX7D_DRAM_PHYM_ALT_ROOT_CLK, IMX7D_DRAM_ALT_ROOT_CLK,
-};
-
 static struct clk_onecell_data clk_data;
 
 static struct clk ** const uart_clks[] __initconst = {
@@ -403,7 +396,6 @@  static void __init imx7d_clocks_init(struct device_node *ccm_node)
 {
 	struct device_node *np;
 	void __iomem *base;
-	int i;
 
 	clks[IMX7D_CLK_DUMMY] = imx_clk_fixed("dummy", 0);
 	clks[IMX7D_OSC_24M_CLK] = of_clk_get_by_name(ccm_node, "osc");
@@ -466,7 +458,7 @@  static void __init imx7d_clocks_init(struct device_node *ccm_node)
 	clks[IMX7D_PLL_SYS_MAIN_120M] = imx_clk_fixed_factor("pll_sys_main_120m", "pll_sys_main_clk", 1, 4);
 	clks[IMX7D_PLL_DRAM_MAIN_533M] = imx_clk_fixed_factor("pll_dram_533m", "pll_dram_main_clk", 1, 2);
 
-	clks[IMX7D_PLL_SYS_MAIN_480M_CLK] = imx_clk_gate_dis("pll_sys_main_480m_clk", "pll_sys_main_480m", base + 0xb0, 4);
+	clks[IMX7D_PLL_SYS_MAIN_480M_CLK] = imx_clk_gate_dis_flags("pll_sys_main_480m_clk", "pll_sys_main_480m", base + 0xb0, 4, CLK_IS_CRITICAL);
 	clks[IMX7D_PLL_SYS_MAIN_240M_CLK] = imx_clk_gate_dis("pll_sys_main_240m_clk", "pll_sys_main_240m", base + 0xb0, 5);
 	clks[IMX7D_PLL_SYS_MAIN_120M_CLK] = imx_clk_gate_dis("pll_sys_main_120m_clk", "pll_sys_main_120m", base + 0xb0, 6);
 	clks[IMX7D_PLL_DRAM_MAIN_533M_CLK] = imx_clk_gate("pll_dram_533m_clk", "pll_dram_533m", base + 0x70, 12);
@@ -719,7 +711,7 @@  static void __init imx7d_clocks_init(struct device_node *ccm_node)
 	clks[IMX7D_ENET_AXI_ROOT_DIV] = imx_clk_divider2("enet_axi_post_div", "enet_axi_pre_div", base + 0x8900, 0, 6);
 	clks[IMX7D_NAND_USDHC_BUS_ROOT_CLK] = imx_clk_divider2("nand_usdhc_root_clk", "nand_usdhc_pre_div", base + 0x8980, 0, 6);
 	clks[IMX7D_AHB_CHANNEL_ROOT_DIV] = imx_clk_divider2("ahb_root_clk", "ahb_pre_div", base + 0x9000, 0, 6);
-	clks[IMX7D_IPG_ROOT_CLK] = imx_clk_divider2("ipg_root_clk", "ahb_root_clk", base + 0x9080, 0, 2);
+	clks[IMX7D_IPG_ROOT_CLK] = imx_clk_divider_flags("ipg_root_clk", "ahb_root_clk", base + 0x9080, 0, 2, CLK_IS_CRITICAL | CLK_OPS_PARENT_ENABLE | CLK_SET_RATE_PARENT);
 	clks[IMX7D_DRAM_ROOT_DIV] = imx_clk_divider2("dram_post_div", "dram_cg", base + 0x9880, 0, 3);
 	clks[IMX7D_DRAM_PHYM_ALT_ROOT_DIV] = imx_clk_divider2("dram_phym_alt_post_div", "dram_phym_alt_pre_div", base + 0xa000, 0, 3);
 	clks[IMX7D_DRAM_ALT_ROOT_DIV] = imx_clk_divider2("dram_alt_post_div", "dram_alt_pre_div", base + 0xa080, 0, 3);
@@ -783,17 +775,17 @@  static void __init imx7d_clocks_init(struct device_node *ccm_node)
 	clks[IMX7D_CLKO1_ROOT_DIV] = imx_clk_divider2("clko1_post_div", "clko1_pre_div", base + 0xbd80, 0, 6);
 	clks[IMX7D_CLKO2_ROOT_DIV] = imx_clk_divider2("clko2_post_div", "clko2_pre_div", base + 0xbe00, 0, 6);
 
-	clks[IMX7D_ARM_A7_ROOT_CLK] = imx_clk_gate4("arm_a7_root_clk", "arm_a7_div", base + 0x4000, 0);
+	clks[IMX7D_ARM_A7_ROOT_CLK] = imx_clk_gate2_flags("arm_a7_root_clk", "arm_a7_div", base + 0x4000, 0, CLK_IS_CRITICAL | CLK_OPS_PARENT_ENABLE);
 	clks[IMX7D_ARM_M4_ROOT_CLK] = imx_clk_gate4("arm_m4_root_clk", "arm_m4_div", base + 0x4010, 0);
-	clks[IMX7D_MAIN_AXI_ROOT_CLK] = imx_clk_gate4("main_axi_root_clk", "axi_post_div", base + 0x4040, 0);
+	clks[IMX7D_MAIN_AXI_ROOT_CLK] = imx_clk_gate2_flags("main_axi_root_clk", "axi_post_div", base + 0x4040, 0, CLK_IS_CRITICAL | CLK_OPS_PARENT_ENABLE);
 	clks[IMX7D_DISP_AXI_ROOT_CLK] = imx_clk_gate4("disp_axi_root_clk", "disp_axi_post_div", base + 0x4050, 0);
 	clks[IMX7D_ENET_AXI_ROOT_CLK] = imx_clk_gate4("enet_axi_root_clk", "enet_axi_post_div", base + 0x4060, 0);
 	clks[IMX7D_OCRAM_CLK] = imx_clk_gate4("ocram_clk", "main_axi_root_clk", base + 0x4110, 0);
 	clks[IMX7D_OCRAM_S_CLK] = imx_clk_gate4("ocram_s_clk", "ahb_root_clk", base + 0x4120, 0);
-	clks[IMX7D_DRAM_ROOT_CLK] = imx_clk_gate4("dram_root_clk", "dram_post_div", base + 0x4130, 0);
-	clks[IMX7D_DRAM_PHYM_ROOT_CLK] = imx_clk_gate4("dram_phym_root_clk", "dram_phym_cg", base + 0x4130, 0);
-	clks[IMX7D_DRAM_PHYM_ALT_ROOT_CLK] = imx_clk_gate4("dram_phym_alt_root_clk", "dram_phym_alt_post_div", base + 0x4130, 0);
-	clks[IMX7D_DRAM_ALT_ROOT_CLK] = imx_clk_gate4("dram_alt_root_clk", "dram_alt_post_div", base + 0x4130, 0);
+	clks[IMX7D_DRAM_ROOT_CLK] = imx_clk_gate2_flags("dram_root_clk", "dram_post_div", base + 0x4130, 0, CLK_IS_CRITICAL | CLK_OPS_PARENT_ENABLE);
+	clks[IMX7D_DRAM_PHYM_ROOT_CLK] = imx_clk_gate2_flags("dram_phym_root_clk", "dram_phym_cg", base + 0x4130, 0, CLK_IS_CRITICAL | CLK_OPS_PARENT_ENABLE);
+	clks[IMX7D_DRAM_PHYM_ALT_ROOT_CLK] = imx_clk_gate2_flags("dram_phym_alt_root_clk", "dram_phym_alt_post_div", base + 0x4130, 0, CLK_IS_CRITICAL | CLK_OPS_PARENT_ENABLE);
+	clks[IMX7D_DRAM_ALT_ROOT_CLK] = imx_clk_gate2_flags("dram_alt_root_clk", "dram_alt_post_div", base + 0x4130, 0, CLK_IS_CRITICAL | CLK_OPS_PARENT_ENABLE);
 	clks[IMX7D_OCOTP_CLK] = imx_clk_gate4("ocotp_clk", "ipg_root_clk", base + 0x4230, 0);
 	clks[IMX7D_SNVS_CLK] = imx_clk_gate4("snvs_clk", "ipg_root_clk", base + 0x4250, 0);
 	clks[IMX7D_MU_ROOT_CLK] = imx_clk_gate4("mu_root_clk", "ipg_root_clk", base + 0x4270, 0);
@@ -882,9 +874,6 @@  static void __init imx7d_clocks_init(struct device_node *ccm_node)
 	clk_data.clk_num = ARRAY_SIZE(clks);
 	of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data);
 
-	for (i = 0; i < ARRAY_SIZE(clks_init_on); i++)
-		clk_prepare_enable(clks[clks_init_on[i]]);
-
 	clk_set_parent(clks[IMX7D_PLL_ARM_MAIN_BYPASS], clks[IMX7D_PLL_ARM_MAIN]);
 	clk_set_parent(clks[IMX7D_PLL_DRAM_MAIN_BYPASS], clks[IMX7D_PLL_DRAM_MAIN]);
 	clk_set_parent(clks[IMX7D_PLL_SYS_MAIN_BYPASS], clks[IMX7D_PLL_SYS_MAIN]);
diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
index 8076ec0..5895e223 100644
--- a/drivers/clk/imx/clk.h
+++ b/drivers/clk/imx/clk.h
@@ -137,6 +137,13 @@  static inline struct clk *imx_clk_gate_dis(const char *name, const char *parent,
 			shift, CLK_GATE_SET_TO_DISABLE, &imx_ccm_lock);
 }
 
+static inline struct clk *imx_clk_gate_dis_flags(const char *name, const char *parent,
+		void __iomem *reg, u8 shift, unsigned long flags)
+{
+	return clk_register_gate(NULL, name, parent, flags | CLK_SET_RATE_PARENT, reg,
+			shift, CLK_GATE_SET_TO_DISABLE, &imx_ccm_lock);
+}
+
 static inline struct clk *imx_clk_gate2(const char *name, const char *parent,
 		void __iomem *reg, u8 shift)
 {