From: Dmitry Rokosov <ddrokosov@sberdevices.ru>
To: Yu Tu <yu.tu@amlogic.com>
Cc: <linux-clk@vger.kernel.org>,
<linux-arm-kernel@lists.infradead.org>,
<linux-amlogic@lists.infradead.org>,
<linux-kernel@vger.kernel.org>, <devicetree@vger.kernel.org>,
Rob Herring <robh+dt@kernel.org>,
Neil Armstrong <neil.armstrong@linaro.org>,
Jerome Brunet <jbrunet@baylibre.com>,
Kevin Hilman <khilman@baylibre.com>,
Michael Turquette <mturquette@baylibre.com>,
Stephen Boyd <sboyd@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
<kelvin.zhang@amlogic.com>, <qi.duan@amlogic.com>
Subject: Re: [PATCH V7 4/4] clk: meson: s4: add support for Amlogic S4 SoC peripheral clock controller
Date: Thu, 27 Apr 2023 11:42:52 +0300 [thread overview]
Message-ID: <20230427084252.yroo3lcu3aapid6v@CAB-WSD-L081021> (raw)
In-Reply-To: <6cc4c3f2-4df5-6456-fe65-71aa4a13c866@amlogic.com>
On Thu, Apr 27, 2023 at 04:15:28PM +0800, Yu Tu wrote:
>
>
> On 2023/4/26 19:05, Dmitry Rokosov wrote:
> > [Some people who received this message don't often get email from ddrokosov@sberdevices.ru. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> >
> > [ EXTERNAL EMAIL ]
> >
> > On Mon, Apr 17, 2023 at 02:50:05PM +0800, Yu Tu wrote:
> > > Add the peripherals clock controller driver in the s4 SoC family.
> > >
> > > Signed-off-by: Yu Tu <yu.tu@amlogic.com>
> > > ---
> > > drivers/clk/meson/Kconfig | 12 +
> > > drivers/clk/meson/Makefile | 1 +
> > > drivers/clk/meson/s4-peripherals.c | 3814 ++++++++++++++++++++++++++++
> > > drivers/clk/meson/s4-peripherals.h | 217 ++
> > > 4 files changed, 4044 insertions(+)
> > > create mode 100644 drivers/clk/meson/s4-peripherals.c
> > > create mode 100644 drivers/clk/meson/s4-peripherals.h
> > >
> > > diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> > > index a663c90a3f3b..a6eb9fa15c74 100644
> > > --- a/drivers/clk/meson/Kconfig
> > > +++ b/drivers/clk/meson/Kconfig
> > > @@ -128,4 +128,16 @@ config COMMON_CLK_S4_PLL
> > > aka s4. Amlogic S805X2 and S905Y4 devices include AQ222 and AQ229.
> > > Say Y if you want the board to work, because plls are the parent of most
> > > peripherals.
> > > +
> > > +config COMMON_CLK_S4
> > > + tristate "S4 SoC Peripherals clock controllers support"
> > > + depends on ARM64
> > > + default y
> > > + select COMMON_CLK_MESON_REGMAP
> > > + select COMMON_CLK_MESON_DUALDIV
> > > + select COMMON_CLK_MESON_VID_PLL_DIV
> > > + help
> > > + Support for the Peripherals clock controller on Amlogic S805X2 and S905Y4
> > > + devices, aka s4. Amlogic S805X2 and S905Y4 devices include AQ222 and AQ229.
> > > + Say Y if you want peripherals to work.
> > > endmenu
> > > diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> > > index 376f49cc13f1..c9130afccb48 100644
> > > --- a/drivers/clk/meson/Makefile
> > > +++ b/drivers/clk/meson/Makefile
> > > @@ -20,3 +20,4 @@ obj-$(CONFIG_COMMON_CLK_GXBB) += gxbb.o gxbb-aoclk.o
> > > obj-$(CONFIG_COMMON_CLK_G12A) += g12a.o g12a-aoclk.o
> > > obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o meson8-ddr.o
> > > obj-$(CONFIG_COMMON_CLK_S4_PLL) += s4-pll.o
> > > +obj-$(CONFIG_COMMON_CLK_S4) += s4-peripherals.o
> >
> > [...]
>
> ?
>
I am using the above-mentioned symbols to indicate that I have removed
a portion of the previous email that is not relevant to my response.
> >
> > > +static struct clk_regmap s4_ceca_32k_clkin = {
> > > + .data = &(struct clk_regmap_gate_data){
> > > + .offset = CLKCTRL_CECA_CTRL0,
> > > + .bit_idx = 31,
> > > + },
> > > + .hw.init = &(struct clk_init_data) {
> > > + .name = "ceca_32k_clkin",
> > > + .ops = &clk_regmap_gate_ops,
> > > + .parent_data = (const struct clk_parent_data []) {
> > > + { .fw_name = "xtal", }
> > > + },
> > > + .num_parents = 1,
> > > + },
> > > +};
> > > +
> > > +static struct clk_regmap s4_ceca_32k_div = {
> > > + .data = &(struct meson_clk_dualdiv_data){
> > > + .n1 = {
> > > + .reg_off = CLKCTRL_CECA_CTRL0,
> > > + .shift = 0,
> > > + .width = 12,
> > > + },
> > > + .n2 = {
> > > + .reg_off = CLKCTRL_CECA_CTRL0,
> > > + .shift = 12,
> > > + .width = 12,
> > > + },
> > > + .m1 = {
> > > + .reg_off = CLKCTRL_CECA_CTRL1,
> > > + .shift = 0,
> > > + .width = 12,
> > > + },
> > > + .m2 = {
> > > + .reg_off = CLKCTRL_CECA_CTRL1,
> > > + .shift = 12,
> > > + .width = 12,
> > > + },
> > > + .dual = {
> > > + .reg_off = CLKCTRL_CECA_CTRL0,
> > > + .shift = 28,
> > > + .width = 1,
> > > + },
> > > + .table = s4_32k_div_table,
> > > + },
> > > + .hw.init = &(struct clk_init_data){
> > > + .name = "ceca_32k_div",
> > > + .ops = &meson_clk_dualdiv_ops,
> > > + .parent_hws = (const struct clk_hw *[]) {
> > > + &s4_ceca_32k_clkin.hw
> > > + },
> > > + .num_parents = 1,
> > > + },
> > > +};
> > > +
> > > +static struct clk_regmap s4_ceca_32k_sel_pre = {
> > > + .data = &(struct clk_regmap_mux_data) {
> > > + .offset = CLKCTRL_CECA_CTRL1,
> > > + .mask = 0x1,
> > > + .shift = 24,
> > > + .flags = CLK_MUX_ROUND_CLOSEST,
> > > + },
> > > + .hw.init = &(struct clk_init_data){
> > > + .name = "ceca_32k_sel_pre",
> > > + .ops = &clk_regmap_mux_ops,
> > > + .parent_hws = (const struct clk_hw *[]) {
> > > + &s4_ceca_32k_div.hw,
> > > + &s4_ceca_32k_clkin.hw
> > > + },
> > > + .num_parents = 2,
> > > + .flags = CLK_SET_RATE_PARENT,
> > > + },
> > > +};
> > > +
> > > +static struct clk_regmap s4_ceca_32k_sel = {
> > > + .data = &(struct clk_regmap_mux_data) {
> > > + .offset = CLKCTRL_CECA_CTRL1,
> > > + .mask = 0x1,
> > > + .shift = 31,
> > > + .flags = CLK_MUX_ROUND_CLOSEST,
> > > + },
> > > + .hw.init = &(struct clk_init_data){
> > > + .name = "ceca_32k_sel",
> > > + .ops = &clk_regmap_mux_ops,
> > > + .parent_hws = (const struct clk_hw *[]) {
> > > + &s4_ceca_32k_sel_pre.hw,
> > > + &s4_rtc_clk.hw
> > > + },
> > > + .num_parents = 2,
> > > + .flags = CLK_SET_RATE_PARENT,
> >
> > In my opinion, all clocks that can inherit from a more accurate RTC clock
> > should be marked with the CLK_SET_RATE_NO_REPARENT flag.
> > This is necessary because in certain situations, it may be required to
> > freeze their parent. The setup of these clocks' parent should be located
> > on the device tree's side.
>
> We don't need to freeze parent,in a real project.
>
In my opinion, an RTC clock is more accurate and its rate can be
adjusted to a specific value. If I choose the RTC clock as the parent,
I don't want to run the rate auto-propagation for the child clock.
Otherwise, the parent can be changed to fit a specific frequency.
> >
> > [...]
> >
> > > +
> > > +/*
> > > + * gen clk is designed for debug/monitor some internal clock quality. Some of the
> > > + * corresponding clock sources are not described in the clock tree and internal clock
> > > + * for debug, so they are skipped.
> > > + */
> > > +static u32 s4_gen_clk_mux_table[] = { 0, 4, 5, 7, 19, 21, 22,
> > > + 23, 24, 25, 26, 27, 28 };
> > > +static const struct clk_parent_data s4_gen_clk_parent_data[] = {
> > > + { .fw_name = "xtal", },
> > > + { .hw = &s4_vid_pll.hw },
> > > + { .fw_name = "gp0_pll", },
> > > + { .fw_name = "hifi_pll", },
> > > + { .fw_name = "fclk_div2", },
> > > + { .fw_name = "fclk_div3", },
> > > + { .fw_name = "fclk_div4", },
> > > + { .fw_name = "fclk_div5", },
> > > + { .fw_name = "fclk_div7", },
> > > + { .fw_name = "mpll0", },
> > > + { .fw_name = "mpll1", },
> > > + { .fw_name = "mpll2", },
> > > + { .fw_name = "mpll3", },
> > > +};
> > > +
> > > +static struct clk_regmap s4_gen_clk_sel = {
> > > + .data = &(struct clk_regmap_mux_data){
> > > + .offset = CLKCTRL_GEN_CLK_CTRL,
> > > + .mask = 0x1f,
> > > + .shift = 12,
> > > + .table = s4_gen_clk_mux_table,
> > > + },
> > > + .hw.init = &(struct clk_init_data){
> > > + .name = "gen_clk_sel",
> > > + .ops = &clk_regmap_mux_ops,
> > > + .parent_data = s4_gen_clk_parent_data,
> > > + .num_parents = ARRAY_SIZE(s4_gen_clk_parent_data),
> >
> > I think, the gen_clk selector should be marked with the
> > CLK_SET_RATE_NO_REPARENT flag. This is because the GEN clock can be
> > connected to an external pad and may be set up directly from the
> > device tree.
>
> This is used by the debug table clock and is not connected externally.
>
Sorry, I do not currently have the Y4 datasheet available.
Previously, the GEN_CLK was muxed to the GPIO on specific boards,
depending on the board layout, and I referred to this situation as
'connect to external pad'.
Therefore, I believe it would be more efficient to have the ability to
freeze the GEN_CLK parent and rate.
> >
> > > + },
> > > +};
> > > +
> > > +static struct clk_regmap s4_gen_clk_div = {
> > > + .data = &(struct clk_regmap_div_data){
> > > + .offset = CLKCTRL_GEN_CLK_CTRL,
> > > + .shift = 0,
> > > + .width = 11,
> > > + },
> > > + .hw.init = &(struct clk_init_data){
> > > + .name = "gen_clk_div",
> > > + .ops = &clk_regmap_divider_ops,
> > > + .parent_hws = (const struct clk_hw *[]) {
> > > + &s4_gen_clk_sel.hw
> > > + },
> > > + .num_parents = 1,
> > > + .flags = CLK_SET_RATE_PARENT,
> > > + },
> > > +};
> > > +
> > > +static struct clk_regmap s4_gen_clk = {
> > > + .data = &(struct clk_regmap_gate_data){
> > > + .offset = CLKCTRL_GEN_CLK_CTRL,
> > > + .bit_idx = 11,
> > > + },
> > > + .hw.init = &(struct clk_init_data) {
> > > + .name = "gen_clk",
> > > + .ops = &clk_regmap_gate_ops,
> > > + .parent_hws = (const struct clk_hw *[]) {
> > > + &s4_gen_clk_div.hw
> > > + },
> > > + .num_parents = 1,
> > > + .flags = CLK_SET_RATE_PARENT | CLK_GET_RATE_NOCACHE,
> > > + },
> > > +};
> > > +
> >
> > [...]
> >
> > --
> > Thank you,
> > Dmitry
--
Thank you,
Dmitry
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
next prev parent reply other threads:[~2023-04-27 8:43 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-17 6:50 [PATCH V7 0/4] Add S4 SoC PLL and Peripheral clock controller Yu Tu
2023-04-17 6:50 ` [PATCH V7 1/4] dt-bindings: clock: document Amlogic S4 SoC PLL " Yu Tu
2023-04-21 17:49 ` Rob Herring
2023-04-17 6:50 ` [PATCH V7 2/4] dt-bindings: clock: document Amlogic S4 SoC peripherals " Yu Tu
2023-04-21 17:50 ` Rob Herring
2023-04-26 10:49 ` Dmitry Rokosov
2023-04-27 8:03 ` Yu Tu
2023-04-27 8:52 ` Dmitry Rokosov
2023-04-27 11:36 ` Neil Armstrong
2023-05-04 8:47 ` Yu Tu
2023-05-04 8:59 ` Yu Tu
2023-04-17 6:50 ` [PATCH V7 3/4] clk: meson: S4: add support for Amlogic S4 SoC PLL clock driver Yu Tu
2023-04-26 11:13 ` Dmitry Rokosov
2023-04-27 8:38 ` Yu Tu
2023-04-27 9:11 ` Dmitry Rokosov
2023-05-04 9:19 ` Yu Tu
2023-04-17 6:50 ` [PATCH V7 4/4] clk: meson: s4: add support for Amlogic S4 SoC peripheral clock controller Yu Tu
2023-04-26 11:05 ` Dmitry Rokosov
2023-04-27 8:15 ` Yu Tu
2023-04-27 8:42 ` Dmitry Rokosov [this message]
2023-05-01 17:37 ` Jerome Brunet
2023-05-04 8:43 ` Yu Tu
2023-04-23 21:33 ` [PATCH V7 0/4] Add S4 SoC PLL and Peripheral " Martin Blumenstingl
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230427084252.yroo3lcu3aapid6v@CAB-WSD-L081021 \
--to=ddrokosov@sberdevices.ru \
--cc=devicetree@vger.kernel.org \
--cc=jbrunet@baylibre.com \
--cc=kelvin.zhang@amlogic.com \
--cc=khilman@baylibre.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-amlogic@lists.infradead.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=martin.blumenstingl@googlemail.com \
--cc=mturquette@baylibre.com \
--cc=neil.armstrong@linaro.org \
--cc=qi.duan@amlogic.com \
--cc=robh+dt@kernel.org \
--cc=sboyd@kernel.org \
--cc=yu.tu@amlogic.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).