[PATCH 0/3] clk: keystone: syscon-clk: fixes for audio refclk

[PATCH 0/3] clk: keystone: syscon-clk: fixes for audio refclk

[Matthias Schiffer]

Currently the driver uses of_clk_hw_onecell_get() for all clocks it
manages, but this is incorrect for the audio refclk, which has 0 rather
than 1 clock cell according to its binding documentation [1]; attempting
to look up the clock when referenced like this in the Device Tree leads
to errors, as uninitialized memory is passed to of_clk_hw_onecell_get()
as the index.

The actual fix is in patch 3; patches 1 and 2 are preparation and
related cleanup. I've added a Fixes: tag to all 3 patches, as they
need to be backported together.

Matthias Schiffer (3):
  clk: keystone: syscon-clk: use struct instead of array for match data
  clk: keystone: syscon-clk: specify whether a parent is required in
    match data
  clk: keystone: syscon-clk: use of_clk_hw_simple_get() for audio refclk

[1] https://www.kernel.org/doc/Documentation/devicetree/bindings/clock/ti,am62-audio-refclk.yaml


 drivers/clk/keystone/syscon-clk.c | 73 ++++++++++++++++++++-----------
 1 file changed, 48 insertions(+), 25 deletions(-)

-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

[PATCH 1/3] clk: keystone: syscon-clk: use struct instead of array for match data

[Matthias Schiffer]

Allow to store information with the match data that is not specific to
the individual clock entries.

Counting the number of elements to a sentinel at runtime is replaced with
the usual ARRAY_SIZE() approach.

No functional change intended.

Fixes: 6acab96ee337 ("clk: keystone: syscon-clk: Add support for audio refclk")
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/clk/keystone/syscon-clk.c | 55 +++++++++++++++++++------------
 1 file changed, 34 insertions(+), 21 deletions(-)

diff --git a/drivers/clk/keystone/syscon-clk.c b/drivers/clk/keystone/syscon-clk.c
index d33f74119488..68ac536f564e 100644
--- a/drivers/clk/keystone/syscon-clk.c
+++ b/drivers/clk/keystone/syscon-clk.c
@@ -18,12 +18,17 @@ struct ti_syscon_gate_clk_priv {
 	u32 idx;
 };
 
-struct ti_syscon_gate_clk_data {
+struct ti_syscon_gate_clk_entry {
 	char *name;
 	u32 offset;
 	u32 bit_idx;
 };
 
+struct ti_syscon_gate_clk_data {
+	const struct ti_syscon_gate_clk_entry *clks;
+	size_t num_clks;
+};
+
 static struct
 ti_syscon_gate_clk_priv *to_ti_syscon_gate_clk_priv(struct clk_hw *hw)
 {
@@ -64,7 +69,7 @@ static const struct clk_ops ti_syscon_gate_clk_ops = {
 static struct clk_hw
 *ti_syscon_gate_clk_register(struct device *dev, struct regmap *regmap,
 			     const char *parent_name,
-			     const struct ti_syscon_gate_clk_data *data)
+			     const struct ti_syscon_gate_clk_entry *data)
 {
 	struct ti_syscon_gate_clk_priv *priv;
 	struct clk_init_data init;
@@ -107,10 +112,10 @@ static struct clk_hw
 
 static int ti_syscon_gate_clk_probe(struct platform_device *pdev)
 {
-	const struct ti_syscon_gate_clk_data *data, *p;
+	const struct ti_syscon_gate_clk_data *data;
 	struct clk_hw_onecell_data *hw_data;
 	struct device *dev = &pdev->dev;
-	int num_clks, num_parents, i;
+	int num_parents, i;
 	const char *parent_name;
 	struct regmap *regmap;
 
@@ -123,10 +128,6 @@ static int ti_syscon_gate_clk_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(regmap),
 				     "failed to get regmap\n");
 
-	num_clks = 0;
-	for (p = data; p->name; p++)
-		num_clks++;
-
 	num_parents = of_clk_get_parent_count(dev->of_node);
 	if (of_device_is_compatible(dev->of_node, "ti,am62-audio-refclk") &&
 	    num_parents == 0) {
@@ -134,21 +135,21 @@ static int ti_syscon_gate_clk_probe(struct platform_device *pdev)
 				     "must specify a parent clock\n");
 	}
 
-	hw_data = devm_kzalloc(dev, struct_size(hw_data, hws, num_clks),
+	hw_data = devm_kzalloc(dev, struct_size(hw_data, hws, data->num_clks),
 			       GFP_KERNEL);
 	if (!hw_data)
 		return -ENOMEM;
 
-	hw_data->num = num_clks;
+	hw_data->num = data->num_clks;
 
 	parent_name = of_clk_get_parent_name(dev->of_node, 0);
-	for (i = 0; i < num_clks; i++) {
+	for (i = 0; i < data->num_clks; i++) {
 		hw_data->hws[i] = ti_syscon_gate_clk_register(dev, regmap,
 							      parent_name,
-							      &data[i]);
+							      &data->clks[i]);
 		if (IS_ERR(hw_data->hws[i]))
 			dev_warn(dev, "failed to register %s\n",
-				 data[i].name);
+				 data->clks[i].name);
 	}
 
 	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
@@ -162,17 +163,20 @@ static int ti_syscon_gate_clk_probe(struct platform_device *pdev)
 		.bit_idx = (_bit_idx),			\
 	}
 
-static const struct ti_syscon_gate_clk_data am654_clk_data[] = {
+static const struct ti_syscon_gate_clk_entry am654_clks[] = {
 	TI_SYSCON_CLK_GATE("ehrpwm_tbclk0", 0x0, 0),
 	TI_SYSCON_CLK_GATE("ehrpwm_tbclk1", 0x4, 0),
 	TI_SYSCON_CLK_GATE("ehrpwm_tbclk2", 0x8, 0),
 	TI_SYSCON_CLK_GATE("ehrpwm_tbclk3", 0xc, 0),
 	TI_SYSCON_CLK_GATE("ehrpwm_tbclk4", 0x10, 0),
 	TI_SYSCON_CLK_GATE("ehrpwm_tbclk5", 0x14, 0),
-	{ /* Sentinel */ },
+};
+static const struct ti_syscon_gate_clk_data am654_clk_data = {
+	.clks = am654_clks,
+	.num_clks = ARRAY_SIZE(am654_clks),
 };
 
-static const struct ti_syscon_gate_clk_data am64_clk_data[] = {
+static const struct ti_syscon_gate_clk_entry am64_clks[] = {
 	TI_SYSCON_CLK_GATE("epwm_tbclk0", 0x0, 0),
 	TI_SYSCON_CLK_GATE("epwm_tbclk1", 0x0, 1),
 	TI_SYSCON_CLK_GATE("epwm_tbclk2", 0x0, 2),
@@ -182,19 +186,28 @@ static const struct ti_syscon_gate_clk_data am64_clk_data[] = {
 	TI_SYSCON_CLK_GATE("epwm_tbclk6", 0x0, 6),
 	TI_SYSCON_CLK_GATE("epwm_tbclk7", 0x0, 7),
 	TI_SYSCON_CLK_GATE("epwm_tbclk8", 0x0, 8),
-	{ /* Sentinel */ },
+};
+static const struct ti_syscon_gate_clk_data am64_clk_data = {
+	.clks = am64_clks,
+	.num_clks = ARRAY_SIZE(am64_clks),
 };
 
-static const struct ti_syscon_gate_clk_data am62_clk_data[] = {
+static const struct ti_syscon_gate_clk_entry am62_clks[] = {
 	TI_SYSCON_CLK_GATE("epwm_tbclk0", 0x0, 0),
 	TI_SYSCON_CLK_GATE("epwm_tbclk1", 0x0, 1),
 	TI_SYSCON_CLK_GATE("epwm_tbclk2", 0x0, 2),
-	{ /* Sentinel */ },
+};
+static const struct ti_syscon_gate_clk_data am62_clk_data = {
+	.clks = am62_clks,
+	.num_clks = ARRAY_SIZE(am62_clks),
 };
 
-static const struct ti_syscon_gate_clk_data am62_audio_clk_data[] = {
+static const struct ti_syscon_gate_clk_entry am62_audio_clks[] = {
 	TI_SYSCON_CLK_GATE("audio_refclk", 0x0, 15),
-	{ /* Sentinel */ },
+};
+static const struct ti_syscon_gate_clk_data am62_audio_clk_data = {
+	.clks = am62_audio_clks,
+	.num_clks = ARRAY_SIZE(am62_audio_clks),
 };
 
 static const struct of_device_id ti_syscon_gate_clk_ids[] = {
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

[PATCH 2/3] clk: keystone: syscon-clk: specify whether a parent is required in match data

[Matthias Schiffer]

Avoid checking the compatible string again in the probe function.

No functional change intended.

Fixes: 6acab96ee337 ("clk: keystone: syscon-clk: Add support for audio refclk")
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/clk/keystone/syscon-clk.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/keystone/syscon-clk.c b/drivers/clk/keystone/syscon-clk.c
index 68ac536f564e..9626a877e072 100644
--- a/drivers/clk/keystone/syscon-clk.c
+++ b/drivers/clk/keystone/syscon-clk.c
@@ -27,6 +27,7 @@ struct ti_syscon_gate_clk_entry {
 struct ti_syscon_gate_clk_data {
 	const struct ti_syscon_gate_clk_entry *clks;
 	size_t num_clks;
+	bool needs_parent;
 };
 
 static struct
@@ -129,11 +130,9 @@ static int ti_syscon_gate_clk_probe(struct platform_device *pdev)
 				     "failed to get regmap\n");
 
 	num_parents = of_clk_get_parent_count(dev->of_node);
-	if (of_device_is_compatible(dev->of_node, "ti,am62-audio-refclk") &&
-	    num_parents == 0) {
+	if (data->needs_parent && num_parents == 0)
 		return dev_err_probe(dev, -EINVAL,
 				     "must specify a parent clock\n");
-	}
 
 	hw_data = devm_kzalloc(dev, struct_size(hw_data, hws, data->num_clks),
 			       GFP_KERNEL);
@@ -208,6 +207,7 @@ static const struct ti_syscon_gate_clk_entry am62_audio_clks[] = {
 static const struct ti_syscon_gate_clk_data am62_audio_clk_data = {
 	.clks = am62_audio_clks,
 	.num_clks = ARRAY_SIZE(am62_audio_clks),
+	.needs_parent = true,
 };
 
 static const struct of_device_id ti_syscon_gate_clk_ids[] = {
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

[PATCH 3/3] clk: keystone: syscon-clk: use of_clk_hw_simple_get() for audio refclk

[Matthias Schiffer]

The binding documentation for ti,am62-audio-refclk specifies that it has 0
clock cells (and Device Trees using the binding as documented have already
existed in vendor kernels for some time). Fix the driver to use
of_clk_hw_simple_get() instead of of_clk_hw_onecell_get(), as attempting
to reference the clock in the Device Tree will fail otherwise.

Fixes: 6acab96ee337 ("clk: keystone: syscon-clk: Add support for audio refclk")
Signed-off-by: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
---
 drivers/clk/keystone/syscon-clk.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/keystone/syscon-clk.c b/drivers/clk/keystone/syscon-clk.c
index 9626a877e072..a539dbf2f48e 100644
--- a/drivers/clk/keystone/syscon-clk.c
+++ b/drivers/clk/keystone/syscon-clk.c
@@ -28,6 +28,7 @@ struct ti_syscon_gate_clk_data {
 	const struct ti_syscon_gate_clk_entry *clks;
 	size_t num_clks;
 	bool needs_parent;
+	bool simple; /* Use of_clk_hw_simple_get() rather than onecell */
 };
 
 static struct
@@ -129,6 +130,10 @@ static int ti_syscon_gate_clk_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(regmap),
 				     "failed to get regmap\n");
 
+	if (data->simple && data->num_clks != 1)
+		return dev_err_probe(dev, -EINVAL,
+				     "simple clocks must have exactly 1 entry\n");
+
 	num_parents = of_clk_get_parent_count(dev->of_node);
 	if (data->needs_parent && num_parents == 0)
 		return dev_err_probe(dev, -EINVAL,
@@ -151,8 +156,12 @@ static int ti_syscon_gate_clk_probe(struct platform_device *pdev)
 				 data->clks[i].name);
 	}
 
-	return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
-					   hw_data);
+	if (data->simple)
+		return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
+						   hw_data->hws[0]);
+	else
+		return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
+						   hw_data);
 }
 
 #define TI_SYSCON_CLK_GATE(_name, _offset, _bit_idx)	\
@@ -208,6 +217,7 @@ static const struct ti_syscon_gate_clk_data am62_audio_clk_data = {
 	.clks = am62_audio_clks,
 	.num_clks = ARRAY_SIZE(am62_audio_clks),
 	.needs_parent = true,
+	.simple = true,
 };
 
 static const struct of_device_id ti_syscon_gate_clk_ids[] = {
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

Re: [PATCH 0/3] clk: keystone: syscon-clk: fixes for audio refclk

[Francesco Dolcini]

On Tue, Aug 01, 2023 at 12:36:06PM +0200, Matthias Schiffer wrote:
> The actual fix is in patch 3; patches 1 and 2 are preparation and
> related cleanup. I've added a Fixes: tag to all 3 patches, as they
> need to be backported together.

Please see this https://lore.kernel.org/all/20230728222639.110409-1-francesco@dolcini.it/
that was sent a few days ago. It fixes the same issue and it's already
reviewed.

Francesco