[PATCH 1/2] dt-bindings: clock: u8500: Add clkout clock bindings

[PATCH 1/2] dt-bindings: clock: u8500: Add clkout clock bindings

[Linus Walleij]

This adds device tree bindings for the externally routed clocks
CLKOUT1 and CLKOUT2 clocks found in the DB8500.

Cc: Ulf Hansson <ulf.hansson@linaro.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 .../bindings/clock/stericsson,u8500-clks.yaml   | 16 ++++++++++++++++
 include/dt-bindings/clock/ste-db8500-clkout.h   | 17 +++++++++++++++++
 2 files changed, 33 insertions(+)
 create mode 100644 include/dt-bindings/clock/ste-db8500-clkout.h

diff --git a/Documentation/devicetree/bindings/clock/stericsson,u8500-clks.yaml b/Documentation/devicetree/bindings/clock/stericsson,u8500-clks.yaml
index 9bc95a308477..afd049be948a 100644
--- a/Documentation/devicetree/bindings/clock/stericsson,u8500-clks.yaml
+++ b/Documentation/devicetree/bindings/clock/stericsson,u8500-clks.yaml
@@ -109,6 +109,22 @@ properties:
 
     additionalProperties: false
 
+  clkout-clock:
+    description: A subnode with three clock cells for externally routed clocks,
+      output clocks. These are two PRCMU-internal clocks that can be divided and
+      muxed out on the pads of the DB8500 SoC. The first cell indicates which
+      output clock we are using, possible values are 0 (CLKOUT1) and 1 (CLKOUT2).
+      The second cell indicates which clock we want to use as source, possible
+      values are 0 thru 7, see the defines for the different source clocks.
+      The third cell is a divider, legal values are 1 thru 63.
+    type: object
+
+    properties:
+      '#clock-cells':
+        const: 3
+
+    additionalProperties: false
+
 required:
   - compatible
   - reg
diff --git a/include/dt-bindings/clock/ste-db8500-clkout.h b/include/dt-bindings/clock/ste-db8500-clkout.h
new file mode 100644
index 000000000000..ca07cb2bd1bc
--- /dev/null
+++ b/include/dt-bindings/clock/ste-db8500-clkout.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __STE_CLK_DB8500_CLKOUT_H__
+#define __STE_CLK_DB8500_CLKOUT_H__
+
+#define DB8500_CLKOUT_1			0
+#define DB8500_CLKOUT_2			1
+
+#define DB8500_CLKOUT_SRC_CLK38M	0
+#define DB8500_CLKOUT_SRC_ACLK		1
+#define DB8500_CLKOUT_SRC_SYSCLK	2
+#define DB8500_CLKOUT_SRC_LCDCLK	3
+#define DB8500_CLKOUT_SRC_SDMMCCLK	4
+#define DB8500_CLKOUT_SRC_TVCLK		5
+#define DB8500_CLKOUT_SRC_TIMCLK	6
+#define DB8500_CLKOUT_SRC_CLK009	7
+
+#endif
-- 
2.35.1

[PATCH 2/2] clk: ux500: Implement the missing CLKOUT clocks

[Linus Walleij]

This implements the two missing CLKOUT clocks for the ux500
(well really U8500/DB8500) SoC.

The clocks are initialized using a specific parent and
divider and these are specified in the device tree, see
the separate binding patch.

The implementation is a bit different in that it will only
create the clock in the clock framework if a user appears
in the device tree, rather than it being registered upfront
like most of the other clocks. This is because the clock
needs parameters for source and divider from the consumer
phandle for the clock to be set up properly when the clock
is registered.

There could be more than one user of a CLKOUT clock, but
we have not seen this in practice. If this happens the
framework prints and info and returns the previously
registered clock.

Using the clocks requires also muxing the CLKOUT1 or
CLKOUT2 to the appropriate pad. In practice this is
achived in a pinctrl handle in the DTS node for the device
using the CLKOUT clock, so this muxing is done separately
from the clock itself. Example:

  haptic@49 {
    compatible = "immersion,isa1200";
    reg = <0x49>;
    (...)
    /* clkout1 from ACLK divided by 8 */
    clocks = <&clkout_clk DB8500_CLKOUT_1 DB8500_CLKOUT_SRC_ACLK 8>;
    pinctrl-names = "default";
    pinctrl-0 = <&isa1200_janice_default>;
  };

  isa1200_janice_default: isa1200_janice {
    /* Bring out clkout1 on pin GPIO227 pin AH7 */
    janice_mux {
      function = "clkout";
      groups = "clkout1_a_1";
    };
    janice_cfg1 {
      pins = "GPIO227_AH7";
      ste,config = <&out_lo>;
    };
  (...)

This was tested successfully with the Immersion ISA1200
haptic feedback unit on the Samsung Galaxy S Advance GT-I9070
(Janice) mobile phone.

As the CLKOUT clocks need some undefined fixed rate parent
clocks that are currently missing from the PRCMU clock
implementation, the three simplest are added in this patch:
clk38m_to_clkgen, aclk and sysclk. The only parent not yet
available in the implementation is clk009, which is a kind
of special muxed and divided clock which isn't even
implemented in the vendor clock driver.

Cc: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/clk/ux500/clk-prcmu.c    | 151 +++++++++++++++++++++++++++++++
 drivers/clk/ux500/clk.h          |   4 +
 drivers/clk/ux500/u8500_of_clk.c |  89 ++++++++++++++++--
 3 files changed, 237 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/ux500/clk-prcmu.c b/drivers/clk/ux500/clk-prcmu.c
index 937b6bb82b30..e6a27c917126 100644
--- a/drivers/clk/ux500/clk-prcmu.c
+++ b/drivers/clk/ux500/clk-prcmu.c
@@ -14,6 +14,7 @@
 #include "clk.h"
 
 #define to_clk_prcmu(_hw) container_of(_hw, struct clk_prcmu, hw)
+#define to_clk_prcmu_clkout(_hw) container_of(_hw, struct clk_prcmu_clkout, hw)
 
 struct clk_prcmu {
 	struct clk_hw hw;
@@ -23,6 +24,15 @@ struct clk_prcmu {
 	int opp_requested;
 };
 
+struct clk_prcmu_clkout {
+	struct clk_hw hw;
+	u8 clkout_id;
+	u8 source;
+	u8 divider;
+	int is_prepared;
+	int is_enabled;
+};
+
 /* PRCMU clock operations. */
 
 static int clk_prcmu_prepare(struct clk_hw *hw)
@@ -344,3 +354,144 @@ struct clk *clk_reg_prcmu_opp_volt_scalable(const char *name,
 	return clk_reg_prcmu(name, parent_name, cg_sel, rate, flags,
 			&clk_prcmu_opp_volt_scalable_ops);
 }
+
+/* The clkout (external) clock is special and need special ops */
+
+static int clk_prcmu_clkout_prepare(struct clk_hw *hw)
+{
+	struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
+	int ret;
+
+	ret = prcmu_config_clkout(clk->clkout_id, clk->source, clk->divider);
+	if (!ret)
+		clk->is_prepared = 1;
+
+	return ret;
+}
+
+static void clk_prcmu_clkout_unprepare(struct clk_hw *hw)
+{
+	struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
+	int ret;
+
+	/* The clkout clock is disabled by dividing by 0 */
+	ret = prcmu_config_clkout(clk->clkout_id, clk->source, 0);
+	if (!ret)
+		clk->is_prepared = 0;
+}
+
+static int clk_prcmu_clkout_is_prepared(struct clk_hw *hw)
+{
+	struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
+	return clk->is_prepared;
+}
+
+static int clk_prcmu_clkout_enable(struct clk_hw *hw)
+{
+	struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
+	clk->is_enabled = 1;
+	return 0;
+}
+
+static void clk_prcmu_clkout_disable(struct clk_hw *hw)
+{
+	struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
+	clk->is_enabled = 0;
+}
+
+static int clk_prcmu_clkout_is_enabled(struct clk_hw *hw)
+{
+	struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
+	return clk->is_enabled;
+}
+
+static unsigned long clk_prcmu_clkout_recalc_rate(struct clk_hw *hw,
+						  unsigned long parent_rate)
+{
+	struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
+
+	if (!clk->divider)
+		return 0;
+	return (parent_rate / clk->divider);
+}
+
+static u8 clk_prcmu_clkout_get_parent(struct clk_hw *hw)
+{
+	struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
+	return clk->source;
+}
+
+static int clk_prcmu_clkout_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
+
+	clk->source = index;
+	/* Make sure the change reaches the hardware immediately */
+	if (clk->is_prepared)
+		return clk_prcmu_clkout_prepare(hw);
+	return 0;
+}
+
+static const struct clk_ops clk_prcmu_clkout_ops = {
+	.prepare = clk_prcmu_clkout_prepare,
+	.unprepare = clk_prcmu_clkout_unprepare,
+	.is_prepared = clk_prcmu_clkout_is_prepared,
+	.enable = clk_prcmu_clkout_enable,
+	.disable = clk_prcmu_clkout_disable,
+	.is_enabled = clk_prcmu_clkout_is_enabled,
+	.recalc_rate = clk_prcmu_clkout_recalc_rate,
+	.get_parent = clk_prcmu_clkout_get_parent,
+	.set_parent = clk_prcmu_clkout_set_parent,
+};
+
+struct clk *clk_reg_prcmu_clkout(const char *name,
+				 const char **parent_names, int num_parents,
+				 u8 source, u8 divider)
+
+{
+	struct clk_prcmu_clkout *clk;
+	struct clk_init_data clk_prcmu_clkout_init;
+	struct clk *clk_reg;
+	u8 clkout_id;
+
+	if (!name) {
+		pr_err("clk_prcmu_clkout: %s invalid arguments passed\n", __func__);
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (!strcmp(name, "clkout1"))
+		clkout_id = 0;
+	else if (!strcmp(name, "clkout2"))
+		clkout_id = 1;
+	else {
+		pr_err("clk_prcmu_clkout: %s bad clock name\n", __func__);
+		return ERR_PTR(-EINVAL);
+	}
+
+	clk = kzalloc(sizeof(*clk), GFP_KERNEL);
+	if (!clk)
+		return ERR_PTR(-ENOMEM);
+
+	clk->clkout_id = clkout_id;
+	clk->is_prepared = 1;
+	clk->is_enabled = 1;
+	clk->source = source;
+	clk->divider = divider;
+
+	clk_prcmu_clkout_init.name = name;
+	clk_prcmu_clkout_init.ops = &clk_prcmu_clkout_ops;
+	clk_prcmu_clkout_init.flags = CLK_GET_RATE_NOCACHE;
+	clk_prcmu_clkout_init.parent_names = parent_names;
+	clk_prcmu_clkout_init.num_parents = num_parents;
+	clk->hw.init = &clk_prcmu_clkout_init;
+
+	clk_reg = clk_register(NULL, &clk->hw);
+	if (IS_ERR_OR_NULL(clk_reg))
+		goto free_clkout;
+
+	return clk_reg;
+free_clkout:
+	kfree(clk);
+	pr_err("clk_prcmu_clkout: %s failed to register clk\n", __func__);
+	return ERR_PTR(-ENOMEM);
+}
diff --git a/drivers/clk/ux500/clk.h b/drivers/clk/ux500/clk.h
index 40cd9fc95b8b..0a656a7212ae 100644
--- a/drivers/clk/ux500/clk.h
+++ b/drivers/clk/ux500/clk.h
@@ -59,6 +59,10 @@ struct clk *clk_reg_prcmu_opp_volt_scalable(const char *name,
 					    unsigned long rate,
 					    unsigned long flags);
 
+struct clk *clk_reg_prcmu_clkout(const char *name,
+				 const char **parent_names, int num_parents,
+				 u8 source, u8 divider);
+
 struct clk *clk_reg_sysctrl_gate(struct device *dev,
 				 const char *name,
 				 const char *parent_name,
diff --git a/drivers/clk/ux500/u8500_of_clk.c b/drivers/clk/ux500/u8500_of_clk.c
index e86ed2eec3fd..dfe8794b3e78 100644
--- a/drivers/clk/ux500/u8500_of_clk.c
+++ b/drivers/clk/ux500/u8500_of_clk.c
@@ -18,6 +18,7 @@
 static struct clk *prcmu_clk[PRCMU_NUM_CLKS];
 static struct clk *prcc_pclk[(PRCC_NUM_PERIPH_CLUSTERS + 1) * PRCC_PERIPHS_PER_CLUSTER];
 static struct clk *prcc_kclk[(PRCC_NUM_PERIPH_CLUSTERS + 1) * PRCC_PERIPHS_PER_CLUSTER];
+static struct clk *clkout_clk[2];
 
 #define PRCC_SHOW(clk, base, bit) \
 	clk[(base * PRCC_PERIPHS_PER_CLUSTER) + bit]
@@ -46,6 +47,70 @@ static struct clk *ux500_twocell_get(struct of_phandle_args *clkspec,
 	return PRCC_SHOW(clk_data, base, bit);
 }
 
+/* Essentially names for the first PRCMU_CLKSRC_* defines */
+static const char *u8500_clkout_parents[] = {
+	"clk38m_to_clkgen",
+	"aclk",
+	"sysclk",
+	"lcdclk",
+	"sdmmcclk",
+	"tvclk",
+	"timclk",
+	/* CLK009 is not implemented, add it if you need it */
+	"clk009",
+};
+
+static struct clk *ux500_clkout_get(struct of_phandle_args *clkspec,
+				    void *data)
+{
+	int id, source, divider;
+	struct clk *clkout;
+
+	if (clkspec->args_count != 3)
+		return  ERR_PTR(-EINVAL);
+
+	id = clkspec->args[0];
+	source = clkspec->args[1];
+	divider = clkspec->args[2];
+
+	if (id < 0 || id > 1) {
+		pr_err("%s: invalid clkout ID %d\n", __func__, id);
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (clkout_clk[id]) {
+		pr_info("%s: clkout%d already registered, not reconfiguring\n",
+			__func__, id + 1);
+		return clkout_clk[id];
+	}
+
+	if (source > 7) {
+		pr_err("%s: invalid source ID %d\n", __func__, source);
+		return ERR_PTR(-EINVAL);
+	}
+
+	if ((divider == 0) || (divider > 63)) {
+		pr_err("%s: invalid divider %d\n", __func__, divider);
+		return ERR_PTR(-EINVAL);
+	}
+
+	pr_debug("registering clkout%d with source %d and divider %d\n",
+		 id + 1, source, divider);
+
+	clkout = clk_reg_prcmu_clkout(id ? "clkout2" : "clkout1",
+				      u8500_clkout_parents,
+				      ARRAY_SIZE(u8500_clkout_parents),
+				      source, divider);
+	if (IS_ERR(clkout)) {
+		pr_err("failed to register clkout%d\n",  id + 1);
+		return ERR_CAST(clkout);
+	}
+
+	clkout_clk[id] = clkout;
+
+	return clkout;
+}
+
 static void u8500_clk_init(struct device_node *np)
 {
 	struct prcmu_fw_version *fw_version;
@@ -89,7 +154,19 @@ static void u8500_clk_init(struct device_node *np)
 				CLK_IGNORE_UNUSED);
 	prcmu_clk[PRCMU_PLLDDR] = clk;
 
-	/* FIXME: Add sys, ulp and int clocks here. */
+	/*
+	 * Read-only clocks that only return their current rate, only used
+	 * as parents to other clocks and not visible in the device tree.
+	 * clk38m_to_clkgen is the same as the SYSCLK, i.e. the root clock.
+	 */
+	clk = clk_reg_prcmu_rate("clk38m_to_clkgen", NULL, PRCMU_SYSCLK,
+				 CLK_IGNORE_UNUSED);
+	clk = clk_reg_prcmu_rate("aclk", NULL, PRCMU_ACLK,
+				 CLK_IGNORE_UNUSED);
+	clk = clk_reg_prcmu_rate("sysclk", NULL, PRCMU_SYSCLK,
+				 CLK_IGNORE_UNUSED);
+
+	/* TODO: add CLK009 if needed */
 
 	rtc_clk = clk_register_fixed_rate(NULL, "rtc32k", "NULL",
 				CLK_IGNORE_UNUSED,
@@ -247,12 +324,6 @@ static void u8500_clk_init(struct device_node *np)
 	twd_clk = clk_register_fixed_factor(NULL, "smp_twd", "armss",
 				CLK_IGNORE_UNUSED, 1, 2);
 
-	/*
-	 * FIXME: Add special handled PRCMU clocks here:
-	 * 1. clkout0yuv, use PRCMU as parent + need regulator + pinctrl.
-	 * 2. ab9540_clkout1yuv, see clkout0yuv
-	 */
-
 	/* PRCC P-clocks */
 	clk = clk_reg_prcc_pclk("p1_pclk0", "per1clk", bases[CLKRST1_INDEX],
 				BIT(0), 0);
@@ -553,6 +624,10 @@ static void u8500_clk_init(struct device_node *np)
 			clk_data.clk_num = ARRAY_SIZE(prcmu_clk);
 			of_clk_add_provider(child, of_clk_src_onecell_get, &clk_data);
 		}
+
+		if (of_node_name_eq(child, "clkout-clock"))
+			of_clk_add_provider(child, ux500_clkout_get, NULL);
+
 		if (of_node_name_eq(child, "prcc-periph-clock"))
 			of_clk_add_provider(child, ux500_twocell_get, prcc_pclk);
 
-- 
2.35.1

Re: [PATCH 2/2] clk: ux500: Implement the missing CLKOUT clocks

[Stephen Boyd]

Quoting Linus Walleij (2022-03-13 16:29:26)
> diff --git a/drivers/clk/ux500/clk-prcmu.c b/drivers/clk/ux500/clk-prcmu.c
> index 937b6bb82b30..e6a27c917126 100644
> --- a/drivers/clk/ux500/clk-prcmu.c
> +++ b/drivers/clk/ux500/clk-prcmu.c
> @@ -14,6 +14,7 @@
>  #include "clk.h"
>  
>  #define to_clk_prcmu(_hw) container_of(_hw, struct clk_prcmu, hw)
> +#define to_clk_prcmu_clkout(_hw) container_of(_hw, struct clk_prcmu_clkout, hw)
>  
>  struct clk_prcmu {
>         struct clk_hw hw;
> @@ -23,6 +24,15 @@ struct clk_prcmu {
>         int opp_requested;
>  };
>  
> +struct clk_prcmu_clkout {
> +       struct clk_hw hw;
> +       u8 clkout_id;
> +       u8 source;
> +       u8 divider;
> +       int is_prepared;
> +       int is_enabled;
> +};
> +
>  /* PRCMU clock operations. */
>  
>  static int clk_prcmu_prepare(struct clk_hw *hw)
> @@ -344,3 +354,144 @@ struct clk *clk_reg_prcmu_opp_volt_scalable(const char *name,
>         return clk_reg_prcmu(name, parent_name, cg_sel, rate, flags,
>                         &clk_prcmu_opp_volt_scalable_ops);
>  }
> +
> +/* The clkout (external) clock is special and need special ops */
> +
> +static int clk_prcmu_clkout_prepare(struct clk_hw *hw)
> +{
> +       struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
> +       int ret;
> +
> +       ret = prcmu_config_clkout(clk->clkout_id, clk->source, clk->divider);
> +       if (!ret)
> +               clk->is_prepared = 1;
> +
> +       return ret;
> +}
> +
> +static void clk_prcmu_clkout_unprepare(struct clk_hw *hw)
> +{
> +       struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
> +       int ret;
> +
> +       /* The clkout clock is disabled by dividing by 0 */
> +       ret = prcmu_config_clkout(clk->clkout_id, clk->source, 0);
> +       if (!ret)
> +               clk->is_prepared = 0;
> +}
> +
> +static int clk_prcmu_clkout_is_prepared(struct clk_hw *hw)
> +{
> +       struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
> +       return clk->is_prepared;
> +}
> +
> +static int clk_prcmu_clkout_enable(struct clk_hw *hw)
> +{
> +       struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
> +       clk->is_enabled = 1;

If this isn't reading the hardware then we can just let the core figure
it out and remove these functions and 'is_enabled'/'is_prepared'
variables.

> +       return 0;
> +}
> +
> +static void clk_prcmu_clkout_disable(struct clk_hw *hw)
> +{
> +       struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
> +       clk->is_enabled = 0;
> +}
> +
> +static int clk_prcmu_clkout_is_enabled(struct clk_hw *hw)
> +{
> +       struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
> +       return clk->is_enabled;
> +}
> +
> +static unsigned long clk_prcmu_clkout_recalc_rate(struct clk_hw *hw,
> +                                                 unsigned long parent_rate)
> +{
> +       struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
> +
> +       if (!clk->divider)
> +               return 0;
> +       return (parent_rate / clk->divider);
> +}
> +
> +static u8 clk_prcmu_clkout_get_parent(struct clk_hw *hw)
> +{
> +       struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
> +       return clk->source;
> +}
> +
> +static int clk_prcmu_clkout_set_parent(struct clk_hw *hw, u8 index)
> +{
> +       struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
> +
> +       clk->source = index;
> +       /* Make sure the change reaches the hardware immediately */
> +       if (clk->is_prepared)
> +               return clk_prcmu_clkout_prepare(hw);
> +       return 0;
> +}
> +
> +static const struct clk_ops clk_prcmu_clkout_ops = {
> +       .prepare = clk_prcmu_clkout_prepare,
> +       .unprepare = clk_prcmu_clkout_unprepare,
> +       .is_prepared = clk_prcmu_clkout_is_prepared,
> +       .enable = clk_prcmu_clkout_enable,
> +       .disable = clk_prcmu_clkout_disable,
> +       .is_enabled = clk_prcmu_clkout_is_enabled,
> +       .recalc_rate = clk_prcmu_clkout_recalc_rate,
> +       .get_parent = clk_prcmu_clkout_get_parent,
> +       .set_parent = clk_prcmu_clkout_set_parent,
> +};
> +
> +struct clk *clk_reg_prcmu_clkout(const char *name,
> +                                const char **parent_names, int num_parents,
> +                                u8 source, u8 divider)
> +
> +{
> +       struct clk_prcmu_clkout *clk;
> +       struct clk_init_data clk_prcmu_clkout_init;
> +       struct clk *clk_reg;
> +       u8 clkout_id;
> +
> +       if (!name) {
> +               pr_err("clk_prcmu_clkout: %s invalid arguments passed\n", __func__);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       if (!strcmp(name, "clkout1"))
> +               clkout_id = 0;
> +       else if (!strcmp(name, "clkout2"))
> +               clkout_id = 1;
> +       else {
> +               pr_err("clk_prcmu_clkout: %s bad clock name\n", __func__);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> +       if (!clk)
> +               return ERR_PTR(-ENOMEM);
> +
> +       clk->clkout_id = clkout_id;
> +       clk->is_prepared = 1;
> +       clk->is_enabled = 1;
> +       clk->source = source;
> +       clk->divider = divider;
> +
> +       clk_prcmu_clkout_init.name = name;
> +       clk_prcmu_clkout_init.ops = &clk_prcmu_clkout_ops;
> +       clk_prcmu_clkout_init.flags = CLK_GET_RATE_NOCACHE;
> +       clk_prcmu_clkout_init.parent_names = parent_names;
> +       clk_prcmu_clkout_init.num_parents = num_parents;
> +       clk->hw.init = &clk_prcmu_clkout_init;
> +
> +       clk_reg = clk_register(NULL, &clk->hw);

Please use clk_hw_register()

> +       if (IS_ERR_OR_NULL(clk_reg))
> +               goto free_clkout;
> +
> +       return clk_reg;
> +free_clkout:
> +       kfree(clk);
> +       pr_err("clk_prcmu_clkout: %s failed to register clk\n", __func__);
> +       return ERR_PTR(-ENOMEM);
> +}
> diff --git a/drivers/clk/ux500/clk.h b/drivers/clk/ux500/clk.h
> index 40cd9fc95b8b..0a656a7212ae 100644
> --- a/drivers/clk/ux500/clk.h
> +++ b/drivers/clk/ux500/clk.h
> @@ -59,6 +59,10 @@ struct clk *clk_reg_prcmu_opp_volt_scalable(const char *name,
>                                             unsigned long rate,
>                                             unsigned long flags);
>  
> +struct clk *clk_reg_prcmu_clkout(const char *name,
> +                                const char **parent_names, int num_parents,
> +                                u8 source, u8 divider);
> +
>  struct clk *clk_reg_sysctrl_gate(struct device *dev,
>                                  const char *name,
>                                  const char *parent_name,
> diff --git a/drivers/clk/ux500/u8500_of_clk.c b/drivers/clk/ux500/u8500_of_clk.c
> index e86ed2eec3fd..dfe8794b3e78 100644
> --- a/drivers/clk/ux500/u8500_of_clk.c
> +++ b/drivers/clk/ux500/u8500_of_clk.c
> @@ -18,6 +18,7 @@
>  static struct clk *prcmu_clk[PRCMU_NUM_CLKS];
>  static struct clk *prcc_pclk[(PRCC_NUM_PERIPH_CLUSTERS + 1) * PRCC_PERIPHS_PER_CLUSTER];
>  static struct clk *prcc_kclk[(PRCC_NUM_PERIPH_CLUSTERS + 1) * PRCC_PERIPHS_PER_CLUSTER];
> +static struct clk *clkout_clk[2];
>  
>  #define PRCC_SHOW(clk, base, bit) \
>         clk[(base * PRCC_PERIPHS_PER_CLUSTER) + bit]
> @@ -46,6 +47,70 @@ static struct clk *ux500_twocell_get(struct of_phandle_args *clkspec,
>         return PRCC_SHOW(clk_data, base, bit);
>  }
>  
> +/* Essentially names for the first PRCMU_CLKSRC_* defines */
> +static const char *u8500_clkout_parents[] = {

const char * const ?

> +       "clk38m_to_clkgen",
> +       "aclk",
> +       "sysclk",
> +       "lcdclk",
> +       "sdmmcclk",
> +       "tvclk",
> +       "timclk",
> +       /* CLK009 is not implemented, add it if you need it */
> +       "clk009",
> +};
> +
> +static struct clk *ux500_clkout_get(struct of_phandle_args *clkspec,
> +                                   void *data)
> +{
> +       int id, source, divider;

unsigned int id?

> +       struct clk *clkout;
> +
> +       if (clkspec->args_count != 3)
> +               return  ERR_PTR(-EINVAL);
> +
> +       id = clkspec->args[0];
> +       source = clkspec->args[1];
> +       divider = clkspec->args[2];
> +
> +       if (id < 0 || id > 1) {

Because clkspec->args is uint32_t and so unlikely to be negative.

> +               pr_err("%s: invalid clkout ID %d\n", __func__, id);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       if (clkout_clk[id]) {
> +               pr_info("%s: clkout%d already registered, not reconfiguring\n",
> +                       __func__, id + 1);
> +               return clkout_clk[id];
> +       }
> +
> +       if (source > 7) {
> +               pr_err("%s: invalid source ID %d\n", __func__, source);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       if ((divider == 0) || (divider > 63)) {

Remove extra parenthesis please.

> +               pr_err("%s: invalid divider %d\n", __func__, divider);
> +               return ERR_PTR(-EINVAL);
> +       }
> +
> +       pr_debug("registering clkout%d with source %d and divider %d\n",
> +                id + 1, source, divider);
> +
> +       clkout = clk_reg_prcmu_clkout(id ? "clkout2" : "clkout1",
> +                                     u8500_clkout_parents,
> +                                     ARRAY_SIZE(u8500_clkout_parents),
> +                                     source, divider);
> +       if (IS_ERR(clkout)) {
> +               pr_err("failed to register clkout%d\n",  id + 1);
> +               return ERR_CAST(clkout);
> +       }
> +
> +       clkout_clk[id] = clkout;
> +
> +       return clkout;
> +}
> +
>  static void u8500_clk_init(struct device_node *np)
>  {
>         struct prcmu_fw_version *fw_version;

Re: [PATCH 1/2] dt-bindings: clock: u8500: Add clkout clock bindings

[Stephen Boyd]

Quoting Linus Walleij (2022-03-13 16:29:25)
> This adds device tree bindings for the externally routed clocks
> CLKOUT1 and CLKOUT2 clocks found in the DB8500.
> 
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  .../bindings/clock/stericsson,u8500-clks.yaml   | 16 ++++++++++++++++
>  include/dt-bindings/clock/ste-db8500-clkout.h   | 17 +++++++++++++++++
>  2 files changed, 33 insertions(+)
>  create mode 100644 include/dt-bindings/clock/ste-db8500-clkout.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/stericsson,u8500-clks.yaml b/Documentation/devicetree/bindings/clock/stericsson,u8500-clks.yaml
> index 9bc95a308477..afd049be948a 100644
> --- a/Documentation/devicetree/bindings/clock/stericsson,u8500-clks.yaml
> +++ b/Documentation/devicetree/bindings/clock/stericsson,u8500-clks.yaml
> @@ -109,6 +109,22 @@ properties:
>  
>      additionalProperties: false
>  
> +  clkout-clock:
> +    description: A subnode with three clock cells for externally routed clocks,
> +      output clocks. These are two PRCMU-internal clocks that can be divided and
> +      muxed out on the pads of the DB8500 SoC. The first cell indicates which
> +      output clock we are using, possible values are 0 (CLKOUT1) and 1 (CLKOUT2).
> +      The second cell indicates which clock we want to use as source, possible
> +      values are 0 thru 7, see the defines for the different source clocks.
> +      The third cell is a divider, legal values are 1 thru 63.

I suspect the description would be shorter if the properties of this
node were described in the binding.

> +    type: object
> +
> +    properties:
> +      '#clock-cells':
> +        const: 3
> +
> +    additionalProperties: false
> +

Can you update the example?

>  required:
>    - compatible
>    - reg

Re: [PATCH 2/2] clk: ux500: Implement the missing CLKOUT clocks

[Ulf Hansson]

On Tue, 15 Mar 2022 at 23:20, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Linus Walleij (2022-03-13 16:29:26)
> > diff --git a/drivers/clk/ux500/clk-prcmu.c b/drivers/clk/ux500/clk-prcmu.c
> > index 937b6bb82b30..e6a27c917126 100644
> > --- a/drivers/clk/ux500/clk-prcmu.c
> > +++ b/drivers/clk/ux500/clk-prcmu.c
> > @@ -14,6 +14,7 @@
> >  #include "clk.h"
> >
> >  #define to_clk_prcmu(_hw) container_of(_hw, struct clk_prcmu, hw)
> > +#define to_clk_prcmu_clkout(_hw) container_of(_hw, struct clk_prcmu_clkout, hw)
> >
> >  struct clk_prcmu {
> >         struct clk_hw hw;
> > @@ -23,6 +24,15 @@ struct clk_prcmu {
> >         int opp_requested;
> >  };
> >
> > +struct clk_prcmu_clkout {
> > +       struct clk_hw hw;
> > +       u8 clkout_id;
> > +       u8 source;
> > +       u8 divider;
> > +       int is_prepared;
> > +       int is_enabled;
> > +};
> > +
> >  /* PRCMU clock operations. */
> >
> >  static int clk_prcmu_prepare(struct clk_hw *hw)
> > @@ -344,3 +354,144 @@ struct clk *clk_reg_prcmu_opp_volt_scalable(const char *name,
> >         return clk_reg_prcmu(name, parent_name, cg_sel, rate, flags,
> >                         &clk_prcmu_opp_volt_scalable_ops);
> >  }
> > +
> > +/* The clkout (external) clock is special and need special ops */
> > +
> > +static int clk_prcmu_clkout_prepare(struct clk_hw *hw)
> > +{
> > +       struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
> > +       int ret;
> > +
> > +       ret = prcmu_config_clkout(clk->clkout_id, clk->source, clk->divider);
> > +       if (!ret)
> > +               clk->is_prepared = 1;
> > +
> > +       return ret;
> > +}
> > +
> > +static void clk_prcmu_clkout_unprepare(struct clk_hw *hw)
> > +{
> > +       struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
> > +       int ret;
> > +
> > +       /* The clkout clock is disabled by dividing by 0 */
> > +       ret = prcmu_config_clkout(clk->clkout_id, clk->source, 0);
> > +       if (!ret)
> > +               clk->is_prepared = 0;
> > +}
> > +
> > +static int clk_prcmu_clkout_is_prepared(struct clk_hw *hw)
> > +{
> > +       struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
> > +       return clk->is_prepared;
> > +}
> > +
> > +static int clk_prcmu_clkout_enable(struct clk_hw *hw)
> > +{
> > +       struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
> > +       clk->is_enabled = 1;
>
> If this isn't reading the hardware then we can just let the core figure
> it out and remove these functions and 'is_enabled'/'is_prepared'
> variables.
>
> > +       return 0;
> > +}
> > +
> > +static void clk_prcmu_clkout_disable(struct clk_hw *hw)
> > +{
> > +       struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
> > +       clk->is_enabled = 0;
> > +}
> > +
> > +static int clk_prcmu_clkout_is_enabled(struct clk_hw *hw)
> > +{
> > +       struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
> > +       return clk->is_enabled;
> > +}
> > +
> > +static unsigned long clk_prcmu_clkout_recalc_rate(struct clk_hw *hw,
> > +                                                 unsigned long parent_rate)
> > +{
> > +       struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
> > +
> > +       if (!clk->divider)
> > +               return 0;
> > +       return (parent_rate / clk->divider);
> > +}
> > +
> > +static u8 clk_prcmu_clkout_get_parent(struct clk_hw *hw)
> > +{
> > +       struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
> > +       return clk->source;
> > +}
> > +
> > +static int clk_prcmu_clkout_set_parent(struct clk_hw *hw, u8 index)
> > +{
> > +       struct clk_prcmu_clkout *clk = to_clk_prcmu_clkout(hw);
> > +
> > +       clk->source = index;
> > +       /* Make sure the change reaches the hardware immediately */
> > +       if (clk->is_prepared)
> > +               return clk_prcmu_clkout_prepare(hw);
> > +       return 0;
> > +}
> > +
> > +static const struct clk_ops clk_prcmu_clkout_ops = {
> > +       .prepare = clk_prcmu_clkout_prepare,
> > +       .unprepare = clk_prcmu_clkout_unprepare,
> > +       .is_prepared = clk_prcmu_clkout_is_prepared,
> > +       .enable = clk_prcmu_clkout_enable,
> > +       .disable = clk_prcmu_clkout_disable,
> > +       .is_enabled = clk_prcmu_clkout_is_enabled,
> > +       .recalc_rate = clk_prcmu_clkout_recalc_rate,
> > +       .get_parent = clk_prcmu_clkout_get_parent,
> > +       .set_parent = clk_prcmu_clkout_set_parent,
> > +};
> > +
> > +struct clk *clk_reg_prcmu_clkout(const char *name,
> > +                                const char **parent_names, int num_parents,
> > +                                u8 source, u8 divider)
> > +
> > +{
> > +       struct clk_prcmu_clkout *clk;
> > +       struct clk_init_data clk_prcmu_clkout_init;
> > +       struct clk *clk_reg;
> > +       u8 clkout_id;
> > +
> > +       if (!name) {
> > +               pr_err("clk_prcmu_clkout: %s invalid arguments passed\n", __func__);
> > +               return ERR_PTR(-EINVAL);
> > +       }
> > +
> > +       if (!strcmp(name, "clkout1"))
> > +               clkout_id = 0;
> > +       else if (!strcmp(name, "clkout2"))
> > +               clkout_id = 1;
> > +       else {
> > +               pr_err("clk_prcmu_clkout: %s bad clock name\n", __func__);
> > +               return ERR_PTR(-EINVAL);
> > +       }
> > +
> > +       clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> > +       if (!clk)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       clk->clkout_id = clkout_id;
> > +       clk->is_prepared = 1;
> > +       clk->is_enabled = 1;
> > +       clk->source = source;
> > +       clk->divider = divider;
> > +
> > +       clk_prcmu_clkout_init.name = name;
> > +       clk_prcmu_clkout_init.ops = &clk_prcmu_clkout_ops;
> > +       clk_prcmu_clkout_init.flags = CLK_GET_RATE_NOCACHE;
> > +       clk_prcmu_clkout_init.parent_names = parent_names;
> > +       clk_prcmu_clkout_init.num_parents = num_parents;
> > +       clk->hw.init = &clk_prcmu_clkout_init;
> > +
> > +       clk_reg = clk_register(NULL, &clk->hw);
>
> Please use clk_hw_register()

I was just about to send a comment like this. I assume Linus just
tried to be consistent with the existing code in this file (which has
turned into being quite old nowadays).

So, I think, either we should start by converting the existing code to
use clk_hw_register() and then make these changes on top - or we allow
Linus $subject patch to use clk_register() and on top we convert over
to use clk_hw_register(), for the entire file/driver. I am fine either
way.

Linus, I am happy to help with this, just tell me.

[...]

Kind regards
Uffe

Re: [PATCH 2/2] clk: ux500: Implement the missing CLKOUT clocks

[Stephen Boyd]

Quoting Ulf Hansson (2022-03-16 03:45:29)
> On Tue, 15 Mar 2022 at 23:20, Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Please use clk_hw_register()
> 
> I was just about to send a comment like this. I assume Linus just
> tried to be consistent with the existing code in this file (which has
> turned into being quite old nowadays).
> 
> So, I think, either we should start by converting the existing code to
> use clk_hw_register() and then make these changes on top - or we allow
> Linus $subject patch to use clk_register() and on top we convert over
> to use clk_hw_register(), for the entire file/driver. I am fine either
> way.
> 
> Linus, I am happy to help with this, just tell me.
> 

Sounds fine to me.