Re: [Qemu-devel] [PATCH v3 07/12] i.MX: Add i.MX6 System Reset Controller device.

[Peter Maydell]

On 2 March 2016 at 05:27, Jean-Christophe Dubois <address@hidden> wrote:
> This controller is also present in i.MX5X devices but they are not
> yet emulated by QEMU.
>
> Signed-off-by: Jean-Christophe Dubois <address@hidden>
> ---

> +static void imx6_src_reset(DeviceState *dev)
> +{
> +    IMX6SRCState *s = IMX6_SRC(dev);
> +
> +    DPRINTF("\n");
> +
> +    memset(s->regs, 0, SRC_MAX * sizeof(uint32_t));

   memset(s->regs, 0, sizeof(s->regs));

is slightly better.

> +
> +    /* Set reset values */
> +    s->regs[SRC_SCR] = 0x521;
> +    s->regs[SRC_SRSR] = 0x1;
> +    s->regs[SRC_SIMR] = 0x1F;
> +}
> +
> +static uint64_t imx6_src_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +    uint32_t value = 0;
> +    IMX6SRCState *s = (IMX6SRCState *)opaque;
> +    uint32_t index = offset >> 2;
> +
> +    if (index < SRC_MAX) {
> +        value = s->regs[index];
> +    } else {
> +        qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad register at offset 0x%"
> +                      HWADDR_PRIx "\n", TYPE_IMX6_SRC, __func__, offset);
> +
> +    }
> +
> +    DPRINTF("reg[%s] => 0x%" PRIx32 "\n", imx6_src_reg_name(index), value);
> +
> +    return (uint64_t)value;

This cast is unnecessary.

> +}
> +
> +static void imx6_src_write(void *opaque, hwaddr offset, uint64_t value,
> +                           unsigned size)
> +{
> +    IMX6SRCState *s = (IMX6SRCState *)opaque;
> +    uint32_t index = offset >> 2;
> +    uint64_t change_mask;
> +
> +    if (index >=  SRC_MAX) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad register at offset 0x%"
> +                      HWADDR_PRIx "\n", TYPE_IMX6_SRC, __func__, offset);
> +        return;
> +    }
> +
> +    DPRINTF("reg[%s] <= 0x%" PRIx32 "\n", imx6_src_reg_name(index),
> +            (uint32_t)value);
> +
> +    change_mask = s->regs[index] ^ (uint32_t)value;

s->regs[] is 32 bit, and you've explicitly cast value to 32-bits,
so why is change_mask 64 bits?

> +        if (EXTRACT(change_mask, CORE3_RST)) {
> +            arm_reset_cpu(3);
> +            clear_bit(CORE3_RST_SHIFT, &value);
> +        }
> +        if (EXTRACT(change_mask, SW_IPU2_RST)) {
> +            /* We pretend the IPU2 is reseted */

"reset" (and below).

> +            clear_bit(SW_IPU2_RST_SHIFT, &value);
> +        }
> +        if (EXTRACT(change_mask, SW_IPU1_RST)) {
> +            /* We pretend the IPU1 is reseted */
> +            clear_bit(SW_IPU1_RST_SHIFT, &value);
> +        }
> +        s->regs[index] = value;
> +        break;

Otherwise
Reviewed-by: Peter Maydell <address@hidden>

thanks
-- PMM