[PATCH 0/4] Rename ssi_transfer to ssi_transfer8

[PATCH 0/4] Rename ssi_transfer to ssi_transfer8

[stephensportia]

From: Portia Stephens <portias@oss.tenstorrent.com>

The ssi_transfer function comments say that it takes a word varying
between 8-bits and 32-bits. ssi_transfer takes a 32-bit arg to transfer
but there is no means to indicate the number of bits that should
actually be transferred. All child classes of SSI_PERIPHERAL class have
transfer functions that, despite accepting a 32-bit tx, only transfer a
single byte; m25p80_transfer8(), ssi_sd_transfer(), ssd0323_transfer().

The current implementation depends on the SSI model to know
what peripheral model will be attached and what transfer size it
expects which is error prone. If a SSI_PERIPHERAL model was written that
accepted 32-bit transfers, it could not attach to any existing SSI
models.

This change updates the the naming of ssi_transfer to ssi_transfer8, as
well as changes the return value and transmit argument to be 8-bit.

Most ssi models handle this correctly already, sending a single byte at
a time. There are a few models that are written to support non 8-bit
transfers but there are no in-tree use cases that connect a peripheral
to the SSI device. These have been updated to use 8-bit transfers.

Portia Stephens (4):
  hw/ssi: Rename ssi_transfer to ssi_transfer8
  hw/ssi/pl022: Fix dropped upper bytes of ssi transfer
  hw/arm/strongarm: Fix dropped upper byte of ssi transfer
  hw/ssi/pnv_spi: Fix dropped upper bytes of ssi transfer

 hw/arm/strongarm.c         |  7 +++++--
 hw/ssi/allwinner-a10-spi.c |  2 +-
 hw/ssi/aspeed_smc.c        | 14 ++++++-------
 hw/ssi/bcm2835_spi.c       |  2 +-
 hw/ssi/ibex_spi_host.c     |  5 +++--
 hw/ssi/imx_spi.c           |  2 +-
 hw/ssi/mss-spi.c           |  2 +-
 hw/ssi/npcm7xx_fiu.c       | 42 +++++++++++++++++++-------------------
 hw/ssi/npcm_pspi.c         |  4 ++--
 hw/ssi/pl022.c             | 12 +++++++----
 hw/ssi/pnv_spi.c           | 28 ++++++++++---------------
 hw/ssi/sifive_spi.c        |  2 +-
 hw/ssi/ssi.c               |  4 ++--
 hw/ssi/stm32f2xx_spi.c     |  2 +-
 hw/ssi/xilinx_spi.c        | 10 ++++-----
 hw/ssi/xilinx_spips.c      |  4 ++--
 hw/ssi/xlnx-versal-ospi.c  |  4 ++--
 include/hw/ssi/ssi.h       | 17 ++++++++-------
 18 files changed, 82 insertions(+), 81 deletions(-)

-- 
2.43.0

[PATCH 1/4] hw/ssi: Rename ssi_transfer to ssi_transfer8

[stephensportia]

From: Portia Stephens <portias@oss.tenstorrent.com>

The ssi_transfer function comments say that it takes a word varying
between 8-bits and 32-bits. ssi_transfer takes a 32-bit arg to transfer
but there is no means to indicate the number of bits that should
actually be transferred. All child classes of SSI_PERIPHERAL class have
transfer functions that, despite accepting a 32-bit tx, only transfer a
single byte; m25p80_transfer8(), ssi_sd_transfer(), ssd0323_transfer().

The current implementation depends on the SSI model to know
what peripheral model will be attached and what transfer size it
expects which is error prone. If a SSI_PERIPHERAL model was written that
accepted 32-bit transfers, it could not attach to any existing SSI
models.

This change updates the naming of ssi_transfer to ssi_transfer8, as
well as changes the return value and transmit argument to be 8-bit.

Signed-off-by: Portia Stephens <portias@oss.tenstorrent.com>
---
 hw/arm/strongarm.c         |  2 +-
 hw/ssi/allwinner-a10-spi.c |  2 +-
 hw/ssi/aspeed_smc.c        | 14 ++++++-------
 hw/ssi/bcm2835_spi.c       |  2 +-
 hw/ssi/ibex_spi_host.c     |  5 +++--
 hw/ssi/imx_spi.c           |  2 +-
 hw/ssi/mss-spi.c           |  2 +-
 hw/ssi/npcm7xx_fiu.c       | 42 +++++++++++++++++++-------------------
 hw/ssi/npcm_pspi.c         |  4 ++--
 hw/ssi/pl022.c             |  2 +-
 hw/ssi/pnv_spi.c           |  2 +-
 hw/ssi/sifive_spi.c        |  2 +-
 hw/ssi/ssi.c               |  4 ++--
 hw/ssi/stm32f2xx_spi.c     |  2 +-
 hw/ssi/xilinx_spi.c        | 10 ++++-----
 hw/ssi/xilinx_spips.c      |  4 ++--
 hw/ssi/xlnx-versal-ospi.c  |  4 ++--
 include/hw/ssi/ssi.h       | 17 ++++++++-------
 18 files changed, 61 insertions(+), 61 deletions(-)

diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c
index 5a3242195a..e400f0a185 100644
--- a/hw/arm/strongarm.c
+++ b/hw/arm/strongarm.c
@@ -1516,7 +1516,7 @@ static void strongarm_ssp_write(void *opaque, hwaddr addr,
             if (s->sscr[1] & SSCR1_LBM) {
                 readval = value;
             } else {
-                readval = ssi_transfer(s->bus, value);
+                readval = ssi_transfer8(s->bus, value);
             }
 
             if (s->rx_level < 0x08) {
diff --git a/hw/ssi/allwinner-a10-spi.c b/hw/ssi/allwinner-a10-spi.c
index 69920b935a..5758e81c10 100644
--- a/hw/ssi/allwinner-a10-spi.c
+++ b/hw/ssi/allwinner-a10-spi.c
@@ -300,7 +300,7 @@ static void allwinner_a10_spi_flush_txfifo(AWA10SPIState *s)
         trace_allwinner_a10_spi_tx(tx);
 
         /* Write one byte at a time */
-        rx = ssi_transfer(s->bus, tx);
+        rx = ssi_transfer8(s->bus, tx);
 
         trace_allwinner_a10_spi_rx(rx);
 
diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
index bf596f7b2d..f6e8dd9457 100644
--- a/hw/ssi/aspeed_smc.c
+++ b/hw/ssi/aspeed_smc.c
@@ -488,10 +488,10 @@ static void aspeed_smc_flash_setup(AspeedSMCFlash *fl, uint32_t addr)
     /* Flash access can not exceed CS segment */
     addr = aspeed_smc_check_segment_addr(fl, addr);
 
-    ssi_transfer(s->spi, cmd);
+    ssi_transfer8(s->spi, cmd);
     while (i--) {
         if (aspeed_smc_addr_byte_enabled(s, i)) {
-            ssi_transfer(s->spi, (addr >> (i * 8)) & 0xff);
+            ssi_transfer8(s->spi, (addr >> (i * 8)) & 0xff);
         }
     }
 
@@ -503,7 +503,7 @@ static void aspeed_smc_flash_setup(AspeedSMCFlash *fl, uint32_t addr)
      */
     if (aspeed_smc_flash_mode(fl) == CTRL_FREADMODE) {
         for (i = 0; i < aspeed_smc_flash_dummy_bytes(fl); i++) {
-            ssi_transfer(fl->controller->spi, s->regs[R_DUMMY_DATA] & 0xff);
+            ssi_transfer8(fl->controller->spi, s->regs[R_DUMMY_DATA] & 0xff);
         }
     }
 }
@@ -519,7 +519,7 @@ static MemTxResult aspeed_smc_flash_read(void *opaque, hwaddr addr,
     switch (aspeed_smc_flash_mode(fl)) {
     case CTRL_USERMODE:
         for (i = 0; i < size; i++) {
-            *data |= (uint64_t) ssi_transfer(s->spi, 0x0) << (8 * i);
+            *data |= (uint64_t) ssi_transfer8(s->spi, 0x0) << (8 * i);
         }
         break;
     case CTRL_READMODE:
@@ -528,7 +528,7 @@ static MemTxResult aspeed_smc_flash_read(void *opaque, hwaddr addr,
         aspeed_smc_flash_setup(fl, addr);
 
         for (i = 0; i < size; i++) {
-            *data |= (uint64_t) ssi_transfer(s->spi, 0x0) << (8 * i);
+            *data |= (uint64_t) ssi_transfer8(s->spi, 0x0) << (8 * i);
         }
 
         aspeed_smc_flash_unselect(fl);
@@ -561,7 +561,7 @@ static MemTxResult aspeed_smc_flash_write(void *opaque, hwaddr addr,
     switch (aspeed_smc_flash_mode(fl)) {
     case CTRL_USERMODE:
         for (i = 0; i < size; i++) {
-            ssi_transfer(s->spi, (data >> (8 * i)) & 0xff);
+            ssi_transfer8(s->spi, (data >> (8 * i)) & 0xff);
         }
         break;
     case CTRL_WRITEMODE:
@@ -569,7 +569,7 @@ static MemTxResult aspeed_smc_flash_write(void *opaque, hwaddr addr,
         aspeed_smc_flash_setup(fl, addr);
 
         for (i = 0; i < size; i++) {
-            ssi_transfer(s->spi, (data >> (8 * i)) & 0xff);
+            ssi_transfer8(s->spi, (data >> (8 * i)) & 0xff);
         }
 
         aspeed_smc_flash_unselect(fl);
diff --git a/hw/ssi/bcm2835_spi.c b/hw/ssi/bcm2835_spi.c
index 01763c458c..7a0a8fa392 100644
--- a/hw/ssi/bcm2835_spi.c
+++ b/hw/ssi/bcm2835_spi.c
@@ -91,7 +91,7 @@ static void bcm2835_spi_flush_tx_fifo(BCM2835SPIState *s)
 
     while (!fifo8_is_empty(&s->tx_fifo) && !fifo8_is_full(&s->rx_fifo)) {
         tx_byte = fifo8_pop(&s->tx_fifo);
-        rx_byte = ssi_transfer(s->bus, tx_byte);
+        rx_byte = ssi_transfer8(s->bus, tx_byte);
         fifo8_push(&s->rx_fifo, rx_byte);
     }
 
diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c
index 1e574c3fcb..b5e556eedc 100644
--- a/hw/ssi/ibex_spi_host.c
+++ b/hw/ssi/ibex_spi_host.c
@@ -236,7 +236,8 @@ static void ibex_spi_host_irq(IbexSPIHostState *s)
 
 static void ibex_spi_host_transfer(IbexSPIHostState *s)
 {
-    uint32_t rx, tx, data;
+    uint32_t data;
+    uint8_t rx, tx;
     /* Get num of one byte transfers */
     uint8_t segment_len = FIELD_EX32(s->regs[IBEX_SPI_HOST_COMMAND],
                                      COMMAND,  LEN);
@@ -254,7 +255,7 @@ static void ibex_spi_host_transfer(IbexSPIHostState *s)
             tx = fifo8_pop(&s->tx_fifo);
         }
 
-        rx = ssi_transfer(s->ssi, tx);
+        rx = ssi_transfer8(s->ssi, tx);
 
         trace_ibex_spi_host_transfer(tx, rx);
 
diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index 8e014b7a7b..b25cf6c559 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -194,7 +194,7 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
             DPRINTF("writing 0x%02x\n", (uint32_t)byte);
 
             /* We need to write one byte at a time */
-            byte = ssi_transfer(s->bus, byte);
+            byte = ssi_transfer8(s->bus, byte);
 
             DPRINTF("0x%02x read\n", (uint32_t)byte);
 
diff --git a/hw/ssi/mss-spi.c b/hw/ssi/mss-spi.c
index 3c118fc0f8..8a7af68a8a 100644
--- a/hw/ssi/mss-spi.c
+++ b/hw/ssi/mss-spi.c
@@ -234,7 +234,7 @@ static void spi_flush_txfifo(MSSSpiState *s)
 
         tx = fifo32_pop(&s->tx_fifo);
         DB_PRINT("data tx:0x%" PRIx32, tx);
-        rx = ssi_transfer(s->spi, tx);
+        rx = ssi_transfer8(s->spi, tx);
         DB_PRINT("data rx:0x%" PRIx32, rx);
 
         if (fifo32_num_used(&s->rx_fifo) == s->fifo_depth) {
diff --git a/hw/ssi/npcm7xx_fiu.c b/hw/ssi/npcm7xx_fiu.c
index d41d877cfb..b0be46aa18 100644
--- a/hw/ssi/npcm7xx_fiu.c
+++ b/hw/ssi/npcm7xx_fiu.c
@@ -162,16 +162,16 @@ static uint64_t npcm7xx_fiu_flash_read(void *opaque, hwaddr addr,
     npcm7xx_fiu_select(fiu, npcm7xx_fiu_cs_index(fiu, f));
 
     drd_cfg = fiu->regs[NPCM7XX_FIU_DRD_CFG];
-    ssi_transfer(fiu->spi, FIU_DRD_CFG_RDCMD(drd_cfg));
+    ssi_transfer8(fiu->spi, FIU_DRD_CFG_RDCMD(drd_cfg));
 
     switch (FIU_DRD_CFG_ADDSIZ(drd_cfg)) {
     case FIU_ADDSIZ_4BYTES:
-        ssi_transfer(fiu->spi, extract32(addr, 24, 8));
+        ssi_transfer8(fiu->spi, extract32(addr, 24, 8));
         /* fall through */
     case FIU_ADDSIZ_3BYTES:
-        ssi_transfer(fiu->spi, extract32(addr, 16, 8));
-        ssi_transfer(fiu->spi, extract32(addr, 8, 8));
-        ssi_transfer(fiu->spi, extract32(addr, 0, 8));
+        ssi_transfer8(fiu->spi, extract32(addr, 16, 8));
+        ssi_transfer8(fiu->spi, extract32(addr, 8, 8));
+        ssi_transfer8(fiu->spi, extract32(addr, 0, 8));
         break;
 
     default:
@@ -182,11 +182,11 @@ static uint64_t npcm7xx_fiu_flash_read(void *opaque, hwaddr addr,
 
     dummy_bytes = FIU_DRD_CFG_DBW(drd_cfg);
     for (i = 0; i < dummy_bytes; i++) {
-        ssi_transfer(fiu->spi, 0);
+        ssi_transfer8(fiu->spi, 0);
     }
 
     for (i = 0; i < size; i++) {
-        value = deposit64(value, 8 * i, 8, ssi_transfer(fiu->spi, 0));
+        value = deposit64(value, 8 * i, 8, ssi_transfer8(fiu->spi, 0));
     }
 
     trace_npcm7xx_fiu_flash_read(DEVICE(fiu)->canonical_path, fiu->active_cs,
@@ -219,16 +219,16 @@ static void npcm7xx_fiu_flash_write(void *opaque, hwaddr addr, uint64_t v,
     npcm7xx_fiu_select(fiu, cs_id);
 
     dwr_cfg = fiu->regs[NPCM7XX_FIU_DWR_CFG];
-    ssi_transfer(fiu->spi, FIU_DWR_CFG_WRCMD(dwr_cfg));
+    ssi_transfer8(fiu->spi, FIU_DWR_CFG_WRCMD(dwr_cfg));
 
     switch (FIU_DWR_CFG_ADDSIZ(dwr_cfg)) {
     case FIU_ADDSIZ_4BYTES:
-        ssi_transfer(fiu->spi, extract32(addr, 24, 8));
+        ssi_transfer8(fiu->spi, extract32(addr, 24, 8));
         /* fall through */
     case FIU_ADDSIZ_3BYTES:
-        ssi_transfer(fiu->spi, extract32(addr, 16, 8));
-        ssi_transfer(fiu->spi, extract32(addr, 8, 8));
-        ssi_transfer(fiu->spi, extract32(addr, 0, 8));
+        ssi_transfer8(fiu->spi, extract32(addr, 16, 8));
+        ssi_transfer8(fiu->spi, extract32(addr, 8, 8));
+        ssi_transfer8(fiu->spi, extract32(addr, 0, 8));
         break;
 
     default:
@@ -238,7 +238,7 @@ static void npcm7xx_fiu_flash_write(void *opaque, hwaddr addr, uint64_t v,
     }
 
     for (i = 0; i < size; i++) {
-        ssi_transfer(fiu->spi, extract64(v, i * 8, 8));
+        ssi_transfer8(fiu->spi, extract64(v, i * 8, 8));
     }
 
     npcm7xx_fiu_deselect(fiu);
@@ -287,16 +287,16 @@ static void send_address(SSIBus *spi, unsigned int addsiz, uint32_t addr)
 {
     switch (addsiz) {
     case 4:
-        ssi_transfer(spi, extract32(addr, 24, 8));
+        ssi_transfer8(spi, extract32(addr, 24, 8));
         /* fall through */
     case 3:
-        ssi_transfer(spi, extract32(addr, 16, 8));
+        ssi_transfer8(spi, extract32(addr, 16, 8));
         /* fall through */
     case 2:
-        ssi_transfer(spi, extract32(addr, 8, 8));
+        ssi_transfer8(spi, extract32(addr, 8, 8));
         /* fall through */
     case 1:
-        ssi_transfer(spi, extract32(addr, 0, 8));
+        ssi_transfer8(spi, extract32(addr, 0, 8));
         /* fall through */
     case 0:
         break;
@@ -309,7 +309,7 @@ static void send_dummy_bytes(SSIBus *spi, uint32_t uma_cfg)
     unsigned int i;
 
     for (i = 0; i < FIU_UMA_CFG_DBSIZ(uma_cfg); i++) {
-        ssi_transfer(spi, 0);
+        ssi_transfer8(spi, 0);
     }
 }
 
@@ -329,7 +329,7 @@ static void npcm7xx_fiu_uma_transaction(NPCM7xxFIUState *s)
     /* Send command, if present. */
     uma_cfg = s->regs[NPCM7XX_FIU_UMA_CFG];
     if (FIU_UMA_CFG_CMDSIZ(uma_cfg) > 0) {
-        ssi_transfer(s->spi, extract32(s->regs[NPCM7XX_FIU_UMA_CMD], 0, 8));
+        ssi_transfer8(s->spi, extract32(s->regs[NPCM7XX_FIU_UMA_CMD], 0, 8));
     }
 
     /* Send address, if present. */
@@ -342,7 +342,7 @@ static void npcm7xx_fiu_uma_transaction(NPCM7xxFIUState *s)
             (i < 16) ? (NPCM7XX_FIU_UMA_DW0 + i / 4) : NPCM7XX_FIU_UMA_DW3;
         unsigned int field = (i % 4) * 8;
 
-        ssi_transfer(s->spi, extract32(s->regs[reg], field, 8));
+        ssi_transfer8(s->spi, extract32(s->regs[reg], field, 8));
     }
 
     /* Send dummy bytes, if present */
@@ -354,7 +354,7 @@ static void npcm7xx_fiu_uma_transaction(NPCM7xxFIUState *s)
         unsigned int field = (i % 4) * 8;
         uint8_t c;
 
-        c = ssi_transfer(s->spi, 0);
+        c = ssi_transfer8(s->spi, 0);
         if (reg <= NPCM7XX_FIU_UMA_DR3) {
             s->regs[reg] = deposit32(s->regs[reg], field, 8, c);
         }
diff --git a/hw/ssi/npcm_pspi.c b/hw/ssi/npcm_pspi.c
index 2e05d5dacb..38ae6e0972 100644
--- a/hw/ssi/npcm_pspi.c
+++ b/hw/ssi/npcm_pspi.c
@@ -77,9 +77,9 @@ static void npcm_pspi_write_data(NPCMPSPIState *s, uint16_t data)
     uint16_t value = 0;
 
     if (FIELD_EX16(s->regs[R_PSPI_CTL1], PSPI_CTL1, MOD)) {
-        value = ssi_transfer(s->spi, extract16(data, 8, 8)) << 8;
+        value = ssi_transfer8(s->spi, extract16(data, 8, 8)) << 8;
     }
-    value |= ssi_transfer(s->spi, extract16(data, 0, 8));
+    value |= ssi_transfer8(s->spi, extract16(data, 0, 8));
     s->regs[R_PSPI_DATA] = value;
 
     /* Mark data as available */
diff --git a/hw/ssi/pl022.c b/hw/ssi/pl022.c
index 715a2d21f4..eaac664ec5 100644
--- a/hw/ssi/pl022.c
+++ b/hw/ssi/pl022.c
@@ -103,7 +103,7 @@ static void pl022_xfer(PL022State *s)
         if (s->cr1 & PL022_CR1_LBM) {
             /* Loopback mode.  */
         } else {
-            val = ssi_transfer(s->ssi, val);
+            val = ssi_transfer8(s->ssi, val);
         }
         s->rx_fifo[o] = val & s->bitmask;
         i = (i + 1) & 7;
diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
index f3add8cab9..e2a8a710da 100644
--- a/hw/ssi/pnv_spi.c
+++ b/hw/ssi/pnv_spi.c
@@ -209,7 +209,7 @@ static void transfer(PnvSpi *s)
                 qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: TX_FIFO underflow\n");
             }
         }
-        rx = ssi_transfer(s->ssi_bus, tx);
+        rx = ssi_transfer8(s->ssi_bus, tx);
         for (int i = 0; i < s->transfer_len; i++) {
             if ((offset + i) >= payload_len) {
                 break;
diff --git a/hw/ssi/sifive_spi.c b/hw/ssi/sifive_spi.c
index 2ece78053b..1a2aac2daa 100644
--- a/hw/ssi/sifive_spi.c
+++ b/hw/ssi/sifive_spi.c
@@ -134,7 +134,7 @@ static void sifive_spi_flush_txfifo(SiFiveSPIState *s)
 
     while (!fifo8_is_empty(&s->tx_fifo)) {
         tx = fifo8_pop(&s->tx_fifo);
-        rx = ssi_transfer(s->spi, tx);
+        rx = ssi_transfer8(s->spi, tx);
 
         if (!fifo8_is_full(&s->rx_fifo)) {
             if (!(s->regs[R_FMT] & FMT_DIR)) {
diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
index 3a4fade2d5..ce2325bbd4 100644
--- a/hw/ssi/ssi.c
+++ b/hw/ssi/ssi.c
@@ -153,11 +153,11 @@ SSIBus *ssi_create_bus(DeviceState *parent, const char *name)
     return SSI_BUS(bus);
 }
 
-uint32_t ssi_transfer(SSIBus *bus, uint32_t val)
+uint8_t ssi_transfer8(SSIBus *bus, uint8_t val)
 {
     BusState *b = BUS(bus);
     BusChild *kid;
-    uint32_t r = 0;
+    uint8_t r = 0;
 
     QTAILQ_FOREACH(kid, &b->children, sibling) {
         SSIPeripheral *p = SSI_PERIPHERAL(kid->child);
diff --git a/hw/ssi/stm32f2xx_spi.c b/hw/ssi/stm32f2xx_spi.c
index 871d57324d..eca0b93f3f 100644
--- a/hw/ssi/stm32f2xx_spi.c
+++ b/hw/ssi/stm32f2xx_spi.c
@@ -59,7 +59,7 @@ static void stm32f2xx_spi_transfer(STM32F2XXSPIState *s)
 {
     DB_PRINT("Data to send: 0x%x\n", s->spi_dr);
 
-    s->spi_dr = ssi_transfer(s->ssi, s->spi_dr);
+    s->spi_dr = ssi_transfer8(s->ssi, s->spi_dr);
     s->spi_sr |= STM_SPI_SR_RXNE;
 
     DB_PRINT("Data received: 0x%x\n", s->spi_dr);
diff --git a/hw/ssi/xilinx_spi.c b/hw/ssi/xilinx_spi.c
index 79f3e8bfae..45e9459396 100644
--- a/hw/ssi/xilinx_spi.c
+++ b/hw/ssi/xilinx_spi.c
@@ -176,18 +176,18 @@ static inline int spi_master_enabled(XilinxSPI *s)
 
 static void spi_flush_txfifo(XilinxSPI *s)
 {
-    uint32_t tx;
-    uint32_t rx;
+   uint8_t tx;
+   uint8_t rx;
 
     while (!fifo8_is_empty(&s->tx_fifo)) {
-        tx = (uint32_t)fifo8_pop(&s->tx_fifo);
+        tx = fifo8_pop(&s->tx_fifo);
         DB_PRINT("data tx:%x\n", tx);
-        rx = ssi_transfer(s->spi, tx);
+        rx = ssi_transfer8(s->spi, tx);
         DB_PRINT("data rx:%x\n", rx);
         if (fifo8_is_full(&s->rx_fifo)) {
             s->regs[R_IPISR] |= IRQ_DRR_OVERRUN;
         } else {
-            fifo8_push(&s->rx_fifo, (uint8_t)rx);
+            fifo8_push(&s->rx_fifo, rx);
             if (fifo8_is_full(&s->rx_fifo)) {
                 s->regs[R_SPISR] |= SR_RX_FULL;
                 s->regs[R_IPISR] |= IRQ_DRR_FULL;
diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
index e4fce2c195..b915000770 100644
--- a/hw/ssi/xilinx_spips.c
+++ b/hw/ssi/xilinx_spips.c
@@ -576,7 +576,7 @@ static void xlnx_zynqmp_qspips_flush_fifo_g(XlnxZynqMPQSPIPS *s)
         busses = ARRAY_FIELD_EX32(s->regs, GQSPI_GF_SNAPSHOT, DATA_BUS_SELECT);
         for (i = 0; i < 2; ++i) {
             DB_PRINT_L(1, "bus %d tx = %02x\n", i, tx_rx[i]);
-            tx_rx[i] = ssi_transfer(XILINX_SPIPS(s)->spi[i], tx_rx[i]);
+            tx_rx[i] = ssi_transfer8(XILINX_SPIPS(s)->spi[i], tx_rx[i]);
             DB_PRINT_L(1, "bus %d rx = %02x\n", i, tx_rx[i]);
         }
         if (s->regs[R_GQSPI_DATA_STS] > 1 &&
@@ -696,7 +696,7 @@ static void xilinx_spips_flush_txfifo(XilinxSPIPS *s)
             int bus = num_effective_busses(s) - 1 - i;
 
             DB_PRINT_L(debug_level, "tx = %02x\n", tx_rx[i]);
-            tx_rx[i] = ssi_transfer(s->spi[bus], (uint32_t)tx_rx[i]);
+            tx_rx[i] = ssi_transfer8(s->spi[bus], tx_rx[i]);
             DB_PRINT_L(debug_level, "rx = %02x\n", tx_rx[i]);
         }
 
diff --git a/hw/ssi/xlnx-versal-ospi.c b/hw/ssi/xlnx-versal-ospi.c
index e25e4c26c2..8f2cb71414 100644
--- a/hw/ssi/xlnx-versal-ospi.c
+++ b/hw/ssi/xlnx-versal-ospi.c
@@ -631,9 +631,9 @@ static void ospi_disable_cs(XlnxVersalOspi *s)
 static void ospi_flush_txfifo(XlnxVersalOspi *s)
 {
     while (!fifo8_is_empty(&s->tx_fifo)) {
-        uint32_t tx_rx = fifo8_pop(&s->tx_fifo);
+        uint8_t tx_rx = fifo8_pop(&s->tx_fifo);
 
-        tx_rx = ssi_transfer(s->spi, tx_rx);
+        tx_rx = ssi_transfer8(s->spi, tx_rx);
         fifo8_push(&s->rx_fifo, tx_rx);
     }
 }
diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
index 6d6d8ccb3d..e8be6c2023 100644
--- a/include/hw/ssi/ssi.h
+++ b/include/hw/ssi/ssi.h
@@ -38,7 +38,7 @@ struct SSIPeripheralClass {
 
     /* if you have standard or no CS behaviour, just override transfer.
      * This is called when the device cs is active (true by default).
-     * See ssi_transfer().
+     * See ssi_transfer8().
      */
     uint32_t (*transfer)(SSIPeripheral *dev, uint32_t val);
     /* called when the CS line changes. Optional, devices only need to implement
@@ -53,7 +53,7 @@ struct SSIPeripheralClass {
      * of the CS behaviour at the device level. transfer, set_cs, and
      * cs_polarity are unused if this is overwritten. Transfer_raw will
      * always be called for the device for every txrx access to the parent bus
-     * See ssi_transfer().
+     * See ssi_transfer8().
      */
     uint32_t (*transfer_raw)(SSIPeripheral *dev, uint32_t val);
 };
@@ -113,18 +113,17 @@ bool ssi_realize_and_unref(DeviceState *dev, SSIBus *bus, Error **errp);
 SSIBus *ssi_create_bus(DeviceState *parent, const char *name);
 
 /**
- * Transfer a word on a SSI bus
+ * Transfer a byte on a SSI bus
  * @bus: SSI bus
- * @val: word to transmit
+ * @val: byte to transmit
  *
- * At the same time, read a word and write the @val one on the SSI bus.
+ * At the same time, read a byte and write the @val one on the SSI bus.
  *
- * SSI words might vary between 8 and 32 bits. The same number of bits
- * written is received.
+ * SSI always transfers and receives 8-bits.
  *
- * Return: word value received
+ * Return: byte received
  */
-uint32_t ssi_transfer(SSIBus *bus, uint32_t val);
+uint8_t ssi_transfer8(SSIBus *bus, uint8_t val);
 
 DeviceState *ssi_get_cs(SSIBus *bus, uint8_t cs_index);
 
-- 
2.43.0

Re: [PATCH 1/4] hw/ssi: Rename ssi_transfer to ssi_transfer8

[Alistair]

On Tue, 2026-08-25 at 14:04 +1000, stephensportia@gmail.com wrote:
> From: Portia Stephens <portias@oss.tenstorrent.com>
> 
> The ssi_transfer function comments say that it takes a word varying
> between 8-bits and 32-bits. ssi_transfer takes a 32-bit arg to
> transfer
> but there is no means to indicate the number of bits that should
> actually be transferred. All child classes of SSI_PERIPHERAL class
> have
> transfer functions that, despite accepting a 32-bit tx, only transfer
> a
> single byte; m25p80_transfer8(), ssi_sd_transfer(),
> ssd0323_transfer().
> 
> The current implementation depends on the SSI model to know
> what peripheral model will be attached and what transfer size it
> expects which is error prone. If a SSI_PERIPHERAL model was written
> that
> accepted 32-bit transfers, it could not attach to any existing SSI
> models.
> 
> This change updates the naming of ssi_transfer to ssi_transfer8, as
> well as changes the return value and transmit argument to be 8-bit.
> 
> Signed-off-by: Portia Stephens <portias@oss.tenstorrent.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/arm/strongarm.c         |  2 +-
>  hw/ssi/allwinner-a10-spi.c |  2 +-
>  hw/ssi/aspeed_smc.c        | 14 ++++++-------
>  hw/ssi/bcm2835_spi.c       |  2 +-
>  hw/ssi/ibex_spi_host.c     |  5 +++--
>  hw/ssi/imx_spi.c           |  2 +-
>  hw/ssi/mss-spi.c           |  2 +-
>  hw/ssi/npcm7xx_fiu.c       | 42 +++++++++++++++++++-----------------
> --
>  hw/ssi/npcm_pspi.c         |  4 ++--
>  hw/ssi/pl022.c             |  2 +-
>  hw/ssi/pnv_spi.c           |  2 +-
>  hw/ssi/sifive_spi.c        |  2 +-
>  hw/ssi/ssi.c               |  4 ++--
>  hw/ssi/stm32f2xx_spi.c     |  2 +-
>  hw/ssi/xilinx_spi.c        | 10 ++++-----
>  hw/ssi/xilinx_spips.c      |  4 ++--
>  hw/ssi/xlnx-versal-ospi.c  |  4 ++--
>  include/hw/ssi/ssi.h       | 17 ++++++++-------
>  18 files changed, 61 insertions(+), 61 deletions(-)
> 
> diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c
> index 5a3242195a..e400f0a185 100644
> --- a/hw/arm/strongarm.c
> +++ b/hw/arm/strongarm.c
> @@ -1516,7 +1516,7 @@ static void strongarm_ssp_write(void *opaque,
> hwaddr addr,
>              if (s->sscr[1] & SSCR1_LBM) {
>                  readval = value;
>              } else {
> -                readval = ssi_transfer(s->bus, value);
> +                readval = ssi_transfer8(s->bus, value);
>              }
>  
>              if (s->rx_level < 0x08) {
> diff --git a/hw/ssi/allwinner-a10-spi.c b/hw/ssi/allwinner-a10-spi.c
> index 69920b935a..5758e81c10 100644
> --- a/hw/ssi/allwinner-a10-spi.c
> +++ b/hw/ssi/allwinner-a10-spi.c
> @@ -300,7 +300,7 @@ static void
> allwinner_a10_spi_flush_txfifo(AWA10SPIState *s)
>          trace_allwinner_a10_spi_tx(tx);
>  
>          /* Write one byte at a time */
> -        rx = ssi_transfer(s->bus, tx);
> +        rx = ssi_transfer8(s->bus, tx);
>  
>          trace_allwinner_a10_spi_rx(rx);
>  
> diff --git a/hw/ssi/aspeed_smc.c b/hw/ssi/aspeed_smc.c
> index bf596f7b2d..f6e8dd9457 100644
> --- a/hw/ssi/aspeed_smc.c
> +++ b/hw/ssi/aspeed_smc.c
> @@ -488,10 +488,10 @@ static void
> aspeed_smc_flash_setup(AspeedSMCFlash *fl, uint32_t addr)
>      /* Flash access can not exceed CS segment */
>      addr = aspeed_smc_check_segment_addr(fl, addr);
>  
> -    ssi_transfer(s->spi, cmd);
> +    ssi_transfer8(s->spi, cmd);
>      while (i--) {
>          if (aspeed_smc_addr_byte_enabled(s, i)) {
> -            ssi_transfer(s->spi, (addr >> (i * 8)) & 0xff);
> +            ssi_transfer8(s->spi, (addr >> (i * 8)) & 0xff);
>          }
>      }
>  
> @@ -503,7 +503,7 @@ static void aspeed_smc_flash_setup(AspeedSMCFlash
> *fl, uint32_t addr)
>       */
>      if (aspeed_smc_flash_mode(fl) == CTRL_FREADMODE) {
>          for (i = 0; i < aspeed_smc_flash_dummy_bytes(fl); i++) {
> -            ssi_transfer(fl->controller->spi, s->regs[R_DUMMY_DATA]
> & 0xff);
> +            ssi_transfer8(fl->controller->spi, s->regs[R_DUMMY_DATA]
> & 0xff);
>          }
>      }
>  }
> @@ -519,7 +519,7 @@ static MemTxResult aspeed_smc_flash_read(void
> *opaque, hwaddr addr,
>      switch (aspeed_smc_flash_mode(fl)) {
>      case CTRL_USERMODE:
>          for (i = 0; i < size; i++) {
> -            *data |= (uint64_t) ssi_transfer(s->spi, 0x0) << (8 *
> i);
> +            *data |= (uint64_t) ssi_transfer8(s->spi, 0x0) << (8 *
> i);
>          }
>          break;
>      case CTRL_READMODE:
> @@ -528,7 +528,7 @@ static MemTxResult aspeed_smc_flash_read(void
> *opaque, hwaddr addr,
>          aspeed_smc_flash_setup(fl, addr);
>  
>          for (i = 0; i < size; i++) {
> -            *data |= (uint64_t) ssi_transfer(s->spi, 0x0) << (8 *
> i);
> +            *data |= (uint64_t) ssi_transfer8(s->spi, 0x0) << (8 *
> i);
>          }
>  
>          aspeed_smc_flash_unselect(fl);
> @@ -561,7 +561,7 @@ static MemTxResult aspeed_smc_flash_write(void
> *opaque, hwaddr addr,
>      switch (aspeed_smc_flash_mode(fl)) {
>      case CTRL_USERMODE:
>          for (i = 0; i < size; i++) {
> -            ssi_transfer(s->spi, (data >> (8 * i)) & 0xff);
> +            ssi_transfer8(s->spi, (data >> (8 * i)) & 0xff);
>          }
>          break;
>      case CTRL_WRITEMODE:
> @@ -569,7 +569,7 @@ static MemTxResult aspeed_smc_flash_write(void
> *opaque, hwaddr addr,
>          aspeed_smc_flash_setup(fl, addr);
>  
>          for (i = 0; i < size; i++) {
> -            ssi_transfer(s->spi, (data >> (8 * i)) & 0xff);
> +            ssi_transfer8(s->spi, (data >> (8 * i)) & 0xff);
>          }
>  
>          aspeed_smc_flash_unselect(fl);
> diff --git a/hw/ssi/bcm2835_spi.c b/hw/ssi/bcm2835_spi.c
> index 01763c458c..7a0a8fa392 100644
> --- a/hw/ssi/bcm2835_spi.c
> +++ b/hw/ssi/bcm2835_spi.c
> @@ -91,7 +91,7 @@ static void
> bcm2835_spi_flush_tx_fifo(BCM2835SPIState *s)
>  
>      while (!fifo8_is_empty(&s->tx_fifo) && !fifo8_is_full(&s-
> >rx_fifo)) {
>          tx_byte = fifo8_pop(&s->tx_fifo);
> -        rx_byte = ssi_transfer(s->bus, tx_byte);
> +        rx_byte = ssi_transfer8(s->bus, tx_byte);
>          fifo8_push(&s->rx_fifo, rx_byte);
>      }
>  
> diff --git a/hw/ssi/ibex_spi_host.c b/hw/ssi/ibex_spi_host.c
> index 1e574c3fcb..b5e556eedc 100644
> --- a/hw/ssi/ibex_spi_host.c
> +++ b/hw/ssi/ibex_spi_host.c
> @@ -236,7 +236,8 @@ static void ibex_spi_host_irq(IbexSPIHostState
> *s)
>  
>  static void ibex_spi_host_transfer(IbexSPIHostState *s)
>  {
> -    uint32_t rx, tx, data;
> +    uint32_t data;
> +    uint8_t rx, tx;
>      /* Get num of one byte transfers */
>      uint8_t segment_len = FIELD_EX32(s->regs[IBEX_SPI_HOST_COMMAND],
>                                       COMMAND,  LEN);
> @@ -254,7 +255,7 @@ static void
> ibex_spi_host_transfer(IbexSPIHostState *s)
>              tx = fifo8_pop(&s->tx_fifo);
>          }
>  
> -        rx = ssi_transfer(s->ssi, tx);
> +        rx = ssi_transfer8(s->ssi, tx);
>  
>          trace_ibex_spi_host_transfer(tx, rx);
>  
> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> index 8e014b7a7b..b25cf6c559 100644
> --- a/hw/ssi/imx_spi.c
> +++ b/hw/ssi/imx_spi.c
> @@ -194,7 +194,7 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
>              DPRINTF("writing 0x%02x\n", (uint32_t)byte);
>  
>              /* We need to write one byte at a time */
> -            byte = ssi_transfer(s->bus, byte);
> +            byte = ssi_transfer8(s->bus, byte);
>  
>              DPRINTF("0x%02x read\n", (uint32_t)byte);
>  
> diff --git a/hw/ssi/mss-spi.c b/hw/ssi/mss-spi.c
> index 3c118fc0f8..8a7af68a8a 100644
> --- a/hw/ssi/mss-spi.c
> +++ b/hw/ssi/mss-spi.c
> @@ -234,7 +234,7 @@ static void spi_flush_txfifo(MSSSpiState *s)
>  
>          tx = fifo32_pop(&s->tx_fifo);
>          DB_PRINT("data tx:0x%" PRIx32, tx);
> -        rx = ssi_transfer(s->spi, tx);
> +        rx = ssi_transfer8(s->spi, tx);
>          DB_PRINT("data rx:0x%" PRIx32, rx);
>  
>          if (fifo32_num_used(&s->rx_fifo) == s->fifo_depth) {
> diff --git a/hw/ssi/npcm7xx_fiu.c b/hw/ssi/npcm7xx_fiu.c
> index d41d877cfb..b0be46aa18 100644
> --- a/hw/ssi/npcm7xx_fiu.c
> +++ b/hw/ssi/npcm7xx_fiu.c
> @@ -162,16 +162,16 @@ static uint64_t npcm7xx_fiu_flash_read(void
> *opaque, hwaddr addr,
>      npcm7xx_fiu_select(fiu, npcm7xx_fiu_cs_index(fiu, f));
>  
>      drd_cfg = fiu->regs[NPCM7XX_FIU_DRD_CFG];
> -    ssi_transfer(fiu->spi, FIU_DRD_CFG_RDCMD(drd_cfg));
> +    ssi_transfer8(fiu->spi, FIU_DRD_CFG_RDCMD(drd_cfg));
>  
>      switch (FIU_DRD_CFG_ADDSIZ(drd_cfg)) {
>      case FIU_ADDSIZ_4BYTES:
> -        ssi_transfer(fiu->spi, extract32(addr, 24, 8));
> +        ssi_transfer8(fiu->spi, extract32(addr, 24, 8));
>          /* fall through */
>      case FIU_ADDSIZ_3BYTES:
> -        ssi_transfer(fiu->spi, extract32(addr, 16, 8));
> -        ssi_transfer(fiu->spi, extract32(addr, 8, 8));
> -        ssi_transfer(fiu->spi, extract32(addr, 0, 8));
> +        ssi_transfer8(fiu->spi, extract32(addr, 16, 8));
> +        ssi_transfer8(fiu->spi, extract32(addr, 8, 8));
> +        ssi_transfer8(fiu->spi, extract32(addr, 0, 8));
>          break;
>  
>      default:
> @@ -182,11 +182,11 @@ static uint64_t npcm7xx_fiu_flash_read(void
> *opaque, hwaddr addr,
>  
>      dummy_bytes = FIU_DRD_CFG_DBW(drd_cfg);
>      for (i = 0; i < dummy_bytes; i++) {
> -        ssi_transfer(fiu->spi, 0);
> +        ssi_transfer8(fiu->spi, 0);
>      }
>  
>      for (i = 0; i < size; i++) {
> -        value = deposit64(value, 8 * i, 8, ssi_transfer(fiu->spi,
> 0));
> +        value = deposit64(value, 8 * i, 8, ssi_transfer8(fiu->spi,
> 0));
>      }
>  
>      trace_npcm7xx_fiu_flash_read(DEVICE(fiu)->canonical_path, fiu-
> >active_cs,
> @@ -219,16 +219,16 @@ static void npcm7xx_fiu_flash_write(void
> *opaque, hwaddr addr, uint64_t v,
>      npcm7xx_fiu_select(fiu, cs_id);
>  
>      dwr_cfg = fiu->regs[NPCM7XX_FIU_DWR_CFG];
> -    ssi_transfer(fiu->spi, FIU_DWR_CFG_WRCMD(dwr_cfg));
> +    ssi_transfer8(fiu->spi, FIU_DWR_CFG_WRCMD(dwr_cfg));
>  
>      switch (FIU_DWR_CFG_ADDSIZ(dwr_cfg)) {
>      case FIU_ADDSIZ_4BYTES:
> -        ssi_transfer(fiu->spi, extract32(addr, 24, 8));
> +        ssi_transfer8(fiu->spi, extract32(addr, 24, 8));
>          /* fall through */
>      case FIU_ADDSIZ_3BYTES:
> -        ssi_transfer(fiu->spi, extract32(addr, 16, 8));
> -        ssi_transfer(fiu->spi, extract32(addr, 8, 8));
> -        ssi_transfer(fiu->spi, extract32(addr, 0, 8));
> +        ssi_transfer8(fiu->spi, extract32(addr, 16, 8));
> +        ssi_transfer8(fiu->spi, extract32(addr, 8, 8));
> +        ssi_transfer8(fiu->spi, extract32(addr, 0, 8));
>          break;
>  
>      default:
> @@ -238,7 +238,7 @@ static void npcm7xx_fiu_flash_write(void *opaque,
> hwaddr addr, uint64_t v,
>      }
>  
>      for (i = 0; i < size; i++) {
> -        ssi_transfer(fiu->spi, extract64(v, i * 8, 8));
> +        ssi_transfer8(fiu->spi, extract64(v, i * 8, 8));
>      }
>  
>      npcm7xx_fiu_deselect(fiu);
> @@ -287,16 +287,16 @@ static void send_address(SSIBus *spi, unsigned
> int addsiz, uint32_t addr)
>  {
>      switch (addsiz) {
>      case 4:
> -        ssi_transfer(spi, extract32(addr, 24, 8));
> +        ssi_transfer8(spi, extract32(addr, 24, 8));
>          /* fall through */
>      case 3:
> -        ssi_transfer(spi, extract32(addr, 16, 8));
> +        ssi_transfer8(spi, extract32(addr, 16, 8));
>          /* fall through */
>      case 2:
> -        ssi_transfer(spi, extract32(addr, 8, 8));
> +        ssi_transfer8(spi, extract32(addr, 8, 8));
>          /* fall through */
>      case 1:
> -        ssi_transfer(spi, extract32(addr, 0, 8));
> +        ssi_transfer8(spi, extract32(addr, 0, 8));
>          /* fall through */
>      case 0:
>          break;
> @@ -309,7 +309,7 @@ static void send_dummy_bytes(SSIBus *spi,
> uint32_t uma_cfg)
>      unsigned int i;
>  
>      for (i = 0; i < FIU_UMA_CFG_DBSIZ(uma_cfg); i++) {
> -        ssi_transfer(spi, 0);
> +        ssi_transfer8(spi, 0);
>      }
>  }
>  
> @@ -329,7 +329,7 @@ static void
> npcm7xx_fiu_uma_transaction(NPCM7xxFIUState *s)
>      /* Send command, if present. */
>      uma_cfg = s->regs[NPCM7XX_FIU_UMA_CFG];
>      if (FIU_UMA_CFG_CMDSIZ(uma_cfg) > 0) {
> -        ssi_transfer(s->spi, extract32(s->regs[NPCM7XX_FIU_UMA_CMD],
> 0, 8));
> +        ssi_transfer8(s->spi, extract32(s-
> >regs[NPCM7XX_FIU_UMA_CMD], 0, 8));
>      }
>  
>      /* Send address, if present. */
> @@ -342,7 +342,7 @@ static void
> npcm7xx_fiu_uma_transaction(NPCM7xxFIUState *s)
>              (i < 16) ? (NPCM7XX_FIU_UMA_DW0 + i / 4) :
> NPCM7XX_FIU_UMA_DW3;
>          unsigned int field = (i % 4) * 8;
>  
> -        ssi_transfer(s->spi, extract32(s->regs[reg], field, 8));
> +        ssi_transfer8(s->spi, extract32(s->regs[reg], field, 8));
>      }
>  
>      /* Send dummy bytes, if present */
> @@ -354,7 +354,7 @@ static void
> npcm7xx_fiu_uma_transaction(NPCM7xxFIUState *s)
>          unsigned int field = (i % 4) * 8;
>          uint8_t c;
>  
> -        c = ssi_transfer(s->spi, 0);
> +        c = ssi_transfer8(s->spi, 0);
>          if (reg <= NPCM7XX_FIU_UMA_DR3) {
>              s->regs[reg] = deposit32(s->regs[reg], field, 8, c);
>          }
> diff --git a/hw/ssi/npcm_pspi.c b/hw/ssi/npcm_pspi.c
> index 2e05d5dacb..38ae6e0972 100644
> --- a/hw/ssi/npcm_pspi.c
> +++ b/hw/ssi/npcm_pspi.c
> @@ -77,9 +77,9 @@ static void npcm_pspi_write_data(NPCMPSPIState *s,
> uint16_t data)
>      uint16_t value = 0;
>  
>      if (FIELD_EX16(s->regs[R_PSPI_CTL1], PSPI_CTL1, MOD)) {
> -        value = ssi_transfer(s->spi, extract16(data, 8, 8)) << 8;
> +        value = ssi_transfer8(s->spi, extract16(data, 8, 8)) << 8;
>      }
> -    value |= ssi_transfer(s->spi, extract16(data, 0, 8));
> +    value |= ssi_transfer8(s->spi, extract16(data, 0, 8));
>      s->regs[R_PSPI_DATA] = value;
>  
>      /* Mark data as available */
> diff --git a/hw/ssi/pl022.c b/hw/ssi/pl022.c
> index 715a2d21f4..eaac664ec5 100644
> --- a/hw/ssi/pl022.c
> +++ b/hw/ssi/pl022.c
> @@ -103,7 +103,7 @@ static void pl022_xfer(PL022State *s)
>          if (s->cr1 & PL022_CR1_LBM) {
>              /* Loopback mode.  */
>          } else {
> -            val = ssi_transfer(s->ssi, val);
> +            val = ssi_transfer8(s->ssi, val);
>          }
>          s->rx_fifo[o] = val & s->bitmask;
>          i = (i + 1) & 7;
> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
> index f3add8cab9..e2a8a710da 100644
> --- a/hw/ssi/pnv_spi.c
> +++ b/hw/ssi/pnv_spi.c
> @@ -209,7 +209,7 @@ static void transfer(PnvSpi *s)
>                  qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: TX_FIFO
> underflow\n");
>              }
>          }
> -        rx = ssi_transfer(s->ssi_bus, tx);
> +        rx = ssi_transfer8(s->ssi_bus, tx);
>          for (int i = 0; i < s->transfer_len; i++) {
>              if ((offset + i) >= payload_len) {
>                  break;
> diff --git a/hw/ssi/sifive_spi.c b/hw/ssi/sifive_spi.c
> index 2ece78053b..1a2aac2daa 100644
> --- a/hw/ssi/sifive_spi.c
> +++ b/hw/ssi/sifive_spi.c
> @@ -134,7 +134,7 @@ static void
> sifive_spi_flush_txfifo(SiFiveSPIState *s)
>  
>      while (!fifo8_is_empty(&s->tx_fifo)) {
>          tx = fifo8_pop(&s->tx_fifo);
> -        rx = ssi_transfer(s->spi, tx);
> +        rx = ssi_transfer8(s->spi, tx);
>  
>          if (!fifo8_is_full(&s->rx_fifo)) {
>              if (!(s->regs[R_FMT] & FMT_DIR)) {
> diff --git a/hw/ssi/ssi.c b/hw/ssi/ssi.c
> index 3a4fade2d5..ce2325bbd4 100644
> --- a/hw/ssi/ssi.c
> +++ b/hw/ssi/ssi.c
> @@ -153,11 +153,11 @@ SSIBus *ssi_create_bus(DeviceState *parent,
> const char *name)
>      return SSI_BUS(bus);
>  }
>  
> -uint32_t ssi_transfer(SSIBus *bus, uint32_t val)
> +uint8_t ssi_transfer8(SSIBus *bus, uint8_t val)
>  {
>      BusState *b = BUS(bus);
>      BusChild *kid;
> -    uint32_t r = 0;
> +    uint8_t r = 0;
>  
>      QTAILQ_FOREACH(kid, &b->children, sibling) {
>          SSIPeripheral *p = SSI_PERIPHERAL(kid->child);
> diff --git a/hw/ssi/stm32f2xx_spi.c b/hw/ssi/stm32f2xx_spi.c
> index 871d57324d..eca0b93f3f 100644
> --- a/hw/ssi/stm32f2xx_spi.c
> +++ b/hw/ssi/stm32f2xx_spi.c
> @@ -59,7 +59,7 @@ static void
> stm32f2xx_spi_transfer(STM32F2XXSPIState *s)
>  {
>      DB_PRINT("Data to send: 0x%x\n", s->spi_dr);
>  
> -    s->spi_dr = ssi_transfer(s->ssi, s->spi_dr);
> +    s->spi_dr = ssi_transfer8(s->ssi, s->spi_dr);
>      s->spi_sr |= STM_SPI_SR_RXNE;
>  
>      DB_PRINT("Data received: 0x%x\n", s->spi_dr);
> diff --git a/hw/ssi/xilinx_spi.c b/hw/ssi/xilinx_spi.c
> index 79f3e8bfae..45e9459396 100644
> --- a/hw/ssi/xilinx_spi.c
> +++ b/hw/ssi/xilinx_spi.c
> @@ -176,18 +176,18 @@ static inline int spi_master_enabled(XilinxSPI
> *s)
>  
>  static void spi_flush_txfifo(XilinxSPI *s)
>  {
> -    uint32_t tx;
> -    uint32_t rx;
> +   uint8_t tx;
> +   uint8_t rx;
>  
>      while (!fifo8_is_empty(&s->tx_fifo)) {
> -        tx = (uint32_t)fifo8_pop(&s->tx_fifo);
> +        tx = fifo8_pop(&s->tx_fifo);
>          DB_PRINT("data tx:%x\n", tx);
> -        rx = ssi_transfer(s->spi, tx);
> +        rx = ssi_transfer8(s->spi, tx);
>          DB_PRINT("data rx:%x\n", rx);
>          if (fifo8_is_full(&s->rx_fifo)) {
>              s->regs[R_IPISR] |= IRQ_DRR_OVERRUN;
>          } else {
> -            fifo8_push(&s->rx_fifo, (uint8_t)rx);
> +            fifo8_push(&s->rx_fifo, rx);
>              if (fifo8_is_full(&s->rx_fifo)) {
>                  s->regs[R_SPISR] |= SR_RX_FULL;
>                  s->regs[R_IPISR] |= IRQ_DRR_FULL;
> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> index e4fce2c195..b915000770 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -576,7 +576,7 @@ static void
> xlnx_zynqmp_qspips_flush_fifo_g(XlnxZynqMPQSPIPS *s)
>          busses = ARRAY_FIELD_EX32(s->regs, GQSPI_GF_SNAPSHOT,
> DATA_BUS_SELECT);
>          for (i = 0; i < 2; ++i) {
>              DB_PRINT_L(1, "bus %d tx = %02x\n", i, tx_rx[i]);
> -            tx_rx[i] = ssi_transfer(XILINX_SPIPS(s)->spi[i],
> tx_rx[i]);
> +            tx_rx[i] = ssi_transfer8(XILINX_SPIPS(s)->spi[i],
> tx_rx[i]);
>              DB_PRINT_L(1, "bus %d rx = %02x\n", i, tx_rx[i]);
>          }
>          if (s->regs[R_GQSPI_DATA_STS] > 1 &&
> @@ -696,7 +696,7 @@ static void xilinx_spips_flush_txfifo(XilinxSPIPS
> *s)
>              int bus = num_effective_busses(s) - 1 - i;
>  
>              DB_PRINT_L(debug_level, "tx = %02x\n", tx_rx[i]);
> -            tx_rx[i] = ssi_transfer(s->spi[bus],
> (uint32_t)tx_rx[i]);
> +            tx_rx[i] = ssi_transfer8(s->spi[bus], tx_rx[i]);
>              DB_PRINT_L(debug_level, "rx = %02x\n", tx_rx[i]);
>          }
>  
> diff --git a/hw/ssi/xlnx-versal-ospi.c b/hw/ssi/xlnx-versal-ospi.c
> index e25e4c26c2..8f2cb71414 100644
> --- a/hw/ssi/xlnx-versal-ospi.c
> +++ b/hw/ssi/xlnx-versal-ospi.c
> @@ -631,9 +631,9 @@ static void ospi_disable_cs(XlnxVersalOspi *s)
>  static void ospi_flush_txfifo(XlnxVersalOspi *s)
>  {
>      while (!fifo8_is_empty(&s->tx_fifo)) {
> -        uint32_t tx_rx = fifo8_pop(&s->tx_fifo);
> +        uint8_t tx_rx = fifo8_pop(&s->tx_fifo);
>  
> -        tx_rx = ssi_transfer(s->spi, tx_rx);
> +        tx_rx = ssi_transfer8(s->spi, tx_rx);
>          fifo8_push(&s->rx_fifo, tx_rx);
>      }
>  }
> diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
> index 6d6d8ccb3d..e8be6c2023 100644
> --- a/include/hw/ssi/ssi.h
> +++ b/include/hw/ssi/ssi.h
> @@ -38,7 +38,7 @@ struct SSIPeripheralClass {
>  
>      /* if you have standard or no CS behaviour, just override
> transfer.
>       * This is called when the device cs is active (true by
> default).
> -     * See ssi_transfer().
> +     * See ssi_transfer8().
>       */
>      uint32_t (*transfer)(SSIPeripheral *dev, uint32_t val);
>      /* called when the CS line changes. Optional, devices only need
> to implement
> @@ -53,7 +53,7 @@ struct SSIPeripheralClass {
>       * of the CS behaviour at the device level. transfer, set_cs,
> and
>       * cs_polarity are unused if this is overwritten. Transfer_raw
> will
>       * always be called for the device for every txrx access to the
> parent bus
> -     * See ssi_transfer().
> +     * See ssi_transfer8().
>       */
>      uint32_t (*transfer_raw)(SSIPeripheral *dev, uint32_t val);
>  };
> @@ -113,18 +113,17 @@ bool ssi_realize_and_unref(DeviceState *dev,
> SSIBus *bus, Error **errp);
>  SSIBus *ssi_create_bus(DeviceState *parent, const char *name);
>  
>  /**
> - * Transfer a word on a SSI bus
> + * Transfer a byte on a SSI bus
>   * @bus: SSI bus
> - * @val: word to transmit
> + * @val: byte to transmit
>   *
> - * At the same time, read a word and write the @val one on the SSI
> bus.
> + * At the same time, read a byte and write the @val one on the SSI
> bus.
>   *
> - * SSI words might vary between 8 and 32 bits. The same number of
> bits
> - * written is received.
> + * SSI always transfers and receives 8-bits.
>   *
> - * Return: word value received
> + * Return: byte received
>   */
> -uint32_t ssi_transfer(SSIBus *bus, uint32_t val);
> +uint8_t ssi_transfer8(SSIBus *bus, uint8_t val);
>  
>  DeviceState *ssi_get_cs(SSIBus *bus, uint8_t cs_index);
>  

[PATCH 2/4] hw/ssi/pl022: Fix dropped upper bytes of ssi transfer

[stephensportia]

From: Portia Stephens <portias@oss.tenstorrent.com>

The pl022 model suports transferring a 8-bit or 16-bit frame width. The
16-bit transfer is broken since the upper 8-bits are being dropped by
the SSI peripheral transfer function. Fix this by adding a second call
to ssi_transfer8() when the frame wdith is 16-bits.

Signed-off-by: Portia Stephens <portias@oss.tenstorrent.com>
---
 hw/ssi/pl022.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/ssi/pl022.c b/hw/ssi/pl022.c
index eaac664ec5..f4ade45ca9 100644
--- a/hw/ssi/pl022.c
+++ b/hw/ssi/pl022.c
@@ -74,7 +74,6 @@ static void pl022_xfer(PL022State *s)
 {
     int i;
     int o;
-    int val;
 
     if ((s->cr1 & PL022_CR1_SSE) == 0) {
         pl022_update(s);
@@ -99,13 +98,18 @@ static void pl022_xfer(PL022State *s)
        the transfer has completed.  */
     while (s->tx_fifo_len && s->rx_fifo_len < 8) {
         DPRINTF("xfer\n");
-        val = s->tx_fifo[i];
+        uint16_t tx = s->tx_fifo[i];
+        uint16_t rx = 0;
         if (s->cr1 & PL022_CR1_LBM) {
             /* Loopback mode.  */
+            rx = tx;
         } else {
-            val = ssi_transfer8(s->ssi, val);
+            if (s->bitmask > 0xff) {
+                rx |= (ssi_transfer8(s->ssi, (tx >> 8) & 0xff) << 8);
+            }
+            rx |= ssi_transfer8(s->ssi, tx & 0xff);
         }
-        s->rx_fifo[o] = val & s->bitmask;
+        s->rx_fifo[o] = rx & s->bitmask;
         i = (i + 1) & 7;
         o = (o + 1) & 7;
         s->tx_fifo_len--;
-- 
2.43.0

Re: [PATCH 2/4] hw/ssi/pl022: Fix dropped upper bytes of ssi transfer

[Alistair]

On Tue, 2026-08-25 at 14:04 +1000, stephensportia@gmail.com wrote:
> From: Portia Stephens <portias@oss.tenstorrent.com>
> 
> The pl022 model suports transferring a 8-bit or 16-bit frame width.
> The
> 16-bit transfer is broken since the upper 8-bits are being dropped by
> the SSI peripheral transfer function. Fix this by adding a second
> call
> to ssi_transfer8() when the frame wdith is 16-bits.
> 
> Signed-off-by: Portia Stephens <portias@oss.tenstorrent.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/ssi/pl022.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ssi/pl022.c b/hw/ssi/pl022.c
> index eaac664ec5..f4ade45ca9 100644
> --- a/hw/ssi/pl022.c
> +++ b/hw/ssi/pl022.c
> @@ -74,7 +74,6 @@ static void pl022_xfer(PL022State *s)
>  {
>      int i;
>      int o;
> -    int val;
>  
>      if ((s->cr1 & PL022_CR1_SSE) == 0) {
>          pl022_update(s);
> @@ -99,13 +98,18 @@ static void pl022_xfer(PL022State *s)
>         the transfer has completed.  */
>      while (s->tx_fifo_len && s->rx_fifo_len < 8) {
>          DPRINTF("xfer\n");
> -        val = s->tx_fifo[i];
> +        uint16_t tx = s->tx_fifo[i];
> +        uint16_t rx = 0;
>          if (s->cr1 & PL022_CR1_LBM) {
>              /* Loopback mode.  */
> +            rx = tx;
>          } else {
> -            val = ssi_transfer8(s->ssi, val);
> +            if (s->bitmask > 0xff) {
> +                rx |= (ssi_transfer8(s->ssi, (tx >> 8) & 0xff) <<
> 8);
> +            }
> +            rx |= ssi_transfer8(s->ssi, tx & 0xff);
>          }
> -        s->rx_fifo[o] = val & s->bitmask;
> +        s->rx_fifo[o] = rx & s->bitmask;
>          i = (i + 1) & 7;
>          o = (o + 1) & 7;
>          s->tx_fifo_len--;

[PATCH 3/4] hw/arm/strongarm: Fix dropped upper byte of ssi transfer

[stephensportia]

From: Portia Stephens <portias@oss.tenstorrent.com>

The strongarm model intends to transfer a 16-bit value over SSI. There
is no machine using this model with a peripheral attached so it is
impossible to know what the intended SSI peripheral is.
There is no in-tree peripheral support for 16-bit transfer, update to
use 8-bit transfer.

Signed-off-by: Portia Stephens <portias@oss.tenstorrent.com>
---
 hw/arm/strongarm.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c
index e400f0a185..2df895c75b 100644
--- a/hw/arm/strongarm.c
+++ b/hw/arm/strongarm.c
@@ -1512,11 +1512,14 @@ static void strongarm_ssp_write(void *opaque, hwaddr addr,
          * there directly to the slave, no need to buffer it.
          */
         if (s->sscr[0] & SSCR0_SSE) {
-            uint32_t readval;
+            uint32_t readval = 0;
             if (s->sscr[1] & SSCR1_LBM) {
                 readval = value;
             } else {
-                readval = ssi_transfer8(s->bus, value);
+                if (SSCR0_DSS(s->sscr[0]) > 8) {
+                    readval |= ssi_transfer8(s->bus, (value >> 8) & 0xff) << 8;
+                }
+                readval |= ssi_transfer8(s->bus, value & 0xff);
             }
 
             if (s->rx_level < 0x08) {
-- 
2.43.0

Re: [PATCH 3/4] hw/arm/strongarm: Fix dropped upper byte of ssi transfer

[Alistair]

On Tue, 2026-08-25 at 14:04 +1000, stephensportia@gmail.com wrote:
> From: Portia Stephens <portias@oss.tenstorrent.com>
> 
> The strongarm model intends to transfer a 16-bit value over SSI.
> There
> is no machine using this model with a peripheral attached so it is
> impossible to know what the intended SSI peripheral is.
> There is no in-tree peripheral support for 16-bit transfer, update to
> use 8-bit transfer.
> 
> Signed-off-by: Portia Stephens <portias@oss.tenstorrent.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/arm/strongarm.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/strongarm.c b/hw/arm/strongarm.c
> index e400f0a185..2df895c75b 100644
> --- a/hw/arm/strongarm.c
> +++ b/hw/arm/strongarm.c
> @@ -1512,11 +1512,14 @@ static void strongarm_ssp_write(void *opaque,
> hwaddr addr,
>           * there directly to the slave, no need to buffer it.
>           */
>          if (s->sscr[0] & SSCR0_SSE) {
> -            uint32_t readval;
> +            uint32_t readval = 0;
>              if (s->sscr[1] & SSCR1_LBM) {
>                  readval = value;
>              } else {
> -                readval = ssi_transfer8(s->bus, value);
> +                if (SSCR0_DSS(s->sscr[0]) > 8) {
> +                    readval |= ssi_transfer8(s->bus, (value >> 8) &
> 0xff) << 8;
> +                }
> +                readval |= ssi_transfer8(s->bus, value & 0xff);
>              }
>  
>              if (s->rx_level < 0x08) {

Re: [PATCH 3/4] hw/arm/strongarm: Fix dropped upper byte of ssi transfer

[Peter Maydell]

On Tue, 25 Aug 2026 at 05:05, <stephensportia@gmail.com> wrote:
>
> From: Portia Stephens <portias@oss.tenstorrent.com>
>
> The strongarm model intends to transfer a 16-bit value over SSI. There
> is no machine using this model with a peripheral attached so it is
> impossible to know what the intended SSI peripheral is.
> There is no in-tree peripheral support for 16-bit transfer, update to
> use 8-bit transfer.

This is the strongarm synchronous serial port; the manual says it
"is used to interface to a variety of analog-to-digital converters,
audio and telecom codecs, memory chips and keypad controllers
as well as other miscellaneous serial devices". I don't know
if it's actually connected to anything on real 'collie' hardware,
but if it is then  I don't think QEMU's ever emulated the connected
hardware.

The data is between 4 and 16 bits long. I think the intention
both for this hardware and for QEMU's ssi_transfer() API
is that the guest software is supposed to know what the device
on the other end of the SSI connection is, and program the
controller appropriately. If it programs the controller to
transfer 16 bits to a device that only expects 8 then things
will go wrong in both hardware and QEMU; conversely if it
sends 8 bits to a device that needs 16 that will also break.

-- PMM

Re: [PATCH 3/4] hw/arm/strongarm: Fix dropped upper byte of ssi transfer

[Portia Stephens]

On Thu, Aug 27, 2026 at 7:09 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 25 Aug 2026 at 05:05, <stephensportia@gmail.com> wrote:
> >
> > From: Portia Stephens <portias@oss.tenstorrent.com>
> >
> > The strongarm model intends to transfer a 16-bit value over SSI. There
> > is no machine using this model with a peripheral attached so it is
> > impossible to know what the intended SSI peripheral is.
> > There is no in-tree peripheral support for 16-bit transfer, update to
> > use 8-bit transfer.
>
> This is the strongarm synchronous serial port; the manual says it
> "is used to interface to a variety of analog-to-digital converters,
> audio and telecom codecs, memory chips and keypad controllers
> as well as other miscellaneous serial devices". I don't know
> if it's actually connected to anything on real 'collie' hardware,
> but if it is then  I don't think QEMU's ever emulated the connected
> hardware.
>
> The data is between 4 and 16 bits long. I think the intention

In this case, how can the device transfer function know what the
expected transfer width is? From my reading of the current
implementation, there is no way for the device's transfer function to
know what the transfer width the controller or guest software expects,
only a value is passed, or am I missing something?

> both for this hardware and for QEMU's ssi_transfer() API
> is that the guest software is supposed to know what the device
> on the other end of the SSI connection is, and program the
> controller appropriately. If it programs the controller to
> transfer 16 bits to a device that only expects 8 then things
> will go wrong in both hardware and QEMU; conversely if it
> sends 8 bits to a device that needs 16 that will also break.
>
> -- PMM

Re: [PATCH 3/4] hw/arm/strongarm: Fix dropped upper byte of ssi transfer

[Peter Maydell]

On Thu, 27 Aug 2026 at 11:53, Portia Stephens <stephensportia@gmail.com> wrote:
>
> On Thu, Aug 27, 2026 at 7:09 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Tue, 25 Aug 2026 at 05:05, <stephensportia@gmail.com> wrote:
> > >
> > > From: Portia Stephens <portias@oss.tenstorrent.com>
> > >
> > > The strongarm model intends to transfer a 16-bit value over SSI. There
> > > is no machine using this model with a peripheral attached so it is
> > > impossible to know what the intended SSI peripheral is.
> > > There is no in-tree peripheral support for 16-bit transfer, update to
> > > use 8-bit transfer.
> >
> > This is the strongarm synchronous serial port; the manual says it
> > "is used to interface to a variety of analog-to-digital converters,
> > audio and telecom codecs, memory chips and keypad controllers
> > as well as other miscellaneous serial devices". I don't know
> > if it's actually connected to anything on real 'collie' hardware,
> > but if it is then  I don't think QEMU's ever emulated the connected
> > hardware.
> >
> > The data is between 4 and 16 bits long. I think the intention
>
> In this case, how can the device transfer function know what the
> expected transfer width is? From my reading of the current
> implementation, there is no way for the device's transfer function to
> know what the transfer width the controller or guest software expects,
> only a value is passed, or am I missing something?

The device is just implemented as "I am a device which has a fixed
transfer width of X". The software running in the guest knows "the
device on the other end of this PL022 is specific device A which
has a transfer width of X", and it programs the PL022 with that
width X.

thanks
-- PMM

[PATCH 4/4] hw/ssi/pnv_spi: Fix dropped upper bytes of ssi transfer

[stephensportia]

From: Portia Stephens <portias@oss.tenstorrent.com>

The pnv_spi model supports transaction sizes of 4 bytes however there
are no in-tree SSI peripherals that support this. Update the model to
use a 8-bit transfer function.

Signed-off-by: Portia Stephens <portias@oss.tenstorrent.com>
---
 hw/ssi/pnv_spi.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
index e2a8a710da..481798b1fa 100644
--- a/hw/ssi/pnv_spi.c
+++ b/hw/ssi/pnv_spi.c
@@ -194,32 +194,26 @@ static void spi_response(PnvSpi *s)
 
 static void transfer(PnvSpi *s)
 {
-    uint32_t tx, rx, payload_len;
+    uint32_t payload_len;
     uint8_t rx_byte;
 
     payload_len = fifo8_num_used(&s->tx_fifo);
     for (int offset = 0; offset < payload_len; offset += s->transfer_len) {
-        tx = 0;
-        for (int i = 0; i < s->transfer_len; i++) {
-            if ((offset + i) >= payload_len) {
-                tx <<= 8;
-            } else if (!fifo8_is_empty(&s->tx_fifo)) {
-                tx = (tx << 8) | fifo8_pop(&s->tx_fifo);
-            } else {
-                qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: TX_FIFO underflow\n");
-            }
-        }
-        rx = ssi_transfer8(s->ssi_bus, tx);
         for (int i = 0; i < s->transfer_len; i++) {
             if ((offset + i) >= payload_len) {
                 break;
             }
-            rx_byte = (rx >> (8 * (s->transfer_len - 1) - i * 8)) & 0xFF;
-            if (!fifo8_is_full(&s->rx_fifo)) {
-                fifo8_push(&s->rx_fifo, rx_byte);
+
+            if (!fifo8_is_empty(&s->tx_fifo)) {
+                rx_byte = ssi_transfer8(s->ssi_bus, fifo8_pop(&s->tx_fifo));
+                if (!fifo8_is_full(&s->rx_fifo)) {
+                    fifo8_push(&s->rx_fifo, rx_byte);
+                } else {
+                    qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: RX_FIFO is full\n");
+                    break;
+                }
             } else {
-                qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: RX_FIFO is full\n");
-                break;
+                qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: TX_FIFO underflow\n");
             }
         }
     }
-- 
2.43.0

Re: [PATCH 4/4] hw/ssi/pnv_spi: Fix dropped upper bytes of ssi transfer

[Alistair]

On Tue, 2026-08-25 at 14:04 +1000, stephensportia@gmail.com wrote:
> From: Portia Stephens <portias@oss.tenstorrent.com>
> 
> The pnv_spi model supports transaction sizes of 4 bytes however there
> are no in-tree SSI peripherals that support this. Update the model to
> use a 8-bit transfer function.
> 
> Signed-off-by: Portia Stephens <portias@oss.tenstorrent.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/ssi/pnv_spi.c | 28 +++++++++++-----------------
>  1 file changed, 11 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/ssi/pnv_spi.c b/hw/ssi/pnv_spi.c
> index e2a8a710da..481798b1fa 100644
> --- a/hw/ssi/pnv_spi.c
> +++ b/hw/ssi/pnv_spi.c
> @@ -194,32 +194,26 @@ static void spi_response(PnvSpi *s)
>  
>  static void transfer(PnvSpi *s)
>  {
> -    uint32_t tx, rx, payload_len;
> +    uint32_t payload_len;
>      uint8_t rx_byte;
>  
>      payload_len = fifo8_num_used(&s->tx_fifo);
>      for (int offset = 0; offset < payload_len; offset += s-
> >transfer_len) {
> -        tx = 0;
> -        for (int i = 0; i < s->transfer_len; i++) {
> -            if ((offset + i) >= payload_len) {
> -                tx <<= 8;
> -            } else if (!fifo8_is_empty(&s->tx_fifo)) {
> -                tx = (tx << 8) | fifo8_pop(&s->tx_fifo);
> -            } else {
> -                qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: TX_FIFO
> underflow\n");
> -            }
> -        }
> -        rx = ssi_transfer8(s->ssi_bus, tx);
>          for (int i = 0; i < s->transfer_len; i++) {
>              if ((offset + i) >= payload_len) {
>                  break;
>              }
> -            rx_byte = (rx >> (8 * (s->transfer_len - 1) - i * 8)) &
> 0xFF;
> -            if (!fifo8_is_full(&s->rx_fifo)) {
> -                fifo8_push(&s->rx_fifo, rx_byte);
> +
> +            if (!fifo8_is_empty(&s->tx_fifo)) {
> +                rx_byte = ssi_transfer8(s->ssi_bus, fifo8_pop(&s-
> >tx_fifo));
> +                if (!fifo8_is_full(&s->rx_fifo)) {
> +                    fifo8_push(&s->rx_fifo, rx_byte);
> +                } else {
> +                    qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: RX_FIFO
> is full\n");
> +                    break;
> +                }
>              } else {
> -                qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: RX_FIFO is
> full\n");
> -                break;
> +                qemu_log_mask(LOG_GUEST_ERROR, "pnv_spi: TX_FIFO
> underflow\n");
>              }
>          }
>      }

Re: [PATCH 0/4] Rename ssi_transfer to ssi_transfer8

[Alistair]

On Tue, 2026-08-25 at 14:04 +1000, stephensportia@gmail.com wrote:
> From: Portia Stephens <portias@oss.tenstorrent.com>
> 
> The ssi_transfer function comments say that it takes a word varying
> between 8-bits and 32-bits. ssi_transfer takes a 32-bit arg to
> transfer
> but there is no means to indicate the number of bits that should
> actually be transferred. All child classes of SSI_PERIPHERAL class
> have
> transfer functions that, despite accepting a 32-bit tx, only transfer
> a
> single byte; m25p80_transfer8(), ssi_sd_transfer(),
> ssd0323_transfer().
> 
> The current implementation depends on the SSI model to know
> what peripheral model will be attached and what transfer size it
> expects which is error prone. If a SSI_PERIPHERAL model was written
> that
> accepted 32-bit transfers, it could not attach to any existing SSI
> models.
> 
> This change updates the the naming of ssi_transfer to ssi_transfer8,
> as
> well as changes the return value and transmit argument to be 8-bit.
> 
> Most ssi models handle this correctly already, sending a single byte
> at
> a time. There are a few models that are written to support non 8-bit
> transfers but there are no in-tree use cases that connect a
> peripheral
> to the SSI device. These have been updated to use 8-bit transfers.
> 
> Portia Stephens (4):
>   hw/ssi: Rename ssi_transfer to ssi_transfer8
>   hw/ssi/pl022: Fix dropped upper bytes of ssi transfer
>   hw/arm/strongarm: Fix dropped upper byte of ssi transfer
>   hw/ssi/pnv_spi: Fix dropped upper bytes of ssi transfer

Thanks!

Applied to riscv-to-apply.next

Alistair

> 
>  hw/arm/strongarm.c         |  7 +++++--
>  hw/ssi/allwinner-a10-spi.c |  2 +-
>  hw/ssi/aspeed_smc.c        | 14 ++++++-------
>  hw/ssi/bcm2835_spi.c       |  2 +-
>  hw/ssi/ibex_spi_host.c     |  5 +++--
>  hw/ssi/imx_spi.c           |  2 +-
>  hw/ssi/mss-spi.c           |  2 +-
>  hw/ssi/npcm7xx_fiu.c       | 42 +++++++++++++++++++-----------------
> --
>  hw/ssi/npcm_pspi.c         |  4 ++--
>  hw/ssi/pl022.c             | 12 +++++++----
>  hw/ssi/pnv_spi.c           | 28 ++++++++++---------------
>  hw/ssi/sifive_spi.c        |  2 +-
>  hw/ssi/ssi.c               |  4 ++--
>  hw/ssi/stm32f2xx_spi.c     |  2 +-
>  hw/ssi/xilinx_spi.c        | 10 ++++-----
>  hw/ssi/xilinx_spips.c      |  4 ++--
>  hw/ssi/xlnx-versal-ospi.c  |  4 ++--
>  include/hw/ssi/ssi.h       | 17 ++++++++-------
>  18 files changed, 82 insertions(+), 81 deletions(-)

Re: [PATCH 0/4] Rename ssi_transfer to ssi_transfer8

[Peter Maydell]

On Thu, 27 Aug 2026 at 06:37, Alistair <alistair@alistair23.me> wrote:
>
> On Tue, 2026-08-25 at 14:04 +1000, stephensportia@gmail.com wrote:
> > From: Portia Stephens <portias@oss.tenstorrent.com>
> >
> > The ssi_transfer function comments say that it takes a word varying
> > between 8-bits and 32-bits. ssi_transfer takes a 32-bit arg to
> > transfer
> > but there is no means to indicate the number of bits that should
> > actually be transferred. All child classes of SSI_PERIPHERAL class
> > have
> > transfer functions that, despite accepting a 32-bit tx, only transfer
> > a
> > single byte; m25p80_transfer8(), ssi_sd_transfer(),
> > ssd0323_transfer().
> >
> > The current implementation depends on the SSI model to know
> > what peripheral model will be attached and what transfer size it
> > expects which is error prone. If a SSI_PERIPHERAL model was written
> > that
> > accepted 32-bit transfers, it could not attach to any existing SSI
> > models.
> >
> > This change updates the the naming of ssi_transfer to ssi_transfer8,
> > as
> > well as changes the return value and transmit argument to be 8-bit.
> >
> > Most ssi models handle this correctly already, sending a single byte
> > at
> > a time. There are a few models that are written to support non 8-bit
> > transfers but there are no in-tree use cases that connect a
> > peripheral
> > to the SSI device. These have been updated to use 8-bit transfers.
> >
> > Portia Stephens (4):
> >   hw/ssi: Rename ssi_transfer to ssi_transfer8
> >   hw/ssi/pl022: Fix dropped upper bytes of ssi transfer
> >   hw/arm/strongarm: Fix dropped upper byte of ssi transfer
> >   hw/ssi/pnv_spi: Fix dropped upper bytes of ssi transfer
>
> Thanks!
>
> Applied to riscv-to-apply.next

Would you mind holding off on that until we figure out whether
this is a correct change and why we need it, please?

thanks
-- PMM

Re: [PATCH 0/4] Rename ssi_transfer to ssi_transfer8

[Alistair]

On Thu, 2026-08-27 at 10:20 +0100, Peter Maydell wrote:
> On Thu, 27 Aug 2026 at 06:37, Alistair <alistair@alistair23.me>
> wrote:
> > 
> > On Tue, 2026-08-25 at 14:04 +1000, stephensportia@gmail.com wrote:
> > > From: Portia Stephens <portias@oss.tenstorrent.com>
> > > 
> > > The ssi_transfer function comments say that it takes a word
> > > varying
> > > between 8-bits and 32-bits. ssi_transfer takes a 32-bit arg to
> > > transfer
> > > but there is no means to indicate the number of bits that should
> > > actually be transferred. All child classes of SSI_PERIPHERAL
> > > class
> > > have
> > > transfer functions that, despite accepting a 32-bit tx, only
> > > transfer
> > > a
> > > single byte; m25p80_transfer8(), ssi_sd_transfer(),
> > > ssd0323_transfer().
> > > 
> > > The current implementation depends on the SSI model to know
> > > what peripheral model will be attached and what transfer size it
> > > expects which is error prone. If a SSI_PERIPHERAL model was
> > > written
> > > that
> > > accepted 32-bit transfers, it could not attach to any existing
> > > SSI
> > > models.
> > > 
> > > This change updates the the naming of ssi_transfer to
> > > ssi_transfer8,
> > > as
> > > well as changes the return value and transmit argument to be 8-
> > > bit.
> > > 
> > > Most ssi models handle this correctly already, sending a single
> > > byte
> > > at
> > > a time. There are a few models that are written to support non 8-
> > > bit
> > > transfers but there are no in-tree use cases that connect a
> > > peripheral
> > > to the SSI device. These have been updated to use 8-bit
> > > transfers.
> > > 
> > > Portia Stephens (4):
> > >   hw/ssi: Rename ssi_transfer to ssi_transfer8
> > >   hw/ssi/pl022: Fix dropped upper bytes of ssi transfer
> > >   hw/arm/strongarm: Fix dropped upper byte of ssi transfer
> > >   hw/ssi/pnv_spi: Fix dropped upper bytes of ssi transfer
> > 
> > Thanks!
> > 
> > Applied to riscv-to-apply.next
> 
> Would you mind holding off on that until we figure out whether
> this is a correct change and why we need it, please?

Sure. Sorry I thought no one else was interested in reading it. Will
drop it until discussions are sorted

Alistair

> 
> thanks
> -- PMM

Re: [PATCH 0/4] Rename ssi_transfer to ssi_transfer8

[Philippe Mathieu-Daudé]

Hi Alistair,

On 27/8/26 07:37, Alistair wrote:
> On Tue, 2026-08-25 at 14:04 +1000, stephensportia@gmail.com wrote:
>> From: Portia Stephens <portias@oss.tenstorrent.com>
>>
>> The ssi_transfer function comments say that it takes a word varying
>> between 8-bits and 32-bits. ssi_transfer takes a 32-bit arg to
>> transfer
>> but there is no means to indicate the number of bits that should
>> actually be transferred. All child classes of SSI_PERIPHERAL class
>> have
>> transfer functions that, despite accepting a 32-bit tx, only transfer
>> a
>> single byte; m25p80_transfer8(), ssi_sd_transfer(),
>> ssd0323_transfer().
>>
>> The current implementation depends on the SSI model to know
>> what peripheral model will be attached and what transfer size it
>> expects which is error prone. If a SSI_PERIPHERAL model was written
>> that
>> accepted 32-bit transfers, it could not attach to any existing SSI
>> models.
>>
>> This change updates the the naming of ssi_transfer to ssi_transfer8,
>> as
>> well as changes the return value and transmit argument to be 8-bit.
>>
>> Most ssi models handle this correctly already, sending a single byte
>> at
>> a time. There are a few models that are written to support non 8-bit
>> transfers but there are no in-tree use cases that connect a
>> peripheral
>> to the SSI device. These have been updated to use 8-bit transfers.
>>
>> Portia Stephens (4):
>>    hw/ssi: Rename ssi_transfer to ssi_transfer8
>>    hw/ssi/pl022: Fix dropped upper bytes of ssi transfer
>>    hw/arm/strongarm: Fix dropped upper byte of ssi transfer
>>    hw/ssi/pnv_spi: Fix dropped upper bytes of ssi transfer
> 
> Thanks!
> 
> Applied to riscv-to-apply.next

Can you hold on before merging this please (or drop it from
your riscv queue)? I tagged this series to review but didn't
got a sufficient large enough slot to look at it.

In short, the reason I think this isn't the correct way to go
is SPI "words" can be any number of bits. While QEMU only
models 8-bit word devices, I have be working with 16-bit and
even 24-bit words ones, so I believe the current implementation
is right. I have be thinking of a better way to model this
granularity in our class hooks, but haven't find a good one yet.
Restricting some devices to 8-bit to simplify them move part
of the complexity to the host controller so I'm not sure it
is useful. I'll return with clearer comments when I get more
time.

Thanks,

Phil.

Re: [PATCH 0/4] Rename ssi_transfer to ssi_transfer8

[Peter Maydell]

On Tue, 25 Aug 2026 at 05:05, <stephensportia@gmail.com> wrote:
>
> From: Portia Stephens <portias@oss.tenstorrent.com>
>
> The ssi_transfer function comments say that it takes a word varying
> between 8-bits and 32-bits. ssi_transfer takes a 32-bit arg to transfer
> but there is no means to indicate the number of bits that should
> actually be transferred. All child classes of SSI_PERIPHERAL class have
> transfer functions that, despite accepting a 32-bit tx, only transfer a
> single byte; m25p80_transfer8(), ssi_sd_transfer(), ssd0323_transfer().
>
> The current implementation depends on the SSI model to know
> what peripheral model will be attached and what transfer size it
> expects which is error prone. If a SSI_PERIPHERAL model was written that
> accepted 32-bit transfers, it could not attach to any existing SSI
> models.

What is the motivation for this change?

As far as I know for the hardware SSI protocol, it is indeed
the case that the SSI controller (and/or the guest software) needs
to know what transfer size the attached device expects: the
controller just transmits (or expects to receive) however many
bits it is programmed for.

> This change updates the the naming of ssi_transfer to ssi_transfer8, as
> well as changes the return value and transmit argument to be 8-bit.

This means that the API will no longer work for a controller
and device that aren't 8-bit. We happen not to have any of those
devices today, but why specifically stop them working?

thanks
-- PMM

Re: [PATCH 0/4] Rename ssi_transfer to ssi_transfer8

[Philippe Mathieu-Daudé]

On 27/8/26 11:19, Peter Maydell wrote:
> On Tue, 25 Aug 2026 at 05:05, <stephensportia@gmail.com> wrote:
>>
>> From: Portia Stephens <portias@oss.tenstorrent.com>
>>
>> The ssi_transfer function comments say that it takes a word varying
>> between 8-bits and 32-bits. ssi_transfer takes a 32-bit arg to transfer
>> but there is no means to indicate the number of bits that should
>> actually be transferred. All child classes of SSI_PERIPHERAL class have
>> transfer functions that, despite accepting a 32-bit tx, only transfer a
>> single byte; m25p80_transfer8(), ssi_sd_transfer(), ssd0323_transfer().
>>
>> The current implementation depends on the SSI model to know
>> what peripheral model will be attached and what transfer size it
>> expects which is error prone. If a SSI_PERIPHERAL model was written that
>> accepted 32-bit transfers, it could not attach to any existing SSI
>> models.
> 
> What is the motivation for this change?
> 
> As far as I know for the hardware SSI protocol, it is indeed
> the case that the SSI controller (and/or the guest software) needs
> to know what transfer size the attached device expects: the
> controller just transmits (or expects to receive) however many
> bits it is programmed for.
> 
>> This change updates the the naming of ssi_transfer to ssi_transfer8, as
>> well as changes the return value and transmit argument to be 8-bit.
> 
> This means that the API will no longer work for a controller
> and device that aren't 8-bit.

Yes exactly. Clearer than the mail I just wrote, thanks hehe.
> We happen not to have any of those
> devices today, but why specifically stop them working?

I guess remembering having see forks with 16-bit devices, but
that was few years ago (I know forks don't have they voice here,
but just to mention the current API is working for them).

Re: [PATCH 0/4] Rename ssi_transfer to ssi_transfer8

[Portia Stephens]

On Thu, Aug 27, 2026 at 7:19 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 25 Aug 2026 at 05:05, <stephensportia@gmail.com> wrote:
> >
> > From: Portia Stephens <portias@oss.tenstorrent.com>
> >
> > The ssi_transfer function comments say that it takes a word varying
> > between 8-bits and 32-bits. ssi_transfer takes a 32-bit arg to transfer
> > but there is no means to indicate the number of bits that should
> > actually be transferred. All child classes of SSI_PERIPHERAL class have
> > transfer functions that, despite accepting a 32-bit tx, only transfer a
> > single byte; m25p80_transfer8(), ssi_sd_transfer(), ssd0323_transfer().
> >
> > The current implementation depends on the SSI model to know
> > what peripheral model will be attached and what transfer size it
> > expects which is error prone. If a SSI_PERIPHERAL model was written that
> > accepted 32-bit transfers, it could not attach to any existing SSI
> > models.
>
> What is the motivation for this change?

The motivation came from reviewing the designware ssi driver [1] and
realizing how error prone it was. The designware ssi supports varying
frame width but there is no way for the controller logic to know what
the transfer size an attached device expects. It seems like all the
code, that is actually connected to devices, is just written to expect
8-bit transfers so it should be made explicit.

https://www.mail-archive.com/qemu-devel@nongnu.org/msg1218153.html

>
> As far as I know for the hardware SSI protocol, it is indeed
> the case that the SSI controller (and/or the guest software) needs
> to know what transfer size the attached device expects: the
> controller just transmits (or expects to receive) however many
> bits it is programmed for.

This is true but do the devices currently model only support 8-bit
transfers in hardware, do they not support multiframe transfer? SPI
supports varying framewidth but the devices are not actually modeled
in this way.

A better solution may be to have multiple transfer functions,
ssi_transfer8, ssi_transfer32, etc. This would require the device to
explicitly set what it is expecting. It is challenging since the only
model's not using 8-bit transfer don't have any devices connected
upstream so testing is challenging.

>
> > This change updates the the naming of ssi_transfer to ssi_transfer8, as
> > well as changes the return value and transmit argument to be 8-bit.
>
> This means that the API will no longer work for a controller
> and device that aren't 8-bit. We happen not to have any of those
> devices today, but why specifically stop them working?
>
> thanks
> -- PMM

Re: [PATCH 0/4] Rename ssi_transfer to ssi_transfer8

[Peter Maydell]

On Thu, 27 Aug 2026 at 11:37, Portia Stephens <stephensportia@gmail.com> wrote:
>
> On Thu, Aug 27, 2026 at 7:19 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Tue, 25 Aug 2026 at 05:05, <stephensportia@gmail.com> wrote:
> > >
> > > From: Portia Stephens <portias@oss.tenstorrent.com>
> > >
> > > The ssi_transfer function comments say that it takes a word varying
> > > between 8-bits and 32-bits. ssi_transfer takes a 32-bit arg to transfer
> > > but there is no means to indicate the number of bits that should
> > > actually be transferred. All child classes of SSI_PERIPHERAL class have
> > > transfer functions that, despite accepting a 32-bit tx, only transfer a
> > > single byte; m25p80_transfer8(), ssi_sd_transfer(), ssd0323_transfer().
> > >
> > > The current implementation depends on the SSI model to know
> > > what peripheral model will be attached and what transfer size it
> > > expects which is error prone. If a SSI_PERIPHERAL model was written that
> > > accepted 32-bit transfers, it could not attach to any existing SSI
> > > models.
> >
> > What is the motivation for this change?
>
> The motivation came from reviewing the designware ssi driver [1] and
> realizing how error prone it was. The designware ssi supports varying
> frame width but there is no way for the controller logic to know what
> the transfer size an attached device expects.

But isn't this just the way the SSI specification is? If we're
modelling a "you just have to get this right in software" bit
of hardware then we don't need to somehow try to add extra
checks in QEMU for whether the software hasn't actually done
things right.

> It seems like all the
> code, that is actually connected to devices, is just written to expect
> 8-bit transfers so it should be made explicit.
>
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg1218153.html
>
> >
> > As far as I know for the hardware SSI protocol, it is indeed
> > the case that the SSI controller (and/or the guest software) needs
> > to know what transfer size the attached device expects: the
> > controller just transmits (or expects to receive) however many
> > bits it is programmed for.
>
> This is true but do the devices currently model only support 8-bit
> transfers in hardware, do they not support multiframe transfer? SPI
> supports varying framewidth but the devices are not actually modeled
> in this way.

Do the actual devices we implement support varying framewidth,
or do they actually have exactly one frame width that the controller
needs to be programmed by the guest to use ?

> A better solution may be to have multiple transfer functions,
> ssi_transfer8, ssi_transfer32, etc. This would require the device to
> explicitly set what it is expecting. It is challenging since the only
> model's not using 8-bit transfer don't have any devices connected
> upstream so testing is challenging.

This doesn't allow for bit widths that aren't a multiple of 8.

thanks
-- PMM

Re: [PATCH 0/4] Rename ssi_transfer to ssi_transfer8

[Alistair]

On Thu, 2026-08-27 at 11:53 +0100, Peter Maydell wrote:
> On Thu, 27 Aug 2026 at 11:37, Portia Stephens
> <stephensportia@gmail.com> wrote:
> > 
> > On Thu, Aug 27, 2026 at 7:19 PM Peter Maydell
> > <peter.maydell@linaro.org> wrote:
> > > 
> > > On Tue, 25 Aug 2026 at 05:05, <stephensportia@gmail.com> wrote:
> > > > 
> > > > From: Portia Stephens <portias@oss.tenstorrent.com>
> > > > 
> > > > The ssi_transfer function comments say that it takes a word
> > > > varying
> > > > between 8-bits and 32-bits. ssi_transfer takes a 32-bit arg to
> > > > transfer
> > > > but there is no means to indicate the number of bits that
> > > > should
> > > > actually be transferred. All child classes of SSI_PERIPHERAL
> > > > class have
> > > > transfer functions that, despite accepting a 32-bit tx, only
> > > > transfer a
> > > > single byte; m25p80_transfer8(), ssi_sd_transfer(),
> > > > ssd0323_transfer().
> > > > 
> > > > The current implementation depends on the SSI model to know
> > > > what peripheral model will be attached and what transfer size
> > > > it
> > > > expects which is error prone. If a SSI_PERIPHERAL model was
> > > > written that
> > > > accepted 32-bit transfers, it could not attach to any existing
> > > > SSI
> > > > models.
> > > 
> > > What is the motivation for this change?
> > 
> > The motivation came from reviewing the designware ssi driver [1]
> > and
> > realizing how error prone it was. The designware ssi supports
> > varying
> > frame width but there is no way for the controller logic to know
> > what
> > the transfer size an attached device expects.
> 
> But isn't this just the way the SSI specification is? If we're
> modelling a "you just have to get this right in software" bit
> of hardware then we don't need to somehow try to add extra
> checks in QEMU for whether the software hasn't actually done
> things right.

From my reading of things this is a bug in QEMU, not a guest issue.

The fact that aspeed_smc_flash_setup() for example iterates over a
larger value to send a single byte at a time shows that currently
everyone *thinks* think function should send a single byte.

So it does seem broken today. Making the current function clear seems
like a good step. Supporting different or larger transfers in the
future is then possible.

> 
> > It seems like all the
> > code, that is actually connected to devices, is just written to
> > expect
> > 8-bit transfers so it should be made explicit.
> > 
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg1218153.html
> > 
> > > 
> > > As far as I know for the hardware SSI protocol, it is indeed
> > > the case that the SSI controller (and/or the guest software)
> > > needs
> > > to know what transfer size the attached device expects: the
> > > controller just transmits (or expects to receive) however many
> > > bits it is programmed for.
> > 
> > This is true but do the devices currently model only support 8-bit
> > transfers in hardware, do they not support multiframe transfer? SPI
> > supports varying framewidth but the devices are not actually
> > modeled
> > in this way.
> 
> Do the actual devices we implement support varying framewidth,
> or do they actually have exactly one frame width that the controller
> needs to be programmed by the guest to use ?
> 
> > A better solution may be to have multiple transfer functions,
> > ssi_transfer8, ssi_transfer32, etc. This would require the device
> > to
> > explicitly set what it is expecting. It is challenging since the
> > only
> > model's not using 8-bit transfer don't have any devices connected
> > upstream so testing is challenging.
> 
> This doesn't allow for bit widths that aren't a multiple of 8.

Wouldn't a ssi_transfer32() just be the same as today?

Alistair

> 
> thanks
> -- PMM

Re: [PATCH 0/4] Rename ssi_transfer to ssi_transfer8

[Peter Maydell]

On Thu, 27 Aug 2026 at 12:03, Alistair <alistair@alistair23.me> wrote:
>
> On Thu, 2026-08-27 at 11:53 +0100, Peter Maydell wrote:
> > But isn't this just the way the SSI specification is? If we're
> > modelling a "you just have to get this right in software" bit
> > of hardware then we don't need to somehow try to add extra
> > checks in QEMU for whether the software hasn't actually done
> > things right.
>
> From my reading of things this is a bug in QEMU, not a guest issue.
>
> The fact that aspeed_smc_flash_setup() for example iterates over a
> larger value to send a single byte at a time shows that currently
> everyone *thinks* think function should send a single byte.

Doesn't that just show that the aspeed flash controller knows
it is always talking to an 8-bit SSI device ? It wouldn't
surprise me if the SPI-NOR standard in particular insisted on
8-bit transfers, though I can't find anything claiming to be
that standard.

> So it does seem broken today. Making the current function clear seems
> like a good step. Supporting different or larger transfers in the
> future is then possible.

I think the current interface already supports different or
larger transfers. We just happen to not be using that.

-- PMM

Re: [PATCH 0/4] Rename ssi_transfer to ssi_transfer8

[Alistair]

On Thu, 2026-08-27 at 12:17 +0100, Peter Maydell wrote:
> On Thu, 27 Aug 2026 at 12:03, Alistair <alistair@alistair23.me>
> wrote:
> > 
> > On Thu, 2026-08-27 at 11:53 +0100, Peter Maydell wrote:
> > > But isn't this just the way the SSI specification is? If we're
> > > modelling a "you just have to get this right in software" bit
> > > of hardware then we don't need to somehow try to add extra
> > > checks in QEMU for whether the software hasn't actually done
> > > things right.
> > 
> > From my reading of things this is a bug in QEMU, not a guest issue.
> > 
> > The fact that aspeed_smc_flash_setup() for example iterates over a
> > larger value to send a single byte at a time shows that currently
> > everyone *thinks* think function should send a single byte.
> 
> Doesn't that just show that the aspeed flash controller knows
> it is always talking to an 8-bit SSI device ? It wouldn't

But I don't think that's guaranteed to be true.

On real hardware the software could know that it's only ever connected
to an 8-bit SSI device, and set the driver accordingly. That seems
entirely possible.

But the hardware seems to support larger sizes reading [1].

In this case the QEMU code is the hardware, and I don't see how we know
we are only ever accessing an 8-bit device.

1:
https://patchew.org/linux/20220304083643.1079142-1-clg@kaod.org/20220304083643.1079142-5-clg@kaod.org/

> surprise me if the SPI-NOR standard in particular insisted on
> 8-bit transfers, though I can't find anything claiming to be
> that standard.
> 
> > So it does seem broken today. Making the current function clear
> > seems
> > like a good step. Supporting different or larger transfers in the
> > future is then possible.
> 
> I think the current interface already supports different or
> larger transfers. We just happen to not be using that.

Kind of. It does support larger transfers, but there is no size
argument. So how can the SSI device know how much was sent? Everything
seems to just assume a single byte, but it has no way to actually know
that.

Alistair

> 
> -- PMM

Re: [PATCH 0/4] Rename ssi_transfer to ssi_transfer8

[Cédric Le Goater]

On 8/28/26 02:07, Alistair wrote:
> On Thu, 2026-08-27 at 12:17 +0100, Peter Maydell wrote:
>> On Thu, 27 Aug 2026 at 12:03, Alistair <alistair@alistair23.me>
>> wrote:
>>>
>>> On Thu, 2026-08-27 at 11:53 +0100, Peter Maydell wrote:
>>>> But isn't this just the way the SSI specification is? If we're
>>>> modelling a "you just have to get this right in software" bit
>>>> of hardware then we don't need to somehow try to add extra
>>>> checks in QEMU for whether the software hasn't actually done
>>>> things right.
>>>
>>>  From my reading of things this is a bug in QEMU, not a guest issue.
>>>
>>> The fact that aspeed_smc_flash_setup() for example iterates over a
>>> larger value to send a single byte at a time shows that currently
>>> everyone *thinks* think function should send a single byte.
>>
>> Doesn't that just show that the aspeed flash controller knows
>> it is always talking to an 8-bit SSI device ? It wouldn't
> 
> But I don't think that's guaranteed to be true.
> 
> On real hardware the software could know that it's only ever connected
> to an 8-bit SSI device, and set the driver accordingly. That seems
> entirely possible.
> 
> But the hardware seems to support larger sizes reading [1].
> 
> In this case the QEMU code is the hardware, and I don't see how we know
> we are only ever accessing an 8-bit device.
> 
> 1:
> https://patchew.org/linux/20220304083643.1079142-1-clg@kaod.org/20220304083643.1079142-5-clg@kaod.org/

On the Aspeed SoC, MMIO accesses (1, 2, 4 bytes) to the SPI controller
are converted by the hardware into SPI bus transfers. The exact
serialization is hardware-specific. QEMU's choice is to generate
byte-oriented SPI transfers.

In any case, SPI is generally byte-oriented, with bits shifted over
1/2/4/8 I/O lines. So byte transfer is sufficient.

C.

> 
>> surprise me if the SPI-NOR standard in particular insisted on
>> 8-bit transfers, though I can't find anything claiming to be
>> that standard.
>>
>>> So it does seem broken today. Making the current function clear
>>> seems
>>> like a good step. Supporting different or larger transfers in the
>>> future is then possible.
>>
>> I think the current interface already supports different or
>> larger transfers. We just happen to not be using that.
> 
> Kind of. It does support larger transfers, but there is no size
> argument. So how can the SSI device know how much was sent? Everything
> seems to just assume a single byte, but it has no way to actually know
> that.
> 
> Alistair
> 
>>
>> -- PMM

Re: [PATCH 0/4] Rename ssi_transfer to ssi_transfer8

[Bin Meng]

On Mon, Aug 31, 2026 at 9:03 PM Cédric Le Goater <clg@kaod.org> wrote:
>
> On 8/28/26 02:07, Alistair wrote:
> > On Thu, 2026-08-27 at 12:17 +0100, Peter Maydell wrote:
> >> On Thu, 27 Aug 2026 at 12:03, Alistair <alistair@alistair23.me>
> >> wrote:
> >>>
> >>> On Thu, 2026-08-27 at 11:53 +0100, Peter Maydell wrote:
> >>>> But isn't this just the way the SSI specification is? If we're
> >>>> modelling a "you just have to get this right in software" bit
> >>>> of hardware then we don't need to somehow try to add extra
> >>>> checks in QEMU for whether the software hasn't actually done
> >>>> things right.
> >>>
> >>>  From my reading of things this is a bug in QEMU, not a guest issue.
> >>>
> >>> The fact that aspeed_smc_flash_setup() for example iterates over a
> >>> larger value to send a single byte at a time shows that currently
> >>> everyone *thinks* think function should send a single byte.
> >>
> >> Doesn't that just show that the aspeed flash controller knows
> >> it is always talking to an 8-bit SSI device ? It wouldn't
> >
> > But I don't think that's guaranteed to be true.
> >
> > On real hardware the software could know that it's only ever connected
> > to an 8-bit SSI device, and set the driver accordingly. That seems
> > entirely possible.
> >
> > But the hardware seems to support larger sizes reading [1].
> >
> > In this case the QEMU code is the hardware, and I don't see how we know
> > we are only ever accessing an 8-bit device.
> >
> > 1:
> > https://patchew.org/linux/20220304083643.1079142-1-clg@kaod.org/20220304083643.1079142-5-clg@kaod.org/
>
> On the Aspeed SoC, MMIO accesses (1, 2, 4 bytes) to the SPI controller
> are converted by the hardware into SPI bus transfers. The exact
> serialization is hardware-specific. QEMU's choice is to generate
> byte-oriented SPI transfers.
>
> In any case, SPI is generally byte-oriented, with bits shifted over
> 1/2/4/8 I/O lines. So byte transfer is sufficient.

Correct, one evidence is the spi-mem driver interface in the Linux
kernel for the spi-nor dummy cycles is converted to bytes. Although
some spi-nor devices do support different bit widths other than
8-bit/16-bit/24-bit/32-bit, other bit widths that are not multiple of
8 are currently not supported in the kernel, neither does QEMU.

>
> C.
>
> >
> >> surprise me if the SPI-NOR standard in particular insisted on
> >> 8-bit transfers, though I can't find anything claiming to be
> >> that standard.
> >>
> >>> So it does seem broken today. Making the current function clear
> >>> seems
> >>> like a good step. Supporting different or larger transfers in the
> >>> future is then possible.
> >>
> >> I think the current interface already supports different or
> >> larger transfers. We just happen to not be using that.
> >
> > Kind of. It does support larger transfers, but there is no size
> > argument. So how can the SSI device know how much was sent? Everything
> > seems to just assume a single byte, but it has no way to actually know
> > that.
> >

Regards,
Bin

Re: [PATCH 0/4] Rename ssi_transfer to ssi_transfer8

[Portia Stephens]

On Thu, Aug 27, 2026 at 8:53 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 27 Aug 2026 at 11:37, Portia Stephens <stephensportia@gmail.com> wrote:
> >
> > On Thu, Aug 27, 2026 at 7:19 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> > >
> > > On Tue, 25 Aug 2026 at 05:05, <stephensportia@gmail.com> wrote:
> > > >
> > > > From: Portia Stephens <portias@oss.tenstorrent.com>
> > > >
> > > > The ssi_transfer function comments say that it takes a word varying
> > > > between 8-bits and 32-bits. ssi_transfer takes a 32-bit arg to transfer
> > > > but there is no means to indicate the number of bits that should
> > > > actually be transferred. All child classes of SSI_PERIPHERAL class have
> > > > transfer functions that, despite accepting a 32-bit tx, only transfer a
> > > > single byte; m25p80_transfer8(), ssi_sd_transfer(), ssd0323_transfer().
> > > >
> > > > The current implementation depends on the SSI model to know
> > > > what peripheral model will be attached and what transfer size it
> > > > expects which is error prone. If a SSI_PERIPHERAL model was written that
> > > > accepted 32-bit transfers, it could not attach to any existing SSI
> > > > models.
> > >
> > > What is the motivation for this change?
> >
> > The motivation came from reviewing the designware ssi driver [1] and
> > realizing how error prone it was. The designware ssi supports varying
> > frame width but there is no way for the controller logic to know what
> > the transfer size an attached device expects.
>
> But isn't this just the way the SSI specification is? If we're
> modelling a "you just have to get this right in software" bit
> of hardware then we don't need to somehow try to add extra
> checks in QEMU for whether the software hasn't actually done
> things right.
>
> > It seems like all the
> > code, that is actually connected to devices, is just written to expect
> > 8-bit transfers so it should be made explicit.
> >
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg1218153.html
> >
> > >
> > > As far as I know for the hardware SSI protocol, it is indeed
> > > the case that the SSI controller (and/or the guest software) needs
> > > to know what transfer size the attached device expects: the
> > > controller just transmits (or expects to receive) however many
> > > bits it is programmed for.
> >
> > This is true but do the devices currently model only support 8-bit
> > transfers in hardware, do they not support multiframe transfer? SPI
> > supports varying framewidth but the devices are not actually modeled
> > in this way.
>
> Do the actual devices we implement support varying framewidth,
> or do they actually have exactly one frame width that the controller
> needs to be programmed by the guest to use ?

From my reading of the m25p80 spec, it does not have a fixed frame
width. The frame would be determined by the CS being driven and the
clock running. A frame could be 1 byte cmd + 3 byte address for a
register read. For read mode, 1 byte cmd + 3 byte address + variable
length read data. I don't understand how the 8-bit transfer size
actually maps to what hardware does.

When writing a controller that supports varying framewidths, it
requires the controller model to know what transfer width will be
expected from the device connected. You could have guest software that
correctly sets the controller framewidth to 32-bits and connects a
device that provides a 32-bit transfer function. How can the
controller differentiate this from the case where the guest software
correctly sets a 32-bit framewidth on the controller for a m25p80 read
which has a 8-bit transfer size and the controller needs to call the
transfer function 4 times to clock out the requested read data.

Maybe I am misunderstanding what a transfer is or what a framewidth
actually is or what the realistic use cases are here.

>
> > A better solution may be to have multiple transfer functions,
> > ssi_transfer8, ssi_transfer32, etc. This would require the device to
> > explicitly set what it is expecting. It is challenging since the only
> > model's not using 8-bit transfer don't have any devices connected
> > upstream so testing is challenging.
>
> This doesn't allow for bit widths that aren't a multiple of 8.
>
> thanks
> -- PMM