[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: |
Cédric Le Goater |
|
Subject: |
Re: [RFC PATCH 3/4] hw/i2c: add slave mode for aspeed_i2c |
|
Date: |
Wed, 6 Apr 2022 11:44:14 +0200 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 |
On 4/6/22 11:16, Klaus Jensen wrote:
On Apr 6 10:52, Cédric Le Goater wrote:
On 4/6/22 09:40, Klaus Jensen wrote:
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.
It will be easier to extend. We have 3 different patchsets modifying this
same event on the mailing list :)
True :)
- 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?
No. sorry. There are multiple fixes required I think.
The layout of the Interrupt Control Register (0x0C) differs on the
AST2400, AST2500, AST2600 SoCs. We need to clarify that first.
bits 11:7 and 31:16 are reserved on all. AST2400 lacks bit 15 which
enables a slave timeout interrupt on AST2500 and AST2600.
Then, the Interrupt Status Register differs also. The AST2400 doesn't
have the Slave Address match indicator and the Slave Address Receiving
pending bits. On the AST2600, there are 3 possibles addresses, only
2 on the AST2500 (and only 1 on the AST2400). So that modifies the
I2CD_INTR_SLAVE_ADDR_RX_MATCH bit.
Ugh. I'm heavily burden by the fact that I do not have a spec sheet
available... I'll try to request one from Aspeed. I reversed this from
the Linux driver, so I'm just tried to match up with how that behaves.
With Slave Address Match indicator, you mean bit 31? There is only one
bit, so how does it work with the third address?
On the AST2400 (only 1 slave address)
* no upper bits
On the AST2500 (2 possible slave addresses),
* bit[31] : Slave Address match indicator
* bit[30] : Slave Address Receiving pending
On the AST2600 (3 possible slave addresses),
* bit[31-30] : Slave Address match indicator
* bit[29] : Slave Address Receiving pending
Since these 2 or 3 bits are read-only. I wonder how this is impacting
your slave implementation. is it ? If not, may be slave address match can
wait for now.
As far as I can tell, the kernel driver uses bit 7 (called
SLAVE_ADDR_RX_MATCH in QEMU and SLAVE_MATCH in Linux) to start the slave
state machinery. It does not use bit 31 (SLAVE_ADDR_MATCH in QEMU). The
naming for bit 7 in Linux is probably off and should be ranamed to match
QEMU?
No. This is a mess :) Both are correct. bit 7 is an interrupt enable/status.
bit 31 "indicates" which address : 1, 2, 3.
I understand that this shouldn't assume to only work with the slave
machinery in Linux, but that is the only platfrom I can currently
experiment with.
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.
thanks, we will address the full layout of this register later when needed.
But there are fields for each address.
Right. Again, I just reversed from the kernel driver and it only sets
the lower byte.
That's fine for now. the first 7bits in each byte represents a slave address.
Thanks,
C.