From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LBuho-0004in-Sf for qemu-devel@nongnu.org; Sun, 14 Dec 2008 12:24:36 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LBuhm-0004iO-D8 for qemu-devel@nongnu.org; Sun, 14 Dec 2008 12:24:36 -0500 Received: from [199.232.76.173] (port=48408 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LBuhm-0004iJ-4D for qemu-devel@nongnu.org; Sun, 14 Dec 2008 12:24:34 -0500 Received: from relay01.mx.bawue.net ([193.7.176.67]:43015) by monty-python.gnu.org with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1LBuhl-0006FX-Lz for qemu-devel@nongnu.org; Sun, 14 Dec 2008 12:24:34 -0500 Date: Sun, 14 Dec 2008 18:24:31 +0100 From: Thiemo Seufer Subject: Re: [Qemu-devel] [PATCH] SH7750/51: add register BCR3, NCR4, PCR, RTCOR, RTCNT, RTCSR, SDMR2, SDMR3 and fix BCR2 support Message-ID: <20081214172431.GC7738@networkno.de> References: <1228477782-19842-1-git-send-email-plagnioj@jcrosoft.com> <4944EFFE.1050604@juno.dti.ne.jp> <20081214145145.GC18560@game.jcrosoft.org> <494527F5.3030109@juno.dti.ne.jp> <20081214163710.GB28340@game.jcrosoft.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20081214163710.GB28340@game.jcrosoft.org> Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Shin-ichiro KAWASAKI , qemu-devel@nongnu.org, Takashi Yoshii , Nobuhiro Iwamatsu 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. Thiemo