Re: [PATCH v2 02/10] net: cadence_gem: Fix the queue address update during wrap around

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Mon, May 04, 2020 at 07:36:00PM +0530, Sai Pavan Boddu wrote:
> During wrap around and reset, queues are pointing to initial base
> address of queue 0, irrespective of what queue we are dealing with.
> Fix it by assigning proper base address every time.

Might want to add wrappers e.g:
static inline uint32_t gem_get_rx_queue_base_addr(CadenceGEMState *s, int q) {
    gem_get_queue_base_addr(s, false, q); 
}

static inline uint32_t gem_get_tx_queue_base_addr(CadenceGEMState *s, int q) {
    gem_get_queue_base_addr(s, true, q); 
}

It makes the packet processing logic a little easier to read, e.g:

s->rx_desc_addr[q] = gem_get_rx_queue_base_addr(s, q);
vs
s->rx_desc_addr[q] = gem_get_queue_base_addr(s, false, q);

Anyway, this looks good to me:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> 
> Signed-off-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>
> ---
>  hw/net/cadence_gem.c | 29 +++++++++++++++++++++++++----
>  1 file changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index 2f244eb..6cb2f64 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -845,6 +845,25 @@ static int get_queue_from_screen(CadenceGEMState *s, uint8_t *rxbuf_ptr,
>      return 0;
>  }
>  
> +static uint32_t gem_get_queue_base_addr(CadenceGEMState *s, bool tx, int q)
> +{
> +    uint32_t base_addr = 0;
> +
> +    switch (q) {
> +    case 0:
> +        base_addr = s->regs[tx ? GEM_TXQBASE : GEM_RXQBASE];
> +        break;
> +    case 1 ... (MAX_PRIORITY_QUEUES - 1):
> +        base_addr = s->regs[(tx ? GEM_TRANSMIT_Q1_PTR :
> +                                 GEM_RECEIVE_Q1_PTR) + q - 1];
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    };
> +
> +    return base_addr;
> +}
> +
>  static hwaddr gem_get_desc_addr(CadenceGEMState *s, bool tx, int q)
>  {
>      hwaddr desc_addr = 0;
> @@ -1044,7 +1063,7 @@ static ssize_t gem_receive(NetClientState *nc, const uint8_t *buf, size_t size)
>          /* Next descriptor */
>          if (rx_desc_get_wrap(s->rx_desc[q])) {
>              DB_PRINT("wrapping RX descriptor list\n");
> -            s->rx_desc_addr[q] = s->regs[GEM_RXQBASE];
> +            s->rx_desc_addr[q] = gem_get_queue_base_addr(s, false, q);
>          } else {
>              DB_PRINT("incrementing RX descriptor list\n");
>              s->rx_desc_addr[q] += 4 * gem_get_desc_len(s, true);
> @@ -1200,7 +1219,8 @@ static void gem_transmit(CadenceGEMState *s)
>                                      sizeof(desc_first));
>                  /* Advance the hardware current descriptor past this packet */
>                  if (tx_desc_get_wrap(desc)) {
> -                    s->tx_desc_addr[q] = s->regs[GEM_TXQBASE];
> +                    s->tx_desc_addr[q] = gem_get_queue_base_addr(s,
> +                                         true, q);
>                  } else {
>                      s->tx_desc_addr[q] = packet_desc_addr +
>                                           4 * gem_get_desc_len(s, false);
> @@ -1252,7 +1272,8 @@ static void gem_transmit(CadenceGEMState *s)
>                  } else {
>                      packet_desc_addr = 0;
>                  }
> -                packet_desc_addr |= s->regs[GEM_TXQBASE];
> +                packet_desc_addr |= gem_get_queue_base_addr(s,
> +                                    true, q);
>              } else {
>                  packet_desc_addr += 4 * gem_get_desc_len(s, false);
>              }
> @@ -1458,7 +1479,7 @@ static void gem_write(void *opaque, hwaddr offset, uint64_t val,
>          if (!(val & GEM_NWCTRL_TXENA)) {
>              /* Reset to start of Q when transmit disabled. */
>              for (i = 0; i < s->num_priority_queues; i++) {
> -                s->tx_desc_addr[i] = s->regs[GEM_TXQBASE];
> +                s->tx_desc_addr[i] = gem_get_queue_base_addr(s, true, i);
>              }
>          }
>          if (gem_can_receive(qemu_get_queue(s->nic))) {
> -- 
> 2.7.4
>