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

From: Lukasz Luba
Date: Fri Feb 01 2019 - 08:03:45 EST

Next message: Tetsuo Handa: "[PATCH linux-next] LSM: Allow syzbot to ignore security= parameter."
Previous message: Vladimir Murzin: "Re:"
In reply to: Chanwoo Choi: "Re: [PATCH v3 2/8] clk: samsung: add new clocks for DMC for Exynos5422 SoC"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxx>
>>> CC: Chanwoo Choi <cw00.choi@xxxxxxxxxxx>
>>> CC: Michael Turquette <mturquette@xxxxxxxxxxxx>
>>> CC: Stephen Boyd <sboyd@xxxxxxxxxx>
>>> CC: Kukjin Kim <kgene@xxxxxxxxxx>
>>> CC: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
>>> CC: linux-samsung-soc@xxxxxxxxxxxxxxx
>>> CC: linux-clk@xxxxxxxxxxxxxxx
>>> CC: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>>> CC: linux-kernel@xxxxxxxxxxxxxxx
>>> Signed-off-by: Lukasz Luba <l.luba@xxxxxxxxxxxxxxxxxxx>
>>> ---
>>> 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 = {
>>>
>>
>>
>
>

Next message: Tetsuo Handa: "[PATCH linux-next] LSM: Allow syzbot to ignore security= parameter."
Previous message: Vladimir Murzin: "Re:"
In reply to: Chanwoo Choi: "Re: [PATCH v3 2/8] clk: samsung: add new clocks for DMC for Exynos5422 SoC"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]