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

[Andrew Jeffery]

> >>>> +
> >>>> +	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.

> 
> >
> >>   
> >>>   
> >>>>    
> >>>> +		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?

Andrew