Re: [PATCH v3 3/5] target/nios2: Exteral Interrupt Controller (EIC)

[Richard Henderson]

On 3/3/22 05:39, Amir Gonnen wrote:
> @@ -55,6 +55,7 @@ static void nios2_cpu_reset(DeviceState *dev)
>
>       memset(env->regs, 0, sizeof(uint32_t) * NUM_CORE_REGS);
>       memset(env->shadow_regs, 0, sizeof(uint32_t) * NUM_REG_SETS * 
> NUM_GP_REGS);
> +    env->regs[CR_STATUS] |= CR_STATUS_RSIE;

status is supposed to be reset to zero other than RSIE.

> @@ -131,13 +144,26 @@ static bool nios2_cpu_exec_interrupt(CPUState *cs, int 
> interrupt_request)
>       Nios2CPU *cpu = NIOS2_CPU(cs);
>       CPUNios2State *env = &cpu->env;
>
> -    if ((interrupt_request & CPU_INTERRUPT_HARD) &&
> -        (env->regs[CR_STATUS] & CR_STATUS_PIE) &&
> -        (env->regs[CR_IPENDING] & env->regs[CR_IENABLE])) {
> -        cs->exception_index = EXCP_IRQ;
> -        nios2_cpu_do_interrupt(cs);
> -        return true;
> +    if (cpu->intc_present) {
> +        if ((interrupt_request & CPU_INTERRUPT_HARD) &&
> +            (env->regs[CR_STATUS] & CR_STATUS_PIE) &&
> +            (env->regs[CR_IPENDING] & env->regs[CR_IENABLE])) {
> +            cs->exception_index = EXCP_IRQ;
> +            nios2_cpu_do_interrupt(cs);
> +            return true;
> +        }
> +    } else {
> +        /*
> +         * IPENDING does not exist with external interrupt controller
> +         * but we still use it to signal an external interrupt
> +         */
> +        if (env->regs[CR_IPENDING] && nios2_take_eic_irq(cpu)) {

Why CR_IPENDING?  The ipending register isn't supposed to exist with the EIC.  Did you in 
fact mean interrupt_request & CPU_INTERRUPT_HARD, as set by nios2_cpu_set_irq?


> -    /*
> -     * These interrupt lines model the IIC (internal interrupt
> -     * controller). QEMU does not currently support the EIC
> -     * (external interrupt controller) -- if we did it would be
> -     * a separate device in hw/intc with a custom interface to
> -     * the CPU, and boards using it would not wire up these IRQ lines.
> -     */

You should note that this is still used for EIC, though only IRQ[0].

There's a fair amount of checking cpu->intc_present, then doing two completely different 
things.  I'm thinking that it might be best to split these into two separate functions, 
and then set up the pointers properly.

You could in fact replace intc_present with two separate cpu classes (which is where many 
of those pointers are registered).  That would be early enough for the cpu_init hook to 
*not* register 32 interrupt lines for the EIC, as per above.


r~