[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH 3/4] hw/i2c: add slave mode for aspeed_i2c
|
From: |
Klaus Jensen |
|
Subject: |
Re: [RFC PATCH 3/4] hw/i2c: add slave mode for aspeed_i2c |
|
Date: |
Wed, 6 Apr 2022 09:40:48 +0200 |
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.jensen@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.jensen@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