Re: [PATCH v4 36/47] hw/misc/xlnx-versal-crl: add the versal2 version

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 22/8/25 17:16, Luc Michel wrote:
> Add the versal2 version of the CRL device. For the implemented part, it
> is similar to the versal version but drives reset line of more devices.
> 
> Signed-off-by: Luc Michel <luc.michel@amd.com>
> Reviewed-by: Francisco Iglesias <francisco.iglesias@amd.com>
> ---
>   include/hw/arm/xlnx-versal-version.h |   1 +
>   include/hw/misc/xlnx-versal-crl.h    | 329 ++++++++++++++++++++++
>   hw/misc/xlnx-versal-crl.c            | 392 +++++++++++++++++++++++++++
>   3 files changed, 722 insertions(+)


> +static DeviceState **versal2_decode_periph_rst(XlnxVersalCRLBase *s,
> +                                               hwaddr addr, size_t *count)
> +{
> +    size_t idx;
> +    XlnxVersal2CRL *xvc = XLNX_VERSAL2_CRL(s);
> +
> +    *count = 1;
> +
> +    switch (addr) {
> +    case A_VERSAL2_RST_RPU_A ... A_VERSAL2_RST_RPU_E:
> +        idx = (addr - A_VERSAL2_RST_RPU_A) / sizeof(uint32_t);
> +        idx *= 2; /* two RPUs per RST_RPU_x registers */
> +        return xvc->cfg.rpu + idx;
> +
> +    case A_VERSAL2_RST_ADMA:
> +        /* A single register fans out to all DMA reset inputs */
> +        *count = ARRAY_SIZE(xvc->cfg.adma);
> +        return xvc->cfg.adma;
> +
> +    case A_VERSAL2_RST_SDMA:
> +        *count = ARRAY_SIZE(xvc->cfg.sdma);
> +        return xvc->cfg.sdma;
> +
> +    case A_VERSAL2_RST_UART0 ... A_VERSAL2_RST_UART1:
> +        idx = (addr - A_VERSAL2_RST_UART0) / sizeof(uint32_t);
> +        return xvc->cfg.uart + idx;
> +
> +    case A_VERSAL2_RST_GEM0 ... A_VERSAL2_RST_GEM1:
> +        idx = (addr - A_VERSAL2_RST_GEM0) / sizeof(uint32_t);
> +        return xvc->cfg.gem + idx;
> +
> +    case A_VERSAL2_RST_USB0 ... A_VERSAL2_RST_USB1:
> +        idx = (addr - A_VERSAL2_RST_USB0) / sizeof(uint32_t);
> +        return xvc->cfg.usb + idx;
> +
> +    case A_VERSAL2_RST_CAN0 ... A_VERSAL2_RST_CAN3:
> +        idx = (addr - A_VERSAL2_RST_CAN0) / sizeof(uint32_t);
> +        return xvc->cfg.can + idx;
> +
> +    default:
> +        /* invalid or unimplemented */
 > +        return NULL;

Can that happen?

Note count=1 when returning. Should we set to 0?

> +    }
> +}

Otherwise,

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>