Modernize rest of the krait drivers

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 <[email protected]>

---
 drivers/clk/clk.c            | 14 ++++++++++++++
 include/linux/clk-provider.h |  1 +
 2 files changed, 15 insertions(+)

drivers/clk/clk.c +0 -0

index 8de6a22498e7..bdd70a88394c 100644

drivers/clk/clk.c +7 -0

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

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

Signed-off-by Ansuel Smith <[email protected]>

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 <[email protected]>

---

 drivers/clk/qcom/gcc-ipq806x.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/clk/qcom/gcc-ipq806x.c +7 -0

index 27f6d7626abb..7271d3afdc89 100644
@@ -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;

Signature · 1 line

2.34.1

Quoting Ansuel Smith (2022-03-21 16:15:33)

Quoted 6 lines

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 <[email protected]>

---

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

Quoted 38 lines

 drivers/clk/qcom/gcc-ipq806x.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/clk/qcom/gcc-ipq806x.c +7 -0

index 27f6d7626abb..7271d3afdc89 100644
@@ -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;

Signature · 1 line

2.34.1

Quoted 5 lines

Quoting Ansuel Smith (2022-03-21 16:15:33)

Quoted 4 lines

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.

Quoted 5 lines

Quoted 2 lines

Signed-off-by Ansuel Smith <[email protected]>

---

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?

Quoting Ansuel Smith (2022-03-24 18:13:49)

Quoted 9 lines · Stephen Boyd

Quoted 8 lines

Quoting Ansuel Smith (2022-03-21 16:15:33)

Quoted 6 lines

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 <[email protected]>

---

Quoted 5 lines

Quoted 2 lines

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

Quoted 10 lines · Stephen Boyd

Quoting Ansuel Smith (2022-03-24 18:13:49)

Quoted 9 lines · Stephen Boyd

Quoted 8 lines

Quoting Ansuel Smith (2022-03-21 16:15:33)

Quoted 6 lines

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 <[email protected]>

---

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

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.

Quoted 36 lines · Ansuel Smith

Quoted 10 lines · Stephen Boyd

Quoting Ansuel Smith (2022-03-24 18:13:49)

Quoted 9 lines · Stephen Boyd

Quoted 8 lines

Quoting Ansuel Smith (2022-03-21 16:15:33)

Quoted 6 lines

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 <[email protected]>

---

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

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.

Quoted 6 lines

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

Signature · 2 lines

With best wishes
Dmitry

Quoted 60 lines · Dmitry Baryshkov

Quoted 36 lines · Ansuel Smith

Quoted 10 lines · Stephen Boyd

Quoting Ansuel Smith (2022-03-24 18:13:49)

Quoted 9 lines · Stephen Boyd

Quoted 8 lines

Quoting Ansuel Smith (2022-03-21 16:15:33)

Quoted 6 lines

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 <[email protected]>

---

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

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.

Quoted 6 lines

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

Quoted 6 lines

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

Signature · 2 lines

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 :(

Use regmap_read_poll_timeout macro instead of do-while structure.

drivers/clk/qcom/clk-hfpll.c +1 -0

+
 #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);

drivers/clk/qcom/clk-hfpll.c +2 -1

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

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 <[email protected]>

---
 drivers/clk/qcom/kpss-xcc.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

drivers/clk/qcom/kpss-xcc.c +4 -4

index 4fec1f9142b8..347f70e9f5fe 100644
@@ -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);

drivers/clk/qcom/kpss-xcc.c +2 -0

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

Quoting Ansuel Smith (2022-03-21 16:15:35)

Quoted 40 lines

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 <[email protected]>

---
 drivers/clk/qcom/kpss-xcc.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

drivers/clk/qcom/kpss-xcc.c +4 -4

index 4fec1f9142b8..347f70e9f5fe 100644
@@ -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;
        }
 

drivers/clk/qcom/kpss-xcc.c static int kpss_xcc_driver_probe(struct platform_device *pdev) · line 58

Quoted 4 lines

drivers/clk/qcom/kpss-xcc.c +1 -3

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

Quoted 3 lines

drivers/clk/qcom/kpss-xcc.c +2 -0

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

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 <[email protected]>

---
  drivers/clk/qcom/kpss-xcc.c | 25 ++++++++-----------------
  1 file changed, 8 insertions(+), 17 deletions(-)

drivers/clk/qcom/kpss-xcc.c +0 -0

index 4fec1f9142b8..347f70e9f5fe 100644

drivers/clk/qcom/kpss-xcc.c +6 -14

+	{ .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,

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 <[email protected]>

---

 drivers/clk/qcom/clk-krait.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/clk/qcom/clk-krait.c +0 -0

index 59f1af415b58..e447fcc3806d 100644

drivers/clk/qcom/clk-krait.c +1 -0

+       spin_unlock_irqrestore(&krait_clock_reg_lock, flags);
 }
 

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 <[email protected]>

---
  drivers/clk/qcom/krait-cc.c | 12 +++++++-----
  1 file changed, 7 insertions(+), 5 deletions(-)

drivers/clk/qcom/krait-cc.c +7 -5

index 645ad9e8dd73..e9508e3104ea 100644
@@ -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);

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 <[email protected]>

---
  drivers/clk/qcom/krait-cc.c | 40 +++++++++++++++++++++++++------------
  1 file changed, 27 insertions(+), 13 deletions(-)

drivers/clk/qcom/krait-cc.c +27 -13

index e9508e3104ea..5f98ee1c3681 100644
@@ -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;

Quoted 4 lines · 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.

Why? Can safe_sel (sec_mux index) change?

Quoted 118 lines

Signed-off-by Ansuel Smith <[email protected]>

---
  drivers/clk/qcom/krait-cc.c | 40 +++++++++++++++++++++++++------------
  1 file changed, 27 insertions(+), 13 deletions(-)

drivers/clk/qcom/krait-cc.c +27 -13

index e9508e3104ea..5f98ee1c3681 100644
@@ -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;

Signature · 2 lines

With best wishes
Dmitry

Quoted 8 lines · Dmitry Baryshkov

Quoted 4 lines · 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.

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.

Quoted 124 lines

Quoted 119 lines

Signed-off-by Ansuel Smith <[email protected]>

---
   drivers/clk/qcom/krait-cc.c | 40 +++++++++++++++++++++++++------------
   1 file changed, 27 insertions(+), 13 deletions(-)

drivers/clk/qcom/krait-cc.c +27 -13

index e9508e3104ea..5f98ee1c3681 100644
@@ -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;

Signature · 2 lines

With best wishes
Dmitry

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

Signed-off-by Ansuel Smith <[email protected]>

---
  drivers/clk/qcom/clk-krait.c | 18 ++++++++++++++++++
  1 file changed, 18 insertions(+)

drivers/clk/qcom/clk-krait.c +18 -0

index 7ba5dbc72bce..061af57b0ec2 100644
@@ -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,

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 <[email protected]>

Reviewed-by Krzysztof Kozlowski <[email protected]>

---
 .../devicetree/bindings/clock/qcom,krait-cc.yaml       | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

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 <[email protected]>

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

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 <[email protected]>

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

Quoted 11 lines · 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 <[email protected]>

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

Quoted 31 lines · Rob Herring

Quoted 11 lines · 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 <[email protected]>

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

Quoted 8 lines

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?

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 <[email protected]>

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

Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt deleted +0 -44

deleted file mode 100644
index e628758950e1..000000000000
@@ -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";
-	};

Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml new file +15 -0

new file mode 100644
index 000000000000..7eb852be02c1
@@ -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 <[email protected]>
+
+description: |
+  Krait Processor Sub-system (KPSS) Global Clock Controller (GCC). Used
+  to control L2 mux (in the current implementation).
+

Quoted 82 lines · 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 <[email protected]>

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

Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt deleted +0 -44

deleted file mode 100644
index e628758950e1..000000000000
@@ -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";
-	};

Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml new file +15 -0

new file mode 100644
index 000000000000..7eb852be02c1
@@ -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 <[email protected]>
+
+description: |
+  Krait Processor Sub-system (KPSS) Global Clock Controller (GCC). Used
+  to control L2 mux (in the current implementation).
+

Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml line 25

Quoted 10 lines

Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml new file +10 -0

+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

Quoted 109 lines · Krzysztof Kozlowski

Quoted 82 lines · 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 <[email protected]>

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

Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt deleted +0 -44

deleted file mode 100644
index e628758950e1..000000000000
@@ -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";
-	};

Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml new file +15 -0

new file mode 100644
index 000000000000..7eb852be02c1
@@ -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 <[email protected]>
+
+description: |
+  Krait Processor Sub-system (KPSS) Global Clock Controller (GCC). Used
+  to control L2 mux (in the current implementation).
+

Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml line 25

Quoted 10 lines

Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml new file +10 -0

+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. :(

Quoted 2 lines

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.

Quoted 110 lines · Ansuel Smith

Quoted 109 lines · Krzysztof Kozlowski

Quoted 82 lines · 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 <[email protected]>

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

Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt deleted +0 -44

deleted file mode 100644
index e628758950e1..000000000000
@@ -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";
-	};

Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml new file +15 -0

new file mode 100644
index 000000000000..7eb852be02c1
@@ -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 <[email protected]>
+
+description: |
+  Krait Processor Sub-system (KPSS) Global Clock Controller (GCC). Used
+  to control L2 mux (in the current implementation).
+

Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml line 25

Quoted 10 lines

Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml new file +10 -0

+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. :(

Quoted 8 lines

Quoted 2 lines

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.

Quoted 3 lines

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

Quoted 7 lines

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.

Quoted 3 lines

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?

Quoted 4 lines

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?

Quoted 3 lines

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

Quoted 11 lines

Quoted 7 lines

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.

Quoted 9 lines

Quoted 3 lines

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?

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

Quoted 4 lines

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.

Quoted 3 lines

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.

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 <[email protected]>

---
 arch/arm/boot/dts/qcom-ipq8064.dtsi | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

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

Quoted 71 lines · 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.)

Quoted 16 lines

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

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

Quoted 3 lines · 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.

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

Quoted 128 lines

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

Signature · 2 lines

With best wishes
Dmitry

Quoted 5 lines · Dmitry Baryshkov

Quoted 3 lines · 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.

Quoted 8 lines

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?

Quoted 136 lines

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

Quoted 121 lines

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

Quoted 7 lines

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

Quoted 3 lines

--
With best wishes
Dmitry

--
Ansuel