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

[BALATON Zoltan]

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