From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1LBpK3-0005lA-QL for qemu-devel@nongnu.org; Sun, 14 Dec 2008 06:39:44 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1LBpK0-0005jU-Ga for qemu-devel@nongnu.org; Sun, 14 Dec 2008 06:39:41 -0500 Received: from [199.232.76.173] (port=39369 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1LBpJx-0005ih-VH for qemu-devel@nongnu.org; Sun, 14 Dec 2008 06:39:38 -0500 Received: from vsmtp04.dti.ne.jp ([202.216.231.139]:35588) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1LBpJw-00066b-QU for qemu-devel@nongnu.org; Sun, 14 Dec 2008 06:39:37 -0500 Message-ID: <4944EFFE.1050604@juno.dti.ne.jp> Date: Sun, 14 Dec 2008 20:37:34 +0900 From: Shin-ichiro KAWASAKI MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] SH7750/51: add register BCR3, NCR4, PCR, RTCOR, RTCNT, RTCSR, SDMR2, SDMR3 and fix BCR2 support References: <1228477782-19842-1-git-send-email-plagnioj@jcrosoft.com> In-Reply-To: <1228477782-19842-1-git-send-email-plagnioj@jcrosoft.com> Content-Type: text/plain; charset=ISO-2022-JP Content-Transfer-Encoding: 7bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: Takashi Yoshii , Jean-Christophe PLAGNIOL-VILLARD , Nobuhiro Iwamatsu Hi, Jean-san. Thanks for your work. Please find my comments on the patch, in line. # When I send mail to your address from my PC, # mail delivery error happens as follows. # # ----- Transcript of session follows ----- # 451 4.4.1 reply: read error from mx1.ovh.net. # 451 4.4.1 reply: read error from mx2.ovh.net. # ... Deferred: Connection timed out with mx2.ovh.net. # # How can I keep in touch with you? Regards, Shin-ichiro KAWASAKI Jean-Christophe PLAGNIOL-VILLARD wrote: > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD > Cc: Takashi Yoshii > Cc: Nobuhiro Iwamatsu > --- > hw/sh7750.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++-- > hw/sh7750_regnames.c | 2 + > hw/sh7750_regs.h | 11 +++++++++ > 3 files changed, 69 insertions(+), 3 deletions(-) > > diff --git a/hw/sh7750.c b/hw/sh7750.c > index 4ae90b1..78843f3 100644 > --- a/hw/sh7750.c > +++ b/hw/sh7750.c > @@ -42,8 +42,12 @@ typedef struct SH7750State { > uint32_t periph_freq; > /* SDRAM controller */ > uint32_t bcr1; > - uint32_t bcr2; > + uint16_t bcr2; > + uint16_t bcr3; > + uint32_t bcr4; > uint16_t rfcr; > + /* PCMCIA controller */ > + uint16_t pcr; > /* IO ports */ > uint16_t gpioic; > uint32_t pctra; > @@ -67,7 +71,10 @@ typedef struct SH7750State { > struct intc_desc intc; > } SH7750State; > > - > +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. > 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? Compared to 'if' statement, 'switch-case' might be more easy to understand, like as follows. case SH7750_SDMR2 ... SH7750_SDMR2 + SDMR2_REGNB > switch (addr) { > /* PRECHARGE ? XXXXX */ > case SH7750_PRECHARGE0_A7: > @@ -301,8 +336,18 @@ static void sh7750_mem_writew(void *opaque, target_phys_addr_t addr, > s->bcr2 = mem_value; > return; > case SH7750_BCR3_A7: > - case SH7750_RTCOR_A7: > + if(is_sh7751r_cpu(s)) { > + s->bcr3 = mem_value; > + return; > + } else { > + error_access("word write", addr); > + assert(0); > + } Same as readw for BCR3. > + case SH7750_PCR_A7: > + s->pcr = mem_value; > + return; > case SH7750_RTCNT_A7: > + case SH7750_RTCOR_A7: > case SH7750_RTCSR_A7: > ignore_access("word write", addr); > return; > @@ -349,6 +394,14 @@ static void sh7750_mem_writel(void *opaque, target_phys_addr_t addr, > s->bcr1 = mem_value; > return; > case SH7750_BCR4_A7: > + if(is_sh7751r_cpu(s)) { > + s->bcr4 = mem_value; > + return; > + } else { > + error_access("long write", addr); > + assert(0); > + } > + return; > case SH7750_WCR1_A7: > case SH7750_WCR2_A7: > case SH7750_WCR3_A7: > diff --git a/hw/sh7750_regnames.c b/hw/sh7750_regnames.c > index 51283c9..77993c1 100644 > --- a/hw/sh7750_regnames.c > +++ b/hw/sh7750_regnames.c > @@ -79,6 +79,8 @@ static regname_t regnames[] = { > REGNAME(SH7750_ICR_A7) > REGNAME(SH7750_BCR3_A7) > REGNAME(SH7750_BCR4_A7) > + REGNAME(SH7750_SDMR2_A7) > + REGNAME(SH7750_SDMR3_A7) > REGNAME(SH7750_PRECHARGE0_A7) > REGNAME(SH7750_PRECHARGE1_A7) {(uint32_t) - 1, 0} > }; > diff --git a/hw/sh7750_regs.h b/hw/sh7750_regs.h > index c8fb328..4ed471b 100644 > --- a/hw/sh7750_regs.h > +++ b/hw/sh7750_regs.h > @@ -979,6 +979,17 @@ > > #define SH7750_RFCR_KEY 0xA400 /* RFCR write key */ > > +/* Synchronous DRAM mode registers - SDMR */ > +#define SH7750_SDMR2_REGOFS 0x900000 /* base offset */ > +#define SH7750_SDMR2_REGNB 0x0FFC /* nb of register */ > +#define SH7750_SDMR2 SH7750_P4_REG32(SH7750_SDMR2_REGOFS) > +#define SH7750_SDMR2_A7 SH7750_A7_REG32(SH7750_SDMR2_REGOFS) > + > +#define SH7750_SDMR3_REGOFS 0x940000 /* offset */ > +#define SH7750_SDMR3_REGNB 0x0FFC /* nb of register */ > +#define SH7750_SDMR3 SH7750_P4_REG32(SH7750_SDMR3_REGOFS) > +#define SH7750_SDMR3_A7 SH7750_A7_REG32(SH7750_SDMR3_REGOFS) > + > /* > * Direct Memory Access Controller (DMAC) > */