Hi Francisco,
On 11/26/2017 08:16 PM, Francisco Iglesias wrote:
> Make tx/rx_data_bytes more generic so they can be reused (when adding
> support for the Zynqmp Generic QSPI).
>
> Signed-off-by: Francisco Iglesias <frasse.igles...@gmail.com>
> Reviewed-by: Edgar E. Iglesias <edgar.igles...@xilinx.com>
> Tested-by: Edgar E. Iglesias <edgar.igles...@xilinx.com>
> ---
> hw/ssi/xilinx_spips.c | 64
> +++++++++++++++++++++++++++++----------------------
> 1 file changed, 37 insertions(+), 27 deletions(-)
>
> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> index 691d48d..4621dbb 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -47,7 +47,7 @@
> /* config register */
> #define R_CONFIG (0x00 / 4)
> #define IFMODE (1U << 31)
> -#define ENDIAN (1 << 26)
> +#define R_CONFIG_ENDIAN (1 << 26)
> #define MODEFAIL_GEN_EN (1 << 17)
> #define MAN_START_COM (1 << 16)
> #define MAN_START_EN (1 << 15)
> @@ -450,13 +450,28 @@ static void xilinx_spips_flush_txfifo(XilinxSPIPS *s)
> }
> }
>
> -static inline void rx_data_bytes(XilinxSPIPS *s, uint8_t *value, int max)
> +static inline void tx_data_bytes(Fifo8 *fifo, uint32_t value, int num, bool
> be)
> {
> int i;
> + for (i = 0; i < num && !fifo8_is_full(fifo); ++i) {
> + if (be) {
> + fifo8_push(fifo, (uint8_t)(value >> 24));
> + value <<= 8;
> + } else {
> + fifo8_push(fifo, (uint8_t)value);
> + value >>= 8;
> + }
> + }
> +}
>
> - for (i = 0; i < max && !fifo8_is_empty(&s->rx_fifo); ++i) {
> - value[i] = fifo8_pop(&s->rx_fifo);
> +static inline int rx_data_bytes(Fifo8 *fifo, uint8_t *value, int max)
> +{
> + int i;
> +
> + for (i = 0; i < max && !fifo8_is_empty(fifo); ++i) {
> + value[i] = fifo8_pop(fifo);
> }
> + return max - i;
> }
>
> static uint64_t xilinx_spips_read(void *opaque, hwaddr addr,
> @@ -466,6 +481,7 @@ static uint64_t xilinx_spips_read(void *opaque, hwaddr
> addr,
> uint32_t mask = ~0;
> uint32_t ret;
> uint8_t rx_buf[4];
> + int shortfall;
>
> addr >>= 2;
> switch (addr) {
> @@ -496,9 +512,13 @@ static uint64_t xilinx_spips_read(void *opaque, hwaddr
> addr,
> break;
> case R_RX_DATA:
> memset(rx_buf, 0, sizeof(rx_buf));
> - rx_data_bytes(s, rx_buf, s->num_txrx_bytes);
> - ret = s->regs[R_CONFIG] & ENDIAN ? cpu_to_be32(*(uint32_t *)rx_buf)
> - : cpu_to_le32(*(uint32_t *)rx_buf);
> + shortfall = rx_data_bytes(&s->rx_fifo, rx_buf, s->num_txrx_bytes);
About this part,
> + ret = s->regs[R_CONFIG] & R_CONFIG_ENDIAN ?
> + cpu_to_be32(*(uint32_t *)rx_buf) :
> + cpu_to_le32(*(uint32_t *)rx_buf);
> + if (!(s->regs[R_CONFIG] & R_CONFIG_ENDIAN)) {
> + ret <<= 8 * shortfall;
> + }
The following looks easier to read to me, probably matter of taste, but
you don't have to wonder the cpu <-> device direction and it remove the
casts:
if (s->regs[R_CONFIG] & R_CONFIG_ENDIAN) {
ret = ldl_be_p(rx_buf);
} else {
ret = ldl_le_p(rx_buf) << (8 * shortfall);
}
What do you think? (before respining)
> DB_PRINT_L(0, "addr=" TARGET_FMT_plx " = %x\n", addr * 4, ret);
> xilinx_spips_update_ixr(s);
> return ret;
> @@ -509,20 +529,6 @@ static uint64_t xilinx_spips_read(void *opaque, hwaddr
> addr,
>
> }
>
> -static inline void tx_data_bytes(XilinxSPIPS *s, uint32_t value, int num)
> -{
> - int i;
> - for (i = 0; i < num && !fifo8_is_full(&s->tx_fifo); ++i) {
> - if (s->regs[R_CONFIG] & ENDIAN) {
> - fifo8_push(&s->tx_fifo, (uint8_t)(value >> 24));
> - value <<= 8;
> - } else {
> - fifo8_push(&s->tx_fifo, (uint8_t)value);
> - value >>= 8;
> - }
> - }
> -}
> -
> static void xilinx_spips_write(void *opaque, hwaddr addr,
> uint64_t value, unsigned size)
> {
> @@ -563,16 +569,20 @@ static void xilinx_spips_write(void *opaque, hwaddr
> addr,
> mask = 0;
> break;
> case R_TX_DATA:
> - tx_data_bytes(s, (uint32_t)value, s->num_txrx_bytes);
> + tx_data_bytes(&s->tx_fifo, (uint32_t)value, s->num_txrx_bytes,
> + s->regs[R_CONFIG] & R_CONFIG_ENDIAN);
> goto no_reg_update;
> case R_TXD1:
> - tx_data_bytes(s, (uint32_t)value, 1);
> + tx_data_bytes(&s->tx_fifo, (uint32_t)value, 1,
> + s->regs[R_CONFIG] & R_CONFIG_ENDIAN);
> goto no_reg_update;
> case R_TXD2:
> - tx_data_bytes(s, (uint32_t)value, 2);
> + tx_data_bytes(&s->tx_fifo, (uint32_t)value, 2,
> + s->regs[R_CONFIG] & R_CONFIG_ENDIAN);
> goto no_reg_update;
> case R_TXD3:
> - tx_data_bytes(s, (uint32_t)value, 3);
> + tx_data_bytes(&s->tx_fifo, (uint32_t)value, 3,
> + s->regs[R_CONFIG] & R_CONFIG_ENDIAN);
> goto no_reg_update;
> }
> s->regs[addr] = (s->regs[addr] & ~mask) | (value & mask);
> @@ -682,11 +692,11 @@ static void lqspi_load_cache(void *opaque, hwaddr addr)
>
> while (cache_entry < LQSPI_CACHE_SIZE) {
> for (i = 0; i < 64; ++i) {
> - tx_data_bytes(s, 0, 1);
> + tx_data_bytes(&s->tx_fifo, 0, 1, false);
> }
> xilinx_spips_flush_txfifo(s);
> for (i = 0; i < 64; ++i) {
> - rx_data_bytes(s, &q->lqspi_buf[cache_entry++], 1);
> + rx_data_bytes(&s->rx_fifo, &q->lqspi_buf[cache_entry++], 1);
> }
> }
>
>
