Re: [PATCH v6 07/12] hw/ssi: Add a model of Xilinx Versal's OSPI flash memory controller

[Luc Michel <luc@lmichel.fr>]

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 <francisco.iglesias@xilinx.com>
> + *
> + * 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