[PATCH v3 0/7] clk: meson: introduce Amlogic A1 SoC Family CPU clock controller driver

[PATCH v3 0/7] clk: meson: introduce Amlogic A1 SoC Family CPU clock controller driver

[Dmitry Rokosov]

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

Changes v3 since v2 at [2]:
    - rename CLK_MESON_PLL_INIT_ONCE to CLK_MESON_PLL_NOINIT_ENABLED to
      accurately describe the behavior when we don't run the
      initialization sequence for an already enabled PLL
    - provide accurate comment about CLK_MESON_PLL_NOINIT_ENABLED flag
      to meson_clk_pll_init() and A1 sys_pll clock definition
    - tag syspll_in and sys_pll input clocks as optional in the a1-pll
      and a1-peripherals clkc bindings per Conor and Rob suggestion
    - move sys_pll_div16 clock from a1-pll clkc to a1-peripherals clkc
      as Jerome suggested

Changes v2 since v1 at [1]:
    - introduce new 'INIT_ONCE' flag to eliminate init for already
      enabled PLL
    - explain why we need to break ABI for a1-pll driver by adding
      sys_pll connections
    - implement sys_pll init sequence, which is applicable when sys_pll
      is disabled
    - remove CLK_IS_CRITICAL from sys_pll
    - move sys_pll_div16 binding to the end per Rob's suggestion
    - add Rob's RvB
    - remove holes from the beginning of the cpu clock controller regmap
    - move a1-cpu.h registers offsets definition to a1-cpu.c
    - set CLK_SET_RATE_GATE for parallel cpu fixed clock source trees
      per Martin's and Jerome's suggestion
    - redesign clock notifier block from cpu_clk to sys_pll to keep
      cpu_clock working continuously (the same implementation is located
      in the g12a clock driver)

Links:
    [1] https://lore.kernel.org/all/20240329205904.25002-1-ddrokosov@salutedevices.com/
    [2] https://lore.kernel.org/all/20240510090933.19464-1-ddrokosov@salutedevices.com/

Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>

Dmitry Rokosov (7):
  clk: meson: add 'NOINIT_ENABLED' flag to eliminate init for enabled
    PLL
  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 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    |   9 +-
 .../bindings/clock/amlogic,a1-pll-clkc.yaml   |   9 +-
 drivers/clk/meson/Kconfig                     |  10 +
 drivers/clk/meson/Makefile                    |   1 +
 drivers/clk/meson/a1-cpu.c                    | 331 ++++++++++++++++++
 drivers/clk/meson/a1-peripherals.c            |  18 +-
 drivers/clk/meson/a1-pll.c                    |  72 ++++
 drivers/clk/meson/a1-pll.h                    |   6 +
 drivers/clk/meson/clk-pll.c                   |  40 ++-
 drivers/clk/meson/clk-pll.h                   |   1 +
 .../dt-bindings/clock/amlogic,a1-cpu-clkc.h   |  19 +
 .../clock/amlogic,a1-peripherals-clkc.h       |   1 +
 .../dt-bindings/clock/amlogic,a1-pll-clkc.h   |   1 +
 14 files changed, 560 insertions(+), 22 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 include/dt-bindings/clock/amlogic,a1-cpu-clkc.h

-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 1/7] clk: meson: add 'NOINIT_ENABLED' flag to eliminate init for enabled PLL

[Dmitry Rokosov]

When dealing with certain PLLs, it is necessary to avoid modifying them
if they have already been initialized by lower levels. For instance, in
the A1 SoC Family, the sys_pll is enabled as the parent for the cpuclk,
and it cannot be disabled during the initialization sequence. Therefore,
initialization phase must be skipped.

Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 drivers/clk/meson/clk-pll.c | 40 ++++++++++++++++++++++---------------
 drivers/clk/meson/clk-pll.h |  1 +
 2 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
index 78d17b2415af..e1132a110aab 100644
--- a/drivers/clk/meson/clk-pll.c
+++ b/drivers/clk/meson/clk-pll.c
@@ -289,11 +289,35 @@ static int meson_clk_pll_wait_lock(struct clk_hw *hw)
 	return -ETIMEDOUT;
 }
 
+static int meson_clk_pll_is_enabled(struct clk_hw *hw)
+{
+	struct clk_regmap *clk = to_clk_regmap(hw);
+	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
+
+	if (MESON_PARM_APPLICABLE(&pll->rst) &&
+	    meson_parm_read(clk->map, &pll->rst))
+		return 0;
+
+	if (!meson_parm_read(clk->map, &pll->en) ||
+	    !meson_parm_read(clk->map, &pll->l))
+		return 0;
+
+	return 1;
+}
+
 static int meson_clk_pll_init(struct clk_hw *hw)
 {
 	struct clk_regmap *clk = to_clk_regmap(hw);
 	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
 
+	/*
+	 * Keep the clock running, which was already initialized and enabled
+	 * from the bootloader stage, to avoid any glitches.
+	 */
+	if ((pll->flags & CLK_MESON_PLL_NOINIT_ENABLED) &&
+	    meson_clk_pll_is_enabled(hw))
+		return 0;
+
 	if (pll->init_count) {
 		if (MESON_PARM_APPLICABLE(&pll->rst))
 			meson_parm_write(clk->map, &pll->rst, 1);
@@ -308,22 +332,6 @@ static int meson_clk_pll_init(struct clk_hw *hw)
 	return 0;
 }
 
-static int meson_clk_pll_is_enabled(struct clk_hw *hw)
-{
-	struct clk_regmap *clk = to_clk_regmap(hw);
-	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
-
-	if (MESON_PARM_APPLICABLE(&pll->rst) &&
-	    meson_parm_read(clk->map, &pll->rst))
-		return 0;
-
-	if (!meson_parm_read(clk->map, &pll->en) ||
-	    !meson_parm_read(clk->map, &pll->l))
-		return 0;
-
-	return 1;
-}
-
 static int meson_clk_pcie_pll_enable(struct clk_hw *hw)
 {
 	int retries = 10;
diff --git a/drivers/clk/meson/clk-pll.h b/drivers/clk/meson/clk-pll.h
index a2228c0fdce5..7b6b87274073 100644
--- a/drivers/clk/meson/clk-pll.h
+++ b/drivers/clk/meson/clk-pll.h
@@ -28,6 +28,7 @@ struct pll_mult_range {
 	}
 
 #define CLK_MESON_PLL_ROUND_CLOSEST	BIT(0)
+#define CLK_MESON_PLL_NOINIT_ENABLED	BIT(1)
 
 struct meson_clk_pll_data {
 	struct parm en;
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 2/7] dt-bindings: clock: meson: a1: pll: introduce new syspll bindings

[Dmitry Rokosov]

The 'syspll' PLL is a general-purpose PLL designed specifically for the
CPU clock. It is capable of producing output frequencies within the
range of 768MHz to 1536MHz.

The 'syspll_in' source clock is an optional parent connection from the
peripherals clock controller.

Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 .../devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml   | 9 +++++++--
 include/dt-bindings/clock/amlogic,a1-pll-clkc.h          | 1 +
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
index a59b188a8bf5..c99274d2a9bd 100644
--- a/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
+++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
@@ -26,11 +26,15 @@ properties:
     items:
       - description: input fixpll_in
       - description: input hifipll_in
+      - description: input syspll_in
+    minItems: 2 # syspll_in is optional
 
   clock-names:
     items:
       - const: fixpll_in
       - const: hifipll_in
+      - const: syspll_in
+    minItems: 2 # syspll_in is optional
 
 required:
   - compatible
@@ -53,7 +57,8 @@ examples:
             reg = <0 0x7c80 0 0x18c>;
             #clock-cells = <1>;
             clocks = <&clkc_periphs CLKID_FIXPLL_IN>,
-                     <&clkc_periphs CLKID_HIFIPLL_IN>;
-            clock-names = "fixpll_in", "hifipll_in";
+                     <&clkc_periphs CLKID_HIFIPLL_IN>,
+                     <&clkc_periphs CLKID_SYSPLL_IN>;
+            clock-names = "fixpll_in", "hifipll_in", "syspll_in";
         };
     };
diff --git a/include/dt-bindings/clock/amlogic,a1-pll-clkc.h b/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
index 2b660c0f2c9f..0dfc5e78a2d5 100644
--- a/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
+++ b/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
@@ -21,5 +21,6 @@
 #define CLKID_FCLK_DIV5		8
 #define CLKID_FCLK_DIV7		9
 #define CLKID_HIFI_PLL		10
+#define CLKID_SYS_PLL		11
 
 #endif /* __A1_PLL_CLKC_H */
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 3/7] clk: meson: a1: pll: support 'syspll' general-purpose PLL for CPU clock

[Dmitry Rokosov]

The 'syspll' PLL, also known as the system PLL, is a general and
essential PLL responsible for generating the CPU clock frequency.
With its wide-ranging capabilities, it is designed to accommodate
frequencies within the range of 768MHz to 1536MHz.

Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 drivers/clk/meson/a1-pll.c | 72 ++++++++++++++++++++++++++++++++++++++
 drivers/clk/meson/a1-pll.h |  6 ++++
 2 files changed, 78 insertions(+)

diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c
index 60b2e53e7e51..286e83199d17 100644
--- a/drivers/clk/meson/a1-pll.c
+++ b/drivers/clk/meson/a1-pll.c
@@ -138,6 +138,76 @@ static struct clk_regmap hifi_pll = {
 	},
 };
 
+static const struct pll_mult_range sys_pll_mult_range = {
+	.min = 32,
+	.max = 64,
+};
+
+static const struct reg_sequence sys_pll_init_regs[] = {
+	{ .reg = ANACTRL_SYSPLL_CTRL1, .def = 0x01800000 },
+	{ .reg = ANACTRL_SYSPLL_CTRL2, .def = 0x00001100 },
+	{ .reg = ANACTRL_SYSPLL_CTRL3, .def = 0x10022300 },
+	{ .reg = ANACTRL_SYSPLL_CTRL4, .def = 0x00300000 },
+	{ .reg = ANACTRL_SYSPLL_CTRL0, .def = 0x01f18432 },
+};
+
+static struct clk_regmap sys_pll = {
+	.data = &(struct meson_clk_pll_data){
+		.en = {
+			.reg_off = ANACTRL_SYSPLL_CTRL0,
+			.shift   = 28,
+			.width   = 1,
+		},
+		.m = {
+			.reg_off = ANACTRL_SYSPLL_CTRL0,
+			.shift   = 0,
+			.width   = 8,
+		},
+		.n = {
+			.reg_off = ANACTRL_SYSPLL_CTRL0,
+			.shift   = 10,
+			.width   = 5,
+		},
+		.frac = {
+			.reg_off = ANACTRL_SYSPLL_CTRL1,
+			.shift   = 0,
+			.width   = 19,
+		},
+		.l = {
+			.reg_off = ANACTRL_SYSPLL_STS,
+			.shift   = 31,
+			.width   = 1,
+		},
+		.current_en = {
+			.reg_off = ANACTRL_SYSPLL_CTRL0,
+			.shift   = 26,
+			.width   = 1,
+		},
+		.l_detect = {
+			.reg_off = ANACTRL_SYSPLL_CTRL2,
+			.shift   = 6,
+			.width   = 1,
+		},
+		.range = &sys_pll_mult_range,
+		.init_regs = sys_pll_init_regs,
+		.init_count = ARRAY_SIZE(sys_pll_init_regs),
+		/*
+		 * The sys_pll clock is usually enabled and initialized in the
+		 * bootloader stage. Additionally, the cpu_clk is connected to
+		 * sys_pll. As a result, it is not allowed to initialize the
+		 * cpu_clk again, as doing so would prevent the CPU from
+		 * executing any instructions.
+		 */
+		.flags = CLK_MESON_PLL_NOINIT_ENABLED,
+	},
+	.hw.init = &(struct clk_init_data){
+		.name = "sys_pll",
+		.ops = &meson_clk_pll_ops,
+		.parent_names = (const char *[]){ "syspll_in" },
+		.num_parents = 1,
+	},
+};
+
 static struct clk_fixed_factor fclk_div2_div = {
 	.mult = 1,
 	.div = 2,
@@ -283,6 +353,7 @@ static struct clk_hw *a1_pll_hw_clks[] = {
 	[CLKID_FCLK_DIV5]	= &fclk_div5.hw,
 	[CLKID_FCLK_DIV7]	= &fclk_div7.hw,
 	[CLKID_HIFI_PLL]	= &hifi_pll.hw,
+	[CLKID_SYS_PLL]		= &sys_pll.hw,
 };
 
 static struct clk_regmap *const a1_pll_regmaps[] = {
@@ -293,6 +364,7 @@ static struct clk_regmap *const a1_pll_regmaps[] = {
 	&fclk_div5,
 	&fclk_div7,
 	&hifi_pll,
+	&sys_pll,
 };
 
 static struct regmap_config a1_pll_regmap_cfg = {
diff --git a/drivers/clk/meson/a1-pll.h b/drivers/clk/meson/a1-pll.h
index 4be17b2bf383..666d9b2137e9 100644
--- a/drivers/clk/meson/a1-pll.h
+++ b/drivers/clk/meson/a1-pll.h
@@ -18,6 +18,12 @@
 #define ANACTRL_FIXPLL_CTRL0	0x0
 #define ANACTRL_FIXPLL_CTRL1	0x4
 #define ANACTRL_FIXPLL_STS	0x14
+#define ANACTRL_SYSPLL_CTRL0	0x80
+#define ANACTRL_SYSPLL_CTRL1	0x84
+#define ANACTRL_SYSPLL_CTRL2	0x88
+#define ANACTRL_SYSPLL_CTRL3	0x8c
+#define ANACTRL_SYSPLL_CTRL4	0x90
+#define ANACTRL_SYSPLL_STS	0x94
 #define ANACTRL_HIFIPLL_CTRL0	0xc0
 #define ANACTRL_HIFIPLL_CTRL1	0xc4
 #define ANACTRL_HIFIPLL_CTRL2	0xc8
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 4/7] dt-bindings: clock: meson: a1: peripherals: support sys_pll input

[Dmitry Rokosov]

The 'sys_pll' input is an optional clock that can be used to generate
'sys_pll_div16', which serves as one of the sources for the GEN clock.

Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 .../bindings/clock/amlogic,a1-peripherals-clkc.yaml      | 9 +++++++--
 include/dt-bindings/clock/amlogic,a1-peripherals-clkc.h  | 1 +
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml
index 6d84cee1bd75..2568ad7dd0ac 100644
--- a/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml
+++ b/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml
@@ -30,6 +30,8 @@ properties:
       - description: input fixed pll div7
       - description: input hifi pll
       - description: input oscillator (usually at 24MHz)
+      - description: input sys pll
+    minItems: 6 # sys_pll is optional
 
   clock-names:
     items:
@@ -39,6 +41,8 @@ properties:
       - const: fclk_div7
       - const: hifi_pll
       - const: xtal
+      - const: sys_pll
+    minItems: 6 # sys_pll is optional
 
 required:
   - compatible
@@ -65,9 +69,10 @@ examples:
                      <&clkc_pll CLKID_FCLK_DIV5>,
                      <&clkc_pll CLKID_FCLK_DIV7>,
                      <&clkc_pll CLKID_HIFI_PLL>,
-                     <&xtal>;
+                     <&xtal>,
+                     <&clkc_pll CLKID_SYS_PLL>;
             clock-names = "fclk_div2", "fclk_div3",
                           "fclk_div5", "fclk_div7",
-                          "hifi_pll", "xtal";
+                          "hifi_pll", "xtal", "sys_pll";
         };
     };
diff --git a/include/dt-bindings/clock/amlogic,a1-peripherals-clkc.h b/include/dt-bindings/clock/amlogic,a1-peripherals-clkc.h
index 06f198ee7623..2ce1a06dc735 100644
--- a/include/dt-bindings/clock/amlogic,a1-peripherals-clkc.h
+++ b/include/dt-bindings/clock/amlogic,a1-peripherals-clkc.h
@@ -164,5 +164,6 @@
 #define CLKID_DMC_SEL		151
 #define CLKID_DMC_DIV		152
 #define CLKID_DMC_SEL2		153
+#define CLKID_SYS_PLL_DIV16	154
 
 #endif /* __A1_PERIPHERALS_CLKC_H */
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 5/7] clk: meson: a1: peripherals: support 'sys_pll_div16' clock as GEN input

[Dmitry Rokosov]

The clock 'sys_pll_div16' is one of the parents of the GEN clock. It is
generated inside the A1 Peripherals clock controller from 'sys_pll' PLL
clock source with a fixed factor.

Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 drivers/clk/meson/a1-peripherals.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/meson/a1-peripherals.c b/drivers/clk/meson/a1-peripherals.c
index 621af1e6e4b2..56e44299982c 100644
--- a/drivers/clk/meson/a1-peripherals.c
+++ b/drivers/clk/meson/a1-peripherals.c
@@ -746,14 +746,27 @@ static struct clk_regmap fclk_div2_divn = {
 	},
 };
 
+static struct clk_fixed_factor sys_pll_div16 = {
+	.mult = 1,
+	.div = 16,
+	.hw.init = &(struct clk_init_data){
+		.name = "sys_pll_div16",
+		.ops = &clk_fixed_factor_ops,
+		.parent_data = &(const struct clk_parent_data) {
+			.fw_name = "sys_pll",
+		},
+		.num_parents = 1,
+	},
+};
+
 /*
- * the index 2 is sys_pll_div16, it will be implemented in the CPU clock driver,
  * the index 4 is the clock measurement source, it's not supported yet
  */
-static u32 gen_table[] = { 0, 1, 3, 5, 6, 7, 8 };
+static u32 gen_table[] = { 0, 1, 2, 3, 5, 6, 7, 8 };
 static const struct clk_parent_data gen_parent_data[] = {
 	{ .fw_name = "xtal", },
 	{ .hw = &rtc.hw },
+	{ .hw = &sys_pll_div16.hw, },
 	{ .fw_name = "hifi_pll", },
 	{ .fw_name = "fclk_div2", },
 	{ .fw_name = "fclk_div3", },
@@ -2024,6 +2037,7 @@ static struct clk_hw *a1_periphs_hw_clks[] = {
 	[CLKID_DMC_SEL]			= &dmc_sel.hw,
 	[CLKID_DMC_DIV]			= &dmc_div.hw,
 	[CLKID_DMC_SEL2]		= &dmc_sel2.hw,
+	[CLKID_SYS_PLL_DIV16]		= &sys_pll_div16.hw,
 };
 
 /* Convenience table to populate regmap in .probe */
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 6/7] dt-bindings: clock: meson: add A1 CPU clock controller bindings

[Dmitry Rokosov]

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 <ddrokosov@salutedevices.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../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..f4958b315ed4
--- /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 <neil.armstrong@linaro.org>
+  - Jerome Brunet <jbrunet@baylibre.com>
+  - Dmitry Rokosov <ddrokosov@salutedevices.com>
+
+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 0xfd000080 0 0x8>;
+            #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 <ddrokosov@salutedevices.com>
+ */
+
+#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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 7/7] clk: meson: a1: add Amlogic A1 CPU clock controller driver

[Dmitry Rokosov]

The CPU clock controller plays a general role in the Amlogic A1 SoC
family by generating CPU clocks. As an APB slave module, it offers the
capability to inherit the CPU clock from two sources: the internal fixed
clock known as 'cpu fixed clock' and the external input provided by the
A1 PLL clock controller, referred to as 'syspll'.

It is important for the driver to handle cpu_clk rate switching
effectively by transitioning to the CPU fixed clock to avoid any
potential execution freezes.

Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 drivers/clk/meson/Kconfig  |  10 ++
 drivers/clk/meson/Makefile |   1 +
 drivers/clk/meson/a1-cpu.c | 331 +++++++++++++++++++++++++++++++++++++
 3 files changed, 342 insertions(+)
 create mode 100644 drivers/clk/meson/a1-cpu.c

diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
index 80c4a18c83d2..148d4495eee3 100644
--- a/drivers/clk/meson/Kconfig
+++ b/drivers/clk/meson/Kconfig
@@ -111,6 +111,16 @@ config COMMON_CLK_AXG_AUDIO
 	  Support for the audio clock controller on AmLogic A113D devices,
 	  aka axg, Say Y if you want audio subsystem to work.
 
+config COMMON_CLK_A1_CPU
+	tristate "Amlogic A1 SoC CPU controller support"
+	depends on ARM64
+	select COMMON_CLK_MESON_REGMAP
+	select COMMON_CLK_MESON_CLKC_UTILS
+	help
+	  Support for the CPU clock controller on Amlogic A113L based
+	  device, A1 SoC Family. Say Y if you want A1 CPU clock controller
+	  to work.
+
 config COMMON_CLK_A1_PLL
 	tristate "Amlogic A1 SoC PLL controller support"
 	depends on ARM64
diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
index 4968fc7ad555..2a06eb0303d6 100644
--- a/drivers/clk/meson/Makefile
+++ b/drivers/clk/meson/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_COMMON_CLK_MESON_AUDIO_RSTC) += meson-audio-rstc.o
 
 obj-$(CONFIG_COMMON_CLK_AXG) += axg.o axg-aoclk.o
 obj-$(CONFIG_COMMON_CLK_AXG_AUDIO) += axg-audio.o
+obj-$(CONFIG_COMMON_CLK_A1_CPU) += a1-cpu.o
 obj-$(CONFIG_COMMON_CLK_A1_PLL) += a1-pll.o
 obj-$(CONFIG_COMMON_CLK_A1_PERIPHERALS) += a1-peripherals.o
 obj-$(CONFIG_COMMON_CLK_A1_AUDIO) += a1-audio.o
diff --git a/drivers/clk/meson/a1-cpu.c b/drivers/clk/meson/a1-cpu.c
new file mode 100644
index 000000000000..a9edabeafea9
--- /dev/null
+++ b/drivers/clk/meson/a1-cpu.c
@@ -0,0 +1,331 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Amlogic A1 SoC family CPU Clock Controller driver.
+ *
+ * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
+ * Author: Dmitry Rokosov <ddrokosov@salutedevices.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include "clk-regmap.h"
+#include "meson-clkc-utils.h"
+
+#include <dt-bindings/clock/amlogic,a1-cpu-clkc.h>
+
+/* CPU Clock Controller register offset */
+#define CPUCTRL_CLK_CTRL0	0x0
+#define CPUCTRL_CLK_CTRL1	0x4
+
+static u32 cpu_fsource_sel_table[] = { 0, 1, 2 };
+static const struct clk_parent_data cpu_fsource_sel_parents[] = {
+	{ .fw_name = "xtal" },
+	{ .fw_name = "fclk_div2" },
+	{ .fw_name = "fclk_div3" },
+};
+
+static struct clk_regmap cpu_fsource_sel0 = {
+	.data = &(struct clk_regmap_mux_data) {
+		.offset = CPUCTRL_CLK_CTRL0,
+		.mask = 0x3,
+		.shift = 0,
+		.table = cpu_fsource_sel_table,
+	},
+	.hw.init = &(struct clk_init_data) {
+		.name = "cpu_fsource_sel0",
+		.ops = &clk_regmap_mux_ops,
+		.parent_data = cpu_fsource_sel_parents,
+		.num_parents = ARRAY_SIZE(cpu_fsource_sel_parents),
+		.flags = CLK_SET_RATE_PARENT,
+	},
+};
+
+static struct clk_regmap cpu_fsource_div0 = {
+	.data = &(struct clk_regmap_div_data) {
+		.offset = CPUCTRL_CLK_CTRL0,
+		.shift = 4,
+		.width = 6,
+	},
+	.hw.init = &(struct clk_init_data) {
+		.name = "cpu_fsource_div0",
+		.ops = &clk_regmap_divider_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&cpu_fsource_sel0.hw
+		},
+		.num_parents = 1,
+		.flags = CLK_SET_RATE_PARENT,
+	},
+};
+
+static struct clk_regmap cpu_fsel0 = {
+	.data = &(struct clk_regmap_mux_data) {
+		.offset = CPUCTRL_CLK_CTRL0,
+		.mask = 0x1,
+		.shift = 2,
+	},
+	.hw.init = &(struct clk_init_data) {
+		.name = "cpu_fsel0",
+		.ops = &clk_regmap_mux_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&cpu_fsource_sel0.hw,
+			&cpu_fsource_div0.hw,
+		},
+		.num_parents = 2,
+		.flags = CLK_SET_RATE_GATE | CLK_SET_RATE_PARENT,
+	},
+};
+
+static struct clk_regmap cpu_fsource_sel1 = {
+	.data = &(struct clk_regmap_mux_data) {
+		.offset = CPUCTRL_CLK_CTRL0,
+		.mask = 0x3,
+		.shift = 16,
+		.table = cpu_fsource_sel_table,
+	},
+	.hw.init = &(struct clk_init_data) {
+		.name = "cpu_fsource_sel1",
+		.ops = &clk_regmap_mux_ops,
+		.parent_data = cpu_fsource_sel_parents,
+		.num_parents = ARRAY_SIZE(cpu_fsource_sel_parents),
+		.flags = CLK_SET_RATE_PARENT,
+	},
+};
+
+static struct clk_regmap cpu_fsource_div1 = {
+	.data = &(struct clk_regmap_div_data) {
+		.offset = CPUCTRL_CLK_CTRL0,
+		.shift = 20,
+		.width = 6,
+	},
+	.hw.init = &(struct clk_init_data) {
+		.name = "cpu_fsource_div1",
+		.ops = &clk_regmap_divider_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&cpu_fsource_sel1.hw
+		},
+		.num_parents = 1,
+		.flags = CLK_SET_RATE_PARENT,
+	},
+};
+
+static struct clk_regmap cpu_fsel1 = {
+	.data = &(struct clk_regmap_mux_data) {
+		.offset = CPUCTRL_CLK_CTRL0,
+		.mask = 0x1,
+		.shift = 18,
+	},
+	.hw.init = &(struct clk_init_data) {
+		.name = "cpu_fsel1",
+		.ops = &clk_regmap_mux_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&cpu_fsource_sel1.hw,
+			&cpu_fsource_div1.hw,
+		},
+		.num_parents = 2,
+		.flags = CLK_SET_RATE_GATE | CLK_SET_RATE_PARENT,
+	},
+};
+
+static struct clk_regmap cpu_fclk = {
+	.data = &(struct clk_regmap_mux_data) {
+		.offset = CPUCTRL_CLK_CTRL0,
+		.mask = 0x1,
+		.shift = 10,
+	},
+	.hw.init = &(struct clk_init_data) {
+		.name = "cpu_fclk",
+		.ops = &clk_regmap_mux_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&cpu_fsel0.hw,
+			&cpu_fsel1.hw,
+		},
+		.num_parents = 2,
+		.flags = CLK_SET_RATE_PARENT,
+	},
+};
+
+static struct clk_regmap cpu_clk = {
+	.data = &(struct clk_regmap_mux_data) {
+		.offset = CPUCTRL_CLK_CTRL0,
+		.mask = 0x1,
+		.shift = 11,
+	},
+	.hw.init = &(struct clk_init_data) {
+		.name = "cpu_clk",
+		.ops = &clk_regmap_mux_ops,
+		.parent_data = (const struct clk_parent_data []) {
+			{ .hw = &cpu_fclk.hw },
+			{ .fw_name = "sys_pll", },
+		},
+		.num_parents = 2,
+		.flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
+	},
+};
+
+/* Array of all clocks registered by this provider */
+static struct clk_hw *a1_cpu_hw_clks[] = {
+	[CLKID_CPU_FSOURCE_SEL0]	= &cpu_fsource_sel0.hw,
+	[CLKID_CPU_FSOURCE_DIV0]	= &cpu_fsource_div0.hw,
+	[CLKID_CPU_FSEL0]		= &cpu_fsel0.hw,
+	[CLKID_CPU_FSOURCE_SEL1]	= &cpu_fsource_sel1.hw,
+	[CLKID_CPU_FSOURCE_DIV1]	= &cpu_fsource_div1.hw,
+	[CLKID_CPU_FSEL1]		= &cpu_fsel1.hw,
+	[CLKID_CPU_FCLK]		= &cpu_fclk.hw,
+	[CLKID_CPU_CLK]			= &cpu_clk.hw,
+};
+
+static struct clk_regmap *const a1_cpu_regmaps[] = {
+	&cpu_fsource_sel0,
+	&cpu_fsource_div0,
+	&cpu_fsel0,
+	&cpu_fsource_sel1,
+	&cpu_fsource_div1,
+	&cpu_fsel1,
+	&cpu_fclk,
+	&cpu_clk,
+};
+
+static struct regmap_config a1_cpu_regmap_cfg = {
+	.reg_bits   = 32,
+	.val_bits   = 32,
+	.reg_stride = 4,
+	.max_register = CPUCTRL_CLK_CTRL1,
+};
+
+static struct meson_clk_hw_data a1_cpu_clks = {
+	.hws = a1_cpu_hw_clks,
+	.num = ARRAY_SIZE(a1_cpu_hw_clks),
+};
+
+struct a1_sys_pll_nb_data {
+	struct notifier_block nb;
+	struct clk_hw *cpu_clk;
+	struct clk_hw *cpu_fclk;
+	struct clk *sys_pll;
+};
+
+static int meson_a1_sys_pll_notifier_cb(struct notifier_block *nb,
+					unsigned long event, void *data)
+{
+	struct a1_sys_pll_nb_data *nbd;
+	int ret = 0;
+
+	nbd = container_of(nb, struct a1_sys_pll_nb_data, nb);
+
+	switch (event) {
+	case PRE_RATE_CHANGE:
+		/*
+		 * Clock sys_pll will be changed to feed cpu_clk,
+		 * configure cpu_clk to use cpu_fclk fixed clock.
+		 */
+		ret = clk_hw_set_parent(nbd->cpu_clk, nbd->cpu_fclk);
+
+		/* Wait for clock propagation */
+		if (!ret)
+			udelay(100);
+
+		break;
+
+	case POST_RATE_CHANGE:
+		 /*
+		  * Clock sys_pll rate has ben calculated,
+		  * switch back cpu_clk to sys_pll
+		  */
+		ret = clk_set_parent(nbd->cpu_clk->clk, nbd->sys_pll);
+
+		/* Wait for clock propagation */
+		if (!ret)
+			udelay(100);
+		break;
+
+	default:
+		pr_warn("Unknown event %lu for sys_pll notifier\n", event);
+		break;
+	}
+
+	return notifier_from_errno(ret);
+}
+
+static struct a1_sys_pll_nb_data a1_sys_pll_nb_data = {
+	.nb.notifier_call = meson_a1_sys_pll_notifier_cb,
+	.cpu_clk = &cpu_clk.hw,
+	.cpu_fclk = &cpu_fclk.hw,
+};
+
+static int meson_a1_dvfs_setup(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct clk *sys_pll;
+	int ret;
+
+	/* Setup clock notifier for sys_pll clk */
+	sys_pll = devm_clk_get(dev, "sys_pll");
+	if (IS_ERR(sys_pll))
+		return dev_err_probe(dev, PTR_ERR(sys_pll),
+				     "can't get sys_pll as notifier clock\n");
+
+	a1_sys_pll_nb_data.sys_pll = sys_pll;
+	ret = devm_clk_notifier_register(dev, sys_pll,
+					 &a1_sys_pll_nb_data.nb);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "can't register sys_pll notifier\n");
+
+	return ret;
+}
+
+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 <ddrokosov@salutedevices.com>");
+MODULE_LICENSE("GPL");
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 4/7] dt-bindings: clock: meson: a1: peripherals: support sys_pll input

[Dmitry Rokosov]

Hello Conor,

On Wed, May 15, 2024 at 09:47:27PM +0300, Dmitry Rokosov wrote:
> The 'sys_pll' input is an optional clock that can be used to generate
> 'sys_pll_div16', which serves as one of the sources for the GEN clock.
> 
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>

I didn't tag this patch with your Acked-by because I've changed the
connection between the A1 PLL and the A1 peripherals controllers from
'sys_pll_div16' to 'sys_pll'.

Please review this patch again, if possible.

> ---
>  .../bindings/clock/amlogic,a1-peripherals-clkc.yaml      | 9 +++++++--
>  include/dt-bindings/clock/amlogic,a1-peripherals-clkc.h  | 1 +
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml
> index 6d84cee1bd75..2568ad7dd0ac 100644
> --- a/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml
> +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml
> @@ -30,6 +30,8 @@ properties:
>        - description: input fixed pll div7
>        - description: input hifi pll
>        - description: input oscillator (usually at 24MHz)
> +      - description: input sys pll
> +    minItems: 6 # sys_pll is optional
>  
>    clock-names:
>      items:
> @@ -39,6 +41,8 @@ properties:
>        - const: fclk_div7
>        - const: hifi_pll
>        - const: xtal
> +      - const: sys_pll
> +    minItems: 6 # sys_pll is optional
>  
>  required:
>    - compatible
> @@ -65,9 +69,10 @@ examples:
>                       <&clkc_pll CLKID_FCLK_DIV5>,
>                       <&clkc_pll CLKID_FCLK_DIV7>,
>                       <&clkc_pll CLKID_HIFI_PLL>,
> -                     <&xtal>;
> +                     <&xtal>,
> +                     <&clkc_pll CLKID_SYS_PLL>;
>              clock-names = "fclk_div2", "fclk_div3",
>                            "fclk_div5", "fclk_div7",
> -                          "hifi_pll", "xtal";
> +                          "hifi_pll", "xtal", "sys_pll";
>          };
>      };
> diff --git a/include/dt-bindings/clock/amlogic,a1-peripherals-clkc.h b/include/dt-bindings/clock/amlogic,a1-peripherals-clkc.h
> index 06f198ee7623..2ce1a06dc735 100644
> --- a/include/dt-bindings/clock/amlogic,a1-peripherals-clkc.h
> +++ b/include/dt-bindings/clock/amlogic,a1-peripherals-clkc.h
> @@ -164,5 +164,6 @@
>  #define CLKID_DMC_SEL		151
>  #define CLKID_DMC_DIV		152
>  #define CLKID_DMC_SEL2		153
> +#define CLKID_SYS_PLL_DIV16	154
>  
>  #endif /* __A1_PERIPHERALS_CLKC_H */
> -- 
> 2.43.0
> 

-- 
Thank you,
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/7] dt-bindings: clock: meson: a1: pll: introduce new syspll bindings

[Rob Herring (Arm)]

On Wed, 15 May 2024 21:47:25 +0300, Dmitry Rokosov wrote:
> The 'syspll' PLL is a general-purpose PLL designed specifically for the
> CPU clock. It is capable of producing output frequencies within the
> range of 768MHz to 1536MHz.
> 
> The 'syspll_in' source clock is an optional parent connection from the
> peripherals clock controller.
> 
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> ---
>  .../devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml   | 9 +++++++--
>  include/dt-bindings/clock/amlogic,a1-pll-clkc.h          | 1 +
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 

Acked-by: Rob Herring (Arm) <robh@kernel.org>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 4/7] dt-bindings: clock: meson: a1: peripherals: support sys_pll input

[Rob Herring (Arm)]

On Wed, 15 May 2024 21:47:27 +0300, Dmitry Rokosov wrote:
> The 'sys_pll' input is an optional clock that can be used to generate
> 'sys_pll_div16', which serves as one of the sources for the GEN clock.
> 
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> ---
>  .../bindings/clock/amlogic,a1-peripherals-clkc.yaml      | 9 +++++++--
>  include/dt-bindings/clock/amlogic,a1-peripherals-clkc.h  | 1 +
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 

Acked-by: Rob Herring (Arm) <robh@kernel.org>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 0/7] clk: meson: introduce Amlogic A1 SoC Family CPU clock controller driver

[Dmitry Rokosov]

Hello guys!

Kindly reminder :)

On Wed, May 15, 2024 at 09:47:23PM +0300, Dmitry Rokosov 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 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
> 
> Changes v3 since v2 at [2]:
>     - rename CLK_MESON_PLL_INIT_ONCE to CLK_MESON_PLL_NOINIT_ENABLED to
>       accurately describe the behavior when we don't run the
>       initialization sequence for an already enabled PLL
>     - provide accurate comment about CLK_MESON_PLL_NOINIT_ENABLED flag
>       to meson_clk_pll_init() and A1 sys_pll clock definition
>     - tag syspll_in and sys_pll input clocks as optional in the a1-pll
>       and a1-peripherals clkc bindings per Conor and Rob suggestion
>     - move sys_pll_div16 clock from a1-pll clkc to a1-peripherals clkc
>       as Jerome suggested
> 
> Changes v2 since v1 at [1]:
>     - introduce new 'INIT_ONCE' flag to eliminate init for already
>       enabled PLL
>     - explain why we need to break ABI for a1-pll driver by adding
>       sys_pll connections
>     - implement sys_pll init sequence, which is applicable when sys_pll
>       is disabled
>     - remove CLK_IS_CRITICAL from sys_pll
>     - move sys_pll_div16 binding to the end per Rob's suggestion
>     - add Rob's RvB
>     - remove holes from the beginning of the cpu clock controller regmap
>     - move a1-cpu.h registers offsets definition to a1-cpu.c
>     - set CLK_SET_RATE_GATE for parallel cpu fixed clock source trees
>       per Martin's and Jerome's suggestion
>     - redesign clock notifier block from cpu_clk to sys_pll to keep
>       cpu_clock working continuously (the same implementation is located
>       in the g12a clock driver)
> 
> Links:
>     [1] https://lore.kernel.org/all/20240329205904.25002-1-ddrokosov@salutedevices.com/
>     [2] https://lore.kernel.org/all/20240510090933.19464-1-ddrokosov@salutedevices.com/
> 
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> 
> Dmitry Rokosov (7):
>   clk: meson: add 'NOINIT_ENABLED' flag to eliminate init for enabled
>     PLL
>   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 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    |   9 +-
>  .../bindings/clock/amlogic,a1-pll-clkc.yaml   |   9 +-
>  drivers/clk/meson/Kconfig                     |  10 +
>  drivers/clk/meson/Makefile                    |   1 +
>  drivers/clk/meson/a1-cpu.c                    | 331 ++++++++++++++++++
>  drivers/clk/meson/a1-peripherals.c            |  18 +-
>  drivers/clk/meson/a1-pll.c                    |  72 ++++
>  drivers/clk/meson/a1-pll.h                    |   6 +
>  drivers/clk/meson/clk-pll.c                   |  40 ++-
>  drivers/clk/meson/clk-pll.h                   |   1 +
>  .../dt-bindings/clock/amlogic,a1-cpu-clkc.h   |  19 +
>  .../clock/amlogic,a1-peripherals-clkc.h       |   1 +
>  .../dt-bindings/clock/amlogic,a1-pll-clkc.h   |   1 +
>  14 files changed, 560 insertions(+), 22 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 include/dt-bindings/clock/amlogic,a1-cpu-clkc.h
> 
> -- 
> 2.43.0
> 

-- 
Thank you,
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/7] clk: meson: a1: pll: support 'syspll' general-purpose PLL for CPU clock

[Jerome Brunet]

On Wed 15 May 2024 at 21:47, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:

> The 'syspll' PLL, also known as the system PLL, is a general and
> essential PLL responsible for generating the CPU clock frequency.
> With its wide-ranging capabilities, it is designed to accommodate
> frequencies within the range of 768MHz to 1536MHz.
>
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> ---
>  drivers/clk/meson/a1-pll.c | 72 ++++++++++++++++++++++++++++++++++++++
>  drivers/clk/meson/a1-pll.h |  6 ++++
>  2 files changed, 78 insertions(+)
>
> diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c
> index 60b2e53e7e51..286e83199d17 100644
> --- a/drivers/clk/meson/a1-pll.c
> +++ b/drivers/clk/meson/a1-pll.c
> @@ -138,6 +138,76 @@ static struct clk_regmap hifi_pll = {
>  	},
>  };
>  
> +static const struct pll_mult_range sys_pll_mult_range = {
> +	.min = 32,
> +	.max = 64,
> +};
> +
> +static const struct reg_sequence sys_pll_init_regs[] = {
> +	{ .reg = ANACTRL_SYSPLL_CTRL1, .def = 0x01800000 },
> +	{ .reg = ANACTRL_SYSPLL_CTRL2, .def = 0x00001100 },
> +	{ .reg = ANACTRL_SYSPLL_CTRL3, .def = 0x10022300 },
> +	{ .reg = ANACTRL_SYSPLL_CTRL4, .def = 0x00300000 },
> +	{ .reg = ANACTRL_SYSPLL_CTRL0, .def = 0x01f18432 },

That last entry is clearly an hard coded rate being poked.
Drop it please

> +};
> +
> +static struct clk_regmap sys_pll = {
> +	.data = &(struct meson_clk_pll_data){
> +		.en = {
> +			.reg_off = ANACTRL_SYSPLL_CTRL0,
> +			.shift   = 28,
> +			.width   = 1,
> +		},
> +		.m = {
> +			.reg_off = ANACTRL_SYSPLL_CTRL0,
> +			.shift   = 0,
> +			.width   = 8,
> +		},
> +		.n = {
> +			.reg_off = ANACTRL_SYSPLL_CTRL0,
> +			.shift   = 10,
> +			.width   = 5,
> +		},
> +		.frac = {
> +			.reg_off = ANACTRL_SYSPLL_CTRL1,
> +			.shift   = 0,
> +			.width   = 19,
> +		},
> +		.l = {
> +			.reg_off = ANACTRL_SYSPLL_STS,
> +			.shift   = 31,
> +			.width   = 1,
> +		},
> +		.current_en = {
> +			.reg_off = ANACTRL_SYSPLL_CTRL0,
> +			.shift   = 26,
> +			.width   = 1,
> +		},
> +		.l_detect = {
> +			.reg_off = ANACTRL_SYSPLL_CTRL2,
> +			.shift   = 6,
> +			.width   = 1,
> +		},
> +		.range = &sys_pll_mult_range,
> +		.init_regs = sys_pll_init_regs,
> +		.init_count = ARRAY_SIZE(sys_pll_init_regs),
> +		/*
> +		 * The sys_pll clock is usually enabled and initialized in the
> +		 * bootloader stage. Additionally, the cpu_clk is connected to
> +		 * sys_pll. As a result, it is not allowed to initialize the
> +		 * cpu_clk again, as doing so would prevent the CPU from
> +		 * executing any instructions.
> +		 */
> +		.flags = CLK_MESON_PLL_NOINIT_ENABLED,
> +	},
> +	.hw.init = &(struct clk_init_data){
> +		.name = "sys_pll",
> +		.ops = &meson_clk_pll_ops,
> +		.parent_names = (const char *[]){ "syspll_in" },
> +		.num_parents = 1,
> +	},
> +};
> +
>  static struct clk_fixed_factor fclk_div2_div = {
>  	.mult = 1,
>  	.div = 2,
> @@ -283,6 +353,7 @@ static struct clk_hw *a1_pll_hw_clks[] = {
>  	[CLKID_FCLK_DIV5]	= &fclk_div5.hw,
>  	[CLKID_FCLK_DIV7]	= &fclk_div7.hw,
>  	[CLKID_HIFI_PLL]	= &hifi_pll.hw,
> +	[CLKID_SYS_PLL]		= &sys_pll.hw,
>  };
>  
>  static struct clk_regmap *const a1_pll_regmaps[] = {
> @@ -293,6 +364,7 @@ static struct clk_regmap *const a1_pll_regmaps[] = {
>  	&fclk_div5,
>  	&fclk_div7,
>  	&hifi_pll,
> +	&sys_pll,
>  };
>  
>  static struct regmap_config a1_pll_regmap_cfg = {
> diff --git a/drivers/clk/meson/a1-pll.h b/drivers/clk/meson/a1-pll.h
> index 4be17b2bf383..666d9b2137e9 100644
> --- a/drivers/clk/meson/a1-pll.h
> +++ b/drivers/clk/meson/a1-pll.h
> @@ -18,6 +18,12 @@
>  #define ANACTRL_FIXPLL_CTRL0	0x0
>  #define ANACTRL_FIXPLL_CTRL1	0x4
>  #define ANACTRL_FIXPLL_STS	0x14
> +#define ANACTRL_SYSPLL_CTRL0	0x80
> +#define ANACTRL_SYSPLL_CTRL1	0x84
> +#define ANACTRL_SYSPLL_CTRL2	0x88
> +#define ANACTRL_SYSPLL_CTRL3	0x8c
> +#define ANACTRL_SYSPLL_CTRL4	0x90
> +#define ANACTRL_SYSPLL_STS	0x94
>  #define ANACTRL_HIFIPLL_CTRL0	0xc0
>  #define ANACTRL_HIFIPLL_CTRL1	0xc4
>  #define ANACTRL_HIFIPLL_CTRL2	0xc8

-- 
Jerome

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 6/7] dt-bindings: clock: meson: add A1 CPU clock controller bindings

[Jerome Brunet]

On Wed 15 May 2024 at 21:47, Dmitry Rokosov <ddrokosov@salutedevices.com> 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 <ddrokosov@salutedevices.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
>  .../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..f4958b315ed4
> --- /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 <neil.armstrong@linaro.org>
> +  - Jerome Brunet <jbrunet@baylibre.com>
> +  - Dmitry Rokosov <ddrokosov@salutedevices.com>
> +
> +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)

According to the documentation, fdiv5 is also an input of the CPU clock
tree.

That is typically the kind of things we'd prefer to get right from the
beginning to avoid modifying the bindings later.

> +
> +  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 0xfd000080 0 0x8>;

If reg is <0 0xfd000080 0 0x8> then node name should be clock-controller@fd000080

> +            #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 <ddrokosov@salutedevices.com>
> + */
> +
> +#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 */

-- 
Jerome

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 7/7] clk: meson: a1: add Amlogic A1 CPU clock controller driver

[Jerome Brunet]

On Wed 15 May 2024 at 21:47, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:

> The CPU clock controller plays a general role in the Amlogic A1 SoC
> family by generating CPU clocks. As an APB slave module, it offers the
> capability to inherit the CPU clock from two sources: the internal fixed
> clock known as 'cpu fixed clock' and the external input provided by the
> A1 PLL clock controller, referred to as 'syspll'.
>
> It is important for the driver to handle cpu_clk rate switching
> effectively by transitioning to the CPU fixed clock to avoid any
> potential execution freezes.
>
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> ---
>  drivers/clk/meson/Kconfig  |  10 ++
>  drivers/clk/meson/Makefile |   1 +
>  drivers/clk/meson/a1-cpu.c | 331 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 342 insertions(+)
>  create mode 100644 drivers/clk/meson/a1-cpu.c
>
> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index 80c4a18c83d2..148d4495eee3 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -111,6 +111,16 @@ config COMMON_CLK_AXG_AUDIO
>  	  Support for the audio clock controller on AmLogic A113D devices,
>  	  aka axg, Say Y if you want audio subsystem to work.
>  
> +config COMMON_CLK_A1_CPU
> +	tristate "Amlogic A1 SoC CPU controller support"
> +	depends on ARM64
> +	select COMMON_CLK_MESON_REGMAP
> +	select COMMON_CLK_MESON_CLKC_UTILS
> +	help
> +	  Support for the CPU clock controller on Amlogic A113L based
> +	  device, A1 SoC Family. Say Y if you want A1 CPU clock controller
> +	  to work.
> +
>  config COMMON_CLK_A1_PLL
>  	tristate "Amlogic A1 SoC PLL controller support"
>  	depends on ARM64
> diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> index 4968fc7ad555..2a06eb0303d6 100644
> --- a/drivers/clk/meson/Makefile
> +++ b/drivers/clk/meson/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_COMMON_CLK_MESON_AUDIO_RSTC) += meson-audio-rstc.o
>  
>  obj-$(CONFIG_COMMON_CLK_AXG) += axg.o axg-aoclk.o
>  obj-$(CONFIG_COMMON_CLK_AXG_AUDIO) += axg-audio.o
> +obj-$(CONFIG_COMMON_CLK_A1_CPU) += a1-cpu.o
>  obj-$(CONFIG_COMMON_CLK_A1_PLL) += a1-pll.o
>  obj-$(CONFIG_COMMON_CLK_A1_PERIPHERALS) += a1-peripherals.o
>  obj-$(CONFIG_COMMON_CLK_A1_AUDIO) += a1-audio.o
> diff --git a/drivers/clk/meson/a1-cpu.c b/drivers/clk/meson/a1-cpu.c
> new file mode 100644
> index 000000000000..a9edabeafea9
> --- /dev/null
> +++ b/drivers/clk/meson/a1-cpu.c
> @@ -0,0 +1,331 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Amlogic A1 SoC family CPU Clock Controller driver.
> + *
> + * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
> + * Author: Dmitry Rokosov <ddrokosov@salutedevices.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include "clk-regmap.h"
> +#include "meson-clkc-utils.h"
> +
> +#include <dt-bindings/clock/amlogic,a1-cpu-clkc.h>
> +
> +/* CPU Clock Controller register offset */
> +#define CPUCTRL_CLK_CTRL0	0x0
> +#define CPUCTRL_CLK_CTRL1	0x4
> +
> +static u32 cpu_fsource_sel_table[] = { 0, 1, 2 };
> +static const struct clk_parent_data cpu_fsource_sel_parents[] = {
> +	{ .fw_name = "xtal" },
> +	{ .fw_name = "fclk_div2" },
> +	{ .fw_name = "fclk_div3" },
> +};
> +
> +static struct clk_regmap cpu_fsource_sel0 = {
> +	.data = &(struct clk_regmap_mux_data) {
> +		.offset = CPUCTRL_CLK_CTRL0,
> +		.mask = 0x3,
> +		.shift = 0,
> +		.table = cpu_fsource_sel_table,
> +	},
> +	.hw.init = &(struct clk_init_data) {
> +		.name = "cpu_fsource_sel0",
> +		.ops = &clk_regmap_mux_ops,
> +		.parent_data = cpu_fsource_sel_parents,
> +		.num_parents = ARRAY_SIZE(cpu_fsource_sel_parents),
> +		.flags = CLK_SET_RATE_PARENT,

I don't think setting the rates of controller parents is appropriate

> +	},
> +};
> +
> +static struct clk_regmap cpu_fsource_div0 = {
> +	.data = &(struct clk_regmap_div_data) {
> +		.offset = CPUCTRL_CLK_CTRL0,
> +		.shift = 4,
> +		.width = 6,
> +	},
> +	.hw.init = &(struct clk_init_data) {
> +		.name = "cpu_fsource_div0",
> +		.ops = &clk_regmap_divider_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&cpu_fsource_sel0.hw
> +		},
> +		.num_parents = 1,
> +		.flags = CLK_SET_RATE_PARENT,
> +	},
> +};
> +
> +static struct clk_regmap cpu_fsel0 = {
> +	.data = &(struct clk_regmap_mux_data) {
> +		.offset = CPUCTRL_CLK_CTRL0,
> +		.mask = 0x1,
> +		.shift = 2,
> +	},
> +	.hw.init = &(struct clk_init_data) {
> +		.name = "cpu_fsel0",
> +		.ops = &clk_regmap_mux_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&cpu_fsource_sel0.hw,
> +			&cpu_fsource_div0.hw,
> +		},
> +		.num_parents = 2,
> +		.flags = CLK_SET_RATE_GATE | CLK_SET_RATE_PARENT,
> +	},
> +};
> +
> +static struct clk_regmap cpu_fsource_sel1 = {
> +	.data = &(struct clk_regmap_mux_data) {
> +		.offset = CPUCTRL_CLK_CTRL0,
> +		.mask = 0x3,
> +		.shift = 16,
> +		.table = cpu_fsource_sel_table,
> +	},
> +	.hw.init = &(struct clk_init_data) {
> +		.name = "cpu_fsource_sel1",
> +		.ops = &clk_regmap_mux_ops,
> +		.parent_data = cpu_fsource_sel_parents,
> +		.num_parents = ARRAY_SIZE(cpu_fsource_sel_parents),
> +		.flags = CLK_SET_RATE_PARENT,
> +	},
> +};
> +
> +static struct clk_regmap cpu_fsource_div1 = {
> +	.data = &(struct clk_regmap_div_data) {
> +		.offset = CPUCTRL_CLK_CTRL0,
> +		.shift = 20,
> +		.width = 6,
> +	},
> +	.hw.init = &(struct clk_init_data) {
> +		.name = "cpu_fsource_div1",
> +		.ops = &clk_regmap_divider_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&cpu_fsource_sel1.hw
> +		},
> +		.num_parents = 1,
> +		.flags = CLK_SET_RATE_PARENT,
> +	},
> +};
> +
> +static struct clk_regmap cpu_fsel1 = {
> +	.data = &(struct clk_regmap_mux_data) {
> +		.offset = CPUCTRL_CLK_CTRL0,
> +		.mask = 0x1,
> +		.shift = 18,
> +	},
> +	.hw.init = &(struct clk_init_data) {
> +		.name = "cpu_fsel1",
> +		.ops = &clk_regmap_mux_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&cpu_fsource_sel1.hw,
> +			&cpu_fsource_div1.hw,
> +		},
> +		.num_parents = 2,
> +		.flags = CLK_SET_RATE_GATE | CLK_SET_RATE_PARENT,
> +	},
> +};
> +
> +static struct clk_regmap cpu_fclk = {
> +	.data = &(struct clk_regmap_mux_data) {
> +		.offset = CPUCTRL_CLK_CTRL0,
> +		.mask = 0x1,
> +		.shift = 10,
> +	},
> +	.hw.init = &(struct clk_init_data) {
> +		.name = "cpu_fclk",
> +		.ops = &clk_regmap_mux_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&cpu_fsel0.hw,
> +			&cpu_fsel1.hw,
> +		},
> +		.num_parents = 2,
> +		.flags = CLK_SET_RATE_PARENT,
> +	},
> +};
> +
> +static struct clk_regmap cpu_clk = {
> +	.data = &(struct clk_regmap_mux_data) {
> +		.offset = CPUCTRL_CLK_CTRL0,
> +		.mask = 0x1,
> +		.shift = 11,
> +	},
> +	.hw.init = &(struct clk_init_data) {
> +		.name = "cpu_clk",
> +		.ops = &clk_regmap_mux_ops,
> +		.parent_data = (const struct clk_parent_data []) {
> +			{ .hw = &cpu_fclk.hw },
> +			{ .fw_name = "sys_pll", },
> +		},

You've put CLK_SET_RATE_GATE on fixed clock path but not the SYS_PLL
... that is odd. IMO there should be a bypass input clock to the sys_pll
with that flag.

> +		.num_parents = 2,
> +		.flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
> +	},
> +};
> +
> +/* Array of all clocks registered by this provider */
> +static struct clk_hw *a1_cpu_hw_clks[] = {
> +	[CLKID_CPU_FSOURCE_SEL0]	= &cpu_fsource_sel0.hw,
> +	[CLKID_CPU_FSOURCE_DIV0]	= &cpu_fsource_div0.hw,
> +	[CLKID_CPU_FSEL0]		= &cpu_fsel0.hw,
> +	[CLKID_CPU_FSOURCE_SEL1]	= &cpu_fsource_sel1.hw,
> +	[CLKID_CPU_FSOURCE_DIV1]	= &cpu_fsource_div1.hw,
> +	[CLKID_CPU_FSEL1]		= &cpu_fsel1.hw,
> +	[CLKID_CPU_FCLK]		= &cpu_fclk.hw,
> +	[CLKID_CPU_CLK]			= &cpu_clk.hw,
> +};
> +
> +static struct clk_regmap *const a1_cpu_regmaps[] = {
> +	&cpu_fsource_sel0,
> +	&cpu_fsource_div0,
> +	&cpu_fsel0,
> +	&cpu_fsource_sel1,
> +	&cpu_fsource_div1,
> +	&cpu_fsel1,
> +	&cpu_fclk,
> +	&cpu_clk,
> +};
> +
> +static struct regmap_config a1_cpu_regmap_cfg = {
> +	.reg_bits   = 32,
> +	.val_bits   = 32,
> +	.reg_stride = 4,
> +	.max_register = CPUCTRL_CLK_CTRL1,
> +};
> +
> +static struct meson_clk_hw_data a1_cpu_clks = {
> +	.hws = a1_cpu_hw_clks,
> +	.num = ARRAY_SIZE(a1_cpu_hw_clks),
> +};
> +
> +struct a1_sys_pll_nb_data {
> +	struct notifier_block nb;
> +	struct clk_hw *cpu_clk;
> +	struct clk_hw *cpu_fclk;
> +	struct clk *sys_pll;
> +};

There are number of things which are wrong with this notifier.

First, and foremost, this is a clock controller driver ... it should not
handle cpufreq policy. There is subsystem for that

> +
> +static int meson_a1_sys_pll_notifier_cb(struct notifier_block *nb,
> +					unsigned long event, void *data)
> +{
> +	struct a1_sys_pll_nb_data *nbd;
> +	int ret = 0;
> +
> +	nbd = container_of(nb, struct a1_sys_pll_nb_data, nb);
> +
> +	switch (event) {
> +	case PRE_RATE_CHANGE:
> +		/*
> +		 * Clock sys_pll will be changed to feed cpu_clk,
> +		 * configure cpu_clk to use cpu_fclk fixed clock.
> +		 */
> +		ret = clk_hw_set_parent(nbd->cpu_clk, nbd->cpu_fclk);


This jumps to whatever was the last frequency below 768MHz ... that does
not seems deterministic or safe.
> +
> +		/* Wait for clock propagation */
> +		if (!ret)
> +			udelay(100);
> +
> +		break;
> +
> +	case POST_RATE_CHANGE:
> +		 /*
> +		  * Clock sys_pll rate has ben calculated,
> +		  * switch back cpu_clk to sys_pll
> +		  */
> +		ret = clk_set_parent(nbd->cpu_clk->clk, nbd->sys_pll);

So whenever sys_pll changes, even if was not used by the CPU at that
time, this will change back to the sys_pll. Again, that seems fragile

> +
> +		/* Wait for clock propagation */
> +		if (!ret)
> +			udelay(100);
> +		break;
> +
> +	default:
> +		pr_warn("Unknown event %lu for sys_pll notifier\n", event);
> +		break;
> +	}
> +
> +	return notifier_from_errno(ret);
> +}
> +
> +static struct a1_sys_pll_nb_data a1_sys_pll_nb_data = {
> +	.nb.notifier_call = meson_a1_sys_pll_notifier_cb,
> +	.cpu_clk = &cpu_clk.hw,
> +	.cpu_fclk = &cpu_fclk.hw,
> +};
> +
> +static int meson_a1_dvfs_setup(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct clk *sys_pll;
> +	int ret;
> +
> +	/* Setup clock notifier for sys_pll clk */
> +	sys_pll = devm_clk_get(dev, "sys_pll");
> +	if (IS_ERR(sys_pll))
> +		return dev_err_probe(dev, PTR_ERR(sys_pll),
> +				     "can't get sys_pll as notifier clock\n");
> +
> +	a1_sys_pll_nb_data.sys_pll = sys_pll;
> +	ret = devm_clk_notifier_register(dev, sys_pll,
> +					 &a1_sys_pll_nb_data.nb);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "can't register sys_pll notifier\n");
> +
> +	return ret;
> +}

I don't think these notifiers are appropriate to handle CPU frequency
change. Cpufreq has a .target_intermediate() callback that seems more
appropriate to switch the CPU to a safe clock while relocking a PLL.

You should have a look at it and probably at the imx-cpufreq-dt.c which
improves on cpufreq-dt.c to handle platform quirks

> +
> +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 <ddrokosov@salutedevices.com>");
> +MODULE_LICENSE("GPL");

-- 
Jerome

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 0/7] clk: meson: introduce Amlogic A1 SoC Family CPU clock controller driver

[Jerome Brunet]

On Wed 15 May 2024 at 21:47, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:

> The CPU clock controller plays a general role in the Amlogic A1 SoC
> family by generating CPU clocks. As an APB slave module, it offers the
> capability to inherit the CPU clock from two sources: the internal fixed
> clock known as 'cpu fixed clock' and the external input provided by the
> A1 PLL clock controller, referred to as 'syspll'.
>
> It is important for the driver to handle the cpu_clk rate switching
> effectively by transitioning to the CPU fixed clock to avoid any
> potential execution freezes.
>

Please group your changes, fixes then bindings then driver.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: (subset) [PATCH v3 0/7] clk: meson: introduce Amlogic A1 SoC Family CPU clock controller driver

[Jerome Brunet]

Applied to clk-meson (v6.11/drivers), thanks!

[1/7] clk: meson: add 'NOINIT_ENABLED' flag to eliminate init for enabled PLL
      https://github.com/BayLibre/clk-meson/commit/d4c83ac16c65
[2/7] dt-bindings: clock: meson: a1: pll: introduce new syspll bindings
      https://github.com/BayLibre/clk-meson/commit/96f3b9787363
[4/7] dt-bindings: clock: meson: a1: peripherals: support sys_pll input
      https://github.com/BayLibre/clk-meson/commit/41056416ed53

Best regards,
--
Jerome


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 6/7] dt-bindings: clock: meson: add A1 CPU clock controller bindings

[Dmitry Rokosov]

Hello Jerome,

Thank you for the review!

On Mon, Jun 10, 2024 at 12:04:09PM +0200, Jerome Brunet wrote:
> On Wed 15 May 2024 at 21:47, Dmitry Rokosov <ddrokosov@salutedevices.com> 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 <ddrokosov@salutedevices.com>
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > ---
> >  .../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..f4958b315ed4
> > --- /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 <neil.armstrong@linaro.org>
> > +  - Jerome Brunet <jbrunet@baylibre.com>
> > +  - Dmitry Rokosov <ddrokosov@salutedevices.com>
> > +
> > +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)
> 
> According to the documentation, fdiv5 is also an input of the CPU clock
> tree.
> 
> That is typically the kind of things we'd prefer to get right from the
> beginning to avoid modifying the bindings later.
> 

Could you please share which documentation you are referencing? I have
the A113L documentation, and there is no mention of the CPU clock IP.
I retrieved below register map from the vendor's custom driver:

===
CPUCTRL_CLK_CTRL0

bits 1:0 - cpu_fsource_sel0
    0 - xtal
    1 - fclk_div2
    2 - fclk_div3

bit 2 - cpu_fsel0
    0 - cpu_fsource_sel0
    1 - cpu_fsource_div0

bit 3 - UNKNONWN

bits 9:4 - cpu_fsource_div0
    Divider value

bit 10 - cpu_fclk
    0 - cpu_fsel0
    1 - cpu_fsel1

bit 11 - cpu_clk
    0 - cpu_fclk
    1 - sys_pll

bits 15:12 - UNKNONWN

bits 17:16 - cpu_fsource_sel1
    0 - xtal
    1 - fclk_div2
    2 - fclk_div3

bit 18 - cpu_fsel1
    0 - cpu_fsource_sel1
    1 - cpu_fsource_div1

bit 19 - UNKNONWN

bits 25:20 - cpu_fsource_div1
    Divider value

bits 31:26 - UNKNONWN
===

As you can see it doesn't have any other inputs except fclk_div2,
fclk_div3, sys_pll and xtal.

> > +
> > +  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 0xfd000080 0 0x8>;
> 
> If reg is <0 0xfd000080 0 0x8> then node name should be clock-controller@fd000080
> 

Okay, I will fix that example in the next version.

[...]

-- 
Thank you,
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 0/7] clk: meson: introduce Amlogic A1 SoC Family CPU clock controller driver

[Dmitry Rokosov]

On Mon, Jun 10, 2024 at 12:13:41PM +0200, Jerome Brunet wrote:
> On Wed 15 May 2024 at 21:47, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
> 
> > The CPU clock controller plays a general role in the Amlogic A1 SoC
> > family by generating CPU clocks. As an APB slave module, it offers the
> > capability to inherit the CPU clock from two sources: the internal fixed
> > clock known as 'cpu fixed clock' and the external input provided by the
> > A1 PLL clock controller, referred to as 'syspll'.
> >
> > It is important for the driver to handle the cpu_clk rate switching
> > effectively by transitioning to the CPU fixed clock to avoid any
> > potential execution freezes.
> >
> 
> Please group your changes, fixes then bindings then driver.

Sure, thank you for the suggestion! 

-- 
Thank you,
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 6/7] dt-bindings: clock: meson: add A1 CPU clock controller bindings

[Jerome Brunet]

On Mon 10 Jun 2024 at 14:18, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:

> Hello Jerome,
>
> Thank you for the review!
>
> On Mon, Jun 10, 2024 at 12:04:09PM +0200, Jerome Brunet wrote:
>> On Wed 15 May 2024 at 21:47, Dmitry Rokosov <ddrokosov@salutedevices.com> 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 <ddrokosov@salutedevices.com>
>> > Reviewed-by: Rob Herring <robh@kernel.org>
>> > ---
>> >  .../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..f4958b315ed4
>> > --- /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 <neil.armstrong@linaro.org>
>> > +  - Jerome Brunet <jbrunet@baylibre.com>
>> > +  - Dmitry Rokosov <ddrokosov@salutedevices.com>
>> > +
>> > +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)
>> 
>> According to the documentation, fdiv5 is also an input of the CPU clock
>> tree.
>> 
>> That is typically the kind of things we'd prefer to get right from the
>> beginning to avoid modifying the bindings later.
>> 
>
> Could you please share which documentation you are referencing? I have
> the A113L documentation, and there is no mention of the CPU clock IP.

You should get in touch with Amlogic.

> I retrieved below register map from the vendor's custom driver:
>
> ===
> CPUCTRL_CLK_CTRL0
>
> bits 1:0 - cpu_fsource_sel0
>     0 - xtal
>     1 - fclk_div2
>     2 - fclk_div3
>
> bit 2 - cpu_fsel0
>     0 - cpu_fsource_sel0
>     1 - cpu_fsource_div0
>
> bit 3 - UNKNONWN
>
> bits 9:4 - cpu_fsource_div0
>     Divider value
>
> bit 10 - cpu_fclk
>     0 - cpu_fsel0
>     1 - cpu_fsel1
>
> bit 11 - cpu_clk
>     0 - cpu_fclk
>     1 - sys_pll
>
> bits 15:12 - UNKNONWN
>
> bits 17:16 - cpu_fsource_sel1
>     0 - xtal
>     1 - fclk_div2
>     2 - fclk_div3
>
> bit 18 - cpu_fsel1
>     0 - cpu_fsource_sel1
>     1 - cpu_fsource_div1
>
> bit 19 - UNKNONWN
>
> bits 25:20 - cpu_fsource_div1
>     Divider value
>
> bits 31:26 - UNKNONWN
> ===
>
> As you can see it doesn't have any other inputs except fclk_div2,
> fclk_div3, sys_pll and xtal.

You might not know what to do with it yet, still it is part of the
documentation and should be part of the bindings too

>
>> > +
>> > +  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 0xfd000080 0 0x8>;
>> 
>> If reg is <0 0xfd000080 0 0x8> then node name should be clock-controller@fd000080
>> 
>
> Okay, I will fix that example in the next version.
>
> [...]

-- 
Jerome

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 6/7] dt-bindings: clock: meson: add A1 CPU clock controller bindings

[Dmitry Rokosov]

On Mon, Jun 10, 2024 at 01:47:06PM +0200, Jerome Brunet wrote:
> On Mon 10 Jun 2024 at 14:18, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
> 
> > Hello Jerome,
> >
> > Thank you for the review!
> >
> > On Mon, Jun 10, 2024 at 12:04:09PM +0200, Jerome Brunet wrote:
> >> On Wed 15 May 2024 at 21:47, Dmitry Rokosov <ddrokosov@salutedevices.com> 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 <ddrokosov@salutedevices.com>
> >> > Reviewed-by: Rob Herring <robh@kernel.org>
> >> > ---
> >> >  .../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..f4958b315ed4
> >> > --- /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 <neil.armstrong@linaro.org>
> >> > +  - Jerome Brunet <jbrunet@baylibre.com>
> >> > +  - Dmitry Rokosov <ddrokosov@salutedevices.com>
> >> > +
> >> > +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)
> >> 
> >> According to the documentation, fdiv5 is also an input of the CPU clock
> >> tree.
> >> 
> >> That is typically the kind of things we'd prefer to get right from the
> >> beginning to avoid modifying the bindings later.
> >> 
> >
> > Could you please share which documentation you are referencing? I have
> > the A113L documentation, and there is no mention of the CPU clock IP.
> 
> You should get in touch with Amlogic.
> 

Okay, I will double check with Amlogic and back with accurate
information.

> > I retrieved below register map from the vendor's custom driver:
> >
> > ===
> > CPUCTRL_CLK_CTRL0
> >
> > bits 1:0 - cpu_fsource_sel0
> >     0 - xtal
> >     1 - fclk_div2
> >     2 - fclk_div3
> >
> > bit 2 - cpu_fsel0
> >     0 - cpu_fsource_sel0
> >     1 - cpu_fsource_div0
> >
> > bit 3 - UNKNONWN
> >
> > bits 9:4 - cpu_fsource_div0
> >     Divider value
> >
> > bit 10 - cpu_fclk
> >     0 - cpu_fsel0
> >     1 - cpu_fsel1
> >
> > bit 11 - cpu_clk
> >     0 - cpu_fclk
> >     1 - sys_pll
> >
> > bits 15:12 - UNKNONWN
> >
> > bits 17:16 - cpu_fsource_sel1
> >     0 - xtal
> >     1 - fclk_div2
> >     2 - fclk_div3
> >
> > bit 18 - cpu_fsel1
> >     0 - cpu_fsource_sel1
> >     1 - cpu_fsource_div1
> >
> > bit 19 - UNKNONWN
> >
> > bits 25:20 - cpu_fsource_div1
> >     Divider value
> >
> > bits 31:26 - UNKNONWN
> > ===
> >
> > As you can see it doesn't have any other inputs except fclk_div2,
> > fclk_div3, sys_pll and xtal.
> 
> You might not know what to do with it yet, still it is part of the
> documentation and should be part of the bindings too
> 
> >
> >> > +
> >> > +  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 0xfd000080 0 0x8>;
> >> 
> >> If reg is <0 0xfd000080 0 0x8> then node name should be clock-controller@fd000080
> >> 
> >
> > Okay, I will fix that example in the next version.
> >
> > [...]
> 
> -- 
> Jerome

-- 
Thank you,
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 7/7] clk: meson: a1: add Amlogic A1 CPU clock controller driver

[Dmitry Rokosov]

On Mon, Jun 10, 2024 at 12:06:31PM +0200, Jerome Brunet wrote:
> On Wed 15 May 2024 at 21:47, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
> 
> > The CPU clock controller plays a general role in the Amlogic A1 SoC
> > family by generating CPU clocks. As an APB slave module, it offers the
> > capability to inherit the CPU clock from two sources: the internal fixed
> > clock known as 'cpu fixed clock' and the external input provided by the
> > A1 PLL clock controller, referred to as 'syspll'.
> >
> > It is important for the driver to handle cpu_clk rate switching
> > effectively by transitioning to the CPU fixed clock to avoid any
> > potential execution freezes.
> >
> > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> > ---
> >  drivers/clk/meson/Kconfig  |  10 ++
> >  drivers/clk/meson/Makefile |   1 +
> >  drivers/clk/meson/a1-cpu.c | 331 +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 342 insertions(+)
> >  create mode 100644 drivers/clk/meson/a1-cpu.c
> >
> > diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> > index 80c4a18c83d2..148d4495eee3 100644
> > --- a/drivers/clk/meson/Kconfig
> > +++ b/drivers/clk/meson/Kconfig
> > @@ -111,6 +111,16 @@ config COMMON_CLK_AXG_AUDIO
> >  	  Support for the audio clock controller on AmLogic A113D devices,
> >  	  aka axg, Say Y if you want audio subsystem to work.
> >  
> > +config COMMON_CLK_A1_CPU
> > +	tristate "Amlogic A1 SoC CPU controller support"
> > +	depends on ARM64
> > +	select COMMON_CLK_MESON_REGMAP
> > +	select COMMON_CLK_MESON_CLKC_UTILS
> > +	help
> > +	  Support for the CPU clock controller on Amlogic A113L based
> > +	  device, A1 SoC Family. Say Y if you want A1 CPU clock controller
> > +	  to work.
> > +
> >  config COMMON_CLK_A1_PLL
> >  	tristate "Amlogic A1 SoC PLL controller support"
> >  	depends on ARM64
> > diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> > index 4968fc7ad555..2a06eb0303d6 100644
> > --- a/drivers/clk/meson/Makefile
> > +++ b/drivers/clk/meson/Makefile
> > @@ -18,6 +18,7 @@ obj-$(CONFIG_COMMON_CLK_MESON_AUDIO_RSTC) += meson-audio-rstc.o
> >  
> >  obj-$(CONFIG_COMMON_CLK_AXG) += axg.o axg-aoclk.o
> >  obj-$(CONFIG_COMMON_CLK_AXG_AUDIO) += axg-audio.o
> > +obj-$(CONFIG_COMMON_CLK_A1_CPU) += a1-cpu.o
> >  obj-$(CONFIG_COMMON_CLK_A1_PLL) += a1-pll.o
> >  obj-$(CONFIG_COMMON_CLK_A1_PERIPHERALS) += a1-peripherals.o
> >  obj-$(CONFIG_COMMON_CLK_A1_AUDIO) += a1-audio.o
> > diff --git a/drivers/clk/meson/a1-cpu.c b/drivers/clk/meson/a1-cpu.c
> > new file mode 100644
> > index 000000000000..a9edabeafea9
> > --- /dev/null
> > +++ b/drivers/clk/meson/a1-cpu.c
> > @@ -0,0 +1,331 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Amlogic A1 SoC family CPU Clock Controller driver.
> > + *
> > + * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
> > + * Author: Dmitry Rokosov <ddrokosov@salutedevices.com>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/platform_device.h>
> > +#include "clk-regmap.h"
> > +#include "meson-clkc-utils.h"
> > +
> > +#include <dt-bindings/clock/amlogic,a1-cpu-clkc.h>
> > +
> > +/* CPU Clock Controller register offset */
> > +#define CPUCTRL_CLK_CTRL0	0x0
> > +#define CPUCTRL_CLK_CTRL1	0x4
> > +
> > +static u32 cpu_fsource_sel_table[] = { 0, 1, 2 };
> > +static const struct clk_parent_data cpu_fsource_sel_parents[] = {
> > +	{ .fw_name = "xtal" },
> > +	{ .fw_name = "fclk_div2" },
> > +	{ .fw_name = "fclk_div3" },
> > +};
> > +
> > +static struct clk_regmap cpu_fsource_sel0 = {
> > +	.data = &(struct clk_regmap_mux_data) {
> > +		.offset = CPUCTRL_CLK_CTRL0,
> > +		.mask = 0x3,
> > +		.shift = 0,
> > +		.table = cpu_fsource_sel_table,
> > +	},
> > +	.hw.init = &(struct clk_init_data) {
> > +		.name = "cpu_fsource_sel0",
> > +		.ops = &clk_regmap_mux_ops,
> > +		.parent_data = cpu_fsource_sel_parents,
> > +		.num_parents = ARRAY_SIZE(cpu_fsource_sel_parents),
> > +		.flags = CLK_SET_RATE_PARENT,
> 
> I don't think setting the rates of controller parents is appropriate
> 
> > +	},
> > +};
> > +
> > +static struct clk_regmap cpu_fsource_div0 = {
> > +	.data = &(struct clk_regmap_div_data) {
> > +		.offset = CPUCTRL_CLK_CTRL0,
> > +		.shift = 4,
> > +		.width = 6,
> > +	},
> > +	.hw.init = &(struct clk_init_data) {
> > +		.name = "cpu_fsource_div0",
> > +		.ops = &clk_regmap_divider_ops,
> > +		.parent_hws = (const struct clk_hw *[]) {
> > +			&cpu_fsource_sel0.hw
> > +		},
> > +		.num_parents = 1,
> > +		.flags = CLK_SET_RATE_PARENT,
> > +	},
> > +};
> > +
> > +static struct clk_regmap cpu_fsel0 = {
> > +	.data = &(struct clk_regmap_mux_data) {
> > +		.offset = CPUCTRL_CLK_CTRL0,
> > +		.mask = 0x1,
> > +		.shift = 2,
> > +	},
> > +	.hw.init = &(struct clk_init_data) {
> > +		.name = "cpu_fsel0",
> > +		.ops = &clk_regmap_mux_ops,
> > +		.parent_hws = (const struct clk_hw *[]) {
> > +			&cpu_fsource_sel0.hw,
> > +			&cpu_fsource_div0.hw,
> > +		},
> > +		.num_parents = 2,
> > +		.flags = CLK_SET_RATE_GATE | CLK_SET_RATE_PARENT,
> > +	},
> > +};
> > +
> > +static struct clk_regmap cpu_fsource_sel1 = {
> > +	.data = &(struct clk_regmap_mux_data) {
> > +		.offset = CPUCTRL_CLK_CTRL0,
> > +		.mask = 0x3,
> > +		.shift = 16,
> > +		.table = cpu_fsource_sel_table,
> > +	},
> > +	.hw.init = &(struct clk_init_data) {
> > +		.name = "cpu_fsource_sel1",
> > +		.ops = &clk_regmap_mux_ops,
> > +		.parent_data = cpu_fsource_sel_parents,
> > +		.num_parents = ARRAY_SIZE(cpu_fsource_sel_parents),
> > +		.flags = CLK_SET_RATE_PARENT,
> > +	},
> > +};
> > +
> > +static struct clk_regmap cpu_fsource_div1 = {
> > +	.data = &(struct clk_regmap_div_data) {
> > +		.offset = CPUCTRL_CLK_CTRL0,
> > +		.shift = 20,
> > +		.width = 6,
> > +	},
> > +	.hw.init = &(struct clk_init_data) {
> > +		.name = "cpu_fsource_div1",
> > +		.ops = &clk_regmap_divider_ops,
> > +		.parent_hws = (const struct clk_hw *[]) {
> > +			&cpu_fsource_sel1.hw
> > +		},
> > +		.num_parents = 1,
> > +		.flags = CLK_SET_RATE_PARENT,
> > +	},
> > +};
> > +
> > +static struct clk_regmap cpu_fsel1 = {
> > +	.data = &(struct clk_regmap_mux_data) {
> > +		.offset = CPUCTRL_CLK_CTRL0,
> > +		.mask = 0x1,
> > +		.shift = 18,
> > +	},
> > +	.hw.init = &(struct clk_init_data) {
> > +		.name = "cpu_fsel1",
> > +		.ops = &clk_regmap_mux_ops,
> > +		.parent_hws = (const struct clk_hw *[]) {
> > +			&cpu_fsource_sel1.hw,
> > +			&cpu_fsource_div1.hw,
> > +		},
> > +		.num_parents = 2,
> > +		.flags = CLK_SET_RATE_GATE | CLK_SET_RATE_PARENT,
> > +	},
> > +};
> > +
> > +static struct clk_regmap cpu_fclk = {
> > +	.data = &(struct clk_regmap_mux_data) {
> > +		.offset = CPUCTRL_CLK_CTRL0,
> > +		.mask = 0x1,
> > +		.shift = 10,
> > +	},
> > +	.hw.init = &(struct clk_init_data) {
> > +		.name = "cpu_fclk",
> > +		.ops = &clk_regmap_mux_ops,
> > +		.parent_hws = (const struct clk_hw *[]) {
> > +			&cpu_fsel0.hw,
> > +			&cpu_fsel1.hw,
> > +		},
> > +		.num_parents = 2,
> > +		.flags = CLK_SET_RATE_PARENT,
> > +	},
> > +};
> > +
> > +static struct clk_regmap cpu_clk = {
> > +	.data = &(struct clk_regmap_mux_data) {
> > +		.offset = CPUCTRL_CLK_CTRL0,
> > +		.mask = 0x1,
> > +		.shift = 11,
> > +	},
> > +	.hw.init = &(struct clk_init_data) {
> > +		.name = "cpu_clk",
> > +		.ops = &clk_regmap_mux_ops,
> > +		.parent_data = (const struct clk_parent_data []) {
> > +			{ .hw = &cpu_fclk.hw },
> > +			{ .fw_name = "sys_pll", },
> > +		},
> 
> You've put CLK_SET_RATE_GATE on fixed clock path but not the SYS_PLL
> ... that is odd. IMO there should be a bypass input clock to the sys_pll
> with that flag.
> 

Apologies for any confusion caused. To clarify, are you proposing the
idea of creating an additional sys_pll_input clock object with the
CLK_SET_RATE_PARENT property, and then using it as the parent clock for
cpu_clk?

> > +		.num_parents = 2,
> > +		.flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
> > +	},
> > +};
> > +
> > +/* Array of all clocks registered by this provider */
> > +static struct clk_hw *a1_cpu_hw_clks[] = {
> > +	[CLKID_CPU_FSOURCE_SEL0]	= &cpu_fsource_sel0.hw,
> > +	[CLKID_CPU_FSOURCE_DIV0]	= &cpu_fsource_div0.hw,
> > +	[CLKID_CPU_FSEL0]		= &cpu_fsel0.hw,
> > +	[CLKID_CPU_FSOURCE_SEL1]	= &cpu_fsource_sel1.hw,
> > +	[CLKID_CPU_FSOURCE_DIV1]	= &cpu_fsource_div1.hw,
> > +	[CLKID_CPU_FSEL1]		= &cpu_fsel1.hw,
> > +	[CLKID_CPU_FCLK]		= &cpu_fclk.hw,
> > +	[CLKID_CPU_CLK]			= &cpu_clk.hw,
> > +};
> > +
> > +static struct clk_regmap *const a1_cpu_regmaps[] = {
> > +	&cpu_fsource_sel0,
> > +	&cpu_fsource_div0,
> > +	&cpu_fsel0,
> > +	&cpu_fsource_sel1,
> > +	&cpu_fsource_div1,
> > +	&cpu_fsel1,
> > +	&cpu_fclk,
> > +	&cpu_clk,
> > +};
> > +
> > +static struct regmap_config a1_cpu_regmap_cfg = {
> > +	.reg_bits   = 32,
> > +	.val_bits   = 32,
> > +	.reg_stride = 4,
> > +	.max_register = CPUCTRL_CLK_CTRL1,
> > +};
> > +
> > +static struct meson_clk_hw_data a1_cpu_clks = {
> > +	.hws = a1_cpu_hw_clks,
> > +	.num = ARRAY_SIZE(a1_cpu_hw_clks),
> > +};
> > +
> > +struct a1_sys_pll_nb_data {
> > +	struct notifier_block nb;
> > +	struct clk_hw *cpu_clk;
> > +	struct clk_hw *cpu_fclk;
> > +	struct clk *sys_pll;
> > +};
> 
> There are number of things which are wrong with this notifier.
> 
> First, and foremost, this is a clock controller driver ... it should not
> handle cpufreq policy. There is subsystem for that
> 
> > +
> > +static int meson_a1_sys_pll_notifier_cb(struct notifier_block *nb,
> > +					unsigned long event, void *data)
> > +{
> > +	struct a1_sys_pll_nb_data *nbd;
> > +	int ret = 0;
> > +
> > +	nbd = container_of(nb, struct a1_sys_pll_nb_data, nb);
> > +
> > +	switch (event) {
> > +	case PRE_RATE_CHANGE:
> > +		/*
> > +		 * Clock sys_pll will be changed to feed cpu_clk,
> > +		 * configure cpu_clk to use cpu_fclk fixed clock.
> > +		 */
> > +		ret = clk_hw_set_parent(nbd->cpu_clk, nbd->cpu_fclk);
> 
> 
> This jumps to whatever was the last frequency below 768MHz ... that does
> not seems deterministic or safe.

Ah, that's an aspect I hadn't considered. You make a valid point. So,
this implies that the g12a clock driver could potentially encounter the
same issue, correct?

> > +
> > +		/* Wait for clock propagation */
> > +		if (!ret)
> > +			udelay(100);
> > +
> > +		break;
> > +
> > +	case POST_RATE_CHANGE:
> > +		 /*
> > +		  * Clock sys_pll rate has ben calculated,
> > +		  * switch back cpu_clk to sys_pll
> > +		  */
> > +		ret = clk_set_parent(nbd->cpu_clk->clk, nbd->sys_pll);
> 
> So whenever sys_pll changes, even if was not used by the CPU at that
> time, this will change back to the sys_pll. Again, that seems fragile
> 

From what I comprehend, only the GEN clock is capable of using sys_pll
as its parent clock. The GEN clock seems more comparable to a diagnostic
clock, implying that when utilized, it should be done with full
awareness and control over its operations.

> > +
> > +		/* Wait for clock propagation */
> > +		if (!ret)
> > +			udelay(100);
> > +		break;
> > +
> > +	default:
> > +		pr_warn("Unknown event %lu for sys_pll notifier\n", event);
> > +		break;
> > +	}
> > +
> > +	return notifier_from_errno(ret);
> > +}
> > +
> > +static struct a1_sys_pll_nb_data a1_sys_pll_nb_data = {
> > +	.nb.notifier_call = meson_a1_sys_pll_notifier_cb,
> > +	.cpu_clk = &cpu_clk.hw,
> > +	.cpu_fclk = &cpu_fclk.hw,
> > +};
> > +
> > +static int meson_a1_dvfs_setup(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct clk *sys_pll;
> > +	int ret;
> > +
> > +	/* Setup clock notifier for sys_pll clk */
> > +	sys_pll = devm_clk_get(dev, "sys_pll");
> > +	if (IS_ERR(sys_pll))
> > +		return dev_err_probe(dev, PTR_ERR(sys_pll),
> > +				     "can't get sys_pll as notifier clock\n");
> > +
> > +	a1_sys_pll_nb_data.sys_pll = sys_pll;
> > +	ret = devm_clk_notifier_register(dev, sys_pll,
> > +					 &a1_sys_pll_nb_data.nb);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "can't register sys_pll notifier\n");
> > +
> > +	return ret;
> > +}
> 
> I don't think these notifiers are appropriate to handle CPU frequency
> change. Cpufreq has a .target_intermediate() callback that seems more
> appropriate to switch the CPU to a safe clock while relocking a PLL.
> 
> You should have a look at it and probably at the imx-cpufreq-dt.c which
> improves on cpufreq-dt.c to handle platform quirks
> 

I believed that the same approach was employed with the g12a clock,
which uses a sys_pll <-> cpu fixed clock transition to ensure stable CPU
clocking. Am I overlooking something? Or does the g12a cpu clock
maintain a fixed frequency, thus indicating it is not fragile?

[...]

-- 
Thank you,
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/7] clk: meson: a1: pll: support 'syspll' general-purpose PLL for CPU clock

[Dmitry Rokosov]

On Mon, Jun 10, 2024 at 12:03:02PM +0200, Jerome Brunet wrote:
> On Wed 15 May 2024 at 21:47, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
> 
> > The 'syspll' PLL, also known as the system PLL, is a general and
> > essential PLL responsible for generating the CPU clock frequency.
> > With its wide-ranging capabilities, it is designed to accommodate
> > frequencies within the range of 768MHz to 1536MHz.
> >
> > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> > ---
> >  drivers/clk/meson/a1-pll.c | 72 ++++++++++++++++++++++++++++++++++++++
> >  drivers/clk/meson/a1-pll.h |  6 ++++
> >  2 files changed, 78 insertions(+)
> >
> > diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c
> > index 60b2e53e7e51..286e83199d17 100644
> > --- a/drivers/clk/meson/a1-pll.c
> > +++ b/drivers/clk/meson/a1-pll.c
> > @@ -138,6 +138,76 @@ static struct clk_regmap hifi_pll = {
> >  	},
> >  };
> >  
> > +static const struct pll_mult_range sys_pll_mult_range = {
> > +	.min = 32,
> > +	.max = 64,
> > +};
> > +
> > +static const struct reg_sequence sys_pll_init_regs[] = {
> > +	{ .reg = ANACTRL_SYSPLL_CTRL1, .def = 0x01800000 },
> > +	{ .reg = ANACTRL_SYSPLL_CTRL2, .def = 0x00001100 },
> > +	{ .reg = ANACTRL_SYSPLL_CTRL3, .def = 0x10022300 },
> > +	{ .reg = ANACTRL_SYSPLL_CTRL4, .def = 0x00300000 },
> > +	{ .reg = ANACTRL_SYSPLL_CTRL0, .def = 0x01f18432 },
> 
> That last entry is clearly an hard coded rate being poked.
> Drop it please
> 

Ah, of course, you are totally right. I will remove hard coded rate in
the next version.

> > +};
> > +
> > +static struct clk_regmap sys_pll = {
> > +	.data = &(struct meson_clk_pll_data){
> > +		.en = {
> > +			.reg_off = ANACTRL_SYSPLL_CTRL0,
> > +			.shift   = 28,
> > +			.width   = 1,
> > +		},
> > +		.m = {
> > +			.reg_off = ANACTRL_SYSPLL_CTRL0,
> > +			.shift   = 0,
> > +			.width   = 8,
> > +		},
> > +		.n = {
> > +			.reg_off = ANACTRL_SYSPLL_CTRL0,
> > +			.shift   = 10,
> > +			.width   = 5,
> > +		},
> > +		.frac = {
> > +			.reg_off = ANACTRL_SYSPLL_CTRL1,
> > +			.shift   = 0,
> > +			.width   = 19,
> > +		},
> > +		.l = {
> > +			.reg_off = ANACTRL_SYSPLL_STS,
> > +			.shift   = 31,
> > +			.width   = 1,
> > +		},
> > +		.current_en = {
> > +			.reg_off = ANACTRL_SYSPLL_CTRL0,
> > +			.shift   = 26,
> > +			.width   = 1,
> > +		},
> > +		.l_detect = {
> > +			.reg_off = ANACTRL_SYSPLL_CTRL2,
> > +			.shift   = 6,
> > +			.width   = 1,
> > +		},
> > +		.range = &sys_pll_mult_range,
> > +		.init_regs = sys_pll_init_regs,
> > +		.init_count = ARRAY_SIZE(sys_pll_init_regs),
> > +		/*
> > +		 * The sys_pll clock is usually enabled and initialized in the
> > +		 * bootloader stage. Additionally, the cpu_clk is connected to
> > +		 * sys_pll. As a result, it is not allowed to initialize the
> > +		 * cpu_clk again, as doing so would prevent the CPU from
> > +		 * executing any instructions.
> > +		 */
> > +		.flags = CLK_MESON_PLL_NOINIT_ENABLED,
> > +	},
> > +	.hw.init = &(struct clk_init_data){
> > +		.name = "sys_pll",
> > +		.ops = &meson_clk_pll_ops,
> > +		.parent_names = (const char *[]){ "syspll_in" },
> > +		.num_parents = 1,
> > +	},
> > +};
> > +
> >  static struct clk_fixed_factor fclk_div2_div = {
> >  	.mult = 1,
> >  	.div = 2,
> > @@ -283,6 +353,7 @@ static struct clk_hw *a1_pll_hw_clks[] = {
> >  	[CLKID_FCLK_DIV5]	= &fclk_div5.hw,
> >  	[CLKID_FCLK_DIV7]	= &fclk_div7.hw,
> >  	[CLKID_HIFI_PLL]	= &hifi_pll.hw,
> > +	[CLKID_SYS_PLL]		= &sys_pll.hw,
> >  };
> >  
> >  static struct clk_regmap *const a1_pll_regmaps[] = {
> > @@ -293,6 +364,7 @@ static struct clk_regmap *const a1_pll_regmaps[] = {
> >  	&fclk_div5,
> >  	&fclk_div7,
> >  	&hifi_pll,
> > +	&sys_pll,
> >  };
> >  
> >  static struct regmap_config a1_pll_regmap_cfg = {
> > diff --git a/drivers/clk/meson/a1-pll.h b/drivers/clk/meson/a1-pll.h
> > index 4be17b2bf383..666d9b2137e9 100644
> > --- a/drivers/clk/meson/a1-pll.h
> > +++ b/drivers/clk/meson/a1-pll.h
> > @@ -18,6 +18,12 @@
> >  #define ANACTRL_FIXPLL_CTRL0	0x0
> >  #define ANACTRL_FIXPLL_CTRL1	0x4
> >  #define ANACTRL_FIXPLL_STS	0x14
> > +#define ANACTRL_SYSPLL_CTRL0	0x80
> > +#define ANACTRL_SYSPLL_CTRL1	0x84
> > +#define ANACTRL_SYSPLL_CTRL2	0x88
> > +#define ANACTRL_SYSPLL_CTRL3	0x8c
> > +#define ANACTRL_SYSPLL_CTRL4	0x90
> > +#define ANACTRL_SYSPLL_STS	0x94
> >  #define ANACTRL_HIFIPLL_CTRL0	0xc0
> >  #define ANACTRL_HIFIPLL_CTRL1	0xc4
> >  #define ANACTRL_HIFIPLL_CTRL2	0xc8
> 
> -- 
> Jerome

-- 
Thank you,
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 7/7] clk: meson: a1: add Amlogic A1 CPU clock controller driver

[Dmitry Rokosov]

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

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

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

-- 
Thank you,
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 6/7] dt-bindings: clock: meson: add A1 CPU clock controller bindings

[Dmitry Rokosov]

On Mon, Jun 10, 2024 at 03:48:42PM +0300, Dmitry Rokosov wrote:
> On Mon, Jun 10, 2024 at 01:47:06PM +0200, Jerome Brunet wrote:
> > On Mon 10 Jun 2024 at 14:18, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
> > 
> > > Hello Jerome,
> > >
> > > Thank you for the review!
> > >
> > > On Mon, Jun 10, 2024 at 12:04:09PM +0200, Jerome Brunet wrote:
> > >> On Wed 15 May 2024 at 21:47, Dmitry Rokosov <ddrokosov@salutedevices.com> 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 <ddrokosov@salutedevices.com>
> > >> > Reviewed-by: Rob Herring <robh@kernel.org>
> > >> > ---
> > >> >  .../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..f4958b315ed4
> > >> > --- /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 <neil.armstrong@linaro.org>
> > >> > +  - Jerome Brunet <jbrunet@baylibre.com>
> > >> > +  - Dmitry Rokosov <ddrokosov@salutedevices.com>
> > >> > +
> > >> > +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)
> > >> 
> > >> According to the documentation, fdiv5 is also an input of the CPU clock
> > >> tree.
> > >> 
> > >> That is typically the kind of things we'd prefer to get right from the
> > >> beginning to avoid modifying the bindings later.
> > >> 
> > >
> > > Could you please share which documentation you are referencing? I have
> > > the A113L documentation, and there is no mention of the CPU clock IP.
> > 
> > You should get in touch with Amlogic.
> > 
> 
> Okay, I will double check with Amlogic and back with accurate
> information.
> 

According to a statement from an Amlogic FAE, there is an error in the
datasheet's CPU clock controller figure. The FAE clarified the
following:

"""
The d5/d7 clock source inside the A1 chip is not supplied to CPU_CLK and
is instead used by other peripherals. Therefore, you can connect a fixed
frequency divider (div5/div7) to peripherals in A1. The CPU control only
supports fclk_div2/div3.
"""

[...]

-- 
Thank you,
Dmitry