[Qemu-devel] Re: [PATCH 2/4] powerpc: Make the decr interrupt type overridable

[Alexander Graf]

Am 20.09.2010 um 14:11 schrieb "Edgar E. Iglesias" <address@hidden>:

> On Mon, Sep 20, 2010 at 12:42:41PM +0200, Alexander Graf wrote:
>> Edgar E. Iglesias wrote:
>>> Make it possible for boards to override the kind of interrupt
>>> to be signaled when the decr timer hits. The 405's signal PIT
>>> interrupts while the 440's signal DECR.
>>> 
>>> Signed-off-by: Edgar E. Iglesias <address@hidden>
>>> ---
>>> hw/ppc.c |   22 ++++++++++++++++++++--
>>> hw/ppc.h |    2 ++
>>> 2 files changed, 22 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/hw/ppc.c b/hw/ppc.c
>>> index 55e3808..3df7801 100644
>>> --- a/hw/ppc.c
>>> +++ b/hw/ppc.c
>>> @@ -769,6 +769,9 @@ struct ppcemb_timer_t {
>>>     struct QEMUTimer *fit_timer;
>>>     uint64_t wdt_next;    /* Tick for next WDT interrupt  */
>>>     struct QEMUTimer *wdt_timer;
>>> +
>>> +    /* 405 have the PIT, 440 have a DECR.  */
>>> +    unsigned int decr_excp;
>>> };
>>> 
>>> /* Fixed interval timer */
>>> @@ -851,7 +854,7 @@ static void cpu_4xx_pit_cb (void *opaque)
>>>     ppcemb_timer = tb_env->opaque;
>>>     env->spr[SPR_40x_TSR] |= 1 << 27;
>>>     if ((env->spr[SPR_40x_TCR] >> 26) & 0x1)
>>> 
>> 
>> Do these registers also apply to 440? If so, they should probably be
>> renamed to 4xx. Also while you're at it - I'd love to have readable
>> #define's for magic numbers :).
> 
> Sure, but it sounds to me like follow-up patches :)
> The code is already full with mixed use of 4xx and 40x.

I agree :).

> 
>>> -        ppc_set_irq(env, PPC_INTERRUPT_PIT, 1);
>>> +        ppc_set_irq(env, ppcemb_timer->decr_excp, 1);
>>>     start_stop_pit(env, tb_env, 1);
>>>     LOG_TB("%s: ar %d ir %d TCR " TARGET_FMT_lx " TSR " TARGET_FMT_lx " "
>>>            "%016" PRIx64 "\n", __func__,
>>> @@ -948,10 +951,15 @@ target_ulong load_40x_pit (CPUState *env)
>>> 
>>> void store_booke_tsr (CPUState *env, target_ulong val)
>>> {
>>> +    ppc_tb_t *tb_env = env->tb_env;
>>> +    ppcemb_timer_t *ppcemb_timer;
>>> +
>>> +    ppcemb_timer = tb_env->opaque;
>>> +
>>>     LOG_TB("%s: val " TARGET_FMT_lx "\n", __func__, val);
>>>     env->spr[SPR_40x_TSR] &= ~(val & 0xFC000000);
>>>     if (val & 0x80000000)
>>> -        ppc_set_irq(env, PPC_INTERRUPT_PIT, 0);
>>> +        ppc_set_irq(env, ppcemb_timer->decr_excp, 0);
>>> }
>>> 
>>> void store_booke_tcr (CPUState *env, target_ulong val)
>>> @@ -977,6 +985,15 @@ static void ppc_emb_set_tb_clk (void *opaque, uint32_t 
>>> freq)
>>>     /* XXX: we should also update all timers */
>>> }
>>> 
>>> +void ppc_emb_timers_set_decr_excp(CPUState *env, unsigned int excp)
>>> +{
>>> +    ppc_tb_t *tb_env = env->tb_env;
>>> +    ppcemb_timer_t *ppcemb_timer;
>>> +
>>> +    ppcemb_timer = tb_env->opaque;
>>> +    ppcemb_timer->decr_excp = excp;
>>> 
>> 
>> Why do you need this? Shouldn't the decrementor type be set by the CPU core?
> 
> Not the way things are modelled today. These blocks are indirectly
> instantiated by the boards. But lets make the decr_excp ...
> 
>> 
>>> +}
>>> +
>>> clk_setup_cb ppc_emb_timers_init (CPUState *env, uint32_t freq)
>>> {
>>>     ppc_tb_t *tb_env;
>>> @@ -996,6 +1013,7 @@ clk_setup_cb ppc_emb_timers_init (CPUState *env, 
>>> uint32_t freq)
>>>             qemu_new_timer(vm_clock, &cpu_4xx_fit_cb, env);
>>>         ppcemb_timer->wdt_timer =
>>>             qemu_new_timer(vm_clock, &cpu_4xx_wdt_cb, env);
>>> +        ppcemb_timer->decr_excp = PPC_INTERRUPT_PIT;
>>> 
>> 
>> If anything, it should be a parameter here.
> 
> ... an argument to ppc_emb_timers_init and ppc_emb_timers_set_decr_excp
> can go away. Does it sound good enough?

Perfect, yeah :)

Alex

> 
> Thanks