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