Re: [PATCH 09/16] hw/i3c/aspeed_i3c: Add data TX and RX

[Jeremy Kerr <jk@codeconstruct.com.au>]

Hi Joe,

> +static uint8_t aspeed_i3c_device_target_addr(AspeedI3CDevice *s,
> +                                             uint16_t offset)
> +{
> +    if (offset > ASPEED_I3C_NR_DEVICES) {
> +        qemu_log_mask(LOG_GUEST_ERROR, "%s: Device addr table offset %d out of "
> +                      "bounds\n", object_get_canonical_path(OBJECT(s)), offset);
> +        /* If we're out of bounds, return an address of 0. */
> +        return 0;
> +    }
> +
> +    uint16_t dev_index = R_DEVICE_ADDR_TABLE_LOC1 + offset;
> +    /* I2C devices use a static address. */
> +    if (aspeed_i3c_device_target_is_i2c(s, offset)) {
> +        return FIELD_EX32(s->regs[dev_index], DEVICE_ADDR_TABLE_LOC1,
> +                          DEV_STATIC_ADDR);
> +    }
> +    return FIELD_EX32(s->regs[dev_index], DEVICE_ADDR_TABLE_LOC1,
> +                      DEV_DYNAMIC_ADDR);
> +}

Depending on the usage of this function, you'll probably want to mask
out the parity bit (the msb) from the DEV_DYNAMIC_ADDR field.

Currently, you're returning this value directly from the ENTDAA handler,
which is ending up assigning addresses with the high bit set.

(doing a bit of a piecemeal review here, as I'm testing out the
model...)

Cheers,


Jeremy