Re: [PATCH 4/6] hw/misc: Allwinner AXP-209 Emulation

[Philippe Mathieu-Daudé]

Hi Strahinja,

On 4/12/22 00:19, Strahinja Jankovic wrote:
> This patch adds minimal support for AXP-209 PMU.
> Most important is chip ID since U-Boot SPL expects version 0x1. Besides
> the chip ID register, reset values for two more registers used by A10
> U-Boot SPL are covered.
>
> Signed-off-by: Strahinja Jankovic <strahinja.p.jankovic@gmail.com>
> ---
>   hw/arm/Kconfig              |   1 +
>   hw/misc/Kconfig             |   4 +
>   hw/misc/allwinner-axp-209.c | 263 ++++++++++++++++++++++++++++++++++++
>   hw/misc/meson.build         |   1 +
>   4 files changed, 269 insertions(+)
>   create mode 100644 hw/misc/allwinner-axp-209.c


> diff --git a/hw/misc/allwinner-axp-209.c b/hw/misc/allwinner-axp-209.c
> new file mode 100644
> index 0000000000..229e3961b6
> --- /dev/null
> +++ b/hw/misc/allwinner-axp-209.c
> @@ -0,0 +1,263 @@
> +/*
> + * AXP-209 Emulation
> + *
> + * Written by Strahinja Jankovic <strahinja.p.jankovic@gmail.com>
> + *

You missed the "Copyright (c) <year> <copyright holders>" line.

> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.

If you mind, please also include:

    * SPDX-License-Identifier: MIT

> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "hw/i2c/i2c.h"
> +#include "migration/vmstate.h"
> +
> +#ifndef AXP_209_ERR_DEBUG
> +#define AXP_209_ERR_DEBUG 0
> +#endif
> +
> +#define TYPE_AXP_209 "allwinner.axp209"
> +
> +#define AXP_209(obj) \
> +    OBJECT_CHECK(AXP209I2CState, (obj), TYPE_AXP_209)
> +
> +#define DB_PRINT(fmt, args...) do { \
> +    if (AXP_209_ERR_DEBUG) { \
> +        fprintf(stderr, "%s: " fmt, __func__, ## args); \

Please replace the DB_PRINT() calls by trace events which are more
powerful: when a tracing backend is present, the events are built
in and you can individually enable them at runtime.

> +    } \
> +} while (0)


> +#define AXP_209_CHIP_VERSION_ID             (0x01)
> +#define AXP_209_DC_DC2_OUT_V_CTRL_RESET     (0x16)
> +#define AXP_209_IRQ_BANK_1_CTRL_RESET       (0xd8)


> +/* Reset all counters and load ID register */
> +static void axp_209_reset_enter(Object *obj, ResetType type)
> +{
> +    AXP209I2CState *s = AXP_209(obj);
> +
> +    memset(s->regs, 0, NR_REGS);
> +    s->ptr = 0;
> +    s->count = 0;
> +    s->regs[REG_CHIP_VERSION] = AXP_209_CHIP_VERSION_ID;
> +    s->regs[REG_DC_DC2_OUT_V_CTRL] = AXP_209_DC_DC2_OUT_V_CTRL_RESET;
> +    s->regs[REG_IRQ_BANK_1_CTRL] = AXP_209_IRQ_BANK_1_CTRL_RESET;
> +}


> +/* Initialization */
> +static void axp_209_init(Object *obj)
> +{
> +    AXP209I2CState *s = AXP_209(obj);
> +
> +    s->count = 0;
> +    s->ptr = 0;
> +    memset(s->regs, 0, NR_REGS);
> +    s->regs[REG_CHIP_VERSION] = AXP_209_CHIP_VERSION_ID;
> +    s->regs[REG_DC_DC2_OUT_V_CTRL] = 0x16;
> +    s->regs[REG_IRQ_BANK_1_CTRL] = 0xd8;

The device initialization flow is:

 - init()
 - realize()
 - reset()

So these values are already set in axp_209_reset_enter().

Besides, you should use the definition you added instead of
magic values (AXP_209_DC_DC2_OUT_V_CTRL_RESET and
AXP_209_IRQ_BANK_1_CTRL_RESET).

> +
> +    DB_PRINT("INIT AXP209\n");
> +
> +    return;
> +}

Otherwise LGTM!

Thanks,

Phil.