From: Andrzej Hajda <a.hajda@samsung.com>
To: Chanwoo Choi <cw00.choi@samsung.com>,
Sylwester Nawrocki <s.nawrocki@samsung.com>
Cc: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
Marek Szyprowski <m.szyprowski@samsung.com>,
Tomasz Figa <tomasz.figa@gmail.com>,
Michael Turquette <mturquette@baylibre.com>,
Stephen Boyd <sboyd@kernel.org>, Kukjin Kim <kgene@kernel.org>,
Krzysztof Kozlowski <krzk@kernel.org>,
"moderated list:SAMSUNG SOC CLOCK DRIVERS"
<linux-samsung-soc@vger.kernel.org>,
"open list:COMMON CLK FRAMEWORK" <linux-clk@vger.kernel.org>
Subject: Re: [PATCH v2 7/7] clk: samsung: add compile time PLL rate validators
Date: Mon, 19 Feb 2018 11:07:17 +0100 [thread overview]
Message-ID: <99c5621b-7a35-51d5-9642-0e8c599df0fa@samsung.com> (raw)
In-Reply-To: <5A8A9C74.6080503@samsung.com>
On 19.02.2018 10:44, Chanwoo Choi wrote:
> Hi,
>
> On 2018년 02월 16일 23:57, Andrzej Hajda wrote:
>> Rates declared in PLL rate tables should match exactly rates calculated
>> from PLL coefficients. To avoid possible mistakes we can use compile
>> time validation.
>> The patch introduces such validators and expands all initializers
>> with additional input frequency parameter, required to validate rates.
>> Since S3C24xx PLLs requires different validators two new macros have
>> been introduced to deal with it.
>> As the patch adds only compile time validators it should not have impact
>> on compiled code.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> Acked-by: Krzysztof Kozlowski <krzk@kernel.org>
>> Acked-by: Tomasz Figa <tomasz.figa@gmail.com>
>> ---
>> Thanks Krzysztof for ack, I hope my changes do not invalidate it.
>>
>> v2:
>> - replaced custom checker with existing BUILD_BUG_ON_ZERO,
>> - fixed comment (by annihilation),
>> - added Acked-By
>> ---
>> drivers/clk/samsung/clk-exynos3250.c | 114 +++++++++++++++++-----------------
>> drivers/clk/samsung/clk-exynos4.c | 102 +++++++++++++++---------------
>> drivers/clk/samsung/clk-exynos5250.c | 54 ++++++++--------
>> drivers/clk/samsung/clk-exynos5260.c | 90 +++++++++++++--------------
>> drivers/clk/samsung/clk-exynos5410.c | 20 +++---
>> drivers/clk/samsung/clk-exynos5420.c | 62 +++++++++----------
>> drivers/clk/samsung/clk-exynos5433.c | 116 +++++++++++++++++------------------
>> drivers/clk/samsung/clk-exynos7.c | 2 +-
>> drivers/clk/samsung/clk-pll.h | 48 ++++++++++++---
>> drivers/clk/samsung/clk-s3c2410.c | 108 ++++++++++++++++----------------
>> 10 files changed, 372 insertions(+), 344 deletions(-)
> [snip]
>
>>
>> diff --git a/drivers/clk/samsung/clk-pll.h b/drivers/clk/samsung/clk-pll.h
>> index 61eb8abbfd9c..1d8c0a581249 100644
>> --- a/drivers/clk/samsung/clk-pll.h
>> +++ b/drivers/clk/samsung/clk-pll.h
>> @@ -41,35 +41,62 @@ enum samsung_pll_type {
>> pll_1460x,
>> };
>>
>> -#define PLL_35XX_RATE(_rate, _m, _p, _s) \
>> +#define PLL_RATE(_fin, _m, _p, _s, _k, _ks) \
>> + ((u64)(_fin) * (BIT(_ks) * (_m) + (_k)) / BIT(_ks) / ((_p) << (_s)))
>> +#define PLL_VALID_RATE(_fin, _fout, _m, _p, _s, _k, _ks) ((_fout) + \
>> + BUILD_BUG_ON_ZERO(PLL_RATE(_fin, _m, _p, _s, _k, _ks) != (_fout)))
>> +
>> +#define PLL_35XX_RATE(_fin, _rate, _m, _p, _s) \
>> + { \
>> + .rate = PLL_VALID_RATE(_fin, _rate, \
>> + _m, _p, _s, 0, 16), \
>> + .mdiv = (_m), \
>> + .pdiv = (_p), \
>> + .sdiv = (_s), \
>> + }
>> +
>> +#define PLL_S3C2410_MPLL_RATE(_fin, _rate, _m, _p, _s) \
>> + { \
>> + .rate = PLL_VALID_RATE(_fin, _rate, \
>> + _m + 8, _p + 2, _s, 0, 16), \
>> + .mdiv = (_m), \
>> + .pdiv = (_p), \
>> + .sdiv = (_s), \
>> + }
>> +
>> +#define PLL_S3C2440_MPLL_RATE(_fin, _rate, _m, _p, _s) \
>> { \
>> - .rate = (_rate), \
>> + .rate = PLL_VALID_RATE(_fin, _rate, \
>> + _m + 8, _p + 2, _s - 1, 0, 16), \
>> .mdiv = (_m), \
>> .pdiv = (_p), \
>> .sdiv = (_s), \
> Do you use 's - 1' instead of 'multiplication 2' of numerator?
Yes, it does the same thing, with assumption _s is greater than 0,
which is true, but I can switch it to multiplication by 2 of numerator,
to be more consistent with current code.
>
>> }
>>
>> -#define PLL_36XX_RATE(_rate, _m, _p, _s, _k) \
>> +#define PLL_36XX_RATE(_fin, _rate, _m, _p, _s, _k) \
>> { \
>> - .rate = (_rate), \
>> + .rate = PLL_VALID_RATE(_fin, _rate, \
>> + _m, _p, _s, _k, 16), \
>> .mdiv = (_m), \
>> .pdiv = (_p), \
>> .sdiv = (_s), \
>> .kdiv = (_k), \
>> }
>>
>> -#define PLL_45XX_RATE(_rate, _m, _p, _s, _afc) \
>> +#define PLL_45XX_RATE(_fin, _rate, _m, _p, _s, _afc) \
>> { \
>> - .rate = (_rate), \
>> + .rate = PLL_VALID_RATE(_fin, _rate, \
>> + _m, _p, _s - 1, 0, 16), \
> In clk-exynos4.c
> PLL_45XX_RATE() is used for exynos4210_plls[apll] which is pll_4508 type.
>
> In clk-pll.c,
> Both pll_4502 and pll_4508 shares the samsung_pll45xx_recalc_rate()
> which is samsung_pll45xx_clk_ops.recalc_rate().
>
> The samsung_pll45xx_recalc_rate() contains the following if statement.
> if (pll->type == pll_4508)
> sdiv = sdiv - 1;
>
> It means that following 's - 1' is not valid for 'pll_4502'.
>> + _m, _p, _s - 1, 0, 16), \
> Even if there are not any use-case of PLL_45XX_RATE for pll_4502 type,
> you need to use the correct name as following.
> - PLL_45XX_RATE() -> PLL_4508_RATE()
Yes, you are right, I will rename it.
Regards
Andrzej
>
>
>> .mdiv = (_m), \
>> .pdiv = (_p), \
>> .sdiv = (_s), \
>> .afc = (_afc), \
>> }
>>
>> -#define PLL_4600_RATE(_rate, _m, _p, _s, _k, _vsel) \
>> +#define PLL_4600_RATE(_fin, _rate, _m, _p, _s, _k, _vsel) \
>> { \
>> - .rate = (_rate), \
>> + .rate = PLL_VALID_RATE(_fin, _rate, \
>> + _m, _p, _s, _k, 16), \
>> .mdiv = (_m), \
>> .pdiv = (_p), \
>> .sdiv = (_s), \
>> @@ -77,9 +104,10 @@ enum samsung_pll_type {
>> .vsel = (_vsel), \
>> }
>>
>> -#define PLL_4650_RATE(_rate, _m, _p, _s, _k, _mfr, _mrr, _vsel) \
>> +#define PLL_4650_RATE(_fin, _rate, _m, _p, _s, _k, _mfr, _mrr, _vsel) \
>> { \
>> - .rate = (_rate), \
>> + .rate = PLL_VALID_RATE(_fin, _rate, \
>> + _m, _p, _s, _k, 10), \
>> .mdiv = (_m), \
>> .pdiv = (_p), \
>> .sdiv = (_s), \
>> diff --git a/drivers/clk/samsung/clk-s3c2410.c b/drivers/clk/samsung/clk-s3c2410.c
>> index d8e58a659467..0c6aa3e51336 100644
>> --- a/drivers/clk/samsung/clk-s3c2410.c
>> +++ b/drivers/clk/samsung/clk-s3c2410.c
>> @@ -162,34 +162,34 @@ struct samsung_clock_alias s3c2410_common_aliases[] __initdata = {
>> static struct samsung_pll_rate_table pll_s3c2410_12mhz_tbl[] __initdata = {
>> /* sorted in descending order */
>> /* 2410A extras */
>> - PLL_35XX_RATE(270000000, 127, 1, 1),
>> - PLL_35XX_RATE(268000000, 126, 1, 1),
>> - PLL_35XX_RATE(266000000, 125, 1, 1),
>> - PLL_35XX_RATE(226000000, 105, 1, 1),
>> - PLL_35XX_RATE(210000000, 132, 2, 1),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 270000000, 127, 1, 1),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 268000000, 126, 1, 1),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 266000000, 125, 1, 1),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 226000000, 105, 1, 1),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 210000000, 132, 2, 1),
>> /* 2410 common */
>> - PLL_35XX_RATE(202800000, 161, 3, 1),
>> - PLL_35XX_RATE(192000000, 88, 1, 1),
>> - PLL_35XX_RATE(186000000, 85, 1, 1),
>> - PLL_35XX_RATE(180000000, 82, 1, 1),
>> - PLL_35XX_RATE(170000000, 77, 1, 1),
>> - PLL_35XX_RATE(158000000, 71, 1, 1),
>> - PLL_35XX_RATE(152000000, 68, 1, 1),
>> - PLL_35XX_RATE(147000000, 90, 2, 1),
>> - PLL_35XX_RATE(135000000, 82, 2, 1),
>> - PLL_35XX_RATE(124000000, 116, 1, 2),
>> - PLL_35XX_RATE(118500000, 150, 2, 2),
>> - PLL_35XX_RATE(113000000, 105, 1, 2),
>> - PLL_35XX_RATE(101250000, 127, 2, 2),
>> - PLL_35XX_RATE(90000000, 112, 2, 2),
>> - PLL_35XX_RATE(84750000, 105, 2, 2),
>> - PLL_35XX_RATE(79000000, 71, 1, 2),
>> - PLL_35XX_RATE(67500000, 82, 2, 2),
>> - PLL_35XX_RATE(56250000, 142, 2, 3),
>> - PLL_35XX_RATE(48000000, 120, 2, 3),
>> - PLL_35XX_RATE(50700000, 161, 3, 3),
>> - PLL_35XX_RATE(45000000, 82, 1, 3),
>> - PLL_35XX_RATE(33750000, 82, 2, 3),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 202800000, 161, 3, 1),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 192000000, 88, 1, 1),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 186000000, 85, 1, 1),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 180000000, 82, 1, 1),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 170000000, 77, 1, 1),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 158000000, 71, 1, 1),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 152000000, 68, 1, 1),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 147000000, 90, 2, 1),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 135000000, 82, 2, 1),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 124000000, 116, 1, 2),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 118500000, 150, 2, 2),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 113000000, 105, 1, 2),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 101250000, 127, 2, 2),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 90000000, 112, 2, 2),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 84750000, 105, 2, 2),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 79000000, 71, 1, 2),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 67500000, 82, 2, 2),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 56250000, 142, 2, 3),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 48000000, 120, 2, 3),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 50700000, 161, 3, 3),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 45000000, 82, 1, 3),
>> + PLL_S3C2410_MPLL_RATE(12 * MHZ, 33750000, 82, 2, 3),
>> { /* sentinel */ },
>> };
>>
>> @@ -229,33 +229,33 @@ struct samsung_clock_alias s3c2410_aliases[] __initdata = {
>>
>> static struct samsung_pll_rate_table pll_s3c244x_12mhz_tbl[] __initdata = {
>> /* sorted in descending order */
>> - PLL_35XX_RATE(400000000, 0x5c, 1, 1),
>> - PLL_35XX_RATE(390000000, 0x7a, 2, 1),
>> - PLL_35XX_RATE(380000000, 0x57, 1, 1),
>> - PLL_35XX_RATE(370000000, 0xb1, 4, 1),
>> - PLL_35XX_RATE(360000000, 0x70, 2, 1),
>> - PLL_35XX_RATE(350000000, 0xa7, 4, 1),
>> - PLL_35XX_RATE(340000000, 0x4d, 1, 1),
>> - PLL_35XX_RATE(330000000, 0x66, 2, 1),
>> - PLL_35XX_RATE(320000000, 0x98, 4, 1),
>> - PLL_35XX_RATE(310000000, 0x93, 4, 1),
>> - PLL_35XX_RATE(300000000, 0x75, 3, 1),
>> - PLL_35XX_RATE(240000000, 0x70, 1, 2),
>> - PLL_35XX_RATE(230000000, 0x6b, 1, 2),
>> - PLL_35XX_RATE(220000000, 0x66, 1, 2),
>> - PLL_35XX_RATE(210000000, 0x84, 2, 2),
>> - PLL_35XX_RATE(200000000, 0x5c, 1, 2),
>> - PLL_35XX_RATE(190000000, 0x57, 1, 2),
>> - PLL_35XX_RATE(180000000, 0x70, 2, 2),
>> - PLL_35XX_RATE(170000000, 0x4d, 1, 2),
>> - PLL_35XX_RATE(160000000, 0x98, 4, 2),
>> - PLL_35XX_RATE(150000000, 0x75, 3, 2),
>> - PLL_35XX_RATE(120000000, 0x70, 1, 3),
>> - PLL_35XX_RATE(110000000, 0x66, 1, 3),
>> - PLL_35XX_RATE(100000000, 0x5c, 1, 3),
>> - PLL_35XX_RATE(90000000, 0x70, 2, 3),
>> - PLL_35XX_RATE(80000000, 0x98, 4, 3),
>> - PLL_35XX_RATE(75000000, 0x75, 3, 3),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 400000000, 0x5c, 1, 1),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 390000000, 0x7a, 2, 1),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 380000000, 0x57, 1, 1),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 370000000, 0xb1, 4, 1),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 360000000, 0x70, 2, 1),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 350000000, 0xa7, 4, 1),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 340000000, 0x4d, 1, 1),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 330000000, 0x66, 2, 1),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 320000000, 0x98, 4, 1),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 310000000, 0x93, 4, 1),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 300000000, 0x75, 3, 1),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 240000000, 0x70, 1, 2),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 230000000, 0x6b, 1, 2),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 220000000, 0x66, 1, 2),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 210000000, 0x84, 2, 2),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 200000000, 0x5c, 1, 2),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 190000000, 0x57, 1, 2),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 180000000, 0x70, 2, 2),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 170000000, 0x4d, 1, 2),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 160000000, 0x98, 4, 2),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 150000000, 0x75, 3, 2),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 120000000, 0x70, 1, 3),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 110000000, 0x66, 1, 3),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 100000000, 0x5c, 1, 3),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 90000000, 0x70, 2, 3),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 80000000, 0x98, 4, 3),
>> + PLL_S3C2440_MPLL_RATE(12 * MHZ, 75000000, 0x75, 3, 3),
>> { /* sentinel */ },
>> };
>>
>>
>
next prev parent reply other threads:[~2018-02-19 10:07 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20180216145803eucas1p2b75e6f26d2979ee129152f674a31e886@eucas1p2.samsung.com>
2018-02-16 14:57 ` [PATCH v2 0/7] clk: samsung: fix PLL rates Andrzej Hajda
2018-02-16 14:57 ` Andrzej Hajda
[not found] ` <CGME20180216145804eucas1p2cbe4de00131e6b7bbd946d349dfc2b2d@eucas1p2.samsung.com>
2018-02-16 14:57 ` [PATCH v2 1/7] clk: samsung: exynos3250: " Andrzej Hajda
2018-02-16 14:57 ` Andrzej Hajda
[not found] ` <CGME20180216145804eucas1p20cc58695fce84f9acc03a16b6913fcb2@eucas1p2.samsung.com>
2018-02-16 14:57 ` [PATCH v2 2/7] clk: samsung: exynos5250: " Andrzej Hajda
2018-02-16 14:57 ` Andrzej Hajda
[not found] ` <CGME20180216145805eucas1p14670777cd5339fda26b95c71b726f866@eucas1p1.samsung.com>
2018-02-16 14:57 ` [PATCH v2 3/7] clk: samsung: exynos5260: " Andrzej Hajda
2018-02-16 14:57 ` Andrzej Hajda
2018-02-19 6:27 ` Chanwoo Choi
[not found] ` <CGME20180216145805eucas1p28452227465bf275bb00dec65ab5b3978@eucas1p2.samsung.com>
2018-02-16 14:57 ` [PATCH v2 4/7] clk: samsung: exynos5433: " Andrzej Hajda
2018-02-16 14:57 ` Andrzej Hajda
2018-02-19 6:27 ` Chanwoo Choi
[not found] ` <CGME20180216145806eucas1p1e939d3138e29d3446df90e1a69ea9624@eucas1p1.samsung.com>
2018-02-16 14:57 ` [PATCH v2 5/7] clk: samsung: exynos7: " Andrzej Hajda
2018-02-16 14:57 ` Andrzej Hajda
2018-02-19 6:28 ` Chanwoo Choi
[not found] ` <CGME20180216145807eucas1p2260082a8178bc746dc8110982f2b0c2b@eucas1p2.samsung.com>
2018-02-16 14:57 ` [PATCH v2 6/7] clk: samsung: s3c2410: " Andrzej Hajda
2018-02-16 14:57 ` Andrzej Hajda
2018-02-20 7:10 ` Chanwoo Choi
[not found] ` <CGME20180216145807eucas1p27282e8889ea210ee082a5ffa3c94a4ff@eucas1p2.samsung.com>
2018-02-16 14:57 ` [PATCH v2 7/7] clk: samsung: add compile time PLL rate validators Andrzej Hajda
2018-02-16 14:57 ` Andrzej Hajda
2018-02-19 9:44 ` Chanwoo Choi
2018-02-19 10:07 ` Andrzej Hajda [this message]
[not found] ` <CGME20180220070544eucas1p17987a47caeba1c07a1d82eb852fcd3b1@eucas1p1.samsung.com>
2018-02-20 7:05 ` [PATCH v3 " Andrzej Hajda
2018-02-20 7:10 ` Chanwoo Choi
2018-03-06 16:46 ` [PATCH v2 0/7] clk: samsung: fix PLL rates Sylwester Nawrocki
2018-02-13 14:25 [PATCH 7/7] clk: samsung: add compile time PLL rate validators Krzysztof Kozlowski
[not found] ` <CGME20180214075346eucas1p1dc7aa2906bdb7dd07caa51c55b8dd340@eucas1p1.samsung.com>
2018-02-14 7:53 ` [PATCH v2 " Andrzej Hajda
2018-02-14 7:53 ` Andrzej Hajda
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=99c5621b-7a35-51d5-9642-0e8c599df0fa@samsung.com \
--to=a.hajda@samsung.com \
--cc=b.zolnierkie@samsung.com \
--cc=cw00.choi@samsung.com \
--cc=kgene@kernel.org \
--cc=krzk@kernel.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=m.szyprowski@samsung.com \
--cc=mturquette@baylibre.com \
--cc=s.nawrocki@samsung.com \
--cc=sboyd@kernel.org \
--cc=tomasz.figa@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.