Skip to content

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

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.

Sign up for GitHub

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
samitsingh7 wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
80c2d19
dts: arm64: intel: intel_socfpga_agilex: Adding device tree for i2c
samitsingh7 Dec 9, 2022
b1a2ae7
drivers: i2c: i2c_dw: adding optional feature support and improvements
samitsingh7 Jul 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions drivers/i2c/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@ config I2C_SHELL
The I2C shell supports scanning, bus recovery, I2C read and write
operations.

config I2C_ZERO_LEN_XFER_SUPPORTED
Copy link
Member

@soburi soburi Mar 21, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@keith-zephyr keith-zephyr Mar 24, 2025

Choose a reason for hiding this comment

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

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.

bool "I2C transfer without data (data length 0)"
default y
Copy link
Member

@henrikbrixandersen henrikbrixandersen Nov 20, 2024

Choose a reason for hiding this comment

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

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

Copy link
Author

@samitsingh7 samitsingh7 Jan 7, 2025

Choose a reason for hiding this comment

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

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.'"

help
Enable this flag to support 0 length I2C transfers(quick commands).
Comment on lines +25 to +29
Copy link
Member

@henrikbrixandersen henrikbrixandersen Jan 12, 2024

Choose a reason for hiding this comment

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

Quick commands are SMBus, not I2C.

Copy link
Author

@samitsingh7 samitsingh7 Feb 7, 2024

Choose a reason for hiding this comment

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

As, few of the I2C IPs support Quick Commands based on the hardware configuration (for various i2c-controller and target device combination on various platforms).

Thus, making use of this Kconfig macro we can enable/disable the support of functionalities or commands which are making use of 0 data-length i2c commands.


config I2C_STATS
bool "I2C device Stats"
depends on STATS
Expand Down
59 changes: 39 additions & 20 deletions drivers/i2c/i2c_dw.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
/* dw_i2c.c - I2C file for Design Ware */
/* i2c_dw.c - I2C file for Design Ware */

/*
* Copyright (c) 2015 Intel Corporation
* Copyright (c) 2022 Andrei-Edward Popa
* Copyright (c) 2015-2023 Intel Corporation
*
* SPDX-License-Identifier: Apache-2.0
*/
Expand Down Expand Up @@ -351,7 +351,7 @@ static inline void i2c_dw_transfer_complete(const struct device *dev)
}

#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);
Copy link
Member

@soburi soburi Mar 21, 2025

Choose a reason for hiding this comment

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

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

				data = i2c_dw_read_byte_non_blocking(port);

static inline void i2c_dw_write_byte_non_blocking(const struct device *dev, uint8_t data);
static void i2c_dw_slave_read_clear_intr_bits(const struct device *dev);
#endif
Expand All @@ -364,8 +364,8 @@ static void i2c_dw_isr(const struct device *port)
int ret = 0;
uint32_t reg_base = get_regs(port);

/* Cache ic_intr_stat for processing, so there is no need to read
* the register multiple times.
/* Cache I2C Interrupt Status Register(ic_intr_stat) for processing,
* so there is no need to read the register multiple times.
*/
intr_stat.raw = read_intr_stat(reg_base);

Expand Down Expand Up @@ -512,7 +512,7 @@ static int i2c_dw_setup(const struct device *dev, uint16_t slave_address)
* Make sure to set both the master_mode and slave_disable_bit
* to both 0 or both 1
*/
LOG_DBG("I2C: host configured as Master Device");
LOG_DBG("I2C: host configured as Controller Device");
ic_con.bits.master_mode = 1U;
ic_con.bits.slave_disable = 1U;
} else {
Expand Down Expand Up @@ -619,7 +619,7 @@ static int i2c_dw_transfer(const struct device *dev, struct i2c_msg *msgs, uint8
{
struct i2c_dw_dev_config *const dw = dev->data;
struct i2c_msg *cur_msg = msgs;
uint8_t msg_left = num_msgs;
uint8_t msg_left;
uint8_t pflags;
int ret;
uint32_t reg_base = get_regs(dev);
Expand All @@ -635,6 +635,21 @@ static int i2c_dw_transfer(const struct device *dev, struct i2c_msg *msgs, uint8
return ret;
}

/*
* I2C transfer with 0 length are invalid but
* SMbus quick commands uses 0 length transfer.
* And in order to support SMbus quick commands, the driver
* has to be enhanced.
*/
Comment on lines +638 to +643
Copy link
Member

@henrikbrixandersen henrikbrixandersen Jan 12, 2024

Choose a reason for hiding this comment

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

This sounds like a feature appropriate for drivers/smbus/, not drivers/i2c/? Same goes for the Kconfig introduced?

Copy link
Author

@samitsingh7 samitsingh7 Feb 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Few of the I2C IPs supports Quick Commands or 0 length i2c commands based on the hardware configuration.
Hence, this comment is added to caution that currently this driver is not supporting any 0 length/quick commands.

for (msg_left = 0; msg_left < num_msgs; msg_left++) {
if (msgs[msg_left].len == 0) {
LOG_ERR("Invalid transfer length 0 for msgs[%d]", msg_left);
return -EINVAL;
}
}

msg_left = num_msgs;

/* First step, check if there is current activity */
if (test_bit_status_activity(reg_base) || (dw->state & I2C_DW_BUSY)) {
ret = -EBUSY;
Expand Down Expand Up @@ -741,14 +756,13 @@ static int i2c_dw_runtime_configure(const struct device *dev, uint32_t config)
struct i2c_dw_dev_config *const dw = dev->data;
const struct i2c_dw_rom_config *const rom = dev->config;
uint32_t value = 0U;
uint32_t rc = 0U;
uint32_t ret = 0U;
uint32_t reg_base = get_regs(dev);

dw->app_config = config;

/* Make sure we have a supported speed for the DesignWare model */
/* and have setup the clock frequency and speed mode */
switch (I2C_SPEED_GET(dw->app_config)) {
switch (I2C_SPEED_GET(config)) {
case I2C_SPEED_STANDARD:
/* Following the directions on DW spec page 59, IC_SS_SCL_LCNT
* must have register values larger than IC_FS_SPKLEN + 7
Expand Down Expand Up @@ -832,14 +846,17 @@ static int i2c_dw_runtime_configure(const struct device *dev, uint32_t config)

dw->hcnt = value;
} else {
rc = -EINVAL;
ret = -EINVAL;
Comment on lines -835 to +849
Copy link
Member

@henrikbrixandersen henrikbrixandersen Nov 20, 2024

Choose a reason for hiding this comment

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

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

Copy link
Author

@samitsingh7 samitsingh7 Jan 7, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@keith-zephyr keith-zephyr Mar 24, 2025

Choose a reason for hiding this comment

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

Please move this rename into a separate commit.

}
break;
default:
/* TODO change */
rc = -EINVAL;
ret = -EINVAL;
}

if (ret == 0) {
dw->app_config = config;
}
/*
* Clear any interrupts currently waiting in the controller
*/
Expand All @@ -852,26 +869,30 @@ static int i2c_dw_runtime_configure(const struct device *dev, uint32_t config)
*/
dw->app_config |= I2C_MODE_CONTROLLER;

return rc;
return ret;
}

#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)
{
uint32_t reg_base = get_regs(dev);

if (!test_bit_status_rfne(reg_base)) { /* Rx FIFO must not be empty */
LOG_ERR("Rx FIFO is empty");
return -EIO;
}

return (uint8_t)read_cmd_data(reg_base);
*data = (uint8_t)read_cmd_data(reg_base);

return 0;
}

static inline void i2c_dw_write_byte_non_blocking(const struct device *dev, uint8_t data)
{
uint32_t reg_base = get_regs(dev);

if (!test_bit_status_tfnt(reg_base)) { /* Tx FIFO must not be full */
LOG_ERR("Tx FIFO is full");
return;
}

Expand Down Expand Up @@ -925,7 +946,7 @@ static int i2c_dw_set_slave_mode(const struct device *dev, uint8_t addr)
write_tx_tl(0, reg_base);
write_rx_tl(0, reg_base);

LOG_DBG("I2C: Host registered as Slave Device");
LOG_DBG("I2C: Host registered as Target Device");

return 0;
}
Expand All @@ -947,13 +968,11 @@ static int i2c_dw_slave_register(const struct device *dev, struct i2c_target_con

static int i2c_dw_slave_unregister(const struct device *dev, struct i2c_target_config *cfg)
{
struct i2c_dw_dev_config *const dw = dev->data;
int ret;
struct i2c_dw_dev_config * const dw = dev->data;

dw->state = I2C_DW_STATE_READY;
ret = i2c_dw_set_master_mode(dev);

return ret;
return (int)i2c_dw_set_master_mode(dev);
Copy link
Member

@soburi soburi Mar 20, 2025

Choose a reason for hiding this comment

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

This cast doesn't seem necessary.

static int i2c_dw_set_master_mode(const struct device *dev)

}

static void i2c_dw_slave_read_clear_intr_bits(const struct device *dev)
Expand Down
4 changes: 2 additions & 2 deletions drivers/i2c/i2c_dw.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/* dw_i2c.h - header for Design Ware I2C operations */
/* i2c_dw.h - header for Design Ware I2C operations */

/*
* Copyright (c) 2015 Intel Corporation
* Copyright (c) 2015-2023 Intel Corporation
*
* SPDX-License-Identifier: Apache-2.0
*/
Expand Down
10 changes: 10 additions & 0 deletions drivers/i2c/i2c_shell.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ static int get_bytes_count_for_hex(char *arg)
return MIN(MAX_BYTES_FOR_REGISTER_INDEX, length);
}

#if CONFIG_I2C_ZERO_LEN_XFER_SUPPORTED
Copy link
Member

@henrikbrixandersen henrikbrixandersen Jan 31, 2025

Choose a reason for hiding this comment

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

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

Copy link
Author

@samitsingh7 samitsingh7 Feb 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @henrikbrixandersen ,

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

Copy link
Author

@samitsingh7 samitsingh7 Mar 17, 2025

Choose a reason for hiding this comment

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

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

/*
* This sends I2C messages without any data (i.e. stop condition after
* sending just the address). If there is an ACK for the address, it
Expand Down Expand Up @@ -94,6 +95,7 @@ static int cmd_i2c_scan(const struct shell *shell_ctx,

return 0;
}
#endif

/* i2c recover <device> */
static int cmd_i2c_recover(const struct shell *shell_ctx,
Expand Down Expand Up @@ -318,6 +320,12 @@ static int cmd_i2c_speed(const struct shell *shell_ctx, size_t argc, char **argv
}

speed = strtol(argv[ARGV_DEV + 1], NULL, 10);

if (speed > I2C_SPEED_DT) {
Copy link
Contributor

@teburd teburd Jul 17, 2023

Choose a reason for hiding this comment

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

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

Copy link
Author

@samitsingh7 samitsingh7 Jul 18, 2023

Choose a reason for hiding this comment

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

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.

shell_error(shell_ctx, "Invalid speed parameter");
return -EINVAL;
}

ret = i2c_get_config(dev, &dev_config);
if (ret == 0) {
dev_config &= ~I2C_SPEED_MASK;
Expand Down Expand Up @@ -349,10 +357,12 @@ static void device_name_get(size_t idx, struct shell_static_entry *entry)
SHELL_DYNAMIC_CMD_CREATE(dsub_device_name, device_name_get);

SHELL_STATIC_SUBCMD_SET_CREATE(sub_i2c_cmds,
#if CONFIG_I2C_ZERO_LEN_XFER_SUPPORTED
SHELL_CMD_ARG(scan, &dsub_device_name,
"Scan I2C devices\n"
"Usage: scan <device>",
cmd_i2c_scan, 2, 0),
#endif
SHELL_CMD_ARG(recover, &dsub_device_name,
"Recover I2C bus\n"
"Usage: recover <device>",
Expand Down
61 changes: 61 additions & 0 deletions dts/arm64/intel/intel_socfpga_agilex.dtsi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <zephyr/dt-bindings/interrupt-controller/arm-gic.h>
#include <zephyr/dt-bindings/clock/intel_socfpga_clock.h>
#include <mem.h>
#include <zephyr/dt-bindings/i2c/i2c.h>

/ {
cpus {
Expand Down Expand Up @@ -188,4 +189,64 @@
clocks = <&clock INTEL_SOCFPGA_CLOCK_WDT>;
status = "disabled";
};

i2c0: i2c@ffc02800 {
compatible = "snps,designware-i2c";
reg = <0xffc02800 0x100>;
interrupt-parent = <&gic>;
interrupts = <GIC_SPI 103 IRQ_TYPE_LEVEL
IRQ_DEFAULT_PRIORITY>;
clock-frequency = <I2C_BITRATE_STANDARD>;
#address-cells = <1>;
#size-cells = <0>;
status = "disabled";
};

i2c1: i2c@ffC02900 {
compatible = "snps,designware-i2c";
reg = <0xffC02900 0x100>;
interrupt-parent = <&gic>;
interrupts = <GIC_SPI 104 IRQ_TYPE_LEVEL
IRQ_DEFAULT_PRIORITY>;
clock-frequency = <I2C_BITRATE_STANDARD>;
#address-cells = <1>;
#size-cells = <0>;
status = "disabled";
};

i2c2: i2c@ffC02A00 {
compatible = "snps,designware-i2c";
reg = <0xffC02A00 0x100>;
interrupt-parent = <&gic>;
interrupts = <GIC_SPI 105 IRQ_TYPE_LEVEL
IRQ_DEFAULT_PRIORITY>;
clock-frequency = <I2C_BITRATE_STANDARD>;
#address-cells = <1>;
#size-cells = <0>;
status = "disabled";
};

i2c3: i2c@ffC02B00 {
compatible = "snps,designware-i2c";
reg = <0xffC02B00 0x100>;
interrupt-parent = <&gic>;
interrupts = <GIC_SPI 106 IRQ_TYPE_LEVEL
IRQ_DEFAULT_PRIORITY>;
clock-frequency = <I2C_BITRATE_STANDARD>;
#address-cells = <1>;
#size-cells = <0>;
status = "disabled";
};

i2c4: i2c@ffC02C00 {
compatible = "snps,designware-i2c";
reg = <0xffC02C00 0x100>;
interrupt-parent = <&gic>;
interrupts = <GIC_SPI 107 IRQ_TYPE_LEVEL
IRQ_DEFAULT_PRIORITY>;
clock-frequency = <I2C_BITRATE_STANDARD>;
#address-cells = <1>;
#size-cells = <0>;
status = "disabled";
};
};
2 changes: 1 addition & 1 deletion dts/bindings/i2c/snps,designware-i2c.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ description: Synopsys DesignWare I2C node

compatible: "snps,designware-i2c"

include: [i2c-controller.yaml, pinctrl-device.yaml, pcie-device.yaml]
include: [i2c-controller.yaml, reset-device.yaml, pinctrl-device.yaml, pcie-device.yaml]

properties:
interrupts:
Expand Down
Loading