[PATCH RFC 0/2] clk: hisilicon: add support for PLL

[PATCH RFC 0/2] clk: hisilicon: add support for PLL

[Yang Xiwen via B4 Relay]

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 <forbidden405@outlook.com>
---
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 <forbidden405@outlook.com>

[PATCH RFC 1/2] clk: hisilicon: rename hi3519 PLL registration function

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

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 <forbidden405@outlook.com>
---
 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

Re: [PATCH RFC 1/2] clk: hisilicon: rename hi3519 PLL registration function

[Stephen Boyd]

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.

Re: [PATCH RFC 1/2] clk: hisilicon: rename hi3519 PLL registration function

[Yang Xiwen]

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

Re: [PATCH RFC 1/2] clk: hisilicon: rename hi3519 PLL registration function

[Stephen Boyd]

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,

Re: [PATCH RFC 1/2] clk: hisilicon: rename hi3519 PLL registration function

[Yang Xiwen]

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

Re: [PATCH RFC 1/2] clk: hisilicon: rename hi3519 PLL registration function

[Stephen Boyd]

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().

[PATCH RFC 2/2] clk: hisilicon: add support for PLL

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

Add support for PLL used by various HiSilicon SoCs

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 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 <forbidden405@outlook.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)
+
+/* 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

Re: [PATCH RFC 2/2] clk: hisilicon: add support for PLL

[Stephen Boyd]

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@outlook.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?

> +
> +/* 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.

Re: [PATCH RFC 2/2] clk: hisilicon: add support for PLL

[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 <forbidden405@outlook.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