Re: [Qemu-devel] [PATCH] SH4: Serial controller improvement and sleep op bug fix

[Aurelien Jarno]

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
 `. `'   address@hidden         | address@hidden
   `-    people.debian.org/~aurel32 | www.aurel32.net