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

[Aurelien Jarno]

On Mon, Dec 15, 2008 at 12:36:21AM +0900, Shin-ichiro KAWASAKI wrote:
> 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));
> }

Those functions are probably available on more CPU than those ones, or
in future CPU. I would suggest to use the new features field introduced
in revision 6013. 

-- 
  .''`.  Aurelien Jarno             | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   address@hidden         | address@hidden
   `-    people.debian.org/~aurel32 | www.aurel32.net