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

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

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";
> +	}

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";
>> +    }
> 

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?

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

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 ?

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

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 !

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 !