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

[Edgar E. Iglesias]

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?
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
>