[Qemu-devel] gic_cpu_write() call in exynos4210_gic_init()

[Qemu-devel] gic_cpu_write() call in exynos4210_gic_init()

[Peter Maydell]

Hi; I've been working on a refactoring of the ARM GIC code (trying
to make it its own sysbus device rather than having the .c file
included from lots of places), and I noticed the following odd code
in hw/exynos4210_gic.c:exynos4210_gic_init():

    gic_cpu_write(&s->gic, 1, 0, 1);

This seems to be trying to enable the CPU interface for CPU 1
(by directly calling the GIC function for writing a word to
that CPU interface). However, doing this in an init function
doesn't seem correct -- the write will change the GIC's internal
state but the change would be undone by a reset. (This happens
to work at the moment because we don't actually reset the GIC
at reset! However I intend to fix that bug...)

What was the intention of this code, and can we just drop it,
or do it in some other way? Does the Exynos GIC really reset
with the second CPU interface enabled?

My guess is that we either want a device property for CPU
interface state on reset (if the Exynos GIC resets differently
to the standard GIC), or this should be being done in the
arm_boot code if it's just trying to emulate behaviour of some
boot ROM...

thanks
-- PMM

Re: [Qemu-devel] gic_cpu_write() call in exynos4210_gic_init()

[Evgeny Voevodin]

On 03.04.2012 20:45, Peter Maydell wrote:
> Hi; I've been working on a refactoring of the ARM GIC code (trying
> to make it its own sysbus device rather than having the .c file
> included from lots of places), and I noticed the following odd code
> in hw/exynos4210_gic.c:exynos4210_gic_init():
>
>      gic_cpu_write(&s->gic, 1, 0, 1);
>
> This seems to be trying to enable the CPU interface for CPU 1
> (by directly calling the GIC function for writing a word to
> that CPU interface). However, doing this in an init function
> doesn't seem correct -- the write will change the GIC's internal
> state but the change would be undone by a reset. (This happens
> to work at the moment because we don't actually reset the GIC
> at reset! However I intend to fix that bug...)
>
> What was the intention of this code, and can we just drop it,
> or do it in some other way? Does the Exynos GIC really reset
> with the second CPU interface enabled?
>
> My guess is that we either want a device property for CPU
> interface state on reset (if the Exynos GIC resets differently
> to the standard GIC), or this should be being done in the
> arm_boot code if it's just trying to emulate behaviour of some
> boot ROM...
>
> thanks
> -- PMM
>
>
  Hi; Linux Kernel enables CPU0 during boot process (3.0 and earlier - 
in Internal GIC; 3.1 and later - in External GIC). CPU1 interface is 
enabled in secondary CPU bootloader by default, but only for Internal 
GIC. Since Kernel version 3.1 External GIC is used and it's CPU 
interface for CPU1 is not enabled. This call is a stub to perform, 
possibly, functionality of earliest SOC bootloader which seems to enable 
external GIC CPU1 interface (nobody saw the code of that). So, since new 
functionality of GIC is available, I think that obvious solution is to 
introduce Exynos-specific secondary CPU bootloader like in highbank, 
that would enable both External and Internal GIC CPU1 interfaces.
I will send patches soon. Thanks.

-- 
Kind regards,
Evgeny Voevodin,
Leading Software Engineer,
ASWG, Moscow R&D center, Samsung Electronics
e-mail: e.voevodin@samsung.com