Re: [PATCH 14/14] aspeed: Add I2C new register DMA slave mode support

[Peter Delevoryas]

> On Jun 28, 2022, at 12:02 AM, Cédric Le Goater <clg@kaod.org> wrote:
> 
> On 6/28/22 00:27, Peter Delevoryas wrote:
>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> 
> Some Intro would be welcome. Please move the patch after Klaus patches.

Yes, I’ll describe the state machine flow. That’s my bad. And I’ll
move it to the beginning with the other i2c patches.

> 
> Thanks,
> 
> C.
> 
> 
> 
>> ---
>>  hw/i2c/aspeed_i2c.c         | 133 ++++++++++++++++++++++++++++++++----
>>  include/hw/i2c/aspeed_i2c.h |   3 +
>>  2 files changed, 124 insertions(+), 12 deletions(-)
>> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
>> index 8a8514586f..fc8b6b62cf 100644
>> --- a/hw/i2c/aspeed_i2c.c
>> +++ b/hw/i2c/aspeed_i2c.c
>> @@ -78,6 +78,18 @@ static inline void 
>> aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
>>      }
>>  }
>>  +static inline void aspeed_i2c_bus_raise_slave_interrupt(AspeedI2CBus *bus)
>> +{
>> +    AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
>> +
>> +    if (!bus->regs[R_I2CS_INTR_STS]) {
>> +        return;
>> +    }
>> +
>> +    bus->controller->intr_status |= 1 << bus->id;
>> +    qemu_irq_raise(aic->bus_get_irq(bus));
>> +}
>> +
>>  static uint64_t aspeed_i2c_bus_old_read(AspeedI2CBus *bus, hwaddr offset,
>>                                          unsigned size)
>>  {
>> @@ -140,8 +152,17 @@ static uint64_t aspeed_i2c_bus_new_read(AspeedI2CBus 
>> *bus, hwaddr offset,
>>      case A_I2CM_DMA_LEN_STS:
>>      case A_I2CC_DMA_ADDR:
>>      case A_I2CC_DMA_LEN:
>> +
>> +    case A_I2CS_DEV_ADDR:
>> +    case A_I2CS_DMA_RX_ADDR:
>> +    case A_I2CS_DMA_LEN:
>> +    case A_I2CS_CMD:
>> +    case A_I2CS_INTR_CTRL:
>> +    case A_I2CS_DMA_LEN_STS:
>>          /* Value is already set, don't do anything. */
>>          break;
>> +    case A_I2CS_INTR_STS:
>> +        break;
>>      case A_I2CM_CMD:
>>          value = SHARED_FIELD_DP32(value, BUS_BUSY_STS, 
>> i2c_bus_busy(bus->bus));
>>          break;
>> @@ -547,12 +568,7 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, 
>> hwaddr offset,
>>        switch (offset) {
>>      case A_I2CC_FUN_CTRL:
>> -        if (SHARED_FIELD_EX32(value, SLAVE_EN)) {
>> -            qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
>> -                          __func__);
>> -            break;
>> -        }
>> -        bus->regs[R_I2CC_FUN_CTRL] = value & 0x007dc3ff;
>> +        bus->regs[R_I2CC_FUN_CTRL] = value;
>>          break;
>>      case A_I2CC_AC_TIMING:
>>          bus->regs[R_I2CC_AC_TIMING] = value & 0x1ffff0ff;
>> @@ -580,6 +596,7 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, 
>> hwaddr offset,
>>                  bus->controller->intr_status &= ~(1 << bus->id);
>>                  qemu_irq_lower(aic->bus_get_irq(bus));
>>              }
>> +            aspeed_i2c_bus_raise_slave_interrupt(bus);
>>              break;
>>          }
>>          bus->regs[R_I2CM_INTR_STS] &= ~(value & 0xf007f07f);
>> @@ -668,15 +685,53 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus 
>> *bus, hwaddr offset,
>>      case A_I2CC_DMA_LEN:
>>          /* RO */
>>          break;
>> -    case A_I2CS_DMA_LEN_STS:
>> -    case A_I2CS_DMA_TX_ADDR:
>> -    case A_I2CS_DMA_RX_ADDR:
>>      case A_I2CS_DEV_ADDR:
>> +        bus->regs[R_I2CS_DEV_ADDR] = value;
>> +        break;
>> +    case A_I2CS_DMA_RX_ADDR:
>> +        bus->regs[R_I2CS_DMA_RX_ADDR] = value;
>> +        break;
>> +    case A_I2CS_DMA_LEN:
>> +        assert(FIELD_EX32(value, I2CS_DMA_LEN, TX_BUF_LEN) == 0);
>> +        if (FIELD_EX32(value, I2CS_DMA_LEN, RX_BUF_LEN_W1T)) {
>> +            ARRAY_FIELD_DP32(bus->regs, I2CS_DMA_LEN, RX_BUF_LEN,
>> +                             FIELD_EX32(value, I2CS_DMA_LEN, RX_BUF_LEN));
>> +        } else {
>> +            bus->regs[R_I2CS_DMA_LEN] = value;
>> +        }
>> +        break;
>> +    case A_I2CS_CMD:
>> +        if (FIELD_EX32(value, I2CS_CMD, W1_CTRL)) {
>> +            bus->regs[R_I2CS_CMD] |= value;
>> +        } else {
>> +            bus->regs[R_I2CS_CMD] = value;
>> +        }
>> +        i2c_slave_set_address(bus->slave, bus->regs[R_I2CS_DEV_ADDR]);
>> +        break;
>>      case A_I2CS_INTR_CTRL:
>> +        bus->regs[R_I2CS_INTR_CTRL] = value;
>> +        break;
>> +
>>      case A_I2CS_INTR_STS:
>> -    case A_I2CS_CMD:
>> -    case A_I2CS_DMA_LEN:
>> -        qemu_log_mask(LOG_UNIMP, "%s: Slave mode is not implemented\n",
>> +        if (ARRAY_FIELD_EX32(bus->regs, I2CS_INTR_CTRL, PKT_CMD_DONE)) {
>> +            if (ARRAY_FIELD_EX32(bus->regs, I2CS_INTR_STS, PKT_CMD_DONE) &&
>> +                FIELD_EX32(value, I2CS_INTR_STS, PKT_CMD_DONE)) {
>> +                bus->regs[R_I2CS_INTR_STS] &= 0xfffc0000;
>> +            }
>> +        } else {
>> +            bus->regs[R_I2CS_INTR_STS] &= ~value;
>> +        }
>> +        if (!bus->regs[R_I2CS_INTR_STS]) {
>> +            bus->controller->intr_status &= ~(1 << bus->id);
>> +            qemu_irq_lower(aic->bus_get_irq(bus));
>> +        }
>> +        aspeed_i2c_bus_raise_interrupt(bus);
>> +        break;
>> +    case A_I2CS_DMA_LEN_STS:
>> +        bus->regs[R_I2CS_DMA_LEN_STS] = 0;
>> +        break;
>> +    case A_I2CS_DMA_TX_ADDR:
>> +        qemu_log_mask(LOG_UNIMP, "%s: Slave mode DMA TX is not 
>> implemented\n",
>>                        __func__);
>>          break;
>>      default:
>> @@ -1037,6 +1092,39 @@ static const TypeInfo aspeed_i2c_info = {
>>      .abstract   = true,
>>  };
>>  +static int aspeed_i2c_bus_new_slave_event(AspeedI2CBus *bus,
>> +                                          enum i2c_event event)
>> +{
>> +    switch (event) {
>> +    case I2C_START_SEND_ASYNC:
>> +        if (!SHARED_ARRAY_FIELD_EX32(bus->regs, R_I2CS_CMD, RX_DMA_EN)) {
>> +            qemu_log_mask(LOG_GUEST_ERROR,
>> +                          "%s: Slave mode RX DMA is not enabled\n", 
>> __func__);
>> +            return -1;
>> +        }
>> +        ARRAY_FIELD_DP32(bus->regs, I2CS_DMA_LEN_STS, RX_LEN, 0);
>> +        bus->regs[R_I2CC_DMA_ADDR] =
>> +            ARRAY_FIELD_EX32(bus->regs, I2CS_DMA_RX_ADDR, ADDR);
>> +        bus->regs[R_I2CC_DMA_LEN] =
>> +            ARRAY_FIELD_EX32(bus->regs, I2CS_DMA_LEN, RX_BUF_LEN) + 1;
>> +        i2c_ack(bus->bus);
>> +        break;
>> +    case I2C_FINISH:
>> +        ARRAY_FIELD_DP32(bus->regs, I2CS_INTR_STS, PKT_CMD_DONE, 1);
>> +        ARRAY_FIELD_DP32(bus->regs, I2CS_INTR_STS, SLAVE_ADDR_RX_MATCH, 1);
>> +        SHARED_ARRAY_FIELD_DP32(bus->regs, R_I2CS_INTR_STS, NORMAL_STOP, 1);
>> +        SHARED_ARRAY_FIELD_DP32(bus->regs, R_I2CS_INTR_STS, RX_DONE, 1);
>> +        aspeed_i2c_bus_raise_slave_interrupt(bus);
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_UNIMP, "%s: i2c event %d unimplemented\n",
>> +                      __func__, event);
>> +        return -1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  static int aspeed_i2c_bus_slave_event(I2CSlave *slave, enum i2c_event event)
>>  {
>>      BusState *qbus = qdev_get_parent_bus(DEVICE(slave));
>> @@ -1045,6 +1133,10 @@ static int aspeed_i2c_bus_slave_event(I2CSlave 
>> *slave, enum i2c_event event)
>>      uint32_t reg_byte_buf = aspeed_i2c_bus_byte_buf_offset(bus);
>>      uint32_t value;
>>  +    if (aspeed_i2c_is_new_mode(bus->controller)) {
>> +        return aspeed_i2c_bus_new_slave_event(bus, event);
>> +    }
>> +
>>      switch (event) {
>>      case I2C_START_SEND_ASYNC:
>>          value = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_byte_buf, TX_BUF);
>> @@ -1073,6 +1165,19 @@ static int aspeed_i2c_bus_slave_event(I2CSlave 
>> *slave, enum i2c_event event)
>>      return 0;
>>  }
>>  +static void aspeed_i2c_bus_new_slave_send_async(AspeedI2CBus *bus, uint8_t 
>> data)
>> +{
>> +    assert(address_space_write(&bus->controller->dram_as,
>> +                               bus->regs[R_I2CC_DMA_ADDR],
>> +                               MEMTXATTRS_UNSPECIFIED, &data, 1) == 
>> MEMTX_OK);
>> +
>> +    bus->regs[R_I2CC_DMA_ADDR]++;
>> +    bus->regs[R_I2CC_DMA_LEN]--;
>> +    ARRAY_FIELD_DP32(bus->regs, I2CS_DMA_LEN_STS, RX_LEN,
>> +                     ARRAY_FIELD_EX32(bus->regs, I2CS_DMA_LEN_STS, RX_LEN) 
>> + 1);
>> +    i2c_ack(bus->bus);
>> +}
>> +
>>  static void aspeed_i2c_bus_slave_send_async(I2CSlave *slave, uint8_t data)
>>  {
>>      BusState *qbus = qdev_get_parent_bus(DEVICE(slave));
>> @@ -1080,6 +1185,10 @@ static void aspeed_i2c_bus_slave_send_async(I2CSlave 
>> *slave, uint8_t data)
>>      uint32_t reg_intr_sts = aspeed_i2c_bus_intr_sts_offset(bus);
>>      uint32_t reg_byte_buf = aspeed_i2c_bus_byte_buf_offset(bus);
>>  +    if (aspeed_i2c_is_new_mode(bus->controller)) {
>> +        return aspeed_i2c_bus_new_slave_send_async(bus, data);
>> +    }
>> +
>>      SHARED_ARRAY_FIELD_DP32(bus->regs, reg_byte_buf, RX_BUF, data);
>>      SHARED_ARRAY_FIELD_DP32(bus->regs, reg_intr_sts, RX_DONE, 1);
>>  diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
>> index ba148b2f6d..300a89b343 100644
>> --- a/include/hw/i2c/aspeed_i2c.h
>> +++ b/include/hw/i2c/aspeed_i2c.h
>> @@ -174,6 +174,8 @@ REG32(I2CM_DMA_LEN, 0x1c)
>>      FIELD(I2CM_DMA_LEN, TX_BUF_LEN_W1T, 15, 1)
>>      FIELD(I2CM_DMA_LEN, TX_BUF_LEN, 0, 11)
>>  REG32(I2CS_INTR_CTRL, 0x20)
>> +    FIELD(I2CS_INTR_CTRL, PKT_CMD_FAIL, 17, 1)
>> +    FIELD(I2CS_INTR_CTRL, PKT_CMD_DONE, 16, 1)
>>  REG32(I2CS_INTR_STS, 0x24)
>>      /* 31:29 shared with I2CD_INTR_STS[31:29] */
>>      FIELD(I2CS_INTR_STS, SLAVE_PARKING_STS, 24, 2)
>> @@ -184,6 +186,7 @@ REG32(I2CS_INTR_STS, 0x24)
>>      FIELD(I2CS_INTR_STS, PKT_CMD_FAIL, 17, 1)
>>      FIELD(I2CS_INTR_STS, PKT_CMD_DONE, 16, 1)
>>      /* 14:0 shared with I2CD_INTR_STS[14:0] */
>> +    FIELD(I2CS_INTR_STS, SLAVE_ADDR_RX_MATCH, 7, 1)
>>  REG32(I2CS_CMD, 0x28)
>>      FIELD(I2CS_CMD, W1_CTRL, 31, 1)
>>      FIELD(I2CS_CMD, PKT_MODE_ACTIVE_ADDR, 17, 2)
>