[PATCH-for-9.0? v2 0/8] hw/clock: Propagate clock changes when STM32L4X5 MUX is updated

[PATCH-for-9.0? v2 0/8] hw/clock: Propagate clock changes when STM32L4X5 MUX is updated

[Philippe Mathieu-Daudé]

Since v1:
- Rework API to only propagate when both clock_set
  and clock_set_mul_div modified the clock params
  (Peter & Luc).
- Use that in zynq_slcr.

Per https://www.qemu.org/docs/master/devel/clocks.html#clock-multiplier-and-divider-settings:

  Note that clock_set_mul_div() does not automatically call
  clock_propagate(). If you make a runtime change to the
  multiplier or divider you must call clock_propagate() yourself.

Fix what we forgot to do that in recent commit ec7d83acbd
("hw/misc/stm32l4x5_rcc: Add an internal clock multiplexer object")

Arnaud Minier (1):
  hw/misc/stm32l4x5_rcc: Propagate period when enabling a clock

Philippe Mathieu-Daudé (7):
  hw/clock: Have clock_set_mul_div() return early when nothing to change
  hw/clock: Pass optional &bool argument to clock_set()
  hw/clock: Pass optional &bool argument to clock_set_ns()
  hw/clock: Pass optional &bool argument to clock_set_hz()
  hw/clock: Pass optional &bool argument to clock_set_mul_div()
  hw/misc/stm32l4x5_rcc: Inline clock_update() in clock_mux_update()
  hw/misc/zynq_slcr: Only propagate clock changes when necessary

 docs/devel/clocks.rst      |  6 +++++-
 include/hw/clock.h         | 21 +++++++++++---------
 hw/arm/aspeed.c            |  2 +-
 hw/arm/fby35.c             |  2 +-
 hw/arm/mps2-tz.c           |  4 ++--
 hw/arm/mps2.c              |  4 ++--
 hw/arm/mps3r.c             |  2 +-
 hw/arm/msf2-soc.c          |  2 +-
 hw/arm/msf2-som.c          |  2 +-
 hw/arm/musca.c             |  4 ++--
 hw/arm/netduino2.c         |  2 +-
 hw/arm/netduinoplus2.c     |  2 +-
 hw/arm/nrf51_soc.c         |  2 +-
 hw/arm/olimex-stm32-h405.c |  2 +-
 hw/arm/stellaris.c         |  2 +-
 hw/arm/stm32f100_soc.c     |  2 +-
 hw/arm/stm32f205_soc.c     |  2 +-
 hw/arm/stm32f405_soc.c     |  2 +-
 hw/arm/stm32vldiscovery.c  |  2 +-
 hw/arm/xilinx_zynq.c       |  2 +-
 hw/char/cadence_uart.c     |  4 ++--
 hw/core/clock.c            | 19 +++++++++++++++----
 hw/misc/bcm2835_cprman.c   |  2 +-
 hw/misc/stm32l4x5_rcc.c    |  9 +++++++--
 hw/misc/zynq_slcr.c        | 39 +++++++++++++++++++++++++-------------
 25 files changed, 89 insertions(+), 53 deletions(-)

-- 
2.41.0

[PATCH-for-9.0 v2 1/8] hw/clock: Have clock_set_mul_div() return early when nothing to change

[Philippe Mathieu-Daudé]

Return early when clock_set_mul_div() is called with
same mul/div values the clock has.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/core/clock.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/core/clock.c b/hw/core/clock.c
index d82e44cd1a..c73f0c2f98 100644
--- a/hw/core/clock.c
+++ b/hw/core/clock.c
@@ -147,6 +147,10 @@ void clock_set_mul_div(Clock *clk, uint32_t multiplier, uint32_t divider)
 {
     assert(divider != 0);
 
+    if (clk->multiplier == multiplier && clk->divider == divider) {
+        return;
+    }
+
     trace_clock_set_mul_div(CLOCK_PATH(clk), clk->multiplier, multiplier,
                             clk->divider, divider);
     clk->multiplier = multiplier;
-- 
2.41.0

Re: [PATCH-for-9.0 v2 1/8] hw/clock: Have clock_set_mul_div() return early when nothing to change

[Alistair Francis]

On Mon, Mar 25, 2024 at 11:34 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> Return early when clock_set_mul_div() is called with
> same mul/div values the clock has.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/core/clock.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/core/clock.c b/hw/core/clock.c
> index d82e44cd1a..c73f0c2f98 100644
> --- a/hw/core/clock.c
> +++ b/hw/core/clock.c
> @@ -147,6 +147,10 @@ void clock_set_mul_div(Clock *clk, uint32_t multiplier, uint32_t divider)
>  {
>      assert(divider != 0);
>
> +    if (clk->multiplier == multiplier && clk->divider == divider) {
> +        return;
> +    }
> +
>      trace_clock_set_mul_div(CLOCK_PATH(clk), clk->multiplier, multiplier,
>                              clk->divider, divider);
>      clk->multiplier = multiplier;
> --
> 2.41.0
>
>

[PATCH-for-9.0? v2 2/8] hw/clock: Pass optional &bool argument to clock_set()

[Philippe Mathieu-Daudé]

Currently clock_set() returns whether the clock has
been changed or not. In order to combine this information
with other clock calls, pass an optional boolean and do
not return anything.  The single caller ignores the return
value, have it use NULL.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/clock.h       | 22 ++++++++++++++++------
 hw/core/clock.c          |  8 +++++---
 hw/misc/bcm2835_cprman.c |  2 +-
 hw/misc/zynq_slcr.c      |  4 ++--
 4 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/include/hw/clock.h b/include/hw/clock.h
index bb12117f67..474bbc07fe 100644
--- a/include/hw/clock.h
+++ b/include/hw/clock.h
@@ -180,21 +180,28 @@ static inline bool clock_has_source(const Clock *clk)
  * clock_set:
  * @clk: the clock to initialize.
  * @value: the clock's value, 0 means unclocked
+ * @changed: set to true if the clock is changed, ignored if set to NULL.
  *
  * Set the local cached period value of @clk to @value.
- *
- * @return: true if the clock is changed.
  */
-bool clock_set(Clock *clk, uint64_t value);
+void clock_set(Clock *clk, uint64_t period, bool *changed);
 
 static inline bool clock_set_hz(Clock *clk, unsigned hz)
 {
-    return clock_set(clk, CLOCK_PERIOD_FROM_HZ(hz));
+    bool changed = false;
+
+    clock_set(clk, CLOCK_PERIOD_FROM_HZ(hz), &changed);
+
+    return changed;
 }
 
 static inline bool clock_set_ns(Clock *clk, unsigned ns)
 {
-    return clock_set(clk, CLOCK_PERIOD_FROM_NS(ns));
+    bool changed = false;
+
+    clock_set(clk, CLOCK_PERIOD_FROM_NS(ns), &changed);
+
+    return changed;
 }
 
 /**
@@ -220,7 +227,10 @@ void clock_propagate(Clock *clk);
  */
 static inline void clock_update(Clock *clk, uint64_t value)
 {
-    if (clock_set(clk, value)) {
+    bool changed = false;
+
+    clock_set(clk, value, &changed);
+    if (changed) {
         clock_propagate(clk);
     }
 }
diff --git a/hw/core/clock.c b/hw/core/clock.c
index c73f0c2f98..e0f257b141 100644
--- a/hw/core/clock.c
+++ b/hw/core/clock.c
@@ -52,16 +52,18 @@ void clock_clear_callback(Clock *clk)
     clock_set_callback(clk, NULL, NULL, 0);
 }
 
-bool clock_set(Clock *clk, uint64_t period)
+void clock_set(Clock *clk, uint64_t period, bool *changed)
 {
     if (clk->period == period) {
-        return false;
+        return;
     }
     trace_clock_set(CLOCK_PATH(clk), CLOCK_PERIOD_TO_HZ(clk->period),
                     CLOCK_PERIOD_TO_HZ(period));
     clk->period = period;
 
-    return true;
+    if (changed) {
+        *changed = true;
+    }
 }
 
 static uint64_t clock_get_child_period(Clock *clk)
diff --git a/hw/misc/bcm2835_cprman.c b/hw/misc/bcm2835_cprman.c
index 91c8f7bd17..f16f7978ae 100644
--- a/hw/misc/bcm2835_cprman.c
+++ b/hw/misc/bcm2835_cprman.c
@@ -680,7 +680,7 @@ static void cprman_init(Object *obj)
     s->xosc = clock_new(obj, "xosc");
     s->gnd = clock_new(obj, "gnd");
 
-    clock_set(s->gnd, 0);
+    clock_set(s->gnd, 0, NULL);
 
     memory_region_init_io(&s->iomem, obj, &cprman_ops,
                           s, "bcm2835-cprman", 0x2000);
diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
index d2ac2e77f2..e637798507 100644
--- a/hw/misc/zynq_slcr.c
+++ b/hw/misc/zynq_slcr.c
@@ -279,9 +279,9 @@ static void zynq_slcr_compute_clocks_internal(ZynqSLCRState *s, uint64_t ps_clk)
 
     /* compute uartX reference clocks */
     clock_set(s->uart0_ref_clk,
-              ZYNQ_COMPUTE_CLK(s, uart_mux, R_UART_CLK_CTRL, CLKACT0));
+              ZYNQ_COMPUTE_CLK(s, uart_mux, R_UART_CLK_CTRL, CLKACT0), NULL);
     clock_set(s->uart1_ref_clk,
-              ZYNQ_COMPUTE_CLK(s, uart_mux, R_UART_CLK_CTRL, CLKACT1));
+              ZYNQ_COMPUTE_CLK(s, uart_mux, R_UART_CLK_CTRL, CLKACT1), NULL);
 }
 
 /**
-- 
2.41.0

Re: [PATCH-for-9.0? v2 2/8] hw/clock: Pass optional &bool argument to clock_set()

[Peter Maydell]

On Mon, 25 Mar 2024 at 13:33, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Currently clock_set() returns whether the clock has
> been changed or not. In order to combine this information
> with other clock calls, pass an optional boolean and do
> not return anything.  The single caller ignores the return
> value, have it use NULL.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  include/hw/clock.h       | 22 ++++++++++++++++------
>  hw/core/clock.c          |  8 +++++---
>  hw/misc/bcm2835_cprman.c |  2 +-
>  hw/misc/zynq_slcr.c      |  4 ++--
>  4 files changed, 24 insertions(+), 12 deletions(-)
>
> diff --git a/include/hw/clock.h b/include/hw/clock.h
> index bb12117f67..474bbc07fe 100644
> --- a/include/hw/clock.h
> +++ b/include/hw/clock.h
> @@ -180,21 +180,28 @@ static inline bool clock_has_source(const Clock *clk)
>   * clock_set:
>   * @clk: the clock to initialize.
>   * @value: the clock's value, 0 means unclocked
> + * @changed: set to true if the clock is changed, ignored if set to NULL.
>   *
>   * Set the local cached period value of @clk to @value.
> - *
> - * @return: true if the clock is changed.
>   */
> -bool clock_set(Clock *clk, uint64_t value);
> +void clock_set(Clock *clk, uint64_t period, bool *changed);

What's wrong with using the return value? Generally
returning a value via passing in a pointer is much
clunkier in C than using the return value, so we only
do it if we have to (e.g. the return value is already
being used for something else, or we need to return
more than one thing at once).

thanks
-- PMM

Re: [PATCH-for-9.0? v2 2/8] hw/clock: Pass optional &bool argument to clock_set()

[Philippe Mathieu-Daudé]

On 25/3/24 14:47, Peter Maydell wrote:
> On Mon, 25 Mar 2024 at 13:33, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Currently clock_set() returns whether the clock has
>> been changed or not. In order to combine this information
>> with other clock calls, pass an optional boolean and do
>> not return anything.  The single caller ignores the return
>> value, have it use NULL.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   include/hw/clock.h       | 22 ++++++++++++++++------
>>   hw/core/clock.c          |  8 +++++---
>>   hw/misc/bcm2835_cprman.c |  2 +-
>>   hw/misc/zynq_slcr.c      |  4 ++--
>>   4 files changed, 24 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/hw/clock.h b/include/hw/clock.h
>> index bb12117f67..474bbc07fe 100644
>> --- a/include/hw/clock.h
>> +++ b/include/hw/clock.h
>> @@ -180,21 +180,28 @@ static inline bool clock_has_source(const Clock *clk)
>>    * clock_set:
>>    * @clk: the clock to initialize.
>>    * @value: the clock's value, 0 means unclocked
>> + * @changed: set to true if the clock is changed, ignored if set to NULL.
>>    *
>>    * Set the local cached period value of @clk to @value.
>> - *
>> - * @return: true if the clock is changed.
>>    */
>> -bool clock_set(Clock *clk, uint64_t value);
>> +void clock_set(Clock *clk, uint64_t period, bool *changed);
> 
> What's wrong with using the return value? Generally
> returning a value via passing in a pointer is much
> clunkier in C than using the return value, so we only
> do it if we have to (e.g. the return value is already
> being used for something else, or we need to return
> more than one thing at once).

Then I'd rather remove (by inlining) the clock_update*() methods,
to have explicit calls to clock_propagate(), after multiple
clock_set*() calls. clock_update*() are used in 4 files:

$ git grep -l clock_update hw/misc/
hw/misc/bcm2835_cprman.c
hw/misc/npcm7xx_clk.c
hw/misc/npcm7xx_mft.c
hw/misc/stm32l4x5_rcc.c
$ git grep clock_update hw/misc/ | wc -l
       37

Regards,

Phil.

Re: [PATCH-for-9.0? v2 2/8] hw/clock: Pass optional &bool argument to clock_set()

[Peter Maydell]

On Mon, 25 Mar 2024 at 14:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 25/3/24 14:47, Peter Maydell wrote:
> > On Mon, 25 Mar 2024 at 13:33, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >>
> >> Currently clock_set() returns whether the clock has
> >> been changed or not. In order to combine this information
> >> with other clock calls, pass an optional boolean and do
> >> not return anything.  The single caller ignores the return
> >> value, have it use NULL.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >> ---
> >>   include/hw/clock.h       | 22 ++++++++++++++++------
> >>   hw/core/clock.c          |  8 +++++---
> >>   hw/misc/bcm2835_cprman.c |  2 +-
> >>   hw/misc/zynq_slcr.c      |  4 ++--
> >>   4 files changed, 24 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/include/hw/clock.h b/include/hw/clock.h
> >> index bb12117f67..474bbc07fe 100644
> >> --- a/include/hw/clock.h
> >> +++ b/include/hw/clock.h
> >> @@ -180,21 +180,28 @@ static inline bool clock_has_source(const Clock *clk)
> >>    * clock_set:
> >>    * @clk: the clock to initialize.
> >>    * @value: the clock's value, 0 means unclocked
> >> + * @changed: set to true if the clock is changed, ignored if set to NULL.
> >>    *
> >>    * Set the local cached period value of @clk to @value.
> >> - *
> >> - * @return: true if the clock is changed.
> >>    */
> >> -bool clock_set(Clock *clk, uint64_t value);
> >> +void clock_set(Clock *clk, uint64_t period, bool *changed);
> >
> > What's wrong with using the return value? Generally
> > returning a value via passing in a pointer is much
> > clunkier in C than using the return value, so we only
> > do it if we have to (e.g. the return value is already
> > being used for something else, or we need to return
> > more than one thing at once).
>
> Then I'd rather remove (by inlining) the clock_update*() methods,
> to have explicit calls to clock_propagate(), after multiple
> clock_set*() calls.

You mean, so that we handle "set the clock period" and
"set the mul/div" the same way, by just setting them and making
it always the caller's responsibility to call clock_propagate() ?

Would you keep the bool return for clock_set and clock_set_mul_div
to tell the caller whether a clock_propagate() call is needed ?

thanks
-- PMM

Re: [PATCH-for-9.0? v2 2/8] hw/clock: Pass optional &bool argument to clock_set()

[Philippe Mathieu-Daudé]

On 25/3/24 15:44, Peter Maydell wrote:
> On Mon, 25 Mar 2024 at 14:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> On 25/3/24 14:47, Peter Maydell wrote:
>>> On Mon, 25 Mar 2024 at 13:33, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>>>
>>>> Currently clock_set() returns whether the clock has
>>>> been changed or not. In order to combine this information
>>>> with other clock calls, pass an optional boolean and do
>>>> not return anything.  The single caller ignores the return
>>>> value, have it use NULL.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>> ---
>>>>    include/hw/clock.h       | 22 ++++++++++++++++------
>>>>    hw/core/clock.c          |  8 +++++---
>>>>    hw/misc/bcm2835_cprman.c |  2 +-
>>>>    hw/misc/zynq_slcr.c      |  4 ++--
>>>>    4 files changed, 24 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/include/hw/clock.h b/include/hw/clock.h
>>>> index bb12117f67..474bbc07fe 100644
>>>> --- a/include/hw/clock.h
>>>> +++ b/include/hw/clock.h
>>>> @@ -180,21 +180,28 @@ static inline bool clock_has_source(const Clock *clk)
>>>>     * clock_set:
>>>>     * @clk: the clock to initialize.
>>>>     * @value: the clock's value, 0 means unclocked
>>>> + * @changed: set to true if the clock is changed, ignored if set to NULL.
>>>>     *
>>>>     * Set the local cached period value of @clk to @value.
>>>> - *
>>>> - * @return: true if the clock is changed.
>>>>     */
>>>> -bool clock_set(Clock *clk, uint64_t value);
>>>> +void clock_set(Clock *clk, uint64_t period, bool *changed);
>>>
>>> What's wrong with using the return value? Generally
>>> returning a value via passing in a pointer is much
>>> clunkier in C than using the return value, so we only
>>> do it if we have to (e.g. the return value is already
>>> being used for something else, or we need to return
>>> more than one thing at once).
>>
>> Then I'd rather remove (by inlining) the clock_update*() methods,
>> to have explicit calls to clock_propagate(), after multiple
>> clock_set*() calls.
> 
> You mean, so that we handle "set the clock period" and
> "set the mul/div" the same way, by just setting them and making
> it always the caller's responsibility to call clock_propagate() ?

Yes, for consistency, to have the clock_set* family behaving
the same way.

> Would you keep the bool return for clock_set and clock_set_mul_div
> to tell the caller whether a clock_propagate() call is needed ?

Yes (sorry for not being clearer). The API change would be
less invasive, possibly acceptable during the freeze.

> 
> thanks
> -- PMM

Re: [PATCH-for-9.0? v2 2/8] hw/clock: Pass optional &bool argument to clock_set()

[Peter Maydell]

On Mon, 25 Mar 2024 at 15:01, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 25/3/24 15:44, Peter Maydell wrote:
> > On Mon, 25 Mar 2024 at 14:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >>
> >> On 25/3/24 14:47, Peter Maydell wrote:
> >>> On Mon, 25 Mar 2024 at 13:33, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >>>>
> >>>> Currently clock_set() returns whether the clock has
> >>>> been changed or not. In order to combine this information
> >>>> with other clock calls, pass an optional boolean and do
> >>>> not return anything.  The single caller ignores the return
> >>>> value, have it use NULL.
> >>>>
> >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >>>> ---
> >>>>    include/hw/clock.h       | 22 ++++++++++++++++------
> >>>>    hw/core/clock.c          |  8 +++++---
> >>>>    hw/misc/bcm2835_cprman.c |  2 +-
> >>>>    hw/misc/zynq_slcr.c      |  4 ++--
> >>>>    4 files changed, 24 insertions(+), 12 deletions(-)
> >>>>
> >>>> diff --git a/include/hw/clock.h b/include/hw/clock.h
> >>>> index bb12117f67..474bbc07fe 100644
> >>>> --- a/include/hw/clock.h
> >>>> +++ b/include/hw/clock.h
> >>>> @@ -180,21 +180,28 @@ static inline bool clock_has_source(const Clock *clk)
> >>>>     * clock_set:
> >>>>     * @clk: the clock to initialize.
> >>>>     * @value: the clock's value, 0 means unclocked
> >>>> + * @changed: set to true if the clock is changed, ignored if set to NULL.
> >>>>     *
> >>>>     * Set the local cached period value of @clk to @value.
> >>>> - *
> >>>> - * @return: true if the clock is changed.
> >>>>     */
> >>>> -bool clock_set(Clock *clk, uint64_t value);
> >>>> +void clock_set(Clock *clk, uint64_t period, bool *changed);
> >>>
> >>> What's wrong with using the return value? Generally
> >>> returning a value via passing in a pointer is much
> >>> clunkier in C than using the return value, so we only
> >>> do it if we have to (e.g. the return value is already
> >>> being used for something else, or we need to return
> >>> more than one thing at once).
> >>
> >> Then I'd rather remove (by inlining) the clock_update*() methods,
> >> to have explicit calls to clock_propagate(), after multiple
> >> clock_set*() calls.
> >
> > You mean, so that we handle "set the clock period" and
> > "set the mul/div" the same way, by just setting them and making
> > it always the caller's responsibility to call clock_propagate() ?
>
> Yes, for consistency, to have the clock_set* family behaving
> the same way.
>
> > Would you keep the bool return for clock_set and clock_set_mul_div
> > to tell the caller whether a clock_propagate() call is needed ?
>
> Yes (sorry for not being clearer). The API change would be
> less invasive, possibly acceptable during the freeze.

Sounds reasonable as an API to me. The other place we currently
do an implicit clock_propagate() is from clock_set_source().
Should we make that require explicit propagate too?

For freeze: is there a way to fix this bug without changing all the
clock APIs first?

thanks
-- PMM

Re: [PATCH-for-9.0? v2 2/8] hw/clock: Pass optional &bool argument to clock_set()

[Philippe Mathieu-Daudé]

On 25/3/24 16:03, Peter Maydell wrote:
> On Mon, 25 Mar 2024 at 15:01, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> On 25/3/24 15:44, Peter Maydell wrote:
>>> On Mon, 25 Mar 2024 at 14:39, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>>>
>>>> On 25/3/24 14:47, Peter Maydell wrote:
>>>>> On Mon, 25 Mar 2024 at 13:33, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>>>>>
>>>>>> Currently clock_set() returns whether the clock has
>>>>>> been changed or not. In order to combine this information
>>>>>> with other clock calls, pass an optional boolean and do
>>>>>> not return anything.  The single caller ignores the return
>>>>>> value, have it use NULL.
>>>>>>
>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>> ---
>>>>>>     include/hw/clock.h       | 22 ++++++++++++++++------
>>>>>>     hw/core/clock.c          |  8 +++++---
>>>>>>     hw/misc/bcm2835_cprman.c |  2 +-
>>>>>>     hw/misc/zynq_slcr.c      |  4 ++--
>>>>>>     4 files changed, 24 insertions(+), 12 deletions(-)
>>>>>>
>>>>>> diff --git a/include/hw/clock.h b/include/hw/clock.h
>>>>>> index bb12117f67..474bbc07fe 100644
>>>>>> --- a/include/hw/clock.h
>>>>>> +++ b/include/hw/clock.h
>>>>>> @@ -180,21 +180,28 @@ static inline bool clock_has_source(const Clock *clk)
>>>>>>      * clock_set:
>>>>>>      * @clk: the clock to initialize.
>>>>>>      * @value: the clock's value, 0 means unclocked
>>>>>> + * @changed: set to true if the clock is changed, ignored if set to NULL.
>>>>>>      *
>>>>>>      * Set the local cached period value of @clk to @value.
>>>>>> - *
>>>>>> - * @return: true if the clock is changed.
>>>>>>      */
>>>>>> -bool clock_set(Clock *clk, uint64_t value);
>>>>>> +void clock_set(Clock *clk, uint64_t period, bool *changed);
>>>>>
>>>>> What's wrong with using the return value? Generally
>>>>> returning a value via passing in a pointer is much
>>>>> clunkier in C than using the return value, so we only
>>>>> do it if we have to (e.g. the return value is already
>>>>> being used for something else, or we need to return
>>>>> more than one thing at once).
>>>>
>>>> Then I'd rather remove (by inlining) the clock_update*() methods,
>>>> to have explicit calls to clock_propagate(), after multiple
>>>> clock_set*() calls.
>>>
>>> You mean, so that we handle "set the clock period" and
>>> "set the mul/div" the same way, by just setting them and making
>>> it always the caller's responsibility to call clock_propagate() ?
>>
>> Yes, for consistency, to have the clock_set* family behaving
>> the same way.
>>
>>> Would you keep the bool return for clock_set and clock_set_mul_div
>>> to tell the caller whether a clock_propagate() call is needed ?
>>
>> Yes (sorry for not being clearer). The API change would be
>> less invasive, possibly acceptable during the freeze.
> 
> Sounds reasonable as an API to me. The other place we currently
> do an implicit clock_propagate() is from clock_set_source().
> Should we make that require explicit propagate too?

For API consistency, I'd rather do the same. Luc, any objection?

> For freeze: is there a way to fix this bug without changing all the
> clock APIs first?

Sure, I'll respin that for Arnaud.

> 
> thanks
> -- PMM

Re: [PATCH-for-9.0? v2 2/8] hw/clock: Pass optional &bool argument to clock_set()

[Philippe Mathieu-Daudé]

On 25/3/24 16:11, Philippe Mathieu-Daudé wrote:
> On 25/3/24 16:03, Peter Maydell wrote:
>> On Mon, 25 Mar 2024 at 15:01, Philippe Mathieu-Daudé 
>> <philmd@linaro.org> wrote:
>>>
>>> On 25/3/24 15:44, Peter Maydell wrote:
>>>> On Mon, 25 Mar 2024 at 14:39, Philippe Mathieu-Daudé 
>>>> <philmd@linaro.org> wrote:
>>>>>
>>>>> On 25/3/24 14:47, Peter Maydell wrote:
>>>>>> On Mon, 25 Mar 2024 at 13:33, Philippe Mathieu-Daudé 
>>>>>> <philmd@linaro.org> wrote:
>>>>>>>
>>>>>>> Currently clock_set() returns whether the clock has
>>>>>>> been changed or not. In order to combine this information
>>>>>>> with other clock calls, pass an optional boolean and do
>>>>>>> not return anything.  The single caller ignores the return
>>>>>>> value, have it use NULL.
>>>>>>>
>>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>>>>>> ---
>>>>>>>     include/hw/clock.h       | 22 ++++++++++++++++------
>>>>>>>     hw/core/clock.c          |  8 +++++---
>>>>>>>     hw/misc/bcm2835_cprman.c |  2 +-
>>>>>>>     hw/misc/zynq_slcr.c      |  4 ++--
>>>>>>>     4 files changed, 24 insertions(+), 12 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/hw/clock.h b/include/hw/clock.h
>>>>>>> index bb12117f67..474bbc07fe 100644
>>>>>>> --- a/include/hw/clock.h
>>>>>>> +++ b/include/hw/clock.h
>>>>>>> @@ -180,21 +180,28 @@ static inline bool clock_has_source(const 
>>>>>>> Clock *clk)
>>>>>>>      * clock_set:
>>>>>>>      * @clk: the clock to initialize.
>>>>>>>      * @value: the clock's value, 0 means unclocked
>>>>>>> + * @changed: set to true if the clock is changed, ignored if set 
>>>>>>> to NULL.
>>>>>>>      *
>>>>>>>      * Set the local cached period value of @clk to @value.
>>>>>>> - *
>>>>>>> - * @return: true if the clock is changed.
>>>>>>>      */
>>>>>>> -bool clock_set(Clock *clk, uint64_t value);
>>>>>>> +void clock_set(Clock *clk, uint64_t period, bool *changed);
>>>>>>
>>>>>> What's wrong with using the return value? Generally
>>>>>> returning a value via passing in a pointer is much
>>>>>> clunkier in C than using the return value, so we only
>>>>>> do it if we have to (e.g. the return value is already
>>>>>> being used for something else, or we need to return
>>>>>> more than one thing at once).
>>>>>
>>>>> Then I'd rather remove (by inlining) the clock_update*() methods,
>>>>> to have explicit calls to clock_propagate(), after multiple
>>>>> clock_set*() calls.
>>>>
>>>> You mean, so that we handle "set the clock period" and
>>>> "set the mul/div" the same way, by just setting them and making
>>>> it always the caller's responsibility to call clock_propagate() ?
>>>
>>> Yes, for consistency, to have the clock_set* family behaving
>>> the same way.
>>>
>>>> Would you keep the bool return for clock_set and clock_set_mul_div
>>>> to tell the caller whether a clock_propagate() call is needed ?
>>>
>>> Yes (sorry for not being clearer). The API change would be
>>> less invasive, possibly acceptable during the freeze.
>>
>> Sounds reasonable as an API to me. The other place we currently
>> do an implicit clock_propagate() is from clock_set_source().
>> Should we make that require explicit propagate too?
> 
> For API consistency, I'd rather do the same. Luc, any objection?

Currently changing clock in clock_set_source() is not supported,
so we can only call this method once (usually before DEVICE_REALIZED).

We might never implement such feature, but again, I'd rather modify
it for API consistency.

>> For freeze: is there a way to fix this bug without changing all the
>> clock APIs first?
> 
> Sure, I'll respin that for Arnaud.
> 
>>
>> thanks
>> -- PMM
> 

[PATCH-for-9.0? v2 3/8] hw/clock: Pass optional &bool argument to clock_set_ns()

[Philippe Mathieu-Daudé]

Pass optional &bool argument to clock_set_ns().
Since all callers ignore the return value, have
them use NULL.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 docs/devel/clocks.rst | 2 +-
 include/hw/clock.h    | 8 ++------
 hw/arm/stellaris.c    | 2 +-
 3 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/docs/devel/clocks.rst b/docs/devel/clocks.rst
index c4d14bde04..3360e5616d 100644
--- a/docs/devel/clocks.rst
+++ b/docs/devel/clocks.rst
@@ -298,7 +298,7 @@ object during device instance init. For example:
     clk = qdev_init_clock_in(DEVICE(dev), "clk-in", clk_in_callback,
                              dev, ClockUpdate);
     /* set initial value to 10ns / 100MHz */
-    clock_set_ns(clk, 10);
+    clock_set_ns(clk, 10, NULL);
 
 To enforce that the clock is wired up by the board code, you can
 call ``clock_has_source()`` in your device's realize method:
diff --git a/include/hw/clock.h b/include/hw/clock.h
index 474bbc07fe..94ed5ba6e6 100644
--- a/include/hw/clock.h
+++ b/include/hw/clock.h
@@ -195,13 +195,9 @@ static inline bool clock_set_hz(Clock *clk, unsigned hz)
     return changed;
 }
 
-static inline bool clock_set_ns(Clock *clk, unsigned ns)
+static inline void clock_set_ns(Clock *clk, unsigned ns, bool *changed)
 {
-    bool changed = false;
-
-    clock_set(clk, CLOCK_PERIOD_FROM_NS(ns), &changed);
-
-    return changed;
+    clock_set(clk, CLOCK_PERIOD_FROM_NS(ns), changed);
 }
 
 /**
diff --git a/hw/arm/stellaris.c b/hw/arm/stellaris.c
index a2f998bf9e..d4381beeb7 100644
--- a/hw/arm/stellaris.c
+++ b/hw/arm/stellaris.c
@@ -279,7 +279,7 @@ static void ssys_calculate_system_clock(ssys_state *s, bool propagate_clock)
     } else {
         period_ns = 5 * (((s->rcc >> 23) & 0xf) + 1);
     }
-    clock_set_ns(s->sysclk, period_ns);
+    clock_set_ns(s->sysclk, period_ns, NULL);
     if (propagate_clock) {
         clock_propagate(s->sysclk);
     }
-- 
2.41.0

[PATCH-for-9.0? v2 4/8] hw/clock: Pass optional &bool argument to clock_set_hz()

[Philippe Mathieu-Daudé]

Pass optional &bool argument to clock_set_ns().
Since all callers ignore the return value, have
them use NULL.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/hw/clock.h         | 8 ++------
 hw/arm/aspeed.c            | 2 +-
 hw/arm/fby35.c             | 2 +-
 hw/arm/mps2-tz.c           | 4 ++--
 hw/arm/mps2.c              | 4 ++--
 hw/arm/mps3r.c             | 2 +-
 hw/arm/msf2-som.c          | 2 +-
 hw/arm/musca.c             | 4 ++--
 hw/arm/netduino2.c         | 2 +-
 hw/arm/netduinoplus2.c     | 2 +-
 hw/arm/nrf51_soc.c         | 2 +-
 hw/arm/olimex-stm32-h405.c | 2 +-
 hw/arm/stm32vldiscovery.c  | 2 +-
 hw/arm/xilinx_zynq.c       | 2 +-
 hw/char/cadence_uart.c     | 4 ++--
 15 files changed, 20 insertions(+), 24 deletions(-)

diff --git a/include/hw/clock.h b/include/hw/clock.h
index 94ed5ba6e6..f0ac410fc8 100644
--- a/include/hw/clock.h
+++ b/include/hw/clock.h
@@ -186,13 +186,9 @@ static inline bool clock_has_source(const Clock *clk)
  */
 void clock_set(Clock *clk, uint64_t period, bool *changed);
 
-static inline bool clock_set_hz(Clock *clk, unsigned hz)
+static inline void clock_set_hz(Clock *clk, unsigned hz, bool *changed)
 {
-    bool changed = false;
-
-    clock_set(clk, CLOCK_PERIOD_FROM_HZ(hz), &changed);
-
-    return changed;
+    clock_set(clk, CLOCK_PERIOD_FROM_HZ(hz), changed);
 }
 
 static inline void clock_set_ns(Clock *clk, unsigned ns, bool *changed)
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 8854581ca8..7e4ed71e9f 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -1521,7 +1521,7 @@ static void aspeed_minibmc_machine_init(MachineState *machine)
     Clock *sysclk;
 
     sysclk = clock_new(OBJECT(machine), "SYSCLK");
-    clock_set_hz(sysclk, SYSCLK_FRQ);
+    clock_set_hz(sysclk, SYSCLK_FRQ, NULL);
 
     bmc->soc = ASPEED_SOC(object_new(amc->soc_name));
     object_property_add_child(OBJECT(machine), "soc", OBJECT(bmc->soc));
diff --git a/hw/arm/fby35.c b/hw/arm/fby35.c
index c9964bd283..dd4abdd768 100644
--- a/hw/arm/fby35.c
+++ b/hw/arm/fby35.c
@@ -119,7 +119,7 @@ static void fby35_bic_init(Fby35State *s)
     AspeedSoCState *soc;
 
     s->bic_sysclk = clock_new(OBJECT(s), "SYSCLK");
-    clock_set_hz(s->bic_sysclk, 200000000ULL);
+    clock_set_hz(s->bic_sysclk, 200000000ULL, NULL);
 
     object_initialize_child(OBJECT(s), "bic", &s->bic, "ast1030-a1");
     soc = ASPEED_SOC(&s->bic);
diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
index a2d18afd79..015db74ab8 100644
--- a/hw/arm/mps2-tz.c
+++ b/hw/arm/mps2-tz.c
@@ -818,9 +818,9 @@ static void mps2tz_common_init(MachineState *machine)
 
     /* These clocks don't need migration because they are fixed-frequency */
     mms->sysclk = clock_new(OBJECT(machine), "SYSCLK");
-    clock_set_hz(mms->sysclk, mmc->sysclk_frq);
+    clock_set_hz(mms->sysclk, mmc->sysclk_frq, NULL);
     mms->s32kclk = clock_new(OBJECT(machine), "S32KCLK");
-    clock_set_hz(mms->s32kclk, S32KCLK_FRQ);
+    clock_set_hz(mms->s32kclk, S32KCLK_FRQ, NULL);
 
     object_initialize_child(OBJECT(machine), TYPE_IOTKIT, &mms->iotkit,
                             mmc->armsse_type);
diff --git a/hw/arm/mps2.c b/hw/arm/mps2.c
index 50919ee46d..7176305239 100644
--- a/hw/arm/mps2.c
+++ b/hw/arm/mps2.c
@@ -151,10 +151,10 @@ static void mps2_common_init(MachineState *machine)
 
     /* This clock doesn't need migration because it is fixed-frequency */
     mms->sysclk = clock_new(OBJECT(machine), "SYSCLK");
-    clock_set_hz(mms->sysclk, SYSCLK_FRQ);
+    clock_set_hz(mms->sysclk, SYSCLK_FRQ, NULL);
 
     mms->refclk = clock_new(OBJECT(machine), "REFCLK");
-    clock_set_hz(mms->refclk, REFCLK_FRQ);
+    clock_set_hz(mms->refclk, REFCLK_FRQ, NULL);
 
     /* The FPGA images have an odd combination of different RAMs,
      * because in hardware they are different implementations and
diff --git a/hw/arm/mps3r.c b/hw/arm/mps3r.c
index 4d55a6564c..84f1f9d3a2 100644
--- a/hw/arm/mps3r.c
+++ b/hw/arm/mps3r.c
@@ -360,7 +360,7 @@ static void mps3r_common_init(MachineState *machine)
     QList *oscclk;
 
     mms->clk = clock_new(OBJECT(machine), "CLK");
-    clock_set_hz(mms->clk, CLK_FRQ);
+    clock_set_hz(mms->clk, CLK_FRQ, NULL);
 
     for (const RAMInfo *ri = mmc->raminfo; ri->name; ri++) {
         MemoryRegion *mr = mr_for_raminfo(mms, ri);
diff --git a/hw/arm/msf2-som.c b/hw/arm/msf2-som.c
index 5c415abe85..420c0ae332 100644
--- a/hw/arm/msf2-som.c
+++ b/hw/arm/msf2-som.c
@@ -71,7 +71,7 @@ static void emcraft_sf2_s2s010_init(MachineState *machine)
      */
     /* This clock doesn't need migration because it is fixed-frequency */
     m3clk = clock_new(OBJECT(machine), "m3clk");
-    clock_set_hz(m3clk, 142 * 1000000);
+    clock_set_hz(m3clk, 142 * 1000000, NULL);
     qdev_connect_clock_in(dev, "m3clk", m3clk);
     qdev_prop_set_uint32(dev, "apb0div", 2);
     qdev_prop_set_uint32(dev, "apb1div", 2);
diff --git a/hw/arm/musca.c b/hw/arm/musca.c
index e2c9d49af5..6fdcb76dfa 100644
--- a/hw/arm/musca.c
+++ b/hw/arm/musca.c
@@ -366,9 +366,9 @@ static void musca_init(MachineState *machine)
     assert(mmc->num_mpcs <= MUSCA_MPC_MAX);
 
     mms->sysclk = clock_new(OBJECT(machine), "SYSCLK");
-    clock_set_hz(mms->sysclk, SYSCLK_FRQ);
+    clock_set_hz(mms->sysclk, SYSCLK_FRQ, NULL);
     mms->s32kclk = clock_new(OBJECT(machine), "S32KCLK");
-    clock_set_hz(mms->s32kclk, S32KCLK_FRQ);
+    clock_set_hz(mms->s32kclk, S32KCLK_FRQ, NULL);
 
     object_initialize_child(OBJECT(machine), "sse-200", &mms->sse,
                             TYPE_SSE200);
diff --git a/hw/arm/netduino2.c b/hw/arm/netduino2.c
index 8b1a9a2437..49a3ee2c47 100644
--- a/hw/arm/netduino2.c
+++ b/hw/arm/netduino2.c
@@ -41,7 +41,7 @@ static void netduino2_init(MachineState *machine)
 
     /* This clock doesn't need migration because it is fixed-frequency */
     sysclk = clock_new(OBJECT(machine), "SYSCLK");
-    clock_set_hz(sysclk, SYSCLK_FRQ);
+    clock_set_hz(sysclk, SYSCLK_FRQ, NULL);
 
     dev = qdev_new(TYPE_STM32F205_SOC);
     object_property_add_child(OBJECT(machine), "soc", OBJECT(dev));
diff --git a/hw/arm/netduinoplus2.c b/hw/arm/netduinoplus2.c
index bccd100354..30d9c0410d 100644
--- a/hw/arm/netduinoplus2.c
+++ b/hw/arm/netduinoplus2.c
@@ -41,7 +41,7 @@ static void netduinoplus2_init(MachineState *machine)
 
     /* This clock doesn't need migration because it is fixed-frequency */
     sysclk = clock_new(OBJECT(machine), "SYSCLK");
-    clock_set_hz(sysclk, SYSCLK_FRQ);
+    clock_set_hz(sysclk, SYSCLK_FRQ, NULL);
 
     dev = qdev_new(TYPE_STM32F405_SOC);
     object_property_add_child(OBJECT(machine), "soc", OBJECT(dev));
diff --git a/hw/arm/nrf51_soc.c b/hw/arm/nrf51_soc.c
index ac53441630..9aebb98934 100644
--- a/hw/arm/nrf51_soc.c
+++ b/hw/arm/nrf51_soc.c
@@ -75,7 +75,7 @@ static void nrf51_soc_realize(DeviceState *dev_soc, Error **errp)
         return;
     }
     /* This clock doesn't need migration because it is fixed-frequency */
-    clock_set_hz(s->sysclk, HCLK_FRQ);
+    clock_set_hz(s->sysclk, HCLK_FRQ, NULL);
     qdev_connect_clock_in(DEVICE(&s->cpu), "cpuclk", s->sysclk);
     /*
      * This SoC has no systick device, so don't connect refclk.
diff --git a/hw/arm/olimex-stm32-h405.c b/hw/arm/olimex-stm32-h405.c
index 4ad7b043be..394875608a 100644
--- a/hw/arm/olimex-stm32-h405.c
+++ b/hw/arm/olimex-stm32-h405.c
@@ -44,7 +44,7 @@ static void olimex_stm32_h405_init(MachineState *machine)
 
     /* This clock doesn't need migration because it is fixed-frequency */
     sysclk = clock_new(OBJECT(machine), "SYSCLK");
-    clock_set_hz(sysclk, SYSCLK_FRQ);
+    clock_set_hz(sysclk, SYSCLK_FRQ, NULL);
 
     dev = qdev_new(TYPE_STM32F405_SOC);
     object_property_add_child(OBJECT(machine), "soc", OBJECT(dev));
diff --git a/hw/arm/stm32vldiscovery.c b/hw/arm/stm32vldiscovery.c
index cc41935160..4be626e878 100644
--- a/hw/arm/stm32vldiscovery.c
+++ b/hw/arm/stm32vldiscovery.c
@@ -44,7 +44,7 @@ static void stm32vldiscovery_init(MachineState *machine)
 
     /* This clock doesn't need migration because it is fixed-frequency */
     sysclk = clock_new(OBJECT(machine), "SYSCLK");
-    clock_set_hz(sysclk, SYSCLK_FRQ);
+    clock_set_hz(sysclk, SYSCLK_FRQ, NULL);
 
     dev = qdev_new(TYPE_STM32F100_SOC);
     object_property_add_child(OBJECT(machine), "soc", OBJECT(dev));
diff --git a/hw/arm/xilinx_zynq.c b/hw/arm/xilinx_zynq.c
index fc3abcbe88..1fd13caa58 100644
--- a/hw/arm/xilinx_zynq.c
+++ b/hw/arm/xilinx_zynq.c
@@ -228,7 +228,7 @@ static void zynq_init(MachineState *machine)
     object_property_add_child(OBJECT(zynq_machine), "ps_clk",
                               OBJECT(zynq_machine->ps_clk));
     object_unref(OBJECT(zynq_machine->ps_clk));
-    clock_set_hz(zynq_machine->ps_clk, PS_CLK_FREQUENCY);
+    clock_set_hz(zynq_machine->ps_clk, PS_CLK_FREQUENCY, NULL);
 
     /* Create slcr, keep a pointer to connect clocks */
     slcr = qdev_new("xilinx-zynq_slcr");
diff --git a/hw/char/cadence_uart.c b/hw/char/cadence_uart.c
index db31d7cc85..4619209af4 100644
--- a/hw/char/cadence_uart.c
+++ b/hw/char/cadence_uart.c
@@ -566,7 +566,7 @@ static void cadence_uart_init(Object *obj)
     s->refclk = qdev_init_clock_in(DEVICE(obj), "refclk",
                                    cadence_uart_refclk_update, s, ClockUpdate);
     /* initialize the frequency in case the clock remains unconnected */
-    clock_set_hz(s->refclk, UART_DEFAULT_REF_CLK);
+    clock_set_hz(s->refclk, UART_DEFAULT_REF_CLK, NULL);
 
     s->char_tx_time = (NANOSECONDS_PER_SECOND / 9600) * 10;
 }
@@ -576,7 +576,7 @@ static int cadence_uart_pre_load(void *opaque)
     CadenceUARTState *s = opaque;
 
     /* the frequency will be overridden if the refclk field is present */
-    clock_set_hz(s->refclk, UART_DEFAULT_REF_CLK);
+    clock_set_hz(s->refclk, UART_DEFAULT_REF_CLK, NULL);
     return 0;
 }
 
-- 
2.41.0

[PATCH-for-9.0? v2 5/8] hw/clock: Pass optional &bool argument to clock_set_mul_div()

[Philippe Mathieu-Daudé]

Pass optional &bool argument to clock_set_ns().
Since all callers ignore the return value, have
them use NULL.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 docs/devel/clocks.rst   | 4 ++++
 include/hw/clock.h      | 3 ++-
 hw/arm/msf2-soc.c       | 2 +-
 hw/arm/stm32f100_soc.c  | 2 +-
 hw/arm/stm32f205_soc.c  | 2 +-
 hw/arm/stm32f405_soc.c  | 2 +-
 hw/core/clock.c         | 7 ++++++-
 hw/misc/stm32l4x5_rcc.c | 2 +-
 8 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/docs/devel/clocks.rst b/docs/devel/clocks.rst
index 3360e5616d..027a3c5dbc 100644
--- a/docs/devel/clocks.rst
+++ b/docs/devel/clocks.rst
@@ -279,6 +279,10 @@ You can change the multiplier and divider of a clock at runtime,
 so you can use this to model clock controller devices which
 have guest-programmable frequency multipliers or dividers.
 
+Similary to ``clock_set()``, ``clock_set_mul_div()`` takes an optional
+boolean pointer which is set to ``true`` if the clock state was modified,
+that it, if the multiplier or the diviser or both were changed by the call.
+
 Note that ``clock_set_mul_div()`` does not automatically call
 ``clock_propagate()``. If you make a runtime change to the
 multiplier or divider you must call clock_propagate() yourself.
diff --git a/include/hw/clock.h b/include/hw/clock.h
index f0ac410fc8..0e4c5b67a2 100644
--- a/include/hw/clock.h
+++ b/include/hw/clock.h
@@ -376,6 +376,7 @@ char *clock_display_freq(Clock *clk);
  * Note that this function does not call clock_propagate(); the
  * caller should do that if necessary.
  */
-void clock_set_mul_div(Clock *clk, uint32_t multiplier, uint32_t divider);
+void clock_set_mul_div(Clock *clk, uint32_t multiplier, uint32_t divider,
+                       bool *changed);
 
 #endif /* QEMU_HW_CLOCK_H */
diff --git a/hw/arm/msf2-soc.c b/hw/arm/msf2-soc.c
index a94a10adcc..7257bd5ded 100644
--- a/hw/arm/msf2-soc.c
+++ b/hw/arm/msf2-soc.c
@@ -111,7 +111,7 @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp)
      * implement the divisor as a fixed /32, which matches the reset value
      * of SYSTICK_CR.
      */
-    clock_set_mul_div(s->refclk, 32, 1);
+    clock_set_mul_div(s->refclk, 32, 1, NULL);
     clock_set_source(s->refclk, s->m3clk);
 
     memory_region_init_rom(&s->nvm, OBJECT(dev_soc), "MSF2.eNVM", s->envm_size,
diff --git a/hw/arm/stm32f100_soc.c b/hw/arm/stm32f100_soc.c
index 808b783515..4879bd6188 100644
--- a/hw/arm/stm32f100_soc.c
+++ b/hw/arm/stm32f100_soc.c
@@ -93,7 +93,7 @@ static void stm32f100_soc_realize(DeviceState *dev_soc, Error **errp)
      */
 
     /* The refclk always runs at frequency HCLK / 8 */
-    clock_set_mul_div(s->refclk, 8, 1);
+    clock_set_mul_div(s->refclk, 8, 1, NULL);
     clock_set_source(s->refclk, s->sysclk);
 
     /*
diff --git a/hw/arm/stm32f205_soc.c b/hw/arm/stm32f205_soc.c
index a451e21f59..aeb0cb0a5a 100644
--- a/hw/arm/stm32f205_soc.c
+++ b/hw/arm/stm32f205_soc.c
@@ -110,7 +110,7 @@ static void stm32f205_soc_realize(DeviceState *dev_soc, Error **errp)
      */
 
     /* The refclk always runs at frequency HCLK / 8 */
-    clock_set_mul_div(s->refclk, 8, 1);
+    clock_set_mul_div(s->refclk, 8, 1, NULL);
     clock_set_source(s->refclk, s->sysclk);
 
     memory_region_init_rom(&s->flash, OBJECT(dev_soc), "STM32F205.flash",
diff --git a/hw/arm/stm32f405_soc.c b/hw/arm/stm32f405_soc.c
index 2ad5b79a06..07cf0e8287 100644
--- a/hw/arm/stm32f405_soc.c
+++ b/hw/arm/stm32f405_soc.c
@@ -115,7 +115,7 @@ static void stm32f405_soc_realize(DeviceState *dev_soc, Error **errp)
      */
 
     /* The refclk always runs at frequency HCLK / 8 */
-    clock_set_mul_div(s->refclk, 8, 1);
+    clock_set_mul_div(s->refclk, 8, 1, NULL);
     clock_set_source(s->refclk, s->sysclk);
 
     memory_region_init_rom(&s->flash, OBJECT(dev_soc), "STM32F405.flash",
diff --git a/hw/core/clock.c b/hw/core/clock.c
index e0f257b141..6ef60a2423 100644
--- a/hw/core/clock.c
+++ b/hw/core/clock.c
@@ -145,7 +145,8 @@ char *clock_display_freq(Clock *clk)
     return freq_to_str(clock_get_hz(clk));
 }
 
-void clock_set_mul_div(Clock *clk, uint32_t multiplier, uint32_t divider)
+void clock_set_mul_div(Clock *clk, uint32_t multiplier, uint32_t divider,
+                       bool *changed)
 {
     assert(divider != 0);
 
@@ -157,6 +158,10 @@ void clock_set_mul_div(Clock *clk, uint32_t multiplier, uint32_t divider)
                             clk->divider, divider);
     clk->multiplier = multiplier;
     clk->divider = divider;
+
+    if (changed) {
+        *changed = true;
+    }
 }
 
 static void clock_initfn(Object *obj)
diff --git a/hw/misc/stm32l4x5_rcc.c b/hw/misc/stm32l4x5_rcc.c
index bc2d63528b..f01113308a 100644
--- a/hw/misc/stm32l4x5_rcc.c
+++ b/hw/misc/stm32l4x5_rcc.c
@@ -59,7 +59,7 @@ static void clock_mux_update(RccClockMuxState *mux, bool bypass_source)
         freq_multiplier = mux->divider;
     }
 
-    clock_set_mul_div(mux->out, freq_multiplier, mux->multiplier);
+    clock_set_mul_div(mux->out, freq_multiplier, mux->multiplier, NULL);
     clock_update(mux->out, clock_get(current_source));
 
     src_freq = clock_get_hz(current_source);
-- 
2.41.0

[PATCH-for-9.0 v2 6/8] hw/misc/stm32l4x5_rcc: Inline clock_update() in clock_mux_update()

[Philippe Mathieu-Daudé]

Trivial inlining in preliminary patch to make the next
one easier to review.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/misc/stm32l4x5_rcc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/misc/stm32l4x5_rcc.c b/hw/misc/stm32l4x5_rcc.c
index f01113308a..8852b434db 100644
--- a/hw/misc/stm32l4x5_rcc.c
+++ b/hw/misc/stm32l4x5_rcc.c
@@ -48,6 +48,8 @@ static void clock_mux_update(RccClockMuxState *mux, bool bypass_source)
     uint64_t src_freq;
     Clock *current_source = mux->srcs[mux->src];
     uint32_t freq_multiplier = 0;
+    bool clk_changed = false;
+
     /*
      * To avoid rounding errors, we use the clock period instead of the
      * frequency.
@@ -60,7 +62,10 @@ static void clock_mux_update(RccClockMuxState *mux, bool bypass_source)
     }
 
     clock_set_mul_div(mux->out, freq_multiplier, mux->multiplier, NULL);
-    clock_update(mux->out, clock_get(current_source));
+    clock_set(mux->out, clock_get(current_source), &clk_changed);
+    if (clk_changed) {
+        clock_propagate(mux->out);
+    }
 
     src_freq = clock_get_hz(current_source);
     /* TODO: can we simply detect if the config changed so that we reduce log spam ? */
-- 
2.41.0

[PATCH-for-9.0? v2 7/8] hw/misc/stm32l4x5_rcc: Propagate period when enabling a clock

[Philippe Mathieu-Daudé]

From: Arnaud Minier <arnaud.minier@telecom-paris.fr>

The "clock_set_mul_div" function doesn't propagate the clock period
to the children if it is changed (e.g. by enabling/disabling a clock
multiplexer).
This was overlooked during the implementation due to late changes.

This commit propagates the change if the multiplier or divider changes.

Fixes: ec7d83acbd ("hw/misc/stm32l4x5_rcc: Add an internal clock multiplexer object")
Signed-off-by: Arnaud Minier <arnaud.minier@telecom-paris.fr>
Signed-off-by: Inès Varhol <ines.varhol@telecom-paris.fr>
Message-ID: <20240317103918.44375-2-arnaud.minier@telecom-paris.fr>
[PMD: Pass argument to clock_set_mul_div()]
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/misc/stm32l4x5_rcc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/misc/stm32l4x5_rcc.c b/hw/misc/stm32l4x5_rcc.c
index 8852b434db..166c727e52 100644
--- a/hw/misc/stm32l4x5_rcc.c
+++ b/hw/misc/stm32l4x5_rcc.c
@@ -61,7 +61,7 @@ static void clock_mux_update(RccClockMuxState *mux, bool bypass_source)
         freq_multiplier = mux->divider;
     }
 
-    clock_set_mul_div(mux->out, freq_multiplier, mux->multiplier, NULL);
+    clock_set_mul_div(mux->out, freq_multiplier, mux->multiplier, &clk_changed);
     clock_set(mux->out, clock_get(current_source), &clk_changed);
     if (clk_changed) {
         clock_propagate(mux->out);
-- 
2.41.0

[PATCH-for-9.1 v2 8/8] hw/misc/zynq_slcr: Only propagate clock changes when necessary

[Philippe Mathieu-Daudé]

Pass &bool to zynq_slcr_compute_clocks[_internal](), so we
can pass it to the clock_set() calls which might update it.
Then check it and only call zynq_slcr_propagate_clocks()
and clock_propagate() when necessary.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/misc/zynq_slcr.c | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
index e637798507..ad9c575137 100644
--- a/hw/misc/zynq_slcr.c
+++ b/hw/misc/zynq_slcr.c
@@ -269,7 +269,8 @@ static uint64_t zynq_slcr_compute_clock(const uint64_t periods[],
     zynq_slcr_compute_clock((plls), (state)->regs[reg], \
                             reg ## _ ## enable_field ## _SHIFT)
 
-static void zynq_slcr_compute_clocks_internal(ZynqSLCRState *s, uint64_t ps_clk)
+static void zynq_slcr_compute_clocks_internal(ZynqSLCRState *s, uint64_t ps_clk,
+                                              bool *changed)
 {
     uint64_t io_pll = zynq_slcr_compute_pll(ps_clk, s->regs[R_IO_PLL_CTRL]);
     uint64_t arm_pll = zynq_slcr_compute_pll(ps_clk, s->regs[R_ARM_PLL_CTRL]);
@@ -279,9 +280,9 @@ static void zynq_slcr_compute_clocks_internal(ZynqSLCRState *s, uint64_t ps_clk)
 
     /* compute uartX reference clocks */
     clock_set(s->uart0_ref_clk,
-              ZYNQ_COMPUTE_CLK(s, uart_mux, R_UART_CLK_CTRL, CLKACT0), NULL);
+              ZYNQ_COMPUTE_CLK(s, uart_mux, R_UART_CLK_CTRL, CLKACT0), changed);
     clock_set(s->uart1_ref_clk,
-              ZYNQ_COMPUTE_CLK(s, uart_mux, R_UART_CLK_CTRL, CLKACT1), NULL);
+              ZYNQ_COMPUTE_CLK(s, uart_mux, R_UART_CLK_CTRL, CLKACT1), changed);
 }
 
 /**
@@ -289,7 +290,7 @@ static void zynq_slcr_compute_clocks_internal(ZynqSLCRState *s, uint64_t ps_clk)
  * But do not propagate them further. Connected clocks
  * will not receive any updates (See zynq_slcr_compute_clocks())
  */
-static void zynq_slcr_compute_clocks(ZynqSLCRState *s)
+static void zynq_slcr_compute_clocks(ZynqSLCRState *s, bool *changed)
 {
     uint64_t ps_clk = clock_get(s->ps_clk);
 
@@ -298,7 +299,7 @@ static void zynq_slcr_compute_clocks(ZynqSLCRState *s)
         ps_clk = 0;
     }
 
-    zynq_slcr_compute_clocks_internal(s, ps_clk);
+    zynq_slcr_compute_clocks_internal(s, ps_clk, changed);
 }
 
 /**
@@ -315,9 +316,12 @@ static void zynq_slcr_propagate_clocks(ZynqSLCRState *s)
 static void zynq_slcr_ps_clk_callback(void *opaque, ClockEvent event)
 {
     ZynqSLCRState *s = (ZynqSLCRState *) opaque;
+    bool propagate = false;
 
-    zynq_slcr_compute_clocks(s);
-    zynq_slcr_propagate_clocks(s);
+    zynq_slcr_compute_clocks(s, &propagate);
+    if (propagate) {
+        zynq_slcr_propagate_clocks(s);
+    }
 }
 
 static void zynq_slcr_reset_init(Object *obj, ResetType type)
@@ -419,19 +423,25 @@ static void zynq_slcr_reset_init(Object *obj, ResetType type)
 static void zynq_slcr_reset_hold(Object *obj)
 {
     ZynqSLCRState *s = ZYNQ_SLCR(obj);
+    bool propagate = false;
 
     /* will disable all output clocks */
-    zynq_slcr_compute_clocks_internal(s, 0);
-    zynq_slcr_propagate_clocks(s);
+    zynq_slcr_compute_clocks_internal(s, 0, &propagate);
+    if (propagate) {
+        zynq_slcr_propagate_clocks(s);
+    }
 }
 
 static void zynq_slcr_reset_exit(Object *obj)
 {
     ZynqSLCRState *s = ZYNQ_SLCR(obj);
+    bool propagate = false;
 
     /* will compute output clocks according to ps_clk and registers */
-    zynq_slcr_compute_clocks_internal(s, clock_get(s->ps_clk));
-    zynq_slcr_propagate_clocks(s);
+    zynq_slcr_compute_clocks_internal(s, clock_get(s->ps_clk), &propagate);
+    if (propagate) {
+        zynq_slcr_propagate_clocks(s);
+    }
 }
 
 static bool zynq_slcr_check_offset(hwaddr offset, bool rnw)
@@ -516,6 +526,7 @@ static void zynq_slcr_write(void *opaque, hwaddr offset,
                           uint64_t val, unsigned size)
 {
     ZynqSLCRState *s = (ZynqSLCRState *)opaque;
+    bool propagate = false;
     offset /= 4;
 
     DB_PRINT("addr: %08" HWADDR_PRIx " data: %08" PRIx64 "\n", offset * 4, val);
@@ -569,8 +580,10 @@ static void zynq_slcr_write(void *opaque, hwaddr offset,
     case R_ARM_PLL_CTRL:
     case R_DDR_PLL_CTRL:
     case R_UART_CLK_CTRL:
-        zynq_slcr_compute_clocks(s);
-        zynq_slcr_propagate_clocks(s);
+        zynq_slcr_compute_clocks(s, &propagate);
+        if (propagate) {
+            zynq_slcr_propagate_clocks(s);
+        }
         break;
     }
 }
-- 
2.41.0