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

[Shin-ichiro KAWASAKI <kawasaki@juno.dti.ne.jp>]

Thiemo Seufer wrote:
> Jean-Christophe PLAGNIOL-VILLARD wrote:
>> On 00:36 Mon 15 Dec     , 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));
>>> }
>> I've download the SH7750 & SH7751 datasheet and yes as it's only availlable on
>> 'R' cpu revision
>>
>> I'll update as it
>>>
>>>>> 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.
>>>
>> Yes it will be nice
> 
> I believe "no new gcc-isms" would be a good policy.

Thank you.  I'll follow it.

So, my suggestion about 'if' vs 'switch-case' is not appropriate.


Regards,
Shin-ichiro KAWASAKI