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

[Lukasz Luba <l.luba@partner.samsung.com>]

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 = {
>>>
>>
>>
> 
>