| Message ID | 20210812093356.1946-10-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm: Get rid of system_clock_scale global | expand |
On 8/12/21 11:33 AM, Peter Maydell wrote: > It is quite common for a clock tree to involve possibly programmable > clock multipliers or dividers, where the frequency of a clock is for > instance divided by 8 to produce a slower clock to feed to a > particular device. > > Currently we provide no convenient mechanism for modelling this. You > can implement it by having an input Clock and an output Clock, and > manually setting the period of the output clock in the period-changed > callback of the input clock, but that's quite clunky. > > This patch adds support in the Clock objects themselves for setting a > multiplier or divider. The effect of setting this on a clock is that > when the clock's period is changed, all the children of the clock are > set to period * multiplier / divider, rather than being set to the > same period as the parent clock. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > docs/devel/clocks.rst | 23 +++++++++++++++++++++++ > include/hw/clock.h | 29 +++++++++++++++++++++++++++++ > hw/core/clock-vmstate.c | 24 +++++++++++++++++++++++- > hw/core/clock.c | 29 +++++++++++++++++++++++++---- > 4 files changed, 100 insertions(+), 5 deletions(-) > > diff --git a/docs/devel/clocks.rst b/docs/devel/clocks.rst > index 956bd147ea0..430fbd842e5 100644 > --- a/docs/devel/clocks.rst > +++ b/docs/devel/clocks.rst > @@ -260,6 +260,29 @@ clocks get the new clock period value: *Clock 2*, *Clock 3* and *Clock 4*. > It is not possible to disconnect a clock or to change the clock connection > after it is connected. > > +Clock multiplier and divider settings > +------------------------------------- > + > +By default, when clocks are connected together, the child > +clocks run with the same period as their source (parent) clock. > +The Clock API supports a built-in period multiplier/divider > +mechanism so you can configure a clock to make its children > +run at a different period from its own. If you call the > +``clock_set_mul_div()`` function you can specify the clock's > +multiplier and divider values. The children of that clock > +will all run with a period of ``parent_period * multiplier / divider``. > +For instance, if the clock has a frequency of 8MHz and you set its > +multiplier to 2 and its divider to 3, the child clocks will run > +at 12MHz. > + > +You can change the multiplier and divider of a clock at runtime, > +so you can use this to model clock controller devices which > +have guest-programmable frequency multipliers or dividers. This looks nice! Does this imply that if I am going to implement the STM32 RCC (Reset and Clock Controller) device, then I should use this new feature? > + > +Note that ``clock_set_mul_div()`` does not automatically call > +``clock_propagate()``. If you make a runtime change to the > +multiplier or divider you must call clock_propagate() yourself.a > + > Unconnected input clocks > ------------------------ > > diff --git a/include/hw/clock.h b/include/hw/clock.h > index a7187eab95e..11f67fb9701 100644 > --- a/include/hw/clock.h > +++ b/include/hw/clock.h > @@ -81,6 +81,10 @@ struct Clock { > void *callback_opaque; > unsigned int callback_events; > > + /* Ratio of the parent clock to run the child clocks at */ > + uint32_t multiplier; > + uint32_t divider; > + > /* Clocks are organized in a clock tree */ > Clock *source; > QLIST_HEAD(, Clock) children; > @@ -350,4 +354,29 @@ static inline bool clock_is_enabled(const Clock *clk) > */ > char *clock_display_freq(Clock *clk); > > +/** > + * clock_set_mul_div: set multiplier/divider for child clocks > + * @clk: clock > + * @multiplier: multiplier value > + * @divider: divider value > + * > + * By default, a Clock's children will all run with the same period > + * as their parent. This function allows you to adjust the multiplier > + * and divider used to derive the child clock frequency. > + * For example, setting a multiplier of 2 and a divider of 3 > + * will run child clocks with a period 2/3 of the parent clock, > + * so if the parent clock is an 8MHz clock the children will > + * be 12MHz. > + * > + * Setting the multiplier to 0 will stop the child clocks. > + * Setting the divider to 0 is a programming error (diagnosed with > + * an assertion failure). > + * Setting a multiplier value that results in the child period > + * overflowing is not diagnosed. > + * > + * Note that this function does not call clock_propagate(); the > + * caller should do that if necessary. > + */ > +void clock_set_mul_div(Clock *clk, uint32_t multiplier, uint32_t divider); > + > #endif /* QEMU_HW_CLOCK_H */ > diff --git a/hw/core/clock-vmstate.c b/hw/core/clock-vmstate.c > index 260b13fc2c8..07bb45d7ed4 100644 > --- a/hw/core/clock-vmstate.c > +++ b/hw/core/clock-vmstate.c > @@ -14,6 +14,24 @@ > #include "migration/vmstate.h" > #include "hw/clock.h" > > +static bool muldiv_needed(void *opaque) > +{ > + Clock *clk = opaque; > + > + return clk->multiplier != 1 || clk->divider != 1; > +} > + > +const VMStateDescription vmstate_muldiv = { > + .name = "clock/muldiv", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = muldiv_needed, > + .fields = (VMStateField[]) { > + VMSTATE_UINT32(multiplier, Clock), > + VMSTATE_UINT32(divider, Clock), > + }, > +}; > + > const VMStateDescription vmstate_clock = { > .name = "clock", > .version_id = 0, > @@ -21,5 +39,9 @@ const VMStateDescription vmstate_clock = { > .fields = (VMStateField[]) { > VMSTATE_UINT64(period, Clock), > VMSTATE_END_OF_LIST() > - } > + }, > + .subsections = (const VMStateDescription*[]) { > + &vmstate_muldiv, > + NULL > + }, > }; > diff --git a/hw/core/clock.c b/hw/core/clock.c > index fc5a99683f8..c371b9e977a 100644 > --- a/hw/core/clock.c > +++ b/hw/core/clock.c > @@ -64,6 +64,15 @@ bool clock_set(Clock *clk, uint64_t period) > return true; > } > > +static uint64_t clock_get_child_period(Clock *clk) > +{ > + /* > + * Return the period to be used for child clocks, which is the parent > + * clock period adjusted for for multiplier and divider effects. > + */ > + return muldiv64(clk->period, clk->multiplier, clk->divider); > +} > + > static void clock_call_callback(Clock *clk, ClockEvent event) > { > /* > @@ -78,15 +87,16 @@ static void clock_call_callback(Clock *clk, ClockEvent event) > static void clock_propagate_period(Clock *clk, bool call_callbacks) > { > Clock *child; > + uint64_t child_period = clock_get_child_period(clk); > > QLIST_FOREACH(child, &clk->children, sibling) { > - if (child->period != clk->period) { > + if (child->period != child_period) { > if (call_callbacks) { > clock_call_callback(child, ClockPreUpdate); > } > - child->period = clk->period; > + child->period = child_period; > trace_clock_update(CLOCK_PATH(child), CLOCK_PATH(clk), > - CLOCK_PERIOD_TO_HZ(clk->period), > + CLOCK_PERIOD_TO_HZ(child->period), > call_callbacks); > if (call_callbacks) { > clock_call_callback(child, ClockUpdate); > @@ -110,7 +120,7 @@ void clock_set_source(Clock *clk, Clock *src) > > trace_clock_set_source(CLOCK_PATH(clk), CLOCK_PATH(src)); > > - clk->period = src->period; > + clk->period = clock_get_child_period(src); > QLIST_INSERT_HEAD(&src->children, clk, sibling); > clk->source = src; > clock_propagate_period(clk, false); > @@ -133,10 +143,21 @@ char *clock_display_freq(Clock *clk) > return freq_to_str(clock_get_hz(clk)); > } > > +void clock_set_mul_div(Clock *clk, uint32_t multiplier, uint32_t divider) > +{ > + assert(divider != 0); > + > + clk->multiplier = multiplier; > + clk->divider = divider; > +} > + > static void clock_initfn(Object *obj) > { > Clock *clk = CLOCK(obj); > > + clk->multiplier = 1; > + clk->divider = 1; > + > QLIST_INIT(&clk->children); > } > > Reviewed-by: Alexandre Iooss <erdnaxe@crans.org> Thanks, -- Alexandre
On Thu, 12 Aug 2021 at 13:08, Alexandre IOOSS <erdnaxe@crans.org> wrote: > > > On 8/12/21 11:33 AM, Peter Maydell wrote: > > It is quite common for a clock tree to involve possibly programmable > > clock multipliers or dividers, where the frequency of a clock is for > > instance divided by 8 to produce a slower clock to feed to a > > particular device. > > > > Currently we provide no convenient mechanism for modelling this. You > > can implement it by having an input Clock and an output Clock, and > > manually setting the period of the output clock in the period-changed > > callback of the input clock, but that's quite clunky. > > > > This patch adds support in the Clock objects themselves for setting a > > multiplier or divider. The effect of setting this on a clock is that > > when the clock's period is changed, all the children of the clock are > > set to period * multiplier / divider, rather than being set to the > > same period as the parent clock. > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > docs/devel/clocks.rst | 23 +++++++++++++++++++++++ > > include/hw/clock.h | 29 +++++++++++++++++++++++++++++ > > hw/core/clock-vmstate.c | 24 +++++++++++++++++++++++- > > hw/core/clock.c | 29 +++++++++++++++++++++++++---- > > 4 files changed, 100 insertions(+), 5 deletions(-) > > > > diff --git a/docs/devel/clocks.rst b/docs/devel/clocks.rst > > index 956bd147ea0..430fbd842e5 100644 > > --- a/docs/devel/clocks.rst > > +++ b/docs/devel/clocks.rst > > @@ -260,6 +260,29 @@ clocks get the new clock period value: *Clock 2*, *Clock 3* and *Clock 4*. > > It is not possible to disconnect a clock or to change the clock connection > > after it is connected. > > > > +Clock multiplier and divider settings > > +------------------------------------- > > + > > +By default, when clocks are connected together, the child > > +clocks run with the same period as their source (parent) clock. > > +The Clock API supports a built-in period multiplier/divider > > +mechanism so you can configure a clock to make its children > > +run at a different period from its own. If you call the > > +``clock_set_mul_div()`` function you can specify the clock's > > +multiplier and divider values. The children of that clock > > +will all run with a period of ``parent_period * multiplier / divider``. > > +For instance, if the clock has a frequency of 8MHz and you set its > > +multiplier to 2 and its divider to 3, the child clocks will run > > +at 12MHz. > > + > > +You can change the multiplier and divider of a clock at runtime, > > +so you can use this to model clock controller devices which > > +have guest-programmable frequency multipliers or dividers. > > This looks nice! > Does this imply that if I am going to implement the STM32 RCC (Reset and > Clock Controller) device, then I should use this new feature? I haven't looked at the RCC spec in detail, but yeah, I'd expect that you'd have a device with one (or perhaps more) input clocks and a lot of output clocks, and as the guest writes to registers that set divider/multiplier settings you'd use clock_set_mul_div() to configure the appropriate output clocks. -- PMM
On Thu, Aug 12, 2021 at 7:38 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > It is quite common for a clock tree to involve possibly programmable > clock multipliers or dividers, where the frequency of a clock is for > instance divided by 8 to produce a slower clock to feed to a > particular device. > > Currently we provide no convenient mechanism for modelling this. You > can implement it by having an input Clock and an output Clock, and > manually setting the period of the output clock in the period-changed > callback of the input clock, but that's quite clunky. > > This patch adds support in the Clock objects themselves for setting a > multiplier or divider. The effect of setting this on a clock is that > when the clock's period is changed, all the children of the clock are > set to period * multiplier / divider, rather than being set to the > same period as the parent clock. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > docs/devel/clocks.rst | 23 +++++++++++++++++++++++ > include/hw/clock.h | 29 +++++++++++++++++++++++++++++ > hw/core/clock-vmstate.c | 24 +++++++++++++++++++++++- > hw/core/clock.c | 29 +++++++++++++++++++++++++---- > 4 files changed, 100 insertions(+), 5 deletions(-) > > diff --git a/docs/devel/clocks.rst b/docs/devel/clocks.rst > index 956bd147ea0..430fbd842e5 100644 > --- a/docs/devel/clocks.rst > +++ b/docs/devel/clocks.rst > @@ -260,6 +260,29 @@ clocks get the new clock period value: *Clock 2*, *Clock 3* and *Clock 4*. > It is not possible to disconnect a clock or to change the clock connection > after it is connected. > > +Clock multiplier and divider settings > +------------------------------------- > + > +By default, when clocks are connected together, the child > +clocks run with the same period as their source (parent) clock. > +The Clock API supports a built-in period multiplier/divider > +mechanism so you can configure a clock to make its children > +run at a different period from its own. If you call the > +``clock_set_mul_div()`` function you can specify the clock's > +multiplier and divider values. The children of that clock > +will all run with a period of ``parent_period * multiplier / divider``. > +For instance, if the clock has a frequency of 8MHz and you set its > +multiplier to 2 and its divider to 3, the child clocks will run > +at 12MHz. > + > +You can change the multiplier and divider of a clock at runtime, > +so you can use this to model clock controller devices which > +have guest-programmable frequency multipliers or dividers. > + > +Note that ``clock_set_mul_div()`` does not automatically call > +``clock_propagate()``. If you make a runtime change to the > +multiplier or divider you must call clock_propagate() yourself.a You have an extra `a` here after the full stop. Otherwise: Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > + > Unconnected input clocks > ------------------------ > > diff --git a/include/hw/clock.h b/include/hw/clock.h > index a7187eab95e..11f67fb9701 100644 > --- a/include/hw/clock.h > +++ b/include/hw/clock.h > @@ -81,6 +81,10 @@ struct Clock { > void *callback_opaque; > unsigned int callback_events; > > + /* Ratio of the parent clock to run the child clocks at */ > + uint32_t multiplier; > + uint32_t divider; > + > /* Clocks are organized in a clock tree */ > Clock *source; > QLIST_HEAD(, Clock) children; > @@ -350,4 +354,29 @@ static inline bool clock_is_enabled(const Clock *clk) > */ > char *clock_display_freq(Clock *clk); > > +/** > + * clock_set_mul_div: set multiplier/divider for child clocks > + * @clk: clock > + * @multiplier: multiplier value > + * @divider: divider value > + * > + * By default, a Clock's children will all run with the same period > + * as their parent. This function allows you to adjust the multiplier > + * and divider used to derive the child clock frequency. > + * For example, setting a multiplier of 2 and a divider of 3 > + * will run child clocks with a period 2/3 of the parent clock, > + * so if the parent clock is an 8MHz clock the children will > + * be 12MHz. > + * > + * Setting the multiplier to 0 will stop the child clocks. > + * Setting the divider to 0 is a programming error (diagnosed with > + * an assertion failure). > + * Setting a multiplier value that results in the child period > + * overflowing is not diagnosed. > + * > + * Note that this function does not call clock_propagate(); the > + * caller should do that if necessary. > + */ > +void clock_set_mul_div(Clock *clk, uint32_t multiplier, uint32_t divider); > + > #endif /* QEMU_HW_CLOCK_H */ > diff --git a/hw/core/clock-vmstate.c b/hw/core/clock-vmstate.c > index 260b13fc2c8..07bb45d7ed4 100644 > --- a/hw/core/clock-vmstate.c > +++ b/hw/core/clock-vmstate.c > @@ -14,6 +14,24 @@ > #include "migration/vmstate.h" > #include "hw/clock.h" > > +static bool muldiv_needed(void *opaque) > +{ > + Clock *clk = opaque; > + > + return clk->multiplier != 1 || clk->divider != 1; > +} > + > +const VMStateDescription vmstate_muldiv = { > + .name = "clock/muldiv", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = muldiv_needed, > + .fields = (VMStateField[]) { > + VMSTATE_UINT32(multiplier, Clock), > + VMSTATE_UINT32(divider, Clock), > + }, > +}; > + > const VMStateDescription vmstate_clock = { > .name = "clock", > .version_id = 0, > @@ -21,5 +39,9 @@ const VMStateDescription vmstate_clock = { > .fields = (VMStateField[]) { > VMSTATE_UINT64(period, Clock), > VMSTATE_END_OF_LIST() > - } > + }, > + .subsections = (const VMStateDescription*[]) { > + &vmstate_muldiv, > + NULL > + }, > }; > diff --git a/hw/core/clock.c b/hw/core/clock.c > index fc5a99683f8..c371b9e977a 100644 > --- a/hw/core/clock.c > +++ b/hw/core/clock.c > @@ -64,6 +64,15 @@ bool clock_set(Clock *clk, uint64_t period) > return true; > } > > +static uint64_t clock_get_child_period(Clock *clk) > +{ > + /* > + * Return the period to be used for child clocks, which is the parent > + * clock period adjusted for for multiplier and divider effects. > + */ > + return muldiv64(clk->period, clk->multiplier, clk->divider); > +} > + > static void clock_call_callback(Clock *clk, ClockEvent event) > { > /* > @@ -78,15 +87,16 @@ static void clock_call_callback(Clock *clk, ClockEvent event) > static void clock_propagate_period(Clock *clk, bool call_callbacks) > { > Clock *child; > + uint64_t child_period = clock_get_child_period(clk); > > QLIST_FOREACH(child, &clk->children, sibling) { > - if (child->period != clk->period) { > + if (child->period != child_period) { > if (call_callbacks) { > clock_call_callback(child, ClockPreUpdate); > } > - child->period = clk->period; > + child->period = child_period; > trace_clock_update(CLOCK_PATH(child), CLOCK_PATH(clk), > - CLOCK_PERIOD_TO_HZ(clk->period), > + CLOCK_PERIOD_TO_HZ(child->period), > call_callbacks); > if (call_callbacks) { > clock_call_callback(child, ClockUpdate); > @@ -110,7 +120,7 @@ void clock_set_source(Clock *clk, Clock *src) > > trace_clock_set_source(CLOCK_PATH(clk), CLOCK_PATH(src)); > > - clk->period = src->period; > + clk->period = clock_get_child_period(src); > QLIST_INSERT_HEAD(&src->children, clk, sibling); > clk->source = src; > clock_propagate_period(clk, false); > @@ -133,10 +143,21 @@ char *clock_display_freq(Clock *clk) > return freq_to_str(clock_get_hz(clk)); > } > > +void clock_set_mul_div(Clock *clk, uint32_t multiplier, uint32_t divider) > +{ > + assert(divider != 0); > + > + clk->multiplier = multiplier; > + clk->divider = divider; > +} > + > static void clock_initfn(Object *obj) > { > Clock *clk = CLOCK(obj); > > + clk->multiplier = 1; > + clk->divider = 1; > + > QLIST_INIT(&clk->children); > } > > -- > 2.20.1 > >
On 8/12/21 11:33 AM, Peter Maydell wrote: > It is quite common for a clock tree to involve possibly programmable > clock multipliers or dividers, where the frequency of a clock is for > instance divided by 8 to produce a slower clock to feed to a > particular device. > > Currently we provide no convenient mechanism for modelling this. You > can implement it by having an input Clock and an output Clock, and > manually setting the period of the output clock in the period-changed > callback of the input clock, but that's quite clunky. > > This patch adds support in the Clock objects themselves for setting a > multiplier or divider. The effect of setting this on a clock is that > when the clock's period is changed, all the children of the clock are > set to period * multiplier / divider, rather than being set to the > same period as the parent clock. This is super nice! > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > docs/devel/clocks.rst | 23 +++++++++++++++++++++++ > include/hw/clock.h | 29 +++++++++++++++++++++++++++++ > hw/core/clock-vmstate.c | 24 +++++++++++++++++++++++- > hw/core/clock.c | 29 +++++++++++++++++++++++++---- > 4 files changed, 100 insertions(+), 5 deletions(-) > +void clock_set_mul_div(Clock *clk, uint32_t multiplier, uint32_t divider) > +{ > + assert(divider != 0); I only wonder if we shouldn't check clock_is_enabled() here. Maybe not assert, but at least report a GUEST_ERROR? Also a trace-event "mul: %u -> %u, div: %u -> %u" would be useful IMHO. > + clk->multiplier = multiplier; > + clk->divider = divider; > +}
Hi Peter, On 10:33 Thu 12 Aug , Peter Maydell wrote: > diff --git a/hw/core/clock.c b/hw/core/clock.c > index fc5a99683f8..c371b9e977a 100644 > --- a/hw/core/clock.c > +++ b/hw/core/clock.c > @@ -133,10 +143,21 @@ char *clock_display_freq(Clock *clk) > return freq_to_str(clock_get_hz(clk)); > } > > +void clock_set_mul_div(Clock *clk, uint32_t multiplier, uint32_t divider) > +{ > + assert(divider != 0); > + Maybe add a trace here to ease debugging? > + clk->multiplier = multiplier; > + clk->divider = divider; > +} > + Otherwise: Reviewed-by: Luc Michel <luc@lmichel.fr>
On Sun, 15 Aug 2021 at 17:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > On 8/12/21 11:33 AM, Peter Maydell wrote: > > It is quite common for a clock tree to involve possibly programmable > > clock multipliers or dividers, where the frequency of a clock is for > > instance divided by 8 to produce a slower clock to feed to a > > particular device. > > > > Currently we provide no convenient mechanism for modelling this. You > > can implement it by having an input Clock and an output Clock, and > > manually setting the period of the output clock in the period-changed > > callback of the input clock, but that's quite clunky. > > > > This patch adds support in the Clock objects themselves for setting a > > multiplier or divider. The effect of setting this on a clock is that > > when the clock's period is changed, all the children of the clock are > > set to period * multiplier / divider, rather than being set to the > > same period as the parent clock. > > This is super nice! > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > docs/devel/clocks.rst | 23 +++++++++++++++++++++++ > > include/hw/clock.h | 29 +++++++++++++++++++++++++++++ > > hw/core/clock-vmstate.c | 24 +++++++++++++++++++++++- > > hw/core/clock.c | 29 +++++++++++++++++++++++++---- > > 4 files changed, 100 insertions(+), 5 deletions(-) > > > +void clock_set_mul_div(Clock *clk, uint32_t multiplier, uint32_t divider) > > +{ > > + assert(divider != 0); > > I only wonder if we shouldn't check clock_is_enabled() here. > Maybe not assert, but at least report a GUEST_ERROR? Setting the multiplier on a disabled clock doesn't seem like an error to me. I would expect a common way for the guest to program a clock-controller would be "first set the divider and any other parameters; finally, enable the clock". -- PMM
On 8/16/21 11:05 AM, Peter Maydell wrote: > On Sun, 15 Aug 2021 at 17:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> >> On 8/12/21 11:33 AM, Peter Maydell wrote: >>> It is quite common for a clock tree to involve possibly programmable >>> clock multipliers or dividers, where the frequency of a clock is for >>> instance divided by 8 to produce a slower clock to feed to a >>> particular device. >>> >>> Currently we provide no convenient mechanism for modelling this. You >>> can implement it by having an input Clock and an output Clock, and >>> manually setting the period of the output clock in the period-changed >>> callback of the input clock, but that's quite clunky. >>> >>> This patch adds support in the Clock objects themselves for setting a >>> multiplier or divider. The effect of setting this on a clock is that >>> when the clock's period is changed, all the children of the clock are >>> set to period * multiplier / divider, rather than being set to the >>> same period as the parent clock. >> >> This is super nice! >> >>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >>> --- >>> docs/devel/clocks.rst | 23 +++++++++++++++++++++++ >>> include/hw/clock.h | 29 +++++++++++++++++++++++++++++ >>> hw/core/clock-vmstate.c | 24 +++++++++++++++++++++++- >>> hw/core/clock.c | 29 +++++++++++++++++++++++++---- >>> 4 files changed, 100 insertions(+), 5 deletions(-) >> >>> +void clock_set_mul_div(Clock *clk, uint32_t multiplier, uint32_t divider) >>> +{ >>> + assert(divider != 0); >> >> I only wonder if we shouldn't check clock_is_enabled() here. >> Maybe not assert, but at least report a GUEST_ERROR? > > Setting the multiplier on a disabled clock doesn't seem like > an error to me. I would expect a common way for the guest to > program a clock-controller would be "first set the divider > and any other parameters; finally, enable the clock". Eh sorry I meant the other way around :/ It is usually either illegal or undefined behavior on real hw to change a clock scale while it is active. Personally I'd be interested to catch guests doing so. I was thinking of: if (clock_is_enabled(clk)) { qemu_log_mask(LOG_GUEST_ERROR, "Changing scale of ENABLED clock '%s'\n'", CLOCK_PATH(clk)); }
On Mon, 16 Aug 2021 at 10:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > On 8/16/21 11:05 AM, Peter Maydell wrote: > > On Sun, 15 Aug 2021 at 17:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > >> I only wonder if we shouldn't check clock_is_enabled() here. > >> Maybe not assert, but at least report a GUEST_ERROR? > > > > Setting the multiplier on a disabled clock doesn't seem like > > an error to me. I would expect a common way for the guest to > > program a clock-controller would be "first set the divider > > and any other parameters; finally, enable the clock". > > Eh sorry I meant the other way around :/ It is usually either > illegal or undefined behavior on real hw to change a clock scale > while it is active. Personally I'd be interested to catch guests > doing so. I was thinking of: > > if (clock_is_enabled(clk)) { > qemu_log_mask(LOG_GUEST_ERROR, > "Changing scale of ENABLED clock '%s'\n'", > CLOCK_PATH(clk)); > } I think if particular clock-controller hardware has that restriction we should be logging guest errors there. (Doing that also has the advantage that we can make the error clearer by being specific about what guest hardware register/device is being mis-programmed.) I don't think we can be certain enough that it's always wrong to change the divider on a running clock to put the error in the common clock API code. (Among other things, I suspect a warning here would be easy to trigger incorrectly when connecting up hard-wired clock dividers at startup.) thanks -- PMM
On 8/16/21 11:36 AM, Peter Maydell wrote: > On Mon, 16 Aug 2021 at 10:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> >> On 8/16/21 11:05 AM, Peter Maydell wrote: >>> On Sun, 15 Aug 2021 at 17:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >>>> I only wonder if we shouldn't check clock_is_enabled() here. >>>> Maybe not assert, but at least report a GUEST_ERROR? >>> >>> Setting the multiplier on a disabled clock doesn't seem like >>> an error to me. I would expect a common way for the guest to >>> program a clock-controller would be "first set the divider >>> and any other parameters; finally, enable the clock". >> >> Eh sorry I meant the other way around :/ It is usually either >> illegal or undefined behavior on real hw to change a clock scale >> while it is active. Personally I'd be interested to catch guests >> doing so. I was thinking of: >> >> if (clock_is_enabled(clk)) { >> qemu_log_mask(LOG_GUEST_ERROR, >> "Changing scale of ENABLED clock '%s'\n'", >> CLOCK_PATH(clk)); >> } > > I think if particular clock-controller hardware has that > restriction we should be logging guest errors there. (Doing that > also has the advantage that we can make the error clearer by being > specific about what guest hardware register/device is being > mis-programmed.) OK. > I don't think we can be certain enough that it's > always wrong to change the divider on a running clock to put the error > in the common clock API code. OK. > (Among other things, I suspect a warning > here would be easy to trigger incorrectly when connecting up hard-wired > clock dividers at startup.) Yes, probably. Thanks for the clarifications. Preferably with a trace-event: Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
On 8/12/21 11:33 AM, Peter Maydell wrote: > It is quite common for a clock tree to involve possibly programmable > clock multipliers or dividers, where the frequency of a clock is for > instance divided by 8 to produce a slower clock to feed to a > particular device. > > Currently we provide no convenient mechanism for modelling this. You > can implement it by having an input Clock and an output Clock, and > manually setting the period of the output clock in the period-changed > callback of the input clock, but that's quite clunky. > > This patch adds support in the Clock objects themselves for setting a > multiplier or divider. The effect of setting this on a clock is that > when the clock's period is changed, all the children of the clock are > set to period * multiplier / divider, rather than being set to the > same period as the parent clock. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > docs/devel/clocks.rst | 23 +++++++++++++++++++++++ > include/hw/clock.h | 29 +++++++++++++++++++++++++++++ > hw/core/clock-vmstate.c | 24 +++++++++++++++++++++++- > hw/core/clock.c | 29 +++++++++++++++++++++++++---- > 4 files changed, 100 insertions(+), 5 deletions(-) > > diff --git a/docs/devel/clocks.rst b/docs/devel/clocks.rst > index 956bd147ea0..430fbd842e5 100644 > --- a/docs/devel/clocks.rst > +++ b/docs/devel/clocks.rst > @@ -260,6 +260,29 @@ clocks get the new clock period value: *Clock 2*, *Clock 3* and *Clock 4*. > It is not possible to disconnect a clock or to change the clock connection > after it is connected. > > +Clock multiplier and divider settings > +------------------------------------- > + > +By default, when clocks are connected together, the child > +clocks run with the same period as their source (parent) clock. > +The Clock API supports a built-in period multiplier/divider > +mechanism so you can configure a clock to make its children > +run at a different period from its own. If you call the > +``clock_set_mul_div()`` function you can specify the clock's > +multiplier and divider values. The children of that clock > +will all run with a period of ``parent_period * multiplier / divider``. > +For instance, if the clock has a frequency of 8MHz and you set its > +multiplier to 2 and its divider to 3, the child clocks will run > +at 12MHz. > + > +You can change the multiplier and divider of a clock at runtime, > +so you can use this to model clock controller devices which > +have guest-programmable frequency multipliers or dividers. > + > +Note that ``clock_set_mul_div()`` does not automatically call > +``clock_propagate()``. If you make a runtime change to the > +multiplier or divider you must call clock_propagate() yourself.a > + > Unconnected input clocks > ------------------------ > > diff --git a/include/hw/clock.h b/include/hw/clock.h > index a7187eab95e..11f67fb9701 100644 > --- a/include/hw/clock.h > +++ b/include/hw/clock.h > @@ -81,6 +81,10 @@ struct Clock { > void *callback_opaque; > unsigned int callback_events; > > + /* Ratio of the parent clock to run the child clocks at */ > + uint32_t multiplier; > + uint32_t divider; > + > /* Clocks are organized in a clock tree */ > Clock *source; > QLIST_HEAD(, Clock) children; > @@ -350,4 +354,29 @@ static inline bool clock_is_enabled(const Clock *clk) > */ > char *clock_display_freq(Clock *clk); > > +/** > + * clock_set_mul_div: set multiplier/divider for child clocks > + * @clk: clock > + * @multiplier: multiplier value > + * @divider: divider value > + * > + * By default, a Clock's children will all run with the same period > + * as their parent. This function allows you to adjust the multiplier > + * and divider used to derive the child clock frequency. > + * For example, setting a multiplier of 2 and a divider of 3 > + * will run child clocks with a period 2/3 of the parent clock, > + * so if the parent clock is an 8MHz clock the children will > + * be 12MHz. > + * > + * Setting the multiplier to 0 will stop the child clocks. > + * Setting the divider to 0 is a programming error (diagnosed with > + * an assertion failure). > + * Setting a multiplier value that results in the child period > + * overflowing is not diagnosed. > + * > + * Note that this function does not call clock_propagate(); the > + * caller should do that if necessary. > + */ > +void clock_set_mul_div(Clock *clk, uint32_t multiplier, uint32_t divider); > + > #endif /* QEMU_HW_CLOCK_H */ > diff --git a/hw/core/clock-vmstate.c b/hw/core/clock-vmstate.c > index 260b13fc2c8..07bb45d7ed4 100644 > --- a/hw/core/clock-vmstate.c > +++ b/hw/core/clock-vmstate.c > @@ -14,6 +14,24 @@ > #include "migration/vmstate.h" > #include "hw/clock.h" > > +static bool muldiv_needed(void *opaque) > +{ > + Clock *clk = opaque; > + > + return clk->multiplier != 1 || clk->divider != 1; > +} > + > +const VMStateDescription vmstate_muldiv = { > + .name = "clock/muldiv", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = muldiv_needed, > + .fields = (VMStateField[]) { > + VMSTATE_UINT32(multiplier, Clock), > + VMSTATE_UINT32(divider, Clock), > + }, > +}; > + > const VMStateDescription vmstate_clock = { > .name = "clock", > .version_id = 0, > @@ -21,5 +39,9 @@ const VMStateDescription vmstate_clock = { > .fields = (VMStateField[]) { > VMSTATE_UINT64(period, Clock), > VMSTATE_END_OF_LIST() > - } > + }, > + .subsections = (const VMStateDescription*[]) { > + &vmstate_muldiv, > + NULL > + }, > }; > diff --git a/hw/core/clock.c b/hw/core/clock.c > index fc5a99683f8..c371b9e977a 100644 > --- a/hw/core/clock.c > +++ b/hw/core/clock.c > @@ -64,6 +64,15 @@ bool clock_set(Clock *clk, uint64_t period) > return true; > } > > +static uint64_t clock_get_child_period(Clock *clk) > +{ > + /* > + * Return the period to be used for child clocks, which is the parent > + * clock period adjusted for for multiplier and divider effects. > + */ > + return muldiv64(clk->period, clk->multiplier, clk->divider); > +} > + > static void clock_call_callback(Clock *clk, ClockEvent event) > { > /* > @@ -78,15 +87,16 @@ static void clock_call_callback(Clock *clk, ClockEvent event) > static void clock_propagate_period(Clock *clk, bool call_callbacks) > { > Clock *child; > + uint64_t child_period = clock_get_child_period(clk); > > QLIST_FOREACH(child, &clk->children, sibling) { > - if (child->period != clk->period) { > + if (child->period != child_period) { > if (call_callbacks) { > clock_call_callback(child, ClockPreUpdate); > } > - child->period = clk->period; > + child->period = child_period; > trace_clock_update(CLOCK_PATH(child), CLOCK_PATH(clk), > - CLOCK_PERIOD_TO_HZ(clk->period), > + CLOCK_PERIOD_TO_HZ(child->period), > call_callbacks); > if (call_callbacks) { > clock_call_callback(child, ClockUpdate); > @@ -110,7 +120,7 @@ void clock_set_source(Clock *clk, Clock *src) > > trace_clock_set_source(CLOCK_PATH(clk), CLOCK_PATH(src)); > > - clk->period = src->period; > + clk->period = clock_get_child_period(src); > QLIST_INSERT_HEAD(&src->children, clk, sibling); > clk->source = src; > clock_propagate_period(clk, false); > @@ -133,10 +143,21 @@ char *clock_display_freq(Clock *clk) > return freq_to_str(clock_get_hz(clk)); > } > > +void clock_set_mul_div(Clock *clk, uint32_t multiplier, uint32_t divider) > +{ > + assert(divider != 0); > + > + clk->multiplier = multiplier; > + clk->divider = divider; > +} > + > static void clock_initfn(Object *obj) > { > Clock *clk = CLOCK(obj); > > + clk->multiplier = 1; > + clk->divider = 1; > + > QLIST_INIT(&clk->children); > } > > Regarding migration, you made the vmstate_muldiv subsection optional. I suppose it is to keep backward migration working in case of simple mul=1,div=1 clocks. Do we need to ensure multiplier and divider fields are set to 1 upon receiving a state if the vmstate_muldiv subsection is absent ? I remember there are post_load() tricks to achieve that. Thanks, Damien
On Tue, 17 Aug 2021 at 10:59, Damien Hedde <damien.hedde@greensocs.com> wrote: > > > > On 8/12/21 11:33 AM, Peter Maydell wrote: > > +void clock_set_mul_div(Clock *clk, uint32_t multiplier, uint32_t divider) > > +{ > > + assert(divider != 0); > > + > > + clk->multiplier = multiplier; > > + clk->divider = divider; > > +} > > + > > static void clock_initfn(Object *obj) > > { > > Clock *clk = CLOCK(obj); > > > > + clk->multiplier = 1; > > + clk->divider = 1; > > + > > QLIST_INIT(&clk->children); > > } > > > > > > Regarding migration, you made the vmstate_muldiv subsection optional. I > suppose it is to keep backward migration working in case of simple > mul=1,div=1 clocks. Yes -- we only send the subsection if the mul,div are something other than 1,1; an inbound migration stream without the subsection then means "use 1,1". > Do we need to ensure multiplier and divider fields are set to 1 upon > receiving a state if the vmstate_muldiv subsection is absent ? I > remember there are post_load() tricks to achieve that. I was relying on "we set the default in clock_initfn()" (by analogy with being able to assume that fields in device state are at their reset values when an inbound migration happens, so if the migration doesn't set them then they stay at those reset values). But thinking about it a bit more I think you're right and we do have to have a pre_load function to set them to 1. Otherwise we would get wrong the case where a board/SoC/device sets a clock up on reset to have a non-1 multiplier, and then later the guest programs it to be 1, and then we migrate like that. (That is, the mul/div at point of migration will be the value-on-reset as set by the device etc code that created the clock, which might be different from what clock_initfn() set it to.) So we need to add something like static int clock_pre_load(void *opaque) { Clock *clk = opaque; /* * The initial out-of-reset settings of the Clock might have been * configured by the device to be different from what we set * in clock_initfn(), so we must here set the default values to * be used if they are not in the inbound migration state. */ clk->multiplier = 1; clk->divider = 1; } thanks -- PMM
On 8/17/21 12:46 PM, Peter Maydell wrote: > On Tue, 17 Aug 2021 at 10:59, Damien Hedde <damien.hedde@greensocs.com> wrote: >> >> >> >> On 8/12/21 11:33 AM, Peter Maydell wrote: > >>> +void clock_set_mul_div(Clock *clk, uint32_t multiplier, uint32_t divider) >>> +{ >>> + assert(divider != 0); >>> + >>> + clk->multiplier = multiplier; >>> + clk->divider = divider; >>> +} >>> + >>> static void clock_initfn(Object *obj) >>> { >>> Clock *clk = CLOCK(obj); >>> >>> + clk->multiplier = 1; >>> + clk->divider = 1; >>> + >>> QLIST_INIT(&clk->children); >>> } >>> >>> >> >> Regarding migration, you made the vmstate_muldiv subsection optional. I >> suppose it is to keep backward migration working in case of simple >> mul=1,div=1 clocks. > > Yes -- we only send the subsection if the mul,div are > something other than 1,1; an inbound migration stream without > the subsection then means "use 1,1". > >> Do we need to ensure multiplier and divider fields are set to 1 upon >> receiving a state if the vmstate_muldiv subsection is absent ? I >> remember there are post_load() tricks to achieve that. > > I was relying on "we set the default in clock_initfn()" (by analogy > with being able to assume that fields in device state are at their > reset values when an inbound migration happens, so if the migration > doesn't set them then they stay at those reset values). But > thinking about it a bit more I think you're right and we do have to > have a pre_load function to set them to 1. Otherwise we would get > wrong the case where a board/SoC/device sets a clock up on reset > to have a non-1 multiplier, and then later the guest programs it to > be 1, and then we migrate like that. (That is, the mul/div at point > of migration will be the value-on-reset as set by the device etc > code that created the clock, which might be different from what > clock_initfn() set it to.) Yes, I think we cannot expect the machine init or reset to keep all clocks in div=1,mul=1 state. > > So we need to add something like > > static int clock_pre_load(void *opaque) > { > Clock *clk = opaque; > /* > * The initial out-of-reset settings of the Clock might have been > * configured by the device to be different from what we set > * in clock_initfn(), so we must here set the default values to > * be used if they are not in the inbound migration state. > */ > clk->multiplier = 1; > clk->divider = 1; > } > With this callback registered in the main vmsd section, Reviewed-by: Damien Hedde <damien.hedde@greensocs.com> Thanks, -- Damien
diff --git a/docs/devel/clocks.rst b/docs/devel/clocks.rst index 956bd147ea0..430fbd842e5 100644 --- a/docs/devel/clocks.rst +++ b/docs/devel/clocks.rst @@ -260,6 +260,29 @@ clocks get the new clock period value: *Clock 2*, *Clock 3* and *Clock 4*. It is not possible to disconnect a clock or to change the clock connection after it is connected. +Clock multiplier and divider settings +------------------------------------- + +By default, when clocks are connected together, the child +clocks run with the same period as their source (parent) clock. +The Clock API supports a built-in period multiplier/divider +mechanism so you can configure a clock to make its children +run at a different period from its own. If you call the +``clock_set_mul_div()`` function you can specify the clock's +multiplier and divider values. The children of that clock +will all run with a period of ``parent_period * multiplier / divider``. +For instance, if the clock has a frequency of 8MHz and you set its +multiplier to 2 and its divider to 3, the child clocks will run +at 12MHz. + +You can change the multiplier and divider of a clock at runtime, +so you can use this to model clock controller devices which +have guest-programmable frequency multipliers or dividers. + +Note that ``clock_set_mul_div()`` does not automatically call +``clock_propagate()``. If you make a runtime change to the +multiplier or divider you must call clock_propagate() yourself.a + Unconnected input clocks ------------------------ diff --git a/include/hw/clock.h b/include/hw/clock.h index a7187eab95e..11f67fb9701 100644 --- a/include/hw/clock.h +++ b/include/hw/clock.h @@ -81,6 +81,10 @@ struct Clock { void *callback_opaque; unsigned int callback_events; + /* Ratio of the parent clock to run the child clocks at */ + uint32_t multiplier; + uint32_t divider; + /* Clocks are organized in a clock tree */ Clock *source; QLIST_HEAD(, Clock) children; @@ -350,4 +354,29 @@ static inline bool clock_is_enabled(const Clock *clk) */ char *clock_display_freq(Clock *clk); +/** + * clock_set_mul_div: set multiplier/divider for child clocks + * @clk: clock + * @multiplier: multiplier value + * @divider: divider value + * + * By default, a Clock's children will all run with the same period + * as their parent. This function allows you to adjust the multiplier + * and divider used to derive the child clock frequency. + * For example, setting a multiplier of 2 and a divider of 3 + * will run child clocks with a period 2/3 of the parent clock, + * so if the parent clock is an 8MHz clock the children will + * be 12MHz. + * + * Setting the multiplier to 0 will stop the child clocks. + * Setting the divider to 0 is a programming error (diagnosed with + * an assertion failure). + * Setting a multiplier value that results in the child period + * overflowing is not diagnosed. + * + * Note that this function does not call clock_propagate(); the + * caller should do that if necessary. + */ +void clock_set_mul_div(Clock *clk, uint32_t multiplier, uint32_t divider); + #endif /* QEMU_HW_CLOCK_H */ diff --git a/hw/core/clock-vmstate.c b/hw/core/clock-vmstate.c index 260b13fc2c8..07bb45d7ed4 100644 --- a/hw/core/clock-vmstate.c +++ b/hw/core/clock-vmstate.c @@ -14,6 +14,24 @@ #include "migration/vmstate.h" #include "hw/clock.h" +static bool muldiv_needed(void *opaque) +{ + Clock *clk = opaque; + + return clk->multiplier != 1 || clk->divider != 1; +} + +const VMStateDescription vmstate_muldiv = { + .name = "clock/muldiv", + .version_id = 1, + .minimum_version_id = 1, + .needed = muldiv_needed, + .fields = (VMStateField[]) { + VMSTATE_UINT32(multiplier, Clock), + VMSTATE_UINT32(divider, Clock), + }, +}; + const VMStateDescription vmstate_clock = { .name = "clock", .version_id = 0, @@ -21,5 +39,9 @@ const VMStateDescription vmstate_clock = { .fields = (VMStateField[]) { VMSTATE_UINT64(period, Clock), VMSTATE_END_OF_LIST() - } + }, + .subsections = (const VMStateDescription*[]) { + &vmstate_muldiv, + NULL + }, }; diff --git a/hw/core/clock.c b/hw/core/clock.c index fc5a99683f8..c371b9e977a 100644 --- a/hw/core/clock.c +++ b/hw/core/clock.c @@ -64,6 +64,15 @@ bool clock_set(Clock *clk, uint64_t period) return true; } +static uint64_t clock_get_child_period(Clock *clk) +{ + /* + * Return the period to be used for child clocks, which is the parent + * clock period adjusted for for multiplier and divider effects. + */ + return muldiv64(clk->period, clk->multiplier, clk->divider); +} + static void clock_call_callback(Clock *clk, ClockEvent event) { /* @@ -78,15 +87,16 @@ static void clock_call_callback(Clock *clk, ClockEvent event) static void clock_propagate_period(Clock *clk, bool call_callbacks) { Clock *child; + uint64_t child_period = clock_get_child_period(clk); QLIST_FOREACH(child, &clk->children, sibling) { - if (child->period != clk->period) { + if (child->period != child_period) { if (call_callbacks) { clock_call_callback(child, ClockPreUpdate); } - child->period = clk->period; + child->period = child_period; trace_clock_update(CLOCK_PATH(child), CLOCK_PATH(clk), - CLOCK_PERIOD_TO_HZ(clk->period), + CLOCK_PERIOD_TO_HZ(child->period), call_callbacks); if (call_callbacks) { clock_call_callback(child, ClockUpdate); @@ -110,7 +120,7 @@ void clock_set_source(Clock *clk, Clock *src) trace_clock_set_source(CLOCK_PATH(clk), CLOCK_PATH(src)); - clk->period = src->period; + clk->period = clock_get_child_period(src); QLIST_INSERT_HEAD(&src->children, clk, sibling); clk->source = src; clock_propagate_period(clk, false); @@ -133,10 +143,21 @@ char *clock_display_freq(Clock *clk) return freq_to_str(clock_get_hz(clk)); } +void clock_set_mul_div(Clock *clk, uint32_t multiplier, uint32_t divider) +{ + assert(divider != 0); + + clk->multiplier = multiplier; + clk->divider = divider; +} + static void clock_initfn(Object *obj) { Clock *clk = CLOCK(obj); + clk->multiplier = 1; + clk->divider = 1; + QLIST_INIT(&clk->children); }
It is quite common for a clock tree to involve possibly programmable clock multipliers or dividers, where the frequency of a clock is for instance divided by 8 to produce a slower clock to feed to a particular device. Currently we provide no convenient mechanism for modelling this. You can implement it by having an input Clock and an output Clock, and manually setting the period of the output clock in the period-changed callback of the input clock, but that's quite clunky. This patch adds support in the Clock objects themselves for setting a multiplier or divider. The effect of setting this on a clock is that when the clock's period is changed, all the children of the clock are set to period * multiplier / divider, rather than being set to the same period as the parent clock. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- docs/devel/clocks.rst | 23 +++++++++++++++++++++++ include/hw/clock.h | 29 +++++++++++++++++++++++++++++ hw/core/clock-vmstate.c | 24 +++++++++++++++++++++++- hw/core/clock.c | 29 +++++++++++++++++++++++++---- 4 files changed, 100 insertions(+), 5 deletions(-) -- 2.20.1