RE: [PATCH v2 04/10] net: cadence_gem: Define access permission for interrupt registers

[Sai Pavan Boddu]

Hi Edgar,

> -----Original Message-----
> From: Edgar E. Iglesias <address@hidden>
> Sent: Monday, May 4, 2020 8:27 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 04/10] net: cadence_gem: Define access permission
> for interrupt registers
> 
> On Mon, May 04, 2020 at 07:36:02PM +0530, Sai Pavan Boddu wrote:
> > Q1 to Q7 ISR's are clear-on-read, IER/IDR registers are write-only,
> > mask reg are read-only.
> >
> > Signed-off-by: Sai Pavan Boddu <address@hidden>
> > ---
> >  hw/net/cadence_gem.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c index
> > a930bf1..c532a14 100644
> > --- a/hw/net/cadence_gem.c
> > +++ b/hw/net/cadence_gem.c
> > @@ -458,6 +458,7 @@ static const uint8_t broadcast_addr[] = { 0xFF, 0xFF,
> 0xFF, 0xFF, 0xFF, 0xFF };
> >   */
> >  static void gem_init_register_masks(CadenceGEMState *s)  {
> > +    unsigned int i;
> >      /* Mask of register bits which are read only */
> >      memset(&s->regs_ro[0], 0, sizeof(s->regs_ro));
> >      s->regs_ro[GEM_NWCTRL]   = 0xFFF80000;
> > @@ -470,10 +471,19 @@ static void
> gem_init_register_masks(CadenceGEMState *s)
> >      s->regs_ro[GEM_ISR]      = 0xFFFFFFFF;
> >      s->regs_ro[GEM_IMR]      = 0xFFFFFFFF;
> >      s->regs_ro[GEM_MODID]    = 0xFFFFFFFF;
> > +    for (i = 0; i < s->num_priority_queues; i++) {
> > +        s->regs_ro[GEM_INT_Q1_STATUS + i] = 0xFFFFFFFF;
> > +        s->regs_ro[GEM_INT_Q1_ENABLE + i] = 0xFFFFE319;
> > +        s->regs_ro[GEM_INT_Q1_DISABLE + i] = 0xFFFFE319;
> 
> Shouldn't these be 0xfffff319?
[Sai Pavan Boddu] This one is right. I would fix it thanks.

Regards,
Sai Pavan
> Perhaps I'm looking at old specs but mine says bits upper bits [31:12] are
> reserved and read-only.
> 
> 
> With that fixed:
> Reviewed-by: Edgar E. Iglesias <address@hidden>
> 
> 
> 
> 
> 
> > +        s->regs_ro[GEM_INT_Q1_MASK + i] = 0xFFFFFFFF;
> 
> > +    }
> >
> >      /* Mask of register bits which are clear on read */
> >      memset(&s->regs_rtc[0], 0, sizeof(s->regs_rtc));
> >      s->regs_rtc[GEM_ISR]      = 0xFFFFFFFF;
> > +    for (i = 0; i < s->num_priority_queues; i++) {
> > +        s->regs_rtc[GEM_INT_Q1_STATUS + i] = 0x00000CE6;
> > +    }
> >
> >      /* Mask of register bits which are write 1 to clear */
> >      memset(&s->regs_w1c[0], 0, sizeof(s->regs_w1c)); @@ -485,6
> > +495,10 @@ static void gem_init_register_masks(CadenceGEMState *s)
> >      s->regs_wo[GEM_NWCTRL]   = 0x00073E60;
> >      s->regs_wo[GEM_IER]      = 0x07FFFFFF;
> >      s->regs_wo[GEM_IDR]      = 0x07FFFFFF;
> > +    for (i = 0; i < s->num_priority_queues; i++) {
> > +        s->regs_wo[GEM_INT_Q1_ENABLE + i] = 0x00000CE6;
> > +        s->regs_wo[GEM_INT_Q1_DISABLE + i] = 0x00000CE6;
> > +    }
> >  }
> >
> >  /*
> > --
> > 2.7.4
> >