[PATCH] clk: rockchip: assign correct id for pclk_ddr and hclk_sd in rk3399

[PATCH] clk: rockchip: assign correct id for pclk_ddr and hclk_sd in rk3399

[Lin Huang]

Assign id PCLK_DDR to pclk_ddr, id HCLK_SD to hclk_sd, so we can
assign frequency for them in dts.

Change-Id: I6c4d15d37ddabe4ed34e2351cf26e660672ae9ee
Signed-off-by: Lin Huang <hl@rock-chips.com>
---
 drivers/clk/rockchip/clk-rk3399.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
index 6847120..a29c99e 100644
--- a/drivers/clk/rockchip/clk-rk3399.c
+++ b/drivers/clk/rockchip/clk-rk3399.c
@@ -670,7 +670,7 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = {
 			RK3399_CLKGATE_CON(9), 7, GFLAGS,
 			&rk3399_uart3_fracmux),
 
-	COMPOSITE(0, "pclk_ddr", mux_pll_src_cpll_gpll_p, CLK_IGNORE_UNUSED,
+	COMPOSITE(PCLK_DDR, "pclk_ddr", mux_pll_src_cpll_gpll_p, CLK_IGNORE_UNUSED,
 			RK3399_CLKSEL_CON(6), 15, 1, MFLAGS, 8, 5, DFLAGS,
 			RK3399_CLKGATE_CON(3), 4, GFLAGS),
 
@@ -886,7 +886,7 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = {
 			RK3399_CLKGATE_CON(31), 8, GFLAGS),
 
 	/* sdio & sdmmc */
-	COMPOSITE(0, "hclk_sd", mux_pll_src_cpll_gpll_p, 0,
+	COMPOSITE(HCLK_SD, "hclk_sd", mux_pll_src_cpll_gpll_p, 0,
 			RK3399_CLKSEL_CON(13), 15, 1, MFLAGS, 8, 5, DFLAGS,
 			RK3399_CLKGATE_CON(12), 13, GFLAGS),
 	GATE(HCLK_SDMMC, "hclk_sdmmc", "hclk_sd", 0,
-- 
2.7.4

[PATCH] clk: rockchip: assign correct id for pclk_ddr and hclk_sd in rk3399

[Lin Huang]

Assign id PCLK_DDR to pclk_ddr, id HCLK_SD to hclk_sd, so we can
assign frequency for them in dts.

Change-Id: I6c4d15d37ddabe4ed34e2351cf26e660672ae9ee
Signed-off-by: Lin Huang <hl-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
---
 drivers/clk/rockchip/clk-rk3399.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
index 6847120..a29c99e 100644
--- a/drivers/clk/rockchip/clk-rk3399.c
+++ b/drivers/clk/rockchip/clk-rk3399.c
@@ -670,7 +670,7 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = {
 			RK3399_CLKGATE_CON(9), 7, GFLAGS,
 			&rk3399_uart3_fracmux),
 
-	COMPOSITE(0, "pclk_ddr", mux_pll_src_cpll_gpll_p, CLK_IGNORE_UNUSED,
+	COMPOSITE(PCLK_DDR, "pclk_ddr", mux_pll_src_cpll_gpll_p, CLK_IGNORE_UNUSED,
 			RK3399_CLKSEL_CON(6), 15, 1, MFLAGS, 8, 5, DFLAGS,
 			RK3399_CLKGATE_CON(3), 4, GFLAGS),
 
@@ -886,7 +886,7 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = {
 			RK3399_CLKGATE_CON(31), 8, GFLAGS),
 
 	/* sdio & sdmmc */
-	COMPOSITE(0, "hclk_sd", mux_pll_src_cpll_gpll_p, 0,
+	COMPOSITE(HCLK_SD, "hclk_sd", mux_pll_src_cpll_gpll_p, 0,
 			RK3399_CLKSEL_CON(13), 15, 1, MFLAGS, 8, 5, DFLAGS,
 			RK3399_CLKGATE_CON(12), 13, GFLAGS),
 	GATE(HCLK_SDMMC, "hclk_sdmmc", "hclk_sd", 0,
-- 
2.7.4

Re: [PATCH] clk: rockchip: assign correct id for pclk_ddr and hclk_sd in rk3399

[Shawn Lin]

Hi Huang,

On 2018/3/15 16:54, Lin Huang wrote:
> Assign id PCLK_DDR to pclk_ddr, id HCLK_SD to hclk_sd, so we can
> assign frequency for them in dts.

I'm curious under which condition that we need assign frequency for
hclk_sd?

> 
> Change-Id: I6c4d15d37ddabe4ed34e2351cf26e660672ae9ee
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> ---
>   drivers/clk/rockchip/clk-rk3399.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
> index 6847120..a29c99e 100644
> --- a/drivers/clk/rockchip/clk-rk3399.c
> +++ b/drivers/clk/rockchip/clk-rk3399.c
> @@ -670,7 +670,7 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = {
>   			RK3399_CLKGATE_CON(9), 7, GFLAGS,
>   			&rk3399_uart3_fracmux),
>   
> -	COMPOSITE(0, "pclk_ddr", mux_pll_src_cpll_gpll_p, CLK_IGNORE_UNUSED,
> +	COMPOSITE(PCLK_DDR, "pclk_ddr", mux_pll_src_cpll_gpll_p, CLK_IGNORE_UNUSED,
>   			RK3399_CLKSEL_CON(6), 15, 1, MFLAGS, 8, 5, DFLAGS,
>   			RK3399_CLKGATE_CON(3), 4, GFLAGS),
>   
> @@ -886,7 +886,7 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = {
>   			RK3399_CLKGATE_CON(31), 8, GFLAGS),
>   
>   	/* sdio & sdmmc */
> -	COMPOSITE(0, "hclk_sd", mux_pll_src_cpll_gpll_p, 0,
> +	COMPOSITE(HCLK_SD, "hclk_sd", mux_pll_src_cpll_gpll_p, 0,
>   			RK3399_CLKSEL_CON(13), 15, 1, MFLAGS, 8, 5, DFLAGS,
>   			RK3399_CLKGATE_CON(12), 13, GFLAGS),
>   	GATE(HCLK_SDMMC, "hclk_sdmmc", "hclk_sd", 0,
> 

Re: [PATCH] clk: rockchip: assign correct id for pclk_ddr and hclk_sd in rk3399

[Shawn Lin]

Hi Huang,

On 2018/3/15 16:54, Lin Huang wrote:
> Assign id PCLK_DDR to pclk_ddr, id HCLK_SD to hclk_sd, so we can
> assign frequency for them in dts.

I'm curious under which condition that we need assign frequency for
hclk_sd?

> 
> Change-Id: I6c4d15d37ddabe4ed34e2351cf26e660672ae9ee
> Signed-off-by: Lin Huang <hl-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
> ---
>   drivers/clk/rockchip/clk-rk3399.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/rockchip/clk-rk3399.c b/drivers/clk/rockchip/clk-rk3399.c
> index 6847120..a29c99e 100644
> --- a/drivers/clk/rockchip/clk-rk3399.c
> +++ b/drivers/clk/rockchip/clk-rk3399.c
> @@ -670,7 +670,7 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = {
>   			RK3399_CLKGATE_CON(9), 7, GFLAGS,
>   			&rk3399_uart3_fracmux),
>   
> -	COMPOSITE(0, "pclk_ddr", mux_pll_src_cpll_gpll_p, CLK_IGNORE_UNUSED,
> +	COMPOSITE(PCLK_DDR, "pclk_ddr", mux_pll_src_cpll_gpll_p, CLK_IGNORE_UNUSED,
>   			RK3399_CLKSEL_CON(6), 15, 1, MFLAGS, 8, 5, DFLAGS,
>   			RK3399_CLKGATE_CON(3), 4, GFLAGS),
>   
> @@ -886,7 +886,7 @@ static struct rockchip_clk_branch rk3399_clk_branches[] __initdata = {
>   			RK3399_CLKGATE_CON(31), 8, GFLAGS),
>   
>   	/* sdio & sdmmc */
> -	COMPOSITE(0, "hclk_sd", mux_pll_src_cpll_gpll_p, 0,
> +	COMPOSITE(HCLK_SD, "hclk_sd", mux_pll_src_cpll_gpll_p, 0,
>   			RK3399_CLKSEL_CON(13), 15, 1, MFLAGS, 8, 5, DFLAGS,
>   			RK3399_CLKGATE_CON(12), 13, GFLAGS),
>   	GATE(HCLK_SDMMC, "hclk_sdmmc", "hclk_sd", 0,
> 

Re: [PATCH] clk: rockchip: assign correct id for pclk_ddr and hclk_sd in rk3399

[hl]

Hi Shawn,


On Thursday, March 15, 2018 05:01 PM, Shawn Lin wrote:
> Hi Huang,
>
> On 2018/3/15 16:54, Lin Huang wrote:
>> Assign id PCLK_DDR to pclk_ddr, id HCLK_SD to hclk_sd, so we can
>> assign frequency for them in dts.
>
> I'm curious under which condition that we need assign frequency for
> hclk_sd?
We may set CPLL frequency higher than 800MHz, with that we need assign clock
to hclk_sd, otherwise it will exceed 200MHz, since we use the default 
cru register value to get hclk_sd for now.
you can check
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/956678/5 

>
>>
>> Change-Id: I6c4d15d37ddabe4ed34e2351cf26e660672ae9ee
>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>> ---
>>   drivers/clk/rockchip/clk-rk3399.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/rockchip/clk-rk3399.c 
>> b/drivers/clk/rockchip/clk-rk3399.c
>> index 6847120..a29c99e 100644
>> --- a/drivers/clk/rockchip/clk-rk3399.c
>> +++ b/drivers/clk/rockchip/clk-rk3399.c
>> @@ -670,7 +670,7 @@ static struct rockchip_clk_branch 
>> rk3399_clk_branches[] __initdata = {
>>               RK3399_CLKGATE_CON(9), 7, GFLAGS,
>>               &rk3399_uart3_fracmux),
>>   -    COMPOSITE(0, "pclk_ddr", mux_pll_src_cpll_gpll_p, 
>> CLK_IGNORE_UNUSED,
>> +    COMPOSITE(PCLK_DDR, "pclk_ddr", mux_pll_src_cpll_gpll_p, 
>> CLK_IGNORE_UNUSED,
>>               RK3399_CLKSEL_CON(6), 15, 1, MFLAGS, 8, 5, DFLAGS,
>>               RK3399_CLKGATE_CON(3), 4, GFLAGS),
>>   @@ -886,7 +886,7 @@ static struct rockchip_clk_branch 
>> rk3399_clk_branches[] __initdata = {
>>               RK3399_CLKGATE_CON(31), 8, GFLAGS),
>>         /* sdio & sdmmc */
>> -    COMPOSITE(0, "hclk_sd", mux_pll_src_cpll_gpll_p, 0,
>> +    COMPOSITE(HCLK_SD, "hclk_sd", mux_pll_src_cpll_gpll_p, 0,
>>               RK3399_CLKSEL_CON(13), 15, 1, MFLAGS, 8, 5, DFLAGS,
>>               RK3399_CLKGATE_CON(12), 13, GFLAGS),
>>       GATE(HCLK_SDMMC, "hclk_sdmmc", "hclk_sd", 0,
>>
>
>
>

Re: [PATCH] clk: rockchip: assign correct id for pclk_ddr and hclk_sd in rk3399

[hl]

Hi Shawn,


On Thursday, March 15, 2018 05:01 PM, Shawn Lin wrote:
> Hi Huang,
>
> On 2018/3/15 16:54, Lin Huang wrote:
>> Assign id PCLK_DDR to pclk_ddr, id HCLK_SD to hclk_sd, so we can
>> assign frequency for them in dts.
>
> I'm curious under which condition that we need assign frequency for
> hclk_sd?
We may set CPLL frequency higher than 800MHz, with that we need assign clock
to hclk_sd, otherwise it will exceed 200MHz, since we use the default 
cru register value to get hclk_sd for now.
you can check
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/956678/5 

>
>>
>> Change-Id: I6c4d15d37ddabe4ed34e2351cf26e660672ae9ee
>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>> ---
>>   drivers/clk/rockchip/clk-rk3399.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/rockchip/clk-rk3399.c 
>> b/drivers/clk/rockchip/clk-rk3399.c
>> index 6847120..a29c99e 100644
>> --- a/drivers/clk/rockchip/clk-rk3399.c
>> +++ b/drivers/clk/rockchip/clk-rk3399.c
>> @@ -670,7 +670,7 @@ static struct rockchip_clk_branch 
>> rk3399_clk_branches[] __initdata = {
>>               RK3399_CLKGATE_CON(9), 7, GFLAGS,
>>               &rk3399_uart3_fracmux),
>>   -    COMPOSITE(0, "pclk_ddr", mux_pll_src_cpll_gpll_p, 
>> CLK_IGNORE_UNUSED,
>> +    COMPOSITE(PCLK_DDR, "pclk_ddr", mux_pll_src_cpll_gpll_p, 
>> CLK_IGNORE_UNUSED,
>>               RK3399_CLKSEL_CON(6), 15, 1, MFLAGS, 8, 5, DFLAGS,
>>               RK3399_CLKGATE_CON(3), 4, GFLAGS),
>>   @@ -886,7 +886,7 @@ static struct rockchip_clk_branch 
>> rk3399_clk_branches[] __initdata = {
>>               RK3399_CLKGATE_CON(31), 8, GFLAGS),
>>         /* sdio & sdmmc */
>> -    COMPOSITE(0, "hclk_sd", mux_pll_src_cpll_gpll_p, 0,
>> +    COMPOSITE(HCLK_SD, "hclk_sd", mux_pll_src_cpll_gpll_p, 0,
>>               RK3399_CLKSEL_CON(13), 15, 1, MFLAGS, 8, 5, DFLAGS,
>>               RK3399_CLKGATE_CON(12), 13, GFLAGS),
>>       GATE(HCLK_SDMMC, "hclk_sdmmc", "hclk_sd", 0,
>>
>
>
>



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] clk: rockchip: assign correct id for pclk_ddr and hclk_sd in rk3399

[Shawn Lin]

[Correct Doug's email address]

On 2018/3/15 17:12, hl wrote:
> Hi Shawn,
> 
> 
> On Thursday, March 15, 2018 05:01 PM, Shawn Lin wrote:
>> Hi Huang,
>>
>> On 2018/3/15 16:54, Lin Huang wrote:
>>> Assign id PCLK_DDR to pclk_ddr, id HCLK_SD to hclk_sd, so we can
>>> assign frequency for them in dts.
>>
>> I'm curious under which condition that we need assign frequency for
>> hclk_sd?
> We may set CPLL frequency higher than 800MHz, with that we need assign 
> clock
> to hclk_sd, otherwise it will exceed 200MHz, since we use the default 
> cru register value to get hclk_sd for now.
> you can check
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/956678/5 
> 

Okay, thanks for pointing me to the actual user case.
I'm fine with that, however, what I am thinking now is:

(1) It's worth elaborating more in the commit msg.
(2) The reason you need set hclk_sd is that it depends on the
defualt settings but lack real owner to explicitly set it to <=200MHz.
So my another question is if that is more about aspect of power
consumption, then it looks okay to me. But if that is a SoC limitation,
then we are under risk for that. Either we should never rely on the
default settings, or we should set its rate range after registering this
clock provider.

Of course, I'm not a clk expert, but just want to learn more bits
from you guys. :)



>>
>>>
>>> Change-Id: I6c4d15d37ddabe4ed34e2351cf26e660672ae9ee
>>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>>> ---
>>>   drivers/clk/rockchip/clk-rk3399.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/clk/rockchip/clk-rk3399.c 
>>> b/drivers/clk/rockchip/clk-rk3399.c
>>> index 6847120..a29c99e 100644
>>> --- a/drivers/clk/rockchip/clk-rk3399.c
>>> +++ b/drivers/clk/rockchip/clk-rk3399.c
>>> @@ -670,7 +670,7 @@ static struct rockchip_clk_branch 
>>> rk3399_clk_branches[] __initdata = {
>>>               RK3399_CLKGATE_CON(9), 7, GFLAGS,
>>>               &rk3399_uart3_fracmux),
>>>   -    COMPOSITE(0, "pclk_ddr", mux_pll_src_cpll_gpll_p, 
>>> CLK_IGNORE_UNUSED,
>>> +    COMPOSITE(PCLK_DDR, "pclk_ddr", mux_pll_src_cpll_gpll_p, 
>>> CLK_IGNORE_UNUSED,
>>>               RK3399_CLKSEL_CON(6), 15, 1, MFLAGS, 8, 5, DFLAGS,
>>>               RK3399_CLKGATE_CON(3), 4, GFLAGS),
>>>   @@ -886,7 +886,7 @@ static struct rockchip_clk_branch 
>>> rk3399_clk_branches[] __initdata = {
>>>               RK3399_CLKGATE_CON(31), 8, GFLAGS),
>>>         /* sdio & sdmmc */
>>> -    COMPOSITE(0, "hclk_sd", mux_pll_src_cpll_gpll_p, 0,
>>> +    COMPOSITE(HCLK_SD, "hclk_sd", mux_pll_src_cpll_gpll_p, 0,
>>>               RK3399_CLKSEL_CON(13), 15, 1, MFLAGS, 8, 5, DFLAGS,
>>>               RK3399_CLKGATE_CON(12), 13, GFLAGS),
>>>       GATE(HCLK_SDMMC, "hclk_sdmmc", "hclk_sd", 0,
>>>
>>
>>
>>
> 
> 
> 
> 


-- 
Best Regards
Shawn Lin

Re: [PATCH] clk: rockchip: assign correct id for pclk_ddr and hclk_sd in rk3399

[Shawn Lin]

[Correct Doug's email address]

On 2018/3/15 17:12, hl wrote:
> Hi Shawn,
> 
> 
> On Thursday, March 15, 2018 05:01 PM, Shawn Lin wrote:
>> Hi Huang,
>>
>> On 2018/3/15 16:54, Lin Huang wrote:
>>> Assign id PCLK_DDR to pclk_ddr, id HCLK_SD to hclk_sd, so we can
>>> assign frequency for them in dts.
>>
>> I'm curious under which condition that we need assign frequency for
>> hclk_sd?
> We may set CPLL frequency higher than 800MHz, with that we need assign 
> clock
> to hclk_sd, otherwise it will exceed 200MHz, since we use the default 
> cru register value to get hclk_sd for now.
> you can check
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/956678/5 
> 

Okay, thanks for pointing me to the actual user case.
I'm fine with that, however, what I am thinking now is:

(1) It's worth elaborating more in the commit msg.
(2) The reason you need set hclk_sd is that it depends on the
defualt settings but lack real owner to explicitly set it to <=200MHz.
So my another question is if that is more about aspect of power
consumption, then it looks okay to me. But if that is a SoC limitation,
then we are under risk for that. Either we should never rely on the
default settings, or we should set its rate range after registering this
clock provider.

Of course, I'm not a clk expert, but just want to learn more bits
from you guys. :)



>>
>>>
>>> Change-Id: I6c4d15d37ddabe4ed34e2351cf26e660672ae9ee
>>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>>> ---
>>>   drivers/clk/rockchip/clk-rk3399.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/clk/rockchip/clk-rk3399.c 
>>> b/drivers/clk/rockchip/clk-rk3399.c
>>> index 6847120..a29c99e 100644
>>> --- a/drivers/clk/rockchip/clk-rk3399.c
>>> +++ b/drivers/clk/rockchip/clk-rk3399.c
>>> @@ -670,7 +670,7 @@ static struct rockchip_clk_branch 
>>> rk3399_clk_branches[] __initdata = {
>>>               RK3399_CLKGATE_CON(9), 7, GFLAGS,
>>>               &rk3399_uart3_fracmux),
>>>   -    COMPOSITE(0, "pclk_ddr", mux_pll_src_cpll_gpll_p, 
>>> CLK_IGNORE_UNUSED,
>>> +    COMPOSITE(PCLK_DDR, "pclk_ddr", mux_pll_src_cpll_gpll_p, 
>>> CLK_IGNORE_UNUSED,
>>>               RK3399_CLKSEL_CON(6), 15, 1, MFLAGS, 8, 5, DFLAGS,
>>>               RK3399_CLKGATE_CON(3), 4, GFLAGS),
>>>   @@ -886,7 +886,7 @@ static struct rockchip_clk_branch 
>>> rk3399_clk_branches[] __initdata = {
>>>               RK3399_CLKGATE_CON(31), 8, GFLAGS),
>>>         /* sdio & sdmmc */
>>> -    COMPOSITE(0, "hclk_sd", mux_pll_src_cpll_gpll_p, 0,
>>> +    COMPOSITE(HCLK_SD, "hclk_sd", mux_pll_src_cpll_gpll_p, 0,
>>>               RK3399_CLKSEL_CON(13), 15, 1, MFLAGS, 8, 5, DFLAGS,
>>>               RK3399_CLKGATE_CON(12), 13, GFLAGS),
>>>       GATE(HCLK_SDMMC, "hclk_sdmmc", "hclk_sd", 0,
>>>
>>
>>
>>
> 
> 
> 
> 


-- 
Best Regards
Shawn Lin


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH] clk: rockchip: assign correct id for pclk_ddr and hclk_sd in rk3399

[Doug Anderson]

Hi,

On Thu, Mar 15, 2018 at 6:40 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
> [Correct Doug's email address]
>
> On 2018/3/15 17:12, hl wrote:
>>
>> Hi Shawn,
>>
>>
>> On Thursday, March 15, 2018 05:01 PM, Shawn Lin wrote:
>>>
>>> Hi Huang,
>>>
>>> On 2018/3/15 16:54, Lin Huang wrote:
>>>>
>>>> Assign id PCLK_DDR to pclk_ddr, id HCLK_SD to hclk_sd, so we can
>>>> assign frequency for them in dts.
>>>
>>>
>>> I'm curious under which condition that we need assign frequency for
>>> hclk_sd?
>>
>> We may set CPLL frequency higher than 800MHz, with that we need assign
>> clock
>> to hclk_sd, otherwise it will exceed 200MHz, since we use the default cru
>> register value to get hclk_sd for now.
>> you can check
>>
>> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/956678/5
>
>
> Okay, thanks for pointing me to the actual user case.
> I'm fine with that, however, what I am thinking now is:
>
> (1) It's worth elaborating more in the commit msg.

Seems like a nice idea to me.


> (2) The reason you need set hclk_sd is that it depends on the
> defualt settings but lack real owner to explicitly set it to <=200MHz.

Actually, in the case of hclk_sd there _is_ an owner to set it to <=
200 MHz.  I'll comment on the gerrit review, but the assigned clock
for hclk_sd probably belongs in the node "sdmmc: dwmmc@fe320000".

...but in any case, you still need the clock ID to do that.


> So my another question is if that is more about aspect of power
> consumption, then it looks okay to me. But if that is a SoC limitation,
> then we are under risk for that. Either we should never rely on the
> default settings, or we should set its rate range after registering this
> clock provider.

I have always wondered about perhaps encoding the min/max clock rate
for each clock somewhere in the source code.  Then whenever we change
clock rates we could enforce that we don't ever go past this min/max.
In think, in theory, this is possible by registering for all the right
callbacks / notifications.  ...but I suspect that doing in a generic /
performant way might be very difficult.  Specifically, whenever a
clock changes you may need to make all sorts of decisions about
re-parenting and also checking whether all your children are out fo
spec.

So without encoding the min/max and having it magically auto-resolve,
the best we can do is just to assign a sane clock rate.  If there's no
subsystem owning this clock then doing so at clock init time is sane
(and that's what we do with many clocks).  If a subsystem owns it,
doing it when the subsystem is probed is sane.


So, to summarize, I'm happy with this change.  I wouldn't mind a
little more justification in the CL description but personally I won't
make a big deal about it.

Reviewed-by: Douglas Anderson <dianders@chromium.org>


-Doug

Re: [PATCH] clk: rockchip: assign correct id for pclk_ddr and hclk_sd in rk3399

[Doug Anderson]

Hi,

On Thu, Mar 15, 2018 at 6:40 AM, Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
> [Correct Doug's email address]
>
> On 2018/3/15 17:12, hl wrote:
>>
>> Hi Shawn,
>>
>>
>> On Thursday, March 15, 2018 05:01 PM, Shawn Lin wrote:
>>>
>>> Hi Huang,
>>>
>>> On 2018/3/15 16:54, Lin Huang wrote:
>>>>
>>>> Assign id PCLK_DDR to pclk_ddr, id HCLK_SD to hclk_sd, so we can
>>>> assign frequency for them in dts.
>>>
>>>
>>> I'm curious under which condition that we need assign frequency for
>>> hclk_sd?
>>
>> We may set CPLL frequency higher than 800MHz, with that we need assign
>> clock
>> to hclk_sd, otherwise it will exceed 200MHz, since we use the default cru
>> register value to get hclk_sd for now.
>> you can check
>>
>> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/956678/5
>
>
> Okay, thanks for pointing me to the actual user case.
> I'm fine with that, however, what I am thinking now is:
>
> (1) It's worth elaborating more in the commit msg.

Seems like a nice idea to me.


> (2) The reason you need set hclk_sd is that it depends on the
> defualt settings but lack real owner to explicitly set it to <=200MHz.

Actually, in the case of hclk_sd there _is_ an owner to set it to <=
200 MHz.  I'll comment on the gerrit review, but the assigned clock
for hclk_sd probably belongs in the node "sdmmc: dwmmc@fe320000".

...but in any case, you still need the clock ID to do that.


> So my another question is if that is more about aspect of power
> consumption, then it looks okay to me. But if that is a SoC limitation,
> then we are under risk for that. Either we should never rely on the
> default settings, or we should set its rate range after registering this
> clock provider.

I have always wondered about perhaps encoding the min/max clock rate
for each clock somewhere in the source code.  Then whenever we change
clock rates we could enforce that we don't ever go past this min/max.
In think, in theory, this is possible by registering for all the right
callbacks / notifications.  ...but I suspect that doing in a generic /
performant way might be very difficult.  Specifically, whenever a
clock changes you may need to make all sorts of decisions about
re-parenting and also checking whether all your children are out fo
spec.

So without encoding the min/max and having it magically auto-resolve,
the best we can do is just to assign a sane clock rate.  If there's no
subsystem owning this clock then doing so at clock init time is sane
(and that's what we do with many clocks).  If a subsystem owns it,
doing it when the subsystem is probed is sane.


So, to summarize, I'm happy with this change.  I wouldn't mind a
little more justification in the CL description but personally I won't
make a big deal about it.

Reviewed-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>


-Doug

Re: [PATCH] clk: rockchip: assign correct id for pclk_ddr and hclk_sd in rk3399

[Shawn Lin]

Hi Doug,

On 2018/3/16 0:20, Doug Anderson wrote:
> Hi,
> 
> On Thu, Mar 15, 2018 at 6:40 AM, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>> [Correct Doug's email address]
>>
>> On 2018/3/15 17:12, hl wrote:
>>>
>>> Hi Shawn,
>>>
>>>
>>> On Thursday, March 15, 2018 05:01 PM, Shawn Lin wrote:
>>>>
>>>> Hi Huang,
>>>>
>>>> On 2018/3/15 16:54, Lin Huang wrote:
>>>>>
>>>>> Assign id PCLK_DDR to pclk_ddr, id HCLK_SD to hclk_sd, so we can
>>>>> assign frequency for them in dts.
>>>>
>>>>
>>>> I'm curious under which condition that we need assign frequency for
>>>> hclk_sd?
>>>
>>> We may set CPLL frequency higher than 800MHz, with that we need assign
>>> clock
>>> to hclk_sd, otherwise it will exceed 200MHz, since we use the default cru
>>> register value to get hclk_sd for now.
>>> you can check
>>>
>>> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/956678/5
>>
>>
>> Okay, thanks for pointing me to the actual user case.
>> I'm fine with that, however, what I am thinking now is:
>>
>> (1) It's worth elaborating more in the commit msg.
> 
> Seems like a nice idea to me.
> 
> 
>> (2) The reason you need set hclk_sd is that it depends on the
>> defualt settings but lack real owner to explicitly set it to <=200MHz.
> 
> Actually, in the case of hclk_sd there _is_ an owner to set it to <=
> 200 MHz.  I'll comment on the gerrit review, but the assigned clock
> for hclk_sd probably belongs in the node "sdmmc: dwmmc@fe320000".

Agreed. hclk_sd is the parent for hclk_sdmmc and hclk_sdmmc_noc, so
it makes more sense to me for sdmmc node to pick it up in rk3399.dtsi

> 
> ...but in any case, you still need the clock ID to do that.
> 

yep.

> 
>> So my another question is if that is more about aspect of power
>> consumption, then it looks okay to me. But if that is a SoC limitation,
>> then we are under risk for that. Either we should never rely on the
>> default settings, or we should set its rate range after registering this
>> clock provider.
> 
> I have always wondered about perhaps encoding the min/max clock rate
> for each clock somewhere in the source code.  Then whenever we change
> clock rates we could enforce that we don't ever go past this min/max. > In think, in theory, this is possible by registering for all the right
> callbacks / notifications.  ...but I suspect that doing in a generic /
> performant way might be very difficult.  Specifically, whenever a
> clock changes you may need to make all sorts of decisions about
> re-parenting and also checking whether all your children are out fo
> spec.

Agreed.

> 
> So without encoding the min/max and having it magically auto-resolve,
> the best we can do is just to assign a sane clock rate.  If there's no
> subsystem owning this clock then doing so at clock init time is sane
> (and that's what we do with many clocks).  If a subsystem owns it,
> doing it when the subsystem is probed is sane.
> 

I'm not convinced it was invented to abuse encoding the rate range for
each clock, but just enclose the the hardware limitation within the
clock framework and per-SoC clock providers. That being said, the author
of per-SoC clock providers is more fimilar with the hardware limitation
than BSP guys, so it's less prone to make fatal mistake by encoding this
kinda limitation somewhere in clk provider drivers.

But yes, I didn't see any hardware limitation here for hclk_sd, so my
best guess is 200MHz is a tradeoff for perf and power. This looks sane
to me by doing it at clock init time, so FWIW for this patch,

Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com>

However, I'm incline to assigne hclk_sd by sdmmc node in Huang's CL.
Then we may never warry about leaving it along again when we randomly
adjust clk hierarchy again. It applies to all this kinda clk providers.

> 
> So, to summarize, I'm happy with this change.  I wouldn't mind a
> little more justification in the CL description but personally I won't
> make a big deal about it.
> 
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> 
> 
> -Doug
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
> 
> 
> 

Re: [PATCH] clk: rockchip: assign correct id for pclk_ddr and hclk_sd in rk3399

[Shawn Lin]

Hi Doug,

On 2018/3/16 0:20, Doug Anderson wrote:
> Hi,
> 
> On Thu, Mar 15, 2018 at 6:40 AM, Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org> wrote:
>> [Correct Doug's email address]
>>
>> On 2018/3/15 17:12, hl wrote:
>>>
>>> Hi Shawn,
>>>
>>>
>>> On Thursday, March 15, 2018 05:01 PM, Shawn Lin wrote:
>>>>
>>>> Hi Huang,
>>>>
>>>> On 2018/3/15 16:54, Lin Huang wrote:
>>>>>
>>>>> Assign id PCLK_DDR to pclk_ddr, id HCLK_SD to hclk_sd, so we can
>>>>> assign frequency for them in dts.
>>>>
>>>>
>>>> I'm curious under which condition that we need assign frequency for
>>>> hclk_sd?
>>>
>>> We may set CPLL frequency higher than 800MHz, with that we need assign
>>> clock
>>> to hclk_sd, otherwise it will exceed 200MHz, since we use the default cru
>>> register value to get hclk_sd for now.
>>> you can check
>>>
>>> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/956678/5
>>
>>
>> Okay, thanks for pointing me to the actual user case.
>> I'm fine with that, however, what I am thinking now is:
>>
>> (1) It's worth elaborating more in the commit msg.
> 
> Seems like a nice idea to me.
> 
> 
>> (2) The reason you need set hclk_sd is that it depends on the
>> defualt settings but lack real owner to explicitly set it to <=200MHz.
> 
> Actually, in the case of hclk_sd there _is_ an owner to set it to <=
> 200 MHz.  I'll comment on the gerrit review, but the assigned clock
> for hclk_sd probably belongs in the node "sdmmc: dwmmc@fe320000".

Agreed. hclk_sd is the parent for hclk_sdmmc and hclk_sdmmc_noc, so
it makes more sense to me for sdmmc node to pick it up in rk3399.dtsi

> 
> ...but in any case, you still need the clock ID to do that.
> 

yep.

> 
>> So my another question is if that is more about aspect of power
>> consumption, then it looks okay to me. But if that is a SoC limitation,
>> then we are under risk for that. Either we should never rely on the
>> default settings, or we should set its rate range after registering this
>> clock provider.
> 
> I have always wondered about perhaps encoding the min/max clock rate
> for each clock somewhere in the source code.  Then whenever we change
> clock rates we could enforce that we don't ever go past this min/max. > In think, in theory, this is possible by registering for all the right
> callbacks / notifications.  ...but I suspect that doing in a generic /
> performant way might be very difficult.  Specifically, whenever a
> clock changes you may need to make all sorts of decisions about
> re-parenting and also checking whether all your children are out fo
> spec.

Agreed.

> 
> So without encoding the min/max and having it magically auto-resolve,
> the best we can do is just to assign a sane clock rate.  If there's no
> subsystem owning this clock then doing so at clock init time is sane
> (and that's what we do with many clocks).  If a subsystem owns it,
> doing it when the subsystem is probed is sane.
> 

I'm not convinced it was invented to abuse encoding the rate range for
each clock, but just enclose the the hardware limitation within the
clock framework and per-SoC clock providers. That being said, the author
of per-SoC clock providers is more fimilar with the hardware limitation
than BSP guys, so it's less prone to make fatal mistake by encoding this
kinda limitation somewhere in clk provider drivers.

But yes, I didn't see any hardware limitation here for hclk_sd, so my
best guess is 200MHz is a tradeoff for perf and power. This looks sane
to me by doing it at clock init time, so FWIW for this patch,

Reviewed-by: Shawn Lin <shawn.lin-TNX95d0MmH7DzftRWevZcw@public.gmane.org>

However, I'm incline to assigne hclk_sd by sdmmc node in Huang's CL.
Then we may never warry about leaving it along again when we randomly
adjust clk hierarchy again. It applies to all this kinda clk providers.

> 
> So, to summarize, I'm happy with this change.  I wouldn't mind a
> little more justification in the CL description but personally I won't
> make a big deal about it.
> 
> Reviewed-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> 
> 
> -Doug
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
> 
> 
>