* [PATCH v3 01/14] hw/i2c/aspeed: Fix R_I2CD_FUN_CTRL reference
2022-06-30 4:51 [PATCH v3 00/14] hw/i2c/aspeed: I2C slave mode DMA RX w/ new regs Peter Delevoryas
@ 2022-06-30 4:51 ` Peter Delevoryas
2022-06-30 4:51 ` [PATCH v3 02/14] hw/i2c/aspeed: Fix DMA len write-enable bit handling Peter Delevoryas
` (14 subsequent siblings)
15 siblings, 0 replies; 34+ messages in thread
From: Peter Delevoryas @ 2022-06-30 4:51 UTC (permalink / raw)
Cc: clg, peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel, pdel
From: Peter Delevoryas <pdel@fb.com>
Very minor, doesn't effect functionality, but this is supposed to be
R_I2CC_FUN_CTRL (new-mode, not old-mode).
Fixes: ba2cccd64e9 ("aspeed: i2c: Add new mode support")
Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
hw/i2c/aspeed_i2c.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 37ae1f2e04..ff33571954 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -552,7 +552,7 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
__func__);
break;
}
- bus->regs[R_I2CD_FUN_CTRL] = value & 0x007dc3ff;
+ bus->regs[R_I2CC_FUN_CTRL] = value & 0x007dc3ff;
break;
case A_I2CC_AC_TIMING:
bus->regs[R_I2CC_AC_TIMING] = value & 0x1ffff0ff;
--
2.37.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 02/14] hw/i2c/aspeed: Fix DMA len write-enable bit handling
2022-06-30 4:51 [PATCH v3 00/14] hw/i2c/aspeed: I2C slave mode DMA RX w/ new regs Peter Delevoryas
2022-06-30 4:51 ` [PATCH v3 01/14] hw/i2c/aspeed: Fix R_I2CD_FUN_CTRL reference Peter Delevoryas
@ 2022-06-30 4:51 ` Peter Delevoryas
2022-06-30 4:51 ` [PATCH v3 03/14] hw/i2c/aspeed: Fix MASTER_EN missing error message Peter Delevoryas
` (13 subsequent siblings)
15 siblings, 0 replies; 34+ messages in thread
From: Peter Delevoryas @ 2022-06-30 4:51 UTC (permalink / raw)
Cc: clg, peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel, pdel
From: Peter Delevoryas <pdel@fb.com>
I noticed i2c rx transfers were getting shortened to "1" on Zephyr. It
seems to be because the Zephyr i2c driver sets the RX DMA len with the
RX field write-enable bit set (bit 31) to avoid a read-modify-write. [1]
/* 0x1C : I2CM Master DMA Transfer Length Register */
I think we should be checking the write-enable bits on the incoming
value, not checking the register array. I'm not sure we're even writing
the write-enable bits to the register array, actually.
[1] https://github.com/AspeedTech-BMC/zephyr/blob/db3dbcc9c52e67a47180890ac938ed380b33f91c/drivers/i2c/i2c_aspeed.c#L145-L148
Fixes: ba2cccd64e90f34 ("aspeed: i2c: Add new mode support")
Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
hw/i2c/aspeed_i2c.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index ff33571954..cbaa7c96fc 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -644,18 +644,18 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
RX_BUF_LEN) + 1;
break;
case A_I2CM_DMA_LEN:
- w1t = ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, RX_BUF_LEN_W1T) ||
- ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, TX_BUF_LEN_W1T);
+ w1t = FIELD_EX32(value, I2CM_DMA_LEN, RX_BUF_LEN_W1T) ||
+ FIELD_EX32(value, I2CM_DMA_LEN, TX_BUF_LEN_W1T);
/* If none of the w1t bits are set, just write to the reg as normal. */
if (!w1t) {
bus->regs[R_I2CM_DMA_LEN] = value;
break;
}
- if (ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, RX_BUF_LEN_W1T)) {
+ if (FIELD_EX32(value, I2CM_DMA_LEN, RX_BUF_LEN_W1T)) {
ARRAY_FIELD_DP32(bus->regs, I2CM_DMA_LEN, RX_BUF_LEN,
FIELD_EX32(value, I2CM_DMA_LEN, RX_BUF_LEN));
}
- if (ARRAY_FIELD_EX32(bus->regs, I2CM_DMA_LEN, TX_BUF_LEN_W1T)) {
+ if (FIELD_EX32(value, I2CM_DMA_LEN, TX_BUF_LEN_W1T)) {
ARRAY_FIELD_DP32(bus->regs, I2CM_DMA_LEN, TX_BUF_LEN,
FIELD_EX32(value, I2CM_DMA_LEN, TX_BUF_LEN));
}
--
2.37.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 03/14] hw/i2c/aspeed: Fix MASTER_EN missing error message
2022-06-30 4:51 [PATCH v3 00/14] hw/i2c/aspeed: I2C slave mode DMA RX w/ new regs Peter Delevoryas
2022-06-30 4:51 ` [PATCH v3 01/14] hw/i2c/aspeed: Fix R_I2CD_FUN_CTRL reference Peter Delevoryas
2022-06-30 4:51 ` [PATCH v3 02/14] hw/i2c/aspeed: Fix DMA len write-enable bit handling Peter Delevoryas
@ 2022-06-30 4:51 ` Peter Delevoryas
2022-06-30 4:51 ` [PATCH v3 04/14] hw/i2c: support multiple masters Peter Delevoryas
` (12 subsequent siblings)
15 siblings, 0 replies; 34+ messages in thread
From: Peter Delevoryas @ 2022-06-30 4:51 UTC (permalink / raw)
Cc: clg, peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel, pdel
From: Peter Delevoryas <pdel@fb.com>
aspeed_i2c_bus_is_master is checking if master mode is enabled in the I2C
bus controller's function-control register, not that slave mode is enabled
or something. The error here is that the guest is trying to trigger an I2C
master mode command while master mode is not enabled.
Fixes: ba2cccd64e90f342 ("aspeed: i2c: Add new mode support")
Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
hw/i2c/aspeed_i2c.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index cbaa7c96fc..c153a1a942 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -601,7 +601,7 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
}
if (!aspeed_i2c_bus_is_master(bus)) {
- qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: Master mode is not enabled\n",
__func__);
break;
}
@@ -744,7 +744,7 @@ static void aspeed_i2c_bus_old_write(AspeedI2CBus *bus, hwaddr offset,
}
if (!aspeed_i2c_bus_is_master(bus)) {
- qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: Master mode is not enabled\n",
__func__);
break;
}
--
2.37.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 04/14] hw/i2c: support multiple masters
2022-06-30 4:51 [PATCH v3 00/14] hw/i2c/aspeed: I2C slave mode DMA RX w/ new regs Peter Delevoryas
` (2 preceding siblings ...)
2022-06-30 4:51 ` [PATCH v3 03/14] hw/i2c/aspeed: Fix MASTER_EN missing error message Peter Delevoryas
@ 2022-06-30 4:51 ` Peter Delevoryas
2022-06-30 4:51 ` [PATCH v3 05/14] hw/i2c: add asynchronous send Peter Delevoryas
` (11 subsequent siblings)
15 siblings, 0 replies; 34+ messages in thread
From: Peter Delevoryas @ 2022-06-30 4:51 UTC (permalink / raw)
Cc: clg, peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel, pdel
From: Klaus Jensen <k.jensen@samsung.com>
Allow slaves to master the bus by registering a bottom halve. If the bus
is busy, the bottom half is queued up. When a slave has succesfully
mastered the bus, the bottom half is scheduled.
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
[ clg : - fixed typos in commit log ]
Message-Id: <20220601210831.67259-4-its@irrelevant.dk>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
hw/i2c/core.c | 34 +++++++++++++++++++++++++++++++++-
include/hw/i2c/i2c.h | 14 ++++++++++++++
2 files changed, 47 insertions(+), 1 deletion(-)
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index d0cb2d32fa..145dce6078 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -13,6 +13,7 @@
#include "migration/vmstate.h"
#include "qapi/error.h"
#include "qemu/module.h"
+#include "qemu/main-loop.h"
#include "trace.h"
#define I2C_BROADCAST 0x00
@@ -62,6 +63,7 @@ I2CBus *i2c_init_bus(DeviceState *parent, const char *name)
bus = I2C_BUS(qbus_new(TYPE_I2C_BUS, parent, name));
QLIST_INIT(&bus->current_devs);
+ QSIMPLEQ_INIT(&bus->pending_masters);
vmstate_register(NULL, VMSTATE_INSTANCE_ID_ANY, &vmstate_i2c_bus, bus);
return bus;
}
@@ -74,7 +76,7 @@ void i2c_slave_set_address(I2CSlave *dev, uint8_t address)
/* Return nonzero if bus is busy. */
int i2c_bus_busy(I2CBus *bus)
{
- return !QLIST_EMPTY(&bus->current_devs);
+ return !QLIST_EMPTY(&bus->current_devs) || bus->bh;
}
bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast,
@@ -180,6 +182,26 @@ int i2c_start_transfer(I2CBus *bus, uint8_t address, bool is_recv)
: I2C_START_SEND);
}
+void i2c_bus_master(I2CBus *bus, QEMUBH *bh)
+{
+ if (i2c_bus_busy(bus)) {
+ I2CPendingMaster *node = g_new(struct I2CPendingMaster, 1);
+ node->bh = bh;
+
+ QSIMPLEQ_INSERT_TAIL(&bus->pending_masters, node, entry);
+
+ return;
+ }
+
+ bus->bh = bh;
+ qemu_bh_schedule(bus->bh);
+}
+
+void i2c_bus_release(I2CBus *bus)
+{
+ bus->bh = NULL;
+}
+
int i2c_start_recv(I2CBus *bus, uint8_t address)
{
return i2c_do_start_transfer(bus, address, I2C_START_RECV);
@@ -206,6 +228,16 @@ void i2c_end_transfer(I2CBus *bus)
g_free(node);
}
bus->broadcast = false;
+
+ if (!QSIMPLEQ_EMPTY(&bus->pending_masters)) {
+ I2CPendingMaster *node = QSIMPLEQ_FIRST(&bus->pending_masters);
+ bus->bh = node->bh;
+
+ QSIMPLEQ_REMOVE_HEAD(&bus->pending_masters, entry);
+ g_free(node);
+
+ qemu_bh_schedule(bus->bh);
+ }
}
int i2c_send(I2CBus *bus, uint8_t data)
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index 5ca3b708c0..be8bb8b78a 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -69,13 +69,25 @@ struct I2CNode {
QLIST_ENTRY(I2CNode) next;
};
+typedef struct I2CPendingMaster I2CPendingMaster;
+
+struct I2CPendingMaster {
+ QEMUBH *bh;
+ QSIMPLEQ_ENTRY(I2CPendingMaster) entry;
+};
+
typedef QLIST_HEAD(I2CNodeList, I2CNode) I2CNodeList;
+typedef QSIMPLEQ_HEAD(I2CPendingMasters, I2CPendingMaster) I2CPendingMasters;
struct I2CBus {
BusState qbus;
I2CNodeList current_devs;
+ I2CPendingMasters pending_masters;
uint8_t saved_address;
bool broadcast;
+
+ /* Set from slave currently mastering the bus. */
+ QEMUBH *bh;
};
I2CBus *i2c_init_bus(DeviceState *parent, const char *name);
@@ -117,6 +129,8 @@ int i2c_start_send(I2CBus *bus, uint8_t address);
void i2c_end_transfer(I2CBus *bus);
void i2c_nack(I2CBus *bus);
+void i2c_bus_master(I2CBus *bus, QEMUBH *bh);
+void i2c_bus_release(I2CBus *bus);
int i2c_send(I2CBus *bus, uint8_t data);
uint8_t i2c_recv(I2CBus *bus);
bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast,
--
2.37.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 05/14] hw/i2c: add asynchronous send
2022-06-30 4:51 [PATCH v3 00/14] hw/i2c/aspeed: I2C slave mode DMA RX w/ new regs Peter Delevoryas
` (3 preceding siblings ...)
2022-06-30 4:51 ` [PATCH v3 04/14] hw/i2c: support multiple masters Peter Delevoryas
@ 2022-06-30 4:51 ` Peter Delevoryas
2022-06-30 4:51 ` [PATCH v3 06/14] hw/i2c/aspeed: add slave device in old register mode Peter Delevoryas
` (10 subsequent siblings)
15 siblings, 0 replies; 34+ messages in thread
From: Peter Delevoryas @ 2022-06-30 4:51 UTC (permalink / raw)
Cc: clg, peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel, pdel
From: Klaus Jensen <k.jensen@samsung.com>
Add an asynchronous version of i2c_send() that requires the slave to
explicitly acknowledge on the bus with i2c_ack().
The current master must use the new i2c_start_send_async() to indicate
that it wants to do an asynchronous transfer. This allows the i2c core
to check if the target slave supports this or not. This approach relies
on adding a new enum i2c_event member, which is why a bunch of other
devices needs changes in their event handling switches.
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Message-Id: <20220601210831.67259-5-its@irrelevant.dk>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
hw/arm/pxa2xx.c | 2 ++
hw/display/sii9022.c | 2 ++
hw/display/ssd0303.c | 2 ++
hw/i2c/core.c | 36 +++++++++++++++++++++++++++++++++++-
hw/i2c/smbus_slave.c | 4 ++++
hw/i2c/trace-events | 2 ++
hw/nvram/eeprom_at24c.c | 2 ++
hw/sensor/lsm303dlhc_mag.c | 2 ++
include/hw/i2c/i2c.h | 16 ++++++++++++++++
9 files changed, 67 insertions(+), 1 deletion(-)
diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
index f4f687df68..93dda83d7a 100644
--- a/hw/arm/pxa2xx.c
+++ b/hw/arm/pxa2xx.c
@@ -1305,6 +1305,8 @@ static int pxa2xx_i2c_event(I2CSlave *i2c, enum i2c_event event)
case I2C_NACK:
s->status |= 1 << 1; /* set ACKNAK */
break;
+ default:
+ return -1;
}
pxa2xx_i2c_update(s);
diff --git a/hw/display/sii9022.c b/hw/display/sii9022.c
index b591a58789..664fd4046d 100644
--- a/hw/display/sii9022.c
+++ b/hw/display/sii9022.c
@@ -76,6 +76,8 @@ static int sii9022_event(I2CSlave *i2c, enum i2c_event event)
break;
case I2C_NACK:
break;
+ default:
+ return -1;
}
return 0;
diff --git a/hw/display/ssd0303.c b/hw/display/ssd0303.c
index aeae22da9c..d67b0ad7b5 100644
--- a/hw/display/ssd0303.c
+++ b/hw/display/ssd0303.c
@@ -196,6 +196,8 @@ static int ssd0303_event(I2CSlave *i2c, enum i2c_event event)
case I2C_NACK:
/* Nothing to do. */
break;
+ default:
+ return -1;
}
return 0;
diff --git a/hw/i2c/core.c b/hw/i2c/core.c
index 145dce6078..d4ba8146bf 100644
--- a/hw/i2c/core.c
+++ b/hw/i2c/core.c
@@ -161,7 +161,8 @@ static int i2c_do_start_transfer(I2CBus *bus, uint8_t address,
start condition. */
if (sc->event) {
- trace_i2c_event("start", s->address);
+ trace_i2c_event(event == I2C_START_SEND ? "start" : "start_async",
+ s->address);
rv = sc->event(s, event);
if (rv && !bus->broadcast) {
if (bus_scanned) {
@@ -212,6 +213,11 @@ int i2c_start_send(I2CBus *bus, uint8_t address)
return i2c_do_start_transfer(bus, address, I2C_START_SEND);
}
+int i2c_start_send_async(I2CBus *bus, uint8_t address)
+{
+ return i2c_do_start_transfer(bus, address, I2C_START_SEND_ASYNC);
+}
+
void i2c_end_transfer(I2CBus *bus)
{
I2CSlaveClass *sc;
@@ -261,6 +267,23 @@ int i2c_send(I2CBus *bus, uint8_t data)
return ret ? -1 : 0;
}
+int i2c_send_async(I2CBus *bus, uint8_t data)
+{
+ I2CNode *node = QLIST_FIRST(&bus->current_devs);
+ I2CSlave *slave = node->elt;
+ I2CSlaveClass *sc = I2C_SLAVE_GET_CLASS(slave);
+
+ if (!sc->send_async) {
+ return -1;
+ }
+
+ trace_i2c_send_async(slave->address, data);
+
+ sc->send_async(slave, data);
+
+ return 0;
+}
+
uint8_t i2c_recv(I2CBus *bus)
{
uint8_t data = 0xff;
@@ -297,6 +320,17 @@ void i2c_nack(I2CBus *bus)
}
}
+void i2c_ack(I2CBus *bus)
+{
+ if (!bus->bh) {
+ return;
+ }
+
+ trace_i2c_ack();
+
+ qemu_bh_schedule(bus->bh);
+}
+
static int i2c_slave_post_load(void *opaque, int version_id)
{
I2CSlave *dev = opaque;
diff --git a/hw/i2c/smbus_slave.c b/hw/i2c/smbus_slave.c
index 5d10e27664..feb3ec6333 100644
--- a/hw/i2c/smbus_slave.c
+++ b/hw/i2c/smbus_slave.c
@@ -143,6 +143,10 @@ static int smbus_i2c_event(I2CSlave *s, enum i2c_event event)
dev->mode = SMBUS_CONFUSED;
break;
}
+ break;
+
+ default:
+ return -1;
}
return 0;
diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
index 209275ed2d..af181d43ee 100644
--- a/hw/i2c/trace-events
+++ b/hw/i2c/trace-events
@@ -4,7 +4,9 @@
i2c_event(const char *event, uint8_t address) "%s(addr:0x%02x)"
i2c_send(uint8_t address, uint8_t data) "send(addr:0x%02x) data:0x%02x"
+i2c_send_async(uint8_t address, uint8_t data) "send_async(addr:0x%02x) data:0x%02x"
i2c_recv(uint8_t address, uint8_t data) "recv(addr:0x%02x) data:0x%02x"
+i2c_ack(void) ""
# aspeed_i2c.c
diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index 01a3093600..d695f6ae89 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -75,6 +75,8 @@ int at24c_eeprom_event(I2CSlave *s, enum i2c_event event)
break;
case I2C_NACK:
break;
+ default:
+ return -1;
}
return 0;
}
diff --git a/hw/sensor/lsm303dlhc_mag.c b/hw/sensor/lsm303dlhc_mag.c
index 4c98ddbf20..bb8d48b2fd 100644
--- a/hw/sensor/lsm303dlhc_mag.c
+++ b/hw/sensor/lsm303dlhc_mag.c
@@ -427,6 +427,8 @@ static int lsm303dlhc_mag_event(I2CSlave *i2c, enum i2c_event event)
break;
case I2C_NACK:
break;
+ default:
+ return -1;
}
s->len = 0;
diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
index be8bb8b78a..9b9581d230 100644
--- a/include/hw/i2c/i2c.h
+++ b/include/hw/i2c/i2c.h
@@ -12,6 +12,7 @@
enum i2c_event {
I2C_START_RECV,
I2C_START_SEND,
+ I2C_START_SEND_ASYNC,
I2C_FINISH,
I2C_NACK /* Masker NACKed a receive byte. */
};
@@ -28,6 +29,9 @@ struct I2CSlaveClass {
/* Master to slave. Returns non-zero for a NAK, 0 for success. */
int (*send)(I2CSlave *s, uint8_t data);
+ /* Master to slave (asynchronous). Receiving slave must call i2c_ack(). */
+ void (*send_async)(I2CSlave *s, uint8_t data);
+
/*
* Slave to master. This cannot fail, the device should always
* return something here.
@@ -127,11 +131,23 @@ int i2c_start_recv(I2CBus *bus, uint8_t address);
*/
int i2c_start_send(I2CBus *bus, uint8_t address);
+/**
+ * i2c_start_send_async: start an asynchronous 'send' transfer on an I2C bus.
+ *
+ * @bus: #I2CBus to be used
+ * @address: address of the slave
+ *
+ * Return: 0 on success, -1 on error
+ */
+int i2c_start_send_async(I2CBus *bus, uint8_t address);
+
void i2c_end_transfer(I2CBus *bus);
void i2c_nack(I2CBus *bus);
+void i2c_ack(I2CBus *bus);
void i2c_bus_master(I2CBus *bus, QEMUBH *bh);
void i2c_bus_release(I2CBus *bus);
int i2c_send(I2CBus *bus, uint8_t data);
+int i2c_send_async(I2CBus *bus, uint8_t data);
uint8_t i2c_recv(I2CBus *bus);
bool i2c_scan_bus(I2CBus *bus, uint8_t address, bool broadcast,
I2CNodeList *current_devs);
--
2.37.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 06/14] hw/i2c/aspeed: add slave device in old register mode
2022-06-30 4:51 [PATCH v3 00/14] hw/i2c/aspeed: I2C slave mode DMA RX w/ new regs Peter Delevoryas
` (4 preceding siblings ...)
2022-06-30 4:51 ` [PATCH v3 05/14] hw/i2c: add asynchronous send Peter Delevoryas
@ 2022-06-30 4:51 ` Peter Delevoryas
2022-06-30 4:51 ` [PATCH v3 07/14] hw/i2c/aspeed: Add new-registers DMA slave mode RX support Peter Delevoryas
` (9 subsequent siblings)
15 siblings, 0 replies; 34+ messages in thread
From: Peter Delevoryas @ 2022-06-30 4:51 UTC (permalink / raw)
Cc: clg, peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel, pdel
From: Klaus Jensen <k.jensen@samsung.com>
Add slave mode functionality for the Aspeed I2C controller in old
register mode. This is implemented by realizing an I2C slave device
owned by the I2C controller and attached to its own bus.
The I2C slave device only implements asynchronous sends on the bus, so
slaves not supporting that will not be able to communicate with it.
Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
[ clg: checkpatch fixes ]
Message-Id: <20220601210831.67259-6-its@irrelevant.dk>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
hw/i2c/aspeed_i2c.c | 89 +++++++++++++++++++++++++++++++++----
include/hw/i2c/aspeed_i2c.h | 8 ++++
2 files changed, 88 insertions(+), 9 deletions(-)
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index c153a1a942..8a8514586f 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -696,9 +696,7 @@ static void aspeed_i2c_bus_old_write(AspeedI2CBus *bus, hwaddr offset,
switch (offset) {
case A_I2CD_FUN_CTRL:
if (SHARED_FIELD_EX32(value, SLAVE_EN)) {
- qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
- __func__);
- break;
+ i2c_slave_set_address(bus->slave, bus->regs[R_I2CD_DEV_ADDR]);
}
bus->regs[R_I2CD_FUN_CTRL] = value & 0x0071C3FF;
break;
@@ -719,12 +717,15 @@ static void aspeed_i2c_bus_old_write(AspeedI2CBus *bus, hwaddr offset,
bus->controller->intr_status &= ~(1 << bus->id);
qemu_irq_lower(aic->bus_get_irq(bus));
}
- if (handle_rx && (SHARED_ARRAY_FIELD_EX32(bus->regs, R_I2CD_CMD,
- M_RX_CMD) ||
- SHARED_ARRAY_FIELD_EX32(bus->regs, R_I2CD_CMD,
- M_S_RX_CMD_LAST))) {
- aspeed_i2c_handle_rx_cmd(bus);
- aspeed_i2c_bus_raise_interrupt(bus);
+ if (handle_rx) {
+ if (SHARED_ARRAY_FIELD_EX32(bus->regs, R_I2CD_CMD, M_RX_CMD) ||
+ SHARED_ARRAY_FIELD_EX32(bus->regs, R_I2CD_CMD,
+ 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 A_I2CD_DEV_ADDR:
@@ -1036,6 +1037,73 @@ static const TypeInfo aspeed_i2c_info = {
.abstract = true,
};
+static int aspeed_i2c_bus_slave_event(I2CSlave *slave, enum i2c_event event)
+{
+ BusState *qbus = qdev_get_parent_bus(DEVICE(slave));
+ AspeedI2CBus *bus = ASPEED_I2C_BUS(qbus->parent);
+ uint32_t reg_intr_sts = aspeed_i2c_bus_intr_sts_offset(bus);
+ uint32_t reg_byte_buf = aspeed_i2c_bus_byte_buf_offset(bus);
+ uint32_t value;
+
+ switch (event) {
+ case I2C_START_SEND_ASYNC:
+ value = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_byte_buf, TX_BUF);
+ SHARED_ARRAY_FIELD_DP32(bus->regs, reg_byte_buf, RX_BUF, value << 1);
+
+ ARRAY_FIELD_DP32(bus->regs, I2CD_INTR_STS, SLAVE_ADDR_RX_MATCH, 1);
+ SHARED_ARRAY_FIELD_DP32(bus->regs, reg_intr_sts, RX_DONE, 1);
+
+ aspeed_i2c_set_state(bus, I2CD_STXD);
+
+ break;
+
+ case I2C_FINISH:
+ SHARED_ARRAY_FIELD_DP32(bus->regs, reg_intr_sts, NORMAL_STOP, 1);
+
+ aspeed_i2c_set_state(bus, I2CD_IDLE);
+
+ break;
+
+ default:
+ return -1;
+ }
+
+ aspeed_i2c_bus_raise_interrupt(bus);
+
+ return 0;
+}
+
+static void aspeed_i2c_bus_slave_send_async(I2CSlave *slave, uint8_t data)
+{
+ BusState *qbus = qdev_get_parent_bus(DEVICE(slave));
+ AspeedI2CBus *bus = ASPEED_I2C_BUS(qbus->parent);
+ uint32_t reg_intr_sts = aspeed_i2c_bus_intr_sts_offset(bus);
+ uint32_t reg_byte_buf = aspeed_i2c_bus_byte_buf_offset(bus);
+
+ SHARED_ARRAY_FIELD_DP32(bus->regs, reg_byte_buf, RX_BUF, data);
+ SHARED_ARRAY_FIELD_DP32(bus->regs, reg_intr_sts, RX_DONE, 1);
+
+ aspeed_i2c_bus_raise_interrupt(bus);
+}
+
+static void aspeed_i2c_bus_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_bus_slave_event;
+ sc->send_async = aspeed_i2c_bus_slave_send_async;
+}
+
+static const TypeInfo aspeed_i2c_bus_slave_info = {
+ .name = TYPE_ASPEED_I2C_BUS_SLAVE,
+ .parent = TYPE_I2C_SLAVE,
+ .instance_size = sizeof(AspeedI2CBusSlave),
+ .class_init = aspeed_i2c_bus_slave_class_init,
+};
+
static void aspeed_i2c_bus_reset(DeviceState *dev)
{
AspeedI2CBus *s = ASPEED_I2C_BUS(dev);
@@ -1060,6 +1128,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 = i2c_slave_create_simple(s->bus, TYPE_ASPEED_I2C_BUS_SLAVE,
+ 0xff);
memory_region_init_io(&s->mr, OBJECT(s), &aspeed_i2c_bus_ops,
s, name, aic->reg_size);
@@ -1219,6 +1289,7 @@ static const TypeInfo aspeed_1030_i2c_info = {
static void aspeed_i2c_register_types(void)
{
type_register_static(&aspeed_i2c_bus_info);
+ type_register_static(&aspeed_i2c_bus_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/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index 1398befc10..ba148b2f6d 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -223,6 +223,9 @@ struct AspeedI2CBus {
struct AspeedI2CState *controller;
+ /* slave mode */
+ I2CSlave *slave;
+
MemoryRegion mr;
I2CBus *bus;
@@ -249,6 +252,11 @@ struct AspeedI2CState {
AddressSpace dram_as;
};
+#define TYPE_ASPEED_I2C_BUS_SLAVE "aspeed.i2c.slave"
+OBJECT_DECLARE_SIMPLE_TYPE(AspeedI2CBusSlave, ASPEED_I2C_BUS_SLAVE)
+struct AspeedI2CBusSlave {
+ I2CSlave i2c;
+};
struct AspeedI2CClass {
SysBusDeviceClass parent_class;
--
2.37.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 07/14] hw/i2c/aspeed: Add new-registers DMA slave mode RX support
2022-06-30 4:51 [PATCH v3 00/14] hw/i2c/aspeed: I2C slave mode DMA RX w/ new regs Peter Delevoryas
` (5 preceding siblings ...)
2022-06-30 4:51 ` [PATCH v3 06/14] hw/i2c/aspeed: add slave device in old register mode Peter Delevoryas
@ 2022-06-30 4:51 ` Peter Delevoryas
2022-06-30 4:51 ` [PATCH v3 08/14] hw/i2c/pmbus: Add idle state to return 0xff's Peter Delevoryas
` (8 subsequent siblings)
15 siblings, 0 replies; 34+ messages in thread
From: Peter Delevoryas @ 2022-06-30 4:51 UTC (permalink / raw)
Cc: clg, peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel, pdel
From: Peter Delevoryas <pdel@fb.com>
This commit adds support for DMA RX in slave mode while using the new
register set in the AST2600 and AST1030. This patch also pretty much
assumes packet mode is enabled, I'm not sure if this will work in DMA
step mode.
This is particularly useful for testing IPMB exchanges between Zephyr
and external devices, which requires multi-master I2C support and DMA in
the new register mode, because the Zephyr drivers from Aspeed use DMA in
the new mode by default. The Zephyr drivers are also using packet mode.
The typical sequence of events for receiving data in DMA slave + packet
mode is that the Zephyr firmware will configure the slave address
register with an address to receive on and configure the bus's function
control register to enable master mode and slave mode simultaneously at
startup, before any transfers are initiated.
RX DMA is enabled in the slave mode command register, and the slave RX
DMA buffer address and slave RX DMA buffer length are set. TX DMA is not
covered in this patch.
When the Aspeed I2C controller receives data from some other I2C master,
it will reset the I2CS_DMA_LEN RX_LEN value to zero, then buffer
incoming data in the RX DMA buffer while incrementing the I2CC_DMA_ADDR
address counter and decrementing the I2CC_DMA_LEN counter. It will also
update the I2CS_DMA_LEN RX_LEN value along the way.
Once all the data has been received, the bus controller will raise an
interrupt indicating a packet command was completed, the slave address
matched, a normal stop condition was seen, and the transfer was an RX
operation.
If the master sent a NACK instead of a normal stop condition, or the
transfer timed out, then a slightly different set of interrupt status
values would be set. Those conditions are not handled in this commit.
The Zephyr firmware then collects data from the RX DMA buffer and clears
the status register by writing the PKT_MODE_EN bit to the status
register. In packet mode, clearing the packet mode interrupt enable bit
also clears most of the other interrupt bits automatically (except for a
few bits above it).
Note: if the master transmit or receive functions were in use
simultaneously with the slave mode receive functionality, then the
master mode functions may have raised the interrupt line for the bus
before the DMA slave transfer is complete. It's important to have the
slave's interrupt status register clear throughout the receive
operation, and if the slave attempts to raise the interrupt before the
master interrupt status is cleared, then it needs to re-raise the
interrupt once the master interrupt status is cleared. (And vice-versa).
That's why in this commit, when the master interrupt status is cleared
and the interrupt line is lowered, we call the slave interrupt _raise_
function, to see if the interrupt was pending. (And again, vice-versa).
Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
hw/i2c/aspeed_i2c.c | 133 ++++++++++++++++++++++++++++++++----
include/hw/i2c/aspeed_i2c.h | 3 +
2 files changed, 124 insertions(+), 12 deletions(-)
diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 8a8514586f..fc8b6b62cf 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -78,6 +78,18 @@ static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
}
}
+static inline void aspeed_i2c_bus_raise_slave_interrupt(AspeedI2CBus *bus)
+{
+ AspeedI2CClass *aic = ASPEED_I2C_GET_CLASS(bus->controller);
+
+ if (!bus->regs[R_I2CS_INTR_STS]) {
+ return;
+ }
+
+ bus->controller->intr_status |= 1 << bus->id;
+ qemu_irq_raise(aic->bus_get_irq(bus));
+}
+
static uint64_t aspeed_i2c_bus_old_read(AspeedI2CBus *bus, hwaddr offset,
unsigned size)
{
@@ -140,8 +152,17 @@ static uint64_t aspeed_i2c_bus_new_read(AspeedI2CBus *bus, hwaddr offset,
case A_I2CM_DMA_LEN_STS:
case A_I2CC_DMA_ADDR:
case A_I2CC_DMA_LEN:
+
+ case A_I2CS_DEV_ADDR:
+ case A_I2CS_DMA_RX_ADDR:
+ case A_I2CS_DMA_LEN:
+ case A_I2CS_CMD:
+ case A_I2CS_INTR_CTRL:
+ case A_I2CS_DMA_LEN_STS:
/* Value is already set, don't do anything. */
break;
+ case A_I2CS_INTR_STS:
+ break;
case A_I2CM_CMD:
value = SHARED_FIELD_DP32(value, BUS_BUSY_STS, i2c_bus_busy(bus->bus));
break;
@@ -547,12 +568,7 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
switch (offset) {
case A_I2CC_FUN_CTRL:
- if (SHARED_FIELD_EX32(value, SLAVE_EN)) {
- qemu_log_mask(LOG_UNIMP, "%s: slave mode not implemented\n",
- __func__);
- break;
- }
- bus->regs[R_I2CC_FUN_CTRL] = value & 0x007dc3ff;
+ bus->regs[R_I2CC_FUN_CTRL] = value;
break;
case A_I2CC_AC_TIMING:
bus->regs[R_I2CC_AC_TIMING] = value & 0x1ffff0ff;
@@ -580,6 +596,7 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
bus->controller->intr_status &= ~(1 << bus->id);
qemu_irq_lower(aic->bus_get_irq(bus));
}
+ aspeed_i2c_bus_raise_slave_interrupt(bus);
break;
}
bus->regs[R_I2CM_INTR_STS] &= ~(value & 0xf007f07f);
@@ -668,15 +685,53 @@ static void aspeed_i2c_bus_new_write(AspeedI2CBus *bus, hwaddr offset,
case A_I2CC_DMA_LEN:
/* RO */
break;
- case A_I2CS_DMA_LEN_STS:
- case A_I2CS_DMA_TX_ADDR:
- case A_I2CS_DMA_RX_ADDR:
case A_I2CS_DEV_ADDR:
+ bus->regs[R_I2CS_DEV_ADDR] = value;
+ break;
+ case A_I2CS_DMA_RX_ADDR:
+ bus->regs[R_I2CS_DMA_RX_ADDR] = value;
+ break;
+ case A_I2CS_DMA_LEN:
+ assert(FIELD_EX32(value, I2CS_DMA_LEN, TX_BUF_LEN) == 0);
+ if (FIELD_EX32(value, I2CS_DMA_LEN, RX_BUF_LEN_W1T)) {
+ ARRAY_FIELD_DP32(bus->regs, I2CS_DMA_LEN, RX_BUF_LEN,
+ FIELD_EX32(value, I2CS_DMA_LEN, RX_BUF_LEN));
+ } else {
+ bus->regs[R_I2CS_DMA_LEN] = value;
+ }
+ break;
+ case A_I2CS_CMD:
+ if (FIELD_EX32(value, I2CS_CMD, W1_CTRL)) {
+ bus->regs[R_I2CS_CMD] |= value;
+ } else {
+ bus->regs[R_I2CS_CMD] = value;
+ }
+ i2c_slave_set_address(bus->slave, bus->regs[R_I2CS_DEV_ADDR]);
+ break;
case A_I2CS_INTR_CTRL:
+ bus->regs[R_I2CS_INTR_CTRL] = value;
+ break;
+
case A_I2CS_INTR_STS:
- case A_I2CS_CMD:
- case A_I2CS_DMA_LEN:
- qemu_log_mask(LOG_UNIMP, "%s: Slave mode is not implemented\n",
+ if (ARRAY_FIELD_EX32(bus->regs, I2CS_INTR_CTRL, PKT_CMD_DONE)) {
+ if (ARRAY_FIELD_EX32(bus->regs, I2CS_INTR_STS, PKT_CMD_DONE) &&
+ FIELD_EX32(value, I2CS_INTR_STS, PKT_CMD_DONE)) {
+ bus->regs[R_I2CS_INTR_STS] &= 0xfffc0000;
+ }
+ } else {
+ bus->regs[R_I2CS_INTR_STS] &= ~value;
+ }
+ if (!bus->regs[R_I2CS_INTR_STS]) {
+ bus->controller->intr_status &= ~(1 << bus->id);
+ qemu_irq_lower(aic->bus_get_irq(bus));
+ }
+ aspeed_i2c_bus_raise_interrupt(bus);
+ break;
+ case A_I2CS_DMA_LEN_STS:
+ bus->regs[R_I2CS_DMA_LEN_STS] = 0;
+ break;
+ case A_I2CS_DMA_TX_ADDR:
+ qemu_log_mask(LOG_UNIMP, "%s: Slave mode DMA TX is not implemented\n",
__func__);
break;
default:
@@ -1037,6 +1092,39 @@ static const TypeInfo aspeed_i2c_info = {
.abstract = true,
};
+static int aspeed_i2c_bus_new_slave_event(AspeedI2CBus *bus,
+ enum i2c_event event)
+{
+ switch (event) {
+ case I2C_START_SEND_ASYNC:
+ if (!SHARED_ARRAY_FIELD_EX32(bus->regs, R_I2CS_CMD, RX_DMA_EN)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Slave mode RX DMA is not enabled\n", __func__);
+ return -1;
+ }
+ ARRAY_FIELD_DP32(bus->regs, I2CS_DMA_LEN_STS, RX_LEN, 0);
+ bus->regs[R_I2CC_DMA_ADDR] =
+ ARRAY_FIELD_EX32(bus->regs, I2CS_DMA_RX_ADDR, ADDR);
+ bus->regs[R_I2CC_DMA_LEN] =
+ ARRAY_FIELD_EX32(bus->regs, I2CS_DMA_LEN, RX_BUF_LEN) + 1;
+ i2c_ack(bus->bus);
+ break;
+ case I2C_FINISH:
+ ARRAY_FIELD_DP32(bus->regs, I2CS_INTR_STS, PKT_CMD_DONE, 1);
+ ARRAY_FIELD_DP32(bus->regs, I2CS_INTR_STS, SLAVE_ADDR_RX_MATCH, 1);
+ SHARED_ARRAY_FIELD_DP32(bus->regs, R_I2CS_INTR_STS, NORMAL_STOP, 1);
+ SHARED_ARRAY_FIELD_DP32(bus->regs, R_I2CS_INTR_STS, RX_DONE, 1);
+ aspeed_i2c_bus_raise_slave_interrupt(bus);
+ break;
+ default:
+ qemu_log_mask(LOG_UNIMP, "%s: i2c event %d unimplemented\n",
+ __func__, event);
+ return -1;
+ }
+
+ return 0;
+}
+
static int aspeed_i2c_bus_slave_event(I2CSlave *slave, enum i2c_event event)
{
BusState *qbus = qdev_get_parent_bus(DEVICE(slave));
@@ -1045,6 +1133,10 @@ static int aspeed_i2c_bus_slave_event(I2CSlave *slave, enum i2c_event event)
uint32_t reg_byte_buf = aspeed_i2c_bus_byte_buf_offset(bus);
uint32_t value;
+ if (aspeed_i2c_is_new_mode(bus->controller)) {
+ return aspeed_i2c_bus_new_slave_event(bus, event);
+ }
+
switch (event) {
case I2C_START_SEND_ASYNC:
value = SHARED_ARRAY_FIELD_EX32(bus->regs, reg_byte_buf, TX_BUF);
@@ -1073,6 +1165,19 @@ static int aspeed_i2c_bus_slave_event(I2CSlave *slave, enum i2c_event event)
return 0;
}
+static void aspeed_i2c_bus_new_slave_send_async(AspeedI2CBus *bus, uint8_t data)
+{
+ assert(address_space_write(&bus->controller->dram_as,
+ bus->regs[R_I2CC_DMA_ADDR],
+ MEMTXATTRS_UNSPECIFIED, &data, 1) == MEMTX_OK);
+
+ bus->regs[R_I2CC_DMA_ADDR]++;
+ bus->regs[R_I2CC_DMA_LEN]--;
+ ARRAY_FIELD_DP32(bus->regs, I2CS_DMA_LEN_STS, RX_LEN,
+ ARRAY_FIELD_EX32(bus->regs, I2CS_DMA_LEN_STS, RX_LEN) + 1);
+ i2c_ack(bus->bus);
+}
+
static void aspeed_i2c_bus_slave_send_async(I2CSlave *slave, uint8_t data)
{
BusState *qbus = qdev_get_parent_bus(DEVICE(slave));
@@ -1080,6 +1185,10 @@ static void aspeed_i2c_bus_slave_send_async(I2CSlave *slave, uint8_t data)
uint32_t reg_intr_sts = aspeed_i2c_bus_intr_sts_offset(bus);
uint32_t reg_byte_buf = aspeed_i2c_bus_byte_buf_offset(bus);
+ if (aspeed_i2c_is_new_mode(bus->controller)) {
+ return aspeed_i2c_bus_new_slave_send_async(bus, data);
+ }
+
SHARED_ARRAY_FIELD_DP32(bus->regs, reg_byte_buf, RX_BUF, data);
SHARED_ARRAY_FIELD_DP32(bus->regs, reg_intr_sts, RX_DONE, 1);
diff --git a/include/hw/i2c/aspeed_i2c.h b/include/hw/i2c/aspeed_i2c.h
index ba148b2f6d..300a89b343 100644
--- a/include/hw/i2c/aspeed_i2c.h
+++ b/include/hw/i2c/aspeed_i2c.h
@@ -174,6 +174,8 @@ REG32(I2CM_DMA_LEN, 0x1c)
FIELD(I2CM_DMA_LEN, TX_BUF_LEN_W1T, 15, 1)
FIELD(I2CM_DMA_LEN, TX_BUF_LEN, 0, 11)
REG32(I2CS_INTR_CTRL, 0x20)
+ FIELD(I2CS_INTR_CTRL, PKT_CMD_FAIL, 17, 1)
+ FIELD(I2CS_INTR_CTRL, PKT_CMD_DONE, 16, 1)
REG32(I2CS_INTR_STS, 0x24)
/* 31:29 shared with I2CD_INTR_STS[31:29] */
FIELD(I2CS_INTR_STS, SLAVE_PARKING_STS, 24, 2)
@@ -184,6 +186,7 @@ REG32(I2CS_INTR_STS, 0x24)
FIELD(I2CS_INTR_STS, PKT_CMD_FAIL, 17, 1)
FIELD(I2CS_INTR_STS, PKT_CMD_DONE, 16, 1)
/* 14:0 shared with I2CD_INTR_STS[14:0] */
+ FIELD(I2CS_INTR_STS, SLAVE_ADDR_RX_MATCH, 7, 1)
REG32(I2CS_CMD, 0x28)
FIELD(I2CS_CMD, W1_CTRL, 31, 1)
FIELD(I2CS_CMD, PKT_MODE_ACTIVE_ADDR, 17, 2)
--
2.37.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* [PATCH v3 08/14] hw/i2c/pmbus: Add idle state to return 0xff's
2022-06-30 4:51 [PATCH v3 00/14] hw/i2c/aspeed: I2C slave mode DMA RX w/ new regs Peter Delevoryas
` (6 preceding siblings ...)
2022-06-30 4:51 ` [PATCH v3 07/14] hw/i2c/aspeed: Add new-registers DMA slave mode RX support Peter Delevoryas
@ 2022-06-30 4:51 ` Peter Delevoryas
2022-06-30 19:18 ` Titus Rwantare
2022-06-30 4:51 ` [PATCH v3 09/14] hw/sensor: Add IC_DEVICE_ID to ISL voltage regulators Peter Delevoryas
` (7 subsequent siblings)
15 siblings, 1 reply; 34+ messages in thread
From: Peter Delevoryas @ 2022-06-30 4:51 UTC (permalink / raw)
Cc: clg, peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel, pdel
From: Peter Delevoryas <pdel@fb.com>
Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
hw/i2c/pmbus_device.c | 9 +++++++++
include/hw/i2c/pmbus_device.h | 7 +++++++
2 files changed, 16 insertions(+)
diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
index 62885fa6a1..f89fea65f3 100644
--- a/hw/i2c/pmbus_device.c
+++ b/hw/i2c/pmbus_device.c
@@ -261,6 +261,11 @@ void pmbus_check_limits(PMBusDevice *pmdev)
}
}
+void pmbus_idle(PMBusDevice *pmdev)
+{
+ pmdev->code = PMBUS_IDLE_STATE;
+}
+
/* assert the status_cml error upon receipt of malformed command */
static void pmbus_cml_error(PMBusDevice *pmdev)
{
@@ -984,6 +989,10 @@ static uint8_t pmbus_receive_byte(SMBusDevice *smd)
}
break;
+ case PMBUS_IDLE_STATE:
+ pmbus_send8(pmdev, PMBUS_ERR_BYTE);
+ break;
+
case PMBUS_CLEAR_FAULTS: /* Send Byte */
case PMBUS_PAGE_PLUS_WRITE: /* Block Write-only */
case PMBUS_STORE_DEFAULT_ALL: /* Send Byte */
diff --git a/include/hw/i2c/pmbus_device.h b/include/hw/i2c/pmbus_device.h
index 0f4d6b3fad..93f5d57c9d 100644
--- a/include/hw/i2c/pmbus_device.h
+++ b/include/hw/i2c/pmbus_device.h
@@ -155,6 +155,7 @@ enum pmbus_registers {
PMBUS_MFR_MAX_TEMP_1 = 0xC0, /* R/W word */
PMBUS_MFR_MAX_TEMP_2 = 0xC1, /* R/W word */
PMBUS_MFR_MAX_TEMP_3 = 0xC2, /* R/W word */
+ PMBUS_IDLE_STATE = 0xFF,
};
/* STATUS_WORD */
@@ -527,6 +528,12 @@ int pmbus_page_config(PMBusDevice *pmdev, uint8_t page_index, uint64_t flags);
*/
void pmbus_check_limits(PMBusDevice *pmdev);
+/**
+ * Enter an idle state where only the PMBUS_ERR_BYTE will be returned
+ * indefinitely until a new command is issued.
+ */
+void pmbus_idle(PMBusDevice *pmdev);
+
extern const VMStateDescription vmstate_pmbus_device;
#define VMSTATE_PMBUS_DEVICE(_field, _state) { \
--
2.37.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v3 08/14] hw/i2c/pmbus: Add idle state to return 0xff's
2022-06-30 4:51 ` [PATCH v3 08/14] hw/i2c/pmbus: Add idle state to return 0xff's Peter Delevoryas
@ 2022-06-30 19:18 ` Titus Rwantare
0 siblings, 0 replies; 34+ messages in thread
From: Titus Rwantare @ 2022-06-30 19:18 UTC (permalink / raw)
To: me
Cc: clg, peter.maydell, andrew, joel, cminyard, qemu-devel, qemu-arm,
zhdaniel, pdel
On Wed, 29 Jun 2022 at 21:52, Peter Delevoryas <me@pjd.dev> wrote:
>
> From: Peter Delevoryas <pdel@fb.com>
>
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> ---
> hw/i2c/pmbus_device.c | 9 +++++++++
> include/hw/i2c/pmbus_device.h | 7 +++++++
> 2 files changed, 16 insertions(+)
>
> diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
> index 62885fa6a1..f89fea65f3 100644
> --- a/hw/i2c/pmbus_device.c
> +++ b/hw/i2c/pmbus_device.c
> @@ -261,6 +261,11 @@ void pmbus_check_limits(PMBusDevice *pmdev)
> }
> }
>
> +void pmbus_idle(PMBusDevice *pmdev)
> +{
> + pmdev->code = PMBUS_IDLE_STATE;
> +}
> +
> /* assert the status_cml error upon receipt of malformed command */
> static void pmbus_cml_error(PMBusDevice *pmdev)
> {
> @@ -984,6 +989,10 @@ static uint8_t pmbus_receive_byte(SMBusDevice *smd)
> }
> break;
>
> + case PMBUS_IDLE_STATE:
> + pmbus_send8(pmdev, PMBUS_ERR_BYTE);
> + break;
> +
> case PMBUS_CLEAR_FAULTS: /* Send Byte */
> case PMBUS_PAGE_PLUS_WRITE: /* Block Write-only */
> case PMBUS_STORE_DEFAULT_ALL: /* Send Byte */
> diff --git a/include/hw/i2c/pmbus_device.h b/include/hw/i2c/pmbus_device.h
> index 0f4d6b3fad..93f5d57c9d 100644
> --- a/include/hw/i2c/pmbus_device.h
> +++ b/include/hw/i2c/pmbus_device.h
> @@ -155,6 +155,7 @@ enum pmbus_registers {
> PMBUS_MFR_MAX_TEMP_1 = 0xC0, /* R/W word */
> PMBUS_MFR_MAX_TEMP_2 = 0xC1, /* R/W word */
> PMBUS_MFR_MAX_TEMP_3 = 0xC2, /* R/W word */
> + PMBUS_IDLE_STATE = 0xFF,
> };
>
> /* STATUS_WORD */
> @@ -527,6 +528,12 @@ int pmbus_page_config(PMBusDevice *pmdev, uint8_t page_index, uint64_t flags);
> */
> void pmbus_check_limits(PMBusDevice *pmdev);
>
> +/**
> + * Enter an idle state where only the PMBUS_ERR_BYTE will be returned
> + * indefinitely until a new command is issued.
> + */
> +void pmbus_idle(PMBusDevice *pmdev);
> +
> extern const VMStateDescription vmstate_pmbus_device;
>
> #define VMSTATE_PMBUS_DEVICE(_field, _state) { \
> --
> 2.37.0
>
Reviewed-by: Titus Rwantare <titusr@google.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v3 09/14] hw/sensor: Add IC_DEVICE_ID to ISL voltage regulators
2022-06-30 4:51 [PATCH v3 00/14] hw/i2c/aspeed: I2C slave mode DMA RX w/ new regs Peter Delevoryas
` (7 preceding siblings ...)
2022-06-30 4:51 ` [PATCH v3 08/14] hw/i2c/pmbus: Add idle state to return 0xff's Peter Delevoryas
@ 2022-06-30 4:51 ` Peter Delevoryas
2022-06-30 19:20 ` Titus Rwantare
2022-06-30 4:51 ` [PATCH v3 10/14] hw/sensor: Add Renesas ISL69259 device model Peter Delevoryas
` (6 subsequent siblings)
15 siblings, 1 reply; 34+ messages in thread
From: Peter Delevoryas @ 2022-06-30 4:51 UTC (permalink / raw)
Cc: clg, peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel, pdel
From: Peter Delevoryas <pdel@fb.com>
This commit adds a passthrough for PMBUS_IC_DEVICE_ID to allow Renesas
voltage regulators to return the integrated circuit device ID if they
would like to.
The behavior is very device specific, so it hasn't been added to the
general PMBUS model. Additionally, if the device ID hasn't been set,
then the voltage regulator will respond with the error byte value. The
guest error message will change slightly for IC_DEVICE_ID with this
commit.
Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
hw/sensor/isl_pmbus_vr.c | 12 ++++++++++++
include/hw/sensor/isl_pmbus_vr.h | 5 +++++
2 files changed, 17 insertions(+)
diff --git a/hw/sensor/isl_pmbus_vr.c b/hw/sensor/isl_pmbus_vr.c
index e11e028884..799ea9d89e 100644
--- a/hw/sensor/isl_pmbus_vr.c
+++ b/hw/sensor/isl_pmbus_vr.c
@@ -15,6 +15,18 @@
static uint8_t isl_pmbus_vr_read_byte(PMBusDevice *pmdev)
{
+ ISLState *s = ISL69260(pmdev);
+
+ switch (pmdev->code) {
+ case PMBUS_IC_DEVICE_ID:
+ if (!s->ic_device_id_len) {
+ break;
+ }
+ pmbus_send(pmdev, s->ic_device_id, s->ic_device_id_len);
+ pmbus_idle(pmdev);
+ return 0;
+ }
+
qemu_log_mask(LOG_GUEST_ERROR,
"%s: reading from unsupported register: 0x%02x\n",
__func__, pmdev->code);
diff --git a/include/hw/sensor/isl_pmbus_vr.h b/include/hw/sensor/isl_pmbus_vr.h
index 3e47ff7e48..aa2c2767df 100644
--- a/include/hw/sensor/isl_pmbus_vr.h
+++ b/include/hw/sensor/isl_pmbus_vr.h
@@ -12,12 +12,17 @@
#include "hw/i2c/pmbus_device.h"
#include "qom/object.h"
+#define TYPE_ISL69259 "isl69259"
#define TYPE_ISL69260 "isl69260"
#define TYPE_RAA228000 "raa228000"
#define TYPE_RAA229004 "raa229004"
+#define ISL_MAX_IC_DEVICE_ID_LEN 16
struct ISLState {
PMBusDevice parent;
+
+ uint8_t ic_device_id[ISL_MAX_IC_DEVICE_ID_LEN];
+ uint8_t ic_device_id_len;
};
OBJECT_DECLARE_SIMPLE_TYPE(ISLState, ISL69260)
--
2.37.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v3 09/14] hw/sensor: Add IC_DEVICE_ID to ISL voltage regulators
2022-06-30 4:51 ` [PATCH v3 09/14] hw/sensor: Add IC_DEVICE_ID to ISL voltage regulators Peter Delevoryas
@ 2022-06-30 19:20 ` Titus Rwantare
0 siblings, 0 replies; 34+ messages in thread
From: Titus Rwantare @ 2022-06-30 19:20 UTC (permalink / raw)
To: me
Cc: clg, peter.maydell, andrew, joel, cminyard, qemu-devel, qemu-arm,
zhdaniel, pdel
On Wed, 29 Jun 2022 at 21:52, Peter Delevoryas <me@pjd.dev> wrote:
>
> From: Peter Delevoryas <pdel@fb.com>
>
> This commit adds a passthrough for PMBUS_IC_DEVICE_ID to allow Renesas
> voltage regulators to return the integrated circuit device ID if they
> would like to.
>
> The behavior is very device specific, so it hasn't been added to the
> general PMBUS model. Additionally, if the device ID hasn't been set,
> then the voltage regulator will respond with the error byte value. The
> guest error message will change slightly for IC_DEVICE_ID with this
> commit.
>
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> ---
> hw/sensor/isl_pmbus_vr.c | 12 ++++++++++++
> include/hw/sensor/isl_pmbus_vr.h | 5 +++++
> 2 files changed, 17 insertions(+)
>
> diff --git a/hw/sensor/isl_pmbus_vr.c b/hw/sensor/isl_pmbus_vr.c
> index e11e028884..799ea9d89e 100644
> --- a/hw/sensor/isl_pmbus_vr.c
> +++ b/hw/sensor/isl_pmbus_vr.c
> @@ -15,6 +15,18 @@
>
> static uint8_t isl_pmbus_vr_read_byte(PMBusDevice *pmdev)
> {
> + ISLState *s = ISL69260(pmdev);
> +
> + switch (pmdev->code) {
> + case PMBUS_IC_DEVICE_ID:
> + if (!s->ic_device_id_len) {
> + break;
> + }
> + pmbus_send(pmdev, s->ic_device_id, s->ic_device_id_len);
> + pmbus_idle(pmdev);
> + return 0;
> + }
> +
> qemu_log_mask(LOG_GUEST_ERROR,
> "%s: reading from unsupported register: 0x%02x\n",
> __func__, pmdev->code);
> diff --git a/include/hw/sensor/isl_pmbus_vr.h b/include/hw/sensor/isl_pmbus_vr.h
> index 3e47ff7e48..aa2c2767df 100644
> --- a/include/hw/sensor/isl_pmbus_vr.h
> +++ b/include/hw/sensor/isl_pmbus_vr.h
> @@ -12,12 +12,17 @@
> #include "hw/i2c/pmbus_device.h"
> #include "qom/object.h"
>
> +#define TYPE_ISL69259 "isl69259"
> #define TYPE_ISL69260 "isl69260"
> #define TYPE_RAA228000 "raa228000"
> #define TYPE_RAA229004 "raa229004"
> +#define ISL_MAX_IC_DEVICE_ID_LEN 16
>
> struct ISLState {
> PMBusDevice parent;
> +
> + uint8_t ic_device_id[ISL_MAX_IC_DEVICE_ID_LEN];
> + uint8_t ic_device_id_len;
> };
>
> OBJECT_DECLARE_SIMPLE_TYPE(ISLState, ISL69260)
> --
> 2.37.0
>
Reviewed-by: Titus Rwantare <titusr@google.com>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v3 10/14] hw/sensor: Add Renesas ISL69259 device model
2022-06-30 4:51 [PATCH v3 00/14] hw/i2c/aspeed: I2C slave mode DMA RX w/ new regs Peter Delevoryas
` (8 preceding siblings ...)
2022-06-30 4:51 ` [PATCH v3 09/14] hw/sensor: Add IC_DEVICE_ID to ISL voltage regulators Peter Delevoryas
@ 2022-06-30 4:51 ` Peter Delevoryas
2022-06-30 6:30 ` Cédric Le Goater
2022-06-30 19:16 ` Titus Rwantare
2022-06-30 4:51 ` [PATCH v3 11/14] hw/misc/aspeed: Add PECI controller Peter Delevoryas
` (5 subsequent siblings)
15 siblings, 2 replies; 34+ messages in thread
From: Peter Delevoryas @ 2022-06-30 4:51 UTC (permalink / raw)
Cc: clg, peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel, pdel
From: Peter Delevoryas <pdel@fb.com>
This adds the ISL69259, using all the same functionality as the existing
ISL69260 but overriding the IC_DEVICE_ID.
Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
hw/sensor/isl_pmbus_vr.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/hw/sensor/isl_pmbus_vr.c b/hw/sensor/isl_pmbus_vr.c
index 799ea9d89e..853d70536f 100644
--- a/hw/sensor/isl_pmbus_vr.c
+++ b/hw/sensor/isl_pmbus_vr.c
@@ -119,6 +119,18 @@ static void raa228000_exit_reset(Object *obj)
pmdev->pages[0].read_temperature_3 = 0;
}
+static void isl69259_exit_reset(Object *obj)
+{
+ ISLState *s = ISL69260(obj);
+ static const uint8_t ic_device_id[] = {0x04, 0x00, 0x81, 0xD2, 0x49, 0x3c};
+ g_assert_cmphex(sizeof(ic_device_id), <=, sizeof(s->ic_device_id));
+
+ isl_pmbus_vr_exit_reset(obj);
+
+ s->ic_device_id_len = sizeof(ic_device_id);
+ memcpy(s->ic_device_id, ic_device_id, sizeof(ic_device_id));
+}
+
static void isl_pmbus_vr_add_props(Object *obj, uint64_t *flags, uint8_t pages)
{
PMBusDevice *pmdev = PMBUS_DEVICE(obj);
@@ -257,6 +269,21 @@ static void raa229004_class_init(ObjectClass *klass, void *data)
isl_pmbus_vr_class_init(klass, data, 2);
}
+static void isl69259_class_init(ObjectClass *klass, void *data)
+{
+ ResettableClass *rc = RESETTABLE_CLASS(klass);
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ dc->desc = "Renesas ISL69259 Digital Multiphase Voltage Regulator";
+ rc->phases.exit = isl69259_exit_reset;
+ isl_pmbus_vr_class_init(klass, data, 2);
+}
+
+static const TypeInfo isl69259_info = {
+ .name = TYPE_ISL69259,
+ .parent = TYPE_ISL69260,
+ .class_init = isl69259_class_init,
+};
+
static const TypeInfo isl69260_info = {
.name = TYPE_ISL69260,
.parent = TYPE_PMBUS_DEVICE,
@@ -283,6 +310,7 @@ static const TypeInfo raa228000_info = {
static void isl_pmbus_vr_register_types(void)
{
+ type_register_static(&isl69259_info);
type_register_static(&isl69260_info);
type_register_static(&raa228000_info);
type_register_static(&raa229004_info);
--
2.37.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v3 10/14] hw/sensor: Add Renesas ISL69259 device model
2022-06-30 4:51 ` [PATCH v3 10/14] hw/sensor: Add Renesas ISL69259 device model Peter Delevoryas
@ 2022-06-30 6:30 ` Cédric Le Goater
2022-06-30 19:16 ` Titus Rwantare
2022-06-30 19:16 ` Titus Rwantare
1 sibling, 1 reply; 34+ messages in thread
From: Cédric Le Goater @ 2022-06-30 6:30 UTC (permalink / raw)
To: Peter Delevoryas
Cc: peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel, pdel
On 6/30/22 06:51, Peter Delevoryas wrote:
> From: Peter Delevoryas <pdel@fb.com>
>
> This adds the ISL69259, using all the same functionality as the existing
> ISL69260 but overriding the IC_DEVICE_ID.
>
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> ---
> hw/sensor/isl_pmbus_vr.c | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/hw/sensor/isl_pmbus_vr.c b/hw/sensor/isl_pmbus_vr.c
> index 799ea9d89e..853d70536f 100644
> --- a/hw/sensor/isl_pmbus_vr.c
> +++ b/hw/sensor/isl_pmbus_vr.c
> @@ -119,6 +119,18 @@ static void raa228000_exit_reset(Object *obj)
> pmdev->pages[0].read_temperature_3 = 0;
> }
>
> +static void isl69259_exit_reset(Object *obj)
> +{
> + ISLState *s = ISL69260(obj);
> + static const uint8_t ic_device_id[] = {0x04, 0x00, 0x81, 0xD2, 0x49, 0x3c};
This looks like an ISLClass attribute to me. In which case, you wouldn't need the
reset handler nor the 'ic_device_id_len' field.
Thanks,
C.
> + g_assert_cmphex(sizeof(ic_device_id), <=, sizeof(s->ic_device_id));
> +
> + isl_pmbus_vr_exit_reset(obj);
> +
> + s->ic_device_id_len = sizeof(ic_device_id);
> + memcpy(s->ic_device_id, ic_device_id, sizeof(ic_device_id));
> +}
> +
> static void isl_pmbus_vr_add_props(Object *obj, uint64_t *flags, uint8_t pages)
> {
> PMBusDevice *pmdev = PMBUS_DEVICE(obj);
> @@ -257,6 +269,21 @@ static void raa229004_class_init(ObjectClass *klass, void *data)
> isl_pmbus_vr_class_init(klass, data, 2);
> }
>
> +static void isl69259_class_init(ObjectClass *klass, void *data)
> +{
> + ResettableClass *rc = RESETTABLE_CLASS(klass);
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + dc->desc = "Renesas ISL69259 Digital Multiphase Voltage Regulator";
> + rc->phases.exit = isl69259_exit_reset;
> + isl_pmbus_vr_class_init(klass, data, 2);
> +}
> +
> +static const TypeInfo isl69259_info = {
> + .name = TYPE_ISL69259,
> + .parent = TYPE_ISL69260,
> + .class_init = isl69259_class_init,
> +};
> +
> static const TypeInfo isl69260_info = {
> .name = TYPE_ISL69260,
> .parent = TYPE_PMBUS_DEVICE,
> @@ -283,6 +310,7 @@ static const TypeInfo raa228000_info = {
>
> static void isl_pmbus_vr_register_types(void)
> {
> + type_register_static(&isl69259_info);
> type_register_static(&isl69260_info);
> type_register_static(&raa228000_info);
> type_register_static(&raa229004_info);
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 10/14] hw/sensor: Add Renesas ISL69259 device model
2022-06-30 6:30 ` Cédric Le Goater
@ 2022-06-30 19:16 ` Titus Rwantare
2022-07-01 5:35 ` Cédric Le Goater
0 siblings, 1 reply; 34+ messages in thread
From: Titus Rwantare @ 2022-06-30 19:16 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Peter Delevoryas, peter.maydell, andrew, joel, cminyard,
qemu-devel, qemu-arm, zhdaniel, pdel
On Wed, 29 Jun 2022 at 23:30, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 6/30/22 06:51, Peter Delevoryas wrote:
> > From: Peter Delevoryas <pdel@fb.com>
> >
> > This adds the ISL69259, using all the same functionality as the existing
> > ISL69260 but overriding the IC_DEVICE_ID.
> >
> > Signed-off-by: Peter Delevoryas <pdel@fb.com>
> > ---
> > hw/sensor/isl_pmbus_vr.c | 28 ++++++++++++++++++++++++++++
> > 1 file changed, 28 insertions(+)
> >
> > diff --git a/hw/sensor/isl_pmbus_vr.c b/hw/sensor/isl_pmbus_vr.c
> > index 799ea9d89e..853d70536f 100644
> > --- a/hw/sensor/isl_pmbus_vr.c
> > +++ b/hw/sensor/isl_pmbus_vr.c
> > @@ -119,6 +119,18 @@ static void raa228000_exit_reset(Object *obj)
> > pmdev->pages[0].read_temperature_3 = 0;
> > }
> >
> > +static void isl69259_exit_reset(Object *obj)
> > +{
> > + ISLState *s = ISL69260(obj);
> > + static const uint8_t ic_device_id[] = {0x04, 0x00, 0x81, 0xD2, 0x49, 0x3c};
>
> This looks like an ISLClass attribute to me. In which case, you wouldn't need the
> reset handler nor the 'ic_device_id_len' field.
>
> Thanks,
>
> C.
I asked for this because, so far, I've been doing all the register
defaults in reset handlers, including read-only registers.
I don't mind either way, but it seemed preferable to have the devices
consistent.
Titus
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 10/14] hw/sensor: Add Renesas ISL69259 device model
2022-06-30 19:16 ` Titus Rwantare
@ 2022-07-01 5:35 ` Cédric Le Goater
0 siblings, 0 replies; 34+ messages in thread
From: Cédric Le Goater @ 2022-07-01 5:35 UTC (permalink / raw)
To: Titus Rwantare
Cc: Peter Delevoryas, peter.maydell, andrew, joel, cminyard,
qemu-devel, qemu-arm, zhdaniel, pdel
On 6/30/22 21:16, Titus Rwantare wrote:
> On Wed, 29 Jun 2022 at 23:30, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> On 6/30/22 06:51, Peter Delevoryas wrote:
>>> From: Peter Delevoryas <pdel@fb.com>
>>>
>>> This adds the ISL69259, using all the same functionality as the existing
>>> ISL69260 but overriding the IC_DEVICE_ID.
>>>
>>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>>> ---
>>> hw/sensor/isl_pmbus_vr.c | 28 ++++++++++++++++++++++++++++
>>> 1 file changed, 28 insertions(+)
>>>
>>> diff --git a/hw/sensor/isl_pmbus_vr.c b/hw/sensor/isl_pmbus_vr.c
>>> index 799ea9d89e..853d70536f 100644
>>> --- a/hw/sensor/isl_pmbus_vr.c
>>> +++ b/hw/sensor/isl_pmbus_vr.c
>>> @@ -119,6 +119,18 @@ static void raa228000_exit_reset(Object *obj)
>>> pmdev->pages[0].read_temperature_3 = 0;
>>> }
>>>
>>> +static void isl69259_exit_reset(Object *obj)
>>> +{
>>> + ISLState *s = ISL69260(obj);
>>> + static const uint8_t ic_device_id[] = {0x04, 0x00, 0x81, 0xD2, 0x49, 0x3c};
>>
>> This looks like an ISLClass attribute to me. In which case, you wouldn't need the
>> reset handler nor the 'ic_device_id_len' field.
>>
>> Thanks,
>>
>> C.
>
> I asked for this because, so far, I've been doing all the register
> defaults in reset handlers, including read-only registers.
> I don't mind either way, but it seemed preferable to have the devices
> consistent.
Sure. Fine for me.
Thanks,
C.
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 10/14] hw/sensor: Add Renesas ISL69259 device model
2022-06-30 4:51 ` [PATCH v3 10/14] hw/sensor: Add Renesas ISL69259 device model Peter Delevoryas
2022-06-30 6:30 ` Cédric Le Goater
@ 2022-06-30 19:16 ` Titus Rwantare
2022-06-30 21:14 ` Peter Delevoryas
1 sibling, 1 reply; 34+ messages in thread
From: Titus Rwantare @ 2022-06-30 19:16 UTC (permalink / raw)
To: me
Cc: clg, peter.maydell, andrew, joel, cminyard, qemu-devel, qemu-arm,
zhdaniel, pdel
On Wed, 29 Jun 2022 at 21:52, Peter Delevoryas <me@pjd.dev> wrote:
>
> From: Peter Delevoryas <pdel@fb.com>
>
> This adds the ISL69259, using all the same functionality as the existing
> ISL69260 but overriding the IC_DEVICE_ID.
>
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> ---
> hw/sensor/isl_pmbus_vr.c | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/hw/sensor/isl_pmbus_vr.c b/hw/sensor/isl_pmbus_vr.c
> index 799ea9d89e..853d70536f 100644
> --- a/hw/sensor/isl_pmbus_vr.c
> +++ b/hw/sensor/isl_pmbus_vr.c
> @@ -119,6 +119,18 @@ static void raa228000_exit_reset(Object *obj)
> pmdev->pages[0].read_temperature_3 = 0;
> }
>
> +static void isl69259_exit_reset(Object *obj)
> +{
> + ISLState *s = ISL69260(obj);
> + static const uint8_t ic_device_id[] = {0x04, 0x00, 0x81, 0xD2, 0x49, 0x3c};
> + g_assert_cmphex(sizeof(ic_device_id), <=, sizeof(s->ic_device_id));
> +
This generates an error from the checkpatch script:
Checking 0010-hw-sensor-Add-Renesas-ISL69259-device-model.patch...
ERROR: Use g_assert or g_assert_not_reached
#27: FILE: hw/sensor/isl_pmbus_vr.c:126:
+ g_assert_cmphex(sizeof(ic_device_id), <=, sizeof(s->ic_device_id));
otherwise, LGTM.
Titus
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 10/14] hw/sensor: Add Renesas ISL69259 device model
2022-06-30 19:16 ` Titus Rwantare
@ 2022-06-30 21:14 ` Peter Delevoryas
0 siblings, 0 replies; 34+ messages in thread
From: Peter Delevoryas @ 2022-06-30 21:14 UTC (permalink / raw)
To: Titus Rwantare
Cc: clg, peter.maydell, andrew, joel, cminyard, qemu-devel, qemu-arm,
zhdaniel, pdel
On Thu, Jun 30, 2022 at 12:16:05PM -0700, Titus Rwantare wrote:
> On Wed, 29 Jun 2022 at 21:52, Peter Delevoryas <me@pjd.dev> wrote:
> >
> > From: Peter Delevoryas <pdel@fb.com>
> >
> > This adds the ISL69259, using all the same functionality as the existing
> > ISL69260 but overriding the IC_DEVICE_ID.
> >
> > Signed-off-by: Peter Delevoryas <pdel@fb.com>
> > ---
> > hw/sensor/isl_pmbus_vr.c | 28 ++++++++++++++++++++++++++++
> > 1 file changed, 28 insertions(+)
> >
> > diff --git a/hw/sensor/isl_pmbus_vr.c b/hw/sensor/isl_pmbus_vr.c
> > index 799ea9d89e..853d70536f 100644
> > --- a/hw/sensor/isl_pmbus_vr.c
> > +++ b/hw/sensor/isl_pmbus_vr.c
> > @@ -119,6 +119,18 @@ static void raa228000_exit_reset(Object *obj)
> > pmdev->pages[0].read_temperature_3 = 0;
> > }
> >
> > +static void isl69259_exit_reset(Object *obj)
> > +{
> > + ISLState *s = ISL69260(obj);
> > + static const uint8_t ic_device_id[] = {0x04, 0x00, 0x81, 0xD2, 0x49, 0x3c};
> > + g_assert_cmphex(sizeof(ic_device_id), <=, sizeof(s->ic_device_id));
> > +
>
> This generates an error from the checkpatch script:
> Checking 0010-hw-sensor-Add-Renesas-ISL69259-device-model.patch...
> ERROR: Use g_assert or g_assert_not_reached
> #27: FILE: hw/sensor/isl_pmbus_vr.c:126:
> + g_assert_cmphex(sizeof(ic_device_id), <=, sizeof(s->ic_device_id));
Argghhh I should have caught this, thanks. I'll replace it with g_assert. I
didn't realize there was some kind of portability issue with using
g_assert_cmphex in non-test code.
>
> otherwise, LGTM.
That's great! Thanks for the review. I'll let you and Cedric sort
out if we want to make IC_DEVICE_ID a class property or keep it
in exit_reset as everything else class-specific is right now.
I'll still resubmit the patches as a separate series though with
the g_assert fix and your reviewed-by tags.
>
>
> Titus
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v3 11/14] hw/misc/aspeed: Add PECI controller
2022-06-30 4:51 [PATCH v3 00/14] hw/i2c/aspeed: I2C slave mode DMA RX w/ new regs Peter Delevoryas
` (9 preceding siblings ...)
2022-06-30 4:51 ` [PATCH v3 10/14] hw/sensor: Add Renesas ISL69259 device model Peter Delevoryas
@ 2022-06-30 4:51 ` Peter Delevoryas
2022-06-30 6:32 ` Cédric Le Goater
2022-06-30 4:51 ` [PATCH v3 12/14] hw/misc/aspeed: Add fby35-sb-cpld Peter Delevoryas
` (4 subsequent siblings)
15 siblings, 1 reply; 34+ messages in thread
From: Peter Delevoryas @ 2022-06-30 4:51 UTC (permalink / raw)
Cc: clg, peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel, pdel
From: Peter Delevoryas <pdel@fb.com>
This introduces a really basic PECI controller that responses to
commands by always setting the response code to success and then raising
an interrupt to indicate the command is done. This helps avoid getting
hit with constant errors if the driver continuously attempts to send a
command and keeps timing out.
The AST2400 and AST2500 only included registers up to 0x5C, not 0xFC.
They supported PECI 1.1, 2.0, and 3.0. The AST2600 and AST1030 support
PECI 4.0, which includes more read/write buffer registers from 0x80 to
0xFC to support 64-byte mode.
This patch doesn't attempt to handle that, or to create a different
version of the controller for the different generations, since it's only
implementing functionality that is common to all generations.
The basic sequence of events is that the firmware will read and write to
various registers and then trigger a command by setting the FIRE bit in
the command register (similar to the I2C controller).
Then the firmware waits for an interrupt from the PECI controller,
expecting the interrupt status register to be filled in with info on
what happened. If the command was transmitted and received successfully,
then response codes from the host CPU will be found in the data buffer
registers.
Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
hw/arm/aspeed_ast10x0.c | 12 +++
hw/arm/aspeed_ast2600.c | 12 +++
hw/arm/aspeed_soc.c | 13 +++
hw/misc/aspeed_peci.c | 152 ++++++++++++++++++++++++++++++++++
hw/misc/meson.build | 3 +-
hw/misc/trace-events | 5 ++
include/hw/arm/aspeed_soc.h | 3 +
include/hw/misc/aspeed_peci.h | 29 +++++++
8 files changed, 228 insertions(+), 1 deletion(-)
create mode 100644 hw/misc/aspeed_peci.c
create mode 100644 include/hw/misc/aspeed_peci.h
diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
index 5df480a21f..56e8de3d89 100644
--- a/hw/arm/aspeed_ast10x0.c
+++ b/hw/arm/aspeed_ast10x0.c
@@ -47,6 +47,7 @@ static const hwaddr aspeed_soc_ast1030_memmap[] = {
[ASPEED_DEV_UART13] = 0x7E790700,
[ASPEED_DEV_WDT] = 0x7E785000,
[ASPEED_DEV_LPC] = 0x7E789000,
+ [ASPEED_DEV_PECI] = 0x7E78B000,
[ASPEED_DEV_I2C] = 0x7E7B0000,
};
@@ -75,6 +76,7 @@ static const int aspeed_soc_ast1030_irqmap[] = {
[ASPEED_DEV_TIMER8] = 23,
[ASPEED_DEV_WDT] = 24,
[ASPEED_DEV_LPC] = 35,
+ [ASPEED_DEV_PECI] = 38,
[ASPEED_DEV_FMC] = 39,
[ASPEED_DEV_PWM] = 44,
[ASPEED_DEV_ADC] = 46,
@@ -133,6 +135,8 @@ static void aspeed_soc_ast1030_init(Object *obj)
object_initialize_child(obj, "lpc", &s->lpc, TYPE_ASPEED_LPC);
+ object_initialize_child(obj, "peci", &s->peci, TYPE_ASPEED_PECI);
+
object_initialize_child(obj, "sbc", &s->sbc, TYPE_ASPEED_SBC);
for (i = 0; i < sc->wdts_num; i++) {
@@ -206,6 +210,14 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c.busses[i]), 0, irq);
}
+ /* PECI */
+ if (!sysbus_realize(SYS_BUS_DEVICE(&s->peci), errp)) {
+ return;
+ }
+ sysbus_mmio_map(SYS_BUS_DEVICE(&s->peci), 0, sc->memmap[ASPEED_DEV_PECI]);
+ sysbus_connect_irq(SYS_BUS_DEVICE(&s->peci), 0,
+ aspeed_soc_get_irq(s, ASPEED_DEV_PECI));
+
/* LPC */
if (!sysbus_realize(SYS_BUS_DEVICE(&s->lpc), errp)) {
return;
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index b0a4199b69..85178fabea 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -59,6 +59,7 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
[ASPEED_DEV_LPC] = 0x1E789000,
[ASPEED_DEV_IBT] = 0x1E789140,
[ASPEED_DEV_I2C] = 0x1E78A000,
+ [ASPEED_DEV_PECI] = 0x1E78B000,
[ASPEED_DEV_UART1] = 0x1E783000,
[ASPEED_DEV_UART2] = 0x1E78D000,
[ASPEED_DEV_UART3] = 0x1E78E000,
@@ -122,6 +123,7 @@ static const int aspeed_soc_ast2600_irqmap[] = {
[ASPEED_DEV_LPC] = 35,
[ASPEED_DEV_IBT] = 143,
[ASPEED_DEV_I2C] = 110, /* 110 -> 125 */
+ [ASPEED_DEV_PECI] = 38,
[ASPEED_DEV_ETH1] = 2,
[ASPEED_DEV_ETH2] = 3,
[ASPEED_DEV_HACE] = 4,
@@ -180,6 +182,8 @@ static void aspeed_soc_ast2600_init(Object *obj)
snprintf(typename, sizeof(typename), "aspeed.i2c-%s", socname);
object_initialize_child(obj, "i2c", &s->i2c, typename);
+ object_initialize_child(obj, "peci", &s->peci, TYPE_ASPEED_PECI);
+
snprintf(typename, sizeof(typename), "aspeed.fmc-%s", socname);
object_initialize_child(obj, "fmc", &s->fmc, typename);
@@ -388,6 +392,14 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c.busses[i]), 0, irq);
}
+ /* PECI */
+ if (!sysbus_realize(SYS_BUS_DEVICE(&s->peci), errp)) {
+ return;
+ }
+ sysbus_mmio_map(SYS_BUS_DEVICE(&s->peci), 0, sc->memmap[ASPEED_DEV_PECI]);
+ sysbus_connect_irq(SYS_BUS_DEVICE(&s->peci), 0,
+ aspeed_soc_get_irq(s, ASPEED_DEV_PECI));
+
/* FMC, The number of CS is set at the board level */
object_property_set_link(OBJECT(&s->fmc), "dram", OBJECT(s->dram_mr),
&error_abort);
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 30574d4276..cb78d9945c 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -45,6 +45,7 @@ static const hwaddr aspeed_soc_ast2400_memmap[] = {
[ASPEED_DEV_LPC] = 0x1E789000,
[ASPEED_DEV_IBT] = 0x1E789140,
[ASPEED_DEV_I2C] = 0x1E78A000,
+ [ASPEED_DEV_PECI] = 0x1E78B000,
[ASPEED_DEV_ETH1] = 0x1E660000,
[ASPEED_DEV_ETH2] = 0x1E680000,
[ASPEED_DEV_UART1] = 0x1E783000,
@@ -80,6 +81,7 @@ static const hwaddr aspeed_soc_ast2500_memmap[] = {
[ASPEED_DEV_LPC] = 0x1E789000,
[ASPEED_DEV_IBT] = 0x1E789140,
[ASPEED_DEV_I2C] = 0x1E78A000,
+ [ASPEED_DEV_PECI] = 0x1E78B000,
[ASPEED_DEV_ETH1] = 0x1E660000,
[ASPEED_DEV_ETH2] = 0x1E680000,
[ASPEED_DEV_UART1] = 0x1E783000,
@@ -118,6 +120,7 @@ static const int aspeed_soc_ast2400_irqmap[] = {
[ASPEED_DEV_PWM] = 28,
[ASPEED_DEV_LPC] = 8,
[ASPEED_DEV_I2C] = 12,
+ [ASPEED_DEV_PECI] = 15,
[ASPEED_DEV_ETH1] = 2,
[ASPEED_DEV_ETH2] = 3,
[ASPEED_DEV_XDMA] = 6,
@@ -174,6 +177,8 @@ static void aspeed_soc_init(Object *obj)
snprintf(typename, sizeof(typename), "aspeed.i2c-%s", socname);
object_initialize_child(obj, "i2c", &s->i2c, typename);
+ object_initialize_child(obj, "peci", &s->peci, TYPE_ASPEED_PECI);
+
snprintf(typename, sizeof(typename), "aspeed.fmc-%s", socname);
object_initialize_child(obj, "fmc", &s->fmc, typename);
@@ -316,6 +321,14 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c), 0,
aspeed_soc_get_irq(s, ASPEED_DEV_I2C));
+ /* PECI */
+ if (!sysbus_realize(SYS_BUS_DEVICE(&s->peci), errp)) {
+ return;
+ }
+ sysbus_mmio_map(SYS_BUS_DEVICE(&s->peci), 0, sc->memmap[ASPEED_DEV_PECI]);
+ sysbus_connect_irq(SYS_BUS_DEVICE(&s->peci), 0,
+ aspeed_soc_get_irq(s, ASPEED_DEV_PECI));
+
/* FMC, The number of CS is set at the board level */
object_property_set_link(OBJECT(&s->fmc), "dram", OBJECT(s->dram_mr),
&error_abort);
diff --git a/hw/misc/aspeed_peci.c b/hw/misc/aspeed_peci.c
new file mode 100644
index 0000000000..93cc672e96
--- /dev/null
+++ b/hw/misc/aspeed_peci.c
@@ -0,0 +1,152 @@
+/*
+ * Aspeed PECI Controller
+ *
+ * Copyright (c) Meta Platforms, Inc. and affiliates. (http://www.meta.com)
+ *
+ * This code is licensed under the GPL version 2 or later. See the COPYING
+ * file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "hw/irq.h"
+#include "hw/misc/aspeed_peci.h"
+#include "hw/registerfields.h"
+#include "trace.h"
+
+#define ASPEED_PECI_CC_RSP_SUCCESS (0x40U)
+
+/* Command Register */
+REG32(PECI_CMD, 0x08)
+ FIELD(PECI_CMD, FIRE, 0, 1)
+
+/* Interrupt Control Register */
+REG32(PECI_INT_CTRL, 0x18)
+
+/* Interrupt Status Register */
+REG32(PECI_INT_STS, 0x1C)
+ FIELD(PECI_INT_STS, CMD_DONE, 0, 1)
+
+/* Rx/Tx Data Buffer Registers */
+REG32(PECI_WR_DATA0, 0x20)
+REG32(PECI_RD_DATA0, 0x30)
+
+static void aspeed_peci_raise_interrupt(AspeedPECIState *s, uint32_t status)
+{
+ trace_aspeed_peci_raise_interrupt(s->regs[R_PECI_INT_CTRL], status);
+
+ s->regs[R_PECI_INT_STS] = s->regs[R_PECI_INT_CTRL] & status;
+ if (!s->regs[R_PECI_INT_STS]) {
+ return;
+ }
+ qemu_irq_raise(s->irq);
+}
+
+static uint64_t aspeed_peci_read(void *opaque, hwaddr offset, unsigned size)
+{
+ AspeedPECIState *s = ASPEED_PECI(opaque);
+ uint64_t data;
+
+ if (offset >= ASPEED_PECI_NR_REGS << 2) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Out-of-bounds read at offset 0x%" HWADDR_PRIx "\n",
+ __func__, offset);
+ return 0;
+ }
+ data = s->regs[offset >> 2];
+
+ trace_aspeed_peci_read(offset, data);
+ return data;
+}
+
+static void aspeed_peci_write(void *opaque, hwaddr offset, uint64_t data,
+ unsigned size)
+{
+ AspeedPECIState *s = ASPEED_PECI(opaque);
+
+ trace_aspeed_peci_write(offset, data);
+
+ if (offset >= ASPEED_PECI_NR_REGS << 2) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Out-of-bounds write at offset 0x%" HWADDR_PRIx "\n",
+ __func__, offset);
+ return;
+ }
+
+ switch (offset) {
+ case A_PECI_INT_STS:
+ s->regs[R_PECI_INT_STS] &= ~data;
+ if (!s->regs[R_PECI_INT_STS]) {
+ qemu_irq_lower(s->irq);
+ }
+ break;
+ case A_PECI_CMD:
+ /*
+ * Only the FIRE bit is writable. Once the command is complete, it
+ * should be cleared. Since we complete the command immediately, the
+ * value is not stored in the register array.
+ */
+ if (!FIELD_EX32(data, PECI_CMD, FIRE)) {
+ break;
+ }
+ if (s->regs[R_PECI_INT_STS]) {
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: Interrupt status must be "
+ "cleared before firing another command: 0x%08x\n",
+ __func__, s->regs[R_PECI_INT_STS]);
+ break;
+ }
+ s->regs[R_PECI_RD_DATA0] = ASPEED_PECI_CC_RSP_SUCCESS;
+ s->regs[R_PECI_WR_DATA0] = ASPEED_PECI_CC_RSP_SUCCESS;
+ aspeed_peci_raise_interrupt(s,
+ FIELD_DP32(0, PECI_INT_STS, CMD_DONE, 1));
+ break;
+ default:
+ s->regs[offset / sizeof(s->regs[0])] = data;
+ break;
+ }
+}
+
+static const MemoryRegionOps aspeed_peci_ops = {
+ .read = aspeed_peci_read,
+ .write = aspeed_peci_write,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static void aspeed_peci_realize(DeviceState *dev, Error **errp)
+{
+ AspeedPECIState *s = ASPEED_PECI(dev);
+ SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
+
+ memory_region_init_io(&s->mmio, OBJECT(s), &aspeed_peci_ops, s,
+ TYPE_ASPEED_PECI, 0x1000);
+ sysbus_init_mmio(sbd, &s->mmio);
+ sysbus_init_irq(sbd, &s->irq);
+}
+
+static void aspeed_peci_reset(DeviceState *dev)
+{
+ AspeedPECIState *s = ASPEED_PECI(dev);
+
+ memset(s->regs, 0, sizeof(s->regs));
+}
+
+static void aspeed_peci_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->realize = aspeed_peci_realize;
+ dc->reset = aspeed_peci_reset;
+ dc->desc = "Aspeed PECI Controller";
+}
+
+static const TypeInfo aspeed_peci_types[] = {
+ {
+ .name = TYPE_ASPEED_PECI,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(AspeedPECIState),
+ .class_init = aspeed_peci_class_init,
+ .abstract = false,
+ },
+};
+
+DEFINE_TYPES(aspeed_peci_types);
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 132b7b7344..95268eddc0 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -116,7 +116,8 @@ softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files(
'aspeed_scu.c',
'aspeed_sbc.c',
'aspeed_sdmc.c',
- 'aspeed_xdma.c'))
+ 'aspeed_xdma.c',
+ 'aspeed_peci.c'))
softmmu_ss.add(when: 'CONFIG_MSF2', if_true: files('msf2-sysreg.c'))
softmmu_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_rng.c'))
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index c5e37b0154..90a0473b06 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -209,6 +209,11 @@ aspeed_i3c_device_write(uint32_t deviceid, uint64_t offset, uint64_t data) "I3C
aspeed_sdmc_write(uint64_t reg, uint64_t data) "reg @0x%" PRIx64 " data: 0x%" PRIx64
aspeed_sdmc_read(uint64_t reg, uint64_t data) "reg @0x%" PRIx64 " data: 0x%" PRIx64
+# aspeed_peci.c
+aspeed_peci_read(uint64_t offset, uint64_t data) "offset 0x%" PRIx64 " data 0x%" PRIx64
+aspeed_peci_write(uint64_t offset, uint64_t data) "offset 0x%" PRIx64 " data 0x%" PRIx64
+aspeed_peci_raise_interrupt(uint32_t ctrl, uint32_t status) "ctrl 0x%" PRIx32 " status 0x%" PRIx32
+
# bcm2835_property.c
bcm2835_mbox_property(uint32_t tag, uint32_t bufsize, size_t resplen) "mbox property tag:0x%08x in_sz:%u out_sz:%zu"
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index 02a5a9ffcb..f72a8db50b 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -34,6 +34,7 @@
#include "hw/usb/hcd-ehci.h"
#include "qom/object.h"
#include "hw/misc/aspeed_lpc.h"
+#include "hw/misc/aspeed_peci.h"
#define ASPEED_SPIS_NUM 2
#define ASPEED_EHCIS_NUM 2
@@ -73,6 +74,7 @@ struct AspeedSoCState {
AspeedSDHCIState sdhci;
AspeedSDHCIState emmc;
AspeedLPCState lpc;
+ AspeedPECIState peci;
uint32_t uart_default;
Clock *sysclk;
};
@@ -145,6 +147,7 @@ enum {
ASPEED_DEV_LPC,
ASPEED_DEV_IBT,
ASPEED_DEV_I2C,
+ ASPEED_DEV_PECI,
ASPEED_DEV_ETH1,
ASPEED_DEV_ETH2,
ASPEED_DEV_ETH3,
diff --git a/include/hw/misc/aspeed_peci.h b/include/hw/misc/aspeed_peci.h
new file mode 100644
index 0000000000..8382707d9f
--- /dev/null
+++ b/include/hw/misc/aspeed_peci.h
@@ -0,0 +1,29 @@
+/*
+ * Aspeed PECI Controller
+ *
+ * Copyright (c) Meta Platforms, Inc. and affiliates. (http://www.meta.com)
+ *
+ * This code is licensed under the GPL version 2 or later. See the COPYING
+ * file in the top-level directory.
+ */
+
+#ifndef ASPEED_PECI_H
+#define ASPEED_PECI_H
+
+#include "hw/sysbus.h"
+
+#define ASPEED_PECI_NR_REGS ((0xFC + 4) >> 2)
+#define TYPE_ASPEED_PECI "aspeed.peci"
+OBJECT_DECLARE_SIMPLE_TYPE(AspeedPECIState, ASPEED_PECI);
+
+struct AspeedPECIState {
+ /* <private> */
+ SysBusDevice parent;
+
+ MemoryRegion mmio;
+ qemu_irq irq;
+
+ uint32_t regs[ASPEED_PECI_NR_REGS];
+};
+
+#endif
--
2.37.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v3 11/14] hw/misc/aspeed: Add PECI controller
2022-06-30 4:51 ` [PATCH v3 11/14] hw/misc/aspeed: Add PECI controller Peter Delevoryas
@ 2022-06-30 6:32 ` Cédric Le Goater
0 siblings, 0 replies; 34+ messages in thread
From: Cédric Le Goater @ 2022-06-30 6:32 UTC (permalink / raw)
To: Peter Delevoryas
Cc: peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel, pdel
On 6/30/22 06:51, Peter Delevoryas wrote:
> From: Peter Delevoryas <pdel@fb.com>
>
> This introduces a really basic PECI controller that responses to
> commands by always setting the response code to success and then raising
> an interrupt to indicate the command is done. This helps avoid getting
> hit with constant errors if the driver continuously attempts to send a
> command and keeps timing out.
>
> The AST2400 and AST2500 only included registers up to 0x5C, not 0xFC.
> They supported PECI 1.1, 2.0, and 3.0. The AST2600 and AST1030 support
> PECI 4.0, which includes more read/write buffer registers from 0x80 to
> 0xFC to support 64-byte mode.
>
> This patch doesn't attempt to handle that, or to create a different
> version of the controller for the different generations, since it's only
> implementing functionality that is common to all generations.
>
> The basic sequence of events is that the firmware will read and write to
> various registers and then trigger a command by setting the FIRE bit in
> the command register (similar to the I2C controller).
>
> Then the firmware waits for an interrupt from the PECI controller,
> expecting the interrupt status register to be filled in with info on
> what happened. If the command was transmitted and received successfully,
> then response codes from the host CPU will be found in the data buffer
> registers.
>
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Thanks,
C.
> ---
> hw/arm/aspeed_ast10x0.c | 12 +++
> hw/arm/aspeed_ast2600.c | 12 +++
> hw/arm/aspeed_soc.c | 13 +++
> hw/misc/aspeed_peci.c | 152 ++++++++++++++++++++++++++++++++++
> hw/misc/meson.build | 3 +-
> hw/misc/trace-events | 5 ++
> include/hw/arm/aspeed_soc.h | 3 +
> include/hw/misc/aspeed_peci.h | 29 +++++++
> 8 files changed, 228 insertions(+), 1 deletion(-)
> create mode 100644 hw/misc/aspeed_peci.c
> create mode 100644 include/hw/misc/aspeed_peci.h
>
> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
> index 5df480a21f..56e8de3d89 100644
> --- a/hw/arm/aspeed_ast10x0.c
> +++ b/hw/arm/aspeed_ast10x0.c
> @@ -47,6 +47,7 @@ static const hwaddr aspeed_soc_ast1030_memmap[] = {
> [ASPEED_DEV_UART13] = 0x7E790700,
> [ASPEED_DEV_WDT] = 0x7E785000,
> [ASPEED_DEV_LPC] = 0x7E789000,
> + [ASPEED_DEV_PECI] = 0x7E78B000,
> [ASPEED_DEV_I2C] = 0x7E7B0000,
> };
>
> @@ -75,6 +76,7 @@ static const int aspeed_soc_ast1030_irqmap[] = {
> [ASPEED_DEV_TIMER8] = 23,
> [ASPEED_DEV_WDT] = 24,
> [ASPEED_DEV_LPC] = 35,
> + [ASPEED_DEV_PECI] = 38,
> [ASPEED_DEV_FMC] = 39,
> [ASPEED_DEV_PWM] = 44,
> [ASPEED_DEV_ADC] = 46,
> @@ -133,6 +135,8 @@ static void aspeed_soc_ast1030_init(Object *obj)
>
> object_initialize_child(obj, "lpc", &s->lpc, TYPE_ASPEED_LPC);
>
> + object_initialize_child(obj, "peci", &s->peci, TYPE_ASPEED_PECI);
> +
> object_initialize_child(obj, "sbc", &s->sbc, TYPE_ASPEED_SBC);
>
> for (i = 0; i < sc->wdts_num; i++) {
> @@ -206,6 +210,14 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
> sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c.busses[i]), 0, irq);
> }
>
> + /* PECI */
> + if (!sysbus_realize(SYS_BUS_DEVICE(&s->peci), errp)) {
> + return;
> + }
> + sysbus_mmio_map(SYS_BUS_DEVICE(&s->peci), 0, sc->memmap[ASPEED_DEV_PECI]);
> + sysbus_connect_irq(SYS_BUS_DEVICE(&s->peci), 0,
> + aspeed_soc_get_irq(s, ASPEED_DEV_PECI));
> +
> /* LPC */
> if (!sysbus_realize(SYS_BUS_DEVICE(&s->lpc), errp)) {
> return;
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index b0a4199b69..85178fabea 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -59,6 +59,7 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
> [ASPEED_DEV_LPC] = 0x1E789000,
> [ASPEED_DEV_IBT] = 0x1E789140,
> [ASPEED_DEV_I2C] = 0x1E78A000,
> + [ASPEED_DEV_PECI] = 0x1E78B000,
> [ASPEED_DEV_UART1] = 0x1E783000,
> [ASPEED_DEV_UART2] = 0x1E78D000,
> [ASPEED_DEV_UART3] = 0x1E78E000,
> @@ -122,6 +123,7 @@ static const int aspeed_soc_ast2600_irqmap[] = {
> [ASPEED_DEV_LPC] = 35,
> [ASPEED_DEV_IBT] = 143,
> [ASPEED_DEV_I2C] = 110, /* 110 -> 125 */
> + [ASPEED_DEV_PECI] = 38,
> [ASPEED_DEV_ETH1] = 2,
> [ASPEED_DEV_ETH2] = 3,
> [ASPEED_DEV_HACE] = 4,
> @@ -180,6 +182,8 @@ static void aspeed_soc_ast2600_init(Object *obj)
> snprintf(typename, sizeof(typename), "aspeed.i2c-%s", socname);
> object_initialize_child(obj, "i2c", &s->i2c, typename);
>
> + object_initialize_child(obj, "peci", &s->peci, TYPE_ASPEED_PECI);
> +
> snprintf(typename, sizeof(typename), "aspeed.fmc-%s", socname);
> object_initialize_child(obj, "fmc", &s->fmc, typename);
>
> @@ -388,6 +392,14 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
> sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c.busses[i]), 0, irq);
> }
>
> + /* PECI */
> + if (!sysbus_realize(SYS_BUS_DEVICE(&s->peci), errp)) {
> + return;
> + }
> + sysbus_mmio_map(SYS_BUS_DEVICE(&s->peci), 0, sc->memmap[ASPEED_DEV_PECI]);
> + sysbus_connect_irq(SYS_BUS_DEVICE(&s->peci), 0,
> + aspeed_soc_get_irq(s, ASPEED_DEV_PECI));
> +
> /* FMC, The number of CS is set at the board level */
> object_property_set_link(OBJECT(&s->fmc), "dram", OBJECT(s->dram_mr),
> &error_abort);
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index 30574d4276..cb78d9945c 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -45,6 +45,7 @@ static const hwaddr aspeed_soc_ast2400_memmap[] = {
> [ASPEED_DEV_LPC] = 0x1E789000,
> [ASPEED_DEV_IBT] = 0x1E789140,
> [ASPEED_DEV_I2C] = 0x1E78A000,
> + [ASPEED_DEV_PECI] = 0x1E78B000,
> [ASPEED_DEV_ETH1] = 0x1E660000,
> [ASPEED_DEV_ETH2] = 0x1E680000,
> [ASPEED_DEV_UART1] = 0x1E783000,
> @@ -80,6 +81,7 @@ static const hwaddr aspeed_soc_ast2500_memmap[] = {
> [ASPEED_DEV_LPC] = 0x1E789000,
> [ASPEED_DEV_IBT] = 0x1E789140,
> [ASPEED_DEV_I2C] = 0x1E78A000,
> + [ASPEED_DEV_PECI] = 0x1E78B000,
> [ASPEED_DEV_ETH1] = 0x1E660000,
> [ASPEED_DEV_ETH2] = 0x1E680000,
> [ASPEED_DEV_UART1] = 0x1E783000,
> @@ -118,6 +120,7 @@ static const int aspeed_soc_ast2400_irqmap[] = {
> [ASPEED_DEV_PWM] = 28,
> [ASPEED_DEV_LPC] = 8,
> [ASPEED_DEV_I2C] = 12,
> + [ASPEED_DEV_PECI] = 15,
> [ASPEED_DEV_ETH1] = 2,
> [ASPEED_DEV_ETH2] = 3,
> [ASPEED_DEV_XDMA] = 6,
> @@ -174,6 +177,8 @@ static void aspeed_soc_init(Object *obj)
> snprintf(typename, sizeof(typename), "aspeed.i2c-%s", socname);
> object_initialize_child(obj, "i2c", &s->i2c, typename);
>
> + object_initialize_child(obj, "peci", &s->peci, TYPE_ASPEED_PECI);
> +
> snprintf(typename, sizeof(typename), "aspeed.fmc-%s", socname);
> object_initialize_child(obj, "fmc", &s->fmc, typename);
>
> @@ -316,6 +321,14 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
> sysbus_connect_irq(SYS_BUS_DEVICE(&s->i2c), 0,
> aspeed_soc_get_irq(s, ASPEED_DEV_I2C));
>
> + /* PECI */
> + if (!sysbus_realize(SYS_BUS_DEVICE(&s->peci), errp)) {
> + return;
> + }
> + sysbus_mmio_map(SYS_BUS_DEVICE(&s->peci), 0, sc->memmap[ASPEED_DEV_PECI]);
> + sysbus_connect_irq(SYS_BUS_DEVICE(&s->peci), 0,
> + aspeed_soc_get_irq(s, ASPEED_DEV_PECI));
> +
> /* FMC, The number of CS is set at the board level */
> object_property_set_link(OBJECT(&s->fmc), "dram", OBJECT(s->dram_mr),
> &error_abort);
> diff --git a/hw/misc/aspeed_peci.c b/hw/misc/aspeed_peci.c
> new file mode 100644
> index 0000000000..93cc672e96
> --- /dev/null
> +++ b/hw/misc/aspeed_peci.c
> @@ -0,0 +1,152 @@
> +/*
> + * Aspeed PECI Controller
> + *
> + * Copyright (c) Meta Platforms, Inc. and affiliates. (http://www.meta.com)
> + *
> + * This code is licensed under the GPL version 2 or later. See the COPYING
> + * file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "hw/irq.h"
> +#include "hw/misc/aspeed_peci.h"
> +#include "hw/registerfields.h"
> +#include "trace.h"
> +
> +#define ASPEED_PECI_CC_RSP_SUCCESS (0x40U)
> +
> +/* Command Register */
> +REG32(PECI_CMD, 0x08)
> + FIELD(PECI_CMD, FIRE, 0, 1)
> +
> +/* Interrupt Control Register */
> +REG32(PECI_INT_CTRL, 0x18)
> +
> +/* Interrupt Status Register */
> +REG32(PECI_INT_STS, 0x1C)
> + FIELD(PECI_INT_STS, CMD_DONE, 0, 1)
> +
> +/* Rx/Tx Data Buffer Registers */
> +REG32(PECI_WR_DATA0, 0x20)
> +REG32(PECI_RD_DATA0, 0x30)
> +
> +static void aspeed_peci_raise_interrupt(AspeedPECIState *s, uint32_t status)
> +{
> + trace_aspeed_peci_raise_interrupt(s->regs[R_PECI_INT_CTRL], status);
> +
> + s->regs[R_PECI_INT_STS] = s->regs[R_PECI_INT_CTRL] & status;
> + if (!s->regs[R_PECI_INT_STS]) {
> + return;
> + }
> + qemu_irq_raise(s->irq);
> +}
> +
> +static uint64_t aspeed_peci_read(void *opaque, hwaddr offset, unsigned size)
> +{
> + AspeedPECIState *s = ASPEED_PECI(opaque);
> + uint64_t data;
> +
> + if (offset >= ASPEED_PECI_NR_REGS << 2) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Out-of-bounds read at offset 0x%" HWADDR_PRIx "\n",
> + __func__, offset);
> + return 0;
> + }
> + data = s->regs[offset >> 2];
> +
> + trace_aspeed_peci_read(offset, data);
> + return data;
> +}
> +
> +static void aspeed_peci_write(void *opaque, hwaddr offset, uint64_t data,
> + unsigned size)
> +{
> + AspeedPECIState *s = ASPEED_PECI(opaque);
> +
> + trace_aspeed_peci_write(offset, data);
> +
> + if (offset >= ASPEED_PECI_NR_REGS << 2) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "%s: Out-of-bounds write at offset 0x%" HWADDR_PRIx "\n",
> + __func__, offset);
> + return;
> + }
> +
> + switch (offset) {
> + case A_PECI_INT_STS:
> + s->regs[R_PECI_INT_STS] &= ~data;
> + if (!s->regs[R_PECI_INT_STS]) {
> + qemu_irq_lower(s->irq);
> + }
> + break;
> + case A_PECI_CMD:
> + /*
> + * Only the FIRE bit is writable. Once the command is complete, it
> + * should be cleared. Since we complete the command immediately, the
> + * value is not stored in the register array.
> + */
> + if (!FIELD_EX32(data, PECI_CMD, FIRE)) {
> + break;
> + }
> + if (s->regs[R_PECI_INT_STS]) {
> + qemu_log_mask(LOG_GUEST_ERROR, "%s: Interrupt status must be "
> + "cleared before firing another command: 0x%08x\n",
> + __func__, s->regs[R_PECI_INT_STS]);
> + break;
> + }
> + s->regs[R_PECI_RD_DATA0] = ASPEED_PECI_CC_RSP_SUCCESS;
> + s->regs[R_PECI_WR_DATA0] = ASPEED_PECI_CC_RSP_SUCCESS;
> + aspeed_peci_raise_interrupt(s,
> + FIELD_DP32(0, PECI_INT_STS, CMD_DONE, 1));
> + break;
> + default:
> + s->regs[offset / sizeof(s->regs[0])] = data;
> + break;
> + }
> +}
> +
> +static const MemoryRegionOps aspeed_peci_ops = {
> + .read = aspeed_peci_read,
> + .write = aspeed_peci_write,
> + .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static void aspeed_peci_realize(DeviceState *dev, Error **errp)
> +{
> + AspeedPECIState *s = ASPEED_PECI(dev);
> + SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +
> + memory_region_init_io(&s->mmio, OBJECT(s), &aspeed_peci_ops, s,
> + TYPE_ASPEED_PECI, 0x1000);
> + sysbus_init_mmio(sbd, &s->mmio);
> + sysbus_init_irq(sbd, &s->irq);
> +}
> +
> +static void aspeed_peci_reset(DeviceState *dev)
> +{
> + AspeedPECIState *s = ASPEED_PECI(dev);
> +
> + memset(s->regs, 0, sizeof(s->regs));
> +}
> +
> +static void aspeed_peci_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->realize = aspeed_peci_realize;
> + dc->reset = aspeed_peci_reset;
> + dc->desc = "Aspeed PECI Controller";
> +}
> +
> +static const TypeInfo aspeed_peci_types[] = {
> + {
> + .name = TYPE_ASPEED_PECI,
> + .parent = TYPE_SYS_BUS_DEVICE,
> + .instance_size = sizeof(AspeedPECIState),
> + .class_init = aspeed_peci_class_init,
> + .abstract = false,
> + },
> +};
> +
> +DEFINE_TYPES(aspeed_peci_types);
> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> index 132b7b7344..95268eddc0 100644
> --- a/hw/misc/meson.build
> +++ b/hw/misc/meson.build
> @@ -116,7 +116,8 @@ softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files(
> 'aspeed_scu.c',
> 'aspeed_sbc.c',
> 'aspeed_sdmc.c',
> - 'aspeed_xdma.c'))
> + 'aspeed_xdma.c',
> + 'aspeed_peci.c'))
>
> softmmu_ss.add(when: 'CONFIG_MSF2', if_true: files('msf2-sysreg.c'))
> softmmu_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_rng.c'))
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index c5e37b0154..90a0473b06 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -209,6 +209,11 @@ aspeed_i3c_device_write(uint32_t deviceid, uint64_t offset, uint64_t data) "I3C
> aspeed_sdmc_write(uint64_t reg, uint64_t data) "reg @0x%" PRIx64 " data: 0x%" PRIx64
> aspeed_sdmc_read(uint64_t reg, uint64_t data) "reg @0x%" PRIx64 " data: 0x%" PRIx64
>
> +# aspeed_peci.c
> +aspeed_peci_read(uint64_t offset, uint64_t data) "offset 0x%" PRIx64 " data 0x%" PRIx64
> +aspeed_peci_write(uint64_t offset, uint64_t data) "offset 0x%" PRIx64 " data 0x%" PRIx64
> +aspeed_peci_raise_interrupt(uint32_t ctrl, uint32_t status) "ctrl 0x%" PRIx32 " status 0x%" PRIx32
> +
> # bcm2835_property.c
> bcm2835_mbox_property(uint32_t tag, uint32_t bufsize, size_t resplen) "mbox property tag:0x%08x in_sz:%u out_sz:%zu"
>
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index 02a5a9ffcb..f72a8db50b 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -34,6 +34,7 @@
> #include "hw/usb/hcd-ehci.h"
> #include "qom/object.h"
> #include "hw/misc/aspeed_lpc.h"
> +#include "hw/misc/aspeed_peci.h"
>
> #define ASPEED_SPIS_NUM 2
> #define ASPEED_EHCIS_NUM 2
> @@ -73,6 +74,7 @@ struct AspeedSoCState {
> AspeedSDHCIState sdhci;
> AspeedSDHCIState emmc;
> AspeedLPCState lpc;
> + AspeedPECIState peci;
> uint32_t uart_default;
> Clock *sysclk;
> };
> @@ -145,6 +147,7 @@ enum {
> ASPEED_DEV_LPC,
> ASPEED_DEV_IBT,
> ASPEED_DEV_I2C,
> + ASPEED_DEV_PECI,
> ASPEED_DEV_ETH1,
> ASPEED_DEV_ETH2,
> ASPEED_DEV_ETH3,
> diff --git a/include/hw/misc/aspeed_peci.h b/include/hw/misc/aspeed_peci.h
> new file mode 100644
> index 0000000000..8382707d9f
> --- /dev/null
> +++ b/include/hw/misc/aspeed_peci.h
> @@ -0,0 +1,29 @@
> +/*
> + * Aspeed PECI Controller
> + *
> + * Copyright (c) Meta Platforms, Inc. and affiliates. (http://www.meta.com)
> + *
> + * This code is licensed under the GPL version 2 or later. See the COPYING
> + * file in the top-level directory.
> + */
> +
> +#ifndef ASPEED_PECI_H
> +#define ASPEED_PECI_H
> +
> +#include "hw/sysbus.h"
> +
> +#define ASPEED_PECI_NR_REGS ((0xFC + 4) >> 2)
> +#define TYPE_ASPEED_PECI "aspeed.peci"
> +OBJECT_DECLARE_SIMPLE_TYPE(AspeedPECIState, ASPEED_PECI);
> +
> +struct AspeedPECIState {
> + /* <private> */
> + SysBusDevice parent;
> +
> + MemoryRegion mmio;
> + qemu_irq irq;
> +
> + uint32_t regs[ASPEED_PECI_NR_REGS];
> +};
> +
> +#endif
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v3 12/14] hw/misc/aspeed: Add fby35-sb-cpld
2022-06-30 4:51 [PATCH v3 00/14] hw/i2c/aspeed: I2C slave mode DMA RX w/ new regs Peter Delevoryas
` (10 preceding siblings ...)
2022-06-30 4:51 ` [PATCH v3 11/14] hw/misc/aspeed: Add PECI controller Peter Delevoryas
@ 2022-06-30 4:51 ` Peter Delevoryas
2022-06-30 6:47 ` Cédric Le Goater
2022-06-30 4:51 ` [PATCH v3 13/14] hw/misc/aspeed: Add intel-me Peter Delevoryas
` (3 subsequent siblings)
15 siblings, 1 reply; 34+ messages in thread
From: Peter Delevoryas @ 2022-06-30 4:51 UTC (permalink / raw)
Cc: clg, peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel, pdel
From: Peter Delevoryas <pdel@fb.com>
fby35 machines have 1 BMC on a baseboard and 2-4 server boards with BIC's.
There are also CPLD's on each of the boards, one type of CPLD on the
baseboard and another type on each of the server boards. This commit adds an
implementation of some of the logic performed by the server board CPLD,
which is connected to the server board BIC.
fby35 machines have 1 baseboard with a BMC (AST2600) and 4 server boards
with bridge interconnects (BIC's, AST1030's). Each server board has a CPLD
on it which provides FRU information and some synchronization functionality
with the BMC. The baseboard also has one CPLD, but it does other stuff.
This commit just adds some of the FRU functionality to allow the BIC to
startup without any errors.
Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
MAINTAINERS | 1 +
hw/misc/fby35_sb_cpld.c | 128 ++++++++++++++++++++++++++++++++++++++++
hw/misc/meson.build | 3 +-
3 files changed, 131 insertions(+), 1 deletion(-)
create mode 100644 hw/misc/fby35_sb_cpld.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 05cf84b58c..3ffd473db1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1067,6 +1067,7 @@ F: hw/net/ftgmac100.c
F: include/hw/net/ftgmac100.h
F: docs/system/arm/aspeed.rst
F: tests/qtest/*aspeed*
+F: hw/misc/fby35_sb_cpld.c
NRF51
M: Joel Stanley <joel@jms.id.au>
diff --git a/hw/misc/fby35_sb_cpld.c b/hw/misc/fby35_sb_cpld.c
new file mode 100644
index 0000000000..f170a6c781
--- /dev/null
+++ b/hw/misc/fby35_sb_cpld.c
@@ -0,0 +1,128 @@
+/*
+ * fby35 Server Board CPLD
+ *
+ * Copyright (c) Meta Platforms, Inc. and affiliates. (http://www.meta.com)
+ *
+ * This code is licensed under the GPL version 2 or later. See the COPYING
+ * file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "hw/i2c/i2c.h"
+#include "hw/registerfields.h"
+
+#define BOARD_ID_CLASS1 0b0000
+#define BOARD_ID_CLASS2 0b0001
+
+#define TYPE_FBY35_SB_CPLD "fby35-sb-cpld"
+OBJECT_DECLARE_SIMPLE_TYPE(Fby35SbCpldState, FBY35_SB_CPLD);
+
+REG8(CLASS_TYPE, 0x5);
+ FIELD(CLASS_TYPE, RESERVED, 0, 2);
+ FIELD(CLASS_TYPE, 1OU_EXPANSION_NOT_PRESENT, 2, 1);
+ FIELD(CLASS_TYPE, 2OU_EXPANSION_NOT_PRESENT, 3, 1);
+ FIELD(CLASS_TYPE, BOARD_ID, 4, 4);
+REG8(BOARD_REVISION, 0x8);
+ FIELD(BOARD_REVISION, VALUE, 0, 4);
+ FIELD(BOARD_REVISION, RESERVED, 4, 4);
+
+struct Fby35SbCpldState {
+ I2CSlave parent_obj;
+
+ uint8_t target_reg;
+ uint32_t regs[10];
+};
+
+static void fby35_sb_cpld_realize(DeviceState *dev, Error **errp)
+{
+ Fby35SbCpldState *s = FBY35_SB_CPLD(dev);
+
+ memset(s->regs, 0, sizeof(s->regs));
+ s->target_reg = 0;
+
+ ARRAY_FIELD_DP32(s->regs, CLASS_TYPE, BOARD_ID, 0b0000);
+ ARRAY_FIELD_DP32(s->regs, CLASS_TYPE, 1OU_EXPANSION_NOT_PRESENT, 1);
+ ARRAY_FIELD_DP32(s->regs, CLASS_TYPE, 2OU_EXPANSION_NOT_PRESENT, 1);
+ ARRAY_FIELD_DP32(s->regs, BOARD_REVISION, VALUE, 0x1);
+}
+
+static int fby35_sb_cpld_i2c_event(I2CSlave *i2c, enum i2c_event event)
+{
+ Fby35SbCpldState *s = FBY35_SB_CPLD(i2c);
+
+ switch (event) {
+ case I2C_START_RECV:
+ break;
+ case I2C_START_SEND:
+ s->target_reg = 0;
+ break;
+ case I2C_START_SEND_ASYNC:
+ case I2C_FINISH:
+ case I2C_NACK:
+ break;
+ }
+
+ return 0;
+}
+
+static uint8_t fby35_sb_cpld_i2c_recv(I2CSlave *i2c)
+{
+ Fby35SbCpldState *s = FBY35_SB_CPLD(i2c);
+
+ switch (s->target_reg) {
+ case R_CLASS_TYPE:
+ case R_BOARD_REVISION:
+ return s->regs[s->target_reg];
+ default:
+ qemu_log_mask(LOG_UNIMP, "%s: Register read unimplemented: 0x%02x\n",
+ __func__, s->target_reg);
+ return 0xff;
+ }
+}
+
+static int fby35_sb_cpld_i2c_send(I2CSlave *i2c, uint8_t data)
+{
+ Fby35SbCpldState *s = FBY35_SB_CPLD(i2c);
+
+ if (s->target_reg == 0) {
+ s->target_reg = data;
+ return 0;
+ }
+
+ switch (s->target_reg) {
+ case R_CLASS_TYPE:
+ case R_BOARD_REVISION:
+ s->regs[s->target_reg] = data;
+ break;
+ default:
+ qemu_log_mask(LOG_UNIMP,
+ "%s: Register write unimplemented: 0x%02x 0x%02x\n",
+ __func__, s->target_reg, data);
+ break;
+ }
+
+ return 0;
+}
+
+static void fby35_sb_cpld_class_init(ObjectClass *oc, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(oc);
+ I2CSlaveClass *i2c = I2C_SLAVE_CLASS(oc);
+
+ dc->realize = fby35_sb_cpld_realize;
+ i2c->event = fby35_sb_cpld_i2c_event;
+ i2c->recv = fby35_sb_cpld_i2c_recv;
+ i2c->send = fby35_sb_cpld_i2c_send;
+}
+
+static const TypeInfo types[] = {
+ {
+ .name = TYPE_FBY35_SB_CPLD,
+ .parent = TYPE_I2C_SLAVE,
+ .instance_size = sizeof(Fby35SbCpldState),
+ .class_init = fby35_sb_cpld_class_init,
+ },
+};
+
+DEFINE_TYPES(types);
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 95268eddc0..948e25c440 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -117,7 +117,8 @@ softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files(
'aspeed_sbc.c',
'aspeed_sdmc.c',
'aspeed_xdma.c',
- 'aspeed_peci.c'))
+ 'aspeed_peci.c',
+ 'fby35_sb_cpld.c'))
softmmu_ss.add(when: 'CONFIG_MSF2', if_true: files('msf2-sysreg.c'))
softmmu_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_rng.c'))
--
2.37.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v3 12/14] hw/misc/aspeed: Add fby35-sb-cpld
2022-06-30 4:51 ` [PATCH v3 12/14] hw/misc/aspeed: Add fby35-sb-cpld Peter Delevoryas
@ 2022-06-30 6:47 ` Cédric Le Goater
0 siblings, 0 replies; 34+ messages in thread
From: Cédric Le Goater @ 2022-06-30 6:47 UTC (permalink / raw)
To: Peter Delevoryas
Cc: peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel, pdel
On 6/30/22 06:51, Peter Delevoryas wrote:
> From: Peter Delevoryas <pdel@fb.com>
>
> fby35 machines have 1 BMC on a baseboard and 2-4 server boards with BIC's.
> There are also CPLD's on each of the boards, one type of CPLD on the
> baseboard and another type on each of the server boards. This commit adds an
> implementation of some of the logic performed by the server board CPLD,
> which is connected to the server board BIC.
>
> fby35 machines have 1 baseboard with a BMC (AST2600) and 4 server boards
> with bridge interconnects (BIC's, AST1030's). Each server board has a CPLD
> on it which provides FRU information and some synchronization functionality
> with the BMC. The baseboard also has one CPLD, but it does other stuff.
>
> This commit just adds some of the FRU functionality to allow the BIC to
> startup without any errors.
>
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> ---
> MAINTAINERS | 1 +
> hw/misc/fby35_sb_cpld.c | 128 ++++++++++++++++++++++++++++++++++++++++
> hw/misc/meson.build | 3 +-
> 3 files changed, 131 insertions(+), 1 deletion(-)
> create mode 100644 hw/misc/fby35_sb_cpld.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 05cf84b58c..3ffd473db1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1067,6 +1067,7 @@ F: hw/net/ftgmac100.c
> F: include/hw/net/ftgmac100.h
> F: docs/system/arm/aspeed.rst
> F: tests/qtest/*aspeed*
> +F: hw/misc/fby35_sb_cpld.c
>
> NRF51
> M: Joel Stanley <joel@jms.id.au>
> diff --git a/hw/misc/fby35_sb_cpld.c b/hw/misc/fby35_sb_cpld.c
> new file mode 100644
> index 0000000000..f170a6c781
> --- /dev/null
> +++ b/hw/misc/fby35_sb_cpld.c
> @@ -0,0 +1,128 @@
> +/*
> + * fby35 Server Board CPLD
> + *
> + * Copyright (c) Meta Platforms, Inc. and affiliates. (http://www.meta.com)
> + *
> + * This code is licensed under the GPL version 2 or later. See the COPYING
> + * file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "hw/i2c/i2c.h"
> +#include "hw/registerfields.h"
> +
> +#define BOARD_ID_CLASS1 0b0000
> +#define BOARD_ID_CLASS2 0b0001
> +
> +#define TYPE_FBY35_SB_CPLD "fby35-sb-cpld"
> +OBJECT_DECLARE_SIMPLE_TYPE(Fby35SbCpldState, FBY35_SB_CPLD);
> +
> +REG8(CLASS_TYPE, 0x5);
> + FIELD(CLASS_TYPE, RESERVED, 0, 2);
> + FIELD(CLASS_TYPE, 1OU_EXPANSION_NOT_PRESENT, 2, 1);
> + FIELD(CLASS_TYPE, 2OU_EXPANSION_NOT_PRESENT, 3, 1);
> + FIELD(CLASS_TYPE, BOARD_ID, 4, 4);
> +REG8(BOARD_REVISION, 0x8);
> + FIELD(BOARD_REVISION, VALUE, 0, 4);
> + FIELD(BOARD_REVISION, RESERVED, 4, 4);
> +
> +struct Fby35SbCpldState {
> + I2CSlave parent_obj;
> +
> + uint8_t target_reg;
> + uint32_t regs[10];
> +};
> +
> +static void fby35_sb_cpld_realize(DeviceState *dev, Error **errp)
> +{
> + Fby35SbCpldState *s = FBY35_SB_CPLD(dev);
> +
> + memset(s->regs, 0, sizeof(s->regs));
> + s->target_reg = 0;
> +
> + ARRAY_FIELD_DP32(s->regs, CLASS_TYPE, BOARD_ID, 0b0000);
> + ARRAY_FIELD_DP32(s->regs, CLASS_TYPE, 1OU_EXPANSION_NOT_PRESENT, 1);
> + ARRAY_FIELD_DP32(s->regs, CLASS_TYPE, 2OU_EXPANSION_NOT_PRESENT, 1);
> + ARRAY_FIELD_DP32(s->regs, BOARD_REVISION, VALUE, 0x1);
> +}
This is a reset handler.
C.
> +static int fby35_sb_cpld_i2c_event(I2CSlave *i2c, enum i2c_event event)
> +{
> + Fby35SbCpldState *s = FBY35_SB_CPLD(i2c);
> +
> + switch (event) {
> + case I2C_START_RECV:
> + break;
> + case I2C_START_SEND:
> + s->target_reg = 0;
> + break;
> + case I2C_START_SEND_ASYNC:
> + case I2C_FINISH:
> + case I2C_NACK:
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static uint8_t fby35_sb_cpld_i2c_recv(I2CSlave *i2c)
> +{
> + Fby35SbCpldState *s = FBY35_SB_CPLD(i2c);
> +
> + switch (s->target_reg) {
> + case R_CLASS_TYPE:
> + case R_BOARD_REVISION:
> + return s->regs[s->target_reg];
> + default:
> + qemu_log_mask(LOG_UNIMP, "%s: Register read unimplemented: 0x%02x\n",
> + __func__, s->target_reg);
> + return 0xff;
> + }
> +}
> +
> +static int fby35_sb_cpld_i2c_send(I2CSlave *i2c, uint8_t data)
> +{
> + Fby35SbCpldState *s = FBY35_SB_CPLD(i2c);
> +
> + if (s->target_reg == 0) {
> + s->target_reg = data;
> + return 0;
> + }
> +
> + switch (s->target_reg) {
> + case R_CLASS_TYPE:
> + case R_BOARD_REVISION:
> + s->regs[s->target_reg] = data;
> + break;
> + default:
> + qemu_log_mask(LOG_UNIMP,
> + "%s: Register write unimplemented: 0x%02x 0x%02x\n",
> + __func__, s->target_reg, data);
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static void fby35_sb_cpld_class_init(ObjectClass *oc, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(oc);
> + I2CSlaveClass *i2c = I2C_SLAVE_CLASS(oc);
> +
> + dc->realize = fby35_sb_cpld_realize;
> + i2c->event = fby35_sb_cpld_i2c_event;
> + i2c->recv = fby35_sb_cpld_i2c_recv;
> + i2c->send = fby35_sb_cpld_i2c_send;
> +}
> +
> +static const TypeInfo types[] = {
> + {
> + .name = TYPE_FBY35_SB_CPLD,
> + .parent = TYPE_I2C_SLAVE,
> + .instance_size = sizeof(Fby35SbCpldState),
> + .class_init = fby35_sb_cpld_class_init,
> + },
> +};
> +
> +DEFINE_TYPES(types);
> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> index 95268eddc0..948e25c440 100644
> --- a/hw/misc/meson.build
> +++ b/hw/misc/meson.build
> @@ -117,7 +117,8 @@ softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files(
> 'aspeed_sbc.c',
> 'aspeed_sdmc.c',
> 'aspeed_xdma.c',
> - 'aspeed_peci.c'))
> + 'aspeed_peci.c',
> + 'fby35_sb_cpld.c'))
>
> softmmu_ss.add(when: 'CONFIG_MSF2', if_true: files('msf2-sysreg.c'))
> softmmu_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_rng.c'))
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v3 13/14] hw/misc/aspeed: Add intel-me
2022-06-30 4:51 [PATCH v3 00/14] hw/i2c/aspeed: I2C slave mode DMA RX w/ new regs Peter Delevoryas
` (11 preceding siblings ...)
2022-06-30 4:51 ` [PATCH v3 12/14] hw/misc/aspeed: Add fby35-sb-cpld Peter Delevoryas
@ 2022-06-30 4:51 ` Peter Delevoryas
2022-06-30 11:09 ` Cédric Le Goater
2022-06-30 4:51 ` [PATCH v3 14/14] hw/arm/aspeed: Add oby35-cl machine Peter Delevoryas
` (2 subsequent siblings)
15 siblings, 1 reply; 34+ messages in thread
From: Peter Delevoryas @ 2022-06-30 4:51 UTC (permalink / raw)
Cc: clg, peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel, pdel
From: Peter Delevoryas <pdel@fb.com>
The Intel Management Engine is an IPMI endpoint that responds to various
IPMI commands. In this commit, I've added some very basic functionality that
will respond back with a respond code of zero (success), while also setting
an appropriate response NetFN (request NetFN + 1), a matching command ID and
sequence number, and the 2 standard checksums. Other data is not provided,
but the model here could be extended to respond to more kinds of requests.
Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
MAINTAINERS | 1 +
hw/misc/intel_me.c | 162 +++++++++++++++++++++++++++++++++++++++++++
hw/misc/meson.build | 3 +-
hw/misc/trace-events | 8 +++
4 files changed, 173 insertions(+), 1 deletion(-)
create mode 100644 hw/misc/intel_me.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 3ffd473db1..3220644bb5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1068,6 +1068,7 @@ F: include/hw/net/ftgmac100.h
F: docs/system/arm/aspeed.rst
F: tests/qtest/*aspeed*
F: hw/misc/fby35_sb_cpld.c
+F: hw/misc/intel_me.c
NRF51
M: Joel Stanley <joel@jms.id.au>
diff --git a/hw/misc/intel_me.c b/hw/misc/intel_me.c
new file mode 100644
index 0000000000..933ae45101
--- /dev/null
+++ b/hw/misc/intel_me.c
@@ -0,0 +1,162 @@
+/*
+ * Copyright (c) Meta Platforms, Inc. and affiliates. (http://www.meta.com)
+ *
+ * This code is licensed under the GPL version 2 or later. See the COPYING
+ * file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/main-loop.h"
+#include "hw/i2c/i2c.h"
+#include "trace.h"
+
+#define TYPE_INTEL_ME "intel-me"
+OBJECT_DECLARE_SIMPLE_TYPE(IntelMEState, INTEL_ME);
+
+struct IntelMEState {
+ I2CSlave parent_obj;
+
+ I2CBus *bus;
+ QEMUBH *bh;
+ int rx_len;
+ int tx_len;
+ int tx_pos;
+ uint8_t rx_buf[512];
+ uint8_t tx_buf[512];
+};
+
+static void intel_me_bh(void *opaque)
+{
+ IntelMEState *s = opaque;
+ I2CSlave *i2c = I2C_SLAVE(s);
+ uint8_t target_addr;
+
+ assert(s->bus->bh == s->bh);
+
+ switch (s->tx_pos) {
+ case 0:
+ target_addr = s->tx_buf[s->tx_pos++];
+ trace_intel_me_tx_start(i2c->address, target_addr);
+ if (i2c_start_send_async(s->bus, target_addr) != 0) {
+ break;
+ }
+ return;
+ default:
+ if (s->tx_pos >= s->tx_len) {
+ break;
+ }
+ trace_intel_me_tx_data(i2c->address, s->tx_buf[s->tx_pos]);
+ if (i2c_send_async(s->bus, s->tx_buf[s->tx_pos++]) != 0) {
+ break;
+ }
+ return;
+ }
+
+ trace_intel_me_tx_end(i2c->address);
+ i2c_end_transfer(s->bus);
+ i2c_bus_release(s->bus);
+ s->tx_len = 0;
+ s->tx_pos = 0;
+ memset(s->tx_buf, 0, sizeof(s->tx_buf));
+}
+
+static void intel_me_realize(DeviceState *dev, Error **errp)
+{
+ IntelMEState *s = INTEL_ME(dev);
+
+ s->bus = I2C_BUS(qdev_get_parent_bus(dev));
+ s->bh = qemu_bh_new(intel_me_bh, s);
+ s->rx_len = 0;
+ s->tx_len = 0;
+ s->tx_pos = 0;
+ memset(s->rx_buf, 0, sizeof(s->rx_buf));
+ memset(s->tx_buf, 0, sizeof(s->tx_buf));
+}
+
+static uint8_t checksum(const uint8_t *ptr, int len)
+{
+ int sum = 0;
+
+ for (int i = 0; i < len; i++) {
+ sum += ptr[i];
+ }
+
+ return 256 - sum;
+}
+
+static int intel_me_i2c_event(I2CSlave *i2c, enum i2c_event event)
+{
+ IntelMEState *s = INTEL_ME(i2c);
+
+ switch (event) {
+ case I2C_START_RECV:
+ break;
+ case I2C_START_SEND:
+ trace_intel_me_rx_start(i2c->address);
+ s->rx_len = 0;
+ memset(s->rx_buf, 0, sizeof(s->rx_buf));
+ break;
+ case I2C_START_SEND_ASYNC:
+ break;
+ case I2C_FINISH:
+ trace_intel_me_rx_end(i2c->address);
+ s->tx_len = 10;
+ s->tx_pos = 0;
+ s->tx_buf[0] = s->rx_buf[2];
+ s->tx_buf[1] = ((s->rx_buf[0] >> 2) + 1) << 2;
+ s->tx_buf[2] = checksum(s->tx_buf, 2);
+ s->tx_buf[3] = i2c->address;
+ s->tx_buf[4] = (s->rx_buf[3] >> 2) << 2;
+ s->tx_buf[5] = s->rx_buf[4];
+ s->tx_buf[6] = 0x00;
+ s->tx_buf[7] = 0x55;
+ s->tx_buf[8] = 0x00;
+ s->tx_buf[9] = checksum(s->tx_buf, s->tx_len - 1);
+ s->tx_buf[0] >>= 1;
+ i2c_bus_master(s->bus, s->bh);
+ break;
+ case I2C_NACK:
+ break;
+ }
+
+ return 0;
+}
+
+static uint8_t intel_me_i2c_recv(I2CSlave *i2c)
+{
+ return 0xff;
+}
+
+static int intel_me_i2c_send(I2CSlave *i2c, uint8_t data)
+{
+ IntelMEState *s = INTEL_ME(i2c);
+
+ trace_intel_me_rx_data(i2c->address, data);
+
+ assert(s->rx_len < sizeof(s->rx_buf));
+ s->rx_buf[s->rx_len++] = data;
+
+ return 0;
+}
+
+static void intel_me_class_init(ObjectClass *oc, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(oc);
+ I2CSlaveClass *i2c = I2C_SLAVE_CLASS(oc);
+
+ dc->realize = intel_me_realize;
+ i2c->event = intel_me_i2c_event;
+ i2c->recv = intel_me_i2c_recv;
+ i2c->send = intel_me_i2c_send;
+}
+
+static const TypeInfo types[] = {
+ {
+ .name = TYPE_INTEL_ME,
+ .parent = TYPE_I2C_SLAVE,
+ .instance_size = sizeof(IntelMEState),
+ .class_init = intel_me_class_init,
+ },
+};
+
+DEFINE_TYPES(types);
diff --git a/hw/misc/meson.build b/hw/misc/meson.build
index 948e25c440..165b9dce6d 100644
--- a/hw/misc/meson.build
+++ b/hw/misc/meson.build
@@ -118,7 +118,8 @@ softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files(
'aspeed_sdmc.c',
'aspeed_xdma.c',
'aspeed_peci.c',
- 'fby35_sb_cpld.c'))
+ 'fby35_sb_cpld.c',
+ 'intel_me.c'))
softmmu_ss.add(when: 'CONFIG_MSF2', if_true: files('msf2-sysreg.c'))
softmmu_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_rng.c'))
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 90a0473b06..7ca23bcf27 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -273,3 +273,11 @@ virt_ctrl_instance_init(void *dev) "ctrl: %p"
lasi_chip_mem_valid(uint64_t addr, uint32_t val) "access to addr 0x%"PRIx64" is %d"
lasi_chip_read(uint64_t addr, uint32_t val) "addr 0x%"PRIx64" val 0x%08x"
lasi_chip_write(uint64_t addr, uint32_t val) "addr 0x%"PRIx64" val 0x%08x"
+
+# intel_me.c
+intel_me_rx_start(uint8_t addr) "addr 0x%02x"
+intel_me_rx_data(uint8_t addr, uint8_t data) "addr 0x%02x data 0x%02x"
+intel_me_rx_end(uint8_t addr) "addr 0x%02x"
+intel_me_tx_start(uint8_t addr, uint8_t target_addr) "addr 0x%02x target_addr 0x%02x"
+intel_me_tx_data(uint8_t addr, uint8_t data) "addr 0x%02x data 0x%02x"
+intel_me_tx_end(uint8_t addr) "addr 0x%02x"
--
2.37.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v3 13/14] hw/misc/aspeed: Add intel-me
2022-06-30 4:51 ` [PATCH v3 13/14] hw/misc/aspeed: Add intel-me Peter Delevoryas
@ 2022-06-30 11:09 ` Cédric Le Goater
2022-06-30 16:20 ` Peter Delevoryas
0 siblings, 1 reply; 34+ messages in thread
From: Cédric Le Goater @ 2022-06-30 11:09 UTC (permalink / raw)
To: Peter Delevoryas
Cc: peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel, pdel
On 6/30/22 06:51, Peter Delevoryas wrote:
> From: Peter Delevoryas <pdel@fb.com>
>
> The Intel Management Engine is an IPMI endpoint that responds to various
> IPMI commands.
Have you looked at the ipmi-bmc-sim device ? It is relatively easy
to attach to a bus.
> In this commit, I've added some very basic functionality that
> will respond back with a respond code of zero (success), while also setting
> an appropriate response NetFN (request NetFN + 1), a matching command ID and
> sequence number, and the 2 standard checksums. Other data is not provided,
> but the model here could be extended to respond to more kinds of requests.
>
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> ---
> MAINTAINERS | 1 +
> hw/misc/intel_me.c | 162 +++++++++++++++++++++++++++++++++++++++++++
> hw/misc/meson.build | 3 +-
> hw/misc/trace-events | 8 +++
> 4 files changed, 173 insertions(+), 1 deletion(-)
> create mode 100644 hw/misc/intel_me.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3ffd473db1..3220644bb5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1068,6 +1068,7 @@ F: include/hw/net/ftgmac100.h
> F: docs/system/arm/aspeed.rst
> F: tests/qtest/*aspeed*
> F: hw/misc/fby35_sb_cpld.c
> +F: hw/misc/intel_me.c
>
> NRF51
> M: Joel Stanley <joel@jms.id.au>
> diff --git a/hw/misc/intel_me.c b/hw/misc/intel_me.c
> new file mode 100644
> index 0000000000..933ae45101
> --- /dev/null
> +++ b/hw/misc/intel_me.c
> @@ -0,0 +1,162 @@
> +/*
> + * Copyright (c) Meta Platforms, Inc. and affiliates. (http://www.meta.com)
> + *
> + * This code is licensed under the GPL version 2 or later. See the COPYING
> + * file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/main-loop.h"
> +#include "hw/i2c/i2c.h"
> +#include "trace.h"
> +
> +#define TYPE_INTEL_ME "intel-me"
> +OBJECT_DECLARE_SIMPLE_TYPE(IntelMEState, INTEL_ME);
> +
> +struct IntelMEState {
> + I2CSlave parent_obj;
> +
> + I2CBus *bus;
> + QEMUBH *bh;
> + int rx_len;
> + int tx_len;
> + int tx_pos;
> + uint8_t rx_buf[512];
> + uint8_t tx_buf[512];
> +};
> +
> +static void intel_me_bh(void *opaque)
> +{
> + IntelMEState *s = opaque;
> + I2CSlave *i2c = I2C_SLAVE(s);
> + uint8_t target_addr;
> +
> + assert(s->bus->bh == s->bh);
> +
> + switch (s->tx_pos) {
> + case 0:
> + target_addr = s->tx_buf[s->tx_pos++];
> + trace_intel_me_tx_start(i2c->address, target_addr);
> + if (i2c_start_send_async(s->bus, target_addr) != 0) {
> + break;
> + }
> + return;
> + default:
> + if (s->tx_pos >= s->tx_len) {
> + break;
> + }
> + trace_intel_me_tx_data(i2c->address, s->tx_buf[s->tx_pos]);
> + if (i2c_send_async(s->bus, s->tx_buf[s->tx_pos++]) != 0) {
> + break;
> + }
> + return;
> + }
> +
> + trace_intel_me_tx_end(i2c->address);
> + i2c_end_transfer(s->bus);
> + i2c_bus_release(s->bus);
> + s->tx_len = 0;
> + s->tx_pos = 0;
> + memset(s->tx_buf, 0, sizeof(s->tx_buf));
> +}
> +
> +static void intel_me_realize(DeviceState *dev, Error **errp)
> +{
> + IntelMEState *s = INTEL_ME(dev);
> +
> + s->bus = I2C_BUS(qdev_get_parent_bus(dev));
> + s->bh = qemu_bh_new(intel_me_bh, s);
> + s->rx_len = 0;
> + s->tx_len = 0;
> + s->tx_pos = 0;
> + memset(s->rx_buf, 0, sizeof(s->rx_buf));
> + memset(s->tx_buf, 0, sizeof(s->tx_buf));
> +}
> +
> +static uint8_t checksum(const uint8_t *ptr, int len)
> +{
> + int sum = 0;
> +
> + for (int i = 0; i < len; i++) {
> + sum += ptr[i];
> + }
> +
> + return 256 - sum;
> +}
> +
> +static int intel_me_i2c_event(I2CSlave *i2c, enum i2c_event event)
> +{
> + IntelMEState *s = INTEL_ME(i2c);
> +
> + switch (event) {
> + case I2C_START_RECV:
> + break;
> + case I2C_START_SEND:
> + trace_intel_me_rx_start(i2c->address);
> + s->rx_len = 0;
> + memset(s->rx_buf, 0, sizeof(s->rx_buf));
> + break;
> + case I2C_START_SEND_ASYNC:
> + break;
> + case I2C_FINISH:
> + trace_intel_me_rx_end(i2c->address);
> + s->tx_len = 10;
> + s->tx_pos = 0;
> + s->tx_buf[0] = s->rx_buf[2];
> + s->tx_buf[1] = ((s->rx_buf[0] >> 2) + 1) << 2;
> + s->tx_buf[2] = checksum(s->tx_buf, 2);
> + s->tx_buf[3] = i2c->address;
> + s->tx_buf[4] = (s->rx_buf[3] >> 2) << 2;
> + s->tx_buf[5] = s->rx_buf[4];
> + s->tx_buf[6] = 0x00;
> + s->tx_buf[7] = 0x55;
> + s->tx_buf[8] = 0x00;
> + s->tx_buf[9] = checksum(s->tx_buf, s->tx_len - 1);
> + s->tx_buf[0] >>= 1;
> + i2c_bus_master(s->bus, s->bh);
> + break;
> + case I2C_NACK:
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static uint8_t intel_me_i2c_recv(I2CSlave *i2c)
> +{
> + return 0xff;
> +}
> +
> +static int intel_me_i2c_send(I2CSlave *i2c, uint8_t data)
> +{
> + IntelMEState *s = INTEL_ME(i2c);
> +
> + trace_intel_me_rx_data(i2c->address, data);
> +
> + assert(s->rx_len < sizeof(s->rx_buf));
> + s->rx_buf[s->rx_len++] = data;
> +
> + return 0;
> +}
> +
> +static void intel_me_class_init(ObjectClass *oc, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(oc);
> + I2CSlaveClass *i2c = I2C_SLAVE_CLASS(oc);
> +
> + dc->realize = intel_me_realize;
> + i2c->event = intel_me_i2c_event;
> + i2c->recv = intel_me_i2c_recv;
> + i2c->send = intel_me_i2c_send;
> +}
> +
> +static const TypeInfo types[] = {
> + {
> + .name = TYPE_INTEL_ME,
> + .parent = TYPE_I2C_SLAVE,
> + .instance_size = sizeof(IntelMEState),
> + .class_init = intel_me_class_init,
> + },
> +};
> +
> +DEFINE_TYPES(types);
> diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> index 948e25c440..165b9dce6d 100644
> --- a/hw/misc/meson.build
> +++ b/hw/misc/meson.build
> @@ -118,7 +118,8 @@ softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files(
> 'aspeed_sdmc.c',
> 'aspeed_xdma.c',
> 'aspeed_peci.c',
> - 'fby35_sb_cpld.c'))
> + 'fby35_sb_cpld.c',
> + 'intel_me.c'))
>
> softmmu_ss.add(when: 'CONFIG_MSF2', if_true: files('msf2-sysreg.c'))
> softmmu_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_rng.c'))
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index 90a0473b06..7ca23bcf27 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -273,3 +273,11 @@ virt_ctrl_instance_init(void *dev) "ctrl: %p"
> lasi_chip_mem_valid(uint64_t addr, uint32_t val) "access to addr 0x%"PRIx64" is %d"
> lasi_chip_read(uint64_t addr, uint32_t val) "addr 0x%"PRIx64" val 0x%08x"
> lasi_chip_write(uint64_t addr, uint32_t val) "addr 0x%"PRIx64" val 0x%08x"
> +
> +# intel_me.c
> +intel_me_rx_start(uint8_t addr) "addr 0x%02x"
> +intel_me_rx_data(uint8_t addr, uint8_t data) "addr 0x%02x data 0x%02x"
> +intel_me_rx_end(uint8_t addr) "addr 0x%02x"
> +intel_me_tx_start(uint8_t addr, uint8_t target_addr) "addr 0x%02x target_addr 0x%02x"
> +intel_me_tx_data(uint8_t addr, uint8_t data) "addr 0x%02x data 0x%02x"
> +intel_me_tx_end(uint8_t addr) "addr 0x%02x"
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 13/14] hw/misc/aspeed: Add intel-me
2022-06-30 11:09 ` Cédric Le Goater
@ 2022-06-30 16:20 ` Peter Delevoryas
0 siblings, 0 replies; 34+ messages in thread
From: Peter Delevoryas @ 2022-06-30 16:20 UTC (permalink / raw)
To: Cédric Le Goater
Cc: peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel, pdel
On Thu, Jun 30, 2022 at 01:09:09PM +0200, Cédric Le Goater wrote:
> On 6/30/22 06:51, Peter Delevoryas wrote:
> > From: Peter Delevoryas <pdel@fb.com>
> >
> > The Intel Management Engine is an IPMI endpoint that responds to various
> > IPMI commands.
>
> Have you looked at the ipmi-bmc-sim device ? It is relatively easy
> to attach to a bus.
No I haven't! I didn't realize there was already some ipmi simulation code,
that's great. I'll look into turning this into an ipmi-me-sim or something.
>
> > In this commit, I've added some very basic functionality that
> > will respond back with a respond code of zero (success), while also setting
> > an appropriate response NetFN (request NetFN + 1), a matching command ID and
> > sequence number, and the 2 standard checksums. Other data is not provided,
> > but the model here could be extended to respond to more kinds of requests.
> >
> > Signed-off-by: Peter Delevoryas <pdel@fb.com>
> > ---
> > MAINTAINERS | 1 +
> > hw/misc/intel_me.c | 162 +++++++++++++++++++++++++++++++++++++++++++
> > hw/misc/meson.build | 3 +-
> > hw/misc/trace-events | 8 +++
> > 4 files changed, 173 insertions(+), 1 deletion(-)
> > create mode 100644 hw/misc/intel_me.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 3ffd473db1..3220644bb5 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1068,6 +1068,7 @@ F: include/hw/net/ftgmac100.h
> > F: docs/system/arm/aspeed.rst
> > F: tests/qtest/*aspeed*
> > F: hw/misc/fby35_sb_cpld.c
> > +F: hw/misc/intel_me.c
> > NRF51
> > M: Joel Stanley <joel@jms.id.au>
> > diff --git a/hw/misc/intel_me.c b/hw/misc/intel_me.c
> > new file mode 100644
> > index 0000000000..933ae45101
> > --- /dev/null
> > +++ b/hw/misc/intel_me.c
> > @@ -0,0 +1,162 @@
> > +/*
> > + * Copyright (c) Meta Platforms, Inc. and affiliates. (http://www.meta.com)
> > + *
> > + * This code is licensed under the GPL version 2 or later. See the COPYING
> > + * file in the top-level directory.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/main-loop.h"
> > +#include "hw/i2c/i2c.h"
> > +#include "trace.h"
> > +
> > +#define TYPE_INTEL_ME "intel-me"
> > +OBJECT_DECLARE_SIMPLE_TYPE(IntelMEState, INTEL_ME);
> > +
> > +struct IntelMEState {
> > + I2CSlave parent_obj;
> > +
> > + I2CBus *bus;
> > + QEMUBH *bh;
> > + int rx_len;
> > + int tx_len;
> > + int tx_pos;
> > + uint8_t rx_buf[512];
> > + uint8_t tx_buf[512];
> > +};
> > +
> > +static void intel_me_bh(void *opaque)
> > +{
> > + IntelMEState *s = opaque;
> > + I2CSlave *i2c = I2C_SLAVE(s);
> > + uint8_t target_addr;
> > +
> > + assert(s->bus->bh == s->bh);
> > +
> > + switch (s->tx_pos) {
> > + case 0:
> > + target_addr = s->tx_buf[s->tx_pos++];
> > + trace_intel_me_tx_start(i2c->address, target_addr);
> > + if (i2c_start_send_async(s->bus, target_addr) != 0) {
> > + break;
> > + }
> > + return;
> > + default:
> > + if (s->tx_pos >= s->tx_len) {
> > + break;
> > + }
> > + trace_intel_me_tx_data(i2c->address, s->tx_buf[s->tx_pos]);
> > + if (i2c_send_async(s->bus, s->tx_buf[s->tx_pos++]) != 0) {
> > + break;
> > + }
> > + return;
> > + }
> > +
> > + trace_intel_me_tx_end(i2c->address);
> > + i2c_end_transfer(s->bus);
> > + i2c_bus_release(s->bus);
> > + s->tx_len = 0;
> > + s->tx_pos = 0;
> > + memset(s->tx_buf, 0, sizeof(s->tx_buf));
> > +}
> > +
> > +static void intel_me_realize(DeviceState *dev, Error **errp)
> > +{
> > + IntelMEState *s = INTEL_ME(dev);
> > +
> > + s->bus = I2C_BUS(qdev_get_parent_bus(dev));
> > + s->bh = qemu_bh_new(intel_me_bh, s);
> > + s->rx_len = 0;
> > + s->tx_len = 0;
> > + s->tx_pos = 0;
> > + memset(s->rx_buf, 0, sizeof(s->rx_buf));
> > + memset(s->tx_buf, 0, sizeof(s->tx_buf));
> > +}
> > +
> > +static uint8_t checksum(const uint8_t *ptr, int len)
> > +{
> > + int sum = 0;
> > +
> > + for (int i = 0; i < len; i++) {
> > + sum += ptr[i];
> > + }
> > +
> > + return 256 - sum;
> > +}
> > +
> > +static int intel_me_i2c_event(I2CSlave *i2c, enum i2c_event event)
> > +{
> > + IntelMEState *s = INTEL_ME(i2c);
> > +
> > + switch (event) {
> > + case I2C_START_RECV:
> > + break;
> > + case I2C_START_SEND:
> > + trace_intel_me_rx_start(i2c->address);
> > + s->rx_len = 0;
> > + memset(s->rx_buf, 0, sizeof(s->rx_buf));
> > + break;
> > + case I2C_START_SEND_ASYNC:
> > + break;
> > + case I2C_FINISH:
> > + trace_intel_me_rx_end(i2c->address);
> > + s->tx_len = 10;
> > + s->tx_pos = 0;
> > + s->tx_buf[0] = s->rx_buf[2];
> > + s->tx_buf[1] = ((s->rx_buf[0] >> 2) + 1) << 2;
> > + s->tx_buf[2] = checksum(s->tx_buf, 2);
> > + s->tx_buf[3] = i2c->address;
> > + s->tx_buf[4] = (s->rx_buf[3] >> 2) << 2;
> > + s->tx_buf[5] = s->rx_buf[4];
> > + s->tx_buf[6] = 0x00;
> > + s->tx_buf[7] = 0x55;
> > + s->tx_buf[8] = 0x00;
> > + s->tx_buf[9] = checksum(s->tx_buf, s->tx_len - 1);
> > + s->tx_buf[0] >>= 1;
> > + i2c_bus_master(s->bus, s->bh);
> > + break;
> > + case I2C_NACK:
> > + break;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static uint8_t intel_me_i2c_recv(I2CSlave *i2c)
> > +{
> > + return 0xff;
> > +}
> > +
> > +static int intel_me_i2c_send(I2CSlave *i2c, uint8_t data)
> > +{
> > + IntelMEState *s = INTEL_ME(i2c);
> > +
> > + trace_intel_me_rx_data(i2c->address, data);
> > +
> > + assert(s->rx_len < sizeof(s->rx_buf));
> > + s->rx_buf[s->rx_len++] = data;
> > +
> > + return 0;
> > +}
> > +
> > +static void intel_me_class_init(ObjectClass *oc, void *data)
> > +{
> > + DeviceClass *dc = DEVICE_CLASS(oc);
> > + I2CSlaveClass *i2c = I2C_SLAVE_CLASS(oc);
> > +
> > + dc->realize = intel_me_realize;
> > + i2c->event = intel_me_i2c_event;
> > + i2c->recv = intel_me_i2c_recv;
> > + i2c->send = intel_me_i2c_send;
> > +}
> > +
> > +static const TypeInfo types[] = {
> > + {
> > + .name = TYPE_INTEL_ME,
> > + .parent = TYPE_I2C_SLAVE,
> > + .instance_size = sizeof(IntelMEState),
> > + .class_init = intel_me_class_init,
> > + },
> > +};
> > +
> > +DEFINE_TYPES(types);
> > diff --git a/hw/misc/meson.build b/hw/misc/meson.build
> > index 948e25c440..165b9dce6d 100644
> > --- a/hw/misc/meson.build
> > +++ b/hw/misc/meson.build
> > @@ -118,7 +118,8 @@ softmmu_ss.add(when: 'CONFIG_ASPEED_SOC', if_true: files(
> > 'aspeed_sdmc.c',
> > 'aspeed_xdma.c',
> > 'aspeed_peci.c',
> > - 'fby35_sb_cpld.c'))
> > + 'fby35_sb_cpld.c',
> > + 'intel_me.c'))
> > softmmu_ss.add(when: 'CONFIG_MSF2', if_true: files('msf2-sysreg.c'))
> > softmmu_ss.add(when: 'CONFIG_NRF51_SOC', if_true: files('nrf51_rng.c'))
> > diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> > index 90a0473b06..7ca23bcf27 100644
> > --- a/hw/misc/trace-events
> > +++ b/hw/misc/trace-events
> > @@ -273,3 +273,11 @@ virt_ctrl_instance_init(void *dev) "ctrl: %p"
> > lasi_chip_mem_valid(uint64_t addr, uint32_t val) "access to addr 0x%"PRIx64" is %d"
> > lasi_chip_read(uint64_t addr, uint32_t val) "addr 0x%"PRIx64" val 0x%08x"
> > lasi_chip_write(uint64_t addr, uint32_t val) "addr 0x%"PRIx64" val 0x%08x"
> > +
> > +# intel_me.c
> > +intel_me_rx_start(uint8_t addr) "addr 0x%02x"
> > +intel_me_rx_data(uint8_t addr, uint8_t data) "addr 0x%02x data 0x%02x"
> > +intel_me_rx_end(uint8_t addr) "addr 0x%02x"
> > +intel_me_tx_start(uint8_t addr, uint8_t target_addr) "addr 0x%02x target_addr 0x%02x"
> > +intel_me_tx_data(uint8_t addr, uint8_t data) "addr 0x%02x data 0x%02x"
> > +intel_me_tx_end(uint8_t addr) "addr 0x%02x"
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH v3 14/14] hw/arm/aspeed: Add oby35-cl machine
2022-06-30 4:51 [PATCH v3 00/14] hw/i2c/aspeed: I2C slave mode DMA RX w/ new regs Peter Delevoryas
` (12 preceding siblings ...)
2022-06-30 4:51 ` [PATCH v3 13/14] hw/misc/aspeed: Add intel-me Peter Delevoryas
@ 2022-06-30 4:51 ` Peter Delevoryas
2022-06-30 11:02 ` Cédric Le Goater
2022-06-30 6:13 ` [PATCH v3 00/14] hw/i2c/aspeed: I2C slave mode DMA RX w/ new regs Cédric Le Goater
2022-06-30 7:50 ` Cédric Le Goater
15 siblings, 1 reply; 34+ messages in thread
From: Peter Delevoryas @ 2022-06-30 4:51 UTC (permalink / raw)
Cc: clg, peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel, pdel
From: Peter Delevoryas <pdel@fb.com>
The fby35 machine includes 4 server boards, each of which has a "bridge
interconnect" (BIC). This chip abstracts the pinout for the server board
into a single endpoint that the baseboard management controller (BMC)
can talk to using IPMB.
This commit adds a machine for testing the BIC on the server board. It
runs OpenBIC (https://github.com/facebook/openbic) and the server board
is called CraterLake, so the code name is oby35-cl. There's also a
variant of the baseboard that replaces the BMC with a BIC, but that
machine is not included here.
A test image can be built from https://github.com/facebook/openbic using
the instructions in the README.md to build the meta-facebook/yv35-cl
recipe, or retrieved from my Github:
wget https://github.com/peterdelevoryas/OpenBIC/releases/download/oby35-cl-2022.17.01/Y35BCL.elf
And you can run this machine with the following command:
qemu-system-arm -machine oby35-cl -nographic -kernel Y35BCL.elf
It should produce output like the following:
[00:00:00.005,000] <inf> usb_dc_aspeed: select ep[0x81] as IN endpoint
[00:00:00.006,000] <inf> usb_dc_aspeed: select ep[0x82] as IN endpoint
[00:00:00.006,000] <wrn> usb_dc_aspeed: pre-selected ep[0x1] as IN endpoint
[00:00:00.006,000] <wrn> usb_dc_aspeed: pre-selected ep[0x2] as IN endpoint
[00:00:00.006,000] <inf> usb_dc_aspeed: select ep[0x3] as OUT endpoint
*** Booting Zephyr OS build v00.01.05 ***
Hello, welcome to yv35 craterlake 2022.25.1
BIC class type(class-1), 1ou present status(0), 2ou present status(0), board revision(0x1)
check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
[init_drive_type] sensor 0x14 post sensor read failed!
[init_drive_type] sensor 0x30 post sensor read failed!
[init_drive_type] sensor 0x39 post sensor read failed!
ipmi_init
[set_DC_status] gpio number(15) status(0)
[set_post_status] gpio number(1) status(1)
uart:~$ [00:00:01.010,000] <inf> kcs_aspeed: KCS3: addr=0xca2, idr=0x2c, odr=0x38, str=0x44
[00:00:01.016,000] <err> spi_nor_multi_dev: [1216][spi1_cs0]SFDP magic 00000000 invalid
[00:00:01.016,000] <err> spi_nor_multi_dev: [1456]SFDP read failed: -22
[00:00:01.010,000] <inf> kcs_aspeed: KCS3: addr=0xca2, idr=0x2c, odr=0x38, str=0x44
[00:00:01.016,000] <err> spi_nor_multi_dev: [1216][spi1_cs0]SFDP magic 00000000 invalid
[00:00:01.016,000] <err> spi_nor_multi_dev: [1456]SFDP read failed: -22
uart:~$ BIC Ready
Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
hw/arm/aspeed.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index a06f7c1b62..75971ef2ca 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -1429,6 +1429,50 @@ static void aspeed_minibmc_machine_ast1030_evb_class_init(ObjectClass *oc,
amc->macs_mask = 0;
}
+static void oby35_cl_i2c_init(AspeedMachineState *bmc)
+{
+ AspeedSoCState *soc = &bmc->soc;
+ I2CBus *i2c[14];
+ I2CBus *ssd[8];
+ int i;
+
+ for (i = 0; i < 14; i++) {
+ i2c[i] = aspeed_i2c_get_bus(&soc->i2c, i);
+ }
+ get_pca9548_channels(i2c[1], 0x71, ssd);
+
+ i2c_slave_create_simple(i2c[0], "fby35-sb-cpld", 0x21);
+ i2c_slave_create_simple(i2c[1], "tmp105", 0x48);
+ i2c_slave_create_simple(i2c[1], "tmp105", 0x49);
+ i2c_slave_create_simple(i2c[1], "tmp105", 0x4a);
+ i2c_slave_create_simple(i2c[1], "adm1272", 0x40);
+ i2c_slave_create_simple(i2c[1], "tmp421", 0x4c);
+ i2c_slave_create_simple(i2c[2], "intel-me", 0x16);
+ i2c_slave_create_simple(i2c[4], "isl69259", 0x76);
+ i2c_slave_create_simple(i2c[4], "isl69259", 0x62);
+ i2c_slave_create_simple(i2c[4], "isl69259", 0x60);
+
+ for (int i = 0; i < 8; i++) {
+ i2c_slave_create_simple(ssd[i], "tmp105", 0x6a);
+ }
+
+ /*
+ * FIXME: This should actually be the BMC, but both the ME and the BMC
+ * are IPMB endpoints, and the current ME implementation is generic
+ * enough to respond normally to some things.
+ */
+ i2c_slave_create_simple(i2c[6], "intel-me", 0x10);
+}
+
+static void aspeed_machine_oby35_cl_class_init(ObjectClass *oc, void *data)
+{
+ MachineClass *mc = MACHINE_CLASS(oc);
+ AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
+
+ mc->desc = "Meta Platforms fby35 CraterLake BIC (Cortex-M4)";
+ amc->i2c_init = oby35_cl_i2c_init;
+}
+
static const TypeInfo aspeed_machine_types[] = {
{
.name = MACHINE_TYPE_NAME("palmetto-bmc"),
@@ -1494,6 +1538,10 @@ static const TypeInfo aspeed_machine_types[] = {
.name = MACHINE_TYPE_NAME("ast1030-evb"),
.parent = TYPE_ASPEED_MACHINE,
.class_init = aspeed_minibmc_machine_ast1030_evb_class_init,
+ }, {
+ .name = MACHINE_TYPE_NAME("oby35-cl"),
+ .parent = MACHINE_TYPE_NAME("ast1030-evb"),
+ .class_init = aspeed_machine_oby35_cl_class_init,
}, {
.name = TYPE_ASPEED_MACHINE,
.parent = TYPE_MACHINE,
--
2.37.0
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [PATCH v3 14/14] hw/arm/aspeed: Add oby35-cl machine
2022-06-30 4:51 ` [PATCH v3 14/14] hw/arm/aspeed: Add oby35-cl machine Peter Delevoryas
@ 2022-06-30 11:02 ` Cédric Le Goater
2022-06-30 16:15 ` Peter Delevoryas
0 siblings, 1 reply; 34+ messages in thread
From: Cédric Le Goater @ 2022-06-30 11:02 UTC (permalink / raw)
To: Peter Delevoryas
Cc: peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel, pdel
On 6/30/22 06:51, Peter Delevoryas wrote:
> From: Peter Delevoryas <pdel@fb.com>
>
> The fby35 machine includes 4 server boards, each of which has a "bridge
> interconnect" (BIC). This chip abstracts the pinout for the server board
> into a single endpoint that the baseboard management controller (BMC)
> can talk to using IPMB.
>
> This commit adds a machine for testing the BIC on the server board. It
> runs OpenBIC (https://github.com/facebook/openbic) and the server board
> is called CraterLake, so the code name is oby35-cl. There's also a
> variant of the baseboard that replaces the BMC with a BIC, but that
> machine is not included here.
>
> A test image can be built from https://github.com/facebook/openbic using
> the instructions in the README.md to build the meta-facebook/yv35-cl
> recipe, or retrieved from my Github:
>
> wget https://github.com/peterdelevoryas/OpenBIC/releases/download/oby35-cl-2022.17.01/Y35BCL.elf
>
> And you can run this machine with the following command:
>
> qemu-system-arm -machine oby35-cl -nographic -kernel Y35BCL.elf
>
> It should produce output like the following:
>
> [00:00:00.005,000] <inf> usb_dc_aspeed: select ep[0x81] as IN endpoint
> [00:00:00.006,000] <inf> usb_dc_aspeed: select ep[0x82] as IN endpoint
> [00:00:00.006,000] <wrn> usb_dc_aspeed: pre-selected ep[0x1] as IN endpoint
> [00:00:00.006,000] <wrn> usb_dc_aspeed: pre-selected ep[0x2] as IN endpoint
> [00:00:00.006,000] <inf> usb_dc_aspeed: select ep[0x3] as OUT endpoint
> *** Booting Zephyr OS build v00.01.05 ***
> Hello, welcome to yv35 craterlake 2022.25.1
> BIC class type(class-1), 1ou present status(0), 2ou present status(0), board revision(0x1)
> check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> [init_drive_type] sensor 0x14 post sensor read failed!
>
> [init_drive_type] sensor 0x30 post sensor read failed!
> [init_drive_type] sensor 0x39 post sensor read failed!
> ipmi_init
> [set_DC_status] gpio number(15) status(0)
> [set_post_status] gpio number(1) status(1)
> uart:~$ [00:00:01.010,000] <inf> kcs_aspeed: KCS3: addr=0xca2, idr=0x2c, odr=0x38, str=0x44
>
> [00:00:01.016,000] <err> spi_nor_multi_dev: [1216][spi1_cs0]SFDP magic 00000000 invalid
> [00:00:01.016,000] <err> spi_nor_multi_dev: [1456]SFDP read failed: -22
> [00:00:01.010,000] <inf> kcs_aspeed: KCS3: addr=0xca2, idr=0x2c, odr=0x38, str=0x44
>
> [00:00:01.016,000] <err> spi_nor_multi_dev: [1216][spi1_cs0]SFDP magic 00000000 invalid
> [00:00:01.016,000] <err> spi_nor_multi_dev: [1456]SFDP read failed: -22
> uart:~$ BIC Ready
>
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
LGTM.
That said I would prefer to introduce the machine first and then
populate with devices.
May be it is time to introduce a new machine file. This one seems
like it could go in a f35.c file, also because a larger f35-* is
in plan. aspeed.c could contain the basic definitions and helpers.
> ---
> hw/arm/aspeed.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 48 insertions(+)
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index a06f7c1b62..75971ef2ca 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -1429,6 +1429,50 @@ static void aspeed_minibmc_machine_ast1030_evb_class_init(ObjectClass *oc,
> amc->macs_mask = 0;
> }
>
> +static void oby35_cl_i2c_init(AspeedMachineState *bmc)
> +{
> + AspeedSoCState *soc = &bmc->soc;
> + I2CBus *i2c[14];
> + I2CBus *ssd[8];
> + int i;
> +
> + for (i = 0; i < 14; i++) {
> + i2c[i] = aspeed_i2c_get_bus(&soc->i2c, i);
> + }
> + get_pca9548_channels(i2c[1], 0x71, ssd);
We should rename to aspeed_get_pca9548_channels
> +
> + i2c_slave_create_simple(i2c[0], "fby35-sb-cpld", 0x21);
> + i2c_slave_create_simple(i2c[1], "tmp105", 0x48);
> + i2c_slave_create_simple(i2c[1], "tmp105", 0x49);
> + i2c_slave_create_simple(i2c[1], "tmp105", 0x4a);
> + i2c_slave_create_simple(i2c[1], "adm1272", 0x40);
> + i2c_slave_create_simple(i2c[1], "tmp421", 0x4c);
> + i2c_slave_create_simple(i2c[2], "intel-me", 0x16);
> + i2c_slave_create_simple(i2c[4], "isl69259", 0x76);
> + i2c_slave_create_simple(i2c[4], "isl69259", 0x62);
> + i2c_slave_create_simple(i2c[4], "isl69259", 0x60);
> +
> + for (int i = 0; i < 8; i++) {
> + i2c_slave_create_simple(ssd[i], "tmp105", 0x6a);
> + }
> +
> + /*
> + * FIXME: This should actually be the BMC, but both the ME and the BMC
QEMU has an embedded IPMI BMC simulator.
> + * are IPMB endpoints, and the current ME implementation is generic
> + * enough to respond normally to some things.
> + */
> + i2c_slave_create_simple(i2c[6], "intel-me", 0x10);
> +}
> +
> +static void aspeed_machine_oby35_cl_class_init(ObjectClass *oc, void *data)
> +{
> + MachineClass *mc = MACHINE_CLASS(oc);
> + AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
> +
> + mc->desc = "Meta Platforms fby35 CraterLake BIC (Cortex-M4)";
> + amc->i2c_init = oby35_cl_i2c_init;
> +}
> +
> static const TypeInfo aspeed_machine_types[] = {
> {
> .name = MACHINE_TYPE_NAME("palmetto-bmc"),
> @@ -1494,6 +1538,10 @@ static const TypeInfo aspeed_machine_types[] = {
> .name = MACHINE_TYPE_NAME("ast1030-evb"),
> .parent = TYPE_ASPEED_MACHINE,
> .class_init = aspeed_minibmc_machine_ast1030_evb_class_init,
> + }, {
> + .name = MACHINE_TYPE_NAME("oby35-cl"),
> + .parent = MACHINE_TYPE_NAME("ast1030-evb"),
hmm, so we are inheriting from the evb ?
C.
> + .class_init = aspeed_machine_oby35_cl_class_init,
> }, {
> .name = TYPE_ASPEED_MACHINE,
> .parent = TYPE_MACHINE,
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 14/14] hw/arm/aspeed: Add oby35-cl machine
2022-06-30 11:02 ` Cédric Le Goater
@ 2022-06-30 16:15 ` Peter Delevoryas
2022-06-30 16:42 ` Cédric Le Goater
0 siblings, 1 reply; 34+ messages in thread
From: Peter Delevoryas @ 2022-06-30 16:15 UTC (permalink / raw)
To: Cédric Le Goater
Cc: Peter Delevoryas, peter.maydell, andrew, joel, cminyard, titusr,
qemu-devel, qemu-arm, zhdaniel
On Thu, Jun 30, 2022 at 01:02:54PM +0200, Cédric Le Goater wrote:
> On 6/30/22 06:51, Peter Delevoryas wrote:
> > From: Peter Delevoryas <pdel@fb.com>
> >
> > The fby35 machine includes 4 server boards, each of which has a "bridge
> > interconnect" (BIC). This chip abstracts the pinout for the server board
> > into a single endpoint that the baseboard management controller (BMC)
> > can talk to using IPMB.
> >
> > This commit adds a machine for testing the BIC on the server board. It
> > runs OpenBIC (https://github.com/facebook/openbic) and the server board
> > is called CraterLake, so the code name is oby35-cl. There's also a
> > variant of the baseboard that replaces the BMC with a BIC, but that
> > machine is not included here.
> >
> > A test image can be built from https://github.com/facebook/openbic using
> > the instructions in the README.md to build the meta-facebook/yv35-cl
> > recipe, or retrieved from my Github:
> >
> > wget https://github.com/peterdelevoryas/OpenBIC/releases/download/oby35-cl-2022.17.01/Y35BCL.elf
> >
> > And you can run this machine with the following command:
> >
> > qemu-system-arm -machine oby35-cl -nographic -kernel Y35BCL.elf
> >
> > It should produce output like the following:
> >
> > [00:00:00.005,000] <inf> usb_dc_aspeed: select ep[0x81] as IN endpoint
> > [00:00:00.006,000] <inf> usb_dc_aspeed: select ep[0x82] as IN endpoint
> > [00:00:00.006,000] <wrn> usb_dc_aspeed: pre-selected ep[0x1] as IN endpoint
> > [00:00:00.006,000] <wrn> usb_dc_aspeed: pre-selected ep[0x2] as IN endpoint
> > [00:00:00.006,000] <inf> usb_dc_aspeed: select ep[0x3] as OUT endpoint
> > *** Booting Zephyr OS build v00.01.05 ***
> > Hello, welcome to yv35 craterlake 2022.25.1
> > BIC class type(class-1), 1ou present status(0), 2ou present status(0), board revision(0x1)
> > check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > [init_drive_type] sensor 0x14 post sensor read failed!
> >
> > [init_drive_type] sensor 0x30 post sensor read failed!
> > [init_drive_type] sensor 0x39 post sensor read failed!
> > ipmi_init
> > [set_DC_status] gpio number(15) status(0)
> > [set_post_status] gpio number(1) status(1)
> > uart:~$ [00:00:01.010,000] <inf> kcs_aspeed: KCS3: addr=0xca2, idr=0x2c, odr=0x38, str=0x44
> >
> > [00:00:01.016,000] <err> spi_nor_multi_dev: [1216][spi1_cs0]SFDP magic 00000000 invalid
> > [00:00:01.016,000] <err> spi_nor_multi_dev: [1456]SFDP read failed: -22
> > [00:00:01.010,000] <inf> kcs_aspeed: KCS3: addr=0xca2, idr=0x2c, odr=0x38, str=0x44
> >
> > [00:00:01.016,000] <err> spi_nor_multi_dev: [1216][spi1_cs0]SFDP magic 00000000 invalid
> > [00:00:01.016,000] <err> spi_nor_multi_dev: [1456]SFDP read failed: -22
> > uart:~$ BIC Ready
> >
> > Signed-off-by: Peter Delevoryas <pdel@fb.com>
>
> LGTM.
>
> That said I would prefer to introduce the machine first and then
> populate with devices.
Ohh ok, I'll submit the machine definition separately all by itself and then
submit any extra devices like the CPLD or ME afterwards.
>
> May be it is time to introduce a new machine file. This one seems
> like it could go in a f35.c file, also because a larger f35-* is
> in plan. aspeed.c could contain the basic definitions and helpers.
Yes, patrick@stwcx.xyz was thinking the same thing. An f35.c (well,
maybe yv35.c or fby35.c would be more appropriate) would be a good
idea. I'll submit another patch up front to move fby35 stuff to
a separate file.
>
> > ---
> > hw/arm/aspeed.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 48 insertions(+)
> >
> > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> > index a06f7c1b62..75971ef2ca 100644
> > --- a/hw/arm/aspeed.c
> > +++ b/hw/arm/aspeed.c
> > @@ -1429,6 +1429,50 @@ static void aspeed_minibmc_machine_ast1030_evb_class_init(ObjectClass *oc,
> > amc->macs_mask = 0;
> > }
> > +static void oby35_cl_i2c_init(AspeedMachineState *bmc)
> > +{
> > + AspeedSoCState *soc = &bmc->soc;
> > + I2CBus *i2c[14];
> > + I2CBus *ssd[8];
> > + int i;
> > +
> > + for (i = 0; i < 14; i++) {
> > + i2c[i] = aspeed_i2c_get_bus(&soc->i2c, i);
> > + }
> > + get_pca9548_channels(i2c[1], 0x71, ssd);
>
> We should rename to aspeed_get_pca9548_channels
+1
>
> > +
> > + i2c_slave_create_simple(i2c[0], "fby35-sb-cpld", 0x21);
> > + i2c_slave_create_simple(i2c[1], "tmp105", 0x48);
> > + i2c_slave_create_simple(i2c[1], "tmp105", 0x49);
> > + i2c_slave_create_simple(i2c[1], "tmp105", 0x4a);
> > + i2c_slave_create_simple(i2c[1], "adm1272", 0x40);
> > + i2c_slave_create_simple(i2c[1], "tmp421", 0x4c);
> > + i2c_slave_create_simple(i2c[2], "intel-me", 0x16);
> > + i2c_slave_create_simple(i2c[4], "isl69259", 0x76);
> > + i2c_slave_create_simple(i2c[4], "isl69259", 0x62);
> > + i2c_slave_create_simple(i2c[4], "isl69259", 0x60);
> > +
> > + for (int i = 0; i < 8; i++) {
> > + i2c_slave_create_simple(ssd[i], "tmp105", 0x6a);
> > + }
> > +
> > + /*
> > + * FIXME: This should actually be the BMC, but both the ME and the BMC
>
> QEMU has an embedded IPMI BMC simulator.
!!! Didn't realize this, definitely going to try using it.
>
> > + * are IPMB endpoints, and the current ME implementation is generic
> > + * enough to respond normally to some things.
> > + */
> > + i2c_slave_create_simple(i2c[6], "intel-me", 0x10);
> > +}
> > +
> > +static void aspeed_machine_oby35_cl_class_init(ObjectClass *oc, void *data)
> > +{
> > + MachineClass *mc = MACHINE_CLASS(oc);
> > + AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
> > +
> > + mc->desc = "Meta Platforms fby35 CraterLake BIC (Cortex-M4)";
> > + amc->i2c_init = oby35_cl_i2c_init;
> > +}
> > +
> > static const TypeInfo aspeed_machine_types[] = {
> > {
> > .name = MACHINE_TYPE_NAME("palmetto-bmc"),
> > @@ -1494,6 +1538,10 @@ static const TypeInfo aspeed_machine_types[] = {
> > .name = MACHINE_TYPE_NAME("ast1030-evb"),
> > .parent = TYPE_ASPEED_MACHINE,
> > .class_init = aspeed_minibmc_machine_ast1030_evb_class_init,
> > + }, {
> > + .name = MACHINE_TYPE_NAME("oby35-cl"),
> > + .parent = MACHINE_TYPE_NAME("ast1030-evb"),
>
> hmm, so we are inheriting from the evb ?
Yeah, I remember this was controversial with fby35-bmc too, maybe I'll
change this in the follow-up. I just like inheriting from the EVB's because
people use the EVB's a lot for testing, most of the time I'm just trying to
add some extra i2c devices/etc, so I override the i2c init. But, maybe it's
good to decouple them.
>
> C.
>
>
> > + .class_init = aspeed_machine_oby35_cl_class_init,
> > }, {
> > .name = TYPE_ASPEED_MACHINE,
> > .parent = TYPE_MACHINE,
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 14/14] hw/arm/aspeed: Add oby35-cl machine
2022-06-30 16:15 ` Peter Delevoryas
@ 2022-06-30 16:42 ` Cédric Le Goater
2022-06-30 17:48 ` Peter Delevoryas
2022-06-30 23:06 ` Peter Delevoryas
0 siblings, 2 replies; 34+ messages in thread
From: Cédric Le Goater @ 2022-06-30 16:42 UTC (permalink / raw)
To: Peter Delevoryas
Cc: peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel
On 6/30/22 18:15, Peter Delevoryas wrote:
> On Thu, Jun 30, 2022 at 01:02:54PM +0200, Cédric Le Goater wrote:
>> On 6/30/22 06:51, Peter Delevoryas wrote:
>>> From: Peter Delevoryas <pdel@fb.com>
>>>
>>> The fby35 machine includes 4 server boards, each of which has a "bridge
>>> interconnect" (BIC). This chip abstracts the pinout for the server board
>>> into a single endpoint that the baseboard management controller (BMC)
>>> can talk to using IPMB.
>>>
>>> This commit adds a machine for testing the BIC on the server board. It
>>> runs OpenBIC (https://github.com/facebook/openbic) and the server board
>>> is called CraterLake, so the code name is oby35-cl. There's also a
>>> variant of the baseboard that replaces the BMC with a BIC, but that
>>> machine is not included here.
>>>
>>> A test image can be built from https://github.com/facebook/openbic using
>>> the instructions in the README.md to build the meta-facebook/yv35-cl
>>> recipe, or retrieved from my Github:
>>>
>>> wget https://github.com/peterdelevoryas/OpenBIC/releases/download/oby35-cl-2022.17.01/Y35BCL.elf
>>>
>>> And you can run this machine with the following command:
>>>
>>> qemu-system-arm -machine oby35-cl -nographic -kernel Y35BCL.elf
>>>
>>> It should produce output like the following:
>>>
>>> [00:00:00.005,000] <inf> usb_dc_aspeed: select ep[0x81] as IN endpoint
>>> [00:00:00.006,000] <inf> usb_dc_aspeed: select ep[0x82] as IN endpoint
>>> [00:00:00.006,000] <wrn> usb_dc_aspeed: pre-selected ep[0x1] as IN endpoint
>>> [00:00:00.006,000] <wrn> usb_dc_aspeed: pre-selected ep[0x2] as IN endpoint
>>> [00:00:00.006,000] <inf> usb_dc_aspeed: select ep[0x3] as OUT endpoint
>>> *** Booting Zephyr OS build v00.01.05 ***
>>> Hello, welcome to yv35 craterlake 2022.25.1
>>> BIC class type(class-1), 1ou present status(0), 2ou present status(0), board revision(0x1)
>>> check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>> check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>> check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>> check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>> check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>> check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>> check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>> check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>> check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>> check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>> check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>> check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>> check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>> check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>> check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>> check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>> check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>> check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>> check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>> check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>> [init_drive_type] sensor 0x14 post sensor read failed!
>>>
>>> [init_drive_type] sensor 0x30 post sensor read failed!
>>> [init_drive_type] sensor 0x39 post sensor read failed!
>>> ipmi_init
>>> [set_DC_status] gpio number(15) status(0)
>>> [set_post_status] gpio number(1) status(1)
>>> uart:~$ [00:00:01.010,000] <inf> kcs_aspeed: KCS3: addr=0xca2, idr=0x2c, odr=0x38, str=0x44
>>>
>>> [00:00:01.016,000] <err> spi_nor_multi_dev: [1216][spi1_cs0]SFDP magic 00000000 invalid
>>> [00:00:01.016,000] <err> spi_nor_multi_dev: [1456]SFDP read failed: -22
>>> [00:00:01.010,000] <inf> kcs_aspeed: KCS3: addr=0xca2, idr=0x2c, odr=0x38, str=0x44
>>>
>>> [00:00:01.016,000] <err> spi_nor_multi_dev: [1216][spi1_cs0]SFDP magic 00000000 invalid
>>> [00:00:01.016,000] <err> spi_nor_multi_dev: [1456]SFDP read failed: -22
>>> uart:~$ BIC Ready
>>>
>>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>>
>> LGTM.
>>
>> That said I would prefer to introduce the machine first and then
>> populate with devices.
>
> Ohh ok, I'll submit the machine definition separately all by itself and then
> submit any extra devices like the CPLD or ME afterwards.
I have kept the "full system" in my tree for now :
cb4481ae1812 aspeed: Add AST2600 (BMC) to fby35 (full system)
c155bf27d3e7 aspeed: Make aspeed_board_init_flashes public (trivial)
3f5485fa88b9 aspeed: Add fby35 skeleton (trivial)
because the ROM vs. execute-in-place is being analyzed. Let's see if
we can make progress and simplify the initial machine.
I have also kept the latest *fby35* emulating the BIC only :
5cfc4b68fdb8 hw/arm/aspeed: Add oby35-cl machine
06f21e024ee7 hw/misc/aspeed: Add intel-me
e96a23571599 hw/misc/aspeed: Add fby35-sb-cpld
to discuss a bit more on the names, files, IPMI, etc. Until now, we had
Aspeed machines modeling EVBs or BMCs. BICs and multi SoC system are new.
Having a review on the common models in 8-10 would be nice.
2a9be57901a3 hw/sensor: Add Renesas ISL69259 device model
85f8352e213a hw/sensor: Add IC_DEVICE_ID to ISL voltage regulators
aea568d56db5 hw/i2c/pmbus: Add idle state to return 0xff's
They should not be too problematic to merge. As soon as Titus has time
to take a look we will know, and I did a comment. So this can be addressed
in parallel with the fby35 machines.
Thanks,
C.
>
>>
>> May be it is time to introduce a new machine file. This one seems
>> like it could go in a f35.c file, also because a larger f35-* is
>> in plan. aspeed.c could contain the basic definitions and helpers.
>
> Yes, patrick@stwcx.xyz was thinking the same thing. An f35.c (well,
> maybe yv35.c or fby35.c would be more appropriate) would be a good
> idea. I'll submit another patch up front to move fby35 stuff to
> a separate file.
>
>>
>>> ---
>>> hw/arm/aspeed.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 48 insertions(+)
>>>
>>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>>> index a06f7c1b62..75971ef2ca 100644
>>> --- a/hw/arm/aspeed.c
>>> +++ b/hw/arm/aspeed.c
>>> @@ -1429,6 +1429,50 @@ static void aspeed_minibmc_machine_ast1030_evb_class_init(ObjectClass *oc,
>>> amc->macs_mask = 0;
>>> }
>>> +static void oby35_cl_i2c_init(AspeedMachineState *bmc)
>>> +{
>>> + AspeedSoCState *soc = &bmc->soc;
>>> + I2CBus *i2c[14];
>>> + I2CBus *ssd[8];
>>> + int i;
>>> +
>>> + for (i = 0; i < 14; i++) {
>>> + i2c[i] = aspeed_i2c_get_bus(&soc->i2c, i);
>>> + }
>>> + get_pca9548_channels(i2c[1], 0x71, ssd);
>>
>> We should rename to aspeed_get_pca9548_channels
>
> +1
>
>>
>>> +
>>> + i2c_slave_create_simple(i2c[0], "fby35-sb-cpld", 0x21);
>>> + i2c_slave_create_simple(i2c[1], "tmp105", 0x48);
>>> + i2c_slave_create_simple(i2c[1], "tmp105", 0x49);
>>> + i2c_slave_create_simple(i2c[1], "tmp105", 0x4a);
>>> + i2c_slave_create_simple(i2c[1], "adm1272", 0x40);
>>> + i2c_slave_create_simple(i2c[1], "tmp421", 0x4c);
>>> + i2c_slave_create_simple(i2c[2], "intel-me", 0x16);
>>> + i2c_slave_create_simple(i2c[4], "isl69259", 0x76);
>>> + i2c_slave_create_simple(i2c[4], "isl69259", 0x62);
>>> + i2c_slave_create_simple(i2c[4], "isl69259", 0x60);
>>> +
>>> + for (int i = 0; i < 8; i++) {
>>> + i2c_slave_create_simple(ssd[i], "tmp105", 0x6a);
>>> + }
>>> +
>>> + /*
>>> + * FIXME: This should actually be the BMC, but both the ME and the BMC
>>
>> QEMU has an embedded IPMI BMC simulator.
>
>
> !!! Didn't realize this, definitely going to try using it.
>
>>
>>> + * are IPMB endpoints, and the current ME implementation is generic
>>> + * enough to respond normally to some things.
>>> + */
>>> + i2c_slave_create_simple(i2c[6], "intel-me", 0x10);
>>> +}
>>> +
>>> +static void aspeed_machine_oby35_cl_class_init(ObjectClass *oc, void *data)
>>> +{
>>> + MachineClass *mc = MACHINE_CLASS(oc);
>>> + AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
>>> +
>>> + mc->desc = "Meta Platforms fby35 CraterLake BIC (Cortex-M4)";
>>> + amc->i2c_init = oby35_cl_i2c_init;
>>> +}
>>> +
>>> static const TypeInfo aspeed_machine_types[] = {
>>> {
>>> .name = MACHINE_TYPE_NAME("palmetto-bmc"),
>>> @@ -1494,6 +1538,10 @@ static const TypeInfo aspeed_machine_types[] = {
>>> .name = MACHINE_TYPE_NAME("ast1030-evb"),
>>> .parent = TYPE_ASPEED_MACHINE,
>>> .class_init = aspeed_minibmc_machine_ast1030_evb_class_init,
>>> + }, {
>>> + .name = MACHINE_TYPE_NAME("oby35-cl"),
>>> + .parent = MACHINE_TYPE_NAME("ast1030-evb"),
>>
>> hmm, so we are inheriting from the evb ?
>
> Yeah, I remember this was controversial with fby35-bmc too, maybe I'll
> change this in the follow-up. I just like inheriting from the EVB's because
> people use the EVB's a lot for testing, most of the time I'm just trying to
> add some extra i2c devices/etc, so I override the i2c init. But, maybe it's
> good to decouple them.
>
>>
>> C.
>>
>>
>>> + .class_init = aspeed_machine_oby35_cl_class_init,
>>> }, {
>>> .name = TYPE_ASPEED_MACHINE,
>>> .parent = TYPE_MACHINE,
>>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 14/14] hw/arm/aspeed: Add oby35-cl machine
2022-06-30 16:42 ` Cédric Le Goater
@ 2022-06-30 17:48 ` Peter Delevoryas
2022-06-30 23:06 ` Peter Delevoryas
1 sibling, 0 replies; 34+ messages in thread
From: Peter Delevoryas @ 2022-06-30 17:48 UTC (permalink / raw)
To: Cédric Le Goater
Cc: peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel
On Thu, Jun 30, 2022 at 06:42:52PM +0200, Cédric Le Goater wrote:
> On 6/30/22 18:15, Peter Delevoryas wrote:
> > On Thu, Jun 30, 2022 at 01:02:54PM +0200, Cédric Le Goater wrote:
> > > On 6/30/22 06:51, Peter Delevoryas wrote:
> > > > From: Peter Delevoryas <pdel@fb.com>
> > > >
> > > > The fby35 machine includes 4 server boards, each of which has a "bridge
> > > > interconnect" (BIC). This chip abstracts the pinout for the server board
> > > > into a single endpoint that the baseboard management controller (BMC)
> > > > can talk to using IPMB.
> > > >
> > > > This commit adds a machine for testing the BIC on the server board. It
> > > > runs OpenBIC (https://github.com/facebook/openbic) and the server board
> > > > is called CraterLake, so the code name is oby35-cl. There's also a
> > > > variant of the baseboard that replaces the BMC with a BIC, but that
> > > > machine is not included here.
> > > >
> > > > A test image can be built from https://github.com/facebook/openbic using
> > > > the instructions in the README.md to build the meta-facebook/yv35-cl
> > > > recipe, or retrieved from my Github:
> > > >
> > > > wget https://github.com/peterdelevoryas/OpenBIC/releases/download/oby35-cl-2022.17.01/Y35BCL.elf
> > > >
> > > > And you can run this machine with the following command:
> > > >
> > > > qemu-system-arm -machine oby35-cl -nographic -kernel Y35BCL.elf
> > > >
> > > > It should produce output like the following:
> > > >
> > > > [00:00:00.005,000] <inf> usb_dc_aspeed: select ep[0x81] as IN endpoint
> > > > [00:00:00.006,000] <inf> usb_dc_aspeed: select ep[0x82] as IN endpoint
> > > > [00:00:00.006,000] <wrn> usb_dc_aspeed: pre-selected ep[0x1] as IN endpoint
> > > > [00:00:00.006,000] <wrn> usb_dc_aspeed: pre-selected ep[0x2] as IN endpoint
> > > > [00:00:00.006,000] <inf> usb_dc_aspeed: select ep[0x3] as OUT endpoint
> > > > *** Booting Zephyr OS build v00.01.05 ***
> > > > Hello, welcome to yv35 craterlake 2022.25.1
> > > > BIC class type(class-1), 1ou present status(0), 2ou present status(0), board revision(0x1)
> > > > check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > [init_drive_type] sensor 0x14 post sensor read failed!
> > > >
> > > > [init_drive_type] sensor 0x30 post sensor read failed!
> > > > [init_drive_type] sensor 0x39 post sensor read failed!
> > > > ipmi_init
> > > > [set_DC_status] gpio number(15) status(0)
> > > > [set_post_status] gpio number(1) status(1)
> > > > uart:~$ [00:00:01.010,000] <inf> kcs_aspeed: KCS3: addr=0xca2, idr=0x2c, odr=0x38, str=0x44
> > > >
> > > > [00:00:01.016,000] <err> spi_nor_multi_dev: [1216][spi1_cs0]SFDP magic 00000000 invalid
> > > > [00:00:01.016,000] <err> spi_nor_multi_dev: [1456]SFDP read failed: -22
> > > > [00:00:01.010,000] <inf> kcs_aspeed: KCS3: addr=0xca2, idr=0x2c, odr=0x38, str=0x44
> > > >
> > > > [00:00:01.016,000] <err> spi_nor_multi_dev: [1216][spi1_cs0]SFDP magic 00000000 invalid
> > > > [00:00:01.016,000] <err> spi_nor_multi_dev: [1456]SFDP read failed: -22
> > > > uart:~$ BIC Ready
> > > >
> > > > Signed-off-by: Peter Delevoryas <pdel@fb.com>
> > >
> > > LGTM.
> > >
> > > That said I would prefer to introduce the machine first and then
> > > populate with devices.
> >
> > Ohh ok, I'll submit the machine definition separately all by itself and then
> > submit any extra devices like the CPLD or ME afterwards.
>
> I have kept the "full system" in my tree for now :
>
> cb4481ae1812 aspeed: Add AST2600 (BMC) to fby35 (full system)
> c155bf27d3e7 aspeed: Make aspeed_board_init_flashes public (trivial)
> 3f5485fa88b9 aspeed: Add fby35 skeleton (trivial)
>
> because the ROM vs. execute-in-place is being analyzed. Let's see if
> we can make progress and simplify the initial machine.
Yeah I saw that thread! I'm excited to see where it goes. Thanks for
taking the time to get the performance measurements and spur the
discussion.
>
> I have also kept the latest *fby35* emulating the BIC only :
>
> 5cfc4b68fdb8 hw/arm/aspeed: Add oby35-cl machine
> 06f21e024ee7 hw/misc/aspeed: Add intel-me
> e96a23571599 hw/misc/aspeed: Add fby35-sb-cpld
>
> to discuss a bit more on the names, files, IPMI, etc. Until now, we had
> Aspeed machines modeling EVBs or BMCs. BICs and multi SoC system are new.
Oh great, ok.
>
> Having a review on the common models in 8-10 would be nice.
+1
>
> 2a9be57901a3 hw/sensor: Add Renesas ISL69259 device model
> 85f8352e213a hw/sensor: Add IC_DEVICE_ID to ISL voltage regulators
> aea568d56db5 hw/i2c/pmbus: Add idle state to return 0xff's
>
> They should not be too problematic to merge. As soon as Titus has time
> to take a look we will know, and I did a comment. So this can be addressed
> in parallel with the fby35 machines.
Yeah I'm going to resubmit these three separately and cc Titus, and
I'm also going to move the IC_DEVICE_ID to a class property like you
suggested. I'm just wondering if I'll need to add an ISLClass, since
right now it's just a simple type using PMBusDeviceClass, but perhaps
if we switch to a class property it would be acceptable to add the
implementation of the IC_DEVICE_ID read command to the generic pmbus
device implementation, instead of only implementing it for the VR's?
Might need a couple more iterations on that.
>
> Thanks,
>
> C.
>
>
>
> >
> > >
> > > May be it is time to introduce a new machine file. This one seems
> > > like it could go in a f35.c file, also because a larger f35-* is
> > > in plan. aspeed.c could contain the basic definitions and helpers.
> >
> > Yes, patrick@stwcx.xyz was thinking the same thing. An f35.c (well,
> > maybe yv35.c or fby35.c would be more appropriate) would be a good
> > idea. I'll submit another patch up front to move fby35 stuff to
> > a separate file.
> >
> > >
> > > > ---
> > > > hw/arm/aspeed.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 48 insertions(+)
> > > >
> > > > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> > > > index a06f7c1b62..75971ef2ca 100644
> > > > --- a/hw/arm/aspeed.c
> > > > +++ b/hw/arm/aspeed.c
> > > > @@ -1429,6 +1429,50 @@ static void aspeed_minibmc_machine_ast1030_evb_class_init(ObjectClass *oc,
> > > > amc->macs_mask = 0;
> > > > }
> > > > +static void oby35_cl_i2c_init(AspeedMachineState *bmc)
> > > > +{
> > > > + AspeedSoCState *soc = &bmc->soc;
> > > > + I2CBus *i2c[14];
> > > > + I2CBus *ssd[8];
> > > > + int i;
> > > > +
> > > > + for (i = 0; i < 14; i++) {
> > > > + i2c[i] = aspeed_i2c_get_bus(&soc->i2c, i);
> > > > + }
> > > > + get_pca9548_channels(i2c[1], 0x71, ssd);
> > >
> > > We should rename to aspeed_get_pca9548_channels
> >
> > +1
> >
> > >
> > > > +
> > > > + i2c_slave_create_simple(i2c[0], "fby35-sb-cpld", 0x21);
> > > > + i2c_slave_create_simple(i2c[1], "tmp105", 0x48);
> > > > + i2c_slave_create_simple(i2c[1], "tmp105", 0x49);
> > > > + i2c_slave_create_simple(i2c[1], "tmp105", 0x4a);
> > > > + i2c_slave_create_simple(i2c[1], "adm1272", 0x40);
> > > > + i2c_slave_create_simple(i2c[1], "tmp421", 0x4c);
> > > > + i2c_slave_create_simple(i2c[2], "intel-me", 0x16);
> > > > + i2c_slave_create_simple(i2c[4], "isl69259", 0x76);
> > > > + i2c_slave_create_simple(i2c[4], "isl69259", 0x62);
> > > > + i2c_slave_create_simple(i2c[4], "isl69259", 0x60);
> > > > +
> > > > + for (int i = 0; i < 8; i++) {
> > > > + i2c_slave_create_simple(ssd[i], "tmp105", 0x6a);
> > > > + }
> > > > +
> > > > + /*
> > > > + * FIXME: This should actually be the BMC, but both the ME and the BMC
> > >
> > > QEMU has an embedded IPMI BMC simulator.
> >
> >
> > !!! Didn't realize this, definitely going to try using it.
> >
> > >
> > > > + * are IPMB endpoints, and the current ME implementation is generic
> > > > + * enough to respond normally to some things.
> > > > + */
> > > > + i2c_slave_create_simple(i2c[6], "intel-me", 0x10);
> > > > +}
> > > > +
> > > > +static void aspeed_machine_oby35_cl_class_init(ObjectClass *oc, void *data)
> > > > +{
> > > > + MachineClass *mc = MACHINE_CLASS(oc);
> > > > + AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
> > > > +
> > > > + mc->desc = "Meta Platforms fby35 CraterLake BIC (Cortex-M4)";
> > > > + amc->i2c_init = oby35_cl_i2c_init;
> > > > +}
> > > > +
> > > > static const TypeInfo aspeed_machine_types[] = {
> > > > {
> > > > .name = MACHINE_TYPE_NAME("palmetto-bmc"),
> > > > @@ -1494,6 +1538,10 @@ static const TypeInfo aspeed_machine_types[] = {
> > > > .name = MACHINE_TYPE_NAME("ast1030-evb"),
> > > > .parent = TYPE_ASPEED_MACHINE,
> > > > .class_init = aspeed_minibmc_machine_ast1030_evb_class_init,
> > > > + }, {
> > > > + .name = MACHINE_TYPE_NAME("oby35-cl"),
> > > > + .parent = MACHINE_TYPE_NAME("ast1030-evb"),
> > >
> > > hmm, so we are inheriting from the evb ?
> >
> > Yeah, I remember this was controversial with fby35-bmc too, maybe I'll
> > change this in the follow-up. I just like inheriting from the EVB's because
> > people use the EVB's a lot for testing, most of the time I'm just trying to
> > add some extra i2c devices/etc, so I override the i2c init. But, maybe it's
> > good to decouple them.
> >
> > >
> > > C.
> > >
> > >
> > > > + .class_init = aspeed_machine_oby35_cl_class_init,
> > > > }, {
> > > > .name = TYPE_ASPEED_MACHINE,
> > > > .parent = TYPE_MACHINE,
> > >
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 14/14] hw/arm/aspeed: Add oby35-cl machine
2022-06-30 16:42 ` Cédric Le Goater
2022-06-30 17:48 ` Peter Delevoryas
@ 2022-06-30 23:06 ` Peter Delevoryas
2022-07-01 5:39 ` Cédric Le Goater
1 sibling, 1 reply; 34+ messages in thread
From: Peter Delevoryas @ 2022-06-30 23:06 UTC (permalink / raw)
To: Cédric Le Goater
Cc: peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel
On Thu, Jun 30, 2022 at 06:42:52PM +0200, Cédric Le Goater wrote:
> On 6/30/22 18:15, Peter Delevoryas wrote:
> > On Thu, Jun 30, 2022 at 01:02:54PM +0200, Cédric Le Goater wrote:
> > > On 6/30/22 06:51, Peter Delevoryas wrote:
> > > > From: Peter Delevoryas <pdel@fb.com>
> > > >
> > > > The fby35 machine includes 4 server boards, each of which has a "bridge
> > > > interconnect" (BIC). This chip abstracts the pinout for the server board
> > > > into a single endpoint that the baseboard management controller (BMC)
> > > > can talk to using IPMB.
> > > >
> > > > This commit adds a machine for testing the BIC on the server board. It
> > > > runs OpenBIC (https://github.com/facebook/openbic) and the server board
> > > > is called CraterLake, so the code name is oby35-cl. There's also a
> > > > variant of the baseboard that replaces the BMC with a BIC, but that
> > > > machine is not included here.
> > > >
> > > > A test image can be built from https://github.com/facebook/openbic using
> > > > the instructions in the README.md to build the meta-facebook/yv35-cl
> > > > recipe, or retrieved from my Github:
> > > >
> > > > wget https://github.com/peterdelevoryas/OpenBIC/releases/download/oby35-cl-2022.17.01/Y35BCL.elf
> > > >
> > > > And you can run this machine with the following command:
> > > >
> > > > qemu-system-arm -machine oby35-cl -nographic -kernel Y35BCL.elf
> > > >
> > > > It should produce output like the following:
> > > >
> > > > [00:00:00.005,000] <inf> usb_dc_aspeed: select ep[0x81] as IN endpoint
> > > > [00:00:00.006,000] <inf> usb_dc_aspeed: select ep[0x82] as IN endpoint
> > > > [00:00:00.006,000] <wrn> usb_dc_aspeed: pre-selected ep[0x1] as IN endpoint
> > > > [00:00:00.006,000] <wrn> usb_dc_aspeed: pre-selected ep[0x2] as IN endpoint
> > > > [00:00:00.006,000] <inf> usb_dc_aspeed: select ep[0x3] as OUT endpoint
> > > > *** Booting Zephyr OS build v00.01.05 ***
> > > > Hello, welcome to yv35 craterlake 2022.25.1
> > > > BIC class type(class-1), 1ou present status(0), 2ou present status(0), board revision(0x1)
> > > > check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
> > > > [init_drive_type] sensor 0x14 post sensor read failed!
> > > >
> > > > [init_drive_type] sensor 0x30 post sensor read failed!
> > > > [init_drive_type] sensor 0x39 post sensor read failed!
> > > > ipmi_init
> > > > [set_DC_status] gpio number(15) status(0)
> > > > [set_post_status] gpio number(1) status(1)
> > > > uart:~$ [00:00:01.010,000] <inf> kcs_aspeed: KCS3: addr=0xca2, idr=0x2c, odr=0x38, str=0x44
> > > >
> > > > [00:00:01.016,000] <err> spi_nor_multi_dev: [1216][spi1_cs0]SFDP magic 00000000 invalid
> > > > [00:00:01.016,000] <err> spi_nor_multi_dev: [1456]SFDP read failed: -22
> > > > [00:00:01.010,000] <inf> kcs_aspeed: KCS3: addr=0xca2, idr=0x2c, odr=0x38, str=0x44
> > > >
> > > > [00:00:01.016,000] <err> spi_nor_multi_dev: [1216][spi1_cs0]SFDP magic 00000000 invalid
> > > > [00:00:01.016,000] <err> spi_nor_multi_dev: [1456]SFDP read failed: -22
> > > > uart:~$ BIC Ready
> > > >
> > > > Signed-off-by: Peter Delevoryas <pdel@fb.com>
> > >
> > > LGTM.
> > >
> > > That said I would prefer to introduce the machine first and then
> > > populate with devices.
> >
> > Ohh ok, I'll submit the machine definition separately all by itself and then
> > submit any extra devices like the CPLD or ME afterwards.
>
> I have kept the "full system" in my tree for now :
>
> cb4481ae1812 aspeed: Add AST2600 (BMC) to fby35 (full system)
> c155bf27d3e7 aspeed: Make aspeed_board_init_flashes public (trivial)
> 3f5485fa88b9 aspeed: Add fby35 skeleton (trivial)
>
> because the ROM vs. execute-in-place is being analyzed. Let's see if
> we can make progress and simplify the initial machine.
>
> I have also kept the latest *fby35* emulating the BIC only :
>
> 5cfc4b68fdb8 hw/arm/aspeed: Add oby35-cl machine
> 06f21e024ee7 hw/misc/aspeed: Add intel-me
> e96a23571599 hw/misc/aspeed: Add fby35-sb-cpld
>
> to discuss a bit more on the names, files, IPMI, etc. Until now, we had
> Aspeed machines modeling EVBs or BMCs. BICs and multi SoC system are new.
Oh shoot, I just remembered, the oby35-cl machine won't work without the VR
patches.
I know the pull request you just made didn't include it, just a reminder. I was
worried that maybe it had been submitted somewhere.
>
> Having a review on the common models in 8-10 would be nice.
>
> 2a9be57901a3 hw/sensor: Add Renesas ISL69259 device model
> 85f8352e213a hw/sensor: Add IC_DEVICE_ID to ISL voltage regulators
> aea568d56db5 hw/i2c/pmbus: Add idle state to return 0xff's
>
> They should not be too problematic to merge. As soon as Titus has time
> to take a look we will know, and I did a comment. So this can be addressed
> in parallel with the fby35 machines.
>
> Thanks,
>
> C.
>
>
>
> >
> > >
> > > May be it is time to introduce a new machine file. This one seems
> > > like it could go in a f35.c file, also because a larger f35-* is
> > > in plan. aspeed.c could contain the basic definitions and helpers.
> >
> > Yes, patrick@stwcx.xyz was thinking the same thing. An f35.c (well,
> > maybe yv35.c or fby35.c would be more appropriate) would be a good
> > idea. I'll submit another patch up front to move fby35 stuff to
> > a separate file.
> >
> > >
> > > > ---
> > > > hw/arm/aspeed.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 48 insertions(+)
> > > >
> > > > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> > > > index a06f7c1b62..75971ef2ca 100644
> > > > --- a/hw/arm/aspeed.c
> > > > +++ b/hw/arm/aspeed.c
> > > > @@ -1429,6 +1429,50 @@ static void aspeed_minibmc_machine_ast1030_evb_class_init(ObjectClass *oc,
> > > > amc->macs_mask = 0;
> > > > }
> > > > +static void oby35_cl_i2c_init(AspeedMachineState *bmc)
> > > > +{
> > > > + AspeedSoCState *soc = &bmc->soc;
> > > > + I2CBus *i2c[14];
> > > > + I2CBus *ssd[8];
> > > > + int i;
> > > > +
> > > > + for (i = 0; i < 14; i++) {
> > > > + i2c[i] = aspeed_i2c_get_bus(&soc->i2c, i);
> > > > + }
> > > > + get_pca9548_channels(i2c[1], 0x71, ssd);
> > >
> > > We should rename to aspeed_get_pca9548_channels
> >
> > +1
> >
> > >
> > > > +
> > > > + i2c_slave_create_simple(i2c[0], "fby35-sb-cpld", 0x21);
> > > > + i2c_slave_create_simple(i2c[1], "tmp105", 0x48);
> > > > + i2c_slave_create_simple(i2c[1], "tmp105", 0x49);
> > > > + i2c_slave_create_simple(i2c[1], "tmp105", 0x4a);
> > > > + i2c_slave_create_simple(i2c[1], "adm1272", 0x40);
> > > > + i2c_slave_create_simple(i2c[1], "tmp421", 0x4c);
> > > > + i2c_slave_create_simple(i2c[2], "intel-me", 0x16);
> > > > + i2c_slave_create_simple(i2c[4], "isl69259", 0x76);
> > > > + i2c_slave_create_simple(i2c[4], "isl69259", 0x62);
> > > > + i2c_slave_create_simple(i2c[4], "isl69259", 0x60);
> > > > +
> > > > + for (int i = 0; i < 8; i++) {
> > > > + i2c_slave_create_simple(ssd[i], "tmp105", 0x6a);
> > > > + }
> > > > +
> > > > + /*
> > > > + * FIXME: This should actually be the BMC, but both the ME and the BMC
> > >
> > > QEMU has an embedded IPMI BMC simulator.
> >
> >
> > !!! Didn't realize this, definitely going to try using it.
> >
> > >
> > > > + * are IPMB endpoints, and the current ME implementation is generic
> > > > + * enough to respond normally to some things.
> > > > + */
> > > > + i2c_slave_create_simple(i2c[6], "intel-me", 0x10);
> > > > +}
> > > > +
> > > > +static void aspeed_machine_oby35_cl_class_init(ObjectClass *oc, void *data)
> > > > +{
> > > > + MachineClass *mc = MACHINE_CLASS(oc);
> > > > + AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
> > > > +
> > > > + mc->desc = "Meta Platforms fby35 CraterLake BIC (Cortex-M4)";
> > > > + amc->i2c_init = oby35_cl_i2c_init;
> > > > +}
> > > > +
> > > > static const TypeInfo aspeed_machine_types[] = {
> > > > {
> > > > .name = MACHINE_TYPE_NAME("palmetto-bmc"),
> > > > @@ -1494,6 +1538,10 @@ static const TypeInfo aspeed_machine_types[] = {
> > > > .name = MACHINE_TYPE_NAME("ast1030-evb"),
> > > > .parent = TYPE_ASPEED_MACHINE,
> > > > .class_init = aspeed_minibmc_machine_ast1030_evb_class_init,
> > > > + }, {
> > > > + .name = MACHINE_TYPE_NAME("oby35-cl"),
> > > > + .parent = MACHINE_TYPE_NAME("ast1030-evb"),
> > >
> > > hmm, so we are inheriting from the evb ?
> >
> > Yeah, I remember this was controversial with fby35-bmc too, maybe I'll
> > change this in the follow-up. I just like inheriting from the EVB's because
> > people use the EVB's a lot for testing, most of the time I'm just trying to
> > add some extra i2c devices/etc, so I override the i2c init. But, maybe it's
> > good to decouple them.
> >
> > >
> > > C.
> > >
> > >
> > > > + .class_init = aspeed_machine_oby35_cl_class_init,
> > > > }, {
> > > > .name = TYPE_ASPEED_MACHINE,
> > > > .parent = TYPE_MACHINE,
> > >
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 14/14] hw/arm/aspeed: Add oby35-cl machine
2022-06-30 23:06 ` Peter Delevoryas
@ 2022-07-01 5:39 ` Cédric Le Goater
0 siblings, 0 replies; 34+ messages in thread
From: Cédric Le Goater @ 2022-07-01 5:39 UTC (permalink / raw)
To: Peter Delevoryas
Cc: peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel
On 7/1/22 01:06, Peter Delevoryas wrote:
> On Thu, Jun 30, 2022 at 06:42:52PM +0200, Cédric Le Goater wrote:
>> On 6/30/22 18:15, Peter Delevoryas wrote:
>>> On Thu, Jun 30, 2022 at 01:02:54PM +0200, Cédric Le Goater wrote:
>>>> On 6/30/22 06:51, Peter Delevoryas wrote:
>>>>> From: Peter Delevoryas <pdel@fb.com>
>>>>>
>>>>> The fby35 machine includes 4 server boards, each of which has a "bridge
>>>>> interconnect" (BIC). This chip abstracts the pinout for the server board
>>>>> into a single endpoint that the baseboard management controller (BMC)
>>>>> can talk to using IPMB.
>>>>>
>>>>> This commit adds a machine for testing the BIC on the server board. It
>>>>> runs OpenBIC (https://github.com/facebook/openbic) and the server board
>>>>> is called CraterLake, so the code name is oby35-cl. There's also a
>>>>> variant of the baseboard that replaces the BMC with a BIC, but that
>>>>> machine is not included here.
>>>>>
>>>>> A test image can be built from https://github.com/facebook/openbic using
>>>>> the instructions in the README.md to build the meta-facebook/yv35-cl
>>>>> recipe, or retrieved from my Github:
>>>>>
>>>>> wget https://github.com/peterdelevoryas/OpenBIC/releases/download/oby35-cl-2022.17.01/Y35BCL.elf
>>>>>
>>>>> And you can run this machine with the following command:
>>>>>
>>>>> qemu-system-arm -machine oby35-cl -nographic -kernel Y35BCL.elf
>>>>>
>>>>> It should produce output like the following:
>>>>>
>>>>> [00:00:00.005,000] <inf> usb_dc_aspeed: select ep[0x81] as IN endpoint
>>>>> [00:00:00.006,000] <inf> usb_dc_aspeed: select ep[0x82] as IN endpoint
>>>>> [00:00:00.006,000] <wrn> usb_dc_aspeed: pre-selected ep[0x1] as IN endpoint
>>>>> [00:00:00.006,000] <wrn> usb_dc_aspeed: pre-selected ep[0x2] as IN endpoint
>>>>> [00:00:00.006,000] <inf> usb_dc_aspeed: select ep[0x3] as OUT endpoint
>>>>> *** Booting Zephyr OS build v00.01.05 ***
>>>>> Hello, welcome to yv35 craterlake 2022.25.1
>>>>> BIC class type(class-1), 1ou present status(0), 2ou present status(0), board revision(0x1)
>>>>> check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>>>> check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>>>> check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>>>> check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>>>> check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>>>> check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>>>> check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>>>> check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>>>> check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>>>> check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>>>> check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>>>> check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>>>> check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>>>> check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>>>> check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>>>> check_vr_type: i2c4 0x62 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>>>> check_vr_type: i2c4 0x76 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>>>> check_vr_type: i2c4 0x76 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>>>> check_vr_type: i2c4 0x60 page 0 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>>>> check_vr_type: i2c4 0x60 page 1 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff]
>>>>> [init_drive_type] sensor 0x14 post sensor read failed!
>>>>>
>>>>> [init_drive_type] sensor 0x30 post sensor read failed!
>>>>> [init_drive_type] sensor 0x39 post sensor read failed!
>>>>> ipmi_init
>>>>> [set_DC_status] gpio number(15) status(0)
>>>>> [set_post_status] gpio number(1) status(1)
>>>>> uart:~$ [00:00:01.010,000] <inf> kcs_aspeed: KCS3: addr=0xca2, idr=0x2c, odr=0x38, str=0x44
>>>>>
>>>>> [00:00:01.016,000] <err> spi_nor_multi_dev: [1216][spi1_cs0]SFDP magic 00000000 invalid
>>>>> [00:00:01.016,000] <err> spi_nor_multi_dev: [1456]SFDP read failed: -22
>>>>> [00:00:01.010,000] <inf> kcs_aspeed: KCS3: addr=0xca2, idr=0x2c, odr=0x38, str=0x44
>>>>>
>>>>> [00:00:01.016,000] <err> spi_nor_multi_dev: [1216][spi1_cs0]SFDP magic 00000000 invalid
>>>>> [00:00:01.016,000] <err> spi_nor_multi_dev: [1456]SFDP read failed: -22
>>>>> uart:~$ BIC Ready
>>>>>
>>>>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>>>>
>>>> LGTM.
>>>>
>>>> That said I would prefer to introduce the machine first and then
>>>> populate with devices.
>>>
>>> Ohh ok, I'll submit the machine definition separately all by itself and then
>>> submit any extra devices like the CPLD or ME afterwards.
>>
>> I have kept the "full system" in my tree for now :
>>
>> cb4481ae1812 aspeed: Add AST2600 (BMC) to fby35 (full system)
>> c155bf27d3e7 aspeed: Make aspeed_board_init_flashes public (trivial)
>> 3f5485fa88b9 aspeed: Add fby35 skeleton (trivial)
>>
>> because the ROM vs. execute-in-place is being analyzed. Let's see if
>> we can make progress and simplify the initial machine.
>>
>> I have also kept the latest *fby35* emulating the BIC only :
>>
>> 5cfc4b68fdb8 hw/arm/aspeed: Add oby35-cl machine
>> 06f21e024ee7 hw/misc/aspeed: Add intel-me
>> e96a23571599 hw/misc/aspeed: Add fby35-sb-cpld
>>
>> to discuss a bit more on the names, files, IPMI, etc. Until now, we had
>> Aspeed machines modeling EVBs or BMCs. BICs and multi SoC system are new.
>
> Oh shoot, I just remembered, the oby35-cl machine won't work without the VR
> patches.
They look ready for upstream now.
C.
>
> I know the pull request you just made didn't include it, just a reminder. I was
> worried that maybe it had been submitted somewhere.
>
>>
>> Having a review on the common models in 8-10 would be nice.
>>
>> 2a9be57901a3 hw/sensor: Add Renesas ISL69259 device model
>> 85f8352e213a hw/sensor: Add IC_DEVICE_ID to ISL voltage regulators
>> aea568d56db5 hw/i2c/pmbus: Add idle state to return 0xff's
>>
>> They should not be too problematic to merge. As soon as Titus has time
>> to take a look we will know, and I did a comment. So this can be addressed
>> in parallel with the fby35 machines.
>>
>> Thanks,
>>
>> C.
>>
>>
>>
>>>
>>>>
>>>> May be it is time to introduce a new machine file. This one seems
>>>> like it could go in a f35.c file, also because a larger f35-* is
>>>> in plan. aspeed.c could contain the basic definitions and helpers.
>>>
>>> Yes, patrick@stwcx.xyz was thinking the same thing. An f35.c (well,
>>> maybe yv35.c or fby35.c would be more appropriate) would be a good
>>> idea. I'll submit another patch up front to move fby35 stuff to
>>> a separate file.
>>>
>>>>
>>>>> ---
>>>>> hw/arm/aspeed.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>> 1 file changed, 48 insertions(+)
>>>>>
>>>>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>>>>> index a06f7c1b62..75971ef2ca 100644
>>>>> --- a/hw/arm/aspeed.c
>>>>> +++ b/hw/arm/aspeed.c
>>>>> @@ -1429,6 +1429,50 @@ static void aspeed_minibmc_machine_ast1030_evb_class_init(ObjectClass *oc,
>>>>> amc->macs_mask = 0;
>>>>> }
>>>>> +static void oby35_cl_i2c_init(AspeedMachineState *bmc)
>>>>> +{
>>>>> + AspeedSoCState *soc = &bmc->soc;
>>>>> + I2CBus *i2c[14];
>>>>> + I2CBus *ssd[8];
>>>>> + int i;
>>>>> +
>>>>> + for (i = 0; i < 14; i++) {
>>>>> + i2c[i] = aspeed_i2c_get_bus(&soc->i2c, i);
>>>>> + }
>>>>> + get_pca9548_channels(i2c[1], 0x71, ssd);
>>>>
>>>> We should rename to aspeed_get_pca9548_channels
>>>
>>> +1
>>>
>>>>
>>>>> +
>>>>> + i2c_slave_create_simple(i2c[0], "fby35-sb-cpld", 0x21);
>>>>> + i2c_slave_create_simple(i2c[1], "tmp105", 0x48);
>>>>> + i2c_slave_create_simple(i2c[1], "tmp105", 0x49);
>>>>> + i2c_slave_create_simple(i2c[1], "tmp105", 0x4a);
>>>>> + i2c_slave_create_simple(i2c[1], "adm1272", 0x40);
>>>>> + i2c_slave_create_simple(i2c[1], "tmp421", 0x4c);
>>>>> + i2c_slave_create_simple(i2c[2], "intel-me", 0x16);
>>>>> + i2c_slave_create_simple(i2c[4], "isl69259", 0x76);
>>>>> + i2c_slave_create_simple(i2c[4], "isl69259", 0x62);
>>>>> + i2c_slave_create_simple(i2c[4], "isl69259", 0x60);
>>>>> +
>>>>> + for (int i = 0; i < 8; i++) {
>>>>> + i2c_slave_create_simple(ssd[i], "tmp105", 0x6a);
>>>>> + }
>>>>> +
>>>>> + /*
>>>>> + * FIXME: This should actually be the BMC, but both the ME and the BMC
>>>>
>>>> QEMU has an embedded IPMI BMC simulator.
>>>
>>>
>>> !!! Didn't realize this, definitely going to try using it.
>>>
>>>>
>>>>> + * are IPMB endpoints, and the current ME implementation is generic
>>>>> + * enough to respond normally to some things.
>>>>> + */
>>>>> + i2c_slave_create_simple(i2c[6], "intel-me", 0x10);
>>>>> +}
>>>>> +
>>>>> +static void aspeed_machine_oby35_cl_class_init(ObjectClass *oc, void *data)
>>>>> +{
>>>>> + MachineClass *mc = MACHINE_CLASS(oc);
>>>>> + AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
>>>>> +
>>>>> + mc->desc = "Meta Platforms fby35 CraterLake BIC (Cortex-M4)";
>>>>> + amc->i2c_init = oby35_cl_i2c_init;
>>>>> +}
>>>>> +
>>>>> static const TypeInfo aspeed_machine_types[] = {
>>>>> {
>>>>> .name = MACHINE_TYPE_NAME("palmetto-bmc"),
>>>>> @@ -1494,6 +1538,10 @@ static const TypeInfo aspeed_machine_types[] = {
>>>>> .name = MACHINE_TYPE_NAME("ast1030-evb"),
>>>>> .parent = TYPE_ASPEED_MACHINE,
>>>>> .class_init = aspeed_minibmc_machine_ast1030_evb_class_init,
>>>>> + }, {
>>>>> + .name = MACHINE_TYPE_NAME("oby35-cl"),
>>>>> + .parent = MACHINE_TYPE_NAME("ast1030-evb"),
>>>>
>>>> hmm, so we are inheriting from the evb ?
>>>
>>> Yeah, I remember this was controversial with fby35-bmc too, maybe I'll
>>> change this in the follow-up. I just like inheriting from the EVB's because
>>> people use the EVB's a lot for testing, most of the time I'm just trying to
>>> add some extra i2c devices/etc, so I override the i2c init. But, maybe it's
>>> good to decouple them.
>>>
>>>>
>>>> C.
>>>>
>>>>
>>>>> + .class_init = aspeed_machine_oby35_cl_class_init,
>>>>> }, {
>>>>> .name = TYPE_ASPEED_MACHINE,
>>>>> .parent = TYPE_MACHINE,
>>>>
>>
>>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH v3 00/14] hw/i2c/aspeed: I2C slave mode DMA RX w/ new regs
2022-06-30 4:51 [PATCH v3 00/14] hw/i2c/aspeed: I2C slave mode DMA RX w/ new regs Peter Delevoryas
` (13 preceding siblings ...)
2022-06-30 4:51 ` [PATCH v3 14/14] hw/arm/aspeed: Add oby35-cl machine Peter Delevoryas
@ 2022-06-30 6:13 ` Cédric Le Goater
2022-06-30 7:50 ` Cédric Le Goater
15 siblings, 0 replies; 34+ messages in thread
From: Cédric Le Goater @ 2022-06-30 6:13 UTC (permalink / raw)
To: Peter Delevoryas
Cc: peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel, pdel
On 6/30/22 06:51, Peter Delevoryas wrote:
> From: Peter Delevoryas <pdel@fb.com>
It is good practice to keep the Reviewed/Tested/Acked tags sent on
the previous version of a patchset. I should have told you in v2.
For next time ! :)
Thanks,
C.
> v3:
> - hw/i2c/pmbus_device:
> - Removed commit that resets the out buf.
> - Removed IC_DEVICE_ID
> - Added commit to allow devices to move to an idle state that
> avoids enqueuing excess data into the out buf.
> - hw/sensor/isl_pmbus_vr:
> - Added IC_DEVICE_ID commit just for voltage regulators.
> - Added ISL69259 with an IC_DEVICE_ID.
> - hw/misc/aspeed_peci:
> - Moved registers from .h to .c
> - Replaced guest_error on interrupt disable case with trace
> for all interrupts (not just when they're disabled).
> - Removed leftover qemu_irq_raise
>
> Thanks,
> Peter
>
> Klaus Jensen (3):
> hw/i2c: support multiple masters
> hw/i2c: add asynchronous send
> hw/i2c/aspeed: add slave device in old register mode
>
> Peter Delevoryas (11):
> hw/i2c/aspeed: Fix R_I2CD_FUN_CTRL reference
> hw/i2c/aspeed: Fix DMA len write-enable bit handling
> hw/i2c/aspeed: Fix MASTER_EN missing error message
> hw/i2c/aspeed: Add new-registers DMA slave mode RX support
> hw/i2c/pmbus: Add idle state to return 0xff's
> hw/sensor: Add IC_DEVICE_ID to ISL voltage regulators
> hw/sensor: Add Renesas ISL69259 device model
> hw/misc/aspeed: Add PECI controller
> hw/misc/aspeed: Add fby35-sb-cpld
> hw/misc/aspeed: Add intel-me
> hw/arm/aspeed: Add oby35-cl machine
>
> MAINTAINERS | 2 +
> hw/arm/aspeed.c | 48 +++++++
> hw/arm/aspeed_ast10x0.c | 12 ++
> hw/arm/aspeed_ast2600.c | 12 ++
> hw/arm/aspeed_soc.c | 13 ++
> hw/arm/pxa2xx.c | 2 +
> hw/display/sii9022.c | 2 +
> hw/display/ssd0303.c | 2 +
> hw/i2c/aspeed_i2c.c | 234 +++++++++++++++++++++++++++----
> hw/i2c/core.c | 70 ++++++++-
> hw/i2c/pmbus_device.c | 9 ++
> hw/i2c/smbus_slave.c | 4 +
> hw/i2c/trace-events | 2 +
> hw/misc/aspeed_peci.c | 152 ++++++++++++++++++++
> hw/misc/fby35_sb_cpld.c | 128 +++++++++++++++++
> hw/misc/intel_me.c | 162 +++++++++++++++++++++
> hw/misc/meson.build | 5 +-
> hw/misc/trace-events | 13 ++
> hw/nvram/eeprom_at24c.c | 2 +
> hw/sensor/isl_pmbus_vr.c | 40 ++++++
> hw/sensor/lsm303dlhc_mag.c | 2 +
> include/hw/arm/aspeed_soc.h | 3 +
> include/hw/i2c/aspeed_i2c.h | 11 ++
> include/hw/i2c/i2c.h | 30 ++++
> include/hw/i2c/pmbus_device.h | 7 +
> include/hw/misc/aspeed_peci.h | 29 ++++
> include/hw/sensor/isl_pmbus_vr.h | 5 +
> 27 files changed, 971 insertions(+), 30 deletions(-)
> create mode 100644 hw/misc/aspeed_peci.c
> create mode 100644 hw/misc/fby35_sb_cpld.c
> create mode 100644 hw/misc/intel_me.c
> create mode 100644 include/hw/misc/aspeed_peci.h
>
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [PATCH v3 00/14] hw/i2c/aspeed: I2C slave mode DMA RX w/ new regs
2022-06-30 4:51 [PATCH v3 00/14] hw/i2c/aspeed: I2C slave mode DMA RX w/ new regs Peter Delevoryas
` (14 preceding siblings ...)
2022-06-30 6:13 ` [PATCH v3 00/14] hw/i2c/aspeed: I2C slave mode DMA RX w/ new regs Cédric Le Goater
@ 2022-06-30 7:50 ` Cédric Le Goater
15 siblings, 0 replies; 34+ messages in thread
From: Cédric Le Goater @ 2022-06-30 7:50 UTC (permalink / raw)
To: Peter Delevoryas
Cc: peter.maydell, andrew, joel, cminyard, titusr, qemu-devel,
qemu-arm, zhdaniel, pdel
On 6/30/22 06:51, Peter Delevoryas wrote:
> From: Peter Delevoryas <pdel@fb.com>
>
> v3:
> - hw/i2c/pmbus_device:
> - Removed commit that resets the out buf.
> - Removed IC_DEVICE_ID
> - Added commit to allow devices to move to an idle state that
> avoids enqueuing excess data into the out buf.
> - hw/sensor/isl_pmbus_vr:
> - Added IC_DEVICE_ID commit just for voltage regulators.
> - Added ISL69259 with an IC_DEVICE_ID.
> - hw/misc/aspeed_peci:
> - Moved registers from .h to .c
> - Replaced guest_error on interrupt disable case with trace
> for all interrupts (not just when they're disabled).
> - Removed leftover qemu_irq_raise
I have taken patches 1-7,11 for the next Aspeed PR.
Thanks,
C.
> Thanks,
> Peter
>
> Klaus Jensen (3):
> hw/i2c: support multiple masters
> hw/i2c: add asynchronous send
> hw/i2c/aspeed: add slave device in old register mode
>
> Peter Delevoryas (11):
> hw/i2c/aspeed: Fix R_I2CD_FUN_CTRL reference
> hw/i2c/aspeed: Fix DMA len write-enable bit handling
> hw/i2c/aspeed: Fix MASTER_EN missing error message
> hw/i2c/aspeed: Add new-registers DMA slave mode RX support
> hw/i2c/pmbus: Add idle state to return 0xff's
> hw/sensor: Add IC_DEVICE_ID to ISL voltage regulators
> hw/sensor: Add Renesas ISL69259 device model
> hw/misc/aspeed: Add PECI controller
> hw/misc/aspeed: Add fby35-sb-cpld
> hw/misc/aspeed: Add intel-me
> hw/arm/aspeed: Add oby35-cl machine
>
> MAINTAINERS | 2 +
> hw/arm/aspeed.c | 48 +++++++
> hw/arm/aspeed_ast10x0.c | 12 ++
> hw/arm/aspeed_ast2600.c | 12 ++
> hw/arm/aspeed_soc.c | 13 ++
> hw/arm/pxa2xx.c | 2 +
> hw/display/sii9022.c | 2 +
> hw/display/ssd0303.c | 2 +
> hw/i2c/aspeed_i2c.c | 234 +++++++++++++++++++++++++++----
> hw/i2c/core.c | 70 ++++++++-
> hw/i2c/pmbus_device.c | 9 ++
> hw/i2c/smbus_slave.c | 4 +
> hw/i2c/trace-events | 2 +
> hw/misc/aspeed_peci.c | 152 ++++++++++++++++++++
> hw/misc/fby35_sb_cpld.c | 128 +++++++++++++++++
> hw/misc/intel_me.c | 162 +++++++++++++++++++++
> hw/misc/meson.build | 5 +-
> hw/misc/trace-events | 13 ++
> hw/nvram/eeprom_at24c.c | 2 +
> hw/sensor/isl_pmbus_vr.c | 40 ++++++
> hw/sensor/lsm303dlhc_mag.c | 2 +
> include/hw/arm/aspeed_soc.h | 3 +
> include/hw/i2c/aspeed_i2c.h | 11 ++
> include/hw/i2c/i2c.h | 30 ++++
> include/hw/i2c/pmbus_device.h | 7 +
> include/hw/misc/aspeed_peci.h | 29 ++++
> include/hw/sensor/isl_pmbus_vr.h | 5 +
> 27 files changed, 971 insertions(+), 30 deletions(-)
> create mode 100644 hw/misc/aspeed_peci.c
> create mode 100644 hw/misc/fby35_sb_cpld.c
> create mode 100644 hw/misc/intel_me.c
> create mode 100644 include/hw/misc/aspeed_peci.h
>
^ permalink raw reply [flat|nested] 34+ messages in thread