Re: [PATCH 3/4] [RFC] hw/i2c/designware_i2c: Switch to QEMU register API

[Nicholas Piggin <npiggin@gmail.com>]

On Wed, May 13, 2026 at 12:03:17PM +1000, Alistair Francis wrote:
> On Thu, May 7, 2026 at 10:07 PM Nicholas Piggin <npiggin@gmail.com> wrote:

[...]

> > +REG32(DW_IC_COMP_PARAM_1,       0xf4) /* Component parameter */
> > +    FIELD(DW_IC_COMP_PARAM_1, TX_FIFO_SIZE,       16, 8)
> > +    FIELD(DW_IC_COMP_PARAM_1, RX_FIFO_SIZE,        8, 8)
> > +    FIELD(DW_IC_COMP_PARAM_1, HAS_ENCODED_PARAMS,  7, 1)
> > +    FIELD(DW_IC_COMP_PARAM_1, HAS_DMA,             6, 1)
> > +    FIELD(DW_IC_COMP_PARAM_1, INTR_IO,             5, 1)
> > +    FIELD(DW_IC_COMP_PARAM_1, HC_COUNT_VAL,        4, 1)
> > +    FIELD(DW_IC_COMP_PARAM_1, HIGH_SPEED_MODE,     2, 2)
> > +    FIELD(DW_IC_COMP_PARAM_1, APB_DATA_WIDTH_32,   0, 2)
> > +REG32(DW_IC_COMP_VERSION,       0xf8) /* I2C component version */
> > +REG32(DW_IC_COMP_TYPE,          0xfc) /* I2C component type */
> 
> This looks fine to me, again no datasheet so no idea if it's right though :)
> 
> For future reference, generally in QEMU when people say the Register
> API they just mean this maco part above

Ah, okay!

> >  static void dw_i2c_update_irq(DesignWareI2CState *s)
> >  {
> > -    int level;
> > -    uint32_t intr = s->ic_raw_intr_stat & s->ic_intr_mask;
> > -
> > -    level = !!((intr & DW_IC_INTR_RX_UNDER) |
> > -        (intr & DW_IC_INTR_RX_OVER) |
> > -        (intr & DW_IC_INTR_RX_FULL) |
> > -        (intr & DW_IC_INTR_TX_OVER) |
> > -        (intr & DW_IC_INTR_TX_EMPTY) |
> > -        (intr & DW_IC_INTR_RD_REQ) |
> > -        (intr & DW_IC_INTR_TX_ABRT) |
> > -        (intr & DW_IC_INTR_RX_DONE) |
> > -        (intr & DW_IC_INTR_ACTIVITY) |
> > -        (intr & DW_IC_INTR_STOP_DET) |
> > -        (intr & DW_IC_INTR_START_DET) |
> > -        (intr & DW_IC_INTR_GEN_CALL) |
> > -        (intr & DW_IC_INTR_RESTART_DET)
> > -        );
> > -    qemu_set_irq(s->irq, level);
> > +    uint32_t intr = s->regs[R_DW_IC_RAW_INTR_STAT] & s->regs[R_DW_IC_INTR_MASK];
> 
> Which gives you advantages like this and the FIELD set/clear which you
> aren't using anyway.

Yeah it's quite nice. I started using FIELD set/clear etc but it looked
to make the patch harder to read and a bit harder to script it. Things
might be cleaned up a bit, although there is not much multi-bit field
manipulation in the driver where those kind fo macros help most.

> > +    return value;
> > +}
> >
> > -    /* This register is invalid at this point. */
> > -    default:
> > -        qemu_log_mask(LOG_GUEST_ERROR,
> > -                      "%s: write to invalid offset or readonly register 0x%"
> > -                      HWADDR_PRIx "\n",
> > -                      DEVICE(s)->canonical_path, offset);
> > -        break;
> > +static const RegisterAccessInfo designware_i2c_regs_info[] = {
> 
> QEMU doesn't expect you to implement the RegisterAccessInfo struct.
> This struct exists as it's easy to autogenerate from RTL (or some
> other source), but you don't have to write it out yourself. You can
> use the REG/FIELD macros above and not RegisterAccessInfo, which is
> what most devices do (Xilinx devices excluded).
> 
> Sorry for the extra hassle and work or converting it to
> RegisterAccessInfo. Keeping the RegisterAccessInfo is fine, just
> letting you know you don't have to do it next time.

Hah! That's fine, it was interesting to try using it anyway. It makes
things more consistent and hopefully less error prone, and ended up with
fewer lines of code.

> I think squash these 4 together and that'll be it for the I2C patch in
> your Atlantis series.

Thanks,
Nick