Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1612470 > unrolled thread

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

Started byStefan Agner <stefan@agner.ch>
First post2017-03-30 02:50 +0200
Last post2017-04-10 13:10 +0200
Articles 7 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC instances Stefan Agner <stefan@agner.ch> - 2017-03-30 02:50 +0200
    Re: [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC  instances Dong Aisheng <dongas86@gmail.com> - 2017-03-31 13:10 +0200
      Re: [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC  instances Stefan Agner <stefan@agner.ch> - 2017-04-01 06:20 +0200
        Re: [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC instances Fabio Estevam <festevam@gmail.com> - 2017-04-02 19:10 +0200
          Re: [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC instances Fabio Estevam <festevam@gmail.com> - 2017-04-05 04:20 +0200
            Re: [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC  instances Stefan Agner <stefan@agner.ch> - 2017-04-05 04:40 +0200
              Re: [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC  instances Dong Aisheng <dongas86@gmail.com> - 2017-04-10 13:10 +0200

#1612470 — [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC instances

FromStefan Agner <stefan@agner.ch>
Date2017-03-30 02:50 +0200
Subject[PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC instances
Message-ID<tqwrL-6Aq-1@gated-at.bofh.it>
The USDHC instances need the USDHC NAND clock in order to operate.
Add the clock as ahb bus clock.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 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

[toc] | [next] | [standalone]

#1613872 — Re: [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC instances

FromDong Aisheng <dongas86@gmail.com>
Date2017-03-31 13:10 +0200
SubjectRe: [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC instances
Message-ID<tr2Bk-3PV-23@gated-at.bofh.it>
In reply to#1612470
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 <stefan@agner.ch>
> ---
>  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 <aisheng.dong@nxp.com>

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
>

[toc] | [prev] | [next] | [standalone]

#1614391 — Re: [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC instances

FromStefan Agner <stefan@agner.ch>
Date2017-04-01 06:20 +0200
SubjectRe: [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC instances
Message-ID<triG5-61o-1@gated-at.bofh.it>
In reply to#1613872
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 <stefan@agner.ch>
>> ---
>>  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 <aisheng.dong@nxp.com>
> 
> 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
>>

[toc] | [prev] | [next] | [standalone]

#1614774

FromFabio Estevam <festevam@gmail.com>
Date2017-04-02 19:10 +0200
Message-ID<trRaO-3jk-19@gated-at.bofh.it>
In reply to#1614391
On Sat, Apr 1, 2017 at 1:15 AM, Stefan Agner <stefan@agner.ch> 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.

[toc] | [prev] | [next] | [standalone]

#1616524

FromFabio Estevam <festevam@gmail.com>
Date2017-04-05 04:20 +0200
Message-ID<tsIIa-4VI-15@gated-at.bofh.it>
In reply to#1614774
On Sun, Apr 2, 2017 at 2:02 PM, Fabio Estevam <festevam@gmail.com> wrote:
> On Sat, Apr 1, 2017 at 1:15 AM, Stefan Agner <stefan@agner.ch> 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);

[toc] | [prev] | [next] | [standalone]

#1616534 — Re: [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC instances

FromStefan Agner <stefan@agner.ch>
Date2017-04-05 04:40 +0200
SubjectRe: [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC instances
Message-ID<tsJ1v-51N-1@gated-at.bofh.it>
In reply to#1616524
On 2017-04-04 19:15, Fabio Estevam wrote:
> On Sun, Apr 2, 2017 at 2:02 PM, Fabio Estevam <festevam@gmail.com> wrote:
>> On Sat, Apr 1, 2017 at 1:15 AM, Stefan Agner <stefan@agner.ch> 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

[toc] | [prev] | [next] | [standalone]

#1619811 — Re: [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC instances

FromDong Aisheng <dongas86@gmail.com>
Date2017-04-10 13:10 +0200
SubjectRe: [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC instances
Message-ID<tuFmO-8iD-19@gated-at.bofh.it>
In reply to#1616534
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 <festevam@gmail.com> wrote:
> >> On Sat, Apr 1, 2017 at 1:15 AM, Stefan Agner <stefan@agner.ch> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web