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

From: Jian Hu
Date: Tue Nov 25 2025 - 22:45:12 EST

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 <jian.hu@xxxxxxxxxxx> wrote:

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

Signed-off-by: Jian Hu <jian.hu@xxxxxxxxxxx>
---
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@xxxxxxxxxxx>
+ */
+
+#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.


+#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.

+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