Re: [Qemu-devel] [PATCH] SH7750/51: add register BCR3, NCR4, PCR, RTCOR, RTCNT, RTCSR, SDMR2, SDMR3 and fix BCR2 support

[Shin-ichiro KAWASAKI]

Jean-Christophe PLAGNIOL-VILLARD wrote:
> > >  
> > > -
> > > +static int inline is_sh7751r_cpu(SH7750State * s)
> > > +{
> > > +       return (s->cpu->id & (SH_CPU_SH7751R));
> > > +}
> > >  /**********************************************************************
> > >   I/O ports
> > >  **********************************************************************/
> > > @@ -212,8 +219,17 @@ static uint32_t sh7750_mem_readw(void *opaque, 
> > > target_phys_addr_t addr)
> > >      switch (addr) {
> > >      case SH7750_BCR2_A7:
> > >         return s->bcr2;
> > > +    case SH7750_BCR3_A7:
> > > +       if(is_sh7751r_cpu(s)) {
> > > +           return s->bcr3;
> > > +       } else {
> > > +           error_access("word read", addr);
> > > +           assert(0);
> > > +       }
> > BCR3 exists not only for SH7751R, but also SH7750.
> > I think is_shh751r_cpu() check and error handling
> > should be removed to simplify the differcence.
> as write in the SH7751r datasheet
>
> Bus Control Register 3 (BCR3) (SH7751R Only)
> Bus Control Register 4 (BCR4) (SH7751R Only)
>
> That's why I've add the check

I see.  Your code is right, but let me add one more comment.

- SH7750 and SH7751 ... does not have BCR3 nor BCR4
- SH7750R and SH7751R ... have BCR3 and BCR4

So, to make it better, how about renaming "is_h7751r_cpu()"
into "has_bcr3_and_bcr4()"?  It will be like this.

static int inline has_bcr3_and_bcr4(SH7750State * s)
{
       return (s->cpu->id & (SH_CPU_SH7750R | SH_CPU_SH7751R));
}


> > Compared to 'if' statement, 'switch-case' might be more easy to
> > understand, like as follows.
> >   case SH7750_SDMR2 ... SH7750_SDMR2 + SDMR2_REGNB
> ok elvenif it's a gcc ony extension

Ah, gccism policy seems not clear in QEMU project.
This extension is used in many hw/*.c, so I guessed it
as a QEMU's usual implementation style.
I hope comments on it from main developers.


Regards,
Shin-ichiro KAWASAKI