On Wed, Dec 27, 2023 at 3:12 AM Minkyu Kang <proms...@gmail.com> wrote:
>
> Hi,
>
>
> 2023년 12월 13일 (수) 12:27, Sam Protsenko <semen.protse...@linaro.org>님이 작성:
>>
>> Heavily based on Linux kernel Samsung clock framework, with some changes
>> to accommodate the differences in U-Boot CCF implementation. It's also
>> quite minimal as compared to the Linux version.
>>
>> Signed-off-by: Sam Protsenko <semen.protse...@linaro.org>
>> ---
[snip]
>> diff --git a/drivers/clk/exynos/clk-pll.h b/drivers/clk/exynos/clk-pll.h
>> new file mode 100644
>> index 000000000000..3b477369aeb8
>> --- /dev/null
>> +++ b/drivers/clk/exynos/clk-pll.h
>> @@ -0,0 +1,23 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ */
>> +/*
>> + * Copyright (C) 2016 Samsung Electronics
>> + * Copyright (C) 2023 Linaro Ltd.
>> + *
>> + * Authors:
>> + * Thomas Abraham <thomas...@exynos.com>
>> + * Sam Protsenko <semen.protse...@linaro.org>
>> + *
>> + * Common Clock Framework support for all PLL's in Samsung platforms.
>> + */
>> +
>> +#ifndef __EXYNOS_CLK_PLL_H
>> +#define __EXYNOS_CLK_PLL_H
>> +
>> +#include <linux/clk-provider.h>
>> +
>> +enum samsung_pll_type {
>> + pll_0822x,
>> + pll_0831x,
>
>
> why don't you modify to uppercase?
>
That code was basically copied over from Linux kernel (from
drivers/clk/samsung/clk-pll.h file). I'm trying to keep it as close to
the original as possible, to ease any possible backporting in future.
Although kernel coding style indeed tends to stick to uppercase in
enums, in my opinion the backporting/compatibility concern outweighs
the style one. Hope it's ok with you if I keep it as is in v2?
>>
>> +};
>> +
>> +#endif /* __EXYNOS_CLK_PLL_H */
>> diff --git a/drivers/clk/exynos/clk.c b/drivers/clk/exynos/clk.c
>> new file mode 100644
>> index 000000000000..430767f072d8
>> --- /dev/null
>> +++ b/drivers/clk/exynos/clk.c
>> @@ -0,0 +1,121 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2023 Linaro Ltd.
>> + * Sam Protsenko <semen.protse...@linaro.org>
>> + *
>> + * This file includes utility functions to register clocks to common
>> + * clock framework for Samsung platforms.
>> + */
>> +
>> +#include <dm.h>
>> +#include "clk.h"
>> +
>> +void samsung_clk_register_mux(void __iomem *base,
>> + const struct samsung_mux_clock *clk_list,
>> + unsigned int nr_clk)
>> +{
>> + unsigned int cnt;
>> +
>> + for (cnt = 0; cnt < nr_clk; cnt++) {
>> + struct clk *clk;
>> + const struct samsung_mux_clock *m;
>
>
> wouldn't it be better if use a more meaningful name like mux?
>
My reasoning for choosing the name that short in this case was because
of super-short scope (3 lines of code), and OTOH this variable is
massively used during that scope, like this:
clk = clk_register_mux(NULL, m->name, m->parent_names,
m->num_parents, m->flags, base + m->offset, m->shift,
m->width, m->mux_flags);
Hope it makes sense. If you still prefer 'mux', please let me know and
I'll use it in v2.
>>
>> +
>> + m = &clk_list[cnt];
>
>
> Is there any possibility that the value is null or wrong (e.g. overflow)
>
I decided to keep it with no error handling because I didn't feel like
it would bring much value. Because this code is supposed to be used
via samsung_cmu_register_one(), and the CMU structure passed to that
function is usually going to be defined in this idiomatic way (as can
be seen in clk-exynos850.c driver):
static const struct samsung_clk_group top_cmu_clks[] = {
{ S_CLK_PLL, top_pure_pll_clks, ARRAY_SIZE(top_pure_pll_clks) },
{ S_CLK_MUX, top_pure_mux_clks, ARRAY_SIZE(top_pure_mux_clks) },
...
and the corresponding clocks structures are also defined like this:
static const struct samsung_mux_clock top_pure_mux_clks[] = {
MUX(CLK_MOUT_SHARED0_PLL, "mout_shared0_pll",
mout_shared0_pll_p,
PLL_CON0_PLL_SHARED0, 4, 1),
MUX(CLK_MOUT_SHARED1_PLL, "mout_shared1_pll",
mout_shared1_pll_p,
PLL_CON0_PLL_SHARED1, 4, 1),
...
I'd say the odds for messing this up are next to none, because of
using ARRAY_SIZE() and clock macros like MUX(). Especially because the
example is already set in clk-exynos850 driver and I assume everybody
would just use it as a template, which usually happens. So after
exploring the alternative approach (with added error handling) I felt
it was unjustifiable cluttered comparing to the more concise version
present in this series, at least in this particular code. Also, that
code resembles its kernel counterpart, where the clock pointer isn't
checked as well.
I'm not even sure how to handle possible errors here, as it's the
critical platform code. So maybe letting it just crash is not a bad
decision too. But if you have a strong opinion on this one, please let
me know how you would like me to handle that.
>> + clk = clk_register_mux(NULL, m->name, m->parent_names,
>> + m->num_parents, m->flags, base + m->offset, m->shift,
>> + m->width, m->mux_flags);
>> + clk_dm(m->id, clk);
>> + }
>>> +}
[snip]
