HiSilicon PLLs are used by various SoCs to provide variable clocks for
various system on the SoC.
Hi3559 has implemented their own PLL driver. Also fix name duplication
because of that.
Signed-off-by: Yang Xiwen <[email protected]>
---
Yang Xiwen (2):
clk: hisilicon: rename hi3519 PLL registration function
clk: hisilicon: add support for PLL
drivers/clk/hisilicon/Makefile | 2 +-
drivers/clk/hisilicon/clk-hi3559a.c | 4 +-
drivers/clk/hisilicon/clk-pll.c | 171 ++++++++++++++++++++++++++++++++++++
drivers/clk/hisilicon/clk.c | 24 +++++
drivers/clk/hisilicon/clk.h | 12 +++
5 files changed, 210 insertions(+), 3 deletions(-)
---
base-commit: 8d3dea210042f54b952b481838c1e7dfc4ec751d
change-id: 20240225-pll-2653677c5e1d
Best regards,
--
Yang Xiwen <[email protected]>
From: Yang Xiwen <[email protected]>
Hi3559 clock drivers implemented their own PLL driver. Unfortunately
our generic PLL driver will use a same name. So add a prefix "_" to
avoid that.
Signed-off-by: Yang Xiwen <[email protected]>
---
drivers/clk/hisilicon/clk-hi3559a.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/hisilicon/clk-hi3559a.c b/drivers/clk/hisilicon/clk-hi3559a.c
index ff4ca0edce06..77fa4203a428 100644
--- a/drivers/clk/hisilicon/clk-hi3559a.c
+++ b/drivers/clk/hisilicon/clk-hi3559a.c
@@ -452,7 +452,7 @@ static const struct clk_ops hisi_clk_pll_ops = {
.recalc_rate = clk_pll_recalc_rate,
};
-static void hisi_clk_register_pll(struct hi3559av100_pll_clock *clks,
+static void _hisi_clk_register_pll(struct hi3559av100_pll_clock *clks,
int nums, struct hisi_clock_data *data, struct device *dev)
{
void __iomem *base = data->base;
@@ -517,7 +517,7 @@ static struct hisi_clock_data *hi3559av100_clk_register(
if (ret)
return ERR_PTR(ret);
- hisi_clk_register_pll(hi3559av100_pll_clks,
+ _hisi_clk_register_pll(hi3559av100_pll_clks,
ARRAY_SIZE(hi3559av100_pll_clks), clk_data, &pdev->dev);
ret = hisi_clk_register_mux(hi3559av100_mux_clks_crg,
--
2.43.0
From: Yang Xiwen <[email protected]>
Add support for PLL used by various HiSilicon SoCs
Signed-off-by: Yang Xiwen <[email protected]>
---
drivers/clk/hisilicon/Makefile | 2 +-
drivers/clk/hisilicon/clk-pll.c | 171 ++++++++++++++++++++++++++++++++++++++++
drivers/clk/hisilicon/clk.c | 24 ++++++
drivers/clk/hisilicon/clk.h | 12 +++
4 files changed, 208 insertions(+), 1 deletion(-)
diff --git a/drivers/clk/hisilicon/Makefile b/drivers/clk/hisilicon/Makefile
index 2978e56cb876..5e4d54b4cdd3 100644
--- a/drivers/clk/hisilicon/Makefile
+++ b/drivers/clk/hisilicon/Makefile
@@ -3,7 +3,7 @@
# Hisilicon Clock specific Makefile
#
-obj-y += clk.o clkgate-separated.o clkdivider-hi6220.o clk-hisi-phase.o
+obj-y += clk.o clkgate-separated.o clkdivider-hi6220.o clk-hisi-phase.o clk-pll.o
obj-$(CONFIG_ARCH_HI3xxx) += clk-hi3620.o
obj-$(CONFIG_ARCH_HIP04) += clk-hip04.o
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 <[email protected]>
+ */
+
+#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)
+
+/* 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
+ *
+ * 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
+ 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
+ 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? */
+
+ 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
+ *
+ * @dev: clk provider
+ * @name: clock name
+ * @parent_name: parent clock, usually 24MHz OSC
+ * #flags: CCF common flags
+ * @reg: register address
+ */
+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.c b/drivers/clk/hisilicon/clk.c
index 09368fd32bef..0e9b0f13b494 100644
--- a/drivers/clk/hisilicon/clk.c
+++ b/drivers/clk/hisilicon/clk.c
@@ -341,3 +341,27 @@ void __init hi6220_clk_register_divider(const struct hi6220_divider_clock *clks,
data->clk_data.clks[clks[i].id] = clk;
}
}
+
+int hisi_clk_register_pll(struct device *dev, const struct hisi_pll_clock *clks,
+ int nums, struct hisi_clock_data *data)
+{
+ struct clk *clk;
+ void __iomem *base = data->base;
+ int i;
+
+ for (i = 0; i < nums; i++) {
+ clk = devm_clk_register_hisi_pll(dev, clks[i].name, clks[i].parent_name,
+ clks[i].flags, base + clks[i].offset);
+ if (IS_ERR(clk)) {
+ pr_err("%s: failed to register clock %s\n",
+ __func__, clks[i].name);
+ return PTR_ERR(clk);
+ }
+
+
+ data->clk_data.clks[clks[i].id] = clk;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(hisi_clk_register_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;
+ unsigned long flags;
+ unsigned long offset;
+};
+
struct clk *hisi_register_clkgate_sep(struct device *, const char *,
const char *, unsigned long,
void __iomem *, u8,
@@ -122,6 +130,8 @@ int hisi_clk_register_mux(const struct hisi_mux_clock *, int,
struct clk *clk_register_hisi_phase(struct device *dev,
const struct hisi_phase_clock *clks,
void __iomem *base, spinlock_t *lock);
+struct clk *devm_clk_register_hisi_pll(struct device *dev, const char *name, const char *parent,
+ unsigned int flags, void __iomem *reg);
int hisi_clk_register_phase(struct device *dev,
const struct hisi_phase_clock *clks,
int nums, struct hisi_clock_data *data);
@@ -133,6 +143,8 @@ void hisi_clk_register_gate_sep(const struct hisi_gate_clock *,
int, struct hisi_clock_data *);
void hi6220_clk_register_divider(const struct hi6220_divider_clock *,
int, struct hisi_clock_data *);
+int hisi_clk_register_pll(struct device *dev, const struct hisi_pll_clock *clks,
+ int nums, struct hisi_clock_data *data);
#define hisi_clk_unregister(type) \
static inline \
--
2.43.0
Quoting Yang Xiwen via B4 Relay (2024-02-24 08:56:09)
> diff --git a/drivers/clk/hisilicon/clk-hi3559a.c b/drivers/clk/hisilicon/clk-hi3559a.c
> index ff4ca0edce06..77fa4203a428 100644
> --- a/drivers/clk/hisilicon/clk-hi3559a.c
> +++ b/drivers/clk/hisilicon/clk-hi3559a.c
> @@ -452,7 +452,7 @@ static const struct clk_ops hisi_clk_pll_ops = {
> .recalc_rate = clk_pll_recalc_rate,
> };
>
> -static void hisi_clk_register_pll(struct hi3559av100_pll_clock *clks,
> +static void _hisi_clk_register_pll(struct hi3559av100_pll_clock *clks,
Prefix it with hi3559a then to be SoC specific please. But this is also
static so I'm not sure why this patch is needed at all.
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 <[email protected]>
> + */
> +
> +#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?
> +
> +/* 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?
> +
> + 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.
On 4/11/2024 2:52 PM, Stephen Boyd wrote:
> Quoting Yang Xiwen via B4 Relay (2024-02-24 08:56:09)
>> diff --git a/drivers/clk/hisilicon/clk-hi3559a.c b/drivers/clk/hisilicon/clk-hi3559a.c
>> index ff4ca0edce06..77fa4203a428 100644
>> --- a/drivers/clk/hisilicon/clk-hi3559a.c
>> +++ b/drivers/clk/hisilicon/clk-hi3559a.c
>> @@ -452,7 +452,7 @@ static const struct clk_ops hisi_clk_pll_ops = {
>> .recalc_rate = clk_pll_recalc_rate,
>> };
>>
>> -static void hisi_clk_register_pll(struct hi3559av100_pll_clock *clks,
>> +static void _hisi_clk_register_pll(struct hi3559av100_pll_clock *clks,
> Prefix it with hi3559a then to be SoC specific please. But this is also
> static so I'm not sure why this patch is needed at all.
it includes the header that marks this function non-static. Also the
prototype is incompatible.
--
Regards,
Yang Xiwen
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 <[email protected]>
>> + */
>> +
>> +#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
Quoting Yang Xiwen (2024-04-11 00:44:33)
> On 4/11/2024 2:52 PM, Stephen Boyd wrote:
> > Quoting Yang Xiwen via B4 Relay (2024-02-24 08:56:09)
> >> diff --git a/drivers/clk/hisilicon/clk-hi3559a.c b/drivers/clk/hisilicon/clk-hi3559a.c
> >> index ff4ca0edce06..77fa4203a428 100644
> >> --- a/drivers/clk/hisilicon/clk-hi3559a.c
> >> +++ b/drivers/clk/hisilicon/clk-hi3559a.c
> >> @@ -452,7 +452,7 @@ static const struct clk_ops hisi_clk_pll_ops = {
> >> .recalc_rate = clk_pll_recalc_rate,
> >> };
> >>
> >> -static void hisi_clk_register_pll(struct hi3559av100_pll_clock *clks,
> >> +static void _hisi_clk_register_pll(struct hi3559av100_pll_clock *clks,
> > Prefix it with hi3559a then to be SoC specific please. But this is also
> > static so I'm not sure why this patch is needed at all.
>
>
> it includes the header that marks this function non-static. Also the
> prototype is incompatible.
What is 'it'?
$ git grep hisi_clk_register_pll
drivers/clk/hisilicon/clk-hi3559a.c:static void hisi_clk_register_pll(struct hi3559av100_pll_clock *clks,
drivers/clk/hisilicon/clk-hi3559a.c: hisi_clk_register_pll(hi3559av100_pll_clks,
On 4/11/2024 3:53 PM, Stephen Boyd wrote:
> Quoting Yang Xiwen (2024-04-11 00:44:33)
>> On 4/11/2024 2:52 PM, Stephen Boyd wrote:
>>> Quoting Yang Xiwen via B4 Relay (2024-02-24 08:56:09)
>>>> diff --git a/drivers/clk/hisilicon/clk-hi3559a.c b/drivers/clk/hisilicon/clk-hi3559a.c
>>>> index ff4ca0edce06..77fa4203a428 100644
>>>> --- a/drivers/clk/hisilicon/clk-hi3559a.c
>>>> +++ b/drivers/clk/hisilicon/clk-hi3559a.c
>>>> @@ -452,7 +452,7 @@ static const struct clk_ops hisi_clk_pll_ops = {
>>>> .recalc_rate = clk_pll_recalc_rate,
>>>> };
>>>>
>>>> -static void hisi_clk_register_pll(struct hi3559av100_pll_clock *clks,
>>>> +static void _hisi_clk_register_pll(struct hi3559av100_pll_clock *clks,
>>> Prefix it with hi3559a then to be SoC specific please. But this is also
>>> static so I'm not sure why this patch is needed at all.
>>
>> it includes the header that marks this function non-static. Also the
>> prototype is incompatible.
> What is 'it'?
The line 18 `#include "clk.h"`, and please see patch 2.
Patch 2 added 2 functions to "clk.h", one of them reused the
`hisi_clk_register_pll` name with a different prototype.
>
> $ git grep hisi_clk_register_pll
> drivers/clk/hisilicon/clk-hi3559a.c:static void hisi_clk_register_pll(struct hi3559av100_pll_clock *clks,
> drivers/clk/hisilicon/clk-hi3559a.c: hisi_clk_register_pll(hi3559av100_pll_clks,
a snippet copied from patch 2:
+int hisi_clk_register_pll(struct device *dev, const struct hisi_pll_clock *clks,
+ int nums, struct hisi_clock_data *data);
--
Regards,
Yang Xiwen
Quoting Yang Xiwen (2024-04-11 03:31:58)
> On 4/11/2024 3:53 PM, Stephen Boyd wrote:
> > Quoting Yang Xiwen (2024-04-11 00:44:33)
> >> On 4/11/2024 2:52 PM, Stephen Boyd wrote:
> >>> Quoting Yang Xiwen via B4 Relay (2024-02-24 08:56:09)
> >>>> diff --git a/drivers/clk/hisilicon/clk-hi3559a.c b/drivers/clk/hisilicon/clk-hi3559a.c
> >>>> index ff4ca0edce06..77fa4203a428 100644
> >>>> --- a/drivers/clk/hisilicon/clk-hi3559a.c
> >>>> +++ b/drivers/clk/hisilicon/clk-hi3559a.c
> >>>> @@ -452,7 +452,7 @@ static const struct clk_ops hisi_clk_pll_ops = {
> >>>> .recalc_rate = clk_pll_recalc_rate,
> >>>> };
> >>>>
> >>>> -static void hisi_clk_register_pll(struct hi3559av100_pll_clock *clks,
> >>>> +static void _hisi_clk_register_pll(struct hi3559av100_pll_clock *clks,
> >>> Prefix it with hi3559a then to be SoC specific please. But this is also
> >>> static so I'm not sure why this patch is needed at all.
> >>
> >> it includes the header that marks this function non-static. Also the
> >> prototype is incompatible.
> > What is 'it'?
>
>
> The line 18 `#include "clk.h"`, and please see patch 2.
>
>
> Patch 2 added 2 functions to "clk.h", one of them reused the
> `hisi_clk_register_pll` name with a different prototype.
>
>
> >
> > $ git grep hisi_clk_register_pll
> > drivers/clk/hisilicon/clk-hi3559a.c:static void hisi_clk_register_pll(struct hi3559av100_pll_clock *clks,
> > drivers/clk/hisilicon/clk-hi3559a.c: hisi_clk_register_pll(hi3559av100_pll_clks,
>
>
> a snippet copied from patch 2:
>
>
> +int hisi_clk_register_pll(struct device *dev, const struct hisi_pll_clock *clks,
> + int nums, struct hisi_clock_data *data);
>
>
Ok, got it. Prefix the existing hisi_clk_register_pll() as
hi3559a_clk_register_pll().