[PATCH linux dev-4.10 v2] drivers: i2c: fsi: Add proper abort method

[Eddie James]

On 10/27/2017 03:41 PM, Andrew Jeffery wrote:
>>>>>> +
>>>>>> +	return -ETIME;
>>>>>> +}
>>>>>> +
>>>>>>     static int fsi_i2c_handle_status(struct fsi_i2c_port *port,
>>>>>>     				 struct i2c_msg *msg, u32 status)
>>>>>>     {
>>>>>>     	int rc;
>>>>>>     	u8 fifo_count;
>>>>>> -	struct fsi_i2c_master *i2c = port->master;
>>>>>> -	u32 dummy = 0;
>>>>>>     
>>>>>>     	if (status & I2C_STAT_ERR) {
>>>>>> -		rc = fsi_i2c_write_reg(i2c->fsi, I2C_FSI_RESET_ERR, &dummy);
>>>>>> +		rc = fsi_i2c_abort(port, status);
>>>>>>     		if (rc)
>>>>>>     			return rc;
>>>>>>     
>>>>>> +		if (status & I2C_STAT_INV_CMD)
>>>>>> +			return -EINVAL;
>>>>>> +
>>>>>> +		if (status & (I2C_STAT_PARITY | I2C_STAT_BE_OVERRUN |
>>>>>> +		    I2C_STAT_BE_ACCESS))
>>>>>> +			return -EPROTO;
>>>>>> +
>>>>>>     		if (status & I2C_STAT_NACK)
>>>>>>     			return -EFAULT;
>>>>>    
>>>>> EFAULT is "Bad address", but slaves can NACK things that aren't addresses.
>>>>> As an example i2c-aspeed calls a NACK an EIO.
>>>>    
>>>> True, but I dislike returning EIO as it's somewhat of a default option
>>>> and makes it very difficult to determine what actually went wrong. How
>>>> about ENXIO?
>>> What does userspace do with that knowledge? Is it worth differentiating? I
>>> can't immediately see that it is. Maybe it's worth a dev_dbg() or something?
>>> I feel ENXIO doesn't address my point either, as it means "No such device or
>>> address", which is still inappropriate for data NACKs.
>> Well, when someone sees an i2c failure and opens a defect and assigns it
>> to me, I would like to know if it was a NACK or something else :) If I
>> see EIO, it could be either some other I2C error, an FSI bus error, o
>> probably even a GPIO error. And dev_dbg probably won't help in the field
>> since it'll be compiled out.
> That's not true currently as we set CONFIG_DYNAMIC_DEBUG:
>
> https://github.com/openbmc/openbmc/blob/51af974f81c1d8a6df250fb38c27c1a0444b2cf3/meta-openbmc-bsp/meta-aspeed/meta-ast2500/recipes-kernel/linux/linux-obmc/defconfig#L200
>
> unless we plan to change that? You can control it via debugfs. We really
> must return something sensible with respect to the error that occurred,
> and neither EFAULT nor ENXIO fit that bill.

Ah, didn't know that was enabled, thanks! However, that still doesn't 
help the situation where we hit the failure and can't recreate it after 
enabling that level of tracing...

I still argue for ENXIO. The Broadcom iProc I2C driver uses ENXIO for 
both data and addr NACKs: 
http://elixir.free-electrons.com/linux/latest/source/drivers/i2c/busses/i2c-bcm-iproc.c#L231

I forgot to switch ECANCELED to EAGAIN in v3, so I'll send up v4.

Cheers,
Eddie

>>>>    
>>>>>    
>>>>>>     
>>>>>> +		if (status & I2C_STAT_LOST_ARB)
>>>>>> +			return -ECANCELED;
>>>>>    
>>>>> So checking around the tree, all of i2c-aspeed, i2c-designware-core,
>>>>> i2c-hix5hd2, i2c-kempld to name a few (I stopped checking at that point) use
>>>>> -EAGAIN for arbitration loss. I don't think -ECANCELLED is appropriate, and
>>>>> certainly think that -EAGAIN is what we want: Nothing went wrong aside from
>>>>> this master lost the arbitration race, so the caller should really just retry.
>>>>    
>>>> Good point, however, for this master, it can often return arbitration
>>>> lost if the clock or data line is stuck, and retries will not work. I'll
>>>> switch it to EAGAIN, callers shouldn't try too many times hopefully...
>>> Can we test status again when a new transfer is starting to see if we need to
>>> recover the bus before proceeding?
>> Well with this change, bus should be recovered after any failure, if
>> recovery is possible. I more meant the case when the clock or data line
>> is really stuck such that the master cannot recover them.
> What do we do in this case then? Power cycle?

Yes... Shouldn't happen except for a hardware fault on the slave side, 
from my experience.

>
> Andrew
>