2017-03-30 00:48:54

by Stefan Agner

[permalink] [raw]
Subject: [PATCH 1/2] clk: imx7d: fix USDHC NAND clock

The USDHC NAND root clock is not gated by any CCM clock gate. Remove
the bogus gate definition.

Signed-off-by: Stefan Agner <[email protected]>
---
The IMX7D_NAND_USDHC_BUS_ROOT_CLK clock is also in clks_init_on.
In a quick test I removed IMX7D_NAND_USDHC_BUS_ROOT_CLK from
clks_init_on, and the system including SD-cards seemed to work
fine... So I guess we could remove the clock from clks_init_on
after the two both changes got applied, any thoughts?

The gate 0x4130 was actually the DRAM gate, hence disabling that
clock lead to disabling this gate, and hence a crash before this
fixes... Maybe that was the reason it landed in clks_init_on...?

--
Stefan

drivers/clk/imx/clk-imx7d.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx7d.c b/drivers/clk/imx/clk-imx7d.c
index ae1d31be906e..4466acaacb71 100644
--- a/drivers/clk/imx/clk-imx7d.c
+++ b/drivers/clk/imx/clk-imx7d.c
@@ -724,7 +724,7 @@ static void __init imx7d_clocks_init(struct device_node *ccm_node)
clks[IMX7D_MAIN_AXI_ROOT_DIV] = imx_clk_divider2("axi_post_div", "axi_pre_div", base + 0x8800, 0, 6);
clks[IMX7D_DISP_AXI_ROOT_DIV] = imx_clk_divider2("disp_axi_post_div", "disp_axi_pre_div", base + 0x8880, 0, 6);
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_DIV] = imx_clk_divider2("nand_usdhc_post_div", "nand_usdhc_pre_div", base + 0x8980, 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_post_div", "ahb_pre_div", base + 0x9000, 0, 6);
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);
@@ -797,7 +797,6 @@ static void __init imx7d_clocks_init(struct device_node *ccm_node)
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", "axi_post_div", base + 0x4110, 0);
clks[IMX7D_OCRAM_S_CLK] = imx_clk_gate4("ocram_s_clk", "ahb_post_div", base + 0x4120, 0);
- clks[IMX7D_NAND_USDHC_BUS_ROOT_CLK] = imx_clk_gate4("nand_usdhc_root_clk", "nand_usdhc_post_div", base + 0x4130, 0);
clks[IMX7D_AHB_CHANNEL_ROOT_CLK] = imx_clk_gate4("ahb_root_clk", "ahb_post_div", base + 0x4200, 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);
--
2.12.1


2017-03-30 00:48:58

by Stefan Agner

[permalink] [raw]
Subject: [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC instances

The USDHC instances need the USDHC NAND clock in order to operate.
Add the clock as ahb bus clock.

Signed-off-by: Stefan Agner <[email protected]>
---
arch/arm/boot/dts/imx7s.dtsi | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
index 5d3a43b8de20..5794febb19a4 100644
--- a/arch/arm/boot/dts/imx7s.dtsi
+++ b/arch/arm/boot/dts/imx7s.dtsi
@@ -936,7 +936,7 @@
reg = <0x30b40000 0x10000>;
interrupts = <GIC_SPI 22 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&clks IMX7D_CLK_DUMMY>,
- <&clks IMX7D_CLK_DUMMY>,
+ <&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>,
<&clks IMX7D_USDHC1_ROOT_CLK>;
clock-names = "ipg", "ahb", "per";
bus-width = <4>;
@@ -948,7 +948,7 @@
reg = <0x30b50000 0x10000>;
interrupts = <GIC_SPI 23 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&clks IMX7D_CLK_DUMMY>,
- <&clks IMX7D_CLK_DUMMY>,
+ <&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>,
<&clks IMX7D_USDHC2_ROOT_CLK>;
clock-names = "ipg", "ahb", "per";
bus-width = <4>;
@@ -960,7 +960,7 @@
reg = <0x30b60000 0x10000>;
interrupts = <GIC_SPI 24 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&clks IMX7D_CLK_DUMMY>,
- <&clks IMX7D_CLK_DUMMY>,
+ <&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>,
<&clks IMX7D_USDHC3_ROOT_CLK>;
clock-names = "ipg", "ahb", "per";
bus-width = <4>;
--
2.12.1

2017-03-31 11:03:52

by Dong Aisheng

[permalink] [raw]
Subject: Re: [PATCH 1/2] clk: imx7d: fix USDHC NAND clock

On Wed, Mar 29, 2017 at 05:50:28PM -0700, Stefan Agner wrote:
> The USDHC NAND root clock is not gated by any CCM clock gate. Remove
> the bogus gate definition.
>
> Signed-off-by: Stefan Agner <[email protected]>
> ---
> The IMX7D_NAND_USDHC_BUS_ROOT_CLK clock is also in clks_init_on.
> In a quick test I removed IMX7D_NAND_USDHC_BUS_ROOT_CLK from
> clks_init_on, and the system including SD-cards seemed to work
> fine... So I guess we could remove the clock from clks_init_on
> after the two both changes got applied, any thoughts?
>

Yes, it can be removed after apply.

> The gate 0x4130 was actually the DRAM gate, hence disabling that
> clock lead to disabling this gate, and hence a crash before this
> fixes... Maybe that was the reason it landed in clks_init_on...?
>

Probably a history reason or mistake. :-)

Acked-by: Dong Aisheng <[email protected]>

Regards
Dong Aisheng

> --
> Stefan
>
> drivers/clk/imx/clk-imx7d.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/clk/imx/clk-imx7d.c b/drivers/clk/imx/clk-imx7d.c
> index ae1d31be906e..4466acaacb71 100644
> --- a/drivers/clk/imx/clk-imx7d.c
> +++ b/drivers/clk/imx/clk-imx7d.c
> @@ -724,7 +724,7 @@ static void __init imx7d_clocks_init(struct device_node *ccm_node)
> clks[IMX7D_MAIN_AXI_ROOT_DIV] = imx_clk_divider2("axi_post_div", "axi_pre_div", base + 0x8800, 0, 6);
> clks[IMX7D_DISP_AXI_ROOT_DIV] = imx_clk_divider2("disp_axi_post_div", "disp_axi_pre_div", base + 0x8880, 0, 6);
> 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_DIV] = imx_clk_divider2("nand_usdhc_post_div", "nand_usdhc_pre_div", base + 0x8980, 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_post_div", "ahb_pre_div", base + 0x9000, 0, 6);
> 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);
> @@ -797,7 +797,6 @@ static void __init imx7d_clocks_init(struct device_node *ccm_node)
> 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", "axi_post_div", base + 0x4110, 0);
> clks[IMX7D_OCRAM_S_CLK] = imx_clk_gate4("ocram_s_clk", "ahb_post_div", base + 0x4120, 0);
> - clks[IMX7D_NAND_USDHC_BUS_ROOT_CLK] = imx_clk_gate4("nand_usdhc_root_clk", "nand_usdhc_post_div", base + 0x4130, 0);
> clks[IMX7D_AHB_CHANNEL_ROOT_CLK] = imx_clk_gate4("ahb_root_clk", "ahb_post_div", base + 0x4200, 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);
> --
> 2.12.1
>

2017-03-31 11:06:32

by Dong Aisheng

[permalink] [raw]
Subject: Re: [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC instances

On Wed, Mar 29, 2017 at 05:50:29PM -0700, Stefan Agner wrote:
> The USDHC instances need the USDHC NAND clock in order to operate.
> Add the clock as ahb bus clock.
>
> Signed-off-by: Stefan Agner <[email protected]>
> ---
> arch/arm/boot/dts/imx7s.dtsi | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
> index 5d3a43b8de20..5794febb19a4 100644
> --- a/arch/arm/boot/dts/imx7s.dtsi
> +++ b/arch/arm/boot/dts/imx7s.dtsi
> @@ -936,7 +936,7 @@
> reg = <0x30b40000 0x10000>;
> interrupts = <GIC_SPI 22 IRQ_TYPE_LEVEL_HIGH>;
> clocks = <&clks IMX7D_CLK_DUMMY>,

Would you please change the left ipg dummy to IMX7D_IPG_ROOT_CLK as well?

Otherwise,

Acked-by: Dong Aisheng <[email protected]>

Regards
Dong Aisheng

> - <&clks IMX7D_CLK_DUMMY>,
> + <&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>,
> <&clks IMX7D_USDHC1_ROOT_CLK>;
> clock-names = "ipg", "ahb", "per";
> bus-width = <4>;
> @@ -948,7 +948,7 @@
> reg = <0x30b50000 0x10000>;
> interrupts = <GIC_SPI 23 IRQ_TYPE_LEVEL_HIGH>;
> clocks = <&clks IMX7D_CLK_DUMMY>,
> - <&clks IMX7D_CLK_DUMMY>,
> + <&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>,
> <&clks IMX7D_USDHC2_ROOT_CLK>;
> clock-names = "ipg", "ahb", "per";
> bus-width = <4>;
> @@ -960,7 +960,7 @@
> reg = <0x30b60000 0x10000>;
> interrupts = <GIC_SPI 24 IRQ_TYPE_LEVEL_HIGH>;
> clocks = <&clks IMX7D_CLK_DUMMY>,
> - <&clks IMX7D_CLK_DUMMY>,
> + <&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>,
> <&clks IMX7D_USDHC3_ROOT_CLK>;
> clock-names = "ipg", "ahb", "per";
> bus-width = <4>;
> --
> 2.12.1
>

2017-04-01 04:15:45

by Stefan Agner

[permalink] [raw]
Subject: Re: [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC instances

On 2017-03-31 20:03, Dong Aisheng wrote:
> On Wed, Mar 29, 2017 at 05:50:29PM -0700, Stefan Agner wrote:
>> The USDHC instances need the USDHC NAND clock in order to operate.
>> Add the clock as ahb bus clock.
>>
>> Signed-off-by: Stefan Agner <[email protected]>
>> ---
>> arch/arm/boot/dts/imx7s.dtsi | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
>> index 5d3a43b8de20..5794febb19a4 100644
>> --- a/arch/arm/boot/dts/imx7s.dtsi
>> +++ b/arch/arm/boot/dts/imx7s.dtsi
>> @@ -936,7 +936,7 @@
>> reg = <0x30b40000 0x10000>;
>> interrupts = <GIC_SPI 22 IRQ_TYPE_LEVEL_HIGH>;
>> clocks = <&clks IMX7D_CLK_DUMMY>,
>
> Would you please change the left ipg dummy to IMX7D_IPG_ROOT_CLK as well?

IMX7D_IPG_ROOT_CLK is currently not a valid clock in upstream... So we
would have to add it to the clock driver first.

I guess we could/should add it anyway at one point? But probably also as
init on, just to make sure Linux does not disable it since it is
currently used by several IPs implicitly.

--
Stefan

>
> Otherwise,
>
> Acked-by: Dong Aisheng <[email protected]>
>
> Regards
> Dong Aisheng
>
>> - <&clks IMX7D_CLK_DUMMY>,
>> + <&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>,
>> <&clks IMX7D_USDHC1_ROOT_CLK>;
>> clock-names = "ipg", "ahb", "per";
>> bus-width = <4>;
>> @@ -948,7 +948,7 @@
>> reg = <0x30b50000 0x10000>;
>> interrupts = <GIC_SPI 23 IRQ_TYPE_LEVEL_HIGH>;
>> clocks = <&clks IMX7D_CLK_DUMMY>,
>> - <&clks IMX7D_CLK_DUMMY>,
>> + <&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>,
>> <&clks IMX7D_USDHC2_ROOT_CLK>;
>> clock-names = "ipg", "ahb", "per";
>> bus-width = <4>;
>> @@ -960,7 +960,7 @@
>> reg = <0x30b60000 0x10000>;
>> interrupts = <GIC_SPI 24 IRQ_TYPE_LEVEL_HIGH>;
>> clocks = <&clks IMX7D_CLK_DUMMY>,
>> - <&clks IMX7D_CLK_DUMMY>,
>> + <&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>,
>> <&clks IMX7D_USDHC3_ROOT_CLK>;
>> clock-names = "ipg", "ahb", "per";
>> bus-width = <4>;
>> --
>> 2.12.1
>>

2017-04-02 17:03:01

by Fabio Estevam

[permalink] [raw]
Subject: Re: [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC instances

On Sat, Apr 1, 2017 at 1:15 AM, Stefan Agner <[email protected]> wrote:

> IMX7D_IPG_ROOT_CLK is currently not a valid clock in upstream... So we
> would have to add it to the clock driver first.
>
> I guess we could/should add it anyway at one point? But probably also as
> init on, just to make sure Linux does not disable it since it is
> currently used by several IPs implicitly.

Yes, I made a previous attempt do add IMX7D_IPG_ROOT_CLK and it did
not work as I did not put it in the init_on clock list.

Will submit a new patch adding it to init_on, thanks.

2017-04-05 02:15:28

by Fabio Estevam

[permalink] [raw]
Subject: Re: [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC instances

On Sun, Apr 2, 2017 at 2:02 PM, Fabio Estevam <[email protected]> wrote:
> On Sat, Apr 1, 2017 at 1:15 AM, Stefan Agner <[email protected]> wrote:
>
>> IMX7D_IPG_ROOT_CLK is currently not a valid clock in upstream... So we
>> would have to add it to the clock driver first.
>>
>> I guess we could/should add it anyway at one point? But probably also as
>> init on, just to make sure Linux does not disable it since it is
>> currently used by several IPs implicitly.
>
> Yes, I made a previous attempt do add IMX7D_IPG_ROOT_CLK and it did
> not work as I did not put it in the init_on clock list.
>
> Will submit a new patch adding it to init_on, thanks.

I thought that adding IMX7D_IPG_ROOT_CLK would do the trick, but the
patch below also causes the kernel to not boot:

--- a/drivers/clk/imx/clk-imx7d.c
+++ b/drivers/clk/imx/clk-imx7d.c
@@ -386,7 +386,7 @@ static int const clks_init_on[] __initconst = {
IMX7D_PLL_SYS_MAIN_480M_CLK, IMX7D_NAND_USDHC_BUS_ROOT_CLK,
IMX7D_DRAM_PHYM_ROOT_CLK, IMX7D_DRAM_ROOT_CLK,
IMX7D_DRAM_PHYM_ALT_ROOT_CLK, IMX7D_DRAM_ALT_ROOT_CLK,
- IMX7D_AHB_CHANNEL_ROOT_CLK,
+ IMX7D_AHB_CHANNEL_ROOT_CLK, IMX7D_IPG_ROOT_CLK,
};

static struct clk_onecell_data clk_data;
@@ -788,7 +788,7 @@ static void __init imx7d_clocks_init(struct
device_node *ccm_node)
clks[IMX7D_WRCLK_ROOT_DIV] =
imx_clk_divider2("wrclk_post_div", "wrclk_pre_div", base + 0xbd00, 0,
6);
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_IPG_ROOT_CLK] = imx_clk_divider2("ipg_root_clk",
"ahb_root_clk", base + 0x9080, 0, 2);
clks[IMX7D_ARM_A7_ROOT_CLK] = imx_clk_gate4("arm_a7_root_clk",
"arm_a7_div", base + 0x4000, 0);
clks[IMX7D_ARM_M4_ROOT_CLK] = imx_clk_gate4("arm_m4_root_clk",
"arm_m4_div", base + 0x4010, 0);
clks[IMX7D_ARM_M0_ROOT_CLK] = imx_clk_gate4("arm_m0_root_clk",
"arm_m0_div", base + 0x4020, 0);

2017-04-05 02:36:17

by Stefan Agner

[permalink] [raw]
Subject: Re: [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC instances

On 2017-04-04 19:15, Fabio Estevam wrote:
> On Sun, Apr 2, 2017 at 2:02 PM, Fabio Estevam <[email protected]> wrote:
>> On Sat, Apr 1, 2017 at 1:15 AM, Stefan Agner <[email protected]> wrote:
>>
>>> IMX7D_IPG_ROOT_CLK is currently not a valid clock in upstream... So we
>>> would have to add it to the clock driver first.
>>>
>>> I guess we could/should add it anyway at one point? But probably also as
>>> init on, just to make sure Linux does not disable it since it is
>>> currently used by several IPs implicitly.
>>
>> Yes, I made a previous attempt do add IMX7D_IPG_ROOT_CLK and it did
>> not work as I did not put it in the init_on clock list.
>>
>> Will submit a new patch adding it to init_on, thanks.
>
> I thought that adding IMX7D_IPG_ROOT_CLK would do the trick, but the
> patch below also causes the kernel to not boot:
>
> --- a/drivers/clk/imx/clk-imx7d.c
> +++ b/drivers/clk/imx/clk-imx7d.c
> @@ -386,7 +386,7 @@ static int const clks_init_on[] __initconst = {
> IMX7D_PLL_SYS_MAIN_480M_CLK, IMX7D_NAND_USDHC_BUS_ROOT_CLK,
> IMX7D_DRAM_PHYM_ROOT_CLK, IMX7D_DRAM_ROOT_CLK,
> IMX7D_DRAM_PHYM_ALT_ROOT_CLK, IMX7D_DRAM_ALT_ROOT_CLK,
> - IMX7D_AHB_CHANNEL_ROOT_CLK,
> + IMX7D_AHB_CHANNEL_ROOT_CLK, IMX7D_IPG_ROOT_CLK,
> };
>
> static struct clk_onecell_data clk_data;
> @@ -788,7 +788,7 @@ static void __init imx7d_clocks_init(struct
> device_node *ccm_node)
> clks[IMX7D_WRCLK_ROOT_DIV] =
> imx_clk_divider2("wrclk_post_div", "wrclk_pre_div", base + 0xbd00, 0,
> 6);
> 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_IPG_ROOT_CLK] = imx_clk_divider2("ipg_root_clk",
> "ahb_root_clk", base + 0x9080, 0, 2);
> clks[IMX7D_ARM_A7_ROOT_CLK] = imx_clk_gate4("arm_a7_root_clk",
> "arm_a7_div", base + 0x4000, 0);
> clks[IMX7D_ARM_M4_ROOT_CLK] = imx_clk_gate4("arm_m4_root_clk",
> "arm_m4_div", base + 0x4010, 0);
> clks[IMX7D_ARM_M0_ROOT_CLK] = imx_clk_gate4("arm_m0_root_clk",
> "arm_m0_div", base + 0x4020, 0);

Hm, imx_clk_divider2 sets CLK_SET_RATE_PARENT, maybe that influences the
parent?

I guess we actually don't want the clock framework to change that clock
rate, not sure whether we can freeze it or similar.

--
Stefan

2017-04-10 11:03:30

by Dong Aisheng

[permalink] [raw]
Subject: Re: [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC instances

On Tue, Apr 04, 2017 at 07:36:01PM -0700, Stefan Agner wrote:
> On 2017-04-04 19:15, Fabio Estevam wrote:
> > On Sun, Apr 2, 2017 at 2:02 PM, Fabio Estevam <[email protected]> wrote:
> >> On Sat, Apr 1, 2017 at 1:15 AM, Stefan Agner <[email protected]> wrote:
> >>
> >>> IMX7D_IPG_ROOT_CLK is currently not a valid clock in upstream... So we
> >>> would have to add it to the clock driver first.
> >>>
> >>> I guess we could/should add it anyway at one point? But probably also as
> >>> init on, just to make sure Linux does not disable it since it is
> >>> currently used by several IPs implicitly.
> >>
> >> Yes, I made a previous attempt do add IMX7D_IPG_ROOT_CLK and it did
> >> not work as I did not put it in the init_on clock list.
> >>
> >> Will submit a new patch adding it to init_on, thanks.
> >
> > I thought that adding IMX7D_IPG_ROOT_CLK would do the trick, but the
> > patch below also causes the kernel to not boot:
> >
> > --- a/drivers/clk/imx/clk-imx7d.c
> > +++ b/drivers/clk/imx/clk-imx7d.c
> > @@ -386,7 +386,7 @@ static int const clks_init_on[] __initconst = {
> > IMX7D_PLL_SYS_MAIN_480M_CLK, IMX7D_NAND_USDHC_BUS_ROOT_CLK,
> > IMX7D_DRAM_PHYM_ROOT_CLK, IMX7D_DRAM_ROOT_CLK,
> > IMX7D_DRAM_PHYM_ALT_ROOT_CLK, IMX7D_DRAM_ALT_ROOT_CLK,
> > - IMX7D_AHB_CHANNEL_ROOT_CLK,
> > + IMX7D_AHB_CHANNEL_ROOT_CLK, IMX7D_IPG_ROOT_CLK,
> > };
> >
> > static struct clk_onecell_data clk_data;
> > @@ -788,7 +788,7 @@ static void __init imx7d_clocks_init(struct
> > device_node *ccm_node)
> > clks[IMX7D_WRCLK_ROOT_DIV] =
> > imx_clk_divider2("wrclk_post_div", "wrclk_pre_div", base + 0xbd00, 0,
> > 6);
> > 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_IPG_ROOT_CLK] = imx_clk_divider2("ipg_root_clk",
> > "ahb_root_clk", base + 0x9080, 0, 2);
> > clks[IMX7D_ARM_A7_ROOT_CLK] = imx_clk_gate4("arm_a7_root_clk",
> > "arm_a7_div", base + 0x4000, 0);
> > clks[IMX7D_ARM_M4_ROOT_CLK] = imx_clk_gate4("arm_m4_root_clk",
> > "arm_m4_div", base + 0x4010, 0);
> > clks[IMX7D_ARM_M0_ROOT_CLK] = imx_clk_gate4("arm_m0_root_clk",
> > "arm_m0_div", base + 0x4020, 0);
>
> Hm, imx_clk_divider2 sets CLK_SET_RATE_PARENT, maybe that influences the
> parent?
>
> I guess we actually don't want the clock framework to change that clock
> rate, not sure whether we can freeze it or similar.
>

This is caused by ahb_root_clk gets disabled accidently and system hangs.

Because this patch defines ipg_root_clk earlier before its parent
(ahb_root_clk) got registered, then it will be marked as a orphan clk
temporarily. Until the parent ahb_root_clk got registered, the clk core
will reparent it to the newly found parent. (see __clk_core_init() function).

Due to CLK_SET_RATE_PARENT flag, the ahb clk will be enabled during
set_parent operation and then disabled after that.
Then system hang cause we still get no chance to run init_on clks.

I just send out a proper fix patch with correct register sequence.

Probably we can switch all imx clk driver to CLK_IS_CRITICAL for critical
clocks in the future, but that's another thing to do later.

Stefan,
I think you can just resend your series based on my patches.

Regards
Dong Aisheng

> --
> Stefan

2017-04-11 23:23:29

by Fabio Estevam

[permalink] [raw]
Subject: Re: [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC instances

On Mon, Apr 10, 2017 at 11:59 PM, Dong Aisheng <[email protected]> wrote:

> This is caused by ahb_root_clk gets disabled accidently and system hangs.
>
> Because this patch defines ipg_root_clk earlier before its parent
> (ahb_root_clk) got registered, then it will be marked as a orphan clk
> temporarily. Until the parent ahb_root_clk got registered, the clk core
> will reparent it to the newly found parent. (see __clk_core_init() function).
>
> Due to CLK_SET_RATE_PARENT flag, the ahb clk will be enabled during
> set_parent operation and then disabled after that.
> Then system hang cause we still get no chance to run init_on clks.
>
> I just send out a proper fix patch with correct register sequence.

Excellent, thanks!