> Date: Fri, 10 Jan 2020 12:07:51 +0100
> From: Claudio Jeker <cje...@diehard.n-r-g.com>
>
> On Sun, Dec 22, 2019 at 04:55:59PM +0100, Mark Kettenis wrote:
> > The diff below contains a couple of improvements to dwiic(4). They're
> > mostly for making ipmi(4) on the Ampere/Lenovo arm64 boxes work
> > better. But they need testing on x86 machines with
> > keyboards.touchpads/touchscreens connected over i2c.
> >
> > Most of the diff is there to properly implement SMBus block write/read
> > transactions. Defines for these are taken from NetBSD (even though
> > NetBSD doesn't actually implement these). Nothing special needs to
> > been done for block writes, but for block reads the slave device sends
> > us byte count before sending us the data bytes. So the code is
> > changed such that we read the byte count first and then adjust the
> > length of the transaction accordingly.
> >
> > The diff then uses this support for block reads in the ipmi(4) SSIF
> > interface code and adds support for multi-part read transactions on
> > top of that. That makes ipmi(4) fully functional on the arm64 machine
> > mentioned above.
> >
> > Well, almost. I found that dwiic(4) still craps out every now and
> > then on this machine. Limiting the number of commands we send to the
> > controller to the size of the Tx FIFO minus one seems to help.
> >
> > Will probably split up the diff before committing.
> >
> > ok?
> >
> >
> > Index: dev/ic/dwiic.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/ic/dwiic.c,v
> > retrieving revision 1.8
> > diff -u -p -r1.8 dwiic.c
> > --- dev/ic/dwiic.c 18 Aug 2019 15:52:45 -0000 1.8
> > +++ dev/ic/dwiic.c 22 Dec 2019 15:40:28 -0000
> > @@ -338,6 +338,13 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
> >
> > dwiic_write(sc, DW_IC_DATA_CMD, cmd);
> >
> > + /*
> > + * For a block read, get the byte count before
> > + * continuing to read the data bytes.
> > + */
> > + if (I2C_OP_READ_P(op) && I2C_OP_BLKMODE_P(op) && readpos == 0)
> > + tx_limit = 1;
> > +
> > tx_limit--;
> > x++;
> >
> > @@ -345,7 +352,7 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
> > * As TXFLR fills up, we need to clear it out by reading all
> > * available data.
> > */
> > - while (I2C_OP_READ_P(op) && (tx_limit == 0 || x == len)) {
> > + while (I2C_OP_READ_P(op) && (tx_limit <= 1 || x == len)) {
> > DPRINTF(("%s: %s: tx_limit %d, sent %d read reqs\n",
> > sc->sc_dev.dv_xname, __func__, tx_limit, x));
> >
>
> I'm a bit confused with the previous two hunks. Why change the tx_limit
> check? Also you set tx_limit to 1 and then immediatly a tx_limit-- follows
> so that the limit is actually 0 which is what you want but then the change
> of the check in the while loop seems wrong.
Ok, let's separate things out a bit more. Here is the diff that just
implements block mode. There the limit is initially set to 1 to make
sure we have a chance to look at the first byte to determine the total
length of the transfer.
ok?
Index: dev/ic/dwiic.c
===================================================================
RCS file: /cvs/src/sys/dev/ic/dwiic.c,v
retrieving revision 1.8
diff -u -p -r1.8 dwiic.c
--- dev/ic/dwiic.c 18 Aug 2019 15:52:45 -0000 1.8
+++ dev/ic/dwiic.c 11 Jan 2020 11:37:53 -0000
@@ -338,6 +338,13 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
dwiic_write(sc, DW_IC_DATA_CMD, cmd);
+ /*
+ * For a block read, get the byte count before
+ * continuing to read the data bytes.
+ */
+ if (I2C_OP_READ_P(op) && I2C_OP_BLKMODE_P(op) && readpos == 0)
+ tx_limit = 1;
+
tx_limit--;
x++;
@@ -374,8 +381,7 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
if (rx_avail == 0) {
printf("%s: timed out reading remaining %d\n",
- sc->sc_dev.dv_xname,
- (int)(len - 1 - readpos));
+ sc->sc_dev.dv_xname, (int)(len - readpos));
sc->sc_i2c_xfer.error = 1;
sc->sc_busy = 0;
@@ -394,6 +400,13 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
}
rx_avail--;
}
+
+ /*
+ * Update the transfer length when doing a
+ * block read.
+ */
+ if (I2C_OP_BLKMODE_P(op) && readpos > 0 && len > b[0])
+ len = b[0] + 1;
if (readpos >= len)
break;
