On Mon, 25 Mar 2024 at 15:01, Philippe Mathieu-Daudé <phi...@linaro.org> wrote: > > On 25/3/24 15:44, Peter Maydell wrote: > > On Mon, 25 Mar 2024 at 14:39, Philippe Mathieu-Daudé <phi...@linaro.org> > > wrote: > >> > >> On 25/3/24 14:47, Peter Maydell wrote: > >>> On Mon, 25 Mar 2024 at 13:33, Philippe Mathieu-Daudé <phi...@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é <phi...@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
