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