[PATCH v6 00/18] Modernize rest of the krait drivers

[PATCH v6 00/18] Modernize rest of the krait drivers

[Ansuel Smith]

This is a follow-up to the ipq806x gcc modernize series. Manu cleanup
changes and also some discoveries of wrong definition notice only with
all these conversions.

The first patch is an improvement of the clk_hw_get_parent_index. The
original idea of clk_hw_get_parent_index was to give a way to access the
parent index but for some reason the final version limited it to the
current index. We change it to give the current parent if is not
provided and to give the requested parent if provided. Any user of this
function is updated to follow the new implementation.

The patch 2 and 3 are some additional fixes for gcc.
The first one is a fix that register the pxo and cxo fixed clock only if
they are not defined in DTS.
The patch 3 require some explaination. In short is a big HACK to prevent
kernel panic with this series.

The kpss-xcc driver is a mess.
The Documentation declare that the clocks should be provided but for some
reason it was never followed.
In fact in the ipq8064 DTSI only the clocks for l2cc are declared but
for cpu0 and cpu1 the clocks are not defined.
The kpss-xcc driver use parent_names so the clks are ignored and never
used so till now it wasn't a problem (ignoring the fact that they
doesn't follow documentation at all)
On top of that, the l2cc node declare the pxo clock in a really strange
way. It's declared using the PXO_SRC gcc clock that is never defined in
the gcc ipq8064 clock table. (the correct way was to declare a fixed
clock in dts and reference that)
To prevent any kind of problem we use the patch 3 and provide the clk
for PXO_SRC in the gcc clock table. We manually provide the clk after
gcc probe.

Patch 4 is just a minor cleanup where we use the poll macro

Patch 5 is the actually kpss-xcc conversion to parent data

Patch 6-7 should be a fixup of a real conver case

Patch 8 converts the krait-cc to parent_data
Patch 9 give some love to the code with some minor fixup
Patch 10 drop the hardcoded safe sel and use the new
clk_hw_get_parent_index to get the safe parent index.
(also I discovered that the parent order was wrong)

Patch 11 is an additional fixup to force the reset of the muxes even
more.

Patch 12-13 are some additiona taken from the qsdk that were missing in
the upstream driver

Patch 14 converts krait-cc to yaml

Patch 15 add to krait-cc Documentation the L2 clocks

Patch 16 converts the kpss-acc driver to yaml and fix some Documentation
error

Patch 17 convets the kpss-gcc driver to yaml

Patch 18 finally adds all this stuff to the ipq8064 dtsi (and fix the
stupid PXO_SRC phandle)

I tested this series on a ipq8064 SoC by running a cache benchmark test
to make sure the changes are correct and we don't silently cause
regressions. Also I compared the output of the clk_summary every time
and we finally have a sane output where the mux are correctly placed in
the correct parent. (till now we had the cpu aux clock all over the
place, probably never cause problems but who knows.)

v6:
- Move dts patch as last patch
- Address commencts from Rob
- Fix warning from make dtbs_check
v5:
- Address comments from Krzysztof
v4:
- Fix more dt-bindings bog errors
v3:
- Split Documentation files for kpss and krait-cc
v2:
- introduce new API instead of fixing the existing one
- do not reorganize variables in krait-cc
- fix some comments error and improve it
- return better error for patch 7
- fix missing new line on patch 16

Ansuel Smith (18):
  clk: introduce clk_hw_get_index_of_parent new API
  clk: qcom: gcc-ipq806x: skip pxo/cxo fixed clk if already present
  clk: qcom: gcc-ipq806x: add PXO_SRC in clk table
  clk: qcom: clk-hfpll: use poll_timeout macro
  clk: qcom: kpss-xcc: convert to parent data API
  clk: qcom: clk-krait: unlock spin after mux completion
  clk: qcom: clk-krait: add hw_parent check for div2_round_rate
  clk: qcom: krait-cc: convert to parent_data API
  clk: qcom: krait-cc: drop pr_info and register qsb only if needed
  clk: qcom: krait-cc: drop hardcoded safe_sel
  clk: qcom: krait-cc: force sec_mux to QSB
  clk: qcom: clk-krait: add apq/ipq8064 errata workaround
  clk: qcom: clk-krait: add enable disable ops
  dt-bindings: clock: Convert qcom,krait-cc to yaml
  dt-bindings: clock: Add L2 clocks to qcom,krait-cc Documentation
  dt-bindings: arm: msm: Convert kpss-acc driver Documentation to yaml
  dt-bindings: arm: msm: Convert kpss-gcc driver Documentation to yaml
  ARM: dts: qcom: qcom-ipq8064: add missing krait-cc compatible and
    clocks

 .../bindings/arm/msm/qcom,kpss-acc.txt        |  49 -----
 .../bindings/arm/msm/qcom,kpss-acc.yaml       |  94 +++++++++
 .../bindings/arm/msm/qcom,kpss-gcc.txt        |  44 -----
 .../bindings/arm/msm/qcom,kpss-gcc.yaml       |  69 +++++++
 .../bindings/clock/qcom,krait-cc.txt          |  34 ----
 .../bindings/clock/qcom,krait-cc.yaml         |  65 ++++++
 arch/arm/boot/dts/qcom-ipq8064.dtsi           |  24 ++-
 drivers/clk/clk.c                             |  14 ++
 drivers/clk/qcom/clk-hfpll.c                  |  13 +-
 drivers/clk/qcom/clk-krait.c                  |  44 ++++-
 drivers/clk/qcom/clk-krait.h                  |   1 +
 drivers/clk/qcom/gcc-ipq806x.c                |  27 ++-
 drivers/clk/qcom/kpss-xcc.c                   |  25 +--
 drivers/clk/qcom/krait-cc.c                   | 186 ++++++++++--------
 include/linux/clk-provider.h                  |   1 +
 15 files changed, 453 insertions(+), 237 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.txt
 create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml
 delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
 create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
 delete mode 100644 Documentation/devicetree/bindings/clock/qcom,krait-cc.txt
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,krait-cc.yaml

-- 
2.34.1

[PATCH v6 01/18] clk: introduce clk_hw_get_index_of_parent new API

[Ansuel Smith]

Clk can have multiple parents. Some clk may require to get the cached
index of other parent that are not current associated with the clk.
We have clk_hw_get_parent_index() that returns the index of the current
parent but we can't get the index of other parents of the provided clk.
Introduce clk_hw_get_index_of_parent() to get the cached index of the
parent of the provided clk. This permits a direct access of the internal
clk_fetch_parent_index().

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/clk/clk.c            | 14 ++++++++++++++
 include/linux/clk-provider.h |  1 +
 2 files changed, 15 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 8de6a22498e7..bdd70a88394c 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -1726,6 +1726,20 @@ int clk_hw_get_parent_index(struct clk_hw *hw)
 }
 EXPORT_SYMBOL_GPL(clk_hw_get_parent_index);
 
+/**
+ * clk_hw_get_index_of_parent - return the index of the parent clock
+ * @hw: clk_hw associated with the clk being consumed
+ * @parent: clk_hw of the parent to be fetched the index of
+ *
+ * Fetches and returns the index of parent clock provided.
+ * Returns -EINVAL if the given parent index can't be provided.
+ */
+int clk_hw_get_index_of_parent(struct clk_hw *hw, const struct clk_hw *parent)
+{
+	return clk_fetch_parent_index(hw->core, parent->core);
+}
+EXPORT_SYMBOL_GPL(clk_hw_get_index_of_parent);
+
 /*
  * Update the orphan status of @core and all its children.
  */
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 2faa6f7aa8a8..5708c0b3ef1c 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -1198,6 +1198,7 @@ unsigned int clk_hw_get_num_parents(const struct clk_hw *hw);
 struct clk_hw *clk_hw_get_parent(const struct clk_hw *hw);
 struct clk_hw *clk_hw_get_parent_by_index(const struct clk_hw *hw,
 					  unsigned int index);
+int clk_hw_get_index_of_parent(struct clk_hw *hw, const struct clk_hw *parent);
 int clk_hw_get_parent_index(struct clk_hw *hw);
 int clk_hw_set_parent(struct clk_hw *hw, struct clk_hw *new_parent);
 unsigned int __clk_get_enable_count(struct clk *clk);
-- 
2.34.1

[PATCH v6 02/18] clk: qcom: gcc-ipq806x: skip pxo/cxo fixed clk if already present

[Ansuel Smith]

Skip pxo/cxo clock registration if they are already defined in DTS as fixed
clock.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/clk/qcom/gcc-ipq806x.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/qcom/gcc-ipq806x.c b/drivers/clk/qcom/gcc-ipq806x.c
index d6b7adb4be38..27f6d7626abb 100644
--- a/drivers/clk/qcom/gcc-ipq806x.c
+++ b/drivers/clk/qcom/gcc-ipq806x.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/regmap.h>
 #include <linux/reset-controller.h>
@@ -3061,15 +3062,22 @@ static int gcc_ipq806x_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct regmap *regmap;
+	struct clk *clk;
 	int ret;
 
-	ret = qcom_cc_register_board_clk(dev, "cxo_board", "cxo", 25000000);
-	if (ret)
-		return ret;
+	clk = clk_get(dev, "cxo");
+	if (IS_ERR(clk)) {
+		ret = qcom_cc_register_board_clk(dev, "cxo_board", "cxo", 25000000);
+		if (ret)
+			return ret;
+	}
 
-	ret = qcom_cc_register_board_clk(dev, "pxo_board", "pxo", 25000000);
-	if (ret)
-		return ret;
+	clk = clk_get(dev, "pxo");
+	if (IS_ERR(clk)) {
+		ret = qcom_cc_register_board_clk(dev, "pxo_board", "pxo", 25000000);
+		if (ret)
+			return ret;
+	}
 
 	ret = qcom_cc_probe(pdev, &gcc_ipq806x_desc);
 	if (ret)
-- 
2.34.1

[PATCH v6 03/18] clk: qcom: gcc-ipq806x: add PXO_SRC in clk table

[Ansuel Smith]

PXO_SRC is currently defined in the gcc include and referenced in the
ipq8064 DTSI. Correctly provide a clk after gcc probe to fix kernel
panic if a driver starts to actually use it.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/clk/qcom/gcc-ipq806x.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/clk/qcom/gcc-ipq806x.c b/drivers/clk/qcom/gcc-ipq806x.c
index 27f6d7626abb..7271d3afdc89 100644
--- a/drivers/clk/qcom/gcc-ipq806x.c
+++ b/drivers/clk/qcom/gcc-ipq806x.c
@@ -26,6 +26,8 @@
 #include "clk-hfpll.h"
 #include "reset.h"
 
+static struct clk_regmap pxo = { };
+
 static struct clk_pll pll0 = {
 	.l_reg = 0x30c4,
 	.m_reg = 0x30c8,
@@ -2754,6 +2756,7 @@ static struct clk_dyn_rcg ubi32_core2_src_clk = {
 };
 
 static struct clk_regmap *gcc_ipq806x_clks[] = {
+	[PXO_SRC] = NULL,
 	[PLL0] = &pll0.clkr,
 	[PLL0_VOTE] = &pll0_vote,
 	[PLL3] = &pll3.clkr,
@@ -3083,6 +3086,10 @@ static int gcc_ipq806x_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	clk = clk_get(dev, "pxo");
+	pxo.hw = *__clk_get_hw(clk);
+	gcc_ipq806x_clks[PXO_SRC] = &pxo;
+
 	regmap = dev_get_regmap(dev, NULL);
 	if (!regmap)
 		return -ENODEV;
-- 
2.34.1

[PATCH v6 04/18] clk: qcom: clk-hfpll: use poll_timeout macro

[Ansuel Smith]

Use regmap_read_poll_timeout macro instead of do-while structure.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/clk/qcom/clk-hfpll.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/qcom/clk-hfpll.c b/drivers/clk/qcom/clk-hfpll.c
index e847d586a73a..a4e347eb4d4d 100644
--- a/drivers/clk/qcom/clk-hfpll.c
+++ b/drivers/clk/qcom/clk-hfpll.c
@@ -12,6 +12,8 @@
 #include "clk-regmap.h"
 #include "clk-hfpll.h"
 
+#define HFPLL_BUSY_WAIT_TIMEOUT	100
+
 #define PLL_OUTCTRL	BIT(0)
 #define PLL_BYPASSNL	BIT(1)
 #define PLL_RESET_N	BIT(2)
@@ -72,13 +74,12 @@ static void __clk_hfpll_enable(struct clk_hw *hw)
 	regmap_update_bits(regmap, hd->mode_reg, PLL_RESET_N, PLL_RESET_N);
 
 	/* Wait for PLL to lock. */
-	if (hd->status_reg) {
-		do {
-			regmap_read(regmap, hd->status_reg, &val);
-		} while (!(val & BIT(hd->lock_bit)));
-	} else {
+	if (hd->status_reg)
+		regmap_read_poll_timeout(regmap, hd->status_reg, val,
+					 !(val & BIT(hd->lock_bit)), USEC_PER_MSEC * 2,
+					 HFPLL_BUSY_WAIT_TIMEOUT * USEC_PER_MSEC);
+	else
 		udelay(60);
-	}
 
 	/* Enable PLL output. */
 	regmap_update_bits(regmap, hd->mode_reg, PLL_OUTCTRL, PLL_OUTCTRL);
-- 
2.34.1

[PATCH v6 05/18] clk: qcom: kpss-xcc: convert to parent data API

[Ansuel Smith]

Convert the driver to parent data API. From the Documentation pll8_vote
and pxo should be declared in the DTS so fw_name can be used instead of
parent_names. Name is still used to save regression on old definition.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/clk/qcom/kpss-xcc.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/drivers/clk/qcom/kpss-xcc.c b/drivers/clk/qcom/kpss-xcc.c
index 4fec1f9142b8..347f70e9f5fe 100644
--- a/drivers/clk/qcom/kpss-xcc.c
+++ b/drivers/clk/qcom/kpss-xcc.c
@@ -12,9 +12,9 @@
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
 
-static const char *aux_parents[] = {
-	"pll8_vote",
-	"pxo",
+static const struct clk_parent_data aux_parents[] = {
+	{ .name = "pll8_vote", .fw_name = "pll8_vote" },
+	{ .name = "pxo", .fw_name = "pxo" },
 };
 
 static unsigned int aux_parent_map[] = {
@@ -32,8 +32,8 @@ MODULE_DEVICE_TABLE(of, kpss_xcc_match_table);
 static int kpss_xcc_driver_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *id;
-	struct clk *clk;
 	void __iomem *base;
+	struct clk_hw *hw;
 	const char *name;
 
 	id = of_match_device(kpss_xcc_match_table, &pdev->dev);
@@ -55,24 +55,15 @@ static int kpss_xcc_driver_probe(struct platform_device *pdev)
 		base += 0x28;
 	}
 
-	clk = clk_register_mux_table(&pdev->dev, name, aux_parents,
-				     ARRAY_SIZE(aux_parents), 0, base, 0, 0x3,
-				     0, aux_parent_map, NULL);
+	hw = __devm_clk_hw_register_mux(&pdev->dev, NULL, name, ARRAY_SIZE(aux_parents),
+					NULL, NULL, aux_parents, 0, base, 0, 0x3,
+					0, aux_parent_map, NULL);
 
-	platform_set_drvdata(pdev, clk);
-
-	return PTR_ERR_OR_ZERO(clk);
-}
-
-static int kpss_xcc_driver_remove(struct platform_device *pdev)
-{
-	clk_unregister_mux(platform_get_drvdata(pdev));
-	return 0;
+	return PTR_ERR_OR_ZERO(hw);
 }
 
 static struct platform_driver kpss_xcc_driver = {
 	.probe = kpss_xcc_driver_probe,
-	.remove = kpss_xcc_driver_remove,
 	.driver = {
 		.name = "kpss-xcc",
 		.of_match_table = kpss_xcc_match_table,
-- 
2.34.1

[PATCH v6 06/18] clk: qcom: clk-krait: unlock spin after mux completion

[Ansuel Smith]

Unlock spinlock after the mux switch is completed to prevent any corner
case of mux request while the switch still needs to be done.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/clk/qcom/clk-krait.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/clk-krait.c b/drivers/clk/qcom/clk-krait.c
index 59f1af415b58..e447fcc3806d 100644
--- a/drivers/clk/qcom/clk-krait.c
+++ b/drivers/clk/qcom/clk-krait.c
@@ -32,11 +32,12 @@ static void __krait_mux_set_sel(struct krait_mux_clk *mux, int sel)
 		regval |= (sel & mux->mask) << (mux->shift + LPL_SHIFT);
 	}
 	krait_set_l2_indirect_reg(mux->offset, regval);
-	spin_unlock_irqrestore(&krait_clock_reg_lock, flags);
 
 	/* Wait for switch to complete. */
 	mb();
 	udelay(1);
+
+	spin_unlock_irqrestore(&krait_clock_reg_lock, flags);
 }
 
 static int krait_mux_set_parent(struct clk_hw *hw, u8 index)
-- 
2.34.1

[PATCH v6 07/18] clk: qcom: clk-krait: add hw_parent check for div2_round_rate

[Ansuel Smith]

Check if hw_parent is present before calculating the round_rate to
prevent kernel panic. On error -EINVAL is reported.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/clk/qcom/clk-krait.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/clk-krait.c b/drivers/clk/qcom/clk-krait.c
index e447fcc3806d..b6b7650dbf15 100644
--- a/drivers/clk/qcom/clk-krait.c
+++ b/drivers/clk/qcom/clk-krait.c
@@ -80,7 +80,12 @@ EXPORT_SYMBOL_GPL(krait_mux_clk_ops);
 static long krait_div2_round_rate(struct clk_hw *hw, unsigned long rate,
 				  unsigned long *parent_rate)
 {
-	*parent_rate = clk_hw_round_rate(clk_hw_get_parent(hw), rate * 2);
+	struct clk_hw *hw_parent = clk_hw_get_parent(hw);
+
+	if (!hw_parent)
+		return -EINVAL;
+
+	*parent_rate = clk_hw_round_rate(hw_parent, rate * 2);
 	return DIV_ROUND_UP(*parent_rate, 2);
 }
 
-- 
2.34.1

[PATCH v6 08/18] clk: qcom: krait-cc: convert to parent_data API

[Ansuel Smith]

Modernize the krait-cc driver to parent-data API and refactor to drop
any use of clk_names. From Documentation all the required clocks should
be declared in DTS so fw_name can be correctly used to get the parents
for all the muxes. Name is also declared to save compatibility with old
implementation. Also fix the parent order of the sec_mux that was wrong
and incorrectly report the wrong safe parent if it's not hardcoded.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/clk/qcom/krait-cc.c | 126 +++++++++++++++++++-----------------
 1 file changed, 66 insertions(+), 60 deletions(-)

diff --git a/drivers/clk/qcom/krait-cc.c b/drivers/clk/qcom/krait-cc.c
index 4d4b657d33c3..645ad9e8dd73 100644
--- a/drivers/clk/qcom/krait-cc.c
+++ b/drivers/clk/qcom/krait-cc.c
@@ -69,21 +69,22 @@ static int krait_notifier_register(struct device *dev, struct clk *clk,
 	return ret;
 }
 
-static int
+static struct clk *
 krait_add_div(struct device *dev, int id, const char *s, unsigned int offset)
 {
 	struct krait_div2_clk *div;
+	static struct clk_parent_data p_data[1];
 	struct clk_init_data init = {
-		.num_parents = 1,
+		.num_parents = ARRAY_SIZE(p_data),
 		.ops = &krait_div2_clk_ops,
 		.flags = CLK_SET_RATE_PARENT,
 	};
-	const char *p_names[1];
 	struct clk *clk;
+	char *parent_name;
 
 	div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL);
 	if (!div)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	div->width = 2;
 	div->shift = 6;
@@ -93,43 +94,49 @@ krait_add_div(struct device *dev, int id, const char *s, unsigned int offset)
 
 	init.name = kasprintf(GFP_KERNEL, "hfpll%s_div", s);
 	if (!init.name)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
-	init.parent_names = p_names;
-	p_names[0] = kasprintf(GFP_KERNEL, "hfpll%s", s);
-	if (!p_names[0]) {
-		kfree(init.name);
-		return -ENOMEM;
+	init.parent_data = p_data;
+	parent_name = kasprintf(GFP_KERNEL, "hfpll%s", s);
+	if (!parent_name) {
+		clk = ERR_PTR(-ENOMEM);
+		goto err_parent_name;
 	}
 
+	p_data[0].fw_name = parent_name;
+	p_data[0].name = parent_name;
+
 	clk = devm_clk_register(dev, &div->hw);
-	kfree(p_names[0]);
+
+	kfree(parent_name);
+err_parent_name:
 	kfree(init.name);
 
-	return PTR_ERR_OR_ZERO(clk);
+	return clk;
 }
 
-static int
+static struct clk *
 krait_add_sec_mux(struct device *dev, int id, const char *s,
 		  unsigned int offset, bool unique_aux)
 {
 	int ret;
 	struct krait_mux_clk *mux;
-	static const char *sec_mux_list[] = {
-		"acpu_aux",
-		"qsb",
+	static struct clk_parent_data sec_mux_list[2] = {
+		{ .name = "qsb", .fw_name = "qsb" },
+		{},
 	};
 	struct clk_init_data init = {
-		.parent_names = sec_mux_list,
+		.parent_data = sec_mux_list,
 		.num_parents = ARRAY_SIZE(sec_mux_list),
 		.ops = &krait_mux_clk_ops,
 		.flags = CLK_SET_RATE_PARENT,
 	};
 	struct clk *clk;
+	char *parent_name;
 
 	mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
 	if (!mux)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	mux->offset = offset;
 	mux->lpl = id >= 0;
@@ -141,44 +148,51 @@ krait_add_sec_mux(struct device *dev, int id, const char *s,
 
 	init.name = kasprintf(GFP_KERNEL, "krait%s_sec_mux", s);
 	if (!init.name)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	if (unique_aux) {
-		sec_mux_list[0] = kasprintf(GFP_KERNEL, "acpu%s_aux", s);
-		if (!sec_mux_list[0]) {
+		parent_name = kasprintf(GFP_KERNEL, "acpu%s_aux", s);
+		if (!parent_name) {
 			clk = ERR_PTR(-ENOMEM);
 			goto err_aux;
 		}
+		sec_mux_list[1].fw_name = parent_name;
+		sec_mux_list[1].name = parent_name;
+	} else {
+		sec_mux_list[1].name = "apu_aux";
 	}
 
 	clk = devm_clk_register(dev, &mux->hw);
+	if (IS_ERR(clk))
+		goto err_clk;
 
 	ret = krait_notifier_register(dev, clk, mux);
 	if (ret)
-		goto unique_aux;
+		clk = ERR_PTR(ret);
 
-unique_aux:
+err_clk:
 	if (unique_aux)
-		kfree(sec_mux_list[0]);
+		kfree(parent_name);
 err_aux:
 	kfree(init.name);
-	return PTR_ERR_OR_ZERO(clk);
+	return clk;
 }
 
 static struct clk *
-krait_add_pri_mux(struct device *dev, int id, const char *s,
-		  unsigned int offset)
+krait_add_pri_mux(struct device *dev, struct clk *hfpll_div, struct clk *sec_mux,
+		  int id, const char *s, unsigned int offset)
 {
 	int ret;
 	struct krait_mux_clk *mux;
-	const char *p_names[3];
+	static struct clk_parent_data p_data[3];
 	struct clk_init_data init = {
-		.parent_names = p_names,
-		.num_parents = ARRAY_SIZE(p_names),
+		.parent_data = p_data,
+		.num_parents = ARRAY_SIZE(p_data),
 		.ops = &krait_mux_clk_ops,
 		.flags = CLK_SET_RATE_PARENT,
 	};
 	struct clk *clk;
+	char *hfpll_name;
 
 	mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL);
 	if (!mux)
@@ -196,36 +210,29 @@ krait_add_pri_mux(struct device *dev, int id, const char *s,
 	if (!init.name)
 		return ERR_PTR(-ENOMEM);
 
-	p_names[0] = kasprintf(GFP_KERNEL, "hfpll%s", s);
-	if (!p_names[0]) {
+	hfpll_name = kasprintf(GFP_KERNEL, "hfpll%s", s);
+	if (!hfpll_name) {
 		clk = ERR_PTR(-ENOMEM);
-		goto err_p0;
+		goto err_hfpll;
 	}
 
-	p_names[1] = kasprintf(GFP_KERNEL, "hfpll%s_div", s);
-	if (!p_names[1]) {
-		clk = ERR_PTR(-ENOMEM);
-		goto err_p1;
-	}
+	p_data[0].fw_name = hfpll_name;
+	p_data[0].name = hfpll_name;
 
-	p_names[2] = kasprintf(GFP_KERNEL, "krait%s_sec_mux", s);
-	if (!p_names[2]) {
-		clk = ERR_PTR(-ENOMEM);
-		goto err_p2;
-	}
+	p_data[1].hw = __clk_get_hw(hfpll_div);
+	p_data[2].hw = __clk_get_hw(sec_mux);
 
 	clk = devm_clk_register(dev, &mux->hw);
+	if (IS_ERR(clk))
+		goto err_clk;
 
 	ret = krait_notifier_register(dev, clk, mux);
 	if (ret)
-		goto err_p3;
-err_p3:
-	kfree(p_names[2]);
-err_p2:
-	kfree(p_names[1]);
-err_p1:
-	kfree(p_names[0]);
-err_p0:
+		clk = ERR_PTR(ret);
+
+err_clk:
+	kfree(hfpll_name);
+err_hfpll:
 	kfree(init.name);
 	return clk;
 }
@@ -233,11 +240,10 @@ krait_add_pri_mux(struct device *dev, int id, const char *s,
 /* id < 0 for L2, otherwise id == physical CPU number */
 static struct clk *krait_add_clks(struct device *dev, int id, bool unique_aux)
 {
-	int ret;
 	unsigned int offset;
 	void *p = NULL;
 	const char *s;
-	struct clk *clk;
+	struct clk *hfpll_div, *sec_mux, *clk;
 
 	if (id >= 0) {
 		offset = 0x4501 + (0x1000 * id);
@@ -249,19 +255,19 @@ static struct clk *krait_add_clks(struct device *dev, int id, bool unique_aux)
 		s = "_l2";
 	}
 
-	ret = krait_add_div(dev, id, s, offset);
-	if (ret) {
-		clk = ERR_PTR(ret);
+	hfpll_div = krait_add_div(dev, id, s, offset);
+	if (IS_ERR(hfpll_div)) {
+		clk = hfpll_div;
 		goto err;
 	}
 
-	ret = krait_add_sec_mux(dev, id, s, offset, unique_aux);
-	if (ret) {
-		clk = ERR_PTR(ret);
+	sec_mux = krait_add_sec_mux(dev, id, s, offset, unique_aux);
+	if (IS_ERR(sec_mux)) {
+		clk = sec_mux;
 		goto err;
 	}
 
-	clk = krait_add_pri_mux(dev, id, s, offset);
+	clk = krait_add_pri_mux(dev, hfpll_div, sec_mux, id, s, offset);
 err:
 	kfree(p);
 	return clk;
-- 
2.34.1

[PATCH v6 09/18] clk: qcom: krait-cc: drop pr_info and register qsb only if needed

[Ansuel Smith]

Replace pr_info() with dev_info() to provide better diagnostics.
Register qsb fixed clk only if it's not declared in DTS.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/clk/qcom/krait-cc.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/qcom/krait-cc.c b/drivers/clk/qcom/krait-cc.c
index 645ad9e8dd73..e9508e3104ea 100644
--- a/drivers/clk/qcom/krait-cc.c
+++ b/drivers/clk/qcom/krait-cc.c
@@ -308,7 +308,9 @@ static int krait_cc_probe(struct platform_device *pdev)
 		return -ENODEV;
 
 	/* Rate is 1 because 0 causes problems for __clk_mux_determine_rate */
-	clk = clk_register_fixed_rate(dev, "qsb", NULL, 0, 1);
+	if (IS_ERR(clk_get(dev, "qsb")))
+		clk = clk_register_fixed_rate(dev, "qsb", NULL, 0, 1);
+
 	if (IS_ERR(clk))
 		return PTR_ERR(clk);
 
@@ -363,25 +365,25 @@ static int krait_cc_probe(struct platform_device *pdev)
 	cur_rate = clk_get_rate(l2_pri_mux_clk);
 	aux_rate = 384000000;
 	if (cur_rate == 1) {
-		pr_info("L2 @ QSB rate. Forcing new rate.\n");
+		dev_info(dev, "L2 @ QSB rate. Forcing new rate.\n");
 		cur_rate = aux_rate;
 	}
 	clk_set_rate(l2_pri_mux_clk, aux_rate);
 	clk_set_rate(l2_pri_mux_clk, 2);
 	clk_set_rate(l2_pri_mux_clk, cur_rate);
-	pr_info("L2 @ %lu KHz\n", clk_get_rate(l2_pri_mux_clk) / 1000);
+	dev_info(dev, "L2 @ %lu KHz\n", clk_get_rate(l2_pri_mux_clk) / 1000);
 	for_each_possible_cpu(cpu) {
 		clk = clks[cpu];
 		cur_rate = clk_get_rate(clk);
 		if (cur_rate == 1) {
-			pr_info("CPU%d @ QSB rate. Forcing new rate.\n", cpu);
+			dev_info(dev, "CPU%d @ QSB rate. Forcing new rate.\n", cpu);
 			cur_rate = aux_rate;
 		}
 
 		clk_set_rate(clk, aux_rate);
 		clk_set_rate(clk, 2);
 		clk_set_rate(clk, cur_rate);
-		pr_info("CPU%d @ %lu KHz\n", cpu, clk_get_rate(clk) / 1000);
+		dev_info(dev, "CPU%d @ %lu KHz\n", cpu, clk_get_rate(clk) / 1000);
 	}
 
 	of_clk_add_provider(dev->of_node, krait_of_get, clks);
-- 
2.34.1

[PATCH v6 10/18] clk: qcom: krait-cc: drop hardcoded safe_sel

[Ansuel Smith]

Drop hardcoded safe_sel definition and use helper to correctly calculate
it. We assume qsb clk is always present as it should be declared in DTS
per Documentation and in the absence of that, it's declared as a fixed
clk.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/clk/qcom/krait-cc.c | 40 +++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/drivers/clk/qcom/krait-cc.c b/drivers/clk/qcom/krait-cc.c
index e9508e3104ea..5f98ee1c3681 100644
--- a/drivers/clk/qcom/krait-cc.c
+++ b/drivers/clk/qcom/krait-cc.c
@@ -26,6 +26,17 @@ static unsigned int pri_mux_map[] = {
 	0,
 };
 
+static u8 krait_get_mux_sel(struct krait_mux_clk *mux, struct clk *safe_clk)
+{
+	struct clk_hw *safe_hw = __clk_get_hw(safe_clk);
+
+	/*
+	 * We can ignore errors from clk_hw_get_index_of_parent()
+	 * as we create these parents in this driver.
+	 */
+	return clk_hw_get_index_of_parent(&mux->hw, safe_hw);
+}
+
 /*
  * Notifier function for switching the muxes to safe parent
  * while the hfpll is getting reprogrammed.
@@ -116,8 +127,8 @@ krait_add_div(struct device *dev, int id, const char *s, unsigned int offset)
 }
 
 static struct clk *
-krait_add_sec_mux(struct device *dev, int id, const char *s,
-		  unsigned int offset, bool unique_aux)
+krait_add_sec_mux(struct device *dev, struct clk *qsb, int id,
+		  const char *s, unsigned int offset, bool unique_aux)
 {
 	int ret;
 	struct krait_mux_clk *mux;
@@ -144,7 +155,6 @@ krait_add_sec_mux(struct device *dev, int id, const char *s,
 	mux->shift = 2;
 	mux->parent_map = sec_mux_map;
 	mux->hw.init = &init;
-	mux->safe_sel = 0;
 
 	init.name = kasprintf(GFP_KERNEL, "krait%s_sec_mux", s);
 	if (!init.name)
@@ -166,6 +176,7 @@ krait_add_sec_mux(struct device *dev, int id, const char *s,
 	if (IS_ERR(clk))
 		goto err_clk;
 
+	mux->safe_sel = krait_get_mux_sel(mux, qsb);
 	ret = krait_notifier_register(dev, clk, mux);
 	if (ret)
 		clk = ERR_PTR(ret);
@@ -204,7 +215,6 @@ krait_add_pri_mux(struct device *dev, struct clk *hfpll_div, struct clk *sec_mux
 	mux->lpl = id >= 0;
 	mux->parent_map = pri_mux_map;
 	mux->hw.init = &init;
-	mux->safe_sel = 2;
 
 	init.name = kasprintf(GFP_KERNEL, "krait%s_pri_mux", s);
 	if (!init.name)
@@ -226,6 +236,7 @@ krait_add_pri_mux(struct device *dev, struct clk *hfpll_div, struct clk *sec_mux
 	if (IS_ERR(clk))
 		goto err_clk;
 
+	mux->safe_sel = krait_get_mux_sel(mux, sec_mux);
 	ret = krait_notifier_register(dev, clk, mux);
 	if (ret)
 		clk = ERR_PTR(ret);
@@ -238,7 +249,9 @@ krait_add_pri_mux(struct device *dev, struct clk *hfpll_div, struct clk *sec_mux
 }
 
 /* id < 0 for L2, otherwise id == physical CPU number */
-static struct clk *krait_add_clks(struct device *dev, int id, bool unique_aux)
+static struct clk *
+krait_add_clks(struct device *dev, struct clk *qsb, int id,
+	       bool unique_aux)
 {
 	unsigned int offset;
 	void *p = NULL;
@@ -261,7 +274,7 @@ static struct clk *krait_add_clks(struct device *dev, int id, bool unique_aux)
 		goto err;
 	}
 
-	sec_mux = krait_add_sec_mux(dev, id, s, offset, unique_aux);
+	sec_mux = krait_add_sec_mux(dev, qsb, id, s, offset, unique_aux);
 	if (IS_ERR(sec_mux)) {
 		clk = sec_mux;
 		goto err;
@@ -301,18 +314,19 @@ static int krait_cc_probe(struct platform_device *pdev)
 	int cpu;
 	struct clk *clk;
 	struct clk **clks;
-	struct clk *l2_pri_mux_clk;
+	struct clk *l2_pri_mux_clk, *qsb;
 
 	id = of_match_device(krait_cc_match_table, dev);
 	if (!id)
 		return -ENODEV;
 
 	/* Rate is 1 because 0 causes problems for __clk_mux_determine_rate */
-	if (IS_ERR(clk_get(dev, "qsb")))
-		clk = clk_register_fixed_rate(dev, "qsb", NULL, 0, 1);
+	qsb = clk_get(dev, "qsb");
+	if (IS_ERR(qsb))
+		qsb = clk_register_fixed_rate(dev, "qsb", NULL, 0, 1);
 
-	if (IS_ERR(clk))
-		return PTR_ERR(clk);
+	if (IS_ERR(qsb))
+		return PTR_ERR(qsb);
 
 	if (!id->data) {
 		clk = clk_register_fixed_factor(dev, "acpu_aux",
@@ -327,13 +341,13 @@ static int krait_cc_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	for_each_possible_cpu(cpu) {
-		clk = krait_add_clks(dev, cpu, id->data);
+		clk = krait_add_clks(dev, qsb, cpu, id->data);
 		if (IS_ERR(clk))
 			return PTR_ERR(clk);
 		clks[cpu] = clk;
 	}
 
-	l2_pri_mux_clk = krait_add_clks(dev, -1, id->data);
+	l2_pri_mux_clk = krait_add_clks(dev, qsb, -1, id->data);
 	if (IS_ERR(l2_pri_mux_clk))
 		return PTR_ERR(l2_pri_mux_clk);
 	clks[4] = l2_pri_mux_clk;
-- 
2.34.1

[PATCH v6 11/18] clk: qcom: krait-cc: force sec_mux to QSB

[Ansuel Smith]

Now that we have converted every driver to parent_data, it was
notice that the bootloader can't really leave the system in a
strange state where l2 or the cpu0/1 can be sourced in a number of ways
for example cpu1 sourcing out of qsb, l2 sourcing out of pxo.
To correctly reset the mux and the HFPLL force the sec_mux to QSB.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/clk/qcom/krait-cc.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/krait-cc.c b/drivers/clk/qcom/krait-cc.c
index 5f98ee1c3681..299eb4c81d96 100644
--- a/drivers/clk/qcom/krait-cc.c
+++ b/drivers/clk/qcom/krait-cc.c
@@ -15,6 +15,8 @@
 
 #include "clk-krait.h"
 
+#define QSB_RATE	1
+
 static unsigned int sec_mux_map[] = {
 	2,
 	0,
@@ -181,6 +183,13 @@ krait_add_sec_mux(struct device *dev, struct clk *qsb, int id,
 	if (ret)
 		clk = ERR_PTR(ret);
 
+	/*
+	 * Force the sec_mux to be set to QSB rate.
+	 * This is needed to correctly set the parents and
+	 * to later reset mux and HFPLL to a known freq.
+	 */
+	clk_set_rate(clk, QSB_RATE);
+
 err_clk:
 	if (unique_aux)
 		kfree(parent_name);
@@ -378,7 +387,7 @@ static int krait_cc_probe(struct platform_device *pdev)
 	 */
 	cur_rate = clk_get_rate(l2_pri_mux_clk);
 	aux_rate = 384000000;
-	if (cur_rate == 1) {
+	if (cur_rate == QSB_RATE) {
 		dev_info(dev, "L2 @ QSB rate. Forcing new rate.\n");
 		cur_rate = aux_rate;
 	}
@@ -389,7 +398,7 @@ static int krait_cc_probe(struct platform_device *pdev)
 	for_each_possible_cpu(cpu) {
 		clk = clks[cpu];
 		cur_rate = clk_get_rate(clk);
-		if (cur_rate == 1) {
+		if (cur_rate == QSB_RATE) {
 			dev_info(dev, "CPU%d @ QSB rate. Forcing new rate.\n", cpu);
 			cur_rate = aux_rate;
 		}
-- 
2.34.1

[PATCH v6 12/18] clk: qcom: clk-krait: add apq/ipq8064 errata workaround

[Ansuel Smith]

Add apq/ipq8064 errata workaround where the sec_src clock gating needs to
be disabled during switching. To enable this set disable_sec_src_gating
in the mux struct.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/clk/qcom/clk-krait.c | 16 ++++++++++++++++
 drivers/clk/qcom/clk-krait.h |  1 +
 drivers/clk/qcom/krait-cc.c  |  1 +
 3 files changed, 18 insertions(+)

diff --git a/drivers/clk/qcom/clk-krait.c b/drivers/clk/qcom/clk-krait.c
index b6b7650dbf15..7ba5dbc72bce 100644
--- a/drivers/clk/qcom/clk-krait.c
+++ b/drivers/clk/qcom/clk-krait.c
@@ -18,13 +18,23 @@
 static DEFINE_SPINLOCK(krait_clock_reg_lock);
 
 #define LPL_SHIFT	8
+#define SECCLKAGD	BIT(4)
+
 static void __krait_mux_set_sel(struct krait_mux_clk *mux, int sel)
 {
 	unsigned long flags;
 	u32 regval;
 
 	spin_lock_irqsave(&krait_clock_reg_lock, flags);
+
 	regval = krait_get_l2_indirect_reg(mux->offset);
+
+	/* apq/ipq8064 Errata: disable sec_src clock gating during switch. */
+	if (mux->disable_sec_src_gating) {
+		regval |= SECCLKAGD;
+		krait_set_l2_indirect_reg(mux->offset, regval);
+	}
+
 	regval &= ~(mux->mask << mux->shift);
 	regval |= (sel & mux->mask) << mux->shift;
 	if (mux->lpl) {
@@ -33,6 +43,12 @@ static void __krait_mux_set_sel(struct krait_mux_clk *mux, int sel)
 	}
 	krait_set_l2_indirect_reg(mux->offset, regval);
 
+	/* apq/ipq8064 Errata: re-enabled sec_src clock gating. */
+	if (mux->disable_sec_src_gating) {
+		regval &= ~SECCLKAGD;
+		krait_set_l2_indirect_reg(mux->offset, regval);
+	}
+
 	/* Wait for switch to complete. */
 	mb();
 	udelay(1);
diff --git a/drivers/clk/qcom/clk-krait.h b/drivers/clk/qcom/clk-krait.h
index 9120bd2f5297..f930538c539e 100644
--- a/drivers/clk/qcom/clk-krait.h
+++ b/drivers/clk/qcom/clk-krait.h
@@ -15,6 +15,7 @@ struct krait_mux_clk {
 	u8		safe_sel;
 	u8		old_index;
 	bool		reparent;
+	bool		disable_sec_src_gating;
 
 	struct clk_hw	hw;
 	struct notifier_block   clk_nb;
diff --git a/drivers/clk/qcom/krait-cc.c b/drivers/clk/qcom/krait-cc.c
index 299eb4c81d96..cb8b267f1dc5 100644
--- a/drivers/clk/qcom/krait-cc.c
+++ b/drivers/clk/qcom/krait-cc.c
@@ -157,6 +157,7 @@ krait_add_sec_mux(struct device *dev, struct clk *qsb, int id,
 	mux->shift = 2;
 	mux->parent_map = sec_mux_map;
 	mux->hw.init = &init;
+	mux->disable_sec_src_gating = true;
 
 	init.name = kasprintf(GFP_KERNEL, "krait%s_sec_mux", s);
 	if (!init.name)
-- 
2.34.1

[PATCH v6 13/18] clk: qcom: clk-krait: add enable disable ops

[Ansuel Smith]

Add enable/disable ops for krait mux. On disable the mux is set to the
safe selection.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/clk/qcom/clk-krait.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/clk/qcom/clk-krait.c b/drivers/clk/qcom/clk-krait.c
index 7ba5dbc72bce..061af57b0ec2 100644
--- a/drivers/clk/qcom/clk-krait.c
+++ b/drivers/clk/qcom/clk-krait.c
@@ -85,7 +85,25 @@ static u8 krait_mux_get_parent(struct clk_hw *hw)
 	return clk_mux_val_to_index(hw, mux->parent_map, 0, sel);
 }
 
+static int krait_mux_enable(struct clk_hw *hw)
+{
+	struct krait_mux_clk *mux = to_krait_mux_clk(hw);
+
+	__krait_mux_set_sel(mux, mux->en_mask);
+
+	return 0;
+}
+
+static void krait_mux_disable(struct clk_hw *hw)
+{
+	struct krait_mux_clk *mux = to_krait_mux_clk(hw);
+
+	__krait_mux_set_sel(mux, mux->safe_sel);
+}
+
 const struct clk_ops krait_mux_clk_ops = {
+	.enable = krait_mux_enable,
+	.disable = krait_mux_disable,
 	.set_parent = krait_mux_set_parent,
 	.get_parent = krait_mux_get_parent,
 	.determine_rate = __clk_mux_determine_rate_closest,
-- 
2.34.1

[PATCH v6 14/18] dt-bindings: clock: Convert qcom,krait-cc to yaml

[Ansuel Smith]

Convert qcom,krait-cc to yaml Documentation.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../bindings/clock/qcom,krait-cc.txt          | 34 -----------
 .../bindings/clock/qcom,krait-cc.yaml         | 59 +++++++++++++++++++
 2 files changed, 59 insertions(+), 34 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/clock/qcom,krait-cc.txt
 create mode 100644 Documentation/devicetree/bindings/clock/qcom,krait-cc.yaml

diff --git a/Documentation/devicetree/bindings/clock/qcom,krait-cc.txt b/Documentation/devicetree/bindings/clock/qcom,krait-cc.txt
deleted file mode 100644
index 030ba60dab08..000000000000
--- a/Documentation/devicetree/bindings/clock/qcom,krait-cc.txt
+++ /dev/null
@@ -1,34 +0,0 @@
-Krait Clock Controller
-
-PROPERTIES
-
-- compatible:
-	Usage: required
-	Value type: <string>
-	Definition: must be one of:
-			"qcom,krait-cc-v1"
-			"qcom,krait-cc-v2"
-
-- #clock-cells:
-	Usage: required
-	Value type: <u32>
-	Definition: must be 1
-
-- clocks:
-	Usage: required
-	Value type: <prop-encoded-array>
-	Definition: reference to the clock parents of hfpll, secondary muxes.
-
-- clock-names:
-	Usage: required
-	Value type: <stringlist>
-	Definition: must be "hfpll0", "hfpll1", "acpu0_aux", "acpu1_aux", "qsb".
-
-Example:
-
-	kraitcc: clock-controller {
-		compatible = "qcom,krait-cc-v1";
-		clocks = <&hfpll0>, <&hfpll1>, <&acpu0_aux>, <&acpu1_aux>, <qsb>;
-		clock-names = "hfpll0", "hfpll1", "acpu0_aux", "acpu1_aux", "qsb";
-		#clock-cells = <1>;
-	};
diff --git a/Documentation/devicetree/bindings/clock/qcom,krait-cc.yaml b/Documentation/devicetree/bindings/clock/qcom,krait-cc.yaml
new file mode 100644
index 000000000000..e879bfbe67ac
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/qcom,krait-cc.yaml
@@ -0,0 +1,59 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/qcom,krait-cc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Krait Clock Controller
+
+maintainers:
+  - Ansuel Smith <ansuelsmth@gmail.com>
+
+description: |
+  Qualcomm Krait Clock Controller used to correctly scale the CPU and the L2
+  rates.
+
+properties:
+  compatible:
+    enum:
+      - qcom,krait-cc-v1
+      - qcom,krait-cc-v2
+
+  clocks:
+    items:
+      - description: phandle to hfpll for CPU0 mux
+      - description: phandle to hfpll for CPU1 mux
+      - description: phandle to CPU0 aux clock
+      - description: phandle to CPU1 aux clock
+      - description: phandle to QSB fixed clk
+
+  clock-names:
+    items:
+      - const: hfpll0
+      - const: hfpll1
+      - const: acpu0_aux
+      - const: acpu1_aux
+      - const: qsb
+
+  '#clock-cells':
+    const: 1
+
+required:
+  - compatible
+  - clocks
+  - clock-names
+  - '#clock-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    clock-controller {
+      compatible = "qcom,krait-cc-v1";
+      clocks = <&hfpll0>, <&hfpll1>,
+               <&acpu0_aux>, <&acpu1_aux>, <&qsb>;
+      clock-names = "hfpll0", "hfpll1",
+                    "acpu0_aux", "acpu1_aux", "qsb";
+      #clock-cells = <1>;
+    };
+...
-- 
2.34.1

[PATCH v6 15/18] dt-bindings: clock: Add L2 clocks to qcom,krait-cc Documentation

[Ansuel Smith]

Krait-cc qcom driver provide also L2 clocks and require the acpu_l2_aux
and the hfpll_l2 clock to be provided. Add these missing clocks to the
Documentation. The driver keep support for both old (it did already used
these clocks and we keep the same naming scheme) and this new
implementation and should prevent any regression by this fixup.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 .../devicetree/bindings/clock/qcom,krait-cc.yaml       | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/qcom,krait-cc.yaml b/Documentation/devicetree/bindings/clock/qcom,krait-cc.yaml
index e879bfbe67ac..de4320a85764 100644
--- a/Documentation/devicetree/bindings/clock/qcom,krait-cc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,krait-cc.yaml
@@ -26,6 +26,8 @@ properties:
       - description: phandle to CPU0 aux clock
       - description: phandle to CPU1 aux clock
       - description: phandle to QSB fixed clk
+      - description: phandle to hfpll for L2 mux
+      - description: phandle to L2 aux clock
 
   clock-names:
     items:
@@ -34,6 +36,8 @@ properties:
       - const: acpu0_aux
       - const: acpu1_aux
       - const: qsb
+      - const: hfpll_l2
+      - const: acpu_l2_aux
 
   '#clock-cells':
     const: 1
@@ -51,9 +55,11 @@ examples:
     clock-controller {
       compatible = "qcom,krait-cc-v1";
       clocks = <&hfpll0>, <&hfpll1>,
-               <&acpu0_aux>, <&acpu1_aux>, <&qsb>;
+               <&acpu0_aux>, <&acpu1_aux>, <&qsb>,
+               <&hfpll_l2>, <&acpu_l2_aux>;
       clock-names = "hfpll0", "hfpll1",
-                    "acpu0_aux", "acpu1_aux", "qsb";
+                    "acpu0_aux", "acpu1_aux", "qsb",
+                    "hfpll_l2", "acpu_l2_aux";
       #clock-cells = <1>;
     };
 ...
-- 
2.34.1

[PATCH v6 16/18] dt-bindings: arm: msm: Convert kpss-acc driver Documentation to yaml

[Ansuel Smith]

Convert kpss-acc driver Documentation to yaml.
The original Documentation was wrong all along. Fix it while we are
converting it.
The example was wrong as kpss-acc-v2 should only expose the regs but we
don't have any driver that expose additional clocks. The kpss-acc driver
is only specific to v1. For this exact reason, limit all the additional
bindings (clocks, clock-names, clock-output-names and #clock-cells) to
v1 and also flag that these bindings should NOT be used for v2.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 .../bindings/arm/msm/qcom,kpss-acc.txt        | 49 ----------
 .../bindings/arm/msm/qcom,kpss-acc.yaml       | 94 +++++++++++++++++++
 2 files changed, 94 insertions(+), 49 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.txt
 create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml

diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.txt b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.txt
deleted file mode 100644
index 7f696362a4a1..000000000000
--- a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.txt
+++ /dev/null
@@ -1,49 +0,0 @@
-Krait Processor Sub-system (KPSS) Application Clock Controller (ACC)
-
-The KPSS ACC provides clock, power domain, and reset control to a Krait CPU.
-There is one ACC register region per CPU within the KPSS remapped region as
-well as an alias register region that remaps accesses to the ACC associated
-with the CPU accessing the region.
-
-PROPERTIES
-
-- compatible:
-	Usage: required
-	Value type: <string>
-	Definition: should be one of:
-			"qcom,kpss-acc-v1"
-			"qcom,kpss-acc-v2"
-
-- reg:
-	Usage: required
-	Value type: <prop-encoded-array>
-	Definition: the first element specifies the base address and size of
-		    the register region. An optional second element specifies
-		    the base address and size of the alias register region.
-
-- clocks:
-        Usage: required
-        Value type: <prop-encoded-array>
-        Definition: reference to the pll parents.
-
-- clock-names:
-        Usage: required
-        Value type: <stringlist>
-        Definition: must be "pll8_vote", "pxo".
-
-- clock-output-names:
-	Usage: optional
-	Value type: <string>
-	Definition: Name of the output clock. Typically acpuX_aux where X is a
-		    CPU number starting at 0.
-
-Example:
-
-	clock-controller@2088000 {
-		compatible = "qcom,kpss-acc-v2";
-		reg = <0x02088000 0x1000>,
-		      <0x02008000 0x1000>;
-		clocks = <&gcc PLL8_VOTE>, <&gcc PXO_SRC>;
-		clock-names = "pll8_vote", "pxo";
-		clock-output-names = "acpu0_aux";
-	};
diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml
new file mode 100644
index 000000000000..707a81a6c30e
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml
@@ -0,0 +1,94 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/arm/msm/qcom,kpss-acc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Krait Processor Sub-system (KPSS) Application Clock Controller (ACC)
+
+maintainers:
+  - Ansuel Smith <ansuelsmth@gmail.com>
+
+description: |
+  The KPSS ACC provides clock, power domain, and reset control to a Krait CPU.
+  There is one ACC register region per CPU within the KPSS remapped region as
+  well as an alias register region that remaps accesses to the ACC associated
+  with the CPU accessing the region.
+
+properties:
+  compatible:
+    enum:
+      - qcom,kpss-acc-v1
+      - qcom,kpss-acc-v2
+
+  reg:
+    items:
+      - description: Base address and size of the register region
+      - description: Optional base address and size of the alias register region
+
+  clocks:
+    items:
+      - description: phandle to pll8_vote
+      - description: phandle to pxo_board
+
+  clock-names:
+    items:
+      - const: pll8_vote
+      - const: pxo
+
+  clock-output-names:
+    description: Name of the aux clock. Krait can have at most 4 cpu.
+    enum:
+      - acpu0_aux
+      - acpu1_aux
+      - acpu2_aux
+      - acpu3_aux
+
+  '#clock-cells':
+    const: 0
+
+required:
+  - compatible
+  - reg
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: qcom,kpss-acc-v1
+      then:
+        required:
+          - clocks
+          - clock-names
+          - clock-output-names
+          - '#clock-cells'
+    else:
+      properties:
+        clocks: false
+        clock-names: false
+        clock-output-names: false
+        '#clock-cells': false
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,gcc-ipq806x.h>
+
+    clock-controller@2088000 {
+      compatible = "qcom,kpss-acc-v1";
+      reg = <0x02088000 0x1000>, <0x02008000 0x1000>;
+      clocks = <&gcc PLL8_VOTE>, <&pxo_board>;
+      clock-names = "pll8_vote", "pxo";
+      clock-output-names = "acpu0_aux";
+      #clock-cells = <0>;
+    };
+
+  - |
+    clock-controller@f9088000 {
+      compatible = "qcom,kpss-acc-v2";
+      reg = <0xf9088000 0x1000>,
+            <0xf9008000 0x1000>;
+    };
+...
-- 
2.34.1

[PATCH v6 17/18] dt-bindings: arm: msm: Convert kpss-gcc driver Documentation to yaml

[Ansuel Smith]

Convert kpss-gcc driver Documentation to yaml. Since kpss-gcc expose a
clock add the required '#clock-cells' binding while converting it.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 .../bindings/arm/msm/qcom,kpss-gcc.txt        | 44 ------------
 .../bindings/arm/msm/qcom,kpss-gcc.yaml       | 69 +++++++++++++++++++
 2 files changed, 69 insertions(+), 44 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
 create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml

diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
deleted file mode 100644
index e628758950e1..000000000000
--- a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
+++ /dev/null
@@ -1,44 +0,0 @@
-Krait Processor Sub-system (KPSS) Global Clock Controller (GCC)
-
-PROPERTIES
-
-- compatible:
-	Usage: required
-	Value type: <string>
-	Definition: should be one of the following. The generic compatible
-			"qcom,kpss-gcc" should also be included.
-			"qcom,kpss-gcc-ipq8064", "qcom,kpss-gcc"
-			"qcom,kpss-gcc-apq8064", "qcom,kpss-gcc"
-			"qcom,kpss-gcc-msm8974", "qcom,kpss-gcc"
-			"qcom,kpss-gcc-msm8960", "qcom,kpss-gcc"
-
-- reg:
-	Usage: required
-	Value type: <prop-encoded-array>
-	Definition: base address and size of the register region
-
-- clocks:
-	Usage: required
-	Value type: <prop-encoded-array>
-	Definition: reference to the pll parents.
-
-- clock-names:
-	Usage: required
-	Value type: <stringlist>
-	Definition: must be "pll8_vote", "pxo".
-
-- clock-output-names:
-	Usage: required
-	Value type: <string>
-	Definition: Name of the output clock. Typically acpu_l2_aux indicating
-		    an L2 cache auxiliary clock.
-
-Example:
-
-	l2cc: clock-controller@2011000 {
-		compatible = "qcom,kpss-gcc-ipq8064", "qcom,kpss-gcc";
-		reg = <0x2011000 0x1000>;
-		clocks = <&gcc PLL8_VOTE>, <&gcc PXO_SRC>;
-		clock-names = "pll8_vote", "pxo";
-		clock-output-names = "acpu_l2_aux";
-	};
diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
new file mode 100644
index 000000000000..7eb852be02c1
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/arm/msm/qcom,kpss-gcc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Krait Processor Sub-system (KPSS) Global Clock Controller (GCC)
+
+maintainers:
+  - Ansuel Smith <ansuelsmth@gmail.com>
+
+description: |
+  Krait Processor Sub-system (KPSS) Global Clock Controller (GCC). Used
+  to control L2 mux (in the current implementation).
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - qcom,kpss-gcc-ipq8064
+          - qcom,kpss-gcc-apq8064
+          - qcom,kpss-gcc-msm8974
+          - qcom,kpss-gcc-msm8960
+      - const: qcom,kpss-gcc
+      - const: syscon
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: phandle to pll8_vote
+      - description: phandle to pxo_board
+
+  clock-names:
+    items:
+      - const: pll8_vote
+      - const: pxo
+
+  clock-output-names:
+    const: acpu_l2_aux
+
+  '#clock-cells':
+    const: 0
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - clock-output-names
+  - '#clock-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,gcc-ipq806x.h>
+
+    clock-controller@2011000 {
+      compatible = "qcom,kpss-gcc-ipq8064", "qcom,kpss-gcc", "syscon";
+      reg = <0x2011000 0x1000>;
+      clocks = <&gcc PLL8_VOTE>, <&pxo_board>;
+      clock-names = "pll8_vote", "pxo";
+      clock-output-names = "acpu_l2_aux";
+      #clock-cells = <0>;
+    };
+...
+
-- 
2.34.1

[PATCH v6 18/18] ARM: dts: qcom: qcom-ipq8064: add missing krait-cc compatible and clocks

[Ansuel Smith]

Add missing krait-cc clock-controller and define missing aux clock for
CPUs. Also change phandle for l2cc node to point to pxo_board instead
of gcc PXO_SRC. This should align ipq8064 dtsi to the new changes in
the Documentation.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 arch/arm/boot/dts/qcom-ipq8064.dtsi | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/qcom-ipq8064.dtsi b/arch/arm/boot/dts/qcom-ipq8064.dtsi
index 996f4458d9fc..cebb5561f5d1 100644
--- a/arch/arm/boot/dts/qcom-ipq8064.dtsi
+++ b/arch/arm/boot/dts/qcom-ipq8064.dtsi
@@ -468,11 +468,19 @@ IRQ_TYPE_EDGE_RISING)>,
 		acc0: clock-controller@2088000 {
 			compatible = "qcom,kpss-acc-v1";
 			reg = <0x02088000 0x1000>, <0x02008000 0x1000>;
+			clock-output-names = "acpu0_aux";
+			clocks = <&gcc PLL8_VOTE>, <&pxo_board>;
+			clock-names = "pll8_vote", "pxo";
+			#clock-cells = <0>;
 		};
 
 		acc1: clock-controller@2098000 {
 			compatible = "qcom,kpss-acc-v1";
 			reg = <0x02098000 0x1000>, <0x02008000 0x1000>;
+			clock-output-names = "acpu1_aux";
+			clocks = <&gcc PLL8_VOTE>, <&pxo_board>;
+			clock-names = "pll8_vote", "pxo";
+			#clock-cells = <0>;
 		};
 
 		adm_dma: dma-controller@18300000 {
@@ -780,11 +788,23 @@ tcsr: syscon@1a400000 {
 		};
 
 		l2cc: clock-controller@2011000 {
-			compatible = "qcom,kpss-gcc", "syscon";
+			compatible = "qcom,kpss-gcc-ipq8064", "qcom,kpss-gcc", "syscon";
 			reg = <0x2011000 0x1000>;
-			clocks = <&gcc PLL8_VOTE>, <&gcc PXO_SRC>;
+			clocks = <&gcc PLL8_VOTE>, <&pxo_board>;
 			clock-names = "pll8_vote", "pxo";
 			clock-output-names = "acpu_l2_aux";
+			#clock-cells = <0>;
+		};
+
+		kraitcc: clock-controller {
+			compatible = "qcom,krait-cc-v1";
+			clocks = <&gcc PLL9>, <&gcc PLL10>,
+				 <&acc0>, <&acc1>, <&qsb>,
+				 <&gcc PLL12>, <&l2cc>;
+			clock-names = "hfpll0", "hfpll1",
+				      "acpu0_aux", "acpu1_aux", "qsb",
+				      "hfpll_l2", "acpu_l2_aux";
+			#clock-cells = <1>;
 		};
 
 		lcc: clock-controller@28000000 {
-- 
2.34.1

Re: [PATCH v6 17/18] dt-bindings: arm: msm: Convert kpss-gcc driver Documentation to yaml

[Rob Herring]

On Tue, 22 Mar 2022 00:15:47 +0100, Ansuel Smith wrote:
> Convert kpss-gcc driver Documentation to yaml. Since kpss-gcc expose a
> clock add the required '#clock-cells' binding while converting it.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  .../bindings/arm/msm/qcom,kpss-gcc.txt        | 44 ------------
>  .../bindings/arm/msm/qcom,kpss-gcc.yaml       | 69 +++++++++++++++++++
>  2 files changed, 69 insertions(+), 44 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
>  create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
> 

Running 'make dtbs_check' with the schema in this patch gives the
following warnings. Consider if they are expected or the schema is
incorrect. These may not be new warnings.

Note that it is not yet a requirement to have 0 warnings for dtbs_check.
This will change in the future.

Full log is available here: https://patchwork.ozlabs.org/patch/1607962


clock-controller@2011000: '#clock-cells' is a required property
	arch/arm/boot/dts/qcom-ipq8064-ap148.dt.yaml
	arch/arm/boot/dts/qcom-ipq8064-rb3011.dt.yaml

clock-controller@2011000: compatible:0: 'qcom,kpss-gcc' is not one of ['qcom,kpss-gcc-ipq8064', 'qcom,kpss-gcc-apq8064', 'qcom,kpss-gcc-msm8974', 'qcom,kpss-gcc-msm8960']
	arch/arm/boot/dts/qcom-ipq8064-ap148.dt.yaml
	arch/arm/boot/dts/qcom-ipq8064-rb3011.dt.yaml

clock-controller@2011000: compatible:1: 'qcom,kpss-gcc' was expected
	arch/arm/boot/dts/qcom-ipq8064-ap148.dt.yaml
	arch/arm/boot/dts/qcom-ipq8064-rb3011.dt.yaml

clock-controller@2011000: compatible: ['qcom,kpss-gcc', 'syscon'] is too short
	arch/arm/boot/dts/qcom-ipq8064-ap148.dt.yaml
	arch/arm/boot/dts/qcom-ipq8064-rb3011.dt.yaml

Re: [PATCH v6 00/18] Modernize rest of the krait drivers

[Rob Herring]

On Mon, Mar 21, 2022 at 6:45 PM Ansuel Smith <ansuelsmth@gmail.com> wrote:
>
> This is a follow-up to the ipq806x gcc modernize series. Manu cleanup
> changes and also some discoveries of wrong definition notice only with
> all these conversions.
>
> The first patch is an improvement of the clk_hw_get_parent_index. The
> original idea of clk_hw_get_parent_index was to give a way to access the
> parent index but for some reason the final version limited it to the
> current index. We change it to give the current parent if is not
> provided and to give the requested parent if provided. Any user of this
> function is updated to follow the new implementation.
>
> The patch 2 and 3 are some additional fixes for gcc.
> The first one is a fix that register the pxo and cxo fixed clock only if
> they are not defined in DTS.
> The patch 3 require some explaination. In short is a big HACK to prevent
> kernel panic with this series.
>
> The kpss-xcc driver is a mess.
> The Documentation declare that the clocks should be provided but for some
> reason it was never followed.
> In fact in the ipq8064 DTSI only the clocks for l2cc are declared but
> for cpu0 and cpu1 the clocks are not defined.
> The kpss-xcc driver use parent_names so the clks are ignored and never
> used so till now it wasn't a problem (ignoring the fact that they
> doesn't follow documentation at all)
> On top of that, the l2cc node declare the pxo clock in a really strange
> way. It's declared using the PXO_SRC gcc clock that is never defined in
> the gcc ipq8064 clock table. (the correct way was to declare a fixed
> clock in dts and reference that)
> To prevent any kind of problem we use the patch 3 and provide the clk
> for PXO_SRC in the gcc clock table. We manually provide the clk after
> gcc probe.
>
> Patch 4 is just a minor cleanup where we use the poll macro
>
> Patch 5 is the actually kpss-xcc conversion to parent data
>
> Patch 6-7 should be a fixup of a real conver case
>
> Patch 8 converts the krait-cc to parent_data
> Patch 9 give some love to the code with some minor fixup
> Patch 10 drop the hardcoded safe sel and use the new
> clk_hw_get_parent_index to get the safe parent index.
> (also I discovered that the parent order was wrong)
>
> Patch 11 is an additional fixup to force the reset of the muxes even
> more.
>
> Patch 12-13 are some additiona taken from the qsdk that were missing in
> the upstream driver
>
> Patch 14 converts krait-cc to yaml
>
> Patch 15 add to krait-cc Documentation the L2 clocks
>
> Patch 16 converts the kpss-acc driver to yaml and fix some Documentation
> error
>
> Patch 17 convets the kpss-gcc driver to yaml
>
> Patch 18 finally adds all this stuff to the ipq8064 dtsi (and fix the
> stupid PXO_SRC phandle)
>
> I tested this series on a ipq8064 SoC by running a cache benchmark test
> to make sure the changes are correct and we don't silently cause
> regressions. Also I compared the output of the clk_summary every time
> and we finally have a sane output where the mux are correctly placed in
> the correct parent. (till now we had the cpu aux clock all over the
> place, probably never cause problems but who knows.)
>
> v6:
> - Move dts patch as last patch
> - Address commencts from Rob
> - Fix warning from make dtbs_check
> v5:
> - Address comments from Krzysztof
> v4:
> - Fix more dt-bindings bog errors
> v3:
> - Split Documentation files for kpss and krait-cc
> v2:
> - introduce new API instead of fixing the existing one
> - do not reorganize variables in krait-cc
> - fix some comments error and improve it
> - return better error for patch 7
> - fix missing new line on patch 16

6 versions in a week is too many, especially with the merge window
starting. Please give some time for review of all the patches and by
more than one person. It doesn't look like any of the clk driver
patches have been reviewed since v1 for example.

Rob

Re: [PATCH v6 18/18] ARM: dts: qcom: qcom-ipq8064: add missing krait-cc compatible and clocks

[Krzysztof Kozlowski]

On 22/03/2022 00:15, Ansuel Smith wrote:
> Add missing krait-cc clock-controller and define missing aux clock for
> CPUs. Also change phandle for l2cc node to point to pxo_board instead
> of gcc PXO_SRC. This should align ipq8064 dtsi to the new changes in
> the Documentation.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  arch/arm/boot/dts/qcom-ipq8064.dtsi | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 

It should not be the last patch in series because now it is impossible
to judge whether you actually fixed Rob's robot reports or not. You
still have warnings as answer to patch #17.


Best regards,
Krzysztof

Re: [PATCH v6 17/18] dt-bindings: arm: msm: Convert kpss-gcc driver Documentation to yaml

[Krzysztof Kozlowski]

On 22/03/2022 00:15, Ansuel Smith wrote:
> Convert kpss-gcc driver Documentation to yaml. Since kpss-gcc expose a
> clock add the required '#clock-cells' binding while converting it.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  .../bindings/arm/msm/qcom,kpss-gcc.txt        | 44 ------------
>  .../bindings/arm/msm/qcom,kpss-gcc.yaml       | 69 +++++++++++++++++++
>  2 files changed, 69 insertions(+), 44 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
>  create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
> deleted file mode 100644
> index e628758950e1..000000000000
> --- a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
> +++ /dev/null
> @@ -1,44 +0,0 @@
> -Krait Processor Sub-system (KPSS) Global Clock Controller (GCC)
> -
> -PROPERTIES
> -
> -- compatible:
> -	Usage: required
> -	Value type: <string>
> -	Definition: should be one of the following. The generic compatible
> -			"qcom,kpss-gcc" should also be included.
> -			"qcom,kpss-gcc-ipq8064", "qcom,kpss-gcc"
> -			"qcom,kpss-gcc-apq8064", "qcom,kpss-gcc"
> -			"qcom,kpss-gcc-msm8974", "qcom,kpss-gcc"
> -			"qcom,kpss-gcc-msm8960", "qcom,kpss-gcc"
> -
> -- reg:
> -	Usage: required
> -	Value type: <prop-encoded-array>
> -	Definition: base address and size of the register region
> -
> -- clocks:
> -	Usage: required
> -	Value type: <prop-encoded-array>
> -	Definition: reference to the pll parents.
> -
> -- clock-names:
> -	Usage: required
> -	Value type: <stringlist>
> -	Definition: must be "pll8_vote", "pxo".
> -
> -- clock-output-names:
> -	Usage: required
> -	Value type: <string>
> -	Definition: Name of the output clock. Typically acpu_l2_aux indicating
> -		    an L2 cache auxiliary clock.
> -
> -Example:
> -
> -	l2cc: clock-controller@2011000 {
> -		compatible = "qcom,kpss-gcc-ipq8064", "qcom,kpss-gcc";
> -		reg = <0x2011000 0x1000>;
> -		clocks = <&gcc PLL8_VOTE>, <&gcc PXO_SRC>;
> -		clock-names = "pll8_vote", "pxo";
> -		clock-output-names = "acpu_l2_aux";
> -	};
> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
> new file mode 100644
> index 000000000000..7eb852be02c1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/arm/msm/qcom,kpss-gcc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Krait Processor Sub-system (KPSS) Global Clock Controller (GCC)
> +
> +maintainers:
> +  - Ansuel Smith <ansuelsmth@gmail.com>
> +
> +description: |
> +  Krait Processor Sub-system (KPSS) Global Clock Controller (GCC). Used
> +  to control L2 mux (in the current implementation).
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - qcom,kpss-gcc-ipq8064
> +          - qcom,kpss-gcc-apq8064
> +          - qcom,kpss-gcc-msm8974
> +          - qcom,kpss-gcc-msm8960
> +      - const: qcom,kpss-gcc
> +      - const: syscon

There was no syscon here before. This is not explained in commit msg or
patch history, while I asked to document explicitly any deviation from
the conversion.

This is not how the process works. You keep making silent/hidden changes
to the bindings and to the patch submission process. It's difficult to
review and it is even more difficult to trust you that you implement
what we ask for. You keep resending versions of the patchset the same
day (two versions yesterday, shortly after another one) which does not
give time to react and review. Plus then you hide some more changes to
regular conversion without explaining them.

NAK. It's really bad process. :(


Best regards,
Krzysztof

Re: [PATCH v6 16/18] dt-bindings: arm: msm: Convert kpss-acc driver Documentation to yaml

[Krzysztof Kozlowski]

On 22/03/2022 00:15, Ansuel Smith wrote:
> Convert kpss-acc driver Documentation to yaml.
> The original Documentation was wrong all along. Fix it while we are
> converting it.
> The example was wrong as kpss-acc-v2 should only expose the regs but we
> don't have any driver that expose additional clocks. The kpss-acc driver
> is only specific to v1. For this exact reason, limit all the additional
> bindings (clocks, clock-names, clock-output-names and #clock-cells) to
> v1 and also flag that these bindings should NOT be used for v2.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  .../bindings/arm/msm/qcom,kpss-acc.txt        | 49 ----------
>  .../bindings/arm/msm/qcom,kpss-acc.yaml       | 94 +++++++++++++++++++
>  2 files changed, 94 insertions(+), 49 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.txt
>  create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml
> 


Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>


Best regards,
Krzysztof

Re: [PATCH v6 17/18] dt-bindings: arm: msm: Convert kpss-gcc driver Documentation to yaml

[Ansuel Smith]

On Tue, Mar 22, 2022 at 11:07:26AM +0100, Krzysztof Kozlowski wrote:
> On 22/03/2022 00:15, Ansuel Smith wrote:
> > Convert kpss-gcc driver Documentation to yaml. Since kpss-gcc expose a
> > clock add the required '#clock-cells' binding while converting it.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >  .../bindings/arm/msm/qcom,kpss-gcc.txt        | 44 ------------
> >  .../bindings/arm/msm/qcom,kpss-gcc.yaml       | 69 +++++++++++++++++++
> >  2 files changed, 69 insertions(+), 44 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
> >  create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
> > deleted file mode 100644
> > index e628758950e1..000000000000
> > --- a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
> > +++ /dev/null
> > @@ -1,44 +0,0 @@
> > -Krait Processor Sub-system (KPSS) Global Clock Controller (GCC)
> > -
> > -PROPERTIES
> > -
> > -- compatible:
> > -	Usage: required
> > -	Value type: <string>
> > -	Definition: should be one of the following. The generic compatible
> > -			"qcom,kpss-gcc" should also be included.
> > -			"qcom,kpss-gcc-ipq8064", "qcom,kpss-gcc"
> > -			"qcom,kpss-gcc-apq8064", "qcom,kpss-gcc"
> > -			"qcom,kpss-gcc-msm8974", "qcom,kpss-gcc"
> > -			"qcom,kpss-gcc-msm8960", "qcom,kpss-gcc"
> > -
> > -- reg:
> > -	Usage: required
> > -	Value type: <prop-encoded-array>
> > -	Definition: base address and size of the register region
> > -
> > -- clocks:
> > -	Usage: required
> > -	Value type: <prop-encoded-array>
> > -	Definition: reference to the pll parents.
> > -
> > -- clock-names:
> > -	Usage: required
> > -	Value type: <stringlist>
> > -	Definition: must be "pll8_vote", "pxo".
> > -
> > -- clock-output-names:
> > -	Usage: required
> > -	Value type: <string>
> > -	Definition: Name of the output clock. Typically acpu_l2_aux indicating
> > -		    an L2 cache auxiliary clock.
> > -
> > -Example:
> > -
> > -	l2cc: clock-controller@2011000 {
> > -		compatible = "qcom,kpss-gcc-ipq8064", "qcom,kpss-gcc";
> > -		reg = <0x2011000 0x1000>;
> > -		clocks = <&gcc PLL8_VOTE>, <&gcc PXO_SRC>;
> > -		clock-names = "pll8_vote", "pxo";
> > -		clock-output-names = "acpu_l2_aux";
> > -	};
> > diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
> > new file mode 100644
> > index 000000000000..7eb852be02c1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
> > @@ -0,0 +1,69 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/arm/msm/qcom,kpss-gcc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Krait Processor Sub-system (KPSS) Global Clock Controller (GCC)
> > +
> > +maintainers:
> > +  - Ansuel Smith <ansuelsmth@gmail.com>
> > +
> > +description: |
> > +  Krait Processor Sub-system (KPSS) Global Clock Controller (GCC). Used
> > +  to control L2 mux (in the current implementation).
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - qcom,kpss-gcc-ipq8064
> > +          - qcom,kpss-gcc-apq8064
> > +          - qcom,kpss-gcc-msm8974
> > +          - qcom,kpss-gcc-msm8960
> > +      - const: qcom,kpss-gcc
> > +      - const: syscon
> 
> There was no syscon here before. This is not explained in commit msg or
> patch history, while I asked to document explicitly any deviation from
> the conversion.
> 
> This is not how the process works. You keep making silent/hidden changes
> to the bindings and to the patch submission process. It's difficult to
> review and it is even more difficult to trust you that you implement
> what we ask for. You keep resending versions of the patchset the same
> day (two versions yesterday, shortly after another one) which does not
> give time to react and review. Plus then you hide some more changes to
> regular conversion without explaining them.
> 
> NAK. It's really bad process. :(
> 
> 
> Best regards,
> Krzysztof

The thing is that i'm trying to fix all the mess of years of keeping bad
Documentation and having dts that never followed Documentation. It's
really nothing silent/hidden. You add review tag to a patch? That won't
change. The bot alert me of some bugs? I push another revision with the
bug fixed. (I understand I should not send that much revision in the
same day but still considering the slow process of reviewing the c code,
I prefer to keep the Documentation part correct and ready)

If you notice the changes across the different patch, it's very minimal
and 99% of it has not changed. Nothing silent just me addressing warning
from the bot. About the trust issue...
Is it really a syscon addition that bad? Again the original
Documentation was just bad so why should we care to have a 100% 1:1
conversion if it should have been not accepted in the first place.
The addition of this new syscon is because in the current dtsi it's
there and I assume it's there as this is a global accessor and probably
other driver would access the same regs (so it's also a syscon)

I understand the complain about putting too much revision... But NAK
this cause I'm trying to fix all this mess just because more and more
problems are coming up and I'm trying to fix them. It's a bit sad.
Hope you can understand that it's not my interest to push silent changes
or other nasty stuff. It's just me fixing the mess and trying to at
least have the Documentation part ready while I wait for c code review.

-- 
	Ansuel

Re: [PATCH v6 00/18] Modernize rest of the krait drivers

[Ansuel Smith]

On Mon, Mar 21, 2022 at 08:56:13PM -0500, Rob Herring wrote:
> On Mon, Mar 21, 2022 at 6:45 PM Ansuel Smith <ansuelsmth@gmail.com> wrote:
> >
> > This is a follow-up to the ipq806x gcc modernize series. Manu cleanup
> > changes and also some discoveries of wrong definition notice only with
> > all these conversions.
> >
> > The first patch is an improvement of the clk_hw_get_parent_index. The
> > original idea of clk_hw_get_parent_index was to give a way to access the
> > parent index but for some reason the final version limited it to the
> > current index. We change it to give the current parent if is not
> > provided and to give the requested parent if provided. Any user of this
> > function is updated to follow the new implementation.
> >
> > The patch 2 and 3 are some additional fixes for gcc.
> > The first one is a fix that register the pxo and cxo fixed clock only if
> > they are not defined in DTS.
> > The patch 3 require some explaination. In short is a big HACK to prevent
> > kernel panic with this series.
> >
> > The kpss-xcc driver is a mess.
> > The Documentation declare that the clocks should be provided but for some
> > reason it was never followed.
> > In fact in the ipq8064 DTSI only the clocks for l2cc are declared but
> > for cpu0 and cpu1 the clocks are not defined.
> > The kpss-xcc driver use parent_names so the clks are ignored and never
> > used so till now it wasn't a problem (ignoring the fact that they
> > doesn't follow documentation at all)
> > On top of that, the l2cc node declare the pxo clock in a really strange
> > way. It's declared using the PXO_SRC gcc clock that is never defined in
> > the gcc ipq8064 clock table. (the correct way was to declare a fixed
> > clock in dts and reference that)
> > To prevent any kind of problem we use the patch 3 and provide the clk
> > for PXO_SRC in the gcc clock table. We manually provide the clk after
> > gcc probe.
> >
> > Patch 4 is just a minor cleanup where we use the poll macro
> >
> > Patch 5 is the actually kpss-xcc conversion to parent data
> >
> > Patch 6-7 should be a fixup of a real conver case
> >
> > Patch 8 converts the krait-cc to parent_data
> > Patch 9 give some love to the code with some minor fixup
> > Patch 10 drop the hardcoded safe sel and use the new
> > clk_hw_get_parent_index to get the safe parent index.
> > (also I discovered that the parent order was wrong)
> >
> > Patch 11 is an additional fixup to force the reset of the muxes even
> > more.
> >
> > Patch 12-13 are some additiona taken from the qsdk that were missing in
> > the upstream driver
> >
> > Patch 14 converts krait-cc to yaml
> >
> > Patch 15 add to krait-cc Documentation the L2 clocks
> >
> > Patch 16 converts the kpss-acc driver to yaml and fix some Documentation
> > error
> >
> > Patch 17 convets the kpss-gcc driver to yaml
> >
> > Patch 18 finally adds all this stuff to the ipq8064 dtsi (and fix the
> > stupid PXO_SRC phandle)
> >
> > I tested this series on a ipq8064 SoC by running a cache benchmark test
> > to make sure the changes are correct and we don't silently cause
> > regressions. Also I compared the output of the clk_summary every time
> > and we finally have a sane output where the mux are correctly placed in
> > the correct parent. (till now we had the cpu aux clock all over the
> > place, probably never cause problems but who knows.)
> >
> > v6:
> > - Move dts patch as last patch
> > - Address commencts from Rob
> > - Fix warning from make dtbs_check
> > v5:
> > - Address comments from Krzysztof
> > v4:
> > - Fix more dt-bindings bog errors
> > v3:
> > - Split Documentation files for kpss and krait-cc
> > v2:
> > - introduce new API instead of fixing the existing one
> > - do not reorganize variables in krait-cc
> > - fix some comments error and improve it
> > - return better error for patch 7
> > - fix missing new line on patch 16
> 
> 6 versions in a week is too many, especially with the merge window
> starting. Please give some time for review of all the patches and by
> more than one person. It doesn't look like any of the clk driver
> patches have been reviewed since v1 for example.
> 
> Rob

Yes sorry. There was an initial review for the clk driver from v1 to
v2 but nothing else. I was trying to make the Documentation ready while
I wait for a second review of the clk code.

Will wait for clk code review to send v7 hoping it will be the final
version.

-- 
	Ansuel

Re: [PATCH v6 17/18] dt-bindings: arm: msm: Convert kpss-gcc driver Documentation to yaml

[Krzysztof Kozlowski]

On 22/03/2022 14:26, Ansuel Smith wrote:
> On Tue, Mar 22, 2022 at 11:07:26AM +0100, Krzysztof Kozlowski wrote:
>> On 22/03/2022 00:15, Ansuel Smith wrote:
>>> Convert kpss-gcc driver Documentation to yaml. Since kpss-gcc expose a
>>> clock add the required '#clock-cells' binding while converting it.
>>>
>>> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
>>> ---
>>>  .../bindings/arm/msm/qcom,kpss-gcc.txt        | 44 ------------
>>>  .../bindings/arm/msm/qcom,kpss-gcc.yaml       | 69 +++++++++++++++++++
>>>  2 files changed, 69 insertions(+), 44 deletions(-)
>>>  delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
>>>  create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
>>> deleted file mode 100644
>>> index e628758950e1..000000000000
>>> --- a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
>>> +++ /dev/null
>>> @@ -1,44 +0,0 @@
>>> -Krait Processor Sub-system (KPSS) Global Clock Controller (GCC)
>>> -
>>> -PROPERTIES
>>> -
>>> -- compatible:
>>> -	Usage: required
>>> -	Value type: <string>
>>> -	Definition: should be one of the following. The generic compatible
>>> -			"qcom,kpss-gcc" should also be included.
>>> -			"qcom,kpss-gcc-ipq8064", "qcom,kpss-gcc"
>>> -			"qcom,kpss-gcc-apq8064", "qcom,kpss-gcc"
>>> -			"qcom,kpss-gcc-msm8974", "qcom,kpss-gcc"
>>> -			"qcom,kpss-gcc-msm8960", "qcom,kpss-gcc"
>>> -
>>> -- reg:
>>> -	Usage: required
>>> -	Value type: <prop-encoded-array>
>>> -	Definition: base address and size of the register region
>>> -
>>> -- clocks:
>>> -	Usage: required
>>> -	Value type: <prop-encoded-array>
>>> -	Definition: reference to the pll parents.
>>> -
>>> -- clock-names:
>>> -	Usage: required
>>> -	Value type: <stringlist>
>>> -	Definition: must be "pll8_vote", "pxo".
>>> -
>>> -- clock-output-names:
>>> -	Usage: required
>>> -	Value type: <string>
>>> -	Definition: Name of the output clock. Typically acpu_l2_aux indicating
>>> -		    an L2 cache auxiliary clock.
>>> -
>>> -Example:
>>> -
>>> -	l2cc: clock-controller@2011000 {
>>> -		compatible = "qcom,kpss-gcc-ipq8064", "qcom,kpss-gcc";
>>> -		reg = <0x2011000 0x1000>;
>>> -		clocks = <&gcc PLL8_VOTE>, <&gcc PXO_SRC>;
>>> -		clock-names = "pll8_vote", "pxo";
>>> -		clock-output-names = "acpu_l2_aux";
>>> -	};
>>> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
>>> new file mode 100644
>>> index 000000000000..7eb852be02c1
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
>>> @@ -0,0 +1,69 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/arm/msm/qcom,kpss-gcc.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Krait Processor Sub-system (KPSS) Global Clock Controller (GCC)
>>> +
>>> +maintainers:
>>> +  - Ansuel Smith <ansuelsmth@gmail.com>
>>> +
>>> +description: |
>>> +  Krait Processor Sub-system (KPSS) Global Clock Controller (GCC). Used
>>> +  to control L2 mux (in the current implementation).
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - enum:
>>> +          - qcom,kpss-gcc-ipq8064
>>> +          - qcom,kpss-gcc-apq8064
>>> +          - qcom,kpss-gcc-msm8974
>>> +          - qcom,kpss-gcc-msm8960
>>> +      - const: qcom,kpss-gcc
>>> +      - const: syscon
>>
>> There was no syscon here before. This is not explained in commit msg or
>> patch history, while I asked to document explicitly any deviation from
>> the conversion.
>>
>> This is not how the process works. You keep making silent/hidden changes
>> to the bindings and to the patch submission process. It's difficult to
>> review and it is even more difficult to trust you that you implement
>> what we ask for. You keep resending versions of the patchset the same
>> day (two versions yesterday, shortly after another one) which does not
>> give time to react and review. Plus then you hide some more changes to
>> regular conversion without explaining them.
>>
>> NAK. It's really bad process. :(
>>
>>
>> Best regards,
>> Krzysztof
> 
> The thing is that i'm trying to fix all the mess of years of keeping bad
> Documentation and having dts that never followed Documentation. It's
> really nothing silent/hidden. You add review tag to a patch? That won't
> change. The bot alert me of some bugs? I push another revision with the
> bug fixed. 

It does not necessarily mean that bindings are bad and such changes
should be documented.

> (I understand I should not send that much revision in the
> same day but still considering the slow process of reviewing the c code,
> I prefer to keep the Documentation part correct and ready)

Rob also pointed to this - sending two versions of this huge patchset
the same day is way too much.

> 
> If you notice the changes across the different patch, it's very minimal
> and 99% of it has not changed. Nothing silent just me addressing warning
> from the bot. About the trust issue...
> Is it really a syscon addition that bad? Again the original
> Documentation was just bad so why should we care to have a 100% 1:1
> conversion if it should have been not accepted in the first place.

Does not have to be 100% but deviations should be either expected or
explained. Bindings are used also outside of Linux kernel.

> The addition of this new syscon is because in the current dtsi it's
> there and I assume it's there as this is a global accessor and probably
> other driver would access the same regs (so it's also a syscon)

If these are assumptions, then they need to be checked. If these were
new bindings, we would discuss/check the need of syscon. Now we do not
question existing properties, because they were accepted. But syscon
compatible was not accepted, so putting it here requires our acknowledgment.

The bindings are probably pure junk, so this is not merely a conversion
how you wrote in commit msg. This is rework of the bindings. Don't hide
rework under "conversion". Conversion is TXT->YAML without any changes...

I asked about this before and the only part you added to commit msg was
"clock-cells". And now I see syscon - so isn't it a bit surprising?

> 
> I understand the complain about putting too much revision... But NAK
> this cause I'm trying to fix all this mess just because more and more
> problems are coming up and I'm trying to fix them. It's a bit sad.

Why you cannot test your changes and fix them all before sending sixth
version? Why the bot has to test your code, not you?

> Hope you can understand that it's not my interest to push silent changes
> or other nasty stuff. It's just me fixing the mess and trying to at
> least have the Documentation part ready while I wait for c code review.
Best regards,
Krzysztof

Re: [PATCH v6 17/18] dt-bindings: arm: msm: Convert kpss-gcc driver Documentation to yaml

[Ansuel Smith]

On Mon, Mar 21, 2022 at 08:50:51PM -0500, Rob Herring wrote:
> On Tue, 22 Mar 2022 00:15:47 +0100, Ansuel Smith wrote:
> > Convert kpss-gcc driver Documentation to yaml. Since kpss-gcc expose a
> > clock add the required '#clock-cells' binding while converting it.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >  .../bindings/arm/msm/qcom,kpss-gcc.txt        | 44 ------------
> >  .../bindings/arm/msm/qcom,kpss-gcc.yaml       | 69 +++++++++++++++++++
> >  2 files changed, 69 insertions(+), 44 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
> >  create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
> > 
> 
> Running 'make dtbs_check' with the schema in this patch gives the
> following warnings. Consider if they are expected or the schema is
> incorrect. These may not be new warnings.
> 
> Note that it is not yet a requirement to have 0 warnings for dtbs_check.
> This will change in the future.
> 
> Full log is available here: https://patchwork.ozlabs.org/patch/1607962
> 
> 
> clock-controller@2011000: '#clock-cells' is a required property
> 	arch/arm/boot/dts/qcom-ipq8064-ap148.dt.yaml
> 	arch/arm/boot/dts/qcom-ipq8064-rb3011.dt.yaml
> 
> clock-controller@2011000: compatible:0: 'qcom,kpss-gcc' is not one of ['qcom,kpss-gcc-ipq8064', 'qcom,kpss-gcc-apq8064', 'qcom,kpss-gcc-msm8974', 'qcom,kpss-gcc-msm8960']
> 	arch/arm/boot/dts/qcom-ipq8064-ap148.dt.yaml
> 	arch/arm/boot/dts/qcom-ipq8064-rb3011.dt.yaml
> 
> clock-controller@2011000: compatible:1: 'qcom,kpss-gcc' was expected
> 	arch/arm/boot/dts/qcom-ipq8064-ap148.dt.yaml
> 	arch/arm/boot/dts/qcom-ipq8064-rb3011.dt.yaml
> 
> clock-controller@2011000: compatible: ['qcom,kpss-gcc', 'syscon'] is too short
> 	arch/arm/boot/dts/qcom-ipq8064-ap148.dt.yaml
> 	arch/arm/boot/dts/qcom-ipq8064-rb3011.dt.yaml
> 

Sorry for the very stupid question but it's something i'm searching for
a bit now... I can't really find Documentation or a guide on how to
check single yaml and dts instead of using the make command and check
everything. Am I missing something or this is not supported?

-- 
	Ansuel

Re: [PATCH v6 17/18] dt-bindings: arm: msm: Convert kpss-gcc driver Documentation to yaml

[Ansuel Smith]

On Wed, Mar 23, 2022 at 10:59:39AM +0100, Krzysztof Kozlowski wrote:
> On 22/03/2022 14:26, Ansuel Smith wrote:
> > On Tue, Mar 22, 2022 at 11:07:26AM +0100, Krzysztof Kozlowski wrote:
> >> On 22/03/2022 00:15, Ansuel Smith wrote:
> >>> Convert kpss-gcc driver Documentation to yaml. Since kpss-gcc expose a
> >>> clock add the required '#clock-cells' binding while converting it.
> >>>
> >>> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> >>> ---
> >>>  .../bindings/arm/msm/qcom,kpss-gcc.txt        | 44 ------------
> >>>  .../bindings/arm/msm/qcom,kpss-gcc.yaml       | 69 +++++++++++++++++++
> >>>  2 files changed, 69 insertions(+), 44 deletions(-)
> >>>  delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
> >>>  create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
> >>> deleted file mode 100644
> >>> index e628758950e1..000000000000
> >>> --- a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
> >>> +++ /dev/null
> >>> @@ -1,44 +0,0 @@
> >>> -Krait Processor Sub-system (KPSS) Global Clock Controller (GCC)
> >>> -
> >>> -PROPERTIES
> >>> -
> >>> -- compatible:
> >>> -	Usage: required
> >>> -	Value type: <string>
> >>> -	Definition: should be one of the following. The generic compatible
> >>> -			"qcom,kpss-gcc" should also be included.
> >>> -			"qcom,kpss-gcc-ipq8064", "qcom,kpss-gcc"
> >>> -			"qcom,kpss-gcc-apq8064", "qcom,kpss-gcc"
> >>> -			"qcom,kpss-gcc-msm8974", "qcom,kpss-gcc"
> >>> -			"qcom,kpss-gcc-msm8960", "qcom,kpss-gcc"
> >>> -
> >>> -- reg:
> >>> -	Usage: required
> >>> -	Value type: <prop-encoded-array>
> >>> -	Definition: base address and size of the register region
> >>> -
> >>> -- clocks:
> >>> -	Usage: required
> >>> -	Value type: <prop-encoded-array>
> >>> -	Definition: reference to the pll parents.
> >>> -
> >>> -- clock-names:
> >>> -	Usage: required
> >>> -	Value type: <stringlist>
> >>> -	Definition: must be "pll8_vote", "pxo".
> >>> -
> >>> -- clock-output-names:
> >>> -	Usage: required
> >>> -	Value type: <string>
> >>> -	Definition: Name of the output clock. Typically acpu_l2_aux indicating
> >>> -		    an L2 cache auxiliary clock.
> >>> -
> >>> -Example:
> >>> -
> >>> -	l2cc: clock-controller@2011000 {
> >>> -		compatible = "qcom,kpss-gcc-ipq8064", "qcom,kpss-gcc";
> >>> -		reg = <0x2011000 0x1000>;
> >>> -		clocks = <&gcc PLL8_VOTE>, <&gcc PXO_SRC>;
> >>> -		clock-names = "pll8_vote", "pxo";
> >>> -		clock-output-names = "acpu_l2_aux";
> >>> -	};
> >>> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
> >>> new file mode 100644
> >>> index 000000000000..7eb852be02c1
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
> >>> @@ -0,0 +1,69 @@
> >>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/arm/msm/qcom,kpss-gcc.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Krait Processor Sub-system (KPSS) Global Clock Controller (GCC)
> >>> +
> >>> +maintainers:
> >>> +  - Ansuel Smith <ansuelsmth@gmail.com>
> >>> +
> >>> +description: |
> >>> +  Krait Processor Sub-system (KPSS) Global Clock Controller (GCC). Used
> >>> +  to control L2 mux (in the current implementation).
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    items:
> >>> +      - enum:
> >>> +          - qcom,kpss-gcc-ipq8064
> >>> +          - qcom,kpss-gcc-apq8064
> >>> +          - qcom,kpss-gcc-msm8974
> >>> +          - qcom,kpss-gcc-msm8960
> >>> +      - const: qcom,kpss-gcc
> >>> +      - const: syscon
> >>
> >> There was no syscon here before. This is not explained in commit msg or
> >> patch history, while I asked to document explicitly any deviation from
> >> the conversion.
> >>
> >> This is not how the process works. You keep making silent/hidden changes
> >> to the bindings and to the patch submission process. It's difficult to
> >> review and it is even more difficult to trust you that you implement
> >> what we ask for. You keep resending versions of the patchset the same
> >> day (two versions yesterday, shortly after another one) which does not
> >> give time to react and review. Plus then you hide some more changes to
> >> regular conversion without explaining them.
> >>
> >> NAK. It's really bad process. :(
> >>
> >>
> >> Best regards,
> >> Krzysztof
> > 
> > The thing is that i'm trying to fix all the mess of years of keeping bad
> > Documentation and having dts that never followed Documentation. It's
> > really nothing silent/hidden. You add review tag to a patch? That won't
> > change. The bot alert me of some bugs? I push another revision with the
> > bug fixed. 
> 
> It does not necessarily mean that bindings are bad and such changes
> should be documented.
> 
> > (I understand I should not send that much revision in the
> > same day but still considering the slow process of reviewing the c code,
> > I prefer to keep the Documentation part correct and ready)
> 
> Rob also pointed to this - sending two versions of this huge patchset
> the same day is way too much.
>

You are totally right. I understand now the problem.

> > 
> > If you notice the changes across the different patch, it's very minimal
> > and 99% of it has not changed. Nothing silent just me addressing warning
> > from the bot. About the trust issue...
> > Is it really a syscon addition that bad? Again the original
> > Documentation was just bad so why should we care to have a 100% 1:1
> > conversion if it should have been not accepted in the first place.
> 
> Does not have to be 100% but deviations should be either expected or
> explained. Bindings are used also outside of Linux kernel.
> 
> > The addition of this new syscon is because in the current dtsi it's
> > there and I assume it's there as this is a global accessor and probably
> > other driver would access the same regs (so it's also a syscon)
> 
> If these are assumptions, then they need to be checked. If these were
> new bindings, we would discuss/check the need of syscon. Now we do not
> question existing properties, because they were accepted. But syscon
> compatible was not accepted, so putting it here requires our acknowledgment.
> 

About this I have a question. If the dts already have some binding and
the Documentation doesn't have them. Should the dts have priority or the
Documentation?
In the case where we can't prove that syscon is needed (for example), can
we remove it from dts (and accept to have inconsistency while the dts
changes are merged) or we should add the extra binding to the
Documentation putting some comments about it and discussing the
inclusion? 

> The bindings are probably pure junk, so this is not merely a conversion
> how you wrote in commit msg. This is rework of the bindings. Don't hide
> rework under "conversion". Conversion is TXT->YAML without any changes...
> 

Ok, thanks for the clarification. I still think should be handled with
conversion + additional commit to add the missing part so I have to fix
my wrong commits.

> I asked about this before and the only part you added to commit msg was
> "clock-cells". And now I see syscon - so isn't it a bit surprising?
> 

You are right... I will just do the 1:1 conversion and put all these
addition to a separate commit to make them clear.

> > 
> > I understand the complain about putting too much revision... But NAK
> > this cause I'm trying to fix all this mess just because more and more
> > problems are coming up and I'm trying to fix them. It's a bit sad.
> 
> Why you cannot test your changes and fix them all before sending sixth
> version? Why the bot has to test your code, not you?
> 

I'm aksed Rob if there is a quicker way to test single Documenation and
dts but it's my fault anyway.

> > Hope you can understand that it's not my interest to push silent changes
> > or other nasty stuff. It's just me fixing the mess and trying to at
> > least have the Documentation part ready while I wait for c code review.
> Best regards,
> Krzysztof

-- 
	Ansuel

Re: [PATCH v6 17/18] dt-bindings: arm: msm: Convert kpss-gcc driver Documentation to yaml

[Rob Herring]

On Wed, Mar 23, 2022 at 11:29:50AM +0100, Ansuel Smith wrote:
> On Mon, Mar 21, 2022 at 08:50:51PM -0500, Rob Herring wrote:
> > On Tue, 22 Mar 2022 00:15:47 +0100, Ansuel Smith wrote:
> > > Convert kpss-gcc driver Documentation to yaml. Since kpss-gcc expose a
> > > clock add the required '#clock-cells' binding while converting it.
> > > 
> > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > > ---
> > >  .../bindings/arm/msm/qcom,kpss-gcc.txt        | 44 ------------
> > >  .../bindings/arm/msm/qcom,kpss-gcc.yaml       | 69 +++++++++++++++++++
> > >  2 files changed, 69 insertions(+), 44 deletions(-)
> > >  delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
> > >  create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
> > > 
> > 
> > Running 'make dtbs_check' with the schema in this patch gives the
> > following warnings. Consider if they are expected or the schema is
> > incorrect. These may not be new warnings.
> > 
> > Note that it is not yet a requirement to have 0 warnings for dtbs_check.
> > This will change in the future.
> > 
> > Full log is available here: https://patchwork.ozlabs.org/patch/1607962
> > 
> > 
> > clock-controller@2011000: '#clock-cells' is a required property
> > 	arch/arm/boot/dts/qcom-ipq8064-ap148.dt.yaml
> > 	arch/arm/boot/dts/qcom-ipq8064-rb3011.dt.yaml
> > 
> > clock-controller@2011000: compatible:0: 'qcom,kpss-gcc' is not one of ['qcom,kpss-gcc-ipq8064', 'qcom,kpss-gcc-apq8064', 'qcom,kpss-gcc-msm8974', 'qcom,kpss-gcc-msm8960']
> > 	arch/arm/boot/dts/qcom-ipq8064-ap148.dt.yaml
> > 	arch/arm/boot/dts/qcom-ipq8064-rb3011.dt.yaml
> > 
> > clock-controller@2011000: compatible:1: 'qcom,kpss-gcc' was expected
> > 	arch/arm/boot/dts/qcom-ipq8064-ap148.dt.yaml
> > 	arch/arm/boot/dts/qcom-ipq8064-rb3011.dt.yaml
> > 
> > clock-controller@2011000: compatible: ['qcom,kpss-gcc', 'syscon'] is too short
> > 	arch/arm/boot/dts/qcom-ipq8064-ap148.dt.yaml
> > 	arch/arm/boot/dts/qcom-ipq8064-rb3011.dt.yaml
> > 
> 
> Sorry for the very stupid question but it's something i'm searching for
> a bit now... I can't really find Documentation or a guide on how to
> check single yaml and dts instead of using the make command and check
> everything. Am I missing something or this is not supported?

make allmodconfig
make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml

And now in next you can do just the filename or a pattern:

make dtbs_check DT_SCHEMA_FILES=qcom,kpss-gcc.yaml

Rob

Re: [PATCH v6 17/18] dt-bindings: arm: msm: Convert kpss-gcc driver Documentation to yaml

[Krzysztof Kozlowski]

On 23/03/2022 12:03, Ansuel Smith wrote:
>>>
>>> If you notice the changes across the different patch, it's very minimal
>>> and 99% of it has not changed. Nothing silent just me addressing warning
>>> from the bot. About the trust issue...
>>> Is it really a syscon addition that bad? Again the original
>>> Documentation was just bad so why should we care to have a 100% 1:1
>>> conversion if it should have been not accepted in the first place.
>>
>> Does not have to be 100% but deviations should be either expected or
>> explained. Bindings are used also outside of Linux kernel.
>>
>>> The addition of this new syscon is because in the current dtsi it's
>>> there and I assume it's there as this is a global accessor and probably
>>> other driver would access the same regs (so it's also a syscon)
>>
>> If these are assumptions, then they need to be checked. If these were
>> new bindings, we would discuss/check the need of syscon. Now we do not
>> question existing properties, because they were accepted. But syscon
>> compatible was not accepted, so putting it here requires our acknowledgment.
>>
> 
> About this I have a question. If the dts already have some binding and
> the Documentation doesn't have them. Should the dts have priority or the
> Documentation?

Depends, usually yes, if the DTS is being actually used. There might be
some exceptions, though. The priority is for the ones which are correct.

Judging by current DTS, the syscon is indeed used in DTS and documented
in bindings. It might be deprecated, because one binding is saying that
mailboxes can be used instead.

Here your call to add syscon looks correct. Just please document it in
commit msg, why it has to be added (there are real users of it: Qualcomm
RPM).

> In the case where we can't prove that syscon is needed (for example), can
> we remove it from dts (and accept to have inconsistency while the dts
> changes are merged) or we should add the extra binding to the
> Documentation putting some comments about it and discussing the
> inclusion? 
> 
>> The bindings are probably pure junk, so this is not merely a conversion
>> how you wrote in commit msg. This is rework of the bindings. Don't hide
>> rework under "conversion". Conversion is TXT->YAML without any changes...
>>
> 
> Ok, thanks for the clarification. I still think should be handled with
> conversion + additional commit to add the missing part so I have to fix
> my wrong commits.
> 
>> I asked about this before and the only part you added to commit msg was
>> "clock-cells". And now I see syscon - so isn't it a bit surprising?
>>
> 
> You are right... I will just do the 1:1 conversion and put all these
> addition to a separate commit to make them clear.
> 
>>>
>>> I understand the complain about putting too much revision... But NAK
>>> this cause I'm trying to fix all this mess just because more and more
>>> problems are coming up and I'm trying to fix them. It's a bit sad.
>>
>> Why you cannot test your changes and fix them all before sending sixth
>> version? Why the bot has to test your code, not you?
>>
> 
> I'm aksed Rob if there is a quicker way to test single Documenation and
> dts but it's my fault anyway.

make -j8 dt_binding_check DT_SCHEMA_FILES="qcom-kpss"

make defconfig (or allyesconfig etc)
make -j8 dtbs_check DT_SCHEMA_FILES="qcom-kpss"


Best regards,
Krzysztof

Re: [PATCH v6 15/18] dt-bindings: clock: Add L2 clocks to qcom,krait-cc Documentation

[Rob Herring]

On Tue, 22 Mar 2022 00:15:45 +0100, Ansuel Smith wrote:
> Krait-cc qcom driver provide also L2 clocks and require the acpu_l2_aux
> and the hfpll_l2 clock to be provided. Add these missing clocks to the
> Documentation. The driver keep support for both old (it did already used
> these clocks and we keep the same naming scheme) and this new
> implementation and should prevent any regression by this fixup.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
> ---
>  .../devicetree/bindings/clock/qcom,krait-cc.yaml       | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v6 06/18] clk: qcom: clk-krait: unlock spin after mux completion

[Stephen Boyd]

Quoting Ansuel Smith (2022-03-21 16:15:36)
> Unlock spinlock after the mux switch is completed to prevent any corner
> case of mux request while the switch still needs to be done.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---

Please add a Fixes tag.

>  drivers/clk/qcom/clk-krait.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/clk-krait.c b/drivers/clk/qcom/clk-krait.c
> index 59f1af415b58..e447fcc3806d 100644
> --- a/drivers/clk/qcom/clk-krait.c
> +++ b/drivers/clk/qcom/clk-krait.c
> @@ -32,11 +32,12 @@ static void __krait_mux_set_sel(struct krait_mux_clk *mux, int sel)
>                 regval |= (sel & mux->mask) << (mux->shift + LPL_SHIFT);
>         }
>         krait_set_l2_indirect_reg(mux->offset, regval);
> -       spin_unlock_irqrestore(&krait_clock_reg_lock, flags);
>  
>         /* Wait for switch to complete. */
>         mb();
>         udelay(1);
> +

Please add a comment here indicating we want to make sure the mux
register isn't modified while switching to the new parent.

> +       spin_unlock_irqrestore(&krait_clock_reg_lock, flags);
>  }
>

Re: [PATCH v6 05/18] clk: qcom: kpss-xcc: convert to parent data API

[Stephen Boyd]

Quoting Ansuel Smith (2022-03-21 16:15:35)
> Convert the driver to parent data API. From the Documentation pll8_vote
> and pxo should be declared in the DTS so fw_name can be used instead of
> parent_names. Name is still used to save regression on old definition.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  drivers/clk/qcom/kpss-xcc.c | 25 ++++++++-----------------
>  1 file changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/clk/qcom/kpss-xcc.c b/drivers/clk/qcom/kpss-xcc.c
> index 4fec1f9142b8..347f70e9f5fe 100644
> --- a/drivers/clk/qcom/kpss-xcc.c
> +++ b/drivers/clk/qcom/kpss-xcc.c
> @@ -12,9 +12,9 @@
>  #include <linux/clk.h>
>  #include <linux/clk-provider.h>
>  
> -static const char *aux_parents[] = {
> -       "pll8_vote",
> -       "pxo",
> +static const struct clk_parent_data aux_parents[] = {
> +       { .name = "pll8_vote", .fw_name = "pll8_vote" },
> +       { .name = "pxo", .fw_name = "pxo" },
>  };
>  
>  static unsigned int aux_parent_map[] = {
> @@ -32,8 +32,8 @@ MODULE_DEVICE_TABLE(of, kpss_xcc_match_table);
>  static int kpss_xcc_driver_probe(struct platform_device *pdev)
>  {
>         const struct of_device_id *id;
> -       struct clk *clk;
>         void __iomem *base;
> +       struct clk_hw *hw;
>         const char *name;
>  
>         id = of_match_device(kpss_xcc_match_table, &pdev->dev);
> @@ -55,24 +55,15 @@ static int kpss_xcc_driver_probe(struct platform_device *pdev)
>                 base += 0x28;
>         }
>  
> -       clk = clk_register_mux_table(&pdev->dev, name, aux_parents,
> -                                    ARRAY_SIZE(aux_parents), 0, base, 0, 0x3,
> -                                    0, aux_parent_map, NULL);
> +       hw = __devm_clk_hw_register_mux(&pdev->dev, NULL, name, ARRAY_SIZE(aux_parents),

Does something in devm_clk_hw_register_mux() not work here? Do we need a
devm_clk_hw_register_mux_parent_data()? If so, please add it.

> +                                       NULL, NULL, aux_parents, 0, base, 0, 0x3,
> +                                       0, aux_parent_map, NULL);
>

Re: [PATCH v6 04/18] clk: qcom: clk-hfpll: use poll_timeout macro

[Stephen Boyd]

Quoting Ansuel Smith (2022-03-21 16:15:34)
> Use regmap_read_poll_timeout macro instead of do-while structure.

Please add a note that this isn't an equivalent translation. Now we
timeout if the PLL fails to lock whereas before we would loop
potentially forever.

> diff --git a/drivers/clk/qcom/clk-hfpll.c b/drivers/clk/qcom/clk-hfpll.c
> index e847d586a73a..a4e347eb4d4d 100644
> --- a/drivers/clk/qcom/clk-hfpll.c
> +++ b/drivers/clk/qcom/clk-hfpll.c
> @@ -12,6 +12,8 @@
>  #include "clk-regmap.h"
>  #include "clk-hfpll.h"
>  
> +#define HFPLL_BUSY_WAIT_TIMEOUT        100

We don't really need a define for this.

> +
>  #define PLL_OUTCTRL    BIT(0)
>  #define PLL_BYPASSNL   BIT(1)
>  #define PLL_RESET_N    BIT(2)
> @@ -72,13 +74,12 @@ static void __clk_hfpll_enable(struct clk_hw *hw)
>         regmap_update_bits(regmap, hd->mode_reg, PLL_RESET_N, PLL_RESET_N);
>  
>         /* Wait for PLL to lock. */
> -       if (hd->status_reg) {
> -               do {
> -                       regmap_read(regmap, hd->status_reg, &val);
> -               } while (!(val & BIT(hd->lock_bit)));
> -       } else {
> +       if (hd->status_reg)
> +               regmap_read_poll_timeout(regmap, hd->status_reg, val,
> +                                        !(val & BIT(hd->lock_bit)), USEC_PER_MSEC * 2,

Why are we waiting between reads?

> +                                        HFPLL_BUSY_WAIT_TIMEOUT * USEC_PER_MSEC);
> +       else
>                 udelay(60);
> -       }
>

Re: [PATCH v6 03/18] clk: qcom: gcc-ipq806x: add PXO_SRC in clk table

[Stephen Boyd]

Quoting Ansuel Smith (2022-03-21 16:15:33)
> PXO_SRC is currently defined in the gcc include and referenced in the
> ipq8064 DTSI. Correctly provide a clk after gcc probe to fix kernel
> panic if a driver starts to actually use it.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---

What is this patch about? clk providers shouldn't be calling clk_get().

>  drivers/clk/qcom/gcc-ipq806x.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/clk/qcom/gcc-ipq806x.c b/drivers/clk/qcom/gcc-ipq806x.c
> index 27f6d7626abb..7271d3afdc89 100644
> --- a/drivers/clk/qcom/gcc-ipq806x.c
> +++ b/drivers/clk/qcom/gcc-ipq806x.c
> @@ -26,6 +26,8 @@
>  #include "clk-hfpll.h"
>  #include "reset.h"
>  
> +static struct clk_regmap pxo = { };
> +
>  static struct clk_pll pll0 = {
>         .l_reg = 0x30c4,
>         .m_reg = 0x30c8,
> @@ -2754,6 +2756,7 @@ static struct clk_dyn_rcg ubi32_core2_src_clk = {
>  };
>  
>  static struct clk_regmap *gcc_ipq806x_clks[] = {
> +       [PXO_SRC] = NULL,
>         [PLL0] = &pll0.clkr,
>         [PLL0_VOTE] = &pll0_vote,
>         [PLL3] = &pll3.clkr,
> @@ -3083,6 +3086,10 @@ static int gcc_ipq806x_probe(struct platform_device *pdev)
>         if (ret)
>                 return ret;
>  
> +       clk = clk_get(dev, "pxo");
> +       pxo.hw = *__clk_get_hw(clk);
> +       gcc_ipq806x_clks[PXO_SRC] = &pxo;
> +
>         regmap = dev_get_regmap(dev, NULL);
>         if (!regmap)
>                 return -ENODEV;
> -- 
> 2.34.1
>

Re: [PATCH v6 05/18] clk: qcom: kpss-xcc: convert to parent data API

[Ansuel Smith]

On Thu, Mar 24, 2022 at 06:06:50PM -0700, Stephen Boyd wrote:
> Quoting Ansuel Smith (2022-03-21 16:15:35)
> > Convert the driver to parent data API. From the Documentation pll8_vote
> > and pxo should be declared in the DTS so fw_name can be used instead of
> > parent_names. Name is still used to save regression on old definition.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >  drivers/clk/qcom/kpss-xcc.c | 25 ++++++++-----------------
> >  1 file changed, 8 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/clk/qcom/kpss-xcc.c b/drivers/clk/qcom/kpss-xcc.c
> > index 4fec1f9142b8..347f70e9f5fe 100644
> > --- a/drivers/clk/qcom/kpss-xcc.c
> > +++ b/drivers/clk/qcom/kpss-xcc.c
> > @@ -12,9 +12,9 @@
> >  #include <linux/clk.h>
> >  #include <linux/clk-provider.h>
> >  
> > -static const char *aux_parents[] = {
> > -       "pll8_vote",
> > -       "pxo",
> > +static const struct clk_parent_data aux_parents[] = {
> > +       { .name = "pll8_vote", .fw_name = "pll8_vote" },
> > +       { .name = "pxo", .fw_name = "pxo" },
> >  };
> >  
> >  static unsigned int aux_parent_map[] = {
> > @@ -32,8 +32,8 @@ MODULE_DEVICE_TABLE(of, kpss_xcc_match_table);
> >  static int kpss_xcc_driver_probe(struct platform_device *pdev)
> >  {
> >         const struct of_device_id *id;
> > -       struct clk *clk;
> >         void __iomem *base;
> > +       struct clk_hw *hw;
> >         const char *name;
> >  
> >         id = of_match_device(kpss_xcc_match_table, &pdev->dev);
> > @@ -55,24 +55,15 @@ static int kpss_xcc_driver_probe(struct platform_device *pdev)
> >                 base += 0x28;
> >         }
> >  
> > -       clk = clk_register_mux_table(&pdev->dev, name, aux_parents,
> > -                                    ARRAY_SIZE(aux_parents), 0, base, 0, 0x3,
> > -                                    0, aux_parent_map, NULL);
> > +       hw = __devm_clk_hw_register_mux(&pdev->dev, NULL, name, ARRAY_SIZE(aux_parents),
> 
> Does something in devm_clk_hw_register_mux() not work here? Do we need a
> devm_clk_hw_register_mux_parent_data()? If so, please add it.
>

Main reason I can't use devm_clk_hw_register_mux is the fact I can't
provide a parent_map. Will add an additional macro. OK.

> > +                                       NULL, NULL, aux_parents, 0, base, 0, 0x3,
> > +                                       0, aux_parent_map, NULL);
> >

-- 
	Ansuel

Re: [PATCH v6 02/18] clk: qcom: gcc-ipq806x: skip pxo/cxo fixed clk if already present

[Stephen Boyd]

Quoting Ansuel Smith (2022-03-21 16:15:32)
> Skip pxo/cxo clock registration if they are already defined in DTS as fixed
> clock.

Why?

> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

Re: [PATCH v6 03/18] clk: qcom: gcc-ipq806x: add PXO_SRC in clk table

[Ansuel Smith]

On Thu, Mar 24, 2022 at 06:10:35PM -0700, Stephen Boyd wrote:
> Quoting Ansuel Smith (2022-03-21 16:15:33)
> > PXO_SRC is currently defined in the gcc include and referenced in the
> > ipq8064 DTSI. Correctly provide a clk after gcc probe to fix kernel
> > panic if a driver starts to actually use it.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> 
> What is this patch about? clk providers shouldn't be calling clk_get().
>

If pxo is passed as a clock in dts and defined as a fixed clock, what
should be used? 

> >  drivers/clk/qcom/gcc-ipq806x.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/clk/qcom/gcc-ipq806x.c b/drivers/clk/qcom/gcc-ipq806x.c
> > index 27f6d7626abb..7271d3afdc89 100644
> > --- a/drivers/clk/qcom/gcc-ipq806x.c
> > +++ b/drivers/clk/qcom/gcc-ipq806x.c
> > @@ -26,6 +26,8 @@
> >  #include "clk-hfpll.h"
> >  #include "reset.h"
> >  
> > +static struct clk_regmap pxo = { };
> > +
> >  static struct clk_pll pll0 = {
> >         .l_reg = 0x30c4,
> >         .m_reg = 0x30c8,
> > @@ -2754,6 +2756,7 @@ static struct clk_dyn_rcg ubi32_core2_src_clk = {
> >  };
> >  
> >  static struct clk_regmap *gcc_ipq806x_clks[] = {
> > +       [PXO_SRC] = NULL,
> >         [PLL0] = &pll0.clkr,
> >         [PLL0_VOTE] = &pll0_vote,
> >         [PLL3] = &pll3.clkr,
> > @@ -3083,6 +3086,10 @@ static int gcc_ipq806x_probe(struct platform_device *pdev)
> >         if (ret)
> >                 return ret;
> >  
> > +       clk = clk_get(dev, "pxo");
> > +       pxo.hw = *__clk_get_hw(clk);
> > +       gcc_ipq806x_clks[PXO_SRC] = &pxo;
> > +
> >         regmap = dev_get_regmap(dev, NULL);
> >         if (!regmap)
> >                 return -ENODEV;
> > -- 
> > 2.34.1
> >

-- 
	Ansuel

Re: [PATCH v6 01/18] clk: introduce clk_hw_get_index_of_parent new API

[Stephen Boyd]

Quoting Ansuel Smith (2022-03-21 16:15:31)
> Clk can have multiple parents. Some clk may require to get the cached
> index of other parent that are not current associated with the clk.
> We have clk_hw_get_parent_index() that returns the index of the current
> parent but we can't get the index of other parents of the provided clk.
> Introduce clk_hw_get_index_of_parent() to get the cached index of the
> parent of the provided clk. This permits a direct access of the internal
> clk_fetch_parent_index().
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  drivers/clk/clk.c            | 14 ++++++++++++++
>  include/linux/clk-provider.h |  1 +
>  2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 8de6a22498e7..bdd70a88394c 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1726,6 +1726,20 @@ int clk_hw_get_parent_index(struct clk_hw *hw)
>  }
>  EXPORT_SYMBOL_GPL(clk_hw_get_parent_index);
>  
> +/**
> + * clk_hw_get_index_of_parent - return the index of the parent clock

s/return/get/

> + * @hw: clk_hw associated with the clk being consumed
> + * @parent: clk_hw of the parent to be fetched the index of

s/to be fetched/to fetch/

> + *
> + * Fetches and returns the index of parent clock provided.
> + * Returns -EINVAL if the given parent index can't be provided.

I know this is copied from clk_hw_get_parent_index, but it can be
improved.

Return: The index corresponding to @parent for @hw or -EINVAL if
@parent isn't a possible parent of @hw.

> + */
> +int clk_hw_get_index_of_parent(struct clk_hw *hw, const struct clk_hw *parent)
> +{
> +       return clk_fetch_parent_index(hw->core, parent->core);
> +}
> +EXPORT_SYMBOL_GPL(clk_hw_get_index_of_parent);
> +
>  /*
>   * Update the orphan status of @core and all its children.
>   */

Re: [PATCH v6 03/18] clk: qcom: gcc-ipq806x: add PXO_SRC in clk table

[Stephen Boyd]

Quoting Ansuel Smith (2022-03-24 18:13:49)
> On Thu, Mar 24, 2022 at 06:10:35PM -0700, Stephen Boyd wrote:
> > Quoting Ansuel Smith (2022-03-21 16:15:33)
> > > PXO_SRC is currently defined in the gcc include and referenced in the
> > > ipq8064 DTSI. Correctly provide a clk after gcc probe to fix kernel
> > > panic if a driver starts to actually use it.
> > > 
> > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > > ---
> > 
> > What is this patch about? clk providers shouldn't be calling clk_get().
> >
> 
> If pxo is passed as a clock in dts and defined as a fixed clock, what
> should be used? 

clk_parent_data

Re: [PATCH v6 03/18] clk: qcom: gcc-ipq806x: add PXO_SRC in clk table

[Ansuel Smith]

On Thu, Mar 24, 2022 at 06:22:29PM -0700, Stephen Boyd wrote:
> Quoting Ansuel Smith (2022-03-24 18:13:49)
> > On Thu, Mar 24, 2022 at 06:10:35PM -0700, Stephen Boyd wrote:
> > > Quoting Ansuel Smith (2022-03-21 16:15:33)
> > > > PXO_SRC is currently defined in the gcc include and referenced in the
> > > > ipq8064 DTSI. Correctly provide a clk after gcc probe to fix kernel
> > > > panic if a driver starts to actually use it.
> > > > 
> > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > > > ---
> > > 
> > > What is this patch about? clk providers shouldn't be calling clk_get().
> > >
> > 
> > If pxo is passed as a clock in dts and defined as a fixed clock, what
> > should be used? 
> 
> clk_parent_data

Sorry but I'm not following you. No idea if you missed the cover letter
where i describe the problem with PXO_SRC.

The problem here is that
- In DTS we have node that reference <&gcc PXO_SRC>
But
- gcc driver NEVER defined PXO_SRC
As
- PXO_SRC is actually pxo_board that should be defined as a fixed-clock
  in dts or is defined using qcom_cc_register_board_clk.

So in theory we should just put in PXO_SRC the clk hw of the
fixed-clock. That is why I'm using clk_get(). I can use __clk_lookup()
as an alternative but I really can't find a way to get the clock defined
from DTS or qcom_cc_register_board_clk.

(I have the same exact problem with the cpu qsb clock where is defined
using fixed-clock API but can also defined directly in DTS and I have to
use clk_get())

I'm totally missing something so I would love some hint on how to solve
this.

-- 
	Ansuel

Re: [PATCH v6 10/18] clk: qcom: krait-cc: drop hardcoded safe_sel

[Dmitry Baryshkov]

On 22/03/2022 02:15, Ansuel Smith wrote:
> Drop hardcoded safe_sel definition and use helper to correctly calculate
> it. We assume qsb clk is always present as it should be declared in DTS
> per Documentation and in the absence of that, it's declared as a fixed
> clk.

Why? Can safe_sel (sec_mux index) change?

> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>   drivers/clk/qcom/krait-cc.c | 40 +++++++++++++++++++++++++------------
>   1 file changed, 27 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/clk/qcom/krait-cc.c b/drivers/clk/qcom/krait-cc.c
> index e9508e3104ea..5f98ee1c3681 100644
> --- a/drivers/clk/qcom/krait-cc.c
> +++ b/drivers/clk/qcom/krait-cc.c
> @@ -26,6 +26,17 @@ static unsigned int pri_mux_map[] = {
>   	0,
>   };
>   
> +static u8 krait_get_mux_sel(struct krait_mux_clk *mux, struct clk *safe_clk)
> +{
> +	struct clk_hw *safe_hw = __clk_get_hw(safe_clk);
> +
> +	/*
> +	 * We can ignore errors from clk_hw_get_index_of_parent()
> +	 * as we create these parents in this driver.
> +	 */
> +	return clk_hw_get_index_of_parent(&mux->hw, safe_hw);
> +}
> +
>   /*
>    * Notifier function for switching the muxes to safe parent
>    * while the hfpll is getting reprogrammed.
> @@ -116,8 +127,8 @@ krait_add_div(struct device *dev, int id, const char *s, unsigned int offset)
>   }
>   
>   static struct clk *
> -krait_add_sec_mux(struct device *dev, int id, const char *s,
> -		  unsigned int offset, bool unique_aux)
> +krait_add_sec_mux(struct device *dev, struct clk *qsb, int id,
> +		  const char *s, unsigned int offset, bool unique_aux)
>   {
>   	int ret;
>   	struct krait_mux_clk *mux;
> @@ -144,7 +155,6 @@ krait_add_sec_mux(struct device *dev, int id, const char *s,
>   	mux->shift = 2;
>   	mux->parent_map = sec_mux_map;
>   	mux->hw.init = &init;
> -	mux->safe_sel = 0;
>   
>   	init.name = kasprintf(GFP_KERNEL, "krait%s_sec_mux", s);
>   	if (!init.name)
> @@ -166,6 +176,7 @@ krait_add_sec_mux(struct device *dev, int id, const char *s,
>   	if (IS_ERR(clk))
>   		goto err_clk;
>   
> +	mux->safe_sel = krait_get_mux_sel(mux, qsb);
>   	ret = krait_notifier_register(dev, clk, mux);
>   	if (ret)
>   		clk = ERR_PTR(ret);
> @@ -204,7 +215,6 @@ krait_add_pri_mux(struct device *dev, struct clk *hfpll_div, struct clk *sec_mux
>   	mux->lpl = id >= 0;
>   	mux->parent_map = pri_mux_map;
>   	mux->hw.init = &init;
> -	mux->safe_sel = 2;
>   
>   	init.name = kasprintf(GFP_KERNEL, "krait%s_pri_mux", s);
>   	if (!init.name)
> @@ -226,6 +236,7 @@ krait_add_pri_mux(struct device *dev, struct clk *hfpll_div, struct clk *sec_mux
>   	if (IS_ERR(clk))
>   		goto err_clk;
>   
> +	mux->safe_sel = krait_get_mux_sel(mux, sec_mux);
>   	ret = krait_notifier_register(dev, clk, mux);
>   	if (ret)
>   		clk = ERR_PTR(ret);
> @@ -238,7 +249,9 @@ krait_add_pri_mux(struct device *dev, struct clk *hfpll_div, struct clk *sec_mux
>   }
>   
>   /* id < 0 for L2, otherwise id == physical CPU number */
> -static struct clk *krait_add_clks(struct device *dev, int id, bool unique_aux)
> +static struct clk *
> +krait_add_clks(struct device *dev, struct clk *qsb, int id,
> +	       bool unique_aux)
>   {
>   	unsigned int offset;
>   	void *p = NULL;
> @@ -261,7 +274,7 @@ static struct clk *krait_add_clks(struct device *dev, int id, bool unique_aux)
>   		goto err;
>   	}
>   
> -	sec_mux = krait_add_sec_mux(dev, id, s, offset, unique_aux);
> +	sec_mux = krait_add_sec_mux(dev, qsb, id, s, offset, unique_aux);
>   	if (IS_ERR(sec_mux)) {
>   		clk = sec_mux;
>   		goto err;
> @@ -301,18 +314,19 @@ static int krait_cc_probe(struct platform_device *pdev)
>   	int cpu;
>   	struct clk *clk;
>   	struct clk **clks;
> -	struct clk *l2_pri_mux_clk;
> +	struct clk *l2_pri_mux_clk, *qsb;
>   
>   	id = of_match_device(krait_cc_match_table, dev);
>   	if (!id)
>   		return -ENODEV;
>   
>   	/* Rate is 1 because 0 causes problems for __clk_mux_determine_rate */
> -	if (IS_ERR(clk_get(dev, "qsb")))
> -		clk = clk_register_fixed_rate(dev, "qsb", NULL, 0, 1);
> +	qsb = clk_get(dev, "qsb");
> +	if (IS_ERR(qsb))
> +		qsb = clk_register_fixed_rate(dev, "qsb", NULL, 0, 1);
>   
> -	if (IS_ERR(clk))
> -		return PTR_ERR(clk);
> +	if (IS_ERR(qsb))
> +		return PTR_ERR(qsb);
>   
>   	if (!id->data) {
>   		clk = clk_register_fixed_factor(dev, "acpu_aux",
> @@ -327,13 +341,13 @@ static int krait_cc_probe(struct platform_device *pdev)
>   		return -ENOMEM;
>   
>   	for_each_possible_cpu(cpu) {
> -		clk = krait_add_clks(dev, cpu, id->data);
> +		clk = krait_add_clks(dev, qsb, cpu, id->data);
>   		if (IS_ERR(clk))
>   			return PTR_ERR(clk);
>   		clks[cpu] = clk;
>   	}
>   
> -	l2_pri_mux_clk = krait_add_clks(dev, -1, id->data);
> +	l2_pri_mux_clk = krait_add_clks(dev, qsb, -1, id->data);
>   	if (IS_ERR(l2_pri_mux_clk))
>   		return PTR_ERR(l2_pri_mux_clk);
>   	clks[4] = l2_pri_mux_clk;


-- 
With best wishes
Dmitry

Re: [PATCH v6 13/18] clk: qcom: clk-krait: add enable disable ops

[Dmitry Baryshkov]

On 22/03/2022 02:15, Ansuel Smith wrote:
> Add enable/disable ops for krait mux. On disable the mux is set to the
> safe selection.

Why? It it used during system suspend? cpuidle? cpufreq?

> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>   drivers/clk/qcom/clk-krait.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/clk/qcom/clk-krait.c b/drivers/clk/qcom/clk-krait.c
> index 7ba5dbc72bce..061af57b0ec2 100644
> --- a/drivers/clk/qcom/clk-krait.c
> +++ b/drivers/clk/qcom/clk-krait.c
> @@ -85,7 +85,25 @@ static u8 krait_mux_get_parent(struct clk_hw *hw)
>   	return clk_mux_val_to_index(hw, mux->parent_map, 0, sel);
>   }
>   
> +static int krait_mux_enable(struct clk_hw *hw)
> +{
> +	struct krait_mux_clk *mux = to_krait_mux_clk(hw);
> +
> +	__krait_mux_set_sel(mux, mux->en_mask);
> +
> +	return 0;
> +}
> +
> +static void krait_mux_disable(struct clk_hw *hw)
> +{
> +	struct krait_mux_clk *mux = to_krait_mux_clk(hw);
> +
> +	__krait_mux_set_sel(mux, mux->safe_sel);
> +}
> +
>   const struct clk_ops krait_mux_clk_ops = {
> +	.enable = krait_mux_enable,
> +	.disable = krait_mux_disable,
>   	.set_parent = krait_mux_set_parent,
>   	.get_parent = krait_mux_get_parent,
>   	.determine_rate = __clk_mux_determine_rate_closest,


-- 
With best wishes
Dmitry

Re: [PATCH v6 00/18] Modernize rest of the krait drivers

[Dmitry Baryshkov]

On 22/03/2022 02:15, Ansuel Smith wrote:
> This is a follow-up to the ipq806x gcc modernize series. Manu cleanup
> changes and also some discoveries of wrong definition notice only with
> all these conversions.

General comment regarding this patch series. It contains fixes, clock 
conversion for several drivers, dts fixes, etc. It's, for example, not 
straightforwardly obvious if Bjorn can pickup patches 04 or 06 without 
picking up other patches.

If would be best if you can split this series or at least pull fixes to 
be the first patches in the pile.

Patch 01 is only used by patch 10, they can stay close.

In some of the commit messages you describe what do they do, but you 
completely omit the reason for the change, why is the change necessary.
(Yes, I spot that because I also too often skip that).

> 
> The first patch is an improvement of the clk_hw_get_parent_index. The
> original idea of clk_hw_get_parent_index was to give a way to access the
> parent index but for some reason the final version limited it to the
> current index. We change it to give the current parent if is not
> provided and to give the requested parent if provided. Any user of this
> function is updated to follow the new implementation.
> 
> The patch 2 and 3 are some additional fixes for gcc.
> The first one is a fix that register the pxo and cxo fixed clock only if
> they are not defined in DTS.
> The patch 3 require some explaination. In short is a big HACK to prevent
> kernel panic with this series.
> 
> The kpss-xcc driver is a mess.
> The Documentation declare that the clocks should be provided but for some
> reason it was never followed.
> In fact in the ipq8064 DTSI only the clocks for l2cc are declared but
> for cpu0 and cpu1 the clocks are not defined.
> The kpss-xcc driver use parent_names so the clks are ignored and never
> used so till now it wasn't a problem (ignoring the fact that they
> doesn't follow documentation at all)
> On top of that, the l2cc node declare the pxo clock in a really strange
> way. It's declared using the PXO_SRC gcc clock that is never defined in
> the gcc ipq8064 clock table. (the correct way was to declare a fixed
> clock in dts and reference that)
> To prevent any kind of problem we use the patch 3 and provide the clk
> for PXO_SRC in the gcc clock table. We manually provide the clk after
> gcc probe.
> 
> Patch 4 is just a minor cleanup where we use the poll macro
> 
> Patch 5 is the actually kpss-xcc conversion to parent data
> 
> Patch 6-7 should be a fixup of a real conver case
> 
> Patch 8 converts the krait-cc to parent_data
> Patch 9 give some love to the code with some minor fixup
> Patch 10 drop the hardcoded safe sel and use the new
> clk_hw_get_parent_index to get the safe parent index.
> (also I discovered that the parent order was wrong)
> 
> Patch 11 is an additional fixup to force the reset of the muxes even
> more.
> 
> Patch 12-13 are some additiona taken from the qsdk that were missing in
> the upstream driver
> 
> Patch 14 converts krait-cc to yaml
> 
> Patch 15 add to krait-cc Documentation the L2 clocks
> 
> Patch 16 converts the kpss-acc driver to yaml and fix some Documentation
> error
> 
> Patch 17 convets the kpss-gcc driver to yaml
> 
> Patch 18 finally adds all this stuff to the ipq8064 dtsi (and fix the
> stupid PXO_SRC phandle)
> 
> I tested this series on a ipq8064 SoC by running a cache benchmark test
> to make sure the changes are correct and we don't silently cause
> regressions. Also I compared the output of the clk_summary every time
> and we finally have a sane output where the mux are correctly placed in
> the correct parent. (till now we had the cpu aux clock all over the
> place, probably never cause problems but who knows.)
> 
> v6:
> - Move dts patch as last patch
> - Address commencts from Rob
> - Fix warning from make dtbs_check
> v5:
> - Address comments from Krzysztof
> v4:
> - Fix more dt-bindings bog errors
> v3:
> - Split Documentation files for kpss and krait-cc
> v2:
> - introduce new API instead of fixing the existing one
> - do not reorganize variables in krait-cc
> - fix some comments error and improve it
> - return better error for patch 7
> - fix missing new line on patch 16
> 
> Ansuel Smith (18):
>    clk: introduce clk_hw_get_index_of_parent new API
>    clk: qcom: gcc-ipq806x: skip pxo/cxo fixed clk if already present
>    clk: qcom: gcc-ipq806x: add PXO_SRC in clk table
>    clk: qcom: clk-hfpll: use poll_timeout macro
>    clk: qcom: kpss-xcc: convert to parent data API
>    clk: qcom: clk-krait: unlock spin after mux completion
>    clk: qcom: clk-krait: add hw_parent check for div2_round_rate
>    clk: qcom: krait-cc: convert to parent_data API
>    clk: qcom: krait-cc: drop pr_info and register qsb only if needed
>    clk: qcom: krait-cc: drop hardcoded safe_sel
>    clk: qcom: krait-cc: force sec_mux to QSB
>    clk: qcom: clk-krait: add apq/ipq8064 errata workaround
>    clk: qcom: clk-krait: add enable disable ops
>    dt-bindings: clock: Convert qcom,krait-cc to yaml
>    dt-bindings: clock: Add L2 clocks to qcom,krait-cc Documentation
>    dt-bindings: arm: msm: Convert kpss-acc driver Documentation to yaml
>    dt-bindings: arm: msm: Convert kpss-gcc driver Documentation to yaml
>    ARM: dts: qcom: qcom-ipq8064: add missing krait-cc compatible and
>      clocks
> 
>   .../bindings/arm/msm/qcom,kpss-acc.txt        |  49 -----
>   .../bindings/arm/msm/qcom,kpss-acc.yaml       |  94 +++++++++
>   .../bindings/arm/msm/qcom,kpss-gcc.txt        |  44 -----
>   .../bindings/arm/msm/qcom,kpss-gcc.yaml       |  69 +++++++
>   .../bindings/clock/qcom,krait-cc.txt          |  34 ----
>   .../bindings/clock/qcom,krait-cc.yaml         |  65 ++++++
>   arch/arm/boot/dts/qcom-ipq8064.dtsi           |  24 ++-
>   drivers/clk/clk.c                             |  14 ++
>   drivers/clk/qcom/clk-hfpll.c                  |  13 +-
>   drivers/clk/qcom/clk-krait.c                  |  44 ++++-
>   drivers/clk/qcom/clk-krait.h                  |   1 +
>   drivers/clk/qcom/gcc-ipq806x.c                |  27 ++-
>   drivers/clk/qcom/kpss-xcc.c                   |  25 +--
>   drivers/clk/qcom/krait-cc.c                   | 186 ++++++++++--------
>   include/linux/clk-provider.h                  |   1 +
>   15 files changed, 453 insertions(+), 237 deletions(-)
>   delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.txt
>   create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml
>   delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
>   create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
>   delete mode 100644 Documentation/devicetree/bindings/clock/qcom,krait-cc.txt
>   create mode 100644 Documentation/devicetree/bindings/clock/qcom,krait-cc.yaml
> 


-- 
With best wishes
Dmitry

Re: [PATCH v6 03/18] clk: qcom: gcc-ipq806x: add PXO_SRC in clk table

[Dmitry Baryshkov]

On Wed, 13 Apr 2022 at 20:00, Ansuel Smith <ansuelsmth@gmail.com> wrote:
>
> On Thu, Mar 24, 2022 at 06:22:29PM -0700, Stephen Boyd wrote:
> > Quoting Ansuel Smith (2022-03-24 18:13:49)
> > > On Thu, Mar 24, 2022 at 06:10:35PM -0700, Stephen Boyd wrote:
> > > > Quoting Ansuel Smith (2022-03-21 16:15:33)
> > > > > PXO_SRC is currently defined in the gcc include and referenced in the
> > > > > ipq8064 DTSI. Correctly provide a clk after gcc probe to fix kernel
> > > > > panic if a driver starts to actually use it.
> > > > >
> > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > > > > ---
> > > >
> > > > What is this patch about? clk providers shouldn't be calling clk_get().
> > > >
> > >
> > > If pxo is passed as a clock in dts and defined as a fixed clock, what
> > > should be used?
> >
> > clk_parent_data
>
> Sorry but I'm not following you. No idea if you missed the cover letter
> where i describe the problem with PXO_SRC.
>
> The problem here is that
> - In DTS we have node that reference <&gcc PXO_SRC>
> But
> - gcc driver NEVER defined PXO_SRC
> As
> - PXO_SRC is actually pxo_board that should be defined as a fixed-clock
>   in dts or is defined using qcom_cc_register_board_clk.
>
> So in theory we should just put in PXO_SRC the clk hw of the
> fixed-clock. That is why I'm using clk_get(). I can use __clk_lookup()
> as an alternative but I really can't find a way to get the clock defined
> from DTS or qcom_cc_register_board_clk.
>
> (I have the same exact problem with the cpu qsb clock where is defined
> using fixed-clock API but can also defined directly in DTS and I have to
> use clk_get())
>
> I'm totally missing something so I would love some hint on how to solve
> this.

When we were doing such conversion for other  platforms, we pointed
clock consumers to the board clocks directly. There is no need to go
through the gcc to fetch pxo.
Instead you can use a <&pxo_board> in the dts directly. Typically the
sequence is the following:
- Minor cleanup of the clock-controller driver
(ARRAY_SIZE(parent_data), removal of unused clock sources, unused enum
entries, etc)
- update drivers to use both .name and  .fw_name in replacement of
parent_names. Use parent_hws where possible.
- update dtsi to reference clocks using clocks/clock-names properties.
Pass board/rpmh/rpm clocks directly to their consumers without
bandaids in the gcc driver.
- (optionally) after several major releases drop parent_data.name
completely. I think we mostly skipped this, since it provides no gain.

This way you don't have to play around clk_get to return PXO_SRC from
gcc clock-controller.

-- 
With best wishes
Dmitry

Re: [PATCH v6 10/18] clk: qcom: krait-cc: drop hardcoded safe_sel

[Ansuel Smith]

On Wed, Apr 13, 2022 at 08:25:31PM +0300, Dmitry Baryshkov wrote:
> On 22/03/2022 02:15, Ansuel Smith wrote:
> > Drop hardcoded safe_sel definition and use helper to correctly calculate
> > it. We assume qsb clk is always present as it should be declared in DTS
> > per Documentation and in the absence of that, it's declared as a fixed
> > clk.
> 
> Why? Can safe_sel (sec_mux index) change?
>

No it can't but I think it would be better to have stuff that is based
on real defined data instead of wrong struct that works just because a
hardcoded value is used.

Example for the reason of using this hardcoded value, nobody notice that
the mux list for the secondary mux was wrong. Now it didn't cause any
problem as we use the secondary mux just to source out of qsb and we
used the hardcoded value so the error was bypassed but as soon as the
value was actually parsed from the defined table, bam secondary mux was
sourcing out of pll8.

I honestly think that dropping the hardcoded value would make more clear
what the safe sel is and what is referring to.

> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >   drivers/clk/qcom/krait-cc.c | 40 +++++++++++++++++++++++++------------
> >   1 file changed, 27 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/clk/qcom/krait-cc.c b/drivers/clk/qcom/krait-cc.c
> > index e9508e3104ea..5f98ee1c3681 100644
> > --- a/drivers/clk/qcom/krait-cc.c
> > +++ b/drivers/clk/qcom/krait-cc.c
> > @@ -26,6 +26,17 @@ static unsigned int pri_mux_map[] = {
> >   	0,
> >   };
> > +static u8 krait_get_mux_sel(struct krait_mux_clk *mux, struct clk *safe_clk)
> > +{
> > +	struct clk_hw *safe_hw = __clk_get_hw(safe_clk);
> > +
> > +	/*
> > +	 * We can ignore errors from clk_hw_get_index_of_parent()
> > +	 * as we create these parents in this driver.
> > +	 */
> > +	return clk_hw_get_index_of_parent(&mux->hw, safe_hw);
> > +}
> > +
> >   /*
> >    * Notifier function for switching the muxes to safe parent
> >    * while the hfpll is getting reprogrammed.
> > @@ -116,8 +127,8 @@ krait_add_div(struct device *dev, int id, const char *s, unsigned int offset)
> >   }
> >   static struct clk *
> > -krait_add_sec_mux(struct device *dev, int id, const char *s,
> > -		  unsigned int offset, bool unique_aux)
> > +krait_add_sec_mux(struct device *dev, struct clk *qsb, int id,
> > +		  const char *s, unsigned int offset, bool unique_aux)
> >   {
> >   	int ret;
> >   	struct krait_mux_clk *mux;
> > @@ -144,7 +155,6 @@ krait_add_sec_mux(struct device *dev, int id, const char *s,
> >   	mux->shift = 2;
> >   	mux->parent_map = sec_mux_map;
> >   	mux->hw.init = &init;
> > -	mux->safe_sel = 0;
> >   	init.name = kasprintf(GFP_KERNEL, "krait%s_sec_mux", s);
> >   	if (!init.name)
> > @@ -166,6 +176,7 @@ krait_add_sec_mux(struct device *dev, int id, const char *s,
> >   	if (IS_ERR(clk))
> >   		goto err_clk;
> > +	mux->safe_sel = krait_get_mux_sel(mux, qsb);
> >   	ret = krait_notifier_register(dev, clk, mux);
> >   	if (ret)
> >   		clk = ERR_PTR(ret);
> > @@ -204,7 +215,6 @@ krait_add_pri_mux(struct device *dev, struct clk *hfpll_div, struct clk *sec_mux
> >   	mux->lpl = id >= 0;
> >   	mux->parent_map = pri_mux_map;
> >   	mux->hw.init = &init;
> > -	mux->safe_sel = 2;
> >   	init.name = kasprintf(GFP_KERNEL, "krait%s_pri_mux", s);
> >   	if (!init.name)
> > @@ -226,6 +236,7 @@ krait_add_pri_mux(struct device *dev, struct clk *hfpll_div, struct clk *sec_mux
> >   	if (IS_ERR(clk))
> >   		goto err_clk;
> > +	mux->safe_sel = krait_get_mux_sel(mux, sec_mux);
> >   	ret = krait_notifier_register(dev, clk, mux);
> >   	if (ret)
> >   		clk = ERR_PTR(ret);
> > @@ -238,7 +249,9 @@ krait_add_pri_mux(struct device *dev, struct clk *hfpll_div, struct clk *sec_mux
> >   }
> >   /* id < 0 for L2, otherwise id == physical CPU number */
> > -static struct clk *krait_add_clks(struct device *dev, int id, bool unique_aux)
> > +static struct clk *
> > +krait_add_clks(struct device *dev, struct clk *qsb, int id,
> > +	       bool unique_aux)
> >   {
> >   	unsigned int offset;
> >   	void *p = NULL;
> > @@ -261,7 +274,7 @@ static struct clk *krait_add_clks(struct device *dev, int id, bool unique_aux)
> >   		goto err;
> >   	}
> > -	sec_mux = krait_add_sec_mux(dev, id, s, offset, unique_aux);
> > +	sec_mux = krait_add_sec_mux(dev, qsb, id, s, offset, unique_aux);
> >   	if (IS_ERR(sec_mux)) {
> >   		clk = sec_mux;
> >   		goto err;
> > @@ -301,18 +314,19 @@ static int krait_cc_probe(struct platform_device *pdev)
> >   	int cpu;
> >   	struct clk *clk;
> >   	struct clk **clks;
> > -	struct clk *l2_pri_mux_clk;
> > +	struct clk *l2_pri_mux_clk, *qsb;
> >   	id = of_match_device(krait_cc_match_table, dev);
> >   	if (!id)
> >   		return -ENODEV;
> >   	/* Rate is 1 because 0 causes problems for __clk_mux_determine_rate */
> > -	if (IS_ERR(clk_get(dev, "qsb")))
> > -		clk = clk_register_fixed_rate(dev, "qsb", NULL, 0, 1);
> > +	qsb = clk_get(dev, "qsb");
> > +	if (IS_ERR(qsb))
> > +		qsb = clk_register_fixed_rate(dev, "qsb", NULL, 0, 1);
> > -	if (IS_ERR(clk))
> > -		return PTR_ERR(clk);
> > +	if (IS_ERR(qsb))
> > +		return PTR_ERR(qsb);
> >   	if (!id->data) {
> >   		clk = clk_register_fixed_factor(dev, "acpu_aux",
> > @@ -327,13 +341,13 @@ static int krait_cc_probe(struct platform_device *pdev)
> >   		return -ENOMEM;
> >   	for_each_possible_cpu(cpu) {
> > -		clk = krait_add_clks(dev, cpu, id->data);
> > +		clk = krait_add_clks(dev, qsb, cpu, id->data);
> >   		if (IS_ERR(clk))
> >   			return PTR_ERR(clk);
> >   		clks[cpu] = clk;
> >   	}
> > -	l2_pri_mux_clk = krait_add_clks(dev, -1, id->data);
> > +	l2_pri_mux_clk = krait_add_clks(dev, qsb, -1, id->data);
> >   	if (IS_ERR(l2_pri_mux_clk))
> >   		return PTR_ERR(l2_pri_mux_clk);
> >   	clks[4] = l2_pri_mux_clk;
> 
> 
> -- 
> With best wishes
> Dmitry

-- 
	Ansuel

Re: [PATCH v6 00/18] Modernize rest of the krait drivers

[Ansuel Smith]

On Wed, Apr 13, 2022 at 08:31:31PM +0300, Dmitry Baryshkov wrote:
> On 22/03/2022 02:15, Ansuel Smith wrote:
> > This is a follow-up to the ipq806x gcc modernize series. Manu cleanup
> > changes and also some discoveries of wrong definition notice only with
> > all these conversions.
> 
> General comment regarding this patch series. It contains fixes, clock
> conversion for several drivers, dts fixes, etc. It's, for example, not
> straightforwardly obvious if Bjorn can pickup patches 04 or 06 without
> picking up other patches.
> 
> If would be best if you can split this series or at least pull fixes to be
> the first patches in the pile.
>

Considering that now this is grown to 21 patch in v7 (that is still have
to push)... Yes I think I have to split this...
Wonder if you can give me some hint. 

- Series for krait-cc
- Series for kpss-acc/gcc
- Single patch for hfpll
- Single patch for gcc fixes
- Series for kpss-xcc
- Series for clk-krait
- Series for dts fixes?

Wonder if this kind of split can work?

> Patch 01 is only used by patch 10, they can stay close.
> 
> In some of the commit messages you describe what do they do, but you
> completely omit the reason for the change, why is the change necessary.
> (Yes, I spot that because I also too often skip that).
> 
> > 
> > The first patch is an improvement of the clk_hw_get_parent_index. The
> > original idea of clk_hw_get_parent_index was to give a way to access the
> > parent index but for some reason the final version limited it to the
> > current index. We change it to give the current parent if is not
> > provided and to give the requested parent if provided. Any user of this
> > function is updated to follow the new implementation.
> > 
> > The patch 2 and 3 are some additional fixes for gcc.
> > The first one is a fix that register the pxo and cxo fixed clock only if
> > they are not defined in DTS.
> > The patch 3 require some explaination. In short is a big HACK to prevent
> > kernel panic with this series.
> > 
> > The kpss-xcc driver is a mess.
> > The Documentation declare that the clocks should be provided but for some
> > reason it was never followed.
> > In fact in the ipq8064 DTSI only the clocks for l2cc are declared but
> > for cpu0 and cpu1 the clocks are not defined.
> > The kpss-xcc driver use parent_names so the clks are ignored and never
> > used so till now it wasn't a problem (ignoring the fact that they
> > doesn't follow documentation at all)
> > On top of that, the l2cc node declare the pxo clock in a really strange
> > way. It's declared using the PXO_SRC gcc clock that is never defined in
> > the gcc ipq8064 clock table. (the correct way was to declare a fixed
> > clock in dts and reference that)
> > To prevent any kind of problem we use the patch 3 and provide the clk
> > for PXO_SRC in the gcc clock table. We manually provide the clk after
> > gcc probe.
> > 
> > Patch 4 is just a minor cleanup where we use the poll macro
> > 
> > Patch 5 is the actually kpss-xcc conversion to parent data
> > 
> > Patch 6-7 should be a fixup of a real conver case
> > 
> > Patch 8 converts the krait-cc to parent_data
> > Patch 9 give some love to the code with some minor fixup
> > Patch 10 drop the hardcoded safe sel and use the new
> > clk_hw_get_parent_index to get the safe parent index.
> > (also I discovered that the parent order was wrong)
> > 
> > Patch 11 is an additional fixup to force the reset of the muxes even
> > more.
> > 
> > Patch 12-13 are some additiona taken from the qsdk that were missing in
> > the upstream driver
> > 
> > Patch 14 converts krait-cc to yaml
> > 
> > Patch 15 add to krait-cc Documentation the L2 clocks
> > 
> > Patch 16 converts the kpss-acc driver to yaml and fix some Documentation
> > error
> > 
> > Patch 17 convets the kpss-gcc driver to yaml
> > 
> > Patch 18 finally adds all this stuff to the ipq8064 dtsi (and fix the
> > stupid PXO_SRC phandle)
> > 
> > I tested this series on a ipq8064 SoC by running a cache benchmark test
> > to make sure the changes are correct and we don't silently cause
> > regressions. Also I compared the output of the clk_summary every time
> > and we finally have a sane output where the mux are correctly placed in
> > the correct parent. (till now we had the cpu aux clock all over the
> > place, probably never cause problems but who knows.)
> > 
> > v6:
> > - Move dts patch as last patch
> > - Address commencts from Rob
> > - Fix warning from make dtbs_check
> > v5:
> > - Address comments from Krzysztof
> > v4:
> > - Fix more dt-bindings bog errors
> > v3:
> > - Split Documentation files for kpss and krait-cc
> > v2:
> > - introduce new API instead of fixing the existing one
> > - do not reorganize variables in krait-cc
> > - fix some comments error and improve it
> > - return better error for patch 7
> > - fix missing new line on patch 16
> > 
> > Ansuel Smith (18):
> >    clk: introduce clk_hw_get_index_of_parent new API
> >    clk: qcom: gcc-ipq806x: skip pxo/cxo fixed clk if already present
> >    clk: qcom: gcc-ipq806x: add PXO_SRC in clk table
> >    clk: qcom: clk-hfpll: use poll_timeout macro
> >    clk: qcom: kpss-xcc: convert to parent data API
> >    clk: qcom: clk-krait: unlock spin after mux completion
> >    clk: qcom: clk-krait: add hw_parent check for div2_round_rate
> >    clk: qcom: krait-cc: convert to parent_data API
> >    clk: qcom: krait-cc: drop pr_info and register qsb only if needed
> >    clk: qcom: krait-cc: drop hardcoded safe_sel
> >    clk: qcom: krait-cc: force sec_mux to QSB
> >    clk: qcom: clk-krait: add apq/ipq8064 errata workaround
> >    clk: qcom: clk-krait: add enable disable ops
> >    dt-bindings: clock: Convert qcom,krait-cc to yaml
> >    dt-bindings: clock: Add L2 clocks to qcom,krait-cc Documentation
> >    dt-bindings: arm: msm: Convert kpss-acc driver Documentation to yaml
> >    dt-bindings: arm: msm: Convert kpss-gcc driver Documentation to yaml
> >    ARM: dts: qcom: qcom-ipq8064: add missing krait-cc compatible and
> >      clocks
> > 
> >   .../bindings/arm/msm/qcom,kpss-acc.txt        |  49 -----
> >   .../bindings/arm/msm/qcom,kpss-acc.yaml       |  94 +++++++++
> >   .../bindings/arm/msm/qcom,kpss-gcc.txt        |  44 -----
> >   .../bindings/arm/msm/qcom,kpss-gcc.yaml       |  69 +++++++
> >   .../bindings/clock/qcom,krait-cc.txt          |  34 ----
> >   .../bindings/clock/qcom,krait-cc.yaml         |  65 ++++++
> >   arch/arm/boot/dts/qcom-ipq8064.dtsi           |  24 ++-
> >   drivers/clk/clk.c                             |  14 ++
> >   drivers/clk/qcom/clk-hfpll.c                  |  13 +-
> >   drivers/clk/qcom/clk-krait.c                  |  44 ++++-
> >   drivers/clk/qcom/clk-krait.h                  |   1 +
> >   drivers/clk/qcom/gcc-ipq806x.c                |  27 ++-
> >   drivers/clk/qcom/kpss-xcc.c                   |  25 +--
> >   drivers/clk/qcom/krait-cc.c                   | 186 ++++++++++--------
> >   include/linux/clk-provider.h                  |   1 +
> >   15 files changed, 453 insertions(+), 237 deletions(-)
> >   delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.txt
> >   create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml
> >   delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
> >   create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
> >   delete mode 100644 Documentation/devicetree/bindings/clock/qcom,krait-cc.txt
> >   create mode 100644 Documentation/devicetree/bindings/clock/qcom,krait-cc.yaml
> > 
> 
> 
> -- 
> With best wishes
> Dmitry

-- 
	Ansuel

Re: [PATCH v6 03/18] clk: qcom: gcc-ipq806x: add PXO_SRC in clk table

[Ansuel Smith]

On Wed, Apr 13, 2022 at 08:32:21PM +0300, Dmitry Baryshkov wrote:
> On Wed, 13 Apr 2022 at 20:00, Ansuel Smith <ansuelsmth@gmail.com> wrote:
> >
> > On Thu, Mar 24, 2022 at 06:22:29PM -0700, Stephen Boyd wrote:
> > > Quoting Ansuel Smith (2022-03-24 18:13:49)
> > > > On Thu, Mar 24, 2022 at 06:10:35PM -0700, Stephen Boyd wrote:
> > > > > Quoting Ansuel Smith (2022-03-21 16:15:33)
> > > > > > PXO_SRC is currently defined in the gcc include and referenced in the
> > > > > > ipq8064 DTSI. Correctly provide a clk after gcc probe to fix kernel
> > > > > > panic if a driver starts to actually use it.
> > > > > >
> > > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > > > > > ---
> > > > >
> > > > > What is this patch about? clk providers shouldn't be calling clk_get().
> > > > >
> > > >
> > > > If pxo is passed as a clock in dts and defined as a fixed clock, what
> > > > should be used?
> > >
> > > clk_parent_data
> >
> > Sorry but I'm not following you. No idea if you missed the cover letter
> > where i describe the problem with PXO_SRC.
> >
> > The problem here is that
> > - In DTS we have node that reference <&gcc PXO_SRC>
> > But
> > - gcc driver NEVER defined PXO_SRC
> > As
> > - PXO_SRC is actually pxo_board that should be defined as a fixed-clock
> >   in dts or is defined using qcom_cc_register_board_clk.
> >
> > So in theory we should just put in PXO_SRC the clk hw of the
> > fixed-clock. That is why I'm using clk_get(). I can use __clk_lookup()
> > as an alternative but I really can't find a way to get the clock defined
> > from DTS or qcom_cc_register_board_clk.
> >
> > (I have the same exact problem with the cpu qsb clock where is defined
> > using fixed-clock API but can also defined directly in DTS and I have to
> > use clk_get())
> >
> > I'm totally missing something so I would love some hint on how to solve
> > this.
> 
> When we were doing such conversion for other  platforms, we pointed
> clock consumers to the board clocks directly. There is no need to go
> through the gcc to fetch pxo.
> Instead you can use a <&pxo_board> in the dts directly. Typically the
> sequence is the following:
> - Minor cleanup of the clock-controller driver
> (ARRAY_SIZE(parent_data), removal of unused clock sources, unused enum
> entries, etc)
> - update drivers to use both .name and  .fw_name in replacement of
> parent_names. Use parent_hws where possible.
> - update dtsi to reference clocks using clocks/clock-names properties.
> Pass board/rpmh/rpm clocks directly to their consumers without
> bandaids in the gcc driver.
> - (optionally) after several major releases drop parent_data.name
> completely. I think we mostly skipped this, since it provides no gain.
> 
> This way you don't have to play around clk_get to return PXO_SRC from
> gcc clock-controller.
> 
> -- 
> With best wishes
> Dmitry

Thanks for the list of steps to do this kind of cleanup.
From what I'm reading this series is ""stuck"" in the sense that I first
have to fix the wrong PXO_SRC reference and then I can continue the
conversion work. A bit sad considering most of the time DTS proposal got
ignored :(

-- 
	Ansuel

Re: [PATCH v6 10/18] clk: qcom: krait-cc: drop hardcoded safe_sel

[Dmitry Baryshkov]

On 13/04/2022 20:35, Ansuel Smith wrote:
> On Wed, Apr 13, 2022 at 08:25:31PM +0300, Dmitry Baryshkov wrote:
>> On 22/03/2022 02:15, Ansuel Smith wrote:
>>> Drop hardcoded safe_sel definition and use helper to correctly calculate
>>> it. We assume qsb clk is always present as it should be declared in DTS
>>> per Documentation and in the absence of that, it's declared as a fixed
>>> clk.
>>
>> Why? Can safe_sel (sec_mux index) change?
>>
> 
> No it can't but I think it would be better to have stuff that is based
> on real defined data instead of wrong struct that works just because a
> hardcoded value is used.
> 
> Example for the reason of using this hardcoded value, nobody notice that
> the mux list for the secondary mux was wrong. Now it didn't cause any
> problem as we use the secondary mux just to source out of qsb and we
> used the hardcoded value so the error was bypassed but as soon as the
> value was actually parsed from the defined table, bam secondary mux was
> sourcing out of pll8.
> 
> I honestly think that dropping the hardcoded value would make more clear
> what the safe sel is and what is referring to.

Can you define indices in the parents and use defined indice instead?

I see your point, but in my opinion adding an API to determine a 
compile-time constant value is a bit of overkill.

> 
>>>
>>> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
>>> ---
>>>    drivers/clk/qcom/krait-cc.c | 40 +++++++++++++++++++++++++------------
>>>    1 file changed, 27 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/clk/qcom/krait-cc.c b/drivers/clk/qcom/krait-cc.c
>>> index e9508e3104ea..5f98ee1c3681 100644
>>> --- a/drivers/clk/qcom/krait-cc.c
>>> +++ b/drivers/clk/qcom/krait-cc.c
>>> @@ -26,6 +26,17 @@ static unsigned int pri_mux_map[] = {
>>>    	0,
>>>    };
>>> +static u8 krait_get_mux_sel(struct krait_mux_clk *mux, struct clk *safe_clk)
>>> +{
>>> +	struct clk_hw *safe_hw = __clk_get_hw(safe_clk);
>>> +
>>> +	/*
>>> +	 * We can ignore errors from clk_hw_get_index_of_parent()
>>> +	 * as we create these parents in this driver.
>>> +	 */
>>> +	return clk_hw_get_index_of_parent(&mux->hw, safe_hw);
>>> +}
>>> +
>>>    /*
>>>     * Notifier function for switching the muxes to safe parent
>>>     * while the hfpll is getting reprogrammed.
>>> @@ -116,8 +127,8 @@ krait_add_div(struct device *dev, int id, const char *s, unsigned int offset)
>>>    }
>>>    static struct clk *
>>> -krait_add_sec_mux(struct device *dev, int id, const char *s,
>>> -		  unsigned int offset, bool unique_aux)
>>> +krait_add_sec_mux(struct device *dev, struct clk *qsb, int id,
>>> +		  const char *s, unsigned int offset, bool unique_aux)
>>>    {
>>>    	int ret;
>>>    	struct krait_mux_clk *mux;
>>> @@ -144,7 +155,6 @@ krait_add_sec_mux(struct device *dev, int id, const char *s,
>>>    	mux->shift = 2;
>>>    	mux->parent_map = sec_mux_map;
>>>    	mux->hw.init = &init;
>>> -	mux->safe_sel = 0;
>>>    	init.name = kasprintf(GFP_KERNEL, "krait%s_sec_mux", s);
>>>    	if (!init.name)
>>> @@ -166,6 +176,7 @@ krait_add_sec_mux(struct device *dev, int id, const char *s,
>>>    	if (IS_ERR(clk))
>>>    		goto err_clk;
>>> +	mux->safe_sel = krait_get_mux_sel(mux, qsb);
>>>    	ret = krait_notifier_register(dev, clk, mux);
>>>    	if (ret)
>>>    		clk = ERR_PTR(ret);
>>> @@ -204,7 +215,6 @@ krait_add_pri_mux(struct device *dev, struct clk *hfpll_div, struct clk *sec_mux
>>>    	mux->lpl = id >= 0;
>>>    	mux->parent_map = pri_mux_map;
>>>    	mux->hw.init = &init;
>>> -	mux->safe_sel = 2;
>>>    	init.name = kasprintf(GFP_KERNEL, "krait%s_pri_mux", s);
>>>    	if (!init.name)
>>> @@ -226,6 +236,7 @@ krait_add_pri_mux(struct device *dev, struct clk *hfpll_div, struct clk *sec_mux
>>>    	if (IS_ERR(clk))
>>>    		goto err_clk;
>>> +	mux->safe_sel = krait_get_mux_sel(mux, sec_mux);
>>>    	ret = krait_notifier_register(dev, clk, mux);
>>>    	if (ret)
>>>    		clk = ERR_PTR(ret);
>>> @@ -238,7 +249,9 @@ krait_add_pri_mux(struct device *dev, struct clk *hfpll_div, struct clk *sec_mux
>>>    }
>>>    /* id < 0 for L2, otherwise id == physical CPU number */
>>> -static struct clk *krait_add_clks(struct device *dev, int id, bool unique_aux)
>>> +static struct clk *
>>> +krait_add_clks(struct device *dev, struct clk *qsb, int id,
>>> +	       bool unique_aux)
>>>    {
>>>    	unsigned int offset;
>>>    	void *p = NULL;
>>> @@ -261,7 +274,7 @@ static struct clk *krait_add_clks(struct device *dev, int id, bool unique_aux)
>>>    		goto err;
>>>    	}
>>> -	sec_mux = krait_add_sec_mux(dev, id, s, offset, unique_aux);
>>> +	sec_mux = krait_add_sec_mux(dev, qsb, id, s, offset, unique_aux);
>>>    	if (IS_ERR(sec_mux)) {
>>>    		clk = sec_mux;
>>>    		goto err;
>>> @@ -301,18 +314,19 @@ static int krait_cc_probe(struct platform_device *pdev)
>>>    	int cpu;
>>>    	struct clk *clk;
>>>    	struct clk **clks;
>>> -	struct clk *l2_pri_mux_clk;
>>> +	struct clk *l2_pri_mux_clk, *qsb;
>>>    	id = of_match_device(krait_cc_match_table, dev);
>>>    	if (!id)
>>>    		return -ENODEV;
>>>    	/* Rate is 1 because 0 causes problems for __clk_mux_determine_rate */
>>> -	if (IS_ERR(clk_get(dev, "qsb")))
>>> -		clk = clk_register_fixed_rate(dev, "qsb", NULL, 0, 1);
>>> +	qsb = clk_get(dev, "qsb");
>>> +	if (IS_ERR(qsb))
>>> +		qsb = clk_register_fixed_rate(dev, "qsb", NULL, 0, 1);
>>> -	if (IS_ERR(clk))
>>> -		return PTR_ERR(clk);
>>> +	if (IS_ERR(qsb))
>>> +		return PTR_ERR(qsb);
>>>    	if (!id->data) {
>>>    		clk = clk_register_fixed_factor(dev, "acpu_aux",
>>> @@ -327,13 +341,13 @@ static int krait_cc_probe(struct platform_device *pdev)
>>>    		return -ENOMEM;
>>>    	for_each_possible_cpu(cpu) {
>>> -		clk = krait_add_clks(dev, cpu, id->data);
>>> +		clk = krait_add_clks(dev, qsb, cpu, id->data);
>>>    		if (IS_ERR(clk))
>>>    			return PTR_ERR(clk);
>>>    		clks[cpu] = clk;
>>>    	}
>>> -	l2_pri_mux_clk = krait_add_clks(dev, -1, id->data);
>>> +	l2_pri_mux_clk = krait_add_clks(dev, qsb, -1, id->data);
>>>    	if (IS_ERR(l2_pri_mux_clk))
>>>    		return PTR_ERR(l2_pri_mux_clk);
>>>    	clks[4] = l2_pri_mux_clk;
>>
>>
>> -- 
>> With best wishes
>> Dmitry
> 


-- 
With best wishes
Dmitry

Re: [PATCH v6 05/18] clk: qcom: kpss-xcc: convert to parent data API

[Dmitry Baryshkov]

On 22/03/2022 02:15, Ansuel Smith wrote:
> Convert the driver to parent data API. From the Documentation pll8_vote
> and pxo should be declared in the DTS so fw_name can be used instead of
> parent_names. Name is still used to save regression on old definition.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>   drivers/clk/qcom/kpss-xcc.c | 25 ++++++++-----------------
>   1 file changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/clk/qcom/kpss-xcc.c b/drivers/clk/qcom/kpss-xcc.c
> index 4fec1f9142b8..347f70e9f5fe 100644
> --- a/drivers/clk/qcom/kpss-xcc.c
> +++ b/drivers/clk/qcom/kpss-xcc.c
> @@ -12,9 +12,9 @@
>   #include <linux/clk.h>
>   #include <linux/clk-provider.h>
>   
> -static const char *aux_parents[] = {
> -	"pll8_vote",
> -	"pxo",
> +static const struct clk_parent_data aux_parents[] = {
> +	{ .name = "pll8_vote", .fw_name = "pll8_vote" },

I'd just use "pll" here for the .fw_name.

> +	{ .name = "pxo", .fw_name = "pxo" },
>   };
>   
>   static unsigned int aux_parent_map[] = {
> @@ -32,8 +32,8 @@ MODULE_DEVICE_TABLE(of, kpss_xcc_match_table);
>   static int kpss_xcc_driver_probe(struct platform_device *pdev)
>   {
>   	const struct of_device_id *id;
> -	struct clk *clk;
>   	void __iomem *base;
> +	struct clk_hw *hw;
>   	const char *name;
>   
>   	id = of_match_device(kpss_xcc_match_table, &pdev->dev);
> @@ -55,24 +55,15 @@ static int kpss_xcc_driver_probe(struct platform_device *pdev)
>   		base += 0x28;
>   	}
>   
> -	clk = clk_register_mux_table(&pdev->dev, name, aux_parents,
> -				     ARRAY_SIZE(aux_parents), 0, base, 0, 0x3,
> -				     0, aux_parent_map, NULL);
> +	hw = __devm_clk_hw_register_mux(&pdev->dev, NULL, name, ARRAY_SIZE(aux_parents),
> +					NULL, NULL, aux_parents, 0, base, 0, 0x3,
> +					0, aux_parent_map, NULL);
>   
> -	platform_set_drvdata(pdev, clk);
> -
> -	return PTR_ERR_OR_ZERO(clk);
> -}
> -
> -static int kpss_xcc_driver_remove(struct platform_device *pdev)
> -{
> -	clk_unregister_mux(platform_get_drvdata(pdev));
> -	return 0;
> +	return PTR_ERR_OR_ZERO(hw);
>   }
>   
>   static struct platform_driver kpss_xcc_driver = {
>   	.probe = kpss_xcc_driver_probe,
> -	.remove = kpss_xcc_driver_remove,
>   	.driver = {
>   		.name = "kpss-xcc",
>   		.of_match_table = kpss_xcc_match_table,


-- 
With best wishes
Dmitry

Re: [PATCH v6 09/18] clk: qcom: krait-cc: drop pr_info and register qsb only if needed

[Dmitry Baryshkov]

On 22/03/2022 02:15, Ansuel Smith wrote:
> Replace pr_info() with dev_info() to provide better diagnostics.
> Register qsb fixed clk only if it's not declared in DTS.

This can be split into two patches

> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>   drivers/clk/qcom/krait-cc.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/clk/qcom/krait-cc.c b/drivers/clk/qcom/krait-cc.c
> index 645ad9e8dd73..e9508e3104ea 100644
> --- a/drivers/clk/qcom/krait-cc.c
> +++ b/drivers/clk/qcom/krait-cc.c
> @@ -308,7 +308,9 @@ static int krait_cc_probe(struct platform_device *pdev)
>   		return -ENODEV;
>   
>   	/* Rate is 1 because 0 causes problems for __clk_mux_determine_rate */
> -	clk = clk_register_fixed_rate(dev, "qsb", NULL, 0, 1);
> +	if (IS_ERR(clk_get(dev, "qsb")))
> +		clk = clk_register_fixed_rate(dev, "qsb", NULL, 0, 1);
> +
>   	if (IS_ERR(clk))
>   		return PTR_ERR(clk);
>   
> @@ -363,25 +365,25 @@ static int krait_cc_probe(struct platform_device *pdev)
>   	cur_rate = clk_get_rate(l2_pri_mux_clk);
>   	aux_rate = 384000000;
>   	if (cur_rate == 1) {
> -		pr_info("L2 @ QSB rate. Forcing new rate.\n");
> +		dev_info(dev, "L2 @ QSB rate. Forcing new rate.\n");
>   		cur_rate = aux_rate;
>   	}
>   	clk_set_rate(l2_pri_mux_clk, aux_rate);
>   	clk_set_rate(l2_pri_mux_clk, 2);
>   	clk_set_rate(l2_pri_mux_clk, cur_rate);
> -	pr_info("L2 @ %lu KHz\n", clk_get_rate(l2_pri_mux_clk) / 1000);
> +	dev_info(dev, "L2 @ %lu KHz\n", clk_get_rate(l2_pri_mux_clk) / 1000);
>   	for_each_possible_cpu(cpu) {
>   		clk = clks[cpu];
>   		cur_rate = clk_get_rate(clk);
>   		if (cur_rate == 1) {
> -			pr_info("CPU%d @ QSB rate. Forcing new rate.\n", cpu);
> +			dev_info(dev, "CPU%d @ QSB rate. Forcing new rate.\n", cpu);
>   			cur_rate = aux_rate;
>   		}
>   
>   		clk_set_rate(clk, aux_rate);
>   		clk_set_rate(clk, 2);
>   		clk_set_rate(clk, cur_rate);
> -		pr_info("CPU%d @ %lu KHz\n", cpu, clk_get_rate(clk) / 1000);
> +		dev_info(dev, "CPU%d @ %lu KHz\n", cpu, clk_get_rate(clk) / 1000);
>   	}
>   
>   	of_clk_add_provider(dev->of_node, krait_of_get, clks);


-- 
With best wishes
Dmitry

Re: [PATCH v6 00/18] Modernize rest of the krait drivers

[Dmitry Baryshkov]

On Wed, 13 Apr 2022 at 20:48, Ansuel Smith <ansuelsmth@gmail.com> wrote:
>
> On Wed, Apr 13, 2022 at 08:31:31PM +0300, Dmitry Baryshkov wrote:
> > On 22/03/2022 02:15, Ansuel Smith wrote:
> > > This is a follow-up to the ipq806x gcc modernize series. Manu cleanup
> > > changes and also some discoveries of wrong definition notice only with
> > > all these conversions.
> >
> > General comment regarding this patch series. It contains fixes, clock
> > conversion for several drivers, dts fixes, etc. It's, for example, not
> > straightforwardly obvious if Bjorn can pickup patches 04 or 06 without
> > picking up other patches.
> >
> > If would be best if you can split this series or at least pull fixes to be
> > the first patches in the pile.
> >
>
> Considering that now this is grown to 21 patch in v7 (that is still have
> to push)... Yes I think I have to split this...
> Wonder if you can give me some hint.
>
> - Series for krait-cc
> - Series for kpss-acc/gcc
> - Single patch for hfpll
> - Single patch for gcc fixes
> - Series for kpss-xcc
> - Series for clk-krait
> - Series for dts fixes?

Yes, this sounds more or less logical. hfpll can go together with any
of the krait patches.
If you'd like a fewer series, the following looks sane too:
- fixes for hfpll, clk-krait, etc. Small changes that can be picked up
immediately.
- modernize gcc,
- update dts to follow gcc changes
- cpufreq drivers conversion
- update dts to follow cpufreq changes

As a generic notice: it might sound awkward, but could you please
split dt-bindings conversions (that were not R-B yet) into separate
parts:
- Just convert to yaml, no changes
- fix this-and-that.

I think Krzyshtof and Rob has already asked for that, but it's still
worth mentioning.

To stop from flooding the list, what about:
- posting fixes patches
- posting and polishing gcc conversion + dts updates

The rest can come up later. It might sound like a delay, but in
reality it might be easier to review.

> Wonder if this kind of split can work?
>
> > Patch 01 is only used by patch 10, they can stay close.
> >
> > In some of the commit messages you describe what do they do, but you
> > completely omit the reason for the change, why is the change necessary.
> > (Yes, I spot that because I also too often skip that).
> >
> > >
> > > The first patch is an improvement of the clk_hw_get_parent_index. The
> > > original idea of clk_hw_get_parent_index was to give a way to access the
> > > parent index but for some reason the final version limited it to the
> > > current index. We change it to give the current parent if is not
> > > provided and to give the requested parent if provided. Any user of this
> > > function is updated to follow the new implementation.
> > >
> > > The patch 2 and 3 are some additional fixes for gcc.
> > > The first one is a fix that register the pxo and cxo fixed clock only if
> > > they are not defined in DTS.
> > > The patch 3 require some explaination. In short is a big HACK to prevent
> > > kernel panic with this series.
> > >
> > > The kpss-xcc driver is a mess.
> > > The Documentation declare that the clocks should be provided but for some
> > > reason it was never followed.
> > > In fact in the ipq8064 DTSI only the clocks for l2cc are declared but
> > > for cpu0 and cpu1 the clocks are not defined.
> > > The kpss-xcc driver use parent_names so the clks are ignored and never
> > > used so till now it wasn't a problem (ignoring the fact that they
> > > doesn't follow documentation at all)
> > > On top of that, the l2cc node declare the pxo clock in a really strange
> > > way. It's declared using the PXO_SRC gcc clock that is never defined in
> > > the gcc ipq8064 clock table. (the correct way was to declare a fixed
> > > clock in dts and reference that)
> > > To prevent any kind of problem we use the patch 3 and provide the clk
> > > for PXO_SRC in the gcc clock table. We manually provide the clk after
> > > gcc probe.
> > >
> > > Patch 4 is just a minor cleanup where we use the poll macro
> > >
> > > Patch 5 is the actually kpss-xcc conversion to parent data
> > >
> > > Patch 6-7 should be a fixup of a real conver case
> > >
> > > Patch 8 converts the krait-cc to parent_data
> > > Patch 9 give some love to the code with some minor fixup
> > > Patch 10 drop the hardcoded safe sel and use the new
> > > clk_hw_get_parent_index to get the safe parent index.
> > > (also I discovered that the parent order was wrong)
> > >
> > > Patch 11 is an additional fixup to force the reset of the muxes even
> > > more.
> > >
> > > Patch 12-13 are some additiona taken from the qsdk that were missing in
> > > the upstream driver
> > >
> > > Patch 14 converts krait-cc to yaml
> > >
> > > Patch 15 add to krait-cc Documentation the L2 clocks
> > >
> > > Patch 16 converts the kpss-acc driver to yaml and fix some Documentation
> > > error
> > >
> > > Patch 17 convets the kpss-gcc driver to yaml
> > >
> > > Patch 18 finally adds all this stuff to the ipq8064 dtsi (and fix the
> > > stupid PXO_SRC phandle)
> > >
> > > I tested this series on a ipq8064 SoC by running a cache benchmark test
> > > to make sure the changes are correct and we don't silently cause
> > > regressions. Also I compared the output of the clk_summary every time
> > > and we finally have a sane output where the mux are correctly placed in
> > > the correct parent. (till now we had the cpu aux clock all over the
> > > place, probably never cause problems but who knows.)
> > >
> > > v6:
> > > - Move dts patch as last patch
> > > - Address commencts from Rob
> > > - Fix warning from make dtbs_check
> > > v5:
> > > - Address comments from Krzysztof
> > > v4:
> > > - Fix more dt-bindings bog errors
> > > v3:
> > > - Split Documentation files for kpss and krait-cc
> > > v2:
> > > - introduce new API instead of fixing the existing one
> > > - do not reorganize variables in krait-cc
> > > - fix some comments error and improve it
> > > - return better error for patch 7
> > > - fix missing new line on patch 16
> > >
> > > Ansuel Smith (18):
> > >    clk: introduce clk_hw_get_index_of_parent new API
> > >    clk: qcom: gcc-ipq806x: skip pxo/cxo fixed clk if already present
> > >    clk: qcom: gcc-ipq806x: add PXO_SRC in clk table
> > >    clk: qcom: clk-hfpll: use poll_timeout macro
> > >    clk: qcom: kpss-xcc: convert to parent data API
> > >    clk: qcom: clk-krait: unlock spin after mux completion
> > >    clk: qcom: clk-krait: add hw_parent check for div2_round_rate
> > >    clk: qcom: krait-cc: convert to parent_data API
> > >    clk: qcom: krait-cc: drop pr_info and register qsb only if needed
> > >    clk: qcom: krait-cc: drop hardcoded safe_sel
> > >    clk: qcom: krait-cc: force sec_mux to QSB
> > >    clk: qcom: clk-krait: add apq/ipq8064 errata workaround
> > >    clk: qcom: clk-krait: add enable disable ops
> > >    dt-bindings: clock: Convert qcom,krait-cc to yaml
> > >    dt-bindings: clock: Add L2 clocks to qcom,krait-cc Documentation
> > >    dt-bindings: arm: msm: Convert kpss-acc driver Documentation to yaml
> > >    dt-bindings: arm: msm: Convert kpss-gcc driver Documentation to yaml
> > >    ARM: dts: qcom: qcom-ipq8064: add missing krait-cc compatible and
> > >      clocks
> > >
> > >   .../bindings/arm/msm/qcom,kpss-acc.txt        |  49 -----
> > >   .../bindings/arm/msm/qcom,kpss-acc.yaml       |  94 +++++++++
> > >   .../bindings/arm/msm/qcom,kpss-gcc.txt        |  44 -----
> > >   .../bindings/arm/msm/qcom,kpss-gcc.yaml       |  69 +++++++
> > >   .../bindings/clock/qcom,krait-cc.txt          |  34 ----
> > >   .../bindings/clock/qcom,krait-cc.yaml         |  65 ++++++
> > >   arch/arm/boot/dts/qcom-ipq8064.dtsi           |  24 ++-
> > >   drivers/clk/clk.c                             |  14 ++
> > >   drivers/clk/qcom/clk-hfpll.c                  |  13 +-
> > >   drivers/clk/qcom/clk-krait.c                  |  44 ++++-
> > >   drivers/clk/qcom/clk-krait.h                  |   1 +
> > >   drivers/clk/qcom/gcc-ipq806x.c                |  27 ++-
> > >   drivers/clk/qcom/kpss-xcc.c                   |  25 +--
> > >   drivers/clk/qcom/krait-cc.c                   | 186 ++++++++++--------
> > >   include/linux/clk-provider.h                  |   1 +
> > >   15 files changed, 453 insertions(+), 237 deletions(-)
> > >   delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.txt
> > >   create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-acc.yaml
> > >   delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
> > >   create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
> > >   delete mode 100644 Documentation/devicetree/bindings/clock/qcom,krait-cc.txt
> > >   create mode 100644 Documentation/devicetree/bindings/clock/qcom,krait-cc.yaml
> > >
> >
> >
> > --
> > With best wishes
> > Dmitry
>
> --
>         Ansuel



-- 
With best wishes
Dmitry

Re: [PATCH v6 03/18] clk: qcom: gcc-ipq806x: add PXO_SRC in clk table

[Dmitry Baryshkov]

On 13/04/2022 20:54, Ansuel Smith wrote:
> On Wed, Apr 13, 2022 at 08:32:21PM +0300, Dmitry Baryshkov wrote:
>> On Wed, 13 Apr 2022 at 20:00, Ansuel Smith <ansuelsmth@gmail.com> wrote:
>>>
>>> On Thu, Mar 24, 2022 at 06:22:29PM -0700, Stephen Boyd wrote:
>>>> Quoting Ansuel Smith (2022-03-24 18:13:49)
>>>>> On Thu, Mar 24, 2022 at 06:10:35PM -0700, Stephen Boyd wrote:
>>>>>> Quoting Ansuel Smith (2022-03-21 16:15:33)
>>>>>>> PXO_SRC is currently defined in the gcc include and referenced in the
>>>>>>> ipq8064 DTSI. Correctly provide a clk after gcc probe to fix kernel
>>>>>>> panic if a driver starts to actually use it.
>>>>>>>
>>>>>>> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
>>>>>>> ---
>>>>>>
>>>>>> What is this patch about? clk providers shouldn't be calling clk_get().
>>>>>>
>>>>>
>>>>> If pxo is passed as a clock in dts and defined as a fixed clock, what
>>>>> should be used?
>>>>
>>>> clk_parent_data
>>>
>>> Sorry but I'm not following you. No idea if you missed the cover letter
>>> where i describe the problem with PXO_SRC.
>>>
>>> The problem here is that
>>> - In DTS we have node that reference <&gcc PXO_SRC>
>>> But
>>> - gcc driver NEVER defined PXO_SRC
>>> As
>>> - PXO_SRC is actually pxo_board that should be defined as a fixed-clock
>>>    in dts or is defined using qcom_cc_register_board_clk.
>>>
>>> So in theory we should just put in PXO_SRC the clk hw of the
>>> fixed-clock. That is why I'm using clk_get(). I can use __clk_lookup()
>>> as an alternative but I really can't find a way to get the clock defined
>>> from DTS or qcom_cc_register_board_clk.
>>>
>>> (I have the same exact problem with the cpu qsb clock where is defined
>>> using fixed-clock API but can also defined directly in DTS and I have to
>>> use clk_get())
>>>
>>> I'm totally missing something so I would love some hint on how to solve
>>> this.
>>
>> When we were doing such conversion for other  platforms, we pointed
>> clock consumers to the board clocks directly. There is no need to go
>> through the gcc to fetch pxo.
>> Instead you can use a <&pxo_board> in the dts directly. Typically the
>> sequence is the following:
>> - Minor cleanup of the clock-controller driver
>> (ARRAY_SIZE(parent_data), removal of unused clock sources, unused enum
>> entries, etc)
>> - update drivers to use both .name and  .fw_name in replacement of
>> parent_names. Use parent_hws where possible.
>> - update dtsi to reference clocks using clocks/clock-names properties.
>> Pass board/rpmh/rpm clocks directly to their consumers without
>> bandaids in the gcc driver.
>> - (optionally) after several major releases drop parent_data.name
>> completely. I think we mostly skipped this, since it provides no gain.
>>
>> This way you don't have to play around clk_get to return PXO_SRC from
>> gcc clock-controller.
>>
>> -- 
>> With best wishes
>> Dmitry
> 
> Thanks for the list of steps to do this kind of cleanup.
>  From what I'm reading this series is ""stuck"" in the sense that I first
> have to fix the wrong PXO_SRC reference and then I can continue the
> conversion work. A bit sad considering most of the time DTS proposal got
> ignored :(

Not really. You can leave "pxo" as is. Use { .fw_name = "pxo", .name = 
"pxo_board" } as parent_data. Then pass <&pxo_board> as the "pxo" clock 
to the consumers. Yes, you will still have the lingering "pxo" / "cxo" 
clocks at this step. It's okay, they might be used by other drivers.

After the whole conversion is finished, you can make "pxo"/"cxo" 
registration conditional on !of_find_property("clocks") rather than 
using clk_get.

As a rule of thumb, you don't have to complete the whole thing in a 
single commit. Having smaller commits might be better.

[And yes, I'm looking forward to testing your cpufreq changes on my 
apq8064 devices].

-- 
With best wishes
Dmitry