Re: [Qemu-devel] [PATCH v9 08/13] xilinx_spips: Make tx/rx_data_bytes more generic and reusable

[Philippe Mathieu-Daudé]

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 <address@hidden>
> Reviewed-by: Edgar E. Iglesias <address@hidden>
> Tested-by: Edgar E. Iglesias <address@hidden>
> ---
>  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);
>              }
>          }
>  
>