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

[PATCH v2 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 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/

Dmitry Rokosov (7):
  clk: meson: introduce 'INIT_ONCE' 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_div16
    input
  clk: meson: a1: peripherals: support 'sys_pll_div16' clock as GEN
    input
  dt-bindings: clock: meson: add A1 CPU clock controller bindings
  clk: meson: a1: add Amlogic A1 CPU clock controller driver

 .../bindings/clock/amlogic,a1-cpu-clkc.yaml   |  64 ++++
 .../clock/amlogic,a1-peripherals-clkc.yaml    |   7 +-
 .../bindings/clock/amlogic,a1-pll-clkc.yaml   |   7 +-
 drivers/clk/meson/Kconfig                     |  10 +
 drivers/clk/meson/Makefile                    |   1 +
 drivers/clk/meson/a1-cpu.c                    | 331 ++++++++++++++++++
 drivers/clk/meson/a1-peripherals.c            |   4 +-
 drivers/clk/meson/a1-pll.c                    |  79 +++++
 drivers/clk/meson/a1-pll.h                    |   6 +
 drivers/clk/meson/clk-pll.c                   |  37 +-
 drivers/clk/meson/clk-pll.h                   |   1 +
 .../dt-bindings/clock/amlogic,a1-cpu-clkc.h   |  19 +
 .../dt-bindings/clock/amlogic,a1-pll-clkc.h   |   2 +
 13 files changed, 546 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-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v2 1/7] clk: meson: introduce 'INIT_ONCE' 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 | 37 +++++++++++++++++++++----------------
 drivers/clk/meson/clk-pll.h |  1 +
 2 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
index 78d17b2415af..47b22a6be2e4 100644
--- a/drivers/clk/meson/clk-pll.c
+++ b/drivers/clk/meson/clk-pll.c
@@ -289,11 +289,32 @@ 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);
 
+	/* Do not init already enabled PLL which marked with 'init_once' */
+	if ((pll->flags & CLK_MESON_PLL_INIT_ONCE) &&
+	    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 +329,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..23195ea4eae1 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_INIT_ONCE		BIT(1)
 
 struct meson_clk_pll_data {
 	struct parm en;
-- 
2.43.0


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

[PATCH v2 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 clock source sys_pll_div16, being one of the GEN clock parents,
plays a crucial role and cannot be tagged as "optional". Unfortunately,
it was not implemented earlier due to the cpu clock ctrl driver's
pending status on the TODO list.

Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 .../devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml     | 7 +++++--
 include/dt-bindings/clock/amlogic,a1-pll-clkc.h            | 2 ++
 2 files changed, 7 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..fbba57031278 100644
--- a/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
+++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
@@ -26,11 +26,13 @@ properties:
     items:
       - description: input fixpll_in
       - description: input hifipll_in
+      - description: input syspll_in
 
   clock-names:
     items:
       - const: fixpll_in
       - const: hifipll_in
+      - const: syspll_in
 
 required:
   - compatible
@@ -53,7 +55,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..a702d610589c 100644
--- a/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
+++ b/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
@@ -21,5 +21,7 @@
 #define CLKID_FCLK_DIV5		8
 #define CLKID_FCLK_DIV7		9
 #define CLKID_HIFI_PLL		10
+#define CLKID_SYS_PLL		11
+#define CLKID_SYS_PLL_DIV16	12
 
 #endif /* __A1_PLL_CLKC_H */
-- 
2.43.0


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

[PATCH v2 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 | 79 ++++++++++++++++++++++++++++++++++++++
 drivers/clk/meson/a1-pll.h |  6 +++
 2 files changed, 85 insertions(+)

diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c
index 60b2e53e7e51..af47ba308bbe 100644
--- a/drivers/clk/meson/a1-pll.c
+++ b/drivers/clk/meson/a1-pll.c
@@ -138,6 +138,82 @@ 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),
+		.flags = CLK_MESON_PLL_INIT_ONCE,
+	},
+	.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 sys_pll_div16 = {
+	.mult = 1,
+	.div = 16,
+	.hw.init = &(struct clk_init_data){
+		.name = "sys_pll_div16",
+		.ops = &clk_fixed_factor_ops,
+		.parent_hws = (const struct clk_hw *[]) {
+			&sys_pll.hw
+		},
+		.num_parents = 1,
+	},
+};
+
 static struct clk_fixed_factor fclk_div2_div = {
 	.mult = 1,
 	.div = 2,
@@ -283,6 +359,8 @@ 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,
+	[CLKID_SYS_PLL_DIV16]	= &sys_pll_div16.hw,
 };
 
 static struct clk_regmap *const a1_pll_regmaps[] = {
@@ -293,6 +371,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-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Dmitry Rokosov]

The 'sys_pll_div16' input clock is used as one of the sources for the
GEN clock.

Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 .../bindings/clock/amlogic,a1-peripherals-clkc.yaml        | 7 +++++--
 1 file changed, 5 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..11862746ba44 100644
--- a/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml
+++ b/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml
@@ -30,6 +30,7 @@ properties:
       - description: input fixed pll div7
       - description: input hifi pll
       - description: input oscillator (usually at 24MHz)
+      - description: input sys pll div16
 
   clock-names:
     items:
@@ -39,6 +40,7 @@ properties:
       - const: fclk_div7
       - const: hifi_pll
       - const: xtal
+      - const: sys_pll_div16
 
 required:
   - compatible
@@ -65,9 +67,10 @@ examples:
                      <&clkc_pll CLKID_FCLK_DIV5>,
                      <&clkc_pll CLKID_FCLK_DIV7>,
                      <&clkc_pll CLKID_HIFI_PLL>,
-                     <&xtal>;
+                     <&xtal>,
+                     <&clkc_pll CLKID_SYS_PLL_DIV16>;
             clock-names = "fclk_div2", "fclk_div3",
                           "fclk_div5", "fclk_div7",
-                          "hifi_pll", "xtal";
+                          "hifi_pll", "xtal", "sys_pll_div16";
         };
     };
-- 
2.43.0


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

[PATCH v2 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 in the A1 PLL clock controller with a fixed factor.

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

diff --git a/drivers/clk/meson/a1-peripherals.c b/drivers/clk/meson/a1-peripherals.c
index 621af1e6e4b2..3c4452f2b146 100644
--- a/drivers/clk/meson/a1-peripherals.c
+++ b/drivers/clk/meson/a1-peripherals.c
@@ -747,13 +747,13 @@ static struct clk_regmap fclk_div2_divn = {
 };
 
 /*
- * 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 },
+	{ .fw_name = "sys_pll_div16", },
 	{ .fw_name = "hifi_pll", },
 	{ .fw_name = "fclk_div2", },
 	{ .fw_name = "fclk_div3", },
-- 
2.43.0


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

[PATCH v2 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-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v2 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-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1909 bytes --]

On Fri, May 10, 2024 at 12:08:56PM +0300, Dmitry Rokosov wrote:
> The 'sys_pll_div16' input clock is used as one of the sources for the
> GEN clock.
> 
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>

Provided that this new clock is optional in the driver,
Acked-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

> ---
>  .../bindings/clock/amlogic,a1-peripherals-clkc.yaml        | 7 +++++--
>  1 file changed, 5 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..11862746ba44 100644
> --- a/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml
> +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml
> @@ -30,6 +30,7 @@ properties:
>        - description: input fixed pll div7
>        - description: input hifi pll
>        - description: input oscillator (usually at 24MHz)
> +      - description: input sys pll div16
>  
>    clock-names:
>      items:
> @@ -39,6 +40,7 @@ properties:
>        - const: fclk_div7
>        - const: hifi_pll
>        - const: xtal
> +      - const: sys_pll_div16
>  
>  required:
>    - compatible
> @@ -65,9 +67,10 @@ examples:
>                       <&clkc_pll CLKID_FCLK_DIV5>,
>                       <&clkc_pll CLKID_FCLK_DIV7>,
>                       <&clkc_pll CLKID_HIFI_PLL>,
> -                     <&xtal>;
> +                     <&xtal>,
> +                     <&clkc_pll CLKID_SYS_PLL_DIV16>;
>              clock-names = "fclk_div2", "fclk_div3",
>                            "fclk_div5", "fclk_div7",
> -                          "hifi_pll", "xtal";
> +                          "hifi_pll", "xtal", "sys_pll_div16";
>          };
>      };
> -- 
> 2.43.0
> 
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

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

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

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 2906 bytes --]

On Fri, May 10, 2024 at 12:08:54PM +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 clock source sys_pll_div16, being one of the GEN clock parents,
> plays a crucial role and cannot be tagged as "optional". Unfortunately,
> it was not implemented earlier due to the cpu clock ctrl driver's
> pending status on the TODO list.

It's fine to not mark it optional in the binding, but it should be
optional in the driver as otherwise backwards compatibility will be
broken. Given this is an integral clock driver, sounds like it would
quite likely break booting on these devices if the driver doesn't treat
syspll_in as optional.
A lesson perhaps in describing the hardware entirely, even if the
drivers don't make use of all the information yet?

Cheers,
Conor.

> 
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> ---
>  .../devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml     | 7 +++++--
>  include/dt-bindings/clock/amlogic,a1-pll-clkc.h            | 2 ++
>  2 files changed, 7 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..fbba57031278 100644
> --- a/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> @@ -26,11 +26,13 @@ properties:
>      items:
>        - description: input fixpll_in
>        - description: input hifipll_in
> +      - description: input syspll_in
>  
>    clock-names:
>      items:
>        - const: fixpll_in
>        - const: hifipll_in
> +      - const: syspll_in
>  
>  required:
>    - compatible
> @@ -53,7 +55,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..a702d610589c 100644
> --- a/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
> +++ b/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
> @@ -21,5 +21,7 @@
>  #define CLKID_FCLK_DIV5		8
>  #define CLKID_FCLK_DIV7		9
>  #define CLKID_HIFI_PLL		10
> +#define CLKID_SYS_PLL		11
> +#define CLKID_SYS_PLL_DIV16	12
>  
>  #endif /* __A1_PLL_CLKC_H */
> -- 
> 2.43.0
> 
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

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

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

[Dmitry Rokosov]

Hello Conor,

Thank you for quick review!

On Sat, May 11, 2024 at 02:08:03PM +0100, Conor Dooley wrote:
> On Fri, May 10, 2024 at 12:08:54PM +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 clock source sys_pll_div16, being one of the GEN clock parents,
> > plays a crucial role and cannot be tagged as "optional". Unfortunately,
> > it was not implemented earlier due to the cpu clock ctrl driver's
> > pending status on the TODO list.
> 
> It's fine to not mark it optional in the binding, but it should be
> optional in the driver as otherwise backwards compatibility will be
> broken. Given this is an integral clock driver, sounds like it would
> quite likely break booting on these devices if the driver doesn't treat
> syspll_in as optional.
> A lesson perhaps in describing the hardware entirely, even if the
> drivers don't make use of all the information yet?

Yes, it's definitely the right lesson for me. However, without syspll or
syspll_in, we cannot utilize CPU power management at all. I will attempt
to make it an optional feature on the driver side, but it might
necessitate additional conditions to disable CPU clock handling when
syspll is unavailable.

> > 
> > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> > ---
> >  .../devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml     | 7 +++++--
> >  include/dt-bindings/clock/amlogic,a1-pll-clkc.h            | 2 ++
> >  2 files changed, 7 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..fbba57031278 100644
> > --- a/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> > +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
> > @@ -26,11 +26,13 @@ properties:
> >      items:
> >        - description: input fixpll_in
> >        - description: input hifipll_in
> > +      - description: input syspll_in
> >  
> >    clock-names:
> >      items:
> >        - const: fixpll_in
> >        - const: hifipll_in
> > +      - const: syspll_in
> >  
> >  required:
> >    - compatible
> > @@ -53,7 +55,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..a702d610589c 100644
> > --- a/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
> > +++ b/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
> > @@ -21,5 +21,7 @@
> >  #define CLKID_FCLK_DIV5		8
> >  #define CLKID_FCLK_DIV7		9
> >  #define CLKID_HIFI_PLL		10
> > +#define CLKID_SYS_PLL		11
> > +#define CLKID_SYS_PLL_DIV16	12
> >  
> >  #endif /* __A1_PLL_CLKC_H */
> > -- 
> > 2.43.0
> > 
> > 



-- 
Thank you,
Dmitry

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

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

[Jerome Brunet]

On Sat 11 May 2024 at 14:03, Conor Dooley <conor@kernel.org> wrote:

> [[PGP Signed Part:Undecided]]
> On Fri, May 10, 2024 at 12:08:56PM +0300, Dmitry Rokosov wrote:
>> The 'sys_pll_div16' input clock is used as one of the sources for the
>> GEN clock.
>> 
>> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
>
> Provided that this new clock is optional in the driver,
> Acked-by: Conor Dooley <conor.dooley@microchip.com>

The way CCF works, it is not going to crash if DT does not have this.
It will be viewed as non-connected input, in a way

>
> Cheers,
> Conor.
>
>> ---
>>  .../bindings/clock/amlogic,a1-peripherals-clkc.yaml        | 7 +++++--
>>  1 file changed, 5 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..11862746ba44 100644
>> --- a/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml
>> +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-peripherals-clkc.yaml
>> @@ -30,6 +30,7 @@ properties:
>>        - description: input fixed pll div7
>>        - description: input hifi pll
>>        - description: input oscillator (usually at 24MHz)
>> +      - description: input sys pll div16
>>  
>>    clock-names:
>>      items:
>> @@ -39,6 +40,7 @@ properties:
>>        - const: fclk_div7
>>        - const: hifi_pll
>>        - const: xtal
>> +      - const: sys_pll_div16
>>  
>>  required:
>>    - compatible
>> @@ -65,9 +67,10 @@ examples:
>>                       <&clkc_pll CLKID_FCLK_DIV5>,
>>                       <&clkc_pll CLKID_FCLK_DIV7>,
>>                       <&clkc_pll CLKID_HIFI_PLL>,
>> -                     <&xtal>;
>> +                     <&xtal>,
>> +                     <&clkc_pll CLKID_SYS_PLL_DIV16>;
>>              clock-names = "fclk_div2", "fclk_div3",
>>                            "fclk_div5", "fclk_div7",
>> -                          "hifi_pll", "xtal";
>> +                          "hifi_pll", "xtal", "sys_pll_div16";
>>          };
>>      };
>> -- 
>> 2.43.0
>> 
>> 
>
> [[End of PGP Signed Part]]


-- 
Jerome

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

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

[Jerome Brunet]

On Sat 11 May 2024 at 14:08, Conor Dooley <conor@kernel.org> wrote:

> [[PGP Signed Part:Undecided]]
> On Fri, May 10, 2024 at 12:08:54PM +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 clock source sys_pll_div16, being one of the GEN clock parents,
>> plays a crucial role and cannot be tagged as "optional". Unfortunately,
>> it was not implemented earlier due to the cpu clock ctrl driver's
>> pending status on the TODO list.
>
> It's fine to not mark it optional in the binding, but it should be
> optional in the driver as otherwise backwards compatibility will be
> broken. Given this is an integral clock driver, sounds like it would
> quite likely break booting on these devices if the driver doesn't treat
> syspll_in as optional.
> A lesson perhaps in describing the hardware entirely, even if the
> drivers don't make use of all the information yet?

That is nice but it is only possible if/when we have perfect knowledge
of the HW being implemented. I don't know about you, but I rarely get
perfect documentation for HW, let alone a public one.

Those things are bound to happen as we implement support for the HW and
discover how it works, not to mention the mistakes humans will
inevitably do. If Linux was only supporting perfectly documented HW, it
would not be supporting much of them I suspect.

Stable API is already hard with ioctl but there, both sides are
perfectly known. That is a fundamental difference with the 'DT ABI'

Getting it right on day 1, every time - because things are set in stone
afterwards - is unrealistic. As a maintainer, I do spend a
disproportionate amount of time checking the bindings submission because
I know how painful it gets to fix things up down the line.

Unless I missed the simple solution to this problem, we can expect the
problem keep happening again and again, no matter the number of lessons
learned.

>
> Cheers,
> Conor.
>
>> 
>> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
>> ---
>>  .../devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml     | 7 +++++--
>>  include/dt-bindings/clock/amlogic,a1-pll-clkc.h            | 2 ++
>>  2 files changed, 7 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..fbba57031278 100644
>> --- a/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>> +++ b/Documentation/devicetree/bindings/clock/amlogic,a1-pll-clkc.yaml
>> @@ -26,11 +26,13 @@ properties:
>>      items:
>>        - description: input fixpll_in
>>        - description: input hifipll_in
>> +      - description: input syspll_in
>>  
>>    clock-names:
>>      items:
>>        - const: fixpll_in
>>        - const: hifipll_in
>> +      - const: syspll_in
>>  
>>  required:
>>    - compatible
>> @@ -53,7 +55,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..a702d610589c 100644
>> --- a/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
>> +++ b/include/dt-bindings/clock/amlogic,a1-pll-clkc.h
>> @@ -21,5 +21,7 @@
>>  #define CLKID_FCLK_DIV5		8
>>  #define CLKID_FCLK_DIV7		9
>>  #define CLKID_HIFI_PLL		10
>> +#define CLKID_SYS_PLL		11
>> +#define CLKID_SYS_PLL_DIV16	12
>>  
>>  #endif /* __A1_PLL_CLKC_H */
>> -- 
>> 2.43.0
>> 
>> 
>
> [[End of PGP Signed Part]]


-- 
Jerome

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

Re: [PATCH v2 1/7] clk: meson: introduce 'INIT_ONCE' flag to eliminate init for enabled PLL

[Jerome Brunet]

On Fri 10 May 2024 at 12:08, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:

> 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 | 37 +++++++++++++++++++++----------------
>  drivers/clk/meson/clk-pll.h |  1 +
>  2 files changed, 22 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
> index 78d17b2415af..47b22a6be2e4 100644
> --- a/drivers/clk/meson/clk-pll.c
> +++ b/drivers/clk/meson/clk-pll.c
> @@ -289,11 +289,32 @@ 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);
>  
> +	/* Do not init already enabled PLL which marked with 'init_once'
> */

That is decribing the code, which we can read. So not really helpful
Saying why you do it, like "Keep the clock running from the bootloader
stage and avoid glitching it ..." gives more context about what you are
trying to do.

> +	if ((pll->flags & CLK_MESON_PLL_INIT_ONCE) &&

I don't like INIT_ONCE. It gives the false impression that

* The PLL is going to be initialized once in Linux if it has the flag
* Is initialised multiple times otherwise 

I agree that currently that carefully reading the code clears that up
but it is misleading

CLK_MESON_PLL_EN_NOINIT ?

> +	    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 +329,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..23195ea4eae1 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_INIT_ONCE		BIT(1)
>  
>  struct meson_clk_pll_data {
>  	struct parm en;


-- 
Jerome

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

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

[Jerome Brunet]

On Fri 10 May 2024 at 12:08, 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 | 79 ++++++++++++++++++++++++++++++++++++++
>  drivers/clk/meson/a1-pll.h |  6 +++
>  2 files changed, 85 insertions(+)
>
> diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c
> index 60b2e53e7e51..af47ba308bbe 100644
> --- a/drivers/clk/meson/a1-pll.c
> +++ b/drivers/clk/meson/a1-pll.c
> @@ -138,6 +138,82 @@ 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),

Like other 'fishy' flags, I would like a clear comment why this flag is
required so, 2y from now, we will know why it was put there and how we
can deal with it.

> +		.flags = CLK_MESON_PLL_INIT_ONCE,
> +	},
> +	.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 sys_pll_div16 = {
> +	.mult = 1,
> +	.div = 16,
> +	.hw.init = &(struct clk_init_data){
> +		.name = "sys_pll_div16",
> +		.ops = &clk_fixed_factor_ops,
> +		.parent_hws = (const struct clk_hw *[]) {
> +			&sys_pll.hw
> +		},
> +		.num_parents = 1,
> +	},
> +};

Unlike the fdivs, this fixed divider is not part of the diagram
describing the syspll clock.

IMO, it could as well be in peripheral controller because it exists
(from what I can see) just testing purposes, to make the sys pll
observable through tst_out or gen_clk.

It also looks less awkward in the bindings.

> +
>  static struct clk_fixed_factor fclk_div2_div = {
>  	.mult = 1,
>  	.div = 2,
> @@ -283,6 +359,8 @@ 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,
> +	[CLKID_SYS_PLL_DIV16]	= &sys_pll_div16.hw,
>  };
>  
>  static struct clk_regmap *const a1_pll_regmaps[] = {
> @@ -293,6 +371,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-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 2808 bytes --]

On Mon, May 13, 2024 at 02:04:41PM +0200, Jerome Brunet wrote:
> 
> On Sat 11 May 2024 at 14:08, Conor Dooley <conor@kernel.org> wrote:
> 
> > [[PGP Signed Part:Undecided]]
> > On Fri, May 10, 2024 at 12:08:54PM +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 clock source sys_pll_div16, being one of the GEN clock parents,
> >> plays a crucial role and cannot be tagged as "optional". Unfortunately,
> >> it was not implemented earlier due to the cpu clock ctrl driver's
> >> pending status on the TODO list.
> >
> > It's fine to not mark it optional in the binding, but it should be
> > optional in the driver as otherwise backwards compatibility will be
> > broken. Given this is an integral clock driver, sounds like it would
> > quite likely break booting on these devices if the driver doesn't treat
> > syspll_in as optional.
> > A lesson perhaps in describing the hardware entirely, even if the
> > drivers don't make use of all the information yet?
> 
> That is nice but it is only possible if/when we have perfect knowledge
> of the HW being implemented. I don't know about you, but I rarely get
> perfect documentation for HW, let alone a public one.
> 
> Those things are bound to happen as we implement support for the HW and
> discover how it works, not to mention the mistakes humans will
> inevitably do. If Linux was only supporting perfectly documented HW, it
> would not be supporting much of them I suspect.

I mean, you can say what you want chief about what you did or didn't
know, but there's a line in one of the drivers that was added back when
the original driver was that talks about the missing clock, so you can't
really act as if there was no knowledge about it. If it hadn't been
previously known about and TODO-listed, I would not have made these
comments.

> Stable API is already hard with ioctl but there, both sides are
> perfectly known. That is a fundamental difference with the 'DT ABI'
> 
> Getting it right on day 1, every time

Wind your neck in, I don't expect you (or anyone else) to get it right
on "day 1, every time". I only expect it to be dealt with in a way that
is compatible with the existing devicetree.

Thanks,
Conor.

> - because things are set in stone
> afterwards - is unrealistic. As a maintainer, I do spend a
> disproportionate amount of time checking the bindings submission because
> I know how painful it gets to fix things up down the line.
> 
> Unless I missed the simple solution to this problem, we can expect the
> problem keep happening again and again, no matter the number of lessons
> learned.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

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

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

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1472 bytes --]

On Mon, May 13, 2024 at 12:18:02PM +0300, Dmitry Rokosov wrote:
> Hello Conor,
> 
> Thank you for quick review!
> 
> On Sat, May 11, 2024 at 02:08:03PM +0100, Conor Dooley wrote:
> > On Fri, May 10, 2024 at 12:08:54PM +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 clock source sys_pll_div16, being one of the GEN clock parents,
> > > plays a crucial role and cannot be tagged as "optional". Unfortunately,
> > > it was not implemented earlier due to the cpu clock ctrl driver's
> > > pending status on the TODO list.
> > 
> > It's fine to not mark it optional in the binding, but it should be
> > optional in the driver as otherwise backwards compatibility will be
> > broken. Given this is an integral clock driver, sounds like it would
> > quite likely break booting on these devices if the driver doesn't treat
> > syspll_in as optional.
> > A lesson perhaps in describing the hardware entirely, even if the
> > drivers don't make use of all the information yet?
> 
> Yes, it's definitely the right lesson for me. However, without syspll or
> syspll_in, we cannot utilize CPU power management at all.

That's the status-quo, right? The incorrect dts would continue to not
support CPU power management and the new one with the correct description
would?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

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

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

[Dmitry Rokosov]

On Mon, May 13, 2024 at 04:48:33PM +0100, Conor Dooley wrote:
> On Mon, May 13, 2024 at 12:18:02PM +0300, Dmitry Rokosov wrote:
> > Hello Conor,
> > 
> > Thank you for quick review!
> > 
> > On Sat, May 11, 2024 at 02:08:03PM +0100, Conor Dooley wrote:
> > > On Fri, May 10, 2024 at 12:08:54PM +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 clock source sys_pll_div16, being one of the GEN clock parents,
> > > > plays a crucial role and cannot be tagged as "optional". Unfortunately,
> > > > it was not implemented earlier due to the cpu clock ctrl driver's
> > > > pending status on the TODO list.
> > > 
> > > It's fine to not mark it optional in the binding, but it should be
> > > optional in the driver as otherwise backwards compatibility will be
> > > broken. Given this is an integral clock driver, sounds like it would
> > > quite likely break booting on these devices if the driver doesn't treat
> > > syspll_in as optional.
> > > A lesson perhaps in describing the hardware entirely, even if the
> > > drivers don't make use of all the information yet?
> > 
> > Yes, it's definitely the right lesson for me. However, without syspll or
> > syspll_in, we cannot utilize CPU power management at all.
> 
> That's the status-quo, right? The incorrect dts would continue to not
> support CPU power management and the new one with the correct description
> would?

Hmmm, correct. Okay, I see, I will support sys_pll as optional
connection :)

-- 
Thank you,
Dmitry

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

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

[Dmitry Rokosov]

On Mon, May 13, 2024 at 02:48:58PM +0200, Jerome Brunet wrote:
> 
> On Fri 10 May 2024 at 12:08, 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 | 79 ++++++++++++++++++++++++++++++++++++++
> >  drivers/clk/meson/a1-pll.h |  6 +++
> >  2 files changed, 85 insertions(+)
> >
> > diff --git a/drivers/clk/meson/a1-pll.c b/drivers/clk/meson/a1-pll.c
> > index 60b2e53e7e51..af47ba308bbe 100644
> > --- a/drivers/clk/meson/a1-pll.c
> > +++ b/drivers/clk/meson/a1-pll.c
> > @@ -138,6 +138,82 @@ 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),
> 
> Like other 'fishy' flags, I would like a clear comment why this flag is
> required so, 2y from now, we will know why it was put there and how we
> can deal with it.
> 

Yep, you are totally correct. The proper comment is required for that.

> > +		.flags = CLK_MESON_PLL_INIT_ONCE,
> > +	},
> > +	.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 sys_pll_div16 = {
> > +	.mult = 1,
> > +	.div = 16,
> > +	.hw.init = &(struct clk_init_data){
> > +		.name = "sys_pll_div16",
> > +		.ops = &clk_fixed_factor_ops,
> > +		.parent_hws = (const struct clk_hw *[]) {
> > +			&sys_pll.hw
> > +		},
> > +		.num_parents = 1,
> > +	},
> > +};
> 
> Unlike the fdivs, this fixed divider is not part of the diagram
> describing the syspll clock.
> 
> IMO, it could as well be in peripheral controller because it exists
> (from what I can see) just testing purposes, to make the sys pll
> observable through tst_out or gen_clk.
> 
> It also looks less awkward in the bindings.
> 

In any case, it is necessary to introduce a new connection. Instead of
using 'sys_pll_div16', it will now be called 'sys_pll'. I agree with you
that this change will make the code more elegant.

> > +
> >  static struct clk_fixed_factor fclk_div2_div = {
> >  	.mult = 1,
> >  	.div = 2,
> > @@ -283,6 +359,8 @@ 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,
> > +	[CLKID_SYS_PLL_DIV16]	= &sys_pll_div16.hw,
> >  };
> >  
> >  static struct clk_regmap *const a1_pll_regmaps[] = {
> > @@ -293,6 +371,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-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v2 1/7] clk: meson: introduce 'INIT_ONCE' flag to eliminate init for enabled PLL

[Dmitry Rokosov]

On Mon, May 13, 2024 at 02:44:06PM +0200, Jerome Brunet wrote:
> 
> On Fri 10 May 2024 at 12:08, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
> 
> > 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 | 37 +++++++++++++++++++++----------------
> >  drivers/clk/meson/clk-pll.h |  1 +
> >  2 files changed, 22 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
> > index 78d17b2415af..47b22a6be2e4 100644
> > --- a/drivers/clk/meson/clk-pll.c
> > +++ b/drivers/clk/meson/clk-pll.c
> > @@ -289,11 +289,32 @@ 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);
> >  
> > +	/* Do not init already enabled PLL which marked with 'init_once'
> > */
> 
> That is decribing the code, which we can read. So not really helpful
> Saying why you do it, like "Keep the clock running from the bootloader
> stage and avoid glitching it ..." gives more context about what you are
> trying to do.
> 

Yes, I agree with you.

"Instead of describing the action, provide the reasoning behind it."

I will incorporate your feedback in the upcoming version.

> > +	if ((pll->flags & CLK_MESON_PLL_INIT_ONCE) &&
> 
> I don't like INIT_ONCE. It gives the false impression that
> 
> * The PLL is going to be initialized once in Linux if it has the flag
> * Is initialised multiple times otherwise 

But that's how things happen. For previous clocks on other platforms, we
assumed that the PLL could be initialized multiple times: once from the
bootloader and once from Linux. We didn't have the ability to disable
initialization from the Linux side before, so it meant that multiple
initializations were potentially possible by default.

> 
> I agree that currently that carefully reading the code clears that up
> but it is misleading
> 
> CLK_MESON_PLL_EN_NOINIT ?
> 

I have been considering this name and its derivatives, such as:

    CLK_MESON_PLL_SKIP_ENABLED
    CLK_MESON_PLL_NOINIT_ENABLED
    CLK_MESON_PLL_INIT_DISABLED_ONLY

However, I find all of these names to be quite long and bulky. It
reminded me of the WARN_ONCE() function, which ensures that a warning
message is only printed once. In my opinion, the name "INIT_ONCE"
accurately reflects the situation.  Nevertheless, if it is your
requirement for me to change the flag name, I am more than willing to do
so, it's not a problem.

> > +	    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 +329,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..23195ea4eae1 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_INIT_ONCE		BIT(1)
> >  
> >  struct meson_clk_pll_data {
> >  	struct parm en;
> 
> 
> -- 
> Jerome

-- 
Thank you,
Dmitry

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

Re: [PATCH v2 1/7] clk: meson: introduce 'INIT_ONCE' flag to eliminate init for enabled PLL

[Jerome Brunet]

On Tue 14 May 2024 at 00:47, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:

>> 
>> I agree that currently that carefully reading the code clears that up
>> but it is misleading
>> 
>> CLK_MESON_PLL_EN_NOINIT ?
>>                         
>
> I have been considering this name and its derivatives, such as:
>
>     CLK_MESON_PLL_SKIP_ENABLED

>     CLK_MESON_PLL_NOINIT_ENABLED

That one accurately describes what you do.
Use this one please

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

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

[Jerome Brunet]

On Mon 13 May 2024 at 21:30, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:

> On Mon, May 13, 2024 at 04:48:33PM +0100, Conor Dooley wrote:
>> On Mon, May 13, 2024 at 12:18:02PM +0300, Dmitry Rokosov wrote:
>> > Hello Conor,
>> > 
>> > Thank you for quick review!
>> > 
>> > On Sat, May 11, 2024 at 02:08:03PM +0100, Conor Dooley wrote:
>> > > On Fri, May 10, 2024 at 12:08:54PM +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 clock source sys_pll_div16, being one of the GEN clock parents,
>> > > > plays a crucial role and cannot be tagged as "optional". Unfortunately,
>> > > > it was not implemented earlier due to the cpu clock ctrl driver's
>> > > > pending status on the TODO list.
>> > > 
>> > > It's fine to not mark it optional in the binding, but it should be
>> > > optional in the driver as otherwise backwards compatibility will be
>> > > broken. Given this is an integral clock driver, sounds like it would
>> > > quite likely break booting on these devices if the driver doesn't treat
>> > > syspll_in as optional.
>> > > A lesson perhaps in describing the hardware entirely, even if the
>> > > drivers don't make use of all the information yet?
>> > 
>> > Yes, it's definitely the right lesson for me. However, without syspll or
>> > syspll_in, we cannot utilize CPU power management at all.
>> 
>> That's the status-quo, right? The incorrect dts would continue to not
>> support CPU power management and the new one with the correct description
>> would?
>
> Hmmm, correct. Okay, I see, I will support sys_pll as optional
> connection :)

Again, the way controller is written, all inputs are actually optional.
The controller does not error out if an input is missing, it behave as
if the input is disconnected

-- 
Jerome

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