[PATCH v2 00/13] hw/i2c/aspeed: Add new-registers DMA slave mode RX support

[PATCH v2 00/13] hw/i2c/aspeed: Add new-registers DMA slave mode RX support

[Peter Delevoryas]

Hey Cedric,

I've gone over the patch series and reordered it a little better.

Changes since v1:
- Replaced printf's with qemu_log_mask or trace events.
- Added more detailed commit messages to several commits.
- Removed one unnecessary patch through reordering intel-me before oby35-cl.
- Replaced PECI register #define's with registerfields.h API.
- Renamed fby35-cpld to fby35-sb-cpld to be more specific.
- Moved the I2C patches to the start of the series, and the
  optional device/machine stuff to the end of the series.

If you'd like, I can separate this into separate patch series.

However, as I mentioned in the previous series, I was using oby35-cl to test
the I2C master and slave mode support in QEMU against Zephyr, and this patch
series includes everything put together.

Hopefully my patch series actually arrives in one piece this time.

Thanks,
Peter

v1: https://lore.kernel.org/qemu-devel/20220627195506.403715-1-pdel@fb.com/

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 (10):
  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: Reset out buf after switching pages
  hw/i2c/pmbus: Add read-only IC_DEVICE_ID support
  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            |   6 +
 hw/i2c/smbus_slave.c             |   4 +
 hw/i2c/trace-events              |   2 +
 hw/misc/aspeed_peci.c            | 136 ++++++++++++++++++
 hw/misc/fby35_sb_cpld.c          | 128 +++++++++++++++++
 hw/misc/intel_me.c               | 162 +++++++++++++++++++++
 hw/misc/meson.build              |   5 +-
 hw/misc/trace-events             |  12 ++
 hw/nvram/eeprom_at24c.c          |   2 +
 hw/sensor/isl_pmbus_vr.c         |  31 ++++
 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    |   1 +
 include/hw/misc/aspeed_peci.h    |  47 +++++++
 include/hw/sensor/isl_pmbus_vr.h |   1 +
 27 files changed, 950 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

-- 
2.30.2

[PATCH v2 01/13] hw/i2c/aspeed: Fix R_I2CD_FUN_CTRL reference

[Peter Delevoryas]

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.30.2

Re: [PATCH v2 01/13] hw/i2c/aspeed: Fix R_I2CD_FUN_CTRL reference

[Cédric Le Goater]

On 6/29/22 05:36, Peter Delevoryas wrote:
> 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>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>   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;

[PATCH v2 02/13] hw/i2c/aspeed: Fix DMA len write-enable bit handling

[Peter Delevoryas]

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.30.2

Re: [PATCH v2 02/13] hw/i2c/aspeed: Fix DMA len write-enable bit handling

[Cédric Le Goater]

On 6/29/22 05:36, Peter Delevoryas wrote:
> 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>


Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>   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));
>           }

[PATCH v2 03/13] hw/i2c/aspeed: Fix MASTER_EN missing error message

[Peter Delevoryas]

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.30.2

Re: [PATCH v2 03/13] hw/i2c/aspeed: Fix MASTER_EN missing error message

[Cédric Le Goater]

On 6/29/22 05:36, Peter Delevoryas wrote:
> 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>


Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>   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;
>           }

[PATCH v2 04/13] hw/i2c: support multiple masters

[Peter Delevoryas]

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.30.2

Re: [PATCH v2 04/13] hw/i2c: support multiple masters

[Cédric Le Goater]

Corey,

On 6/29/22 05:36, Peter Delevoryas wrote:
> 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>
> ---

If it is OK with you, I plan to include this I2C extension in the
next Aspeed PR.

Thanks,

C.

>   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,

[PATCH v2 05/13] hw/i2c: add asynchronous send

[Peter Delevoryas]

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.30.2

[PATCH v2 06/13] hw/i2c/aspeed: add slave device in old register mode

[Peter Delevoryas]

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.30.2

[PATCH v2 07/13] hw/i2c/aspeed: Add new-registers DMA slave mode RX support

[Peter Delevoryas]

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.30.2

[PATCH v2 08/13] hw/i2c/pmbus: Reset out buf after switching pages

[Peter Delevoryas]

When a pmbus device switches pages, it should clears its output buffer so
that the next transaction doesn't emit data from the previous page.

Fixes: 3746d5c15e70570b ("hw/i2c: add support for PMBus”)
Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
 hw/i2c/pmbus_device.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
index 62885fa6a1..efddc36fd9 100644
--- a/hw/i2c/pmbus_device.c
+++ b/hw/i2c/pmbus_device.c
@@ -1088,6 +1088,7 @@ static int pmbus_write_data(SMBusDevice *smd, uint8_t *buf, uint8_t len)
 
     if (pmdev->code == PMBUS_PAGE) {
         pmdev->page = pmbus_receive8(pmdev);
+        pmdev->out_buf_len = 0;
         return 0;
     }
 
-- 
2.30.2

Re: [PATCH v2 08/13] hw/i2c/pmbus: Reset out buf after switching pages

[Cédric Le Goater]

On 6/29/22 05:36, Peter Delevoryas wrote:
> When a pmbus device switches pages, it should clears its output buffer so
> that the next transaction doesn't emit data from the previous page.
> 
> Fixes: 3746d5c15e70570b ("hw/i2c: add support for PMBus”)
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> ---

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

>   hw/i2c/pmbus_device.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
> index 62885fa6a1..efddc36fd9 100644
> --- a/hw/i2c/pmbus_device.c
> +++ b/hw/i2c/pmbus_device.c
> @@ -1088,6 +1088,7 @@ static int pmbus_write_data(SMBusDevice *smd, uint8_t *buf, uint8_t len)
>   
>       if (pmdev->code == PMBUS_PAGE) {
>           pmdev->page = pmbus_receive8(pmdev);
> +        pmdev->out_buf_len = 0;
>           return 0;
>       }
>   

Re: [PATCH v2 08/13] hw/i2c/pmbus: Reset out buf after switching pages

[Titus Rwantare]

On Tue, 28 Jun 2022 at 20:36, Peter Delevoryas
<peterdelevoryas@gmail.com> wrote:
>
> When a pmbus device switches pages, it should clears its output buffer so
> that the next transaction doesn't emit data from the previous page.
>
> Fixes: 3746d5c15e70570b ("hw/i2c: add support for PMBus”)
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> ---
>  hw/i2c/pmbus_device.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
> index 62885fa6a1..efddc36fd9 100644
> --- a/hw/i2c/pmbus_device.c
> +++ b/hw/i2c/pmbus_device.c
> @@ -1088,6 +1088,7 @@ static int pmbus_write_data(SMBusDevice *smd, uint8_t *buf, uint8_t len)
>
>      if (pmdev->code == PMBUS_PAGE) {
>          pmdev->page = pmbus_receive8(pmdev);
> +        pmdev->out_buf_len = 0;
>          return 0;
>      }
>

I suspect you were running into this because ic_device_id was putting
too much data in the output buffer. Still, I wouldn't want the buffer
cleared if the page hasn't changed. Some drivers write the same page
before every read.

Titus

Re: [PATCH v2 08/13] hw/i2c/pmbus: Reset out buf after switching pages

[Peter Delevoryas]

> On Jun 29, 2022, at 11:05 AM, Titus Rwantare <titusr@google.com> wrote:
> 
> On Tue, 28 Jun 2022 at 20:36, Peter Delevoryas
> <peterdelevoryas@gmail.com> wrote:
>> 
>> When a pmbus device switches pages, it should clears its output buffer so
>> that the next transaction doesn't emit data from the previous page.
>> 
>> Fixes: 3746d5c15e70570b ("hw/i2c: add support for PMBus”)
>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>> ---
>> hw/i2c/pmbus_device.c | 1 +
>> 1 file changed, 1 insertion(+)
>> 
>> diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
>> index 62885fa6a1..efddc36fd9 100644
>> --- a/hw/i2c/pmbus_device.c
>> +++ b/hw/i2c/pmbus_device.c
>> @@ -1088,6 +1088,7 @@ static int pmbus_write_data(SMBusDevice *smd, uint8_t *buf, uint8_t len)
>> 
>>     if (pmdev->code == PMBUS_PAGE) {
>>         pmdev->page = pmbus_receive8(pmdev);
>> +        pmdev->out_buf_len = 0;
>>         return 0;
>>     }
>> 
> 
> I suspect you were running into this because ic_device_id was putting
> too much data in the output buffer. Still, I wouldn't want the buffer
> cleared if the page hasn't changed. Some drivers write the same page
> before every read.

Yes you’re correct: this is the code that was querying the ic_device_id [1]:

    memset(&msg, 0, sizeof(msg));
    msg.bus = sensor_config[index].port;
    msg.target_addr = sensor_config[index].target_addr;
    msg.tx_len = 1;
    msg.rx_len = 7;
    msg.data[0] = PMBUS_IC_DEVICE_ID;
    if (i2c_master_read(&msg, retry)) {
        printf("Failed to read VR IC_DEVICE_ID: register(0x%x)\n", PMBUS_IC_DEVICE_ID);
        return;
    }

By sending a buffer that was way larger than the rx buffer of 7, it was
leaving stuff lying around for the next query.

I’ll test it out and see what happens if I fix the IC_DEVICE_ID length
transmitted by the ISL69259 to 4, maybe we don’t need this patch. But,
at the very least, I’ll make sure to gate this on the page value changing,
not just being set.

Thanks for the review, this was really useful. I’m not very familiar
with pmbus devices.

[1] https://github.com/facebook/OpenBIC/blob/cda4c00b032b1d9c9b94c45faa2c0eca4886a0a3/meta-facebook/yv35-cl/src/platform/plat_sensor_table.c#L332-L355

> 
> Titus

Re: [PATCH v2 08/13] hw/i2c/pmbus: Reset out buf after switching pages

[Peter Delevoryas]

> On Jun 29, 2022, at 11:28 AM, Peter Delevoryas <pdel@fb.com> wrote:
> 
> 
> 
>> On Jun 29, 2022, at 11:05 AM, Titus Rwantare <titusr@google.com> wrote:
>> 
>> On Tue, 28 Jun 2022 at 20:36, Peter Delevoryas
>> <peterdelevoryas@gmail.com> wrote:
>>> 
>>> When a pmbus device switches pages, it should clears its output buffer so
>>> that the next transaction doesn't emit data from the previous page.
>>> 
>>> Fixes: 3746d5c15e70570b ("hw/i2c: add support for PMBus”)
>>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>>> ---
>>> hw/i2c/pmbus_device.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>> 
>>> diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
>>> index 62885fa6a1..efddc36fd9 100644
>>> --- a/hw/i2c/pmbus_device.c
>>> +++ b/hw/i2c/pmbus_device.c
>>> @@ -1088,6 +1088,7 @@ static int pmbus_write_data(SMBusDevice *smd, uint8_t *buf, uint8_t len)
>>> 
>>> if (pmdev->code == PMBUS_PAGE) {
>>> pmdev->page = pmbus_receive8(pmdev);
>>> + pmdev->out_buf_len = 0;
>>> return 0;
>>> }
>>> 
>> 
>> I suspect you were running into this because ic_device_id was putting
>> too much data in the output buffer. Still, I wouldn't want the buffer
>> cleared if the page hasn't changed. Some drivers write the same page
>> before every read.
> 
> Yes you’re correct: this is the code that was querying the ic_device_id [1]:
> 
> memset(&msg, 0, sizeof(msg));
> msg.bus = sensor_config[index].port;
> msg.target_addr = sensor_config[index].target_addr;
> msg.tx_len = 1;
> msg.rx_len = 7;
> msg.data[0] = PMBUS_IC_DEVICE_ID;
> if (i2c_master_read(&msg, retry)) {
> printf("Failed to read VR IC_DEVICE_ID: register(0x%x)\n", PMBUS_IC_DEVICE_ID);
> return;
> }
> 
> By sending a buffer that was way larger than the rx buffer of 7, it was
> leaving stuff lying around for the next query.
> 
> I’ll test it out and see what happens if I fix the IC_DEVICE_ID length
> transmitted by the ISL69259 to 4, maybe we don’t need this patch. But,
> at the very least, I’ll make sure to gate this on the page value changing,
> not just being set.

Hmmm, actually I’m not going to change this. It seems that our Zephyr code
is actually querying one of our devices multiple times, setting the page
to zero before each read, and expecting it to return the device ID without
any wrapping. If it was only resetting the output buffer on page
commands that change the value of the page, then the Zephyr code
wouldn’t work. I also added some printing and tested it on some hardware:

check_vr_type: i2c4 76 page 00 [04 00 81 d2 49 56 ff]
endpoint
check_vr_type: i2c4 76 page 01 [04 00 81 d2 49 56 ff]

[00:00:00.
check_vr_type: i2c4 60 page 00 [04 00 81 d2 49 3c ff]
003,000]
heck_vr_type: i2c4 60 page 01 [04 00 81 d2 49 3c ff]
m<inf> usb_d
check_vr_type: i2c4 62 page 00 [04 00 81 d2 49 d4 ff] <<<<<
c_aspeed: se
check_vr_type: i2c4 76 page 00 [04 00 81 d2 49 56 ff]
lect ep[0x3]
check_vr_type: i2c4 76 page 01 [04 00 81 d2 49 56 ff]
 as OUT endp
check_vr_type: i2c4 60 page 00 [04 00 81 d2 49 3c ff]
oint
[0
check_vr_type: i2c4 60 page 01 [04 00 81 d2 49 3c ff]
0:00:00.059,
check_vr_type: i2c4 62 page 00 [04 00 81 d2 49 d4 ff] <<<<<
000] <in
check_vr_type: i2c4 76 page 00 [04 00 81 d2 49 56 ff]
f> kcs_aspee
check_vr_type: i2c4 76 page 01 [04 00 81 d2 49 56 ff]
d: KCS3: add
check_vr_type: i2c4 60 page 00 [04 00 81 d2 49 3c ff]
r=0xca2, idr
check_vr_type: i2c4 60 page 01 [04 00 81 d2 49 3c ff]
=0x2c, odr=0
check_vr_type: i2c4 62 page 00 [04 00 81 d2 49 d4 ff]
x38, str=0x4
check_vr_type: i2c4 76 page 00 [04 00 81 d2 49 56 ff]
4

[00:
check_vr_type: i2c4 76 page 01 [04 00 81 d2 49 56 ff]
00:00.059,00
check_vr_type: i2c4 60 page 00 [04 00 81 d2 49 3c ff]
0] <e
check_vr_type: i2c4 60 page 01 [04 00 81 d2 49 3c ff]

Correct me if I’m wrong, but I think the VR at 0x62
is getting queried multiple times, and resetting the
page is causing it to go back to the start.

Also, here’s an example where I removed the pre-read
page-set command and increased the output buffer size
to 64:

heck_vr_type: i2c4 62 page 00 [04 00 81 d2 49 d4 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff f
f ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff]
00:00:00.003,000] <inf> usb_dc_asp
check_vr_type: i2c4 76 page 00 [04 00 81 d2 49 56 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff f
f ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff]
eed: select ep[0x3] as OUT endpoint
heck_vr_type: i2c4 76 page 00 [04 00 81 d2 49 56 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff]
m
[00:00:00.059,000] <inf> kcs_asp
check_vr_type: i2c4 60 page 00 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff f
f ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff]
eed: KCS3: addr=0xca2, idr=0x2c, odr=0
check_vr_type: i2c4 60 page 00 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff f
f ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff]
x38, str=0x44

[00:00:00.060,000]
check_vr_type: i2c4 62 page 00 [04 00 81 d2 49 d4 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff f
f ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff]
 <err> spi_nor_multi_dev: [1216
check_vr_type: i2c4 76 page 00 [04 00 81 d2 49 56 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff f
f ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff]
][spi1_cs0]SFDP magic 00000000 invalid
check_vr_type: i2c4 76 page 00 [04 00 81 d2 49 56 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff f
f ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff]

[00:00:00.060,000] <err> s
check_vr_type: i2c4 60 page 00 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff f
f ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff]
pi_nor_multi_dev: [1456]SFDP read faile
check_vr_type: i2c4 60 page 00 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff f
f ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff]

So, actually, it seems like the IC_DEVICE_ID
is 6 bytes (I only had 5), and there’s no
wrapping behavior. Perhaps I don’t need to
add this, and instead I should remove the
wrapping behavior for the ISL69259? I’m not
sure what the behavior is for other kinds
of PMBUS devices or voltage regulators.

Thanks,
Peter

> 
> Thanks for the review, this was really useful. I’m not very familiar
> with pmbus devices.
> 
> [1] https://github.com/facebook/OpenBIC/blob/cda4c00b032b1d9c9b94c45faa2c0eca4886a0a3/meta-facebook/yv35-cl/src/platform/plat_sensor_table.c#L332-L355
> 
>> 
>> Titus

[PATCH v2 09/13] hw/i2c/pmbus: Add read-only IC_DEVICE_ID support

[Peter Delevoryas]

Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
 hw/i2c/pmbus_device.c            |  5 +++++
 hw/sensor/isl_pmbus_vr.c         | 31 +++++++++++++++++++++++++++++++
 include/hw/i2c/pmbus_device.h    |  1 +
 include/hw/sensor/isl_pmbus_vr.h |  1 +
 4 files changed, 38 insertions(+)

diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
index efddc36fd9..82131fff85 100644
--- a/hw/i2c/pmbus_device.c
+++ b/hw/i2c/pmbus_device.c
@@ -984,6 +984,11 @@ static uint8_t pmbus_receive_byte(SMBusDevice *smd)
         }
         break;
 
+    case PMBUS_IC_DEVICE_ID:
+        pmbus_send(pmdev, pmdev->pages[index].ic_device_id,
+                   sizeof(pmdev->pages[index].ic_device_id));
+        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/hw/sensor/isl_pmbus_vr.c b/hw/sensor/isl_pmbus_vr.c
index e11e028884..b12c46ab6d 100644
--- a/hw/sensor/isl_pmbus_vr.c
+++ b/hw/sensor/isl_pmbus_vr.c
@@ -218,6 +218,28 @@ static void isl_pmbus_vr_class_init(ObjectClass *klass, void *data,
     k->device_num_pages = pages;
 }
 
+static void isl69259_init(Object *obj)
+{
+    static const uint8_t ic_device_id[] = {0x04, 0x00, 0x81, 0xD2, 0x49};
+    PMBusDevice *pmdev = PMBUS_DEVICE(obj);
+    int i;
+
+    raa22xx_init(obj);
+    for (i = 0; i < pmdev->num_pages; i++) {
+        memcpy(pmdev->pages[i].ic_device_id, ic_device_id,
+               sizeof(ic_device_id));
+    }
+}
+
+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 = isl_pmbus_vr_exit_reset;
+    isl_pmbus_vr_class_init(klass, data, 2);
+}
+
 static void isl69260_class_init(ObjectClass *klass, void *data)
 {
     ResettableClass *rc = RESETTABLE_CLASS(klass);
@@ -245,6 +267,14 @@ static void raa229004_class_init(ObjectClass *klass, void *data)
     isl_pmbus_vr_class_init(klass, data, 2);
 }
 
+static const TypeInfo isl69259_info = {
+    .name = TYPE_ISL69259,
+    .parent = TYPE_PMBUS_DEVICE,
+    .instance_size = sizeof(ISLState),
+    .instance_init = isl69259_init,
+    .class_init = isl69259_class_init,
+};
+
 static const TypeInfo isl69260_info = {
     .name = TYPE_ISL69260,
     .parent = TYPE_PMBUS_DEVICE,
@@ -271,6 +301,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);
diff --git a/include/hw/i2c/pmbus_device.h b/include/hw/i2c/pmbus_device.h
index 0f4d6b3fad..aed7809841 100644
--- a/include/hw/i2c/pmbus_device.h
+++ b/include/hw/i2c/pmbus_device.h
@@ -407,6 +407,7 @@ typedef struct PMBusPage {
     uint16_t mfr_max_temp_1;           /* R/W word */
     uint16_t mfr_max_temp_2;           /* R/W word */
     uint16_t mfr_max_temp_3;           /* R/W word */
+    uint8_t ic_device_id[16];          /* Read-Only block-read */
 } PMBusPage;
 
 /* State */
diff --git a/include/hw/sensor/isl_pmbus_vr.h b/include/hw/sensor/isl_pmbus_vr.h
index 3e47ff7e48..d501b3bc82 100644
--- a/include/hw/sensor/isl_pmbus_vr.h
+++ b/include/hw/sensor/isl_pmbus_vr.h
@@ -12,6 +12,7 @@
 #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"
-- 
2.30.2

Re: [PATCH v2 09/13] hw/i2c/pmbus: Add read-only IC_DEVICE_ID support

[Cédric Le Goater]

On 6/29/22 05:36, Peter Delevoryas wrote:
> Signed-off-by: Peter Delevoryas <pdel@fb.com>

This is also adding a "Renesas ISL69259 Digital Multiphase Voltage
Regulator" device. There should be 2 patches.

Thanks,

C.



> ---
>   hw/i2c/pmbus_device.c            |  5 +++++
>   hw/sensor/isl_pmbus_vr.c         | 31 +++++++++++++++++++++++++++++++
>   include/hw/i2c/pmbus_device.h    |  1 +
>   include/hw/sensor/isl_pmbus_vr.h |  1 +
>   4 files changed, 38 insertions(+)
> 
> diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
> index efddc36fd9..82131fff85 100644
> --- a/hw/i2c/pmbus_device.c
> +++ b/hw/i2c/pmbus_device.c
> @@ -984,6 +984,11 @@ static uint8_t pmbus_receive_byte(SMBusDevice *smd)
>           }
>           break;
>   
> +    case PMBUS_IC_DEVICE_ID:
> +        pmbus_send(pmdev, pmdev->pages[index].ic_device_id,
> +                   sizeof(pmdev->pages[index].ic_device_id));
> +        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/hw/sensor/isl_pmbus_vr.c b/hw/sensor/isl_pmbus_vr.c
> index e11e028884..b12c46ab6d 100644
> --- a/hw/sensor/isl_pmbus_vr.c
> +++ b/hw/sensor/isl_pmbus_vr.c
> @@ -218,6 +218,28 @@ static void isl_pmbus_vr_class_init(ObjectClass *klass, void *data,
>       k->device_num_pages = pages;
>   }
>   
> +static void isl69259_init(Object *obj)
> +{
> +    static const uint8_t ic_device_id[] = {0x04, 0x00, 0x81, 0xD2, 0x49};
> +    PMBusDevice *pmdev = PMBUS_DEVICE(obj);
> +    int i;
> +
> +    raa22xx_init(obj);
> +    for (i = 0; i < pmdev->num_pages; i++) {
> +        memcpy(pmdev->pages[i].ic_device_id, ic_device_id,
> +               sizeof(ic_device_id));
> +    }
> +}
> +
> +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 = isl_pmbus_vr_exit_reset;
> +    isl_pmbus_vr_class_init(klass, data, 2);
> +}
> +
>   static void isl69260_class_init(ObjectClass *klass, void *data)
>   {
>       ResettableClass *rc = RESETTABLE_CLASS(klass);
> @@ -245,6 +267,14 @@ static void raa229004_class_init(ObjectClass *klass, void *data)
>       isl_pmbus_vr_class_init(klass, data, 2);
>   }
>   
> +static const TypeInfo isl69259_info = {
> +    .name = TYPE_ISL69259,
> +    .parent = TYPE_PMBUS_DEVICE,
> +    .instance_size = sizeof(ISLState),
> +    .instance_init = isl69259_init,
> +    .class_init = isl69259_class_init,
> +};
> +
>   static const TypeInfo isl69260_info = {
>       .name = TYPE_ISL69260,
>       .parent = TYPE_PMBUS_DEVICE,
> @@ -271,6 +301,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);
> diff --git a/include/hw/i2c/pmbus_device.h b/include/hw/i2c/pmbus_device.h
> index 0f4d6b3fad..aed7809841 100644
> --- a/include/hw/i2c/pmbus_device.h
> +++ b/include/hw/i2c/pmbus_device.h
> @@ -407,6 +407,7 @@ typedef struct PMBusPage {
>       uint16_t mfr_max_temp_1;           /* R/W word */
>       uint16_t mfr_max_temp_2;           /* R/W word */
>       uint16_t mfr_max_temp_3;           /* R/W word */
> +    uint8_t ic_device_id[16];          /* Read-Only block-read */
>   } PMBusPage;
>   
>   /* State */
> diff --git a/include/hw/sensor/isl_pmbus_vr.h b/include/hw/sensor/isl_pmbus_vr.h
> index 3e47ff7e48..d501b3bc82 100644
> --- a/include/hw/sensor/isl_pmbus_vr.h
> +++ b/include/hw/sensor/isl_pmbus_vr.h
> @@ -12,6 +12,7 @@
>   #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"

Re: [PATCH v2 09/13] hw/i2c/pmbus: Add read-only IC_DEVICE_ID support

[Peter Delevoryas]

> On Jun 29, 2022, at 1:40 AM, Cédric Le Goater <clg@kaod.org> wrote:
> 
> On 6/29/22 05:36, Peter Delevoryas wrote:
>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> 
> This is also adding a "Renesas ISL69259 Digital Multiphase Voltage
> Regulator" device. There should be 2 patches.

Hmmmm yes definitely, I’ll fix this. One patch to add IC_DEVICE_ID
to pmbus, one to add ISL69259 to isl_pmbus_vr.c

> 
> Thanks,
> 
> C.
> 
> 
> 
>> ---
>>  hw/i2c/pmbus_device.c            |  5 +++++
>>  hw/sensor/isl_pmbus_vr.c         | 31 +++++++++++++++++++++++++++++++
>>  include/hw/i2c/pmbus_device.h    |  1 +
>>  include/hw/sensor/isl_pmbus_vr.h |  1 +
>>  4 files changed, 38 insertions(+)
>> diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
>> index efddc36fd9..82131fff85 100644
>> --- a/hw/i2c/pmbus_device.c
>> +++ b/hw/i2c/pmbus_device.c
>> @@ -984,6 +984,11 @@ static uint8_t pmbus_receive_byte(SMBusDevice *smd)
>>          }
>>          break;
>>  +    case PMBUS_IC_DEVICE_ID:
>> +        pmbus_send(pmdev, pmdev->pages[index].ic_device_id,
>> +                   sizeof(pmdev->pages[index].ic_device_id));
>> +        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/hw/sensor/isl_pmbus_vr.c b/hw/sensor/isl_pmbus_vr.c
>> index e11e028884..b12c46ab6d 100644
>> --- a/hw/sensor/isl_pmbus_vr.c
>> +++ b/hw/sensor/isl_pmbus_vr.c
>> @@ -218,6 +218,28 @@ static void isl_pmbus_vr_class_init(ObjectClass *klass, void *data,
>>      k->device_num_pages = pages;
>>  }
>>  +static void isl69259_init(Object *obj)
>> +{
>> +    static const uint8_t ic_device_id[] = {0x04, 0x00, 0x81, 0xD2, 0x49};
>> +    PMBusDevice *pmdev = PMBUS_DEVICE(obj);
>> +    int i;
>> +
>> +    raa22xx_init(obj);
>> +    for (i = 0; i < pmdev->num_pages; i++) {
>> +        memcpy(pmdev->pages[i].ic_device_id, ic_device_id,
>> +               sizeof(ic_device_id));
>> +    }
>> +}
>> +
>> +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 = isl_pmbus_vr_exit_reset;
>> +    isl_pmbus_vr_class_init(klass, data, 2);
>> +}
>> +
>>  static void isl69260_class_init(ObjectClass *klass, void *data)
>>  {
>>      ResettableClass *rc = RESETTABLE_CLASS(klass);
>> @@ -245,6 +267,14 @@ static void raa229004_class_init(ObjectClass *klass, void *data)
>>      isl_pmbus_vr_class_init(klass, data, 2);
>>  }
>>  +static const TypeInfo isl69259_info = {
>> +    .name = TYPE_ISL69259,
>> +    .parent = TYPE_PMBUS_DEVICE,
>> +    .instance_size = sizeof(ISLState),
>> +    .instance_init = isl69259_init,
>> +    .class_init = isl69259_class_init,
>> +};
>> +
>>  static const TypeInfo isl69260_info = {
>>      .name = TYPE_ISL69260,
>>      .parent = TYPE_PMBUS_DEVICE,
>> @@ -271,6 +301,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);
>> diff --git a/include/hw/i2c/pmbus_device.h b/include/hw/i2c/pmbus_device.h
>> index 0f4d6b3fad..aed7809841 100644
>> --- a/include/hw/i2c/pmbus_device.h
>> +++ b/include/hw/i2c/pmbus_device.h
>> @@ -407,6 +407,7 @@ typedef struct PMBusPage {
>>      uint16_t mfr_max_temp_1;           /* R/W word */
>>      uint16_t mfr_max_temp_2;           /* R/W word */
>>      uint16_t mfr_max_temp_3;           /* R/W word */
>> +    uint8_t ic_device_id[16];          /* Read-Only block-read */
>>  } PMBusPage;
>>    /* State */
>> diff --git a/include/hw/sensor/isl_pmbus_vr.h b/include/hw/sensor/isl_pmbus_vr.h
>> index 3e47ff7e48..d501b3bc82 100644
>> --- a/include/hw/sensor/isl_pmbus_vr.h
>> +++ b/include/hw/sensor/isl_pmbus_vr.h
>> @@ -12,6 +12,7 @@
>>  #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"
> 

Re: [PATCH v2 09/13] hw/i2c/pmbus: Add read-only IC_DEVICE_ID support

[Titus Rwantare]

On Tue, 28 Jun 2022 at 20:36, Peter Delevoryas
<peterdelevoryas@gmail.com> wrote:
>
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> ---

> --- a/hw/i2c/pmbus_device.c
> +++ b/hw/i2c/pmbus_device.c
> @@ -984,6 +984,11 @@ static uint8_t pmbus_receive_byte(SMBusDevice *smd)
>          }
>          break;
>
> +    case PMBUS_IC_DEVICE_ID:
> +        pmbus_send(pmdev, pmdev->pages[index].ic_device_id,
> +                   sizeof(pmdev->pages[index].ic_device_id));
> +        break;
> +

I don't think it's a good idea to add this here because this sends 16
bytes for all PMBus devices. I have at least one device that formats
IC_DEVICE_ID differently that I've not got permission to upstream.
The spec leaves the size and format up to the manufacturer, so this is
best done in isl_pmbus_vr.c in isl_pmbus_vr_read_byte().
Look at the adm1272_read_byte() which is more interesting than
isl_pmbus_vr one as an example.


>      case PMBUS_CLEAR_FAULTS:              /* Send Byte */
>      case PMBUS_PAGE_PLUS_WRITE:           /* Block Write-only */
>      case PMBUS_STORE_DEFAULT_ALL:         /* Send Byte */
> diff --git a/hw/sensor/isl_pmbus_vr.c b/hw/sensor/isl_pmbus_vr.c
> index e11e028884..b12c46ab6d 100644
> --- a/hw/sensor/isl_pmbus_vr.c
> +++ b/hw/sensor/isl_pmbus_vr.c
> @@ -218,6 +218,28 @@ static void isl_pmbus_vr_class_init(ObjectClass *klass, void *data,
>      k->device_num_pages = pages;
>  }
>
> +static void isl69259_init(Object *obj)
> +{
> +    static const uint8_t ic_device_id[] = {0x04, 0x00, 0x81, 0xD2, 0x49};
> +    PMBusDevice *pmdev = PMBUS_DEVICE(obj);
> +    int i;
> +
> +    raa22xx_init(obj);
> +    for (i = 0; i < pmdev->num_pages; i++) {
> +        memcpy(pmdev->pages[i].ic_device_id, ic_device_id,
> +               sizeof(ic_device_id));
> +    }
> +}
> +

We tend to set default register values in exit_reset() calls. You can
do something like in raa228000_exit_reset()


> diff --git a/include/hw/i2c/pmbus_device.h b/include/hw/i2c/pmbus_device.h
> index 0f4d6b3fad..aed7809841 100644
> --- a/include/hw/i2c/pmbus_device.h
> +++ b/include/hw/i2c/pmbus_device.h
> @@ -407,6 +407,7 @@ typedef struct PMBusPage {
>      uint16_t mfr_max_temp_1;           /* R/W word */
>      uint16_t mfr_max_temp_2;           /* R/W word */
>      uint16_t mfr_max_temp_3;           /* R/W word */
> +    uint8_t ic_device_id[16];          /* Read-Only block-read */

You wouldn't be able to do this here either, since the size could be
anything for other devices.

Thanks for the new device. It helps me see where to expand on PMBus.


Titus

Re: [PATCH v2 09/13] hw/i2c/pmbus: Add read-only IC_DEVICE_ID support

[Peter Delevoryas]

> On Jun 29, 2022, at 11:04 AM, Titus Rwantare <titusr@google.com> wrote:
> 
> On Tue, 28 Jun 2022 at 20:36, Peter Delevoryas
> <peterdelevoryas@gmail.com> wrote:
>> 
>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>> ---
> 
>> --- a/hw/i2c/pmbus_device.c
>> +++ b/hw/i2c/pmbus_device.c
>> @@ -984,6 +984,11 @@ static uint8_t pmbus_receive_byte(SMBusDevice *smd)
>>         }
>>         break;
>> 
>> +    case PMBUS_IC_DEVICE_ID:
>> +        pmbus_send(pmdev, pmdev->pages[index].ic_device_id,
>> +                   sizeof(pmdev->pages[index].ic_device_id));
>> +        break;
>> +
> 
> I don't think it's a good idea to add this here because this sends 16
> bytes for all PMBus devices. I have at least one device that formats
> IC_DEVICE_ID differently that I've not got permission to upstream.
> The spec leaves the size and format up to the manufacturer, so this is
> best done in isl_pmbus_vr.c in isl_pmbus_vr_read_byte().
> Look at the adm1272_read_byte() which is more interesting than
> isl_pmbus_vr one as an example.

Argh, yes, you’re absolutely right. I didn’t read the spec in very
much detail, I see now that the length is device-specific. For the
ISL69259 I see that it’s 4 bytes, which makes sense now. This
is not the exact datasheet for the ISL69259, but I think the IC_DEVICE_ID
part is the same.

	https://www.renesas.com/us/en/document/dst/isl68229-isl68239-datasheet

Putting the logic in isl_pmbus_vr_read_byte() is a good idea, I hadn’t
seen the implementation in adm1272_read_byte(), that looks great,
I’ll just add a switch on the command code and move the error message
to the default case.

> 
> 
>>     case PMBUS_CLEAR_FAULTS:              /* Send Byte */
>>     case PMBUS_PAGE_PLUS_WRITE:           /* Block Write-only */
>>     case PMBUS_STORE_DEFAULT_ALL:         /* Send Byte */
>> diff --git a/hw/sensor/isl_pmbus_vr.c b/hw/sensor/isl_pmbus_vr.c
>> index e11e028884..b12c46ab6d 100644
>> --- a/hw/sensor/isl_pmbus_vr.c
>> +++ b/hw/sensor/isl_pmbus_vr.c
>> @@ -218,6 +218,28 @@ static void isl_pmbus_vr_class_init(ObjectClass *klass, void *data,
>>     k->device_num_pages = pages;
>> }
>> 
>> +static void isl69259_init(Object *obj)
>> +{
>> +    static const uint8_t ic_device_id[] = {0x04, 0x00, 0x81, 0xD2, 0x49};
>> +    PMBusDevice *pmdev = PMBUS_DEVICE(obj);
>> +    int i;
>> +
>> +    raa22xx_init(obj);
>> +    for (i = 0; i < pmdev->num_pages; i++) {
>> +        memcpy(pmdev->pages[i].ic_device_id, ic_device_id,
>> +               sizeof(ic_device_id));
>> +    }
>> +}
>> +
> 
> We tend to set default register values in exit_reset() calls. You can
> do something like in raa228000_exit_reset()

Oh got it. If I can move the logic to isl_pmbus_vr_read_byte perhaps
I can avoid this whole function though.

> 
> 
>> diff --git a/include/hw/i2c/pmbus_device.h b/include/hw/i2c/pmbus_device.h
>> index 0f4d6b3fad..aed7809841 100644
>> --- a/include/hw/i2c/pmbus_device.h
>> +++ b/include/hw/i2c/pmbus_device.h
>> @@ -407,6 +407,7 @@ typedef struct PMBusPage {
>>     uint16_t mfr_max_temp_1;           /* R/W word */
>>     uint16_t mfr_max_temp_2;           /* R/W word */
>>     uint16_t mfr_max_temp_3;           /* R/W word */
>> +    uint8_t ic_device_id[16];          /* Read-Only block-read */
> 
> You wouldn't be able to do this here either, since the size could be
> anything for other devices.

Right, yeah I see what you mean.

> 
> Thanks for the new device. It helps me see where to expand on PMBus.

Thanks for adding the whole pmbus infrastructure! It’s really useful.
And thanks for the review.

Off-topic, but I’ve been meaning to reach out to you guys (Google
engineers working on QEMU for OpenBMC) about your Nuvoton NPCM845R
series, my team is interested in using it. I was just curious about
the status of it and if there’s any features missing and what plans
you have for the future, maybe we can collaborate.

Thanks!
Peter

> 
> 
> Titus

Re: [PATCH v2 09/13] hw/i2c/pmbus: Add read-only IC_DEVICE_ID support

[Patrick Venture]

[-- Attachment #1: Type: text/plain, Size: 4736 bytes --]

On Wed, Jun 29, 2022 at 11:34 AM Peter Delevoryas <pdel@fb.com> wrote:

>
>
> > On Jun 29, 2022, at 11:04 AM, Titus Rwantare <titusr@google.com> wrote:
> >
> > On Tue, 28 Jun 2022 at 20:36, Peter Delevoryas
> > <peterdelevoryas@gmail.com> wrote:
> >>
> >> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> >> ---
> >
> >> --- a/hw/i2c/pmbus_device.c
> >> +++ b/hw/i2c/pmbus_device.c
> >> @@ -984,6 +984,11 @@ static uint8_t pmbus_receive_byte(SMBusDevice *smd)
> >>         }
> >>         break;
> >>
> >> +    case PMBUS_IC_DEVICE_ID:
> >> +        pmbus_send(pmdev, pmdev->pages[index].ic_device_id,
> >> +                   sizeof(pmdev->pages[index].ic_device_id));
> >> +        break;
> >> +
> >
> > I don't think it's a good idea to add this here because this sends 16
> > bytes for all PMBus devices. I have at least one device that formats
> > IC_DEVICE_ID differently that I've not got permission to upstream.
> > The spec leaves the size and format up to the manufacturer, so this is
> > best done in isl_pmbus_vr.c in isl_pmbus_vr_read_byte().
> > Look at the adm1272_read_byte() which is more interesting than
> > isl_pmbus_vr one as an example.
>
> Argh, yes, you’re absolutely right. I didn’t read the spec in very
> much detail, I see now that the length is device-specific. For the
> ISL69259 I see that it’s 4 bytes, which makes sense now. This
> is not the exact datasheet for the ISL69259, but I think the IC_DEVICE_ID
> part is the same.
>
>
> https://www.renesas.com/us/en/document/dst/isl68229-isl68239-datasheet
>
> Putting the logic in isl_pmbus_vr_read_byte() is a good idea, I hadn’t
> seen the implementation in adm1272_read_byte(), that looks great,
> I’ll just add a switch on the command code and move the error message
> to the default case.
>
> >
> >
> >>     case PMBUS_CLEAR_FAULTS:              /* Send Byte */
> >>     case PMBUS_PAGE_PLUS_WRITE:           /* Block Write-only */
> >>     case PMBUS_STORE_DEFAULT_ALL:         /* Send Byte */
> >> diff --git a/hw/sensor/isl_pmbus_vr.c b/hw/sensor/isl_pmbus_vr.c
> >> index e11e028884..b12c46ab6d 100644
> >> --- a/hw/sensor/isl_pmbus_vr.c
> >> +++ b/hw/sensor/isl_pmbus_vr.c
> >> @@ -218,6 +218,28 @@ static void isl_pmbus_vr_class_init(ObjectClass
> *klass, void *data,
> >>     k->device_num_pages = pages;
> >> }
> >>
> >> +static void isl69259_init(Object *obj)
> >> +{
> >> +    static const uint8_t ic_device_id[] = {0x04, 0x00, 0x81, 0xD2,
> 0x49};
> >> +    PMBusDevice *pmdev = PMBUS_DEVICE(obj);
> >> +    int i;
> >> +
> >> +    raa22xx_init(obj);
> >> +    for (i = 0; i < pmdev->num_pages; i++) {
> >> +        memcpy(pmdev->pages[i].ic_device_id, ic_device_id,
> >> +               sizeof(ic_device_id));
> >> +    }
> >> +}
> >> +
> >
> > We tend to set default register values in exit_reset() calls. You can
> > do something like in raa228000_exit_reset()
>
> Oh got it. If I can move the logic to isl_pmbus_vr_read_byte perhaps
> I can avoid this whole function though.
>
> >
> >
> >> diff --git a/include/hw/i2c/pmbus_device.h
> b/include/hw/i2c/pmbus_device.h
> >> index 0f4d6b3fad..aed7809841 100644
> >> --- a/include/hw/i2c/pmbus_device.h
> >> +++ b/include/hw/i2c/pmbus_device.h
> >> @@ -407,6 +407,7 @@ typedef struct PMBusPage {
> >>     uint16_t mfr_max_temp_1;           /* R/W word */
> >>     uint16_t mfr_max_temp_2;           /* R/W word */
> >>     uint16_t mfr_max_temp_3;           /* R/W word */
> >> +    uint8_t ic_device_id[16];          /* Read-Only block-read */
> >
> > You wouldn't be able to do this here either, since the size could be
> > anything for other devices.
>
> Right, yeah I see what you mean.
>
> >
> > Thanks for the new device. It helps me see where to expand on PMBus.
>
> Thanks for adding the whole pmbus infrastructure! It’s really useful.
> And thanks for the review.
>
> Off-topic, but I’ve been meaning to reach out to you guys (Google
> engineers working on QEMU for OpenBMC) about your Nuvoton NPCM845R
> series, my team is interested in using it. I was just curious about
> the status of it and if there’s any features missing and what plans
> you have for the future, maybe we can collaborate.
>

Peter, feel free to reach out to me, or Titus, and we can sync up.  I used
to work with Patrick who's now at Fb on OpenBMC stuff.  We sent a bunch of
the Nuvoton patches up for review recently, and we're actively adding more
devices, etc.

We also have quite a few patches downstream we're looking to upstream,
including PECI, and sensors, etc, etc that we've seen on BMC servers.

Patrick


>
> Thanks!
> Peter
>
> >
> >
> > Titus
>
>

[-- Attachment #2: Type: text/html, Size: 6324 bytes --]

Re: [PATCH v2 09/13] hw/i2c/pmbus: Add read-only IC_DEVICE_ID support

[Cédric Le Goater]

>      > Thanks for the new device. It helps me see where to expand on PMBus.
> 
>     Thanks for adding the whole pmbus infrastructure! It’s really useful.
>     And thanks for the review.
> 
>     Off-topic, but I’ve been meaning to reach out to you guys (Google
>     engineers working on QEMU for OpenBMC) about your Nuvoton NPCM845R
>     series, my team is interested in using it. I was just curious about
>     the status of it and if there’s any features missing and what plans
>     you have for the future, maybe we can collaborate.
> 
> 
> Peter, feel free to reach out to me, or Titus, and we can sync up.  I used to work with Patrick who's now at Fb on OpenBMC stuff.  We sent a bunch of the Nuvoton patches up for review recently, and we're actively adding more devices, etc.
> 
> We also have quite a few patches downstream we're looking to upstream, including PECI, and sensors, etc, etc that we've seen on BMC servers.

So a simple PECI model is now merged. Sensors are always welcome,
it's nice to have properties to change values. On my wish-list
are PCIe and a working USB gadget network device.

Thanks,

C.

[PATCH v2 10/13] hw/misc/aspeed: Add PECI controller

[Peter Delevoryas]

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         | 136 ++++++++++++++++++++++++++++++++++
 hw/misc/meson.build           |   3 +-
 hw/misc/trace-events          |   4 +
 include/hw/arm/aspeed_soc.h   |   3 +
 include/hw/misc/aspeed_peci.h |  47 ++++++++++++
 8 files changed, 229 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..91637e29b2
--- /dev/null
+++ b/hw/misc/aspeed_peci.c
@@ -0,0 +1,136 @@
+/*
+ * 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 "trace.h"
+
+static void aspeed_peci_raise_interrupt(AspeedPECIState *s, uint32_t status)
+{
+    s->regs[R_PECI_INT_STS] = s->regs[R_PECI_INT_CTRL] & status;
+    if (!s->regs[R_PECI_INT_STS]) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Interrupts disabled: ctrl 0x%08x sts 0x%08x\n",
+                      __func__, s->regs[R_PECI_INT_CTRL], status);
+        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 / sizeof(s->regs[0])];
+
+    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));
+        qemu_irq_raise(s->irq);
+        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..af0b9c5dbf 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -209,6 +209,10 @@ 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
+
 # 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..8746f93ad7
--- /dev/null
+++ b/include/hw/misc/aspeed_peci.h
@@ -0,0 +1,47 @@
+/*
+ * 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"
+#include "hw/registerfields.h"
+
+#define ASPEED_PECI_NR_REGS ((0xFC + 4) >> 2)
+#define ASPEED_PECI_CC_RSP_SUCCESS (0x40U)
+
+#define TYPE_ASPEED_PECI "aspeed.peci"
+OBJECT_DECLARE_SIMPLE_TYPE(AspeedPECIState, ASPEED_PECI);
+
+/* 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)
+
+struct AspeedPECIState {
+    /* <private> */
+    SysBusDevice parent;
+
+    MemoryRegion mmio;
+    qemu_irq irq;
+
+    uint32_t regs[ASPEED_PECI_NR_REGS];
+};
+
+#endif
-- 
2.30.2

Re: [PATCH v2 10/13] hw/misc/aspeed: Add PECI controller

[Cédric Le Goater]

On 6/29/22 05:36, Peter Delevoryas wrote:
> 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>


LGTM. A few small comments below.


> ---
>   hw/arm/aspeed_ast10x0.c       |  12 +++
>   hw/arm/aspeed_ast2600.c       |  12 +++
>   hw/arm/aspeed_soc.c           |  13 ++++
>   hw/misc/aspeed_peci.c         | 136 ++++++++++++++++++++++++++++++++++
>   hw/misc/meson.build           |   3 +-
>   hw/misc/trace-events          |   4 +
>   include/hw/arm/aspeed_soc.h   |   3 +
>   include/hw/misc/aspeed_peci.h |  47 ++++++++++++
>   8 files changed, 229 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..91637e29b2
> --- /dev/null
> +++ b/hw/misc/aspeed_peci.c
> @@ -0,0 +1,136 @@
> +/*
> + * 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 "trace.h"
> +
> +static void aspeed_peci_raise_interrupt(AspeedPECIState *s, uint32_t status)
> +{
> +    s->regs[R_PECI_INT_STS] = s->regs[R_PECI_INT_CTRL] & status;
> +    if (!s->regs[R_PECI_INT_STS]) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Interrupts disabled: ctrl 0x%08x sts 0x%08x\n",
> +                      __func__, s->regs[R_PECI_INT_CTRL], status);
> +        return;

Why is it an error ? May be use a trace event instead.

> +    }
> +    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 / sizeof(s->regs[0])];

That's a complex way to grab the register value. May be introduce :

    int reg = 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));
> +        qemu_irq_raise(s->irq);

That's a lot of raise !

> +        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..af0b9c5dbf 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -209,6 +209,10 @@ 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
> +
>   # 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..8746f93ad7
> --- /dev/null
> +++ b/include/hw/misc/aspeed_peci.h
> @@ -0,0 +1,47 @@
> +/*
> + * 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"
> +#include "hw/registerfields.h"
> +
> +#define ASPEED_PECI_NR_REGS ((0xFC + 4) >> 2)
> +#define ASPEED_PECI_CC_RSP_SUCCESS (0x40U)
> +
> +#define TYPE_ASPEED_PECI "aspeed.peci"
> +OBJECT_DECLARE_SIMPLE_TYPE(AspeedPECIState, ASPEED_PECI);
> +
> +/* 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)

I would put the registerfields definitions in the .c file if you don't need
them elsewhere.

Thanks,

C.

> +struct AspeedPECIState {
> +    /* <private> */
> +    SysBusDevice parent;
> +
> +    MemoryRegion mmio;
> +    qemu_irq irq;
> +
> +    uint32_t regs[ASPEED_PECI_NR_REGS];
> +};
> +
> +#endif

Re: [PATCH v2 10/13] hw/misc/aspeed: Add PECI controller

[Peter Delevoryas]

> On Jun 29, 2022, at 2:20 AM, Cédric Le Goater <clg@kaod.org> wrote:
> 
> On 6/29/22 05:36, Peter Delevoryas wrote:
>> 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>
> 
> 
> LGTM. A few small comments below.
> 
> 
>> ---
>>  hw/arm/aspeed_ast10x0.c       |  12 +++
>>  hw/arm/aspeed_ast2600.c       |  12 +++
>>  hw/arm/aspeed_soc.c           |  13 ++++
>>  hw/misc/aspeed_peci.c         | 136 ++++++++++++++++++++++++++++++++++
>>  hw/misc/meson.build           |   3 +-
>>  hw/misc/trace-events          |   4 +
>>  include/hw/arm/aspeed_soc.h   |   3 +
>>  include/hw/misc/aspeed_peci.h |  47 ++++++++++++
>>  8 files changed, 229 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..91637e29b2
>> --- /dev/null
>> +++ b/hw/misc/aspeed_peci.c
>> @@ -0,0 +1,136 @@
>> +/*
>> + * 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 "trace.h"
>> +
>> +static void aspeed_peci_raise_interrupt(AspeedPECIState *s, uint32_t status)
>> +{
>> +    s->regs[R_PECI_INT_STS] = s->regs[R_PECI_INT_CTRL] & status;
>> +    if (!s->regs[R_PECI_INT_STS]) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: Interrupts disabled: ctrl 0x%08x sts 0x%08x\n",
>> +                      __func__, s->regs[R_PECI_INT_CTRL], status);
>> +        return;
> 
> Why is it an error ? May be use a trace event instead.

Yeah, I’ll just use a trace event. I wanted to have an error log if the
guest OS triggers some interrupt path but doesn’t have any of the
interrupts enabled. But, that’s kind of assuming a lot, perhaps there’s
a code path that the guest OS purposefully disabled all of the interrupts.

> 
>> +    }
>> +    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 / sizeof(s->regs[0])];
> 
> That's a complex way to grab the register value. May be introduce :
> 
>   int reg = offset >> 2;

Haha yes, this was a silly change, I had it as >> 2 originally,
I’ll change it back.

> 
> 
>> +
>> +    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));
>> +        qemu_irq_raise(s->irq);
> 
> That's a lot of raise !

!!! I forgot to remove qemu_irq_raise when switching to the new function,
thanks for catching this.

> 
>> +        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..af0b9c5dbf 100644
>> --- a/hw/misc/trace-events
>> +++ b/hw/misc/trace-events
>> @@ -209,6 +209,10 @@ 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
>> +
>>  # 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..8746f93ad7
>> --- /dev/null
>> +++ b/include/hw/misc/aspeed_peci.h
>> @@ -0,0 +1,47 @@
>> +/*
>> + * 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"
>> +#include "hw/registerfields.h"
>> +
>> +#define ASPEED_PECI_NR_REGS ((0xFC + 4) >> 2)
>> +#define ASPEED_PECI_CC_RSP_SUCCESS (0x40U)
>> +
>> +#define TYPE_ASPEED_PECI "aspeed.peci"
>> +OBJECT_DECLARE_SIMPLE_TYPE(AspeedPECIState, ASPEED_PECI);
>> +
>> +/* 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)
> 
> I would put the registerfields definitions in the .c file if you don't need
> them elsewhere.

Ohhhh yes very good point, I’ll do that.

Speaking of which: why do we have the register definitions in the
aspeed I2C header? Maybe because we have all those inline “is_new_mode”
functions. Seems like we could move all that to the the .c file, only
the struct definition needs to be in the header to support the SoC
definition right?

Anyways yeah I’ll move them to the .c

> 
> Thanks,
> 
> C.
> 
>> +struct AspeedPECIState {
>> +    /* <private> */
>> +    SysBusDevice parent;
>> +
>> +    MemoryRegion mmio;
>> +    qemu_irq irq;
>> +
>> +    uint32_t regs[ASPEED_PECI_NR_REGS];
>> +};
>> +
>> +#endif
> 

Re: [PATCH v2 10/13] hw/misc/aspeed: Add PECI controller

[Cédric Le Goater]

On 6/29/22 18:07, Peter Delevoryas wrote:
> 
> 
>> On Jun 29, 2022, at 2:20 AM, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> On 6/29/22 05:36, Peter Delevoryas wrote:
>>> 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>
>>
>>
>> LGTM. A few small comments below.
>>
>>
>>> ---
>>>   hw/arm/aspeed_ast10x0.c       |  12 +++
>>>   hw/arm/aspeed_ast2600.c       |  12 +++
>>>   hw/arm/aspeed_soc.c           |  13 ++++
>>>   hw/misc/aspeed_peci.c         | 136 ++++++++++++++++++++++++++++++++++
>>>   hw/misc/meson.build           |   3 +-
>>>   hw/misc/trace-events          |   4 +
>>>   include/hw/arm/aspeed_soc.h   |   3 +
>>>   include/hw/misc/aspeed_peci.h |  47 ++++++++++++
>>>   8 files changed, 229 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..91637e29b2
>>> --- /dev/null
>>> +++ b/hw/misc/aspeed_peci.c
>>> @@ -0,0 +1,136 @@
>>> +/*
>>> + * 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 "trace.h"
>>> +
>>> +static void aspeed_peci_raise_interrupt(AspeedPECIState *s, uint32_t status)
>>> +{
>>> +    s->regs[R_PECI_INT_STS] = s->regs[R_PECI_INT_CTRL] & status;
>>> +    if (!s->regs[R_PECI_INT_STS]) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>> +                      "%s: Interrupts disabled: ctrl 0x%08x sts 0x%08x\n",
>>> +                      __func__, s->regs[R_PECI_INT_CTRL], status);
>>> +        return;
>>
>> Why is it an error ? May be use a trace event instead.
> 
> Yeah, I’ll just use a trace event. I wanted to have an error log if the
> guest OS triggers some interrupt path but doesn’t have any of the
> interrupts enabled. But, that’s kind of assuming a lot, perhaps there’s
> a code path that the guest OS purposefully disabled all of the interrupts.
> 
>>
>>> +    }
>>> +    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 / sizeof(s->regs[0])];
>>
>> That's a complex way to grab the register value. May be introduce :
>>
>>    int reg = offset >> 2;
> 
> Haha yes, this was a silly change, I had it as >> 2 originally,
> I’ll change it back.
> 
>>
>>
>>> +
>>> +    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));
>>> +        qemu_irq_raise(s->irq);
>>
>> That's a lot of raise !
> 
> !!! I forgot to remove qemu_irq_raise when switching to the new function,
> thanks for catching this.
> 
>>
>>> +        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..af0b9c5dbf 100644
>>> --- a/hw/misc/trace-events
>>> +++ b/hw/misc/trace-events
>>> @@ -209,6 +209,10 @@ 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
>>> +
>>>   # 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..8746f93ad7
>>> --- /dev/null
>>> +++ b/include/hw/misc/aspeed_peci.h
>>> @@ -0,0 +1,47 @@
>>> +/*
>>> + * 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"
>>> +#include "hw/registerfields.h"
>>> +
>>> +#define ASPEED_PECI_NR_REGS ((0xFC + 4) >> 2)
>>> +#define ASPEED_PECI_CC_RSP_SUCCESS (0x40U)
>>> +
>>> +#define TYPE_ASPEED_PECI "aspeed.peci"
>>> +OBJECT_DECLARE_SIMPLE_TYPE(AspeedPECIState, ASPEED_PECI);
>>> +
>>> +/* 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)
>>
>> I would put the registerfields definitions in the .c file if you don't need
>> them elsewhere.
> 
> Ohhhh yes very good point, I’ll do that.
> 
> Speaking of which: why do we have the register definitions in the
> aspeed I2C header? Maybe because we have all those inline “is_new_mode”
> functions. 

Yes. I let that through. Not the best option because we could have
conflicts with other models.

> Seems like we could move all that to the the .c file, only
> the struct definition needs to be in the header to support the SoC
> definition right?

There are no strict rules. For now, it is OK I would say.

> Anyways yeah I’ll move them to the .c


Thanks,

C.

[PATCH v2 11/13] hw/misc/aspeed: Add fby35-sb-cpld

[Peter Delevoryas]

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.30.2

[PATCH v2 12/13] hw/misc/aspeed: Add intel-me

[Peter Delevoryas]

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 af0b9c5dbf..089668ff6c 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -272,3 +272,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.30.2

[PATCH v2 13/13] hw/arm/aspeed: Add oby35-cl machine

[Peter Delevoryas]

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.

The codename for this board is oby35-cl, which means "OpenBIC
Yosemite3.5 CraterLake". There is also a variant of the BIC called
"OpenBIC Yosemite3.5 Baseboard", which is an image built to run from the
baseboard as a replacement for the BMC, that's not included here, but
that's why the "-cl" suffix is included.

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.008,000] <inf> usb_dc_aspeed: select ep[0x81] as IN endpoint
    [00:00:00.009,000] <inf> usb_dc_aspeed: select ep[0x82] as IN endpoint
    [00:00:00.009,000] <wrn> usb_dc_aspeed: pre-selected ep[0x1] as IN endpoint
    [00:00:00.009,000] <wrn> usb_dc_aspeed: pre-selected ep[0x2] as IN endpoint
    [00:00:00.009,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)
    [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:00.249,000] <inf> kcs_aspeed: KCS3: addr=0xca2, idr=0x2c, odr=0x38, str=0x44

    [00:00:00.255,000] <err> spi_nor_multi_dev: [1216][spi1_cs0]SFDP magic 00000000 invalid
    [00:00:00.255,000] <err> spi_nor_multi_dev: [1456]SFDP read failed: -22
    [00:00:00.249,000] <inf> kcs_aspeed: KCS3: addr=0xca2, idr=0x2c, odr=0x38, str=0x44

    [00:00:00.255,000] <err> spi_nor_multi_dev: [1216][spi1_cs0]SFDP magic 00000000 invalid
    [00:00:00.255,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.30.2