1. Patchew
  2. QEMU
  3. [PATCH v2 00/11] More SH4 clean ups
  4. View patch

[PATCH v2 08/11] hw/intc/sh_intc: Use existing macro instead of local one

BALATON Zoltan posted 11 patches 4 years, 3 months ago
Maintainers: "Marc-André Lureau" <marcandre.lureau@redhat.com>, Yoshinori Sato <ysato@users.sourceforge.jp>, Paolo Bonzini <pbonzini@redhat.com>, Magnus Damm <magnus.damm@gmail.com>
There is a newer version of this series
[PATCH v2 08/11] hw/intc/sh_intc: Use existing macro instead of local one
Posted by BALATON Zoltan 4 years, 3 months ago 
The INTC_A7 local macro does the same as the A7ADDR from
include/sh/sh.h so use the latter and drop the local macro definiion.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/intc/sh_intc.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
index c1058d97c0..0bd27aaf4f 100644
--- a/hw/intc/sh_intc.c
+++ b/hw/intc/sh_intc.c
@@ -16,8 +16,6 @@
 #include "hw/sh4/sh.h"
 #include "trace.h"
 
-#define INTC_A7(x) ((x) & 0x1fffffff)
-
 void sh_intc_toggle_source(struct intc_source *source,
                            int enable_adj, int assert_adj)
 {
@@ -112,12 +110,12 @@ int sh_intc_get_pending_vector(struct intc_desc *desc, int imask)
 static unsigned int sh_intc_mode(unsigned long address,
                                  unsigned long set_reg, unsigned long clr_reg)
 {
-    if ((address != INTC_A7(set_reg)) &&
-        (address != INTC_A7(clr_reg)))
+    if ((address != A7ADDR(set_reg)) &&
+        (address != A7ADDR(clr_reg)))
         return INTC_MODE_NONE;
 
     if (set_reg && clr_reg) {
-        if (address == INTC_A7(set_reg)) {
+        if (address == A7ADDR(set_reg)) {
             return INTC_MODE_DUAL_SET;
         } else {
             return INTC_MODE_DUAL_CLR;
@@ -297,11 +295,11 @@ static unsigned int sh_intc_register(MemoryRegion *sysmem,
 
 #define SH_INTC_IOMEM_FORMAT "interrupt-controller-%s-%s-%s"
     snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "p4");
-    memory_region_init_alias(iomem_p4, NULL, name, iomem, INTC_A7(address), 4);
+    memory_region_init_alias(iomem_p4, NULL, name, iomem, A7ADDR(address), 4);
     memory_region_add_subregion(sysmem, P4ADDR(address), iomem_p4);
 
     snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "a7");
-    memory_region_init_alias(iomem_a7, NULL, name, iomem, INTC_A7(address), 4);
+    memory_region_init_alias(iomem_a7, NULL, name, iomem, A7ADDR(address), 4);
     memory_region_add_subregion(sysmem, A7ADDR(address), iomem_a7);
 #undef SH_INTC_IOMEM_FORMAT
 
-- 
2.21.4
Re: [PATCH v2 08/11] hw/intc/sh_intc: Use existing macro instead of local one
Posted by Philippe Mathieu-Daudé 4 years, 3 months ago 
On 10/27/21 15:46, BALATON Zoltan wrote:
> The INTC_A7 local macro does the same as the A7ADDR from
> include/sh/sh.h so use the latter and drop the local macro definiion.

Typo "definition".

> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/intc/sh_intc.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
> index c1058d97c0..0bd27aaf4f 100644
> --- a/hw/intc/sh_intc.c
> +++ b/hw/intc/sh_intc.c
> @@ -16,8 +16,6 @@
>  #include "hw/sh4/sh.h"
>  #include "trace.h"
>  
> -#define INTC_A7(x) ((x) & 0x1fffffff)
> -
>  void sh_intc_toggle_source(struct intc_source *source,
>                             int enable_adj, int assert_adj)
>  {
> @@ -112,12 +110,12 @@ int sh_intc_get_pending_vector(struct intc_desc *desc, int imask)
>  static unsigned int sh_intc_mode(unsigned long address,
>                                   unsigned long set_reg, unsigned long clr_reg)
>  {
> -    if ((address != INTC_A7(set_reg)) &&
> -        (address != INTC_A7(clr_reg)))
> +    if ((address != A7ADDR(set_reg)) &&
> +        (address != A7ADDR(clr_reg)))
>          return INTC_MODE_NONE;
>  
>      if (set_reg && clr_reg) {
> -        if (address == INTC_A7(set_reg)) {
> +        if (address == A7ADDR(set_reg)) {
>              return INTC_MODE_DUAL_SET;
>          } else {
>              return INTC_MODE_DUAL_CLR;
> @@ -297,11 +295,11 @@ static unsigned int sh_intc_register(MemoryRegion *sysmem,
>  
>  #define SH_INTC_IOMEM_FORMAT "interrupt-controller-%s-%s-%s"
>      snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "p4");
> -    memory_region_init_alias(iomem_p4, NULL, name, iomem, INTC_A7(address), 4);
> +    memory_region_init_alias(iomem_p4, NULL, name, iomem, A7ADDR(address), 4);
>      memory_region_add_subregion(sysmem, P4ADDR(address), iomem_p4);
>  
>      snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "a7");
> -    memory_region_init_alias(iomem_a7, NULL, name, iomem, INTC_A7(address), 4);
> +    memory_region_init_alias(iomem_a7, NULL, name, iomem, A7ADDR(address), 4);

I wonder why the address is masked out... It looks there is a mismatch
in the memory region mapping. Anyway this predates this cleanup, so:

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Re: [PATCH v2 08/11] hw/intc/sh_intc: Use existing macro instead of local one
Posted by BALATON Zoltan 4 years, 3 months ago 
On Wed, 27 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/27/21 15:46, BALATON Zoltan wrote:
>> The INTC_A7 local macro does the same as the A7ADDR from
>> include/sh/sh.h so use the latter and drop the local macro definiion.
>
> Typo "definition".
>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/intc/sh_intc.c | 12 +++++-------
>>  1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
>> index c1058d97c0..0bd27aaf4f 100644
>> --- a/hw/intc/sh_intc.c
>> +++ b/hw/intc/sh_intc.c
>> @@ -16,8 +16,6 @@
>>  #include "hw/sh4/sh.h"
>>  #include "trace.h"
>>
>> -#define INTC_A7(x) ((x) & 0x1fffffff)
>> -
>>  void sh_intc_toggle_source(struct intc_source *source,
>>                             int enable_adj, int assert_adj)
>>  {
>> @@ -112,12 +110,12 @@ int sh_intc_get_pending_vector(struct intc_desc *desc, int imask)
>>  static unsigned int sh_intc_mode(unsigned long address,
>>                                   unsigned long set_reg, unsigned long clr_reg)
>>  {
>> -    if ((address != INTC_A7(set_reg)) &&
>> -        (address != INTC_A7(clr_reg)))
>> +    if ((address != A7ADDR(set_reg)) &&
>> +        (address != A7ADDR(clr_reg)))
>>          return INTC_MODE_NONE;
>>
>>      if (set_reg && clr_reg) {
>> -        if (address == INTC_A7(set_reg)) {
>> +        if (address == A7ADDR(set_reg)) {
>>              return INTC_MODE_DUAL_SET;
>>          } else {
>>              return INTC_MODE_DUAL_CLR;
>> @@ -297,11 +295,11 @@ static unsigned int sh_intc_register(MemoryRegion *sysmem,
>>
>>  #define SH_INTC_IOMEM_FORMAT "interrupt-controller-%s-%s-%s"
>>      snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "p4");
>> -    memory_region_init_alias(iomem_p4, NULL, name, iomem, INTC_A7(address), 4);
>> +    memory_region_init_alias(iomem_p4, NULL, name, iomem, A7ADDR(address), 4);
>>      memory_region_add_subregion(sysmem, P4ADDR(address), iomem_p4);
>>
>>      snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "a7");
>> -    memory_region_init_alias(iomem_a7, NULL, name, iomem, INTC_A7(address), 4);
>> +    memory_region_init_alias(iomem_a7, NULL, name, iomem, A7ADDR(address), 4);
>
> I wonder why the address is masked out... It looks there is a mismatch
> in the memory region mapping. Anyway this predates this cleanup, so:

This seems to be a peculiarity of the SH architecture. Like MIPS, it has 
some strange memory mapping conventions where same registers appear in 
different areas at predefined addresses. These macros just calculate that 
address.

Regards,
BALATON Zoltan

> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
>
Re: [PATCH v2 08/11] hw/intc/sh_intc: Use existing macro instead of local one
Posted by BALATON Zoltan 4 years, 3 months ago 
On Wed, 27 Oct 2021, BALATON Zoltan wrote:
> On Wed, 27 Oct 2021, Philippe Mathieu-Daudé wrote:
>> On 10/27/21 15:46, BALATON Zoltan wrote:
>>> The INTC_A7 local macro does the same as the A7ADDR from
>>> include/sh/sh.h so use the latter and drop the local macro definiion.
>> 
>> Typo "definition".
>> 
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>  hw/intc/sh_intc.c | 12 +++++-------
>>>  1 file changed, 5 insertions(+), 7 deletions(-)
>>> 
>>> diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
>>> index c1058d97c0..0bd27aaf4f 100644
>>> --- a/hw/intc/sh_intc.c
>>> +++ b/hw/intc/sh_intc.c
>>> @@ -16,8 +16,6 @@
>>>  #include "hw/sh4/sh.h"
>>>  #include "trace.h"
>>> 
>>> -#define INTC_A7(x) ((x) & 0x1fffffff)
>>> -
>>>  void sh_intc_toggle_source(struct intc_source *source,
>>>                             int enable_adj, int assert_adj)
>>>  {
>>> @@ -112,12 +110,12 @@ int sh_intc_get_pending_vector(struct intc_desc 
>>> *desc, int imask)
>>>  static unsigned int sh_intc_mode(unsigned long address,
>>>                                   unsigned long set_reg, unsigned long 
>>> clr_reg)
>>>  {
>>> -    if ((address != INTC_A7(set_reg)) &&
>>> -        (address != INTC_A7(clr_reg)))
>>> +    if ((address != A7ADDR(set_reg)) &&
>>> +        (address != A7ADDR(clr_reg)))
>>>          return INTC_MODE_NONE;
>>>
>>>      if (set_reg && clr_reg) {
>>> -        if (address == INTC_A7(set_reg)) {
>>> +        if (address == A7ADDR(set_reg)) {
>>>              return INTC_MODE_DUAL_SET;
>>>          } else {
>>>              return INTC_MODE_DUAL_CLR;
>>> @@ -297,11 +295,11 @@ static unsigned int sh_intc_register(MemoryRegion 
>>> *sysmem,
>>>
>>>  #define SH_INTC_IOMEM_FORMAT "interrupt-controller-%s-%s-%s"
>>>      snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, 
>>> "p4");
>>> -    memory_region_init_alias(iomem_p4, NULL, name, iomem, 
>>> INTC_A7(address), 4);
>>> +    memory_region_init_alias(iomem_p4, NULL, name, iomem, 
>>> A7ADDR(address), 4);
>>>      memory_region_add_subregion(sysmem, P4ADDR(address), iomem_p4);
>>>
>>>      snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, 
>>> "a7");
>>> -    memory_region_init_alias(iomem_a7, NULL, name, iomem, 
>>> INTC_A7(address), 4);
>>> +    memory_region_init_alias(iomem_a7, NULL, name, iomem, 
>>> A7ADDR(address), 4);
>> 
>> I wonder why the address is masked out... It looks there is a mismatch
>> in the memory region mapping. Anyway this predates this cleanup, so:
>
> This seems to be a peculiarity of the SH architecture. Like MIPS, it has some 
> strange memory mapping conventions where same registers appear in different 
> areas at predefined addresses. These macros just calculate that address.

Hmm, on second look it creates alias at A7ADDR into the original region so 
maybe reducing the size from 4GB could break that. I'll have to check 
again what this is doing.

Regards,
BALATON Zoltan

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.