| Message ID | 20240325133259.57235-3-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/clock: Propagate clock changes when STM32L4X5 MUX is updated | expand |
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
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.
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
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
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
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
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 >
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); } /**
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(-)