Re: Another dwiic(4) fix
On Sat, Aug 17, 2019 at 06:42:01PM +0200, Mark Kettenis wrote:
> The timeout when waiting for data to be received for polled mode is
> too small for taling to the BMC on the Ampere/Lenovo arm64 server.
> This bumps it to 50 ms, which is still lower than what it is for
> non-polled mode.
>
> I also found that things worked much better if we checked for the
> actually STOP detection bit to come instead of looking at the activity
> bit in the status register.
>
> Also tested on my Asus x205ta, which has an i2c keyboard.
>
> ok?
>
>
This causes no regressions on the 2 machines I tried.
Probably the right time to get it in, ok mlarkin
> Index: dwiic.c
> ===
> RCS file: /cvs/src/sys/dev/ic/dwiic.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 dwiic.c
> --- dwiic.c 6 Aug 2019 06:56:29 - 1.6
> +++ dwiic.c 17 Aug 2019 16:12:39 -
> @@ -350,7 +350,7 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
> sc->sc_dev.dv_xname, __func__, tx_limit, x));
>
> if (flags & I2C_F_POLL) {
> - for (retries = 100; retries > 0; retries--) {
> + for (retries = 1000; retries > 0; retries--) {
> rx_avail = dwiic_read(sc, DW_IC_RXFLR);
> if (rx_avail > 0)
> break;
> @@ -407,14 +407,13 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
>
> if (I2C_OP_STOP_P(op) && I2C_OP_WRITE_P(op)) {
> if (flags & I2C_F_POLL) {
> - /* wait for bus to be idle */
> for (retries = 100; retries > 0; retries--) {
> - st = dwiic_read(sc, DW_IC_STATUS);
> - if (!(st & DW_IC_STATUS_ACTIVITY))
> + st = dwiic_read(sc, DW_IC_RAW_INTR_STAT);
> + if (st & DW_IC_INTR_STOP_DET)
> break;
> DELAY(1000);
> }
> - if (st & DW_IC_STATUS_ACTIVITY)
> + if (!(st & DW_IC_INTR_STOP_DET))
> printf("%s: timed out waiting for bus idle\n",
> sc->sc_dev.dv_xname);
> } else {
>
Another dwiic(4) fix
The timeout when waiting for data to be received for polled mode is
too small for taling to the BMC on the Ampere/Lenovo arm64 server.
This bumps it to 50 ms, which is still lower than what it is for
non-polled mode.
I also found that things worked much better if we checked for the
actually STOP detection bit to come instead of looking at the activity
bit in the status register.
Also tested on my Asus x205ta, which has an i2c keyboard.
ok?
Index: dwiic.c
===
RCS file: /cvs/src/sys/dev/ic/dwiic.c,v
retrieving revision 1.6
diff -u -p -r1.6 dwiic.c
--- dwiic.c 6 Aug 2019 06:56:29 - 1.6
+++ dwiic.c 17 Aug 2019 16:12:39 -
@@ -350,7 +350,7 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
sc->sc_dev.dv_xname, __func__, tx_limit, x));
if (flags & I2C_F_POLL) {
- for (retries = 100; retries > 0; retries--) {
+ for (retries = 1000; retries > 0; retries--) {
rx_avail = dwiic_read(sc, DW_IC_RXFLR);
if (rx_avail > 0)
break;
@@ -407,14 +407,13 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
if (I2C_OP_STOP_P(op) && I2C_OP_WRITE_P(op)) {
if (flags & I2C_F_POLL) {
- /* wait for bus to be idle */
for (retries = 100; retries > 0; retries--) {
- st = dwiic_read(sc, DW_IC_STATUS);
- if (!(st & DW_IC_STATUS_ACTIVITY))
+ st = dwiic_read(sc, DW_IC_RAW_INTR_STAT);
+ if (st & DW_IC_INTR_STOP_DET)
break;
DELAY(1000);
}
- if (st & DW_IC_STATUS_ACTIVITY)
+ if (!(st & DW_IC_INTR_STOP_DET))
printf("%s: timed out waiting for bus idle\n",
sc->sc_dev.dv_xname);
} else {
Re: dwiic(4) fix
My iatp device works the same as before, good on boot, but fails
reading main memory map, etc on resume.
On Wed, May 23, 2018 at 11:18 PM, Mike Larkin wrote:
> On Tue, May 22, 2018 at 05:43:01PM +0200, Mark Kettenis wrote:
>> > Date: Mon, 21 May 2018 17:25:47 -0700
>> > From: Mike Larkin
>> >
>> > On Mon, May 21, 2018 at 12:44:47PM +0200, Mark Kettenis wrote:
>> > > The diff below fixes I2C_OP_WRITE_WITH_STOP operations. Currently we
>> > > run the read completion code for those operations, but since there is
>> > > no data to read, this fails. Instead, this waits until a stop is
>> > > detected. That doesn't seem to work for I2C_F_POLL operations though,
>> > > so in that case we wait until the bus is idle. This also adds
>> > > interlocks (using splbio/splx) on the existing tsleep() operations.
>> > >
>> > > This doesn't fix all the issues with the driver. In particular it
>> > > seems that large reads still cause problems. I think a somewhat more
>> > > invasive re-write of the dwiic_i2c_exec() function is needed. But I'd
>> > > like to get this in before I embark on that.
>> > >
>> > > ok?
>> > >
>> >
>> > The diff reads ok to me, do you want us to do any sort of testing here
>> > before
>> > commit?
>>
>> Testing is always appreciated, and a goood idea if you own a machine
>> that relies on dwiic(4). For example those with machines that have a
>> keyboard or touchpad/screen connected over i2c.
>>
>> Cheers,
>>
>> Mark
>>
>
> No regressions seen here.
>
> -ml
>
>> > > Index: dev/ic/dwiic.c
>> > > ===
>> > > RCS file: /cvs/src/sys/dev/ic/dwiic.c,v
>> > > retrieving revision 1.3
>> > > diff -u -p -r1.3 dwiic.c
>> > > --- dev/ic/dwiic.c19 Jan 2018 18:20:38 - 1.3
>> > > +++ dev/ic/dwiic.c21 May 2018 10:37:50 -
>> > > @@ -192,6 +192,7 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
>> > > u_int32_t ic_con, st, cmd, resp;
>> > > int retries, tx_limit, rx_avail, x, readpos;
>> > > uint8_t *b;
>> > > + int s;
>> > >
>> > > if (sc->sc_busy)
>> > > return 1;
>> > > @@ -245,12 +246,14 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
>> > > if (flags & I2C_F_POLL)
>> > > DELAY(200);
>> > > else {
>> > > + s = splbio();
>> > > dwiic_read(sc, DW_IC_CLR_INTR);
>> > > dwiic_write(sc, DW_IC_INTR_MASK, DW_IC_INTR_TX_EMPTY);
>> > >
>> > > if (tsleep(&sc->sc_writewait, PRIBIO, "dwiic", hz / 2) != 0)
>> > > printf("%s: timed out waiting for tx_empty intr\n",
>> > > sc->sc_dev.dv_xname);
>> > > + splx(s);
>> > > }
>> > >
>> > > /* send our command, one byte at a time */
>> > > @@ -320,7 +323,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 (tx_limit == 0 || x == len) {
>> > > + while (I2C_OP_READ_P(op) && (tx_limit == 0 || x == len)) {
>> > > DPRINTF(("%s: %s: tx_limit %d, sent %d read reqs\n",
>> > > sc->sc_dev.dv_xname, __func__, tx_limit, x));
>> > >
>> > > @@ -332,6 +335,7 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
>> > > DELAY(50);
>> > > }
>> > > } else {
>> > > + s = splbio();
>> > > dwiic_read(sc, DW_IC_CLR_INTR);
>> > > dwiic_write(sc, DW_IC_INTR_MASK,
>> > > DW_IC_INTR_RX_FULL);
>> > > @@ -341,6 +345,7 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
>> > > printf("%s: timed out waiting for "
>> > > "rx_full intr\n",
>> > > sc->sc_dev.dv_xname);
>> > > + splx(s);
>> > >
>> > > rx_avail = dwiic_read(sc, DW_IC_RXFLR);
>> > > }
>> > > @@ -378,6 +383,32 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
>> > > }
>> > > }
>> > >
>> > > + if (I2C_OP_STOP_P(op) && I2C_OP_WRITE_P(op)) {
>> > > + if (flags & I2C_F_POLL) {
>> > > + /* wait for bus to be idle */
>> > > + for (retries = 100; retries > 0; retries--) {
>> > > + st = dwiic_read(sc, DW_IC_STATUS);
>> > > + if (!(st & DW_IC_STATUS_ACTIVITY))
>> > > + break;
>> > > + DELAY(1000);
>> > > + }
>> > > + if (st & DW_IC_STATUS_ACTIVITY)
>> > > + printf("%s: timed out waiting for bus idle\n",
>> > > + sc->sc_dev.dv_xname);
>> > > + } else {
>> > > + s = splbio();
>> > > + while (sc->sc_busy) {
>> > > + dwiic_write(sc
Re: dwiic(4) fix
On Tue, May 22, 2018 at 05:43:01PM +0200, Mark Kettenis wrote:
> > Date: Mon, 21 May 2018 17:25:47 -0700
> > From: Mike Larkin
> >
> > On Mon, May 21, 2018 at 12:44:47PM +0200, Mark Kettenis wrote:
> > > The diff below fixes I2C_OP_WRITE_WITH_STOP operations. Currently we
> > > run the read completion code for those operations, but since there is
> > > no data to read, this fails. Instead, this waits until a stop is
> > > detected. That doesn't seem to work for I2C_F_POLL operations though,
> > > so in that case we wait until the bus is idle. This also adds
> > > interlocks (using splbio/splx) on the existing tsleep() operations.
> > >
> > > This doesn't fix all the issues with the driver. In particular it
> > > seems that large reads still cause problems. I think a somewhat more
> > > invasive re-write of the dwiic_i2c_exec() function is needed. But I'd
> > > like to get this in before I embark on that.
> > >
> > > ok?
> > >
> >
> > The diff reads ok to me, do you want us to do any sort of testing here
> > before
> > commit?
>
> Testing is always appreciated, and a goood idea if you own a machine
> that relies on dwiic(4). For example those with machines that have a
> keyboard or touchpad/screen connected over i2c.
>
> Cheers,
>
> Mark
>
No regressions seen here.
-ml
> > > Index: dev/ic/dwiic.c
> > > ===
> > > RCS file: /cvs/src/sys/dev/ic/dwiic.c,v
> > > retrieving revision 1.3
> > > diff -u -p -r1.3 dwiic.c
> > > --- dev/ic/dwiic.c19 Jan 2018 18:20:38 - 1.3
> > > +++ dev/ic/dwiic.c21 May 2018 10:37:50 -
> > > @@ -192,6 +192,7 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
> > > u_int32_t ic_con, st, cmd, resp;
> > > int retries, tx_limit, rx_avail, x, readpos;
> > > uint8_t *b;
> > > + int s;
> > >
> > > if (sc->sc_busy)
> > > return 1;
> > > @@ -245,12 +246,14 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
> > > if (flags & I2C_F_POLL)
> > > DELAY(200);
> > > else {
> > > + s = splbio();
> > > dwiic_read(sc, DW_IC_CLR_INTR);
> > > dwiic_write(sc, DW_IC_INTR_MASK, DW_IC_INTR_TX_EMPTY);
> > >
> > > if (tsleep(&sc->sc_writewait, PRIBIO, "dwiic", hz / 2) != 0)
> > > printf("%s: timed out waiting for tx_empty intr\n",
> > > sc->sc_dev.dv_xname);
> > > + splx(s);
> > > }
> > >
> > > /* send our command, one byte at a time */
> > > @@ -320,7 +323,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 (tx_limit == 0 || x == len) {
> > > + while (I2C_OP_READ_P(op) && (tx_limit == 0 || x == len)) {
> > > DPRINTF(("%s: %s: tx_limit %d, sent %d read reqs\n",
> > > sc->sc_dev.dv_xname, __func__, tx_limit, x));
> > >
> > > @@ -332,6 +335,7 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
> > > DELAY(50);
> > > }
> > > } else {
> > > + s = splbio();
> > > dwiic_read(sc, DW_IC_CLR_INTR);
> > > dwiic_write(sc, DW_IC_INTR_MASK,
> > > DW_IC_INTR_RX_FULL);
> > > @@ -341,6 +345,7 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
> > > printf("%s: timed out waiting for "
> > > "rx_full intr\n",
> > > sc->sc_dev.dv_xname);
> > > + splx(s);
> > >
> > > rx_avail = dwiic_read(sc, DW_IC_RXFLR);
> > > }
> > > @@ -378,6 +383,32 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
> > > }
> > > }
> > >
> > > + if (I2C_OP_STOP_P(op) && I2C_OP_WRITE_P(op)) {
> > > + if (flags & I2C_F_POLL) {
> > > + /* wait for bus to be idle */
> > > + for (retries = 100; retries > 0; retries--) {
> > > + st = dwiic_read(sc, DW_IC_STATUS);
> > > + if (!(st & DW_IC_STATUS_ACTIVITY))
> > > + break;
> > > + DELAY(1000);
> > > + }
> > > + if (st & DW_IC_STATUS_ACTIVITY)
> > > + printf("%s: timed out waiting for bus idle\n",
> > > + sc->sc_dev.dv_xname);
> > > + } else {
> > > + s = splbio();
> > > + while (sc->sc_busy) {
> > > + dwiic_write(sc, DW_IC_INTR_MASK,
> > > + DW_IC_INTR_STOP_DET);
> > > + if (tsleep(&sc->sc_busy, PRIBIO, "dwiic",
> > > + hz / 2) != 0)
> > > + printf("%s: t
Re: dwiic(4) fix
On Tue, May 22, 2018 at 05:43:01PM +0200, Mark Kettenis wrote: > > Date: Mon, 21 May 2018 17:25:47 -0700 > > From: Mike Larkin > > > > On Mon, May 21, 2018 at 12:44:47PM +0200, Mark Kettenis wrote: > > > The diff below fixes I2C_OP_WRITE_WITH_STOP operations. Currently we > > > run the read completion code for those operations, but since there is > > > no data to read, this fails. Instead, this waits until a stop is > > > detected. That doesn't seem to work for I2C_F_POLL operations though, > > > so in that case we wait until the bus is idle. This also adds > > > interlocks (using splbio/splx) on the existing tsleep() operations. > > > > > > This doesn't fix all the issues with the driver. In particular it > > > seems that large reads still cause problems. I think a somewhat more > > > invasive re-write of the dwiic_i2c_exec() function is needed. But I'd > > > like to get this in before I embark on that. > > > > > > ok? > > > > > > > The diff reads ok to me, do you want us to do any sort of testing here > > before > > commit? > > Testing is always appreciated, and a goood idea if you own a machine > that relies on dwiic(4). For example those with machines that have a > keyboard or touchpad/screen connected over i2c. > Looks fine on the Asus UX390UAK. I don't notice a change with your diff applied. The touchpad still works. Remi OpenBSD 6.3-current (GENERIC.MP) #4: Tue May 22 22:16:21 CEST 2018 r...@typhoon.relo.ch:/usr/src/sys/arch/amd64/compile/GENERIC.MP real mem = 17041674240 (16252MB) avail mem = 16517115904 (15751MB) mpath0 at root scsibus0 at mpath0: 256 targets mainbus0 at root bios0 at mainbus0: SMBIOS rev. 3.0 @ 0x9b16d000 (31 entries) bios0: vendor American Megatrends Inc. version "UX390UAK.312" date 04/25/2017 bios0: ASUSTeK COMPUTER INC. UX390UAK acpi0 at bios0: rev 2 acpi0: sleep states S0 S3 S4 S5 acpi0: tables DSDT FACP APIC FPDT ECDT MCFG FIDT SSDT MSDM SSDT HPET UEFI SSDT LPIT WSMT SSDT SSDT DBGP DBG2 BGRT DMAR TPM2 acpi0: wakeup devices GLAN(S4) XHC_(S3) XDCI(S4) HDAS(S4) acpitimer0 at acpi0: 3579545 Hz, 24 bits acpimadt0 at acpi0 addr 0xfee0: PC-AT compat cpu0 at mainbus0: apid 0 (boot processor) cpu0: Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz, 2594.76 MHz cpu0: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,3DNOWP,PERF,ITSC,FSGSBASE,SGX,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,MPX,RDSEED,ADX,SMAP,CLFLUSHOPT,PT,IBRS,IBPB,STIBP,SENSOR,ARAT,MELTDOWN cpu0: 256KB 64b/line 8-way L2 cache cpu0: smt 0, core 0, package 0 mtrr: Pentium Pro MTRR support, 10 var ranges, 88 fixed ranges cpu0: apic clock running at 23MHz cpu0: mwait min=64, max=64, C-substates=0.2.1.2.4.1.1.1, IBE cpu1 at mainbus0: apid 2 (application processor) cpu1: Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz, 2593.93 MHz cpu1: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,3DNOWP,PERF,ITSC,FSGSBASE,SGX,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,MPX,RDSEED,ADX,SMAP,CLFLUSHOPT,PT,IBRS,IBPB,STIBP,SENSOR,ARAT,MELTDOWN cpu1: 256KB 64b/line 8-way L2 cache cpu1: smt 0, core 1, package 0 cpu2 at mainbus0: apid 1 (application processor) cpu2: Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz, 2593.93 MHz cpu2: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,3DNOWP,PERF,ITSC,FSGSBASE,SGX,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,MPX,RDSEED,ADX,SMAP,CLFLUSHOPT,PT,IBRS,IBPB,STIBP,SENSOR,ARAT,MELTDOWN cpu2: 256KB 64b/line 8-way L2 cache cpu2: smt 1, core 0, package 0 cpu3 at mainbus0: apid 3 (application processor) cpu3: Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz, 2593.93 MHz cpu3: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,DS,ACPI,MMX,FXSR,SSE,SSE2,SS,HTT,TM,PBE,SSE3,PCLMUL,DTES64,MWAIT,DS-CPL,VMX,EST,TM2,SSSE3,SDBG,FMA3,CX16,xTPR,PDCM,PCID,SSE4.1,SSE4.2,x2APIC,MOVBE,POPCNT,DEADLINE,AES,XSAVE,AVX,F16C,RDRAND,NXE,PAGE1GB,RDTSCP,LONG,LAHF,ABM,3DNOWP,PERF,ITSC,FSGSBASE,SGX,BMI1,AVX2,SMEP,BMI2,ERMS,INVPCID,MPX,RDSEED,ADX,SMAP,CLFLUSHOPT,PT,IBRS,IBPB,STIBP,SENSOR,ARAT,MELTDOWN cpu3: 256KB 64b/line 8-way L2 cache cpu3: smt 1, core 1, package 0 ioapic0 at mainbus0: apid 2 pa 0xfec0, version 20, 120 pins acpiec0 at acpi0 acpimcfg0 at acpi0 addr 0xf000, bus 0-127 acpihpet0 at acpi0: 2399 Hz acpiprt0 at acpi0: bus 0 (PCI0) acpiprt1 at acpi0: bus -1 (PEG0) acpiprt2 a
Re: dwiic(4) fix
> Date: Mon, 21 May 2018 17:25:47 -0700
> From: Mike Larkin
>
> On Mon, May 21, 2018 at 12:44:47PM +0200, Mark Kettenis wrote:
> > The diff below fixes I2C_OP_WRITE_WITH_STOP operations. Currently we
> > run the read completion code for those operations, but since there is
> > no data to read, this fails. Instead, this waits until a stop is
> > detected. That doesn't seem to work for I2C_F_POLL operations though,
> > so in that case we wait until the bus is idle. This also adds
> > interlocks (using splbio/splx) on the existing tsleep() operations.
> >
> > This doesn't fix all the issues with the driver. In particular it
> > seems that large reads still cause problems. I think a somewhat more
> > invasive re-write of the dwiic_i2c_exec() function is needed. But I'd
> > like to get this in before I embark on that.
> >
> > ok?
> >
>
> The diff reads ok to me, do you want us to do any sort of testing here before
> commit?
Testing is always appreciated, and a goood idea if you own a machine
that relies on dwiic(4). For example those with machines that have a
keyboard or touchpad/screen connected over i2c.
Cheers,
Mark
> > Index: dev/ic/dwiic.c
> > ===
> > RCS file: /cvs/src/sys/dev/ic/dwiic.c,v
> > retrieving revision 1.3
> > diff -u -p -r1.3 dwiic.c
> > --- dev/ic/dwiic.c 19 Jan 2018 18:20:38 - 1.3
> > +++ dev/ic/dwiic.c 21 May 2018 10:37:50 -
> > @@ -192,6 +192,7 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
> > u_int32_t ic_con, st, cmd, resp;
> > int retries, tx_limit, rx_avail, x, readpos;
> > uint8_t *b;
> > + int s;
> >
> > if (sc->sc_busy)
> > return 1;
> > @@ -245,12 +246,14 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
> > if (flags & I2C_F_POLL)
> > DELAY(200);
> > else {
> > + s = splbio();
> > dwiic_read(sc, DW_IC_CLR_INTR);
> > dwiic_write(sc, DW_IC_INTR_MASK, DW_IC_INTR_TX_EMPTY);
> >
> > if (tsleep(&sc->sc_writewait, PRIBIO, "dwiic", hz / 2) != 0)
> > printf("%s: timed out waiting for tx_empty intr\n",
> > sc->sc_dev.dv_xname);
> > + splx(s);
> > }
> >
> > /* send our command, one byte at a time */
> > @@ -320,7 +323,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 (tx_limit == 0 || x == len) {
> > + while (I2C_OP_READ_P(op) && (tx_limit == 0 || x == len)) {
> > DPRINTF(("%s: %s: tx_limit %d, sent %d read reqs\n",
> > sc->sc_dev.dv_xname, __func__, tx_limit, x));
> >
> > @@ -332,6 +335,7 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
> > DELAY(50);
> > }
> > } else {
> > + s = splbio();
> > dwiic_read(sc, DW_IC_CLR_INTR);
> > dwiic_write(sc, DW_IC_INTR_MASK,
> > DW_IC_INTR_RX_FULL);
> > @@ -341,6 +345,7 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
> > printf("%s: timed out waiting for "
> > "rx_full intr\n",
> > sc->sc_dev.dv_xname);
> > + splx(s);
> >
> > rx_avail = dwiic_read(sc, DW_IC_RXFLR);
> > }
> > @@ -378,6 +383,32 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
> > }
> > }
> >
> > + if (I2C_OP_STOP_P(op) && I2C_OP_WRITE_P(op)) {
> > + if (flags & I2C_F_POLL) {
> > + /* wait for bus to be idle */
> > + for (retries = 100; retries > 0; retries--) {
> > + st = dwiic_read(sc, DW_IC_STATUS);
> > + if (!(st & DW_IC_STATUS_ACTIVITY))
> > + break;
> > + DELAY(1000);
> > + }
> > + if (st & DW_IC_STATUS_ACTIVITY)
> > + printf("%s: timed out waiting for bus idle\n",
> > + sc->sc_dev.dv_xname);
> > + } else {
> > + s = splbio();
> > + while (sc->sc_busy) {
> > + dwiic_write(sc, DW_IC_INTR_MASK,
> > + DW_IC_INTR_STOP_DET);
> > + if (tsleep(&sc->sc_busy, PRIBIO, "dwiic",
> > + hz / 2) != 0)
> > + printf("%s: timed out waiting for "
> > + "stop intr\n",
> > + sc->sc_dev.dv_xname);
> > + }
> > + splx(s);
> >
Re: dwiic(4) fix
On Mon, May 21, 2018 at 12:44:47PM +0200, Mark Kettenis wrote:
> The diff below fixes I2C_OP_WRITE_WITH_STOP operations. Currently we
> run the read completion code for those operations, but since there is
> no data to read, this fails. Instead, this waits until a stop is
> detected. That doesn't seem to work for I2C_F_POLL operations though,
> so in that case we wait until the bus is idle. This also adds
> interlocks (using splbio/splx) on the existing tsleep() operations.
>
> This doesn't fix all the issues with the driver. In particular it
> seems that large reads still cause problems. I think a somewhat more
> invasive re-write of the dwiic_i2c_exec() function is needed. But I'd
> like to get this in before I embark on that.
>
> ok?
>
The diff reads ok to me, do you want us to do any sort of testing here before
commit?
-ml
>
> Index: dev/ic/dwiic.c
> ===
> RCS file: /cvs/src/sys/dev/ic/dwiic.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 dwiic.c
> --- dev/ic/dwiic.c19 Jan 2018 18:20:38 - 1.3
> +++ dev/ic/dwiic.c21 May 2018 10:37:50 -
> @@ -192,6 +192,7 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
> u_int32_t ic_con, st, cmd, resp;
> int retries, tx_limit, rx_avail, x, readpos;
> uint8_t *b;
> + int s;
>
> if (sc->sc_busy)
> return 1;
> @@ -245,12 +246,14 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
> if (flags & I2C_F_POLL)
> DELAY(200);
> else {
> + s = splbio();
> dwiic_read(sc, DW_IC_CLR_INTR);
> dwiic_write(sc, DW_IC_INTR_MASK, DW_IC_INTR_TX_EMPTY);
>
> if (tsleep(&sc->sc_writewait, PRIBIO, "dwiic", hz / 2) != 0)
> printf("%s: timed out waiting for tx_empty intr\n",
> sc->sc_dev.dv_xname);
> + splx(s);
> }
>
> /* send our command, one byte at a time */
> @@ -320,7 +323,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 (tx_limit == 0 || x == len) {
> + while (I2C_OP_READ_P(op) && (tx_limit == 0 || x == len)) {
> DPRINTF(("%s: %s: tx_limit %d, sent %d read reqs\n",
> sc->sc_dev.dv_xname, __func__, tx_limit, x));
>
> @@ -332,6 +335,7 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
> DELAY(50);
> }
> } else {
> + s = splbio();
> dwiic_read(sc, DW_IC_CLR_INTR);
> dwiic_write(sc, DW_IC_INTR_MASK,
> DW_IC_INTR_RX_FULL);
> @@ -341,6 +345,7 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
> printf("%s: timed out waiting for "
> "rx_full intr\n",
> sc->sc_dev.dv_xname);
> + splx(s);
>
> rx_avail = dwiic_read(sc, DW_IC_RXFLR);
> }
> @@ -378,6 +383,32 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
> }
> }
>
> + if (I2C_OP_STOP_P(op) && I2C_OP_WRITE_P(op)) {
> + if (flags & I2C_F_POLL) {
> + /* wait for bus to be idle */
> + for (retries = 100; retries > 0; retries--) {
> + st = dwiic_read(sc, DW_IC_STATUS);
> + if (!(st & DW_IC_STATUS_ACTIVITY))
> + break;
> + DELAY(1000);
> + }
> + if (st & DW_IC_STATUS_ACTIVITY)
> + printf("%s: timed out waiting for bus idle\n",
> + sc->sc_dev.dv_xname);
> + } else {
> + s = splbio();
> + while (sc->sc_busy) {
> + dwiic_write(sc, DW_IC_INTR_MASK,
> + DW_IC_INTR_STOP_DET);
> + if (tsleep(&sc->sc_busy, PRIBIO, "dwiic",
> + hz / 2) != 0)
> + printf("%s: timed out waiting for "
> + "stop intr\n",
> + sc->sc_dev.dv_xname);
> + }
> + splx(s);
> + }
> + }
> sc->sc_busy = 0;
>
> return 0;
> @@ -449,6 +480,11 @@ dwiic_intr(void *arg)
> DPRINTF(("%s: %s: waking up writer\n",
> sc->sc_dev.dv_xname, __func__));
> wakeup(&sc->sc_writewait);
> + }
> + if (
dwiic(4) fix
The diff below fixes I2C_OP_WRITE_WITH_STOP operations. Currently we
run the read completion code for those operations, but since there is
no data to read, this fails. Instead, this waits until a stop is
detected. That doesn't seem to work for I2C_F_POLL operations though,
so in that case we wait until the bus is idle. This also adds
interlocks (using splbio/splx) on the existing tsleep() operations.
This doesn't fix all the issues with the driver. In particular it
seems that large reads still cause problems. I think a somewhat more
invasive re-write of the dwiic_i2c_exec() function is needed. But I'd
like to get this in before I embark on that.
ok?
Index: dev/ic/dwiic.c
===
RCS file: /cvs/src/sys/dev/ic/dwiic.c,v
retrieving revision 1.3
diff -u -p -r1.3 dwiic.c
--- dev/ic/dwiic.c 19 Jan 2018 18:20:38 - 1.3
+++ dev/ic/dwiic.c 21 May 2018 10:37:50 -
@@ -192,6 +192,7 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
u_int32_t ic_con, st, cmd, resp;
int retries, tx_limit, rx_avail, x, readpos;
uint8_t *b;
+ int s;
if (sc->sc_busy)
return 1;
@@ -245,12 +246,14 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
if (flags & I2C_F_POLL)
DELAY(200);
else {
+ s = splbio();
dwiic_read(sc, DW_IC_CLR_INTR);
dwiic_write(sc, DW_IC_INTR_MASK, DW_IC_INTR_TX_EMPTY);
if (tsleep(&sc->sc_writewait, PRIBIO, "dwiic", hz / 2) != 0)
printf("%s: timed out waiting for tx_empty intr\n",
sc->sc_dev.dv_xname);
+ splx(s);
}
/* send our command, one byte at a time */
@@ -320,7 +323,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 (tx_limit == 0 || x == len) {
+ while (I2C_OP_READ_P(op) && (tx_limit == 0 || x == len)) {
DPRINTF(("%s: %s: tx_limit %d, sent %d read reqs\n",
sc->sc_dev.dv_xname, __func__, tx_limit, x));
@@ -332,6 +335,7 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
DELAY(50);
}
} else {
+ s = splbio();
dwiic_read(sc, DW_IC_CLR_INTR);
dwiic_write(sc, DW_IC_INTR_MASK,
DW_IC_INTR_RX_FULL);
@@ -341,6 +345,7 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
printf("%s: timed out waiting for "
"rx_full intr\n",
sc->sc_dev.dv_xname);
+ splx(s);
rx_avail = dwiic_read(sc, DW_IC_RXFLR);
}
@@ -378,6 +383,32 @@ dwiic_i2c_exec(void *cookie, i2c_op_t op
}
}
+ if (I2C_OP_STOP_P(op) && I2C_OP_WRITE_P(op)) {
+ if (flags & I2C_F_POLL) {
+ /* wait for bus to be idle */
+ for (retries = 100; retries > 0; retries--) {
+ st = dwiic_read(sc, DW_IC_STATUS);
+ if (!(st & DW_IC_STATUS_ACTIVITY))
+ break;
+ DELAY(1000);
+ }
+ if (st & DW_IC_STATUS_ACTIVITY)
+ printf("%s: timed out waiting for bus idle\n",
+ sc->sc_dev.dv_xname);
+ } else {
+ s = splbio();
+ while (sc->sc_busy) {
+ dwiic_write(sc, DW_IC_INTR_MASK,
+ DW_IC_INTR_STOP_DET);
+ if (tsleep(&sc->sc_busy, PRIBIO, "dwiic",
+ hz / 2) != 0)
+ printf("%s: timed out waiting for "
+ "stop intr\n",
+ sc->sc_dev.dv_xname);
+ }
+ splx(s);
+ }
+ }
sc->sc_busy = 0;
return 0;
@@ -449,6 +480,11 @@ dwiic_intr(void *arg)
DPRINTF(("%s: %s: waking up writer\n",
sc->sc_dev.dv_xname, __func__));
wakeup(&sc->sc_writewait);
+ }
+ if (stat & DW_IC_INTR_STOP_DET) {
+ dwiic_write(sc, DW_IC_INTR_MASK, 0);
+ sc->sc_busy = 0;
+ wakeup(&sc->sc_busy);
}
}
