| Message ID | 20241007132721.168428-3-gatien.chevallier@foss.st.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for stm32mp25x RNG | expand |
On 10/11/24 13:24, Marek Vasut wrote: > On 10/11/24 11:55 AM, Gatien CHEVALLIER wrote: >> >> >> On 10/7/24 15:54, Marek Vasut wrote: >>> On 10/7/24 3:27 PM, Gatien Chevallier wrote: >>>> 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> >>> A bit of a higher-level design question -- can you use drivers/clk/ >>> clk-bulk.c clk_bulk_*() to handle all these disparate count of clock >>> easily ? >> >> Hi, I'd like to make sure that we enable the core clock before the bus >> clock so that the RNG hardware block can start its internal tests while >> we ungate the bus clock. It's not a strong opinion but it feels better. > Maybe this could still work if the struct clk_bulk_data {} is ordered > that way, so the bus clock are first, and the rest afterward ? I guess you meant, the core first. Putting the bus clock first with the updated YAML doc generates a warning when checking the bindings. I guess what you propose is OK then. Core clock is defined first in the device tree.
On 10/11/24 2:07 PM, Gatien CHEVALLIER wrote: > > > On 10/11/24 13:24, Marek Vasut wrote: >> On 10/11/24 11:55 AM, Gatien CHEVALLIER wrote: >>> >>> >>> On 10/7/24 15:54, Marek Vasut wrote: >>>> On 10/7/24 3:27 PM, Gatien Chevallier wrote: >>>>> 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> >>>> A bit of a higher-level design question -- can you use drivers/clk/ >>>> clk-bulk.c clk_bulk_*() to handle all these disparate count of clock >>>> easily ? >>> >>> Hi, I'd like to make sure that we enable the core clock before the bus >>> clock so that the RNG hardware block can start its internal tests while >>> we ungate the bus clock. It's not a strong opinion but it feels better. >> Maybe this could still work if the struct clk_bulk_data {} is ordered >> that way, so the bus clock are first, and the rest afterward ? > > I guess you meant, the core first. Err, yes, core. > Putting the bus clock first with the updated YAML doc generates a > warning when checking the bindings. I guess what you propose is OK > then. Core clock is defined first in the device tree. Not in DT, leave DT as-is. Look at struct clk_bulk_data , I think when you use the clk_bulk_*() functions, you pass in a list of struct clk_bulk_data, which each describes one clock, so just make sure that list of struct clk_bulk_data in the driver is ordered the way you need it to be ordered and you should be fine.
On 10/11/24 14:38, Marek Vasut wrote: > On 10/11/24 2:07 PM, Gatien CHEVALLIER wrote: >> >> >> On 10/11/24 13:24, Marek Vasut wrote: >>> On 10/11/24 11:55 AM, Gatien CHEVALLIER wrote: >>>> >>>> >>>> On 10/7/24 15:54, Marek Vasut wrote: >>>>> On 10/7/24 3:27 PM, Gatien Chevallier wrote: >>>>>> 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> >>>>> A bit of a higher-level design question -- can you use drivers/clk/ >>>>> clk-bulk.c clk_bulk_*() to handle all these disparate count of >>>>> clock easily ? >>>> >>>> Hi, I'd like to make sure that we enable the core clock before the bus >>>> clock so that the RNG hardware block can start its internal tests while >>>> we ungate the bus clock. It's not a strong opinion but it feels better. >>> Maybe this could still work if the struct clk_bulk_data {} is ordered >>> that way, so the bus clock are first, and the rest afterward ? >> >> I guess you meant, the core first. > > Err, yes, core. > >> Putting the bus clock first with the updated YAML doc generates a >> warning when checking the bindings. I guess what you propose is OK >> then. Core clock is defined first in the device tree. > > Not in DT, leave DT as-is. Look at struct clk_bulk_data , I think when > you use the clk_bulk_*() functions, you pass in a list of struct > clk_bulk_data, which each describes one clock, so just make sure that > list of struct clk_bulk_data in the driver is ordered the way you need > it to be ordered and you should be fine. I've sent a V2 with something that is functional but not aesthetic. You'll tell me if that's what you had in mind. Best regards, Gatien
On 10/11/24 5:51 PM, Gatien CHEVALLIER wrote: > > > On 10/11/24 14:38, Marek Vasut wrote: >> On 10/11/24 2:07 PM, Gatien CHEVALLIER wrote: >>> >>> >>> On 10/11/24 13:24, Marek Vasut wrote: >>>> On 10/11/24 11:55 AM, Gatien CHEVALLIER wrote: >>>>> >>>>> >>>>> On 10/7/24 15:54, Marek Vasut wrote: >>>>>> On 10/7/24 3:27 PM, Gatien Chevallier wrote: >>>>>>> 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> >>>>>> A bit of a higher-level design question -- can you use drivers/ >>>>>> clk/ clk-bulk.c clk_bulk_*() to handle all these disparate count >>>>>> of clock easily ? >>>>> >>>>> Hi, I'd like to make sure that we enable the core clock before the bus >>>>> clock so that the RNG hardware block can start its internal tests >>>>> while >>>>> we ungate the bus clock. It's not a strong opinion but it feels >>>>> better. >>>> Maybe this could still work if the struct clk_bulk_data {} is >>>> ordered that way, so the bus clock are first, and the rest afterward ? >>> >>> I guess you meant, the core first. >> >> Err, yes, core. >> >>> Putting the bus clock first with the updated YAML doc generates a >>> warning when checking the bindings. I guess what you propose is OK >>> then. Core clock is defined first in the device tree. >> >> Not in DT, leave DT as-is. Look at struct clk_bulk_data , I think when >> you use the clk_bulk_*() functions, you pass in a list of struct >> clk_bulk_data, which each describes one clock, so just make sure that >> list of struct clk_bulk_data in the driver is ordered the way you need >> it to be ordered and you should be fine. > > I've sent a V2 with something that is functional but not aesthetic. > You'll tell me if that's what you had in mind. I sent you a slightly tweaked example which should work too and should be a bit nicer.
diff --git a/drivers/char/hw_random/stm32-rng.c b/drivers/char/hw_random/stm32-rng.c index 9d041a67c295..e7051005768d 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; @@ -73,6 +74,7 @@ struct stm32_rng_private { struct device *dev; void __iomem *base; struct clk *clk; + struct clk *bus_clk; struct reset_control *rst; struct stm32_rng_config pm_conf; const struct stm32_rng_data *data; @@ -292,6 +294,14 @@ static int stm32_rng_init(struct hwrng *rng) if (err) return err; + if (priv->bus_clk) { + err = clk_prepare_enable(priv->bus_clk); + if (err) { + clk_disable_unprepare(priv->clk); + return err; + } + } + /* clear error indicators */ writel_relaxed(0, priv->base + RNG_SR); @@ -329,6 +339,8 @@ static int stm32_rng_init(struct hwrng *rng) 10, 50000); if (err) { clk_disable_unprepare(priv->clk); + if (priv->bus_clk) + clk_disable_unprepare(priv->bus_clk); dev_err(priv->dev, "%s: timeout %x!\n", __func__, reg); return -EINVAL; } @@ -356,8 +368,11 @@ static int stm32_rng_init(struct hwrng *rng) reg & RNG_SR_DRDY, 10, 100000); if (err || (reg & ~RNG_SR_DRDY)) { + if (priv->bus_clk) + clk_disable_unprepare(priv->bus_clk); clk_disable_unprepare(priv->clk); dev_err(priv->dev, "%s: timeout:%x SR: %x!\n", __func__, err, reg); + return -EINVAL; } @@ -379,6 +394,9 @@ 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); + + if (priv->bus_clk) + clk_disable_unprepare(priv->bus_clk); clk_disable_unprepare(priv->clk); return 0; @@ -393,6 +411,14 @@ static int __maybe_unused stm32_rng_suspend(struct device *dev) if (err) return err; + if (priv->bus_clk) { + err = clk_prepare_enable(priv->bus_clk); + if (err) { + clk_disable_unprepare(priv->clk); + return err; + } + } + if (priv->data->has_cond_reset) { priv->pm_conf.nscr = readl_relaxed(priv->base + RNG_NSCR); priv->pm_conf.htcr = readl_relaxed(priv->base + RNG_HTCR); @@ -403,6 +429,8 @@ static int __maybe_unused stm32_rng_suspend(struct device *dev) writel_relaxed(priv->pm_conf.cr, priv->base + RNG_CR); + if (priv->bus_clk) + clk_disable_unprepare(priv->bus_clk); clk_disable_unprepare(priv->clk); return 0; @@ -418,6 +446,14 @@ static int __maybe_unused stm32_rng_runtime_resume(struct device *dev) if (err) return err; + if (priv->bus_clk) { + err = clk_prepare_enable(priv->bus_clk); + if (err) { + clk_disable_unprepare(priv->clk); + return err; + } + } + /* Clean error indications */ writel_relaxed(0, priv->base + RNG_SR); @@ -438,6 +474,14 @@ static int __maybe_unused stm32_rng_resume(struct device *dev) if (err) return err; + if (priv->bus_clk) { + err = clk_prepare_enable(priv->bus_clk); + if (err) { + clk_disable_unprepare(priv->clk); + return err; + } + } + /* Clean error indications */ writel_relaxed(0, priv->base + RNG_SR); @@ -462,6 +506,8 @@ static int __maybe_unused stm32_rng_resume(struct device *dev) reg & ~RNG_CR_CONDRST, 10, 100000); if (err) { + if (priv->bus_clk) + clk_disable_unprepare(priv->bus_clk); clk_disable_unprepare(priv->clk); dev_err(priv->dev, "%s: timeout:%x CR: %x!\n", __func__, err, reg); return -EINVAL; @@ -473,6 +519,8 @@ static int __maybe_unused stm32_rng_resume(struct device *dev) } clk_disable_unprepare(priv->clk); + if (priv->bus_clk) + clk_disable_unprepare(priv->bus_clk); return 0; } @@ -484,9 +532,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 +553,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 +588,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 +610,20 @@ static int stm32_rng_probe(struct platform_device *ofdev) priv->rng.read = stm32_rng_read; priv->rng.quality = 900; + if (priv->data->nb_clock > 1) { + priv->clk = devm_clk_get(&ofdev->dev, "rng_clk"); + if (IS_ERR(priv->clk)) + return PTR_ERR(priv->clk); + + priv->bus_clk = devm_clk_get(&ofdev->dev, "rng_hclk"); + if (IS_ERR(priv->clk)) + return PTR_ERR(priv->bus_clk); + } else { + priv->clk = devm_clk_get(&ofdev->dev, NULL); + if (IS_ERR(priv->clk)) + return PTR_ERR(priv->clk); + } + 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> --- drivers/char/hw_random/stm32-rng.c | 81 ++++++++++++++++++++++++++++-- 1 file changed, 77 insertions(+), 4 deletions(-)