Received-SPF: pass (google.com: domain of linux-kernel+bounces-208214-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 10 Jun 2024 16:08:24 +0300
From: Dmitry Rokosov <ddrokosov@salutedevices.com>
To: Jerome Brunet <jbrunet@baylibre.com>
CC: <neil.armstrong@linaro.org>, <mturquette@baylibre.com>,
	<sboyd@kernel.org>, <robh+dt@kernel.org>,
	<krzysztof.kozlowski+dt@linaro.org>, <khilman@baylibre.com>,
	<martin.blumenstingl@googlemail.com>, <jian.hu@amlogic.com>,
	<kernel@sberdevices.ru>, <rockosov@gmail.com>,
	<linux-amlogic@lists.infradead.org>, <linux-clk@vger.kernel.org>,
	<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v3 7/7] clk: meson: a1: add Amlogic A1 CPU clock
 controller driver
Message-ID: <20240610130824.dt7matrj4tespizl@CAB-WSD-L081021>
References: <20240515185103.20256-1-ddrokosov@salutedevices.com>
 <20240515185103.20256-8-ddrokosov@salutedevices.com>
 <1jmsntp0wo.fsf@starbuckisacylon.baylibre.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <1jmsntp0wo.fsf@starbuckisacylon.baylibre.com>
User-Agent: NeoMutt/20220415

On Mon, Jun 10, 2024 at 12:06:31PM +0200, Jerome Brunet wrote:
> On Wed 15 May 2024 at 21:47, Dmitry Rokosov <ddrokosov@salutedevices.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@salutedevices.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@salutedevices.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?

[...]

-- 
Thank you,
Dmitry