Re: [PATCH v5 4/5] clk: meson: t7: add support for the T7 SoC PLL clock

Hi, Jerome

Thanks for your review.


On 11/24/2025 5:26 PM, Jerome Brunet wrote:

[ EXTERNAL EMAIL ]

On Fri 21 Nov 2025 at 18:59, Jian Hu [off-list ref] wrote:

quoted

Add PLL clock controller driver for the Amlogic T7 SoC family.

Signed-off-by: Jian Hu <redacted>
---
  drivers/clk/meson/Kconfig  |   14 +
  drivers/clk/meson/Makefile |    1 +
  drivers/clk/meson/t7-pll.c | 1068 ++++++++++++++++++++++++++++++++++++
  3 files changed, 1083 insertions(+)
  create mode 100644 drivers/clk/meson/t7-pll.c

......

diff --git a/drivers/clk/meson/t7-pll.c b/drivers/clk/meson/t7-pll.c
new file mode 100644
index 000000000000..bee8a7489371
--- /dev/null
+++ b/drivers/clk/meson/t7-pll.c

@@ -0,0 +1,1068 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR MIT)
+/*
+ * Copyright (C) 2024-2025 Amlogic, Inc. All rights reserved.
+ * Author: Jian Hu <jian.hu@amlogic.com>
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/platform_device.h>
+#include "clk-regmap.h"
+#include "clk-pll.h"
+#include "clk-mpll.h"
+#include "meson-clkc-utils.h"
+#include <dt-bindings/clock/amlogic,t7-pll-clkc.h>
+
+#define GP0PLL_CTRL0         0x00
+#define GP0PLL_CTRL1         0x04
+#define GP0PLL_CTRL2         0x08
+#define GP0PLL_CTRL3         0x0c
+#define GP0PLL_CTRL4         0x10
+#define GP0PLL_CTRL5         0x14
+#define GP0PLL_CTRL6         0x18
+#define GP0PLL_STS           0x1c

A new line between each register region would help readability

Okay, I will add new line for each register region.

quoted

+#define GP1PLL_CTRL0         0x00
+#define GP1PLL_CTRL1         0x04
+#define GP1PLL_CTRL2         0x08
+#define GP1PLL_CTRL3         0x0c
+#define GP1PLL_STS           0x1c
+#define HIFIPLL_CTRL0                0x00
+#define HIFIPLL_CTRL1                0x04
+#define HIFIPLL_CTRL2                0x08
+#define HIFIPLL_CTRL3                0x0c
+#define HIFIPLL_CTRL4                0x10
+#define HIFIPLL_CTRL5                0x14
+#define HIFIPLL_CTRL6                0x18
+#define HIFIPLL_STS          0x1c
+#define PCIEPLL_CTRL0                0x00
+#define PCIEPLL_CTRL1                0x04
+#define PCIEPLL_CTRL2                0x08
+#define PCIEPLL_CTRL3                0x0c
+#define PCIEPLL_CTRL4                0x10
+#define PCIEPLL_CTRL5                0x14
+#define PCIEPLL_STS          0x18
+#define MPLL_CTRL0           0x00
+#define MPLL_CTRL1           0x04
+#define MPLL_CTRL2           0x08
+#define MPLL_CTRL3           0x0c
+#define MPLL_CTRL4           0x10
+#define MPLL_CTRL5           0x14
+#define MPLL_CTRL6           0x18
+#define MPLL_CTRL7           0x1c
+#define MPLL_CTRL8           0x20
+#define MPLL_STS             0x24
+#define HDMIPLL_CTRL0                0x00
+#define HDMIPLL_CTRL1                0x04
+#define HDMIPLL_CTRL2                0x08
+#define HDMIPLL_CTRL3                0x0c
+#define HDMIPLL_CTRL4                0x10
+#define HDMIPLL_CTRL5                0x14
+#define HDMIPLL_CTRL6                0x18
+#define HDMIPLL_STS          0x1c
+#define MCLK_PLL_CNTL0               0x00
+#define MCLK_PLL_CNTL1               0x04
+#define MCLK_PLL_CNTL2               0x08
+#define MCLK_PLL_CNTL3               0x0c
+#define MCLK_PLL_CNTL4               0x10
+#define MCLK_PLL_STS         0x14
+
+static const struct pll_mult_range t7_media_pll_mult_range = {
+     .min = 125,
+     .max = 250,
+};
+
+static const struct reg_sequence t7_gp0_init_regs[] = {
+     { .reg = GP0PLL_CTRL1,  .def = 0x00000000 },
+     { .reg = GP0PLL_CTRL2,  .def = 0x00000000 },
+     { .reg = GP0PLL_CTRL3,  .def = 0x48681c00 },
+     { .reg = GP0PLL_CTRL4,  .def = 0x88770290 },
+     { .reg = GP0PLL_CTRL5,  .def = 0x3927200a },
+     { .reg = GP0PLL_CTRL6,  .def = 0x56540000 },
+};
+
+static struct clk_regmap t7_gp0_pll_dco = {
+     .data = &(struct meson_clk_pll_data){
+             .en = {
+                     .reg_off = GP0PLL_CTRL0,
+                     .shift   = 28,
+                     .width   = 1,
+             },
+             .m = {
+                     .reg_off = GP0PLL_CTRL0,
+                     .shift   = 0,
+                     .width   = 8,
+             },
+             .n = {
+                     .reg_off = GP0PLL_CTRL0,
+                     .shift   = 10,
+                     .width   = 5,
+             },
+             .l = {
+                     .reg_off = GP0PLL_STS,
+                     .shift   = 31,
+                     .width   = 1,
+             },
+             .rst = {
+                     .reg_off = GP0PLL_CTRL0,
+                     .shift   = 29,
+                     .width   = 1,
+             },
+             .range = &t7_media_pll_mult_range,
+             .init_regs = t7_gp0_init_regs,
+             .init_count = ARRAY_SIZE(t7_gp0_init_regs),
+     },
+     .hw.init = &(struct clk_init_data){
+             .name = "gp0_pll_dco",
+             .ops = &meson_clk_pll_ops,
+             .parent_data = &(const struct clk_parent_data) {
+                     .fw_name = "in0",
+             },
+             .num_parents = 1,
+     },
+};
+
+static struct clk_regmap t7_gp0_pll = {
+     .data = &(struct clk_regmap_div_data){
+             .offset = GP0PLL_CTRL0,
+             .shift = 16,
+             .width = 3,
+             .flags = CLK_DIVIDER_POWER_OF_TWO,
+     },
+     .hw.init = &(struct clk_init_data) {
+             .name = "gp0_pll",
+             .ops = &clk_regmap_divider_ops,
+             .parent_hws = (const struct clk_hw *[]) {
+                     &t7_gp0_pll_dco.hw
+             },
+             .num_parents = 1,
+             .flags = CLK_SET_RATE_PARENT,
+     },
+};
+
+/*
+ * The gp1 pll IP is different with gp0 pll, the PLL DCO range is
+ * 1.6GHZ - 3.2GHZ, and the reg_sequence is short
+ */

Nitpick: if you are not going to explain why the reg_sequence is short
the last part of the comment is not very useful.

Compared with GP0 PLL, GP1 PLL is a newly designed PLL. GP1 PLL DCO 
range is 1.6GHz - 3.2GHz.

Since GP0 uses 7 registers while GP1 uses only 4, the register sequence 
of GP1 is shorter than that of GP0.

It is a known fact from the register definition. I will remove 'the 
reg_requence is short'.


After updated, the comment is:

Compared with GP0 PLL, GP1 PLL is a newly designed PLL with a DCO range 
of 1.6GHz to 3.2GHz.

quoted

+static const struct pll_mult_range t7_gp1_pll_mult_range = {
+     .min = 67,
+     .max = 133,
+};
+
+static const struct reg_sequence t7_gp1_init_regs[] = {
+     { .reg = GP1PLL_CTRL1,  .def = 0x1420500f },
+     { .reg = GP1PLL_CTRL2,  .def = 0x00023001 },
+     { .reg = GP1PLL_CTRL3,  .def = 0x00000000 },
+};
+
......

--
Jerome