[PATCH v2 10/11] hw/intc/sh_intc: Clean up iomem region

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 10/11] hw/intc/sh_intc: Clean up iomem region
Posted by BALATON Zoltan 4 years, 3 months ago
Fix the size of the iomem region and rename it to "intc" from
"interrupt-controller" which makes the info mtree output less wide as
it is already too wide because of all the aliases. Also drop the
format macro which was only used twice in close proximity so we can
just use the literal string instead without a macro definition.

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

diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
index 18461ff554..fc1905f299 100644
--- a/hw/intc/sh_intc.c
+++ b/hw/intc/sh_intc.c
@@ -288,15 +288,13 @@ static unsigned int sh_intc_register(MemoryRegion *sysmem,
     iomem_p4 = desc->iomem_aliases + index;
     iomem_a7 = iomem_p4 + 1;
 
-#define SH_INTC_IOMEM_FORMAT "interrupt-controller-%s-%s-%s"
-    snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "p4");
+    snprintf(name, sizeof(name), "intc-%s-%s-%s", type, action, "p4");
     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");
+    snprintf(name, sizeof(name), "intc-%s-%s-%s", type, action, "a7");
     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
 
     /* used to increment aliases index */
     return 2;
@@ -432,9 +430,7 @@ int sh_intc_init(MemoryRegion *sysmem,
     }
 
     desc->irqs = qemu_allocate_irqs(sh_intc_set_irq, desc, nr_sources);
-
-    memory_region_init_io(&desc->iomem, NULL, &sh_intc_ops, desc,
-                          "interrupt-controller", 0x100000000ULL);
+    memory_region_init_io(&desc->iomem, NULL, &sh_intc_ops, desc, "intc", 4);
 
 #define INT_REG_PARAMS(reg_struct, type, action, j) \
         reg_struct->action##_reg, #type, #action, j
-- 
2.21.4


Re: [PATCH v2 10/11] hw/intc/sh_intc: Clean up iomem region
Posted by Philippe Mathieu-Daudé 4 years, 3 months ago
On 10/27/21 15:46, BALATON Zoltan wrote:
> Fix the size of the iomem region and rename it to "intc" from
> "interrupt-controller" which makes the info mtree output less wide as
> it is already too wide because of all the aliases. Also drop the
> format macro which was only used twice in close proximity so we can
> just use the literal string instead without a macro definition.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/intc/sh_intc.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)

> -    memory_region_init_io(&desc->iomem, NULL, &sh_intc_ops, desc,
> -                          "interrupt-controller", 0x100000000ULL);
> +    memory_region_init_io(&desc->iomem, NULL, &sh_intc_ops, desc, "intc", 4);

Why the region size change from 4GB -> 4B? Did you mean '4 * GiB'?

Re: [PATCH v2 10/11] hw/intc/sh_intc: Clean up iomem region
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:
>> Fix the size of the iomem region and rename it to "intc" from
>> "interrupt-controller" which makes the info mtree output less wide as
>> it is already too wide because of all the aliases. Also drop the
>> format macro which was only used twice in close proximity so we can
>> just use the literal string instead without a macro definition.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/intc/sh_intc.c | 10 +++-------
>>  1 file changed, 3 insertions(+), 7 deletions(-)
>
>> -    memory_region_init_io(&desc->iomem, NULL, &sh_intc_ops, desc,
>> -                          "interrupt-controller", 0x100000000ULL);
>> +    memory_region_init_io(&desc->iomem, NULL, &sh_intc_ops, desc, "intc", 4);
>
> Why the region size change from 4GB -> 4B? Did you mean '4 * GiB'?

No, it's really just 4 bytes, like the sh_serial region is 0x28 bytes but 
previously these were unnecessarily allocated as 4GB and then mapped in 
sysmem via the small 4 byte (or 0x28 byte for sh_serial) alias regions 
only. So we don't actually need these to be 4GB as there's nothing beyond 
the actual length so just declare them the necessary size. (I'm thinking 
maybe later we can drop one of the P4 or A7 alias and map the actual iomem 
at one of these directly and use an alias for the other but that's a 
separate clean up later.)

Regards,
BALATON Zoltan