On Thu 04 Apr 2024 at 18:59, Neil Armstrong <neil.armstrong@xxxxxxxxxx> wrote:
> On 04/04/2024 10:13, Jerome Brunet wrote:
>> On Wed 03 Apr 2024 at 09:46, Neil Armstrong <neil.armstrong@xxxxxxxxxx>
>> wrote:
>>
>>> The VCLK and VCLK_DIV clocks have supplementary bits.
>>>
>>> The VCLK gate has a "SOFT RESET" bit to toggle after the whole
>>> VCLK sub-tree rate has been set, this is implemented in
>>> the gate enable callback.
>>>
>>> The VCLK_DIV clocks as enable and reset bits used to disable
>>> and reset the divider, associated with CLK_SET_RATE_GATE it ensures
>>> the rate is set while the divider is disabled and in reset mode.
>>>
>>> The VCLK_DIV enable bit isn't implemented as a gate since it's part
>>> of the divider logic and vendor does this exact sequence to ensure
>>> the divider is correctly set.
>> The checkpatch warning is still there. Is it a choice or a mistake ?
>> Documentation says "GPL v2" exists for historic reason which seems to
>> hint "GPL" is preferred, and I suppose this is why checkpatch warns for
>> it.
>
> Well I didn't see this warning, this is what I fixed:
>
> $ scripts/checkpatch.pl --strict drivers/clk/meson/vclk.c
> CHECK: Alignment should match open parenthesis
> #63: FILE: drivers/clk/meson/vclk.c:63:
> +static unsigned long meson_vclk_div_recalc_rate(struct clk_hw *hw,
> + unsigned long prate)
>
> CHECK: Alignment should match open parenthesis
> #73: FILE: drivers/clk/meson/vclk.c:73:
> +static int meson_vclk_div_determine_rate(struct clk_hw *hw,
> + struct clk_rate_request *req)
>
> CHECK: Alignment should match open parenthesis
> #83: FILE: drivers/clk/meson/vclk.c:83:
> +static int meson_vclk_div_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
>
I would not ask a respin solely for this. It's nice to fix it but I was
mostly after the warning TBH.
> <snip>
>
> It seems that checking a commit triggers an extra check....
>
> $ scripts/checkpatch.pl --strict -G 1bac9f6aa3c3
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #58:
> new file mode 100644
>
> <snip>
>
> WARNING: Prefer "GPL" over "GPL v2" - see commit bf7fbeeae6db ("module: Cure the MODULE_LICENSE "GPL" vs. "GPL v2" bogosity")
> #203: FILE: drivers/clk/meson/vclk.c:141:
> +MODULE_LICENSE("GPL v2");
Hum, I'm running checkpatch against the mail itself, not the commit. I
still get the warning
>
> <snip>
>
> Neil
>
>>
>>>
>>> Signed-off-by: Neil Armstrong <neil.armstrong@xxxxxxxxxx>
>>> ---
>>> drivers/clk/meson/Kconfig | 4 ++
>>> drivers/clk/meson/Makefile | 1 +
>>> drivers/clk/meson/vclk.c | 141 +++++++++++++++++++++++++++++++++++++++++++++
>>> drivers/clk/meson/vclk.h | 51 ++++++++++++++++
>>> 4 files changed, 197 insertions(+)
>>>
>>> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
>>> index 29ffd14d267b..8a9823789fa3 100644
>>> --- a/drivers/clk/meson/Kconfig
>>> +++ b/drivers/clk/meson/Kconfig
>>> @@ -30,6 +30,10 @@ config COMMON_CLK_MESON_VID_PLL_DIV
>>> tristate
>>> select COMMON_CLK_MESON_REGMAP
>>> +config COMMON_CLK_MESON_VCLK
>>> + tristate
>>> + select COMMON_CLK_MESON_REGMAP
>>> +
>>> config COMMON_CLK_MESON_CLKC_UTILS
>>> tristate
>>> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
>>> index 9ee4b954c896..9ba43fe7a07a 100644
>>> --- a/drivers/clk/meson/Makefile
>>> +++ b/drivers/clk/meson/Makefile
>>> @@ -12,6 +12,7 @@ obj-$(CONFIG_COMMON_CLK_MESON_PLL) += clk-pll.o
>>> obj-$(CONFIG_COMMON_CLK_MESON_REGMAP) += clk-regmap.o
>>> obj-$(CONFIG_COMMON_CLK_MESON_SCLK_DIV) += sclk-div.o
>>> obj-$(CONFIG_COMMON_CLK_MESON_VID_PLL_DIV) += vid-pll-div.o
>>> +obj-$(CONFIG_COMMON_CLK_MESON_VCLK) += vclk.o
>>> # Amlogic Clock controllers
>>> diff --git a/drivers/clk/meson/vclk.c b/drivers/clk/meson/vclk.c
>>> new file mode 100644
>>> index 000000000000..45dc216941ea
>>> --- /dev/null
>>> +++ b/drivers/clk/meson/vclk.c
>>> @@ -0,0 +1,141 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (c) 2024 Neil Armstrong <neil.armstrong@xxxxxxxxxx>
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include "vclk.h"
>>> +
>>> +/* The VCLK gate has a supplementary reset bit to pulse after ungating */
>>> +
>>> +static inline struct meson_vclk_gate_data *
>>> +clk_get_meson_vclk_gate_data(struct clk_regmap *clk)
>>> +{
>>> + return (struct meson_vclk_gate_data *)clk->data;
>>> +}
>>> +
>>> +static int meson_vclk_gate_enable(struct clk_hw *hw)
>>> +{
>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>> + struct meson_vclk_gate_data *vclk = clk_get_meson_vclk_gate_data(clk);
>>> +
>>> + meson_parm_write(clk->map, &vclk->enable, 1);
>>> +
>>> + /* Do a reset pulse */
>>> + meson_parm_write(clk->map, &vclk->reset, 1);
>>> + meson_parm_write(clk->map, &vclk->reset, 0);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void meson_vclk_gate_disable(struct clk_hw *hw)
>>> +{
>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>> + struct meson_vclk_gate_data *vclk = clk_get_meson_vclk_gate_data(clk);
>>> +
>>> + meson_parm_write(clk->map, &vclk->enable, 0);
>>> +}
>>> +
>>> +static int meson_vclk_gate_is_enabled(struct clk_hw *hw)
>>> +{
>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>> + struct meson_vclk_gate_data *vclk = clk_get_meson_vclk_gate_data(clk);
>>> +
>>> + return meson_parm_read(clk->map, &vclk->enable);
>>> +}
>>> +
>>> +const struct clk_ops meson_vclk_gate_ops = {
>>> + .enable = meson_vclk_gate_enable,
>>> + .disable = meson_vclk_gate_disable,
>>> + .is_enabled = meson_vclk_gate_is_enabled,
>>> +};
>>> +EXPORT_SYMBOL_GPL(meson_vclk_gate_ops);
>>> +
>>> +/* The VCLK Divider has supplementary reset & enable bits */
>>> +
>>> +static inline struct meson_vclk_div_data *
>>> +clk_get_meson_vclk_div_data(struct clk_regmap *clk)
>>> +{
>>> + return (struct meson_vclk_div_data *)clk->data;
>>> +}
>>> +
>>> +static unsigned long meson_vclk_div_recalc_rate(struct clk_hw *hw,
>>> + unsigned long prate)
>>> +{
>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>> + struct meson_vclk_div_data *vclk = clk_get_meson_vclk_div_data(clk);
>>> +
>>> + return divider_recalc_rate(hw, prate, meson_parm_read(clk->map, &vclk->div),
>>> + vclk->table, vclk->flags, vclk->div.width);
>>> +}
>>> +
>>> +static int meson_vclk_div_determine_rate(struct clk_hw *hw,
>>> + struct clk_rate_request *req)
>>> +{
>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>> + struct meson_vclk_div_data *vclk = clk_get_meson_vclk_div_data(clk);
>>> +
>>> + return divider_determine_rate(hw, req, vclk->table, vclk->div.width,
>>> + vclk->flags);
>>> +}
>>> +
>>> +static int meson_vclk_div_set_rate(struct clk_hw *hw, unsigned long rate,
>>> + unsigned long parent_rate)
>>> +{
>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>> + struct meson_vclk_div_data *vclk = clk_get_meson_vclk_div_data(clk);
>>> + int ret;
>>> +
>>> + ret = divider_get_val(rate, parent_rate, vclk->table, vclk->div.width,
>>> + vclk->flags);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + meson_parm_write(clk->map, &vclk->div, ret);
>>> +
>>> + return 0;
>>> +};
>>> +
>>> +static int meson_vclk_div_enable(struct clk_hw *hw)
>>> +{
>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>> + struct meson_vclk_div_data *vclk = clk_get_meson_vclk_div_data(clk);
>>> +
>>> + /* Unreset the divider when ungating */
>>> + meson_parm_write(clk->map, &vclk->reset, 0);
>>> + meson_parm_write(clk->map, &vclk->enable, 1);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void meson_vclk_div_disable(struct clk_hw *hw)
>>> +{
>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>> + struct meson_vclk_div_data *vclk = clk_get_meson_vclk_div_data(clk);
>>> +
>>> + /* Reset the divider when gating */
>>> + meson_parm_write(clk->map, &vclk->enable, 0);
>>> + meson_parm_write(clk->map, &vclk->reset, 1);
>>> +}
>>> +
>>> +static int meson_vclk_div_is_enabled(struct clk_hw *hw)
>>> +{
>>> + struct clk_regmap *clk = to_clk_regmap(hw);
>>> + struct meson_vclk_div_data *vclk = clk_get_meson_vclk_div_data(clk);
>>> +
>>> + return meson_parm_read(clk->map, &vclk->enable);
>>> +}
>>> +
>>> +const struct clk_ops meson_vclk_div_ops = {
>>> + .recalc_rate = meson_vclk_div_recalc_rate,
>>> + .determine_rate = meson_vclk_div_determine_rate,
>>> + .set_rate = meson_vclk_div_set_rate,
>>> + .enable = meson_vclk_div_enable,
>>> + .disable = meson_vclk_div_disable,
>>> + .is_enabled = meson_vclk_div_is_enabled,
>>> +};
>>> +EXPORT_SYMBOL_GPL(meson_vclk_div_ops);
>>> +
>>> +MODULE_DESCRIPTION("Amlogic vclk clock driver");
>>> +MODULE_AUTHOR("Neil Armstrong <neil.armstrong@xxxxxxxxxx>");
>>> +MODULE_LICENSE("GPL v2");
>>> diff --git a/drivers/clk/meson/vclk.h b/drivers/clk/meson/vclk.h
>>> new file mode 100644
>>> index 000000000000..20b0b181db09
>>> --- /dev/null
>>> +++ b/drivers/clk/meson/vclk.h
>>> @@ -0,0 +1,51 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + * Copyright (c) 2024 Neil Armstrong <neil.armstrong@xxxxxxxxxx>
>>> + */
>>> +
>>> +#ifndef __VCLK_H
>>> +#define __VCLK_H
>>> +
>>> +#include "clk-regmap.h"
>>> +#include "parm.h"
>>> +
>>> +/**
>>> + * struct meson_vclk_gate_data - vclk_gate regmap backed specific data
>>> + *
>>> + * @enable: vclk enable field
>>> + * @reset: vclk reset field
>>> + * @flags: hardware-specific flags
>>> + *
>>> + * Flags:
>>> + * Same as clk_gate except CLK_GATE_HIWORD_MASK which is ignored
>>> + */
>>> +struct meson_vclk_gate_data {
>>> + struct parm enable;
>>> + struct parm reset;
>>> + u8 flags;
>>> +};
>>> +
>>> +extern const struct clk_ops meson_vclk_gate_ops;
>>> +
>>> +/**
>>> + * struct meson_vclk_div_data - vclk_div regmap back specific data
>>> + *
>>> + * @div: divider field
>>> + * @enable: vclk divider enable field
>>> + * @reset: vclk divider reset field
>>> + * @table: array of value/divider pairs, last entry should have div = 0
>>> + *
>>> + * Flags:
>>> + * Same as clk_divider except CLK_DIVIDER_HIWORD_MASK which is ignored
>>> + */
>>> +struct meson_vclk_div_data {
>>> + struct parm div;
>>> + struct parm enable;
>>> + struct parm reset;
>>> + const struct clk_div_table *table;
>>> + u8 flags;
>>> +};
>>> +
>>> +extern const struct clk_ops meson_vclk_div_ops;
>>> +
>>> +#endif /* __VCLK_H */
>>
--
Jerome
