Re: [PATCH i2c-next v6] i2c: aspeed: Handle master/slave combined irq events properly

[CÃdric Le Goater]

On 09/13/2018 03:33 PM, Guenter Roeck wrote:
> On 09/12/2018 10:45 PM, CÃdric Le Goater wrote
> 
> [ ... ]
> 
>>> ---
>>> qemu:
>>>
>>> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
>>> index c762c73..0d4aa08 100644
>>> --- a/hw/i2c/aspeed_i2c.c
>>> +++ b/hw/i2c/aspeed_i2c.c
>>> @@ -180,6 +180,33 @@ static uint8_t aspeed_i2c_get_state(AspeedI2CBus *bus)
>>> ÂÂÂÂÂ return (bus->cmd >> I2CD_TX_STATE_SHIFT) & I2CD_TX_STATE_MASK;
>>> Â }
>>> Â +static void aspeed_i2c_handle_rx_cmd(AspeedI2CBus *bus)
>>> +{
>>> +ÂÂÂ int ret;
>>> +
>>> +ÂÂÂ if (!(bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST))) {
>>> +ÂÂÂÂÂÂÂ return;
>>> +ÂÂÂ }
>>
>> it deserves a comment to understand which scenario we are trying to handle.
>>
>>> +ÂÂÂ if (bus->intr_status & I2CD_INTR_RX_DONE) {
>>> +ÂÂÂÂÂÂÂ return;
>>> +ÂÂÂ }
>>
>> should be handled in aspeed_i2c_bus_handle_cmd() I think
>>
> 
> I moved those two checks into the calling code.

ok

 
>>> +ÂÂÂ aspeed_i2c_set_state(bus, I2CD_MRXD);
>>> +ÂÂÂ ret = i2c_recv(bus->bus);
>>> +ÂÂÂ if (ret < 0) {
>>> +ÂÂÂÂÂÂÂ qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
>>> +ÂÂÂÂÂÂÂ ret = 0xff;
>>> +ÂÂÂ } else {
>>> +ÂÂÂÂÂÂÂ bus->intr_status |= I2CD_INTR_RX_DONE;
>>> +ÂÂÂ }
>>> +ÂÂÂ bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
>>> +ÂÂÂ if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
>>> +ÂÂÂÂÂÂÂ i2c_nack(bus->bus);
>>> +ÂÂÂ }
>>> +ÂÂÂ bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
>>> +ÂÂÂ aspeed_i2c_set_state(bus, I2CD_MACTIVE);
>>> +}
>>> +
>>> Â /*
>>> ÂÂ * The state machine needs some refinement. It is only used to track
>>> ÂÂ * invalid STOP commands for the moment.
>>> @@ -188,7 +215,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>>> Â {
>>> ÂÂÂÂÂ bus->cmd &= ~0xFFFF;
>>> ÂÂÂÂÂ bus->cmd |= value & 0xFFFF;
>>> -ÂÂÂ bus->intr_status = 0;> +ÂÂÂ bus->intr_status &= I2CD_INTR_RX_DONE;
>>
>> it deserves a comment to understand which scenario we are trying to handle.
>> ÂÂ 
> 
> Ok. FWIW, I wonder if intr_status should be touched here in the first place,
> but I neither have the hardware nor a datasheet, so I don't know if any bits
> are auto-cleared.

I just pushed a patch on my branch with some more explanation : 

	https://github.com/legoater/qemu/commits/aspeed-3.1

> 
>>> ÂÂÂÂÂ if (bus->cmd & I2CD_M_START_CMD) {
>>> ÂÂÂÂÂÂÂÂÂ uint8_t state = aspeed_i2c_get_state(bus) & I2CD_MACTIVE ?
>>> @@ -227,22 +254,7 @@ static void aspeed_i2c_bus_handle_cmd(AspeedI2CBus *bus, uint64_t value)
>>> ÂÂÂÂÂ }
>>> Â ÂÂÂÂÂ if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) {
>>> -ÂÂÂÂÂÂÂ int ret;
>>> -
>>> -ÂÂÂÂÂÂÂ aspeed_i2c_set_state(bus, I2CD_MRXD);
>>> -ÂÂÂÂÂÂÂ ret = i2c_recv(bus->bus);
>>> -ÂÂÂÂÂÂÂ if (ret < 0) {
>>> -ÂÂÂÂÂÂÂÂÂÂÂ qemu_log_mask(LOG_GUEST_ERROR, "%s: read failed\n", __func__);
>>> -ÂÂÂÂÂÂÂÂÂÂÂ ret = 0xff;
>>> -ÂÂÂÂÂÂÂ } else {
>>> -ÂÂÂÂÂÂÂÂÂÂÂ bus->intr_status |= I2CD_INTR_RX_DONE;
>>> -ÂÂÂÂÂÂÂ }
>>> -ÂÂÂÂÂÂÂ bus->buf = (ret & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT;
>>> -ÂÂÂÂÂÂÂ if (bus->cmd & I2CD_M_S_RX_CMD_LAST) {
>>> -ÂÂÂÂÂÂÂÂÂÂÂ i2c_nack(bus->bus);
>>> -ÂÂÂÂÂÂÂ }
>>> -ÂÂÂÂÂÂÂ bus->cmd &= ~(I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST);
>>> -ÂÂÂÂÂÂÂ aspeed_i2c_set_state(bus, I2CD_MACTIVE);
>>> +ÂÂÂÂÂÂÂ aspeed_i2c_handle_rx_cmd(bus);
>>> ÂÂÂÂÂ }
>>> Â ÂÂÂÂÂ if (bus->cmd & I2CD_M_STOP_CMD) {
>>> @@ -263,6 +275,7 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
>>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ uint64_t value, unsigned size)
>>> Â {
>>> ÂÂÂÂÂ AspeedI2CBus *bus = opaque;
>>> +ÂÂÂ int status;
>>> Â ÂÂÂÂÂ switch (offset) {
>>> ÂÂÂÂÂ case I2CD_FUN_CTRL_REG:
>>> @@ -283,9 +296,16 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr offset,
>>> ÂÂÂÂÂÂÂÂÂ bus->intr_ctrl = value & 0x7FFF;
>>> ÂÂÂÂÂÂÂÂÂ break;
>>> ÂÂÂÂÂ case I2CD_INTR_STS_REG:
>>> +ÂÂÂÂÂÂÂ status = bus->intr_status;
>>> ÂÂÂÂÂÂÂÂÂ bus->intr_status &= ~(value & 0x7FFF);
>>> -ÂÂÂÂÂÂÂ bus->controller->intr_status &= ~(1 << bus->id);
>>> -ÂÂÂÂÂÂÂ qemu_irq_lower(bus->controller->irq);
>>> +ÂÂÂÂÂÂÂ if (!bus->intr_status) {
>>> +ÂÂÂÂÂÂÂÂÂÂÂ bus->controller->intr_status &= ~(1 << bus->id);
>>> +ÂÂÂÂÂÂÂÂÂÂÂ qemu_irq_lower(bus->controller->irq);
>>> +ÂÂÂÂÂÂÂ }
>>
>> That part below is indeed something to fix. I had a similar patch.
>>
> 
> Should I split it out as separate patch ? Alternatively, if you submitted
> your patch already, I'll be happy to use it as base for mine.

See below. 

Thanks a lot,

C.