Hi Luc,
All the suggestions and corrections look good to me so brought them in in
v7!
Thank you very much reviewing!
Best regards,
Francisco Iglesias
On [2022 Jan 18] Tue 22:46:32, Luc Michel wrote:
> 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.igles...@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
