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

[Francisco Iglesias]

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.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