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 the cpu_clk rate switching
effectively by transitioning to the CPU fixed clock to avoid any
potential execution freezes.
Validation:
* to double-check all clk flags, run the below helper script:
```
pushd /sys/kernel/debug/clk
for f in *; do
if [[ -f "$f/clk_flags" ]]; then
flags="$(cat $f/clk_flags | awk '{$1=$1};1' | sed ':a;N;$!ba;s/\n/ | /g')"
echo -e "$f: $flags"
fi
done
popd
```
* to trace the current clks state, use the
'/sys/kernel/debug/clk/clk_dump' node with jq post-processing:
```
$ cat /sys/kernel/debug/clk/clk_dump | jq '.' > clk_dump.json
```
* to see the CPU clock hierarchy, use the
'/sys/kernel/debug/clk/clk_summary' node with jq post-processing:
```
$ cat /sys/kernel/debug/clk/clk_summary | jq '.' > clk_dump.json
```
when cpu_clk is inherited from sys_pll, it should be:
```
syspll_in 1 1 0 24000000 0 0 50000 Y deviceless no_connection_id
sys_pll 2 2 0 1200000000 0 0 50000 Y deviceless no_connection_id
cpu_clk 1 1 0 1200000000 0 0 50000 Y cpu0 no_connection_id
cpu0 no_connection_id
fd000000.clock-controller dvfs
deviceless no_connection_id
```
and from cpu fixed clock:
```
fclk_div3_div 1 1 0 512000000 0 0 50000 Y deviceless no_connection_id
fclk_div3 4 4 0 512000000 0 0 50000 Y deviceless no_connection_id
cpu_fsource_sel0 1 1 0 512000000 0 0 50000 Y deviceless no_connection_id
cpu_fsource_div0 1 1 0 128000000 0 0 50000 Y deviceless no_connection_id
cpu_fsel0 1 1 0 128000000 0 0 50000 Y deviceless no_connection_id
cpu_fclk 1 1 0 128000000 0 0 50000 Y deviceless no_connection_id
cpu_clk 1 1 0 128000000 0 0 50000 Y cpu0 no_connection_id
cpu0 no_connection_id
fd000000.clock-controller dvfs
deviceless no_connection_id
```
* to debug cpu clk rate propagation and proper parent switching, compile
kernel with the following definition:
$ sed -i "s/undef CLOCK_ALLOW_WRITE_DEBUGFS/define CLOCK_ALLOW_WRITE_DEBUGFS/g" drivers/clk/clk.c
after that, clk_rate debug node for each clock will be available for
write operation
Dmitry Rokosov (6):
dt-bindings: clock: meson: a1: pll: introduce new syspll bindings
clk: meson: a1: pll: support 'syspll' general-purpose PLL for CPU
clock
dt-bindings: clock: meson: a1: peripherals: support sys_pll_div16
input
clk: meson: a1: peripherals: support 'sys_pll_div16' clock as GEN
input
dt-bindings: clock: meson: add A1 CPU clock controller bindings
clk: meson: a1: add Amlogic A1 CPU clock controller driver
.../bindings/clock/amlogic,a1-cpu-clkc.yaml | 64 ++++
.../clock/amlogic,a1-peripherals-clkc.yaml | 5 +-
.../bindings/clock/amlogic,a1-pll-clkc.yaml | 7 +-
drivers/clk/meson/Kconfig | 10 +
drivers/clk/meson/Makefile | 1 +
drivers/clk/meson/a1-cpu.c | 324 ++++++++++++++++++
drivers/clk/meson/a1-cpu.h | 16 +
drivers/clk/meson/a1-peripherals.c | 4 +-
drivers/clk/meson/a1-pll.c | 78 +++++
drivers/clk/meson/a1-pll.h | 6 +
.../dt-bindings/clock/amlogic,a1-cpu-clkc.h | 19 +
.../dt-bindings/clock/amlogic,a1-pll-clkc.h | 2 +
12 files changed, 531 insertions(+), 5 deletions(-)
create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-cpu-clkc.yaml
create mode 100644 drivers/clk/meson/a1-cpu.c
create mode 100644 drivers/clk/meson/a1-cpu.h
create mode 100644 include/dt-bindings/clock/amlogic,a1-cpu-clkc.h
--
2.43.0
The 'sys_pll_div16' input clock is used as one of the sources for the
GEN clock.
Signed-off-by: Dmitry Rokosov <[email protected]>
---
.../bindings/clock/amlogic,a1-peripherals-clkc.yaml | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml
index 6d84cee1bd75..f6668991ff1f 100644
--- a/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml
+++ b/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml
@@ -29,6 +29,7 @@ properties:
- description: input fixed pll div5
- description: input fixed pll div7
- description: input hifi pll
+ - description: input sys pll div16
- description: input oscillator (usually at 24MHz)
clock-names:
@@ -38,6 +39,7 @@ properties:
- const: fclk_div5
- const: fclk_div7
- const: hifi_pll
+ - const: sys_pll_div16
- const: xtal
required:
@@ -65,9 +67,10 @@ examples:
<&clkc_pll CLKID_FCLK_DIV5>,
<&clkc_pll CLKID_FCLK_DIV7>,
<&clkc_pll CLKID_HIFI_PLL>,
+ <&clkc_pll CLKID_SYS_PLL_DIV16>,
<&xtal>;
clock-names = "fclk_div2", "fclk_div3",
"fclk_div5", "fclk_div7",
- "hifi_pll", "xtal";
+ "hifi_pll", "sys_pll_div16", "xtal";
};
};
--
2.43.0
Add the documentation and dt bindings for Amlogic A1 CPU clock
controller.
This controller consists of the general 'cpu_clk' and two main parents:
'cpu fixed clock' and 'syspll'. The 'cpu fixed clock' is an internal
fixed clock, while the 'syspll' serves as an external input from the A1
PLL clock controller.
Signed-off-by: Dmitry Rokosov <[email protected]>
---
.../bindings/clock/amlogic,a1-cpu-clkc.yaml | 64 +++++++++++++++++++
.../dt-bindings/clock/amlogic,a1-cpu-clkc.h | 19 ++++++
2 files changed, 83 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-cpu-clkc.yaml
create mode 100644 include/dt-bindings/clock/amlogic,a1-cpu-clkc.h
diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-cpu-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-cpu-clkc.yaml
new file mode 100644
index 000000000000..d52d2e084ae7
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/amlogic,a1-cpu-clkc.yaml
@@ -0,0 +1,64 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/amlogic,a1-cpu-clkc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Amlogic A1 CPU Clock Control Unit
+
+maintainers:
+ - Neil Armstrong <[email protected]>
+ - Jerome Brunet <[email protected]>
+ - Dmitry Rokosov <[email protected]>
+
+properties:
+ compatible:
+ const: amlogic,a1-cpu-clkc
+
+ '#clock-cells':
+ const: 1
+
+ reg:
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: input fixed pll div2
+ - description: input fixed pll div3
+ - description: input sys pll
+ - description: input oscillator (usually at 24MHz)
+
+ clock-names:
+ items:
+ - const: fclk_div2
+ - const: fclk_div3
+ - const: sys_pll
+ - const: xtal
+
+required:
+ - compatible
+ - '#clock-cells'
+ - reg
+ - clocks
+ - clock-names
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/amlogic,a1-pll-clkc.h>
+ apb {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ clock-controller@fd000000 {
+ compatible = "amlogic,a1-cpu-clkc";
+ reg = <0 0xfd000000 0 0x88>;
+ #clock-cells = <1>;
+ clocks = <&clkc_pll CLKID_FCLK_DIV2>,
+ <&clkc_pll CLKID_FCLK_DIV3>,
+ <&clkc_pll CLKID_SYS_PLL>,
+ <&xtal>;
+ clock-names = "fclk_div2", "fclk_div3", "sys_pll", "xtal";
+ };
+ };
diff --git a/include/dt-bindings/clock/amlogic,a1-cpu-clkc.h b/include/dt-bindings/clock/amlogic,a1-cpu-clkc.h
new file mode 100644
index 000000000000..1d321c6eddb7
--- /dev/null
+++ b/include/dt-bindings/clock/amlogic,a1-cpu-clkc.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
+/*
+ * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
+ * Author: Dmitry Rokosov <[email protected]>
+ */
+
+#ifndef __A1_CPU_CLKC_H
+#define __A1_CPU_CLKC_H
+
+#define CLKID_CPU_FSOURCE_SEL0 0
+#define CLKID_CPU_FSOURCE_DIV0 1
+#define CLKID_CPU_FSEL0 2
+#define CLKID_CPU_FSOURCE_SEL1 3
+#define CLKID_CPU_FSOURCE_DIV1 4
+#define CLKID_CPU_FSEL1 5
+#define CLKID_CPU_FCLK 6
+#define CLKID_CPU_CLK 7
+
+#endif /* __A1_CPU_CLKC_H */
--
2.43.0
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 <[email protected]>
---
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 <[email protected]>
+ */
+
+#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 <[email protected]>");
+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 <[email protected]>
+ */
+
+#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 */
--
2.43.0
Hi Dmitry,
On Fri, Mar 29, 2024 at 9:59 PM Dmitry Rokosov
<[email protected]> 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
On Fri, Mar 29, 2024 at 11:58:43PM +0300, Dmitry Rokosov wrote:
> The 'sys_pll_div16' input clock is used as one of the sources for the
> GEN clock.
>
> Signed-off-by: Dmitry Rokosov <[email protected]>
> ---
> .../bindings/clock/amlogic,a1-peripherals-clkc.yaml | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml
> index 6d84cee1bd75..f6668991ff1f 100644
> --- a/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml
> +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml
> @@ -29,6 +29,7 @@ properties:
> - description: input fixed pll div5
> - description: input fixed pll div7
> - description: input hifi pll
> + - description: input sys pll div16
> - description: input oscillator (usually at 24MHz)
>
> clock-names:
> @@ -38,6 +39,7 @@ properties:
> - const: fclk_div5
> - const: fclk_div7
> - const: hifi_pll
> + - const: sys_pll_div16
> - const: xtal
And adding an entry in the middle is also an ABI break. New entries go
on the end (and should be optional).
On Fri, 29 Mar 2024 23:58:45 +0300, Dmitry Rokosov wrote:
> Add the documentation and dt bindings for Amlogic A1 CPU clock
> controller.
>
> This controller consists of the general 'cpu_clk' and two main parents:
> 'cpu fixed clock' and 'syspll'. The 'cpu fixed clock' is an internal
> fixed clock, while the 'syspll' serves as an external input from the A1
> PLL clock controller.
>
> Signed-off-by: Dmitry Rokosov <[email protected]>
> ---
> .../bindings/clock/amlogic,a1-cpu-clkc.yaml | 64 +++++++++++++++++++
> .../dt-bindings/clock/amlogic,a1-cpu-clkc.h | 19 ++++++
> 2 files changed, 83 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/clock/amlogic,a1-cpu-clkc.yaml
> create mode 100644 include/dt-bindings/clock/amlogic,a1-cpu-clkc.h
>
Reviewed-by: Rob Herring <[email protected]>
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
> <[email protected]> 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.
--
Thank you,
Dmitry
Hello Rob,
Thank you for the quick review.
On Mon, Apr 01, 2024 at 09:21:36AM -0500, Rob Herring wrote:
> On Fri, Mar 29, 2024 at 11:58:43PM +0300, Dmitry Rokosov wrote:
> > The 'sys_pll_div16' input clock is used as one of the sources for the
> > GEN clock.
> >
> > Signed-off-by: Dmitry Rokosov <[email protected]>
> > ---
> > .../bindings/clock/amlogic,a1-peripherals-clkc.yaml | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml
> > index 6d84cee1bd75..f6668991ff1f 100644
> > --- a/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml
> > +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml
> > @@ -29,6 +29,7 @@ properties:
> > - description: input fixed pll div5
> > - description: input fixed pll div7
> > - description: input hifi pll
> > + - description: input sys pll div16
> > - description: input oscillator (usually at 24MHz)
> >
> > clock-names:
> > @@ -38,6 +39,7 @@ properties:
> > - const: fclk_div5
> > - const: fclk_div7
> > - const: hifi_pll
> > + - const: sys_pll_div16
> > - const: xtal
>
> And adding an entry in the middle is also an ABI break. New entries go
> on the end (and should be optional).
The clock source sys_pll_div16, being one of the GEN clock parents,
plays a crucial role and cannot be tagged as "optional". Unfortunately,
it was not implemented earlier due to the cpu clock ctrl driver's
pending status on the TODO list.
I would greatly appreciate your advice on the best and simplest way to
resolve this matter in an effective manner..
--
Thank you,
Dmitry
On Mon 01 Apr 2024 at 20:12, Dmitry Rokosov <[email protected]> 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
>> <[email protected]> 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
On Fri 29 Mar 2024 at 23:58, Dmitry Rokosov <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
> + */
> +
> +#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 <[email protected]>");
> +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 <[email protected]>
> + */
> +
> +#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 */
--
Jerome
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 <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > + */
> > +
> > +#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 <[email protected]>");
> > +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 <[email protected]>
> > + */
> > +
> > +#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
[...]
--
Thank you,
Dmitry
On Tue, Apr 02, 2024 at 11:27:24AM +0200, Jerome Brunet wrote:
>
> On Mon 01 Apr 2024 at 20:12, Dmitry Rokosov <[email protected]> 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
> >> <[email protected]> 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.
--
Thank you,
Dmitry
On Tue 02 Apr 2024 at 14:05, Dmitry Rokosov <[email protected]> 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 <[email protected]> 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 <[email protected]>
>> > ---
>> > 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 <[email protected]>
>> > + */
>> > +
>> > +#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 <[email protected]>");
>> > +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 <[email protected]>
>> > + */
>> > +
>> > +#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 ...
>
> [...]
--
Jerome
On Tue, Apr 02, 2024 at 04:11:12PM +0200, Jerome Brunet wrote:
>
> On Tue 02 Apr 2024 at 14:05, Dmitry Rokosov <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >> > ---
> >> > 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 <[email protected]>
> >> > + */
> >> > +
> >> > +#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 <[email protected]>");
> >> > +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 <[email protected]>
> >> > + */
> >> > +
> >> > +#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.
--
Thank you,
Dmitry
On Tue 02 Apr 2024 at 19:11, Dmitry Rokosov <[email protected]> wrote:
> On Tue, Apr 02, 2024 at 04:11:12PM +0200, Jerome Brunet wrote:
>>
>> On Tue 02 Apr 2024 at 14:05, Dmitry Rokosov <[email protected]> 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 <[email protected]> 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 <[email protected]>
>> >> > ---
>> >> > 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 <[email protected]>
>> >> > + */
>> >> > +
>> >> > +#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 <[email protected]>");
>> >> > +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 <[email protected]>
>> >> > + */
>> >> > +
>> >> > +#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.
--
Jerome