[RESEND PATCH v1 0/4] fix some clock configuration for the RK3036 platform

[RESEND PATCH v1 0/4] fix some clock configuration for the RK3036 platform

[Xing Zheng]

Hi:
  In the development work, we found that some of the previous incorrect
clock configuration on the RK3036 platform, we should fix them.



Xing Zheng (4):
  clk: rockchip: rk3036: fix the FLAGs for clock mux
  clk: rockchip: rk3036: fix uarts clock error
  clk: rockchip: rk3036: rename emac ext source clock
  clk: rockchip: rk3036: fix and add node id for emac clock

 drivers/clk/rockchip/clk-rk3036.c      |   33 ++++++++++++++++----------------
 include/dt-bindings/clock/rk3036-cru.h |    2 ++
 2 files changed, 19 insertions(+), 16 deletions(-)

-- 
1.7.9.5

[RESEND PATCH v1 4/4] clk: rockchip: rk3036: fix and add node id for emac clock

[Xing Zheng]

Due to referred old version TRM, there is incorrect emac clock node,
we should fix it. The SEL_21_9 is the parent of SEL_21_4.

In the emac driver, we need to refer HCLK_MAC, and  because There are
only 3PLLs (APLL/GPLL/DPLL) on the rk3036, most clock are under the
GPLL, and it is unable to provide the accurate rate for mac_ref which
need to 50MHz probability, we should let it under the APLL and are
able to set the freq which integer multiples of 50MHz, so we add these
emac node for reference.

Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
---

 drivers/clk/rockchip/clk-rk3036.c      |   11 ++++++-----
 include/dt-bindings/clock/rk3036-cru.h |    2 ++
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/rockchip/clk-rk3036.c b/drivers/clk/rockchip/clk-rk3036.c
index 7420cbe..7863c4d 100644
--- a/drivers/clk/rockchip/clk-rk3036.c
+++ b/drivers/clk/rockchip/clk-rk3036.c
@@ -328,13 +328,14 @@ static struct rockchip_clk_branch rk3036_clk_branches[] __initdata = {
 			RK2928_CLKSEL_CON(16), 0, 2, MFLAGS, 2, 5, DFLAGS,
 			RK2928_CLKGATE_CON(10), 5, GFLAGS),
 
-	COMPOSITE_NOGATE(0, "mac_pll_src", mux_pll_src_3plls_p, 0,
-			RK2928_CLKSEL_CON(21), 0, 2, MFLAGS, 4, 5, DFLAGS),
+	MUX(SCLK_MACPLL, "mac_pll_pre", mux_pll_src_3plls_p, 0,
+			RK2928_CLKSEL_CON(21), 0, 2, MFLAGS),
+	DIV(0, "mac_pll_src", "mac_pll_pre", 0,
+			RK2928_CLKSEL_CON(21), 9, 5, DFLAGS),
 	MUX(SCLK_MACREF, "mac_clk_ref", mux_mac_p, CLK_SET_RATE_PARENT,
 			RK2928_CLKSEL_CON(21), 3, 1, MFLAGS),
-
 	COMPOSITE_NOMUX(SCLK_MAC, "mac_clk", "mac_clk_ref", 0,
-			RK2928_CLKSEL_CON(21), 9, 5, DFLAGS,
+			RK2928_CLKSEL_CON(21), 4, 5, DFLAGS,
 			RK2928_CLKGATE_CON(2), 6, GFLAGS),
 
 	MUX(SCLK_HDMI, "dclk_hdmi", mux_dclk_p, 0,
@@ -389,7 +390,7 @@ static struct rockchip_clk_branch rk3036_clk_branches[] __initdata = {
 	GATE(HCLK_OTG1, "hclk_otg1", "hclk_peri", CLK_IGNORE_UNUSED, RK2928_CLKGATE_CON(7), 3, GFLAGS),
 	GATE(HCLK_I2S, "hclk_i2s", "hclk_peri", 0, RK2928_CLKGATE_CON(7), 2, GFLAGS),
 	GATE(0, "hclk_sfc", "hclk_peri", CLK_IGNORE_UNUSED, RK2928_CLKGATE_CON(3), 14, GFLAGS),
-	GATE(0, "hclk_mac", "hclk_peri", CLK_IGNORE_UNUSED, RK2928_CLKGATE_CON(3), 15, GFLAGS),
+	GATE(HCLK_MAC, "hclk_mac", "hclk_peri", 0, RK2928_CLKGATE_CON(3), 5, GFLAGS),
 
 	/* pclk_peri gates */
 	GATE(0, "pclk_peri_matrix", "pclk_peri", CLK_IGNORE_UNUSED, RK2928_CLKGATE_CON(4), 1, GFLAGS),
diff --git a/include/dt-bindings/clock/rk3036-cru.h b/include/dt-bindings/clock/rk3036-cru.h
index ebc7a7b..de44109 100644
--- a/include/dt-bindings/clock/rk3036-cru.h
+++ b/include/dt-bindings/clock/rk3036-cru.h
@@ -54,6 +54,7 @@
 #define SCLK_PVTM_VIDEO		125
 #define SCLK_MAC		151
 #define SCLK_MACREF		152
+#define SCLK_MACPLL		153
 #define SCLK_SFC		160
 
 /* aclk gates */
@@ -92,6 +93,7 @@
 #define HCLK_SDMMC		456
 #define HCLK_SDIO		457
 #define HCLK_EMMC		459
+#define HCLK_MAC		460
 #define HCLK_I2S		462
 #define HCLK_LCDC		465
 #define HCLK_ROM		467
-- 
1.7.9.5

Re: [RESEND PATCH v1 4/4] clk: rockchip: rk3036: fix and add node id for emac clock

[Heiko Stübner]

Hi,

Am Montag, 28. Dezember 2015, 17:03:53 schrieb Xing Zheng:
> Due to referred old version TRM, there is incorrect emac clock node,
> we should fix it. The SEL_21_9 is the parent of SEL_21_4.
> 
> In the emac driver, we need to refer HCLK_MAC, and  because There are
> only 3PLLs (APLL/GPLL/DPLL) on the rk3036, most clock are under the
> GPLL, and it is unable to provide the accurate rate for mac_ref which
> need to 50MHz probability, we should let it under the APLL and are
> able to set the freq which integer multiples of 50MHz, so we add these
> emac node for reference.

I don't really follow here. While I do understand that the emac needs 50MHz, I 
don't think using the APLL as source is helpful.

The APLL is the main clocksource for the cpu-cores, including frequency 
scaling, and while it currently only lists 816MHz as sole frequency, you're 
pretty much guaranteed to not get your correct multiple of 50MHz from there 
either. And limiting the cpu to just do 600MHz to get the mac working sounds 
pretty bad ;-) .


In the rk3036 cru-node the gpll gets set to 594MHz. Is there a special reason 
why it needs to be 594MHz and cannot be a round 600MHz? Because that would 
also provide your 50MHz-multiple nicely.

> Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
> ---
> 
>  drivers/clk/rockchip/clk-rk3036.c      |   11 ++++++-----
>  include/dt-bindings/clock/rk3036-cru.h |    2 ++
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clk/rockchip/clk-rk3036.c
> b/drivers/clk/rockchip/clk-rk3036.c index 7420cbe..7863c4d 100644
> --- a/drivers/clk/rockchip/clk-rk3036.c
> +++ b/drivers/clk/rockchip/clk-rk3036.c
> @@ -328,13 +328,14 @@ static struct rockchip_clk_branch
> rk3036_clk_branches[] __initdata = { RK2928_CLKSEL_CON(16), 0, 2, MFLAGS,
> 2, 5, DFLAGS,
>  			RK2928_CLKGATE_CON(10), 5, GFLAGS),
> 
> -	COMPOSITE_NOGATE(0, "mac_pll_src", mux_pll_src_3plls_p, 0,
> -			RK2928_CLKSEL_CON(21), 0, 2, MFLAGS, 4, 5, DFLAGS),
> +	MUX(SCLK_MACPLL, "mac_pll_pre", mux_pll_src_3plls_p, 0,
> +			RK2928_CLKSEL_CON(21), 0, 2, MFLAGS),
> +	DIV(0, "mac_pll_src", "mac_pll_pre", 0,
> +			RK2928_CLKSEL_CON(21), 9, 5, DFLAGS),
>  	MUX(SCLK_MACREF, "mac_clk_ref", mux_mac_p, CLK_SET_RATE_PARENT,
>  			RK2928_CLKSEL_CON(21), 3, 1, MFLAGS),
> -
>  	COMPOSITE_NOMUX(SCLK_MAC, "mac_clk", "mac_clk_ref", 0,
> -			RK2928_CLKSEL_CON(21), 9, 5, DFLAGS,
> +			RK2928_CLKSEL_CON(21), 4, 5, DFLAGS,
>  			RK2928_CLKGATE_CON(2), 6, GFLAGS),
> 
>  	MUX(SCLK_HDMI, "dclk_hdmi", mux_dclk_p, 0,
> @@ -389,7 +390,7 @@ static struct rockchip_clk_branch rk3036_clk_branches[]
> __initdata = { GATE(HCLK_OTG1, "hclk_otg1", "hclk_peri", CLK_IGNORE_UNUSED,
> RK2928_CLKGATE_CON(7), 3, GFLAGS), GATE(HCLK_I2S, "hclk_i2s", "hclk_peri",
> 0, RK2928_CLKGATE_CON(7), 2, GFLAGS), GATE(0, "hclk_sfc", "hclk_peri",
> CLK_IGNORE_UNUSED, RK2928_CLKGATE_CON(3), 14, GFLAGS), -	GATE(0,
> "hclk_mac", "hclk_peri", CLK_IGNORE_UNUSED, RK2928_CLKGATE_CON(3), 15,
> GFLAGS), +	GATE(HCLK_MAC, "hclk_mac", "hclk_peri", 0,
> RK2928_CLKGATE_CON(3), 5, GFLAGS),
> 
>  	/* pclk_peri gates */
>  	GATE(0, "pclk_peri_matrix", "pclk_peri", CLK_IGNORE_UNUSED,
> RK2928_CLKGATE_CON(4), 1, GFLAGS), diff --git
> a/include/dt-bindings/clock/rk3036-cru.h
> b/include/dt-bindings/clock/rk3036-cru.h index ebc7a7b..de44109 100644
> --- a/include/dt-bindings/clock/rk3036-cru.h
> +++ b/include/dt-bindings/clock/rk3036-cru.h
> @@ -54,6 +54,7 @@
>  #define SCLK_PVTM_VIDEO		125
>  #define SCLK_MAC		151
>  #define SCLK_MACREF		152
> +#define SCLK_MACPLL		153
>  #define SCLK_SFC		160
> 
>  /* aclk gates */
> @@ -92,6 +93,7 @@

please separate the hclk addition into two separate patches:
patch1: add the clock-id to the dt-binding header
patch2: use the clock in the clock-driver

>  #define HCLK_SDMMC		456
>  #define HCLK_SDIO		457
>  #define HCLK_EMMC		459
> +#define HCLK_MAC		460
>  #define HCLK_I2S		462
>  #define HCLK_LCDC		465
>  #define HCLK_ROM		467


Thanks
Heiko

Re: [RESEND PATCH v1 4/4] clk: rockchip: rk3036: fix and add node id for emac clock

[Yakir Yang]

Hi Heiko,

On 12/28/2015 08:41 PM, Heiko Stübner wrote:
> Hi,
>
> Am Montag, 28. Dezember 2015, 17:03:53 schrieb Xing Zheng:
>> Due to referred old version TRM, there is incorrect emac clock node,
>> we should fix it. The SEL_21_9 is the parent of SEL_21_4.
>>
>> In the emac driver, we need to refer HCLK_MAC, and  because There are
>> only 3PLLs (APLL/GPLL/DPLL) on the rk3036, most clock are under the
>> GPLL, and it is unable to provide the accurate rate for mac_ref which
>> need to 50MHz probability, we should let it under the APLL and are
>> able to set the freq which integer multiples of 50MHz, so we add these
>> emac node for reference.
> I don't really follow here. While I do understand that the emac needs 50MHz, I
> don't think using the APLL as source is helpful.
>
> The APLL is the main clocksource for the cpu-cores, including frequency
> scaling, and while it currently only lists 816MHz as sole frequency, you're
> pretty much guaranteed to not get your correct multiple of 50MHz from there
> either. And limiting the cpu to just do 600MHz to get the mac working sounds
> pretty bad ;-) .
>
>
> In the rk3036 cru-node the gpll gets set to 594MHz. Is there a special reason
> why it needs to be 594MHz and cannot be a round 600MHz? Because that would
> also provide your 50MHz-multiple nicely.

Yes, this magic 594MHz would help to support the standard HDMI 
resolutions, here are the math:

1920x1080-60Hz DCLK = 148.5MHz = 594MHz / 4
1280x720-60Hz DCLK = 74.25MHz = 594MHz / 8
720x480-60Hz DCLK = 27MHz = 594MHz / 22

Thanks,
- Yakir

>> Signed-off-by: Xing Zheng <zhengxing@rock-chips.com>
>> ---
>>
>>   drivers/clk/rockchip/clk-rk3036.c      |   11 ++++++-----
>>   include/dt-bindings/clock/rk3036-cru.h |    2 ++
>>   2 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/clk/rockchip/clk-rk3036.c
>> b/drivers/clk/rockchip/clk-rk3036.c index 7420cbe..7863c4d 100644
>> --- a/drivers/clk/rockchip/clk-rk3036.c
>> +++ b/drivers/clk/rockchip/clk-rk3036.c
>> @@ -328,13 +328,14 @@ static struct rockchip_clk_branch
>> rk3036_clk_branches[] __initdata = { RK2928_CLKSEL_CON(16), 0, 2, MFLAGS,
>> 2, 5, DFLAGS,
>>   			RK2928_CLKGATE_CON(10), 5, GFLAGS),
>>
>> -	COMPOSITE_NOGATE(0, "mac_pll_src", mux_pll_src_3plls_p, 0,
>> -			RK2928_CLKSEL_CON(21), 0, 2, MFLAGS, 4, 5, DFLAGS),
>> +	MUX(SCLK_MACPLL, "mac_pll_pre", mux_pll_src_3plls_p, 0,
>> +			RK2928_CLKSEL_CON(21), 0, 2, MFLAGS),
>> +	DIV(0, "mac_pll_src", "mac_pll_pre", 0,
>> +			RK2928_CLKSEL_CON(21), 9, 5, DFLAGS),
>>   	MUX(SCLK_MACREF, "mac_clk_ref", mux_mac_p, CLK_SET_RATE_PARENT,
>>   			RK2928_CLKSEL_CON(21), 3, 1, MFLAGS),
>> -
>>   	COMPOSITE_NOMUX(SCLK_MAC, "mac_clk", "mac_clk_ref", 0,
>> -			RK2928_CLKSEL_CON(21), 9, 5, DFLAGS,
>> +			RK2928_CLKSEL_CON(21), 4, 5, DFLAGS,
>>   			RK2928_CLKGATE_CON(2), 6, GFLAGS),
>>
>>   	MUX(SCLK_HDMI, "dclk_hdmi", mux_dclk_p, 0,
>> @@ -389,7 +390,7 @@ static struct rockchip_clk_branch rk3036_clk_branches[]
>> __initdata = { GATE(HCLK_OTG1, "hclk_otg1", "hclk_peri", CLK_IGNORE_UNUSED,
>> RK2928_CLKGATE_CON(7), 3, GFLAGS), GATE(HCLK_I2S, "hclk_i2s", "hclk_peri",
>> 0, RK2928_CLKGATE_CON(7), 2, GFLAGS), GATE(0, "hclk_sfc", "hclk_peri",
>> CLK_IGNORE_UNUSED, RK2928_CLKGATE_CON(3), 14, GFLAGS), -	GATE(0,
>> "hclk_mac", "hclk_peri", CLK_IGNORE_UNUSED, RK2928_CLKGATE_CON(3), 15,
>> GFLAGS), +	GATE(HCLK_MAC, "hclk_mac", "hclk_peri", 0,
>> RK2928_CLKGATE_CON(3), 5, GFLAGS),
>>
>>   	/* pclk_peri gates */
>>   	GATE(0, "pclk_peri_matrix", "pclk_peri", CLK_IGNORE_UNUSED,
>> RK2928_CLKGATE_CON(4), 1, GFLAGS), diff --git
>> a/include/dt-bindings/clock/rk3036-cru.h
>> b/include/dt-bindings/clock/rk3036-cru.h index ebc7a7b..de44109 100644
>> --- a/include/dt-bindings/clock/rk3036-cru.h
>> +++ b/include/dt-bindings/clock/rk3036-cru.h
>> @@ -54,6 +54,7 @@
>>   #define SCLK_PVTM_VIDEO		125
>>   #define SCLK_MAC		151
>>   #define SCLK_MACREF		152
>> +#define SCLK_MACPLL		153
>>   #define SCLK_SFC		160
>>
>>   /* aclk gates */
>> @@ -92,6 +93,7 @@
> please separate the hclk addition into two separate patches:
> patch1: add the clock-id to the dt-binding header
> patch2: use the clock in the clock-driver
>
>>   #define HCLK_SDMMC		456
>>   #define HCLK_SDIO		457
>>   #define HCLK_EMMC		459
>> +#define HCLK_MAC		460
>>   #define HCLK_I2S		462
>>   #define HCLK_LCDC		465
>>   #define HCLK_ROM		467
>
> Thanks
> Heiko
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
>
>

Re: [RESEND PATCH v1 4/4] clk: rockchip: rk3036: fix and add node id for emac clock

[Xing Zheng]

On 2015年12月29日 09:59, Yakir Yang wrote:
> Hi Heiko,
>
> On 12/28/2015 08:41 PM, Heiko Stübner wrote:
>> Hi,
>>
>> Am Montag, 28. Dezember 2015, 17:03:53 schrieb Xing Zheng:
>>> Due to referred old version TRM, there is incorrect emac clock node,
>>> we should fix it. The SEL_21_9 is the parent of SEL_21_4.
>>>
>>> In the emac driver, we need to refer HCLK_MAC, and because There are
>>> only 3PLLs (APLL/GPLL/DPLL) on the rk3036, most clock are under the
>>> GPLL, and it is unable to provide the accurate rate for mac_ref which
>>> need to 50MHz probability, we should let it under the APLL and are
>>> able to set the freq which integer multiples of 50MHz, so we add these
>>> emac node for reference.
>> I don't really follow here. While I do understand that the emac needs 
>> 50MHz, I
>> don't think using the APLL as source is helpful.
>>
>> The APLL is the main clocksource for the cpu-cores, including frequency
>> scaling, and while it currently only lists 816MHz as sole frequency, 
>> you're
>> pretty much guaranteed to not get your correct multiple of 50MHz from 
>> there
>> either. And limiting the cpu to just do 600MHz to get the mac working 
>> sounds
>> pretty bad ;-) .
>>
>>
>> In the rk3036 cru-node the gpll gets set to 594MHz. Is there a 
>> special reason
>> why it needs to be 594MHz and cannot be a round 600MHz? Because that 
>> would
>> also provide your 50MHz-multiple nicely.
>
> Yes, this magic 594MHz would help to support the standard HDMI 
> resolutions, here are the math:
>
> 1920x1080-60Hz DCLK = 148.5MHz = 594MHz / 4
> 1280x720-60Hz DCLK = 74.25MHz = 594MHz / 8
> 720x480-60Hz DCLK = 27MHz = 594MHz / 22
>
> Thanks,
> - Yakir
Thanks Yakir.

Hi Heiko,
 From the above, do you have better idea for the RK3036's emac without 
ext-oscillator?

Thanks. :-)
>
>>> Signed-off-by: Xing Zheng <zhengxing-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>> ---
>>>
>>> drivers/clk/rockchip/clk-rk3036.c | 11 ++++++-----
>>> include/dt-bindings/clock/rk3036-cru.h | 2 ++
>>> 2 files changed, 8 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/clk/rockchip/clk-rk3036.c
>>> b/drivers/clk/rockchip/clk-rk3036.c index 7420cbe..7863c4d 100644
>>> --- a/drivers/clk/rockchip/clk-rk3036.c
>>> +++ b/drivers/clk/rockchip/clk-rk3036.c
>>> @@ -328,13 +328,14 @@ static struct rockchip_clk_branch
>>> rk3036_clk_branches[] __initdata = { RK2928_CLKSEL_CON(16), 0, 2, 
>>> MFLAGS,
>>> 2, 5, DFLAGS,
>>> RK2928_CLKGATE_CON(10), 5, GFLAGS),
>>>
>>> - COMPOSITE_NOGATE(0, "mac_pll_src", mux_pll_src_3plls_p, 0,
>>> - RK2928_CLKSEL_CON(21), 0, 2, MFLAGS, 4, 5, DFLAGS),
>>> + MUX(SCLK_MACPLL, "mac_pll_pre", mux_pll_src_3plls_p, 0,
>>> + RK2928_CLKSEL_CON(21), 0, 2, MFLAGS),
>>> + DIV(0, "mac_pll_src", "mac_pll_pre", 0,
>>> + RK2928_CLKSEL_CON(21), 9, 5, DFLAGS),
>>> MUX(SCLK_MACREF, "mac_clk_ref", mux_mac_p, CLK_SET_RATE_PARENT,
>>> RK2928_CLKSEL_CON(21), 3, 1, MFLAGS),
>>> -
>>> COMPOSITE_NOMUX(SCLK_MAC, "mac_clk", "mac_clk_ref", 0,
>>> - RK2928_CLKSEL_CON(21), 9, 5, DFLAGS,
>>> + RK2928_CLKSEL_CON(21), 4, 5, DFLAGS,
>>> RK2928_CLKGATE_CON(2), 6, GFLAGS),
>>>
>>> MUX(SCLK_HDMI, "dclk_hdmi", mux_dclk_p, 0,
>>> @@ -389,7 +390,7 @@ static struct rockchip_clk_branch 
>>> rk3036_clk_branches[]
>>> __initdata = { GATE(HCLK_OTG1, "hclk_otg1", "hclk_peri", 
>>> CLK_IGNORE_UNUSED,
>>> RK2928_CLKGATE_CON(7), 3, GFLAGS), GATE(HCLK_I2S, "hclk_i2s", 
>>> "hclk_peri",
>>> 0, RK2928_CLKGATE_CON(7), 2, GFLAGS), GATE(0, "hclk_sfc", "hclk_peri",
>>> CLK_IGNORE_UNUSED, RK2928_CLKGATE_CON(3), 14, GFLAGS), - GATE(0,
>>> "hclk_mac", "hclk_peri", CLK_IGNORE_UNUSED, RK2928_CLKGATE_CON(3), 15,
>>> GFLAGS), + GATE(HCLK_MAC, "hclk_mac", "hclk_peri", 0,
>>> RK2928_CLKGATE_CON(3), 5, GFLAGS),
>>>
>>> /* pclk_peri gates */
>>> GATE(0, "pclk_peri_matrix", "pclk_peri", CLK_IGNORE_UNUSED,
>>> RK2928_CLKGATE_CON(4), 1, GFLAGS), diff --git
>>> a/include/dt-bindings/clock/rk3036-cru.h
>>> b/include/dt-bindings/clock/rk3036-cru.h index ebc7a7b..de44109 100644
>>> --- a/include/dt-bindings/clock/rk3036-cru.h
>>> +++ b/include/dt-bindings/clock/rk3036-cru.h
>>> @@ -54,6 +54,7 @@
>>> #define SCLK_PVTM_VIDEO 125
>>> #define SCLK_MAC 151
>>> #define SCLK_MACREF 152
>>> +#define SCLK_MACPLL 153
>>> #define SCLK_SFC 160
>>>
>>> /* aclk gates */
>>> @@ -92,6 +93,7 @@
>> please separate the hclk addition into two separate patches:
>> patch1: add the clock-id to the dt-binding header
>> patch2: use the clock in the clock-driver
Done.
>>
>>> #define HCLK_SDMMC 456
>>> #define HCLK_SDIO 457
>>> #define HCLK_EMMC 459
>>> +#define HCLK_MAC 460
>>> #define HCLK_I2S 462
>>> #define HCLK_LCDC 465
>>> #define HCLK_ROM 467
>>
>> Thanks
>> Heiko
>>
>> _______________________________________________
>> Linux-rockchip mailing list
>> Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>>
>>
>>
>
>
>


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND PATCH v1 4/4] clk: rockchip: rk3036: fix and add node id for emac clock

[Heiko Stübner]

[-- Attachment #1: Type: text/plain, Size: 2560 bytes --]

Hi Xing,

Am Dienstag, 29. Dezember 2015, 10:34:09 schrieb Xing Zheng:
> On 2015年12月29日 09:59, Yakir Yang wrote:
> > On 12/28/2015 08:41 PM, Heiko Stübner wrote:
> >> Am Montag, 28. Dezember 2015, 17:03:53 schrieb Xing Zheng:
> >>> Due to referred old version TRM, there is incorrect emac clock node,
> >>> we should fix it. The SEL_21_9 is the parent of SEL_21_4.
> >>> 
> >>> In the emac driver, we need to refer HCLK_MAC, and because There are
> >>> only 3PLLs (APLL/GPLL/DPLL) on the rk3036, most clock are under the
> >>> GPLL, and it is unable to provide the accurate rate for mac_ref which
> >>> need to 50MHz probability, we should let it under the APLL and are
> >>> able to set the freq which integer multiples of 50MHz, so we add these
> >>> emac node for reference.
> >> 
> >> I don't really follow here. While I do understand that the emac needs
> >> 50MHz, I
> >> don't think using the APLL as source is helpful.
> >> 
> >> The APLL is the main clocksource for the cpu-cores, including frequency
> >> scaling, and while it currently only lists 816MHz as sole frequency,
> >> you're
> >> pretty much guaranteed to not get your correct multiple of 50MHz from
> >> there
> >> either. And limiting the cpu to just do 600MHz to get the mac working
> >> sounds
> >> pretty bad ;-) .
> >> 
> >> 
> >> In the rk3036 cru-node the gpll gets set to 594MHz. Is there a
> >> special reason
> >> why it needs to be 594MHz and cannot be a round 600MHz? Because that
> >> would
> >> also provide your 50MHz-multiple nicely.
> > 
> > Yes, this magic 594MHz would help to support the standard HDMI
> > resolutions, here are the math:
> > 
> > 1920x1080-60Hz DCLK = 148.5MHz = 594MHz / 4
> > 1280x720-60Hz DCLK = 74.25MHz = 594MHz / 8
> > 720x480-60Hz DCLK = 27MHz = 594MHz / 22
> > 
> > Thanks,
> > - Yakir
> 
> Thanks Yakir.
> 
> Hi Heiko,
>  From the above, do you have better idea for the RK3036's emac without
> ext-oscillator?

During the last days I did play a bit with the clock framework. As I don't 
have a Kylin (or any rk3036) board, I did build a test-case with pclk_cpu on 
the rk3188 (which can be affected by the armclk if not reparented to the 
gpll), which got sucessfully adapted to get back to (or near) the originally 
requested frequency.

So ideally you could roll back your mux/div split here and try the attached 
diff. In theory it should help :-) .
As can be seen by the FIXMEs, not fully finished, but I'd like to determine if 
it fixes the issue at least.


Heiko

[-- Attachment #2: clk-keep-req-rate.diff --]
[-- Type: text/x-patch, Size: 2997 bytes --]

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 7bbb0fd..83a7234 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1423,6 +1423,9 @@ static struct clk_core *clk_propagate_rate_change(struct clk_core *core,
 	return fail_clk;
 }
 
+static int clk_core_set_rate_nolock(struct clk_core *core,
+				    unsigned long req_rate);
+
 /*
  * walk down a subtree and set the new rates notifying the rate
  * change on the way
@@ -1438,6 +1441,7 @@ static void clk_change_rate(struct clk_core *core)
 
 	old_rate = core->rate;
 
+printk("%s: %s requested %lu, new %lu\n", __func__, core->name, core->req_rate, core->new_rate);
 	if (core->new_parent)
 		best_parent_rate = core->new_parent->rate;
 	else if (core->parent)
@@ -1507,6 +1511,12 @@ static void clk_change_rate(struct clk_core *core)
 	/* handle the new child who might not be in core->children yet */
 	if (core->new_child)
 		clk_change_rate(core->new_child);
+
+	/* FIXME: add flag to limit to specific clocks? */
+	if (core->req_rate && core->new_rate != old_rate) {
+		printk("\t\ttrying to adapt %s\n", core->name);
+		clk_core_set_rate_nolock(core, core->req_rate);
+	}
 }
 
 static int clk_core_set_rate_nolock(struct clk_core *core,
@@ -1520,8 +1530,10 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 		return 0;
 
 	/* bail early if nothing to do */
-	if (rate == clk_core_get_rate_nolock(core))
+	if (rate == clk_core_get_rate_nolock(core)) {
+		core->req_rate = req_rate;
 		return 0;
+	}
 
 	if ((core->flags & CLK_SET_RATE_GATE) && core->prepare_count)
 		return -EBUSY;
@@ -1612,9 +1624,14 @@ int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max)
 	clk_prepare_lock();
 
 	if (min != clk->min_rate || max != clk->max_rate) {
+		unsigned long rate = clk->core->req_rate;
+
+		if (!rate)
+			rate = clk->core->rate;
+
 		clk->min_rate = min;
 		clk->max_rate = max;
-		ret = clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
+		ret = clk_core_set_rate_nolock(clk->core, rate);
 	}
 
 	clk_prepare_unlock();
@@ -2451,7 +2468,7 @@ static int __clk_init(struct device *dev, struct clk *clk_user)
 		rate = core->parent->rate;
 	else
 		rate = 0;
-	core->rate = core->req_rate = rate;
+	core->rate = rate;
 
 	/*
 	 * walk the list of orphan clocks and reparent any that are children of
@@ -2798,6 +2815,7 @@ int __clk_get(struct clk *clk)
 
 void __clk_put(struct clk *clk)
 {
+	unsigned long rate;
 	struct module *owner;
 
 	if (!clk || WARN_ON_ONCE(IS_ERR(clk)))
@@ -2806,9 +2824,13 @@ void __clk_put(struct clk *clk)
 	clk_prepare_lock();
 
 	hlist_del(&clk->clks_node);
-	if (clk->min_rate > clk->core->req_rate ||
-	    clk->max_rate < clk->core->req_rate)
-		clk_core_set_rate_nolock(clk->core, clk->core->req_rate);
+
+	rate = clk->core->req_rate;
+	if (!rate)
+		rate = clk->core->rate;
+
+	if (clk->min_rate > rate || clk->max_rate < rate)
+		clk_core_set_rate_nolock(clk->core, rate);
 
 	owner = clk->core->owner;
 	kref_put(&clk->core->ref, __clk_release);

Re: [RESEND PATCH v1 4/4] clk: rockchip: rk3036: fix and add node id for emac clock

[Xing Zheng]

Hi Heiko,
    Thank you for your patch, I will apply and test it later.

Thanks.

> 在 2016年1月2日，06:10，Heiko Stübner <heiko@sntech.de> 写道：
> 
> Hi Xing,
> 
> Am Dienstag, 29. Dezember 2015, 10:34:09 schrieb Xing Zheng:
>> On 2015年12月29日 09:59, Yakir Yang wrote:
>>> On 12/28/2015 08:41 PM, Heiko Stübner wrote:
>>>> Am Montag, 28. Dezember 2015, 17:03:53 schrieb Xing Zheng:
>>>>> Due to referred old version TRM, there is incorrect emac clock node,
>>>>> we should fix it. The SEL_21_9 is the parent of SEL_21_4.
>>>>> 
>>>>> In the emac driver, we need to refer HCLK_MAC, and because There are
>>>>> only 3PLLs (APLL/GPLL/DPLL) on the rk3036, most clock are under the
>>>>> GPLL, and it is unable to provide the accurate rate for mac_ref which
>>>>> need to 50MHz probability, we should let it under the APLL and are
>>>>> able to set the freq which integer multiples of 50MHz, so we add these
>>>>> emac node for reference.
>>>> 
>>>> I don't really follow here. While I do understand that the emac needs
>>>> 50MHz, I
>>>> don't think using the APLL as source is helpful.
>>>> 
>>>> The APLL is the main clocksource for the cpu-cores, including frequency
>>>> scaling, and while it currently only lists 816MHz as sole frequency,
>>>> you're
>>>> pretty much guaranteed to not get your correct multiple of 50MHz from
>>>> there
>>>> either. And limiting the cpu to just do 600MHz to get the mac working
>>>> sounds
>>>> pretty bad ;-) .
>>>> 
>>>> 
>>>> In the rk3036 cru-node the gpll gets set to 594MHz. Is there a
>>>> special reason
>>>> why it needs to be 594MHz and cannot be a round 600MHz? Because that
>>>> would
>>>> also provide your 50MHz-multiple nicely.
>>> 
>>> Yes, this magic 594MHz would help to support the standard HDMI
>>> resolutions, here are the math:
>>> 
>>> 1920x1080-60Hz DCLK = 148.5MHz = 594MHz / 4
>>> 1280x720-60Hz DCLK = 74.25MHz = 594MHz / 8
>>> 720x480-60Hz DCLK = 27MHz = 594MHz / 22
>>> 
>>> Thanks,
>>> - Yakir
>> 
>> Thanks Yakir.
>> 
>> Hi Heiko,
>> From the above, do you have better idea for the RK3036's emac without
>> ext-oscillator?
> 
> During the last days I did play a bit with the clock framework. As I don't 
> have a Kylin (or any rk3036) board, I did build a test-case with pclk_cpu on 
> the rk3188 (which can be affected by the armclk if not reparented to the 
> gpll), which got sucessfully adapted to get back to (or near) the originally 
> requested frequency.
> 
> So ideally you could roll back your mux/div split here and try the attached 
> diff. In theory it should help :-) .
> As can be seen by the FIXMEs, not fully finished, but I'd like to determine if 
> it fixes the issue at least.
> 
> 
> Heiko
> <clk-keep-req-rate.diff>

Re: [RESEND PATCH v1 4/4] clk: rockchip: rk3036: fix and add node id for emac clock

[Xing Zheng]

Hi Heiko,

On 2016年01月02日 10:34, Xing Zheng wrote:
> Hi Heiko,
>     Thank you for your patch, I will apply and test it later.
>
> Thanks.
>
>> 在 2016年1月2日，06:10，Heiko Stübner <heiko@sntech.de> 写道：
>>
>> Hi Xing,
>>
>> Am Dienstag, 29. Dezember 2015, 10:34:09 schrieb Xing Zheng:
>>> On 2015年12月29日 09:59, Yakir Yang wrote:
>>>> On 12/28/2015 08:41 PM, Heiko Stübner wrote:
>>>>> Am Montag, 28. Dezember 2015, 17:03:53 schrieb Xing Zheng:
>>>>>> Due to referred old version TRM, there is incorrect emac clock node,
>>>>>> we should fix it. The SEL_21_9 is the parent of SEL_21_4.
>>>>>>
>>>>>> In the emac driver, we need to refer HCLK_MAC, and because There are
>>>>>> only 3PLLs (APLL/GPLL/DPLL) on the rk3036, most clock are under the
>>>>>> GPLL, and it is unable to provide the accurate rate for mac_ref which
>>>>>> need to 50MHz probability, we should let it under the APLL and are
>>>>>> able to set the freq which integer multiples of 50MHz, so we add these
>>>>>> emac node for reference.
>>>>> I don't really follow here. While I do understand that the emac needs
>>>>> 50MHz, I
>>>>> don't think using the APLL as source is helpful.
>>>>>
>>>>> The APLL is the main clocksource for the cpu-cores, including frequency
>>>>> scaling, and while it currently only lists 816MHz as sole frequency,
>>>>> you're
>>>>> pretty much guaranteed to not get your correct multiple of 50MHz from
>>>>> there
>>>>> either. And limiting the cpu to just do 600MHz to get the mac working
>>>>> sounds
>>>>> pretty bad ;-) .
>>>>>
>>>>>
>>>>> In the rk3036 cru-node the gpll gets set to 594MHz. Is there a
>>>>> special reason
>>>>> why it needs to be 594MHz and cannot be a round 600MHz? Because that
>>>>> would
>>>>> also provide your 50MHz-multiple nicely.
>>>> Yes, this magic 594MHz would help to support the standard HDMI
>>>> resolutions, here are the math:
>>>>
>>>> 1920x1080-60Hz DCLK = 148.5MHz = 594MHz / 4
>>>> 1280x720-60Hz DCLK = 74.25MHz = 594MHz / 8
>>>> 720x480-60Hz DCLK = 27MHz = 594MHz / 22
>>>>
>>>> Thanks,
>>>> - Yakir
>>> Thanks Yakir.
>>>
>>> Hi Heiko,
>>> From the above, do you have better idea for the RK3036's emac without
>>> ext-oscillator?
>> During the last days I did play a bit with the clock framework. As I don't 
>> have a Kylin (or any rk3036) board, I did build a test-case with pclk_cpu on 
>> the rk3188 (which can be affected by the armclk if not reparented to the 
>> gpll), which got sucessfully adapted to get back to (or near) the originally 
>> requested frequency.
>>
>> So ideally you could roll back your mux/div split here and try the attached 
>> diff. In theory it should help :-) .
>> As can be seen by the FIXMEs, not fully finished, but I'd like to determine if 
>> it fixes the issue at least.
>>
>>
>> Heiko
>> <clk-keep-req-rate.diff>

I tested your patch just now, it seems to work well. I will do more tests.

Thank you. :-)