diff mbox series

[RFC,v4,18/71] sh4: convert to cpu_halted

Message ID 20181025144644.15464-18-cota@braap.org (mailing list archive)
State New, archived
Headers show
Series [RFC,v4,01/71] cpu: convert queued work to a QSIMPLEQ | expand

Commit Message

Emilio Cota Oct. 25, 2018, 2:45 p.m. UTC 
Cc: Aurelien Jarno <aurelien@aurel32.net>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Emilio G. Cota <cota@braap.org>
---
 target/sh4/op_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Alex Bennée Oct. 31, 2018, 1:54 p.m. UTC | #1 
Emilio G. Cota <cota@braap.org> writes:

> Cc: Aurelien Jarno <aurelien@aurel32.net>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Emilio G. Cota <cota@braap.org>
> ---
>  target/sh4/op_helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/sh4/op_helper.c b/target/sh4/op_helper.c
> index 4f825bae5a..57cc363ccc 100644
> --- a/target/sh4/op_helper.c
> +++ b/target/sh4/op_helper.c
> @@ -105,7 +105,7 @@ void helper_sleep(CPUSH4State *env)
>  {
>      CPUState *cs = CPU(sh_env_get_cpu(env));
>
> -    cs->halted = 1;
> +    cpu_halted_set(cs, 1);

Looks good:

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

>      env->in_sleep = 1;

I wonder if you could drop env->in_sleep from CPUSH4State?
The test in superh_cpu_do_interrupt:

        if (do_irq && !env->in_sleep) {
            return; /* masked */
        }
    }
    env->in_sleep = 0;

maybe be simplified is we cpu_set_halted(cs, 0) when servicing a
delivered irq?

>      raise_exception(env, EXCP_HLT, 0);
>  }


--
Alex Bennée
Emilio Cota Oct. 31, 2018, 4:26 p.m. UTC | #2 
On Wed, Oct 31, 2018 at 13:54:14 +0000, Alex Bennée wrote:
> 
> Emilio G. Cota <cota@braap.org> writes:
> 
> > Cc: Aurelien Jarno <aurelien@aurel32.net>
> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> > Signed-off-by: Emilio G. Cota <cota@braap.org>
> > ---
> >  target/sh4/op_helper.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/sh4/op_helper.c b/target/sh4/op_helper.c
> > index 4f825bae5a..57cc363ccc 100644
> > --- a/target/sh4/op_helper.c
> > +++ b/target/sh4/op_helper.c
> > @@ -105,7 +105,7 @@ void helper_sleep(CPUSH4State *env)
> >  {
> >      CPUState *cs = CPU(sh_env_get_cpu(env));
> >
> > -    cs->halted = 1;
> > +    cpu_halted_set(cs, 1);
> 
> Looks good:
> 
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

Thanks!

> >      env->in_sleep = 1;
> 
> I wonder if you could drop env->in_sleep from CPUSH4State?
> The test in superh_cpu_do_interrupt:
> 
>         if (do_irq && !env->in_sleep) {
>             return; /* masked */
>         }
>     }
>     env->in_sleep = 0;
> 
> maybe be simplified is we cpu_set_halted(cs, 0) when servicing a
> delivered irq?

I don't know this target well, but on a quick look I'd worry about
that change interfering with other (generic) setters of
cpu->halted, e.g. cpu_common_reset, as well as the CPU loop
where cpu->halted is read.

In any case I'd leave additional cleanups out of this series,
which is already so long that git rebase is choking a bit
every time I add an R-b tag :D

Thanks,

		Emilio
Alex Bennée Oct. 31, 2018, 4:38 p.m. UTC | #3 
Emilio G. Cota <cota@braap.org> writes:

> On Wed, Oct 31, 2018 at 13:54:14 +0000, Alex Bennée wrote:
>>
>> Emilio G. Cota <cota@braap.org> writes:
>>
>> > Cc: Aurelien Jarno <aurelien@aurel32.net>
>> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> > Signed-off-by: Emilio G. Cota <cota@braap.org>
>> > ---
>> >  target/sh4/op_helper.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/target/sh4/op_helper.c b/target/sh4/op_helper.c
>> > index 4f825bae5a..57cc363ccc 100644
>> > --- a/target/sh4/op_helper.c
>> > +++ b/target/sh4/op_helper.c
>> > @@ -105,7 +105,7 @@ void helper_sleep(CPUSH4State *env)
>> >  {
>> >      CPUState *cs = CPU(sh_env_get_cpu(env));
>> >
>> > -    cs->halted = 1;
>> > +    cpu_halted_set(cs, 1);
>>
>> Looks good:
>>
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>
> Thanks!
>
>> >      env->in_sleep = 1;
>>
>> I wonder if you could drop env->in_sleep from CPUSH4State?
>> The test in superh_cpu_do_interrupt:
>>
>>         if (do_irq && !env->in_sleep) {
>>             return; /* masked */
>>         }
>>     }
>>     env->in_sleep = 0;
>>
>> maybe be simplified is we cpu_set_halted(cs, 0) when servicing a
>> delivered irq?
>
> I don't know this target well, but on a quick look I'd worry about
> that change interfering with other (generic) setters of
> cpu->halted, e.g. cpu_common_reset, as well as the CPU loop
> where cpu->halted is read.
>
> In any case I'd leave additional cleanups out of this series,
> which is already so long that git rebase is choking a bit
> every time I add an R-b tag :D

No worries, it was more a general query thrown in the direction of the
sh4 maintainers ;-)

--
Alex Bennée
diff mbox series

Patch

diff --git a/target/sh4/op_helper.c b/target/sh4/op_helper.c
index 4f825bae5a..57cc363ccc 100644
--- a/target/sh4/op_helper.c
+++ b/target/sh4/op_helper.c
@@ -105,7 +105,7 @@  void helper_sleep(CPUSH4State *env)
 {
     CPUState *cs = CPU(sh_env_get_cpu(env));
 
-    cs->halted = 1;
+    cpu_halted_set(cs, 1);
     env->in_sleep = 1;
     raise_exception(env, EXCP_HLT, 0);
 }