Re: [RFC PATCH 3/4] hw/i2c: add slave mode for aspeed_i2c

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 11495 bytes --]

On Apr  6 08:14, Cédric Le Goater wrote:
> Hello Klaus,
> 
> On 3/31/22 18:57, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > Add slave mode functionality for the Aspeed I2C controller. This is
> > implemented by creating an Aspeed I2C Slave device that attaches to the
> > bus.
> > 
> > This i2c slave device only implements the asynchronous version of
> > i2c_send() and the event callback.
> > 
> > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > ---
> >   hw/i2c/aspeed_i2c.c         | 95 +++++++++++++++++++++++++++++++++----
> >   hw/i2c/trace-events         |  2 +-
> >   hw/misc/meson.build         |  2 +
> >   include/hw/i2c/aspeed_i2c.h |  8 ++++
> >   4 files changed, 97 insertions(+), 10 deletions(-)
> > 
> > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> > index 03a4f5a91010..61b6424434f7 100644
> > --- a/hw/i2c/aspeed_i2c.c
> > +++ b/hw/i2c/aspeed_i2c.c
> > @@ -163,10 +163,15 @@ static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
> >             bus->intr_status & I2CD_INTR_TX_NAK ? "nak|" : "",
> >             bus->intr_status & I2CD_INTR_TX_ACK ? "ack|" : "",
> >             bus->intr_status & I2CD_INTR_RX_DONE ? "done|" : "",
> > +          bus->intr_status & I2CD_INTR_SLAVE_ADDR_RX_MATCH ? "slave-match|" : "",
> >             bus->intr_status & I2CD_INTR_NORMAL_STOP ? "normal|" : "",
> >             bus->intr_status & I2CD_INTR_ABNORMAL ? "abnormal" : "");
> 
> Troy introduced a similar change in his "new mode" proposal. I think
> it is time to change the 'aspeed_i2c_bus_raise_interrupt' trace event
> 
> Could you please update trace_aspeed_i2c_bus_raise_interrupt() to take
> a single status string ?
> 

I'm not sure it will be "prettier". But I'll give it a shot.

> > -    bus->intr_status &= bus->intr_ctrl;
> > +    /*
> > +     * WORKAROUND: the Linux Aspeed I2C driver masks SLAVE_ADDR_RX_MATCH for
> > +     * some reason, not sure if it is a bug...
> > +     */
> > +    bus->intr_status &= (bus->intr_ctrl | I2CD_INTR_SLAVE_ADDR_RX_MATCH);
> 
> It comes from the initial support for the AST2400 SoC.
> 
> We should introduce a 'intr_ctrl_mask' attribute in AspeedI2CClass and
> fix the AST24000 value to 0x7FFF ...
> 

I'm not sure I understand. Do you suggest that we always use a fixed
mask here and disregard what the host sets in intr_ctrl?

In any case, isn't it a bug in the Linux kernel driver that it neglects
to set bit 7 (slave match) in the INTR_CTRL register?

> >       if (bus->intr_status) {
> >           bus->controller->intr_status |= 1 << bus->id;
> >           qemu_irq_raise(aic->bus_get_irq(bus));
> > @@ -196,6 +201,9 @@ static uint64_t aspeed_i2c_bus_read(void *opaque, hwaddr offset,
> >       case I2CD_INTR_STS_REG:
> >           value = bus->intr_status;
> >           break;
> > +    case I2CD_DEV_ADDR_REG:
> > +        value = bus->dev_addr;
> > +        break;
> 
> You can introduce support for this register in a preliminary patch but
> keep the slave activation for later (I2CD_SLAVE_EN bit)
> 

Understood.

> >       case I2CD_POOL_CTRL_REG:
> >           value = bus->pool_ctrl;
> >           break;
> > @@ -535,10 +543,9 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
> >       switch (offset) {
> >       case I2CD_FUN_CTRL_REG:
> >           if (value & I2CD_SLAVE_EN) {
> > -            qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
> > -                          __func__);
> > -            break;
> > +            i2c_slave_set_address(&bus->slave->i2c, bus->dev_addr);
> >           }
> > +
> >           bus->ctrl = value & 0x0071C3FF;
> >           break;
> >       case I2CD_AC_TIMING_REG1:
> > @@ -558,14 +565,19 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
> >               bus->controller->intr_status &= ~(1 << bus->id);
> >               qemu_irq_lower(aic->bus_get_irq(bus));
> >           }
> > -        if (handle_rx && (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
> > -            aspeed_i2c_handle_rx_cmd(bus);
> > -            aspeed_i2c_bus_raise_interrupt(bus);
> > +
> > +        if (handle_rx) {
> > +            if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
> > +                aspeed_i2c_handle_rx_cmd(bus);
> > +                aspeed_i2c_bus_raise_interrupt(bus);
> > +            } else if (aspeed_i2c_get_state(bus) == I2CD_STXD) {
> > +                i2c_ack(bus->bus);
> > +            }
> >           }
> > +
> > 
> >           break;
> >       case I2CD_DEV_ADDR_REG:
> > -        qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
> > -                      __func__);
> > +        bus->dev_addr = value;
> >           break;
> >       case I2CD_POOL_CTRL_REG:
> >           bus->pool_ctrl &= ~0xffffff;
> > @@ -852,12 +864,74 @@ static const TypeInfo aspeed_i2c_info = {
> >       .abstract   = true,
> >   };
> > +static int aspeed_i2c_slave_event(I2CSlave *slave, enum i2c_event event)
> > +{
> > +    AspeedI2CSlave *s = ASPEED_I2C_SLAVE(slave);
> > +    AspeedI2CBus *bus = s->bus;
> > +
> > +    switch (event) {
> > +    case I2C_START_SEND:
> > +        bus->buf = bus->dev_addr << 1;
> > +
> > +        bus->buf &= I2CD_BYTE_BUF_RX_MASK;
> > +        bus->buf <<= I2CD_BYTE_BUF_RX_SHIFT;
> > +
> > +        bus->intr_status |= (I2CD_INTR_SLAVE_ADDR_RX_MATCH | I2CD_INTR_RX_DONE);
> > +        aspeed_i2c_set_state(bus, I2CD_STXD);
> > +
> > +        break;
> > +
> > +    case I2C_FINISH:
> > +        bus->intr_status |= I2CD_INTR_NORMAL_STOP;
> > +        aspeed_i2c_set_state(bus, I2CD_IDLE);
> > +
> > +        break;
> > +
> > +    default:
> > +        return -1;
> > +    }
> > +
> > +    aspeed_i2c_bus_raise_interrupt(bus);
> > +
> > +    return 0;
> > +}
> > +
> > +static void aspeed_i2c_slave_send_async(I2CSlave *slave, uint8_t data)
> > +{
> > +    AspeedI2CSlave *s = ASPEED_I2C_SLAVE(slave);
> > +    AspeedI2CBus *bus = s->bus;
> > +
> > +    bus->buf = (data & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
> > +    bus->intr_status |= I2CD_INTR_RX_DONE;
> > +
> > +    aspeed_i2c_bus_raise_interrupt(bus);
> > +}
> > +
> > +static void aspeed_i2c_slave_class_init(ObjectClass *klass, void *Data)
> > +{
> > +    DeviceClass *dc = DEVICE_CLASS(klass);
> > +    I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
> > +
> > +    dc->desc = "Aspeed I2C Bus Slave";
> > +
> > +    sc->event = aspeed_i2c_slave_event;
> > +    sc->send_async = aspeed_i2c_slave_send_async;
> > +}
> > +
> > +static const TypeInfo aspeed_i2c_slave_info = {
> > +    .name          = TYPE_ASPEED_I2C_SLAVE,
> > +    .parent        = TYPE_I2C_SLAVE,
> > +    .instance_size = sizeof(AspeedI2CSlave),
> > +    .class_init    = aspeed_i2c_slave_class_init,
> > +};
> > +
> >   static void aspeed_i2c_bus_reset(DeviceState *dev)
> >   {
> >       AspeedI2CBus *s = ASPEED_I2C_BUS(dev);
> >       s->intr_ctrl = 0;
> >       s->intr_status = 0;
> > +    s->dev_addr = 0;
> 
> Please include the new reg in vmstate.
> 

Understood.

> >       s->cmd = 0;
> >       s->buf = 0;
> >       s->dma_addr = 0;
> > @@ -881,6 +955,8 @@ static void aspeed_i2c_bus_realize(DeviceState *dev, Error **errp)
> >       sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq);
> >       s->bus = i2c_init_bus(dev, name);
> > +    s->slave = ASPEED_I2C_SLAVE(i2c_slave_create_simple(s->bus, TYPE_ASPEED_I2C_SLAVE, 0xff));
> > +    s->slave->bus = s;
> >       memory_region_init_io(&s->mr, OBJECT(s), &aspeed_i2c_bus_ops,
> >                             s, name, aic->reg_size);
> > @@ -1016,6 +1092,7 @@ static const TypeInfo aspeed_2600_i2c_info = {
> >   static void aspeed_i2c_register_types(void)
> >   {
> >       type_register_static(&aspeed_i2c_bus_info);
> > +    type_register_static(&aspeed_i2c_slave_info);
> >       type_register_static(&aspeed_i2c_info);
> >       type_register_static(&aspeed_2400_i2c_info);
> >       type_register_static(&aspeed_2500_i2c_info);
> > diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
> > index 7d8907c1eede..85e4bddff936 100644
> > --- a/hw/i2c/trace-events
> > +++ b/hw/i2c/trace-events
> > @@ -9,7 +9,7 @@ i2c_recv(uint8_t address, uint8_t data) "recv(addr:0x%02x) data:0x%02x"
> >   # aspeed_i2c.c
> >   aspeed_i2c_bus_cmd(uint32_t cmd, const char *cmd_flags, uint32_t count, uint32_t intr_status) "handling cmd=0x%x %s count=%d intr=0x%x"
> > -aspeed_i2c_bus_raise_interrupt(uint32_t intr_status, const char *str1, const char *str2, const char *str3, const char *str4, const char *str5) "handled intr=0x%x %s%s%s%s%s"
> > +aspeed_i2c_bus_raise_interrupt(uint32_t intr_status, const char *str1, const char *str2, const char *str3, const char *str4, const char *str5, const char *str6) "handled intr=0x%x %s%s%s%s%s%s"
> >   aspeed_i2c_bus_read(uint32_t busid, uint64_t offset, unsigned size, uint64_t value) "bus[%d]: To 0x%" PRIx64 " of size %u: 0x%" PRIx64
> >   aspeed_i2c_bus_write(uint32_t busid, uint64_t offset, unsigned size, uint64_t value) "bus[%d]: To 0x%" PRIx64 " of size %u: 0x%" PRIx64
> >   aspeed_i2c_bus_send(const char *mode, int i, int count, uint8_t byte) "%s send %d/%d 0x%02x"
> > diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> > index 6fb69612e064..c1c1abea41dd 100644
> > --- a/hw/misc/meson.build
> > +++ b/hw/misc/meson.build
> > @@ -122,6 +122,8 @@ softmmu_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_rng.c'))
> >   softmmu_ss.add(when: 'CONFIG_GRLIB', if_true: files('grlib_ahb_apb_pnp.c'))
> > +softmmu_ss.add(when: 'CONFIG_I2C', if_true: files('i2c-echo.c'))
> 
> That's for another patch.
> 

Yeah, was a mistake on my part when chopping up my work.

> > +
> >   specific_ss.add(when: 'CONFIG_AVR_POWER', if_true: files('avr_power.c'))
> >   specific_ss.add(when: 'CONFIG_IMX', if_true: files('imx6_src.c'))
> > diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
> > index 4b9be09274c7..3f1a9c07a00b 100644
> > --- a/include/hw/i2c/aspeed_i2c.h
> > +++ b/include/hw/i2c/aspeed_i2c.h
> > @@ -42,6 +42,7 @@ struct AspeedI2CBus {
> >       SysBusDevice parent_obj;
> >       struct AspeedI2CState *controller;
> > +    struct AspeedI2CSlave *slave;
> >       MemoryRegion mr;
> > @@ -53,6 +54,7 @@ struct AspeedI2CBus {
> >       uint32_t timing[2];
> >       uint32_t intr_ctrl;
> >       uint32_t intr_status;
> > +    uint32_t dev_addr;
> >       uint32_t cmd;
> >       uint32_t buf;
> >       uint32_t pool_ctrl;
> > @@ -76,6 +78,12 @@ struct AspeedI2CState {
> >       AddressSpace dram_as;
> >   };
> > +#define TYPE_ASPEED_I2C_SLAVE "aspeed.i2c.slave"
> > +OBJECT_DECLARE_SIMPLE_TYPE(AspeedI2CSlave, ASPEED_I2C_SLAVE)
> > +struct AspeedI2CSlave {
> > +    I2CSlave i2c;
> > +    AspeedI2CBus *bus;
> > +};
> 
> AFAICT, AspeedI2CSlave is not that useful since it doesn't maintain any
> state. The QOM interface proposal looks like a better approach.
> 

Agree.

> >   struct AspeedI2CClass {
> >       SysBusDeviceClass parent_class;
> 
> 
> If you could send these 3 patches :
>   - aspeed_i2c_bus_raise_interrupt trace event rework
>   - intr_ctrl_mask class attribute
>   - dev_addr support
> 
> I will queue them quickly and we will focus on adding slave support only.
> 

Cool, but please see my question on the intr_ctrl_mask.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]