[PATCH 0/4] Introduce MediaTek frequency hopping driver

[PATCH 0/4] Introduce MediaTek frequency hopping driver

[Johnson Wang]

Introduce MediaTek frequency hopping and spread spectrum clocking control
for MT8186.

Johnson Wang (4):
  clk: mediatek: Export PLL operations symbols
  dt-bindings: arm: mediatek: Add new bindings of MediaTek frequency
    hopping
  clk: mediatek: Add new clock driver to handle FHCTL hardware
  clk: mediatek: Change PLL register API for MT8186

 .../bindings/arm/mediatek/mediatek,fhctl.yaml |  49 ++++
 drivers/clk/mediatek/Makefile                 |   2 +-
 drivers/clk/mediatek/clk-fhctl.c              | 258 +++++++++++++++++
 drivers/clk/mediatek/clk-fhctl.h              |  27 ++
 drivers/clk/mediatek/clk-mt8186-apmixedsys.c  |  65 ++++-
 drivers/clk/mediatek/clk-pll.c                |  84 +++---
 drivers/clk/mediatek/clk-pll.h                |  56 ++++
 drivers/clk/mediatek/clk-pllfh.c              | 271 ++++++++++++++++++
 drivers/clk/mediatek/clk-pllfh.h              |  81 ++++++
 9 files changed, 839 insertions(+), 54 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml
 create mode 100644 drivers/clk/mediatek/clk-fhctl.c
 create mode 100644 drivers/clk/mediatek/clk-fhctl.h
 create mode 100644 drivers/clk/mediatek/clk-pllfh.c
 create mode 100644 drivers/clk/mediatek/clk-pllfh.h

-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/4] clk: mediatek: Export PLL operations symbols

[Johnson Wang]

Export PLL operations and register functions for different type
of clock driver used.

Co-developed-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
Signed-off-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
---
 drivers/clk/mediatek/clk-pll.c | 84 ++++++++++++++--------------------
 drivers/clk/mediatek/clk-pll.h | 56 +++++++++++++++++++++++
 2 files changed, 90 insertions(+), 50 deletions(-)

diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
index 54e6cfd29dfc..a4eca5fd539c 100644
--- a/drivers/clk/mediatek/clk-pll.c
+++ b/drivers/clk/mediatek/clk-pll.c
@@ -27,37 +27,10 @@
 
 #define AUDPLL_TUNER_EN		BIT(31)
 
-#define POSTDIV_MASK		0x7
-
 /* default 7 bits integer, can be overridden with pcwibits. */
 #define INTEGER_BITS		7
 
-/*
- * MediaTek PLLs are configured through their pcw value. The pcw value describes
- * a divider in the PLL feedback loop which consists of 7 bits for the integer
- * part and the remaining bits (if present) for the fractional part. Also they
- * have a 3 bit power-of-two post divider.
- */
-
-struct mtk_clk_pll {
-	struct clk_hw	hw;
-	void __iomem	*base_addr;
-	void __iomem	*pd_addr;
-	void __iomem	*pwr_addr;
-	void __iomem	*tuner_addr;
-	void __iomem	*tuner_en_addr;
-	void __iomem	*pcw_addr;
-	void __iomem	*pcw_chg_addr;
-	void __iomem	*en_addr;
-	const struct mtk_pll_data *data;
-};
-
-static inline struct mtk_clk_pll *to_mtk_clk_pll(struct clk_hw *hw)
-{
-	return container_of(hw, struct mtk_clk_pll, hw);
-}
-
-static int mtk_pll_is_prepared(struct clk_hw *hw)
+int mtk_pll_is_prepared(struct clk_hw *hw)
 {
 	struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
 
@@ -161,8 +134,8 @@ static void mtk_pll_set_rate_regs(struct mtk_clk_pll *pll, u32 pcw,
  * @fin:	The input frequency
  *
  */
-static void mtk_pll_calc_values(struct mtk_clk_pll *pll, u32 *pcw, u32 *postdiv,
-		u32 freq, u32 fin)
+void mtk_pll_calc_values(struct mtk_clk_pll *pll, u32 *pcw, u32 *postdiv,
+			 u32 freq, u32 fin)
 {
 	unsigned long fmin = pll->data->fmin ? pll->data->fmin : (1000 * MHZ);
 	const struct mtk_pll_div_table *div_table = pll->data->div_table;
@@ -198,8 +171,8 @@ static void mtk_pll_calc_values(struct mtk_clk_pll *pll, u32 *pcw, u32 *postdiv,
 	*pcw = (u32)_pcw;
 }
 
-static int mtk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
-		unsigned long parent_rate)
+int mtk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
+		     unsigned long parent_rate)
 {
 	struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
 	u32 pcw = 0;
@@ -211,8 +184,7 @@ static int mtk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
 	return 0;
 }
 
-static unsigned long mtk_pll_recalc_rate(struct clk_hw *hw,
-		unsigned long parent_rate)
+unsigned long mtk_pll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
 {
 	struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
 	u32 postdiv;
@@ -227,8 +199,8 @@ static unsigned long mtk_pll_recalc_rate(struct clk_hw *hw,
 	return __mtk_pll_recalc_rate(pll, parent_rate, pcw, postdiv);
 }
 
-static long mtk_pll_round_rate(struct clk_hw *hw, unsigned long rate,
-		unsigned long *prate)
+long mtk_pll_round_rate(struct clk_hw *hw, unsigned long rate,
+			unsigned long *prate)
 {
 	struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
 	u32 pcw = 0;
@@ -239,7 +211,7 @@ static long mtk_pll_round_rate(struct clk_hw *hw, unsigned long rate,
 	return __mtk_pll_recalc_rate(pll, *prate, pcw, postdiv);
 }
 
-static int mtk_pll_prepare(struct clk_hw *hw)
+int mtk_pll_prepare(struct clk_hw *hw)
 {
 	struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
 	u32 r;
@@ -273,7 +245,7 @@ static int mtk_pll_prepare(struct clk_hw *hw)
 	return 0;
 }
 
-static void mtk_pll_unprepare(struct clk_hw *hw)
+void mtk_pll_unprepare(struct clk_hw *hw)
 {
 	struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
 	u32 r;
@@ -301,7 +273,7 @@ static void mtk_pll_unprepare(struct clk_hw *hw)
 	writel(r, pll->pwr_addr);
 }
 
-static const struct clk_ops mtk_pll_ops = {
+const struct clk_ops mtk_pll_ops = {
 	.is_prepared	= mtk_pll_is_prepared,
 	.prepare	= mtk_pll_prepare,
 	.unprepare	= mtk_pll_unprepare,
@@ -310,18 +282,15 @@ static const struct clk_ops mtk_pll_ops = {
 	.set_rate	= mtk_pll_set_rate,
 };
 
-static struct clk_hw *mtk_clk_register_pll(const struct mtk_pll_data *data,
-		void __iomem *base)
+struct clk_hw *mtk_clk_register_pll_ops(struct mtk_clk_pll *pll,
+					const struct mtk_pll_data *data,
+					void __iomem *base,
+					const struct clk_ops *pll_ops)
 {
-	struct mtk_clk_pll *pll;
 	struct clk_init_data init = {};
 	int ret;
 	const char *parent_name = "clk26m";
 
-	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
-	if (!pll)
-		return ERR_PTR(-ENOMEM);
-
 	pll->base_addr = base + data->reg;
 	pll->pwr_addr = base + data->pwr_reg;
 	pll->pd_addr = base + data->pd_reg;
@@ -343,7 +312,7 @@ static struct clk_hw *mtk_clk_register_pll(const struct mtk_pll_data *data,
 
 	init.name = data->name;
 	init.flags = (data->flags & PLL_AO) ? CLK_IS_CRITICAL : 0;
-	init.ops = &mtk_pll_ops;
+	init.ops = pll_ops;
 	if (data->parent_name)
 		init.parent_names = &data->parent_name;
 	else
@@ -360,7 +329,22 @@ static struct clk_hw *mtk_clk_register_pll(const struct mtk_pll_data *data,
 	return &pll->hw;
 }
 
-static void mtk_clk_unregister_pll(struct clk_hw *hw)
+struct clk_hw *mtk_clk_register_pll(const struct mtk_pll_data *data,
+				    void __iomem *base)
+{
+	struct mtk_clk_pll *pll;
+	struct clk_hw *hw;
+
+	pll = kzalloc(sizeof(*pll), GFP_KERNEL);
+	if (!pll)
+		return ERR_PTR(-ENOMEM);
+
+	hw = mtk_clk_register_pll_ops(pll, data, base, &mtk_pll_ops);
+
+	return hw;
+}
+
+void mtk_clk_unregister_pll(struct clk_hw *hw)
 {
 	struct mtk_clk_pll *pll;
 
@@ -423,8 +407,8 @@ int mtk_clk_register_plls(struct device_node *node,
 }
 EXPORT_SYMBOL_GPL(mtk_clk_register_plls);
 
-static __iomem void *mtk_clk_pll_get_base(struct clk_hw *hw,
-					  const struct mtk_pll_data *data)
+__iomem void *mtk_clk_pll_get_base(struct clk_hw *hw,
+				   const struct mtk_pll_data *data)
 {
 	struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
 
diff --git a/drivers/clk/mediatek/clk-pll.h b/drivers/clk/mediatek/clk-pll.h
index fe3199715688..e87ab08eea9b 100644
--- a/drivers/clk/mediatek/clk-pll.h
+++ b/drivers/clk/mediatek/clk-pll.h
@@ -7,6 +7,7 @@
 #ifndef __DRV_CLK_MTK_PLL_H
 #define __DRV_CLK_MTK_PLL_H
 
+#include <linux/clk-provider.h>
 #include <linux/types.h>
 
 struct clk_ops;
@@ -20,6 +21,7 @@ struct mtk_pll_div_table {
 
 #define HAVE_RST_BAR	BIT(0)
 #define PLL_AO		BIT(1)
+#define POSTDIV_MASK	0x7
 
 struct mtk_pll_data {
 	int id;
@@ -48,10 +50,64 @@ struct mtk_pll_data {
 	u8 pll_en_bit; /* Assume 0, indicates BIT(0) by default */
 };
 
+/*
+ * MediaTek PLLs are configured through their pcw value. The pcw value describes
+ * a divider in the PLL feedback loop which consists of 7 bits for the integer
+ * part and the remaining bits (if present) for the fractional part. Also they
+ * have a 3 bit power-of-two post divider.
+ */
+
+struct mtk_clk_pll {
+	struct clk_hw	hw;
+	void __iomem	*base_addr;
+	void __iomem	*pd_addr;
+	void __iomem	*pwr_addr;
+	void __iomem	*tuner_addr;
+	void __iomem	*tuner_en_addr;
+	void __iomem	*pcw_addr;
+	void __iomem	*pcw_chg_addr;
+	void __iomem	*en_addr;
+	const struct mtk_pll_data *data;
+};
+
+
 int mtk_clk_register_plls(struct device_node *node,
 			  const struct mtk_pll_data *plls, int num_plls,
 			  struct clk_hw_onecell_data *clk_data);
 void mtk_clk_unregister_plls(const struct mtk_pll_data *plls, int num_plls,
 			     struct clk_hw_onecell_data *clk_data);
 
+extern const struct clk_ops mtk_pll_ops;
+
+static inline struct mtk_clk_pll *to_mtk_clk_pll(struct clk_hw *hw)
+{
+	return container_of(hw, struct mtk_clk_pll, hw);
+}
+
+int mtk_pll_is_prepared(struct clk_hw *hw);
+
+int mtk_pll_prepare(struct clk_hw *hw);
+
+void mtk_pll_unprepare(struct clk_hw *hw);
+
+unsigned long mtk_pll_recalc_rate(struct clk_hw *hw, unsigned long parent_rate);
+
+void mtk_pll_calc_values(struct mtk_clk_pll *pll, u32 *pcw, u32 *postdiv,
+			 u32 freq, u32 fin);
+int mtk_pll_set_rate(struct clk_hw *hw, unsigned long rate,
+		     unsigned long parent_rate);
+long mtk_pll_round_rate(struct clk_hw *hw, unsigned long rate,
+			unsigned long *prate);
+
+struct clk_hw *mtk_clk_register_pll_ops(struct mtk_clk_pll *pll,
+					const struct mtk_pll_data *data,
+					void __iomem *base,
+					const struct clk_ops *pll_ops);
+struct clk_hw *mtk_clk_register_pll(const struct mtk_pll_data *data,
+				    void __iomem *base);
+void mtk_clk_unregister_pll(struct clk_hw *hw);
+
+__iomem void *mtk_clk_pll_get_base(struct clk_hw *hw,
+				   const struct mtk_pll_data *data);
+
 #endif /* __DRV_CLK_MTK_PLL_H */
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/4] dt-bindings: arm: mediatek: Add new bindings of MediaTek frequency hopping

[Johnson Wang]

Add the new binding documentation for MediaTek frequency hopping
and spread spectrum clocking control.

Co-developed-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
Signed-off-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
---
 .../bindings/arm/mediatek/mediatek,fhctl.yaml | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml

diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml
new file mode 100644
index 000000000000..c5d76410538b
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/arm/mediatek/mediatek,fhctl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek frequency hopping and spread spectrum clocking control
+
+maintainers:
+  - Edward-JW Yang <edward-jw.yang@mediatek.com>
+
+description: |
+  Frequency hopping control (FHCTL) is a piece of hardware that control
+  some PLLs to adopt "hopping" mechanism to adjust their frequency.
+  Spread spectrum clocking (SSC) is another function provided by this hardware.
+
+properties:
+  compatible:
+    const: mediatek,fhctl
+
+  reg:
+    maxItems: 1
+
+  mediatek,hopping-ssc-percents:
+    description: |
+      Determine the enablement of frequency hopping feature and the percentage
+      of spread spectrum clocking for PLLs.
+    $ref: /schemas/types.yaml#/definitions/uint32-matrix
+    items:
+      items:
+        - description: PLL id that is expected to enable frequency hopping.
+        - description: The percentage of spread spectrum clocking for one PLL.
+          minimum: 0
+          maximum: 8
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/mt8186-clk.h>
+    fhctl: fhctl@1000ce00 {
+        compatible = "mediatek,fhctl";
+        reg = <0x1000c000 0xe00>;
+        mediatek,hopping-ssc-percents = <CLK_APMIXED_MSDCPLL 3>;
+    };
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/4] clk: mediatek: Add new clock driver to handle FHCTL hardware

[Johnson Wang]

To implement frequency hopping and spread spectrum clocking
function, we introduce new clock type and APIs to handle
FHCTL hardware.

Co-developed-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
Signed-off-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
---
 drivers/clk/mediatek/Makefile    |   2 +-
 drivers/clk/mediatek/clk-fhctl.c | 258 +++++++++++++++++++++++++++++
 drivers/clk/mediatek/clk-fhctl.h |  27 +++
 drivers/clk/mediatek/clk-pllfh.c | 271 +++++++++++++++++++++++++++++++
 drivers/clk/mediatek/clk-pllfh.h |  81 +++++++++
 5 files changed, 638 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/mediatek/clk-fhctl.c
 create mode 100644 drivers/clk/mediatek/clk-fhctl.h
 create mode 100644 drivers/clk/mediatek/clk-pllfh.c
 create mode 100644 drivers/clk/mediatek/clk-pllfh.h

diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
index caf2ce93d666..0e674a55e51e 100644
--- a/drivers/clk/mediatek/Makefile
+++ b/drivers/clk/mediatek/Makefile
@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
-obj-$(CONFIG_COMMON_CLK_MEDIATEK) += clk-mtk.o clk-pll.o clk-gate.o clk-apmixed.o clk-cpumux.o reset.o clk-mux.o
+obj-$(CONFIG_COMMON_CLK_MEDIATEK) += clk-mtk.o clk-pll.o clk-gate.o clk-apmixed.o clk-cpumux.o reset.o clk-mux.o clk-fhctl.o clk-pllfh.o
 
 obj-$(CONFIG_COMMON_CLK_MT6765) += clk-mt6765.o
 obj-$(CONFIG_COMMON_CLK_MT6765_AUDIOSYS) += clk-mt6765-audio.o
diff --git a/drivers/clk/mediatek/clk-fhctl.c b/drivers/clk/mediatek/clk-fhctl.c
new file mode 100644
index 000000000000..69bd4ce2bc72
--- /dev/null
+++ b/drivers/clk/mediatek/clk-fhctl.c
@@ -0,0 +1,258 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2022 MediaTek Inc.
+ * Author: Edward-JW Yang <edward-jw.yang@mediatek.com>
+ */
+
+#include <linux/iopoll.h>
+
+#include "clk-mtk.h"
+#include "clk-pllfh.h"
+#include "clk-fhctl.h"
+
+#define PERCENT_TO_DDSLMT(dds, percent_m10) \
+	((((dds) * (percent_m10)) >> 5) / 100)
+
+static const struct fhctl_offset fhctl_offset = {
+	.offset_hp_en = 0x0,
+	.offset_clk_con = 0x8,
+	.offset_rst_con = 0xc,
+	.offset_slope0 = 0x10,
+	.offset_slope1 = 0x14,
+	.offset_cfg = 0x0,
+	.offset_updnlmt = 0x4,
+	.offset_dds = 0x8,
+	.offset_dvfs = 0xc,
+	.offset_mon = 0x10,
+};
+
+const struct fhctl_offset *fhctl_get_offset_table(void)
+{
+	return &fhctl_offset;
+}
+
+static void dump_hw(struct mtk_clk_pll *pll, struct fh_pll_regs *regs,
+		    const struct fh_pll_data *data)
+{
+	pr_info("hp_en<%x>,clk_con<%x>,slope0<%x>,slope1<%x>\n",
+		readl(regs->reg_hp_en), readl(regs->reg_clk_con),
+		readl(regs->reg_slope0), readl(regs->reg_slope1));
+	pr_info("cfg<%x>,lmt<%x>,dds<%x>,dvfs<%x>,mon<%x>\n",
+		readl(regs->reg_cfg), readl(regs->reg_updnlmt),
+		readl(regs->reg_dds), readl(regs->reg_dvfs),
+		readl(regs->reg_mon));
+	pr_info("pcw<%x>\n", readl(pll->pcw_addr));
+}
+
+static int fhctl_set_ssc_regs(struct mtk_clk_pll *pll, struct fh_pll_regs *regs,
+			      const struct fh_pll_data *data, u32 rate)
+{
+	u32 updnlmt_val, r;
+
+	writel((readl(regs->reg_cfg) & ~(data->frddsx_en)), regs->reg_cfg);
+	writel((readl(regs->reg_cfg) & ~(data->sfstrx_en)), regs->reg_cfg);
+	writel((readl(regs->reg_cfg) & ~(data->fhctlx_en)), regs->reg_cfg);
+
+	if (rate > 0) {
+		/* Set the relative parameter registers (dt/df/upbnd/downbnd) */
+		r = readl(regs->reg_cfg);
+		r &= ~(data->msk_frddsx_dys);
+		r |= (data->df_val << (ffs(data->msk_frddsx_dys) - 1));
+		writel(r, regs->reg_cfg);
+
+		r = readl(regs->reg_cfg);
+		r &= ~(data->msk_frddsx_dts);
+		r |= (data->dt_val << (ffs(data->msk_frddsx_dts) - 1));
+		writel(r, regs->reg_cfg);
+
+		writel((readl(pll->pcw_addr) & data->dds_mask) | data->tgl_org,
+			regs->reg_dds);
+
+		/* Calculate UPDNLMT */
+		updnlmt_val = PERCENT_TO_DDSLMT((readl(regs->reg_dds) &
+						 data->dds_mask), rate) <<
+						 data->updnlmt_shft;
+
+		writel(updnlmt_val, regs->reg_updnlmt);
+		writel(readl(regs->reg_hp_en) | BIT(data->fh_id),
+		       regs->reg_hp_en);
+		/* Enable SSC */
+		writel(readl(regs->reg_cfg) | data->frddsx_en, regs->reg_cfg);
+		/* Enable Hopping control */
+		writel(readl(regs->reg_cfg) | data->fhctlx_en, regs->reg_cfg);
+
+	} else {
+		/* Switch to APMIXEDSYS control */
+		writel(readl(regs->reg_hp_en) & ~BIT(data->fh_id),
+		       regs->reg_hp_en);
+		/* Wait for DDS to be stable */
+		udelay(30);
+	}
+
+	return 0;
+}
+
+static int hopping_hw_flow(struct mtk_clk_pll *pll, struct fh_pll_regs *regs,
+			   const struct fh_pll_data *data,
+			   struct fh_pll_state *state, unsigned int new_dds)
+{
+	u32 dds_mask = data->dds_mask;
+	u32 mon_dds = 0;
+	u32 con_pcw_tmp;
+	int ret;
+
+	if (state->ssc_rate)
+		fhctl_set_ssc_regs(pll, regs, data, 0);
+
+	writel((readl(pll->pcw_addr) & dds_mask) | data->tgl_org,
+		regs->reg_dds);
+
+	writel(readl(regs->reg_cfg) | data->sfstrx_en, regs->reg_cfg);
+	writel(readl(regs->reg_cfg) | data->fhctlx_en, regs->reg_cfg);
+	writel(data->slope0_value, regs->reg_slope0);
+	writel(data->slope1_value, regs->reg_slope1);
+
+	writel(readl(regs->reg_hp_en) | BIT(data->fh_id), regs->reg_hp_en);
+	writel((new_dds) | (data->dvfs_tri), regs->reg_dvfs);
+
+	/* Wait 1000 us until DDS stable */
+	ret = readl_poll_timeout_atomic(regs->reg_mon, mon_dds,
+				       (mon_dds & dds_mask) == new_dds,
+					10, 1000);
+	if (ret) {
+		pr_warn("%s: FHCTL hopping timeout\n", pll->data->name);
+		dump_hw(pll, regs, data);
+	}
+
+	con_pcw_tmp = readl(pll->pcw_addr) & (~dds_mask);
+	con_pcw_tmp = (con_pcw_tmp | (readl(regs->reg_mon) & dds_mask) |
+		       data->pcwchg);
+
+	writel(con_pcw_tmp, pll->pcw_addr);
+	writel(readl(regs->reg_hp_en) & ~BIT(data->fh_id), regs->reg_hp_en);
+
+	if (state->ssc_rate)
+		fhctl_set_ssc_regs(pll, regs, data, state->ssc_rate);
+
+	return ret;
+}
+
+static unsigned int __get_postdiv(struct mtk_clk_pll *pll,
+				  struct fh_pll_regs *regs,
+				  const struct fh_pll_data *fh_data)
+{
+	unsigned int regval;
+
+	regval = readl(pll->pd_addr) >> pll->data->pd_shift;
+	regval &= POSTDIV_MASK;
+
+	return (1 << regval);
+}
+
+static void __set_postdiv(struct mtk_clk_pll *pll, struct fh_pll_regs *regs,
+			  const struct fh_pll_data *data, int postdiv)
+{
+	unsigned int regval;
+
+	regval = readl(pll->pd_addr);
+	regval &= ~(POSTDIV_MASK << pll->data->pd_shift);
+	regval |= (ffs(postdiv) - 1) << pll->data->pd_shift;
+	writel(regval, pll->pd_addr);
+}
+
+static int fhctl_hopping(struct mtk_fh *fh, unsigned int new_dds, int postdiv)
+{
+	const struct fh_pll_data *data = &fh->pllfh_data->data;
+	struct fh_pll_state *state = &fh->pllfh_data->state;
+	struct fh_pll_regs *regs = &fh->regs;
+	struct mtk_clk_pll *pll = &fh->clk_pll;
+	spinlock_t *lock = fh->lock;
+	unsigned int pll_postdiv;
+	unsigned long flags = 0;
+	int ret;
+
+	if (postdiv > 0) {
+		pll_postdiv = __get_postdiv(pll, regs, data);
+
+		if (postdiv > pll_postdiv)
+			__set_postdiv(pll, regs, data, postdiv);
+	}
+
+	spin_lock_irqsave(lock, flags);
+
+	ret = hopping_hw_flow(pll, regs, data, state, new_dds);
+
+	spin_unlock_irqrestore(lock, flags);
+
+	if (postdiv > 0) {
+		if (postdiv < pll_postdiv)
+			__set_postdiv(pll, regs, data, postdiv);
+	}
+
+	return ret;
+}
+
+static int __fhctl_ssc_enable(struct mtk_fh *fh, u32 rate)
+{
+	const struct fh_pll_data *data = &fh->pllfh_data->data;
+	struct fh_pll_state *state = &fh->pllfh_data->state;
+	struct fh_pll_regs *regs = &fh->regs;
+	struct mtk_clk_pll *pll = &fh->clk_pll;
+	spinlock_t *lock = fh->lock;
+	unsigned long flags = 0;
+
+	spin_lock_irqsave(lock, flags);
+
+	fhctl_set_ssc_regs(pll, regs, data, rate);
+	state->ssc_rate = rate;
+
+	spin_unlock_irqrestore(lock, flags);
+
+	return 0;
+}
+
+static int fhctl_ssc_enable(struct mtk_fh *fh, u32 rate)
+{
+	return __fhctl_ssc_enable(fh, rate);
+}
+
+static int fhctl_ssc_disable(struct mtk_fh *fh)
+{
+	return __fhctl_ssc_enable(fh, 0);
+}
+
+static const struct fh_operation fhctl_ops = {
+	.hopping = fhctl_hopping,
+	.ssc_enable = fhctl_ssc_enable,
+	.ssc_disable = fhctl_ssc_disable,
+};
+
+const struct fh_operation *fhctl_get_ops(void)
+{
+	return &fhctl_ops;
+}
+
+void fhctl_hw_init(struct mtk_fh *fh)
+{
+	const struct fh_pll_data data = fh->pllfh_data->data;
+	struct fh_pll_state state = fh->pllfh_data->state;
+	struct fh_pll_regs regs = fh->regs;
+	u32 val;
+
+	/* initial hw register */
+	val = readl(regs.reg_clk_con) | BIT(data.fh_id);
+	writel(val, regs.reg_clk_con);
+
+	val = readl(regs.reg_rst_con) & ~BIT(data.fh_id);
+	writel(val, regs.reg_rst_con);
+	val = readl(regs.reg_rst_con) | BIT(data.fh_id);
+	writel(val, regs.reg_rst_con);
+
+	writel(0x0, regs.reg_cfg);
+	writel(0x0, regs.reg_updnlmt);
+	writel(0x0, regs.reg_dds);
+
+	/* enable ssc if needed */
+	if (state.ssc_rate)
+		fh->ops->ssc_enable(fh, state.ssc_rate);
+}
diff --git a/drivers/clk/mediatek/clk-fhctl.h b/drivers/clk/mediatek/clk-fhctl.h
new file mode 100644
index 000000000000..3cd4921c39e9
--- /dev/null
+++ b/drivers/clk/mediatek/clk-fhctl.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2022 MediaTek Inc.
+ * Author: Edward-JW Yang <edward-jw.yang@mediatek.com>
+ */
+
+#ifndef __CLK_FHCTL_H
+#define __CLK_FHCTL_H
+
+
+struct fhctl_offset {
+	u32 offset_hp_en;
+	u32 offset_clk_con;
+	u32 offset_rst_con;
+	u32 offset_slope0;
+	u32 offset_slope1;
+	u32 offset_cfg;
+	u32 offset_updnlmt;
+	u32 offset_dds;
+	u32 offset_dvfs;
+	u32 offset_mon;
+};
+const struct fhctl_offset *fhctl_get_offset_table(void);
+const struct fh_operation *fhctl_get_ops(void);
+void fhctl_hw_init(struct mtk_fh *fh);
+
+#endif
diff --git a/drivers/clk/mediatek/clk-pllfh.c b/drivers/clk/mediatek/clk-pllfh.c
new file mode 100644
index 000000000000..71b35323b526
--- /dev/null
+++ b/drivers/clk/mediatek/clk-pllfh.c
@@ -0,0 +1,271 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2022 MediaTek Inc.
+ * Author: Edward-JW Yang <edward-jw.yang@mediatek.com>
+ */
+
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/clkdev.h>
+#include <linux/delay.h>
+
+#include "clk-mtk.h"
+#include "clk-pllfh.h"
+#include "clk-fhctl.h"
+
+static DEFINE_SPINLOCK(pllfh_lock);
+
+inline struct mtk_fh *to_mtk_fh(struct clk_hw *hw)
+{
+	struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
+
+	return container_of(pll, struct mtk_fh, clk_pll);
+}
+
+static int mtk_fhctl_set_rate(struct clk_hw *hw, unsigned long rate,
+			      unsigned long parent_rate)
+{
+	struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
+	struct mtk_fh *fh = to_mtk_fh(hw);
+	u32 pcw = 0;
+	u32 postdiv;
+
+	mtk_pll_calc_values(pll, &pcw, &postdiv, rate, parent_rate);
+	fh->ops->hopping(fh, pcw, postdiv);
+
+	return 0;
+}
+
+static const struct clk_ops mtk_pllfh_ops = {
+	.is_prepared	= mtk_pll_is_prepared,
+	.prepare	= mtk_pll_prepare,
+	.unprepare	= mtk_pll_unprepare,
+	.recalc_rate	= mtk_pll_recalc_rate,
+	.round_rate	= mtk_pll_round_rate,
+	.set_rate	= mtk_fhctl_set_rate,
+};
+
+static struct mtk_pllfh_data *get_pllfh_by_id(struct mtk_pllfh_data *pllfhs,
+					      int num_fhs, int pll_id)
+{
+	int i;
+
+	for (i = 0; i < num_fhs; i++)
+		if (pllfhs[i].data.pll_id == pll_id)
+			return &pllfhs[i];
+
+	return NULL;
+}
+
+void fhctl_parse_dt(struct mtk_pllfh_data *pllfhs, int num_fhs)
+{
+	void __iomem *base;
+	struct device_node *node;
+	const u8 *compatible_node = "mediatek,fhctl";
+	u32 tmp = 0;
+	u32 num_cells, pll_id, ssc_rate;
+	int offset, i;
+
+	node = of_find_compatible_node(NULL, NULL, compatible_node);
+	if (!node) {
+		pr_err("cannot find \"%s\"\n", compatible_node);
+		return;
+	}
+
+	base = of_iomap(node, 0);
+	if (!base) {
+		pr_err("%s(): ioremap failed\n", __func__);
+		return;
+	}
+
+	if (!of_get_property(node, "mediatek,hopping-ssc-percents", &tmp)) {
+		pr_err("%s(): failed get property\n", __func__);
+		return;
+	}
+
+	num_cells = tmp / (sizeof(u32) * 2);
+
+	for (i = 0; i < num_cells; i++) {
+		struct mtk_pllfh_data *pllfh;
+
+		offset = i * 2;
+		of_property_read_u32_index(node,
+					   "mediatek,hopping-ssc-percents",
+					   offset, &pll_id);
+		of_property_read_u32_index(node,
+					   "mediatek,hopping-ssc-percents",
+					   offset + 1, &ssc_rate);
+		pllfh = get_pllfh_by_id(pllfhs, num_fhs, pll_id);
+		if (!pllfh)
+			continue;
+
+		pllfh->state.fh_enable = 1;
+		pllfh->state.ssc_rate = ssc_rate;
+		pllfh->state.base = base;
+	}
+}
+
+static void pllfh_init(struct mtk_fh *fh, struct mtk_pllfh_data *pllfh_data)
+{
+	struct fh_pll_regs *regs = &fh->regs;
+	const struct fhctl_offset *offset;
+	void __iomem *base = pllfh_data->state.base;
+	void __iomem *fhx_base = base + pllfh_data->data.fhx_offset;
+
+	offset = fhctl_get_offset_table();
+
+	regs->reg_hp_en = base + offset->offset_hp_en;
+	regs->reg_clk_con = base + offset->offset_clk_con;
+	regs->reg_rst_con = base + offset->offset_rst_con;
+	regs->reg_slope0 = base + offset->offset_slope0;
+	regs->reg_slope1 = base + offset->offset_slope1;
+
+	regs->reg_cfg = fhx_base + offset->offset_cfg;
+	regs->reg_updnlmt = fhx_base + offset->offset_updnlmt;
+	regs->reg_dds = fhx_base + offset->offset_dds;
+	regs->reg_dvfs = fhx_base + offset->offset_dvfs;
+	regs->reg_mon = fhx_base + offset->offset_mon;
+
+	fh->pllfh_data = pllfh_data;
+	fh->lock = &pllfh_lock;
+
+	fh->ops = fhctl_get_ops();
+}
+
+static bool fhctl_is_supported_and_enabled(const struct mtk_pllfh_data *pllfh)
+{
+	return pllfh && (pllfh->state.fh_enable == 1);
+}
+
+static struct clk_hw *
+mtk_clk_register_pllfh(const struct mtk_pll_data *pll_data,
+		       struct mtk_pllfh_data *pllfh_data, void __iomem *base)
+{
+	struct clk_hw *hw;
+	struct mtk_fh *fh;
+
+	fh = kzalloc(sizeof(*fh), GFP_KERNEL);
+	if (!fh)
+		return ERR_PTR(-ENOMEM);
+
+	pllfh_init(fh, pllfh_data);
+
+	hw = mtk_clk_register_pll_ops(&fh->clk_pll, pll_data, base,
+				      &mtk_pllfh_ops);
+
+	if (IS_ERR(hw))
+		kfree(fh);
+	else
+		fhctl_hw_init(fh);
+
+	return hw;
+}
+
+static void mtk_clk_unregister_pllfh(struct clk_hw *hw)
+{
+	struct mtk_fh *fh;
+
+	if (!hw)
+		return;
+
+	fh = to_mtk_fh(hw);
+
+	clk_hw_unregister(hw);
+	kfree(fh);
+}
+
+int mtk_clk_register_pllfhs(struct device_node *node,
+			    const struct mtk_pll_data *plls, int num_plls,
+			    struct mtk_pllfh_data *pllfhs, int num_fhs,
+			    struct clk_hw_onecell_data *clk_data)
+{
+	void __iomem *base;
+	int i;
+	struct clk_hw *hw;
+
+	base = of_iomap(node, 0);
+	if (!base) {
+		pr_err("%s(): ioremap failed\n", __func__);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < num_plls; i++) {
+		const struct mtk_pll_data *pll = &plls[i];
+		struct mtk_pllfh_data *pllfh;
+
+		pllfh = get_pllfh_by_id(pllfhs, num_fhs, pll->id);
+
+		if (fhctl_is_supported_and_enabled(pllfh))
+			hw = mtk_clk_register_pllfh(pll, pllfh, base);
+		else
+			hw = mtk_clk_register_pll(pll, base);
+
+		if (IS_ERR(hw)) {
+			pr_err("Failed to register clk %s: %pe\n", pll->name,
+			       hw);
+			goto err;
+		}
+
+		clk_data->hws[pll->id] = hw;
+	}
+
+	return 0;
+
+err:
+	while (--i >= 0) {
+		const struct mtk_pll_data *pll = &plls[i];
+		struct mtk_pllfh_data *pllfh;
+
+		pllfh = get_pllfh_by_id(pllfhs, num_fhs, pll->id);
+
+		if (fhctl_is_supported_and_enabled(pllfh))
+			mtk_clk_unregister_pllfh(clk_data->hws[pll->id]);
+		else
+			mtk_clk_unregister_pll(clk_data->hws[pll->id]);
+
+		clk_data->hws[pll->id] = ERR_PTR(-ENOENT);
+	}
+
+	iounmap(base);
+
+	return PTR_ERR(hw);
+}
+
+void mtk_clk_unregister_pllfhs(const struct mtk_pll_data *plls, int num_plls,
+			       struct mtk_pllfh_data *pllfhs, int num_fhs,
+			       struct clk_hw_onecell_data *clk_data)
+{
+	void __iomem *base = NULL, *fhctl_base = NULL;
+	int i;
+
+	if (!clk_data)
+		return;
+
+	for (i = num_plls; i > 0; i--) {
+		const struct mtk_pll_data *pll = &plls[i - 1];
+		struct mtk_pllfh_data *pllfh;
+
+		if (IS_ERR_OR_NULL(clk_data->hws[pll->id]))
+			continue;
+
+		pllfh = get_pllfh_by_id(pllfhs, num_fhs, pll->id);
+
+		if (fhctl_is_supported_and_enabled(pllfh)) {
+			fhctl_base = pllfh->state.base;
+			mtk_clk_unregister_pllfh(clk_data->hws[pll->id]);
+		} else {
+			base = mtk_clk_pll_get_base(clk_data->hws[pll->id],
+						    pll);
+			mtk_clk_unregister_pll(clk_data->hws[pll->id]);
+		}
+
+		clk_data->hws[pll->id] = ERR_PTR(-ENOENT);
+	}
+
+	if (fhctl_base)
+		iounmap(fhctl_base);
+
+	iounmap(base);
+}
diff --git a/drivers/clk/mediatek/clk-pllfh.h b/drivers/clk/mediatek/clk-pllfh.h
new file mode 100644
index 000000000000..d9f7c8527548
--- /dev/null
+++ b/drivers/clk/mediatek/clk-pllfh.h
@@ -0,0 +1,81 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2022 MediaTek Inc.
+ * Author: Edward-JW Yang <edward-jw.yang@mediatek.com>
+ */
+
+#ifndef __DRV_CLKFH_H
+#define __DRV_CLKFH_H
+
+#include "clk-pll.h"
+
+struct fh_pll_state {
+	u32 fh_enable;
+	u32 ssc_rate;
+	void __iomem *base;
+};
+
+struct fh_pll_data {
+	int pll_id;
+	int fh_id;
+	u32 fhx_offset;
+	u32 dds_mask;
+	u32 slope0_value;
+	u32 slope1_value;
+	u32 sfstrx_en;
+	u32 frddsx_en;
+	u32 fhctlx_en;
+	u32 tgl_org;
+	u32 dvfs_tri;
+	u32 pcwchg;
+	u32 dt_val;
+	u32 df_val;
+	u32 updnlmt_shft;
+	u32 msk_frddsx_dys;
+	u32 msk_frddsx_dts;
+};
+
+struct mtk_pllfh_data {
+	struct fh_pll_state state;
+	const struct fh_pll_data data;
+};
+
+struct fh_pll_regs {
+	void __iomem *reg_hp_en;
+	void __iomem *reg_clk_con;
+	void __iomem *reg_rst_con;
+	void __iomem *reg_slope0;
+	void __iomem *reg_slope1;
+	void __iomem *reg_cfg;
+	void __iomem *reg_updnlmt;
+	void __iomem *reg_dds;
+	void __iomem *reg_dvfs;
+	void __iomem *reg_mon;
+};
+
+struct mtk_fh {
+	struct mtk_clk_pll clk_pll;
+	struct fh_pll_regs regs;
+	struct mtk_pllfh_data *pllfh_data;
+	const struct fh_operation *ops;
+	spinlock_t *lock;
+};
+
+struct fh_operation {
+	int (*hopping)(struct mtk_fh *fh, unsigned int new_dds, int postdiv);
+	int (*ssc_enable)(struct mtk_fh *fh, u32 rate);
+	int (*ssc_disable)(struct mtk_fh *fh);
+};
+
+int mtk_clk_register_pllfhs(struct device_node *node,
+			    const struct mtk_pll_data *plls, int num_plls,
+			    struct mtk_pllfh_data *pllfhs, int num_pllfhs,
+			    struct clk_hw_onecell_data *clk_data);
+
+void mtk_clk_unregister_pllfhs(const struct mtk_pll_data *plls, int num_plls,
+			       struct mtk_pllfh_data *pllfhs, int num_fhs,
+			       struct clk_hw_onecell_data *clk_data);
+
+void fhctl_parse_dt(struct mtk_pllfh_data *pllfhs, int num_pllfhs);
+
+#endif /* __DRV_CLKFH_H */
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/4] clk: mediatek: Change PLL register API for MT8186

[Johnson Wang]

Use mtk_clk_register_pllfhs() to enhance frequency hopping and
spread spectrum clocking control for MT8186.

Co-developed-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
Signed-off-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
---
 drivers/clk/mediatek/clk-mt8186-apmixedsys.c | 65 +++++++++++++++++++-
 1 file changed, 62 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/mediatek/clk-mt8186-apmixedsys.c b/drivers/clk/mediatek/clk-mt8186-apmixedsys.c
index e692a2a67ce1..f4406d6ad4ea 100644
--- a/drivers/clk/mediatek/clk-mt8186-apmixedsys.c
+++ b/drivers/clk/mediatek/clk-mt8186-apmixedsys.c
@@ -9,6 +9,7 @@
 
 #include "clk-mtk.h"
 #include "clk-pll.h"
+#include "clk-pllfh.h"
 
 #define MT8186_PLL_FMAX		(3800UL * MHZ)
 #define MT8186_PLL_FMIN		(1500UL * MHZ)
@@ -76,6 +77,59 @@ static const struct mtk_pll_data plls[] = {
 	    0, 0, 32, 0x034C, 24, 0x0044, 0x000C, 5, 0x0350),
 };
 
+enum fh_pll_id {
+	FH_ARMPLL_LL,
+	FH_ARMPLL_BL,
+	FH_CCIPLL,
+	FH_MAINPLL,
+	FH_MMPLL,
+	FH_TVDPLL,
+	FH_RESERVE6,
+	FH_ADSPPLL,
+	FH_MFGPLL,
+	FH_NNAPLL,
+	FH_NNA2PLL,
+	FH_MSDCPLL,
+	FH_RESERVE12,
+	FH_NR_FH,
+};
+
+#define FH(_pllid, _fhid, _offset) {					\
+		.data = {						\
+			.pll_id = _pllid,				\
+			.fh_id = _fhid,					\
+			.fhx_offset = _offset,				\
+			.dds_mask = GENMASK(21, 0),			\
+			.slope0_value = 0x6003c97,			\
+			.slope1_value = 0x6003c97,			\
+			.sfstrx_en = BIT(2),				\
+			.frddsx_en = BIT(1),				\
+			.fhctlx_en = BIT(0),				\
+			.tgl_org = BIT(31),				\
+			.dvfs_tri = BIT(31),				\
+			.pcwchg = BIT(31),				\
+			.dt_val = 0x0,					\
+			.df_val = 0x9,					\
+			.updnlmt_shft = 16,				\
+			.msk_frddsx_dys = GENMASK(23, 20),		\
+			.msk_frddsx_dts = GENMASK(19, 16),		\
+		},							\
+	}
+
+static struct mtk_pllfh_data pllfhs[] = {
+	FH(CLK_APMIXED_ARMPLL_LL, FH_ARMPLL_LL, 0x003C),
+	FH(CLK_APMIXED_ARMPLL_BL, FH_ARMPLL_BL, 0x0050),
+	FH(CLK_APMIXED_CCIPLL, FH_CCIPLL, 0x0064),
+	FH(CLK_APMIXED_MAINPLL, FH_MAINPLL, 0x0078),
+	FH(CLK_APMIXED_MMPLL, FH_MMPLL, 0x008C),
+	FH(CLK_APMIXED_TVDPLL, FH_TVDPLL, 0x00A0),
+	FH(CLK_APMIXED_ADSPPLL, FH_ADSPPLL, 0x00C8),
+	FH(CLK_APMIXED_MFGPLL, FH_MFGPLL, 0x00DC),
+	FH(CLK_APMIXED_NNAPLL, FH_NNAPLL, 0x00F0),
+	FH(CLK_APMIXED_NNA2PLL, FH_NNA2PLL, 0x0104),
+	FH(CLK_APMIXED_MSDCPLL, FH_MSDCPLL, 0x0118),
+};
+
 static const struct of_device_id of_match_clk_mt8186_apmixed[] = {
 	{ .compatible = "mediatek,mt8186-apmixedsys", },
 	{}
@@ -91,7 +145,10 @@ static int clk_mt8186_apmixed_probe(struct platform_device *pdev)
 	if (!clk_data)
 		return -ENOMEM;
 
-	r = mtk_clk_register_plls(node, plls, ARRAY_SIZE(plls), clk_data);
+	fhctl_parse_dt(pllfhs, ARRAY_SIZE(pllfhs));
+
+	r = mtk_clk_register_pllfhs(node, plls, ARRAY_SIZE(plls),
+				    pllfhs, ARRAY_SIZE(pllfhs), clk_data);
 	if (r)
 		goto free_apmixed_data;
 
@@ -104,7 +161,8 @@ static int clk_mt8186_apmixed_probe(struct platform_device *pdev)
 	return r;
 
 unregister_plls:
-	mtk_clk_unregister_plls(plls, ARRAY_SIZE(plls), clk_data);
+	mtk_clk_unregister_pllfhs(plls, ARRAY_SIZE(plls), pllfhs,
+				  ARRAY_SIZE(pllfhs), clk_data);
 free_apmixed_data:
 	mtk_free_clk_data(clk_data);
 	return r;
@@ -116,7 +174,8 @@ static int clk_mt8186_apmixed_remove(struct platform_device *pdev)
 	struct clk_hw_onecell_data *clk_data = platform_get_drvdata(pdev);
 
 	of_clk_del_provider(node);
-	mtk_clk_unregister_plls(plls, ARRAY_SIZE(plls), clk_data);
+	mtk_clk_unregister_pllfhs(plls, ARRAY_SIZE(plls), pllfhs,
+				  ARRAY_SIZE(pllfhs), clk_data);
 	mtk_free_clk_data(clk_data);
 
 	return 0;
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] dt-bindings: arm: mediatek: Add new bindings of MediaTek frequency hopping

[Krzysztof Kozlowski]

On 31/08/2022 15:48, Johnson Wang wrote:
> Add the new binding documentation for MediaTek frequency hopping
> and spread spectrum clocking control.
> 
> Co-developed-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
> Signed-off-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
> Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
> ---
>  .../bindings/arm/mediatek/mediatek,fhctl.yaml | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml
> new file mode 100644
> index 000000000000..c5d76410538b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/mediatek/mediatek,fhctl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek frequency hopping and spread spectrum clocking control
> +
> +maintainers:
> +  - Edward-JW Yang <edward-jw.yang@mediatek.com>
> +
> +description: |
> +  Frequency hopping control (FHCTL) is a piece of hardware that control
> +  some PLLs to adopt "hopping" mechanism to adjust their frequency.
> +  Spread spectrum clocking (SSC) is another function provided by this hardware.
> +
> +properties:
> +  compatible:
> +    const: mediatek,fhctl

You need SoC/device specific compatibles. Preferably only SoC specific,
without generic fallback, unless you can guarantee (while representing
MediaTek), that generic fallback will cover all of their SoCs?

> +
> +  reg:
> +    maxItems: 1
> +
> +  mediatek,hopping-ssc-percents:
> +    description: |
> +      Determine the enablement of frequency hopping feature and the percentage
> +      of spread spectrum clocking for PLLs.
> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> +    items:
> +      items:
> +        - description: PLL id that is expected to enable frequency hopping.

So the clocks are indices from some specific, yet unnamed
clock-controller? This feels hacky. You should rather take here clock
phandles (1) or integrate it into specific clock controller (2). The
reason is that either your device does something on top of existing
clocks (option 1, thus it takes clock as inputs) or it modifies existing
clocks (option 2, thus it is integral part of clock-controller).


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/4] Introduce MediaTek frequency hopping driver

[Krzysztof Kozlowski]

On 31/08/2022 15:48, Johnson Wang wrote:
> Introduce MediaTek frequency hopping and spread spectrum clocking control
> for MT8186.

With one line introduction, you do not help us to understand this. :(

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] dt-bindings: arm: mediatek: Add new bindings of MediaTek frequency hopping

[AngeloGioacchino Del Regno]

Il 31/08/22 15:19, Krzysztof Kozlowski ha scritto:
> On 31/08/2022 15:48, Johnson Wang wrote:
>> Add the new binding documentation for MediaTek frequency hopping
>> and spread spectrum clocking control.
>>
>> Co-developed-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
>> Signed-off-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
>> Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
>> ---
>>   .../bindings/arm/mediatek/mediatek,fhctl.yaml | 49 +++++++++++++++++++
>>   1 file changed, 49 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml
>> new file mode 100644
>> index 000000000000..c5d76410538b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml
>> @@ -0,0 +1,49 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/arm/mediatek/mediatek,fhctl.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: MediaTek frequency hopping and spread spectrum clocking control
>> +
>> +maintainers:
>> +  - Edward-JW Yang <edward-jw.yang@mediatek.com>
>> +
>> +description: |
>> +  Frequency hopping control (FHCTL) is a piece of hardware that control
>> +  some PLLs to adopt "hopping" mechanism to adjust their frequency.
>> +  Spread spectrum clocking (SSC) is another function provided by this hardware.
>> +
>> +properties:
>> +  compatible:
>> +    const: mediatek,fhctl
> 
> You need SoC/device specific compatibles. Preferably only SoC specific,
> without generic fallback, unless you can guarantee (while representing
> MediaTek), that generic fallback will cover all of their SoCs?
> 
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  mediatek,hopping-ssc-percents:
>> +    description: |
>> +      Determine the enablement of frequency hopping feature and the percentage
>> +      of spread spectrum clocking for PLLs.
>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
>> +    items:
>> +      items:
>> +        - description: PLL id that is expected to enable frequency hopping.
> 
> So the clocks are indices from some specific, yet unnamed
> clock-controller? This feels hacky. You should rather take here clock
> phandles (1) or integrate it into specific clock controller (2). The
> reason is that either your device does something on top of existing
> clocks (option 1, thus it takes clock as inputs) or it modifies existing
> clocks (option 2, thus it is integral part of clock-controller).
> 

FHCTL is a MCU that handles (some, or all, depending on what's supported on the
SoC and what's needed by the board) PLL frequency setting, doing it in steps and
avoiding overshooting and other issues.

We had a pretty big conversation about this a while ago and the indices instead
of phandles is actually my fault, that happened because I initially proposed your
option 2 but then for a number of reasons we went with this kind of solution.

I know it's going to be a long read, but the entire conversation is on the list [1]

Cheers,
Angelo

[1]: 
https://patchwork.kernel.org/project/linux-mediatek/patch/20220612135414.3003-3-johnson.wang@mediatek.com/

> 
> Best regards,
> Krzysztof



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/4] clk: mediatek: Add new clock driver to handle FHCTL hardware

[AngeloGioacchino Del Regno]

Il 31/08/22 14:48, Johnson Wang ha scritto:
> To implement frequency hopping and spread spectrum clocking
> function, we introduce new clock type and APIs to handle
> FHCTL hardware.
> 
> Co-developed-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
> Signed-off-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
> Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
> ---
>   drivers/clk/mediatek/Makefile    |   2 +-
>   drivers/clk/mediatek/clk-fhctl.c | 258 +++++++++++++++++++++++++++++
>   drivers/clk/mediatek/clk-fhctl.h |  27 +++
>   drivers/clk/mediatek/clk-pllfh.c | 271 +++++++++++++++++++++++++++++++
>   drivers/clk/mediatek/clk-pllfh.h |  81 +++++++++
>   5 files changed, 638 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/clk/mediatek/clk-fhctl.c
>   create mode 100644 drivers/clk/mediatek/clk-fhctl.h
>   create mode 100644 drivers/clk/mediatek/clk-pllfh.c
>   create mode 100644 drivers/clk/mediatek/clk-pllfh.h
> 
> diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
> index caf2ce93d666..0e674a55e51e 100644
> --- a/drivers/clk/mediatek/Makefile
> +++ b/drivers/clk/mediatek/Makefile
> @@ -1,5 +1,5 @@
>   # SPDX-License-Identifier: GPL-2.0
> -obj-$(CONFIG_COMMON_CLK_MEDIATEK) += clk-mtk.o clk-pll.o clk-gate.o clk-apmixed.o clk-cpumux.o reset.o clk-mux.o
> +obj-$(CONFIG_COMMON_CLK_MEDIATEK) += clk-mtk.o clk-pll.o clk-gate.o clk-apmixed.o clk-cpumux.o reset.o clk-mux.o clk-fhctl.o clk-pllfh.o
>   
>   obj-$(CONFIG_COMMON_CLK_MT6765) += clk-mt6765.o
>   obj-$(CONFIG_COMMON_CLK_MT6765_AUDIOSYS) += clk-mt6765-audio.o
> diff --git a/drivers/clk/mediatek/clk-fhctl.c b/drivers/clk/mediatek/clk-fhctl.c
> new file mode 100644
> index 000000000000..69bd4ce2bc72
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-fhctl.c
> @@ -0,0 +1,258 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022 MediaTek Inc.
> + * Author: Edward-JW Yang <edward-jw.yang@mediatek.com>
> + */
> +

#include <linux/io.h>

Please don't rely on implicit header inclusion.

> +#include <linux/iopoll.h>
> +
> +#include "clk-mtk.h"
> +#include "clk-pllfh.h"
> +#include "clk-fhctl.h"
> +
> +#define PERCENT_TO_DDSLMT(dds, percent_m10) \
> +	((((dds) * (percent_m10)) >> 5) / 100)
> +
> +static const struct fhctl_offset fhctl_offset = {
> +	.offset_hp_en = 0x0,
> +	.offset_clk_con = 0x8,
> +	.offset_rst_con = 0xc,
> +	.offset_slope0 = 0x10,
> +	.offset_slope1 = 0x14,
> +	.offset_cfg = 0x0,
> +	.offset_updnlmt = 0x4,
> +	.offset_dds = 0x8,
> +	.offset_dvfs = 0xc,
> +	.offset_mon = 0x10,
> +};
> +
> +const struct fhctl_offset *fhctl_get_offset_table(void)
> +{
> +	return &fhctl_offset;
> +}
> +
> +static void dump_hw(struct mtk_clk_pll *pll, struct fh_pll_regs *regs,
> +		    const struct fh_pll_data *data)
> +{
> +	pr_info("hp_en<%x>,clk_con<%x>,slope0<%x>,slope1<%x>\n",
> +		readl(regs->reg_hp_en), readl(regs->reg_clk_con),
> +		readl(regs->reg_slope0), readl(regs->reg_slope1));
> +	pr_info("cfg<%x>,lmt<%x>,dds<%x>,dvfs<%x>,mon<%x>\n",
> +		readl(regs->reg_cfg), readl(regs->reg_updnlmt),
> +		readl(regs->reg_dds), readl(regs->reg_dvfs),
> +		readl(regs->reg_mon));
> +	pr_info("pcw<%x>\n", readl(pll->pcw_addr));
> +}
> +
> +static int fhctl_set_ssc_regs(struct mtk_clk_pll *pll, struct fh_pll_regs *regs,
> +			      const struct fh_pll_data *data, u32 rate)
> +{
> +	u32 updnlmt_val, r;
> +
> +	writel((readl(regs->reg_cfg) & ~(data->frddsx_en)), regs->reg_cfg);
> +	writel((readl(regs->reg_cfg) & ~(data->sfstrx_en)), regs->reg_cfg);
> +	writel((readl(regs->reg_cfg) & ~(data->fhctlx_en)), regs->reg_cfg);
> +
> +	if (rate > 0) {
> +		/* Set the relative parameter registers (dt/df/upbnd/downbnd) */
> +		r = readl(regs->reg_cfg);
> +		r &= ~(data->msk_frddsx_dys);
> +		r |= (data->df_val << (ffs(data->msk_frddsx_dys) - 1));
> +		writel(r, regs->reg_cfg);
> +
> +		r = readl(regs->reg_cfg);
> +		r &= ~(data->msk_frddsx_dts);
> +		r |= (data->dt_val << (ffs(data->msk_frddsx_dts) - 1));
> +		writel(r, regs->reg_cfg);
> +
> +		writel((readl(pll->pcw_addr) & data->dds_mask) | data->tgl_org,
> +			regs->reg_dds);
> +
> +		/* Calculate UPDNLMT */
> +		updnlmt_val = PERCENT_TO_DDSLMT((readl(regs->reg_dds) &
> +						 data->dds_mask), rate) <<
> +						 data->updnlmt_shft;
> +
> +		writel(updnlmt_val, regs->reg_updnlmt);
> +		writel(readl(regs->reg_hp_en) | BIT(data->fh_id),
> +		       regs->reg_hp_en);
> +		/* Enable SSC */
> +		writel(readl(regs->reg_cfg) | data->frddsx_en, regs->reg_cfg);
> +		/* Enable Hopping control */
> +		writel(readl(regs->reg_cfg) | data->fhctlx_en, regs->reg_cfg);
> +
> +	} else {
> +		/* Switch to APMIXEDSYS control */
> +		writel(readl(regs->reg_hp_en) & ~BIT(data->fh_id),
> +		       regs->reg_hp_en);
> +		/* Wait for DDS to be stable */
> +		udelay(30);
> +	}
> +
> +	return 0;
> +}
> +
> +static int hopping_hw_flow(struct mtk_clk_pll *pll, struct fh_pll_regs *regs,
> +			   const struct fh_pll_data *data,
> +			   struct fh_pll_state *state, unsigned int new_dds)
> +{
> +	u32 dds_mask = data->dds_mask;
> +	u32 mon_dds = 0;
> +	u32 con_pcw_tmp;
> +	int ret;
> +
> +	if (state->ssc_rate)
> +		fhctl_set_ssc_regs(pll, regs, data, 0);
> +
> +	writel((readl(pll->pcw_addr) & dds_mask) | data->tgl_org,
> +		regs->reg_dds);
> +
> +	writel(readl(regs->reg_cfg) | data->sfstrx_en, regs->reg_cfg);
> +	writel(readl(regs->reg_cfg) | data->fhctlx_en, regs->reg_cfg);
> +	writel(data->slope0_value, regs->reg_slope0);
> +	writel(data->slope1_value, regs->reg_slope1);
> +
> +	writel(readl(regs->reg_hp_en) | BIT(data->fh_id), regs->reg_hp_en);
> +	writel((new_dds) | (data->dvfs_tri), regs->reg_dvfs);
> +
> +	/* Wait 1000 us until DDS stable */
> +	ret = readl_poll_timeout_atomic(regs->reg_mon, mon_dds,
> +				       (mon_dds & dds_mask) == new_dds,
> +					10, 1000);
> +	if (ret) {
> +		pr_warn("%s: FHCTL hopping timeout\n", pll->data->name);
> +		dump_hw(pll, regs, data);
> +	}
> +
> +	con_pcw_tmp = readl(pll->pcw_addr) & (~dds_mask);
> +	con_pcw_tmp = (con_pcw_tmp | (readl(regs->reg_mon) & dds_mask) |
> +		       data->pcwchg);
> +
> +	writel(con_pcw_tmp, pll->pcw_addr);
> +	writel(readl(regs->reg_hp_en) & ~BIT(data->fh_id), regs->reg_hp_en);
> +
> +	if (state->ssc_rate)
> +		fhctl_set_ssc_regs(pll, regs, data, state->ssc_rate);

Does it really make sense to set the SSC registers if the FH operation times out?

> +
> +	return ret;
> +}
> +
> +static unsigned int __get_postdiv(struct mtk_clk_pll *pll,
> +				  struct fh_pll_regs *regs,
> +				  const struct fh_pll_data *fh_data)

You don't need regs, nor fh_data, so this can be as simple as

static unsigned int __get_postdiv(struct mtk_clk_pll *pll)

> +{
> +	unsigned int regval;
> +
> +	regval = readl(pll->pd_addr) >> pll->data->pd_shift;
> +	regval &= POSTDIV_MASK;
> +

	return BIT(regval);

> +	return (1 << regval);
> +}
> +
> +static void __set_postdiv(struct mtk_clk_pll *pll, struct fh_pll_regs *regs,
> +			  const struct fh_pll_data *data, int postdiv)

static void __set_postdiv(struct mtk_clk_pll *pll, unsigned int val)

> +{
> +	unsigned int regval;
> +
> +	regval = readl(pll->pd_addr);
> +	regval &= ~(POSTDIV_MASK << pll->data->pd_shift);
> +	regval |= (ffs(postdiv) - 1) << pll->data->pd_shift;
> +	writel(regval, pll->pd_addr);
> +}
> +
> +static int fhctl_hopping(struct mtk_fh *fh, unsigned int new_dds, int postdiv)

The postdiv cannot ever be negative, so you can change it to an unsigned type...

static int fhctl_hopping(struct mtk_fh *fh, unsigned int new_dds,
			 unsigned int postdiv)

> +{
> +	const struct fh_pll_data *data = &fh->pllfh_data->data;
> +	struct fh_pll_state *state = &fh->pllfh_data->state;
> +	struct fh_pll_regs *regs = &fh->regs;
> +	struct mtk_clk_pll *pll = &fh->clk_pll;
> +	spinlock_t *lock = fh->lock;
> +	unsigned int pll_postdiv;
> +	unsigned long flags = 0;
> +	int ret;
> +

...so since postdiv is unsigned, we can write this conditional as

	if (postdiv)

> +	if (postdiv > 0) {
> +		pll_postdiv = __get_postdiv(pll, regs, data);
> +
> +		if (postdiv > pll_postdiv)
> +			__set_postdiv(pll, regs, data, postdiv);
> +	}
> +
> +	spin_lock_irqsave(lock, flags);
> +
> +	ret = hopping_hw_flow(pll, regs, data, state, new_dds);
> +
> +	spin_unlock_irqrestore(lock, flags);
> +
> +	if (postdiv > 0) {
> +		if (postdiv < pll_postdiv)

...and this one as

if (postdiv && postdiv < pll_postdiv)

> +			__set_postdiv(pll, regs, data, postdiv);
> +	}
> +
> +	return ret;
> +}
> +
> +static int __fhctl_ssc_enable(struct mtk_fh *fh, u32 rate)
> +{
> +	const struct fh_pll_data *data = &fh->pllfh_data->data;
> +	struct fh_pll_state *state = &fh->pllfh_data->state;
> +	struct fh_pll_regs *regs = &fh->regs;
> +	struct mtk_clk_pll *pll = &fh->clk_pll;
> +	spinlock_t *lock = fh->lock;
> +	unsigned long flags = 0;
> +
> +	spin_lock_irqsave(lock, flags);
> +
> +	fhctl_set_ssc_regs(pll, regs, data, rate);
> +	state->ssc_rate = rate;
> +
> +	spin_unlock_irqrestore(lock, flags);
> +
> +	return 0;
> +}
> +
> +static int fhctl_ssc_enable(struct mtk_fh *fh, u32 rate)
> +{
> +	return __fhctl_ssc_enable(fh, rate);
> +}
> +
> +static int fhctl_ssc_disable(struct mtk_fh *fh)
> +{
> +	return __fhctl_ssc_enable(fh, 0);
> +}
> +
> +static const struct fh_operation fhctl_ops = {
> +	.hopping = fhctl_hopping,
> +	.ssc_enable = fhctl_ssc_enable,

Just one callback for ssc_enable is enough, it's fine to keep the logic as
ssc_enable(fh, >0) to enable and ssc_enable(fh <=0) to disable.

> +	.ssc_disable = fhctl_ssc_disable,
> +};
> +
> +const struct fh_operation *fhctl_get_ops(void)
> +{
> +	return &fhctl_ops;
> +}
> +
> +void fhctl_hw_init(struct mtk_fh *fh)
> +{
> +	const struct fh_pll_data data = fh->pllfh_data->data;
> +	struct fh_pll_state state = fh->pllfh_data->state;
> +	struct fh_pll_regs regs = fh->regs;
> +	u32 val;
> +
> +	/* initial hw register */
> +	val = readl(regs.reg_clk_con) | BIT(data.fh_id);
> +	writel(val, regs.reg_clk_con);
> +
> +	val = readl(regs.reg_rst_con) & ~BIT(data.fh_id);
> +	writel(val, regs.reg_rst_con);
> +	val = readl(regs.reg_rst_con) | BIT(data.fh_id);
> +	writel(val, regs.reg_rst_con);
> +
> +	writel(0x0, regs.reg_cfg);
> +	writel(0x0, regs.reg_updnlmt);
> +	writel(0x0, regs.reg_dds);
> +
> +	/* enable ssc if needed */
> +	if (state.ssc_rate)
> +		fh->ops->ssc_enable(fh, state.ssc_rate);
> +}
> diff --git a/drivers/clk/mediatek/clk-fhctl.h b/drivers/clk/mediatek/clk-fhctl.h
> new file mode 100644
> index 000000000000..3cd4921c39e9
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-fhctl.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2022 MediaTek Inc.
> + * Author: Edward-JW Yang <edward-jw.yang@mediatek.com>
> + */
> +
> +#ifndef __CLK_FHCTL_H
> +#define __CLK_FHCTL_H
> +

One blank line is enough.

> +
> +struct fhctl_offset {
> +	u32 offset_hp_en;
> +	u32 offset_clk_con;
> +	u32 offset_rst_con;
> +	u32 offset_slope0;
> +	u32 offset_slope1;
> +	u32 offset_cfg;
> +	u32 offset_updnlmt;
> +	u32 offset_dds;
> +	u32 offset_dvfs;
> +	u32 offset_mon;
> +};
> +const struct fhctl_offset *fhctl_get_offset_table(void);
> +const struct fh_operation *fhctl_get_ops(void);
> +void fhctl_hw_init(struct mtk_fh *fh);
> +
> +#endif
> diff --git a/drivers/clk/mediatek/clk-pllfh.c b/drivers/clk/mediatek/clk-pllfh.c
> new file mode 100644
> index 000000000000..71b35323b526
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-pllfh.c
> @@ -0,0 +1,271 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022 MediaTek Inc.
> + * Author: Edward-JW Yang <edward-jw.yang@mediatek.com>
> + */
> +
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/clkdev.h>
> +#include <linux/delay.h>
> +
> +#include "clk-mtk.h"
> +#include "clk-pllfh.h"
> +#include "clk-fhctl.h"
> +
> +static DEFINE_SPINLOCK(pllfh_lock);
> +
> +inline struct mtk_fh *to_mtk_fh(struct clk_hw *hw)
> +{
> +	struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> +
> +	return container_of(pll, struct mtk_fh, clk_pll);
> +}
> +
> +static int mtk_fhctl_set_rate(struct clk_hw *hw, unsigned long rate,
> +			      unsigned long parent_rate)
> +{
> +	struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> +	struct mtk_fh *fh = to_mtk_fh(hw);
> +	u32 pcw = 0;
> +	u32 postdiv;
> +
> +	mtk_pll_calc_values(pll, &pcw, &postdiv, rate, parent_rate);
> +	fh->ops->hopping(fh, pcw, postdiv);

return fh->ops->hopping(....);

> +
> +	return 0;
> +}
> +

..snip..

> diff --git a/drivers/clk/mediatek/clk-pllfh.h b/drivers/clk/mediatek/clk-pllfh.h
> new file mode 100644
> index 000000000000..d9f7c8527548
> --- /dev/null
> +++ b/drivers/clk/mediatek/clk-pllfh.h
> @@ -0,0 +1,81 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (c) 2022 MediaTek Inc.
> + * Author: Edward-JW Yang <edward-jw.yang@mediatek.com>
> + */
> +
> +#ifndef __DRV_CLKFH_H
> +#define __DRV_CLKFH_H
> +
> +#include "clk-pll.h"
> +
> +struct fh_pll_state {

Please, base as first member.

> +	u32 fh_enable;
> +	u32 ssc_rate;
> +	void __iomem *base;
> +};
> +

Regards,
Angelo



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] dt-bindings: arm: mediatek: Add new bindings of MediaTek frequency hopping

[Krzysztof Kozlowski]

On 01/09/2022 11:04, AngeloGioacchino Del Regno wrote:
> Il 31/08/22 15:19, Krzysztof Kozlowski ha scritto:
>> On 31/08/2022 15:48, Johnson Wang wrote:
>>> Add the new binding documentation for MediaTek frequency hopping
>>> and spread spectrum clocking control.
>>>
>>> Co-developed-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
>>> Signed-off-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
>>> Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
>>> ---
>>>   .../bindings/arm/mediatek/mediatek,fhctl.yaml | 49 +++++++++++++++++++
>>>   1 file changed, 49 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml
>>> new file mode 100644
>>> index 000000000000..c5d76410538b
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml
>>> @@ -0,0 +1,49 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/arm/mediatek/mediatek,fhctl.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: MediaTek frequency hopping and spread spectrum clocking control
>>> +
>>> +maintainers:
>>> +  - Edward-JW Yang <edward-jw.yang@mediatek.com>
>>> +
>>> +description: |
>>> +  Frequency hopping control (FHCTL) is a piece of hardware that control
>>> +  some PLLs to adopt "hopping" mechanism to adjust their frequency.
>>> +  Spread spectrum clocking (SSC) is another function provided by this hardware.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: mediatek,fhctl
>>
>> You need SoC/device specific compatibles. Preferably only SoC specific,
>> without generic fallback, unless you can guarantee (while representing
>> MediaTek), that generic fallback will cover all of their SoCs?
>>
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  mediatek,hopping-ssc-percents:
>>> +    description: |
>>> +      Determine the enablement of frequency hopping feature and the percentage
>>> +      of spread spectrum clocking for PLLs.
>>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
>>> +    items:
>>> +      items:
>>> +        - description: PLL id that is expected to enable frequency hopping.
>>
>> So the clocks are indices from some specific, yet unnamed
>> clock-controller? This feels hacky. You should rather take here clock
>> phandles (1) or integrate it into specific clock controller (2). The
>> reason is that either your device does something on top of existing
>> clocks (option 1, thus it takes clock as inputs) or it modifies existing
>> clocks (option 2, thus it is integral part of clock-controller).
>>
> 
> FHCTL is a MCU that handles (some, or all, depending on what's supported on the
> SoC and what's needed by the board) PLL frequency setting, doing it in steps and
> avoiding overshooting and other issues.
> 
> We had a pretty big conversation about this a while ago and the indices instead
> of phandles is actually my fault, that happened because I initially proposed your
> option 2 but then for a number of reasons we went with this kind of solution.
> 
> I know it's going to be a long read, but the entire conversation is on the list [1]
> 

Sorry, but it's a hacky architecture where one device (which is a clock
provider) and second device have no relationship in hardware description
but both play with each other resources. That's simply not a proper
hardware description, so again:

1. If this is separate device (as you indicated), then it needs
expressing the dependencies and uses of other device resources.

2. If this is not a separate device, but integral part of clock
controller, then it would be fine but then probably should be child of
that device.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] dt-bindings: arm: mediatek: Add new bindings of MediaTek frequency hopping

[Krzysztof Kozlowski]

On 01/09/2022 11:04, AngeloGioacchino Del Regno wrote:
> I know it's going to be a long read, but the entire conversation is on the list [1]
> 
> Cheers,
> Angelo
> 
> [1]: 
> https://patchwork.kernel.org/project/linux-mediatek/patch/20220612135414.3003-3-johnson.wang@mediatek.com/

I see there hundreds of lines of quoted text without trimming to actual
parts, so no, no one is going to read it.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] dt-bindings: arm: mediatek: Add new bindings of MediaTek frequency hopping

[AngeloGioacchino Del Regno]

Il 01/09/22 11:42, Krzysztof Kozlowski ha scritto:
> On 01/09/2022 11:04, AngeloGioacchino Del Regno wrote:
>> Il 31/08/22 15:19, Krzysztof Kozlowski ha scritto:
>>> On 31/08/2022 15:48, Johnson Wang wrote:
>>>> Add the new binding documentation for MediaTek frequency hopping
>>>> and spread spectrum clocking control.
>>>>
>>>> Co-developed-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
>>>> Signed-off-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
>>>> Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
>>>> ---
>>>>    .../bindings/arm/mediatek/mediatek,fhctl.yaml | 49 +++++++++++++++++++
>>>>    1 file changed, 49 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml b/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml
>>>> new file mode 100644
>>>> index 000000000000..c5d76410538b
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml
>>>> @@ -0,0 +1,49 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/arm/mediatek/mediatek,fhctl.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: MediaTek frequency hopping and spread spectrum clocking control
>>>> +
>>>> +maintainers:
>>>> +  - Edward-JW Yang <edward-jw.yang@mediatek.com>
>>>> +
>>>> +description: |
>>>> +  Frequency hopping control (FHCTL) is a piece of hardware that control
>>>> +  some PLLs to adopt "hopping" mechanism to adjust their frequency.
>>>> +  Spread spectrum clocking (SSC) is another function provided by this hardware.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    const: mediatek,fhctl
>>>
>>> You need SoC/device specific compatibles. Preferably only SoC specific,
>>> without generic fallback, unless you can guarantee (while representing
>>> MediaTek), that generic fallback will cover all of their SoCs?
>>>
>>>> +
>>>> +  reg:
>>>> +    maxItems: 1
>>>> +
>>>> +  mediatek,hopping-ssc-percents:
>>>> +    description: |
>>>> +      Determine the enablement of frequency hopping feature and the percentage
>>>> +      of spread spectrum clocking for PLLs.
>>>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
>>>> +    items:
>>>> +      items:
>>>> +        - description: PLL id that is expected to enable frequency hopping.
>>>
>>> So the clocks are indices from some specific, yet unnamed
>>> clock-controller? This feels hacky. You should rather take here clock
>>> phandles (1) or integrate it into specific clock controller (2). The
>>> reason is that either your device does something on top of existing
>>> clocks (option 1, thus it takes clock as inputs) or it modifies existing
>>> clocks (option 2, thus it is integral part of clock-controller).
>>>
>>
>> FHCTL is a MCU that handles (some, or all, depending on what's supported on the
>> SoC and what's needed by the board) PLL frequency setting, doing it in steps and
>> avoiding overshooting and other issues.
>>
>> We had a pretty big conversation about this a while ago and the indices instead
>> of phandles is actually my fault, that happened because I initially proposed your
>> option 2 but then for a number of reasons we went with this kind of solution.
>>
>> I know it's going to be a long read, but the entire conversation is on the list [1]
>>
> 
> Sorry, but it's a hacky architecture where one device (which is a clock
> provider) and second device have no relationship in hardware description
> but both play with each other resources.

Yes, that's exactly how it is hardware-side. Except, just to be as clear as
possible, FHCTL plays with the clock provider, but *not* vice-versa.

> That's simply not a proper
> hardware description, so again:
> 
> 1. If this is separate device (as you indicated), then it needs
> expressing the dependencies and uses of other device resources.

Agreed. In this case, what about...

mediatek,hopping-ssc-percents = <&provider CLK_SOMEPLL 3>;

or would it be better to specify the clocks in a separated property?

clocks = <&provider CLK_SOMEPLL>, <&provider CLK_SOME_OTHER_PLL>;
mediatek,hopping-ssc-percents = <3>, <5>;

Thanks,
Angelo

> 
> 2. If this is not a separate device, but integral part of clock
> controller, then it would be fine but then probably should be child of
> that device.
> 
> Best regards,
> Krzysztof



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] dt-bindings: arm: mediatek: Add new bindings of MediaTek frequency hopping

[Krzysztof Kozlowski]

On 01/09/2022 13:22, AngeloGioacchino Del Regno wrote:
>> That's simply not a proper
>> hardware description, so again:
>>
>> 1. If this is separate device (as you indicated), then it needs
>> expressing the dependencies and uses of other device resources.
> 
> Agreed. In this case, what about...
> 
> mediatek,hopping-ssc-percents = <&provider CLK_SOMEPLL 3>;
> 
> or would it be better to specify the clocks in a separated property?
> 
> clocks = <&provider CLK_SOMEPLL>, <&provider CLK_SOME_OTHER_PLL>;
> mediatek,hopping-ssc-percents = <3>, <5>;
> 

I propose the last one - using standard clocks property and a matching
table.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] dt-bindings: arm: mediatek: Add new bindings of MediaTek frequency hopping

[AngeloGioacchino Del Regno]

Il 01/09/22 12:30, Krzysztof Kozlowski ha scritto:
> On 01/09/2022 13:22, AngeloGioacchino Del Regno wrote:
>>> That's simply not a proper
>>> hardware description, so again:
>>>
>>> 1. If this is separate device (as you indicated), then it needs
>>> expressing the dependencies and uses of other device resources.
>>
>> Agreed. In this case, what about...
>>
>> mediatek,hopping-ssc-percents = <&provider CLK_SOMEPLL 3>;
>>
>> or would it be better to specify the clocks in a separated property?
>>
>> clocks = <&provider CLK_SOMEPLL>, <&provider CLK_SOME_OTHER_PLL>;
>> mediatek,hopping-ssc-percents = <3>, <5>;
>>
> 
> I propose the last one - using standard clocks property and a matching
> table.
> 

Right. I like the last one a bit better, as well.

Thanks for the advices!
Angelo


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/4] Introduce MediaTek frequency hopping driver

[Johnson Wang]

On Wed, 2022-08-31 at 16:20 +0300, Krzysztof Kozlowski wrote:
> On 31/08/2022 15:48, Johnson Wang wrote:
> > Introduce MediaTek frequency hopping and spread spectrum clocking
> > control
> > for MT8186.
> 
> With one line introduction, you do not help us to understand this. :(
> 
> Best regards,
> Krzysztof

Hi Krzysztof,

Ok, I will describe more in the next version.

BRs,
Johnson Wang


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] dt-bindings: arm: mediatek: Add new bindings of MediaTek frequency hopping

[Johnson Wang]

On Wed, 2022-08-31 at 16:19 +0300, Krzysztof Kozlowski wrote:
> On 31/08/2022 15:48, Johnson Wang wrote:
> > Add the new binding documentation for MediaTek frequency hopping
> > and spread spectrum clocking control.
> > 
> > Co-developed-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
> > Signed-off-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
> > Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
> > ---
> >  .../bindings/arm/mediatek/mediatek,fhctl.yaml | 49
> > +++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yam
> > l
> > b/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yam
> > l
> > new file mode 100644
> > index 000000000000..c5d76410538b
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yam
> > l
> > @@ -0,0 +1,49 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: 
> > https://urldefense.com/v3/__http://devicetree.org/schemas/arm/mediatek/mediatek,fhctl.yaml*__;Iw!!CTRNKA9wMg0ARbw!ysl-bMp7yP9Ym70o6EVB8A36MBxcXGap8doEKR_SbaAQSy8-_RU5jvrWTjzETut_6eXNGut4j-3dY0q7xJdpQbmaOw$
> >  
> > +$schema: 
> > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!ysl-bMp7yP9Ym70o6EVB8A36MBxcXGap8doEKR_SbaAQSy8-_RU5jvrWTjzETut_6eXNGut4j-3dY0q7xJezt7_RBw$
> >  
> > +
> > +title: MediaTek frequency hopping and spread spectrum clocking
> > control
> > +
> > +maintainers:
> > +  - Edward-JW Yang <edward-jw.yang@mediatek.com>
> > +
> > +description: |
> > +  Frequency hopping control (FHCTL) is a piece of hardware that
> > control
> > +  some PLLs to adopt "hopping" mechanism to adjust their
> > frequency.
> > +  Spread spectrum clocking (SSC) is another function provided by
> > this hardware.
> > +
> > +properties:
> > +  compatible:
> > +    const: mediatek,fhctl
> 
> You need SoC/device specific compatibles. Preferably only SoC
> specific,
> without generic fallback, unless you can guarantee (while
> representing
> MediaTek), that generic fallback will cover all of their SoCs?
> 

Hi Krzysztof,

At this moment, we plan to support FHCTL feature for MT8186 only.

If you prefer SoC-specific compatble, we will improve that in the next
version.

Thanks for your suggestion.

BRs,
Johnson Wang
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  mediatek,hopping-ssc-percents:
> > +    description: |
> > +      Determine the enablement of frequency hopping feature and
> > the percentage
> > +      of spread spectrum clocking for PLLs.
> > +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > +    items:
> > +      items:
> > +        - description: PLL id that is expected to enable frequency
> > hopping.
> 
> So the clocks are indices from some specific, yet unnamed
> clock-controller? This feels hacky. You should rather take here clock
> phandles (1) or integrate it into specific clock controller (2). The
> reason is that either your device does something on top of existing
> clocks (option 1, thus it takes clock as inputs) or it modifies
> existing
> clocks (option 2, thus it is integral part of clock-controller).
> 
> 
> Best regards,
> Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] dt-bindings: arm: mediatek: Add new bindings of MediaTek frequency hopping

[Krzysztof Kozlowski]

On 02/09/2022 08:39, Johnson Wang wrote:
> On Wed, 2022-08-31 at 16:19 +0300, Krzysztof Kozlowski wrote:
>> On 31/08/2022 15:48, Johnson Wang wrote:
>>> Add the new binding documentation for MediaTek frequency hopping
>>> and spread spectrum clocking control.
>>>
>>> Co-developed-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
>>> Signed-off-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
>>> Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
>>> ---
>>>  .../bindings/arm/mediatek/mediatek,fhctl.yaml | 49
>>> +++++++++++++++++++
>>>  1 file changed, 49 insertions(+)
>>>  create mode 100644
>>> Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yaml
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yam
>>> l
>>> b/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yam
>>> l
>>> new file mode 100644
>>> index 000000000000..c5d76410538b
>>> --- /dev/null
>>> +++
>>> b/Documentation/devicetree/bindings/arm/mediatek/mediatek,fhctl.yam
>>> l
>>> @@ -0,0 +1,49 @@
>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: 
>>> https://urldefense.com/v3/__http://devicetree.org/schemas/arm/mediatek/mediatek,fhctl.yaml*__;Iw!!CTRNKA9wMg0ARbw!ysl-bMp7yP9Ym70o6EVB8A36MBxcXGap8doEKR_SbaAQSy8-_RU5jvrWTjzETut_6eXNGut4j-3dY0q7xJdpQbmaOw$
>>>  
>>> +$schema: 
>>> https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!ysl-bMp7yP9Ym70o6EVB8A36MBxcXGap8doEKR_SbaAQSy8-_RU5jvrWTjzETut_6eXNGut4j-3dY0q7xJezt7_RBw$
>>>  
>>> +
>>> +title: MediaTek frequency hopping and spread spectrum clocking
>>> control
>>> +
>>> +maintainers:
>>> +  - Edward-JW Yang <edward-jw.yang@mediatek.com>
>>> +
>>> +description: |
>>> +  Frequency hopping control (FHCTL) is a piece of hardware that
>>> control
>>> +  some PLLs to adopt "hopping" mechanism to adjust their
>>> frequency.
>>> +  Spread spectrum clocking (SSC) is another function provided by
>>> this hardware.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: mediatek,fhctl
>>
>> You need SoC/device specific compatibles. Preferably only SoC
>> specific,
>> without generic fallback, unless you can guarantee (while
>> representing
>> MediaTek), that generic fallback will cover all of their SoCs?
>>
> 
> Hi Krzysztof,
> 
> At this moment, we plan to support FHCTL feature for MT8186 only.
> 
> If you prefer SoC-specific compatble, we will improve that in the next
> version.

Then make it only for mt8186.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/4] clk: mediatek: Add new clock driver to handle FHCTL hardware

[Edward-JW Yang]

Hi Angelo,

Thanks for suggestions.

On Thu, 2022-09-01 at 16:05 +0800, AngeloGioacchino Del Regno wrote:
> Il 31/08/22 14:48, Johnson Wang ha scritto:
> > To implement frequency hopping and spread spectrum clocking
> > function, we introduce new clock type and APIs to handle
> > FHCTL hardware.
> > 
> > Co-developed-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
> > Signed-off-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
> > Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
> > ---
> >   drivers/clk/mediatek/Makefile    |   2 +-
> >   drivers/clk/mediatek/clk-fhctl.c | 258 +++++++++++++++++++++++++++++
> >   drivers/clk/mediatek/clk-fhctl.h |  27 +++
> >   drivers/clk/mediatek/clk-pllfh.c | 271 +++++++++++++++++++++++++++++++
> >   drivers/clk/mediatek/clk-pllfh.h |  81 +++++++++
> >   5 files changed, 638 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/clk/mediatek/clk-fhctl.c
> >   create mode 100644 drivers/clk/mediatek/clk-fhctl.h
> >   create mode 100644 drivers/clk/mediatek/clk-pllfh.c
> >   create mode 100644 drivers/clk/mediatek/clk-pllfh.h
> > 
> > diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
> > index caf2ce93d666..0e674a55e51e 100644
> > --- a/drivers/clk/mediatek/Makefile
> > +++ b/drivers/clk/mediatek/Makefile
> > @@ -1,5 +1,5 @@
> >   # SPDX-License-Identifier: GPL-2.0
> > -obj-$(CONFIG_COMMON_CLK_MEDIATEK) += clk-mtk.o clk-pll.o clk-gate.o clk-apmixed.o clk-cpumux.o reset.o clk-mux.o
> > +obj-$(CONFIG_COMMON_CLK_MEDIATEK) += clk-mtk.o clk-pll.o clk-gate.o clk-apmixed.o clk-cpumux.o reset.o clk-mux.o clk-fhctl.o clk-pllfh.o
> >   
> >   obj-$(CONFIG_COMMON_CLK_MT6765) += clk-mt6765.o
> >   obj-$(CONFIG_COMMON_CLK_MT6765_AUDIOSYS) += clk-mt6765-audio.o
> > diff --git a/drivers/clk/mediatek/clk-fhctl.c b/drivers/clk/mediatek/clk-fhctl.c
> > new file mode 100644
> > index 000000000000..69bd4ce2bc72
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-fhctl.c
> > @@ -0,0 +1,258 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2022 MediaTek Inc.
> > + * Author: Edward-JW Yang <edward-jw.yang@mediatek.com>
> > + */
> > +
> 
> #include <linux/io.h>
> 
> Please don't rely on implicit header inclusion.

OK, I'll fix it. Thanks.

> 
> > +#include <linux/iopoll.h>
> > +
> > +#include "clk-mtk.h"
> > +#include "clk-pllfh.h"
> > +#include "clk-fhctl.h"
> > +
> > +#define PERCENT_TO_DDSLMT(dds, percent_m10) \
> > +	((((dds) * (percent_m10)) >> 5) / 100)
> > +
> > +static const struct fhctl_offset fhctl_offset = {
> > +	.offset_hp_en = 0x0,
> > +	.offset_clk_con = 0x8,
> > +	.offset_rst_con = 0xc,
> > +	.offset_slope0 = 0x10,
> > +	.offset_slope1 = 0x14,
> > +	.offset_cfg = 0x0,
> > +	.offset_updnlmt = 0x4,
> > +	.offset_dds = 0x8,
> > +	.offset_dvfs = 0xc,
> > +	.offset_mon = 0x10,
> > +};
> > +
> > +const struct fhctl_offset *fhctl_get_offset_table(void)
> > +{
> > +	return &fhctl_offset;
> > +}
> > +
> > +static void dump_hw(struct mtk_clk_pll *pll, struct fh_pll_regs *regs,
> > +		    const struct fh_pll_data *data)
> > +{
> > +	pr_info("hp_en<%x>,clk_con<%x>,slope0<%x>,slope1<%x>\n",
> > +		readl(regs->reg_hp_en), readl(regs->reg_clk_con),
> > +		readl(regs->reg_slope0), readl(regs->reg_slope1));
> > +	pr_info("cfg<%x>,lmt<%x>,dds<%x>,dvfs<%x>,mon<%x>\n",
> > +		readl(regs->reg_cfg), readl(regs->reg_updnlmt),
> > +		readl(regs->reg_dds), readl(regs->reg_dvfs),
> > +		readl(regs->reg_mon));
> > +	pr_info("pcw<%x>\n", readl(pll->pcw_addr));
> > +}
> > +
> > +static int fhctl_set_ssc_regs(struct mtk_clk_pll *pll, struct fh_pll_regs *regs,
> > +			      const struct fh_pll_data *data, u32 rate)
> > +{
> > +	u32 updnlmt_val, r;
> > +
> > +	writel((readl(regs->reg_cfg) & ~(data->frddsx_en)), regs->reg_cfg);
> > +	writel((readl(regs->reg_cfg) & ~(data->sfstrx_en)), regs->reg_cfg);
> > +	writel((readl(regs->reg_cfg) & ~(data->fhctlx_en)), regs->reg_cfg);
> > +
> > +	if (rate > 0) {
> > +		/* Set the relative parameter registers (dt/df/upbnd/downbnd) */
> > +		r = readl(regs->reg_cfg);
> > +		r &= ~(data->msk_frddsx_dys);
> > +		r |= (data->df_val << (ffs(data->msk_frddsx_dys) - 1));
> > +		writel(r, regs->reg_cfg);
> > +
> > +		r = readl(regs->reg_cfg);
> > +		r &= ~(data->msk_frddsx_dts);
> > +		r |= (data->dt_val << (ffs(data->msk_frddsx_dts) - 1));
> > +		writel(r, regs->reg_cfg);
> > +
> > +		writel((readl(pll->pcw_addr) & data->dds_mask) | data->tgl_org,
> > +			regs->reg_dds);
> > +
> > +		/* Calculate UPDNLMT */
> > +		updnlmt_val = PERCENT_TO_DDSLMT((readl(regs->reg_dds) &
> > +						 data->dds_mask), rate) <<
> > +						 data->updnlmt_shft;
> > +
> > +		writel(updnlmt_val, regs->reg_updnlmt);
> > +		writel(readl(regs->reg_hp_en) | BIT(data->fh_id),
> > +		       regs->reg_hp_en);
> > +		/* Enable SSC */
> > +		writel(readl(regs->reg_cfg) | data->frddsx_en, regs->reg_cfg);
> > +		/* Enable Hopping control */
> > +		writel(readl(regs->reg_cfg) | data->fhctlx_en, regs->reg_cfg);
> > +
> > +	} else {
> > +		/* Switch to APMIXEDSYS control */
> > +		writel(readl(regs->reg_hp_en) & ~BIT(data->fh_id),
> > +		       regs->reg_hp_en);
> > +		/* Wait for DDS to be stable */
> > +		udelay(30);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int hopping_hw_flow(struct mtk_clk_pll *pll, struct fh_pll_regs *regs,
> > +			   const struct fh_pll_data *data,
> > +			   struct fh_pll_state *state, unsigned int new_dds)
> > +{
> > +	u32 dds_mask = data->dds_mask;
> > +	u32 mon_dds = 0;
> > +	u32 con_pcw_tmp;
> > +	int ret;
> > +
> > +	if (state->ssc_rate)
> > +		fhctl_set_ssc_regs(pll, regs, data, 0);
> > +
> > +	writel((readl(pll->pcw_addr) & dds_mask) | data->tgl_org,
> > +		regs->reg_dds);
> > +
> > +	writel(readl(regs->reg_cfg) | data->sfstrx_en, regs->reg_cfg);
> > +	writel(readl(regs->reg_cfg) | data->fhctlx_en, regs->reg_cfg);
> > +	writel(data->slope0_value, regs->reg_slope0);
> > +	writel(data->slope1_value, regs->reg_slope1);
> > +
> > +	writel(readl(regs->reg_hp_en) | BIT(data->fh_id), regs->reg_hp_en);
> > +	writel((new_dds) | (data->dvfs_tri), regs->reg_dvfs);
> > +
> > +	/* Wait 1000 us until DDS stable */
> > +	ret = readl_poll_timeout_atomic(regs->reg_mon, mon_dds,
> > +				       (mon_dds & dds_mask) == new_dds,
> > +					10, 1000);
> > +	if (ret) {
> > +		pr_warn("%s: FHCTL hopping timeout\n", pll->data->name);
> > +		dump_hw(pll, regs, data);
> > +	}
> > +
> > +	con_pcw_tmp = readl(pll->pcw_addr) & (~dds_mask);
> > +	con_pcw_tmp = (con_pcw_tmp | (readl(regs->reg_mon) & dds_mask) |
> > +		       data->pcwchg);
> > +
> > +	writel(con_pcw_tmp, pll->pcw_addr);
> > +	writel(readl(regs->reg_hp_en) & ~BIT(data->fh_id), regs->reg_hp_en);
> > +
> > +	if (state->ssc_rate)
> > +		fhctl_set_ssc_regs(pll, regs, data, state->ssc_rate);
> 
> Does it really make sense to set the SSC registers if the FH operation times out?

FH opration times out means DDS doesn't reach the target dds value but the de-sense issue
may still exist. We can still base on current dds_mon value to do SSC.

> 
> > +
> > +	return ret;
> > +}
> > +
> > +static unsigned int __get_postdiv(struct mtk_clk_pll *pll,
> > +				  struct fh_pll_regs *regs,
> > +				  const struct fh_pll_data *fh_data)
> 
> You don't need regs, nor fh_data, so this can be as simple as
> 
> static unsigned int __get_postdiv(struct mtk_clk_pll *pll)

OK, I'll fix it. Thanks.

> 
> > +{
> > +	unsigned int regval;
> > +
> > +	regval = readl(pll->pd_addr) >> pll->data->pd_shift;
> > +	regval &= POSTDIV_MASK;
> > +
> 
> 	return BIT(regval);

OK, I'll fix it. Thanks.

> 
> > +	return (1 << regval);
> > +}
> > +
> > +static void __set_postdiv(struct mtk_clk_pll *pll, struct fh_pll_regs *regs,
> > +			  const struct fh_pll_data *data, int postdiv)
> 
> static void __set_postdiv(struct mtk_clk_pll *pll, unsigned int val)

OK, I'll fix it. Thanks.

> 
> > +{
> > +	unsigned int regval;
> > +
> > +	regval = readl(pll->pd_addr);
> > +	regval &= ~(POSTDIV_MASK << pll->data->pd_shift);
> > +	regval |= (ffs(postdiv) - 1) << pll->data->pd_shift;
> > +	writel(regval, pll->pd_addr);
> > +}
> > +
> > +static int fhctl_hopping(struct mtk_fh *fh, unsigned int new_dds, int postdiv)
> 
> The postdiv cannot ever be negative, so you can change it to an unsigned type...
> 
> static int fhctl_hopping(struct mtk_fh *fh, unsigned int new_dds,
> 			 unsigned int postdiv)

OK, I'll fix it. Thanks.

> 
> > +{
> > +	const struct fh_pll_data *data = &fh->pllfh_data->data;
> > +	struct fh_pll_state *state = &fh->pllfh_data->state;
> > +	struct fh_pll_regs *regs = &fh->regs;
> > +	struct mtk_clk_pll *pll = &fh->clk_pll;
> > +	spinlock_t *lock = fh->lock;
> > +	unsigned int pll_postdiv;
> > +	unsigned long flags = 0;
> > +	int ret;
> > +
> 
> ...so since postdiv is unsigned, we can write this conditional as
> 
> 	if (postdiv)

OK, I'll fix it. Thanks.

> 
> > +	if (postdiv > 0) {
> > +		pll_postdiv = __get_postdiv(pll, regs, data);
> > +
> > +		if (postdiv > pll_postdiv)
> > +			__set_postdiv(pll, regs, data, postdiv);
> > +	}
> > +
> > +	spin_lock_irqsave(lock, flags);
> > +
> > +	ret = hopping_hw_flow(pll, regs, data, state, new_dds);
> > +
> > +	spin_unlock_irqrestore(lock, flags);
> > +
> > +	if (postdiv > 0) {
> > +		if (postdiv < pll_postdiv)
> 
> ...and this one as
> 
> if (postdiv && postdiv < pll_postdiv)

OK, I'll fix it. Thanks.

> 
> > +			__set_postdiv(pll, regs, data, postdiv);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int __fhctl_ssc_enable(struct mtk_fh *fh, u32 rate)
> > +{
> > +	const struct fh_pll_data *data = &fh->pllfh_data->data;
> > +	struct fh_pll_state *state = &fh->pllfh_data->state;
> > +	struct fh_pll_regs *regs = &fh->regs;
> > +	struct mtk_clk_pll *pll = &fh->clk_pll;
> > +	spinlock_t *lock = fh->lock;
> > +	unsigned long flags = 0;
> > +
> > +	spin_lock_irqsave(lock, flags);
> > +
> > +	fhctl_set_ssc_regs(pll, regs, data, rate);
> > +	state->ssc_rate = rate;
> > +
> > +	spin_unlock_irqrestore(lock, flags);
> > +
> > +	return 0;
> > +}
> > +
> > +static int fhctl_ssc_enable(struct mtk_fh *fh, u32 rate)
> > +{
> > +	return __fhctl_ssc_enable(fh, rate);
> > +}
> > +
> > +static int fhctl_ssc_disable(struct mtk_fh *fh)
> > +{
> > +	return __fhctl_ssc_enable(fh, 0);
> > +}
> > +
> > +static const struct fh_operation fhctl_ops = {
> > +	.hopping = fhctl_hopping,
> > +	.ssc_enable = fhctl_ssc_enable,
> 
> Just one callback for ssc_enable is enough, it's fine to keep the logic as
> ssc_enable(fh, >0) to enable and ssc_enable(fh <=0) to disable.

OK, I'll drop ssc_disable callback.

> 
> > +	.ssc_disable = fhctl_ssc_disable,
> > +};
> > +
> > +const struct fh_operation *fhctl_get_ops(void)
> > +{
> > +	return &fhctl_ops;
> > +}
> > +
> > +void fhctl_hw_init(struct mtk_fh *fh)
> > +{
> > +	const struct fh_pll_data data = fh->pllfh_data->data;
> > +	struct fh_pll_state state = fh->pllfh_data->state;
> > +	struct fh_pll_regs regs = fh->regs;
> > +	u32 val;
> > +
> > +	/* initial hw register */
> > +	val = readl(regs.reg_clk_con) | BIT(data.fh_id);
> > +	writel(val, regs.reg_clk_con);
> > +
> > +	val = readl(regs.reg_rst_con) & ~BIT(data.fh_id);
> > +	writel(val, regs.reg_rst_con);
> > +	val = readl(regs.reg_rst_con) | BIT(data.fh_id);
> > +	writel(val, regs.reg_rst_con);
> > +
> > +	writel(0x0, regs.reg_cfg);
> > +	writel(0x0, regs.reg_updnlmt);
> > +	writel(0x0, regs.reg_dds);
> > +
> > +	/* enable ssc if needed */
> > +	if (state.ssc_rate)
> > +		fh->ops->ssc_enable(fh, state.ssc_rate);
> > +}
> > diff --git a/drivers/clk/mediatek/clk-fhctl.h b/drivers/clk/mediatek/clk-fhctl.h
> > new file mode 100644
> > index 000000000000..3cd4921c39e9
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-fhctl.h
> > @@ -0,0 +1,27 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (c) 2022 MediaTek Inc.
> > + * Author: Edward-JW Yang <edward-jw.yang@mediatek.com>
> > + */
> > +
> > +#ifndef __CLK_FHCTL_H
> > +#define __CLK_FHCTL_H
> > +
> 
> One blank line is enough.

OK, I'll fix it. Thanks

> 
> > +
> > +struct fhctl_offset {
> > +	u32 offset_hp_en;
> > +	u32 offset_clk_con;
> > +	u32 offset_rst_con;
> > +	u32 offset_slope0;
> > +	u32 offset_slope1;
> > +	u32 offset_cfg;
> > +	u32 offset_updnlmt;
> > +	u32 offset_dds;
> > +	u32 offset_dvfs;
> > +	u32 offset_mon;
> > +};
> > +const struct fhctl_offset *fhctl_get_offset_table(void);
> > +const struct fh_operation *fhctl_get_ops(void);
> > +void fhctl_hw_init(struct mtk_fh *fh);
> > +
> > +#endif
> > diff --git a/drivers/clk/mediatek/clk-pllfh.c b/drivers/clk/mediatek/clk-pllfh.c
> > new file mode 100644
> > index 000000000000..71b35323b526
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-pllfh.c
> > @@ -0,0 +1,271 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2022 MediaTek Inc.
> > + * Author: Edward-JW Yang <edward-jw.yang@mediatek.com>
> > + */
> > +
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/io.h>
> > +#include <linux/slab.h>
> > +#include <linux/clkdev.h>
> > +#include <linux/delay.h>
> > +
> > +#include "clk-mtk.h"
> > +#include "clk-pllfh.h"
> > +#include "clk-fhctl.h"
> > +
> > +static DEFINE_SPINLOCK(pllfh_lock);
> > +
> > +inline struct mtk_fh *to_mtk_fh(struct clk_hw *hw)
> > +{
> > +	struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> > +
> > +	return container_of(pll, struct mtk_fh, clk_pll);
> > +}
> > +
> > +static int mtk_fhctl_set_rate(struct clk_hw *hw, unsigned long rate,
> > +			      unsigned long parent_rate)
> > +{
> > +	struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> > +	struct mtk_fh *fh = to_mtk_fh(hw);
> > +	u32 pcw = 0;
> > +	u32 postdiv;
> > +
> > +	mtk_pll_calc_values(pll, &pcw, &postdiv, rate, parent_rate);
> > +	fh->ops->hopping(fh, pcw, postdiv);
> 
> return fh->ops->hopping(....);

OK, I'll fix it. Thanks.

> 
> > +
> > +	return 0;
> > +}
> > +
> 
> ..snip..
> 
> > diff --git a/drivers/clk/mediatek/clk-pllfh.h b/drivers/clk/mediatek/clk-pllfh.h
> > new file mode 100644
> > index 000000000000..d9f7c8527548
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-pllfh.h
> > @@ -0,0 +1,81 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (c) 2022 MediaTek Inc.
> > + * Author: Edward-JW Yang <edward-jw.yang@mediatek.com>
> > + */
> > +
> > +#ifndef __DRV_CLKFH_H
> > +#define __DRV_CLKFH_H
> > +
> > +#include "clk-pll.h"
> > +
> > +struct fh_pll_state {
> 
> Please, base as first member.

OK, I'll modify it.

> 
> > +	u32 fh_enable;
> > +	u32 ssc_rate;
> > +	void __iomem *base;
> > +};
> > +
> 
> Regards,
> Angelo
> 
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/4] clk: mediatek: Add new clock driver to handle FHCTL hardware

[Edward-JW Yang]

Hi Angelo,

Thanks for suggestions.

On Thu, 2022-09-01 at 16:05 +0800, AngeloGioacchino Del Regno wrote:
> Il 31/08/22 14:48, Johnson Wang ha scritto:
> > To implement frequency hopping and spread spectrum clocking
> > function, we introduce new clock type and APIs to handle
> > FHCTL hardware.
> > 
> > Co-developed-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
> > Signed-off-by: Edward-JW Yang <edward-jw.yang@mediatek.com>
> > Signed-off-by: Johnson Wang <johnson.wang@mediatek.com>
> > ---
> >   drivers/clk/mediatek/Makefile    |   2 +-
> >   drivers/clk/mediatek/clk-fhctl.c | 258 +++++++++++++++++++++++++++++
> >   drivers/clk/mediatek/clk-fhctl.h |  27 +++
> >   drivers/clk/mediatek/clk-pllfh.c | 271 +++++++++++++++++++++++++++++++
> >   drivers/clk/mediatek/clk-pllfh.h |  81 +++++++++
> >   5 files changed, 638 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/clk/mediatek/clk-fhctl.c
> >   create mode 100644 drivers/clk/mediatek/clk-fhctl.h
> >   create mode 100644 drivers/clk/mediatek/clk-pllfh.c
> >   create mode 100644 drivers/clk/mediatek/clk-pllfh.h
> > 
> > diff --git a/drivers/clk/mediatek/Makefile b/drivers/clk/mediatek/Makefile
> > index caf2ce93d666..0e674a55e51e 100644
> > --- a/drivers/clk/mediatek/Makefile
> > +++ b/drivers/clk/mediatek/Makefile
> > @@ -1,5 +1,5 @@
> >   # SPDX-License-Identifier: GPL-2.0
> > -obj-$(CONFIG_COMMON_CLK_MEDIATEK) += clk-mtk.o clk-pll.o clk-gate.o clk-apmixed.o clk-cpumux.o reset.o clk-mux.o
> > +obj-$(CONFIG_COMMON_CLK_MEDIATEK) += clk-mtk.o clk-pll.o clk-gate.o clk-apmixed.o clk-cpumux.o reset.o clk-mux.o clk-fhctl.o clk-pllfh.o
> >   
> >   obj-$(CONFIG_COMMON_CLK_MT6765) += clk-mt6765.o
> >   obj-$(CONFIG_COMMON_CLK_MT6765_AUDIOSYS) += clk-mt6765-audio.o
> > diff --git a/drivers/clk/mediatek/clk-fhctl.c b/drivers/clk/mediatek/clk-fhctl.c
> > new file mode 100644
> > index 000000000000..69bd4ce2bc72
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-fhctl.c
> > @@ -0,0 +1,258 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2022 MediaTek Inc.
> > + * Author: Edward-JW Yang <edward-jw.yang@mediatek.com>
> > + */
> > +
> 
> #include <linux/io.h>
> 
> Please don't rely on implicit header inclusion.

OK, I'll fix it. Thanks.

> 
> > +#include <linux/iopoll.h>
> > +
> > +#include "clk-mtk.h"
> > +#include "clk-pllfh.h"
> > +#include "clk-fhctl.h"
> > +
> > +#define PERCENT_TO_DDSLMT(dds, percent_m10) \
> > +	((((dds) * (percent_m10)) >> 5) / 100)
> > +
> > +static const struct fhctl_offset fhctl_offset = {
> > +	.offset_hp_en = 0x0,
> > +	.offset_clk_con = 0x8,
> > +	.offset_rst_con = 0xc,
> > +	.offset_slope0 = 0x10,
> > +	.offset_slope1 = 0x14,
> > +	.offset_cfg = 0x0,
> > +	.offset_updnlmt = 0x4,
> > +	.offset_dds = 0x8,
> > +	.offset_dvfs = 0xc,
> > +	.offset_mon = 0x10,
> > +};
> > +
> > +const struct fhctl_offset *fhctl_get_offset_table(void)
> > +{
> > +	return &fhctl_offset;
> > +}
> > +
> > +static void dump_hw(struct mtk_clk_pll *pll, struct fh_pll_regs *regs,
> > +		    const struct fh_pll_data *data)
> > +{
> > +	pr_info("hp_en<%x>,clk_con<%x>,slope0<%x>,slope1<%x>\n",
> > +		readl(regs->reg_hp_en), readl(regs->reg_clk_con),
> > +		readl(regs->reg_slope0), readl(regs->reg_slope1));
> > +	pr_info("cfg<%x>,lmt<%x>,dds<%x>,dvfs<%x>,mon<%x>\n",
> > +		readl(regs->reg_cfg), readl(regs->reg_updnlmt),
> > +		readl(regs->reg_dds), readl(regs->reg_dvfs),
> > +		readl(regs->reg_mon));
> > +	pr_info("pcw<%x>\n", readl(pll->pcw_addr));
> > +}
> > +
> > +static int fhctl_set_ssc_regs(struct mtk_clk_pll *pll, struct fh_pll_regs *regs,
> > +			      const struct fh_pll_data *data, u32 rate)
> > +{
> > +	u32 updnlmt_val, r;
> > +
> > +	writel((readl(regs->reg_cfg) & ~(data->frddsx_en)), regs->reg_cfg);
> > +	writel((readl(regs->reg_cfg) & ~(data->sfstrx_en)), regs->reg_cfg);
> > +	writel((readl(regs->reg_cfg) & ~(data->fhctlx_en)), regs->reg_cfg);
> > +
> > +	if (rate > 0) {
> > +		/* Set the relative parameter registers (dt/df/upbnd/downbnd) */
> > +		r = readl(regs->reg_cfg);
> > +		r &= ~(data->msk_frddsx_dys);
> > +		r |= (data->df_val << (ffs(data->msk_frddsx_dys) - 1));
> > +		writel(r, regs->reg_cfg);
> > +
> > +		r = readl(regs->reg_cfg);
> > +		r &= ~(data->msk_frddsx_dts);
> > +		r |= (data->dt_val << (ffs(data->msk_frddsx_dts) - 1));
> > +		writel(r, regs->reg_cfg);
> > +
> > +		writel((readl(pll->pcw_addr) & data->dds_mask) | data->tgl_org,
> > +			regs->reg_dds);
> > +
> > +		/* Calculate UPDNLMT */
> > +		updnlmt_val = PERCENT_TO_DDSLMT((readl(regs->reg_dds) &
> > +						 data->dds_mask), rate) <<
> > +						 data->updnlmt_shft;
> > +
> > +		writel(updnlmt_val, regs->reg_updnlmt);
> > +		writel(readl(regs->reg_hp_en) | BIT(data->fh_id),
> > +		       regs->reg_hp_en);
> > +		/* Enable SSC */
> > +		writel(readl(regs->reg_cfg) | data->frddsx_en, regs->reg_cfg);
> > +		/* Enable Hopping control */
> > +		writel(readl(regs->reg_cfg) | data->fhctlx_en, regs->reg_cfg);
> > +
> > +	} else {
> > +		/* Switch to APMIXEDSYS control */
> > +		writel(readl(regs->reg_hp_en) & ~BIT(data->fh_id),
> > +		       regs->reg_hp_en);
> > +		/* Wait for DDS to be stable */
> > +		udelay(30);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int hopping_hw_flow(struct mtk_clk_pll *pll, struct fh_pll_regs *regs,
> > +			   const struct fh_pll_data *data,
> > +			   struct fh_pll_state *state, unsigned int new_dds)
> > +{
> > +	u32 dds_mask = data->dds_mask;
> > +	u32 mon_dds = 0;
> > +	u32 con_pcw_tmp;
> > +	int ret;
> > +
> > +	if (state->ssc_rate)
> > +		fhctl_set_ssc_regs(pll, regs, data, 0);
> > +
> > +	writel((readl(pll->pcw_addr) & dds_mask) | data->tgl_org,
> > +		regs->reg_dds);
> > +
> > +	writel(readl(regs->reg_cfg) | data->sfstrx_en, regs->reg_cfg);
> > +	writel(readl(regs->reg_cfg) | data->fhctlx_en, regs->reg_cfg);
> > +	writel(data->slope0_value, regs->reg_slope0);
> > +	writel(data->slope1_value, regs->reg_slope1);
> > +
> > +	writel(readl(regs->reg_hp_en) | BIT(data->fh_id), regs->reg_hp_en);
> > +	writel((new_dds) | (data->dvfs_tri), regs->reg_dvfs);
> > +
> > +	/* Wait 1000 us until DDS stable */
> > +	ret = readl_poll_timeout_atomic(regs->reg_mon, mon_dds,
> > +				       (mon_dds & dds_mask) == new_dds,
> > +					10, 1000);
> > +	if (ret) {
> > +		pr_warn("%s: FHCTL hopping timeout\n", pll->data->name);
> > +		dump_hw(pll, regs, data);
> > +	}
> > +
> > +	con_pcw_tmp = readl(pll->pcw_addr) & (~dds_mask);
> > +	con_pcw_tmp = (con_pcw_tmp | (readl(regs->reg_mon) & dds_mask) |
> > +		       data->pcwchg);
> > +
> > +	writel(con_pcw_tmp, pll->pcw_addr);
> > +	writel(readl(regs->reg_hp_en) & ~BIT(data->fh_id), regs->reg_hp_en);
> > +
> > +	if (state->ssc_rate)
> > +		fhctl_set_ssc_regs(pll, regs, data, state->ssc_rate);
> 
> Does it really make sense to set the SSC registers if the FH operation times out?

FH opration times out means DDS doesn't reach the target dds value but the de-sense issue
may still exist. We can still base on current dds_mon value to do SSC.

> 
> > +
> > +	return ret;
> > +}
> > +
> > +static unsigned int __get_postdiv(struct mtk_clk_pll *pll,
> > +				  struct fh_pll_regs *regs,
> > +				  const struct fh_pll_data *fh_data)
> 
> You don't need regs, nor fh_data, so this can be as simple as
> 
> static unsigned int __get_postdiv(struct mtk_clk_pll *pll)

OK, I'll fix it. Thanks.

> 
> > +{
> > +	unsigned int regval;
> > +
> > +	regval = readl(pll->pd_addr) >> pll->data->pd_shift;
> > +	regval &= POSTDIV_MASK;
> > +
> 
> 	return BIT(regval);

OK, I'll fix it. Thanks.

> 
> > +	return (1 << regval);
> > +}
> > +
> > +static void __set_postdiv(struct mtk_clk_pll *pll, struct fh_pll_regs *regs,
> > +			  const struct fh_pll_data *data, int postdiv)
> 
> static void __set_postdiv(struct mtk_clk_pll *pll, unsigned int val)

OK, I'll fix it. Thanks.

> 
> > +{
> > +	unsigned int regval;
> > +
> > +	regval = readl(pll->pd_addr);
> > +	regval &= ~(POSTDIV_MASK << pll->data->pd_shift);
> > +	regval |= (ffs(postdiv) - 1) << pll->data->pd_shift;
> > +	writel(regval, pll->pd_addr);
> > +}
> > +
> > +static int fhctl_hopping(struct mtk_fh *fh, unsigned int new_dds, int postdiv)
> 
> The postdiv cannot ever be negative, so you can change it to an unsigned type...
> 
> static int fhctl_hopping(struct mtk_fh *fh, unsigned int new_dds,
> 			 unsigned int postdiv)

OK, I'll fix it. Thanks.

> 
> > +{
> > +	const struct fh_pll_data *data = &fh->pllfh_data->data;
> > +	struct fh_pll_state *state = &fh->pllfh_data->state;
> > +	struct fh_pll_regs *regs = &fh->regs;
> > +	struct mtk_clk_pll *pll = &fh->clk_pll;
> > +	spinlock_t *lock = fh->lock;
> > +	unsigned int pll_postdiv;
> > +	unsigned long flags = 0;
> > +	int ret;
> > +
> 
> ...so since postdiv is unsigned, we can write this conditional as
> 
> 	if (postdiv)

OK, I'll fix it. Thanks.

> 
> > +	if (postdiv > 0) {
> > +		pll_postdiv = __get_postdiv(pll, regs, data);
> > +
> > +		if (postdiv > pll_postdiv)
> > +			__set_postdiv(pll, regs, data, postdiv);
> > +	}
> > +
> > +	spin_lock_irqsave(lock, flags);
> > +
> > +	ret = hopping_hw_flow(pll, regs, data, state, new_dds);
> > +
> > +	spin_unlock_irqrestore(lock, flags);
> > +
> > +	if (postdiv > 0) {
> > +		if (postdiv < pll_postdiv)
> 
> ...and this one as
> 
> if (postdiv && postdiv < pll_postdiv)

OK, I'll fix it. Thanks.

> 
> > +			__set_postdiv(pll, regs, data, postdiv);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int __fhctl_ssc_enable(struct mtk_fh *fh, u32 rate)
> > +{
> > +	const struct fh_pll_data *data = &fh->pllfh_data->data;
> > +	struct fh_pll_state *state = &fh->pllfh_data->state;
> > +	struct fh_pll_regs *regs = &fh->regs;
> > +	struct mtk_clk_pll *pll = &fh->clk_pll;
> > +	spinlock_t *lock = fh->lock;
> > +	unsigned long flags = 0;
> > +
> > +	spin_lock_irqsave(lock, flags);
> > +
> > +	fhctl_set_ssc_regs(pll, regs, data, rate);
> > +	state->ssc_rate = rate;
> > +
> > +	spin_unlock_irqrestore(lock, flags);
> > +
> > +	return 0;
> > +}
> > +
> > +static int fhctl_ssc_enable(struct mtk_fh *fh, u32 rate)
> > +{
> > +	return __fhctl_ssc_enable(fh, rate);
> > +}
> > +
> > +static int fhctl_ssc_disable(struct mtk_fh *fh)
> > +{
> > +	return __fhctl_ssc_enable(fh, 0);
> > +}
> > +
> > +static const struct fh_operation fhctl_ops = {
> > +	.hopping = fhctl_hopping,
> > +	.ssc_enable = fhctl_ssc_enable,
> 
> Just one callback for ssc_enable is enough, it's fine to keep the logic as
> ssc_enable(fh, >0) to enable and ssc_enable(fh <=0) to disable.

OK, I'll drop ssc_disable callback.

> 
> > +	.ssc_disable = fhctl_ssc_disable,
> > +};
> > +
> > +const struct fh_operation *fhctl_get_ops(void)
> > +{
> > +	return &fhctl_ops;
> > +}
> > +
> > +void fhctl_hw_init(struct mtk_fh *fh)
> > +{
> > +	const struct fh_pll_data data = fh->pllfh_data->data;
> > +	struct fh_pll_state state = fh->pllfh_data->state;
> > +	struct fh_pll_regs regs = fh->regs;
> > +	u32 val;
> > +
> > +	/* initial hw register */
> > +	val = readl(regs.reg_clk_con) | BIT(data.fh_id);
> > +	writel(val, regs.reg_clk_con);
> > +
> > +	val = readl(regs.reg_rst_con) & ~BIT(data.fh_id);
> > +	writel(val, regs.reg_rst_con);
> > +	val = readl(regs.reg_rst_con) | BIT(data.fh_id);
> > +	writel(val, regs.reg_rst_con);
> > +
> > +	writel(0x0, regs.reg_cfg);
> > +	writel(0x0, regs.reg_updnlmt);
> > +	writel(0x0, regs.reg_dds);
> > +
> > +	/* enable ssc if needed */
> > +	if (state.ssc_rate)
> > +		fh->ops->ssc_enable(fh, state.ssc_rate);
> > +}
> > diff --git a/drivers/clk/mediatek/clk-fhctl.h b/drivers/clk/mediatek/clk-fhctl.h
> > new file mode 100644
> > index 000000000000..3cd4921c39e9
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-fhctl.h
> > @@ -0,0 +1,27 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (c) 2022 MediaTek Inc.
> > + * Author: Edward-JW Yang <edward-jw.yang@mediatek.com>
> > + */
> > +
> > +#ifndef __CLK_FHCTL_H
> > +#define __CLK_FHCTL_H
> > +
> 
> One blank line is enough.

OK, I'll fix it. Thanks

> 
> > +
> > +struct fhctl_offset {
> > +	u32 offset_hp_en;
> > +	u32 offset_clk_con;
> > +	u32 offset_rst_con;
> > +	u32 offset_slope0;
> > +	u32 offset_slope1;
> > +	u32 offset_cfg;
> > +	u32 offset_updnlmt;
> > +	u32 offset_dds;
> > +	u32 offset_dvfs;
> > +	u32 offset_mon;
> > +};
> > +const struct fhctl_offset *fhctl_get_offset_table(void);
> > +const struct fh_operation *fhctl_get_ops(void);
> > +void fhctl_hw_init(struct mtk_fh *fh);
> > +
> > +#endif
> > diff --git a/drivers/clk/mediatek/clk-pllfh.c b/drivers/clk/mediatek/clk-pllfh.c
> > new file mode 100644
> > index 000000000000..71b35323b526
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-pllfh.c
> > @@ -0,0 +1,271 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2022 MediaTek Inc.
> > + * Author: Edward-JW Yang <edward-jw.yang@mediatek.com>
> > + */
> > +
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/io.h>
> > +#include <linux/slab.h>
> > +#include <linux/clkdev.h>
> > +#include <linux/delay.h>
> > +
> > +#include "clk-mtk.h"
> > +#include "clk-pllfh.h"
> > +#include "clk-fhctl.h"
> > +
> > +static DEFINE_SPINLOCK(pllfh_lock);
> > +
> > +inline struct mtk_fh *to_mtk_fh(struct clk_hw *hw)
> > +{
> > +	struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> > +
> > +	return container_of(pll, struct mtk_fh, clk_pll);
> > +}
> > +
> > +static int mtk_fhctl_set_rate(struct clk_hw *hw, unsigned long rate,
> > +			      unsigned long parent_rate)
> > +{
> > +	struct mtk_clk_pll *pll = to_mtk_clk_pll(hw);
> > +	struct mtk_fh *fh = to_mtk_fh(hw);
> > +	u32 pcw = 0;
> > +	u32 postdiv;
> > +
> > +	mtk_pll_calc_values(pll, &pcw, &postdiv, rate, parent_rate);
> > +	fh->ops->hopping(fh, pcw, postdiv);
> 
> return fh->ops->hopping(....);

OK, I'll fix it. Thanks.

> 
> > +
> > +	return 0;
> > +}
> > +
> 
> ..snip..
> 
> > diff --git a/drivers/clk/mediatek/clk-pllfh.h b/drivers/clk/mediatek/clk-pllfh.h
> > new file mode 100644
> > index 000000000000..d9f7c8527548
> > --- /dev/null
> > +++ b/drivers/clk/mediatek/clk-pllfh.h
> > @@ -0,0 +1,81 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Copyright (c) 2022 MediaTek Inc.
> > + * Author: Edward-JW Yang <edward-jw.yang@mediatek.com>
> > + */
> > +
> > +#ifndef __DRV_CLKFH_H
> > +#define __DRV_CLKFH_H
> > +
> > +#include "clk-pll.h"
> > +
> > +struct fh_pll_state {
> 
> Please, base as first member.

OK, I'll modify it.

> 
> > +	u32 fh_enable;
> > +	u32 ssc_rate;
> > +	void __iomem *base;
> > +};
> > +
> 
> Regards,
> Angelo
> 
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel