Messages in this thread | | | Date | Mon, 27 Nov 2023 17:14:37 +0100 | From | Neil Armstrong <> | Subject | Re: [PATCH v9 07/12] clk: meson: add vclk driver |
| |
On 24/11/2023 15:41, Jerome Brunet wrote: > > On Fri 24 Nov 2023 at 09:41, Neil Armstrong <neil.armstrong@linaro.org> wrote: > >> The VCLK and VCLK_DIV clocks have supplementary bits. >> >> The VCLK 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. >> >> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org> >> --- >> drivers/clk/meson/Kconfig | 5 ++ >> drivers/clk/meson/Makefile | 1 + >> drivers/clk/meson/vclk.c | 141 +++++++++++++++++++++++++++++++++++++++++++++ >> drivers/clk/meson/vclk.h | 51 ++++++++++++++++ >> 4 files changed, 198 insertions(+) >> >> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig >> index 29ffd14d267b..59a40a49f8e1 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 >> >> @@ -140,6 +144,7 @@ config COMMON_CLK_G12A >> select COMMON_CLK_MESON_EE_CLKC >> select COMMON_CLK_MESON_CPU_DYNDIV >> select COMMON_CLK_MESON_VID_PLL_DIV >> + select COMMON_CLK_MESON_VCLK > > This particular line belong in the next patch > >> select MFD_SYSCON >> help >> Support for the clock controller on Amlogic S905D2, S905X2 and S905Y2 >> 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..47f08a52b49f >> --- /dev/null >> +++ b/drivers/clk/meson/vclk.c >> @@ -0,0 +1,141 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (c) 2023 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 clk_regmap_vclk_data * >> +clk_get_regmap_vclk_data(struct clk_regmap *clk) >> +{ >> + return (struct clk_regmap_vclk_data *)clk->data; >> +} >> + >> +static int clk_regmap_vclk_enable(struct clk_hw *hw) >> +{ >> + struct clk_regmap *clk = to_clk_regmap(hw); >> + struct clk_regmap_vclk_data *vclk = clk_get_regmap_vclk_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 clk_regmap_vclk_disable(struct clk_hw *hw) >> +{ >> + struct clk_regmap *clk = to_clk_regmap(hw); >> + struct clk_regmap_vclk_data *vclk = clk_get_regmap_vclk_data(clk); >> + >> + meson_parm_write(clk->map, &vclk->enable, 0); >> +} >> + >> +static int clk_regmap_vclk_is_enabled(struct clk_hw *hw) >> +{ >> + struct clk_regmap *clk = to_clk_regmap(hw); >> + struct clk_regmap_vclk_data *vclk = clk_get_regmap_vclk_data(clk); >> + >> + return meson_parm_read(clk->map, &vclk->enable); >> +} >> + >> +const struct clk_ops clk_regmap_vclk_ops = { >> + .enable = clk_regmap_vclk_enable, >> + .disable = clk_regmap_vclk_disable, >> + .is_enabled = clk_regmap_vclk_is_enabled, >> +}; >> +EXPORT_SYMBOL_GPL(clk_regmap_vclk_ops); > > s/clk_regmap_vclk/meson_vclk at least for what is exported, ideally most > all the code. > > I get clk_regmap_ comes from code copied from clk_regmap.c. > The reason the this part is different (and not using parm) if that when > I converted amlogic to regmap, I hope we could make this generic, > possibly converging between aml and qcom (which was the only other > platform using regmap for clock at the time). This is why clk_regmap.c > is a bit different from the other driver. > > For the aml specific drivers, best to look at the mpll or cpu-dyndiv one. > >> + >> +/* The VCLK Divider has supplementary reset & enable bits */ >> + >> +static inline struct clk_regmap_vclk_div_data * >> +clk_get_regmap_vclk_div_data(struct clk_regmap *clk) >> +{ >> + return (struct clk_regmap_vclk_div_data *)clk->data; >> +} >> + >> +static unsigned long clk_regmap_vclk_div_recalc_rate(struct clk_hw *hw, >> + unsigned long prate) >> +{ >> + struct clk_regmap *clk = to_clk_regmap(hw); >> + struct clk_regmap_vclk_div_data *vclk = clk_get_regmap_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 clk_regmap_vclk_div_determine_rate(struct clk_hw *hw, >> + struct clk_rate_request *req) >> +{ >> + struct clk_regmap *clk = to_clk_regmap(hw); >> + struct clk_regmap_vclk_div_data *vclk = clk_get_regmap_vclk_div_data(clk); >> + >> + return divider_determine_rate(hw, req, vclk->table, vclk->div.width, >> + vclk->flags); >> +} >> + >> +static int clk_regmap_vclk_div_set_rate(struct clk_hw *hw, unsigned long rate, >> + unsigned long parent_rate) >> +{ >> + struct clk_regmap *clk = to_clk_regmap(hw); >> + struct clk_regmap_vclk_div_data *vclk = clk_get_regmap_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 clk_regmap_vclk_div_enable(struct clk_hw *hw) >> +{ >> + struct clk_regmap *clk = to_clk_regmap(hw); >> + struct clk_regmap_vclk_div_data *vclk = clk_get_regmap_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 clk_regmap_vclk_div_disable(struct clk_hw *hw) >> +{ >> + struct clk_regmap *clk = to_clk_regmap(hw); >> + struct clk_regmap_vclk_div_data *vclk = clk_get_regmap_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 clk_regmap_vclk_div_is_enabled(struct clk_hw *hw) >> +{ >> + struct clk_regmap *clk = to_clk_regmap(hw); >> + struct clk_regmap_vclk_div_data *vclk = clk_get_regmap_vclk_div_data(clk); >> + >> + return meson_parm_read(clk->map, &vclk->enable); >> +} >> + >> +const struct clk_ops clk_regmap_vclk_div_ops = { >> + .recalc_rate = clk_regmap_vclk_div_recalc_rate, >> + .determine_rate = clk_regmap_vclk_div_determine_rate, >> + .set_rate = clk_regmap_vclk_div_set_rate, >> + .enable = clk_regmap_vclk_div_enable, >> + .disable = clk_regmap_vclk_div_disable, >> + .is_enabled = clk_regmap_vclk_div_is_enabled, >> +}; >> +EXPORT_SYMBOL_GPL(clk_regmap_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..4f25d7ad2717 >> --- /dev/null >> +++ b/drivers/clk/meson/vclk.h
Is vclk.c/h ok ? clk-vclk doesn't look pretty, but I can switch to it to keep files organized.
Neil
>> @@ -0,0 +1,51 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* >> + * Copyright (c) 2023 Neil Armstrong <neil.armstrong@linaro.org> >> + */ >> + >> +#ifndef __VCLK_H >> +#define __VCLK_H > > This is too generic. > Please add the MESON prefix like the other clock driver please. > >> + >> +#include "clk-regmap.h" >> +#include "parm.h" >> + >> +/** >> + * struct clk_regmap_vclk_data - vclk 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 clk_regmap_vclk_data { >> + struct parm enable; >> + struct parm reset; >> + u8 flags; >> +}; >> + >> +extern const struct clk_ops clk_regmap_vclk_ops; >> + >> +/** >> + * struct clk_regmap_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 clk_regmap_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 clk_regmap_vclk_div_ops; >> + >> +#endif /* __VCLK_H */ >
| |