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