| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <27588ccf-d6f6-42c2-86e2-92cb1addcd24@amlogic.com>
Date: Wed, 26 Nov 2025 11:44:56 +0800
From: Jian Hu <jian.hu@...ogic.com>
To: Jerome Brunet <jbrunet@...libre.com>
Cc: Xianwei Zhao <xianwei.zhao@...ogic.com>, Chuan Liu
<chuan.liu@...ogic.com>, Neil Armstrong <neil.armstrong@...aro.org>,
Kevin Hilman <khilman@...libre.com>, Stephen Boyd <sboyd@...nel.org>,
Michael Turquette <mturquette@...libre.com>,
Dmitry Rokosov <ddrokosov@...rdevices.ru>, robh+dt <robh+dt@...nel.org>,
Rob Herring <robh@...nel.org>, devicetree <devicetree@...r.kernel.org>,
linux-clk <linux-clk@...r.kernel.org>,
linux-amlogic <linux-amlogic@...ts.infradead.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>
Subject: 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 <jian.hu@...ogic.com> wrote:
>
>> Add PLL clock controller driver for the Amlogic T7 SoC family.
>>
>> Signed-off-by: Jian Hu <jian.hu@...ogic.com>
>> ---
>> 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@...ogic.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.
>
>> +#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
Powered by blists - more mailing lists