On Sat, Jan 11, 2020 at 12:46:17PM +0100, Mark Kettenis wrote:
> > 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?
>
Sure, that is OK claudio@
>
> 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;
--
:wq Claudio
