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

From: "Philippe Mathieu-Daudé" <philmd@linaro.org>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-devel@nongnu.org,
	"Inès Varhol" <ines.varhol@telecom-paris.fr>,
	"Arnaud Minier" <arnaud.minier@telecom-paris.fr>,
	"Damien Hedde" <damien.hedde@dahe.fr>,
	qemu-arm@nongnu.org, "Alistair Francis" <alistair@alistair23.me>,
	"Luc Michel" <luc@lmichel.fr>,
	"Edgar E. Iglesias" <edgar.iglesias@gmail.com>
Subject: Re: [PATCH-for-9.0? v2 2/8] hw/clock: Pass optional &bool argument to clock_set()
Date: Mon, 25 Mar 2024 16:11:26 +0100	[thread overview]
Message-ID: <1d42c033-333d-4fdf-b4b7-7dc19b831982@linaro.org> (raw)
In-Reply-To: <CAFEAcA8fMVVBxhH8NoPMcNarRYMZSFN1x5GWLg=KSxg1p_FK3Q@mail.gmail.com>

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