Hi Philippe, On Sun, Dec 4, 2022 at 10:39 PM Philippe Mathieu-Daudé <phi...@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.janko...@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.janko...@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.