| Message ID | 20231124-amlogic-v6-4-upstream-dsi-ccf-vim3-v9-7-95256ed139e6@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | drm/meson: add support for MIPI DSI Display | expand |
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 > @@ -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 */
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 Indeed > >> 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. Ack > >> + >> +/* 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 >> @@ -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. Ack > >> + >> +#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 */ > Thanks, Neil
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 */ >
On Mon 27 Nov 2023 at 17:14, Neil Armstrong <neil.armstrong@linaro.org> wrote: > 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. I don't have a strong opinion about it. I would have suggested vclk-div.c/h - like sclk ... but you do have gate ops in there, so ... :/ This made me realize that one does not really go without the other. It is more a coherent block, isn't it ? Would it make more sense to have these 2 merged in a single clk_ops ? It's bit late to point this out, sorry about that. I let you decide whether to merge the ops or not and which name to pick. If you keep them separated, meson_vclk_gate_ops instead of just meson_vclk_ops, to make things clear. > > 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 */ >>
On 27/11/2023 17:24, Jerome Brunet wrote: > > On Mon 27 Nov 2023 at 17:14, Neil Armstrong <neil.armstrong@linaro.org> wrote: > >> 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. > > I don't have a strong opinion about it. > I would have suggested vclk-div.c/h - like sclk ... but you do have gate > ops in there, so ... :/ > > This made me realize that one does not really go without the other. > It is more a coherent block, isn't it ? > Would it make more sense to have these 2 merged in a single clk_ops ? No those a 2 separate blocks, one is for VCLK clock input and the other is for the DIV block, and the reset isn't used in the same way so I can't merge them. > > It's bit late to point this out, sorry about that. > > I let you decide whether to merge the ops or not and which name to pick. > > If you keep them separated, meson_vclk_gate_ops instead of just > meson_vclk_ops, to make things clear. Ack > >> >> 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 */ >>> > >
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 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); + +/* 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 @@ -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 + +#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 */
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(+)