| Message ID | 20241011-rng-mp25-v2-v2-2-76fd6170280c@foss.st.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | Add support for stm32mp25x RNG | expand |
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 ?
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
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 !
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);
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(-)