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

[Sai Pavan Boddu]

Hi Edgar,

> -----Original Message-----
> From: Edgar E. Iglesias <address@hidden>
> Sent: Monday, May 4, 2020 8:32 PM
> To: Sai Pavan Boddu <address@hidden>
> Cc: Alistair Francis <address@hidden>; Peter Maydell
> <address@hidden>; Jason Wang <address@hidden>; Markus
> Armbruster <address@hidden>; Philippe Mathieu-Daudé
> <address@hidden>; Tong Ho <address@hidden>; Ramon Fried
> <address@hidden>; address@hidden; qemu-
> address@hidden
> Subject: Re: [PATCH v2 05/10] net: cadence_gem: Set ISR according to queue
> in use
> 
> 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...
[Sai Pavan Boddu] Yeah, it would be nice. Will try to include this in v3

Thanks,
Sai Pavan
> 
> 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
> >