| 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:
<SEZPR06MB69599B8BF84A23C579C0047396052@SEZPR06MB6959.apcprd06.prod.outlook.com>
Date: Thu, 11 Apr 2024 15:54:02 +0800
From: Yang Xiwen <forbidden405@...look.com>
To: Stephen Boyd <sboyd@...nel.org>,
Michael Turquette <mturquette@...libre.com>,
Yang Xiwen via B4 Relay <devnull+forbidden405.outlook.com@...nel.org>
Cc: David Yang <mmyangfl@...il.com>, Igor Opaniuk
<igor.opaniuk@...ndries.io>,
Jorge Ramirez-Ortiz Gmail <jorge.ramirez.ortiz@...il.com>,
linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC 2/2] clk: hisilicon: add support for PLL
On 4/11/2024 2:59 PM, Stephen Boyd wrote:
> Quoting Yang Xiwen via B4 Relay (2024-02-24 08:56:10)
>> diff --git a/drivers/clk/hisilicon/clk-pll.c b/drivers/clk/hisilicon/clk-pll.c
>> new file mode 100644
>> index 000000000000..c5c07a65fcf4
>> --- /dev/null
>> +++ b/drivers/clk/hisilicon/clk-pll.c
>> @@ -0,0 +1,171 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * PLL driver for HiSilicon SoCs
>> + *
>> + * Copyright 2024 (c) Yang Xiwen <forbidden405@...look.com>
>> + */
>> +
>> +#include <linux/bitops.h>
>> +#include <linux/device.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +
>> +#include "clk.h"
>> +
>> +/* PLL has two conf regs in total */
>> +#define HISI_PLL_CFG(n) ((n) * 4)
> Isn't HISI_PLL_CFG1 or HISI_PLL_CFG0 shorter then?
Okay. There aren't too many registers anyway.
>
>> +
>> +/* reg 0 definitions */
>> +#define HISI_PLL_FRAC GENMASK(23, 0)
>> +#define HISI_PLL_POSTDIV1 GENMASK(26, 24)
>> +#define HISI_PLL_POSTDIV2 GENMASK(30, 28)
>> +
>> +/* reg 1 definitions */
>> +#define HISI_PLL_FBDIV GENMASK(11, 0)
>> +#define HISI_PLL_REFDIV GENMASK(17, 12)
>> +#define HISI_PLL_PD BIT(20)
>> +#define HISI_PLL_FOUTVCOPD BIT(21)
>> +#define HISI_PLL_FOUT4PHASEPD BIT(22)
>> +#define HISI_PLL_FOUTPOSTDIVPD BIT(23)
>> +#define HISI_PLL_DACPD BIT(24)
>> +#define HISI_PLL_DSMPD BIT(25)
>> +#define HISI_PLL_BYPASS BIT(26)
>> +
>> +/*
>> + * Datasheet said the maximum is 3.2GHz,
>> + * but tests show it can be very high
> Sounds like you're over-clocking. Just follow the datasheet please.
>
>> + *
>> + * Leave some margin here (8 GHz should be fine)
>> + */
>> +#define HISI_PLL_FOUTVCO_MAX_RATE 8000000000
>> +/* 800 MHz */
>> +#define HISI_PLL_FOUTVCO_MIN_RATE 800000000
>> +
>> +struct hisi_pll {
>> + struct clk_hw hw;
>> + void __iomem *base;
>> + u8 postdiv1, postdiv2, refdiv;
>> + u32 divisor;
>> +};
>> +
>> +#define to_hisi_pll(_hw) container_of(_hw, struct hisi_pll, hw)
>> +
>> +static int hisi_pll_prepare(struct clk_hw *hw)
>> +{
>> + struct hisi_pll *pll = to_hisi_pll(hw);
>> + u32 reg;
>> +
>> + reg = readl(pll->base + HISI_PLL_CFG(0));
>> + pll->postdiv1 = FIELD_GET(HISI_PLL_POSTDIV1, reg);
>> + pll->postdiv2 = FIELD_GET(HISI_PLL_POSTDIV2, reg);
>> + // We don't use frac, clear it
> Kernel comments are /* like this */
>
>> + reg &= ~HISI_PLL_FRAC;
>> + writel(reg, pll->base + HISI_PLL_CFG(0));
>> +
>> + reg = readl(pll->base + HISI_PLL_CFG(1));
>> + pll->refdiv = FIELD_GET(HISI_PLL_REFDIV, reg);
>> +
>> + pll->divisor = pll->refdiv * pll->postdiv1 * pll->postdiv2;
>> +
>> + // return -EINVAL if boot loader does not init PLL correctly
> Yeah we got that by reading the code, no comment needed.
>
>> + if (pll->divisor == 0) {
>> + pr_err("%s: PLLs are not initialized by boot loader correctly!\n", __func__);
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int hisi_pll_set_rate(struct clk_hw *hw, ulong rate, ulong parent_rate)
>> +{
>> + struct hisi_pll *pll = to_hisi_pll(hw);
>> + u64 fbdiv = rate * pll->divisor;
>> + u32 reg;
>> +
>> + do_div(fbdiv, parent_rate);
>> +
>> + reg = readl(pll->base + HISI_PLL_CFG(1));
>> + reg &= ~HISI_PLL_FBDIV;
>> + reg |= FIELD_PREP(HISI_PLL_FBDIV, fbdiv);
>> + writel(reg, pll->base + HISI_PLL_CFG(1));
>> +
>> + /* TODO: wait for PLL lock? */
> Yes?
The PLL lock is not implemented for some SoC. I guess it's okay to
simply ignore the lock flag.
>
>> +
>> + return 0;
>> +}
>> +
>> +static int hisi_pll_determine_rate(struct clk_hw *hw, struct clk_rate_request *req)
>> +{
>> + struct hisi_pll *pll = to_hisi_pll(hw);
>> + u64 vco, ref_rate = req->best_parent_rate;
>> +
>> + if (ref_rate == 0)
>> + return -EINVAL;
>> +
>> + do_div(ref_rate, pll->refdiv);
>> + vco = clamp(req->rate * (pll->postdiv1 * pll->postdiv2),
>> + HISI_PLL_FOUTVCO_MIN_RATE, HISI_PLL_FOUTVCO_MAX_RATE);
>> + vco = rounddown(vco, ref_rate);
>> + if (vco < HISI_PLL_FOUTVCO_MIN_RATE)
>> + vco += ref_rate;
>> +
>> + do_div(vco, pll->postdiv1 * pll->postdiv2);
>> + req->rate = vco;
>> +
>> + return 0;
>> +}
>> +
>> +static ulong hisi_pll_recalc_rate(struct clk_hw *hw, ulong parent_rate)
>> +{
>> + struct hisi_pll *pll = to_hisi_pll(hw);
>> + u32 reg, fbdiv;
>> +
>> + reg = readl(pll->base + HISI_PLL_CFG(1));
>> + fbdiv = FIELD_GET(HISI_PLL_FBDIV, reg);
>> + parent_rate *= fbdiv;
>> + do_div(parent_rate, pll->divisor);
>> +
>> + return parent_rate;
>> +}
>> +
>> +static const struct clk_ops hisi_pll_ops = {
>> + .prepare = hisi_pll_prepare,
>> + .set_rate = hisi_pll_set_rate,
>> + .determine_rate = hisi_pll_determine_rate,
>> + .recalc_rate = hisi_pll_recalc_rate,
>> +};
>> +
>> +/*
>> + * devm_hisi_pll_register - register a HiSilicon PLL
> Use kernel-doc please https://docs.kernel.org/doc-guide/kernel-doc.html
>
>> + *
>> + * @dev: clk provider
>> + * @name: clock name
>> + * @parent_name: parent clock, usually 24MHz OSC
>> + * #flags: CCF common flags
>> + * @reg: register address
> Missing Return:
>
>> + */
>> +struct clk *devm_clk_register_hisi_pll(struct device *dev, const char *name, const char *parent,
>> + unsigned int flags, void __iomem *reg)
>> +{
>> + struct hisi_pll *pll;
>> + struct clk_init_data init;
>> +
>> + pll = devm_kzalloc(dev, sizeof(*pll), GFP_KERNEL);
>> + if (!pll)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + if (!parent)
>> + return ERR_PTR(-EINVAL);
>> +
>> + init.name = name;
>> + init.ops = &hisi_pll_ops;
>> + init.flags = flags;
>> + init.parent_names = &parent;
>> + init.num_parents = 1;
>> +
>> + pll->base = reg;
>> + pll->hw.init = &init;
>> +
>> + return devm_clk_register(dev, &pll->hw);
>> +}
>> +EXPORT_SYMBOL_GPL(devm_clk_register_hisi_pll);
>> diff --git a/drivers/clk/hisilicon/clk.h b/drivers/clk/hisilicon/clk.h
>> index 7a9b42e1b027..8c59f3927152 100644
>> --- a/drivers/clk/hisilicon/clk.h
>> +++ b/drivers/clk/hisilicon/clk.h
>> @@ -103,6 +103,14 @@ struct hisi_gate_clock {
>> const char *alias;
>> };
>>
>> +struct hisi_pll_clock {
>> + unsigned int id;
>> + const char *name;
>> + const char *parent_name;
> No string parent names for new code. Use struct clk_parent_data or
> clk_hw directly.
Would be inconsistent with other HiSilicon clock APIs. But i think it's
okay.
--
Regards,
Yang Xiwen
Powered by blists - more mailing lists