[PATCH v2 0/4] Add support for stm32mp25x RNG

[PATCH v2 0/4] Add support for stm32mp25x RNG

[Gatien Chevallier]

This patchset adds support for the Random Number
Generator(RNG) present on the stm32mp25x platforms.
On these platforms, the clock management and the RNG
parameters are different.

While there, update the RNG max clock frequency on
stm32mp15 platforms according to the latest specs.

Tested on the stm32mp257f-ev1 platform with a deep
power sequence with rngtest before/after the sequence with
satisfying results.

Same was done on stm32mp135f-dk to make sure no regression was added.

On stm32mp157c-dk2, I didn't perform a power sequence but the rngtest
results were satisfying.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
---
Changes in V2:
	-Fixes in bindings
	-Removed MP25 RNG example
	-Renamed RNG clocks for mp25 to "core" and "bus"

---
Gatien Chevallier (4):
      dt-bindings: rng: add st,stm32mp25-rng support
      hwrng: stm32 - implement support for STM32MP25x platforms
      hwrng: stm32 - update STM32MP15 RNG max clock frequency
      arm64: dts: st: add RNG node on stm32mp251

 .../devicetree/bindings/rng/st,stm32-rng.yaml      | 30 +++++++-
 arch/arm64/boot/dts/st/stm32mp251.dtsi             | 10 +++
 drivers/char/hw_random/stm32-rng.c                 | 87 +++++++++++++++++-----
 3 files changed, 107 insertions(+), 20 deletions(-)
---
base-commit: 1d227fcc72223cbdd34d0ce13541cbaab5e0d72f
change-id: 20241011-rng-mp25-v2-b6460ef11e1f

Best regards,
-- 
Gatien Chevallier <gatien.chevallier@foss.st.com>

[PATCH v2 1/4] dt-bindings: rng: add st,stm32mp25-rng support

[Gatien Chevallier]

Add RNG STM32MP25x platforms compatible. Update the clock
properties management to support all versions.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
---
Changes in V2
	-Fix missing min/maxItems
	-Removed MP25 RNG example
	-Renamed RNG clocks for mp25 to "core" and "bus"
---
 .../devicetree/bindings/rng/st,stm32-rng.yaml      | 30 +++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/rng/st,stm32-rng.yaml b/Documentation/devicetree/bindings/rng/st,stm32-rng.yaml
index 340d01d481d12ce8664a60db42182ddaf0d1385b..5d553f7f706f8d7c17aea07e4130a34af764a635 100644
--- a/Documentation/devicetree/bindings/rng/st,stm32-rng.yaml
+++ b/Documentation/devicetree/bindings/rng/st,stm32-rng.yaml
@@ -18,12 +18,20 @@ properties:
     enum:
       - st,stm32-rng
       - st,stm32mp13-rng
+      - st,stm32mp25-rng
 
   reg:
     maxItems: 1
 
   clocks:
-    maxItems: 1
+    minItems: 1
+    maxItems: 2
+
+  clock-names:
+    minItems: 1
+    items:
+      - const: core
+      - const: bus
 
   resets:
     maxItems: 1
@@ -57,6 +65,26 @@ allOf:
       properties:
         st,rng-lock-conf: false
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - st,stm32-rng
+              - st,stm32mp13-rng
+    then:
+      properties:
+        clocks:
+          maxItems: 1
+        clock-names: false
+    else:
+      properties:
+        clocks:
+          minItems: 2
+          maxItems: 2
+      required:
+        - clock-names
+
 additionalProperties: false
 
 examples:

-- 
2.25.1

[PATCH v2 2/4] hwrng: stm32 - implement support for STM32MP25x platforms

[Gatien Chevallier]

Implement the support for STM32MP25x platforms. On this platform, a
security clock is shared between some hardware blocks. For the RNG,
it is the RNG kernel clock. Therefore, the gate is no more shared
between the RNG bus and kernel clocks as on STM32MP1x platforms and
the bus clock has to be managed on its own.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
---

Changes in V2
	-Renamed RNG clocks to "core" and "bus"
	-Use clk_bulk_* APIs instead of handling each clock. Just make
	 sure that the RNG core clock is first
---
 drivers/char/hw_random/stm32-rng.c | 85 ++++++++++++++++++++++++++++++--------
 1 file changed, 67 insertions(+), 18 deletions(-)

diff --git a/drivers/char/hw_random/stm32-rng.c b/drivers/char/hw_random/stm32-rng.c
index 9d041a67c295a54d283d235bbcf5a9ab7a8baa5c..62aa9f87415d2518b0c1cb5fb51b0b646422ed35 100644
--- a/drivers/char/hw_random/stm32-rng.c
+++ b/drivers/char/hw_random/stm32-rng.c
@@ -49,6 +49,7 @@
 
 struct stm32_rng_data {
 	uint	max_clock_rate;
+	uint	nb_clock;
 	u32	cr;
 	u32	nscr;
 	u32	htcr;
@@ -72,7 +73,7 @@ struct stm32_rng_private {
 	struct hwrng rng;
 	struct device *dev;
 	void __iomem *base;
-	struct clk *clk;
+	struct clk_bulk_data *clk_bulk;
 	struct reset_control *rst;
 	struct stm32_rng_config pm_conf;
 	const struct stm32_rng_data *data;
@@ -266,7 +267,7 @@ static uint stm32_rng_clock_freq_restrain(struct hwrng *rng)
 	unsigned long clock_rate = 0;
 	uint clock_div = 0;
 
-	clock_rate = clk_get_rate(priv->clk);
+	clock_rate = clk_get_rate(priv->clk_bulk[0].clk);
 
 	/*
 	 * Get the exponent to apply on the CLKDIV field in RNG_CR register
@@ -276,7 +277,7 @@ static uint stm32_rng_clock_freq_restrain(struct hwrng *rng)
 	while ((clock_rate >> clock_div) > priv->data->max_clock_rate)
 		clock_div++;
 
-	pr_debug("RNG clk rate : %lu\n", clk_get_rate(priv->clk) >> clock_div);
+	pr_debug("RNG clk rate : %lu\n", clk_get_rate(priv->clk_bulk[0].clk) >> clock_div);
 
 	return clock_div;
 }
@@ -288,7 +289,7 @@ static int stm32_rng_init(struct hwrng *rng)
 	int err;
 	u32 reg;
 
-	err = clk_prepare_enable(priv->clk);
+	err = clk_bulk_prepare_enable(priv->data->nb_clock, priv->clk_bulk);
 	if (err)
 		return err;
 
@@ -328,7 +329,7 @@ static int stm32_rng_init(struct hwrng *rng)
 							(!(reg & RNG_CR_CONDRST)),
 							10, 50000);
 		if (err) {
-			clk_disable_unprepare(priv->clk);
+			clk_bulk_disable_unprepare(priv->data->nb_clock, priv->clk_bulk);
 			dev_err(priv->dev, "%s: timeout %x!\n", __func__, reg);
 			return -EINVAL;
 		}
@@ -356,12 +357,13 @@ static int stm32_rng_init(struct hwrng *rng)
 						reg & RNG_SR_DRDY,
 						10, 100000);
 	if (err || (reg & ~RNG_SR_DRDY)) {
-		clk_disable_unprepare(priv->clk);
+		clk_bulk_disable_unprepare(priv->data->nb_clock, priv->clk_bulk);
 		dev_err(priv->dev, "%s: timeout:%x SR: %x!\n", __func__, err, reg);
+
 		return -EINVAL;
 	}
 
-	clk_disable_unprepare(priv->clk);
+	clk_bulk_disable_unprepare(priv->data->nb_clock, priv->clk_bulk);
 
 	return 0;
 }
@@ -379,7 +381,8 @@ static int __maybe_unused stm32_rng_runtime_suspend(struct device *dev)
 	reg = readl_relaxed(priv->base + RNG_CR);
 	reg &= ~RNG_CR_RNGEN;
 	writel_relaxed(reg, priv->base + RNG_CR);
-	clk_disable_unprepare(priv->clk);
+
+	clk_bulk_disable_unprepare(priv->data->nb_clock, priv->clk_bulk);
 
 	return 0;
 }
@@ -389,7 +392,7 @@ static int __maybe_unused stm32_rng_suspend(struct device *dev)
 	struct stm32_rng_private *priv = dev_get_drvdata(dev);
 	int err;
 
-	err = clk_prepare_enable(priv->clk);
+	err = clk_bulk_prepare_enable(priv->data->nb_clock, priv->clk_bulk);
 	if (err)
 		return err;
 
@@ -403,7 +406,7 @@ static int __maybe_unused stm32_rng_suspend(struct device *dev)
 
 	writel_relaxed(priv->pm_conf.cr, priv->base + RNG_CR);
 
-	clk_disable_unprepare(priv->clk);
+	clk_bulk_disable_unprepare(priv->data->nb_clock, priv->clk_bulk);
 
 	return 0;
 }
@@ -414,7 +417,7 @@ static int __maybe_unused stm32_rng_runtime_resume(struct device *dev)
 	int err;
 	u32 reg;
 
-	err = clk_prepare_enable(priv->clk);
+	err = clk_bulk_prepare_enable(priv->data->nb_clock, priv->clk_bulk);
 	if (err)
 		return err;
 
@@ -434,7 +437,7 @@ static int __maybe_unused stm32_rng_resume(struct device *dev)
 	int err;
 	u32 reg;
 
-	err = clk_prepare_enable(priv->clk);
+	err = clk_bulk_prepare_enable(priv->data->nb_clock, priv->clk_bulk);
 	if (err)
 		return err;
 
@@ -462,7 +465,7 @@ static int __maybe_unused stm32_rng_resume(struct device *dev)
 							reg & ~RNG_CR_CONDRST, 10, 100000);
 
 		if (err) {
-			clk_disable_unprepare(priv->clk);
+			clk_bulk_disable_unprepare(priv->data->nb_clock, priv->clk_bulk);
 			dev_err(priv->dev, "%s: timeout:%x CR: %x!\n", __func__, err, reg);
 			return -EINVAL;
 		}
@@ -472,7 +475,7 @@ static int __maybe_unused stm32_rng_resume(struct device *dev)
 		writel_relaxed(reg, priv->base + RNG_CR);
 	}
 
-	clk_disable_unprepare(priv->clk);
+	clk_bulk_disable_unprepare(priv->data->nb_clock, priv->clk_bulk);
 
 	return 0;
 }
@@ -484,9 +487,19 @@ static const struct dev_pm_ops __maybe_unused stm32_rng_pm_ops = {
 				stm32_rng_resume)
 };
 
+static const struct stm32_rng_data stm32mp25_rng_data = {
+	.has_cond_reset = true,
+	.max_clock_rate = 48000000,
+	.nb_clock = 2,
+	.cr = 0x00F00D00,
+	.nscr = 0x2B5BB,
+	.htcr = 0x969D,
+};
+
 static const struct stm32_rng_data stm32mp13_rng_data = {
 	.has_cond_reset = true,
 	.max_clock_rate = 48000000,
+	.nb_clock = 1,
 	.cr = 0x00F00D00,
 	.nscr = 0x2B5BB,
 	.htcr = 0x969D,
@@ -495,9 +508,14 @@ static const struct stm32_rng_data stm32mp13_rng_data = {
 static const struct stm32_rng_data stm32_rng_data = {
 	.has_cond_reset = false,
 	.max_clock_rate = 3000000,
+	.nb_clock = 1,
 };
 
 static const struct of_device_id stm32_rng_match[] = {
+	{
+		.compatible = "st,stm32mp25-rng",
+		.data = &stm32mp25_rng_data,
+	},
 	{
 		.compatible = "st,stm32mp13-rng",
 		.data = &stm32mp13_rng_data,
@@ -525,10 +543,6 @@ static int stm32_rng_probe(struct platform_device *ofdev)
 	if (IS_ERR(priv->base))
 		return PTR_ERR(priv->base);
 
-	priv->clk = devm_clk_get(&ofdev->dev, NULL);
-	if (IS_ERR(priv->clk))
-		return PTR_ERR(priv->clk);
-
 	priv->rst = devm_reset_control_get(&ofdev->dev, NULL);
 	if (!IS_ERR(priv->rst)) {
 		reset_control_assert(priv->rst);
@@ -551,6 +565,41 @@ static int stm32_rng_probe(struct platform_device *ofdev)
 	priv->rng.read = stm32_rng_read;
 	priv->rng.quality = 900;
 
+	if (!priv->data->nb_clock || priv->data->nb_clock > 2)
+		return -EINVAL;
+
+	priv->clk_bulk = devm_kzalloc(dev, priv->data->nb_clock * sizeof(*priv->clk_bulk),
+				      GFP_KERNEL);
+	if (!priv->clk_bulk)
+		return -ENOMEM;
+
+	if (priv->data->nb_clock == 2) {
+		struct clk *clk;
+		struct clk *bus_clk;
+
+		clk = devm_clk_get(&ofdev->dev, "core");
+		if (IS_ERR(clk))
+			return PTR_ERR(clk);
+
+		bus_clk = devm_clk_get(&ofdev->dev, "bus");
+		if (IS_ERR(clk))
+			return PTR_ERR(bus_clk);
+
+		priv->clk_bulk[0].clk = clk;
+		priv->clk_bulk[0].id = "core";
+		priv->clk_bulk[1].clk = bus_clk;
+		priv->clk_bulk[1].id = "bus";
+	} else {
+		struct clk *clk;
+
+		clk = devm_clk_get(&ofdev->dev, NULL);
+		if (IS_ERR(clk))
+			return PTR_ERR(clk);
+
+		priv->clk_bulk[0].clk = clk;
+		priv->clk_bulk[0].id = "core";
+	}
+
 	pm_runtime_set_autosuspend_delay(dev, 100);
 	pm_runtime_use_autosuspend(dev);
 	pm_runtime_enable(dev);

-- 
2.25.1

[PATCH v2 3/4] hwrng: stm32 - update STM32MP15 RNG max clock frequency

[Gatien Chevallier]

RNG max clock frequency can be updated to 48MHz for stm32mp1x
platforms according to the latest specifications.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
---
 drivers/char/hw_random/stm32-rng.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/hw_random/stm32-rng.c b/drivers/char/hw_random/stm32-rng.c
index 62aa9f87415d2518b0c1cb5fb51b0b646422ed35..ef8324b914fc2de2e7a6bd9eb69c081c26cd1ecd 100644
--- a/drivers/char/hw_random/stm32-rng.c
+++ b/drivers/char/hw_random/stm32-rng.c
@@ -507,7 +507,7 @@ static const struct stm32_rng_data stm32mp13_rng_data = {
 
 static const struct stm32_rng_data stm32_rng_data = {
 	.has_cond_reset = false,
-	.max_clock_rate = 3000000,
+	.max_clock_rate = 48000000,
 	.nb_clock = 1,
 };
 

-- 
2.25.1

[PATCH v2 4/4] arm64: dts: st: add RNG node on stm32mp251

[Gatien Chevallier]

Update the device-tree stm32mp251.dtsi by adding the Random Number
Generator(RNG) node.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
---

Changes in V2
	-Renamed RNG clocks to "core" and "bus"
---
 arch/arm64/boot/dts/st/stm32mp251.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/st/stm32mp251.dtsi b/arch/arm64/boot/dts/st/stm32mp251.dtsi
index 1167cf63d7e87aaa15c5c1ed70a9f6511fd818d4..273da5f62294422b587b13404b499b5ffe6c148e 100644
--- a/arch/arm64/boot/dts/st/stm32mp251.dtsi
+++ b/arch/arm64/boot/dts/st/stm32mp251.dtsi
@@ -493,6 +493,16 @@ uart8: serial@40380000 {
 				status = "disabled";
 			};
 
+			rng: rng@42020000 {
+				compatible = "st,stm32mp25-rng";
+				reg = <0x42020000 0x400>;
+				clocks = <&clk_rcbsec>, <&rcc CK_BUS_RNG>;
+				clock-names = "core", "bus";
+				resets = <&rcc RNG_R>;
+				access-controllers = <&rifsc 92>;
+				status = "disabled";
+			};
+
 			spi8: spi@46020000 {
 				#address-cells = <1>;
 				#size-cells = <0>;

-- 
2.25.1

Re: [PATCH v2 2/4] hwrng: stm32 - implement support for STM32MP25x platforms

[Marek Vasut]

On 10/11/24 5:41 PM, Gatien Chevallier wrote:

[...]

> @@ -551,6 +565,41 @@ static int stm32_rng_probe(struct platform_device *ofdev)
>   	priv->rng.read = stm32_rng_read;
>   	priv->rng.quality = 900;
>   
> +	if (!priv->data->nb_clock || priv->data->nb_clock > 2)
> +		return -EINVAL;
> +
> +	priv->clk_bulk = devm_kzalloc(dev, priv->data->nb_clock * sizeof(*priv->clk_bulk),
> +				      GFP_KERNEL);
> +	if (!priv->clk_bulk)
> +		return -ENOMEM;

Try this:

ret = devm_clk_bulk_get(dev, priv->data->nb_clock, priv->clk_bulk);
...
// Swap the clock if they are not in the right order:
if (priv->data->nb_clock == 2 &&
     strcmp(__clk_get_name(priv->clk_bulk[0].clk), "core"))
{
  const char *id = priv->clk_bulk[1].id;
  struct clk *clk = priv->clk_bulk[1].clk;
  priv->clk_bulk[1].id = priv->clk_bulk[0].id;
  priv->clk_bulk[1].clk = priv->clk_bulk[0].clk;
  priv->clk_bulk[0].id = id;
  priv->clk_bulk[0].clk = clk;
}

> +	if (priv->data->nb_clock == 2) {
> +		struct clk *clk;
> +		struct clk *bus_clk;
> +
> +		clk = devm_clk_get(&ofdev->dev, "core");
> +		if (IS_ERR(clk))
> +			return PTR_ERR(clk);
> +
> +		bus_clk = devm_clk_get(&ofdev->dev, "bus");
> +		if (IS_ERR(clk))
> +			return PTR_ERR(bus_clk);
> +
> +		priv->clk_bulk[0].clk = clk;
> +		priv->clk_bulk[0].id = "core";
> +		priv->clk_bulk[1].clk = bus_clk;
> +		priv->clk_bulk[1].id = "bus";
> +	} else {
> +		struct clk *clk;
> +
> +		clk = devm_clk_get(&ofdev->dev, NULL);
> +		if (IS_ERR(clk))
> +			return PTR_ERR(clk);
> +
> +		priv->clk_bulk[0].clk = clk;
> +		priv->clk_bulk[0].id = "core";
> +	}

Re: [PATCH v2 3/4] hwrng: stm32 - update STM32MP15 RNG max clock frequency

[Marek Vasut]

On 10/11/24 5:41 PM, Gatien Chevallier wrote:
> RNG max clock frequency can be updated to 48MHz for stm32mp1x
> platforms according to the latest specifications.
> 
> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
Reviewed-by: Marek Vasut <marex@denx.de>

Re: [PATCH v2 4/4] arm64: dts: st: add RNG node on stm32mp251

[Marek Vasut]

On 10/11/24 5:41 PM, Gatien Chevallier wrote:
> Update the device-tree stm32mp251.dtsi by adding the Random Number
> Generator(RNG) node.
> 
> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
Reviewed-by: Marek Vasut <marex@denx.de>

Re: [PATCH v2 1/4] dt-bindings: rng: add st,stm32mp25-rng support

[Krzysztof Kozlowski]

On Fri, Oct 11, 2024 at 05:41:41PM +0200, Gatien Chevallier wrote:
>    clocks:
> -    maxItems: 1
> +    minItems: 1
> +    maxItems: 2
> +
> +  clock-names:
> +    minItems: 1
> +    items:
> +      - const: core
> +      - const: bus
>  
>    resets:
>      maxItems: 1
> @@ -57,6 +65,26 @@ allOf:
>        properties:
>          st,rng-lock-conf: false
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - st,stm32-rng
> +              - st,stm32mp13-rng
> +    then:
> +      properties:
> +        clocks:
> +          maxItems: 1
> +        clock-names: false
> +    else:
> +      properties:
> +        clocks:
> +          minItems: 2
> +          maxItems: 2

Missing clock-names constraint. They *always* go in sync with clocks.

Best regards,
Krzysztof

Re: [PATCH v2 2/4] hwrng: stm32 - implement support for STM32MP25x platforms

[Gatien CHEVALLIER]

On 10/11/24 18:17, Marek Vasut wrote:
> On 10/11/24 5:41 PM, Gatien Chevallier wrote:
> 
> [...]
> 
>> @@ -551,6 +565,41 @@ static int stm32_rng_probe(struct platform_device 
>> *ofdev)
>>       priv->rng.read = stm32_rng_read;
>>       priv->rng.quality = 900;
>> +    if (!priv->data->nb_clock || priv->data->nb_clock > 2)
>> +        return -EINVAL;
>> +
>> +    priv->clk_bulk = devm_kzalloc(dev, priv->data->nb_clock * 
>> sizeof(*priv->clk_bulk),
>> +                      GFP_KERNEL);
>> +    if (!priv->clk_bulk)
>> +        return -ENOMEM;
> 
> Try this:
> 
> ret = devm_clk_bulk_get(dev, priv->data->nb_clock, priv->clk_bulk);
> ...
> // Swap the clock if they are not in the right order:
> if (priv->data->nb_clock == 2 &&
>      strcmp(__clk_get_name(priv->clk_bulk[0].clk), "core"))
> {
>   const char *id = priv->clk_bulk[1].id;
>   struct clk *clk = priv->clk_bulk[1].clk;
>   priv->clk_bulk[1].id = priv->clk_bulk[0].id;
>   priv->clk_bulk[1].clk = priv->clk_bulk[0].clk;
>   priv->clk_bulk[0].id = id;
>   priv->clk_bulk[0].clk = clk;
> }
> 

Hi Marek,

This won't work as the name returned by this API is clk->core->name.
AFAICT, it doesn't correspond to the names present in the device tree
under the "clock-names" property.
Any other idea or are you fine with what's below?

Thanks,
Gatien

>> +    if (priv->data->nb_clock == 2) {
>> +        struct clk *clk;
>> +        struct clk *bus_clk;
>> +
>> +        clk = devm_clk_get(&ofdev->dev, "core");
>> +        if (IS_ERR(clk))
>> +            return PTR_ERR(clk);
>> +
>> +        bus_clk = devm_clk_get(&ofdev->dev, "bus");
>> +        if (IS_ERR(clk))
>> +            return PTR_ERR(bus_clk);
>> +
>> +        priv->clk_bulk[0].clk = clk;
>> +        priv->clk_bulk[0].id = "core";
>> +        priv->clk_bulk[1].clk = bus_clk;
>> +        priv->clk_bulk[1].id = "bus";
>> +    } else {
>> +        struct clk *clk;
>> +
>> +        clk = devm_clk_get(&ofdev->dev, NULL);
>> +        if (IS_ERR(clk))
>> +            return PTR_ERR(clk);
>> +
>> +        priv->clk_bulk[0].clk = clk;
>> +        priv->clk_bulk[0].id = "core";
>> +    }
> 

Re: [PATCH v2 2/4] hwrng: stm32 - implement support for STM32MP25x platforms

[Marek Vasut]

On 10/14/24 10:38 AM, Gatien CHEVALLIER wrote:
> 
> 
> On 10/11/24 18:17, Marek Vasut wrote:
>> On 10/11/24 5:41 PM, Gatien Chevallier wrote:
>>
>> [...]
>>
>>> @@ -551,6 +565,41 @@ static int stm32_rng_probe(struct 
>>> platform_device *ofdev)
>>>       priv->rng.read = stm32_rng_read;
>>>       priv->rng.quality = 900;
>>> +    if (!priv->data->nb_clock || priv->data->nb_clock > 2)
>>> +        return -EINVAL;
>>> +
>>> +    priv->clk_bulk = devm_kzalloc(dev, priv->data->nb_clock * 
>>> sizeof(*priv->clk_bulk),
>>> +                      GFP_KERNEL);
>>> +    if (!priv->clk_bulk)
>>> +        return -ENOMEM;
>>
>> Try this:
>>
>> ret = devm_clk_bulk_get(dev, priv->data->nb_clock, priv->clk_bulk);
>> ...
>> // Swap the clock if they are not in the right order:
>> if (priv->data->nb_clock == 2 &&
>>      strcmp(__clk_get_name(priv->clk_bulk[0].clk), "core"))
>> {
>>   const char *id = priv->clk_bulk[1].id;
>>   struct clk *clk = priv->clk_bulk[1].clk;
>>   priv->clk_bulk[1].id = priv->clk_bulk[0].id;
>>   priv->clk_bulk[1].clk = priv->clk_bulk[0].clk;
>>   priv->clk_bulk[0].id = id;
>>   priv->clk_bulk[0].clk = clk;
>> }
>>
> 
> Hi Marek,
> 
> This won't work as the name returned by this API is clk->core->name.
> AFAICT, it doesn't correspond to the names present in the device tree
> under the "clock-names" property.
> Any other idea or are you fine with what's below?
Hmmm, it is not great, but at least it reduces the changes throughout 
the driver, so that is an improvement.

I guess one could do some of_clk_get() and clk_is_match() in probe to 
look up the clock in OF by name and then compare which clock is which 
before swapping them in clk_bulk[] array, but that might be too convoluted?

Re: [PATCH v2 1/4] dt-bindings: rng: add st,stm32mp25-rng support

[Gatien CHEVALLIER]

On 10/14/24 09:29, Krzysztof Kozlowski wrote:
> On Fri, Oct 11, 2024 at 05:41:41PM +0200, Gatien Chevallier wrote:
>>     clocks:
>> -    maxItems: 1
>> +    minItems: 1
>> +    maxItems: 2
>> +
>> +  clock-names:
>> +    minItems: 1
>> +    items:
>> +      - const: core
>> +      - const: bus
>>   
>>     resets:
>>       maxItems: 1
>> @@ -57,6 +65,26 @@ allOf:
>>         properties:
>>           st,rng-lock-conf: false
>>   
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - st,stm32-rng
>> +              - st,stm32mp13-rng
>> +    then:
>> +      properties:
>> +        clocks:
>> +          maxItems: 1
>> +        clock-names: false
>> +    else:
>> +      properties:
>> +        clocks:
>> +          minItems: 2
>> +          maxItems: 2
> 
> Missing clock-names constraint. They *always* go in sync with clocks.
> 
> Best regards,
> Krzysztof
> 

Done for V3,

Best regards,
Gatien

Re: [PATCH v2 2/4] hwrng: stm32 - implement support for STM32MP25x platforms

[Gatien CHEVALLIER]

On 10/14/24 10:52, Marek Vasut wrote:
> On 10/14/24 10:38 AM, Gatien CHEVALLIER wrote:
>>
>>
>> On 10/11/24 18:17, Marek Vasut wrote:
>>> On 10/11/24 5:41 PM, Gatien Chevallier wrote:
>>>
>>> [...]
>>>
>>>> @@ -551,6 +565,41 @@ static int stm32_rng_probe(struct 
>>>> platform_device *ofdev)
>>>>       priv->rng.read = stm32_rng_read;
>>>>       priv->rng.quality = 900;
>>>> +    if (!priv->data->nb_clock || priv->data->nb_clock > 2)
>>>> +        return -EINVAL;
>>>> +
>>>> +    priv->clk_bulk = devm_kzalloc(dev, priv->data->nb_clock * 
>>>> sizeof(*priv->clk_bulk),
>>>> +                      GFP_KERNEL);
>>>> +    if (!priv->clk_bulk)
>>>> +        return -ENOMEM;
>>>
>>> Try this:
>>>
>>> ret = devm_clk_bulk_get(dev, priv->data->nb_clock, priv->clk_bulk);
>>> ...
>>> // Swap the clock if they are not in the right order:
>>> if (priv->data->nb_clock == 2 &&
>>>      strcmp(__clk_get_name(priv->clk_bulk[0].clk), "core"))
>>> {
>>>   const char *id = priv->clk_bulk[1].id;
>>>   struct clk *clk = priv->clk_bulk[1].clk;
>>>   priv->clk_bulk[1].id = priv->clk_bulk[0].id;
>>>   priv->clk_bulk[1].clk = priv->clk_bulk[0].clk;
>>>   priv->clk_bulk[0].id = id;
>>>   priv->clk_bulk[0].clk = clk;
>>> }
>>>
>>
>> Hi Marek,
>>
>> This won't work as the name returned by this API is clk->core->name.
>> AFAICT, it doesn't correspond to the names present in the device tree
>> under the "clock-names" property.
>> Any other idea or are you fine with what's below?
> Hmmm, it is not great, but at least it reduces the changes throughout 
> the driver, so that is an improvement.
> 
> I guess one could do some of_clk_get() and clk_is_match() in probe to 
> look up the clock in OF by name and then compare which clock is which 
> before swapping them in clk_bulk[] array, but that might be too convoluted?

Yes, probably too much. What's present in the patch is not close to
perfection but has the advantage of being straightforward. If we agree
on that, I'll send a V3 containing the modifications in the bindings
file.

Thanks for reviewing,
Gatien

Re: [PATCH v2 2/4] hwrng: stm32 - implement support for STM32MP25x platforms

[Marek Vasut]

On 10/14/24 2:36 PM, Gatien CHEVALLIER wrote:
> 
> 
> On 10/14/24 10:52, Marek Vasut wrote:
>> On 10/14/24 10:38 AM, Gatien CHEVALLIER wrote:
>>>
>>>
>>> On 10/11/24 18:17, Marek Vasut wrote:
>>>> On 10/11/24 5:41 PM, Gatien Chevallier wrote:
>>>>
>>>> [...]
>>>>
>>>>> @@ -551,6 +565,41 @@ static int stm32_rng_probe(struct 
>>>>> platform_device *ofdev)
>>>>>       priv->rng.read = stm32_rng_read;
>>>>>       priv->rng.quality = 900;
>>>>> +    if (!priv->data->nb_clock || priv->data->nb_clock > 2)
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    priv->clk_bulk = devm_kzalloc(dev, priv->data->nb_clock * 
>>>>> sizeof(*priv->clk_bulk),
>>>>> +                      GFP_KERNEL);
>>>>> +    if (!priv->clk_bulk)
>>>>> +        return -ENOMEM;
>>>>
>>>> Try this:
>>>>
>>>> ret = devm_clk_bulk_get(dev, priv->data->nb_clock, priv->clk_bulk);
>>>> ...
>>>> // Swap the clock if they are not in the right order:
>>>> if (priv->data->nb_clock == 2 &&
>>>>      strcmp(__clk_get_name(priv->clk_bulk[0].clk), "core"))
>>>> {
>>>>   const char *id = priv->clk_bulk[1].id;
>>>>   struct clk *clk = priv->clk_bulk[1].clk;
>>>>   priv->clk_bulk[1].id = priv->clk_bulk[0].id;
>>>>   priv->clk_bulk[1].clk = priv->clk_bulk[0].clk;
>>>>   priv->clk_bulk[0].id = id;
>>>>   priv->clk_bulk[0].clk = clk;
>>>> }
>>>>
>>>
>>> Hi Marek,
>>>
>>> This won't work as the name returned by this API is clk->core->name.
>>> AFAICT, it doesn't correspond to the names present in the device tree
>>> under the "clock-names" property.
>>> Any other idea or are you fine with what's below?
>> Hmmm, it is not great, but at least it reduces the changes throughout 
>> the driver, so that is an improvement.
>>
>> I guess one could do some of_clk_get() and clk_is_match() in probe to 
>> look up the clock in OF by name and then compare which clock is which 
>> before swapping them in clk_bulk[] array, but that might be too 
>> convoluted?
> 
> Yes, probably too much. What's present in the patch is not close to
> perfection but has the advantage of being straightforward. If we agree
> on that, I'll send a V3 containing the modifications in the bindings
> file.
Errr, I'm sorry, maybe there is a way to do this better. Look at 
drivers/clk/clk-bulk.c :

  15 static int __must_check of_clk_bulk_get(struct device_node *np, int 
num_clks,
  16                                         struct clk_bulk_data *clks)
  17 {
  18         int ret;
  19         int i;
  20
  21         for (i = 0; i < num_clks; i++) {
  22                 clks[i].id = NULL;
  23                 clks[i].clk = NULL;
  24         }
  25
  26         for (i = 0; i < num_clks; i++) {
  27                 of_property_read_string_index(np, "clock-names", i, 
&clks[i].id);
  28                 clks[i].clk = of_clk_get(np, i);

If I read this right, then clks[i].id should be the DT clock name. So 
the swap conditional above could use .id to identify whether the first 
position is core clock or not, like this:

if (priv->data->nb_clock == 2 &&
     strcmp(__clk_get_name(priv->clk_bulk[0].id), "core"))
                                             ^^

You might need to use devm_clk_bulk_get_all() to access the 
of_clk_bulk_get() .

Or am I missing something still ?

Re: [PATCH v2 2/4] hwrng: stm32 - implement support for STM32MP25x platforms

[kernel test robot]

Hi Gatien,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 1d227fcc72223cbdd34d0ce13541cbaab5e0d72f]

url:    https://github.com/intel-lab-lkp/linux/commits/Gatien-Chevallier/dt-bindings-rng-add-st-stm32mp25-rng-support/20241011-234913
base:   1d227fcc72223cbdd34d0ce13541cbaab5e0d72f
patch link:    https://lore.kernel.org/r/20241011-rng-mp25-v2-v2-2-76fd6170280c%40foss.st.com
patch subject: [PATCH v2 2/4] hwrng: stm32 - implement support for STM32MP25x platforms
config: nios2-randconfig-r053-20241015 (https://download.01.org/0day-ci/archive/20241015/202410151421.5UhVRFdF-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 14.1.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410151421.5UhVRFdF-lkp@intel.com/

cocci warnings: (new ones prefixed by >>)
>> drivers/char/hw_random/stm32-rng.c:585:6-12: inconsistent IS_ERR and PTR_ERR on line 586.

vim +585 drivers/char/hw_random/stm32-rng.c

   530	
   531	static int stm32_rng_probe(struct platform_device *ofdev)
   532	{
   533		struct device *dev = &ofdev->dev;
   534		struct device_node *np = ofdev->dev.of_node;
   535		struct stm32_rng_private *priv;
   536		struct resource *res;
   537	
   538		priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
   539		if (!priv)
   540			return -ENOMEM;
   541	
   542		priv->base = devm_platform_get_and_ioremap_resource(ofdev, 0, &res);
   543		if (IS_ERR(priv->base))
   544			return PTR_ERR(priv->base);
   545	
   546		priv->rst = devm_reset_control_get(&ofdev->dev, NULL);
   547		if (!IS_ERR(priv->rst)) {
   548			reset_control_assert(priv->rst);
   549			udelay(2);
   550			reset_control_deassert(priv->rst);
   551		}
   552	
   553		priv->ced = of_property_read_bool(np, "clock-error-detect");
   554		priv->lock_conf = of_property_read_bool(np, "st,rng-lock-conf");
   555		priv->dev = dev;
   556	
   557		priv->data = of_device_get_match_data(dev);
   558		if (!priv->data)
   559			return -ENODEV;
   560	
   561		dev_set_drvdata(dev, priv);
   562	
   563		priv->rng.name = dev_driver_string(dev);
   564		priv->rng.init = stm32_rng_init;
   565		priv->rng.read = stm32_rng_read;
   566		priv->rng.quality = 900;
   567	
   568		if (!priv->data->nb_clock || priv->data->nb_clock > 2)
   569			return -EINVAL;
   570	
   571		priv->clk_bulk = devm_kzalloc(dev, priv->data->nb_clock * sizeof(*priv->clk_bulk),
   572					      GFP_KERNEL);
   573		if (!priv->clk_bulk)
   574			return -ENOMEM;
   575	
   576		if (priv->data->nb_clock == 2) {
   577			struct clk *clk;
   578			struct clk *bus_clk;
   579	
   580			clk = devm_clk_get(&ofdev->dev, "core");
   581			if (IS_ERR(clk))
   582				return PTR_ERR(clk);
   583	
   584			bus_clk = devm_clk_get(&ofdev->dev, "bus");
 > 585			if (IS_ERR(clk))
 > 586				return PTR_ERR(bus_clk);
   587	
   588			priv->clk_bulk[0].clk = clk;
   589			priv->clk_bulk[0].id = "core";
   590			priv->clk_bulk[1].clk = bus_clk;
   591			priv->clk_bulk[1].id = "bus";
   592		} else {
   593			struct clk *clk;
   594	
   595			clk = devm_clk_get(&ofdev->dev, NULL);
   596			if (IS_ERR(clk))
   597				return PTR_ERR(clk);
   598	
   599			priv->clk_bulk[0].clk = clk;
   600			priv->clk_bulk[0].id = "core";
   601		}
   602	
   603		pm_runtime_set_autosuspend_delay(dev, 100);
   604		pm_runtime_use_autosuspend(dev);
   605		pm_runtime_enable(dev);
   606	
   607		return devm_hwrng_register(dev, &priv->rng);
   608	}
   609	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 2/4] hwrng: stm32 - implement support for STM32MP25x platforms

[Gatien CHEVALLIER]

On 10/14/24 20:55, Marek Vasut wrote:
> On 10/14/24 2:36 PM, Gatien CHEVALLIER wrote:
>>
>>
>> On 10/14/24 10:52, Marek Vasut wrote:
>>> On 10/14/24 10:38 AM, Gatien CHEVALLIER wrote:
>>>>
>>>>
>>>> On 10/11/24 18:17, Marek Vasut wrote:
>>>>> On 10/11/24 5:41 PM, Gatien Chevallier wrote:
>>>>>
>>>>> [...]
>>>>>
>>>>>> @@ -551,6 +565,41 @@ static int stm32_rng_probe(struct 
>>>>>> platform_device *ofdev)
>>>>>>       priv->rng.read = stm32_rng_read;
>>>>>>       priv->rng.quality = 900;
>>>>>> +    if (!priv->data->nb_clock || priv->data->nb_clock > 2)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    priv->clk_bulk = devm_kzalloc(dev, priv->data->nb_clock * 
>>>>>> sizeof(*priv->clk_bulk),
>>>>>> +                      GFP_KERNEL);
>>>>>> +    if (!priv->clk_bulk)
>>>>>> +        return -ENOMEM;
>>>>>
>>>>> Try this:
>>>>>
>>>>> ret = devm_clk_bulk_get(dev, priv->data->nb_clock, priv->clk_bulk);
>>>>> ...
>>>>> // Swap the clock if they are not in the right order:
>>>>> if (priv->data->nb_clock == 2 &&
>>>>>      strcmp(__clk_get_name(priv->clk_bulk[0].clk), "core"))
>>>>> {
>>>>>   const char *id = priv->clk_bulk[1].id;
>>>>>   struct clk *clk = priv->clk_bulk[1].clk;
>>>>>   priv->clk_bulk[1].id = priv->clk_bulk[0].id;
>>>>>   priv->clk_bulk[1].clk = priv->clk_bulk[0].clk;
>>>>>   priv->clk_bulk[0].id = id;
>>>>>   priv->clk_bulk[0].clk = clk;
>>>>> }
>>>>>
>>>>
>>>> Hi Marek,
>>>>
>>>> This won't work as the name returned by this API is clk->core->name.
>>>> AFAICT, it doesn't correspond to the names present in the device tree
>>>> under the "clock-names" property.
>>>> Any other idea or are you fine with what's below?
>>> Hmmm, it is not great, but at least it reduces the changes throughout 
>>> the driver, so that is an improvement.
>>>
>>> I guess one could do some of_clk_get() and clk_is_match() in probe to 
>>> look up the clock in OF by name and then compare which clock is which 
>>> before swapping them in clk_bulk[] array, but that might be too 
>>> convoluted?
>>
>> Yes, probably too much. What's present in the patch is not close to
>> perfection but has the advantage of being straightforward. If we agree
>> on that, I'll send a V3 containing the modifications in the bindings
>> file.
> Errr, I'm sorry, maybe there is a way to do this better. Look at 
> drivers/clk/clk-bulk.c :
> 
>   15 static int __must_check of_clk_bulk_get(struct device_node *np, int 
> num_clks,
>   16                                         struct clk_bulk_data *clks)
>   17 {
>   18         int ret;
>   19         int i;
>   20
>   21         for (i = 0; i < num_clks; i++) {
>   22                 clks[i].id = NULL;
>   23                 clks[i].clk = NULL;
>   24         }
>   25
>   26         for (i = 0; i < num_clks; i++) {
>   27                 of_property_read_string_index(np, "clock-names", i, 
> &clks[i].id);
>   28                 clks[i].clk = of_clk_get(np, i);
> 
> If I read this right, then clks[i].id should be the DT clock name. So 
> the swap conditional above could use .id to identify whether the first 
> position is core clock or not, like this:
> 
> if (priv->data->nb_clock == 2 &&
>      strcmp(__clk_get_name(priv->clk_bulk[0].id), "core"))
>                                              ^^
> 
> You might need to use devm_clk_bulk_get_all() to access the 
> of_clk_bulk_get() .
> 
> Or am I missing something still ?

Oooooh I see, devm_clk_bulk_get() and devm_clk_bulk_get_all() use
a different path. I don't understand why, to be honest... The doc
doesn't state this difference either.

I'll give this a try while also correcting the issue that the robot
highlighted.

Best regards,
Gatien

Re: [PATCH v2 2/4] hwrng: stm32 - implement support for STM32MP25x platforms

[Marek Vasut]

On 10/15/24 5:10 PM, Gatien CHEVALLIER wrote:
> 
> 
> On 10/14/24 20:55, Marek Vasut wrote:
>> On 10/14/24 2:36 PM, Gatien CHEVALLIER wrote:
>>>
>>>
>>> On 10/14/24 10:52, Marek Vasut wrote:
>>>> On 10/14/24 10:38 AM, Gatien CHEVALLIER wrote:
>>>>>
>>>>>
>>>>> On 10/11/24 18:17, Marek Vasut wrote:
>>>>>> On 10/11/24 5:41 PM, Gatien Chevallier wrote:
>>>>>>
>>>>>> [...]
>>>>>>
>>>>>>> @@ -551,6 +565,41 @@ static int stm32_rng_probe(struct 
>>>>>>> platform_device *ofdev)
>>>>>>>       priv->rng.read = stm32_rng_read;
>>>>>>>       priv->rng.quality = 900;
>>>>>>> +    if (!priv->data->nb_clock || priv->data->nb_clock > 2)
>>>>>>> +        return -EINVAL;
>>>>>>> +
>>>>>>> +    priv->clk_bulk = devm_kzalloc(dev, priv->data->nb_clock * 
>>>>>>> sizeof(*priv->clk_bulk),
>>>>>>> +                      GFP_KERNEL);
>>>>>>> +    if (!priv->clk_bulk)
>>>>>>> +        return -ENOMEM;
>>>>>>
>>>>>> Try this:
>>>>>>
>>>>>> ret = devm_clk_bulk_get(dev, priv->data->nb_clock, priv->clk_bulk);
>>>>>> ...
>>>>>> // Swap the clock if they are not in the right order:
>>>>>> if (priv->data->nb_clock == 2 &&
>>>>>>      strcmp(__clk_get_name(priv->clk_bulk[0].clk), "core"))
>>>>>> {
>>>>>>   const char *id = priv->clk_bulk[1].id;
>>>>>>   struct clk *clk = priv->clk_bulk[1].clk;
>>>>>>   priv->clk_bulk[1].id = priv->clk_bulk[0].id;
>>>>>>   priv->clk_bulk[1].clk = priv->clk_bulk[0].clk;
>>>>>>   priv->clk_bulk[0].id = id;
>>>>>>   priv->clk_bulk[0].clk = clk;
>>>>>> }
>>>>>>
>>>>>
>>>>> Hi Marek,
>>>>>
>>>>> This won't work as the name returned by this API is clk->core->name.
>>>>> AFAICT, it doesn't correspond to the names present in the device tree
>>>>> under the "clock-names" property.
>>>>> Any other idea or are you fine with what's below?
>>>> Hmmm, it is not great, but at least it reduces the changes 
>>>> throughout the driver, so that is an improvement.
>>>>
>>>> I guess one could do some of_clk_get() and clk_is_match() in probe 
>>>> to look up the clock in OF by name and then compare which clock is 
>>>> which before swapping them in clk_bulk[] array, but that might be 
>>>> too convoluted?
>>>
>>> Yes, probably too much. What's present in the patch is not close to
>>> perfection but has the advantage of being straightforward. If we agree
>>> on that, I'll send a V3 containing the modifications in the bindings
>>> file.
>> Errr, I'm sorry, maybe there is a way to do this better. Look at 
>> drivers/clk/clk-bulk.c :
>>
>>   15 static int __must_check of_clk_bulk_get(struct device_node *np, 
>> int num_clks,
>>   16                                         struct clk_bulk_data *clks)
>>   17 {
>>   18         int ret;
>>   19         int i;
>>   20
>>   21         for (i = 0; i < num_clks; i++) {
>>   22                 clks[i].id = NULL;
>>   23                 clks[i].clk = NULL;
>>   24         }
>>   25
>>   26         for (i = 0; i < num_clks; i++) {
>>   27                 of_property_read_string_index(np, "clock-names", 
>> i, &clks[i].id);
>>   28                 clks[i].clk = of_clk_get(np, i);
>>
>> If I read this right, then clks[i].id should be the DT clock name. So 
>> the swap conditional above could use .id to identify whether the first 
>> position is core clock or not, like this:
>>
>> if (priv->data->nb_clock == 2 &&
>>      strcmp(__clk_get_name(priv->clk_bulk[0].id), "core"))
>>                                              ^^
>>
>> You might need to use devm_clk_bulk_get_all() to access the 
>> of_clk_bulk_get() .
>>
>> Or am I missing something still ?
> 
> Oooooh I see, devm_clk_bulk_get() and devm_clk_bulk_get_all() use
> a different path. I don't understand why, to be honest... The doc
> doesn't state this difference either.

Indeed, but maybe git log could clarify that ? I learnt about this 
useful trick at last year Embedded Recipes:

$ git log -L:clk_bulk_get_all:drivers/clk/clk-bulk.c

> I'll give this a try while also correcting the issue that the robot
> highlighted.
Thank you !

Re: [PATCH v2 2/4] hwrng: stm32 - implement support for STM32MP25x platforms

[Gatien CHEVALLIER]

On 10/15/24 17:39, Marek Vasut wrote:
> On 10/15/24 5:10 PM, Gatien CHEVALLIER wrote:
>>
>>
>> On 10/14/24 20:55, Marek Vasut wrote:
>>> On 10/14/24 2:36 PM, Gatien CHEVALLIER wrote:
>>>>
>>>>
>>>> On 10/14/24 10:52, Marek Vasut wrote:
>>>>> On 10/14/24 10:38 AM, Gatien CHEVALLIER wrote:
>>>>>>
>>>>>>
>>>>>> On 10/11/24 18:17, Marek Vasut wrote:
>>>>>>> On 10/11/24 5:41 PM, Gatien Chevallier wrote:
>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>> @@ -551,6 +565,41 @@ static int stm32_rng_probe(struct 
>>>>>>>> platform_device *ofdev)
>>>>>>>>       priv->rng.read = stm32_rng_read;
>>>>>>>>       priv->rng.quality = 900;
>>>>>>>> +    if (!priv->data->nb_clock || priv->data->nb_clock > 2)
>>>>>>>> +        return -EINVAL;
>>>>>>>> +
>>>>>>>> +    priv->clk_bulk = devm_kzalloc(dev, priv->data->nb_clock * 
>>>>>>>> sizeof(*priv->clk_bulk),
>>>>>>>> +                      GFP_KERNEL);
>>>>>>>> +    if (!priv->clk_bulk)
>>>>>>>> +        return -ENOMEM;
>>>>>>>
>>>>>>> Try this:
>>>>>>>
>>>>>>> ret = devm_clk_bulk_get(dev, priv->data->nb_clock, priv->clk_bulk);
>>>>>>> ...
>>>>>>> // Swap the clock if they are not in the right order:
>>>>>>> if (priv->data->nb_clock == 2 &&
>>>>>>>      strcmp(__clk_get_name(priv->clk_bulk[0].clk), "core"))
>>>>>>> {
>>>>>>>   const char *id = priv->clk_bulk[1].id;
>>>>>>>   struct clk *clk = priv->clk_bulk[1].clk;
>>>>>>>   priv->clk_bulk[1].id = priv->clk_bulk[0].id;
>>>>>>>   priv->clk_bulk[1].clk = priv->clk_bulk[0].clk;
>>>>>>>   priv->clk_bulk[0].id = id;
>>>>>>>   priv->clk_bulk[0].clk = clk;
>>>>>>> }
>>>>>>>
>>>>>>
>>>>>> Hi Marek,
>>>>>>
>>>>>> This won't work as the name returned by this API is clk->core->name.
>>>>>> AFAICT, it doesn't correspond to the names present in the device tree
>>>>>> under the "clock-names" property.
>>>>>> Any other idea or are you fine with what's below?
>>>>> Hmmm, it is not great, but at least it reduces the changes 
>>>>> throughout the driver, so that is an improvement.
>>>>>
>>>>> I guess one could do some of_clk_get() and clk_is_match() in probe 
>>>>> to look up the clock in OF by name and then compare which clock is 
>>>>> which before swapping them in clk_bulk[] array, but that might be 
>>>>> too convoluted?
>>>>
>>>> Yes, probably too much. What's present in the patch is not close to
>>>> perfection but has the advantage of being straightforward. If we agree
>>>> on that, I'll send a V3 containing the modifications in the bindings
>>>> file.
>>> Errr, I'm sorry, maybe there is a way to do this better. Look at 
>>> drivers/clk/clk-bulk.c :
>>>
>>>   15 static int __must_check of_clk_bulk_get(struct device_node *np, 
>>> int num_clks,
>>>   16                                         struct clk_bulk_data *clks)
>>>   17 {
>>>   18         int ret;
>>>   19         int i;
>>>   20
>>>   21         for (i = 0; i < num_clks; i++) {
>>>   22                 clks[i].id = NULL;
>>>   23                 clks[i].clk = NULL;
>>>   24         }
>>>   25
>>>   26         for (i = 0; i < num_clks; i++) {
>>>   27                 of_property_read_string_index(np, "clock-names", 
>>> i, &clks[i].id);
>>>   28                 clks[i].clk = of_clk_get(np, i);
>>>
>>> If I read this right, then clks[i].id should be the DT clock name. So 
>>> the swap conditional above could use .id to identify whether the 
>>> first position is core clock or not, like this:
>>>
>>> if (priv->data->nb_clock == 2 &&
>>>      strcmp(__clk_get_name(priv->clk_bulk[0].id), "core"))
>>>                                              ^^
>>>
>>> You might need to use devm_clk_bulk_get_all() to access the 
>>> of_clk_bulk_get() .
>>>
>>> Or am I missing something still ?
>>
>> Oooooh I see, devm_clk_bulk_get() and devm_clk_bulk_get_all() use
>> a different path. I don't understand why, to be honest... The doc
>> doesn't state this difference either.
> 
> Indeed, but maybe git log could clarify that ? I learnt about this 
> useful trick at last year Embedded Recipes:
> 
> $ git log -L:clk_bulk_get_all:drivers/clk/clk-bulk.c
> 

Will definitely keep that somewhere, thanks.

>> I'll give this a try while also correcting the issue that the robot
>> highlighted.
> Thank you !