diff mbox

[v2,1/1] arm_gic: Update ID registers based on revision

Message ID 118acc7c92ab1eac93ad2dcb25944569f4c6ae59.1453328768.git.alistair.francis@xilinx.com (mailing list archive)
State New, archived
Headers show

Commit Message

Alistair Francis Jan. 20, 2016, 10:42 p.m. UTC
Update the GIC ID registers (registers above 0xfe0) based on the GIC
revision instead of using the sames values for all GIC implementations.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Tested-by: Sören Brinkmann <soren.brinkmann@xilinx.com>
---
V2:
 - Update array indexing to match new values
 - Check the maximum value of offset as well

 hw/intc/arm_gic.c | 35 ++++++++++++++++++++++++++++++-----
 1 file changed, 30 insertions(+), 5 deletions(-)

Comments

Peter Maydell Jan. 20, 2016, 10:48 p.m. UTC | #1
On 20 January 2016 at 22:42, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> Update the GIC ID registers (registers above 0xfe0) based on the GIC
> revision instead of using the sames values for all GIC implementations.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Tested-by: Sören Brinkmann <soren.brinkmann@xilinx.com>
> ---
> V2:
>  - Update array indexing to match new values
>  - Check the maximum value of offset as well
>
>  hw/intc/arm_gic.c | 35 ++++++++++++++++++++++++++++++-----
>  1 file changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
> index 13e297d..8eb3a2d 100644
> --- a/hw/intc/arm_gic.c
> +++ b/hw/intc/arm_gic.c
> @@ -31,8 +31,16 @@ do { fprintf(stderr, "arm_gic: " fmt , ## __VA_ARGS__); } while (0)
>  #define DPRINTF(fmt, ...) do {} while(0)
>  #endif
>
> -static const uint8_t gic_id[] = {
> -    0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1
> +static const uint8_t gic_id_11mpcore[] = {
> +    0x00, 0x00, 0x00, 0x00, 0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1
> +};
> +
> +static const uint8_t gic_id_gicv1[] = {
> +    0x04, 0x00, 0x00, 0x00, 0x90, 0xb3, 0x1b, 0x00, 0x0d, 0xf0, 0x05, 0xb1
> +};
> +
> +static const uint8_t gic_id_gicv2[] = {
> +    0x04, 0x00, 0x00, 0x00, 0x90, 0xb4, 0x2b, 0x00, 0x0d, 0xf0, 0x05, 0xb1
>  };
>
>  static inline int gic_get_current_cpu(GICState *s)
> @@ -683,13 +691,30 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
>          }
>
>          res = s->sgi_pending[irq][cpu];
> -    } else if (offset < 0xfe0) {
> +    } else if (offset < 0xfd0) {
>          goto bad_reg;
> -    } else /* offset >= 0xfe0 */ {
> +    } else /* offset >= 0xfd0 */ {
>          if (offset & 3) {
>              res = 0;
> +        } else if (offset > 0xfdf) {
> +            goto bad_reg;

Where did this condition come from? It will mean all
the ID regs fe0 and above end up treated as bad registers
rather than taking the code below to return the ID value.

>          } else {
> -            res = gic_id[(offset - 0xfe0) >> 2];
> +            switch (s->revision) {
> +            case REV_11MPCORE:
> +                res = gic_id_11mpcore[(offset - 0xfd0) >> 2];
> +                break;
> +            case 1:
> +                res = gic_id_gicv1[(offset - 0xfd0) >> 2];
> +                break;
> +            case 2:
> +                res = gic_id_gicv2[(offset - 0xfd0) >> 2];
> +                break;
> +            case REV_NVIC:
> +                /* Shouldn't be able to get here */
> +                abort();
> +            default:
> +                res = 0;
> +            }
>          }
>      }
>      return res;
> --
> 2.5.0
>

thanks
-- PMM
Alistair Francis Jan. 20, 2016, 11:08 p.m. UTC | #2
On Wed, Jan 20, 2016 at 2:48 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 20 January 2016 at 22:42, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> Update the GIC ID registers (registers above 0xfe0) based on the GIC
>> revision instead of using the sames values for all GIC implementations.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> Tested-by: Sören Brinkmann <soren.brinkmann@xilinx.com>
>> ---
>> V2:
>>  - Update array indexing to match new values
>>  - Check the maximum value of offset as well
>>
>>  hw/intc/arm_gic.c | 35 ++++++++++++++++++++++++++++++-----
>>  1 file changed, 30 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>> index 13e297d..8eb3a2d 100644
>> --- a/hw/intc/arm_gic.c
>> +++ b/hw/intc/arm_gic.c
>> @@ -31,8 +31,16 @@ do { fprintf(stderr, "arm_gic: " fmt , ## __VA_ARGS__); } while (0)
>>  #define DPRINTF(fmt, ...) do {} while(0)
>>  #endif
>>
>> -static const uint8_t gic_id[] = {
>> -    0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1
>> +static const uint8_t gic_id_11mpcore[] = {
>> +    0x00, 0x00, 0x00, 0x00, 0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1
>> +};
>> +
>> +static const uint8_t gic_id_gicv1[] = {
>> +    0x04, 0x00, 0x00, 0x00, 0x90, 0xb3, 0x1b, 0x00, 0x0d, 0xf0, 0x05, 0xb1
>> +};
>> +
>> +static const uint8_t gic_id_gicv2[] = {
>> +    0x04, 0x00, 0x00, 0x00, 0x90, 0xb4, 0x2b, 0x00, 0x0d, 0xf0, 0x05, 0xb1
>>  };
>>
>>  static inline int gic_get_current_cpu(GICState *s)
>> @@ -683,13 +691,30 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
>>          }
>>
>>          res = s->sgi_pending[irq][cpu];
>> -    } else if (offset < 0xfe0) {
>> +    } else if (offset < 0xfd0) {
>>          goto bad_reg;
>> -    } else /* offset >= 0xfe0 */ {
>> +    } else /* offset >= 0xfd0 */ {
>>          if (offset & 3) {
>>              res = 0;
>> +        } else if (offset > 0xfdf) {
>> +            goto bad_reg;
>
> Where did this condition come from? It will mean all
> the ID regs fe0 and above end up treated as bad registers
> rather than taking the code below to return the ID value.

Woops, I copied the wrong value. I mean that to be 0xffc

Do you still want a maximum check (otherwise it will walk off the array)?

Thanks,

Alistair

>
>>          } else {
>> -            res = gic_id[(offset - 0xfe0) >> 2];
>> +            switch (s->revision) {
>> +            case REV_11MPCORE:
>> +                res = gic_id_11mpcore[(offset - 0xfd0) >> 2];
>> +                break;
>> +            case 1:
>> +                res = gic_id_gicv1[(offset - 0xfd0) >> 2];
>> +                break;
>> +            case 2:
>> +                res = gic_id_gicv2[(offset - 0xfd0) >> 2];
>> +                break;
>> +            case REV_NVIC:
>> +                /* Shouldn't be able to get here */
>> +                abort();
>> +            default:
>> +                res = 0;
>> +            }
>>          }
>>      }
>>      return res;
>> --
>> 2.5.0
>>
>
> thanks
> -- PMM
>
Peter Maydell Jan. 20, 2016, 11:37 p.m. UTC | #3
On 20 January 2016 at 23:08, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> On Wed, Jan 20, 2016 at 2:48 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 20 January 2016 at 22:42, Alistair Francis
>> <alistair.francis@xilinx.com> wrote:
>>> Update the GIC ID registers (registers above 0xfe0) based on the GIC
>>> revision instead of using the sames values for all GIC implementations.
>>>
>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>> Tested-by: Sören Brinkmann <soren.brinkmann@xilinx.com>
>>> ---
>>> V2:
>>>  - Update array indexing to match new values
>>>  - Check the maximum value of offset as well
>>>
>>>  hw/intc/arm_gic.c | 35 ++++++++++++++++++++++++++++++-----
>>>  1 file changed, 30 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>>> index 13e297d..8eb3a2d 100644
>>> --- a/hw/intc/arm_gic.c
>>> +++ b/hw/intc/arm_gic.c
>>> @@ -31,8 +31,16 @@ do { fprintf(stderr, "arm_gic: " fmt , ## __VA_ARGS__); } while (0)
>>>  #define DPRINTF(fmt, ...) do {} while(0)
>>>  #endif
>>>
>>> -static const uint8_t gic_id[] = {
>>> -    0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1
>>> +static const uint8_t gic_id_11mpcore[] = {
>>> +    0x00, 0x00, 0x00, 0x00, 0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1
>>> +};
>>> +
>>> +static const uint8_t gic_id_gicv1[] = {
>>> +    0x04, 0x00, 0x00, 0x00, 0x90, 0xb3, 0x1b, 0x00, 0x0d, 0xf0, 0x05, 0xb1
>>> +};
>>> +
>>> +static const uint8_t gic_id_gicv2[] = {
>>> +    0x04, 0x00, 0x00, 0x00, 0x90, 0xb4, 0x2b, 0x00, 0x0d, 0xf0, 0x05, 0xb1
>>>  };
>>>
>>>  static inline int gic_get_current_cpu(GICState *s)
>>> @@ -683,13 +691,30 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
>>>          }
>>>
>>>          res = s->sgi_pending[irq][cpu];
>>> -    } else if (offset < 0xfe0) {
>>> +    } else if (offset < 0xfd0) {
>>>          goto bad_reg;
>>> -    } else /* offset >= 0xfe0 */ {
>>> +    } else /* offset >= 0xfd0 */ {
>>>          if (offset & 3) {
>>>              res = 0;
>>> +        } else if (offset > 0xfdf) {
>>> +            goto bad_reg;
>>
>> Where did this condition come from? It will mean all
>> the ID regs fe0 and above end up treated as bad registers
>> rather than taking the code below to return the ID value.
>
> Woops, I copied the wrong value. I mean that to be 0xffc
>
> Do you still want a maximum check (otherwise it will walk off the array)?

FFD..FFF should read as zeroes (like the other non-multiple-of-4
offset addresses in this region), and you can't get more than 0xfff
because the size of the memory region is 0x1000. That's not
entirely obvious though, so if you want a max check you can
put one in. I think that having it match the style of the rest
of the if..else if ladder would be better though:

  ...
 else if (offset < 0x1000) {
     array code here;
 else {
     g_assert_not_reached();
 }

or something.

thanks
-- PMM
Alistair Francis Jan. 20, 2016, 11:49 p.m. UTC | #4
On Wed, Jan 20, 2016 at 3:37 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 20 January 2016 at 23:08, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> On Wed, Jan 20, 2016 at 2:48 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> On 20 January 2016 at 22:42, Alistair Francis
>>> <alistair.francis@xilinx.com> wrote:
>>>> Update the GIC ID registers (registers above 0xfe0) based on the GIC
>>>> revision instead of using the sames values for all GIC implementations.
>>>>
>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>> Tested-by: Sören Brinkmann <soren.brinkmann@xilinx.com>
>>>> ---
>>>> V2:
>>>>  - Update array indexing to match new values
>>>>  - Check the maximum value of offset as well
>>>>
>>>>  hw/intc/arm_gic.c | 35 ++++++++++++++++++++++++++++++-----
>>>>  1 file changed, 30 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
>>>> index 13e297d..8eb3a2d 100644
>>>> --- a/hw/intc/arm_gic.c
>>>> +++ b/hw/intc/arm_gic.c
>>>> @@ -31,8 +31,16 @@ do { fprintf(stderr, "arm_gic: " fmt , ## __VA_ARGS__); } while (0)
>>>>  #define DPRINTF(fmt, ...) do {} while(0)
>>>>  #endif
>>>>
>>>> -static const uint8_t gic_id[] = {
>>>> -    0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1
>>>> +static const uint8_t gic_id_11mpcore[] = {
>>>> +    0x00, 0x00, 0x00, 0x00, 0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1
>>>> +};
>>>> +
>>>> +static const uint8_t gic_id_gicv1[] = {
>>>> +    0x04, 0x00, 0x00, 0x00, 0x90, 0xb3, 0x1b, 0x00, 0x0d, 0xf0, 0x05, 0xb1
>>>> +};
>>>> +
>>>> +static const uint8_t gic_id_gicv2[] = {
>>>> +    0x04, 0x00, 0x00, 0x00, 0x90, 0xb4, 0x2b, 0x00, 0x0d, 0xf0, 0x05, 0xb1
>>>>  };
>>>>
>>>>  static inline int gic_get_current_cpu(GICState *s)
>>>> @@ -683,13 +691,30 @@ static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
>>>>          }
>>>>
>>>>          res = s->sgi_pending[irq][cpu];
>>>> -    } else if (offset < 0xfe0) {
>>>> +    } else if (offset < 0xfd0) {
>>>>          goto bad_reg;
>>>> -    } else /* offset >= 0xfe0 */ {
>>>> +    } else /* offset >= 0xfd0 */ {
>>>>          if (offset & 3) {
>>>>              res = 0;
>>>> +        } else if (offset > 0xfdf) {
>>>> +            goto bad_reg;
>>>
>>> Where did this condition come from? It will mean all
>>> the ID regs fe0 and above end up treated as bad registers
>>> rather than taking the code below to return the ID value.
>>
>> Woops, I copied the wrong value. I mean that to be 0xffc
>>
>> Do you still want a maximum check (otherwise it will walk off the array)?
>
> FFD..FFF should read as zeroes (like the other non-multiple-of-4
> offset addresses in this region), and you can't get more than 0xfff
> because the size of the memory region is 0x1000. That's not
> entirely obvious though, so if you want a max check you can
> put one in. I think that having it match the style of the rest
> of the if..else if ladder would be better though:
>
>   ...
>  else if (offset < 0x1000) {
>      array code here;
>  else {
>      g_assert_not_reached();
>  }
>

Ok, I like that.

Thanks,

Alistair

> or something.
>
> thanks
> -- PMM
>
diff mbox

Patch

diff --git a/hw/intc/arm_gic.c b/hw/intc/arm_gic.c
index 13e297d..8eb3a2d 100644
--- a/hw/intc/arm_gic.c
+++ b/hw/intc/arm_gic.c
@@ -31,8 +31,16 @@  do { fprintf(stderr, "arm_gic: " fmt , ## __VA_ARGS__); } while (0)
 #define DPRINTF(fmt, ...) do {} while(0)
 #endif
 
-static const uint8_t gic_id[] = {
-    0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1
+static const uint8_t gic_id_11mpcore[] = {
+    0x00, 0x00, 0x00, 0x00, 0x90, 0x13, 0x04, 0x00, 0x0d, 0xf0, 0x05, 0xb1
+};
+
+static const uint8_t gic_id_gicv1[] = {
+    0x04, 0x00, 0x00, 0x00, 0x90, 0xb3, 0x1b, 0x00, 0x0d, 0xf0, 0x05, 0xb1
+};
+
+static const uint8_t gic_id_gicv2[] = {
+    0x04, 0x00, 0x00, 0x00, 0x90, 0xb4, 0x2b, 0x00, 0x0d, 0xf0, 0x05, 0xb1
 };
 
 static inline int gic_get_current_cpu(GICState *s)
@@ -683,13 +691,30 @@  static uint32_t gic_dist_readb(void *opaque, hwaddr offset, MemTxAttrs attrs)
         }
 
         res = s->sgi_pending[irq][cpu];
-    } else if (offset < 0xfe0) {
+    } else if (offset < 0xfd0) {
         goto bad_reg;
-    } else /* offset >= 0xfe0 */ {
+    } else /* offset >= 0xfd0 */ {
         if (offset & 3) {
             res = 0;
+        } else if (offset > 0xfdf) {
+            goto bad_reg;
         } else {
-            res = gic_id[(offset - 0xfe0) >> 2];
+            switch (s->revision) {
+            case REV_11MPCORE:
+                res = gic_id_11mpcore[(offset - 0xfd0) >> 2];
+                break;
+            case 1:
+                res = gic_id_gicv1[(offset - 0xfd0) >> 2];
+                break;
+            case 2:
+                res = gic_id_gicv2[(offset - 0xfd0) >> 2];
+                break;
+            case REV_NVIC:
+                /* Shouldn't be able to get here */
+                abort();
+            default:
+                res = 0;
+            }
         }
     }
     return res;