Hi Francisco,
Impressive beast :-) Nicely done. Maybe I would have split it in a
couple of commits to ease review. Also, you can use
[diff]
orderFile = scripts/git.orderfile
as a local config in your QEMU git so that files are placed in a
sensible order (.h files will come first), which ease a bit the
reviewing process.
See my remarks below. My biggest concern is about the tx_sram fifo.
The rest are small suggestions here and there.
On 15:28 Fri 14 Jan , Francisco Iglesias wrote:
[snip]
> +
> +static int ospi_stig_membank_rd_bytes(XlnxVersalOspi *s)
> +{
> + int rd_data_fld = ARRAY_FIELD_EX32(s->regs, FLASH_COMMAND_CTRL_MEM_REG,
> + NB_OF_STIG_READ_BYTES_FLD);
> + int sizes[6] = { 16, 32, 64, 128, 256, 512 };
static const int sizes[6]
(or return (rd_data_fld < 6) ? (1 << (4 + rd_data_fld)) : 0; )
> + return (rd_data_fld < 6) ? sizes[rd_data_fld] : 0;
> +}
> +
[snip]
> +
> +static void ospi_ahb_decoder_enable_cs(XlnxVersalOspi *s, hwaddr addr)
> +{
> + int cs = ospi_ahb_decoder_cs(s, addr);
> +
> + if (cs >= 0) {
> + for (int i = 0; i < s->num_cs; i++) {
> + if (cs == i) {
> + qemu_set_irq(s->cs_lines[i], 0);
> + } else {
> + qemu_set_irq(s->cs_lines[i], 1);
> + }
Maybe `qemu_set_irq(s->cs_lines[i], cs != i);` instead of the if/else?
> + }
> + }
> +}
> +
[snip]
> +
> +static void ospi_stig_fill_membank(XlnxVersalOspi *s)
> +{
> + int num_rd_bytes = ospi_stig_membank_rd_bytes(s);
> + int idx = num_rd_bytes - 8; /* first of last 8 */
> + int i;
> +
> + for (i = 0; i < num_rd_bytes; i++) {
> + s->stig_membank[i] = fifo8_pop(&s->rx_fifo);
> + }
> +
Even though ospi_stig_membank_rd_bytes is safe, I would add a
g_assert((idx + 4) < ARRAY_SIZE(s->stig_membank));
here, to be future proof :-)
> + /* Fill in lower upper regs */
> + s->regs[R_FLASH_RD_DATA_LOWER_REG] = ldl_le_p(&s->stig_membank[idx]);
> + s->regs[R_FLASH_RD_DATA_UPPER_REG] = ldl_le_p(&s->stig_membank[idx + 4]);
> +}
> +
[snip]
> +
> +static void ospi_tx_sram_write(XlnxVersalOspi *s, uint64_t value,
> + unsigned int size)
> +{
> + int i;
> + for (i = 0; i < size; i++) {
> + fifo8_push(&s->tx_sram, value >> 8 * i);
By tracing the callers of this function, it seems that `size' is the
size of an MMIO access. But you don't seem to check if the tx_sram fifo
can accept `size' elements (the fifo8_push doc stats it is undefined
behaviour to push on a full fifo).
> + }
> +}
> +
> +
> +static void ospi_indac_write(void *opaque, uint64_t value, unsigned int size)
> +{
> + XlnxVersalOspi *s = XILINX_VERSAL_OSPI(opaque);
> +
> + if (s->ind_write_disabled) {
> + g_assert_not_reached();
> + }
g_assert(!s->ind_write_disabled);
> +
> + if (!ospi_ind_op_completed(s->wr_ind_op)) {
> + ospi_tx_sram_write(s, value, size);
> + ospi_do_indirect_write(s);
> + } else {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "OSPI wr into indac area while no ongoing indac wr\n");
> + }
> +}
> +
[snip]
> diff --git a/include/hw/ssi/xlnx-versal-ospi.h
> b/include/hw/ssi/xlnx-versal-ospi.h
> new file mode 100644
> index 0000000000..c454ff3016
> --- /dev/null
> +++ b/include/hw/ssi/xlnx-versal-ospi.h
> @@ -0,0 +1,111 @@
> +/*
> + * Header file for the Xilinx Versal's OSPI controller
> + *
> + * Copyright (C) 2021 Xilinx Inc
> + * Written by Francisco Iglesias <[email protected]>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> copy
> + * of this software and associated documentation files (the "Software"), to
> deal
> + * in the Software without restriction, including without limitation the
> rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +/*
> + * This is a model of Xilinx Versal's Octal SPI flash memory controller
> + * documented in Versal's Technical Reference manual [1] and the Versal ACAP
> + * Register reference [2].
> + *
> + * References:
> + *
> + * [1] Versal ACAP Technical Reference Manual,
> + *
> https://www.xilinx.com/support/documentation/architecture-manuals/am011-versal-acap-trm.pdf
> + *
> + * [2] Versal ACAP Register Reference,
> + *
> https://www.xilinx.com/html_docs/registers/am012/am012-versal-register-reference.html#mod___ospi.html
> + *
> + *
> + * QEMU interface:
> + * + sysbus MMIO region 0: MemoryRegion for the device's registers
> + * + sysbus MMIO region 1: MemoryRegion for flash memory linear address space
> + * (data transfer).
> + * + sysbus IRQ 0: Device interrupt.
> + * + Named GPIO input "ospi-mux-sel": 0: enables indirect access mode
> + * and 1: enables direct access mode.
> + * + Property "dac-with-indac": Allow both direct accesses and indirect
> + * accesses simultaneously.
> + * + Property "indac-write-disabled": Disable indirect access writes.
> + */
> +
> +#ifndef XILINX_VERSAL_OSPI_H
> +#define XILINX_VERSAL_OSPI_H
> +
> +#include "hw/register.h"
> +#include "hw/ssi/ssi.h"
> +#include "qemu/fifo32.h"
fifo8.h ?
Thanks.
--
Luc
