drivers: i2c_dw: add support for agilex platform and reset #60427

samitsingh7 · 2023-07-17T07:12:16Z

Updated driver to reset the i2c node if its corresponding dtsi instance has a resets property.
Handling invalid 0 length i2c transfer and logging error if i2c transfer is initiated with length 0.
Modified i2c shell to support i2c scan command only if I2C_ZERO_LEN_XFER_SUPPORTED Kconfig flag is enabled.
Updated i2c shell for checking i2c-speed parameter in i2c speed shell command.

github-actions

Hello @samitsingh7, and thank you very much for your first pull request to the Zephyr project!

A project maintainer just triggered our CI pipeline to run it against your PR and ensure it's compliant and doesn't cause any issues. You might want to take this opportunity to review the project's Contributor Expectations and make any updates to your pull request if necessary. 😊

teburd · 2023-07-17T15:56:45Z

drivers/i2c/i2c_shell.c

This seems like a strange ifdef being added here, can you explain?

i2c scan shell command initiates i2c transfer without any data which is not a standard in i2c protocol but SMbus supports such transfers.
Also, the current i2c_dw driver is not handling 0 length i2c transfer. Thus, issuing this shell command could hang the system(which is happening in case of i2c_dw driver).

So by introducing the kconfig macro: CONFIG_I2C_SMBUS_SUPPORTED,
By default, this macro is enabled and the i2c scan shell command can be issued but if we disable this macro explicitly then the i2c scan shell command will not be available and thus preventing the program from getting hung if such i2c transfers without data are not handled in the driver.

So if I'm understanding this correctly, the issue is on this device the zero sized transfer used by i2c scan here https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/i2c/i2c_shell.c#L79 is unsupported by this particular hardware or driver.

There's nothing in the i2c specification regarding sending an address only being invalid that I can find. Do you have a source for this being invalid?

The Kconfig here makes sense in terms of this particular hardware and driver combo possibly not supporting i2c scan. But to have a Kconfig like this is a bit strange to me. Instead the driver should return an error code on such transfers and this Kconfig is no longer needed I'd think.

If it's something the driver can possibly do on some instances of the designware controller then it should be a Kconfig along the lines of I2C_DW_HAS_ZERO_LEN_XFER or so and worked with in that way.

There's nothing in the i2c specification regarding sending an address only being invalid that I can find. Do you have a source for this being invalid?

I agree that sending an address only in i2c transaction (Quick Command) is valid.

As per the comment of the i2c-scan function:-

WARNING: As there is no standard I2C detection command, this code uses arbitrary SMBus commands (namely SMBus quick write and SMBus receive byte) to probe for devices. This operation can confuse your I2C bus, cause data loss, and is known to corrupt the Atmel AT24RF08 EEPROM found on many IBM Thinkpad laptops. https://manpages.debian.org/buster/i2c-tools/i2cdetect.8.en.html

We are issuing SMbus Quick command for i2c-scan functionality. Also, it is well documented that this operation could confuse i2c bus and cause data loss. or corrupt the target device. The same is documented here: https://manpages.debian.org/buster/i2c-tools/i2cdetect.8.en.html#WARNING.

Another such combination where this was not working: #54694.

Also, few of the I2C IPs support Quick Commands or 0 length i2c command based on the hardware configuration.
Thus, making use of this Kconfig macro we can avoid such conditions from various i2c controller and target device combination on various platform by disabling i2c-scan functionality for such applications where 0 length command is not supported.

The Kconfig here makes sense in terms of this particular hardware and driver combo possibly not supporting i2c scan.

Exactly as mentioned above and observed from various OS and hardware combination that i2c scan command could cause an issue. Thus introducing Kconfig macro which could help in enabling and disabling for the combination user have.

Instead the driver should return an error code on such transfers and this Kconfig is no longer needed I'd think.

Totally agreed and doing the same here, we are doing 0 length check and returning an error code - https://github.com/samitsingh7/zephyr_main/blob/i2c_dw_driver/drivers/i2c/i2c_dw.c#L478
The introduction of this Kconfig is rather for facilitating user to disable the i2c scan functionality from shell and safeguard from vulnerabilities from data corruption or i2c bus hang.

There could be different platforms where the i2c controller is not configured to support i2c quick commands.
Then all such platforms can make use of this macro to enable or disable this shell command.
We are still open to suggestions on alternative names for this macro: I2C_ZERO_LEN_XFER_SUPPORTED or I2C_QUICK_CMD_EN ?

Thanks for reviewing the changes!

Lets rename it I2C_ZERO_LEN_XFER_SUPPORTED as I think thats closer to the feature

Requested changes done

teburd · 2023-07-17T15:57:16Z

drivers/i2c/i2c_shell.c

This doesn't make much sense to me, why is this needed

i2c shell command: i2c speed <device> <input>
will result in setting up the same speed either if we pass input as (1 or 9), (2 or 10), (3 or 11)..
Because in i2c speed shell function, I2C_SPEED_SET(speed) macro masks the speed parameter with 0x7.

Thus, introduced the check for the input parameter as passing the value more than 0x7 is not possible.

teburd · 2023-07-17T15:57:39Z

drivers/i2c/i2c_shell.c

This doesn't make much sense, this is i2c not smbus (which has a separate API)

The name of the macro: CONFIG_I2C_SMBUS_SUPPORTED, is chosen like this because i2c transfer without any data is supported by SMbus.

By default, this macro is enabled and the i2c scan shell command will be available for an execution but if we disable this macro explicitly then the i2c scan shell command will not be available and thus preventing the program from getting hung if i2c transfers without data are not handled in the driver (ex: i2c_dw driver).

drivers/i2c/i2c_dw.c

teburd · 2023-08-01T13:25:27Z

Renaming of the Kconfig option is a +1 from me

github-actions · 2023-10-01T00:29:50Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

samitsingh7 · 2023-10-01T19:24:27Z

I would like to have the stale label removed

samitsingh7 · 2023-11-06T07:06:16Z

Renaming of the Kconfig option is a +1 from me

Done

github-actions · 2024-01-06T00:30:00Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

malto101 · 2024-11-19T06:56:36Z

drivers/i2c/i2c_dw.c

Changing back to ret will mostly solve the CI problem

henrikbrixandersen · 2024-11-20T11:13:55Z

drivers/i2c/Kconfig

Shouldn't this be disabled by default and selected by drivers supporting zero-length transfers?

Some specific hardware configuration does not support zero-length transfers. In such cases, explicitly disable it. However, the default behavior should remain enabled to avoid impacting other configurations unnecessarily, given the current default is set to 'enabled.'"

henrikbrixandersen · 2024-11-20T11:15:23Z

drivers/i2c/i2c_dw.c

We usually do not indent variable names like this in Zephyr. Please leave this change out of this PR.

henrikbrixandersen · 2024-11-20T11:17:20Z

drivers/i2c/i2c_dw.c

Please leave the empty line before the return statement intact.

henrikbrixandersen · 2024-11-20T11:18:45Z

drivers/i2c/i2c_dw.c

Please avoid mixing reformatting with functional changes.

Agreed, reverted reformatting changes

henrikbrixandersen · 2024-11-20T11:24:32Z

drivers/i2c/i2c_dw.c

This change looks unrelated to the actual, functional change. Please leave this kind of changes out as it makes reading the diff much harder.

Yes, the change is unrelated to functionality.
The variable name 'rc' was not very intuitive, so I updated it to 'ret' for clarity.
In the future, I’ll avoid making such changes.

Please move this rename into a separate commit.

Added device tree for I2C Signed-off-by: samit singh sikarwar <[email protected]>

Handling invalid 0 length i2c transfer and logging error if i2c transfer is initiated with length 0. Modified i2c shell to support i2c scan command only if I2C_ZERO_LEN_XFER_SUPPORTED Kconfig flag is enabled. Updated i2c shell for checking i2c-speed parameter in i2c speed shell command. Signed-off-by: samit singh sikarwar <[email protected]>

samitsingh7 · 2025-01-07T15:02:50Z

Hi @henrikbrixandersen-

Addressed all the comments.

Seeking your review.

samitsingh7 · 2025-01-07T15:03:40Z

Hi @teburd :
Seeking your review, addressed all the comments.

henrikbrixandersen · 2025-01-29T20:37:30Z

Hi @henrikbrixandersen-

Addressed all the comments.

Seeking your review.

No, you did not address all of my comments. You are still adding support for Quick Commands, which is an SMBus feature, to the I2C API, which I commented on earlier, while we have a dedicated SMBus API.

samitsingh7 · 2025-01-29T20:49:40Z

You are still adding support for Quick Commands, which is an SMBus feature, to the I2C API, which I commented on earlier, while we have a dedicated SMBus API.

Hi @henrikbrixandersen -
I understand your concern regarding SMBus Quick Commands, but I am not adding support for SMBus features to the I2C API. The purpose of the I2C_ZERO_LEN_XFER_SUPPORTED macro is solely to provide a way to prevent system hangs in drivers that do not handle zero-length I2C transfers (such as i2c_dw).

The existing i2c scan shell command initiates a transfer with zero-length data, which is not standard in I2C but is supported in SMBus. Since the i2c_dw driver does not handle this case properly, issuing the command can lead to a system hang. The macro allows users to disable the command explicitly, avoiding this issue without modifying I2C behavior.

This change does not introduce SMBus support into the I2C API but rather provides a safeguard against unintended hangs in non-SMBus-capable I2C drivers. Let me know if further clarification is needed.

henrikbrixandersen · 2025-01-31T13:17:57Z

drivers/i2c/i2c_shell.c

 	return MIN(MAX_BYTES_FOR_REGISTER_INDEX, length);
 }

+#if CONFIG_I2C_ZERO_LEN_XFER_SUPPORTED


I still do not understand why the Kconfig is needed. The driver returns an error on unsupported msg lengths. Why isn't this sufficient?

Hi @henrikbrixandersen ,

I have explained the reasoning behind it here: #60427 (comment)

Hi @henrikbrixandersen, Please let me know if anything else is blocking this PR.

soburi · 2025-03-20T23:51:05Z

drivers/i2c/i2c_dw.c

-	ret = i2c_dw_set_master_mode(dev);

-	return ret;
+	return (int)i2c_dw_set_master_mode(dev);


This cast doesn't seem necessary.

static int i2c_dw_set_master_mode(const struct device *dev)

soburi · 2025-03-21T00:01:46Z

drivers/i2c/i2c_dw.c


 #ifdef CONFIG_I2C_TARGET
-static inline uint8_t i2c_dw_read_byte_non_blocking(const struct device *dev);
+static inline int i2c_dw_read_byte_non_blocking(const struct device *dev, uint8_t *data);


Line 461 is still eave as old signature. Please run compilation.

data = i2c_dw_read_byte_non_blocking(port);

soburi · 2025-03-21T03:52:57Z

drivers/i2c/Kconfig

 	  The I2C shell supports scanning, bus recovery, I2C read and write
 	  operations.

+config I2C_ZERO_LEN_XFER_SUPPORTED


If this config use only in i2c_sell.c, the name should be better start with I2C_SHELL_.

I agree with @henrikbrixandersen - this Kconfig doesn't seem necessary if the I2C controllers that cannot support zero length transfers are required to return an error instead.

If you're going to keep the Kconfig, I suggest flipping the logic so that the Kconfig turns off zero length transfers. Something like CONFIG_I2C_DISABLE_ZERO_LENGTH_TRANSFER.

Also, could the zero length check get moved directly into the common i2c_transfer() syscall instead.

github-actions · 2025-05-24T00:37:36Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

github-actions bot reviewed Jul 17, 2023

View reviewed changes

samitsingh7 changed the title ~~I2c dw driver~~ drivers: i2c_dw: add support for agilex platform and reset Jul 17, 2023

samitsingh7 marked this pull request as ready for review July 17, 2023 11:38

samitsingh7 requested review from dcpleung, galak and nashif as code owners July 17, 2023 11:38

zephyrbot added area: Devicetree Binding PR modifies or adds a Device Tree binding area: ARM64 ARM (64-bit) Architecture area: I2C labels Jul 17, 2023

zephyrbot requested review from SgrrZhf, carlocaione, npitre, povergoing and teburd July 17, 2023 11:38

zephyrbot assigned teburd Jul 17, 2023

teburd requested changes Jul 17, 2023

View reviewed changes

samitsingh7 force-pushed the i2c_dw_driver branch from 7089cbc to 057fc65 Compare July 18, 2023 13:09

samitsingh7 requested a review from teburd July 19, 2023 07:12

github-actions bot added the Stale label Oct 1, 2023

teburd removed the Stale label Oct 1, 2023

samitsingh7 force-pushed the i2c_dw_driver branch from 057fc65 to fcb90c9 Compare November 6, 2023 07:02

zephyrbot added the platform: Intel SoC FPGA Agilex Intel Corporation, SoC FPGA Agilex label Nov 6, 2023

samitsingh7 force-pushed the i2c_dw_driver branch from fcb90c9 to 26770a8 Compare November 6, 2023 09:48

teburd previously approved these changes Nov 6, 2023

View reviewed changes

samitsingh7 force-pushed the i2c_dw_driver branch from 26770a8 to 1f407a9 Compare November 6, 2023 16:29

samitsingh7 force-pushed the i2c_dw_driver branch 2 times, most recently from bb63b97 to d5a4a49 Compare November 18, 2024 19:07

malto101 reviewed Nov 19, 2024

View reviewed changes

samitsingh7 dismissed teburd’s stale review via 655df96 November 19, 2024 07:25

samitsingh7 force-pushed the i2c_dw_driver branch from d5a4a49 to 655df96 Compare November 19, 2024 07:25

samitsingh7 requested review from malto101 and teburd November 19, 2024 09:25

teburd previously approved these changes Nov 19, 2024

View reviewed changes

henrikbrixandersen requested changes Nov 20, 2024

View reviewed changes

samitsingh7 added 2 commits January 7, 2025 14:53

dts: arm64: intel: intel_socfpga_agilex: Adding device tree for i2c

80c2d19

Added device tree for I2C Signed-off-by: samit singh sikarwar <[email protected]>

samitsingh7 dismissed teburd’s stale review via b1a2ae7 January 7, 2025 14:56

samitsingh7 force-pushed the i2c_dw_driver branch from 655df96 to b1a2ae7 Compare January 7, 2025 14:56

samitsingh7 requested review from henrikbrixandersen and teburd January 7, 2025 15:00

teburd approved these changes Jan 29, 2025

View reviewed changes

henrikbrixandersen reviewed Jan 31, 2025

View reviewed changes

samitsingh7 requested a review from henrikbrixandersen March 17, 2025 10:31

teburd requested review from keith-zephyr and soburi and removed request for malto101 March 19, 2025 18:58

soburi requested changes Mar 21, 2025

View reviewed changes

soburi reviewed Mar 21, 2025

View reviewed changes

github-actions bot added the Stale label May 24, 2025

github-actions bot closed this Jun 7, 2025

drivers: i2c_dw: add support for agilex platform and reset #60427

drivers: i2c_dw: add support for agilex platform and reset #60427

Uh oh!

Conversation

samitsingh7 commented Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samitsingh7 Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samitsingh7 Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

teburd commented Aug 1, 2023

Uh oh!

github-actions bot commented Oct 1, 2023

Uh oh!

samitsingh7 commented Oct 1, 2023

Uh oh!

samitsingh7 commented Nov 6, 2023

Uh oh!

github-actions bot commented Jan 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samitsingh7 commented Jan 7, 2025

Uh oh!

samitsingh7 commented Jan 7, 2025

Uh oh!

henrikbrixandersen commented Jan 29, 2025

Uh oh!

samitsingh7 commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

samitsingh7 commented Jul 17, 2023 •

edited

Loading

samitsingh7 Jul 19, 2023 •

edited

Loading

samitsingh7 Jul 19, 2023 •

edited

Loading

samitsingh7 commented Jan 29, 2025 •

edited

Loading

samitsingh7 Feb 1, 2025 •

edited

Loading