Skip to content
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

drivers: i2c: Introduce I2C timeout for SAM0 #79311

Merged

Conversation

kowalewskijan
Copy link
Contributor

Adds configurable timeout for I2C transactions and provides bus recovery functionality.

Tested on SAME53J20A

Copy link
Member

@nandojve nandojve left a comment

Choose a reason for hiding this comment

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

Hi @kowalewskijan ,

Interesting, a few points:

1- Could you explain on the commit messages what you are solving and why? It will be nice that you add information about a few use cases too. In this case, are you trying to solve some of the problems related on 36.6.2.4.2 Transmitting Address Packets?

2- I can imagine that it is not possible to have a HW test to avoid regressions for this, right?

3- Zephyr is moving to have all dependencies defined by each driver. This means that you need to add GPIO and PINCTRL dependencies at Kconfig.

drivers/i2c/i2c_sam0.c Outdated Show resolved Hide resolved
drivers/i2c/i2c_sam0.c Outdated Show resolved Hide resolved
@kowalewskijan
Copy link
Contributor Author

@nandojve, what I mainly want to add is the timeout for I2C in sam0 driver. Right now, once the I2C is blocked due to malfunctioning I2C slave device or any other hardware issue that may cause the I2C to be not functional, the driver will hang the whole system during the device init phase. I believe introducing timeouts in such case will greatly improve usability for these SoCs. Regarding recovering the I2C it's just a typical way to recover it, but thinking about it, maybe it should handle the I2C errors more holistic based on STATUS register. If you want, we can split this into 1st having the timeout which is definitely beneficial for overall stability of the system for these SoCs and later add recovery features based on STATUS register.

@nandojve
Copy link
Member

nandojve commented Nov 2, 2024

Hi @kowalewskijan ,

@nandojve, what I mainly want to add is the timeout for I2C in sam0 driver. Right now, once the I2C is blocked due to malfunctioning I2C slave device or any other hardware issue that may cause the I2C to be not functional, the driver will hang the whole system during the device init phase. I believe introducing timeouts in such case will greatly improve usability for these SoCs. Regarding recovering the I2C it's just a typical way to recover it, but thinking about it, maybe it should handle the I2C errors more holistic based on STATUS register. If you want, we can split this into 1st having the timeout which is definitely beneficial for overall stability of the system for these SoCs and later add recovery features based on STATUS register.

I like the idea about split into 2 parts. I'm fine about timeouts unless @teburd have a different opinion. What I really appreciate is to receive a solution that uses the STATUS registers to perform the bus recover.

@teburd
Copy link
Collaborator

teburd commented Nov 2, 2024

Hi @kowalewskijan ,

@nandojve, what I mainly want to add is the timeout for I2C in sam0 driver. Right now, once the I2C is blocked due to malfunctioning I2C slave device or any other hardware issue that may cause the I2C to be not functional, the driver will hang the whole system during the device init phase. I believe introducing timeouts in such case will greatly improve usability for these SoCs. Regarding recovering the I2C it's just a typical way to recover it, but thinking about it, maybe it should handle the I2C errors more holistic based on STATUS register. If you want, we can split this into 1st having the timeout which is definitely beneficial for overall stability of the system for these SoCs and later add recovery features based on STATUS register.

I like the idea about split into 2 parts. I'm fine about timeouts unless @teburd have a different opinion. What I really appreciate is to receive a solution that uses the STATUS registers to perform the bus recover.

I don't have a strong opinion on this myself. I trust your judgment here @nandojve

Adds configurable timeout for I2C transactions for SAM0 SoCs.

Signed-off-by: Jan Kowalewski <[email protected]>
@kowalewskijan kowalewskijan changed the title drivers: i2c: Introduce I2C timeout and bus recovery for SAM0 drivers: i2c: Introduce I2C timeout for SAM0 Nov 5, 2024
@kowalewskijan
Copy link
Contributor Author

@nandojve - I extracted only I2C timeouts for this PR

@nashif nashif merged commit 4288962 into zephyrproject-rtos:main Nov 16, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: I2C platform: Microchip SAM Microchip SAM Platform (formerly Atmel SAM)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants