* [PATCH 0/2] hw/i2c/dw: Add DesignWare I2C controller emulator @ 2026-01-06 13:12 AlanoSong 2026-01-06 13:12 ` [PATCH 1/2] " AlanoSong 2026-01-06 13:12 ` [PATCH 2/2] hw/arm/virt: Add DesignWare I2C controller AlanoSong 0 siblings, 2 replies; 11+ messages in thread From: AlanoSong @ 2026-01-06 13:12 UTC (permalink / raw) To: qemu-devel, qemu-arm Cc: cminyard, peter.maydell, philmd, ani, pbonzini, shannon.zhaosl Hi all, This series try to add DesignWare I2C controller according to DesignWare I2C databook v2.01a. Cause DesignWare I2C controller is commonly used on arm soc chip, I add it onto arm virt board, and also an at24c eeprom for r/w operation. I also add the I2C controller and at24c eeprom into the acpi table of arm virt board. The code above has been confirmed with i2c-tools under linux v6.18 kernel driver. Thanks for your review! Alano Song. Alano Song (2): hw/i2c/dw: Add DesignWare I2C controller emulator hw/arm/virt: Add DesignWare I2C controller hw/arm/Kconfig | 1 + hw/arm/virt-acpi-build.c | 32 +++ hw/arm/virt.c | 38 ++- hw/i2c/Kconfig | 4 + hw/i2c/dw_i2c.c | 517 +++++++++++++++++++++++++++++++++++++++ hw/i2c/meson.build | 1 + hw/i2c/trace-events | 4 + include/hw/arm/virt.h | 1 + include/hw/i2c/dw_i2c.h | 151 ++++++++++++ 9 files changed, 748 insertions(+), 1 deletion(-) create mode 100644 hw/i2c/dw_i2c.c create mode 100644 include/hw/i2c/dw_i2c.h -- 2.43.0 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] hw/i2c/dw: Add DesignWare I2C controller emulator 2026-01-06 13:12 [PATCH 0/2] hw/i2c/dw: Add DesignWare I2C controller emulator AlanoSong @ 2026-01-06 13:12 ` AlanoSong 2026-01-06 15:52 ` Jonathan Cameron via 2026-01-06 13:12 ` [PATCH 2/2] hw/arm/virt: Add DesignWare I2C controller AlanoSong 1 sibling, 1 reply; 11+ messages in thread From: AlanoSong @ 2026-01-06 13:12 UTC (permalink / raw) To: qemu-devel, qemu-arm Cc: cminyard, peter.maydell, philmd, ani, pbonzini, shannon.zhaosl, Alano Song Add DesignWare I2C controller according to DesignWare I2C databook v2.01a. Confirmed the model with i2c-tools under v6.18 linux driver. The slave mode is not implemented, cause this feature is usually not used. The 10 bit slave address is not implemented, cause this feature is usually not used, and not supported by qemu I2C core bus currently. Signed-off-by: Alano Song <AlanoSong@163.com> --- hw/i2c/Kconfig | 4 + hw/i2c/dw_i2c.c | 517 ++++++++++++++++++++++++++++++++++++++++ hw/i2c/meson.build | 1 + hw/i2c/trace-events | 4 + include/hw/i2c/dw_i2c.h | 151 ++++++++++++ 5 files changed, 677 insertions(+) create mode 100644 hw/i2c/dw_i2c.c create mode 100644 include/hw/i2c/dw_i2c.h diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig index 596a7a3165..6bb20d45de 100644 --- a/hw/i2c/Kconfig +++ b/hw/i2c/Kconfig @@ -30,6 +30,10 @@ config IMX_I2C bool select I2C +config DW_I2C + bool + select I2C + config MPC_I2C bool select I2C diff --git a/hw/i2c/dw_i2c.c b/hw/i2c/dw_i2c.c new file mode 100644 index 0000000000..a5a31ec78c --- /dev/null +++ b/hw/i2c/dw_i2c.c @@ -0,0 +1,517 @@ +/* + * DesignWare I2C Bus Serial Interface Emulation + * + * Copyright (C) 2026 Alano Song <AlanoSong@163.com> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, see <http://www.gnu.org/licenses/>. + * + */ + +#include "qemu/osdep.h" +#include "hw/i2c/dw_i2c.h" +#include "hw/core/irq.h" +#include "hw/i2c/i2c.h" +#include "migration/vmstate.h" +#include "qemu/log.h" +#include "qemu/module.h" +#include "trace.h" + +static const char *dw_i2c_get_regname(uint64_t offset) +{ + switch (offset) { + case DW_IC_CON: return "CON"; + case DW_IC_TAR: return "TAR"; + case DW_IC_SAR: return "SAR"; + case DW_IC_DATA_CMD: return "DATA_CMD"; + case DW_IC_SS_SCL_HCNT: return "SS_SCL_HCNT"; + case DW_IC_SS_SCL_LCNT: return "SS_SCL_LCNT"; + case DW_IC_FS_SCL_HCNT: return "FS_SCL_HCNT"; + case DW_IC_FS_SCL_LCNT: return "FS_SCL_LCNT"; + case DW_IC_INTR_STAT: return "INTR_STAT"; + case DW_IC_INTR_MASK: return "INTR_MASK"; + case DW_IC_RAW_INTR_STAT: return "RAW_INTR_STAT"; + case DW_IC_RX_TL: return "RX_TL"; + case DW_IC_TX_TL: return "TX_TL"; + case DW_IC_CLR_INTR: return "CLR_INTR"; + case DW_IC_CLR_RX_UNDER: return "CLR_RX_UNDER"; + case DW_IC_CLR_RX_OVER: return "CLR_RX_OVER"; + case DW_IC_CLR_TX_OVER: return "CLR_TX_OVER"; + case DW_IC_CLR_RD_REQ: return "CLR_RD_REQ"; + case DW_IC_CLR_TX_ABRT: return "CLR_TX_ABRT"; + case DW_IC_CLR_RX_DONE: return "CLR_RX_DONE"; + case DW_IC_CLR_ACTIVITY: return "CLR_ACTIVITY"; + case DW_IC_CLR_STOP_DET: return "CLR_STOP_DET"; + case DW_IC_CLR_START_DET: return "CLR_START_DET"; + case DW_IC_CLR_GEN_CALL: return "CLR_GEN_CALL"; + case DW_IC_ENABLE: return "ENABLE"; + case DW_IC_STATUS: return "STATUS"; + case DW_IC_TXFLR: return "TXFLR"; + case DW_IC_RXFLR: return "RXFLR"; + case DW_IC_SDA_HOLD: return "SDA_HOLD"; + case DW_IC_TX_ABRT_SOURCE: return "TX_ABRT_SOURCE"; + case DW_IC_ENABLE_STATUS: return "ENABLE_STATUS"; + case DW_IC_COMP_PARAM_1: return "COMP_PARAM_1"; + case DW_IC_COMP_VERSION: return "COMP_VERSION"; + case DW_IC_COMP_TYPE: return "COMP_TYPE"; + default: return "[?]"; + } +} + +/* + * If we change reg_raw_intr_stat or reg_intr_mask, + * must call this function to update reg_intr_stat and irq line. + */ +static void dw_i2c_update_intr(DWI2CState *s) +{ + s->reg_intr_stat = s->reg_raw_intr_stat & s->reg_intr_mask; + if (s->reg_intr_stat) { + qemu_irq_raise(s->irq); + } else { + qemu_irq_lower(s->irq); + } +} + +static void dw_i2c_try_clear_intr(DWI2CState *s) +{ + if (!s->reg_intr_stat) { + qemu_irq_lower(s->irq); + } +} + +static uint32_t dw_i2c_read_data_cmd(DWI2CState *s) +{ + uint32_t byte = 0; + + if (fifo8_is_empty(&s->rx_fifo)) { + s->reg_raw_intr_stat |= DW_IC_INTR_RX_UNDER; + dw_i2c_update_intr(s); + } else { + byte = fifo8_pop(&s->rx_fifo); + + /* + * Driver may set reg_rx_tl as 0, + * so we also need to check if rx_fifo is empty here. + */ + if (fifo8_num_used(&s->rx_fifo) < s->reg_rx_tl || + fifo8_is_empty(&s->rx_fifo)) { + s->reg_raw_intr_stat &= ~DW_IC_INTR_RX_FULL; + dw_i2c_update_intr(s); + } + } + + return byte; +} + +static uint64_t dw_i2c_read(void *opaque, hwaddr offset, unsigned size) +{ + DWI2CState *s = DW_I2C(opaque); + uint32_t val = 0; + + switch (offset) { + case DW_IC_CON: + val = s->reg_con; + break; + case DW_IC_TAR: + val = s->reg_tar; + break; + case DW_IC_SAR: + qemu_log_mask(LOG_UNIMP, "[%s]%s: slave mode not implemented\n", + TYPE_DW_I2C, __func__); + break; + case DW_IC_DATA_CMD: + val = dw_i2c_read_data_cmd(s); + break; + case DW_IC_INTR_STAT: + val = s->reg_intr_stat; + break; + case DW_IC_INTR_MASK: + val = s->reg_intr_mask; + break; + case DW_IC_RAW_INTR_STAT: + val = s->reg_raw_intr_stat; + break; + case DW_IC_RX_TL: + val = s->reg_rx_tl; + break; + case DW_IC_TX_TL: + val = s->reg_tx_tl; + break; + case DW_IC_CLR_INTR: + s->reg_intr_stat = 0; + s->reg_tx_abrt_source = 0; + dw_i2c_try_clear_intr(s); + break; + case DW_IC_CLR_RX_UNDER: + s->reg_raw_intr_stat &= ~DW_IC_INTR_RX_UNDER; + dw_i2c_update_intr(s); + break; + case DW_IC_CLR_RX_OVER: + s->reg_raw_intr_stat &= ~DW_IC_INTR_RX_OVER; + dw_i2c_update_intr(s); + break; + case DW_IC_CLR_TX_OVER: + s->reg_raw_intr_stat &= ~DW_IC_INTR_TX_OVER; + dw_i2c_update_intr(s); + break; + case DW_IC_CLR_RD_REQ: + s->reg_raw_intr_stat &= ~DW_IC_INTR_RD_REQ; + dw_i2c_update_intr(s); + break; + case DW_IC_CLR_TX_ABRT: + s->reg_raw_intr_stat &= ~DW_IC_INTR_TX_ABRT; + s->reg_tx_abrt_source = 0; + dw_i2c_update_intr(s); + break; + case DW_IC_CLR_RX_DONE: + s->reg_raw_intr_stat &= ~DW_IC_INTR_RX_DONE; + dw_i2c_update_intr(s); + break; + case DW_IC_CLR_ACTIVITY: + s->reg_raw_intr_stat &= ~DW_IC_INTR_ACTIVITY; + dw_i2c_update_intr(s); + break; + case DW_IC_CLR_STOP_DET: + s->reg_raw_intr_stat &= ~DW_IC_INTR_STOP_DET; + dw_i2c_update_intr(s); + break; + case DW_IC_CLR_START_DET: + s->reg_raw_intr_stat &= ~DW_IC_INTR_START_DET; + dw_i2c_update_intr(s); + break; + case DW_IC_ENABLE: + val = s->reg_enable; + break; + case DW_IC_STATUS: + val = s->reg_status; + break; + case DW_IC_TXFLR: + val = s->reg_txflr; + break; + case DW_IC_RXFLR: + s->reg_rxflr = fifo8_num_used(&s->rx_fifo); + val = s->reg_rxflr; + break; + case DW_IC_SDA_HOLD: + val = s->reg_sda_hold; + break; + case DW_IC_TX_ABRT_SOURCE: + val = s->reg_tx_abrt_source; + break; + case DW_IC_ENABLE_STATUS: + val = s->reg_enable_status; + break; + case DW_IC_COMP_PARAM_1: + val = s->reg_comp_param_1; + break; + case DW_IC_COMP_VERSION: + val = s->reg_comp_param_ver; + break; + case DW_IC_COMP_TYPE: + val = s->reg_comp_type_num; + break; + default: + qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad read addr at offset 0x%" + HWADDR_PRIx "\n", TYPE_DW_I2C, __func__, offset); + break; + } + + trace_dw_i2c_read(DEVICE(s)->canonical_path, dw_i2c_get_regname(offset), + offset, val); + + return (uint64_t)val; +} + +static void dw_i2c_write_con(DWI2CState *s, uint32_t val) +{ + if (!(s->reg_enable & DW_IC_ENABLE_ENABLE)) { + s->reg_con = val; + } +} + +static void dw_i2c_write_tar(DWI2CState *s, uint32_t val) +{ + /* 10 bit address mode not support in current I2C bus core */ + if (val & DW_IC_TAR_10BITADDR_MASTER) { + qemu_log_mask(LOG_UNIMP, "[%s]%s: 10 bit addr not implemented\n", + TYPE_DW_I2C, __func__); + return; + } + + if (!(s->reg_enable & DW_IC_ENABLE_ENABLE)) { + /* + * DesignWare I2C controller uses r/w bit in DW_IC_DATA_CMD + * to indicate r/w operation, so linux driver will not set + * the r/w bit in DW_IC_TAR, this value is the final slave + * address on the I2C bus. + */ + s->reg_tar = val; + s->addr_mask = 0x7f; + } +} + +static void dw_i2c_write_data_cmd(DWI2CState *s, uint32_t val) +{ + bool no_ack = false; + uint8_t byte = val & DW_IC_DATA_CMD_DAT_MASK; + + if (!(s->reg_enable & DW_IC_ENABLE_ENABLE)) { + return; + } + + if (!s->bus_active) { + if (i2c_start_transfer(s->bus, (s->reg_tar & s->addr_mask), + val & DW_IC_DATA_CMD_READ)) { + no_ack = true; + } else { + s->bus_active = true; + } + } + + if (s->bus_active) { + if (val & DW_IC_DATA_CMD_READ) { + byte = i2c_recv(s->bus); + + if (fifo8_is_full(&s->rx_fifo)) { + s->reg_raw_intr_stat |= DW_IC_INTR_RX_OVER; + } else { + fifo8_push(&s->rx_fifo, byte); + + if (fifo8_num_used(&s->rx_fifo) >= s->reg_rx_tl) { + s->reg_raw_intr_stat |= DW_IC_INTR_RX_FULL; + } + } + } else { + if (i2c_send(s->bus, byte)) { + no_ack = true; + } else { + s->reg_raw_intr_stat |= DW_IC_INTR_TX_EMPTY; + } + } + } + + if (no_ack) { + i2c_end_transfer(s->bus); + s->bus_active = false; + s->reg_raw_intr_stat |= DW_IC_INTR_TX_ABRT; + s->reg_tx_abrt_source |= DW_IC_TX_ABRT_7B_ADDR_NOACK; + } + + if (val & DW_IC_DATA_CMD_STOP) { + i2c_end_transfer(s->bus); + s->bus_active = false; + + if (val & DW_IC_DATA_CMD_READ) { + s->reg_raw_intr_stat |= DW_IC_INTR_RX_DONE; + } + + s->reg_raw_intr_stat |= DW_IC_INTR_STOP_DET; + } + + dw_i2c_update_intr(s); +} + +static void dw_i2c_write_enable(DWI2CState *s, uint32_t val) +{ + s->reg_enable = val; + + if (s->reg_enable & DW_IC_ENABLE_ENABLE) { + if (i2c_scan_bus(s->bus, (s->reg_tar & s->addr_mask), 0, + &s->bus->current_devs)) { + s->reg_raw_intr_stat |= DW_IC_INTR_START_DET | DW_IC_INTR_TX_EMPTY | + DW_IC_INTR_ACTIVITY; + s->reg_status |= DW_IC_STATUS_ACTIVITY; + } else { + s->reg_raw_intr_stat |= DW_IC_INTR_TX_ABRT; + s->reg_status &= ~DW_IC_STATUS_ACTIVITY; + s->reg_tx_abrt_source |= DW_IC_TX_ABRT_7B_ADDR_NOACK; + } + + s->reg_enable_status |= DW_IC_ENABLE_STATUS_EN; + } else { + i2c_end_transfer(s->bus); + fifo8_reset(&s->rx_fifo); + + s->addr_mask = 0; + s->bus_active = false; + s->reg_status = 0; + s->reg_enable_status = 0; + s->reg_raw_intr_stat = 0; + } + + dw_i2c_update_intr(s); +} + +static void dw_i2c_write(void *opaque, hwaddr offset, uint64_t value, + unsigned size) +{ + DWI2CState *s = DW_I2C(opaque); + uint32_t val = value & 0xffffffff; + + trace_dw_i2c_write(DEVICE(s)->canonical_path, dw_i2c_get_regname(offset), + offset, val); + + switch (offset) { + case DW_IC_CON: + dw_i2c_write_con(s, val); + break; + case DW_IC_TAR: + dw_i2c_write_tar(s, val); + break; + case DW_IC_SAR: + qemu_log_mask(LOG_UNIMP, "[%s]%s: slave mode not implemented\n", + TYPE_DW_I2C, __func__); + break; + case DW_IC_DATA_CMD: + dw_i2c_write_data_cmd(s, val); + break; + case DW_IC_SS_SCL_HCNT: + s->reg_ss_scl_hcnt = val; + break; + case DW_IC_SS_SCL_LCNT: + s->reg_ss_scl_lcnt = val; + break; + case DW_IC_FS_SCL_HCNT: + s->reg_fs_scl_hcnt = val; + break; + case DW_IC_FS_SCL_LCNT: + s->reg_fs_scl_lcnt = val; + break; + case DW_IC_INTR_MASK: + s->reg_intr_mask = val; + dw_i2c_update_intr(s); + break; + case DW_IC_RX_TL: + s->reg_rx_tl = val; + break; + case DW_IC_TX_TL: + s->reg_tx_tl = val; + break; + case DW_IC_SDA_HOLD: + s->reg_sda_hold = val; + break; + case DW_IC_ENABLE: + dw_i2c_write_enable(s, val); + break; + default: + qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad write addr at offset 0x%" + HWADDR_PRIx "\n", TYPE_DW_I2C, __func__, offset); + break; + } +} + +static void dw_i2c_reset(DeviceState *dev) +{ + DWI2CState *s = DW_I2C(dev); + s->bus_active = false; + s->addr_mask = 0; + fifo8_reset(&s->rx_fifo); + + s->reg_con = 0; + s->reg_tar = 0; + s->reg_ss_scl_hcnt = 0; + s->reg_ss_scl_lcnt = 0; + s->reg_fs_scl_hcnt = 0; + s->reg_fs_scl_lcnt = 0; + s->reg_intr_stat = 0; + s->reg_intr_mask = 0; + s->reg_raw_intr_stat = 0; + s->reg_rx_tl = 0; + s->reg_tx_tl = 0; + s->reg_enable = 0; + s->reg_status = 0; + s->reg_txflr = 0; + s->reg_rxflr = 0; + s->reg_tx_abrt_source = 0; + s->reg_enable_status = 0; + s->reg_comp_param_1 = DW_IC_COMP_PARAM_1_VALUE; + s->reg_comp_param_ver = DW_IC_SDA_HOLD_MIN_VERS; + s->reg_comp_type_num = DW_IC_COMP_TYPE_VALUE; +} + +static const MemoryRegionOps dw_i2c_ops = { + .read = dw_i2c_read, + .write = dw_i2c_write, + .valid.min_access_size = 4, + .valid.max_access_size = 4, + .endianness = DEVICE_NATIVE_ENDIAN, +}; + +static const VMStateDescription dw_i2c_vmstate = { + .name = TYPE_DW_I2C, + .version_id = 1, + .minimum_version_id = 1, + .fields = (const VMStateField[]) { + VMSTATE_FIFO8(rx_fifo, DWI2CState), + VMSTATE_BOOL(bus_active, DWI2CState), + VMSTATE_UINT32(addr_mask, DWI2CState), + VMSTATE_UINT32(reg_con, DWI2CState), + VMSTATE_UINT32(reg_tar, DWI2CState), + VMSTATE_UINT32(reg_ss_scl_hcnt, DWI2CState), + VMSTATE_UINT32(reg_ss_scl_lcnt, DWI2CState), + VMSTATE_UINT32(reg_fs_scl_hcnt, DWI2CState), + VMSTATE_UINT32(reg_fs_scl_lcnt, DWI2CState), + VMSTATE_UINT32(reg_intr_stat, DWI2CState), + VMSTATE_UINT32(reg_intr_mask, DWI2CState), + VMSTATE_UINT32(reg_raw_intr_stat, DWI2CState), + VMSTATE_UINT32(reg_rx_tl, DWI2CState), + VMSTATE_UINT32(reg_tx_tl, DWI2CState), + VMSTATE_UINT32(reg_sda_hold, DWI2CState), + VMSTATE_UINT32(reg_enable, DWI2CState), + VMSTATE_UINT32(reg_status, DWI2CState), + VMSTATE_UINT32(reg_txflr, DWI2CState), + VMSTATE_UINT32(reg_rxflr, DWI2CState), + VMSTATE_UINT32(reg_tx_abrt_source, DWI2CState), + VMSTATE_UINT32(reg_enable_status, DWI2CState), + VMSTATE_UINT32(reg_comp_param_1, DWI2CState), + VMSTATE_UINT32(reg_comp_param_ver, DWI2CState), + VMSTATE_UINT32(reg_comp_type_num, DWI2CState), + VMSTATE_END_OF_LIST() + } +}; + +static void dw_i2c_realize(DeviceState *dev, Error **errp) +{ + DWI2CState *s = DW_I2C(dev); + + memory_region_init_io(&s->iomem, OBJECT(s), &dw_i2c_ops, s, + TYPE_DW_I2C, 0x1000); + sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem); + sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq); + s->bus = i2c_init_bus(dev, NULL); + fifo8_create(&s->rx_fifo, DW_I2C_RX_FIFO_DEPTH); +} + +static void dw_i2c_class_init(ObjectClass *klass, const void *data) +{ + DeviceClass *dc = DEVICE_CLASS(klass); + + dc->vmsd = &dw_i2c_vmstate; + device_class_set_legacy_reset(dc, dw_i2c_reset); + dc->realize = dw_i2c_realize; + dc->desc = "DesignWare I2C controller"; +} + +static const TypeInfo dw_i2c_type_info = { + .name = TYPE_DW_I2C, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_size = sizeof(DWI2CState), + .class_init = dw_i2c_class_init, +}; + +static void dw_i2c_register_types(void) +{ + type_register_static(&dw_i2c_type_info); +} + +type_init(dw_i2c_register_types) diff --git a/hw/i2c/meson.build b/hw/i2c/meson.build index c459adcb59..fb92df0b69 100644 --- a/hw/i2c/meson.build +++ b/hw/i2c/meson.build @@ -7,6 +7,7 @@ i2c_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_i2c.c')) i2c_ss.add(when: 'CONFIG_BITBANG_I2C', if_true: files('bitbang_i2c.c')) i2c_ss.add(when: 'CONFIG_EXYNOS4', if_true: files('exynos4210_i2c.c')) i2c_ss.add(when: 'CONFIG_IMX_I2C', if_true: files('imx_i2c.c')) +i2c_ss.add(when: 'CONFIG_DW_I2C', if_true: files('dw_i2c.c')) i2c_ss.add(when: 'CONFIG_MPC_I2C', if_true: files('mpc_i2c.c')) i2c_ss.add(when: 'CONFIG_ALLWINNER_I2C', if_true: files('allwinner-i2c.c')) i2c_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('microbit_i2c.c')) diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events index 1ad0e95c0e..5f65755a4e 100644 --- a/hw/i2c/trace-events +++ b/hw/i2c/trace-events @@ -61,3 +61,7 @@ pca954x_read_data(uint8_t value) "PCA954X read data: 0x%02x" imx_i2c_read(const char *id, const char *reg, uint64_t ofs, uint64_t value) "%s:[%s (0x%" PRIx64 ")] -> 0x%02" PRIx64 imx_i2c_write(const char *id, const char *reg, uint64_t ofs, uint64_t value) "%s:[%s (0x%" PRIx64 ")] <- 0x%02" PRIx64 + +# dw_i2c.c +dw_i2c_read(const char *id, const char *reg, uint64_t ofs, uint64_t value) "%s:[%s (0x%" PRIx64 ")] -> 0x%02" PRIx64 +dw_i2c_write(const char *id, const char *reg, uint64_t ofs, uint64_t value) "%s:[%s (0x%" PRIx64 ")] <- 0x%02" PRIx64 diff --git a/include/hw/i2c/dw_i2c.h b/include/hw/i2c/dw_i2c.h new file mode 100644 index 0000000000..512c749f21 --- /dev/null +++ b/include/hw/i2c/dw_i2c.h @@ -0,0 +1,151 @@ +/* + * DesignWare I2C Bus Serial Interface Emulation + * + * Copyright (C) 2026 Alano Song <AlanoSong@163.com> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, see <http://www.gnu.org/licenses/>. + * + */ + +#ifndef DW_I2C_H +#define DW_I2C_H + +#include "hw/core/sysbus.h" +#include "qom/object.h" +#include "qemu/fifo8.h" + +#define TYPE_DW_I2C "dw.i2c" +OBJECT_DECLARE_SIMPLE_TYPE(DWI2CState, DW_I2C) + +#define DW_I2C_TX_FIFO_DEPTH 16 +#define DW_I2C_RX_FIFO_DEPTH 16 + +#define DW_IC_CON 0x00 +#define DW_IC_TAR 0x04 +#define DW_IC_SAR 0x08 +#define DW_IC_DATA_CMD 0x10 +#define DW_IC_SS_SCL_HCNT 0x14 +#define DW_IC_SS_SCL_LCNT 0x18 +#define DW_IC_FS_SCL_HCNT 0x1c +#define DW_IC_FS_SCL_LCNT 0x20 +#define DW_IC_INTR_STAT 0x2c +#define DW_IC_INTR_MASK 0x30 +#define DW_IC_RAW_INTR_STAT 0x34 +#define DW_IC_RX_TL 0x38 +#define DW_IC_TX_TL 0x3c +#define DW_IC_CLR_INTR 0x40 +#define DW_IC_CLR_RX_UNDER 0x44 +#define DW_IC_CLR_RX_OVER 0x48 +#define DW_IC_CLR_TX_OVER 0x4c +#define DW_IC_CLR_RD_REQ 0x50 +#define DW_IC_CLR_TX_ABRT 0x54 +#define DW_IC_CLR_RX_DONE 0x58 +#define DW_IC_CLR_ACTIVITY 0x5c +#define DW_IC_CLR_STOP_DET 0x60 +#define DW_IC_CLR_START_DET 0x64 +#define DW_IC_CLR_GEN_CALL 0x68 +#define DW_IC_ENABLE 0x6c +#define DW_IC_STATUS 0x70 +#define DW_IC_TXFLR 0x74 +#define DW_IC_RXFLR 0x78 +#define DW_IC_SDA_HOLD 0x7c +#define DW_IC_TX_ABRT_SOURCE 0x80 +#define DW_IC_ENABLE_STATUS 0x9c +#define DW_IC_COMP_PARAM_1 0xf4 +#define DW_IC_COMP_VERSION 0xf8 +#define DW_IC_COMP_TYPE 0xfc + +#define DW_IC_CON_MASTER BIT(0) +#define DW_IC_CON_SPEED_STANDARD (0x1 << 1) +#define DW_IC_CON_SPEED_FAST (0x2 << 1) +#define DW_IC_CON_SPEED_HIGH (0x3 << 1) +#define DW_IC_CON_RESTART_EN BIT(5) + +#define DW_IC_TAR_10BITADDR_MASTER BIT(12) + +#define DW_IC_DATA_CMD_DAT_MASK 0xff +#define DW_IC_DATA_CMD_READ BIT(8) +#define DW_IC_DATA_CMD_STOP BIT(9) +#define DW_IC_DATA_CMD_RESTART BIT(10) + +#define DW_IC_INTR_RX_UNDER BIT(0) +#define DW_IC_INTR_RX_OVER BIT(1) +#define DW_IC_INTR_RX_FULL BIT(2) +#define DW_IC_INTR_TX_OVER BIT(3) +#define DW_IC_INTR_TX_EMPTY BIT(4) +#define DW_IC_INTR_RD_REQ BIT(5) +#define DW_IC_INTR_TX_ABRT BIT(6) +#define DW_IC_INTR_RX_DONE BIT(7) +#define DW_IC_INTR_ACTIVITY BIT(8) +#define DW_IC_INTR_STOP_DET BIT(9) +#define DW_IC_INTR_START_DET BIT(10) + +#define DW_IC_ENABLE_ENABLE BIT(0) +#define DW_IC_ENABLE_ABORT BIT(1) + +#define DW_IC_STATUS_ACTIVITY BIT(0) +#define DW_IC_STATUS_TFNF BIT(1) +#define DW_IC_STATUS_TFE BIT(2) +#define DW_IC_STATUS_RFNE BIT(3) +#define DW_IC_STATUS_RFF BIT(4) +#define DW_IC_STATUS_MASTER_ACTIVITY BIT(5) + +#define DW_IC_TX_ABRT_7B_ADDR_NOACK BIT(0) + +#define DW_IC_ENABLE_STATUS_EN BIT(0) + +#define DW_IC_COMP_PARAM_1_SPEED_MODE_FAST (0x2 << 2) +#define DW_IC_COMP_PARAM_1_VALUE (((DW_I2C_TX_FIFO_DEPTH - 1) & 0xff) << 16 | \ + ((DW_I2C_RX_FIFO_DEPTH - 1) & 0xff) << 8 | \ + DW_IC_COMP_PARAM_1_SPEED_MODE_FAST) +#define DW_IC_SDA_HOLD_MIN_VERS 0x3131312a /* "111*" == v1.11* */ +#define DW_IC_COMP_TYPE_VALUE 0x44570140 /* "DW" + 0x0140 */ + +typedef struct DWI2CState { + /*< private >*/ + SysBusDevice parent_obj; + + /*< public >*/ + MemoryRegion iomem; + qemu_irq irq; + I2CBus *bus; + + bool bus_active; + Fifo8 rx_fifo; + uint32_t addr_mask; + + uint32_t reg_con; + uint32_t reg_tar; + uint32_t reg_ss_scl_hcnt; + uint32_t reg_ss_scl_lcnt; + uint32_t reg_fs_scl_hcnt; + uint32_t reg_fs_scl_lcnt; + uint32_t reg_intr_stat; + uint32_t reg_intr_mask; + uint32_t reg_raw_intr_stat; + uint32_t reg_rx_tl; + uint32_t reg_tx_tl; + uint32_t reg_sda_hold; + uint32_t reg_enable; + uint32_t reg_status; + uint32_t reg_txflr; + uint32_t reg_rxflr; + uint32_t reg_tx_abrt_source; + uint32_t reg_enable_status; + uint32_t reg_comp_param_1; + uint32_t reg_comp_param_ver; + uint32_t reg_comp_type_num; +} DWI2CState; + +#endif /* DW_I2C_H */ -- 2.43.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] hw/i2c/dw: Add DesignWare I2C controller emulator 2026-01-06 13:12 ` [PATCH 1/2] " AlanoSong @ 2026-01-06 15:52 ` Jonathan Cameron via 0 siblings, 0 replies; 11+ messages in thread From: Jonathan Cameron via @ 2026-01-06 15:52 UTC (permalink / raw) To: AlanoSong Cc: qemu-devel, qemu-arm, cminyard, peter.maydell, philmd, ani, pbonzini, shannon.zhaosl On Tue, 6 Jan 2026 21:12:52 +0800 AlanoSong@163.com wrote: > Add DesignWare I2C controller according to DesignWare > I2C databook v2.01a. > Confirmed the model with i2c-tools under v6.18 > linux driver. > > The slave mode is not implemented, cause this feature > is usually not used. > > The 10 bit slave address is not implemented, cause > this feature is usually not used, and not supported > by qemu I2C core bus currently. > > Signed-off-by: Alano Song <AlanoSong@163.com> Hi Alano, A very superficial review. Sadly I don't have time to dig much deeper at the moment. Good to see this support. Jonathan > diff --git a/hw/i2c/dw_i2c.c b/hw/i2c/dw_i2c.c > new file mode 100644 > index 0000000000..a5a31ec78c > --- /dev/null > +++ b/hw/i2c/dw_i2c.c > @@ -0,0 +1,517 @@ > +/* > + * DesignWare I2C Bus Serial Interface Emulation > + * > + * Copyright (C) 2026 Alano Song <AlanoSong@163.com> > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the > + * Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License > + * for more details. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program; if not, see <http://www.gnu.org/licenses/>. > + * > + */ As below. Can probably replace the boilerplate with an SPDX license line and just have the copyright + intro text here. > diff --git a/include/hw/i2c/dw_i2c.h b/include/hw/i2c/dw_i2c.h > new file mode 100644 > index 0000000000..512c749f21 > --- /dev/null > +++ b/include/hw/i2c/dw_i2c.h > @@ -0,0 +1,151 @@ > +/* > + * DesignWare I2C Bus Serial Interface Emulation > + * > + * Copyright (C) 2026 Alano Song <AlanoSong@163.com> > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the > + * Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License > + * for more details. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program; if not, see <http://www.gnu.org/licenses/>. For new code in QEMU I think we are fine with just SPDX. See many examples in tree. It saves on repeating this boilerplate. > + * > + */ > + > +#ifndef DW_I2C_H > +#define DW_I2C_H > + > +#include "hw/core/sysbus.h" > +#include "qom/object.h" > +#include "qemu/fifo8.h" > + > +#define TYPE_DW_I2C "dw.i2c" > +OBJECT_DECLARE_SIMPLE_TYPE(DWI2CState, DW_I2C) > + > +#define DW_I2C_TX_FIFO_DEPTH 16 > +#define DW_I2C_RX_FIFO_DEPTH 16 > + > +#define DW_IC_CON_MASTER BIT(0) > +#define DW_IC_CON_SPEED_STANDARD (0x1 << 1) > +#define DW_IC_CON_SPEED_FAST (0x2 << 1) > +#define DW_IC_CON_SPEED_HIGH (0x3 << 1) Smells of value being written to a field rather than a series of values with meaning on their own. Qemu has a rich set of register field macros. I'd look at using those to make your life easier. > +#define DW_IC_CON_RESTART_EN BIT(5) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] hw/i2c/dw: Add DesignWare I2C controller emulator @ 2026-01-06 15:52 ` Jonathan Cameron via 0 siblings, 0 replies; 11+ messages in thread From: Jonathan Cameron via @ 2026-01-06 15:52 UTC (permalink / raw) To: AlanoSong Cc: qemu-devel, qemu-arm, cminyard, peter.maydell, philmd, ani, pbonzini, shannon.zhaosl On Tue, 6 Jan 2026 21:12:52 +0800 AlanoSong@163.com wrote: > Add DesignWare I2C controller according to DesignWare > I2C databook v2.01a. > Confirmed the model with i2c-tools under v6.18 > linux driver. > > The slave mode is not implemented, cause this feature > is usually not used. > > The 10 bit slave address is not implemented, cause > this feature is usually not used, and not supported > by qemu I2C core bus currently. > > Signed-off-by: Alano Song <AlanoSong@163.com> Hi Alano, A very superficial review. Sadly I don't have time to dig much deeper at the moment. Good to see this support. Jonathan > diff --git a/hw/i2c/dw_i2c.c b/hw/i2c/dw_i2c.c > new file mode 100644 > index 0000000000..a5a31ec78c > --- /dev/null > +++ b/hw/i2c/dw_i2c.c > @@ -0,0 +1,517 @@ > +/* > + * DesignWare I2C Bus Serial Interface Emulation > + * > + * Copyright (C) 2026 Alano Song <AlanoSong@163.com> > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the > + * Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License > + * for more details. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program; if not, see <http://www.gnu.org/licenses/>. > + * > + */ As below. Can probably replace the boilerplate with an SPDX license line and just have the copyright + intro text here. > diff --git a/include/hw/i2c/dw_i2c.h b/include/hw/i2c/dw_i2c.h > new file mode 100644 > index 0000000000..512c749f21 > --- /dev/null > +++ b/include/hw/i2c/dw_i2c.h > @@ -0,0 +1,151 @@ > +/* > + * DesignWare I2C Bus Serial Interface Emulation > + * > + * Copyright (C) 2026 Alano Song <AlanoSong@163.com> > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the > + * Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License > + * for more details. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program; if not, see <http://www.gnu.org/licenses/>. For new code in QEMU I think we are fine with just SPDX. See many examples in tree. It saves on repeating this boilerplate. > + * > + */ > + > +#ifndef DW_I2C_H > +#define DW_I2C_H > + > +#include "hw/core/sysbus.h" > +#include "qom/object.h" > +#include "qemu/fifo8.h" > + > +#define TYPE_DW_I2C "dw.i2c" > +OBJECT_DECLARE_SIMPLE_TYPE(DWI2CState, DW_I2C) > + > +#define DW_I2C_TX_FIFO_DEPTH 16 > +#define DW_I2C_RX_FIFO_DEPTH 16 > + > +#define DW_IC_CON_MASTER BIT(0) > +#define DW_IC_CON_SPEED_STANDARD (0x1 << 1) > +#define DW_IC_CON_SPEED_FAST (0x2 << 1) > +#define DW_IC_CON_SPEED_HIGH (0x3 << 1) Smells of value being written to a field rather than a series of values with meaning on their own. Qemu has a rich set of register field macros. I'd look at using those to make your life easier. > +#define DW_IC_CON_RESTART_EN BIT(5) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re:Re: [PATCH 1/2] hw/i2c/dw: Add DesignWare I2C controller emulator 2026-01-06 15:52 ` Jonathan Cameron via (?) @ 2026-01-07 1:50 ` Alano Song -1 siblings, 0 replies; 11+ messages in thread From: Alano Song @ 2026-01-07 1:50 UTC (permalink / raw) To: Jonathan Cameron Cc: qemu-devel, qemu-arm, cminyard, peter.maydell, philmd, ani, pbonzini, shannon.zhaosl [-- Attachment #1: Type: text/plain, Size: 4061 bytes --] At 2026-01-06 23:52:18, "Jonathan Cameron" <jonathan.cameron@huawei.com> wrote: >On Tue, 6 Jan 2026 21:12:52 +0800 >AlanoSong@163.com wrote: > >> Add DesignWare I2C controller according to DesignWare >> I2C databook v2.01a. >> Confirmed the model with i2c-tools under v6.18 >> linux driver. >> >> The slave mode is not implemented, cause this feature >> is usually not used. >> >> The 10 bit slave address is not implemented, cause >> this feature is usually not used, and not supported >> by qemu I2C core bus currently. >> >> Signed-off-by: Alano Song <AlanoSong@163.com> >Hi Alano, > >A very superficial review. Sadly I don't have time to dig >much deeper at the moment. > >Good to see this support. > >Jonathan > Still appreciate for your patient review and reply! >> diff --git a/hw/i2c/dw_i2c.c b/hw/i2c/dw_i2c.c >> new file mode 100644 >> index 0000000000..a5a31ec78c >> --- /dev/null >> +++ b/hw/i2c/dw_i2c.c >> @@ -0,0 +1,517 @@ >> +/* >> + * DesignWare I2C Bus Serial Interface Emulation >> + * >> + * Copyright (C) 2026 Alano Song <AlanoSong@163.com> >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU General Public License as published by the >> + * Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, but WITHOUT >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License >> + * for more details. >> + * >> + * You should have received a copy of the GNU General Public License along >> + * with this program; if not, see <http://www.gnu.org/licenses/>. >> + * >> + */ >As below. Can probably replace the boilerplate with an SPDX license >line and just have the copyright + intro text here. > Okay, I will submit version 2 patch to refine it. > >> diff --git a/include/hw/i2c/dw_i2c.h b/include/hw/i2c/dw_i2c.h >> new file mode 100644 >> index 0000000000..512c749f21 >> --- /dev/null >> +++ b/include/hw/i2c/dw_i2c.h >> @@ -0,0 +1,151 @@ >> +/* >> + * DesignWare I2C Bus Serial Interface Emulation >> + * >> + * Copyright (C) 2026 Alano Song <AlanoSong@163.com> >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU General Public License as published by the >> + * Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, but WITHOUT >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License >> + * for more details. >> + * >> + * You should have received a copy of the GNU General Public License along >> + * with this program; if not, see <http://www.gnu.org/licenses/>. > >For new code in QEMU I think we are fine with just SPDX. See many examples >in tree. It saves on repeating this boilerplate. > Okay, I will submit version 2 patch to refine it. >> + * >> + */ >> + >> +#ifndef DW_I2C_H >> +#define DW_I2C_H >> + >> +#include "hw/core/sysbus.h" >> +#include "qom/object.h" >> +#include "qemu/fifo8.h" >> + >> +#define TYPE_DW_I2C "dw.i2c" >> +OBJECT_DECLARE_SIMPLE_TYPE(DWI2CState, DW_I2C) >> + >> +#define DW_I2C_TX_FIFO_DEPTH 16 >> +#define DW_I2C_RX_FIFO_DEPTH 16 > >> + >> +#define DW_IC_CON_MASTER BIT(0) >> +#define DW_IC_CON_SPEED_STANDARD (0x1 << 1) >> +#define DW_IC_CON_SPEED_FAST (0x2 << 1) >> +#define DW_IC_CON_SPEED_HIGH (0x3 << 1) > >Smells of value being written to a field rather than >a series of values with meaning on their own. Qemu >has a rich set of register field macros. I'd look >at using those to make your life easier. > Your suggestion is good, I will refine this part in version 2 patch. >> +#define DW_IC_CON_RESTART_EN BIT(5) [-- Attachment #2: Type: text/html, Size: 5075 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] hw/arm/virt: Add DesignWare I2C controller 2026-01-06 13:12 [PATCH 0/2] hw/i2c/dw: Add DesignWare I2C controller emulator AlanoSong 2026-01-06 13:12 ` [PATCH 1/2] " AlanoSong @ 2026-01-06 13:12 ` AlanoSong 2026-01-06 15:45 ` Jonathan Cameron via 1 sibling, 1 reply; 11+ messages in thread From: AlanoSong @ 2026-01-06 13:12 UTC (permalink / raw) To: qemu-devel, qemu-arm Cc: cminyard, peter.maydell, philmd, ani, pbonzini, shannon.zhaosl, Alano Song From: Alano Song <AlanoSong@163.com> Add DesignWare I2C controller onto virt board, and also an at24c eeprom for r/w operation. Add these two devices into arm virt acpi table. Confirmed with i2c-tools under v6.18 linux driver. Signed-off-by: Alano Song <AlanoSong@163.com> --- hw/arm/Kconfig | 1 + hw/arm/virt-acpi-build.c | 32 ++++++++++++++++++++++++++++++++ hw/arm/virt.c | 38 +++++++++++++++++++++++++++++++++++++- include/hw/arm/virt.h | 1 + 4 files changed, 71 insertions(+), 1 deletion(-) diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig index 97d747e206..f23c063474 100644 --- a/hw/arm/Kconfig +++ b/hw/arm/Kconfig @@ -36,6 +36,7 @@ config ARM_VIRT select VIRTIO_MEM_SUPPORTED select ACPI_CXL select ACPI_HMAT + select DW_I2C config CUBIEBOARD bool diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index 03b4342574..3d06356169 100644 --- a/hw/arm/virt-acpi-build.c +++ b/hw/arm/virt-acpi-build.c @@ -100,6 +100,34 @@ static void acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap, aml_append(scope, dev); } +static void acpi_dsdt_add_i2c(Aml *scope, const MemMapEntry *i2c_memmap, + uint32_t i2c_irq) +{ + Aml *i2c_dev, *eprm_dev, *crs; + + i2c_dev = aml_device("I2C0"); + aml_append(i2c_dev, aml_name_decl("_HID", aml_string("INT3433"))); + aml_append(i2c_dev, aml_name_decl("_UID", aml_int(0))); + + crs = aml_resource_template(); + aml_append(crs, aml_memory32_fixed(i2c_memmap->base, + i2c_memmap->size, AML_READ_WRITE)); + aml_append(crs, aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH, + AML_EXCLUSIVE, &i2c_irq, 1)); + aml_append(i2c_dev, aml_name_decl("_CRS", crs)); + + eprm_dev = aml_device("EPRM"); + aml_append(eprm_dev, aml_name_decl("_HID", aml_string("INT3499"))); + aml_append(eprm_dev, aml_name_decl("_UID", aml_int(0))); + + crs = aml_resource_template(); + aml_append(crs, aml_i2c_serial_bus_device(0x50, "^")); + aml_append(eprm_dev, aml_name_decl("_CRS", crs)); + + aml_append(i2c_dev, eprm_dev); + aml_append(scope, i2c_dev); +} + static void acpi_dsdt_add_flash(Aml *scope, const MemMapEntry *flash_memmap) { Aml *dev, *crs; @@ -1037,6 +1065,10 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) acpi_dsdt_add_uart(scope, &memmap[VIRT_UART1], (irqmap[VIRT_UART1] + ARM_SPI_BASE), 1); } + + acpi_dsdt_add_i2c(scope, &memmap[VIRT_I2C], + irqmap[VIRT_I2C] + ARM_SPI_BASE); + if (vmc->acpi_expose_flash) { acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]); } diff --git a/hw/arm/virt.c b/hw/arm/virt.c index fd0e28f030..8fd37126d1 100644 --- a/hw/arm/virt.c +++ b/hw/arm/virt.c @@ -38,6 +38,8 @@ #include "hw/arm/boot.h" #include "hw/arm/primecell.h" #include "hw/arm/virt.h" +#include "hw/i2c/dw_i2c.h" +#include "hw/nvram/eeprom_at24c.h" #include "hw/arm/machines-qom.h" #include "hw/block/flash.h" #include "hw/display/ramfb.h" @@ -193,7 +195,8 @@ static const MemMapEntry base_memmap[] = { [VIRT_NVDIMM_ACPI] = { 0x09090000, NVDIMM_ACPI_IO_LEN}, [VIRT_PVTIME] = { 0x090a0000, 0x00010000 }, [VIRT_SECURE_GPIO] = { 0x090b0000, 0x00001000 }, - [VIRT_ACPI_PCIHP] = { 0x090c0000, ACPI_PCIHP_SIZE }, + [VIRT_I2C] = { 0x090c0000, 0x00001000 }, + [VIRT_ACPI_PCIHP] = { 0x090d0000, ACPI_PCIHP_SIZE }, [VIRT_MMIO] = { 0x0a000000, 0x00000200 }, /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */ [VIRT_PLATFORM_BUS] = { 0x0c000000, 0x02000000 }, @@ -245,6 +248,7 @@ static const int a15irqmap[] = { [VIRT_GPIO] = 7, [VIRT_UART1] = 8, [VIRT_ACPI_GED] = 9, + [VIRT_I2C] = 10, [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */ [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */ [VIRT_SMMU] = 74, /* ...to 74 + NUM_SMMU_IRQS - 1 */ @@ -1016,6 +1020,36 @@ static void create_uart(const VirtMachineState *vms, int uart, g_free(nodename); } +static void create_i2c(const VirtMachineState *vms, int i2c) +{ + char *nodename = NULL; + hwaddr base = vms->memmap[i2c].base; + hwaddr size = vms->memmap[i2c].size; + int irq = vms->irqmap[i2c]; + MachineState *ms = MACHINE(vms); + DeviceState *dev = sysbus_create_simple(TYPE_DW_I2C, base, + qdev_get_gpio_in(vms->gic, irq)); + DWI2CState *s = DW_I2C(dev); + + nodename = g_strdup_printf("/dw-i2c@%" PRIx64, base); + qemu_fdt_add_subnode(ms->fdt, nodename); + qemu_fdt_setprop_string(ms->fdt, nodename, "compatible", + "snps,designware-i2c"); + qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg", + 2, base, 2, size); + qemu_fdt_setprop_cells(ms->fdt, nodename, "interrupts", + GIC_FDT_IRQ_TYPE_SPI, irq, + GIC_FDT_IRQ_FLAGS_LEVEL_HI); + + if (s && s->bus) { + at24c_eeprom_init(s->bus, 0x50, 256); + } else { + fprintf(stderr, "Warning: DW I2C created but bus not available\n"); + } + + g_free(nodename); +} + static void create_rtc(const VirtMachineState *vms) { char *nodename; @@ -2493,6 +2527,8 @@ static void machvirt_init(MachineState *machine) create_uart(vms, VIRT_UART1, secure_sysmem, serial_hd(1), true); } + create_i2c(vms, VIRT_I2C); + if (vms->secure) { create_secure_ram(vms, secure_sysmem, secure_tag_sysmem); } diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h index 8694aaa4e2..911beea7fd 100644 --- a/include/hw/arm/virt.h +++ b/include/hw/arm/virt.h @@ -75,6 +75,7 @@ enum { VIRT_PLATFORM_BUS, VIRT_GPIO, VIRT_UART1, + VIRT_I2C, VIRT_SECURE_MEM, VIRT_SECURE_GPIO, VIRT_PCDIMM_ACPI, -- 2.43.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] hw/arm/virt: Add DesignWare I2C controller 2026-01-06 13:12 ` [PATCH 2/2] hw/arm/virt: Add DesignWare I2C controller AlanoSong @ 2026-01-06 15:45 ` Jonathan Cameron via 0 siblings, 0 replies; 11+ messages in thread From: Jonathan Cameron via @ 2026-01-06 15:45 UTC (permalink / raw) To: AlanoSong Cc: qemu-devel, qemu-arm, cminyard, peter.maydell, philmd, ani, pbonzini, shannon.zhaosl On Tue, 6 Jan 2026 21:12:53 +0800 AlanoSong@163.com wrote: > From: Alano Song <AlanoSong@163.com> > > Add DesignWare I2C controller onto virt board, > and also an at24c eeprom for r/w operation. > > Add these two devices into arm virt acpi table. > > Confirmed with i2c-tools under v6.18 linux driver. > Hi Alano, > Signed-off-by: Alano Song <AlanoSong@163.com> Perhaps a silly question but why do you want this on arm/virt? I've been carrying a backed up version of the aspeed i2c but for that we are using it with MCTP (I'm guessing this one isn't capable enough) and devices on that are inherently discoverable unlike normal I2C devices. Even so I don't plan to upstream that as for the CXL fabric stuff I can use MCTP over USB instead and don't need to change arm/virt at all. I'm not sure how useful an eeprom is beyond verifying your control emulation, but perhaps that's all that is intended? > --- > hw/arm/Kconfig | 1 + > hw/arm/virt-acpi-build.c | 32 ++++++++++++++++++++++++++++++++ > hw/arm/virt.c | 38 +++++++++++++++++++++++++++++++++++++- > include/hw/arm/virt.h | 1 + > 4 files changed, 71 insertions(+), 1 deletion(-) > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index 03b4342574..3d06356169 100644 > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -100,6 +100,34 @@ static void acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap, > aml_append(scope, dev); > } > An example of the AML blob would be useful in the patch description (or the one updating the ACPI test tables). I'd also expect a lot of bios test failures given you didn't change the data files. Run make check-qtest and see what blow up. > +static void acpi_dsdt_add_i2c(Aml *scope, const MemMapEntry *i2c_memmap, > + uint32_t i2c_irq) > +{ > + Aml *i2c_dev, *eprm_dev, *crs; > + > + i2c_dev = aml_device("I2C0"); > + aml_append(i2c_dev, aml_name_decl("_HID", aml_string("INT3433"))); That seems to be a valid intel PNP ID, but please add a reference to where it came from (I'll guess the kernel driver rather than some document?) > + aml_append(i2c_dev, aml_name_decl("_UID", aml_int(0))); > + > + crs = aml_resource_template(); > + aml_append(crs, aml_memory32_fixed(i2c_memmap->base, > + i2c_memmap->size, AML_READ_WRITE)); > + aml_append(crs, aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH, > + AML_EXCLUSIVE, &i2c_irq, 1)); > + aml_append(i2c_dev, aml_name_decl("_CRS", crs)); > + > + eprm_dev = aml_device("EPRM"); > + aml_append(eprm_dev, aml_name_decl("_HID", aml_string("INT3499"))); Likewise, a reference for this PNP format ACPI ID would be good. > + aml_append(eprm_dev, aml_name_decl("_UID", aml_int(0))); > + > + crs = aml_resource_template(); > + aml_append(crs, aml_i2c_serial_bus_device(0x50, "^")); This is the bit that made me ask for a blob in the patch description. I have very little idea what that actually does in AML :( (the "^" in particular) > + aml_append(eprm_dev, aml_name_decl("_CRS", crs)); > + > + aml_append(i2c_dev, eprm_dev); > + aml_append(scope, i2c_dev); > +} > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index fd0e28f030..8fd37126d1 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -38,6 +38,8 @@ > #include "hw/arm/boot.h" > #include "hw/arm/primecell.h" > #include "hw/arm/virt.h" > +#include "hw/i2c/dw_i2c.h" > +#include "hw/nvram/eeprom_at24c.h" > #include "hw/arm/machines-qom.h" > #include "hw/block/flash.h" > #include "hw/display/ramfb.h" > @@ -193,7 +195,8 @@ static const MemMapEntry base_memmap[] = { > [VIRT_NVDIMM_ACPI] = { 0x09090000, NVDIMM_ACPI_IO_LEN}, > [VIRT_PVTIME] = { 0x090a0000, 0x00010000 }, > [VIRT_SECURE_GPIO] = { 0x090b0000, 0x00001000 }, > - [VIRT_ACPI_PCIHP] = { 0x090c0000, ACPI_PCIHP_SIZE }, > + [VIRT_I2C] = { 0x090c0000, 0x00001000 }, > + [VIRT_ACPI_PCIHP] = { 0x090d0000, ACPI_PCIHP_SIZE }, This breaks VM migration. You need to put your new memory in a hole in the memory map, not move things. > [VIRT_MMIO] = { 0x0a000000, 0x00000200 }, > /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */ > [VIRT_PLATFORM_BUS] = { 0x0c000000, 0x02000000 }, > @@ -245,6 +248,7 @@ static const int a15irqmap[] = { > [VIRT_GPIO] = 7, > [VIRT_UART1] = 8, > [VIRT_ACPI_GED] = 9, > + [VIRT_I2C] = 10, > [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */ > [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */ > [VIRT_SMMU] = 74, /* ...to 74 + NUM_SMMU_IRQS - 1 */ > @@ -1016,6 +1020,36 @@ static void create_uart(const VirtMachineState *vms, int uart, > g_free(nodename); > } > > +static void create_i2c(const VirtMachineState *vms, int i2c) > +{ > + char *nodename = NULL; Always overridden so don't set initial value. Maybe use a g_autofree to avoid having to tidy it up on exit from the function. > + hwaddr base = vms->memmap[i2c].base; > + hwaddr size = vms->memmap[i2c].size; > + int irq = vms->irqmap[i2c]; > + MachineState *ms = MACHINE(vms); > + DeviceState *dev = sysbus_create_simple(TYPE_DW_I2C, base, > + qdev_get_gpio_in(vms->gic, irq)); > + DWI2CState *s = DW_I2C(dev); > + > + nodename = g_strdup_printf("/dw-i2c@%" PRIx64, base); > + qemu_fdt_add_subnode(ms->fdt, nodename); > + qemu_fdt_setprop_string(ms->fdt, nodename, "compatible", > + "snps,designware-i2c"); > + qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg", > + 2, base, 2, size); > + qemu_fdt_setprop_cells(ms->fdt, nodename, "interrupts", > + GIC_FDT_IRQ_TYPE_SPI, irq, > + GIC_FDT_IRQ_FLAGS_LEVEL_HI); > + > + if (s && s->bus) { > + at24c_eeprom_init(s->bus, 0x50, 256); > + } else { > + fprintf(stderr, "Warning: DW I2C created but bus not available\n"); > + } > + > + g_free(nodename); > +} ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] hw/arm/virt: Add DesignWare I2C controller @ 2026-01-06 15:45 ` Jonathan Cameron via 0 siblings, 0 replies; 11+ messages in thread From: Jonathan Cameron via @ 2026-01-06 15:45 UTC (permalink / raw) To: AlanoSong Cc: qemu-devel, qemu-arm, cminyard, peter.maydell, philmd, ani, pbonzini, shannon.zhaosl On Tue, 6 Jan 2026 21:12:53 +0800 AlanoSong@163.com wrote: > From: Alano Song <AlanoSong@163.com> > > Add DesignWare I2C controller onto virt board, > and also an at24c eeprom for r/w operation. > > Add these two devices into arm virt acpi table. > > Confirmed with i2c-tools under v6.18 linux driver. > Hi Alano, > Signed-off-by: Alano Song <AlanoSong@163.com> Perhaps a silly question but why do you want this on arm/virt? I've been carrying a backed up version of the aspeed i2c but for that we are using it with MCTP (I'm guessing this one isn't capable enough) and devices on that are inherently discoverable unlike normal I2C devices. Even so I don't plan to upstream that as for the CXL fabric stuff I can use MCTP over USB instead and don't need to change arm/virt at all. I'm not sure how useful an eeprom is beyond verifying your control emulation, but perhaps that's all that is intended? > --- > hw/arm/Kconfig | 1 + > hw/arm/virt-acpi-build.c | 32 ++++++++++++++++++++++++++++++++ > hw/arm/virt.c | 38 +++++++++++++++++++++++++++++++++++++- > include/hw/arm/virt.h | 1 + > 4 files changed, 71 insertions(+), 1 deletion(-) > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > index 03b4342574..3d06356169 100644 > --- a/hw/arm/virt-acpi-build.c > +++ b/hw/arm/virt-acpi-build.c > @@ -100,6 +100,34 @@ static void acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap, > aml_append(scope, dev); > } > An example of the AML blob would be useful in the patch description (or the one updating the ACPI test tables). I'd also expect a lot of bios test failures given you didn't change the data files. Run make check-qtest and see what blow up. > +static void acpi_dsdt_add_i2c(Aml *scope, const MemMapEntry *i2c_memmap, > + uint32_t i2c_irq) > +{ > + Aml *i2c_dev, *eprm_dev, *crs; > + > + i2c_dev = aml_device("I2C0"); > + aml_append(i2c_dev, aml_name_decl("_HID", aml_string("INT3433"))); That seems to be a valid intel PNP ID, but please add a reference to where it came from (I'll guess the kernel driver rather than some document?) > + aml_append(i2c_dev, aml_name_decl("_UID", aml_int(0))); > + > + crs = aml_resource_template(); > + aml_append(crs, aml_memory32_fixed(i2c_memmap->base, > + i2c_memmap->size, AML_READ_WRITE)); > + aml_append(crs, aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH, > + AML_EXCLUSIVE, &i2c_irq, 1)); > + aml_append(i2c_dev, aml_name_decl("_CRS", crs)); > + > + eprm_dev = aml_device("EPRM"); > + aml_append(eprm_dev, aml_name_decl("_HID", aml_string("INT3499"))); Likewise, a reference for this PNP format ACPI ID would be good. > + aml_append(eprm_dev, aml_name_decl("_UID", aml_int(0))); > + > + crs = aml_resource_template(); > + aml_append(crs, aml_i2c_serial_bus_device(0x50, "^")); This is the bit that made me ask for a blob in the patch description. I have very little idea what that actually does in AML :( (the "^" in particular) > + aml_append(eprm_dev, aml_name_decl("_CRS", crs)); > + > + aml_append(i2c_dev, eprm_dev); > + aml_append(scope, i2c_dev); > +} > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > index fd0e28f030..8fd37126d1 100644 > --- a/hw/arm/virt.c > +++ b/hw/arm/virt.c > @@ -38,6 +38,8 @@ > #include "hw/arm/boot.h" > #include "hw/arm/primecell.h" > #include "hw/arm/virt.h" > +#include "hw/i2c/dw_i2c.h" > +#include "hw/nvram/eeprom_at24c.h" > #include "hw/arm/machines-qom.h" > #include "hw/block/flash.h" > #include "hw/display/ramfb.h" > @@ -193,7 +195,8 @@ static const MemMapEntry base_memmap[] = { > [VIRT_NVDIMM_ACPI] = { 0x09090000, NVDIMM_ACPI_IO_LEN}, > [VIRT_PVTIME] = { 0x090a0000, 0x00010000 }, > [VIRT_SECURE_GPIO] = { 0x090b0000, 0x00001000 }, > - [VIRT_ACPI_PCIHP] = { 0x090c0000, ACPI_PCIHP_SIZE }, > + [VIRT_I2C] = { 0x090c0000, 0x00001000 }, > + [VIRT_ACPI_PCIHP] = { 0x090d0000, ACPI_PCIHP_SIZE }, This breaks VM migration. You need to put your new memory in a hole in the memory map, not move things. > [VIRT_MMIO] = { 0x0a000000, 0x00000200 }, > /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */ > [VIRT_PLATFORM_BUS] = { 0x0c000000, 0x02000000 }, > @@ -245,6 +248,7 @@ static const int a15irqmap[] = { > [VIRT_GPIO] = 7, > [VIRT_UART1] = 8, > [VIRT_ACPI_GED] = 9, > + [VIRT_I2C] = 10, > [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */ > [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */ > [VIRT_SMMU] = 74, /* ...to 74 + NUM_SMMU_IRQS - 1 */ > @@ -1016,6 +1020,36 @@ static void create_uart(const VirtMachineState *vms, int uart, > g_free(nodename); > } > > +static void create_i2c(const VirtMachineState *vms, int i2c) > +{ > + char *nodename = NULL; Always overridden so don't set initial value. Maybe use a g_autofree to avoid having to tidy it up on exit from the function. > + hwaddr base = vms->memmap[i2c].base; > + hwaddr size = vms->memmap[i2c].size; > + int irq = vms->irqmap[i2c]; > + MachineState *ms = MACHINE(vms); > + DeviceState *dev = sysbus_create_simple(TYPE_DW_I2C, base, > + qdev_get_gpio_in(vms->gic, irq)); > + DWI2CState *s = DW_I2C(dev); > + > + nodename = g_strdup_printf("/dw-i2c@%" PRIx64, base); > + qemu_fdt_add_subnode(ms->fdt, nodename); > + qemu_fdt_setprop_string(ms->fdt, nodename, "compatible", > + "snps,designware-i2c"); > + qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg", > + 2, base, 2, size); > + qemu_fdt_setprop_cells(ms->fdt, nodename, "interrupts", > + GIC_FDT_IRQ_TYPE_SPI, irq, > + GIC_FDT_IRQ_FLAGS_LEVEL_HI); > + > + if (s && s->bus) { > + at24c_eeprom_init(s->bus, 0x50, 256); > + } else { > + fprintf(stderr, "Warning: DW I2C created but bus not available\n"); > + } > + > + g_free(nodename); > +} ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re:Re: [PATCH 2/2] hw/arm/virt: Add DesignWare I2C controller 2026-01-06 15:45 ` Jonathan Cameron via (?) @ 2026-01-07 7:07 ` Alano Song 2026-01-07 9:47 ` Jonathan Cameron via -1 siblings, 1 reply; 11+ messages in thread From: Alano Song @ 2026-01-07 7:07 UTC (permalink / raw) To: Jonathan Cameron Cc: qemu-devel, qemu-arm, cminyard, peter.maydell, philmd, ani, pbonzini, shannon.zhaosl [-- Attachment #1: Type: text/plain, Size: 7517 bytes --] At 2026-01-06 23:45:22, "Jonathan Cameron via" <qemu-devel@nongnu.org> wrote: >On Tue, 6 Jan 2026 21:12:53 +0800 >AlanoSong@163.com wrote: > >> From: Alano Song <AlanoSong@163.com> >> >> Add DesignWare I2C controller onto virt board, >> and also an at24c eeprom for r/w operation. >> >> Add these two devices into arm virt acpi table. >> >> Confirmed with i2c-tools under v6.18 linux driver. >> >Hi Alano, > >> Signed-off-by: Alano Song <AlanoSong@163.com> > >Perhaps a silly question but why do you want this on arm/virt? > >I've been carrying a backed up version of the aspeed i2c but for >that we are using it with MCTP (I'm guessing this one isn't capable >enough) and devices on that are inherently discoverable unlike >normal I2C devices. Even so I don't plan to upstream that as for >the CXL fabric stuff I can use MCTP over USB instead and don't >need to change arm/virt at all. > Cause we are emulating our soc chip on qemu, and our first choice of machine board is arm/virt. But we unfortunately found there is no DesignWare I2C model and no I2C device in arm/virt. Someone else may encounter this problem as well, so I decide to solve it. >I'm not sure how useful an eeprom is beyond verifying your control emulation, >but perhaps that's all that is intended? > > Yes you are right, single I2C controller cannot be verified, so I add an eeprom to work with it. >> --- >> hw/arm/Kconfig | 1 + >> hw/arm/virt-acpi-build.c | 32 ++++++++++++++++++++++++++++++++ >> hw/arm/virt.c | 38 +++++++++++++++++++++++++++++++++++++- >> include/hw/arm/virt.h | 1 + >> 4 files changed, 71 insertions(+), 1 deletion(-) > >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c >> index 03b4342574..3d06356169 100644 >> --- a/hw/arm/virt-acpi-build.c >> +++ b/hw/arm/virt-acpi-build.c >> @@ -100,6 +100,34 @@ static void acpi_dsdt_add_uart(Aml *scope, const MemMapEntry *uart_memmap, >> aml_append(scope, dev); >> } >> > >An example of the AML blob would be useful in the patch description (or >the one updating the ACPI test tables). >I'd also expect a lot of bios test failures given you didn't change the >data files. Run make check-qtest and see what blow up. > Your suggestion is quiet right, I will add comment of ACPI table in patch v2 description. And I make check-qtest, you are right, I will fix the test failure leaded by this patch in patch v2. >> +static void acpi_dsdt_add_i2c(Aml *scope, const MemMapEntry *i2c_memmap, >> + uint32_t i2c_irq) >> +{ >> + Aml *i2c_dev, *eprm_dev, *crs; >> + >> + i2c_dev = aml_device("I2C0"); >> + aml_append(i2c_dev, aml_name_decl("_HID", aml_string("INT3433"))); > >That seems to be a valid intel PNP ID, but please add a reference to where it >came from (I'll guess the kernel driver rather than some document?) > Yes, you are right, this PNP ID comes from kernel driver(i2c-designware-platdrv.c/dw_i2c_acpi_match) it's just for testing convenience under linux kernel. I will add comment for it. >> + aml_append(i2c_dev, aml_name_decl("_UID", aml_int(0))); >> + >> + crs = aml_resource_template(); >> + aml_append(crs, aml_memory32_fixed(i2c_memmap->base, >> + i2c_memmap->size, AML_READ_WRITE)); >> + aml_append(crs, aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH, >> + AML_EXCLUSIVE, &i2c_irq, 1)); >> + aml_append(i2c_dev, aml_name_decl("_CRS", crs)); >> + >> + eprm_dev = aml_device("EPRM"); >> + aml_append(eprm_dev, aml_name_decl("_HID", aml_string("INT3499"))); > >Likewise, a reference for this PNP format ACPI ID would be good. > Okay, I will add comment for it :) >> + aml_append(eprm_dev, aml_name_decl("_UID", aml_int(0))); >> + >> + crs = aml_resource_template(); >> + aml_append(crs, aml_i2c_serial_bus_device(0x50, "^")); > >This is the bit that made me ask for a blob in the patch description. >I have very little idea what that actually does in AML :( (the "^" >in particular) > Here, "^" represents the father device of eeprom ("EPRM"), just the I2C controller device ("I2C0"). I will add description for it in patch v2. >> + aml_append(eprm_dev, aml_name_decl("_CRS", crs)); >> + >> + aml_append(i2c_dev, eprm_dev); >> + aml_append(scope, i2c_dev); >> +} > >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c >> index fd0e28f030..8fd37126d1 100644 >> --- a/hw/arm/virt.c >> +++ b/hw/arm/virt.c >> @@ -38,6 +38,8 @@ >> #include "hw/arm/boot.h" >> #include "hw/arm/primecell.h" >> #include "hw/arm/virt.h" >> +#include "hw/i2c/dw_i2c.h" >> +#include "hw/nvram/eeprom_at24c.h" >> #include "hw/arm/machines-qom.h" >> #include "hw/block/flash.h" >> #include "hw/display/ramfb.h" >> @@ -193,7 +195,8 @@ static const MemMapEntry base_memmap[] = { >> [VIRT_NVDIMM_ACPI] = { 0x09090000, NVDIMM_ACPI_IO_LEN}, >> [VIRT_PVTIME] = { 0x090a0000, 0x00010000 }, >> [VIRT_SECURE_GPIO] = { 0x090b0000, 0x00001000 }, >> - [VIRT_ACPI_PCIHP] = { 0x090c0000, ACPI_PCIHP_SIZE }, >> + [VIRT_I2C] = { 0x090c0000, 0x00001000 }, >> + [VIRT_ACPI_PCIHP] = { 0x090d0000, ACPI_PCIHP_SIZE }, > >This breaks VM migration. You need to put your new memory in a hole >in the memory map, not move things. > Okay, I will refine this part code :) >> [VIRT_MMIO] = { 0x0a000000, 0x00000200 }, >> /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */ >> [VIRT_PLATFORM_BUS] = { 0x0c000000, 0x02000000 }, >> @@ -245,6 +248,7 @@ static const int a15irqmap[] = { >> [VIRT_GPIO] = 7, >> [VIRT_UART1] = 8, >> [VIRT_ACPI_GED] = 9, >> + [VIRT_I2C] = 10, >> [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */ >> [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */ >> [VIRT_SMMU] = 74, /* ...to 74 + NUM_SMMU_IRQS - 1 */ >> @@ -1016,6 +1020,36 @@ static void create_uart(const VirtMachineState *vms, int uart, >> g_free(nodename); >> } >> >> +static void create_i2c(const VirtMachineState *vms, int i2c) >> +{ >> + char *nodename = NULL; > >Always overridden so don't set initial value. Maybe use a g_autofree >to avoid having to tidy it up on exit from the function. > Thanks for your advice, I will refine this part code :) >> + hwaddr base = vms->memmap[i2c].base; >> + hwaddr size = vms->memmap[i2c].size; >> + int irq = vms->irqmap[i2c]; >> + MachineState *ms = MACHINE(vms); >> + DeviceState *dev = sysbus_create_simple(TYPE_DW_I2C, base, >> + qdev_get_gpio_in(vms->gic, irq)); >> + DWI2CState *s = DW_I2C(dev); >> + >> + nodename = g_strdup_printf("/dw-i2c@%" PRIx64, base); >> + qemu_fdt_add_subnode(ms->fdt, nodename); >> + qemu_fdt_setprop_string(ms->fdt, nodename, "compatible", >> + "snps,designware-i2c"); >> + qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg", >> + 2, base, 2, size); >> + qemu_fdt_setprop_cells(ms->fdt, nodename, "interrupts", >> + GIC_FDT_IRQ_TYPE_SPI, irq, >> + GIC_FDT_IRQ_FLAGS_LEVEL_HI); >> + >> + if (s && s->bus) { >> + at24c_eeprom_init(s->bus, 0x50, 256); >> + } else { >> + fprintf(stderr, "Warning: DW I2C created but bus not available\n"); >> + } >> + >> + g_free(nodename); >> +} > [-- Attachment #2: Type: text/html, Size: 9132 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] hw/arm/virt: Add DesignWare I2C controller 2026-01-07 7:07 ` Alano Song @ 2026-01-07 9:47 ` Jonathan Cameron via 0 siblings, 0 replies; 11+ messages in thread From: Jonathan Cameron via @ 2026-01-07 9:47 UTC (permalink / raw) To: Alano Song Cc: qemu-devel, qemu-arm, cminyard, peter.maydell, philmd, ani, pbonzini, shannon.zhaosl On Wed, 7 Jan 2026 15:07:10 +0800 (CST) "Alano Song" <alanosong@163.com> wrote: > At 2026-01-06 23:45:22, "Jonathan Cameron via" <qemu-devel@nongnu.org> wrote: > >On Tue, 6 Jan 2026 21:12:53 +0800 > >AlanoSong@163.com wrote: > > > >> From: Alano Song <AlanoSong@163.com> > >> > >> Add DesignWare I2C controller onto virt board, > >> and also an at24c eeprom for r/w operation. > >> > >> Add these two devices into arm virt acpi table. > >> > >> Confirmed with i2c-tools under v6.18 linux driver. > >> > >Hi Alano, > > > >> Signed-off-by: Alano Song <AlanoSong@163.com> > > > >Perhaps a silly question but why do you want this on arm/virt? > > > >I've been carrying a backed up version of the aspeed i2c but for > >that we are using it with MCTP (I'm guessing this one isn't capable > >enough) and devices on that are inherently discoverable unlike > >normal I2C devices. Even so I don't plan to upstream that as for > >the CXL fabric stuff I can use MCTP over USB instead and don't > >need to change arm/virt at all. > > > > > Cause we are emulating our soc chip on qemu, and our first choice of > machine board is arm/virt. Are you planning to ultimately upstream support for emulating your SoC? If you do, is a new emulated board perhaps appropriate? > But we unfortunately found there is no DesignWare I2C model and no > I2C device in arm/virt. > > Someone else may encounter this problem as well, so I decide to solve it. It's definitely good to have emulation of this fairly common IP. > > > >I'm not sure how useful an eeprom is beyond verifying your control emulation, > >but perhaps that's all that is intended? > > > > > > > > Yes you are right, single I2C controller cannot be verified, so I > add an eeprom to work with it. Within arm virt one concern is that this unconditionally enabled so attacks on emulation code are a real possibility (in VM usecases) > > > >> --- > >> +static void acpi_dsdt_add_i2c(Aml *scope, const MemMapEntry *i2c_memmap, > >> + uint32_t i2c_irq) > >> +{ > >> + Aml *i2c_dev, *eprm_dev, *crs; > >> + > >> + i2c_dev = aml_device("I2C0"); > >> + aml_append(i2c_dev, aml_name_decl("_HID", aml_string("INT3433"))); > > > >That seems to be a valid intel PNP ID, but please add a reference to where it > >came from (I'll guess the kernel driver rather than some document?) > > > > > > Yes, you are right, this PNP ID comes from kernel > driver(i2c-designware-platdrv.c/dw_i2c_acpi_match) > it's just for testing convenience under linux kernel. If you are ultimately emulating a real SoC I'd suggest using an ID issued by they vendor of the board or the SoC. They will need to have a suitable ACPI or PNP ID though (and so be a member of UEFI.org) That will allow for any quirks in the kernel driver (and you'd need to emulate them as well). Who knows what slight differences there might be with the Intel version of the IP. > > > >> + aml_append(eprm_dev, aml_name_decl("_UID", aml_int(0))); > >> + > >> + crs = aml_resource_template(); > >> + aml_append(crs, aml_i2c_serial_bus_device(0x50, "^")); > > > >This is the bit that made me ask for a blob in the patch description. > >I have very little idea what that actually does in AML :( (the "^" > >in particular) > > > > > > Here, "^" represents the father device of eeprom ("EPRM"), > just the I2C controller device ("I2C0"). > I will add description for it in patch v2. Ah. Ok. I didn't know about that and couldn't find it in the ACPI spec :( A more specific reference like you suggest is fine. Jonathan ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] hw/arm/virt: Add DesignWare I2C controller @ 2026-01-07 9:47 ` Jonathan Cameron via 0 siblings, 0 replies; 11+ messages in thread From: Jonathan Cameron via @ 2026-01-07 9:47 UTC (permalink / raw) To: Alano Song Cc: qemu-devel, qemu-arm, cminyard, peter.maydell, philmd, ani, pbonzini, shannon.zhaosl On Wed, 7 Jan 2026 15:07:10 +0800 (CST) "Alano Song" <alanosong@163.com> wrote: > At 2026-01-06 23:45:22, "Jonathan Cameron via" <qemu-devel@nongnu.org> wrote: > >On Tue, 6 Jan 2026 21:12:53 +0800 > >AlanoSong@163.com wrote: > > > >> From: Alano Song <AlanoSong@163.com> > >> > >> Add DesignWare I2C controller onto virt board, > >> and also an at24c eeprom for r/w operation. > >> > >> Add these two devices into arm virt acpi table. > >> > >> Confirmed with i2c-tools under v6.18 linux driver. > >> > >Hi Alano, > > > >> Signed-off-by: Alano Song <AlanoSong@163.com> > > > >Perhaps a silly question but why do you want this on arm/virt? > > > >I've been carrying a backed up version of the aspeed i2c but for > >that we are using it with MCTP (I'm guessing this one isn't capable > >enough) and devices on that are inherently discoverable unlike > >normal I2C devices. Even so I don't plan to upstream that as for > >the CXL fabric stuff I can use MCTP over USB instead and don't > >need to change arm/virt at all. > > > > > Cause we are emulating our soc chip on qemu, and our first choice of > machine board is arm/virt. Are you planning to ultimately upstream support for emulating your SoC? If you do, is a new emulated board perhaps appropriate? > But we unfortunately found there is no DesignWare I2C model and no > I2C device in arm/virt. > > Someone else may encounter this problem as well, so I decide to solve it. It's definitely good to have emulation of this fairly common IP. > > > >I'm not sure how useful an eeprom is beyond verifying your control emulation, > >but perhaps that's all that is intended? > > > > > > > > Yes you are right, single I2C controller cannot be verified, so I > add an eeprom to work with it. Within arm virt one concern is that this unconditionally enabled so attacks on emulation code are a real possibility (in VM usecases) > > > >> --- > >> +static void acpi_dsdt_add_i2c(Aml *scope, const MemMapEntry *i2c_memmap, > >> + uint32_t i2c_irq) > >> +{ > >> + Aml *i2c_dev, *eprm_dev, *crs; > >> + > >> + i2c_dev = aml_device("I2C0"); > >> + aml_append(i2c_dev, aml_name_decl("_HID", aml_string("INT3433"))); > > > >That seems to be a valid intel PNP ID, but please add a reference to where it > >came from (I'll guess the kernel driver rather than some document?) > > > > > > Yes, you are right, this PNP ID comes from kernel > driver(i2c-designware-platdrv.c/dw_i2c_acpi_match) > it's just for testing convenience under linux kernel. If you are ultimately emulating a real SoC I'd suggest using an ID issued by they vendor of the board or the SoC. They will need to have a suitable ACPI or PNP ID though (and so be a member of UEFI.org) That will allow for any quirks in the kernel driver (and you'd need to emulate them as well). Who knows what slight differences there might be with the Intel version of the IP. > > > >> + aml_append(eprm_dev, aml_name_decl("_UID", aml_int(0))); > >> + > >> + crs = aml_resource_template(); > >> + aml_append(crs, aml_i2c_serial_bus_device(0x50, "^")); > > > >This is the bit that made me ask for a blob in the patch description. > >I have very little idea what that actually does in AML :( (the "^" > >in particular) > > > > > > Here, "^" represents the father device of eeprom ("EPRM"), > just the I2C controller device ("I2C0"). > I will add description for it in patch v2. Ah. Ok. I didn't know about that and couldn't find it in the ACPI spec :( A more specific reference like you suggest is fine. Jonathan ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2026-01-07 9:48 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-01-06 13:12 [PATCH 0/2] hw/i2c/dw: Add DesignWare I2C controller emulator AlanoSong 2026-01-06 13:12 ` [PATCH 1/2] " AlanoSong 2026-01-06 15:52 ` Jonathan Cameron via 2026-01-06 15:52 ` Jonathan Cameron via 2026-01-07 1:50 ` Alano Song 2026-01-06 13:12 ` [PATCH 2/2] hw/arm/virt: Add DesignWare I2C controller AlanoSong 2026-01-06 15:45 ` Jonathan Cameron via 2026-01-06 15:45 ` Jonathan Cameron via 2026-01-07 7:07 ` Alano Song 2026-01-07 9:47 ` Jonathan Cameron via 2026-01-07 9:47 ` Jonathan Cameron via
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.