> From: "Stephen Graf" <s_g...@telus.net>
> Date: Thu, 14 Dec 2017 12:54:02 -0800
>
> My recent experience with the dwxe driver has emboldened me to look at the
> sxitwi driver again. A few months ago I worked with Artturi Alm to get the
> driver working with a driver he wrote for a BME280 sensor. At that time I
> tested the non-interrupt option (I2C_F_POLL) only. This time I tried the
> interrupt option as I thought it would be more appropriate in a production
> situation. The non-interrupt option can tie up the system for periods of
> time.
>
> My environment is an orange pi one (Allwinner H3) with one BME280 sensor on
> i2c bus 0 and another on bus 1. Most of my testing was done with a scope on
> the SDA and SCL lines of bus 0 and printf statements in the drivers. The
> BME280 driver does a lot of device calibration reading and setup in the
> attach phase and then reads data once a second into sysctl hw.sensors.
>
> My first issue is that the dtb has the i2c busses disabled. So I had to dtc
> the dtb, enable the busses 0 and 1 and add BME280 sections to the i2c
> busses. The dtb provides a third i2c bus, but the orange pi one does not
> bring it out to the header and so it makes no sense to enable it.
>
> A serious issue that hindered my testing is that the printf statements in
> the driver affected the console driver, garbling the output and often
> stopping all output and input. The output always appeared correctly in the
> message log file. Even now it seems that the console is affected when the
> sxitwi driver is in use and there is a lot of other console output.
>
> Another item I noticed is that the i2c busses were running at half the
> standard speed of 100kHz. The comments in the driver code, which are taken
> right out of the Allwinner doc, set the speed for a 48MHz master clock.
> However, the orange pi one runs at 24MHz.
>
> The sxitwi driver was sprinkled with delays. I took most of them out
> without any side effects.
>
> I added a bus reset function to help in error recovery.
>
> In my testing I left the BME280 driver to do device initialization in
> non-interrupt mode and only changed the cyclic data read to use interrupts.
> This made testing easier as it is hard to capture something on a scope that
> happens only on system boot. Also I am not clear if interrupts are working
> at system boot. It seems that the interrupt controller is set up after the
> sxitwi controller.
>
> It looks like the device status can change after the interrupt is released
> in the interrupt service routine, sxitwi_intr. Therefore I saved the status
> for later use when the driver wakes up in sxitwi_wait. To make that work I
> separated the interrupt and non-interrupt parts of sxitwi_wait and added
> some recovery code.
>
> I took out a lot of the sxitwi_send_stop code because as the comment says
> "Interrupt is not generated" and there is no need to do anything. The next
> send start will wait, or for that matter another master on the bus may be
> trying to do a start and will wait for the stop to complete.
>
> I sort of messed up the formatting (tabs spaces?) with all my changes and I
> hesitate do show a diff. The working code for sxitwi and BNE280 drivers is
> attached. Is the sxitwi driver used by any other devices other than
> Allwinner? Is anyone interested in testing my code?
Hi Stephen,
The sxitwi(4) driver indeed doesn't work very well on the newer SoCs.
But it works fine on my Allwinner A20 board.
As I now have a Banana Pi M2 Berry with an AXPI221a PMIC connected
over I2C I made some time to look into the issue. There are some
things in your diff that didn't make a lot of sense, but while trying
to fix them in a better way, I only managed to make things worse it
seems. So I've taken the radical approach to disable interrupt mode
completely for now. Together with the removal of the the excessive
delays that seems to make things much better on both the A20 and the
V40 boards that periodically read the PMIC registers to update sensor
values. My plan is to commit the diff below unless some other
developer objects to it and then see if I can properly fix interrupt
mode.
Regarding you conclusions and questions above:
Yes, I came to the conclusion that the bus is running at half the
speed as well. The proper solution is to calculate the dividers based
on the input frequency instead of hardcoding them. The input
frequency can be found by calling clock_get_frequency(). I might need
to add a few more clocks to sxiccmu(4) first for this to work, so I'll
address that in a future diff. Running the I2C bus at 50 kHz is
probably fine for most I2C devices.
We do need to make sure the bus is idle after sending a STOP before
starting another transfer. So your change twsi_send_stop() is not ok.
I'm not sure under what circumstances a bus reset would help. Linux
doesn't seem to implement one. So I left that bit out for now.
I think it would be ok to add the BME280 driver to the tree, but it
needs some cleanup first. Normal code should not use symbols that
start with an underscore! I also don't think we should document
registers in the source code when documentation is publically
available.
Cheers,
Mark
Index: dev/fdt/sxitwi.c
===================================================================
RCS file: /cvs/src/sys/dev/fdt/sxitwi.c,v
retrieving revision 1.5
diff -u -p -r1.5 sxitwi.c
--- dev/fdt/sxitwi.c 30 Dec 2017 19:04:00 -0000 1.5
+++ dev/fdt/sxitwi.c 31 Dec 2017 11:36:32 -0000
@@ -128,12 +128,6 @@
#define SOFTRESET_VAL 0 /* reset value */
-#define TWSI_RETRY_COUNT 1000 /* retry loop count */
-#define TWSI_RETRY_DELAY 1 /* retry delay */
-#define TWSI_STAT_DELAY 1 /* poll status delay */
-#define TWSI_READ_DELAY 2 /* read delay */
-#define TWSI_WRITE_DELAY 2 /* write delay */
-
struct sxitwi_softc {
struct device sc_dev;
bus_space_tag_t sc_iot;
@@ -291,21 +285,13 @@ sxitwi_bus_scan(struct device *self, str
u_int
sxitwi_read_4(struct sxitwi_softc *sc, u_int reg)
{
- u_int val = bus_space_read_4(sc->sc_iot, sc->sc_ioh, reg);
-
- delay(TWSI_READ_DELAY);
-
- return val;
+ return bus_space_read_4(sc->sc_iot, sc->sc_ioh, reg);
}
void
sxitwi_write_4(struct sxitwi_softc *sc, u_int reg, u_int val)
{
bus_space_write_4(sc->sc_iot, sc->sc_ioh, reg, val);
-
- delay(TWSI_WRITE_DELAY);
-
- return;
}
int
@@ -317,7 +303,6 @@ sxitwi_intr(void *arg)
val = sxitwi_read_4(sc, TWSI_CONTROL);
if (val & CONTROL_IFLG) {
sxitwi_write_4(sc, TWSI_CONTROL, val & ~CONTROL_INTEN);
- wakeup(&sc->sc_dev);
return 1;
}
return 0;
@@ -364,19 +349,19 @@ int
sxitwi_send_stop(void *v, int flags)
{
struct sxitwi_softc *sc = v;
- int retry = TWSI_RETRY_COUNT;
+ int timo;
sc->sc_started = 0;
/* Interrupt is not generated for STAT_NRS. */
sxitwi_write_4(sc, TWSI_CONTROL, CONTROL_STOP | sc->sc_twsien_iflg);
- while (--retry > 0) {
+ for (timo = 100; timo > 0; timo--) {
if (sxitwi_read_4(sc, TWSI_STATUS) == STAT_NRS)
return 0;
- delay(TWSI_STAT_DELAY);
+ delay(1);
}
- return -1;
+ return ETIMEDOUT;
}
int
@@ -458,30 +443,21 @@ int
sxitwi_wait(struct sxitwi_softc *sc, u_int control, u_int expect, int flags)
{
u_int status;
- int timo, error = 0;
+ int timo;
- delay(5);
- if (!(flags & I2C_F_POLL))
- control |= CONTROL_INTEN;
sxitwi_write_4(sc, TWSI_CONTROL, control | sc->sc_twsien_iflg);
- timo = 0;
- do {
+ for (timo = 10000; timo > 0; timo--) {
control = sxitwi_read_4(sc, TWSI_CONTROL);
if (control & CONTROL_IFLG)
break;
- if (flags & I2C_F_POLL)
- delay(TWSI_RETRY_DELAY);
- else {
- error = tsleep(&sc->sc_dev, PWAIT, "sxitwi", 100);
- if (error)
- return error;
- }
- } while (++timo < 1000000);
+ delay(1);
+ }
+ if (timo == 0)
+ return ETIMEDOUT;
status = sxitwi_read_4(sc, TWSI_STATUS);
if (status != expect)
return EIO;
-
- return error;
+ return 0;
}
