| Message ID | 5da9ba36d28dc15715bb886d314ea598df352eb5.1364962908.git.peter.crosthwaite@xilinx.com |
|---|---|
| State | New |
| Headers | show |
On 3 April 2013 05:33, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > The QSPI controller was using byte-wide stripes when striping across > the two flashes in dual parallel mode. The real hardware however uses > individual bit striping. QEMU misbehaves in the (corner) case where > data is written/read in dual-parallel mode and read/written back in > single mode. > > Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > --- > > hw/xilinx_spips.c | 74 ++++++++++++++++++++++++++++++++++++++--------------- > 1 files changed, 53 insertions(+), 21 deletions(-) > > diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c > index a2019e4..43ce2d8 100644 > --- a/hw/xilinx_spips.c > +++ b/hw/xilinx_spips.c > @@ -258,35 +258,67 @@ static void xilinx_spips_reset(DeviceState *d) > xilinx_spips_update_cs_lines(s); > } > > +static inline void stripe8(uint8_t *x, int num, bool dir) > +{ > + uint8_t r[num]; > + memset(r, 0, sizeof(uint8_t) * num); Don't interleave code and variable definitions, please. > + int idx[2] = {0, 0}; > + int bit[2] = {0, 0}; > + int d = dir; > + > + for (idx[0] = 0; idx[0] < num; ++idx[0]) { > + for (bit[0] = 0; bit[0] < 8; ++bit[0]) { > + r[idx[d]] |= x[idx[!d]] & 1 << bit[!d] ? 1 << bit[d] : 0; > + idx[1] = (idx[1] + 1) % num; > + if (!idx[1]) { > + bit[1]++; > + } > + } > + } > + memcpy(x, r, sizeof(uint8_t) * num); > +} This function could really use a comment saying what it's doing... thanks -- PMM
Hi Peter, On Sat, Apr 6, 2013 at 4:59 AM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 3 April 2013 05:33, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: >> The QSPI controller was using byte-wide stripes when striping across >> the two flashes in dual parallel mode. The real hardware however uses >> individual bit striping. QEMU misbehaves in the (corner) case where >> data is written/read in dual-parallel mode and read/written back in >> single mode. >> >> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> >> --- >> >> hw/xilinx_spips.c | 74 ++++++++++++++++++++++++++++++++++++++--------------- >> 1 files changed, 53 insertions(+), 21 deletions(-) >> >> diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c >> index a2019e4..43ce2d8 100644 >> --- a/hw/xilinx_spips.c >> +++ b/hw/xilinx_spips.c >> @@ -258,35 +258,67 @@ static void xilinx_spips_reset(DeviceState *d) >> xilinx_spips_update_cs_lines(s); >> } >> >> +static inline void stripe8(uint8_t *x, int num, bool dir) >> +{ >> + uint8_t r[num]; >> + memset(r, 0, sizeof(uint8_t) * num); > > Don't interleave code and variable definitions, please. > >> + int idx[2] = {0, 0}; >> + int bit[2] = {0, 0}; >> + int d = dir; >> + >> + for (idx[0] = 0; idx[0] < num; ++idx[0]) { >> + for (bit[0] = 0; bit[0] < 8; ++bit[0]) { >> + r[idx[d]] |= x[idx[!d]] & 1 << bit[!d] ? 1 << bit[d] : 0; >> + idx[1] = (idx[1] + 1) % num; >> + if (!idx[1]) { >> + bit[1]++; >> + } >> + } >> + } >> + memcpy(x, r, sizeof(uint8_t) * num); >> +} > > This function could really use a comment saying what it's doing... > +/* N way (num) in place bit striper. Lay out row wise bits (LSB to MSB) + * column wise (from element 0 to N-1). num is the length of x, and dir + * reverses the direction of the transform. Best illustrated by example: + * Each digit in the below array is a single bit (num == 3): + * + * {{ 76543210, } ----- stripe (dir == false) -----> {{ FCheb630, } + * { hgfedcba, } { GDAfc741, } + * { HGFEDCBA, }} <---- upstripe (dir == true) ----- { HEBgda52, }} + */ + Regards, Peter > > thanks > -- PMM >
diff --git a/hw/xilinx_spips.c b/hw/xilinx_spips.c index a2019e4..43ce2d8 100644 --- a/hw/xilinx_spips.c +++ b/hw/xilinx_spips.c @@ -258,35 +258,67 @@ static void xilinx_spips_reset(DeviceState *d) xilinx_spips_update_cs_lines(s); } +static inline void stripe8(uint8_t *x, int num, bool dir) +{ + uint8_t r[num]; + memset(r, 0, sizeof(uint8_t) * num); + int idx[2] = {0, 0}; + int bit[2] = {0, 0}; + int d = dir; + + for (idx[0] = 0; idx[0] < num; ++idx[0]) { + for (bit[0] = 0; bit[0] < 8; ++bit[0]) { + r[idx[d]] |= x[idx[!d]] & 1 << bit[!d] ? 1 << bit[d] : 0; + idx[1] = (idx[1] + 1) % num; + if (!idx[1]) { + bit[1]++; + } + } + } + memcpy(x, r, sizeof(uint8_t) * num); +} + static void xilinx_spips_flush_txfifo(XilinxSPIPS *s) { for (;;) { int i; - uint8_t rx; uint8_t tx = 0; + uint8_t tx_rx[num_effective_busses(s)]; - for (i = 0; i < num_effective_busses(s); ++i) { - if (!i || s->snoop_state == SNOOP_STRIPING) { - if (fifo8_is_empty(&s->tx_fifo)) { - if (!(s->regs[R_LQSPI_CFG] & LQSPI_CFG_LQ_MODE)) { - s->regs[R_INTR_STATUS] |= IXR_TX_FIFO_UNDERFLOW; - } - xilinx_spips_update_ixr(s); - return; - } else { - tx = fifo8_pop(&s->tx_fifo); - } + if (fifo8_is_empty(&s->tx_fifo)) { + if (!(s->regs[R_LQSPI_CFG] & LQSPI_CFG_LQ_MODE)) { + s->regs[R_INTR_STATUS] |= IXR_TX_FIFO_UNDERFLOW; + } + xilinx_spips_update_ixr(s); + return; + } else if (s->snoop_state == SNOOP_STRIPING) { + for (i = 0; i < num_effective_busses(s); ++i) { + tx_rx[i] = fifo8_pop(&s->tx_fifo); } - rx = ssi_transfer(s->spi[i], (uint32_t)tx); - DB_PRINT("tx = %02x rx = %02x\n", tx, rx); - if (!i || s->snoop_state == SNOOP_STRIPING) { - if (fifo8_is_full(&s->rx_fifo)) { - s->regs[R_INTR_STATUS] |= IXR_RX_FIFO_OVERFLOW; - DB_PRINT("rx FIFO overflow"); - } else { - fifo8_push(&s->rx_fifo, (uint8_t)rx); - } + stripe8(tx_rx, num_effective_busses(s), false); + } else { + tx = fifo8_pop(&s->tx_fifo); + for (i = 0; i < num_effective_busses(s); ++i) { + tx_rx[i] = tx; + } + } + + for (i = 0; i < num_effective_busses(s); ++i) { + DB_PRINT("tx = %02x\n", tx_rx[i]); + tx_rx[i] = ssi_transfer(s->spi[i], (uint32_t)tx_rx[i]); + DB_PRINT("rx = %02x\n", tx_rx[i]); + } + + if (fifo8_is_full(&s->rx_fifo)) { + s->regs[R_INTR_STATUS] |= IXR_RX_FIFO_OVERFLOW; + DB_PRINT("rx FIFO overflow"); + } else if (s->snoop_state == SNOOP_STRIPING) { + stripe8(tx_rx, num_effective_busses(s), true); + for (i = 0; i < num_effective_busses(s); ++i) { + fifo8_push(&s->rx_fifo, (uint8_t)tx_rx[i]); } + } else { + fifo8_push(&s->rx_fifo, (uint8_t)tx_rx[0]); } switch (s->snoop_state) {
The QSPI controller was using byte-wide stripes when striping across the two flashes in dual parallel mode. The real hardware however uses individual bit striping. QEMU misbehaves in the (corner) case where data is written/read in dual-parallel mode and read/written back in single mode. Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> --- hw/xilinx_spips.c | 74 ++++++++++++++++++++++++++++++++++++++--------------- 1 files changed, 53 insertions(+), 21 deletions(-)