| Message ID | 20181025144644.15464-18-cota@braap.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC,v4,01/71] cpu: convert queued work to a QSIMPLEQ | expand |
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
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
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 --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); }