[PATCH v3 2/8] clk: samsung: add new clocks for DMC for Exynos5422 SoC

[PATCH v3 2/8] clk: samsung: add new clocks for DMC for Exynos5422 SoC

[Lukasz Luba]

This patch provides support for clocks needed for Dynamic Memory Controller
in Exynos5422 SoC. It adds CDREX base register addresses, new DIV, MUX and
GATE entries.

CC: Sylwester Nawrocki <s.nawrocki@samsung.com>
CC: Chanwoo Choi <cw00.choi@samsung.com>
CC: Michael Turquette <mturquette@baylibre.com>
CC: Stephen Boyd <sboyd@kernel.org>
CC: Kukjin Kim <kgene@kernel.org>
CC: Krzysztof Kozlowski <krzk@kernel.org>
CC: linux-samsung-soc@vger.kernel.org
CC: linux-clk@vger.kernel.org
CC: linux-arm-kernel@lists.infradead.org
CC: linux-kernel@vger.kernel.org
Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
---
 drivers/clk/samsung/clk-exynos5420.c | 48 +++++++++++++++++++++++++++++++++---
 1 file changed, 44 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
index 34cce3c..3e87421 100644
--- a/drivers/clk/samsung/clk-exynos5420.c
+++ b/drivers/clk/samsung/clk-exynos5420.c
@@ -132,6 +132,8 @@
 #define BPLL_LOCK		0x20010
 #define BPLL_CON0		0x20110
 #define SRC_CDREX		0x20200
+#define GATE_BUS_CDREX0		0x20700
+#define GATE_BUS_CDREX1		0x20704
 #define DIV_CDREX0		0x20500
 #define DIV_CDREX1		0x20504
 #define KPLL_LOCK		0x28000
@@ -248,6 +250,8 @@ static const unsigned long exynos5x_clk_regs[] __initconst = {
 	DIV_CDREX1,
 	SRC_KFC,
 	DIV_KFC0,
+	GATE_BUS_CDREX0,
+	GATE_BUS_CDREX1,
 };
 
 static const unsigned long exynos5800_clk_regs[] __initconst = {
@@ -425,6 +429,10 @@ PNAME(mout_group13_5800_p)	= { "dout_osc_div", "mout_sw_aclkfl1_550_cam" };
 PNAME(mout_group14_5800_p)	= { "dout_aclk550_cam", "dout_sclk_sw" };
 PNAME(mout_group15_5800_p)	= { "dout_osc_div", "mout_sw_aclk550_cam" };
 PNAME(mout_group16_5800_p)	= { "dout_osc_div", "mout_mau_epll_clk" };
+PNAME(mout_mx_mspll_ccore_phy_p) = { "sclk_bpll", "mout_dpll_ctrl",
+					"mout_mpll_ctrl", "ff_dout_spll2",
+					"mout_sclk_spll"};
+
 
 /* fixed rate clocks generated outside the soc */
 static struct samsung_fixed_rate_clock
@@ -450,7 +458,7 @@ static const struct samsung_fixed_factor_clock
 static const struct samsung_fixed_factor_clock
 		exynos5800_fixed_factor_clks[] __initconst = {
 	FFACTOR(0, "ff_dout_epll2", "mout_sclk_epll", 1, 2, 0),
-	FFACTOR(0, "ff_dout_spll2", "mout_sclk_spll", 1, 2, 0),
+	FFACTOR(CLK_FF_DOUT_SPLL2, "ff_dout_spll2", "mout_sclk_spll", 1, 2, 0),
 };
 
 static const struct samsung_mux_clock exynos5800_mux_clks[] __initconst = {
@@ -472,11 +480,14 @@ static const struct samsung_mux_clock exynos5800_mux_clks[] __initconst = {
 	MUX(0, "mout_aclk300_disp1", mout_group5_5800_p, SRC_TOP2, 24, 2),
 	MUX(0, "mout_aclk300_gscl", mout_group5_5800_p, SRC_TOP2, 28, 2),
 
+	MUX(CLK_MOUT_MX_MSPLL_CCORE_PHY, "mout_mx_mspll_ccore_phy",
+		mout_mx_mspll_ccore_phy_p, SRC_TOP7, 0, 3),
+
 	MUX(CLK_MOUT_MX_MSPLL_CCORE, "mout_mx_mspll_ccore",
-			mout_mx_mspll_ccore_p, SRC_TOP7, 16, 2),
+			mout_mx_mspll_ccore_p, SRC_TOP7, 16, 3),
 	MUX_F(CLK_MOUT_MAU_EPLL, "mout_mau_epll_clk", mout_mau_epll_clk_5800_p,
 			SRC_TOP7, 20, 2, CLK_SET_RATE_PARENT, 0),
-	MUX(0, "sclk_bpll", mout_bpll_p, SRC_TOP7, 24, 1),
+	MUX(CLK_SCLK_BPLL, "sclk_bpll", mout_bpll_p, SRC_TOP7, 24, 1),
 	MUX(0, "mout_epll2", mout_epll2_5800_p, SRC_TOP7, 28, 1),
 
 	MUX(0, "mout_aclk550_cam", mout_group3_5800_p, SRC_TOP8, 16, 3),
@@ -648,7 +659,7 @@ static const struct samsung_mux_clock exynos5x_mux_clks[] __initconst = {
 
 	MUX(0, "mout_sclk_mpll", mout_mpll_p, SRC_TOP6, 0, 1),
 	MUX(CLK_MOUT_VPLL, "mout_sclk_vpll", mout_vpll_p, SRC_TOP6, 4, 1),
-	MUX(0, "mout_sclk_spll", mout_spll_p, SRC_TOP6, 8, 1),
+	MUX(CLK_MOUT_SCLK_SPLL, "mout_sclk_spll", mout_spll_p, SRC_TOP6, 8, 1),
 	MUX(0, "mout_sclk_ipll", mout_ipll_p, SRC_TOP6, 12, 1),
 	MUX(0, "mout_sclk_rpll", mout_rpll_p, SRC_TOP6, 16, 1),
 	MUX_F(CLK_MOUT_EPLL, "mout_sclk_epll", mout_epll_p, SRC_TOP6, 20, 1,
@@ -814,9 +825,13 @@ static const struct samsung_div_clock exynos5x_div_clks[] __initconst = {
 			DIV_CDREX0, 16, 3),
 	DIV(CLK_DOUT_CCLK_DREX0, "dout_cclk_drex0", "dout_clk2x_phy0",
 			DIV_CDREX0, 8, 3),
+	DIV(0, "dout_cclk_drex1", "dout_clk2x_phy0", DIV_CDREX0, 8, 3),
 	DIV(CLK_DOUT_CLK2X_PHY0, "dout_clk2x_phy0", "dout_sclk_cdrex",
 			DIV_CDREX0, 3, 5),
 
+	DIV(0, "dout_pclk_drex0", "dout_cclk_drex0", DIV_CDREX0, 28, 3),
+	DIV(0, "dout_pclk_drex1", "dout_cclk_drex1", DIV_CDREX0, 28, 3),
+
 	DIV(CLK_DOUT_PCLK_CORE_MEM, "dout_pclk_core_mem", "mout_mclk_cdrex",
 			DIV_CDREX1, 8, 3),
 
@@ -1170,6 +1185,31 @@ static const struct samsung_gate_clock exynos5x_gate_clks[] __initconst = {
 			GATE_TOP_SCLK_ISP, 12, CLK_SET_RATE_PARENT, 0),
 
 	GATE(CLK_G3D, "g3d", "mout_user_aclk_g3d", GATE_IP_G3D, 9, 0, 0),
+
+	GATE(CLK_CLKM_PHY0, "clkm_phy0", "dout_sclk_cdrex",
+			GATE_BUS_CDREX0, 0, 0, 0),
+	GATE(CLK_CLKM_PHY1, "clkm_phy1", "dout_sclk_cdrex",
+			GATE_BUS_CDREX0, 1, 0, 0),
+	GATE(0, "mx_mspll_ccore_phy", "mout_mx_mspll_ccore_phy",
+			SRC_MASK_TOP7, 0, CLK_IGNORE_UNUSED, 0),
+
+	GATE(CLK_ACLK_PPMU_DREX0_0, "aclk_ppmu_drex0_0", "dout_aclk_cdrex1",
+			GATE_BUS_CDREX1, 15, CLK_IGNORE_UNUSED, 0),
+	GATE(CLK_ACLK_PPMU_DREX0_1, "aclk_ppmu_drex0_1", "dout_aclk_cdrex1",
+			GATE_BUS_CDREX1, 14, CLK_IGNORE_UNUSED, 0),
+	GATE(CLK_ACLK_PPMU_DREX1_0, "aclk_ppmu_drex1_0", "dout_aclk_cdrex1",
+			GATE_BUS_CDREX1, 13, CLK_IGNORE_UNUSED, 0),
+	GATE(CLK_ACLK_PPMU_DREX1_1, "aclk_ppmu_drex1_1", "dout_aclk_cdrex1",
+			GATE_BUS_CDREX1, 12, CLK_IGNORE_UNUSED, 0),
+
+	GATE(CLK_PCLK_PPMU_DREX0_0, "pclk_ppmu_drex0_0", "dout_pclk_cdrex",
+			GATE_BUS_CDREX1, 29, CLK_IGNORE_UNUSED, 0),
+	GATE(CLK_PCLK_PPMU_DREX0_1, "pclk_ppmu_drex0_1", "dout_pclk_cdrex",
+			GATE_BUS_CDREX1, 28, CLK_IGNORE_UNUSED, 0),
+	GATE(CLK_PCLK_PPMU_DREX1_0, "pclk_ppmu_drex1_0", "dout_pclk_cdrex",
+			GATE_BUS_CDREX1, 27, CLK_IGNORE_UNUSED, 0),
+	GATE(CLK_PCLK_PPMU_DREX1_1, "pclk_ppmu_drex1_1", "dout_pclk_cdrex",
+			GATE_BUS_CDREX1, 26, CLK_IGNORE_UNUSED, 0),
 };
 
 static const struct samsung_div_clock exynos5x_disp_div_clks[] __initconst = {
-- 
2.7.4

Re: [PATCH v3 2/8] clk: samsung: add new clocks for DMC for Exynos5422 SoC

[Chanwoo Choi]

Hi,

When I reviewed this patch, the almost changes are wrong.
Frankly, I can't believe that you had tested and verified it
on real board. Please check my comments.
If I misunderstood, please let me know.

On 19. 1. 31. 오후 5:49, Lukasz Luba wrote:
> This patch provides support for clocks needed for Dynamic Memory Controller
> in Exynos5422 SoC. It adds CDREX base register addresses, new DIV, MUX and
> GATE entries.
> 
> CC: Sylwester Nawrocki <s.nawrocki@samsung.com>
> CC: Chanwoo Choi <cw00.choi@samsung.com>
> CC: Michael Turquette <mturquette@baylibre.com>
> CC: Stephen Boyd <sboyd@kernel.org>
> CC: Kukjin Kim <kgene@kernel.org>
> CC: Krzysztof Kozlowski <krzk@kernel.org>
> CC: linux-samsung-soc@vger.kernel.org
> CC: linux-clk@vger.kernel.org
> CC: linux-arm-kernel@lists.infradead.org
> CC: linux-kernel@vger.kernel.org
> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> ---
>  drivers/clk/samsung/clk-exynos5420.c | 48 +++++++++++++++++++++++++++++++++---
>  1 file changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
> index 34cce3c..3e87421 100644
> --- a/drivers/clk/samsung/clk-exynos5420.c
> +++ b/drivers/clk/samsung/clk-exynos5420.c
> @@ -132,6 +132,8 @@
>  #define BPLL_LOCK		0x20010
>  #define BPLL_CON0		0x20110
>  #define SRC_CDREX		0x20200
> +#define GATE_BUS_CDREX0		0x20700
> +#define GATE_BUS_CDREX1		0x20704
>  #define DIV_CDREX0		0x20500
>  #define DIV_CDREX1		0x20504
>  #define KPLL_LOCK		0x28000
> @@ -248,6 +250,8 @@ static const unsigned long exynos5x_clk_regs[] __initconst = {
>  	DIV_CDREX1,
>  	SRC_KFC,
>  	DIV_KFC0,
> +	GATE_BUS_CDREX0,
> +	GATE_BUS_CDREX1,
>  };
>  
>  static const unsigned long exynos5800_clk_regs[] __initconst = {
> @@ -425,6 +429,10 @@ PNAME(mout_group13_5800_p)	= { "dout_osc_div", "mout_sw_aclkfl1_550_cam" };
>  PNAME(mout_group14_5800_p)	= { "dout_aclk550_cam", "dout_sclk_sw" };
>  PNAME(mout_group15_5800_p)	= { "dout_osc_div", "mout_sw_aclk550_cam" };
>  PNAME(mout_group16_5800_p)	= { "dout_osc_div", "mout_mau_epll_clk" };
> +PNAME(mout_mx_mspll_ccore_phy_p) = { "sclk_bpll", "mout_dpll_ctrl",
> +					"mout_mpll_ctrl", "ff_dout_spll2",
> +					"mout_sclk_spll"};

- mout_dpll_ctrl was not defined. This patch doesn't define it.
- mout_mpll_ctrl was not defined. ditto.
- ff_dout_spll2 was only registered when SOC is EXYNOS5800.
  It meant that ff_dout_spll2 was not registered on exynos5422 board.

It is wrong patch. You would have not checked the parent clocks
except for sclk_bpll.

Also,
In the exynos5422 datasheet, MUX_MX_MSPLL_CCORE_PHY_SEL is possible
having the six parents as following:
- sclk_bpll
- sclk_dpll
- sclk_mpll
- sclk_spll2
- sclk_spll
- sclk_epll

Why do you missing last 'sclk_epll'?


> +
>  
>  /* fixed rate clocks generated outside the soc */
>  static struct samsung_fixed_rate_clock
> @@ -450,7 +458,7 @@ static const struct samsung_fixed_factor_clock
>  static const struct samsung_fixed_factor_clock
>  		exynos5800_fixed_factor_clks[] __initconst = {
>  	FFACTOR(0, "ff_dout_epll2", "mout_sclk_epll", 1, 2, 0),
> -	FFACTOR(0, "ff_dout_spll2", "mout_sclk_spll", 1, 2, 0),
> +	FFACTOR(CLK_FF_DOUT_SPLL2, "ff_dout_spll2", "mout_sclk_spll", 1, 2, 0),

It doesn't affect the Exynos5422 because exynos5800_fixed_factor_clks[]
is registered when SOC is EXYNOS5800. Exynos5422 board cannot use this clock.

>  };
>  
>  static const struct samsung_mux_clock exynos5800_mux_clks[] __initconst = {
> @@ -472,11 +480,14 @@ static const struct samsung_mux_clock exynos5800_mux_clks[] __initconst = {
>  	MUX(0, "mout_aclk300_disp1", mout_group5_5800_p, SRC_TOP2, 24, 2),
>  	MUX(0, "mout_aclk300_gscl", mout_group5_5800_p, SRC_TOP2, 28, 2),
>  
> +	MUX(CLK_MOUT_MX_MSPLL_CCORE_PHY, "mout_mx_mspll_ccore_phy",
> +		mout_mx_mspll_ccore_phy_p, SRC_TOP7, 0, 3),
> +

Why do you modify the exynos5800_mux_clks instead of exynos5420_mux_clks
or exynos5x_mux_clks? In the coverletter this patch is for Exynos5422 board.
Did you test it?

>  	MUX(CLK_MOUT_MX_MSPLL_CCORE, "mout_mx_mspll_ccore",
> -			mout_mx_mspll_ccore_p, SRC_TOP7, 16, 2),
> +			mout_mx_mspll_ccore_p, SRC_TOP7, 16, 3),

ditto.

>  	MUX_F(CLK_MOUT_MAU_EPLL, "mout_mau_epll_clk", mout_mau_epll_clk_5800_p,
>  			SRC_TOP7, 20, 2, CLK_SET_RATE_PARENT, 0),
> -	MUX(0, "sclk_bpll", mout_bpll_p, SRC_TOP7, 24, 1),
> +	MUX(CLK_SCLK_BPLL, "sclk_bpll", mout_bpll_p, SRC_TOP7, 24, 1),

ditto.

>  	MUX(0, "mout_epll2", mout_epll2_5800_p, SRC_TOP7, 28, 1),
>  
>  	MUX(0, "mout_aclk550_cam", mout_group3_5800_p, SRC_TOP8, 16, 3),
> @@ -648,7 +659,7 @@ static const struct samsung_mux_clock exynos5x_mux_clks[] __initconst = {
>  
>  	MUX(0, "mout_sclk_mpll", mout_mpll_p, SRC_TOP6, 0, 1),
>  	MUX(CLK_MOUT_VPLL, "mout_sclk_vpll", mout_vpll_p, SRC_TOP6, 4, 1),
> -	MUX(0, "mout_sclk_spll", mout_spll_p, SRC_TOP6, 8, 1),
> +	MUX(CLK_MOUT_SCLK_SPLL, "mout_sclk_spll", mout_spll_p, SRC_TOP6, 8, 1),
>  	MUX(0, "mout_sclk_ipll", mout_ipll_p, SRC_TOP6, 12, 1),
>  	MUX(0, "mout_sclk_rpll", mout_rpll_p, SRC_TOP6, 16, 1),
>  	MUX_F(CLK_MOUT_EPLL, "mout_sclk_epll", mout_epll_p, SRC_TOP6, 20, 1,
> @@ -814,9 +825,13 @@ static const struct samsung_div_clock exynos5x_div_clks[] __initconst = {
>  			DIV_CDREX0, 16, 3),
>  	DIV(CLK_DOUT_CCLK_DREX0, "dout_cclk_drex0", "dout_clk2x_phy0",
>  			DIV_CDREX0, 8, 3),
> +	DIV(0, "dout_cclk_drex1", "dout_clk2x_phy0", DIV_CDREX0, 8, 3),

Hmm. CLK_DIV_CDREX0[10:8] of DIV_CDREX0 register was already implemented
by CLK_DOUT_CCLK_DREX0. It is fault.

Also, PCLK_CORE_MEM_RATIO[10:8] of DIV_CDREX1 register was defined as following
in clock-exynos5420.c.
- DIV(CLK_DOUT_PCLK_CORE_MEM, "dout_pclk_core_mem", "mout_mclk_cdrex", DIV_CDREX1, 8, 3), 


>  	DIV(CLK_DOUT_CLK2X_PHY0, "dout_clk2x_phy0", "dout_sclk_cdrex",
>  			DIV_CDREX0, 3, 5),
>  
> +	DIV(0, "dout_pclk_drex0", "dout_cclk_drex0", DIV_CDREX0, 28, 3),
> +	DIV(0, "dout_pclk_drex1", "dout_cclk_drex1", DIV_CDREX0, 28, 3),

dout_cclk_drex1 is wrong. It is fault.

> +
>  	DIV(CLK_DOUT_PCLK_CORE_MEM, "dout_pclk_core_mem", "mout_mclk_cdrex",
>  			DIV_CDREX1, 8, 3),
>  
> @@ -1170,6 +1185,31 @@ static const struct samsung_gate_clock exynos5x_gate_clks[] __initconst = {
>  			GATE_TOP_SCLK_ISP, 12, CLK_SET_RATE_PARENT, 0),
>  
>  	GATE(CLK_G3D, "g3d", "mout_user_aclk_g3d", GATE_IP_G3D, 9, 0, 0),
> +
> +	GATE(CLK_CLKM_PHY0, "clkm_phy0", "dout_sclk_cdrex",
> +			GATE_BUS_CDREX0, 0, 0, 0),
> +	GATE(CLK_CLKM_PHY1, "clkm_phy1", "dout_sclk_cdrex",
> +			GATE_BUS_CDREX0, 1, 0, 0),
> +	GATE(0, "mx_mspll_ccore_phy", "mout_mx_mspll_ccore_phy",
> +			SRC_MASK_TOP7, 0, CLK_IGNORE_UNUSED, 0),
> +
> +	GATE(CLK_ACLK_PPMU_DREX0_0, "aclk_ppmu_drex0_0", "dout_aclk_cdrex1",
> +			GATE_BUS_CDREX1, 15, CLK_IGNORE_UNUSED, 0),
> +	GATE(CLK_ACLK_PPMU_DREX0_1, "aclk_ppmu_drex0_1", "dout_aclk_cdrex1",
> +			GATE_BUS_CDREX1, 14, CLK_IGNORE_UNUSED, 0),
> +	GATE(CLK_ACLK_PPMU_DREX1_0, "aclk_ppmu_drex1_0", "dout_aclk_cdrex1",
> +			GATE_BUS_CDREX1, 13, CLK_IGNORE_UNUSED, 0),
> +	GATE(CLK_ACLK_PPMU_DREX1_1, "aclk_ppmu_drex1_1", "dout_aclk_cdrex1",
> +			GATE_BUS_CDREX1, 12, CLK_IGNORE_UNUSED, 0),
> +
> +	GATE(CLK_PCLK_PPMU_DREX0_0, "pclk_ppmu_drex0_0", "dout_pclk_cdrex",
> +			GATE_BUS_CDREX1, 29, CLK_IGNORE_UNUSED, 0),
> +	GATE(CLK_PCLK_PPMU_DREX0_1, "pclk_ppmu_drex0_1", "dout_pclk_cdrex",
> +			GATE_BUS_CDREX1, 28, CLK_IGNORE_UNUSED, 0),
> +	GATE(CLK_PCLK_PPMU_DREX1_0, "pclk_ppmu_drex1_0", "dout_pclk_cdrex",
> +			GATE_BUS_CDREX1, 27, CLK_IGNORE_UNUSED, 0),
> +	GATE(CLK_PCLK_PPMU_DREX1_1, "pclk_ppmu_drex1_1", "dout_pclk_cdrex",
> +			GATE_BUS_CDREX1, 26, CLK_IGNORE_UNUSED, 0),
>  };
>  
>  static const struct samsung_div_clock exynos5x_disp_div_clks[] __initconst = {
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v3 2/8] clk: samsung: add new clocks for DMC for Exynos5422 SoC

[Chanwoo Choi]

Hi,

There are some wrong comments by me. Sorry for confusion.

On 19. 2. 1. 오후 5:07, Chanwoo Choi wrote:
> Hi,
> 
> When I reviewed this patch, the almost changes are wrong.
> Frankly, I can't believe that you had tested and verified it
> on real board. Please check my comments.
> If I misunderstood, please let me know.
> 
> On 19. 1. 31. 오후 5:49, Lukasz Luba wrote:
>> This patch provides support for clocks needed for Dynamic Memory Controller
>> in Exynos5422 SoC. It adds CDREX base register addresses, new DIV, MUX and
>> GATE entries.
>>
>> CC: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> CC: Chanwoo Choi <cw00.choi@samsung.com>
>> CC: Michael Turquette <mturquette@baylibre.com>
>> CC: Stephen Boyd <sboyd@kernel.org>
>> CC: Kukjin Kim <kgene@kernel.org>
>> CC: Krzysztof Kozlowski <krzk@kernel.org>
>> CC: linux-samsung-soc@vger.kernel.org
>> CC: linux-clk@vger.kernel.org
>> CC: linux-arm-kernel@lists.infradead.org
>> CC: linux-kernel@vger.kernel.org
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>>  drivers/clk/samsung/clk-exynos5420.c | 48 +++++++++++++++++++++++++++++++++---
>>  1 file changed, 44 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
>> index 34cce3c..3e87421 100644
>> --- a/drivers/clk/samsung/clk-exynos5420.c
>> +++ b/drivers/clk/samsung/clk-exynos5420.c
>> @@ -132,6 +132,8 @@
>>  #define BPLL_LOCK		0x20010
>>  #define BPLL_CON0		0x20110
>>  #define SRC_CDREX		0x20200
>> +#define GATE_BUS_CDREX0		0x20700
>> +#define GATE_BUS_CDREX1		0x20704
>>  #define DIV_CDREX0		0x20500
>>  #define DIV_CDREX1		0x20504
>>  #define KPLL_LOCK		0x28000
>> @@ -248,6 +250,8 @@ static const unsigned long exynos5x_clk_regs[] __initconst = {
>>  	DIV_CDREX1,
>>  	SRC_KFC,
>>  	DIV_KFC0,
>> +	GATE_BUS_CDREX0,
>> +	GATE_BUS_CDREX1,
>>  };
>>  
>>  static const unsigned long exynos5800_clk_regs[] __initconst = {
>> @@ -425,6 +429,10 @@ PNAME(mout_group13_5800_p)	= { "dout_osc_div", "mout_sw_aclkfl1_550_cam" };
>>  PNAME(mout_group14_5800_p)	= { "dout_aclk550_cam", "dout_sclk_sw" };
>>  PNAME(mout_group15_5800_p)	= { "dout_osc_div", "mout_sw_aclk550_cam" };
>>  PNAME(mout_group16_5800_p)	= { "dout_osc_div", "mout_mau_epll_clk" };
>> +PNAME(mout_mx_mspll_ccore_phy_p) = { "sclk_bpll", "mout_dpll_ctrl",
>> +					"mout_mpll_ctrl", "ff_dout_spll2",
>> +					"mout_sclk_spll"};
> 
> - mout_dpll_ctrl was not defined. This patch doesn't define it.
> - mout_mpll_ctrl was not defined. ditto.
> - ff_dout_spll2 was only registered when SOC is EXYNOS5800.
>   It meant that ff_dout_spll2 was not registered on exynos5422 board.
> 
> It is wrong patch. You would have not checked the parent clocks
> except for sclk_bpll.
> 
> Also,
> In the exynos5422 datasheet, MUX_MX_MSPLL_CCORE_PHY_SEL is possible
> having the six parents as following:
> - sclk_bpll
> - sclk_dpll
> - sclk_mpll
> - sclk_spll2
> - sclk_spll
> - sclk_epll
> 
> Why do you missing last 'sclk_epll'?
> 
> 
>> +
>>  
>>  /* fixed rate clocks generated outside the soc */
>>  static struct samsung_fixed_rate_clock
>> @@ -450,7 +458,7 @@ static const struct samsung_fixed_factor_clock
>>  static const struct samsung_fixed_factor_clock
>>  		exynos5800_fixed_factor_clks[] __initconst = {
>>  	FFACTOR(0, "ff_dout_epll2", "mout_sclk_epll", 1, 2, 0),
>> -	FFACTOR(0, "ff_dout_spll2", "mout_sclk_spll", 1, 2, 0),
>> +	FFACTOR(CLK_FF_DOUT_SPLL2, "ff_dout_spll2", "mout_sclk_spll", 1, 2, 0),
> 
> It doesn't affect the Exynos5422 because exynos5800_fixed_factor_clks[]
> is registered when SOC is EXYNOS5800. Exynos5422 board cannot use this clock.

It is my fault. Please ignore this comment.

> 
>>  };
>>  
>>  static const struct samsung_mux_clock exynos5800_mux_clks[] __initconst = {
>> @@ -472,11 +480,14 @@ static const struct samsung_mux_clock exynos5800_mux_clks[] __initconst = {
>>  	MUX(0, "mout_aclk300_disp1", mout_group5_5800_p, SRC_TOP2, 24, 2),
>>  	MUX(0, "mout_aclk300_gscl", mout_group5_5800_p, SRC_TOP2, 28, 2),
>>  
>> +	MUX(CLK_MOUT_MX_MSPLL_CCORE_PHY, "mout_mx_mspll_ccore_phy",
>> +		mout_mx_mspll_ccore_phy_p, SRC_TOP7, 0, 3),
>> +
> 
> Why do you modify the exynos5800_mux_clks instead of exynos5420_mux_clks
> or exynos5x_mux_clks? In the coverletter this patch is for Exynos5422 board.
> Did you test it?

It is my fault. Please ignore this comment.

> 
>>  	MUX(CLK_MOUT_MX_MSPLL_CCORE, "mout_mx_mspll_ccore",
>> -			mout_mx_mspll_ccore_p, SRC_TOP7, 16, 2),
>> +			mout_mx_mspll_ccore_p, SRC_TOP7, 16, 3),
> 
> ditto.

It is my fault. Please ignore this comment.

> 
>>  	MUX_F(CLK_MOUT_MAU_EPLL, "mout_mau_epll_clk", mout_mau_epll_clk_5800_p,
>>  			SRC_TOP7, 20, 2, CLK_SET_RATE_PARENT, 0),
>> -	MUX(0, "sclk_bpll", mout_bpll_p, SRC_TOP7, 24, 1),
>> +	MUX(CLK_SCLK_BPLL, "sclk_bpll", mout_bpll_p, SRC_TOP7, 24, 1),
> 
> ditto.

It is my fault. Please ignore this comment.

> 
>>  	MUX(0, "mout_epll2", mout_epll2_5800_p, SRC_TOP7, 28, 1),
>>  
>>  	MUX(0, "mout_aclk550_cam", mout_group3_5800_p, SRC_TOP8, 16, 3),
>> @@ -648,7 +659,7 @@ static const struct samsung_mux_clock exynos5x_mux_clks[] __initconst = {
>>  
>>  	MUX(0, "mout_sclk_mpll", mout_mpll_p, SRC_TOP6, 0, 1),
>>  	MUX(CLK_MOUT_VPLL, "mout_sclk_vpll", mout_vpll_p, SRC_TOP6, 4, 1),
>> -	MUX(0, "mout_sclk_spll", mout_spll_p, SRC_TOP6, 8, 1),
>> +	MUX(CLK_MOUT_SCLK_SPLL, "mout_sclk_spll", mout_spll_p, SRC_TOP6, 8, 1),
>>  	MUX(0, "mout_sclk_ipll", mout_ipll_p, SRC_TOP6, 12, 1),
>>  	MUX(0, "mout_sclk_rpll", mout_rpll_p, SRC_TOP6, 16, 1),
>>  	MUX_F(CLK_MOUT_EPLL, "mout_sclk_epll", mout_epll_p, SRC_TOP6, 20, 1,
>> @@ -814,9 +825,13 @@ static const struct samsung_div_clock exynos5x_div_clks[] __initconst = {
>>  			DIV_CDREX0, 16, 3),
>>  	DIV(CLK_DOUT_CCLK_DREX0, "dout_cclk_drex0", "dout_clk2x_phy0",
>>  			DIV_CDREX0, 8, 3),
>> +	DIV(0, "dout_cclk_drex1", "dout_clk2x_phy0", DIV_CDREX0, 8, 3),
> 
> Hmm. CLK_DIV_CDREX0[10:8] of DIV_CDREX0 register was already implemented
> by CLK_DOUT_CCLK_DREX0. It is fault.
> 
> Also, PCLK_CORE_MEM_RATIO[10:8] of DIV_CDREX1 register was defined as following
> in clock-exynos5420.c.
> - DIV(CLK_DOUT_PCLK_CORE_MEM, "dout_pclk_core_mem", "mout_mclk_cdrex", DIV_CDREX1, 8, 3), 
> 
> 
>>  	DIV(CLK_DOUT_CLK2X_PHY0, "dout_clk2x_phy0", "dout_sclk_cdrex",
>>  			DIV_CDREX0, 3, 5),
>>  
>> +	DIV(0, "dout_pclk_drex0", "dout_cclk_drex0", DIV_CDREX0, 28, 3),
>> +	DIV(0, "dout_pclk_drex1", "dout_cclk_drex1", DIV_CDREX0, 28, 3),
> 
> dout_cclk_drex1 is wrong. It is fault.
> 
>> +
>>  	DIV(CLK_DOUT_PCLK_CORE_MEM, "dout_pclk_core_mem", "mout_mclk_cdrex",
>>  			DIV_CDREX1, 8, 3),
>>  
>> @@ -1170,6 +1185,31 @@ static const struct samsung_gate_clock exynos5x_gate_clks[] __initconst = {
>>  			GATE_TOP_SCLK_ISP, 12, CLK_SET_RATE_PARENT, 0),
>>  
>>  	GATE(CLK_G3D, "g3d", "mout_user_aclk_g3d", GATE_IP_G3D, 9, 0, 0),
>> +
>> +	GATE(CLK_CLKM_PHY0, "clkm_phy0", "dout_sclk_cdrex",
>> +			GATE_BUS_CDREX0, 0, 0, 0),
>> +	GATE(CLK_CLKM_PHY1, "clkm_phy1", "dout_sclk_cdrex",
>> +			GATE_BUS_CDREX0, 1, 0, 0),
>> +	GATE(0, "mx_mspll_ccore_phy", "mout_mx_mspll_ccore_phy",
>> +			SRC_MASK_TOP7, 0, CLK_IGNORE_UNUSED, 0),
>> +
>> +	GATE(CLK_ACLK_PPMU_DREX0_0, "aclk_ppmu_drex0_0", "dout_aclk_cdrex1",
>> +			GATE_BUS_CDREX1, 15, CLK_IGNORE_UNUSED, 0),
>> +	GATE(CLK_ACLK_PPMU_DREX0_1, "aclk_ppmu_drex0_1", "dout_aclk_cdrex1",
>> +			GATE_BUS_CDREX1, 14, CLK_IGNORE_UNUSED, 0),
>> +	GATE(CLK_ACLK_PPMU_DREX1_0, "aclk_ppmu_drex1_0", "dout_aclk_cdrex1",
>> +			GATE_BUS_CDREX1, 13, CLK_IGNORE_UNUSED, 0),
>> +	GATE(CLK_ACLK_PPMU_DREX1_1, "aclk_ppmu_drex1_1", "dout_aclk_cdrex1",
>> +			GATE_BUS_CDREX1, 12, CLK_IGNORE_UNUSED, 0),
>> +
>> +	GATE(CLK_PCLK_PPMU_DREX0_0, "pclk_ppmu_drex0_0", "dout_pclk_cdrex",
>> +			GATE_BUS_CDREX1, 29, CLK_IGNORE_UNUSED, 0),
>> +	GATE(CLK_PCLK_PPMU_DREX0_1, "pclk_ppmu_drex0_1", "dout_pclk_cdrex",
>> +			GATE_BUS_CDREX1, 28, CLK_IGNORE_UNUSED, 0),
>> +	GATE(CLK_PCLK_PPMU_DREX1_0, "pclk_ppmu_drex1_0", "dout_pclk_cdrex",
>> +			GATE_BUS_CDREX1, 27, CLK_IGNORE_UNUSED, 0),
>> +	GATE(CLK_PCLK_PPMU_DREX1_1, "pclk_ppmu_drex1_1", "dout_pclk_cdrex",
>> +			GATE_BUS_CDREX1, 26, CLK_IGNORE_UNUSED, 0),
>>  };
>>  
>>  static const struct samsung_div_clock exynos5x_disp_div_clks[] __initconst = {
>>
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v3 2/8] clk: samsung: add new clocks for DMC for Exynos5422 SoC

[Lukasz Luba]

Hi Chanwoo,

On 2/1/19 10:20 AM, Chanwoo Choi wrote:
> Hi,
> 
> There are some wrong comments by me. Sorry for confusion.
No problem.
> 
> On 19. 2. 1. 오후 5:07, Chanwoo Choi wrote:
>> Hi,
>>
>> When I reviewed this patch, the almost changes are wrong.
>> Frankly, I can't believe that you had tested and verified it
>> on real board. Please check my comments.
>> If I misunderstood, please let me know.
>>
>> On 19. 1. 31. 오후 5:49, Lukasz Luba wrote:
>>> This patch provides support for clocks needed for Dynamic Memory Controller
>>> in Exynos5422 SoC. It adds CDREX base register addresses, new DIV, MUX and
>>> GATE entries.
>>>
>>> CC: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>> CC: Chanwoo Choi <cw00.choi@samsung.com>
>>> CC: Michael Turquette <mturquette@baylibre.com>
>>> CC: Stephen Boyd <sboyd@kernel.org>
>>> CC: Kukjin Kim <kgene@kernel.org>
>>> CC: Krzysztof Kozlowski <krzk@kernel.org>
>>> CC: linux-samsung-soc@vger.kernel.org
>>> CC: linux-clk@vger.kernel.org
>>> CC: linux-arm-kernel@lists.infradead.org
>>> CC: linux-kernel@vger.kernel.org
>>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>>> ---
>>>   drivers/clk/samsung/clk-exynos5420.c | 48 +++++++++++++++++++++++++++++++++---
>>>   1 file changed, 44 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
>>> index 34cce3c..3e87421 100644
>>> --- a/drivers/clk/samsung/clk-exynos5420.c
>>> +++ b/drivers/clk/samsung/clk-exynos5420.c
>>> @@ -132,6 +132,8 @@
>>>   #define BPLL_LOCK		0x20010
>>>   #define BPLL_CON0		0x20110
>>>   #define SRC_CDREX		0x20200
>>> +#define GATE_BUS_CDREX0		0x20700
>>> +#define GATE_BUS_CDREX1		0x20704
>>>   #define DIV_CDREX0		0x20500
>>>   #define DIV_CDREX1		0x20504
>>>   #define KPLL_LOCK		0x28000
>>> @@ -248,6 +250,8 @@ static const unsigned long exynos5x_clk_regs[] __initconst = {
>>>   	DIV_CDREX1,
>>>   	SRC_KFC,
>>>   	DIV_KFC0,
>>> +	GATE_BUS_CDREX0,
>>> +	GATE_BUS_CDREX1,
>>>   };
>>>   
>>>   static const unsigned long exynos5800_clk_regs[] __initconst = {
>>> @@ -425,6 +429,10 @@ PNAME(mout_group13_5800_p)	= { "dout_osc_div", "mout_sw_aclkfl1_550_cam" };
>>>   PNAME(mout_group14_5800_p)	= { "dout_aclk550_cam", "dout_sclk_sw" };
>>>   PNAME(mout_group15_5800_p)	= { "dout_osc_div", "mout_sw_aclk550_cam" };
>>>   PNAME(mout_group16_5800_p)	= { "dout_osc_div", "mout_mau_epll_clk" };
>>> +PNAME(mout_mx_mspll_ccore_phy_p) = { "sclk_bpll", "mout_dpll_ctrl",
>>> +					"mout_mpll_ctrl", "ff_dout_spll2",
>>> +					"mout_sclk_spll"};
>>
>> - mout_dpll_ctrl was not defined. This patch doesn't define it.
>> - mout_mpll_ctrl was not defined. ditto.
OK, I will add them.
>> - ff_dout_spll2 was only registered when SOC is EXYNOS5800.
>>    It meant that ff_dout_spll2 was not registered on exynos5422 board.
It is registered for Exynos5422:
     fout_spll                         1        1        0   800000000 
        0     0  50000
        mout_sclk_spll                 4        4        0   800000000 
        0     0  50000
           ff_dout_spll2               2        2        0   400000000 
        0     0  50000
              mout_mx_mspll_ccore       1        1        0   400000000 
         0     0  50000

>>
>> It is wrong patch. You would have not checked the parent clocks
>> except for sclk_bpll.
>>
>> Also,
>> In the exynos5422 datasheet, MUX_MX_MSPLL_CCORE_PHY_SEL is possible
>> having the six parents as following:
>> - sclk_bpll
>> - sclk_dpll
>> - sclk_mpll
>> - sclk_spll2
>> - sclk_spll
>> - sclk_epll
>>
>> Why do you missing last 'sclk_epll'?
I will add that clock to the list of possible sources.
>>
>>
>>> +
>>>   
>>>   /* fixed rate clocks generated outside the soc */
>>>   static struct samsung_fixed_rate_clock
>>> @@ -450,7 +458,7 @@ static const struct samsung_fixed_factor_clock
>>>   static const struct samsung_fixed_factor_clock
>>>   		exynos5800_fixed_factor_clks[] __initconst = {
>>>   	FFACTOR(0, "ff_dout_epll2", "mout_sclk_epll", 1, 2, 0),
>>> -	FFACTOR(0, "ff_dout_spll2", "mout_sclk_spll", 1, 2, 0),
>>> +	FFACTOR(CLK_FF_DOUT_SPLL2, "ff_dout_spll2", "mout_sclk_spll", 1, 2, 0),
>>
>> It doesn't affect the Exynos5422 because exynos5800_fixed_factor_clks[]
>> is registered when SOC is EXYNOS5800. Exynos5422 board cannot use this clock.
> 
> It is my fault. Please ignore this comment.
> 
>>
>>>   };
>>>   
>>>   static const struct samsung_mux_clock exynos5800_mux_clks[] __initconst = {
>>> @@ -472,11 +480,14 @@ static const struct samsung_mux_clock exynos5800_mux_clks[] __initconst = {
>>>   	MUX(0, "mout_aclk300_disp1", mout_group5_5800_p, SRC_TOP2, 24, 2),
>>>   	MUX(0, "mout_aclk300_gscl", mout_group5_5800_p, SRC_TOP2, 28, 2),
>>>   
>>> +	MUX(CLK_MOUT_MX_MSPLL_CCORE_PHY, "mout_mx_mspll_ccore_phy",
>>> +		mout_mx_mspll_ccore_phy_p, SRC_TOP7, 0, 3),
>>> +
>>
>> Why do you modify the exynos5800_mux_clks instead of exynos5420_mux_clks
>> or exynos5x_mux_clks? In the coverletter this patch is for Exynos5422 board.
>> Did you test it?
> 
> It is my fault. Please ignore this comment.
> 
>>
>>>   	MUX(CLK_MOUT_MX_MSPLL_CCORE, "mout_mx_mspll_ccore",
>>> -			mout_mx_mspll_ccore_p, SRC_TOP7, 16, 2),
>>> +			mout_mx_mspll_ccore_p, SRC_TOP7, 16, 3),
>>
>> ditto.
> 
> It is my fault. Please ignore this comment.
> 
>>
>>>   	MUX_F(CLK_MOUT_MAU_EPLL, "mout_mau_epll_clk", mout_mau_epll_clk_5800_p,
>>>   			SRC_TOP7, 20, 2, CLK_SET_RATE_PARENT, 0),
>>> -	MUX(0, "sclk_bpll", mout_bpll_p, SRC_TOP7, 24, 1),
>>> +	MUX(CLK_SCLK_BPLL, "sclk_bpll", mout_bpll_p, SRC_TOP7, 24, 1),
>>
>> ditto.
> 
> It is my fault. Please ignore this comment.
> 
>>
>>>   	MUX(0, "mout_epll2", mout_epll2_5800_p, SRC_TOP7, 28, 1),
>>>   
>>>   	MUX(0, "mout_aclk550_cam", mout_group3_5800_p, SRC_TOP8, 16, 3),
>>> @@ -648,7 +659,7 @@ static const struct samsung_mux_clock exynos5x_mux_clks[] __initconst = {
>>>   
>>>   	MUX(0, "mout_sclk_mpll", mout_mpll_p, SRC_TOP6, 0, 1),
>>>   	MUX(CLK_MOUT_VPLL, "mout_sclk_vpll", mout_vpll_p, SRC_TOP6, 4, 1),
>>> -	MUX(0, "mout_sclk_spll", mout_spll_p, SRC_TOP6, 8, 1),
>>> +	MUX(CLK_MOUT_SCLK_SPLL, "mout_sclk_spll", mout_spll_p, SRC_TOP6, 8, 1),
>>>   	MUX(0, "mout_sclk_ipll", mout_ipll_p, SRC_TOP6, 12, 1),
>>>   	MUX(0, "mout_sclk_rpll", mout_rpll_p, SRC_TOP6, 16, 1),
>>>   	MUX_F(CLK_MOUT_EPLL, "mout_sclk_epll", mout_epll_p, SRC_TOP6, 20, 1,
>>> @@ -814,9 +825,13 @@ static const struct samsung_div_clock exynos5x_div_clks[] __initconst = {
>>>   			DIV_CDREX0, 16, 3),
>>>   	DIV(CLK_DOUT_CCLK_DREX0, "dout_cclk_drex0", "dout_clk2x_phy0",
>>>   			DIV_CDREX0, 8, 3),
>>> +	DIV(0, "dout_cclk_drex1", "dout_clk2x_phy0", DIV_CDREX0, 8, 3),
>>
>> Hmm. CLK_DIV_CDREX0[10:8] of DIV_CDREX0 register was already implemented
>> by CLK_DOUT_CCLK_DREX0. It is fault.
Please check my comment bellow.
>>
>> Also, PCLK_CORE_MEM_RATIO[10:8] of DIV_CDREX1 register was defined as following
>> in clock-exynos5420.c.
>> - DIV(CLK_DOUT_PCLK_CORE_MEM, "dout_pclk_core_mem", "mout_mclk_cdrex", DIV_CDREX1, 8, 3),
>>
>>
>>>   	DIV(CLK_DOUT_CLK2X_PHY0, "dout_clk2x_phy0", "dout_sclk_cdrex",
>>>   			DIV_CDREX0, 3, 5),
>>>   
>>> +	DIV(0, "dout_pclk_drex0", "dout_cclk_drex0", DIV_CDREX0, 28, 3),
>>> +	DIV(0, "dout_pclk_drex1", "dout_cclk_drex1", DIV_CDREX0, 28, 3),
>>
>> dout_cclk_drex1 is wrong. It is fault.
So, your suggestion is to not define the clocks:
dout_cclk_drex1, out_pclk_drex1
because they use the same register bits as:
dout_cclk_drex0, dout_pclk_drex0

I have added them to have the information in clk_summary which now shows 
the full topology:
     fout_bpll                         3        3        0   825000000 
        0     0  50000
        mout_bpll                      3        3        0   825000000 
        0     0  50000
           mout_mclk_cdrex             1        1        0   825000000 
        0     0  50000
              dout_pclk_core_mem       0        0        0   206250000 
        0     0  50000
              dout_sclk_cdrex          1        1        0   825000000 
        0     0  50000
                 clkm_phy1             0        0        0   825000000 
        0     0  50000
                 clkm_phy0             0        0        0   825000000 
        0     0  50000
                 dout_clk2x_phy0       1        1        0   825000000 
        0     0  50000
                    dout_aclk_cdrex1       1        1        0 
412500000          0     0  50000
                       aclk_ppmu_drex1_1       0        0        0 
412500000          0     0  50000
                       aclk_ppmu_drex1_0       0        0        0 
412500000          0     0  50000
                       aclk_ppmu_drex0_1       0        0        0 
412500000          0     0  50000
                       aclk_ppmu_drex0_0       0        0        0 
412500000          0     0  50000
                       dout_pclk_cdrex       3        3        0 
103125000          0     0  50000
                          pclk_ppmu_drex1_1       1        1        0 
103125000          0     0  50000
                          pclk_ppmu_drex1_0       1        1        0 
103125000          0     0  50000
                          pclk_ppmu_drex0_1       0        0        0 
103125000          0     0  50000
                          pclk_ppmu_drex0_0       1        1        0 
103125000          0     0  50000
                    dout_cclk_drex0       0        0        0 
412500000          0     0  50000
                       dout_pclk_drex0       0        0        0 
103125000          0     0  50000
                    dout_cclk_drex1       0        0        0 
412500000          0     0  50000
                       dout_pclk_drex1       0        0        0 
103125000          0     0  50000

The output is comprehensive, it also shows the speed of the
performance counters used by simpleondemand_governor in devfreq.
I was trying to keep it align with the documentation.

When I remove dout_cclk_drex1 and out_pclk_drex1, it will be working,
only the clk information would not show the full topology as is in the
documentation.
Do you prefer to remove these two clocks?

Thank you the review.

Regards,
Lukasz Luba

>>
>>> +
>>>   	DIV(CLK_DOUT_PCLK_CORE_MEM, "dout_pclk_core_mem", "mout_mclk_cdrex",
>>>   			DIV_CDREX1, 8, 3),
>>>   
>>> @@ -1170,6 +1185,31 @@ static const struct samsung_gate_clock exynos5x_gate_clks[] __initconst = {
>>>   			GATE_TOP_SCLK_ISP, 12, CLK_SET_RATE_PARENT, 0),
>>>   
>>>   	GATE(CLK_G3D, "g3d", "mout_user_aclk_g3d", GATE_IP_G3D, 9, 0, 0),
>>> +
>>> +	GATE(CLK_CLKM_PHY0, "clkm_phy0", "dout_sclk_cdrex",
>>> +			GATE_BUS_CDREX0, 0, 0, 0),
>>> +	GATE(CLK_CLKM_PHY1, "clkm_phy1", "dout_sclk_cdrex",
>>> +			GATE_BUS_CDREX0, 1, 0, 0),
>>> +	GATE(0, "mx_mspll_ccore_phy", "mout_mx_mspll_ccore_phy",
>>> +			SRC_MASK_TOP7, 0, CLK_IGNORE_UNUSED, 0),
>>> +
>>> +	GATE(CLK_ACLK_PPMU_DREX0_0, "aclk_ppmu_drex0_0", "dout_aclk_cdrex1",
>>> +			GATE_BUS_CDREX1, 15, CLK_IGNORE_UNUSED, 0),
>>> +	GATE(CLK_ACLK_PPMU_DREX0_1, "aclk_ppmu_drex0_1", "dout_aclk_cdrex1",
>>> +			GATE_BUS_CDREX1, 14, CLK_IGNORE_UNUSED, 0),
>>> +	GATE(CLK_ACLK_PPMU_DREX1_0, "aclk_ppmu_drex1_0", "dout_aclk_cdrex1",
>>> +			GATE_BUS_CDREX1, 13, CLK_IGNORE_UNUSED, 0),
>>> +	GATE(CLK_ACLK_PPMU_DREX1_1, "aclk_ppmu_drex1_1", "dout_aclk_cdrex1",
>>> +			GATE_BUS_CDREX1, 12, CLK_IGNORE_UNUSED, 0),
>>> +
>>> +	GATE(CLK_PCLK_PPMU_DREX0_0, "pclk_ppmu_drex0_0", "dout_pclk_cdrex",
>>> +			GATE_BUS_CDREX1, 29, CLK_IGNORE_UNUSED, 0),
>>> +	GATE(CLK_PCLK_PPMU_DREX0_1, "pclk_ppmu_drex0_1", "dout_pclk_cdrex",
>>> +			GATE_BUS_CDREX1, 28, CLK_IGNORE_UNUSED, 0),
>>> +	GATE(CLK_PCLK_PPMU_DREX1_0, "pclk_ppmu_drex1_0", "dout_pclk_cdrex",
>>> +			GATE_BUS_CDREX1, 27, CLK_IGNORE_UNUSED, 0),
>>> +	GATE(CLK_PCLK_PPMU_DREX1_1, "pclk_ppmu_drex1_1", "dout_pclk_cdrex",
>>> +			GATE_BUS_CDREX1, 26, CLK_IGNORE_UNUSED, 0),
>>>   };
>>>   
>>>   static const struct samsung_div_clock exynos5x_disp_div_clks[] __initconst = {
>>>
>>
>>
> 
> 

[PATCH v3 3/8] clk: samsung: add BPLL rate table for Exynos 5422 SoC

[Lukasz Luba]

Add new table rate for BPLL for Exynos5422 SoC supporting Dynamic Memory
Controller frequencies for driver's DRAM timings.

CC: Sylwester Nawrocki <s.nawrocki@samsung.com>
CC: Chanwoo Choi <cw00.choi@samsung.com>
CC: Michael Turquette <mturquette@baylibre.com>
CC: Stephen Boyd <sboyd@kernel.org>
CC: Kukjin Kim <kgene@kernel.org>
CC: Krzysztof Kozlowski <krzk@kernel.org>
CC: linux-samsung-soc@vger.kernel.org
CC: linux-clk@vger.kernel.org
CC: linux-arm-kernel@lists.infradead.org
CC: linux-kernel@vger.kernel.org
Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
---
 drivers/clk/samsung/clk-exynos5420.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
index 3e87421..8bf9579 100644
--- a/drivers/clk/samsung/clk-exynos5420.c
+++ b/drivers/clk/samsung/clk-exynos5420.c
@@ -1325,6 +1325,19 @@ static const struct samsung_pll_rate_table exynos5420_pll2550x_24mhz_tbl[] __ini
 	PLL_35XX_RATE(24 * MHZ, 200000000,  200, 3, 3),
 };
 
+static const struct samsung_pll_rate_table exynos5422_bpll_rate_table[] = {
+	PLL_35XX_RATE(24 * MHZ, 933000000, 311, 4, 1),
+	PLL_35XX_RATE(24 * MHZ, 825000000, 275, 4, 1),
+	PLL_35XX_RATE(24 * MHZ, 728000000, 182, 3, 1),
+	PLL_35XX_RATE(24 * MHZ, 633000000, 211, 4, 1),
+	PLL_35XX_RATE(24 * MHZ, 543000000, 181, 2, 2),
+	PLL_35XX_RATE(24 * MHZ, 413000000, 413, 6, 2),
+	PLL_35XX_RATE(24 * MHZ, 275000000, 275, 3, 3),
+	PLL_35XX_RATE(24 * MHZ, 206000000, 206, 3, 3),
+	PLL_35XX_RATE(24 * MHZ, 165000000, 110, 2, 3),
+	PLL_35XX_RATE(24 * MHZ, 138000000, 184, 2, 4),
+};
+
 static const struct samsung_pll_rate_table exynos5420_epll_24mhz_tbl[] = {
 	PLL_36XX_RATE(24 * MHZ, 600000000U, 100, 2, 1, 0),
 	PLL_36XX_RATE(24 * MHZ, 400000000U, 200, 3, 2, 0),
@@ -1467,7 +1480,7 @@ static void __init exynos5x_clk_init(struct device_node *np,
 		exynos5x_plls[apll].rate_table = exynos5420_pll2550x_24mhz_tbl;
 		exynos5x_plls[epll].rate_table = exynos5420_epll_24mhz_tbl;
 		exynos5x_plls[kpll].rate_table = exynos5420_pll2550x_24mhz_tbl;
-		exynos5x_plls[bpll].rate_table = exynos5420_pll2550x_24mhz_tbl;
+		exynos5x_plls[bpll].rate_table = exynos5422_bpll_rate_table;
 	}
 
 	samsung_clk_register_pll(ctx, exynos5x_plls, ARRAY_SIZE(exynos5x_plls),
-- 
2.7.4

Re: [PATCH v3 3/8] clk: samsung: add BPLL rate table for Exynos 5422 SoC

[Chanwoo Choi]

Hi,

On 19. 1. 31. 오후 5:49, Lukasz Luba wrote:
> Add new table rate for BPLL for Exynos5422 SoC supporting Dynamic Memory
> Controller frequencies for driver's DRAM timings.
> 
> CC: Sylwester Nawrocki <s.nawrocki@samsung.com>
> CC: Chanwoo Choi <cw00.choi@samsung.com>
> CC: Michael Turquette <mturquette@baylibre.com>
> CC: Stephen Boyd <sboyd@kernel.org>
> CC: Kukjin Kim <kgene@kernel.org>
> CC: Krzysztof Kozlowski <krzk@kernel.org>
> CC: linux-samsung-soc@vger.kernel.org
> CC: linux-clk@vger.kernel.org
> CC: linux-arm-kernel@lists.infradead.org
> CC: linux-kernel@vger.kernel.org
> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> ---
>  drivers/clk/samsung/clk-exynos5420.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
> index 3e87421..8bf9579 100644
> --- a/drivers/clk/samsung/clk-exynos5420.c
> +++ b/drivers/clk/samsung/clk-exynos5420.c
> @@ -1325,6 +1325,19 @@ static const struct samsung_pll_rate_table exynos5420_pll2550x_24mhz_tbl[] __ini
>  	PLL_35XX_RATE(24 * MHZ, 200000000,  200, 3, 3),
>  };
>  
> +static const struct samsung_pll_rate_table exynos5422_bpll_rate_table[] = {
> +	PLL_35XX_RATE(24 * MHZ, 933000000, 311, 4, 1),
> +	PLL_35XX_RATE(24 * MHZ, 825000000, 275, 4, 1),
> +	PLL_35XX_RATE(24 * MHZ, 728000000, 182, 3, 1),
> +	PLL_35XX_RATE(24 * MHZ, 633000000, 211, 4, 1),
> +	PLL_35XX_RATE(24 * MHZ, 543000000, 181, 2, 2),
> +	PLL_35XX_RATE(24 * MHZ, 413000000, 413, 6, 2),
> +	PLL_35XX_RATE(24 * MHZ, 275000000, 275, 3, 3),
> +	PLL_35XX_RATE(24 * MHZ, 206000000, 206, 3, 3),
> +	PLL_35XX_RATE(24 * MHZ, 165000000, 110, 2, 3),
> +	PLL_35XX_RATE(24 * MHZ, 138000000, 184, 2, 4),

Except for 825Mhz, I can't find the target frequency
on Exynos5422 TRM document. Usually, Exynos TRM specified
the supported stable clocks. It means that undefined clocks
are not stable as I knew. Where do you find them?

When I calculated the PLL frequency with PMS value, it is correct.
But, just we need to check the reference of undefined clocks on TRM
in order to guarantee the stable operation.

Remove 933/138Mhz because exynos5433-dmc.c doesn't use 933Mhz and 138Mhz
and also Exynos5422 TRM doesn't define 933/138Mhz on pll table.

> +};
> +
>  static const struct samsung_pll_rate_table exynos5420_epll_24mhz_tbl[] = {
>  	PLL_36XX_RATE(24 * MHZ, 600000000U, 100, 2, 1, 0),
>  	PLL_36XX_RATE(24 * MHZ, 400000000U, 200, 3, 2, 0),
> @@ -1467,7 +1480,7 @@ static void __init exynos5x_clk_init(struct device_node *np,
>  		exynos5x_plls[apll].rate_table = exynos5420_pll2550x_24mhz_tbl;
>  		exynos5x_plls[epll].rate_table = exynos5420_epll_24mhz_tbl;
>  		exynos5x_plls[kpll].rate_table = exynos5420_pll2550x_24mhz_tbl;
> -		exynos5x_plls[bpll].rate_table = exynos5420_pll2550x_24mhz_tbl;
> +		exynos5x_plls[bpll].rate_table = exynos5422_bpll_rate_table;

Exynos5422 used the same PLL table for apll, kpll, bpll and so on.
You don't need to make the separate pll table. Just add new entries
to exynos5420_pll2550x_24mhz_tbl table.

>  	}
>  
>  	samsung_clk_register_pll(ctx, exynos5x_plls, ARRAY_SIZE(exynos5x_plls),
> 

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v3 3/8] clk: samsung: add BPLL rate table for Exynos 5422 SoC

[Lukasz Luba]

Hi Chanwoo,

On 2/1/19 9:44 AM, Chanwoo Choi wrote:
> Hi,
> 
> On 19. 1. 31. 오후 5:49, Lukasz Luba wrote:
>> Add new table rate for BPLL for Exynos5422 SoC supporting Dynamic Memory
>> Controller frequencies for driver's DRAM timings.
>>
>> CC: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> CC: Chanwoo Choi <cw00.choi@samsung.com>
>> CC: Michael Turquette <mturquette@baylibre.com>
>> CC: Stephen Boyd <sboyd@kernel.org>
>> CC: Kukjin Kim <kgene@kernel.org>
>> CC: Krzysztof Kozlowski <krzk@kernel.org>
>> CC: linux-samsung-soc@vger.kernel.org
>> CC: linux-clk@vger.kernel.org
>> CC: linux-arm-kernel@lists.infradead.org
>> CC: linux-kernel@vger.kernel.org
>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>> ---
>>   drivers/clk/samsung/clk-exynos5420.c | 15 ++++++++++++++-
>>   1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
>> index 3e87421..8bf9579 100644
>> --- a/drivers/clk/samsung/clk-exynos5420.c
>> +++ b/drivers/clk/samsung/clk-exynos5420.c
>> @@ -1325,6 +1325,19 @@ static const struct samsung_pll_rate_table exynos5420_pll2550x_24mhz_tbl[] __ini
>>   	PLL_35XX_RATE(24 * MHZ, 200000000,  200, 3, 3),
>>   };
>>   
>> +static const struct samsung_pll_rate_table exynos5422_bpll_rate_table[] = {
>> +	PLL_35XX_RATE(24 * MHZ, 933000000, 311, 4, 1),
>> +	PLL_35XX_RATE(24 * MHZ, 825000000, 275, 4, 1),
>> +	PLL_35XX_RATE(24 * MHZ, 728000000, 182, 3, 1),
>> +	PLL_35XX_RATE(24 * MHZ, 633000000, 211, 4, 1),
>> +	PLL_35XX_RATE(24 * MHZ, 543000000, 181, 2, 2),
>> +	PLL_35XX_RATE(24 * MHZ, 413000000, 413, 6, 2),
>> +	PLL_35XX_RATE(24 * MHZ, 275000000, 275, 3, 3),
>> +	PLL_35XX_RATE(24 * MHZ, 206000000, 206, 3, 3),
>> +	PLL_35XX_RATE(24 * MHZ, 165000000, 110, 2, 3),
>> +	PLL_35XX_RATE(24 * MHZ, 138000000, 184, 2, 4),
> 
> Except for 825Mhz, I can't find the target frequency
> on Exynos5422 TRM document. Usually, Exynos TRM specified
> the supported stable clocks. It means that undefined clocks
> are not stable as I knew. Where do you find them?
> 
> When I calculated the PLL frequency with PMS value, it is correct.
> But, just we need to check the reference of undefined clocks on TRM
> in order to guarantee the stable operation.
They values live in vendor code for Android.
I have tested the DMC & DDR with these ratios in stress scenarios
for a few days and it was stable.

> 
> Remove 933/138Mhz because exynos5433-dmc.c doesn't use 933Mhz and 138Mhz
> and also Exynos5422 TRM doesn't define 933/138Mhz on pll table.
OK, I will remove them.
> 
>> +};
>> +
>>   static const struct samsung_pll_rate_table exynos5420_epll_24mhz_tbl[] = {
>>   	PLL_36XX_RATE(24 * MHZ, 600000000U, 100, 2, 1, 0),
>>   	PLL_36XX_RATE(24 * MHZ, 400000000U, 200, 3, 2, 0),
>> @@ -1467,7 +1480,7 @@ static void __init exynos5x_clk_init(struct device_node *np,
>>   		exynos5x_plls[apll].rate_table = exynos5420_pll2550x_24mhz_tbl;
>>   		exynos5x_plls[epll].rate_table = exynos5420_epll_24mhz_tbl;
>>   		exynos5x_plls[kpll].rate_table = exynos5420_pll2550x_24mhz_tbl;
>> -		exynos5x_plls[bpll].rate_table = exynos5420_pll2550x_24mhz_tbl;
>> +		exynos5x_plls[bpll].rate_table = exynos5422_bpll_rate_table;
> 
> Exynos5422 used the same PLL table for apll, kpll, bpll and so on.
> You don't need to make the separate pll table. Just add new entries
> to exynos5420_pll2550x_24mhz_tbl table.
OK, I will extend the exynos5420_pll2550x_24mhz_tbl table.

In v4 patch set, it will be fixed.

Regards,
Lukasz
> 
>>   	}
>>   
>>   	samsung_clk_register_pll(ctx, exynos5x_plls, ARRAY_SIZE(exynos5x_plls),
>>
> 

Re: [PATCH v3 3/8] clk: samsung: add BPLL rate table for Exynos 5422 SoC

[Sylwester Nawrocki]

On 2/1/19 14:56, Lukasz Luba wrote:
>> Exynos5422 used the same PLL table for apll, kpll, bpll and so on.
>> You don't need to make the separate pll table. Just add new entries
>> to exynos5420_pll2550x_24mhz_tbl table.
> OK, I will extend the exynos5420_pll2550x_24mhz_tbl table.
> 
> In v4 patch set, it will be fixed.

I would prefer to keep the rate table separate for BPLL, until correctness
of new rates introduced in the patch and their applicability to the other PLLs
is confirmed by the hardware team and verified in tests.


-- 
Regards,
Sylwester

Re: [PATCH v3 3/8] clk: samsung: add BPLL rate table for Exynos 5422 SoC

[Lukasz Luba]

Hi Sylwester,

On 2/1/19 3:19 PM, Sylwester Nawrocki wrote:
> On 2/1/19 14:56, Lukasz Luba wrote:
>>> Exynos5422 used the same PLL table for apll, kpll, bpll and so on.
>>> You don't need to make the separate pll table. Just add new entries
>>> to exynos5420_pll2550x_24mhz_tbl table.
>> OK, I will extend the exynos5420_pll2550x_24mhz_tbl table.
>>
>> In v4 patch set, it will be fixed.
> 
> I would prefer to keep the rate table separate for BPLL, until correctness
> of new rates introduced in the patch and their applicability to the other PLLs
> is confirmed by the hardware team and verified in tests.
Good point, I share the same opinion. So, this new table for BPLL will stay.
Do you agree Chanwoo?
The BPLL is only used only by DMC.

Regards,
Lukasz
> 

Re: [PATCH v3 3/8] clk: samsung: add BPLL rate table for Exynos 5422 SoC

[Chanwoo Choi]

Hi Lukasz,

2019년 2월 1일 (금) 오후 11:22, Lukasz Luba <l.luba@partner.samsung.com>님이 작성:

>
> Hi Chanwoo,
>
> On 2/1/19 9:44 AM, Chanwoo Choi wrote:
> > Hi,
> >
> > On 19. 1. 31. 오후 5:49, Lukasz Luba wrote:
> >> Add new table rate for BPLL for Exynos5422 SoC supporting Dynamic Memory
> >> Controller frequencies for driver's DRAM timings.
> >>
> >> CC: Sylwester Nawrocki <s.nawrocki@samsung.com>
> >> CC: Chanwoo Choi <cw00.choi@samsung.com>
> >> CC: Michael Turquette <mturquette@baylibre.com>
> >> CC: Stephen Boyd <sboyd@kernel.org>
> >> CC: Kukjin Kim <kgene@kernel.org>
> >> CC: Krzysztof Kozlowski <krzk@kernel.org>
> >> CC: linux-samsung-soc@vger.kernel.org
> >> CC: linux-clk@vger.kernel.org
> >> CC: linux-arm-kernel@lists.infradead.org
> >> CC: linux-kernel@vger.kernel.org
> >> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
> >> ---
> >>   drivers/clk/samsung/clk-exynos5420.c | 15 ++++++++++++++-
> >>   1 file changed, 14 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
> >> index 3e87421..8bf9579 100644
> >> --- a/drivers/clk/samsung/clk-exynos5420.c
> >> +++ b/drivers/clk/samsung/clk-exynos5420.c
> >> @@ -1325,6 +1325,19 @@ static const struct samsung_pll_rate_table exynos5420_pll2550x_24mhz_tbl[] __ini
> >>      PLL_35XX_RATE(24 * MHZ, 200000000,  200, 3, 3),
> >>   };
> >>
> >> +static const struct samsung_pll_rate_table exynos5422_bpll_rate_table[] = {
> >> +    PLL_35XX_RATE(24 * MHZ, 933000000, 311, 4, 1),
> >> +    PLL_35XX_RATE(24 * MHZ, 825000000, 275, 4, 1),
> >> +    PLL_35XX_RATE(24 * MHZ, 728000000, 182, 3, 1),
> >> +    PLL_35XX_RATE(24 * MHZ, 633000000, 211, 4, 1),
> >> +    PLL_35XX_RATE(24 * MHZ, 543000000, 181, 2, 2),
> >> +    PLL_35XX_RATE(24 * MHZ, 413000000, 413, 6, 2),
> >> +    PLL_35XX_RATE(24 * MHZ, 275000000, 275, 3, 3),
> >> +    PLL_35XX_RATE(24 * MHZ, 206000000, 206, 3, 3),
> >> +    PLL_35XX_RATE(24 * MHZ, 165000000, 110, 2, 3),
> >> +    PLL_35XX_RATE(24 * MHZ, 138000000, 184, 2, 4),
> >
> > Except for 825Mhz, I can't find the target frequency
> > on Exynos5422 TRM document. Usually, Exynos TRM specified
> > the supported stable clocks. It means that undefined clocks
> > are not stable as I knew. Where do you find them?
> >
> > When I calculated the PLL frequency with PMS value, it is correct.
> > But, just we need to check the reference of undefined clocks on TRM
> > in order to guarantee the stable operation.
> They values live in vendor code for Android.
> I have tested the DMC & DDR with these ratios in stress scenarios
> for a few days and it was stable.

If possible, please share the url of original vendor code.

>
> >
> > Remove 933/138Mhz because exynos5433-dmc.c doesn't use 933Mhz and 138Mhz
> > and also Exynos5422 TRM doesn't define 933/138Mhz on pll table.
> OK, I will remove them.
> >
> >> +};
> >> +
> >>   static const struct samsung_pll_rate_table exynos5420_epll_24mhz_tbl[] = {
> >>      PLL_36XX_RATE(24 * MHZ, 600000000U, 100, 2, 1, 0),
> >>      PLL_36XX_RATE(24 * MHZ, 400000000U, 200, 3, 2, 0),
> >> @@ -1467,7 +1480,7 @@ static void __init exynos5x_clk_init(struct device_node *np,
> >>              exynos5x_plls[apll].rate_table = exynos5420_pll2550x_24mhz_tbl;
> >>              exynos5x_plls[epll].rate_table = exynos5420_epll_24mhz_tbl;
> >>              exynos5x_plls[kpll].rate_table = exynos5420_pll2550x_24mhz_tbl;
> >> -            exynos5x_plls[bpll].rate_table = exynos5420_pll2550x_24mhz_tbl;
> >> +            exynos5x_plls[bpll].rate_table = exynos5422_bpll_rate_table;
> >
> > Exynos5422 used the same PLL table for apll, kpll, bpll and so on.
> > You don't need to make the separate pll table. Just add new entries
> > to exynos5420_pll2550x_24mhz_tbl table.
> OK, I will extend the exynos5420_pll2550x_24mhz_tbl table.
>
> In v4 patch set, it will be fixed.
>
> Regards,
> Lukasz
> >
> >>      }
> >>
> >>      samsung_clk_register_pll(ctx, exynos5x_plls, ARRAY_SIZE(exynos5x_plls),
> >>
> >



--
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v3 3/8] clk: samsung: add BPLL rate table for Exynos 5422 SoC

[Lukasz Luba]

Hi Chanwoo,

On 2/3/19 8:54 AM, Chanwoo Choi wrote:
> Hi Lukasz,
> 
> 2019년 2월 1일 (금) 오후 11:22, Lukasz Luba <l.luba@partner.samsung.com>님이 작성:
> 
>>
>> Hi Chanwoo,
>>
>> On 2/1/19 9:44 AM, Chanwoo Choi wrote:
>>> Hi,
>>>
>>> On 19. 1. 31. 오후 5:49, Lukasz Luba wrote:
>>>> Add new table rate for BPLL for Exynos5422 SoC supporting Dynamic Memory
>>>> Controller frequencies for driver's DRAM timings.
>>>>
>>>> CC: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>>> CC: Chanwoo Choi <cw00.choi@samsung.com>
>>>> CC: Michael Turquette <mturquette@baylibre.com>
>>>> CC: Stephen Boyd <sboyd@kernel.org>
>>>> CC: Kukjin Kim <kgene@kernel.org>
>>>> CC: Krzysztof Kozlowski <krzk@kernel.org>
>>>> CC: linux-samsung-soc@vger.kernel.org
>>>> CC: linux-clk@vger.kernel.org
>>>> CC: linux-arm-kernel@lists.infradead.org
>>>> CC: linux-kernel@vger.kernel.org
>>>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>>>> ---
>>>>    drivers/clk/samsung/clk-exynos5420.c | 15 ++++++++++++++-
>>>>    1 file changed, 14 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
>>>> index 3e87421..8bf9579 100644
>>>> --- a/drivers/clk/samsung/clk-exynos5420.c
>>>> +++ b/drivers/clk/samsung/clk-exynos5420.c
>>>> @@ -1325,6 +1325,19 @@ static const struct samsung_pll_rate_table exynos5420_pll2550x_24mhz_tbl[] __ini
>>>>       PLL_35XX_RATE(24 * MHZ, 200000000,  200, 3, 3),
>>>>    };
>>>>
>>>> +static const struct samsung_pll_rate_table exynos5422_bpll_rate_table[] = {
>>>> +    PLL_35XX_RATE(24 * MHZ, 933000000, 311, 4, 1),
>>>> +    PLL_35XX_RATE(24 * MHZ, 825000000, 275, 4, 1),
>>>> +    PLL_35XX_RATE(24 * MHZ, 728000000, 182, 3, 1),
>>>> +    PLL_35XX_RATE(24 * MHZ, 633000000, 211, 4, 1),
>>>> +    PLL_35XX_RATE(24 * MHZ, 543000000, 181, 2, 2),
>>>> +    PLL_35XX_RATE(24 * MHZ, 413000000, 413, 6, 2),
>>>> +    PLL_35XX_RATE(24 * MHZ, 275000000, 275, 3, 3),
>>>> +    PLL_35XX_RATE(24 * MHZ, 206000000, 206, 3, 3),
>>>> +    PLL_35XX_RATE(24 * MHZ, 165000000, 110, 2, 3),
>>>> +    PLL_35XX_RATE(24 * MHZ, 138000000, 184, 2, 4),
>>>
>>> Except for 825Mhz, I can't find the target frequency
>>> on Exynos5422 TRM document. Usually, Exynos TRM specified
>>> the supported stable clocks. It means that undefined clocks
>>> are not stable as I knew. Where do you find them?
>>>
>>> When I calculated the PLL frequency with PMS value, it is correct.
>>> But, just we need to check the reference of undefined clocks on TRM
>>> in order to guarantee the stable operation.
>> They values live in vendor code for Android.
>> I have tested the DMC & DDR with these ratios in stress scenarios
>> for a few days and it was stable.
> 
> If possible, please share the url of original vendor code.
Here is the vendor code for the BPLL values:
https://github.com/hardkernel/linux/blob/odroidxu3-3.10.y-android/drivers/clk/samsung/clk-exynos5422.c#L2026

Regards,
Lukasz
> 
>>
>>>
>>> Remove 933/138Mhz because exynos5433-dmc.c doesn't use 933Mhz and 138Mhz
>>> and also Exynos5422 TRM doesn't define 933/138Mhz on pll table.
>> OK, I will remove them.
>>>
>>>> +};
>>>> +
>>>>    static const struct samsung_pll_rate_table exynos5420_epll_24mhz_tbl[] = {
>>>>       PLL_36XX_RATE(24 * MHZ, 600000000U, 100, 2, 1, 0),
>>>>       PLL_36XX_RATE(24 * MHZ, 400000000U, 200, 3, 2, 0),
>>>> @@ -1467,7 +1480,7 @@ static void __init exynos5x_clk_init(struct device_node *np,
>>>>               exynos5x_plls[apll].rate_table = exynos5420_pll2550x_24mhz_tbl;
>>>>               exynos5x_plls[epll].rate_table = exynos5420_epll_24mhz_tbl;
>>>>               exynos5x_plls[kpll].rate_table = exynos5420_pll2550x_24mhz_tbl;
>>>> -            exynos5x_plls[bpll].rate_table = exynos5420_pll2550x_24mhz_tbl;
>>>> +            exynos5x_plls[bpll].rate_table = exynos5422_bpll_rate_table;
>>>
>>> Exynos5422 used the same PLL table for apll, kpll, bpll and so on.
>>> You don't need to make the separate pll table. Just add new entries
>>> to exynos5420_pll2550x_24mhz_tbl table.
>> OK, I will extend the exynos5420_pll2550x_24mhz_tbl table.
>>
>> In v4 patch set, it will be fixed.
>>
>> Regards,
>> Lukasz
>>>
>>>>       }
>>>>
>>>>       samsung_clk_register_pll(ctx, exynos5x_plls, ARRAY_SIZE(exynos5x_plls),
>>>>
>>>
> 
> 
> 
> --
> Best Regards,
> Chanwoo Choi
> Samsung Electronics
> 
> 

Re: [PATCH v3 3/8] clk: samsung: add BPLL rate table for Exynos 5422 SoC

[Chanwoo Choi]

Hi Lukasz,

On 19. 2. 11. 오후 7:21, Lukasz Luba wrote:
> Hi Chanwoo,
> 
> On 2/3/19 8:54 AM, Chanwoo Choi wrote:
>> Hi Lukasz,
>>
>> 2019년 2월 1일 (금) 오후 11:22, Lukasz Luba <l.luba@partner.samsung.com>님이 작성:
>>
>>>
>>> Hi Chanwoo,
>>>
>>> On 2/1/19 9:44 AM, Chanwoo Choi wrote:
>>>> Hi,
>>>>
>>>> On 19. 1. 31. 오후 5:49, Lukasz Luba wrote:
>>>>> Add new table rate for BPLL for Exynos5422 SoC supporting Dynamic Memory
>>>>> Controller frequencies for driver's DRAM timings.
>>>>>
>>>>> CC: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>>>> CC: Chanwoo Choi <cw00.choi@samsung.com>
>>>>> CC: Michael Turquette <mturquette@baylibre.com>
>>>>> CC: Stephen Boyd <sboyd@kernel.org>
>>>>> CC: Kukjin Kim <kgene@kernel.org>
>>>>> CC: Krzysztof Kozlowski <krzk@kernel.org>
>>>>> CC: linux-samsung-soc@vger.kernel.org
>>>>> CC: linux-clk@vger.kernel.org
>>>>> CC: linux-arm-kernel@lists.infradead.org
>>>>> CC: linux-kernel@vger.kernel.org
>>>>> Signed-off-by: Lukasz Luba <l.luba@partner.samsung.com>
>>>>> ---
>>>>>    drivers/clk/samsung/clk-exynos5420.c | 15 ++++++++++++++-
>>>>>    1 file changed, 14 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/clk/samsung/clk-exynos5420.c b/drivers/clk/samsung/clk-exynos5420.c
>>>>> index 3e87421..8bf9579 100644
>>>>> --- a/drivers/clk/samsung/clk-exynos5420.c
>>>>> +++ b/drivers/clk/samsung/clk-exynos5420.c
>>>>> @@ -1325,6 +1325,19 @@ static const struct samsung_pll_rate_table exynos5420_pll2550x_24mhz_tbl[] __ini
>>>>>       PLL_35XX_RATE(24 * MHZ, 200000000,  200, 3, 3),
>>>>>    };
>>>>>
>>>>> +static const struct samsung_pll_rate_table exynos5422_bpll_rate_table[] = {
>>>>> +    PLL_35XX_RATE(24 * MHZ, 933000000, 311, 4, 1),
>>>>> +    PLL_35XX_RATE(24 * MHZ, 825000000, 275, 4, 1),
>>>>> +    PLL_35XX_RATE(24 * MHZ, 728000000, 182, 3, 1),
>>>>> +    PLL_35XX_RATE(24 * MHZ, 633000000, 211, 4, 1),
>>>>> +    PLL_35XX_RATE(24 * MHZ, 543000000, 181, 2, 2),
>>>>> +    PLL_35XX_RATE(24 * MHZ, 413000000, 413, 6, 2),
>>>>> +    PLL_35XX_RATE(24 * MHZ, 275000000, 275, 3, 3),
>>>>> +    PLL_35XX_RATE(24 * MHZ, 206000000, 206, 3, 3),
>>>>> +    PLL_35XX_RATE(24 * MHZ, 165000000, 110, 2, 3),
>>>>> +    PLL_35XX_RATE(24 * MHZ, 138000000, 184, 2, 4),
>>>>
>>>> Except for 825Mhz, I can't find the target frequency
>>>> on Exynos5422 TRM document. Usually, Exynos TRM specified
>>>> the supported stable clocks. It means that undefined clocks
>>>> are not stable as I knew. Where do you find them?
>>>>
>>>> When I calculated the PLL frequency with PMS value, it is correct.
>>>> But, just we need to check the reference of undefined clocks on TRM
>>>> in order to guarantee the stable operation.
>>> They values live in vendor code for Android.
>>> I have tested the DMC & DDR with these ratios in stress scenarios
>>> for a few days and it was stable.
>>
>> If possible, please share the url of original vendor code.
> Here is the vendor code for the BPLL values:
> https://github.com/hardkernel/linux/blob/odroidxu3-3.10.y-android/drivers/clk/samsung/clk-exynos5422.c#L2026

Thanks for sharing.

bpll_rate_table has two different supported frequency
according to exynos5422 revision as following:
But, this patch only has only one frequency set
for CONFIG_SOC_EXYNOS5422_REV_0.

Could you guarantee that all Exynos5422-based Odroid-xu3 board
has CONFIG_SOC_EXYNOS5422_REV_0 ersion? If not guaranteed,
some board might be fault because of using the unsupported
frequencies.

It is dangerous and unstable to use the unsupported frequencies to SoC.

struct samsung_pll_rate_table bpll_rate_table[] = {
	/* rate		p	m	s	k */
#ifdef CONFIG_SOC_EXYNOS5422_REV_0
	{ 933000000U,   4,  311,    1,  0},
	{ 925000000U,   4,  307,    1,  0},
	{ 825000000U,   4,  275,    1,  0},
	{ 728000000U,   3,  182,    1,  0},
	{ 633000000U,   4,  211,    1,  0},
	{ 543000000U,   2,  181,    2,  0},
	{ 413000000U,   6,  413,    2,  0},
	{ 275000000U,   3,  275,    3,  0},
	{ 206000000U,   3,  206,    3,  0},
	{ 165000000U,   2,  110,    3,  0},
	{ 138000000U,   2,  184,    4,  0},
#else
	{ 933000000U,   4,  311,    1,  0},
	{ 800000000U,   3,  200,    1,  0},
	{ 733000000U,   2,  122,    1,  0},
	{ 667000000U,   2,  111,    1,  0},
	{ 533000000U,   3,  266,    2,  0},
	{ 400000000U,   3,  200,    2,  0},
	{ 266000000U,   3,  266,    3,  0},
	{ 200000000U,   3,  200,    3,  0},
	{ 160000000U,   3,  160,    3,  0},
#endif
};


> 
> Regards,
> Lukasz
>>
>>>
>>>>
>>>> Remove 933/138Mhz because exynos5433-dmc.c doesn't use 933Mhz and 138Mhz
>>>> and also Exynos5422 TRM doesn't define 933/138Mhz on pll table.
>>> OK, I will remove them.
>>>>
>>>>> +};
>>>>> +
>>>>>    static const struct samsung_pll_rate_table exynos5420_epll_24mhz_tbl[] = {
>>>>>       PLL_36XX_RATE(24 * MHZ, 600000000U, 100, 2, 1, 0),
>>>>>       PLL_36XX_RATE(24 * MHZ, 400000000U, 200, 3, 2, 0),
>>>>> @@ -1467,7 +1480,7 @@ static void __init exynos5x_clk_init(struct device_node *np,
>>>>>               exynos5x_plls[apll].rate_table = exynos5420_pll2550x_24mhz_tbl;
>>>>>               exynos5x_plls[epll].rate_table = exynos5420_epll_24mhz_tbl;
>>>>>               exynos5x_plls[kpll].rate_table = exynos5420_pll2550x_24mhz_tbl;
>>>>> -            exynos5x_plls[bpll].rate_table = exynos5420_pll2550x_24mhz_tbl;
>>>>> +            exynos5x_plls[bpll].rate_table = exynos5422_bpll_rate_table;
>>>>
>>>> Exynos5422 used the same PLL table for apll, kpll, bpll and so on.
>>>> You don't need to make the separate pll table. Just add new entries
>>>> to exynos5420_pll2550x_24mhz_tbl table.
>>> OK, I will extend the exynos5420_pll2550x_24mhz_tbl table.
>>>
>>> In v4 patch set, it will be fixed.
>>>
>>> Regards,
>>> Lukasz
>>>>
>>>>>       }
>>>>>
>>>>>       samsung_clk_register_pll(ctx, exynos5x_plls, ARRAY_SIZE(exynos5x_plls),
>>>>>
>>>>
>>
>>
>>
>> --
>> Best Regards,
>> Chanwoo Choi
>> Samsung Electronics
>>
>>
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics