Re: [PATCH v3 3/4] hw/misc: Add MIPS Trickbox device

[Thomas Huth <thuth@redhat.com>]

On 08/03/2023 01.07, Philippe Mathieu-Daudé wrote:
> From: Jiaxun Yang <jiaxun.yang@flygoat.com>
> 
> MIPS Trickbox is a emulated device present in MIPS's IASIM simulator
> for decades. It's capable of managing simulator status, signaling
> interrupts, doing DMA and EJTAG signal stimulations.
> 
> For now we just use definition of this device and implement power
> management related functions.
> 
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> Message-Id: <20230304223803.55764-2-jiaxun.yang@flygoat.com>
> [PMD: Remove pointless mask in mips_trickbox_write(),
>        declare QOM macros using OBJECT_DECLARE_SIMPLE_TYPE()]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/misc/Kconfig                 |  3 +
>   hw/misc/meson.build             |  1 +
>   hw/misc/mips_trickbox.c         | 97 +++++++++++++++++++++++++++++++++
>   hw/misc/trace-events            |  4 ++
>   include/hw/misc/mips_trickbox.h | 39 +++++++++++++
>   5 files changed, 144 insertions(+)
>   create mode 100644 hw/misc/mips_trickbox.c
>   create mode 100644 include/hw/misc/mips_trickbox.h
> 
> diff --git a/hw/misc/Kconfig b/hw/misc/Kconfig
> index 2ef5781ef8..9f09da23c1 100644
> --- a/hw/misc/Kconfig
> +++ b/hw/misc/Kconfig
> @@ -85,6 +85,9 @@ config STM32F4XX_EXTI
>   config MIPS_ITU
>       bool
>   
> +config MIPS_TRICKBOX
> +    bool
> +
>   config MPS2_FPGAIO
>       bool
>       select LED
> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> index a40245ad44..4b6c50832c 100644
> --- a/hw/misc/meson.build
> +++ b/hw/misc/meson.build
> @@ -136,6 +136,7 @@ specific_ss.add(when: 'CONFIG_MAC_VIA', if_true: files('mac_via.c'))
>   
>   specific_ss.add(when: 'CONFIG_MIPS_CPS', if_true: files('mips_cmgcr.c', 'mips_cpc.c'))
>   specific_ss.add(when: 'CONFIG_MIPS_ITU', if_true: files('mips_itu.c'))
> +specific_ss.add(when: 'CONFIG_MIPS_TRICKBOX', if_true: files('mips_trickbox.c'))
>   
>   softmmu_ss.add(when: 'CONFIG_SBSA_REF', if_true: files('sbsa_ec.c'))
>   
> diff --git a/hw/misc/mips_trickbox.c b/hw/misc/mips_trickbox.c
> new file mode 100644
> index 0000000000..86b00a8c0d
> --- /dev/null
> +++ b/hw/misc/mips_trickbox.c
> @@ -0,0 +1,97 @@
> +/*
> + * SPDX-License-Identifier: LGPL-2.0-or-later
> + *
> + * MIPS Trickbox

I'd maybe add the description from the commit message here to tell the one 
who's looking at this source code what this file is all about.

> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/sysbus.h"
> +#include "qapi/error.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "trace.h"
> +#include "sysemu/runstate.h"
> +#include "hw/misc/mips_trickbox.h"
> +
> +static uint64_t mips_trickbox_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> +    uint64_t value = 0;
> +
> +    qemu_log_mask(LOG_UNIMP,
> +                    "%s: unimplemented register read 0x%02"HWADDR_PRIx"\n",
> +                    __func__, addr);
> +    trace_mips_trickbox_read(size, value);

Does it really make sense to have both, a trace point and a log for the same 
thing?

  Thomas