| 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: <Z_drN1MM-Fi7pM4Q@ketchup>
Date: Thu, 10 Apr 2025 06:54:47 +0000
From: Haylen Chu <heylenay@....org>
To: Alex Elder <elder@...cstar.com>,
Michael Turquette <mturquette@...libre.com>,
Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Haylen Chu <heylenay@...look.com>, Yixun Lan <dlan@...too.org>,
Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>,
Albert Ou <aou@...s.berkeley.edu>, Alexandre Ghiti <alex@...ti.fr>
Cc: linux-riscv@...ts.infradead.org, linux-clk@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
spacemit@...ts.linux.dev, Inochi Amaoto <inochiama@...look.com>,
Chen Wang <unicornxdotw@...mail.com>,
Jisheng Zhang <jszhang@...nel.org>,
Meng Zhang <zhangmeng.kevin@...ux.spacemit.com>
Subject: Re: [PATCH v6 3/6] clk: spacemit: Add clock support for SpacemiT K1
SoC
On Tue, Apr 08, 2025 at 02:37:23PM -0500, Alex Elder wrote:
> On 4/1/25 12:24 PM, Haylen Chu wrote:
> > The clock tree of K1 SoC contains three main types of clock hardware
> > (PLL/DDN/MIX) and has control registers split into several multifunction
> > devices: APBS (PLLs), MPMU, APBC and APMU.
> >
> > All register operations are done through regmap to ensure atomiciy
> > between concurrent operations of clock driver and reset,
> > power-domain driver that will be introduced in the future.
> >
> > Signed-off-by: Haylen Chu <heylenay@....org>
>
> I have a few more comments here but I think this is getting very
> close to ready. You addressed pretty much everything I mentioned.
>
> > ---
> > drivers/clk/Kconfig | 1 +
> > drivers/clk/Makefile | 1 +
> > drivers/clk/spacemit/Kconfig | 18 +
> > drivers/clk/spacemit/Makefile | 5 +
> > drivers/clk/spacemit/apbc_clks | 100 +++
> > drivers/clk/spacemit/ccu-k1.c | 1316 +++++++++++++++++++++++++++++
> > drivers/clk/spacemit/ccu_common.h | 48 ++
> > drivers/clk/spacemit/ccu_ddn.c | 83 ++
> > drivers/clk/spacemit/ccu_ddn.h | 47 ++
> > drivers/clk/spacemit/ccu_mix.c | 268 ++++++
> > drivers/clk/spacemit/ccu_mix.h | 218 +++++
> > drivers/clk/spacemit/ccu_pll.c | 157 ++++
> > drivers/clk/spacemit/ccu_pll.h | 86 ++
> > 13 files changed, 2348 insertions(+)
> > create mode 100644 drivers/clk/spacemit/Kconfig
> > create mode 100644 drivers/clk/spacemit/Makefile
> > create mode 100644 drivers/clk/spacemit/apbc_clks
> > create mode 100644 drivers/clk/spacemit/ccu-k1.c
> > create mode 100644 drivers/clk/spacemit/ccu_common.h
> > create mode 100644 drivers/clk/spacemit/ccu_ddn.c
> > create mode 100644 drivers/clk/spacemit/ccu_ddn.h
> > create mode 100644 drivers/clk/spacemit/ccu_mix.c
> > create mode 100644 drivers/clk/spacemit/ccu_mix.h
> > create mode 100644 drivers/clk/spacemit/ccu_pll.c
> > create mode 100644 drivers/clk/spacemit/ccu_pll.h
> >
...
> > diff --git a/drivers/clk/spacemit/Kconfig b/drivers/clk/spacemit/Kconfig
> > new file mode 100644
> > index 000000000000..4c4df845b3cb
> > --- /dev/null
> > +++ b/drivers/clk/spacemit/Kconfig
> > @@ -0,0 +1,18 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +
> > +config SPACEMIT_CCU
> > + tristate "Clock support for SpacemiT SoCs"
>
> I don't know the answer to this, but... Should this be a Boolean
> rather than tristate? Can a SpacemiT K1 SoC function without the
> clock driver built in to the kernel?
>
As mentioned in the other reply, modules in initramfs are a solution.
I'd like to give the user rights to shrink their kernel image, thus
prefer to keep our driver buildable as a module.
> > + depends on ARCH_SPACEMIT || COMPILE_TEST
> > + select MFD_SYSCON
> > + help
> > + Say Y to enable clock controller unit support for SpacemiT SoCs.
> > +
> > +if SPACEMIT_CCU
> > +
> > +config SPACEMIT_K1_CCU
> > + tristate "Support for SpacemiT K1 SoC"
>
> If you decide SPACEMIT_CCU needs to be Boolean, this one should
> be Boolean too.
>
> > + depends on ARCH_SPACEMIT || COMPILE_TEST
> > + help
> > + Support for clock controller unit in SpacemiT K1 SoC.
> > +
> > +endif
...
> > --- /dev/null
> > +++ b/drivers/clk/spacemit/apbc_clks
>
> This entire file seems to be an accidental addition.
Definitely. I'll remove it in v7.
> > @@ -0,0 +1,100 @@
> > + [CLK_UART0] = &uart0_clk.common.hw,
> > + [CLK_UART2] = &uart2_clk.common.hw,
> > + [CLK_UART3] = &uart3_clk.common.hw,
> > + [CLK_UART4] = &uart4_clk.common.hw,
> > + [CLK_UART5] = &uart5_clk.common.hw,
> > + [CLK_UART6] = &uart6_clk.common.hw,
>
> . . .
>
> > diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c
> > new file mode 100644
> > index 000000000000..cd95c4f9c127
> > --- /dev/null
> > +++ b/drivers/clk/spacemit/ccu-k1.c
> > @@ -0,0 +1,1316 @@
...
> > +/* APBS clocks start, APBS region contains and only contains all PLL clocks */
> > +
> > +/* Frequency of pll{1,2} must not be updated at runtime */
> > +static const struct ccu_pll_rate_tbl pll1_rate_tbl[] = {
> > + CCU_PLL_RATE(2457600000UL, 0x0050dd64, 0x330ccccd),
> > +};
> > +
> > +static const struct ccu_pll_rate_tbl pll2_rate_tbl[] = {
>
> You have added new rates to this PLL table. But you still
> say above that the rate for PLL2 cannot be updated at runtime.
> It's slightly confusing.
As now we have a codepath to adjust PLLs running at unknown rates
to a known state, the extra entries don't really matter. So it could be
removed.
PLL1 and PLL2 are both required to run at fixed frequencies to provide
the exact clock frequency for the peripherals. Technically the frequency
could be alternated as long as the PLL is disabled before changing the
rate. So I'd like to correct the comment as well,
/*
* PLL{1,2} must run at fixed frequencies to provide clocks in correct
* rates for peripherals.
*/
> (By the way, I am pretty sure I've stopped seeing the WARN()
> call. I'm sorry I haven't gotten back to you with more
> details on that. I'll try that when I'm done with this
> review.)
It's nice to hear that my fixes work.
> > + CCU_PLL_RATE(2457600000UL, 0x0050dd64, 0x330ccccd),
> > + CCU_PLL_RATE(2800000000UL, 0x0050dd66, 0x3a155555),
> > + CCU_PLL_RATE(3000000000UL, 0x0050dd66, 0x3fe00000),
> > + CCU_PLL_RATE(3200000000UL, 0x0050dd67, 0x43eaaaab),
> > +};
> > +
> > +static const struct ccu_pll_rate_tbl pll3_rate_tbl[] = {
>
> You added more rates to this table too. You should call attention
> to things like that, when things have changed between versions of
> your patches. (If you did, I missed it.)
I've mentioned it in the cover letter :)
> - synchronize PLL configuration entries with the vendor kernel
>
> > + CCU_PLL_RATE(1600000000UL, 0x0050cd61, 0x43eaaaab),
> > + CCU_PLL_RATE(1800000000UL, 0x0050cd61, 0x4b000000),
> > + CCU_PLL_RATE(2000000000UL, 0x0050dd62, 0x2aeaaaab),
> > + CCU_PLL_RATE(2457600000UL, 0x0050dd64, 0x330ccccd),
> > + CCU_PLL_RATE(3000000000UL, 0x0050dd66, 0x3fe00000),
> > + CCU_PLL_RATE(3200000000UL, 0x0050dd67, 0x43eaaaab),
> > +};
> > +
> > +CCU_PLL_DEFINE(pll1, pll1_rate_tbl, APBS_PLL1_SWCR1, APBS_PLL1_SWCR3, MPMU_POSR,
> > + POSR_PLL1_LOCK, CLK_SET_RATE_GATE);
> > +CCU_PLL_DEFINE(pll2, pll2_rate_tbl, APBS_PLL2_SWCR1, APBS_PLL2_SWCR3, MPMU_POSR,
> > + POSR_PLL2_LOCK, CLK_SET_RATE_GATE);
> > +CCU_PLL_DEFINE(pll3, pll3_rate_tbl, APBS_PLL3_SWCR1, APBS_PLL3_SWCR3, MPMU_POSR,
> > + POSR_PLL3_LOCK, CLK_SET_RATE_GATE);
> > +
>
> I suspect Yixun would like you to have lines like the next one be
> 84 characters wide--slighly wider than the 80 column limit.
>
> I'm not going to ask you to change it (but he might).
Okay, I'm willing to take the style changes in v7 anyway.
...
> > +
> > +struct spacemit_ccu_data {
> > + struct clk_hw **hws;
> > + size_t num;
> > +};
>
> The above structure type ought to be defined nearer to top
> of the source file. (I move it in my reset series, but I
> won't have to if you do.) I make another suggestion about
> this type below.
I thought it helps to have the definition near the place where the first
time it's used. But if you consider it's better to place definitions of
all datatypes at first, I'm willing to follow.
...
> > +static const struct spacemit_ccu_data k1_ccu_apmu_data = {
> > + .hws = k1_ccu_apmu_hws,
> > + .num = ARRAY_SIZE(k1_ccu_apmu_hws),
> > +};
> > +
> > +static int spacemit_ccu_register(struct device *dev,
> > + struct regmap *regmap, struct regmap *lock_regmap,
>
> Since you're splitting the arguments across 3 lines, I'd
> just put each one on its own line. (Personal preference.)
This looks better to me as well. Will take it.
> > + const struct spacemit_ccu_data *data)
> > +{
> > + struct clk_hw_onecell_data *clk_data;
> > + int i, ret;
> > +
> > + clk_data = devm_kzalloc(dev, struct_size(clk_data, hws, data->num),
> > + GFP_KERNEL);
> > + if (!clk_data)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < data->num; i++) {
> > + struct clk_hw *hw = data->hws[i];
> > + struct ccu_common *common;
> > + const char *name;
> > +
> > + if (!hw) {
> > + clk_data->hws[i] = ERR_PTR(-ENOENT);
> > + continue;
> > + }
> > +
> > + name = hw->init->name;
> > +
> > + common = hw_to_ccu_common(hw);
> > + common->regmap = regmap;
> > + common->lock_regmap = lock_regmap;
> > +
> > + ret = devm_clk_hw_register(dev, hw);
> > + if (ret) {
> > + dev_err(dev, "Cannot register clock %d - %s\n",
> > + i, name);
> > + return ret;
> > + }
> > +
> > + clk_data->hws[i] = hw;
> > + }
> > +
> > + clk_data->num = data->num;
> > +
> > + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data);
> > + if (ret)
> > + dev_err(dev, "failed to add clock hardware provider (%d)\n", ret);
> > +
> > + return ret;
> > +}
> > +
> > +static int k1_ccu_probe(struct platform_device *pdev)
> > +{
> > + struct regmap *base_regmap, *lock_regmap = NULL;
> > + struct device *dev = &pdev->dev;
> > + int ret;
> > +
> > + base_regmap = device_node_to_regmap(dev->of_node);
> > + if (IS_ERR(base_regmap))
> > + return dev_err_probe(dev, PTR_ERR(base_regmap),
> > + "failed to get regmap\n");
> > +
> > + /*
> > + * The lock status of PLLs locate in MPMU region, while PLLs themselves
> > + * are in APBS region. Reference to MPMU syscon is required to check PLL
> > + * status.
> > + */
>
> I have two comments on this next section (handling the
> lock being in a different area of memory).
>
> First, now that you're defining a spacemit_ccu_data structure
> type, you could add a field to that type with an identifier (an
> enumerated type, one per CCU).
>
> Then the following test could check that field rather than
> the compatible string. This isn't much different in practice,
> but it would allow future code to make other decisions based
> on the identifier--long after initialization.
I'd like to put off the change until we really need to make such
decisions, afaik we don't in current version of clock and reset driver.
> Second, why does it matter what region the memory is in (whether
> it's MPMU or something else)? I think it would be better to
> specify that as a "lock-region" or maybe "regmap-lock" property.
> It's value is a simple phandle, and you're just looking up the
> regmap from it.
Actually it does somehow matter, offsets of registers representing the
lock status of PLLs are hardcoded, thus I don't think a generic name is
very useful. And we've already had comments in both dt-bindings and
driver to make purpose of the property clear.
> If you think this is a good idea you'd need to get input and
> approval from the DT maintainers.
>
> > + if (of_device_is_compatible(dev->of_node, "spacemit,k1-pll")) {
> > + struct device_node *mpmu = of_parse_phandle(dev->of_node,
> > + "spacemit,mpmu", 0);
> > + if (!mpmu)
> > + return dev_err_probe(dev, -ENODEV,
> > + "Cannot parse MPMU region\n");
> > +
> > + lock_regmap = device_node_to_regmap(mpmu);
> > + of_node_put(mpmu);
> > +
> > + if (IS_ERR(lock_regmap))
> > + return dev_err_probe(dev, PTR_ERR(lock_regmap),
> > + "failed to get lock regmap\n");
> > + }
> > +
> > + ret = spacemit_ccu_register(dev, base_regmap, lock_regmap,
> > + of_device_get_match_data(dev));
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to register clocks\n");
> > +
> > + return 0;
> > +}
>
> . . .
>
> > diff --git a/drivers/clk/spacemit/ccu_pll.c b/drivers/clk/spacemit/ccu_pll.c
> > new file mode 100644
> > index 000000000000..971c489c261a
> > --- /dev/null
> > +++ b/drivers/clk/spacemit/ccu_pll.c
>
> . . .
>
> > +static int ccu_pll_init(struct clk_hw *hw)
> > +{
> > + struct ccu_pll *pll = hw_to_ccu_pll(hw);
> > +
> > + if (ccu_pll_lookup_matched_entry(pll))
> > + return 0;
> > +
> > + ccu_pll_disable(hw);
> > + ccu_pll_update_param(pll, &pll->config.rate_tbl[0]);
> > +
>
> Looks like you now ensure the rate is set to one of those
> in the table if it isn't already. Nice work.
>
> -Alex
And thank you for your patient review and hints.
Regards,
Haylen Chu
Powered by blists - more mailing lists