Messages in this thread | | | Date | Mon, 8 Apr 2024 11:19:03 +0200 | From | neil.armstrong@linaro ... | Subject | Re: [PATCH v12 2/7] clk: meson: add vclk driver |
| |
On 05/04/2024 09:00, Jerome Brunet wrote: > > On Thu 04 Apr 2024 at 18:59, Neil Armstrong <neil.armstrong@linaro.org> wrote: > >> On 04/04/2024 10:13, Jerome Brunet wrote: >>> On Wed 03 Apr 2024 at 09:46, Neil Armstrong <neil.armstrong@linaro.org> >>> 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
Patch or commit seems to trigger more tests than a file directly, anyway I sent a follow-up patch: https://lore.kernel.org/all/20240408-amlogic-v6-9-upstream-fix-clk-module-license-v1-1-366ddc0f3db9@linaro.org/
Thanks, Neil
> >> >> <snip> >> >> Neil >> >>> >>>> >>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >>>> --- >>>> 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@linaro.org> >>>> + */ >>>> + >>>> +#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@linaro.org>"); >>>> +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@linaro.org> >>>> + */ >>>> + >>>> +#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 */ >>> > >
| |