[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 4/6] hw/misc: Allwinner AXP-209 Emulation
From: |
Strahinja Jankovic |
Subject: |
Re: [PATCH 4/6] hw/misc: Allwinner AXP-209 Emulation |
Date: |
Mon, 5 Dec 2022 22:07:14 +0100 |
Hi Philippe,
On Sun, Dec 4, 2022 at 10:39 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> 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.
Ok, I will add it.
>
> > + * 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
Ok, I will add it.
>
> > + */
> > +
> > +#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.
I will do my best to update this to trace events. Have not used them
before, but I will look at other places in code and docs.
>
> > + } \
> > +} 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().
Thanks, that makes perfect sense. I will update .init and .reset
functions accordingly in v2 of the patch.
>
> 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).
Yes, that was an oversight. I used the macros in .reset, but I forgot
to update them in .init.
>
> > +
> > + DB_PRINT("INIT AXP209\n");
> > +
> > + return;
> > +}
>
> Otherwise LGTM!
Thanks!
Best regards,
Strahinja
>
> Thanks,
>
> Phil.
- Re: [PATCH 6/6] hw/arm: Allwinner A10 enable SPL load from MMC, (continued)
- [PATCH 2/6] hw/misc: Allwinner A10 DRAM Controller Emulation, Strahinja Jankovic, 2022/12/03
- [PATCH 3/6] hw/i2c: Allwinner TWI/I2C Emulation, Strahinja Jankovic, 2022/12/03
- [PATCH 5/6] hw/arm: Add AXP-209 to Cubieboard, Strahinja Jankovic, 2022/12/03
- [PATCH 4/6] hw/misc: Allwinner AXP-209 Emulation, Strahinja Jankovic, 2022/12/03
- Re: [PATCH 0/6] Enable Cubieboard A10 boot SPL from SD card, Niek Linnenbank, 2022/12/07