Re: [PATCH v2 1/4] clock: Add ClockEvent parameter to callbacks
Hi Peter,
On 2/9/21 2:20 PM, Peter Maydell wrote:
> The Clock framework allows users to specify a callback which is
> called after the clock's period has been updated. Some users need to
> also have a callback which is called before the clock period is
> updated.
>
> As the first step in adding support for notifying Clock users on
> pre-update events, add an argument to the ClockCallback to specify
> what event is being notified, and add an argument to the various
> functions for registering a callback to specify which events are
> of interest to that callback.
>
> Note that the documentation update renders correct the previously
> incorrect claim in 'Adding a new clock' that callbacks "will be
> explained in a following section".
>
> Signed-off-by: Peter Maydell
> ---
> v1->v2: (suggested by Luc) instead of making callback functions check
> whether 'event' is one they are interested in, specify mask of
> interesting events at callback registration time.
> ---
> docs/devel/clocks.rst| 52 +++-
> include/hw/clock.h | 21 +++--
> include/hw/qdev-clock.h | 17 ---
> hw/adc/npcm7xx_adc.c | 2 +-
> hw/arm/armsse.c | 9 +++---
> hw/char/cadence_uart.c | 4 +--
> hw/char/ibex_uart.c | 4 +--
> hw/char/pl011.c | 5 +--
> hw/core/clock.c | 20 +---
> hw/core/qdev-clock.c | 8 +++--
> hw/mips/cps.c| 2 +-
> hw/misc/bcm2835_cprman.c | 23 --
> hw/misc/npcm7xx_clk.c| 26 +---
> hw/misc/npcm7xx_pwm.c| 2 +-
> hw/misc/zynq_slcr.c | 5 +--
> hw/timer/cmsdk-apb-dualtimer.c | 5 +--
> hw/timer/cmsdk-apb-timer.c | 4 +--
> hw/timer/npcm7xx_timer.c | 2 +-
> hw/watchdog/cmsdk-apb-watchdog.c | 5 +--
> target/mips/cpu.c| 2 +-
> 20 files changed, 160 insertions(+), 58 deletions(-)
> diff --git a/include/hw/clock.h b/include/hw/clock.h
> index e5f45e2626d..5c73b4e7ae9 100644
> --- a/include/hw/clock.h
> +++ b/include/hw/clock.h
> @@ -22,7 +22,17 @@
> #define TYPE_CLOCK "clock"
> OBJECT_DECLARE_SIMPLE_TYPE(Clock, CLOCK)
>
> -typedef void ClockCallback(void *opaque);
> +/*
> + * Argument to ClockCallback functions indicating why the callback
> + * has been called. A mask of these values logically ORed together
> + * is used to specify which events are interesting when the callback
> + * is registered, so these values must all be different bit values.
> + */
> +typedef enum ClockEvent {
> +ClockUpdate = 1, /* Clock period has just updated */
Just wondering loudly (QEMU doesn't have enum naming conventions),
maybe rename ClockUpdate -> ClockUpdateEvent to clarify.
> +} ClockEvent;
> +
> +typedef void ClockCallback(void *opaque, ClockEvent event);
>
> /*
> * clock store a value representing the clock's period in 2^-32ns unit.
> @@ -50,6 +60,7 @@ typedef void ClockCallback(void *opaque);
> * @canonical_path: clock path string cache (used for trace purpose)
> * @callback: called when clock changes
> * @callback_opaque: argument for @callback
> + * @callback_events: mask of events when callback should be called
> * @source: source (or parent in clock tree) of the clock
> * @children: list of clocks connected to this one (it is their source)
> * @sibling: structure used to form a clock list
> @@ -67,6 +78,7 @@ struct Clock {
> char *canonical_path;
> ClockCallback *callback;
> void *callback_opaque;
> +int callback_events;
Isn't "unsigned" recommended for bit mask?
Otherwise (minor Hao Wu's NULL remark):
Reviewed-by: Philippe Mathieu-Daudé
Re: [PATCH v2 1/4] clock: Add ClockEvent parameter to callbacks
On 13:20 Tue 09 Feb , Peter Maydell wrote:
> The Clock framework allows users to specify a callback which is
> called after the clock's period has been updated. Some users need to
> also have a callback which is called before the clock period is
> updated.
>
> As the first step in adding support for notifying Clock users on
> pre-update events, add an argument to the ClockCallback to specify
> what event is being notified, and add an argument to the various
> functions for registering a callback to specify which events are
> of interest to that callback.
>
> Note that the documentation update renders correct the previously
> incorrect claim in 'Adding a new clock' that callbacks "will be
> explained in a following section".
>
> Signed-off-by: Peter Maydell
With Hao's comment addressed:
Reviewed-by: Luc Michel
> ---
> v1->v2: (suggested by Luc) instead of making callback functions check
> whether 'event' is one they are interested in, specify mask of
> interesting events at callback registration time.
> ---
> docs/devel/clocks.rst| 52 +++-
> include/hw/clock.h | 21 +++--
> include/hw/qdev-clock.h | 17 ---
> hw/adc/npcm7xx_adc.c | 2 +-
> hw/arm/armsse.c | 9 +++---
> hw/char/cadence_uart.c | 4 +--
> hw/char/ibex_uart.c | 4 +--
> hw/char/pl011.c | 5 +--
> hw/core/clock.c | 20 +---
> hw/core/qdev-clock.c | 8 +++--
> hw/mips/cps.c| 2 +-
> hw/misc/bcm2835_cprman.c | 23 --
> hw/misc/npcm7xx_clk.c| 26 +---
> hw/misc/npcm7xx_pwm.c| 2 +-
> hw/misc/zynq_slcr.c | 5 +--
> hw/timer/cmsdk-apb-dualtimer.c | 5 +--
> hw/timer/cmsdk-apb-timer.c | 4 +--
> hw/timer/npcm7xx_timer.c | 2 +-
> hw/watchdog/cmsdk-apb-watchdog.c | 5 +--
> target/mips/cpu.c| 2 +-
> 20 files changed, 160 insertions(+), 58 deletions(-)
>
> diff --git a/docs/devel/clocks.rst b/docs/devel/clocks.rst
> index c54bbb82409..cd344e3fe5d 100644
> --- a/docs/devel/clocks.rst
> +++ b/docs/devel/clocks.rst
> @@ -80,11 +80,12 @@ Adding clocks to a device must be done during the init
> method of the Device
> instance.
>
> To add an input clock to a device, the function ``qdev_init_clock_in()``
> -must be used. It takes the name, a callback and an opaque parameter
> -for the callback (this will be explained in a following section).
> +must be used. It takes the name, a callback, an opaque parameter
> +for the callback and a mask of events when the callback should be
> +called (this will be explained in a following section).
> Output is simpler; only the name is required. Typically::
>
> -qdev_init_clock_in(DEVICE(dev), "clk_in", clk_in_callback, dev);
> +qdev_init_clock_in(DEVICE(dev), "clk_in", clk_in_callback, dev,
> ClockUpdate);
> qdev_init_clock_out(DEVICE(dev), "clk_out");
>
> Both functions return the created Clock pointer, which should be saved in the
> @@ -113,7 +114,7 @@ output.
> * callback for the input clock (see "Callback on input clock
> * change" section below for more information).
> */
> -static void clk_in_callback(void *opaque);
> +static void clk_in_callback(void *opaque, ClockEvent event);
>
> /*
> * static array describing clocks:
> @@ -124,7 +125,7 @@ output.
> * the clk_out field of a MyDeviceState structure.
> */
> static const ClockPortInitArray mydev_clocks = {
> -QDEV_CLOCK_IN(MyDeviceState, clk_in, clk_in_callback),
> +QDEV_CLOCK_IN(MyDeviceState, clk_in, clk_in_callback, ClockUpdate),
> QDEV_CLOCK_OUT(MyDeviceState, clk_out),
> QDEV_CLOCK_END
> };
> @@ -153,6 +154,40 @@ nothing else to do. This value will be propagated to
> other clocks when
> connecting the clocks together and devices will fetch the right value during
> the first reset.
>
> +Clock callbacks
> +---
> +
> +You can give a clock a callback function in several ways:
> +
> + * by passing it as an argument to ``qdev_init_clock_in()``
> + * as an argument to the ``QDEV_CLOCK_IN()`` macro initializing an
> + array to be passed to ``qdev_init_clocks()``
> + * by directly calling the ``clock_set_callback()`` function
> +
> +The callback function must be of this type:
> +
> +.. code-block:: c
> +
> + typedef void ClockCallback(void *opaque, ClockEvent event);
> +
> +The ``opaque`` argument is the pointer passed to ``qdev_init_clock_in()``
> +or ``clock_set_callback()``; for ``qdev_init_clocks()`` it is the
> +``dev`` device pointer.
> +
> +The ``event`` argument specifies why the callback has been called.
> +When you register the callback you specify a mask of ClockEvent values
> +that you are interested in. The callback will only be called for those
> +events.
> +
> +The events currently
Re: [PATCH v2 1/4] clock: Add ClockEvent parameter to callbacks
On Wed, 10 Feb 2021 at 20:53, Hao Wu wrote: > > > > On Tue, Feb 9, 2021 at 5:24 AM Peter Maydell wrote: >> diff --git a/hw/adc/npcm7xx_adc.c b/hw/adc/npcm7xx_adc.c >> index 870a6d50c27..573f4876dc6 100644 >> --- a/hw/adc/npcm7xx_adc.c >> +++ b/hw/adc/npcm7xx_adc.c >> @@ -238,7 +238,7 @@ static void npcm7xx_adc_init(Object *obj) >> memory_region_init_io(>iomem, obj, _adc_ops, s, >>TYPE_NPCM7XX_ADC, 4 * KiB); >> sysbus_init_mmio(sbd, >iomem); >> -s->clock = qdev_init_clock_in(DEVICE(s), "clock", NULL, NULL); >> +s->clock = qdev_init_clock_in(DEVICE(s), "clock", NULL, NULL, >> ClockUpdate); > > Since there's no callback here should it be > s->clock = qdev_init_clock_in(DEVICE(s), "clock", NULL, NULL, 0); > ? Yes; thanks for the catch. (The function ignores the events argument if there's no callback function specified, but 0 makes more sense.) -- PMM
Re: [PATCH v2 1/4] clock: Add ClockEvent parameter to callbacks
On Tue, Feb 9, 2021 at 5:24 AM Peter Maydell
wrote:
> The Clock framework allows users to specify a callback which is
> called after the clock's period has been updated. Some users need to
> also have a callback which is called before the clock period is
> updated.
>
> As the first step in adding support for notifying Clock users on
> pre-update events, add an argument to the ClockCallback to specify
> what event is being notified, and add an argument to the various
> functions for registering a callback to specify which events are
> of interest to that callback.
>
> Note that the documentation update renders correct the previously
> incorrect claim in 'Adding a new clock' that callbacks "will be
> explained in a following section".
>
> Signed-off-by: Peter Maydell
> ---
> v1->v2: (suggested by Luc) instead of making callback functions check
> whether 'event' is one they are interested in, specify mask of
> interesting events at callback registration time.
> ---
> docs/devel/clocks.rst| 52 +++-
> include/hw/clock.h | 21 +++--
> include/hw/qdev-clock.h | 17 ---
> hw/adc/npcm7xx_adc.c | 2 +-
> hw/arm/armsse.c | 9 +++---
> hw/char/cadence_uart.c | 4 +--
> hw/char/ibex_uart.c | 4 +--
> hw/char/pl011.c | 5 +--
> hw/core/clock.c | 20 +---
> hw/core/qdev-clock.c | 8 +++--
> hw/mips/cps.c| 2 +-
> hw/misc/bcm2835_cprman.c | 23 --
> hw/misc/npcm7xx_clk.c| 26 +---
> hw/misc/npcm7xx_pwm.c| 2 +-
> hw/misc/zynq_slcr.c | 5 +--
> hw/timer/cmsdk-apb-dualtimer.c | 5 +--
> hw/timer/cmsdk-apb-timer.c | 4 +--
> hw/timer/npcm7xx_timer.c | 2 +-
> hw/watchdog/cmsdk-apb-watchdog.c | 5 +--
> target/mips/cpu.c| 2 +-
> 20 files changed, 160 insertions(+), 58 deletions(-)
>
> diff --git a/docs/devel/clocks.rst b/docs/devel/clocks.rst
> index c54bbb82409..cd344e3fe5d 100644
> --- a/docs/devel/clocks.rst
> +++ b/docs/devel/clocks.rst
> @@ -80,11 +80,12 @@ Adding clocks to a device must be done during the init
> method of the Device
> instance.
>
> To add an input clock to a device, the function ``qdev_init_clock_in()``
> -must be used. It takes the name, a callback and an opaque parameter
> -for the callback (this will be explained in a following section).
> +must be used. It takes the name, a callback, an opaque parameter
> +for the callback and a mask of events when the callback should be
> +called (this will be explained in a following section).
> Output is simpler; only the name is required. Typically::
>
> -qdev_init_clock_in(DEVICE(dev), "clk_in", clk_in_callback, dev);
> +qdev_init_clock_in(DEVICE(dev), "clk_in", clk_in_callback, dev,
> ClockUpdate);
> qdev_init_clock_out(DEVICE(dev), "clk_out");
>
> Both functions return the created Clock pointer, which should be saved in
> the
> @@ -113,7 +114,7 @@ output.
> * callback for the input clock (see "Callback on input clock
> * change" section below for more information).
> */
> -static void clk_in_callback(void *opaque);
> +static void clk_in_callback(void *opaque, ClockEvent event);
>
> /*
> * static array describing clocks:
> @@ -124,7 +125,7 @@ output.
> * the clk_out field of a MyDeviceState structure.
> */
> static const ClockPortInitArray mydev_clocks = {
> -QDEV_CLOCK_IN(MyDeviceState, clk_in, clk_in_callback),
> +QDEV_CLOCK_IN(MyDeviceState, clk_in, clk_in_callback,
> ClockUpdate),
> QDEV_CLOCK_OUT(MyDeviceState, clk_out),
> QDEV_CLOCK_END
> };
> @@ -153,6 +154,40 @@ nothing else to do. This value will be propagated to
> other clocks when
> connecting the clocks together and devices will fetch the right value
> during
> the first reset.
>
> +Clock callbacks
> +---
> +
> +You can give a clock a callback function in several ways:
> +
> + * by passing it as an argument to ``qdev_init_clock_in()``
> + * as an argument to the ``QDEV_CLOCK_IN()`` macro initializing an
> + array to be passed to ``qdev_init_clocks()``
> + * by directly calling the ``clock_set_callback()`` function
> +
> +The callback function must be of this type:
> +
> +.. code-block:: c
> +
> + typedef void ClockCallback(void *opaque, ClockEvent event);
> +
> +The ``opaque`` argument is the pointer passed to ``qdev_init_clock_in()``
> +or ``clock_set_callback()``; for ``qdev_init_clocks()`` it is the
> +``dev`` device pointer.
> +
> +The ``event`` argument specifies why the callback has been called.
> +When you register the callback you specify a mask of ClockEvent values
> +that you are interested in. The callback will only be called for those
> +events.
> +
> +The events currently supported are:
> +
> + * ``ClockUpdate`` : called after the
