Hi Luc,
On Wed, Jul 16, 2025 at 11:54:15AM +0200, Luc Michel wrote:
> Refactor the device reset logic to have a common register write callback
> for all the devices. This uses a decode function to map the register
> address to the actual peripheral to reset. This refactoring changes the
> CPU property name from cpu_r5[*] to rpu[*] to ease with the connections
> in the Versal SoC. It also fixes a bug where the gem device pointer
> was mapped to the usb link property.
>
> Signed-off-by: Luc Michel <luc.mic...@amd.com>
> ---
> include/hw/misc/xlnx-versal-crl.h | 8 +-
> hw/misc/xlnx-versal-crl.c | 163 ++++++++++++++++--------------
> 2 files changed, 92 insertions(+), 79 deletions(-)
>
> diff --git a/include/hw/misc/xlnx-versal-crl.h
> b/include/hw/misc/xlnx-versal-crl.h
> index 2b39d203a67..7e50a95ad3c 100644
> --- a/include/hw/misc/xlnx-versal-crl.h
> +++ b/include/hw/misc/xlnx-versal-crl.h
> @@ -218,33 +218,33 @@ REG32(PSM_RST_MODE, 0x370)
> FIELD(PSM_RST_MODE, WAKEUP, 2, 1)
> FIELD(PSM_RST_MODE, RST_MODE, 0, 2)
>
> #define CRL_R_MAX (R_PSM_RST_MODE + 1)
>
> -#define RPU_MAX_CPU 2
> -
> struct XlnxVersalCRLBase {
> SysBusDevice parent_obj;
>
> RegisterInfoArray *reg_array;
> uint32_t *regs;
> };
>
> struct XlnxVersalCRLBaseClass {
> SysBusDeviceClass parent_class;
> +
> + DeviceState ** (*decode_periph_rst)(XlnxVersalCRLBase *s, hwaddr, size_t
> *);
> };
>
> struct XlnxVersalCRL {
> XlnxVersalCRLBase parent_obj;
> qemu_irq irq;
>
> struct {
> - ARMCPU *cpu_r5[RPU_MAX_CPU];
> + DeviceState *rpu[2];
> DeviceState *adma[8];
> DeviceState *uart[2];
> DeviceState *gem[2];
> - DeviceState *usb;
> + DeviceState *usb[1];
> } cfg;
>
> uint32_t regs[CRL_R_MAX];
> RegisterInfo regs_info[CRL_R_MAX];
> };
> diff --git a/hw/misc/xlnx-versal-crl.c b/hw/misc/xlnx-versal-crl.c
> index be89e0da40d..115327cfcf4 100644
> --- a/hw/misc/xlnx-versal-crl.c
> +++ b/hw/misc/xlnx-versal-crl.c
> @@ -53,94 +53,103 @@ static uint64_t crl_disable_prew(RegisterInfo *reg,
> uint64_t val64)
> s->regs[R_IR_MASK] |= val;
> crl_update_irq(s);
> return 0;
> }
>
> -static void crl_reset_dev(XlnxVersalCRL *s, DeviceState *dev,
> - bool rst_old, bool rst_new)
> +static DeviceState **versal_decode_periph_rst(XlnxVersalCRLBase *s,
> + hwaddr addr, size_t *count)
One space is missing above but otherwise:
Reviewed-by: Francisco Iglesias <francisco.igles...@amd.com>
> {
> - device_cold_reset(dev);
> -}
> + size_t idx;
> + XlnxVersalCRL *xvc = XLNX_VERSAL_CRL(s);
>
> -static void crl_reset_cpu(XlnxVersalCRL *s, ARMCPU *armcpu,
> - bool rst_old, bool rst_new)
> -{
> - if (rst_new) {
> - arm_set_cpu_off(arm_cpu_mp_affinity(armcpu));
> - } else {
> - arm_set_cpu_on_and_reset(arm_cpu_mp_affinity(armcpu));
> - }
> -}
> + *count = 1;
>
> -#define REGFIELD_RESET(type, s, reg, f, new_val, dev) { \
> - bool old_f = ARRAY_FIELD_EX32((s)->regs, reg, f); \
> - bool new_f = FIELD_EX32(new_val, reg, f); \
> - \
> - /* Detect edges. */ \
> - if (dev && old_f != new_f) { \
> - crl_reset_ ## type(s, dev, old_f, new_f); \
> - } \
> -}
> + switch (addr) {
> + case A_RST_CPU_R5:
> + return xvc->cfg.rpu;
>
> -static uint64_t crl_rst_r5_prew(RegisterInfo *reg, uint64_t val64)
> -{
> - XlnxVersalCRL *s = XLNX_VERSAL_CRL(reg->opaque);
> + case A_RST_ADMA:
> + /* A single register fans out to all DMA reset inputs */
> + *count = ARRAY_SIZE(xvc->cfg.adma);
> + return xvc->cfg.adma;
>
> - REGFIELD_RESET(cpu, s, RST_CPU_R5, RESET_CPU0, val64, s->cfg.cpu_r5[0]);
> - REGFIELD_RESET(cpu, s, RST_CPU_R5, RESET_CPU1, val64, s->cfg.cpu_r5[1]);
> - return val64;
> -}
> + case A_RST_UART0 ... A_RST_UART1:
> + idx = (addr - A_RST_UART0) / sizeof(uint32_t);
> + return xvc->cfg.uart + idx;
>
> -static uint64_t crl_rst_adma_prew(RegisterInfo *reg, uint64_t val64)
> -{
> - XlnxVersalCRL *s = XLNX_VERSAL_CRL(reg->opaque);
> - int i;
> + case A_RST_GEM0 ... A_RST_GEM1:
> + idx = (addr - A_RST_GEM0) / sizeof(uint32_t);
> + return xvc->cfg.gem + idx;
> +
> + case A_RST_USB0:
> + return xvc->cfg.usb;
>
> - /* A single register fans out to all ADMA reset inputs. */
> - for (i = 0; i < ARRAY_SIZE(s->cfg.adma); i++) {
> - REGFIELD_RESET(dev, s, RST_ADMA, RESET, val64, s->cfg.adma[i]);
> + default:
> + /* invalid or unimplemented */
> + return NULL;
> }
> - return val64;
> }
>
> -static uint64_t crl_rst_uart0_prew(RegisterInfo *reg, uint64_t val64)
> +static uint64_t crl_rst_cpu_prew(RegisterInfo *reg, uint64_t val64)
> {
> - XlnxVersalCRL *s = XLNX_VERSAL_CRL(reg->opaque);
> + XlnxVersalCRLBase *s = XLNX_VERSAL_CRL_BASE(reg->opaque);
> + XlnxVersalCRLBaseClass *xvcbc = XLNX_VERSAL_CRL_BASE_GET_CLASS(s);
> + DeviceState **dev;
> + size_t i, count;
>
> - REGFIELD_RESET(dev, s, RST_UART0, RESET, val64, s->cfg.uart[0]);
> - return val64;
> -}
> + dev = xvcbc->decode_periph_rst(s, reg->access->addr, &count);
>
> -static uint64_t crl_rst_uart1_prew(RegisterInfo *reg, uint64_t val64)
> -{
> - XlnxVersalCRL *s = XLNX_VERSAL_CRL(reg->opaque);
> + for (i = 0; i < 2; i++) {
> + bool prev, new;
> + uint64_t aff;
>
> - REGFIELD_RESET(dev, s, RST_UART1, RESET, val64, s->cfg.uart[1]);
> - return val64;
> -}
> + prev = extract32(s->regs[reg->access->addr / 4], i, 1);
> + new = extract32(val64, i, 1);
>
> -static uint64_t crl_rst_gem0_prew(RegisterInfo *reg, uint64_t val64)
> -{
> - XlnxVersalCRL *s = XLNX_VERSAL_CRL(reg->opaque);
> + if (prev == new) {
> + continue;
> + }
>
> - REGFIELD_RESET(dev, s, RST_GEM0, RESET, val64, s->cfg.gem[0]);
> - return val64;
> -}
> + aff = arm_cpu_mp_affinity(ARM_CPU(dev[i]));
>
> -static uint64_t crl_rst_gem1_prew(RegisterInfo *reg, uint64_t val64)
> -{
> - XlnxVersalCRL *s = XLNX_VERSAL_CRL(reg->opaque);
> + if (new) {
> + arm_set_cpu_off(aff);
> + } else {
> + arm_set_cpu_on_and_reset(aff);
> + }
> + }
>
> - REGFIELD_RESET(dev, s, RST_GEM1, RESET, val64, s->cfg.gem[1]);
> return val64;
> }
>
> -static uint64_t crl_rst_usb_prew(RegisterInfo *reg, uint64_t val64)
> +static uint64_t crl_rst_dev_prew(RegisterInfo *reg, uint64_t val64)
> {
> - XlnxVersalCRL *s = XLNX_VERSAL_CRL(reg->opaque);
> + XlnxVersalCRLBase *s = XLNX_VERSAL_CRL_BASE(reg->opaque);
> + XlnxVersalCRLBaseClass *xvcbc = XLNX_VERSAL_CRL_BASE_GET_CLASS(s);
> + DeviceState **dev;
> + bool prev, new;
> + size_t i, count;
> +
> + dev = xvcbc->decode_periph_rst(s, reg->access->addr, &count);
> +
> + if (dev == NULL) {
> + return val64;
> + }
> +
> + prev = s->regs[reg->access->addr / 4] & 0x1;
> + new = val64 & 0x1;
> +
> + if (prev == new) {
> + return val64;
> + }
> +
> + for (i = 0; i < count; i++) {
> + if (dev[i]) {
> + device_cold_reset(dev[i]);
> + }
> + }
>
> - REGFIELD_RESET(dev, s, RST_USB0, RESET, val64, s->cfg.usb);
> return val64;
> }
>
> static const RegisterAccessInfo crl_regs_info[] = {
> { .name = "ERR_CTRL", .addr = A_ERR_CTRL,
> @@ -242,31 +251,31 @@ static const RegisterAccessInfo crl_regs_info[] = {
> .reset = 0x3c00,
> .rsvd = 0xfdfc00f8,
> },{ .name = "RST_CPU_R5", .addr = A_RST_CPU_R5,
> .reset = 0x17,
> .rsvd = 0x8,
> - .pre_write = crl_rst_r5_prew,
> + .pre_write = crl_rst_cpu_prew,
> },{ .name = "RST_ADMA", .addr = A_RST_ADMA,
> .reset = 0x1,
> - .pre_write = crl_rst_adma_prew,
> + .pre_write = crl_rst_dev_prew,
> },{ .name = "RST_GEM0", .addr = A_RST_GEM0,
> .reset = 0x1,
> - .pre_write = crl_rst_gem0_prew,
> + .pre_write = crl_rst_dev_prew,
> },{ .name = "RST_GEM1", .addr = A_RST_GEM1,
> .reset = 0x1,
> - .pre_write = crl_rst_gem1_prew,
> + .pre_write = crl_rst_dev_prew,
> },{ .name = "RST_SPARE", .addr = A_RST_SPARE,
> .reset = 0x1,
> },{ .name = "RST_USB0", .addr = A_RST_USB0,
> .reset = 0x1,
> - .pre_write = crl_rst_usb_prew,
> + .pre_write = crl_rst_dev_prew,
> },{ .name = "RST_UART0", .addr = A_RST_UART0,
> .reset = 0x1,
> - .pre_write = crl_rst_uart0_prew,
> + .pre_write = crl_rst_dev_prew,
> },{ .name = "RST_UART1", .addr = A_RST_UART1,
> .reset = 0x1,
> - .pre_write = crl_rst_uart1_prew,
> + .pre_write = crl_rst_dev_prew,
> },{ .name = "RST_SPI0", .addr = A_RST_SPI0,
> .reset = 0x1,
> },{ .name = "RST_SPI1", .addr = A_RST_SPI1,
> .reset = 0x1,
> },{ .name = "RST_CAN0", .addr = A_RST_CAN0,
> @@ -341,13 +350,13 @@ static void versal_crl_init(Object *obj)
> CRL_R_MAX * 4);
> xvcb->regs = s->regs;
> sysbus_init_mmio(sbd, &xvcb->reg_array->mem);
> sysbus_init_irq(sbd, &s->irq);
>
> - for (i = 0; i < ARRAY_SIZE(s->cfg.cpu_r5); ++i) {
> - object_property_add_link(obj, "cpu_r5[*]", TYPE_ARM_CPU,
> - (Object **)&s->cfg.cpu_r5[i],
> + for (i = 0; i < ARRAY_SIZE(s->cfg.rpu); ++i) {
> + object_property_add_link(obj, "rpu[*]", TYPE_ARM_CPU,
> + (Object **)&s->cfg.rpu[i],
> qdev_prop_allow_set_link_before_realize,
> OBJ_PROP_LINK_STRONG);
> }
>
> for (i = 0; i < ARRAY_SIZE(s->cfg.adma); ++i) {
> @@ -369,14 +378,16 @@ static void versal_crl_init(Object *obj)
> (Object **)&s->cfg.gem[i],
> qdev_prop_allow_set_link_before_realize,
> OBJ_PROP_LINK_STRONG);
> }
>
> - object_property_add_link(obj, "usb", TYPE_DEVICE,
> - (Object **)&s->cfg.gem[i],
> - qdev_prop_allow_set_link_before_realize,
> - OBJ_PROP_LINK_STRONG);
> + for (i = 0; i < ARRAY_SIZE(s->cfg.usb); ++i) {
> + object_property_add_link(obj, "usb[*]", TYPE_DEVICE,
> + (Object **)&s->cfg.usb[i],
> + qdev_prop_allow_set_link_before_realize,
> + OBJ_PROP_LINK_STRONG);
> + }
> }
>
> static void crl_finalize(Object *obj)
> {
> XlnxVersalCRLBase *s = XLNX_VERSAL_CRL_BASE(obj);
> @@ -394,15 +405,17 @@ static const VMStateDescription vmstate_versal_crl = {
> };
>
> static void versal_crl_class_init(ObjectClass *klass, const void *data)
> {
> DeviceClass *dc = DEVICE_CLASS(klass);
> + XlnxVersalCRLBaseClass *xvcc = XLNX_VERSAL_CRL_BASE_CLASS(klass);
> ResettableClass *rc = RESETTABLE_CLASS(klass);
>
> dc->vmsd = &vmstate_versal_crl;
> rc->phases.enter = versal_crl_reset_enter;
> rc->phases.hold = versal_crl_reset_hold;
> + xvcc->decode_periph_rst = versal_decode_periph_rst;
> }
>
> static const TypeInfo crl_base_info = {
> .name = TYPE_XLNX_VERSAL_CRL_BASE,
> .parent = TYPE_SYS_BUS_DEVICE,
> --
> 2.50.0
>
