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

[Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>]

> >  
> > -
> > +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
> 
> >      case SH7750_FRQCR_A7:
> >  	return 0;
> > +    case SH7750_PCR_A7:
> > +	return s->pcr;
> >      case SH7750_RFCR_A7:
> >  	fprintf(stderr,
> >  		"Read access to refresh count register, incrementing\n");
> > @@ -222,6 +238,11 @@ static uint32_t sh7750_mem_readw(void *opaque, target_phys_addr_t addr)
> >  	return porta_lines(s);
> >      case SH7750_PDTRB_A7:
> >  	return portb_lines(s);
> > +    case SH7750_RTCOR_A7:
> > +    case SH7750_RTCNT_A7:
> > +    case SH7750_RTCSR_A7:
> > +	ignore_access("word read", addr);
> > +	return 0;
> >      case 0x1fd00000:
> >          return s->icr;
> >      default:
> > @@ -238,6 +259,12 @@ static uint32_t sh7750_mem_readl(void *opaque, target_phys_addr_t addr)
> >      case SH7750_BCR1_A7:
> >  	return s->bcr1;
> >      case SH7750_BCR4_A7:
> > +	if(is_sh7751r_cpu(s)) {
> > +	    return s->bcr4;
> > +	} else {
> > +	    error_access("long read", addr);
> > +	    assert(0);
> > +	}
> 
> For BCR4, same as above.
> 
> >      case SH7750_WCR1_A7:
> >      case SH7750_WCR2_A7:
> >      case SH7750_WCR3_A7:
> > @@ -274,9 +301,17 @@ static uint32_t sh7750_mem_readl(void *opaque, target_phys_addr_t addr)
> >      }
> >  }
> >  
> > +#define is_in_sdrmx(a, x) (a >= SH7750_SDMR ## x ## _A7 \
> > +			&& a <= (SH7750_SDMR ## x ## _A7 + SH7750_SDMR ## x ## _REGNB))
> >  static void sh7750_mem_writeb(void *opaque, target_phys_addr_t addr,
> >  			      uint32_t mem_value)
> >  {
> > +
> > +    if (is_in_sdrmx(addr, 2) || is_in_sdrmx(addr, 3)) {
> > +	ignore_access("word write", addr);
> > +	return;
> > +    }
> > +
> 
> SRMR2 and SDMR3 region overlaps with the PRECHARGE0/1 register.
> If you introduce them, PRECHARGE0/1 register should be removed.
> # U-Boot does not seem to touch these regions.  Does it?
IIRC no I'll check it
> 
> Compared to 'if' statement, 'switch-case' might be more easy to
> understand, like as follows.
>   case SH7750_SDMR2 ... SH7750_SDMR2 + SDMR2_REGNB
ok evenif it's a gcc only extension

Best Regards,
J.