From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Keuu5-0006P1-PY for qemu-devel@nongnu.org; Sun, 14 Sep 2008 12:56:53 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Keuu4-0006Mf-8D for qemu-devel@nongnu.org; Sun, 14 Sep 2008 12:56:52 -0400 Received: from [199.232.76.173] (port=57970 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Keuu4-0006MW-2t for qemu-devel@nongnu.org; Sun, 14 Sep 2008 12:56:52 -0400 Received: from hall.aurel32.net ([91.121.138.14]:59788) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1Keuu3-0002ch-C5 for qemu-devel@nongnu.org; Sun, 14 Sep 2008 12:56:51 -0400 Date: Sun, 14 Sep 2008 18:56:48 +0200 From: Aurelien Jarno Subject: Re: [Qemu-devel] [PATCH] SH4: Serial controller improvement and sleep op bug fix Message-ID: <20080914165648.GE22422@volta.aurel32.net> References: <48C3E662.1010204@juno.dti.ne.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline In-Reply-To: <48C3E662.1010204@juno.dti.ne.jp> 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 Cc: qemu-devel@nongnu.org On Sun, Sep 07, 2008 at 11:34:10PM +0900, Shin-ichiro KAWASAKI wrote: > Hi, all. Hi! > This patch adds some improvements for SH4 system emulation. > It enables serial controller to receive characters, so the > shell can receive command inputs. Now we can input 'ls' > and see it works, at last! > > It does: > - adds receive character feature to SH4 SCIF. > SH4-SCI feature implementation work is left. > - fixed a bug on 'sleep' instruction, which have caused halt of idle task. > As i386 'hlt' instruction does, it should save PC before sleep. Next time could you please send two different patches, so that one patch doesn't block the other. > I checked the patch with the kernel at the following URL, > which I mentioned before. > > > > http://www.assembla.com/file/qemu-sh4/3_zImage > > > The command line is like this. > > > % ./qemu/sh4-softmmu/qemu-system-sh4 -M r2d -serial vc -serial stdio -m 1024M -kernel zImage > > Comments on it and commit to the trunk will be appreciated. Please find my comments below. > Regards, > Shin-ichiro KAWASAKI > > > Index: target-sh4/helper.h > =================================================================== > --- target-sh4/helper.h (revision 5132) > +++ target-sh4/helper.h (working copy) > @@ -6,7 +6,7 @@ > DEF_HELPER(void, helper_raise_illegal_instruction, (void)) > DEF_HELPER(void, helper_raise_slot_illegal_instruction, (void)) > DEF_HELPER(void, helper_debug, (void)) > -DEF_HELPER(void, helper_sleep, (void)) > +DEF_HELPER(void, helper_sleep, (int32_t)) > DEF_HELPER(void, helper_trapa, (uint32_t)) > > DEF_HELPER(uint32_t, helper_addv, (uint32_t, uint32_t)) > Index: target-sh4/op_helper.c > =================================================================== > --- target-sh4/op_helper.c (revision 5132) > +++ target-sh4/op_helper.c (working copy) > @@ -94,10 +94,11 @@ > cpu_loop_exit(); > } > > -void helper_sleep(void) > +void helper_sleep(int32_t next_pc) Given then env->pc is unsigned, I think it should be uint32_t instead of int32_t here. Same in helper.h > { > env->halted = 1; > env->exception_index = EXCP_HLT; > + env-> pc = next_pc; > cpu_loop_exit(); > } > > Index: target-sh4/translate.c > =================================================================== > --- target-sh4/translate.c (revision 5132) > +++ target-sh4/translate.c (working copy) > @@ -446,7 +446,7 @@ > return; > case 0x001b: /* sleep */ > if (ctx->memidx) { > - tcg_gen_helper_0_0(helper_sleep); > + tcg_gen_helper_0_1(helper_sleep, tcg_const_i32(ctx->pc + 2)); > } else { > tcg_gen_helper_0_0(helper_raise_illegal_instruction); > ctx->bstate = BS_EXCP; > Index: hw/sh_serial.c > =================================================================== > --- hw/sh_serial.c (revision 5132) > +++ hw/sh_serial.c (working copy) > @@ -53,6 +53,7 @@ > int freq; > int feat; > int flags; > + int rtrg; > > CharDriverState *chr; > > @@ -80,6 +81,7 @@ > s->brr = val; > return; > case 0x08: /* SCR */ > + /* TODO : For SH7751, SCIF mask should be 0xfb. */ > s->scr = val & ((s->feat & SH_SERIAL_FEAT_SCIF) ? 0xfa : 0xff); > if (!(val & (1 << 5))) > s->flags |= SH_SERIAL_FLAG_TEND; > @@ -89,6 +91,9 @@ > else if (!(val & (1 << 7)) && s->txi->asserted) > sh_intc_toggle_source(s->txi, 0, -1); > } > + if (!(val & (1 << 6)) && s->rxi->asserted) { > + sh_intc_toggle_source(s->rxi, 0, -1); > + } > return; > case 0x0c: /* FTDR / TDR */ > if (s->chr) { > @@ -117,12 +122,37 @@ > s->flags &= ~SH_SERIAL_FLAG_RDF; > if (!(val & (1 << 0))) > s->flags &= ~SH_SERIAL_FLAG_DR; > + > + if (!(val & (1 << 1)) || !(val & (1 << 0))) { > + if (s->rxi && s->rxi->asserted) { > + sh_intc_toggle_source(s->rxi, 0, -1); > + } > + } > return; > case 0x18: /* FCR */ > s->fcr = val; > + switch ((val >> 6) & 3) { > + case 0: > + s->rtrg = 1; > + break; > + case 1: > + s->rtrg = 4; > + break; > + case 2: > + s->rtrg = 8; > + break; > + case 3: > + s->rtrg = 14; > + break; > + } > + if (val & (1 << 1)) { > + s->rx_cnt = 0; > + s->sr &= ~(1 << 1); > + } > + > return; > case 0x20: /* SPTR */ > - s->sptr = val; > + s->sptr = val & 0xf3; > return; > case 0x24: /* LSR */ > return; > @@ -194,6 +224,15 @@ > s->flags |= SH_SERIAL_FLAG_TDE | SH_SERIAL_FLAG_TEND; > > break; > + case 0x14: > + if (s->rx_cnt > 0) { > + ret = s->rx_fifo[0]; > + s->rx_cnt--; > + memmove(&s->rx_fifo[0], &s->rx_fifo[1], s->rx_cnt); I don't think moving the whole buffer each time a character is read is a good idea. I would suggest to use a circular buffer instead. Have a look at hw/serial.c how it is done. > + if (s->rx_cnt < s->rtrg) > + s->flags &= ~SH_SERIAL_FLAG_RDF; > + } > + break; > #if 0 > case 0x18: > ret = s->fcr; > @@ -219,6 +258,9 @@ > case 0x10: > ret = 0; > break; > + case 0x14: > + ret = s->rx_fifo[0]; > + break; > case 0x1c: > ret = s->sptr; > break; > @@ -240,15 +282,30 @@ > > static int sh_serial_can_receive(sh_serial_state *s) > { > - return 0; > + return s->scr & (1 << 4); > } > > static void sh_serial_receive_byte(sh_serial_state *s, int ch) > { > + if (s->feat & SH_SERIAL_FEAT_SCIF) { > + if (s->rx_cnt < sizeof(s->rx_fifo)) { > + s->rx_fifo[s->rx_cnt++] = ch; > + if (s->rx_cnt >= s->rtrg) { > + s->flags |= SH_SERIAL_FLAG_RDF; > + if (s->scr & (1 << 6) && s->rxi) { > + sh_intc_toggle_source(s->rxi, 0, 1); > + } > + } > + } > + } else { > + s->rx_fifo[0] = ch; > + } > } > > static void sh_serial_receive_break(sh_serial_state *s) > { > + if (s->feat & SH_SERIAL_FEAT_SCIF) > + s->sr |= (1 << 4); > } > > static int sh_serial_can_receive1(void *opaque) > @@ -313,6 +370,7 @@ > s->base = base; > s->feat = feat; > s->flags = SH_SERIAL_FLAG_TEND | SH_SERIAL_FLAG_TDE; > + s->rtrg = 1; > > s->smr = 0; > s->brr = 0xff; > @@ -323,6 +381,7 @@ > s->fcr = 0; > } > else { > + memset(s->rx_fifo, 0, sizeof(s->rx_fifo)); > s->dr = 0xff; > } > > > > -- .''`. Aurelien Jarno | GPG: 1024D/F1BCDB73 : :' : Debian developer | Electrical Engineer `. `' aurel32@debian.org | aurelien@aurel32.net `- people.debian.org/~aurel32 | www.aurel32.net