On 1/2/25 16:28, Bernhard Beschow wrote:


Am 30. Januar 2025 23:05:53 UTC schrieb "Philippe Mathieu-Daudé" 
<phi...@linaro.org>:
Cc'ing AMD folks

Hi Bernhard,

TL;DR; can't you use the PCF8574 which is a more complete model of I/O
expander? (See hw/gpio/pcf8574.c)

If it is software-compatible then I could use it. I'm modeling a real board 
whose device tree I'd like to use unchanged, so I don't have much choice. The 
only reason I need it is because its absence clogs the i2c bus.

No clue about compatibility. If you unfortunately need to add it,
then please address my comments in the next version.


Best regards,
Bernhard


On 20/1/25 21:37, Bernhard Beschow wrote:
Xilinx QEMU implements a TCA6416 device model which may be useful for the
broader QEMU community, so upstream it. In the Xilinx fork, the device model
gets compiled whenever CONFIG_CADENCE is true, so have it maintained by the
"hw/*/cadence_*" maintainers.

The code is based on Xilinx QEMU version xilinx_v2024.2 plus the following
modifications:
* Use OBJECT_DECLARE_SIMPLE_TYPE()
* Port from DPRINTF() to trace events
* Follow QEMU conventions for naming of state struct
* Rename type to not contain a ','
* Use DEFINE_TYPES() macro
* Implement under hw/gpio since device is i2c client and gpio provider
* Have dedicated Kconfig switch

Signed-off-by: Bernhard Beschow <shen...@gmail.com>

--
I have a use for TCA6416 emulation. Question: Are Xilinx Zynq maintainers
willing to maintain this device model?
---
   MAINTAINERS          |   1 +
   hw/gpio/tca6416.c    | 122 +++++++++++++++++++++++++++++++++++++++++++
   hw/gpio/Kconfig      |   5 ++
   hw/gpio/meson.build  |   1 +
   hw/gpio/trace-events |   4 ++
   5 files changed, 133 insertions(+)
   create mode 100644 hw/gpio/tca6416.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7531d65429..0cac9c90bc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1030,6 +1030,7 @@ S: Maintained
   F: hw/*/xilinx_*
   F: hw/*/cadence_*
   F: hw/misc/zynq_slcr.c
+F: hw/gpio/tca6416.c
   F: hw/adc/zynq-xadc.c
   F: include/hw/misc/zynq_slcr.h
   F: include/hw/adc/zynq-xadc.h
diff --git a/hw/gpio/tca6416.c b/hw/gpio/tca6416.c
new file mode 100644
index 0000000000..81ed7a654d
--- /dev/null
+++ b/hw/gpio/tca6416.c
@@ -0,0 +1,122 @@
+/*
+ * QEMU model of TCA6416 16-Bit I/O Expander

Please add datasheet reference.

+ *
+ * Copyright (c) 2018 Xilinx Inc.
+ *
+ * Written by Sai Pavan Boddu <sai.pavan.bo...@xilinx.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to 
deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING 
FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.

Sai/Edgar/Francisco, should we update to AMD here? Remove or update the
email address? Also, can you Ack the replacement of this license by a
SPDX tag?

+ */
+#include "qemu/osdep.h"
+#include "hw/i2c/i2c.h"
+#include "trace.h"
+
+#define TYPE_TCA6416 "tca6416"
+OBJECT_DECLARE_SIMPLE_TYPE(Tca6416State, TCA6416)
+
+#define IN_PORT0    0
+#define IN_PORT1    1
+#define OUT_PORT0   2
+#define OUT_PORT1   3
+#define POL_INV0    4
+#define POL_INV1    5
+#define CONF_PORT0  6
+#define CONF_PORT1  7

enum up to here?

+#define RMAX (CONF_PORT1 + 1)
+
+enum tca6416_events {
+     ADDR_DONE,
+     ADDRESSING,
+};
+
+struct Tca6416State {
+     I2CSlave i2c;
+
+     uint8_t addr;
+     uint8_t state;
+     uint8_t regs[RMAX];
+};
+
+static uint8_t tca6416_read(I2CSlave *i2c)
+{
+    Tca6416State *s = TCA6416(i2c);
+    uint8_t ret;
+
+    ret = s->regs[s->addr];
+    trace_tca6416_read(ret);
+    return ret;
+}
+
+static int tca6416_write(I2CSlave *i2c, uint8_t data)
+{
+    Tca6416State *s = TCA6416(i2c);
+
+    trace_tca6416_write(data);
+    if (s->state == ADDRESSING) {
+        s->addr = data;

I suppose HW masks here.

          s->addr = data & 0xf;

+    } else {
+        s->regs[s->addr] = data;

(otherwise this could overflow).

So this device isn't doing anything actually (I'd have
expected 1 IRQ and 16 GPIO lines). What is the point,
at least in this state?

+    }
+
+    return 0;
+}
+
+static void tca6416_realize(DeviceState *dev, Error **errp)
+{
+    Tca6416State *s = TCA6416(dev);
+

Missing:

       s->regs[OUT_PORT0] = 0xff;
       s->regs[OUT_PORT1] = 0xff;
       s->regs[POL_INV0] = 0x00;
       s->regs[POL_INV1] = 0x00;

+    s->regs[CONF_PORT0] = 0xFF;
+    s->regs[CONF_PORT1] = 0xFF;

Style is 0xff.

That said, this is not a REALIZE but RESET handler.

+}
+
+static int tca6416_event(I2CSlave *i2c, enum i2c_event event)
+{
+    Tca6416State *s = TCA6416(i2c);
+
+    switch (event) {
+    case I2C_START_SEND:
+        s->state = ADDRESSING;
+        break;
+    default:
+         s->state = ADDR_DONE;
+    };
+    return 0;
+}
+
+static void tca6416_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    I2CSlaveClass *k = I2C_SLAVE_CLASS(klass);
+
+    dc->realize = tca6416_realize;

(tca6416_realize is a reset handler).

Please complete:

       dc->desc = "TCA6416 I/O Expander";
       set_bit(DEVICE_CATEGORY_MISC, dc->categories);

No migration state?

+    k->recv = tca6416_read;
+    k->send = tca6416_write;
+    k->event = tca6416_event;
+}> +
+static const TypeInfo tca6416_types[] = {
+    {
+        .name = TYPE_TCA6416,
+        .parent = TYPE_I2C_SLAVE,
+        .class_init = tca6416_class_init,
+        .instance_size = sizeof(Tca6416State),
+    }
+};
+
+DEFINE_TYPES(tca6416_types)
diff --git a/hw/gpio/Kconfig b/hw/gpio/Kconfig
index c423e10f59..a240cf2de2 100644
--- a/hw/gpio/Kconfig
+++ b/hw/gpio/Kconfig
@@ -20,5 +20,10 @@ config PCF8574
       bool
       depends on I2C
   +config TCA6416
+    bool
+    depends on I2C
+    default y if I2C_DEVICES
+
   config ZAURUS_SCOOP
       bool
diff --git a/hw/gpio/meson.build b/hw/gpio/meson.build
index 74840619c0..b3ff7c7460 100644
--- a/hw/gpio/meson.build
+++ b/hw/gpio/meson.build
@@ -18,3 +18,4 @@ system_ss.add(when: 'CONFIG_STM32L4X5_SOC', if_true: 
files('stm32l4x5_gpio.c'))
   system_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files('aspeed_gpio.c'))
   system_ss.add(when: 'CONFIG_SIFIVE_GPIO', if_true: files('sifive_gpio.c'))
   system_ss.add(when: 'CONFIG_PCF8574', if_true: files('pcf8574.c'))
+system_ss.add(when: 'CONFIG_TCA6416', if_true: files('tca6416.c'))
diff --git a/hw/gpio/trace-events b/hw/gpio/trace-events
index cea896b28f..6724f2efb8 100644
--- a/hw/gpio/trace-events
+++ b/hw/gpio/trace-events
@@ -46,3 +46,7 @@ stm32l4x5_gpio_read(char *gpio, uint64_t addr) "GPIO%s addr: 0x%" 
PRIx64 " "
   stm32l4x5_gpio_write(char *gpio, uint64_t addr, uint64_t data) "GPIO%s addr: 0x%" PRIx64 " 
val: 0x%" PRIx64 ""
   stm32l4x5_gpio_update_idr(char *gpio, uint32_t old_idr, uint32_t new_idr) "GPIO%s 
from: 0x%x to: 0x%x"
   stm32l4x5_gpio_pins(char *gpio, uint16_t disconnected, uint16_t high) "GPIO%s 
disconnected pins: 0x%x levels: 0x%x"
+
+# tca6416.c
+tca6416_write(uint8_t value) "0x%02x"

"wr 0x%02x"

+tca6416_read(uint8_t value) "0x%02x"

"rd ..."

Regards,

Phil.


Reply via email to