[v1,6/6] clk: meson: a1: add Amlogic A1 CPU clock controller driver

Commit Message

Dmitry Rokosov March 29, 2024, 8:58 p.m. UTC

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 <[e-mail]>
---
 drivers/clk/meson/Kconfig  |  10 ++
 drivers/clk/meson/Makefile |   1 +
 drivers/clk/meson/a1-cpu.c | 324 +++++++++++++++++++++++++++++++++++++
 drivers/clk/meson/a1-cpu.h |  16 ++
 4 files changed, 351 insertions(+)
 create mode 100644 drivers/clk/meson/a1-cpu.c
 create mode 100644 drivers/clk/meson/a1-cpu.h

Comments

Martin Blumenstingl March 31, 2024, 9:40 p.m. UTC | #1

Hi Dmitry,

On Fri, Mar 29, 2024 at 9:59 PM Dmitry Rokosov
<[e-mail]> wrote:
[...]
> +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,
Have you considered the CLK_SET_RATE_GATE flag for &cpu_fsel0.hw and
&cpu_fsel1.hw and then dropping the clock notifier below?
We use that approach with the Mali GPU clock on other SoCs, see for
example commit 8daeaea99caa ("clk: meson: meson8b: make the CCF use
the glitch-free mali mux").
It may differ from what Amlogic does in their BSP, but I don't think
that there's any harm (if it works in general) because CCF (common
clock framework) will set all clocks in the "inactive" tree and then
as a last step just change the mux (&cpu_fclk.hw). So at no point in
time will we get any other rate than a) the original CPU clock rate
before the rate change b) the new desired CPU clock rate. This is
because we have two symmetric clock trees.


Best regards,
Martin

Dmitry Rokosov April 1, 2024, 5:12 p.m. UTC | #2

Hello Martin,

Thank you for quick response. Please find my thoughts below.

On Sun, Mar 31, 2024 at 11:40:13PM +0200, Martin Blumenstingl wrote:
> Hi Dmitry,
> 
> On Fri, Mar 29, 2024 at 9:59 PM Dmitry Rokosov
> <[e-mail]> wrote:
> [...]
> > +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,
> Have you considered the CLK_SET_RATE_GATE flag for &cpu_fsel0.hw and
> &cpu_fsel1.hw and then dropping the clock notifier below?
> We use that approach with the Mali GPU clock on other SoCs, see for
> example commit 8daeaea99caa ("clk: meson: meson8b: make the CCF use
> the glitch-free mali mux").
> It may differ from what Amlogic does in their BSP,

Amlogic in their BSP takes a different approach, which is slightly
different from mine. They cleverly change the parent of cpu_clk directly
by forking the cpufreq driver to a custom version. I must admit, it's
quite an "interesting and amazing" idea :) but it's not architecturally
correct totally.

> but I don't think
> that there's any harm (if it works in general) because CCF (common
> clock framework) will set all clocks in the "inactive" tree and then
> as a last step just change the mux (&cpu_fclk.hw). So at no point in
> time will we get any other rate than a) the original CPU clock rate
> before the rate change b) the new desired CPU clock rate. This is
> because we have two symmetric clock trees.

Now, let's dive into the specifics of the issue we're facing. I've
examined the CLK_SET_RATE_GATE flag, which, to my understanding, blocks
rate changes for the entire clock chain. However, in this particular
situation, it doesn't provide the solution we need.

Here's the problem we're dealing with:

1) The CPU clock can have the following frequency points:

  available frequency steps:  128 MHz, 256 MHz, 512 MHz, 768 MHz, 1.01 GHz, 1.20 GHz

When we run the cpupower, we get the following information:
# cpupower -c 0,1 frequency-info
analyzing CPU 0:
  driver: cpufreq-dt
  CPUs which run at the same hardware frequency: 0 1
  CPUs which need to have their frequency coordinated by software: 0 1
  maximum transition latency: 50.0 us
  hardware limits: 128 MHz - 1.20 GHz
  available frequency steps:  128 MHz, 256 MHz, 512 MHz, 768 MHz, 1.01 GHz, 1.20 GHz
  available cpufreq governors: conservative ondemand userspace performance schedutil
  current policy: frequency should be within 128 MHz and 128 MHz.
                  The governor "schedutil" may decide which speed to use
                  within this range.
  current CPU frequency: 128 MHz (asserted by call to hardware)
analyzing CPU 1:
  driver: cpufreq-dt
  CPUs which run at the same hardware frequency: 0 1
  CPUs which need to have their frequency coordinated by software: 0 1
  maximum transition latency: 50.0 us
  hardware limits: 128 MHz - 1.20 GHz
  available frequency steps:  128 MHz, 256 MHz, 512 MHz, 768 MHz, 1.01 GHz, 1.20 GHz
  available cpufreq governors: conservative ondemand userspace performance schedutil
  current policy: frequency should be within 128 MHz and 128 MHz.
                  The governor "schedutil" may decide which speed to use
                  within this range.
  current CPU frequency: 128 MHz (asserted by call to hardware)

2) For the frequency points 128 MHz, 256 MHz, and 512 MHz, the CPU fixed
clock should be used. Fortunately, we don't encounter any freeze
problems when we attempt to change its rate at these frequencies.

3) However, for the frequency points 768 MHz, 1.01 GHz, and 1.20 GHz,
the sys_pll is used as the clock source because it's a faster option.
Now, let's imagine that we want to change the CPU clock from 768 MHz to
1.01 GHz. Unfortunately, it's not possible due to the broken sys_pll,
and any execution attempts will result in a hang.

4) As you can observe, in this case, we actually don't need to lock the
rate for the sys_pll chain. We want to change the rate instead. Hence,
I'm not aware of any other method to achieve this except by switching
the cpu_clk parent to a stable clock using clock notifier block.
Interestingly, I've noticed a similar approach in the CPU clock drivers
of Rockchip, Qualcomm, and Mediatek.

Jerome Brunet April 2, 2024, 9:27 a.m. UTC | #3

On Mon 01 Apr 2024 at 20:12, Dmitry Rokosov <[e-mail]> wrote:

> Hello Martin,
>
> Thank you for quick response. Please find my thoughts below.
>
> On Sun, Mar 31, 2024 at 11:40:13PM +0200, Martin Blumenstingl wrote:
>> Hi Dmitry,
>> 
>> On Fri, Mar 29, 2024 at 9:59 PM Dmitry Rokosov
>> <[e-mail]> wrote:
>> [...]
>> > +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,
>> Have you considered the CLK_SET_RATE_GATE flag for &cpu_fsel0.hw and
>> &cpu_fsel1.hw and then dropping the clock notifier below?
>> We use that approach with the Mali GPU clock on other SoCs, see for
>> example commit 8daeaea99caa ("clk: meson: meson8b: make the CCF use
>> the glitch-free mali mux").
>> It may differ from what Amlogic does in their BSP,
>
> Amlogic in their BSP takes a different approach, which is slightly
> different from mine. They cleverly change the parent of cpu_clk directly
> by forking the cpufreq driver to a custom version. I must admit, it's
> quite an "interesting and amazing" idea :) but it's not architecturally
> correct totally.

I disagree. Martin's suggestion is correct for the fsel part which is
symetric.

>
>> but I don't think
>> that there's any harm (if it works in general) because CCF (common
>> clock framework) will set all clocks in the "inactive" tree and then
>> as a last step just change the mux (&cpu_fclk.hw). So at no point in
>> time will we get any other rate than a) the original CPU clock rate
>> before the rate change b) the new desired CPU clock rate. This is
>> because we have two symmetric clock trees.
>
> Now, let's dive into the specifics of the issue we're facing. I've
> examined the CLK_SET_RATE_GATE flag, which, to my understanding, blocks
> rate changes for the entire clock chain. However, in this particular
> situation, it doesn't provide the solution we need.
>
> Here's the problem we're dealing with:
>
> 1) The CPU clock can have the following frequency points:
>
>   available frequency steps:  128 MHz, 256 MHz, 512 MHz, 768 MHz, 1.01 GHz, 1.20 GHz
>
> When we run the cpupower, we get the following information:
> # cpupower -c 0,1 frequency-info
> analyzing CPU 0:
>   driver: cpufreq-dt
>   CPUs which run at the same hardware frequency: 0 1
>   CPUs which need to have their frequency coordinated by software: 0 1
>   maximum transition latency: 50.0 us
>   hardware limits: 128 MHz - 1.20 GHz
>   available frequency steps:  128 MHz, 256 MHz, 512 MHz, 768 MHz, 1.01 GHz, 1.20 GHz
>   available cpufreq governors: conservative ondemand userspace performance schedutil
>   current policy: frequency should be within 128 MHz and 128 MHz.
>                   The governor "schedutil" may decide which speed to use
>                   within this range.
>   current CPU frequency: 128 MHz (asserted by call to hardware)
> analyzing CPU 1:
>   driver: cpufreq-dt
>   CPUs which run at the same hardware frequency: 0 1
>   CPUs which need to have their frequency coordinated by software: 0 1
>   maximum transition latency: 50.0 us
>   hardware limits: 128 MHz - 1.20 GHz
>   available frequency steps:  128 MHz, 256 MHz, 512 MHz, 768 MHz, 1.01 GHz, 1.20 GHz
>   available cpufreq governors: conservative ondemand userspace performance schedutil
>   current policy: frequency should be within 128 MHz and 128 MHz.
>                   The governor "schedutil" may decide which speed to use
>                   within this range.
>   current CPU frequency: 128 MHz (asserted by call to hardware)
>
> 2) For the frequency points 128 MHz, 256 MHz, and 512 MHz, the CPU fixed
> clock should be used.

Apparently, you are relying on the SYS PLL lowest possible rate to
enfore this contraint, which I suppose is 24 * 32 = 768MHz. It would be
nice to clearly say so.

> Fortunately, we don't encounter any freeze
> problems when we attempt to change its rate at these frequencies.

That does not sound very solid ...

>
> 3) However, for the frequency points 768 MHz, 1.01 GHz, and 1.20 GHz,
> the sys_pll is used as the clock source because it's a faster option.
> Now, let's imagine that we want to change the CPU clock from 768 MHz to
> 1.01 GHz. Unfortunately, it's not possible due to the broken sys_pll,
> and any execution attempts will result in a hang.

... Because PLL needs to relock, it is going to be off for a while. That
is not "broken", unless there is something else ?

>
> 4) As you can observe, in this case, we actually don't need to lock the
> rate for the sys_pll chain.

In which case ? I'm lost.

> We want to change the rate instead.

... How are you going to do that without relocking the PLL ?

> Hence,
> I'm not aware of any other method to achieve this except by switching
> the cpu_clk parent to a stable clock using clock notifier block.
> Interestingly, I've noticed a similar approach in the CPU clock drivers
> of Rockchip, Qualcomm, and Mediatek.

There is an example of syspll notifier in the g12 clock controller.
You should have a look at it

Jerome Brunet April 2, 2024, 9:35 a.m. UTC | #4

On Fri 29 Mar 2024 at 23:58, Dmitry Rokosov <[e-mail]> 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 <[e-mail]>
> ---
>  drivers/clk/meson/Kconfig  |  10 ++
>  drivers/clk/meson/Makefile |   1 +
>  drivers/clk/meson/a1-cpu.c | 324 +++++++++++++++++++++++++++++++++++++
>  drivers/clk/meson/a1-cpu.h |  16 ++
>  4 files changed, 351 insertions(+)
>  create mode 100644 drivers/clk/meson/a1-cpu.c
>  create mode 100644 drivers/clk/meson/a1-cpu.h
>
> 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..5f5d8ae112e5
> --- /dev/null
> +++ b/drivers/clk/meson/a1-cpu.c
> @@ -0,0 +1,324 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Amlogic A1 SoC family CPU Clock Controller driver.
> + *
> + * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
> + * Author: Dmitry Rokosov <[e-mail]>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include "a1-cpu.h"
> +#include "clk-regmap.h"
> +#include "meson-clkc-utils.h"
> +
> +#include <dt-bindings/clock/amlogic,a1-cpu-clkc.h>
> +
> +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,
> +	},
> +};
> +
> +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_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_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", },
> +		},
> +		.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_cpu_clk_nb_data {
> +	const struct clk_ops *mux_ops;

That's fishy ...

> +	struct clk_hw *cpu_clk;
> +	struct notifier_block nb;
> +	u8 parent;
> +};
> +
> +#define MESON_A1_CPU_CLK_GET_PARENT(nbd) \
> +	((nbd)->mux_ops->get_parent((nbd)->cpu_clk))
> +#define MESON_A1_CPU_CLK_SET_PARENT(nbd, index) \
> +	((nbd)->mux_ops->set_parent((nbd)->cpu_clk, index))

... Directly going for the mux ops ??!?? No way !

We have a framework to handle the clocks, the whole point is to use it,
not bypass it ! 

> +
> +static int meson_a1_cpu_clk_notifier_cb(struct notifier_block *nb,
> +					unsigned long event, void *data)
> +{
> +	struct a1_cpu_clk_nb_data *nbd;
> +	int ret = 0;
> +
> +	nbd = container_of(nb, struct a1_cpu_clk_nb_data, nb);
> +
> +	switch (event) {
> +	case PRE_RATE_CHANGE:
> +		nbd->parent = MESON_A1_CPU_CLK_GET_PARENT(nbd);
> +		/* Fallback to the CPU fixed clock */
> +		ret = MESON_A1_CPU_CLK_SET_PARENT(nbd, 0);
> +		/* Wait for clock propagation */
> +		udelay(100);
> +		break;
> +
> +	case POST_RATE_CHANGE:
> +	case ABORT_RATE_CHANGE:
> +		/* Back to the original parent clock */
> +		ret = MESON_A1_CPU_CLK_SET_PARENT(nbd, nbd->parent);
> +		/* Wait for clock propagation */
> +		udelay(100);
> +		break;
> +
> +	default:
> +		pr_warn("Unknown event %lu for %s notifier block\n",
> +			event, clk_hw_get_name(nbd->cpu_clk));
> +		break;
> +	}
> +
> +	return notifier_from_errno(ret);
> +}
> +
> +static struct a1_cpu_clk_nb_data a1_cpu_clk_nb_data = {
> +	.mux_ops = &clk_regmap_mux_ops,
> +	.cpu_clk = &cpu_clk.hw,
> +	.nb.notifier_call = meson_a1_cpu_clk_notifier_cb,
> +};
> +
> +static int meson_a1_dvfs_setup(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct clk *notifier_clk;
> +	int ret;
> +
> +	/* Setup clock notifier for cpu_clk */
> +	notifier_clk = devm_clk_hw_get_clk(dev, &cpu_clk.hw, "dvfs");
> +	if (IS_ERR(notifier_clk))
> +		return dev_err_probe(dev, PTR_ERR(notifier_clk),
> +				     "can't get cpu_clk as notifier clock\n");
> +
> +	ret = devm_clk_notifier_register(dev, notifier_clk,
> +					 &a1_cpu_clk_nb_data.nb);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "can't register cpu_clk notifier\n");
> +
> +	return ret;
> +}
> +
> +static int meson_a1_cpu_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	void __iomem *base;
> +	struct regmap *map;
> +	int clkid, i, err;
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return dev_err_probe(dev, PTR_ERR(base),
> +				     "can't ioremap resource\n");
> +
> +	map = devm_regmap_init_mmio(dev, base, &a1_cpu_regmap_cfg);
> +	if (IS_ERR(map))
> +		return dev_err_probe(dev, PTR_ERR(map),
> +				     "can't init regmap mmio region\n");
> +
> +	/* Populate regmap for the regmap backed clocks */
> +	for (i = 0; i < ARRAY_SIZE(a1_cpu_regmaps); i++)
> +		a1_cpu_regmaps[i]->map = map;
> +
> +	for (clkid = 0; clkid < a1_cpu_clks.num; clkid++) {
> +		err = devm_clk_hw_register(dev, a1_cpu_clks.hws[clkid]);
> +		if (err)
> +			return dev_err_probe(dev, err,
> +					     "clock[%d] registration failed\n",
> +					     clkid);
> +	}
> +
> +	err = devm_of_clk_add_hw_provider(dev, meson_clk_hw_get, &a1_cpu_clks);
> +	if (err)
> +		return dev_err_probe(dev, err, "can't add clk hw provider\n");

I wonder if there is a window of opportunity to poke the syspll without
your notifier here. That being said, the situation would be similar on g12.

> +
> +	return meson_a1_dvfs_setup(pdev);



> +}
> +
> +static const struct of_device_id a1_cpu_clkc_match_table[] = {
> +	{ .compatible = "amlogic,a1-cpu-clkc", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, a1_cpu_clkc_match_table);
> +
> +static struct platform_driver a1_cpu_clkc_driver = {
> +	.probe = meson_a1_cpu_probe,
> +	.driver = {
> +		.name = "a1-cpu-clkc",
> +		.of_match_table = a1_cpu_clkc_match_table,
> +	},
> +};
> +
> +module_platform_driver(a1_cpu_clkc_driver);
> +MODULE_AUTHOR("Dmitry Rokosov <[e-mail]>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/clk/meson/a1-cpu.h b/drivers/clk/meson/a1-cpu.h
> new file mode 100644
> index 000000000000..e9af4117e26f
> --- /dev/null
> +++ b/drivers/clk/meson/a1-cpu.h

There is not point putting the definition here in a header
These are clearly not going to be shared with another driver.

Please drop this file

> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Amlogic A1 CPU Clock Controller internals
> + *
> + * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
> + * Author: Dmitry Rokosov <[e-mail]>
> + */
> +
> +#ifndef __A1_CPU_H
> +#define __A1_CPU_H
> +
> +/* cpu clock controller register offset */
> +#define CPUCTRL_CLK_CTRL0	0x80
> +#define CPUCTRL_CLK_CTRL1	0x84

You are claiming the registers from 0x00 to 0x84 (included), but only
using these 2 registers ? What is the rest ? Are you sure there is only
clocks in there ?

> +
> +#endif /* __A1_CPU_H */

Dmitry Rokosov April 2, 2024, 11:05 a.m. UTC | #5

Hello Jerome,

On Tue, Apr 02, 2024 at 11:35:49AM +0200, Jerome Brunet wrote:
> 
> On Fri 29 Mar 2024 at 23:58, Dmitry Rokosov <[e-mail]> 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 <[e-mail]>
> > ---
> >  drivers/clk/meson/Kconfig  |  10 ++
> >  drivers/clk/meson/Makefile |   1 +
> >  drivers/clk/meson/a1-cpu.c | 324 +++++++++++++++++++++++++++++++++++++
> >  drivers/clk/meson/a1-cpu.h |  16 ++
> >  4 files changed, 351 insertions(+)
> >  create mode 100644 drivers/clk/meson/a1-cpu.c
> >  create mode 100644 drivers/clk/meson/a1-cpu.h
> >
> > 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..5f5d8ae112e5
> > --- /dev/null
> > +++ b/drivers/clk/meson/a1-cpu.c
> > @@ -0,0 +1,324 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Amlogic A1 SoC family CPU Clock Controller driver.
> > + *
> > + * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
> > + * Author: Dmitry Rokosov <[e-mail]>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/platform_device.h>
> > +#include "a1-cpu.h"
> > +#include "clk-regmap.h"
> > +#include "meson-clkc-utils.h"
> > +
> > +#include <dt-bindings/clock/amlogic,a1-cpu-clkc.h>
> > +
> > +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,
> > +	},
> > +};
> > +
> > +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_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_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", },
> > +		},
> > +		.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_cpu_clk_nb_data {
> > +	const struct clk_ops *mux_ops;
> 
> That's fishy ...
> 
> > +	struct clk_hw *cpu_clk;
> > +	struct notifier_block nb;
> > +	u8 parent;
> > +};
> > +
> > +#define MESON_A1_CPU_CLK_GET_PARENT(nbd) \
> > +	((nbd)->mux_ops->get_parent((nbd)->cpu_clk))
> > +#define MESON_A1_CPU_CLK_SET_PARENT(nbd, index) \
> > +	((nbd)->mux_ops->set_parent((nbd)->cpu_clk, index))
> 
> ... Directly going for the mux ops ??!?? No way !
> 
> We have a framework to handle the clocks, the whole point is to use it,
> not bypass it ! 
> 

I suppose you understand my approach, which is quite similar to what is
happening in the Mediatek driver:

https://elixir.bootlin.com/linux/latest/source/drivers/clk/mediatek/clk-mux.c#L295

Initially, I attempted to set the parent using the clk_set_parent() API.
However, I encountered a problem with recursive calling of the
notifier_block. This issue arises because the parent triggers
notifications for its children, leading to repeated calls to the
notifier_block.

I find it puzzling why I cannot call an internal function or callback
within the internal driver context. After all, the notifier block is
just a part of the set_rate() flow. From a global Clock Control
Framework perspective, the context should not change.

> > +
> > +static int meson_a1_cpu_clk_notifier_cb(struct notifier_block *nb,
> > +					unsigned long event, void *data)
> > +{
> > +	struct a1_cpu_clk_nb_data *nbd;
> > +	int ret = 0;
> > +
> > +	nbd = container_of(nb, struct a1_cpu_clk_nb_data, nb);
> > +
> > +	switch (event) {
> > +	case PRE_RATE_CHANGE:
> > +		nbd->parent = MESON_A1_CPU_CLK_GET_PARENT(nbd);
> > +		/* Fallback to the CPU fixed clock */
> > +		ret = MESON_A1_CPU_CLK_SET_PARENT(nbd, 0);
> > +		/* Wait for clock propagation */
> > +		udelay(100);
> > +		break;
> > +
> > +	case POST_RATE_CHANGE:
> > +	case ABORT_RATE_CHANGE:
> > +		/* Back to the original parent clock */
> > +		ret = MESON_A1_CPU_CLK_SET_PARENT(nbd, nbd->parent);
> > +		/* Wait for clock propagation */
> > +		udelay(100);
> > +		break;
> > +
> > +	default:
> > +		pr_warn("Unknown event %lu for %s notifier block\n",
> > +			event, clk_hw_get_name(nbd->cpu_clk));
> > +		break;
> > +	}
> > +
> > +	return notifier_from_errno(ret);
> > +}
> > +
> > +static struct a1_cpu_clk_nb_data a1_cpu_clk_nb_data = {
> > +	.mux_ops = &clk_regmap_mux_ops,
> > +	.cpu_clk = &cpu_clk.hw,
> > +	.nb.notifier_call = meson_a1_cpu_clk_notifier_cb,
> > +};
> > +
> > +static int meson_a1_dvfs_setup(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct clk *notifier_clk;
> > +	int ret;
> > +
> > +	/* Setup clock notifier for cpu_clk */
> > +	notifier_clk = devm_clk_hw_get_clk(dev, &cpu_clk.hw, "dvfs");
> > +	if (IS_ERR(notifier_clk))
> > +		return dev_err_probe(dev, PTR_ERR(notifier_clk),
> > +				     "can't get cpu_clk as notifier clock\n");
> > +
> > +	ret = devm_clk_notifier_register(dev, notifier_clk,
> > +					 &a1_cpu_clk_nb_data.nb);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "can't register cpu_clk notifier\n");
> > +
> > +	return ret;
> > +}
> > +
> > +static int meson_a1_cpu_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	void __iomem *base;
> > +	struct regmap *map;
> > +	int clkid, i, err;
> > +
> > +	base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(base))
> > +		return dev_err_probe(dev, PTR_ERR(base),
> > +				     "can't ioremap resource\n");
> > +
> > +	map = devm_regmap_init_mmio(dev, base, &a1_cpu_regmap_cfg);
> > +	if (IS_ERR(map))
> > +		return dev_err_probe(dev, PTR_ERR(map),
> > +				     "can't init regmap mmio region\n");
> > +
> > +	/* Populate regmap for the regmap backed clocks */
> > +	for (i = 0; i < ARRAY_SIZE(a1_cpu_regmaps); i++)
> > +		a1_cpu_regmaps[i]->map = map;
> > +
> > +	for (clkid = 0; clkid < a1_cpu_clks.num; clkid++) {
> > +		err = devm_clk_hw_register(dev, a1_cpu_clks.hws[clkid]);
> > +		if (err)
> > +			return dev_err_probe(dev, err,
> > +					     "clock[%d] registration failed\n",
> > +					     clkid);
> > +	}
> > +
> > +	err = devm_of_clk_add_hw_provider(dev, meson_clk_hw_get, &a1_cpu_clks);
> > +	if (err)
> > +		return dev_err_probe(dev, err, "can't add clk hw provider\n");
> 
> I wonder if there is a window of opportunity to poke the syspll without
> your notifier here. That being said, the situation would be similar on g12.
> 

Yes, I have taken into account what you did in the G12A CPU clock
relations. My thoughts were that it might not be applicable for the A1
case. This is because the sys_pll should be located in a different
driver from a logical perspective. Consequently, we cannot configure the
sys_pll notifier block to manage the cpu_clk from a different driver.
However, if I were to move the sys_pll clock object to the A1 CPU clock
controller, I believe the g12a sys_pll notifier approach would work.

> > +
> > +	return meson_a1_dvfs_setup(pdev);
> 
> 
> 
> > +}
> > +
> > +static const struct of_device_id a1_cpu_clkc_match_table[] = {
> > +	{ .compatible = "amlogic,a1-cpu-clkc", },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, a1_cpu_clkc_match_table);
> > +
> > +static struct platform_driver a1_cpu_clkc_driver = {
> > +	.probe = meson_a1_cpu_probe,
> > +	.driver = {
> > +		.name = "a1-cpu-clkc",
> > +		.of_match_table = a1_cpu_clkc_match_table,
> > +	},
> > +};
> > +
> > +module_platform_driver(a1_cpu_clkc_driver);
> > +MODULE_AUTHOR("Dmitry Rokosov <[e-mail]>");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/clk/meson/a1-cpu.h b/drivers/clk/meson/a1-cpu.h
> > new file mode 100644
> > index 000000000000..e9af4117e26f
> > --- /dev/null
> > +++ b/drivers/clk/meson/a1-cpu.h
> 
> There is not point putting the definition here in a header
> These are clearly not going to be shared with another driver.
> 
> Please drop this file
> 

The same approach was applied to the Peripherals and PLL A1 drivers.
Honestly, I am not a fan of having different file organization within a
single logical code folder.

Please refer to:

drivers/clk/meson/a1-peripherals.h
drivers/clk/meson/a1-pll.h

> > @@ -0,0 +1,16 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Amlogic A1 CPU Clock Controller internals
> > + *
> > + * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
> > + * Author: Dmitry Rokosov <[e-mail]>
> > + */
> > +
> > +#ifndef __A1_CPU_H
> > +#define __A1_CPU_H
> > +
> > +/* cpu clock controller register offset */
> > +#define CPUCTRL_CLK_CTRL0	0x80
> > +#define CPUCTRL_CLK_CTRL1	0x84
> 
> You are claiming the registers from 0x00 to 0x84 (included), but only
> using these 2 registers ? What is the rest ? Are you sure there is only
> clocks in there ?
> 

Yes, unfortunately, the register map for this IP is not described in the
A1 Datasheet. The only available information about it can be found in
the vendor clock driver, which provides details for only two registers
used to configure the CPU clock.

From vendor kernel dtsi:

	clkc: clock-controller {
		compatible = "amlogic,a1-clkc";
		#clock-cells = <1>;
		reg = <0x0 0xfe000800 0x0 0x100>,
		      <0x0 0xfe007c00 0x0 0x21c>,
		      <0x0 0xfd000000 0x0 0x88>; <==== CPU clock regmap
		reg-names = "basic", "pll",
			    "cpu_clk";
		clocks = <&xtal>;
		clock-names = "core";
		status = "okay";
	};

From vendor clkc driver:

	/*
	 * CPU clok register offset
	 * APB_BASE:  APB1_BASE_ADDR = 0xfd000000
	 */

	#define CPUCTRL_CLK_CTRL0		0x80
	#define CPUCTRL_CLK_CTRL1		0x84

[...]

Dmitry Rokosov April 2, 2024, 11:43 a.m. UTC | #6

On Tue, Apr 02, 2024 at 11:27:24AM +0200, Jerome Brunet wrote:
> 
> On Mon 01 Apr 2024 at 20:12, Dmitry Rokosov <[e-mail]> wrote:
> 
> > Hello Martin,
> >
> > Thank you for quick response. Please find my thoughts below.
> >
> > On Sun, Mar 31, 2024 at 11:40:13PM +0200, Martin Blumenstingl wrote:
> >> Hi Dmitry,
> >> 
> >> On Fri, Mar 29, 2024 at 9:59 PM Dmitry Rokosov
> >> <[e-mail]> wrote:
> >> [...]
> >> > +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,
> >> Have you considered the CLK_SET_RATE_GATE flag for &cpu_fsel0.hw and
> >> &cpu_fsel1.hw and then dropping the clock notifier below?
> >> We use that approach with the Mali GPU clock on other SoCs, see for
> >> example commit 8daeaea99caa ("clk: meson: meson8b: make the CCF use
> >> the glitch-free mali mux").
> >> It may differ from what Amlogic does in their BSP,
> >
> > Amlogic in their BSP takes a different approach, which is slightly
> > different from mine. They cleverly change the parent of cpu_clk directly
> > by forking the cpufreq driver to a custom version. I must admit, it's
> > quite an "interesting and amazing" idea :) but it's not architecturally
> > correct totally.
> 
> I disagree. Martin's suggestion is correct for the fsel part which is
> symetric.
> 

It seems that I didn't fully understand Martin's suggestion. I was
confused by the advice to remove the notifier block and tried to explain
why it's not possible. However, I believe it is reasonable to protect
the cpu_fselX from rate propagation, because it is symmetric, as you
mentioned.

> >
> >> but I don't think
> >> that there's any harm (if it works in general) because CCF (common
> >> clock framework) will set all clocks in the "inactive" tree and then
> >> as a last step just change the mux (&cpu_fclk.hw). So at no point in
> >> time will we get any other rate than a) the original CPU clock rate
> >> before the rate change b) the new desired CPU clock rate. This is
> >> because we have two symmetric clock trees.
> >
> > Now, let's dive into the specifics of the issue we're facing. I've
> > examined the CLK_SET_RATE_GATE flag, which, to my understanding, blocks
> > rate changes for the entire clock chain. However, in this particular
> > situation, it doesn't provide the solution we need.
> >
> > Here's the problem we're dealing with:
> >
> > 1) The CPU clock can have the following frequency points:
> >
> >   available frequency steps:  128 MHz, 256 MHz, 512 MHz, 768 MHz, 1.01 GHz, 1.20 GHz
> >
> > When we run the cpupower, we get the following information:
> > # cpupower -c 0,1 frequency-info
> > analyzing CPU 0:
> >   driver: cpufreq-dt
> >   CPUs which run at the same hardware frequency: 0 1
> >   CPUs which need to have their frequency coordinated by software: 0 1
> >   maximum transition latency: 50.0 us
> >   hardware limits: 128 MHz - 1.20 GHz
> >   available frequency steps:  128 MHz, 256 MHz, 512 MHz, 768 MHz, 1.01 GHz, 1.20 GHz
> >   available cpufreq governors: conservative ondemand userspace performance schedutil
> >   current policy: frequency should be within 128 MHz and 128 MHz.
> >                   The governor "schedutil" may decide which speed to use
> >                   within this range.
> >   current CPU frequency: 128 MHz (asserted by call to hardware)
> > analyzing CPU 1:
> >   driver: cpufreq-dt
> >   CPUs which run at the same hardware frequency: 0 1
> >   CPUs which need to have their frequency coordinated by software: 0 1
> >   maximum transition latency: 50.0 us
> >   hardware limits: 128 MHz - 1.20 GHz
> >   available frequency steps:  128 MHz, 256 MHz, 512 MHz, 768 MHz, 1.01 GHz, 1.20 GHz
> >   available cpufreq governors: conservative ondemand userspace performance schedutil
> >   current policy: frequency should be within 128 MHz and 128 MHz.
> >                   The governor "schedutil" may decide which speed to use
> >                   within this range.
> >   current CPU frequency: 128 MHz (asserted by call to hardware)
> >
> > 2) For the frequency points 128 MHz, 256 MHz, and 512 MHz, the CPU fixed
> > clock should be used.
> 
> Apparently, you are relying on the SYS PLL lowest possible rate to
> enfore this contraint, which I suppose is 24 * 32 = 768MHz. It would be
> nice to clearly say so.
> 

Based on my understanding, the minimum frequency that sys_pll can
provide is not relevant. The CPU fixed clock is considered a "safety"
clock, and I can confidently connect the cpu_clk parent to that stable
clock without any issues. CCF will decide which parent will be used in
the end of rate changing process.

> > Fortunately, we don't encounter any freeze
> > problems when we attempt to change its rate at these frequencies.
> 
> That does not sound very solid ...
> 

Why? Per my understanding, CPU fixed clock guarantees this behaviour.

> >
> > 3) However, for the frequency points 768 MHz, 1.01 GHz, and 1.20 GHz,
> > the sys_pll is used as the clock source because it's a faster option.
> > Now, let's imagine that we want to change the CPU clock from 768 MHz to
> > 1.01 GHz. Unfortunately, it's not possible due to the broken sys_pll,
> > and any execution attempts will result in a hang.
> 
> ... Because PLL needs to relock, it is going to be off for a while. That
> is not "broken", unless there is something else ?
> 

Sorry for wrong terminology. I meant that sys_pll cannot be used as a
clock source (clock parent) while we are changing its rate.

> >
> > 4) As you can observe, in this case, we actually don't need to lock the
> > rate for the sys_pll chain.
> 
> In which case ? I'm lost.
> 

In the case for which notifier block was applied - cpu_clk and sys_pll
rate propagation.

> > We want to change the rate instead.
> 
> ... How are you going to do that without relocking the PLL ?
> 

I'm afraid, this is terminology miss from my side again. By 'sys_pll
lock' I mean rate lock using CLK_SET_RATE_GATE. I want to say that we
can't prohibit rate propagation of sys_pll chain, because we want to
change the rate, this is our main goal.

> > Hence,
> > I'm not aware of any other method to achieve this except by switching
> > the cpu_clk parent to a stable clock using clock notifier block.
> > Interestingly, I've noticed a similar approach in the CPU clock drivers
> > of Rockchip, Qualcomm, and Mediatek.
> 
> There is an example of syspll notifier in the g12 clock controller.
> You should have a look at it

Okay. As I mentioned in another email reply, in order to make it happen,
it is required to move the sys_pll clock to the a1-cpu driver. However,
I thought that this approach may not be correct from a logical
perspective. I will try.

Jerome Brunet April 2, 2024, 2:11 p.m. UTC | #7

On Tue 02 Apr 2024 at 14:05, Dmitry Rokosov <[e-mail]> wrote:

> Hello Jerome,
>
> On Tue, Apr 02, 2024 at 11:35:49AM +0200, Jerome Brunet wrote:
>> 
>> On Fri 29 Mar 2024 at 23:58, Dmitry Rokosov <[e-mail]> 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 <[e-mail]>
>> > ---
>> >  drivers/clk/meson/Kconfig  |  10 ++
>> >  drivers/clk/meson/Makefile |   1 +
>> >  drivers/clk/meson/a1-cpu.c | 324 +++++++++++++++++++++++++++++++++++++
>> >  drivers/clk/meson/a1-cpu.h |  16 ++
>> >  4 files changed, 351 insertions(+)
>> >  create mode 100644 drivers/clk/meson/a1-cpu.c
>> >  create mode 100644 drivers/clk/meson/a1-cpu.h
>> >
>> > 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..5f5d8ae112e5
>> > --- /dev/null
>> > +++ b/drivers/clk/meson/a1-cpu.c
>> > @@ -0,0 +1,324 @@
>> > +// SPDX-License-Identifier: GPL-2.0+
>> > +/*
>> > + * Amlogic A1 SoC family CPU Clock Controller driver.
>> > + *
>> > + * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
>> > + * Author: Dmitry Rokosov <[e-mail]>
>> > + */
>> > +
>> > +#include <linux/clk.h>
>> > +#include <linux/clk-provider.h>
>> > +#include <linux/mod_devicetable.h>
>> > +#include <linux/platform_device.h>
>> > +#include "a1-cpu.h"
>> > +#include "clk-regmap.h"
>> > +#include "meson-clkc-utils.h"
>> > +
>> > +#include <dt-bindings/clock/amlogic,a1-cpu-clkc.h>
>> > +
>> > +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,
>> > +	},
>> > +};
>> > +
>> > +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_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_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", },
>> > +		},
>> > +		.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_cpu_clk_nb_data {
>> > +	const struct clk_ops *mux_ops;
>> 
>> That's fishy ...
>> 
>> > +	struct clk_hw *cpu_clk;
>> > +	struct notifier_block nb;
>> > +	u8 parent;
>> > +};
>> > +
>> > +#define MESON_A1_CPU_CLK_GET_PARENT(nbd) \
>> > +	((nbd)->mux_ops->get_parent((nbd)->cpu_clk))
>> > +#define MESON_A1_CPU_CLK_SET_PARENT(nbd, index) \
>> > +	((nbd)->mux_ops->set_parent((nbd)->cpu_clk, index))
>> 
>> ... Directly going for the mux ops ??!?? No way !
>> 
>> We have a framework to handle the clocks, the whole point is to use it,
>> not bypass it ! 
>> 
>
> I suppose you understand my approach, which is quite similar to what is
> happening in the Mediatek driver:
>
> https://elixir.bootlin.com/linux/latest/source/drivers/clk/mediatek/clk-mux.c#L295
>
> Initially, I attempted to set the parent using the clk_set_parent() API.
> However, I encountered a problem with recursive calling of the
> notifier_block. This issue arises because the parent triggers
> notifications for its children, leading to repeated calls to the
> notifier_block.
>
> I find it puzzling why I cannot call an internal function or callback
> within the internal driver context. After all, the notifier block is
> just a part of the set_rate() flow. From a global Clock Control
> Framework perspective, the context should not change.

I don't care what MTK is doing TBH. Forcefully calling ops on a clock,
hoping they are going to match with the clock type is wrong.

There is a clk_hw API. Please it.

>
>> > +
>> > +static int meson_a1_cpu_clk_notifier_cb(struct notifier_block *nb,
>> > +					unsigned long event, void *data)
>> > +{
>> > +	struct a1_cpu_clk_nb_data *nbd;
>> > +	int ret = 0;
>> > +
>> > +	nbd = container_of(nb, struct a1_cpu_clk_nb_data, nb);
>> > +
>> > +	switch (event) {
>> > +	case PRE_RATE_CHANGE:
>> > +		nbd->parent = MESON_A1_CPU_CLK_GET_PARENT(nbd);
>> > +		/* Fallback to the CPU fixed clock */
>> > +		ret = MESON_A1_CPU_CLK_SET_PARENT(nbd, 0);
>> > +		/* Wait for clock propagation */
>> > +		udelay(100);
>> > +		break;
>> > +
>> > +	case POST_RATE_CHANGE:
>> > +	case ABORT_RATE_CHANGE:
>> > +		/* Back to the original parent clock */
>> > +		ret = MESON_A1_CPU_CLK_SET_PARENT(nbd, nbd->parent);
>> > +		/* Wait for clock propagation */
>> > +		udelay(100);
>> > +		break;
>> > +
>> > +	default:
>> > +		pr_warn("Unknown event %lu for %s notifier block\n",
>> > +			event, clk_hw_get_name(nbd->cpu_clk));
>> > +		break;
>> > +	}
>> > +
>> > +	return notifier_from_errno(ret);
>> > +}
>> > +
>> > +static struct a1_cpu_clk_nb_data a1_cpu_clk_nb_data = {
>> > +	.mux_ops = &clk_regmap_mux_ops,
>> > +	.cpu_clk = &cpu_clk.hw,
>> > +	.nb.notifier_call = meson_a1_cpu_clk_notifier_cb,
>> > +};
>> > +
>> > +static int meson_a1_dvfs_setup(struct platform_device *pdev)
>> > +{
>> > +	struct device *dev = &pdev->dev;
>> > +	struct clk *notifier_clk;
>> > +	int ret;
>> > +
>> > +	/* Setup clock notifier for cpu_clk */
>> > +	notifier_clk = devm_clk_hw_get_clk(dev, &cpu_clk.hw, "dvfs");
>> > +	if (IS_ERR(notifier_clk))
>> > +		return dev_err_probe(dev, PTR_ERR(notifier_clk),
>> > +				     "can't get cpu_clk as notifier clock\n");
>> > +
>> > +	ret = devm_clk_notifier_register(dev, notifier_clk,
>> > +					 &a1_cpu_clk_nb_data.nb);
>> > +	if (ret)
>> > +		return dev_err_probe(dev, ret,
>> > +				     "can't register cpu_clk notifier\n");
>> > +
>> > +	return ret;
>> > +}
>> > +
>> > +static int meson_a1_cpu_probe(struct platform_device *pdev)
>> > +{
>> > +	struct device *dev = &pdev->dev;
>> > +	void __iomem *base;
>> > +	struct regmap *map;
>> > +	int clkid, i, err;
>> > +
>> > +	base = devm_platform_ioremap_resource(pdev, 0);
>> > +	if (IS_ERR(base))
>> > +		return dev_err_probe(dev, PTR_ERR(base),
>> > +				     "can't ioremap resource\n");
>> > +
>> > +	map = devm_regmap_init_mmio(dev, base, &a1_cpu_regmap_cfg);
>> > +	if (IS_ERR(map))
>> > +		return dev_err_probe(dev, PTR_ERR(map),
>> > +				     "can't init regmap mmio region\n");
>> > +
>> > +	/* Populate regmap for the regmap backed clocks */
>> > +	for (i = 0; i < ARRAY_SIZE(a1_cpu_regmaps); i++)
>> > +		a1_cpu_regmaps[i]->map = map;
>> > +
>> > +	for (clkid = 0; clkid < a1_cpu_clks.num; clkid++) {
>> > +		err = devm_clk_hw_register(dev, a1_cpu_clks.hws[clkid]);
>> > +		if (err)
>> > +			return dev_err_probe(dev, err,
>> > +					     "clock[%d] registration failed\n",
>> > +					     clkid);
>> > +	}
>> > +
>> > +	err = devm_of_clk_add_hw_provider(dev, meson_clk_hw_get, &a1_cpu_clks);
>> > +	if (err)
>> > +		return dev_err_probe(dev, err, "can't add clk hw provider\n");
>> 
>> I wonder if there is a window of opportunity to poke the syspll without
>> your notifier here. That being said, the situation would be similar on g12.
>> 
>
> Yes, I have taken into account what you did in the G12A CPU clock
> relations. My thoughts were that it might not be applicable for the A1
> case. This is because the sys_pll should be located in a different
> driver from a logical perspective. Consequently, we cannot configure the
> sys_pll notifier block to manage the cpu_clk from a different driver.
> However, if I were to move the sys_pll clock object to the A1 CPU clock
> controller, I believe the g12a sys_pll notifier approach would work.
>

I fail to see the connection between the number of device and the
approach you took.


>> > +
>> > +	return meson_a1_dvfs_setup(pdev);
>> 
>> 
>> 
>> > +}
>> > +
>> > +static const struct of_device_id a1_cpu_clkc_match_table[] = {
>> > +	{ .compatible = "amlogic,a1-cpu-clkc", },
>> > +	{}
>> > +};
>> > +MODULE_DEVICE_TABLE(of, a1_cpu_clkc_match_table);
>> > +
>> > +static struct platform_driver a1_cpu_clkc_driver = {
>> > +	.probe = meson_a1_cpu_probe,
>> > +	.driver = {
>> > +		.name = "a1-cpu-clkc",
>> > +		.of_match_table = a1_cpu_clkc_match_table,
>> > +	},
>> > +};
>> > +
>> > +module_platform_driver(a1_cpu_clkc_driver);
>> > +MODULE_AUTHOR("Dmitry Rokosov <[e-mail]>");
>> > +MODULE_LICENSE("GPL");
>> > diff --git a/drivers/clk/meson/a1-cpu.h b/drivers/clk/meson/a1-cpu.h
>> > new file mode 100644
>> > index 000000000000..e9af4117e26f
>> > --- /dev/null
>> > +++ b/drivers/clk/meson/a1-cpu.h
>> 
>> There is not point putting the definition here in a header
>> These are clearly not going to be shared with another driver.
>> 
>> Please drop this file
>> 
>
> The same approach was applied to the Peripherals and PLL A1 drivers.
> Honestly, I am not a fan of having different file organization within a
> single logical code folder.
>
> Please refer to:
>
> drivers/clk/meson/a1-peripherals.h
> drivers/clk/meson/a1-pll.h

I understand. There was a time where it made sense, some definition
could have been shared back then. This is no longer the case and it
would probably welcome a rework.

... and again, just pointing to another code does really invalidate my
 point.

>
>> > @@ -0,0 +1,16 @@
>> > +/* SPDX-License-Identifier: GPL-2.0+ */
>> > +/*
>> > + * Amlogic A1 CPU Clock Controller internals
>> > + *
>> > + * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
>> > + * Author: Dmitry Rokosov <[e-mail]>
>> > + */
>> > +
>> > +#ifndef __A1_CPU_H
>> > +#define __A1_CPU_H
>> > +
>> > +/* cpu clock controller register offset */
>> > +#define CPUCTRL_CLK_CTRL0	0x80
>> > +#define CPUCTRL_CLK_CTRL1	0x84
>> 
>> You are claiming the registers from 0x00 to 0x84 (included), but only
>> using these 2 registers ? What is the rest ? Are you sure there is only
>> clocks in there ?
>> 
>
> Yes, unfortunately, the register map for this IP is not described in the
> A1 Datasheet. The only available information about it can be found in
> the vendor clock driver, which provides details for only two registers
> used to configure the CPU clock.
>
> From vendor kernel dtsi:
>
> 	clkc: clock-controller {
> 		compatible = "amlogic,a1-clkc";
> 		#clock-cells = <1>;
> 		reg = <0x0 0xfe000800 0x0 0x100>,
> 		      <0x0 0xfe007c00 0x0 0x21c>,
> 		      <0x0 0xfd000000 0x0 0x88>; <==== CPU clock regmap
> 		reg-names = "basic", "pll",
> 			    "cpu_clk";
> 		clocks = <&xtal>;
> 		clock-names = "core";
> 		status = "okay";
> 	};
>
> From vendor clkc driver:
>
> 	/*
> 	 * CPU clok register offset
> 	 * APB_BASE:  APB1_BASE_ADDR = 0xfd000000
> 	 */
>
> 	#define CPUCTRL_CLK_CTRL0		0x80
> 	#define CPUCTRL_CLK_CTRL1		0x84

If you had clock in 0x0 and 0x80, then I would agree claiming 0x0-0x88
is reasonable, even if you don't really know what is in between. It
would be a fair assumption.

It is not the case here.
For all we know it could resets, power domains, etc ...

>
> [...]

Dmitry Rokosov April 2, 2024, 4:11 p.m. UTC | #8

On Tue, Apr 02, 2024 at 04:11:12PM +0200, Jerome Brunet wrote:
> 
> On Tue 02 Apr 2024 at 14:05, Dmitry Rokosov <[e-mail]> wrote:
> 
> > Hello Jerome,
> >
> > On Tue, Apr 02, 2024 at 11:35:49AM +0200, Jerome Brunet wrote:
> >> 
> >> On Fri 29 Mar 2024 at 23:58, Dmitry Rokosov <[e-mail]> 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 <[e-mail]>
> >> > ---
> >> >  drivers/clk/meson/Kconfig  |  10 ++
> >> >  drivers/clk/meson/Makefile |   1 +
> >> >  drivers/clk/meson/a1-cpu.c | 324 +++++++++++++++++++++++++++++++++++++
> >> >  drivers/clk/meson/a1-cpu.h |  16 ++
> >> >  4 files changed, 351 insertions(+)
> >> >  create mode 100644 drivers/clk/meson/a1-cpu.c
> >> >  create mode 100644 drivers/clk/meson/a1-cpu.h
> >> >
> >> > 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..5f5d8ae112e5
> >> > --- /dev/null
> >> > +++ b/drivers/clk/meson/a1-cpu.c
> >> > @@ -0,0 +1,324 @@
> >> > +// SPDX-License-Identifier: GPL-2.0+
> >> > +/*
> >> > + * Amlogic A1 SoC family CPU Clock Controller driver.
> >> > + *
> >> > + * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
> >> > + * Author: Dmitry Rokosov <[e-mail]>
> >> > + */
> >> > +
> >> > +#include <linux/clk.h>
> >> > +#include <linux/clk-provider.h>
> >> > +#include <linux/mod_devicetable.h>
> >> > +#include <linux/platform_device.h>
> >> > +#include "a1-cpu.h"
> >> > +#include "clk-regmap.h"
> >> > +#include "meson-clkc-utils.h"
> >> > +
> >> > +#include <dt-bindings/clock/amlogic,a1-cpu-clkc.h>
> >> > +
> >> > +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,
> >> > +	},
> >> > +};
> >> > +
> >> > +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_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_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", },
> >> > +		},
> >> > +		.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_cpu_clk_nb_data {
> >> > +	const struct clk_ops *mux_ops;
> >> 
> >> That's fishy ...
> >> 
> >> > +	struct clk_hw *cpu_clk;
> >> > +	struct notifier_block nb;
> >> > +	u8 parent;
> >> > +};
> >> > +
> >> > +#define MESON_A1_CPU_CLK_GET_PARENT(nbd) \
> >> > +	((nbd)->mux_ops->get_parent((nbd)->cpu_clk))
> >> > +#define MESON_A1_CPU_CLK_SET_PARENT(nbd, index) \
> >> > +	((nbd)->mux_ops->set_parent((nbd)->cpu_clk, index))
> >> 
> >> ... Directly going for the mux ops ??!?? No way !
> >> 
> >> We have a framework to handle the clocks, the whole point is to use it,
> >> not bypass it ! 
> >> 
> >
> > I suppose you understand my approach, which is quite similar to what is
> > happening in the Mediatek driver:
> >
> > https://elixir.bootlin.com/linux/latest/source/drivers/clk/mediatek/clk-mux.c#L295
> >
> > Initially, I attempted to set the parent using the clk_set_parent() API.
> > However, I encountered a problem with recursive calling of the
> > notifier_block. This issue arises because the parent triggers
> > notifications for its children, leading to repeated calls to the
> > notifier_block.
> >
> > I find it puzzling why I cannot call an internal function or callback
> > within the internal driver context. After all, the notifier block is
> > just a part of the set_rate() flow. From a global Clock Control
> > Framework perspective, the context should not change.
> 
> I don't care what MTK is doing TBH. Forcefully calling ops on a clock,
> hoping they are going to match with the clock type is wrong.
> 
> There is a clk_hw API. Please it.
> 

Yes, if sys_pll has a notifier_block instead of cpu_clk, there will be
no problem with the clk_hw API.

I will rework that point.

> >
> >> > +
> >> > +static int meson_a1_cpu_clk_notifier_cb(struct notifier_block *nb,
> >> > +					unsigned long event, void *data)
> >> > +{
> >> > +	struct a1_cpu_clk_nb_data *nbd;
> >> > +	int ret = 0;
> >> > +
> >> > +	nbd = container_of(nb, struct a1_cpu_clk_nb_data, nb);
> >> > +
> >> > +	switch (event) {
> >> > +	case PRE_RATE_CHANGE:
> >> > +		nbd->parent = MESON_A1_CPU_CLK_GET_PARENT(nbd);
> >> > +		/* Fallback to the CPU fixed clock */
> >> > +		ret = MESON_A1_CPU_CLK_SET_PARENT(nbd, 0);
> >> > +		/* Wait for clock propagation */
> >> > +		udelay(100);
> >> > +		break;
> >> > +
> >> > +	case POST_RATE_CHANGE:
> >> > +	case ABORT_RATE_CHANGE:
> >> > +		/* Back to the original parent clock */
> >> > +		ret = MESON_A1_CPU_CLK_SET_PARENT(nbd, nbd->parent);
> >> > +		/* Wait for clock propagation */
> >> > +		udelay(100);
> >> > +		break;
> >> > +
> >> > +	default:
> >> > +		pr_warn("Unknown event %lu for %s notifier block\n",
> >> > +			event, clk_hw_get_name(nbd->cpu_clk));
> >> > +		break;
> >> > +	}
> >> > +
> >> > +	return notifier_from_errno(ret);
> >> > +}
> >> > +
> >> > +static struct a1_cpu_clk_nb_data a1_cpu_clk_nb_data = {
> >> > +	.mux_ops = &clk_regmap_mux_ops,
> >> > +	.cpu_clk = &cpu_clk.hw,
> >> > +	.nb.notifier_call = meson_a1_cpu_clk_notifier_cb,
> >> > +};
> >> > +
> >> > +static int meson_a1_dvfs_setup(struct platform_device *pdev)
> >> > +{
> >> > +	struct device *dev = &pdev->dev;
> >> > +	struct clk *notifier_clk;
> >> > +	int ret;
> >> > +
> >> > +	/* Setup clock notifier for cpu_clk */
> >> > +	notifier_clk = devm_clk_hw_get_clk(dev, &cpu_clk.hw, "dvfs");
> >> > +	if (IS_ERR(notifier_clk))
> >> > +		return dev_err_probe(dev, PTR_ERR(notifier_clk),
> >> > +				     "can't get cpu_clk as notifier clock\n");
> >> > +
> >> > +	ret = devm_clk_notifier_register(dev, notifier_clk,
> >> > +					 &a1_cpu_clk_nb_data.nb);
> >> > +	if (ret)
> >> > +		return dev_err_probe(dev, ret,
> >> > +				     "can't register cpu_clk notifier\n");
> >> > +
> >> > +	return ret;
> >> > +}
> >> > +
> >> > +static int meson_a1_cpu_probe(struct platform_device *pdev)
> >> > +{
> >> > +	struct device *dev = &pdev->dev;
> >> > +	void __iomem *base;
> >> > +	struct regmap *map;
> >> > +	int clkid, i, err;
> >> > +
> >> > +	base = devm_platform_ioremap_resource(pdev, 0);
> >> > +	if (IS_ERR(base))
> >> > +		return dev_err_probe(dev, PTR_ERR(base),
> >> > +				     "can't ioremap resource\n");
> >> > +
> >> > +	map = devm_regmap_init_mmio(dev, base, &a1_cpu_regmap_cfg);
> >> > +	if (IS_ERR(map))
> >> > +		return dev_err_probe(dev, PTR_ERR(map),
> >> > +				     "can't init regmap mmio region\n");
> >> > +
> >> > +	/* Populate regmap for the regmap backed clocks */
> >> > +	for (i = 0; i < ARRAY_SIZE(a1_cpu_regmaps); i++)
> >> > +		a1_cpu_regmaps[i]->map = map;
> >> > +
> >> > +	for (clkid = 0; clkid < a1_cpu_clks.num; clkid++) {
> >> > +		err = devm_clk_hw_register(dev, a1_cpu_clks.hws[clkid]);
> >> > +		if (err)
> >> > +			return dev_err_probe(dev, err,
> >> > +					     "clock[%d] registration failed\n",
> >> > +					     clkid);
> >> > +	}
> >> > +
> >> > +	err = devm_of_clk_add_hw_provider(dev, meson_clk_hw_get, &a1_cpu_clks);
> >> > +	if (err)
> >> > +		return dev_err_probe(dev, err, "can't add clk hw provider\n");
> >> 
> >> I wonder if there is a window of opportunity to poke the syspll without
> >> your notifier here. That being said, the situation would be similar on g12.
> >> 
> >
> > Yes, I have taken into account what you did in the G12A CPU clock
> > relations. My thoughts were that it might not be applicable for the A1
> > case. This is because the sys_pll should be located in a different
> > driver from a logical perspective. Consequently, we cannot configure the
> > sys_pll notifier block to manage the cpu_clk from a different driver.
> > However, if I were to move the sys_pll clock object to the A1 CPU clock
> > controller, I believe the g12a sys_pll notifier approach would work.
> >
> 
> I fail to see the connection between the number of device and the
> approach you took.
> 
> 
> >> > +
> >> > +	return meson_a1_dvfs_setup(pdev);
> >> 
> >> 
> >> 
> >> > +}
> >> > +
> >> > +static const struct of_device_id a1_cpu_clkc_match_table[] = {
> >> > +	{ .compatible = "amlogic,a1-cpu-clkc", },
> >> > +	{}
> >> > +};
> >> > +MODULE_DEVICE_TABLE(of, a1_cpu_clkc_match_table);
> >> > +
> >> > +static struct platform_driver a1_cpu_clkc_driver = {
> >> > +	.probe = meson_a1_cpu_probe,
> >> > +	.driver = {
> >> > +		.name = "a1-cpu-clkc",
> >> > +		.of_match_table = a1_cpu_clkc_match_table,
> >> > +	},
> >> > +};
> >> > +
> >> > +module_platform_driver(a1_cpu_clkc_driver);
> >> > +MODULE_AUTHOR("Dmitry Rokosov <[e-mail]>");
> >> > +MODULE_LICENSE("GPL");
> >> > diff --git a/drivers/clk/meson/a1-cpu.h b/drivers/clk/meson/a1-cpu.h
> >> > new file mode 100644
> >> > index 000000000000..e9af4117e26f
> >> > --- /dev/null
> >> > +++ b/drivers/clk/meson/a1-cpu.h
> >> 
> >> There is not point putting the definition here in a header
> >> These are clearly not going to be shared with another driver.
> >> 
> >> Please drop this file
> >> 
> >
> > The same approach was applied to the Peripherals and PLL A1 drivers.
> > Honestly, I am not a fan of having different file organization within a
> > single logical code folder.
> >
> > Please refer to:
> >
> > drivers/clk/meson/a1-peripherals.h
> > drivers/clk/meson/a1-pll.h
> 
> I understand. There was a time where it made sense, some definition
> could have been shared back then. This is no longer the case and it
> would probably welcome a rework.
> 
> ... and again, just pointing to another code does really invalidate my
>  point.
> 
> >
> >> > @@ -0,0 +1,16 @@
> >> > +/* SPDX-License-Identifier: GPL-2.0+ */
> >> > +/*
> >> > + * Amlogic A1 CPU Clock Controller internals
> >> > + *
> >> > + * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
> >> > + * Author: Dmitry Rokosov <[e-mail]>
> >> > + */
> >> > +
> >> > +#ifndef __A1_CPU_H
> >> > +#define __A1_CPU_H
> >> > +
> >> > +/* cpu clock controller register offset */
> >> > +#define CPUCTRL_CLK_CTRL0	0x80
> >> > +#define CPUCTRL_CLK_CTRL1	0x84
> >> 
> >> You are claiming the registers from 0x00 to 0x84 (included), but only
> >> using these 2 registers ? What is the rest ? Are you sure there is only
> >> clocks in there ?
> >> 
> >
> > Yes, unfortunately, the register map for this IP is not described in the
> > A1 Datasheet. The only available information about it can be found in
> > the vendor clock driver, which provides details for only two registers
> > used to configure the CPU clock.
> >
> > From vendor kernel dtsi:
> >
> > 	clkc: clock-controller {
> > 		compatible = "amlogic,a1-clkc";
> > 		#clock-cells = <1>;
> > 		reg = <0x0 0xfe000800 0x0 0x100>,
> > 		      <0x0 0xfe007c00 0x0 0x21c>,
> > 		      <0x0 0xfd000000 0x0 0x88>; <==== CPU clock regmap
> > 		reg-names = "basic", "pll",
> > 			    "cpu_clk";
> > 		clocks = <&xtal>;
> > 		clock-names = "core";
> > 		status = "okay";
> > 	};
> >
> > From vendor clkc driver:
> >
> > 	/*
> > 	 * CPU clok register offset
> > 	 * APB_BASE:  APB1_BASE_ADDR = 0xfd000000
> > 	 */
> >
> > 	#define CPUCTRL_CLK_CTRL0		0x80
> > 	#define CPUCTRL_CLK_CTRL1		0x84
> 
> If you had clock in 0x0 and 0x80, then I would agree claiming 0x0-0x88
> is reasonable, even if you don't really know what is in between. It
> would be a fair assumption.
> 
> It is not the case here.
> For all we know it could resets, power domains, etc ...
> 

However, we do not have any information indicating that the gap from
0x00-0x80 contains additional registers. It is possible that there are
internal registers, but they are not mentioned in the vendor datasheet
or driver. Therefore, in this case, I personally prefer to rely on the
vendor mapping.

Jerome Brunet April 2, 2024, 4:17 p.m. UTC | #9

On Tue 02 Apr 2024 at 19:11, Dmitry Rokosov <[e-mail]> wrote:

> On Tue, Apr 02, 2024 at 04:11:12PM +0200, Jerome Brunet wrote:
>> 
>> On Tue 02 Apr 2024 at 14:05, Dmitry Rokosov <[e-mail]> wrote:
>> 
>> > Hello Jerome,
>> >
>> > On Tue, Apr 02, 2024 at 11:35:49AM +0200, Jerome Brunet wrote:
>> >> 
>> >> On Fri 29 Mar 2024 at 23:58, Dmitry Rokosov <[e-mail]> 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 <[e-mail]>
>> >> > ---
>> >> >  drivers/clk/meson/Kconfig  |  10 ++
>> >> >  drivers/clk/meson/Makefile |   1 +
>> >> >  drivers/clk/meson/a1-cpu.c | 324 +++++++++++++++++++++++++++++++++++++
>> >> >  drivers/clk/meson/a1-cpu.h |  16 ++
>> >> >  4 files changed, 351 insertions(+)
>> >> >  create mode 100644 drivers/clk/meson/a1-cpu.c
>> >> >  create mode 100644 drivers/clk/meson/a1-cpu.h
>> >> >
>> >> > 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..5f5d8ae112e5
>> >> > --- /dev/null
>> >> > +++ b/drivers/clk/meson/a1-cpu.c
>> >> > @@ -0,0 +1,324 @@
>> >> > +// SPDX-License-Identifier: GPL-2.0+
>> >> > +/*
>> >> > + * Amlogic A1 SoC family CPU Clock Controller driver.
>> >> > + *
>> >> > + * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
>> >> > + * Author: Dmitry Rokosov <[e-mail]>
>> >> > + */
>> >> > +
>> >> > +#include <linux/clk.h>
>> >> > +#include <linux/clk-provider.h>
>> >> > +#include <linux/mod_devicetable.h>
>> >> > +#include <linux/platform_device.h>
>> >> > +#include "a1-cpu.h"
>> >> > +#include "clk-regmap.h"
>> >> > +#include "meson-clkc-utils.h"
>> >> > +
>> >> > +#include <dt-bindings/clock/amlogic,a1-cpu-clkc.h>
>> >> > +
>> >> > +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,
>> >> > +	},
>> >> > +};
>> >> > +
>> >> > +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_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_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", },
>> >> > +		},
>> >> > +		.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_cpu_clk_nb_data {
>> >> > +	const struct clk_ops *mux_ops;
>> >> 
>> >> That's fishy ...
>> >> 
>> >> > +	struct clk_hw *cpu_clk;
>> >> > +	struct notifier_block nb;
>> >> > +	u8 parent;
>> >> > +};
>> >> > +
>> >> > +#define MESON_A1_CPU_CLK_GET_PARENT(nbd) \
>> >> > +	((nbd)->mux_ops->get_parent((nbd)->cpu_clk))
>> >> > +#define MESON_A1_CPU_CLK_SET_PARENT(nbd, index) \
>> >> > +	((nbd)->mux_ops->set_parent((nbd)->cpu_clk, index))
>> >> 
>> >> ... Directly going for the mux ops ??!?? No way !
>> >> 
>> >> We have a framework to handle the clocks, the whole point is to use it,
>> >> not bypass it ! 
>> >> 
>> >
>> > I suppose you understand my approach, which is quite similar to what is
>> > happening in the Mediatek driver:
>> >
>> > https://elixir.bootlin.com/linux/latest/source/drivers/clk/mediatek/clk-mux.c#L295
>> >
>> > Initially, I attempted to set the parent using the clk_set_parent() API.
>> > However, I encountered a problem with recursive calling of the
>> > notifier_block. This issue arises because the parent triggers
>> > notifications for its children, leading to repeated calls to the
>> > notifier_block.
>> >
>> > I find it puzzling why I cannot call an internal function or callback
>> > within the internal driver context. After all, the notifier block is
>> > just a part of the set_rate() flow. From a global Clock Control
>> > Framework perspective, the context should not change.
>> 
>> I don't care what MTK is doing TBH. Forcefully calling ops on a clock,
>> hoping they are going to match with the clock type is wrong.
>> 
>> There is a clk_hw API. Please it.
>> 
>
> Yes, if sys_pll has a notifier_block instead of cpu_clk, there will be
> no problem with the clk_hw API.
>
> I will rework that point.
>
>> >
>> >> > +
>> >> > +static int meson_a1_cpu_clk_notifier_cb(struct notifier_block *nb,
>> >> > +					unsigned long event, void *data)
>> >> > +{
>> >> > +	struct a1_cpu_clk_nb_data *nbd;
>> >> > +	int ret = 0;
>> >> > +
>> >> > +	nbd = container_of(nb, struct a1_cpu_clk_nb_data, nb);
>> >> > +
>> >> > +	switch (event) {
>> >> > +	case PRE_RATE_CHANGE:
>> >> > +		nbd->parent = MESON_A1_CPU_CLK_GET_PARENT(nbd);
>> >> > +		/* Fallback to the CPU fixed clock */
>> >> > +		ret = MESON_A1_CPU_CLK_SET_PARENT(nbd, 0);
>> >> > +		/* Wait for clock propagation */
>> >> > +		udelay(100);
>> >> > +		break;
>> >> > +
>> >> > +	case POST_RATE_CHANGE:
>> >> > +	case ABORT_RATE_CHANGE:
>> >> > +		/* Back to the original parent clock */
>> >> > +		ret = MESON_A1_CPU_CLK_SET_PARENT(nbd, nbd->parent);
>> >> > +		/* Wait for clock propagation */
>> >> > +		udelay(100);
>> >> > +		break;
>> >> > +
>> >> > +	default:
>> >> > +		pr_warn("Unknown event %lu for %s notifier block\n",
>> >> > +			event, clk_hw_get_name(nbd->cpu_clk));
>> >> > +		break;
>> >> > +	}
>> >> > +
>> >> > +	return notifier_from_errno(ret);
>> >> > +}
>> >> > +
>> >> > +static struct a1_cpu_clk_nb_data a1_cpu_clk_nb_data = {
>> >> > +	.mux_ops = &clk_regmap_mux_ops,
>> >> > +	.cpu_clk = &cpu_clk.hw,
>> >> > +	.nb.notifier_call = meson_a1_cpu_clk_notifier_cb,
>> >> > +};
>> >> > +
>> >> > +static int meson_a1_dvfs_setup(struct platform_device *pdev)
>> >> > +{
>> >> > +	struct device *dev = &pdev->dev;
>> >> > +	struct clk *notifier_clk;
>> >> > +	int ret;
>> >> > +
>> >> > +	/* Setup clock notifier for cpu_clk */
>> >> > +	notifier_clk = devm_clk_hw_get_clk(dev, &cpu_clk.hw, "dvfs");
>> >> > +	if (IS_ERR(notifier_clk))
>> >> > +		return dev_err_probe(dev, PTR_ERR(notifier_clk),
>> >> > +				     "can't get cpu_clk as notifier clock\n");
>> >> > +
>> >> > +	ret = devm_clk_notifier_register(dev, notifier_clk,
>> >> > +					 &a1_cpu_clk_nb_data.nb);
>> >> > +	if (ret)
>> >> > +		return dev_err_probe(dev, ret,
>> >> > +				     "can't register cpu_clk notifier\n");
>> >> > +
>> >> > +	return ret;
>> >> > +}
>> >> > +
>> >> > +static int meson_a1_cpu_probe(struct platform_device *pdev)
>> >> > +{
>> >> > +	struct device *dev = &pdev->dev;
>> >> > +	void __iomem *base;
>> >> > +	struct regmap *map;
>> >> > +	int clkid, i, err;
>> >> > +
>> >> > +	base = devm_platform_ioremap_resource(pdev, 0);
>> >> > +	if (IS_ERR(base))
>> >> > +		return dev_err_probe(dev, PTR_ERR(base),
>> >> > +				     "can't ioremap resource\n");
>> >> > +
>> >> > +	map = devm_regmap_init_mmio(dev, base, &a1_cpu_regmap_cfg);
>> >> > +	if (IS_ERR(map))
>> >> > +		return dev_err_probe(dev, PTR_ERR(map),
>> >> > +				     "can't init regmap mmio region\n");
>> >> > +
>> >> > +	/* Populate regmap for the regmap backed clocks */
>> >> > +	for (i = 0; i < ARRAY_SIZE(a1_cpu_regmaps); i++)
>> >> > +		a1_cpu_regmaps[i]->map = map;
>> >> > +
>> >> > +	for (clkid = 0; clkid < a1_cpu_clks.num; clkid++) {
>> >> > +		err = devm_clk_hw_register(dev, a1_cpu_clks.hws[clkid]);
>> >> > +		if (err)
>> >> > +			return dev_err_probe(dev, err,
>> >> > +					     "clock[%d] registration failed\n",
>> >> > +					     clkid);
>> >> > +	}
>> >> > +
>> >> > +	err = devm_of_clk_add_hw_provider(dev, meson_clk_hw_get, &a1_cpu_clks);
>> >> > +	if (err)
>> >> > +		return dev_err_probe(dev, err, "can't add clk hw provider\n");
>> >> 
>> >> I wonder if there is a window of opportunity to poke the syspll without
>> >> your notifier here. That being said, the situation would be similar on g12.
>> >> 
>> >
>> > Yes, I have taken into account what you did in the G12A CPU clock
>> > relations. My thoughts were that it might not be applicable for the A1
>> > case. This is because the sys_pll should be located in a different
>> > driver from a logical perspective. Consequently, we cannot configure the
>> > sys_pll notifier block to manage the cpu_clk from a different driver.
>> > However, if I were to move the sys_pll clock object to the A1 CPU clock
>> > controller, I believe the g12a sys_pll notifier approach would work.
>> >
>> 
>> I fail to see the connection between the number of device and the
>> approach you took.
>> 
>> 
>> >> > +
>> >> > +	return meson_a1_dvfs_setup(pdev);
>> >> 
>> >> 
>> >> 
>> >> > +}
>> >> > +
>> >> > +static const struct of_device_id a1_cpu_clkc_match_table[] = {
>> >> > +	{ .compatible = "amlogic,a1-cpu-clkc", },
>> >> > +	{}
>> >> > +};
>> >> > +MODULE_DEVICE_TABLE(of, a1_cpu_clkc_match_table);
>> >> > +
>> >> > +static struct platform_driver a1_cpu_clkc_driver = {
>> >> > +	.probe = meson_a1_cpu_probe,
>> >> > +	.driver = {
>> >> > +		.name = "a1-cpu-clkc",
>> >> > +		.of_match_table = a1_cpu_clkc_match_table,
>> >> > +	},
>> >> > +};
>> >> > +
>> >> > +module_platform_driver(a1_cpu_clkc_driver);
>> >> > +MODULE_AUTHOR("Dmitry Rokosov <[e-mail]>");
>> >> > +MODULE_LICENSE("GPL");
>> >> > diff --git a/drivers/clk/meson/a1-cpu.h b/drivers/clk/meson/a1-cpu.h
>> >> > new file mode 100644
>> >> > index 000000000000..e9af4117e26f
>> >> > --- /dev/null
>> >> > +++ b/drivers/clk/meson/a1-cpu.h
>> >> 
>> >> There is not point putting the definition here in a header
>> >> These are clearly not going to be shared with another driver.
>> >> 
>> >> Please drop this file
>> >> 
>> >
>> > The same approach was applied to the Peripherals and PLL A1 drivers.
>> > Honestly, I am not a fan of having different file organization within a
>> > single logical code folder.
>> >
>> > Please refer to:
>> >
>> > drivers/clk/meson/a1-peripherals.h
>> > drivers/clk/meson/a1-pll.h
>> 
>> I understand. There was a time where it made sense, some definition
>> could have been shared back then. This is no longer the case and it
>> would probably welcome a rework.
>> 
>> ... and again, just pointing to another code does really invalidate my
>>  point.
>> 
>> >
>> >> > @@ -0,0 +1,16 @@
>> >> > +/* SPDX-License-Identifier: GPL-2.0+ */
>> >> > +/*
>> >> > + * Amlogic A1 CPU Clock Controller internals
>> >> > + *
>> >> > + * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
>> >> > + * Author: Dmitry Rokosov <[e-mail]>
>> >> > + */
>> >> > +
>> >> > +#ifndef __A1_CPU_H
>> >> > +#define __A1_CPU_H
>> >> > +
>> >> > +/* cpu clock controller register offset */
>> >> > +#define CPUCTRL_CLK_CTRL0	0x80
>> >> > +#define CPUCTRL_CLK_CTRL1	0x84
>> >> 
>> >> You are claiming the registers from 0x00 to 0x84 (included), but only
>> >> using these 2 registers ? What is the rest ? Are you sure there is only
>> >> clocks in there ?
>> >> 
>> >
>> > Yes, unfortunately, the register map for this IP is not described in the
>> > A1 Datasheet. The only available information about it can be found in
>> > the vendor clock driver, which provides details for only two registers
>> > used to configure the CPU clock.
>> >
>> > From vendor kernel dtsi:
>> >
>> > 	clkc: clock-controller {
>> > 		compatible = "amlogic,a1-clkc";
>> > 		#clock-cells = <1>;
>> > 		reg = <0x0 0xfe000800 0x0 0x100>,
>> > 		      <0x0 0xfe007c00 0x0 0x21c>,
>> > 		      <0x0 0xfd000000 0x0 0x88>; <==== CPU clock regmap
>> > 		reg-names = "basic", "pll",
>> > 			    "cpu_clk";
>> > 		clocks = <&xtal>;
>> > 		clock-names = "core";
>> > 		status = "okay";
>> > 	};
>> >
>> > From vendor clkc driver:
>> >
>> > 	/*
>> > 	 * CPU clok register offset
>> > 	 * APB_BASE:  APB1_BASE_ADDR = 0xfd000000
>> > 	 */
>> >
>> > 	#define CPUCTRL_CLK_CTRL0		0x80
>> > 	#define CPUCTRL_CLK_CTRL1		0x84
>> 
>> If you had clock in 0x0 and 0x80, then I would agree claiming 0x0-0x88
>> is reasonable, even if you don't really know what is in between. It
>> would be a fair assumption.
>> 
>> It is not the case here.
>> For all we know it could resets, power domains, etc ...
>> 
>
> However, we do not have any information indicating that the gap from
> 0x00-0x80 contains additional registers. It is possible that there are
> internal registers, but they are not mentioned in the vendor datasheet
> or driver. Therefore, in this case, I personally prefer to rely on the
> vendor mapping.

I understand your preference. My request remains.

Patch

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..5f5d8ae112e5
--- /dev/null
+++ b/drivers/clk/meson/a1-cpu.c
@@ -0,0 +1,324 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Amlogic A1 SoC family CPU Clock Controller driver.
+ *
+ * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
+ * Author: Dmitry Rokosov <[e-mail]>
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include "a1-cpu.h"
+#include "clk-regmap.h"
+#include "meson-clkc-utils.h"
+
+#include <dt-bindings/clock/amlogic,a1-cpu-clkc.h>
+
+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,
+	},
+};
+
+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_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_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", },
+		},
+		.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_cpu_clk_nb_data {
+	const struct clk_ops *mux_ops;
+	struct clk_hw *cpu_clk;
+	struct notifier_block nb;
+	u8 parent;
+};
+
+#define MESON_A1_CPU_CLK_GET_PARENT(nbd) \
+	((nbd)->mux_ops->get_parent((nbd)->cpu_clk))
+#define MESON_A1_CPU_CLK_SET_PARENT(nbd, index) \
+	((nbd)->mux_ops->set_parent((nbd)->cpu_clk, index))
+
+static int meson_a1_cpu_clk_notifier_cb(struct notifier_block *nb,
+					unsigned long event, void *data)
+{
+	struct a1_cpu_clk_nb_data *nbd;
+	int ret = 0;
+
+	nbd = container_of(nb, struct a1_cpu_clk_nb_data, nb);
+
+	switch (event) {
+	case PRE_RATE_CHANGE:
+		nbd->parent = MESON_A1_CPU_CLK_GET_PARENT(nbd);
+		/* Fallback to the CPU fixed clock */
+		ret = MESON_A1_CPU_CLK_SET_PARENT(nbd, 0);
+		/* Wait for clock propagation */
+		udelay(100);
+		break;
+
+	case POST_RATE_CHANGE:
+	case ABORT_RATE_CHANGE:
+		/* Back to the original parent clock */
+		ret = MESON_A1_CPU_CLK_SET_PARENT(nbd, nbd->parent);
+		/* Wait for clock propagation */
+		udelay(100);
+		break;
+
+	default:
+		pr_warn("Unknown event %lu for %s notifier block\n",
+			event, clk_hw_get_name(nbd->cpu_clk));
+		break;
+	}
+
+	return notifier_from_errno(ret);
+}
+
+static struct a1_cpu_clk_nb_data a1_cpu_clk_nb_data = {
+	.mux_ops = &clk_regmap_mux_ops,
+	.cpu_clk = &cpu_clk.hw,
+	.nb.notifier_call = meson_a1_cpu_clk_notifier_cb,
+};
+
+static int meson_a1_dvfs_setup(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct clk *notifier_clk;
+	int ret;
+
+	/* Setup clock notifier for cpu_clk */
+	notifier_clk = devm_clk_hw_get_clk(dev, &cpu_clk.hw, "dvfs");
+	if (IS_ERR(notifier_clk))
+		return dev_err_probe(dev, PTR_ERR(notifier_clk),
+				     "can't get cpu_clk as notifier clock\n");
+
+	ret = devm_clk_notifier_register(dev, notifier_clk,
+					 &a1_cpu_clk_nb_data.nb);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "can't register cpu_clk notifier\n");
+
+	return ret;
+}
+
+static int meson_a1_cpu_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	void __iomem *base;
+	struct regmap *map;
+	int clkid, i, err;
+
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base))
+		return dev_err_probe(dev, PTR_ERR(base),
+				     "can't ioremap resource\n");
+
+	map = devm_regmap_init_mmio(dev, base, &a1_cpu_regmap_cfg);
+	if (IS_ERR(map))
+		return dev_err_probe(dev, PTR_ERR(map),
+				     "can't init regmap mmio region\n");
+
+	/* Populate regmap for the regmap backed clocks */
+	for (i = 0; i < ARRAY_SIZE(a1_cpu_regmaps); i++)
+		a1_cpu_regmaps[i]->map = map;
+
+	for (clkid = 0; clkid < a1_cpu_clks.num; clkid++) {
+		err = devm_clk_hw_register(dev, a1_cpu_clks.hws[clkid]);
+		if (err)
+			return dev_err_probe(dev, err,
+					     "clock[%d] registration failed\n",
+					     clkid);
+	}
+
+	err = devm_of_clk_add_hw_provider(dev, meson_clk_hw_get, &a1_cpu_clks);
+	if (err)
+		return dev_err_probe(dev, err, "can't add clk hw provider\n");
+
+	return meson_a1_dvfs_setup(pdev);
+}
+
+static const struct of_device_id a1_cpu_clkc_match_table[] = {
+	{ .compatible = "amlogic,a1-cpu-clkc", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, a1_cpu_clkc_match_table);
+
+static struct platform_driver a1_cpu_clkc_driver = {
+	.probe = meson_a1_cpu_probe,
+	.driver = {
+		.name = "a1-cpu-clkc",
+		.of_match_table = a1_cpu_clkc_match_table,
+	},
+};
+
+module_platform_driver(a1_cpu_clkc_driver);
+MODULE_AUTHOR("Dmitry Rokosov <[e-mail]>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/clk/meson/a1-cpu.h b/drivers/clk/meson/a1-cpu.h
new file mode 100644
index 000000000000..e9af4117e26f
--- /dev/null
+++ b/drivers/clk/meson/a1-cpu.h
@@ -0,0 +1,16 @@ 
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Amlogic A1 CPU Clock Controller internals
+ *
+ * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
+ * Author: Dmitry Rokosov <[e-mail]>
+ */
+
+#ifndef __A1_CPU_H
+#define __A1_CPU_H
+
+/* cpu clock controller register offset */
+#define CPUCTRL_CLK_CTRL0	0x80
+#define CPUCTRL_CLK_CTRL1	0x84
+
+#endif /* __A1_CPU_H */