Skip to content

drivers: i2c_sam0: Optimize and add i2c slave #24050

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

sslupsky
Copy link
Contributor

@sslupsky sslupsky commented Apr 2, 2020

Optimizations:
When using system power management, the bus transactions
can be interrupted by low power modes which causes excessive
power consumption and can cause the transactions to terminate
before completed. Further, other interrupts can cause bus transaction
anomalies.

When using the RESTART flag, the MB and SB bits must be cleared.

This commit prevents interruption of the bus during low power modes and
correctly clears the MB or SB flags when a bus transaction is restarted.
Also fixes an initialization problem with the INACTOUT timer and ensures
the LOWTOUT is properly configured when using i2c_configure().

Add i2c slave support:
The existing sam0 i2c driver does not support slave mode.

This commit enables slave support for sam0 devices.

The Slave support is based on Linux I2C Slave support by Wolfram Sang
and documented at
https://www.kernel.org/doc/Documentation/i2c/slave-interface

The code for this commit is largely based on code taken from PR #5224
and the stm mcu implementation. This was modified to work with the
sam0 SERCOM perhiperal and integrated into the sam0 driver.

Fix whitespace and comments

This PR replaces #24047

Fixes #21711
Fixes #21549
Fixes #21233
Fixes #21232
Fixes #21114
Fixes #21092

@sslupsky
Copy link
Contributor Author

sslupsky commented Apr 15, 2020

There is an issue when two transactions run back to back. I added an idle check loop and timeout to address this.

@pabigot
Copy link
Collaborator

pabigot commented May 11, 2020

@sslupsky Could you revisit this PR, please?

First the single commit has far too much stuff in it. It should be split out into separate commits that fix each isolated problem separately. (It looks like it was that way originally, but then got squashed; I believe that was a misunderstanding of expectations that's been corrected.)

We're particularly interested in #21232 which is medium priority so affects the release. Ideally a fix for that would be pulled out to a separate PR that we can merge: this one has too much other stuff in it.

I don't see how this PR fixes #21114; if it doesn't please remove it from the list of fixed issues. (I edited the description so that all of them, not just the first on the line, were recognized by github as related.)

@sslupsky
Copy link
Contributor Author

sslupsky commented May 12, 2020

@pabigot ok, i can split this up. It will likely take a while to split them apart though.

Regarding #21114, i believe the erratic timeouts were because the i2c driver was doing some very weird things. Since fixing this driver and the rtc, I no longer see the erratic timeouts.

@carlescufi
Copy link
Member

@sslupsky I would be most grateful if you could split the fix to #21232 in a separate PR as soon as possible. Thanks!

@carlescufi carlescufi requested review from mnkp and stephanosio May 26, 2020 14:55
@stephanosio stephanosio requested a review from nandojve May 26, 2020 15:03
@nandojve
Copy link
Member

@sslupsky could you rebase.

@github-actions github-actions bot added the has-conflicts Issue/PR has conflicts with another issue/PR label Jun 29, 2020
@sslupsky
Copy link
Contributor Author

@nandojve Against what? master? I haven't updated my local to the most recent master since release of 2.3 yet.

@nandojve
Copy link
Member

@nandojve Against what? master? I haven't updated my local to the most recent master since release of 2.3 yet.

Well, there is a file conflict, at least for me shows below:
I hope you can update soon.
image

@sslupsky
Copy link
Contributor Author

Ok, I will have a look. I am filtering out the commit for the i2c slave changes and will push the updated PR shortly.

@sslupsky sslupsky force-pushed the sam0-i2c-optimizations branch from 3345b25 to ed1e180 Compare June 30, 2020 17:20
@sslupsky
Copy link
Contributor Author

@nandojve @pabigot I pulled the i2c slave changes into a separate commit.

@github-actions github-actions bot removed the has-conflicts Issue/PR has conflicts with another issue/PR label Jun 30, 2020
@sslupsky
Copy link
Contributor Author

I added a commit for some changes made since the original PR was submitted.

@github-actions github-actions bot added has-conflicts Issue/PR has conflicts with another issue/PR and removed has-conflicts Issue/PR has conflicts with another issue/PR labels Jun 30, 2020
@github-actions github-actions bot added has-conflicts Issue/PR has conflicts with another issue/PR and removed has-conflicts Issue/PR has conflicts with another issue/PR labels Jul 2, 2020
@sslupsky sslupsky force-pushed the sam0-i2c-optimizations branch from 293862d to 672fd65 Compare July 10, 2020 15:48
@sslupsky
Copy link
Contributor Author

hmm, I forced pushed the changes but it seems to have only updated the commit dealing with the i2c_slave. Can someone please advise how to push the changes to the earlier commit (I squashed those changes).

sslupsky added 2 commits July 10, 2020 10:32
Optimizations:
When using system power management, the bus transactions
can be interrupted by low power modes which causes excessive
power consumption and can cause the transactions to terminate
before completed. Further, other interrupts can cause bus transaction
anomalies.

When using the RESTART flag, the MB and SB bits must be cleared.

This commit prevents interruption of the bus during low power modes and
correctly clears the MB or SB flags when a bus transaction is restarted.
Also fixes an initialization problem with the INACTOUT timer and ensures
the LOWTOUT is properly configured when using i2c_configure().

The GCLK_SERCOM_SLOW clock is used to time the i2c
time-outs and must be configured to use a 32KHz oscillator.
See 28.6.3.1

This commit configures GCLK4 to 32768 Hz

Fixes zephyrproject-rtos#21711, zephyrproject-rtos#21549, zephyrproject-rtos#21233, zephyrproject-rtos#21232, zephyrproject-rtos#21114, zephyrproject-rtos#21092

A problem can arise when back to back transactions occur before
the STOP transaction is complete.

This commit also introduces a busy wait for the STOP to complete
with an timeout in the event there is a bus failure.

Signed-off-by: Steven Slupsky <[email protected]>
The existing sam0 i2c driver does not support slave mode.

This commit enables slave support for sam0 devices.

The Slave support is based on Linux I2C Slave support by Wolfram Sang
and documented at
https://www.kernel.org/doc/Documentation/i2c/slave-interface

The code for this commit is largely based on code taken from PR zephyrproject-rtos#5224
and the stm mcu implementation. This was modified to work with the
sam0 SERCOM perhiperal and integrated into the sam0 driver.

A slave must wait for data ready when there is an address match
to ensure the data is valid.

Disable SCLSM (SCL stetch mode) when operating in slave mode
to ensure that i2c ACK's are handled properly.

Fix k_timeout_t.

Signed-off-by: Steven Slupsky <[email protected]>
@sslupsky sslupsky force-pushed the sam0-i2c-optimizations branch from 672fd65 to 8c5cad9 Compare July 10, 2020 16:35
@sslupsky
Copy link
Contributor Author

Ok. I think I have properly rebased this now and the two commits accurately separate the i2c slave changes.

@dleach02
Copy link
Member

needs a review from the relevant folks

@pabigot pabigot self-requested a review August 14, 2020 23:48
@nandojve
Copy link
Member

Hi @sslupsky, you are doing a great job, thanks!

I would like run this on HW and I have two R21 and one D20 that I can use it. How do you suggests to do it? How do you are testing, loopback? Do you have anything that can share to speed up my tests?

I would like know what app is building this on CI, do you know one? We need make sure code is covered on CI.

Copy link
Collaborator

@pabigot pabigot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR needs to be updated for the change to replace u32_t with uint32_t back in June. A pinned message in the general slack channel has these instructions, which would need to be adapted now.

git checkout ${BR}
git rebase e77985a2f0 # last commit before rename
rm -rf c99
git format-patch -o c99 e77985a2f0..HEAD 
sed -i.bak -r \
  -e 's/\bu(8|16|32|64)_t\b/uint\1_t/g' \
  -e 's/\bs(8|16|32|64)_t\b/int\1_t/g' \
  c99/*
for f in c99/*.patch ; do
  diff -u ${f}.bak ${f}
done
git branch BU/${BR}
git reset --hard a2d4292a96  # the last commit of the rename
git am c99/*.patch

@sslupsky
Copy link
Contributor Author

sslupsky commented Sep 8, 2020

@nandojve I have a couple boards that I have been using for my testing. Mostly, I have tested with a few sensors on my board (temp/humidity, barometer, ambient light, accelerometer). I have another board to board connection where two D21's talk to each other using I2C as well. The sensor application periodically reads the sensors and the application on the other board pulls the sensor readings from the sensor board using the AT24 memory device emulation driver.

I do not have a dedicated test suite so there is no CI set up.

@sslupsky sslupsky closed this Sep 8, 2020
@sslupsky sslupsky deleted the sam0-i2c-optimizations branch September 8, 2020 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: I2C has-conflicts Issue/PR has conflicts with another issue/PR
Projects
None yet
6 participants