Date: Mon, 10 Jun 2024 23:10:16 +0300
From: Dmitry Rokosov <ddrokosov@...utedevices.com>
To: Jerome Brunet <jbrunet@...libre.com>
CC: <neil.armstrong@...aro.org>, <mturquette@...libre.com>,
	<sboyd@...nel.org>, <robh+dt@...nel.org>,
	<krzysztof.kozlowski+dt@...aro.org>, <khilman@...libre.com>,
	<martin.blumenstingl@...glemail.com>, <jian.hu@...ogic.com>,
	<kernel@...rdevices.ru>, <rockosov@...il.com>,
	<linux-amlogic@...ts.infradead.org>, <linux-clk@...r.kernel.org>,
	<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v3 7/7] clk: meson: a1: add Amlogic A1 CPU clock
 controller driver

On Mon, Jun 10, 2024 at 04:08:24PM +0300, Dmitry Rokosov wrote:
> On Mon, Jun 10, 2024 at 12:06:31PM +0200, Jerome Brunet wrote:
> > On Wed 15 May 2024 at 21:47, Dmitry Rokosov <ddrokosov@...utedevices.com> wrote:
> > 
> > > The CPU clock controller plays a general role in the Amlogic A1 SoC
> > > family by generating CPU clocks. As an APB slave module, it offers the
> > > capability to inherit the CPU clock from two sources: the internal fixed
> > > clock known as 'cpu fixed clock' and the external input provided by the
> > > A1 PLL clock controller, referred to as 'syspll'.
> > >
> > > It is important for the driver to handle cpu_clk rate switching
> > > effectively by transitioning to the CPU fixed clock to avoid any
> > > potential execution freezes.
> > >
> > > Signed-off-by: Dmitry Rokosov <ddrokosov@...utedevices.com>
> > > ---
> > >  drivers/clk/meson/Kconfig  |  10 ++
> > >  drivers/clk/meson/Makefile |   1 +
> > >  drivers/clk/meson/a1-cpu.c | 331 +++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 342 insertions(+)
> > >  create mode 100644 drivers/clk/meson/a1-cpu.c
> > >
> > > diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> > > index 80c4a18c83d2..148d4495eee3 100644
> > > --- a/drivers/clk/meson/Kconfig
> > > +++ b/drivers/clk/meson/Kconfig
> > > @@ -111,6 +111,16 @@ config COMMON_CLK_AXG_AUDIO
> > >  	  Support for the audio clock controller on AmLogic A113D devices,
> > >  	  aka axg, Say Y if you want audio subsystem to work.
> > >  
> > > +config COMMON_CLK_A1_CPU
> > > +	tristate "Amlogic A1 SoC CPU controller support"
> > > +	depends on ARM64
> > > +	select COMMON_CLK_MESON_REGMAP
> > > +	select COMMON_CLK_MESON_CLKC_UTILS
> > > +	help
> > > +	  Support for the CPU clock controller on Amlogic A113L based
> > > +	  device, A1 SoC Family. Say Y if you want A1 CPU clock controller
> > > +	  to work.
> > > +
> > >  config COMMON_CLK_A1_PLL
> > >  	tristate "Amlogic A1 SoC PLL controller support"
> > >  	depends on ARM64
> > > diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> > > index 4968fc7ad555..2a06eb0303d6 100644
> > > --- a/drivers/clk/meson/Makefile
> > > +++ b/drivers/clk/meson/Makefile
> > > @@ -18,6 +18,7 @@ obj-$(CONFIG_COMMON_CLK_MESON_AUDIO_RSTC) += meson-audio-rstc.o
> > >  
> > >  obj-$(CONFIG_COMMON_CLK_AXG) += axg.o axg-aoclk.o
> > >  obj-$(CONFIG_COMMON_CLK_AXG_AUDIO) += axg-audio.o
> > > +obj-$(CONFIG_COMMON_CLK_A1_CPU) += a1-cpu.o
> > >  obj-$(CONFIG_COMMON_CLK_A1_PLL) += a1-pll.o
> > >  obj-$(CONFIG_COMMON_CLK_A1_PERIPHERALS) += a1-peripherals.o
> > >  obj-$(CONFIG_COMMON_CLK_A1_AUDIO) += a1-audio.o
> > > diff --git a/drivers/clk/meson/a1-cpu.c b/drivers/clk/meson/a1-cpu.c
> > > new file mode 100644
> > > index 000000000000..a9edabeafea9
> > > --- /dev/null
> > > +++ b/drivers/clk/meson/a1-cpu.c
> > > @@ -0,0 +1,331 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Amlogic A1 SoC family CPU Clock Controller driver.
> > > + *
> > > + * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
> > > + * Author: Dmitry Rokosov <ddrokosov@...utedevices.com>
> > > + */
> > > +
> > > +#include <linux/clk.h>
> > > +#include <linux/clk-provider.h>
> > > +#include <linux/mod_devicetable.h>
> > > +#include <linux/platform_device.h>
> > > +#include "clk-regmap.h"
> > > +#include "meson-clkc-utils.h"
> > > +
> > > +#include <dt-bindings/clock/amlogic,a1-cpu-clkc.h>
> > > +
> > > +/* CPU Clock Controller register offset */
> > > +#define CPUCTRL_CLK_CTRL0	0x0
> > > +#define CPUCTRL_CLK_CTRL1	0x4
> > > +
> > > +static u32 cpu_fsource_sel_table[] = { 0, 1, 2 };
> > > +static const struct clk_parent_data cpu_fsource_sel_parents[] = {
> > > +	{ .fw_name = "xtal" },
> > > +	{ .fw_name = "fclk_div2" },
> > > +	{ .fw_name = "fclk_div3" },
> > > +};
> > > +
> > > +static struct clk_regmap cpu_fsource_sel0 = {
> > > +	.data = &(struct clk_regmap_mux_data) {
> > > +		.offset = CPUCTRL_CLK_CTRL0,
> > > +		.mask = 0x3,
> > > +		.shift = 0,
> > > +		.table = cpu_fsource_sel_table,
> > > +	},
> > > +	.hw.init = &(struct clk_init_data) {
> > > +		.name = "cpu_fsource_sel0",
> > > +		.ops = &clk_regmap_mux_ops,
> > > +		.parent_data = cpu_fsource_sel_parents,
> > > +		.num_parents = ARRAY_SIZE(cpu_fsource_sel_parents),
> > > +		.flags = CLK_SET_RATE_PARENT,
> > 
> > I don't think setting the rates of controller parents is appropriate
> > 
> > > +	},
> > > +};
> > > +
> > > +static struct clk_regmap cpu_fsource_div0 = {
> > > +	.data = &(struct clk_regmap_div_data) {
> > > +		.offset = CPUCTRL_CLK_CTRL0,
> > > +		.shift = 4,
> > > +		.width = 6,
> > > +	},
> > > +	.hw.init = &(struct clk_init_data) {
> > > +		.name = "cpu_fsource_div0",
> > > +		.ops = &clk_regmap_divider_ops,
> > > +		.parent_hws = (const struct clk_hw *[]) {
> > > +			&cpu_fsource_sel0.hw
> > > +		},
> > > +		.num_parents = 1,
> > > +		.flags = CLK_SET_RATE_PARENT,
> > > +	},
> > > +};
> > > +
> > > +static struct clk_regmap cpu_fsel0 = {
> > > +	.data = &(struct clk_regmap_mux_data) {
> > > +		.offset = CPUCTRL_CLK_CTRL0,
> > > +		.mask = 0x1,
> > > +		.shift = 2,
> > > +	},
> > > +	.hw.init = &(struct clk_init_data) {
> > > +		.name = "cpu_fsel0",
> > > +		.ops = &clk_regmap_mux_ops,
> > > +		.parent_hws = (const struct clk_hw *[]) {
> > > +			&cpu_fsource_sel0.hw,
> > > +			&cpu_fsource_div0.hw,
> > > +		},
> > > +		.num_parents = 2,
> > > +		.flags = CLK_SET_RATE_GATE | CLK_SET_RATE_PARENT,
> > > +	},
> > > +};
> > > +
> > > +static struct clk_regmap cpu_fsource_sel1 = {
> > > +	.data = &(struct clk_regmap_mux_data) {
> > > +		.offset = CPUCTRL_CLK_CTRL0,
> > > +		.mask = 0x3,
> > > +		.shift = 16,
> > > +		.table = cpu_fsource_sel_table,
> > > +	},
> > > +	.hw.init = &(struct clk_init_data) {
> > > +		.name = "cpu_fsource_sel1",
> > > +		.ops = &clk_regmap_mux_ops,
> > > +		.parent_data = cpu_fsource_sel_parents,
> > > +		.num_parents = ARRAY_SIZE(cpu_fsource_sel_parents),
> > > +		.flags = CLK_SET_RATE_PARENT,
> > > +	},
> > > +};
> > > +
> > > +static struct clk_regmap cpu_fsource_div1 = {
> > > +	.data = &(struct clk_regmap_div_data) {
> > > +		.offset = CPUCTRL_CLK_CTRL0,
> > > +		.shift = 20,
> > > +		.width = 6,
> > > +	},
> > > +	.hw.init = &(struct clk_init_data) {
> > > +		.name = "cpu_fsource_div1",
> > > +		.ops = &clk_regmap_divider_ops,
> > > +		.parent_hws = (const struct clk_hw *[]) {
> > > +			&cpu_fsource_sel1.hw
> > > +		},
> > > +		.num_parents = 1,
> > > +		.flags = CLK_SET_RATE_PARENT,
> > > +	},
> > > +};
> > > +
> > > +static struct clk_regmap cpu_fsel1 = {
> > > +	.data = &(struct clk_regmap_mux_data) {
> > > +		.offset = CPUCTRL_CLK_CTRL0,
> > > +		.mask = 0x1,
> > > +		.shift = 18,
> > > +	},
> > > +	.hw.init = &(struct clk_init_data) {
> > > +		.name = "cpu_fsel1",
> > > +		.ops = &clk_regmap_mux_ops,
> > > +		.parent_hws = (const struct clk_hw *[]) {
> > > +			&cpu_fsource_sel1.hw,
> > > +			&cpu_fsource_div1.hw,
> > > +		},
> > > +		.num_parents = 2,
> > > +		.flags = CLK_SET_RATE_GATE | CLK_SET_RATE_PARENT,
> > > +	},
> > > +};
> > > +
> > > +static struct clk_regmap cpu_fclk = {
> > > +	.data = &(struct clk_regmap_mux_data) {
> > > +		.offset = CPUCTRL_CLK_CTRL0,
> > > +		.mask = 0x1,
> > > +		.shift = 10,
> > > +	},
> > > +	.hw.init = &(struct clk_init_data) {
> > > +		.name = "cpu_fclk",
> > > +		.ops = &clk_regmap_mux_ops,
> > > +		.parent_hws = (const struct clk_hw *[]) {
> > > +			&cpu_fsel0.hw,
> > > +			&cpu_fsel1.hw,
> > > +		},
> > > +		.num_parents = 2,
> > > +		.flags = CLK_SET_RATE_PARENT,
> > > +	},
> > > +};
> > > +
> > > +static struct clk_regmap cpu_clk = {
> > > +	.data = &(struct clk_regmap_mux_data) {
> > > +		.offset = CPUCTRL_CLK_CTRL0,
> > > +		.mask = 0x1,
> > > +		.shift = 11,
> > > +	},
> > > +	.hw.init = &(struct clk_init_data) {
> > > +		.name = "cpu_clk",
> > > +		.ops = &clk_regmap_mux_ops,
> > > +		.parent_data = (const struct clk_parent_data []) {
> > > +			{ .hw = &cpu_fclk.hw },
> > > +			{ .fw_name = "sys_pll", },
> > > +		},
> > 
> > You've put CLK_SET_RATE_GATE on fixed clock path but not the SYS_PLL
> > ... that is odd. IMO there should be a bypass input clock to the sys_pll
> > with that flag.
> > 
> 
> Apologies for any confusion caused. To clarify, are you proposing the
> idea of creating an additional sys_pll_input clock object with the
> CLK_SET_RATE_PARENT property, and then using it as the parent clock for
> cpu_clk?
> 
> > > +		.num_parents = 2,
> > > +		.flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
> > > +	},
> > > +};
> > > +
> > > +/* Array of all clocks registered by this provider */
> > > +static struct clk_hw *a1_cpu_hw_clks[] = {
> > > +	[CLKID_CPU_FSOURCE_SEL0]	= &cpu_fsource_sel0.hw,
> > > +	[CLKID_CPU_FSOURCE_DIV0]	= &cpu_fsource_div0.hw,
> > > +	[CLKID_CPU_FSEL0]		= &cpu_fsel0.hw,
> > > +	[CLKID_CPU_FSOURCE_SEL1]	= &cpu_fsource_sel1.hw,
> > > +	[CLKID_CPU_FSOURCE_DIV1]	= &cpu_fsource_div1.hw,
> > > +	[CLKID_CPU_FSEL1]		= &cpu_fsel1.hw,
> > > +	[CLKID_CPU_FCLK]		= &cpu_fclk.hw,
> > > +	[CLKID_CPU_CLK]			= &cpu_clk.hw,
> > > +};
> > > +
> > > +static struct clk_regmap *const a1_cpu_regmaps[] = {
> > > +	&cpu_fsource_sel0,
> > > +	&cpu_fsource_div0,
> > > +	&cpu_fsel0,
> > > +	&cpu_fsource_sel1,
> > > +	&cpu_fsource_div1,
> > > +	&cpu_fsel1,
> > > +	&cpu_fclk,
> > > +	&cpu_clk,
> > > +};
> > > +
> > > +static struct regmap_config a1_cpu_regmap_cfg = {
> > > +	.reg_bits   = 32,
> > > +	.val_bits   = 32,
> > > +	.reg_stride = 4,
> > > +	.max_register = CPUCTRL_CLK_CTRL1,
> > > +};
> > > +
> > > +static struct meson_clk_hw_data a1_cpu_clks = {
> > > +	.hws = a1_cpu_hw_clks,
> > > +	.num = ARRAY_SIZE(a1_cpu_hw_clks),
> > > +};
> > > +
> > > +struct a1_sys_pll_nb_data {
> > > +	struct notifier_block nb;
> > > +	struct clk_hw *cpu_clk;
> > > +	struct clk_hw *cpu_fclk;
> > > +	struct clk *sys_pll;
> > > +};
> > 
> > There are number of things which are wrong with this notifier.
> > 
> > First, and foremost, this is a clock controller driver ... it should not
> > handle cpufreq policy. There is subsystem for that
> > 
> > > +
> > > +static int meson_a1_sys_pll_notifier_cb(struct notifier_block *nb,
> > > +					unsigned long event, void *data)
> > > +{
> > > +	struct a1_sys_pll_nb_data *nbd;
> > > +	int ret = 0;
> > > +
> > > +	nbd = container_of(nb, struct a1_sys_pll_nb_data, nb);
> > > +
> > > +	switch (event) {
> > > +	case PRE_RATE_CHANGE:
> > > +		/*
> > > +		 * Clock sys_pll will be changed to feed cpu_clk,
> > > +		 * configure cpu_clk to use cpu_fclk fixed clock.
> > > +		 */
> > > +		ret = clk_hw_set_parent(nbd->cpu_clk, nbd->cpu_fclk);
> > 
> > 
> > This jumps to whatever was the last frequency below 768MHz ... that does
> > not seems deterministic or safe.
> 
> Ah, that's an aspect I hadn't considered. You make a valid point. So,
> this implies that the g12a clock driver could potentially encounter the
> same issue, correct?
> 
> > > +
> > > +		/* Wait for clock propagation */
> > > +		if (!ret)
> > > +			udelay(100);
> > > +
> > > +		break;
> > > +
> > > +	case POST_RATE_CHANGE:
> > > +		 /*
> > > +		  * Clock sys_pll rate has ben calculated,
> > > +		  * switch back cpu_clk to sys_pll
> > > +		  */
> > > +		ret = clk_set_parent(nbd->cpu_clk->clk, nbd->sys_pll);
> > 
> > So whenever sys_pll changes, even if was not used by the CPU at that
> > time, this will change back to the sys_pll. Again, that seems fragile
> > 
> 
> From what I comprehend, only the GEN clock is capable of using sys_pll
> as its parent clock. The GEN clock seems more comparable to a diagnostic
> clock, implying that when utilized, it should be done with full
> awareness and control over its operations.
> 
> > > +
> > > +		/* Wait for clock propagation */
> > > +		if (!ret)
> > > +			udelay(100);
> > > +		break;
> > > +
> > > +	default:
> > > +		pr_warn("Unknown event %lu for sys_pll notifier\n", event);
> > > +		break;
> > > +	}
> > > +
> > > +	return notifier_from_errno(ret);
> > > +}
> > > +
> > > +static struct a1_sys_pll_nb_data a1_sys_pll_nb_data = {
> > > +	.nb.notifier_call = meson_a1_sys_pll_notifier_cb,
> > > +	.cpu_clk = &cpu_clk.hw,
> > > +	.cpu_fclk = &cpu_fclk.hw,
> > > +};
> > > +
> > > +static int meson_a1_dvfs_setup(struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct clk *sys_pll;
> > > +	int ret;
> > > +
> > > +	/* Setup clock notifier for sys_pll clk */
> > > +	sys_pll = devm_clk_get(dev, "sys_pll");
> > > +	if (IS_ERR(sys_pll))
> > > +		return dev_err_probe(dev, PTR_ERR(sys_pll),
> > > +				     "can't get sys_pll as notifier clock\n");
> > > +
> > > +	a1_sys_pll_nb_data.sys_pll = sys_pll;
> > > +	ret = devm_clk_notifier_register(dev, sys_pll,
> > > +					 &a1_sys_pll_nb_data.nb);
> > > +	if (ret)
> > > +		return dev_err_probe(dev, ret,
> > > +				     "can't register sys_pll notifier\n");
> > > +
> > > +	return ret;
> > > +}
> > 
> > I don't think these notifiers are appropriate to handle CPU frequency
> > change. Cpufreq has a .target_intermediate() callback that seems more
> > appropriate to switch the CPU to a safe clock while relocking a PLL.
> > 
> > You should have a look at it and probably at the imx-cpufreq-dt.c which
> > improves on cpufreq-dt.c to handle platform quirks
> > 
> 
> I believed that the same approach was employed with the g12a clock,
> which uses a sys_pll <-> cpu fixed clock transition to ensure stable CPU
> clocking. Am I overlooking something? Or does the g12a cpu clock
> maintain a fixed frequency, thus indicating it is not fragile?
> 
> [...]

Based on your suggestion, I explored the imx-cpufreq-dt driver and it
does seem like a more suitable place to implement CPU clock switching.
Thank you for pointing that out!

However, from my understanding, it appears that we also need to redesign
the g12a clock driver's CPU clock notifier. I would love to hear your
thoughts on this. It seems like a necessary step to ensure a
comprehensive solution.

-- 
Thank you,
Dmitry

Powered by blists - more mailing lists