Re: [PATCH v2 05/10] net: cadence_gem: Set ISR according to queue in use

[Edgar E. Iglesias]

On Mon, May 04, 2020 at 07:36:03PM +0530, Sai Pavan Boddu wrote:
> Set ISR according to queue in use, added interrupt support for
> all queues.

Would it help to add a gem_set_isr(CadenceGEMState *s, int q, uint32_t flag) ?
Instead of open coding these if (q == 0) else... all over the place...

Anyway, the logic looks good to me:
Reviewed-by: Edgar E. Iglesias <address@hidden>



> 
> Signed-off-by: Sai Pavan Boddu <address@hidden>
> ---
>  hw/net/cadence_gem.c | 31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c
> index c532a14..beb38ec 100644
> --- a/hw/net/cadence_gem.c
> +++ b/hw/net/cadence_gem.c
> @@ -896,7 +896,13 @@ static void gem_get_rx_desc(CadenceGEMState *s, int q)
>      if (rx_desc_get_ownership(s->rx_desc[q]) == 1) {
>          DB_PRINT("descriptor 0x%" HWADDR_PRIx " owned by sw.\n", desc_addr);
>          s->regs[GEM_RXSTATUS] |= GEM_RXSTATUS_NOBUF;
> -        s->regs[GEM_ISR] |= GEM_INT_RXUSED & ~(s->regs[GEM_IMR]);
> +        if (q == 0) {
> +            s->regs[GEM_ISR] |= GEM_INT_RXUSED & ~(s->regs[GEM_IMR]);
> +        } else {
> +            s->regs[GEM_INT_Q1_STATUS + q - 1] |= GEM_INT_RXUSED &
> +                                          ~(s->regs[GEM_INT_Q1_MASK + q - 
> 1]);
> +        }
> +
>          /* Handle interrupt consequences */
>          gem_update_int_status(s);
>      }
> @@ -1071,8 +1077,12 @@ static ssize_t gem_receive(NetClientState *nc, const 
> uint8_t *buf, size_t size)
>      gem_receive_updatestats(s, buf, size);
>  
>      s->regs[GEM_RXSTATUS] |= GEM_RXSTATUS_FRMRCVD;
> -    s->regs[GEM_ISR] |= GEM_INT_RXCMPL & ~(s->regs[GEM_IMR]);
> -
> +    if (q == 0) {
> +        s->regs[GEM_ISR] |= GEM_INT_RXCMPL & ~(s->regs[GEM_IMR]);
> +    } else {
> +        s->regs[GEM_INT_Q1_STATUS + q - 1] |= GEM_INT_RXCMPL &
> +                                      ~(s->regs[GEM_INT_Q1_MASK + q - 1]);
> +    }
>      /* Handle interrupt consequences */
>      gem_update_int_status(s);
>  
> @@ -1223,12 +1233,12 @@ static void gem_transmit(CadenceGEMState *s)
>                  DB_PRINT("TX descriptor next: 0x%08x\n", s->tx_desc_addr[q]);
>  
>                  s->regs[GEM_TXSTATUS] |= GEM_TXSTATUS_TXCMPL;
> -                s->regs[GEM_ISR] |= GEM_INT_TXCMPL & ~(s->regs[GEM_IMR]);
> -
> +                if (q == 0) {
> +                    s->regs[GEM_ISR] |= GEM_INT_TXCMPL & ~(s->regs[GEM_IMR]);
> +                } else {
>                  /* Update queue interrupt status */
> -                if (s->num_priority_queues > 1) {
> -                    s->regs[GEM_INT_Q1_STATUS + q] |=
> -                            GEM_INT_TXCMPL & ~(s->regs[GEM_INT_Q1_MASK + q]);
> +                    s->regs[GEM_INT_Q1_STATUS + q - 1] |=
> +                            GEM_INT_TXCMPL & ~s->regs[GEM_INT_Q1_MASK + q - 
> 1];
>                  }
>  
>                  /* Handle interrupt consequences */
> @@ -1280,7 +1290,10 @@ static void gem_transmit(CadenceGEMState *s)
>  
>          if (tx_desc_get_used(desc)) {
>              s->regs[GEM_TXSTATUS] |= GEM_TXSTATUS_USED;
> -            s->regs[GEM_ISR] |= GEM_INT_TXUSED & ~(s->regs[GEM_IMR]);
> +            /* IRQ TXUSED is defined only for queue 0 */
> +            if (q == 0) {
> +                s->regs[GEM_ISR] |= GEM_INT_TXUSED & ~(s->regs[GEM_IMR]);
> +            }
>              gem_update_int_status(s);
>          }
>      }
> -- 
> 2.7.4
>