sxitwi on orangepi one (H3) re-visited

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

sxitwi on orangepi one (H3) re-visited

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

bme280.c (19K) Download Attachment
sxitwi.c (17K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: sxitwi on orangepi one (H3) re-visited

Mark Kettenis
> From: "Stephen Graf" <[hidden email]>
> 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;
 }

Reply | Threaded
Open this post in threaded view
|

Re: sxitwi on orangepi one (H3) re-visited

s_graf
In reply to this post by s_graf
Thank you for looking at my code. I will try your code on my orange pi one
in the near future.

Regarding the wait for bus free a on stop condition: After a stop, the only
thing that can be done is a start.  When the start bit is set the controller
"will transmit a START condition on the bus when the bus is free".  So the
wait is done by the hardware in the start routine.  It is necessary for the
start hardware to wait for a free bus as it could be a different independent
master waiting to grab the bus. Have you tested without any stop delay?  It
works, as it should, on my system.

Regarding the bus reset:  When (not if!) an error occurs, it is important to
reset to a known default state.  This incudes not only the hardware but also
flags such as the started flag.  In my testing, I had an issue with the
power lead to the sensor on my breadboard being intermittent.  Without the
reset the sensor would often permanently quit on an error.  With the reset
it would work again on the next read.
My programming experience dates back to writing assembler code on mini
computers to control monstrous paper making machines. Things would often go
wrong and sending an error message to a remote console was never an
acceptable solution.

Regarding the bus clock speed:  Ideally this should be as parameter set
outside of the driver on system startup.  Different applications require
different bus speeds and one bus should be able to run at a different speed
than another. It should not be necessary to recompile the kernel to change
the bus speed.

Does the existing driver work with interrupts on any of your hardware?

-----Original Message-----
From: Mark Kettenis [mailto:[hidden email]]
Sent: Sunday, December 31, 2017 4:06 AM
To: [hidden email]
Cc: [hidden email]; [hidden email]; [hidden email]
Subject: Re: sxitwi on orangepi one (H3) re-visited

> From: "Stephen Graf" <[hidden email]>
> 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;
 }


Reply | Threaded
Open this post in threaded view
|

Re: sxitwi on orangepi one (H3) re-visited

s_graf
In reply to this post by s_graf
I did some further testing with interrupts and set the BME280 device to use
interrupts on setup.  This occurs in the attach phase on system boot.  It
did not work even after I added an early start to the ampintc driver:

sxipio0 at simplebus0: 94 pins
ampintc0 at simplebus0 nirq 160, ncpu 4
sxipio1 at simplebus0: 12 pins

I suspect that interrupts are not set up on system boot so I went back to
the BME280 code that does device setup non-interrupt and cyclic read with
interrupt.

I am not happy to rip out the interrupt code that I have tested and works on
the H3 device.  I looked at the V40 docs and they seem to be identical to
the H3 for the TWI.  If the code does not meet openBSD standards I am quite
willing to work to get it right.  The code I provided also works in
non-interrupt mode.

Could we work together on the interrupt code and get the whole driver
working properly?  It should not be a major effort.

As I mentioned above there is an issue trying to use interrupts on system
boot.

-----Original Message-----
From: [hidden email] [mailto:[hidden email]] On Behalf Of
Stephen Graf
Sent: Sunday, December 31, 2017 11:57 AM
To: 'Mark Kettenis' <[hidden email]>
Cc: [hidden email]; [hidden email]
Subject: Re: sxitwi on orangepi one (H3) re-visited

Thank you for looking at my code. I will try your code on my orange pi one
in the near future.

Regarding the wait for bus free a on stop condition: After a stop, the only
thing that can be done is a start.  When the start bit is set the controller
"will transmit a START condition on the bus when the bus is free".  So the
wait is done by the hardware in the start routine.  It is necessary for the
start hardware to wait for a free bus as it could be a different independent
master waiting to grab the bus. Have you tested without any stop delay?  It
works, as it should, on my system.

Regarding the bus reset:  When (not if!) an error occurs, it is important to
reset to a known default state.  This incudes not only the hardware but also
flags such as the started flag.  In my testing, I had an issue with the
power lead to the sensor on my breadboard being intermittent.  Without the
reset the sensor would often permanently quit on an error.  With the reset
it would work again on the next read.
My programming experience dates back to writing assembler code on mini
computers to control monstrous paper making machines. Things would often go
wrong and sending an error message to a remote console was never an
acceptable solution.

Regarding the bus clock speed:  Ideally this should be as parameter set
outside of the driver on system startup.  Different applications require
different bus speeds and one bus should be able to run at a different speed
than another. It should not be necessary to recompile the kernel to change
the bus speed.

Does the existing driver work with interrupts on any of your hardware?

-----Original Message-----
From: Mark Kettenis [mailto:[hidden email]]
Sent: Sunday, December 31, 2017 4:06 AM
To: [hidden email]
Cc: [hidden email]; [hidden email]; [hidden email]
Subject: Re: sxitwi on orangepi one (H3) re-visited

> From: "Stephen Graf" <[hidden email]>
> 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;
 }



Reply | Threaded
Open this post in threaded view
|

Re: sxitwi on orangepi one (H3) re-visited

Mark Kettenis
> From: "Stephen Graf" <[hidden email]>
> Date: Tue, 2 Jan 2018 11:16:18 -0800
>
> I did some further testing with interrupts and set the BME280 device to use
> interrupts on setup.  This occurs in the attach phase on system boot.  It
> did not work even after I added an early start to the ampintc driver:
>
> sxipio0 at simplebus0: 94 pins
> ampintc0 at simplebus0 nirq 160, ncpu 4
> sxipio1 at simplebus0: 12 pins
>
> I suspect that interrupts are not set up on system boot so I went back to
> the BME280 code that does device setup non-interrupt and cyclic read with
> interrupt.

Yes.  On OpenBSD, interrupts remain disabled until late in the
autoconf procedure.  Up until that point, the global variable "cold"
remains set to 1.  This is why many drivers check this variable and
poll for command completion as long as cold is non-zero.

> I am not happy to rip out the interrupt code that I have tested and works on
> the H3 device.  I looked at the V40 docs and they seem to be identical to
> the H3 for the TWI.  If the code does not meet openBSD standards I am quite
> willing to work to get it right.  The code I provided also works in
> non-interrupt mode.

Yes the hardware blocks seem to be identical.

> Could we work together on the interrupt code and get the whole driver
> working properly?  It should not be a major effort.

Certainly.  I'm just proposing to remove the bogus code such that we
can see things a bit more clearly.

>
> As I mentioned above there is an issue trying to use interrupts on system
> boot.
>
> -----Original Message-----
> From: [hidden email] [mailto:[hidden email]] On Behalf Of
> Stephen Graf
> Sent: Sunday, December 31, 2017 11:57 AM
> To: 'Mark Kettenis' <[hidden email]>
> Cc: [hidden email]; [hidden email]
> Subject: Re: sxitwi on orangepi one (H3) re-visited
>
> Thank you for looking at my code. I will try your code on my orange pi one
> in the near future.
>
> Regarding the wait for bus free a on stop condition: After a stop, the only
> thing that can be done is a start.  When the start bit is set the controller
> "will transmit a START condition on the bus when the bus is free".  So the
> wait is done by the hardware in the start routine.  It is necessary for the
> start hardware to wait for a free bus as it could be a different independent
> master waiting to grab the bus. Have you tested without any stop delay?  It
> works, as it should, on my system.

I did test this on the A20 system, and it failed there.  But my
testing may have been flawed.  I'll re-test.

> Regarding the bus reset:  When (not if!) an error occurs, it is important to
> reset to a known default state.  This incudes not only the hardware but also
> flags such as the started flag.  In my testing, I had an issue with the
> power lead to the sensor on my breadboard being intermittent.  Without the
> reset the sensor would often permanently quit on an error.  With the reset
> it would work again on the next read.

Right.  What state did you end up in when this happens?

> My programming experience dates back to writing assembler code on mini
> computers to control monstrous paper making machines. Things would often go
> wrong and sending an error message to a remote console was never an
> acceptable solution.
>
> Regarding the bus clock speed:  Ideally this should be as parameter set
> outside of the driver on system startup.  Different applications require
> different bus speeds and one bus should be able to run at a different speed
> than another. It should not be necessary to recompile the kernel to change
> the bus speed.
>
> Does the existing driver work with interrupts on any of your hardware?

Yes.  It works on the A20, but not on the V40.

> -----Original Message-----
> From: Mark Kettenis [mailto:[hidden email]]
> Sent: Sunday, December 31, 2017 4:06 AM
> To: [hidden email]
> Cc: [hidden email]; [hidden email]; [hidden email]
> Subject: Re: sxitwi on orangepi one (H3) re-visited
>
> > From: "Stephen Graf" <[hidden email]>
> > 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;
>  }
>
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: sxitwi on orangepi one (H3) re-visited

s_graf
In reply to this post by s_graf
I tested your simplification changes with success.

Then I removed the wait code in send stop, retested and again had success.
You should be able to convince yourself that it is not necessary to wait for
the stop code to complete, since there is no status code nor interrupt for
stop complete. Also from the Allwinner docs, it is possible to assert the
stop and start bits at the same time, again implying that the software does
not have to wait for the stop.  The i2c protocol supports multiple masters
and so a potential master must monitor the bus and wait for a bus idle
condition (which a stop generates), before attempting a start.  The
Allwinner hardware does the idle bus check and wait in the start bit
processing. Since the only thing that is possible after a stop is a start,
there seems to be no reason for the software to wait for the stop to
complete and that is why there is no hardware to check for a stop to
complete.

Regarding the bus speed, I would like to recommend that bus speed
information be put in the dtb.  It could be as simple as specifying the M
and N values.  The driver could generate defaults if there is nothing
specified in the dtb.  I do not think there should be a lot of code in the
driver to try to calculate a speed from the master clock.  The dtb for the
orange pi one comes with the i2c busses disabled and I have to edit it
anyway to enable them and add the i2c devices.

I then went on and added/modified the driver for interrupts and successfully
tested.  I tested with two bme280 sensors.  The setup code on the bme280
driver is done at boot (attach) time and has to use polling, while the
cyclic ongoing read of the sensors is done with interrupts.

Console boot extract:
com0: console
sxitwi0 at simplebus0
iic0 at sxitwi0
bme0 at iic0 addr 0x77: BME280 60
sxitwi1 at simplebus0
iic1 at sxitwi1
bme1 at iic1 addr 0x76: BME280 60
ampintc0 at simplebus0 nirq 160, ncpu 4
sxirtc0 at simplebus0

Sensor readout:
openbsdop1$ sysctl hw.sensors
hw.sensors.bme0.temp0=21.89 degC
hw.sensors.bme0.humidity0=35.23%
hw.sensors.bme0.pressure0=100.79 Pa
hw.sensors.bme1.temp0=20.83 degC
hw.sensors.bme1.humidity0=40.87%
hw.sensors.bme1.pressure0=100.83 Pa
openbsdop1$

Attached are files for a diff from ver 1.5, the same diff with notes and the
entire driver code as tested.  Would you please review the code and test if
possible on any devices you have.  If you have questions/suggestions please
reply.

-----Original Message-----
From: [hidden email] [mailto:[hidden email]] On Behalf Of Mark
Kettenis
Sent: Wednesday, January 3, 2018 1:03 PM
To: [hidden email]
Cc: [hidden email]; [hidden email]
Subject: Re: sxitwi on orangepi one (H3) re-visited

> From: "Stephen Graf" <[hidden email]>
> Date: Tue, 2 Jan 2018 11:16:18 -0800
>
> I did some further testing with interrupts and set the BME280 device
> to use interrupts on setup.  This occurs in the attach phase on system
> boot.  It did not work even after I added an early start to the ampintc
driver:
>
> sxipio0 at simplebus0: 94 pins
> ampintc0 at simplebus0 nirq 160, ncpu 4
> sxipio1 at simplebus0: 12 pins
>
> I suspect that interrupts are not set up on system boot so I went back
> to the BME280 code that does device setup non-interrupt and cyclic
> read with interrupt.

Yes.  On OpenBSD, interrupts remain disabled until late in the autoconf
procedure.  Up until that point, the global variable "cold"
remains set to 1.  This is why many drivers check this variable and poll for
command completion as long as cold is non-zero.

> I am not happy to rip out the interrupt code that I have tested and
> works on the H3 device.  I looked at the V40 docs and they seem to be
> identical to the H3 for the TWI.  If the code does not meet openBSD
> standards I am quite willing to work to get it right.  The code I
> provided also works in non-interrupt mode.

Yes the hardware blocks seem to be identical.

> Could we work together on the interrupt code and get the whole driver
> working properly?  It should not be a major effort.

Certainly.  I'm just proposing to remove the bogus code such that we can see
things a bit more clearly.

>
> As I mentioned above there is an issue trying to use interrupts on
> system boot.
>
> -----Original Message-----
> From: [hidden email] [mailto:[hidden email]] On Behalf
> Of Stephen Graf
> Sent: Sunday, December 31, 2017 11:57 AM
> To: 'Mark Kettenis' <[hidden email]>
> Cc: [hidden email]; [hidden email]
> Subject: Re: sxitwi on orangepi one (H3) re-visited
>
> Thank you for looking at my code. I will try your code on my orange pi
> one in the near future.
>
> Regarding the wait for bus free a on stop condition: After a stop, the
> only thing that can be done is a start.  When the start bit is set the
> controller "will transmit a START condition on the bus when the bus is
> free".  So the wait is done by the hardware in the start routine.  It
> is necessary for the start hardware to wait for a free bus as it could
> be a different independent master waiting to grab the bus. Have you
> tested without any stop delay?  It works, as it should, on my system.
I did test this on the A20 system, and it failed there.  But my testing may
have been flawed.  I'll re-test.

> Regarding the bus reset:  When (not if!) an error occurs, it is
> important to reset to a known default state.  This incudes not only
> the hardware but also flags such as the started flag.  In my testing,
> I had an issue with the power lead to the sensor on my breadboard
> being intermittent.  Without the reset the sensor would often
> permanently quit on an error.  With the reset it would work again on the
next read.

Right.  What state did you end up in when this happens?

> My programming experience dates back to writing assembler code on mini
> computers to control monstrous paper making machines. Things would
> often go wrong and sending an error message to a remote console was
> never an acceptable solution.
>
> Regarding the bus clock speed:  Ideally this should be as parameter
> set outside of the driver on system startup.  Different applications
> require different bus speeds and one bus should be able to run at a
> different speed than another. It should not be necessary to recompile
> the kernel to change the bus speed.
>
> Does the existing driver work with interrupts on any of your hardware?
Yes.  It works on the A20, but not on the V40.

> -----Original Message-----
> From: Mark Kettenis [mailto:[hidden email]]
> Sent: Sunday, December 31, 2017 4:06 AM
> To: [hidden email]
> Cc: [hidden email]; [hidden email]; [hidden email]
> Subject: Re: sxitwi on orangepi one (H3) re-visited
>
> > From: "Stephen Graf" <[hidden email]>
> > 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;
>  }
>
>
>
>


sxitwi.c (16K) Download Attachment
switwi_interrupt_diff_annotated.txt (9K) Download Attachment
switwi_interrupt_diff.txt (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: sxitwi on orangepi one (H3) re-visited

Mark Kettenis
> From: "Stephen Graf" <[hidden email]>
> Date: Thu, 4 Jan 2018 16:23:56 -0800
>
> I tested your simplification changes with success.
>
> Then I removed the wait code in send stop, retested and again had success.
> You should be able to convince yourself that it is not necessary to wait for
> the stop code to complete, since there is no status code nor interrupt for
> stop complete. Also from the Allwinner docs, it is possible to assert the
> stop and start bits at the same time, again implying that the software does
> not have to wait for the stop.  The i2c protocol supports multiple masters
> and so a potential master must monitor the bus and wait for a bus idle
> condition (which a stop generates), before attempting a start.  The
> Allwinner hardware does the idle bus check and wait in the start bit
> processing. Since the only thing that is possible after a stop is a start,
> there seems to be no reason for the software to wait for the stop to
> complete and that is why there is no hardware to check for a stop to
> complete.

I re-tested the A20 yesterday and removing the polling loop after
sending a STOP works just fine there as well.  So I adjusted my diff
an committed it.  That should allow us to focus on the other issues.
We really try to seperate out issues and address them with individual
patches.

> Regarding the bus speed, I would like to recommend that bus speed
> information be put in the dtb.  It could be as simple as specifying the M
> and N values.  The driver could generate defaults if there is nothing
> specified in the dtb.  I do not think there should be a lot of code in the
> driver to try to calculate a speed from the master clock.  The dtb for the
> orange pi one comes with the i2c busses disabled and I have to edit it
> anyway to enable them and add the i2c devices.

The i2c bindings allow you to add a "clock-frequency" property to the
i2c controller node to specify the desired bus clock frequency.  See
Documentation/devicetree/bindings/i2c/{i2c.txt|i2c-mv64xxx.txt} in the
Linux source tree for details.  If the property is absent the standard
speed of 100000 Hz is assumed.  While none of the Allwinner device
trees seem to include this property, it is easy to look for it anyway.

> I then went on and added/modified the driver for interrupts and successfully
> tested.  I tested with two bme280 sensors.  The setup code on the bme280
> driver is done at boot (attach) time and has to use polling, while the
> cyclic ongoing read of the sensors is done with interrupts.
>
> Console boot extract:
> com0: console
> sxitwi0 at simplebus0
> iic0 at sxitwi0
> bme0 at iic0 addr 0x77: BME280 60
> sxitwi1 at simplebus0
> iic1 at sxitwi1
> bme1 at iic1 addr 0x76: BME280 60
> ampintc0 at simplebus0 nirq 160, ncpu 4
> sxirtc0 at simplebus0
>
> Sensor readout:
> openbsdop1$ sysctl hw.sensors
> hw.sensors.bme0.temp0=21.89 degC
> hw.sensors.bme0.humidity0=35.23%
> hw.sensors.bme0.pressure0=100.79 Pa
> hw.sensors.bme1.temp0=20.83 degC
> hw.sensors.bme1.humidity0=40.87%
> hw.sensors.bme1.pressure0=100.83 Pa
> openbsdop1$
>
> Attached are files for a diff from ver 1.5, the same diff with notes and the
> entire driver code as tested.  Would you please review the code and test if
> possible on any devices you have.  If you have questions/suggestions please
> reply.

I think I know how to re-implement the interrupt support properly.
Currently testing this approach.  I'll also handle the bus clock speed
issue as I know how to do that properly now.  But if you could at some
point send me a new diff for adding the reset functionality, that
would be great.  Please use diff -up to generate your diffs and avoid
making arbitrary whitespace changes.  We use tabs instead of spaces
when possible and not following our standards results in noisy diffs
that are harder to review.

Cheers,

Mark

> -----Original Message-----
> From: [hidden email] [mailto:[hidden email]] On Behalf Of Mark
> Kettenis
> Sent: Wednesday, January 3, 2018 1:03 PM
> To: [hidden email]
> Cc: [hidden email]; [hidden email]
> Subject: Re: sxitwi on orangepi one (H3) re-visited
>
> > From: "Stephen Graf" <[hidden email]>
> > Date: Tue, 2 Jan 2018 11:16:18 -0800
> >
> > I did some further testing with interrupts and set the BME280 device
> > to use interrupts on setup.  This occurs in the attach phase on system
> > boot.  It did not work even after I added an early start to the ampintc
> driver:
> >
> > sxipio0 at simplebus0: 94 pins
> > ampintc0 at simplebus0 nirq 160, ncpu 4
> > sxipio1 at simplebus0: 12 pins
> >
> > I suspect that interrupts are not set up on system boot so I went back
> > to the BME280 code that does device setup non-interrupt and cyclic
> > read with interrupt.
>
> Yes.  On OpenBSD, interrupts remain disabled until late in the autoconf
> procedure.  Up until that point, the global variable "cold"
> remains set to 1.  This is why many drivers check this variable and poll for
> command completion as long as cold is non-zero.
>
> > I am not happy to rip out the interrupt code that I have tested and
> > works on the H3 device.  I looked at the V40 docs and they seem to be
> > identical to the H3 for the TWI.  If the code does not meet openBSD
> > standards I am quite willing to work to get it right.  The code I
> > provided also works in non-interrupt mode.
>
> Yes the hardware blocks seem to be identical.
>
> > Could we work together on the interrupt code and get the whole driver
> > working properly?  It should not be a major effort.
>
> Certainly.  I'm just proposing to remove the bogus code such that we can see
> things a bit more clearly.
> >
> > As I mentioned above there is an issue trying to use interrupts on
> > system boot.
> >
> > -----Original Message-----
> > From: [hidden email] [mailto:[hidden email]] On Behalf
> > Of Stephen Graf
> > Sent: Sunday, December 31, 2017 11:57 AM
> > To: 'Mark Kettenis' <[hidden email]>
> > Cc: [hidden email]; [hidden email]
> > Subject: Re: sxitwi on orangepi one (H3) re-visited
> >
> > Thank you for looking at my code. I will try your code on my orange pi
> > one in the near future.
> >
> > Regarding the wait for bus free a on stop condition: After a stop, the
> > only thing that can be done is a start.  When the start bit is set the
> > controller "will transmit a START condition on the bus when the bus is
> > free".  So the wait is done by the hardware in the start routine.  It
> > is necessary for the start hardware to wait for a free bus as it could
> > be a different independent master waiting to grab the bus. Have you
> > tested without any stop delay?  It works, as it should, on my system.
>
> I did test this on the A20 system, and it failed there.  But my testing may
> have been flawed.  I'll re-test.
>
> > Regarding the bus reset:  When (not if!) an error occurs, it is
> > important to reset to a known default state.  This incudes not only
> > the hardware but also flags such as the started flag.  In my testing,
> > I had an issue with the power lead to the sensor on my breadboard
> > being intermittent.  Without the reset the sensor would often
> > permanently quit on an error.  With the reset it would work again on the
> next read.
>
> Right.  What state did you end up in when this happens?
>
> > My programming experience dates back to writing assembler code on mini
> > computers to control monstrous paper making machines. Things would
> > often go wrong and sending an error message to a remote console was
> > never an acceptable solution.
> >
> > Regarding the bus clock speed:  Ideally this should be as parameter
> > set outside of the driver on system startup.  Different applications
> > require different bus speeds and one bus should be able to run at a
> > different speed than another. It should not be necessary to recompile
> > the kernel to change the bus speed.
> >
> > Does the existing driver work with interrupts on any of your hardware?
>
> Yes.  It works on the A20, but not on the V40.
>
> > -----Original Message-----
> > From: Mark Kettenis [mailto:[hidden email]]
> > Sent: Sunday, December 31, 2017 4:06 AM
> > To: [hidden email]
> > Cc: [hidden email]; [hidden email]; [hidden email]
> > Subject: Re: sxitwi on orangepi one (H3) re-visited
> >
> > > From: "Stephen Graf" <[hidden email]>
> > > 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;
> >  }
> >
> >
> >
> >
>
>
> ------=_NextPart_000_009A_01D38578.770321A0
> Content-Type: text/plain; charset=us-ascii
> X-Former-Content-Type: application/octet-stream;
> name="sxitwi.c"
> Content-Transfer-Encoding: 7bit
> X-Former-Content-Transfer-Encoding: quoted-printable
> X-Former-Content-Disposition: attachment;
> filename="sxitwi.c"
>
> [file:/home/kettenis/detached/sxitwi_0002.c]
> ------=_NextPart_000_009A_01D38578.770321A0
> Content-Type: text/plain; charset=us-ascii
> X-Former-Content-Type: text/plain;
> name="switwi_interrupt_diff_annotated.txt"
> Content-Transfer-Encoding: 7bit
> X-Former-Content-Transfer-Encoding: quoted-printable
> X-Former-Content-Disposition: attachment;
> filename="switwi_interrupt_diff_annotated.txt"
>
> [file:/home/kettenis/detached/switwi_interrupt_diff_annotated_0001.txt]
> ------=_NextPart_000_009A_01D38578.770321A0
> Content-Type: text/plain; charset=us-ascii
> X-Former-Content-Type: text/plain;
> name="switwi_interrupt_diff.txt"
> Content-Transfer-Encoding: 7bit
> X-Former-Content-Transfer-Encoding: quoted-printable
> X-Former-Content-Disposition: attachment;
> filename="switwi_interrupt_diff.txt"
>
> [file:/home/kettenis/detached/switwi_interrupt_diff_0001.txt]
> ------=_NextPart_000_009A_01D38578.770321A0--
>
>

Reply | Threaded
Open this post in threaded view
|

Re: sxitwi on orangepi one (H3) re-visited

s_graf
In reply to this post by s_graf
Thank you for the work you are doing on this driver.

Attached is a diff to ver 1.6 that implements the bus reset on error.  I
tested this on my orange pi one.

-----Original Message-----
From: [hidden email] [mailto:[hidden email]] On Behalf Of Mark
Kettenis
Sent: Friday, January 5, 2018 5:23 AM
To: [hidden email]
Cc: [hidden email]; [hidden email]
Subject: Re: sxitwi on orangepi one (H3) re-visited

> From: "Stephen Graf" <[hidden email]>
> Date: Thu, 4 Jan 2018 16:23:56 -0800
>
> I tested your simplification changes with success.
>
> Then I removed the wait code in send stop, retested and again had success.
> You should be able to convince yourself that it is not necessary to
> wait for the stop code to complete, since there is no status code nor
> interrupt for stop complete. Also from the Allwinner docs, it is
> possible to assert the stop and start bits at the same time, again
> implying that the software does not have to wait for the stop.  The
> i2c protocol supports multiple masters and so a potential master must
> monitor the bus and wait for a bus idle condition (which a stop
> generates), before attempting a start.  The Allwinner hardware does
> the idle bus check and wait in the start bit processing. Since the
> only thing that is possible after a stop is a start, there seems to be
> no reason for the software to wait for the stop to complete and that
> is why there is no hardware to check for a stop to complete.
I re-tested the A20 yesterday and removing the polling loop after sending a
STOP works just fine there as well.  So I adjusted my diff an committed it.
That should allow us to focus on the other issues.
We really try to seperate out issues and address them with individual
patches.

> Regarding the bus speed, I would like to recommend that bus speed
> information be put in the dtb.  It could be as simple as specifying
> the M and N values.  The driver could generate defaults if there is
> nothing specified in the dtb.  I do not think there should be a lot of
> code in the driver to try to calculate a speed from the master clock.  
> The dtb for the orange pi one comes with the i2c busses disabled and I
> have to edit it anyway to enable them and add the i2c devices.

The i2c bindings allow you to add a "clock-frequency" property to the i2c
controller node to specify the desired bus clock frequency.  See
Documentation/devicetree/bindings/i2c/{i2c.txt|i2c-mv64xxx.txt} in the Linux
source tree for details.  If the property is absent the standard speed of
100000 Hz is assumed.  While none of the Allwinner device trees seem to
include this property, it is easy to look for it anyway.

> I then went on and added/modified the driver for interrupts and
> successfully tested.  I tested with two bme280 sensors.  The setup
> code on the bme280 driver is done at boot (attach) time and has to use
> polling, while the cyclic ongoing read of the sensors is done with
interrupts.

>
> Console boot extract:
> com0: console
> sxitwi0 at simplebus0
> iic0 at sxitwi0
> bme0 at iic0 addr 0x77: BME280 60
> sxitwi1 at simplebus0
> iic1 at sxitwi1
> bme1 at iic1 addr 0x76: BME280 60
> ampintc0 at simplebus0 nirq 160, ncpu 4
> sxirtc0 at simplebus0
>
> Sensor readout:
> openbsdop1$ sysctl hw.sensors
> hw.sensors.bme0.temp0=21.89 degC
> hw.sensors.bme0.humidity0=35.23%
> hw.sensors.bme0.pressure0=100.79 Pa
> hw.sensors.bme1.temp0=20.83 degC
> hw.sensors.bme1.humidity0=40.87%
> hw.sensors.bme1.pressure0=100.83 Pa
> openbsdop1$
>
> Attached are files for a diff from ver 1.5, the same diff with notes
> and the entire driver code as tested.  Would you please review the
> code and test if possible on any devices you have.  If you have
> questions/suggestions please reply.
I think I know how to re-implement the interrupt support properly.
Currently testing this approach.  I'll also handle the bus clock speed issue
as I know how to do that properly now.  But if you could at some point send
me a new diff for adding the reset functionality, that would be great.
Please use diff -up to generate your diffs and avoid making arbitrary
whitespace changes.  We use tabs instead of spaces when possible and not
following our standards results in noisy diffs that are harder to review.

Cheers,

Mark

> -----Original Message-----
> From: [hidden email] [mailto:[hidden email]] On Behalf
> Of Mark Kettenis
> Sent: Wednesday, January 3, 2018 1:03 PM
> To: [hidden email]
> Cc: [hidden email]; [hidden email]
> Subject: Re: sxitwi on orangepi one (H3) re-visited
>
> > From: "Stephen Graf" <[hidden email]>
> > Date: Tue, 2 Jan 2018 11:16:18 -0800
> >
> > I did some further testing with interrupts and set the BME280 device
> > to use interrupts on setup.  This occurs in the attach phase on
> > system boot.  It did not work even after I added an early start to
> > the ampintc
> driver:
> >
> > sxipio0 at simplebus0: 94 pins
> > ampintc0 at simplebus0 nirq 160, ncpu 4
> > sxipio1 at simplebus0: 12 pins
> >
> > I suspect that interrupts are not set up on system boot so I went
> > back to the BME280 code that does device setup non-interrupt and
> > cyclic read with interrupt.
>
> Yes.  On OpenBSD, interrupts remain disabled until late in the
> autoconf procedure.  Up until that point, the global variable "cold"
> remains set to 1.  This is why many drivers check this variable and
> poll for command completion as long as cold is non-zero.
>
> > I am not happy to rip out the interrupt code that I have tested and
> > works on the H3 device.  I looked at the V40 docs and they seem to
> > be identical to the H3 for the TWI.  If the code does not meet
> > openBSD standards I am quite willing to work to get it right.  The
> > code I provided also works in non-interrupt mode.
>
> Yes the hardware blocks seem to be identical.
>
> > Could we work together on the interrupt code and get the whole
> > driver working properly?  It should not be a major effort.
>
> Certainly.  I'm just proposing to remove the bogus code such that we
> can see things a bit more clearly.
> >
> > As I mentioned above there is an issue trying to use interrupts on
> > system boot.
> >
> > -----Original Message-----
> > From: [hidden email] [mailto:[hidden email]] On Behalf
> > Of Stephen Graf
> > Sent: Sunday, December 31, 2017 11:57 AM
> > To: 'Mark Kettenis' <[hidden email]>
> > Cc: [hidden email]; [hidden email]
> > Subject: Re: sxitwi on orangepi one (H3) re-visited
> >
> > Thank you for looking at my code. I will try your code on my orange
> > pi one in the near future.
> >
> > Regarding the wait for bus free a on stop condition: After a stop,
> > the only thing that can be done is a start.  When the start bit is
> > set the controller "will transmit a START condition on the bus when
> > the bus is free".  So the wait is done by the hardware in the start
> > routine.  It is necessary for the start hardware to wait for a free
> > bus as it could be a different independent master waiting to grab
> > the bus. Have you tested without any stop delay?  It works, as it
should, on my system.

>
> I did test this on the A20 system, and it failed there.  But my
> testing may have been flawed.  I'll re-test.
>
> > Regarding the bus reset:  When (not if!) an error occurs, it is
> > important to reset to a known default state.  This incudes not only
> > the hardware but also flags such as the started flag.  In my
> > testing, I had an issue with the power lead to the sensor on my
> > breadboard being intermittent.  Without the reset the sensor would
> > often permanently quit on an error.  With the reset it would work
> > again on the
> next read.
>
> Right.  What state did you end up in when this happens?
>
> > My programming experience dates back to writing assembler code on
> > mini computers to control monstrous paper making machines. Things
> > would often go wrong and sending an error message to a remote
> > console was never an acceptable solution.
> >
> > Regarding the bus clock speed:  Ideally this should be as parameter
> > set outside of the driver on system startup.  Different applications
> > require different bus speeds and one bus should be able to run at a
> > different speed than another. It should not be necessary to
> > recompile the kernel to change the bus speed.
> >
> > Does the existing driver work with interrupts on any of your hardware?
>
> Yes.  It works on the A20, but not on the V40.
>
> > -----Original Message-----
> > From: Mark Kettenis [mailto:[hidden email]]
> > Sent: Sunday, December 31, 2017 4:06 AM
> > To: [hidden email]
> > Cc: [hidden email]; [hidden email]; [hidden email]
> > Subject: Re: sxitwi on orangepi one (H3) re-visited
> >
> > > From: "Stephen Graf" <[hidden email]>
> > > 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;
> >  }
> >
> >
> >
> >
>
>
> ------=_NextPart_000_009A_01D38578.770321A0
> Content-Type: text/plain; charset=us-ascii
> X-Former-Content-Type: application/octet-stream;
> name="sxitwi.c"
> Content-Transfer-Encoding: 7bit
> X-Former-Content-Transfer-Encoding: quoted-printable
> X-Former-Content-Disposition: attachment;
> filename="sxitwi.c"
>
> [file:/home/kettenis/detached/sxitwi_0002.c]
> ------=_NextPart_000_009A_01D38578.770321A0
> Content-Type: text/plain; charset=us-ascii
> X-Former-Content-Type: text/plain;
> name="switwi_interrupt_diff_annotated.txt"
> Content-Transfer-Encoding: 7bit
> X-Former-Content-Transfer-Encoding: quoted-printable
> X-Former-Content-Disposition: attachment;
> filename="switwi_interrupt_diff_annotated.txt"
>
> [file:/home/kettenis/detached/switwi_interrupt_diff_annotated_0001.txt
> ]
> ------=_NextPart_000_009A_01D38578.770321A0
> Content-Type: text/plain; charset=us-ascii
> X-Former-Content-Type: text/plain;
> name="switwi_interrupt_diff.txt"
> Content-Transfer-Encoding: 7bit
> X-Former-Content-Transfer-Encoding: quoted-printable
> X-Former-Content-Disposition: attachment;
> filename="switwi_interrupt_diff.txt"
>
> [file:/home/kettenis/detached/switwi_interrupt_diff_0001.txt]
> ------=_NextPart_000_009A_01D38578.770321A0--
>
>


diff.txt (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: sxitwi on orangepi one (H3) re-visited

s_graf
In reply to this post by s_graf
Please disregard this update.  There is a typo and I could not have tested
it, probably did not notice that the build failed.  I will try again on ver
1.7.

-----Original Message-----
From: Stephen Graf [mailto:[hidden email]]
Sent: Friday, January 5, 2018 9:35 PM
To: 'Mark Kettenis' <[hidden email]>
Cc: [hidden email]; [hidden email]
Subject: RE: sxitwi on orangepi one (H3) re-visited

Thank you for the work you are doing on this driver.

Attached is a diff to ver 1.6 that implements the bus reset on error.  I
tested this on my orange pi one.

-----Original Message-----
From: [hidden email] [mailto:[hidden email]] On Behalf Of Mark
Kettenis
Sent: Friday, January 5, 2018 5:23 AM
To: [hidden email]
Cc: [hidden email]; [hidden email]
Subject: Re: sxitwi on orangepi one (H3) re-visited

> From: "Stephen Graf" <[hidden email]>
> Date: Thu, 4 Jan 2018 16:23:56 -0800
>
> I tested your simplification changes with success.
>
> Then I removed the wait code in send stop, retested and again had
success.

> You should be able to convince yourself that it is not necessary to
> wait for the stop code to complete, since there is no status code nor
> interrupt for stop complete. Also from the Allwinner docs, it is
> possible to assert the stop and start bits at the same time, again
> implying that the software does not have to wait for the stop.  The
> i2c protocol supports multiple masters and so a potential master must
> monitor the bus and wait for a bus idle condition (which a stop
> generates), before attempting a start.  The Allwinner hardware does
> the idle bus check and wait in the start bit processing. Since the
> only thing that is possible after a stop is a start, there seems to be
> no reason for the software to wait for the stop to complete and that
> is why there is no hardware to check for a stop to complete.

I re-tested the A20 yesterday and removing the polling loop after sending a
STOP works just fine there as well.  So I adjusted my diff an committed it.
That should allow us to focus on the other issues.
We really try to seperate out issues and address them with individual
patches.

> Regarding the bus speed, I would like to recommend that bus speed
> information be put in the dtb.  It could be as simple as specifying
> the M and N values.  The driver could generate defaults if there is
> nothing specified in the dtb.  I do not think there should be a lot of
> code in the driver to try to calculate a speed from the master clock.
> The dtb for the orange pi one comes with the i2c busses disabled and I
> have to edit it anyway to enable them and add the i2c devices.

The i2c bindings allow you to add a "clock-frequency" property to the i2c
controller node to specify the desired bus clock frequency.  See
Documentation/devicetree/bindings/i2c/{i2c.txt|i2c-mv64xxx.txt} in the Linux
source tree for details.  If the property is absent the standard speed of
100000 Hz is assumed.  While none of the Allwinner device trees seem to
include this property, it is easy to look for it anyway.

> I then went on and added/modified the driver for interrupts and
> successfully tested.  I tested with two bme280 sensors.  The setup
> code on the bme280 driver is done at boot (attach) time and has to use
> polling, while the cyclic ongoing read of the sensors is done with
interrupts.

>
> Console boot extract:
> com0: console
> sxitwi0 at simplebus0
> iic0 at sxitwi0
> bme0 at iic0 addr 0x77: BME280 60
> sxitwi1 at simplebus0
> iic1 at sxitwi1
> bme1 at iic1 addr 0x76: BME280 60
> ampintc0 at simplebus0 nirq 160, ncpu 4
> sxirtc0 at simplebus0
>
> Sensor readout:
> openbsdop1$ sysctl hw.sensors
> hw.sensors.bme0.temp0=21.89 degC
> hw.sensors.bme0.humidity0=35.23%
> hw.sensors.bme0.pressure0=100.79 Pa
> hw.sensors.bme1.temp0=20.83 degC
> hw.sensors.bme1.humidity0=40.87%
> hw.sensors.bme1.pressure0=100.83 Pa
> openbsdop1$
>
> Attached are files for a diff from ver 1.5, the same diff with notes
> and the entire driver code as tested.  Would you please review the
> code and test if possible on any devices you have.  If you have
> questions/suggestions please reply.

I think I know how to re-implement the interrupt support properly.
Currently testing this approach.  I'll also handle the bus clock speed issue
as I know how to do that properly now.  But if you could at some point send
me a new diff for adding the reset functionality, that would be great.
Please use diff -up to generate your diffs and avoid making arbitrary
whitespace changes.  We use tabs instead of spaces when possible and not
following our standards results in noisy diffs that are harder to review.

Cheers,

Mark

> -----Original Message-----
> From: [hidden email] [mailto:[hidden email]] On Behalf
> Of Mark Kettenis
> Sent: Wednesday, January 3, 2018 1:03 PM
> To: [hidden email]
> Cc: [hidden email]; [hidden email]
> Subject: Re: sxitwi on orangepi one (H3) re-visited
>
> > From: "Stephen Graf" <[hidden email]>
> > Date: Tue, 2 Jan 2018 11:16:18 -0800
> >
> > I did some further testing with interrupts and set the BME280 device
> > to use interrupts on setup.  This occurs in the attach phase on
> > system boot.  It did not work even after I added an early start to
> > the ampintc
> driver:
> >
> > sxipio0 at simplebus0: 94 pins
> > ampintc0 at simplebus0 nirq 160, ncpu 4
> > sxipio1 at simplebus0: 12 pins
> >
> > I suspect that interrupts are not set up on system boot so I went
> > back to the BME280 code that does device setup non-interrupt and
> > cyclic read with interrupt.
>
> Yes.  On OpenBSD, interrupts remain disabled until late in the
> autoconf procedure.  Up until that point, the global variable "cold"
> remains set to 1.  This is why many drivers check this variable and
> poll for command completion as long as cold is non-zero.
>
> > I am not happy to rip out the interrupt code that I have tested and
> > works on the H3 device.  I looked at the V40 docs and they seem to
> > be identical to the H3 for the TWI.  If the code does not meet
> > openBSD standards I am quite willing to work to get it right.  The
> > code I provided also works in non-interrupt mode.
>
> Yes the hardware blocks seem to be identical.
>
> > Could we work together on the interrupt code and get the whole
> > driver working properly?  It should not be a major effort.
>
> Certainly.  I'm just proposing to remove the bogus code such that we
> can see things a bit more clearly.
> >
> > As I mentioned above there is an issue trying to use interrupts on
> > system boot.
> >
> > -----Original Message-----
> > From: [hidden email] [mailto:[hidden email]] On Behalf
> > Of Stephen Graf
> > Sent: Sunday, December 31, 2017 11:57 AM
> > To: 'Mark Kettenis' <[hidden email]>
> > Cc: [hidden email]; [hidden email]
> > Subject: Re: sxitwi on orangepi one (H3) re-visited
> >
> > Thank you for looking at my code. I will try your code on my orange
> > pi one in the near future.
> >
> > Regarding the wait for bus free a on stop condition: After a stop,
> > the only thing that can be done is a start.  When the start bit is
> > set the controller "will transmit a START condition on the bus when
> > the bus is free".  So the wait is done by the hardware in the start
> > routine.  It is necessary for the start hardware to wait for a free
> > bus as it could be a different independent master waiting to grab
> > the bus. Have you tested without any stop delay?  It works, as it
should, on my system.

>
> I did test this on the A20 system, and it failed there.  But my
> testing may have been flawed.  I'll re-test.
>
> > Regarding the bus reset:  When (not if!) an error occurs, it is
> > important to reset to a known default state.  This incudes not only
> > the hardware but also flags such as the started flag.  In my
> > testing, I had an issue with the power lead to the sensor on my
> > breadboard being intermittent.  Without the reset the sensor would
> > often permanently quit on an error.  With the reset it would work
> > again on the
> next read.
>
> Right.  What state did you end up in when this happens?
>
> > My programming experience dates back to writing assembler code on
> > mini computers to control monstrous paper making machines. Things
> > would often go wrong and sending an error message to a remote
> > console was never an acceptable solution.
> >
> > Regarding the bus clock speed:  Ideally this should be as parameter
> > set outside of the driver on system startup.  Different applications
> > require different bus speeds and one bus should be able to run at a
> > different speed than another. It should not be necessary to
> > recompile the kernel to change the bus speed.
> >
> > Does the existing driver work with interrupts on any of your hardware?
>
> Yes.  It works on the A20, but not on the V40.
>
> > -----Original Message-----
> > From: Mark Kettenis [mailto:[hidden email]]
> > Sent: Sunday, December 31, 2017 4:06 AM
> > To: [hidden email]
> > Cc: [hidden email]; [hidden email]; [hidden email]
> > Subject: Re: sxitwi on orangepi one (H3) re-visited
> >
> > > From: "Stephen Graf" <[hidden email]>
> > > 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;
> >  }
> >
> >
> >
> >
>
>
> ------=_NextPart_000_009A_01D38578.770321A0
> Content-Type: text/plain; charset=us-ascii
> X-Former-Content-Type: application/octet-stream;
> name="sxitwi.c"
> Content-Transfer-Encoding: 7bit
> X-Former-Content-Transfer-Encoding: quoted-printable
> X-Former-Content-Disposition: attachment;
> filename="sxitwi.c"
>
> [file:/home/kettenis/detached/sxitwi_0002.c]
> ------=_NextPart_000_009A_01D38578.770321A0
> Content-Type: text/plain; charset=us-ascii
> X-Former-Content-Type: text/plain;
> name="switwi_interrupt_diff_annotated.txt"
> Content-Transfer-Encoding: 7bit
> X-Former-Content-Transfer-Encoding: quoted-printable
> X-Former-Content-Disposition: attachment;
> filename="switwi_interrupt_diff_annotated.txt"
>
> [file:/home/kettenis/detached/switwi_interrupt_diff_annotated_0001.txt
> ]
> ------=_NextPart_000_009A_01D38578.770321A0
> Content-Type: text/plain; charset=us-ascii
> X-Former-Content-Type: text/plain;
> name="switwi_interrupt_diff.txt"
> Content-Transfer-Encoding: 7bit
> X-Former-Content-Transfer-Encoding: quoted-printable
> X-Former-Content-Disposition: attachment;
> filename="switwi_interrupt_diff.txt"
>
> [file:/home/kettenis/detached/switwi_interrupt_diff_0001.txt]
> ------=_NextPart_000_009A_01D38578.770321A0--
>
>