On Apr 6 08:14, Cédric Le Goater wrote: > Hello Klaus, > > On 3/31/22 18:57, Klaus Jensen wrote: > > From: Klaus Jensen <k.jen...@samsung.com> > > > > Add slave mode functionality for the Aspeed I2C controller. This is > > implemented by creating an Aspeed I2C Slave device that attaches to the > > bus. > > > > This i2c slave device only implements the asynchronous version of > > i2c_send() and the event callback. > > > > Signed-off-by: Klaus Jensen <k.jen...@samsung.com> > > --- > > hw/i2c/aspeed_i2c.c | 95 +++++++++++++++++++++++++++++++++---- > > hw/i2c/trace-events | 2 +- > > hw/misc/meson.build | 2 + > > include/hw/i2c/aspeed_i2c.h | 8 ++++ > > 4 files changed, 97 insertions(+), 10 deletions(-) > > > > diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c > > index 03a4f5a91010..61b6424434f7 100644 > > --- a/hw/i2c/aspeed_i2c.c > > +++ b/hw/i2c/aspeed_i2c.c > > @@ -163,10 +163,15 @@ static inline void > > aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus) > > bus->intr_status & I2CD_INTR_TX_NAK ? "nak|" : "", > > bus->intr_status & I2CD_INTR_TX_ACK ? "ack|" : "", > > bus->intr_status & I2CD_INTR_RX_DONE ? "done|" : "", > > + bus->intr_status & I2CD_INTR_SLAVE_ADDR_RX_MATCH ? > > "slave-match|" : "", > > bus->intr_status & I2CD_INTR_NORMAL_STOP ? "normal|" : "", > > bus->intr_status & I2CD_INTR_ABNORMAL ? "abnormal" : ""); > > Troy introduced a similar change in his "new mode" proposal. I think > it is time to change the 'aspeed_i2c_bus_raise_interrupt' trace event > > Could you please update trace_aspeed_i2c_bus_raise_interrupt() to take > a single status string ? >
I'm not sure it will be "prettier". But I'll give it a shot. > > - bus->intr_status &= bus->intr_ctrl; > > + /* > > + * WORKAROUND: the Linux Aspeed I2C driver masks SLAVE_ADDR_RX_MATCH > > for > > + * some reason, not sure if it is a bug... > > + */ > > + bus->intr_status &= (bus->intr_ctrl | I2CD_INTR_SLAVE_ADDR_RX_MATCH); > > It comes from the initial support for the AST2400 SoC. > > We should introduce a 'intr_ctrl_mask' attribute in AspeedI2CClass and > fix the AST24000 value to 0x7FFF ... > I'm not sure I understand. Do you suggest that we always use a fixed mask here and disregard what the host sets in intr_ctrl? In any case, isn't it a bug in the Linux kernel driver that it neglects to set bit 7 (slave match) in the INTR_CTRL register? > > if (bus->intr_status) { > > bus->controller->intr_status |= 1 << bus->id; > > qemu_irq_raise(aic->bus_get_irq(bus)); > > @@ -196,6 +201,9 @@ static uint64_t aspeed_i2c_bus_read(void *opaque, > > hwaddr offset, > > case I2CD_INTR_STS_REG: > > value = bus->intr_status; > > break; > > + case I2CD_DEV_ADDR_REG: > > + value = bus->dev_addr; > > + break; > > You can introduce support for this register in a preliminary patch but > keep the slave activation for later (I2CD_SLAVE_EN bit) > Understood. > > case I2CD_POOL_CTRL_REG: > > value = bus->pool_ctrl; > > break; > > @@ -535,10 +543,9 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr > > offset, > > switch (offset) { > > case I2CD_FUN_CTRL_REG: > > if (value & I2CD_SLAVE_EN) { > > - qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n", > > - __func__); > > - break; > > + i2c_slave_set_address(&bus->slave->i2c, bus->dev_addr); > > } > > + > > bus->ctrl = value & 0x0071C3FF; > > break; > > case I2CD_AC_TIMING_REG1: > > @@ -558,14 +565,19 @@ static void aspeed_i2c_bus_write(void *opaque, hwaddr > > offset, > > bus->controller->intr_status &= ~(1 << bus->id); > > qemu_irq_lower(aic->bus_get_irq(bus)); > > } > > - if (handle_rx && (bus->cmd & (I2CD_M_RX_CMD | > > I2CD_M_S_RX_CMD_LAST))) { > > - aspeed_i2c_handle_rx_cmd(bus); > > - aspeed_i2c_bus_raise_interrupt(bus); > > + > > + if (handle_rx) { > > + if (bus->cmd & (I2CD_M_RX_CMD | I2CD_M_S_RX_CMD_LAST)) { > > + aspeed_i2c_handle_rx_cmd(bus); > > + aspeed_i2c_bus_raise_interrupt(bus); > > + } else if (aspeed_i2c_get_state(bus) == I2CD_STXD) { > > + i2c_ack(bus->bus); > > + } > > } > > + > > > > break; > > case I2CD_DEV_ADDR_REG: > > - qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n", > > - __func__); > > + bus->dev_addr = value; > > break; > > case I2CD_POOL_CTRL_REG: > > bus->pool_ctrl &= ~0xffffff; > > @@ -852,12 +864,74 @@ static const TypeInfo aspeed_i2c_info = { > > .abstract = true, > > }; > > +static int aspeed_i2c_slave_event(I2CSlave *slave, enum i2c_event event) > > +{ > > + AspeedI2CSlave *s = ASPEED_I2C_SLAVE(slave); > > + AspeedI2CBus *bus = s->bus; > > + > > + switch (event) { > > + case I2C_START_SEND: > > + bus->buf = bus->dev_addr << 1; > > + > > + bus->buf &= I2CD_BYTE_BUF_RX_MASK; > > + bus->buf <<= I2CD_BYTE_BUF_RX_SHIFT; > > + > > + bus->intr_status |= (I2CD_INTR_SLAVE_ADDR_RX_MATCH | > > I2CD_INTR_RX_DONE); > > + aspeed_i2c_set_state(bus, I2CD_STXD); > > + > > + break; > > + > > + case I2C_FINISH: > > + bus->intr_status |= I2CD_INTR_NORMAL_STOP; > > + aspeed_i2c_set_state(bus, I2CD_IDLE); > > + > > + break; > > + > > + default: > > + return -1; > > + } > > + > > + aspeed_i2c_bus_raise_interrupt(bus); > > + > > + return 0; > > +} > > + > > +static void aspeed_i2c_slave_send_async(I2CSlave *slave, uint8_t data) > > +{ > > + AspeedI2CSlave *s = ASPEED_I2C_SLAVE(slave); > > + AspeedI2CBus *bus = s->bus; > > + > > + bus->buf = (data & I2CD_BYTE_BUF_RX_MASK) << I2CD_BYTE_BUF_RX_SHIFT; > > + bus->intr_status |= I2CD_INTR_RX_DONE; > > + > > + aspeed_i2c_bus_raise_interrupt(bus); > > +} > > + > > +static void aspeed_i2c_slave_class_init(ObjectClass *klass, void *Data) > > +{ > > + DeviceClass *dc = DEVICE_CLASS(klass); > > + I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass); > > + > > + dc->desc = "Aspeed I2C Bus Slave"; > > + > > + sc->event = aspeed_i2c_slave_event; > > + sc->send_async = aspeed_i2c_slave_send_async; > > +} > > + > > +static const TypeInfo aspeed_i2c_slave_info = { > > + .name = TYPE_ASPEED_I2C_SLAVE, > > + .parent = TYPE_I2C_SLAVE, > > + .instance_size = sizeof(AspeedI2CSlave), > > + .class_init = aspeed_i2c_slave_class_init, > > +}; > > + > > static void aspeed_i2c_bus_reset(DeviceState *dev) > > { > > AspeedI2CBus *s = ASPEED_I2C_BUS(dev); > > s->intr_ctrl = 0; > > s->intr_status = 0; > > + s->dev_addr = 0; > > Please include the new reg in vmstate. > Understood. > > s->cmd = 0; > > s->buf = 0; > > s->dma_addr = 0; > > @@ -881,6 +955,8 @@ static void aspeed_i2c_bus_realize(DeviceState *dev, > > Error **errp) > > sysbus_init_irq(SYS_BUS_DEVICE(dev), &s->irq); > > s->bus = i2c_init_bus(dev, name); > > + s->slave = ASPEED_I2C_SLAVE(i2c_slave_create_simple(s->bus, > > TYPE_ASPEED_I2C_SLAVE, 0xff)); > > + s->slave->bus = s; > > memory_region_init_io(&s->mr, OBJECT(s), &aspeed_i2c_bus_ops, > > s, name, aic->reg_size); > > @@ -1016,6 +1092,7 @@ static const TypeInfo aspeed_2600_i2c_info = { > > static void aspeed_i2c_register_types(void) > > { > > type_register_static(&aspeed_i2c_bus_info); > > + type_register_static(&aspeed_i2c_slave_info); > > type_register_static(&aspeed_i2c_info); > > type_register_static(&aspeed_2400_i2c_info); > > type_register_static(&aspeed_2500_i2c_info); > > diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events > > index 7d8907c1eede..85e4bddff936 100644 > > --- a/hw/i2c/trace-events > > +++ b/hw/i2c/trace-events > > @@ -9,7 +9,7 @@ i2c_recv(uint8_t address, uint8_t data) "recv(addr:0x%02x) > > data:0x%02x" > > # aspeed_i2c.c > > aspeed_i2c_bus_cmd(uint32_t cmd, const char *cmd_flags, uint32_t count, > > uint32_t intr_status) "handling cmd=0x%x %s count=%d intr=0x%x" > > -aspeed_i2c_bus_raise_interrupt(uint32_t intr_status, const char *str1, > > const char *str2, const char *str3, const char *str4, const char *str5) > > "handled intr=0x%x %s%s%s%s%s" > > +aspeed_i2c_bus_raise_interrupt(uint32_t intr_status, const char *str1, > > const char *str2, const char *str3, const char *str4, const char *str5, > > const char *str6) "handled intr=0x%x %s%s%s%s%s%s" > > aspeed_i2c_bus_read(uint32_t busid, uint64_t offset, unsigned size, > > uint64_t value) "bus[%d]: To 0x%" PRIx64 " of size %u: 0x%" PRIx64 > > aspeed_i2c_bus_write(uint32_t busid, uint64_t offset, unsigned size, > > uint64_t value) "bus[%d]: To 0x%" PRIx64 " of size %u: 0x%" PRIx64 > > aspeed_i2c_bus_send(const char *mode, int i, int count, uint8_t byte) "%s > > send %d/%d 0x%02x" > > diff --git a/hw/misc/meson.build b/hw/misc/meson.build > > index 6fb69612e064..c1c1abea41dd 100644 > > --- a/hw/misc/meson.build > > +++ b/hw/misc/meson.build > > @@ -122,6 +122,8 @@ softmmu_ss.add(when: 'CONFIG_NRF51_SOC', if_true: > > files('nrf51_rng.c')) > > softmmu_ss.add(when: 'CONFIG_GRLIB', if_true: > > files('grlib_ahb_apb_pnp.c')) > > +softmmu_ss.add(when: 'CONFIG_I2C', if_true: files('i2c-echo.c')) > > That's for another patch. > Yeah, was a mistake on my part when chopping up my work. > > + > > specific_ss.add(when: 'CONFIG_AVR_POWER', if_true: files('avr_power.c')) > > specific_ss.add(when: 'CONFIG_IMX', if_true: files('imx6_src.c')) > > diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h > > index 4b9be09274c7..3f1a9c07a00b 100644 > > --- a/include/hw/i2c/aspeed_i2c.h > > +++ b/include/hw/i2c/aspeed_i2c.h > > @@ -42,6 +42,7 @@ struct AspeedI2CBus { > > SysBusDevice parent_obj; > > struct AspeedI2CState *controller; > > + struct AspeedI2CSlave *slave; > > MemoryRegion mr; > > @@ -53,6 +54,7 @@ struct AspeedI2CBus { > > uint32_t timing[2]; > > uint32_t intr_ctrl; > > uint32_t intr_status; > > + uint32_t dev_addr; > > uint32_t cmd; > > uint32_t buf; > > uint32_t pool_ctrl; > > @@ -76,6 +78,12 @@ struct AspeedI2CState { > > AddressSpace dram_as; > > }; > > +#define TYPE_ASPEED_I2C_SLAVE "aspeed.i2c.slave" > > +OBJECT_DECLARE_SIMPLE_TYPE(AspeedI2CSlave, ASPEED_I2C_SLAVE) > > +struct AspeedI2CSlave { > > + I2CSlave i2c; > > + AspeedI2CBus *bus; > > +}; > > AFAICT, AspeedI2CSlave is not that useful since it doesn't maintain any > state. The QOM interface proposal looks like a better approach. > Agree. > > struct AspeedI2CClass { > > SysBusDeviceClass parent_class; > > > If you could send these 3 patches : > - aspeed_i2c_bus_raise_interrupt trace event rework > - intr_ctrl_mask class attribute > - dev_addr support > > I will queue them quickly and we will focus on adding slave support only. > Cool, but please see my question on the intr_ctrl_mask.
signature.asc
Description: PGP signature