[PATCH 0/3] target/avr: Few fixes

[PATCH 0/3] target/avr: Few fixes

[Philippe Mathieu-Daudé]

Few fixes on top of the AVR merger series Thomas sent yesterday:

https://www.mail-archive.com/qemu-devel@nongnu.org/msg720089.html
Based-on: <20200705140315.260514-1-huth@tuxfamily.org>

Philippe Mathieu-Daudé (3):
  target/avr: Drop tlb_flush() in avr_cpu_reset()
  target/avr: Fix $PC displayed address
  target/avr: Fix SBRC/SBRS instructions

 target/avr/cpu.c       | 4 +---
 target/avr/translate.c | 4 ++--
 2 files changed, 3 insertions(+), 5 deletions(-)

-- 
2.21.3

[PATCH 1/3] target/avr: Drop tlb_flush() in avr_cpu_reset()

[Philippe Mathieu-Daudé]

Since commit 1f5c00cfdb tlb_flush() is called from cpu_common_reset().

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/avr/cpu.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index 4e4dd4f6aa..50fb1c378b 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -78,8 +78,6 @@ static void avr_cpu_reset(DeviceState *ds)
     env->skip = 0;
 
     memset(env->r, 0, sizeof(env->r));
-
-    tlb_flush(cs);
 }
 
 static void avr_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)
-- 
2.21.3

Re: [PATCH 1/3] target/avr: Drop tlb_flush() in avr_cpu_reset()

[Alex Bennée]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> Since commit 1f5c00cfdb tlb_flush() is called from cpu_common_reset().
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  target/avr/cpu.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> index 4e4dd4f6aa..50fb1c378b 100644
> --- a/target/avr/cpu.c
> +++ b/target/avr/cpu.c
> @@ -78,8 +78,6 @@ static void avr_cpu_reset(DeviceState *ds)
>      env->skip = 0;
>  
>      memset(env->r, 0, sizeof(env->r));
> -
> -    tlb_flush(cs);
>  }
>  
>  static void avr_cpu_disas_set_info(CPUState *cpu, disassemble_info *info)


-- 
Alex Bennée

[PATCH 2/3] target/avr: Fix $PC displayed address

[Philippe Mathieu-Daudé]

$PC is 16-bit wide. Other registers display addresses on a byte
granularity.
To have a coherent ouput, display $PC using byte granularity too.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/avr/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/avr/cpu.c b/target/avr/cpu.c
index 50fb1c378b..9be464991f 100644
--- a/target/avr/cpu.c
+++ b/target/avr/cpu.c
@@ -151,7 +151,7 @@ static void avr_cpu_dump_state(CPUState *cs, FILE *f, int flags)
     int i;
 
     qemu_fprintf(f, "\n");
-    qemu_fprintf(f, "PC:    %06x\n", env->pc_w);
+    qemu_fprintf(f, "PC:    %06x\n", env->pc_w * 2);
     qemu_fprintf(f, "SP:      %04x\n", env->sp);
     qemu_fprintf(f, "rampD:     %02x\n", env->rampD >> 16);
     qemu_fprintf(f, "rampX:     %02x\n", env->rampX >> 16);
-- 
2.21.3

Re: [PATCH 2/3] target/avr: Fix $PC displayed address

[Alex Bennée]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> $PC is 16-bit wide. Other registers display addresses on a byte
> granularity.
> To have a coherent ouput, display $PC using byte granularity too.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  target/avr/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/avr/cpu.c b/target/avr/cpu.c
> index 50fb1c378b..9be464991f 100644
> --- a/target/avr/cpu.c
> +++ b/target/avr/cpu.c
> @@ -151,7 +151,7 @@ static void avr_cpu_dump_state(CPUState *cs, FILE *f, int flags)
>      int i;
>  
>      qemu_fprintf(f, "\n");
> -    qemu_fprintf(f, "PC:    %06x\n", env->pc_w);
> +    qemu_fprintf(f, "PC:    %06x\n", env->pc_w * 2);

OK this was confusing until I grepped around the code and found the
comment in set pc:

    cpu->env.pc_w = value / 2; /* internally PC points to words */

so it makes sense I guess but I didn't pick it up from the name. Maybe
worth adding the comment in CPUAVRstate?

Anyway:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


>      qemu_fprintf(f, "SP:      %04x\n", env->sp);
>      qemu_fprintf(f, "rampD:     %02x\n", env->rampD >> 16);
>      qemu_fprintf(f, "rampX:     %02x\n", env->rampX >> 16);


-- 
Alex Bennée

[RFC PATCH 3/3] target/avr: Fix SBRC/SBRS instructions

[Philippe Mathieu-Daudé]

SBRC/SBRS instructions seem to be inverted.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 target/avr/translate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/avr/translate.c b/target/avr/translate.c
index fe03e676df..2f77fe3ba7 100644
--- a/target/avr/translate.c
+++ b/target/avr/translate.c
@@ -1385,7 +1385,7 @@ static bool trans_SBRC(DisasContext *ctx, arg_SBRC *a)
 {
     TCGv Rr = cpu_r[a->rr];
 
-    ctx->skip_cond = TCG_COND_EQ;
+    ctx->skip_cond = TCG_COND_NE;
     ctx->skip_var0 = tcg_temp_new();
     ctx->free_skip_var0 = true;
 
@@ -1401,7 +1401,7 @@ static bool trans_SBRS(DisasContext *ctx, arg_SBRS *a)
 {
     TCGv Rr = cpu_r[a->rr];
 
-    ctx->skip_cond = TCG_COND_NE;
+    ctx->skip_cond = TCG_COND_EQ;
     ctx->skip_var0 = tcg_temp_new();
     ctx->free_skip_var0 = true;
 
-- 
2.21.3

Re: [RFC PATCH 3/3] target/avr: Fix SBRC/SBRS instructions

[Alex Bennée]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> SBRC/SBRS instructions seem to be inverted.

I'm having trouble following exactly how the skip logic is meant to
work. Intuitively I would expect a skip if clear to be TCG_COND_EQ
because that is true if bit & mask is compared to 0 but it's not clear
that what happens.

It would be easier if we actually had some instruction tests. I see
gcc-avr is a thing.

>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  target/avr/translate.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/avr/translate.c b/target/avr/translate.c
> index fe03e676df..2f77fe3ba7 100644
> --- a/target/avr/translate.c
> +++ b/target/avr/translate.c
> @@ -1385,7 +1385,7 @@ static bool trans_SBRC(DisasContext *ctx, arg_SBRC *a)
>  {
>      TCGv Rr = cpu_r[a->rr];
>  
> -    ctx->skip_cond = TCG_COND_EQ;
> +    ctx->skip_cond = TCG_COND_NE;
>      ctx->skip_var0 = tcg_temp_new();
>      ctx->free_skip_var0 = true;
>  
> @@ -1401,7 +1401,7 @@ static bool trans_SBRS(DisasContext *ctx, arg_SBRS *a)
>  {
>      TCGv Rr = cpu_r[a->rr];
>  
> -    ctx->skip_cond = TCG_COND_NE;
> +    ctx->skip_cond = TCG_COND_EQ;
>      ctx->skip_var0 = tcg_temp_new();
>      ctx->free_skip_var0 = true;


-- 
Alex Bennée