Hi Peter, Thank you very much for the review and detailed comments.
I will try to address all comments in the v2 of the patches, but I have some questions I added below. On Mon, Sep 30, 2024 at 4:45 PM Peter Maydell <peter.mayd...@linaro.org> wrote: > On Thu, 19 Sept 2024 at 22:55, Strahinja Jankovic > <strahinjapjanko...@gmail.com> wrote: > > > > This patch implements Allwinner A10 SPI controller emulation. > > Only master-mode functionality is implemented. > > > > Since U-Boot and Linux SPI drivers for Allwinner A10 perform only > > byte-wide CPU access (no DMA) to the peripheral, the emulated > > controller does not implement DMA control and supports only byte-wide > > access. > > > > Docs are also updated for Cubieboard to indicate SPI availability. > > > > Signed-off-by: Strahinja Jankovic <strahinja.p.janko...@gmail.com> > > Hi; thanks for this patch. It generally looks good to me; > my comments below are not major. > > > docs/system/arm/cubieboard.rst | 1 + > > hw/arm/Kconfig | 1 + > > hw/arm/allwinner-a10.c | 8 + > > hw/ssi/Kconfig | 4 + > > hw/ssi/allwinner-a10-spi.c | 534 +++++++++++++++++++++++++++++ > > hw/ssi/meson.build | 1 + > > hw/ssi/trace-events | 10 + > > include/hw/arm/allwinner-a10.h | 2 + > > include/hw/ssi/allwinner-a10-spi.h | 56 +++ > > For "add new device to existing board" changes we generally > prefer them to be two commits: > * add the new device model > * wire up the device in the board model > > Could you split this patch in two, please? > Yes, I will split this into two patches. > > > diff --git a/hw/ssi/allwinner-a10-spi.c b/hw/ssi/allwinner-a10-spi.c > > new file mode 100644 > > index 0000000000..ea44ae46a3 > > --- /dev/null > > +++ b/hw/ssi/allwinner-a10-spi.c > > @@ -0,0 +1,534 @@ > > +/* > > + * Allwinner SPI Bus Serial Interface Emulation > > + * > > + * Copyright (C) 2024 Strahinja Jankovic < > strahinja.p.janko...@gmail.com> > > + * > > + * This program is free software; you can redistribute it and/or > modify it > > + * under the terms of the GNU General Public License as published by > the > > + * Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + * > > + * This program is distributed in the hope that it will be useful, but > WITHOUT > > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY > or > > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License > > + * for more details. > > + * > > + * You should have received a copy of the GNU General Public License > along > > + * with this program; if not, see <http://www.gnu.org/licenses/>. > > + * > > + * SPDX-License-Identifier: MIT > > This SPDX tag does not match the human-readable license. We're OK with > either GPL-2.0-or-later or MIT, but which one do you want? > > Also, the .h file license-and-copyright comment has no SPDX tag; > we don't currently insist on SPDX tags, but you might wish to > be consistent. > Good points, I will update this. > > > +/* Control register */ > > +#define SPI_CTL_SDC (1 << 19) > > +#define SPI_CTL_TP_EN (1 << 18) > > +#define SPI_CTL_SS_LEVEL (1 << 17) > > +#define SPI_CTL_SS_CTRL (1 << 16) > > +#define SPI_CTL_DHB (1 << 15) > > +#define SPI_CTL_DDB (1 << 14) > > +#define SPI_CTL_SS (3 << 12) > > +#define SPI_CTL_SS_SHIFT (12) > > +#define SPI_CTL_RPSM (1 << 11) > > +#define SPI_CTL_XCH (1 << 10) > > +#define SPI_CTL_RF_RST (1 << 9) > > +#define SPI_CTL_TF_RST (1 << 8) > > +#define SPI_CTL_SSCTL (1 << 7) > > +#define SPI_CTL_LMTF (1 << 6) > > +#define SPI_CTL_DMAMC (1 << 5) > > +#define SPI_CTL_SSPOL (1 << 4) > > +#define SPI_CTL_POL (1 << 3) > > +#define SPI_CTL_PHA (1 << 2) > > +#define SPI_CTL_MODE (1 << 1) > > +#define SPI_CTL_EN (1 << 0) > > +#define SPI_CTL_MASK (0xFFFFFu) > > +#define SPI_CTL_RESET (0x0002001Cu) > > Brackets aren't necessary around single integer values, > only around expressions. > > > > +static void allwinner_a10_spi_update_irq(AWA10SPIState *s) > > +{ > > + int level; > > + > > + if (fifo8_is_empty(&s->rx_fifo)) { > > + s->regs[REG_INDEX(SPI_INT_STA_REG)] &= ~SPI_INT_STA_RR; > > + } else { > > + s->regs[REG_INDEX(SPI_INT_STA_REG)] |= SPI_INT_STA_RR; > > + } > > + > > + if (fifo8_num_used(&s->rx_fifo) >= (AW_A10_SPI_FIFO_SIZE >> 2)) { > > + s->regs[REG_INDEX(SPI_INT_STA_REG)] |= SPI_INT_STA_RF14; > > + } else { > > + s->regs[REG_INDEX(SPI_INT_STA_REG)] &= ~SPI_INT_STA_RF14; > > + } > > + > > + if (fifo8_num_used(&s->rx_fifo) >= (AW_A10_SPI_FIFO_SIZE >> 1)) { > > + s->regs[REG_INDEX(SPI_INT_STA_REG)] |= SPI_INT_STA_RHF; > > + } else { > > + s->regs[REG_INDEX(SPI_INT_STA_REG)] &= ~SPI_INT_STA_RHF; > > + } > > + > > + if (fifo8_num_free(&s->rx_fifo) <= (AW_A10_SPI_FIFO_SIZE >> 2)) { > > + s->regs[REG_INDEX(SPI_INT_STA_REG)] |= SPI_INT_STA_RF34; > > + } else { > > + s->regs[REG_INDEX(SPI_INT_STA_REG)] &= ~SPI_INT_STA_RF34; > > + } > > + > > + if (fifo8_is_full(&s->rx_fifo)) { > > + s->regs[REG_INDEX(SPI_INT_STA_REG)] |= SPI_INT_STA_RF; > > + } else { > > + s->regs[REG_INDEX(SPI_INT_STA_REG)] &= ~SPI_INT_STA_RF; > > + } > > + > > + if (fifo8_is_empty(&s->tx_fifo)) { > > + s->regs[REG_INDEX(SPI_INT_STA_REG)] |= SPI_INT_STA_TE; > > + } else { > > + s->regs[REG_INDEX(SPI_INT_STA_REG)] &= ~SPI_INT_STA_TE; > > + } > > + > > + if (fifo8_num_free(&s->tx_fifo) >= (AW_A10_SPI_FIFO_SIZE >> 2)) { > > + s->regs[REG_INDEX(SPI_INT_STA_REG)] |= SPI_INT_STA_TE14; > > + } else { > > + s->regs[REG_INDEX(SPI_INT_STA_REG)] &= ~SPI_INT_STA_TE14; > > + } > > + > > + if (fifo8_num_free(&s->tx_fifo) >= (AW_A10_SPI_FIFO_SIZE >> 1)) { > > + s->regs[REG_INDEX(SPI_INT_STA_REG)] |= SPI_INT_STA_THE; > > + } else { > > + s->regs[REG_INDEX(SPI_INT_STA_REG)] &= ~SPI_INT_STA_THE; > > + } > > + > > + if (fifo8_num_used(&s->tx_fifo) <= (AW_A10_SPI_FIFO_SIZE >> 2)) { > > + s->regs[REG_INDEX(SPI_INT_STA_REG)] |= SPI_INT_STA_TE34; > > + } else { > > + s->regs[REG_INDEX(SPI_INT_STA_REG)] &= ~SPI_INT_STA_TE34; > > + } > > + > > + if (fifo8_is_full(&s->rx_fifo)) { > > + s->regs[REG_INDEX(SPI_INT_STA_REG)] |= SPI_INT_STA_TF; > > + } else { > > + s->regs[REG_INDEX(SPI_INT_STA_REG)] &= ~SPI_INT_STA_TF; > > + } > > + > > + level = s->regs[REG_INDEX(SPI_INT_STA_REG)] & > > + s->regs[REG_INDEX(SPI_INTCTL_REG)] ? > > + 1 : > > + 0; > > If you declare 'level' as a 'bool' you can avoid this > awkward "? 1 : 0" expression. > > > + > > + qemu_set_irq(s->irq, level); > > + > > + trace_allwinner_a10_spi_update_irq(level); > > +} > > + > > +static void allwinner_a10_spi_flush_txfifo(AWA10SPIState *s) > > +{ > > + uint8_t tx; > > + uint8_t rx; > > + uint32_t burst_count = s->regs[REG_INDEX(SPI_BC_REG)]; > > + uint32_t tx_burst = s->regs[REG_INDEX(SPI_TC_REG)]; > > + trace_allwinner_a10_spi_burst_length(tx_burst); > > + > > + > trace_allwinner_a10_spi_flush_txfifo_begin(fifo8_num_used(&s->tx_fifo), > > + > fifo8_num_used(&s->rx_fifo)); > > + > > + while (!fifo8_is_empty(&s->tx_fifo)) { > > + tx = fifo8_pop(&s->tx_fifo); > > + rx = 0; > > + bool fill_rx = true; > > Variable declarations should always go at the start of a block, > not in the middle of one. (Since tx and rx are not used > outside the loop, you could fix this by declaring them > here rather than at the top of the function if you like.) > > > + > > + trace_allwinner_a10_spi_tx(tx); > > + > > + /* Write one byte at a time */ > > + rx = ssi_transfer(s->bus, tx); > > + > > + trace_allwinner_a10_spi_rx(rx); > > + > > + /* Check DHB here to determing if RX bytes should be stored */ > > Typo: "determine" > > > + if (s->regs[REG_INDEX(SPI_CTL_REG)] & SPI_CTL_DHB) { > > + /* Store rx bytes only after WTC transfers */ > > + if (tx_burst > 0u) { > > + fill_rx = false; > > + tx_burst--; > > + } > > + } > > + > > + if (fill_rx) { > > + if (fifo8_is_full(&s->rx_fifo)) { > > + s->regs[REG_INDEX(SPI_INT_STA_REG)] |= SPI_INT_STA_RF; > > + } else { > > + fifo8_push(&s->rx_fifo, rx); > > + } > > + } > > + > > + allwinner_a10_spi_update_irq(s); > > + > > + burst_count--; > > + > > + if (burst_count == 0) { > > + s->regs[REG_INDEX(SPI_INT_STA_REG)] |= SPI_INT_STA_TC; > > + s->regs[REG_INDEX(SPI_CTL_REG)] &= ~SPI_CTL_XCH; > > + break; > > + } > > + } > > + > > + if (fifo8_is_empty(&s->tx_fifo)) { > > + s->regs[REG_INDEX(SPI_INT_STA_REG)] |= SPI_INT_STA_TC; > > + s->regs[REG_INDEX(SPI_CTL_REG)] &= ~SPI_CTL_XCH; > > + } > > + > > + > trace_allwinner_a10_spi_flush_txfifo_end(fifo8_num_used(&s->tx_fifo), > > + > fifo8_num_used(&s->rx_fifo)); > > +} > > + > > +static uint64_t allwinner_a10_spi_read(void *opaque, hwaddr offset, > > + unsigned size) > > +{ > > + uint32_t value = 0; > > + AWA10SPIState *s = opaque; > > + uint32_t index = offset >> 2; > > The MemoryRegionOps defines that size == 1 is permitted, > but this calculation of index without any validation > of offset means that if the guest writes a byte to > offset 1 we will treat that identically to writing a byte > to offset 0. This probably isn't what the hardware does. > I checked once more the User manual, and it does not mention unaligned access (example for RXDATA) In 8-bits SPI bus width, this register can be accessed in byte, half-word or word unit by AHB. In byte accessing method, if there are words in RXFIFO, the top word is returned and the RXFIFO depth is decreased by 1. In half-word accessing method, the two SPI bursts are returned and the RXFIFO depth is decrease by 2. In word accessing method, the four SPI bursts are returned and the RXFIFO depth is decreased by 4. I chose not to cover the half-word and word access methods, since neither Linux kernel nor U-Boot use it (both use only byte access). I guess that I could add `.valid.unaligned = false` when initializing `MemoryRegionOps`? Best regards, Strahinja > > > + > > + if (offset > SPI_FIFO_STA_REG) { > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "[%s]%s: Bad register at offset 0x%" HWADDR_PRIx > "\n", > > + TYPE_AW_A10_SPI, __func__, offset); > > + return 0; > > + } > > + > > + value = s->regs[index]; > > + > > + if (allwinner_a10_spi_is_enabled(s)) { > > + switch (offset) { > > + case SPI_RXDATA_REG: > > + if (fifo8_is_empty(&s->rx_fifo)) { > > + /* value is undefined */ > > + value = 0xdeadbeef; > > + } else { > > + /* read from the RX FIFO */ > > + value = fifo8_pop(&s->rx_fifo); > > + } > > + break; > > + case SPI_TXDATA_REG: > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "[%s]%s: Trying to read from TX FIFO\n", > > + TYPE_AW_A10_SPI, __func__); > > + > > + /* Reading from TXDATA gives 0 */ > > + break; > > + case SPI_FIFO_STA_REG: > > + /* Read current tx/rx fifo data count */ > > + value = fifo8_num_used(&s->tx_fifo) << > SPI_FIFO_STA_TF_CNT_SHIFT | > > + fifo8_num_used(&s->rx_fifo) << > SPI_FIFO_STA_RF_CNT_SHIFT; > > + default: > > + break; > > + } > > + > > + allwinner_a10_spi_update_irq(s); > > + } > > + trace_allwinner_a10_spi_read(allwinner_a10_spi_get_regname(offset), > value); > > + > > + return (uint64_t)value; > > The cast here is unnecessary as the function return type is uint64_t. > > > +} > > + > > +static void allwinner_a10_spi_write(void *opaque, hwaddr offset, > uint64_t value, > > + unsigned size) > > +{ > > + AWA10SPIState *s = opaque; > > + uint32_t index = offset >> 2; > > Similar remarks here about byte and halfword writes to > non-4-aligned offsets. > > > + int i = 0; > > + > > + if (offset > SPI_FIFO_STA_REG) { > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "[%s]%s: Bad register at offset 0x%" HWADDR_PRIx > "\n", > > + TYPE_AW_A10_SPI, __func__, offset); > > + return; > > + } > > + > > + trace_allwinner_a10_spi_write(allwinner_a10_spi_get_regname(offset), > > + (uint32_t)value); > > + > > + if (!allwinner_a10_spi_is_enabled(s)) { > > + /* Block is disabled */ > > + if (offset != SPI_CTL_REG) { > > + /* Ignore access */ > > + return; > > + } > > + } > > + > > + switch (offset) { > > + case SPI_RXDATA_REG: > > + qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Trying to write to RX > FIFO\n", > > + TYPE_AW_A10_SPI, __func__); > > + break; > > + case SPI_TXDATA_REG: > > + if (fifo8_is_full(&s->tx_fifo)) { > > + /* Ignore writes if queue is full */ > > + break; > > + } > > + > > + fifo8_push(&s->tx_fifo, (uint8_t)value); > > + > > + break; > > + case SPI_INT_STA_REG: > > + /* Handle W1C bits - everything except SPI_INT_STA_INT_CBF. */ > > + value &= ~SPI_INT_STA_INT_CBF; > > + s->regs[REG_INDEX(SPI_INT_STA_REG)] &= ~(value & > SPI_INT_STA_MASK); > > + break; > > + case SPI_CTL_REG: > > + s->regs[REG_INDEX(SPI_CTL_REG)] = value; > > + > > + for (i = 0; i < AW_A10_SPI_CS_LINES_NR; i++) { > > + qemu_set_irq( > > + s->cs_lines[i], > > + i == allwinner_a10_spi_selected_channel(s) ? > > + !!(s->regs[REG_INDEX(SPI_CTL_REG)] & > SPI_CTL_SS_LEVEL) : > > + !(s->regs[REG_INDEX(SPI_CTL_REG)] & SPI_CTL_SSPOL)); > > This might be easier to read if we factored out the ?: > expression into a function with a descriptive name. > > > + } > > + > > + if (s->regs[REG_INDEX(SPI_CTL_REG)] & SPI_CTL_XCH) { > > + /* Request to start emitting */ > > + allwinner_a10_spi_flush_txfifo(s); > > + } > > + if (s->regs[REG_INDEX(SPI_CTL_REG)] & SPI_CTL_TF_RST) { > > + allwinner_a10_spi_txfifo_reset(s); > > + s->regs[REG_INDEX(SPI_CTL_REG)] &= ~SPI_CTL_TF_RST; > > + } > > + if (s->regs[REG_INDEX(SPI_CTL_REG)] & SPI_CTL_RF_RST) { > > + allwinner_a10_spi_rxfifo_reset(s); > > + s->regs[REG_INDEX(SPI_CTL_REG)] &= ~SPI_CTL_RF_RST; > > + } > > + break; > > + default: > > + s->regs[index] = value; > > + break; > > + } > > + > > + allwinner_a10_spi_update_irq(s); > > +} > > + > > +static const MemoryRegionOps allwinner_a10_spi_ops = { > > + .read = allwinner_a10_spi_read, > > + .write = allwinner_a10_spi_write, > > + .valid.min_access_size = 1, > > + .valid.max_access_size = 4, > > + .endianness = DEVICE_NATIVE_ENDIAN, > > +}; > > + > > +static const VMStateDescription allwinner_a10_spi_vmstate = { > > + .name = TYPE_AW_A10_SPI, > > + .version_id = 1, > > + .minimum_version_id = 1, > > + .fields = > > + (const VMStateField[]){ > > + VMSTATE_FIFO8(tx_fifo, AWA10SPIState), > > + VMSTATE_FIFO8(rx_fifo, AWA10SPIState), > > + VMSTATE_UINT32_ARRAY(regs, AWA10SPIState, > AW_A10_SPI_REGS_NUM), > > + VMSTATE_END_OF_LIST() } > > Our usual indent for this is > .fields = (const VMStateField[]) { > VMSTATE_FOO(...), > ... > VMSTATE_END_OF_LIST() > } > +}; > > + > > > --- a/hw/ssi/trace-events > > +++ b/hw/ssi/trace-events > > @@ -53,3 +53,13 @@ pnv_spi_rx_read_N2frame(void) "" > > pnv_spi_shift_rx(uint8_t byte, uint32_t index) "byte = 0x%2.2x into RDR > from payload index %d" > > pnv_spi_sequencer_stop_requested(const char* reason) "due to %s" > > pnv_spi_RDR_match(const char* result) "%s" > > + > > +# allwinner_a10_spi.c > > +allwinner_a10_spi_update_irq(uint32_t level) "IRQ level is %d" > > +allwinner_a10_spi_flush_txfifo_begin(uint32_t tx, uint32_t rx) "Begin: > TX Fifo Size = %d, RX Fifo Size = %d" > > +allwinner_a10_spi_flush_txfifo_end(uint32_t tx, uint32_t rx) "End: TX > Fifo Size = %d, RX Fifo Size = %d" > > +allwinner_a10_spi_burst_length(uint32_t len) "Burst length = %d" > > +allwinner_a10_spi_tx(uint8_t byte) "write 0x%02x" > > +allwinner_a10_spi_rx(uint8_t byte) "read 0x%02x" > > +allwinner_a10_spi_read(const char* regname, uint32_t value) "reg[%s] => > 0x%" PRIx32 > > +allwinner_a10_spi_write(const char* regname, uint32_t value) "reg[%s] > <= 0x%" PRIx32 > > Be consistent about whether you want to use PRI* macros > for uint32_t types or not, please. (Admittedly QEMU itself is > not super consistent here either, but I think the more common > option is to use %d, %x etc for uint32_t, because we can assume > that 'int' is a 32-bit type. It's only uint64_t that requires the > PRI macros.) > > thanks > -- PMM >
