| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJKOXPeziDFrxHcKL8LPTcT9nVbeZqbjGjEL6ioSaWHVdpF_vQ@mail.gmail.com>
Date: Sat, 11 Jul 2015 15:36:35 +0900
From: Krzysztof Kozlowski <k.kozlowski@...sung.com>
To: Javier Martinez Canillas <javier@....samsung.com>
Cc: Krzysztof Kozlowski <k.kozlowski@...sung.com>,
Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
Thomas Abraham <thomas.ab@...sung.com>,
Sylwester Nawrocki <s.nawrocki@...sung.com>,
Michael Turquette <mturquette@...libre.com>,
Kukjin Kim <kgene.kim@...sung.com>,
Kukjin Kim <kgene@...nel.org>,
Viresh Kumar <viresh.kumar@...aro.org>,
Tomasz Figa <tomasz.figa@...il.com>,
Lukasz Majewski <l.majewski@...sung.com>,
Heiko Stuebner <heiko@...ech.de>,
Chanwoo Choi <cw00.choi@...sung.com>,
Kevin Hilman <khilman@...aro.org>,
Javier Martinez Canillas <javier@...hile0.org>,
Tobias Jakobi <tjakobi@...h.uni-bielefeld.de>,
Anand Moon <linux.amoon@...il.com>,
linux-samsung-soc@...r.kernel.org, linux-clk@...r.kernel.org,
linux-pm@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/7] clk: samsung: exynos4x12: add cpu clock
configuration data and instantiate cpu clock
2015-07-11 1:12 GMT+09:00 Javier Martinez Canillas <javier@....samsung.com>:
> Hello Krzysztof,
>
> On 07/10/2015 01:30 AM, Krzysztof Kozlowski wrote:
>> On 10.07.2015 00:43, Bartlomiej Zolnierkiewicz wrote:
>>> With the addition of the new Samsung specific cpu-clock type, the
>>> arm clock can be represented as a cpu-clock type. Add the CPU clock
>>> configuration data and instantiate the CPU clock type for Exynos4x12.
>>>
>>> Based on the earlier work by Thomas Abraham.
>>>
>>> Cc: Tomasz Figa <tomasz.figa@...il.com>
>>> Cc: Michael Turquette <mturquette@...libre.com>
>>> Cc: Javier Martinez Canillas <javier@...hile0.org>
>>> Cc: Thomas Abraham <thomas.ab@...sung.com>
>>> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
>>> ---
>>> drivers/clk/samsung/clk-exynos4.c | 50 +++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 50 insertions(+)
>>>
>>> diff --git a/drivers/clk/samsung/clk-exynos4.c b/drivers/clk/samsung/clk-exynos4.c
>>> index cae2c048..3071260 100644
>>> --- a/drivers/clk/samsung/clk-exynos4.c
>>> +++ b/drivers/clk/samsung/clk-exynos4.c
>>> @@ -1396,6 +1396,45 @@ static const struct exynos_cpuclk_cfg_data e4210_armclk_d[] __initconst = {
>>> { 0 },
>>> };
>>>
>>> +static const struct exynos_cpuclk_cfg_data e4212_armclk_d[] __initconst = {
>>> + { 1500000, E4210_CPU_DIV0(2, 1, 6, 0, 7, 3), E4210_CPU_DIV1(2, 6), },
>>> + { 1400000, E4210_CPU_DIV0(2, 1, 6, 0, 7, 3), E4210_CPU_DIV1(2, 6), },
>>> + { 1300000, E4210_CPU_DIV0(2, 1, 5, 0, 7, 3), E4210_CPU_DIV1(2, 5), },
>>> + { 1200000, E4210_CPU_DIV0(2, 1, 5, 0, 7, 3), E4210_CPU_DIV1(2, 5), },
>>> + { 1100000, E4210_CPU_DIV0(2, 1, 4, 0, 6, 3), E4210_CPU_DIV1(2, 4), },
>>> + { 1000000, E4210_CPU_DIV0(1, 1, 4, 0, 5, 2), E4210_CPU_DIV1(2, 4), },
>>> + { 900000, E4210_CPU_DIV0(1, 1, 3, 0, 5, 2), E4210_CPU_DIV1(2, 3), },
>>> + { 800000, E4210_CPU_DIV0(1, 1, 3, 0, 5, 2), E4210_CPU_DIV1(2, 3), },
>>> + { 700000, E4210_CPU_DIV0(1, 1, 3, 0, 4, 2), E4210_CPU_DIV1(2, 3), },
>>> + { 600000, E4210_CPU_DIV0(1, 1, 3, 0, 4, 2), E4210_CPU_DIV1(2, 3), },
>>> + { 500000, E4210_CPU_DIV0(1, 1, 3, 0, 4, 2), E4210_CPU_DIV1(2, 3), },
>>> + { 400000, E4210_CPU_DIV0(1, 1, 3, 0, 4, 2), E4210_CPU_DIV1(2, 3), },
>>> + { 300000, E4210_CPU_DIV0(1, 1, 2, 0, 4, 2), E4210_CPU_DIV1(2, 3), },
>>> + { 200000, E4210_CPU_DIV0(1, 1, 1, 0, 3, 1), E4210_CPU_DIV1(2, 3), },
>>> + { 0 },
>>> +};
>>> +
>>> +#define E4412_CPU_DIV1(cores, hpm, copy) \
>>> + (((cores) << 8) | ((hpm) << 4) | ((copy) << 0))
>>> +
>>> +static const struct exynos_cpuclk_cfg_data e4412_armclk_d[] __initconst = {
>>> + { 1500000, E4210_CPU_DIV0(2, 1, 6, 0, 7, 3), E4412_CPU_DIV1(7, 0, 6), },
>>> + { 1400000, E4210_CPU_DIV0(2, 1, 6, 0, 7, 3), E4412_CPU_DIV1(6, 0, 6), },
>>> + { 1300000, E4210_CPU_DIV0(2, 1, 5, 0, 7, 3), E4412_CPU_DIV1(6, 0, 5), },
>>> + { 1200000, E4210_CPU_DIV0(2, 1, 5, 0, 7, 3), E4412_CPU_DIV1(5, 0, 5), },
>>> + { 1100000, E4210_CPU_DIV0(2, 1, 4, 0, 6, 3), E4412_CPU_DIV1(5, 0, 4), },
>>> + { 1000000, E4210_CPU_DIV0(1, 1, 4, 0, 5, 2), E4412_CPU_DIV1(4, 0, 4), },
>>> + { 900000, E4210_CPU_DIV0(1, 1, 3, 0, 5, 2), E4412_CPU_DIV1(4, 0, 3), },
>>> + { 800000, E4210_CPU_DIV0(1, 1, 3, 0, 5, 2), E4412_CPU_DIV1(3, 0, 3), },
>>> + { 700000, E4210_CPU_DIV0(1, 1, 3, 0, 4, 2), E4412_CPU_DIV1(3, 0, 3), },
>>> + { 600000, E4210_CPU_DIV0(1, 1, 3, 0, 4, 2), E4412_CPU_DIV1(2, 0, 3), },
>>> + { 500000, E4210_CPU_DIV0(1, 1, 3, 0, 4, 2), E4412_CPU_DIV1(2, 0, 3), },
>>> + { 400000, E4210_CPU_DIV0(1, 1, 3, 0, 4, 2), E4412_CPU_DIV1(1, 0, 3), },
>>> + { 300000, E4210_CPU_DIV0(1, 1, 2, 0, 4, 2), E4412_CPU_DIV1(1, 0, 3), },
>>> + { 200000, E4210_CPU_DIV0(1, 1, 1, 0, 3, 1), E4412_CPU_DIV1(0, 0, 3), },
>>> + { 0 },
>>> +};
>>
>> Numbers look fine!
>>
>>> +
>>> /* register exynos4 clocks */
>>> static void __init exynos4_clk_init(struct device_node *np,
>>> enum exynos4_soc soc)
>>> @@ -1489,6 +1528,17 @@ static void __init exynos4_clk_init(struct device_node *np,
>>> samsung_clk_register_fixed_factor(ctx,
>>> exynos4x12_fixed_factor_clks,
>>> ARRAY_SIZE(exynos4x12_fixed_factor_clks));
>>> + if (of_machine_is_compatible("samsung,exynos4412")) {
>>
>> The driver uses here enum exynos4_soc to differentiate between SoC
>> (unless I missed some changes). This of_machine_is_compatible() makes
>> sense but introduces inconsistency. I would prefer sticking to one
>> convention: always enum or switch everything (before this patch) to
>> of_compatible.
>>
>
> When reviewing this patch I also ran into the same thing because as you
> said, it's not consistent. But digging a little bit I found that is not
> that easy since the two are not checking exactly the same.
>
> The enum is to differentiate between "samsung,exynos4412-clock" and
> "samsung,exynos4210-clock" while the of_machine_is_compatible() is for
> "samsung,exynos4412" and "samsung,exynos4212".
>
> The problem is that both exynos4412 and exynos4212 use the same
> "samsung,exynos4412-clock" compatible for their clock controller nodes.
> But there are differences so it would had been better to also have a
> "samsung,exynos4212-clock" to avoid the of_machine_is_compatible() but
> that is not possible anymore without breaking DT backward compatibility.
>
> On the other hand, if of_machine_is_compatible() is used for everything,
> then there is no point anymore to have both "samsung,exynos4412-clock"
> and "samsung,exynos4210-clock". A single "samsung,exynos4-clock" plus
> checking the SoC would had been enough.
>
> That's why I thought that Bart's approach was sensible although is true
> that the of_compatible() check can be moved to exynos4412_clk_init()
> and the enum be extended so at least exynos4_clk_init() is consistent.
You're right, I missed that difference. Thanks for explaining, I agree now:
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@...sung.com>
Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists