[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 06/11] hw/ssi: Add a model of Xilinx Versal's OSPI flash m
From: |
Peter Maydell |
Subject: |
Re: [PATCH v4 06/11] hw/ssi: Add a model of Xilinx Versal's OSPI flash memory controller |
Date: |
Fri, 10 Dec 2021 16:02:22 +0000 |
On Wed, 1 Dec 2021 at 15:40, Francisco Iglesias
<francisco.iglesias@xilinx.com> wrote:
>
> Add a model of Xilinx Versal's OSPI flash memory controller.
>
> Signed-off-by: Francisco Iglesias <francisco.iglesias@xilinx.com>
> +#define SZ_512MBIT (512 * 1024 * 1024)
> +#define SZ_1GBIT (1024 * 1024 * 1024)
> +#define SZ_2GBIT (2ULL * SZ_1GBIT)
> +#define SZ_4GBIT (4ULL * SZ_1GBIT)
> +
> +#define IS_IND_DMA_START(op) (op->done_bytes == 0)
> +/*
> + * Bit field size of R_INDIRECT_WRITE_XFER_CTRL_REG_NUM_IND_OPS_DONE_FLD
> + * is 2 bits, which can record max of 3 indac operations.
> + */
> +#define IND_OPS_DONE_MAX 3
> +
> +typedef enum {
> + WREN = 0x6,
> +} FlashCMD;
> +
> +/* Type to avoid cpu endian byte swaps */
> +typedef union {
> + uint64_t u64;
> + uint8_t u8[8];
> +} OSPIRdData;
Don't hand-roll this kind of thing, please. (I'll suggest
code below in the places where you use this union.)
> +static unsigned int single_cs(XlnxVersalOspi *s)
> +{
> + unsigned int field = ARRAY_FIELD_EX32(s->regs,
> + CONFIG_REG, PERIPH_CS_LINES_FLD);
> + int i;
> +
> + /*
> + * 4'bXXX0 -> 4'b1110
> + * 4'bXX0X -> 4'b1101
> + * 4'bX0XX -> 4'b1011
This chart is ambiguous, because 0b1100 matches both the
first two lines, for instance. The code implements
0bXXX0 -> 0b1110
0bXX01 -> 0b1101
0bX011 -> 0b1011
etc
> + * 4'b0XXX -> 4'b0111
> + * 4'b1111 -> 4'b1111
> + */
> + for (i = 0; i < 4; i++) {
> + if ((field & (1 << i)) == 0) {
> + return (~(1 << i)) & 0xF;
> + }
> + }
> + return 0;
The comment says that if the input is 0b1111 then the
output is also 0b1111, but unless I'm misreading the code we
will return 0 in that case. Which is correct ?
Assuming you want 0b1111 input to give 0b1111 output,
you can do this without the loop, like this:
return (field | ~(field + 1)) & 0xf;
(which uses a variant on the "isolate the rightmost 0-bit"
trick from here:
https://emre.me/computer-science/bit-manipulation-tricks/ )
If you do it that way do add a comment, because it's a bit
obscure :-)
> +static void ospi_rx_fifo_pop_stig_rd_data(XlnxVersalOspi *s)
> +{
> + int size = ospi_stig_rd_data_len(s);
> + OSPIRdData res = {};
> + int i;
> +
> + size = MIN(fifo8_num_used(&s->rx_fifo), size);
I think I would assert(size <= 8) here; given the size of
the data field that ospi_stig_rd_data_len() reads from it
must be true, but it's not immediately obvious that this
function won't overrun the array so the assert() helps readers.
> + for (i = 0; i < size; i++) {
> + res.u8[i] = fifo8_pop(&s->rx_fifo);
> + }
> +
> + s->regs[R_FLASH_RD_DATA_LOWER_REG] = res.u64 & 0xFFFFFFFF;
> + s->regs[R_FLASH_RD_DATA_UPPER_REG] = (res.u64 >> 32) & 0xFFFFFFFF;
This will give different values for these registers depending
on whether the host is big-endian or little-endian, because if
the bytes come out of the FIFO in the order 00 11 22 33 44 55 66 77
then on a BE host res.u64 is 0x0011223344556677 and so
LOWER_REG is 0x44556677, whereas on a LE host res.u64 is
0x7766554433221100 and LOWER_REG is 0x33221100.
Instead of the union:
uint8_t bytes[8] = {};
// pop into the bytes array
s->regs[LOWER_REG] = ldl_le_p(bytes);
s->regs[UPPER_REG] = ldl_le_p(bytes + 4);
I assume here that the desired behaviour is that if the
bytes come out of the FIFO in the order 00 11 22 33 44 55 66 77 then
LOWER_REG reads 0x33221100 and UPPER_REG 0x77665544.
> + /* Write out tx_fifo in maximum page sz chunks */
> + while (!ospi_ind_op_completed(op) && fifo8_num_used(&s->tx_sram) > 0) {
> + next_b = ind_op_next_byte(op);
> + end_b = next_b + MIN(fifo8_num_used(&s->tx_sram), pagesz);
> +
> + /* Dont cross page boundery */
"boundary"
> +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 */
> + uint32_t lower = 0;
> + uint32_t upper = 0;
> + int i;
> +
> + for (i = 0; i < num_rd_bytes; i++) {
> + s->stig_membank[i] = fifo8_pop(&s->rx_fifo);
> + }
> +
> + /* Fill in lower upper regs */
> + for (i = 0; i < 4; i++) {
> + lower |= ((uint32_t)s->stig_membank[idx++]) << 8 * i;
> + }
> +
> + for (i = 0; i < 4; i++) {
> + upper |= ((uint32_t)s->stig_membank[idx++]) << 8 * i;
> + }
You can replace these loops with
lower = ldl_le_p(s->stig_membank[idx]);
upper = ldl_le_p(s->stig_membank[idx + 4]);
idx += 8;
> + s->regs[R_FLASH_RD_DATA_LOWER_REG] = lower;
> + s->regs[R_FLASH_RD_DATA_UPPER_REG] = upper;
> +}
> +
> +static uint64_t ospi_rx_sram_read(XlnxVersalOspi *s, unsigned int size)
> +{
> + OSPIRdData ret = {};
> + int i;
> +
> + if (size < 4 && fifo8_num_used(&s->rx_sram) >= 4) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "OSPI only last read of internal "
> + "sram is allowed to be < 32 bits\n");
> + }
> +
> + size = MIN(fifo8_num_used(&s->rx_sram), size);
> + for (i = 0; i < size; i++) {
> + ret.u8[i] = fifo8_pop(&s->rx_sram);
> + }
> + return ret.u64;
Similarly to ospi_rx_fifo_pop_stig_rd_data(), you want to read
into a local uint8_t bytes[8] (and assert about size), but
here because we want a uint64_t rather than two uint32_t we can
return ldq_le_p(bytes);
> +}
> +
> +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);
> + }
> +}
> +
> +static uint64_t ospi_do_dac_read(void *opaque, hwaddr addr, unsigned int
> size)
> +{
> + XlnxVersalOspi *s = XILINX_VERSAL_OSPI(opaque);
> + OSPIRdData ret = {};
> + int i;
> +
> + /* Create first section of read cmd */
> + ospi_tx_fifo_push_rd_op_addr(s, (uint32_t) addr);
> +
> + /* Enable cs and transmit first part */
> + ospi_dac_cs(s, addr);
> + ospi_flush_txfifo(s);
> +
> + fifo8_reset(&s->rx_fifo);
> +
> + /* transmit second part (data) */
> + for (i = 0; i < size; ++i) {
> + fifo8_push(&s->tx_fifo, 0);
> + }
> + ospi_flush_txfifo(s);
> +
> + /* fill in result */
> + size = MIN(fifo8_num_used(&s->rx_fifo), size);
> +
> + for (i = 0; i < size; i++) {
> + ret.u8[i] = fifo8_pop(&s->rx_fifo);
> + }
> +
> + /* done */
> + ospi_disable_cs(s);
> +
> + return ret.u64;
Same as ospi_rx_sram_read().
> +}
> +/* Return dev-obj from reg-region created by register_init_block32 */
> +static XlnxVersalOspi *xilinx_ospi_of_mr(void *mr_accessor)
> +{
> + RegisterInfoArray *reg_array = mr_accessor;
> + Object *dev;
> +
> + assert(reg_array != NULL);
You don't really need to assert() this kind of thing -- if it is
NULL then the code is going to crash immediately on the following
line anyway. assert()s are most useful for turning "weird misbehaviour"
and "something goes wrong a long way away from the point where we
could have detected it" bugs into "crash where it's clear what
the problem is" bugs.
> +
> + dev = reg_array->mem.owner;
> + assert(dev);
> +
> + return XILINX_VERSAL_OSPI(dev);
> +}
This is another device that would benefit from a "QEMU interface"
comment describing the properties/gpios/etc.
> +#ifndef XILINX_VERSAL_OSPI_H
> +#define XILINX_VERSAL_OSPI_H
> +
> +#include "hw/register.h"
> +#include "hw/ssi/ssi.h"
> +#include "qemu/fifo32.h"
> +#include "hw/dma/dma-ctrl-if.h"
> +
> +#define TYPE_XILINX_VERSAL_OSPI "xlnx.versal-ospi"
> +
> +#define XILINX_VERSAL_OSPI(obj) \
> + OBJECT_CHECK(XlnxVersalOspi, (obj), TYPE_XILINX_VERSAL_OSPI)
Same comment about macros as the other device.
> +
> +#define XILINX_VERSAL_OSPI_R_MAX (0xfc / 4 + 1)
thanks
-- PMM
- Re: [PATCH v4 02/11] hw/arm/xlnx-versal: Connect Versal's PMC SLCR, (continued)
- [PATCH v4 03/11] include/hw/dma/xlnx_csu_dma: Add in missing includes in the header, Francisco Iglesias, 2021/12/01
- [PATCH v4 01/11] hw/misc: Add a model of Versal's PMC SLCR, Francisco Iglesias, 2021/12/01
- [PATCH v4 05/11] hw/dma/xlnx_csu_dma: Implement the DMA control interface, Francisco Iglesias, 2021/12/01
- [PATCH v4 04/11] hw/dma: Add the DMA control interface, Francisco Iglesias, 2021/12/01
- [PATCH v4 06/11] hw/ssi: Add a model of Xilinx Versal's OSPI flash memory controller, Francisco Iglesias, 2021/12/01
- Re: [PATCH v4 06/11] hw/ssi: Add a model of Xilinx Versal's OSPI flash memory controller,
Peter Maydell <=
- [PATCH v4 08/11] hw/block/m25p80: Add support for Micron Xccela flash mt35xu01g, Francisco Iglesias, 2021/12/01
- [PATCH v4 09/11] hw/arm/xlnx-versal-virt: Connect mt35xu01g flashes to the OSPI, Francisco Iglesias, 2021/12/01
- [PATCH v4 07/11] hw/arm/xlnx-versal: Connect the OSPI flash memory controller model, Francisco Iglesias, 2021/12/01
- [PATCH v4 11/11] docs/devel: Add documentation for the DMA control interface, Francisco Iglesias, 2021/12/01
- [PATCH v4 10/11] MAINTAINERS: Add an entry for Xilinx Versal OSPI, Francisco Iglesias, 2021/12/01