Re: [XEN/ARM PATCH v6 1/1] Add OdroidXU board (Exynos 5410)

[Suriyan Ramasami <suriyan.r@gmail.com>]

On Mon, Sep 8, 2014 at 3:20 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Thu, 2014-09-04 at 15:57 -0700, Suriyan Ramasami wrote:
>>     XEN/ARM: Add support for the Odroid-XU board.
>>
>>     The Odroid-XU from hardkernel is an Exynos 5410 based board.
>>     This patch adds support to the above said board.
>
> Please could you describe a bit more fully the refactoring you've done
> and the new platform you've introduced. e.g. that we now have a general
> exynos5 (which currently includes only the 4210) and that the previous
> 5250 is a separate platform because of A, B, and C which differ from the
> generic.
>
> Assuming there are no other changes to the patch please feel free to
> just reply here with the updated commit text and I'll fold it in as I
> commit.
>
OK, and here it is ...

    Introduced a generic PLATFORM exynos5 which hopefully is
applicable to the majority of  exynos5 based SoCs. It currently has
only been tested on an exynos5410 based (OdroidXU) board and hence
that is the only board listed.

    Previously only the Arndale board, based on an exynos5250 was
supported. It was the only exynos based platform that was supported
and it was called exynos5. It has now been renamed to exynos5250. The
Arndale currently is a separate platform mostly cause I do not have
one to test and for the most part the code path for that board is
preserved. To be specific it varies from the generic implementation as
follows:
1. exynos5250 based specific DT mapping for CHIPID and PWM region. I
believe mainline kernel's DTS for the arndale has those mappings
already in place.
2. exynos5250 based cpu up code. It appears that exynos5250 already
has the secondary core powered up and in wfe and hence a
cpu_up_send_sgi suffices. Here too, I believe that the generic code
path might be acceptable.

Most of the code for the cpu bring up has been ported over from
mainline linux, and hence should be generic enough for future exynos
based SoCs. All reference to hardcoded memory locations have been
avoided. They are now gleaned from the device tree.

>> @@ -77,20 +125,127 @@ static int __init exynos5_smp_init(void)
>>
>>      printk("Set SYSRAM to %"PRIpaddr" (%p)\n",
>>             __pa(init_secondary), init_secondary);
>> -    writel(__pa(init_secondary), sysram);
>> +    writel(__pa(init_secondary), sysram + 0x1c);
>
> I think this 01c offset is new, where does it come from? Was the old
> code wrong (again, if this just needs a few words in the commit log I
> can fold them in as I commit).
>
This 0x1c comes from the Non Secure memory area that u-boot SPL moves
its code into. I believe this is the same for all exynos's (i believe
they have this same code pattern in lowlevel_init.S. This is what
u-boot copies over to start of IRAM_NS_BASE:
nscode_base:
        b       1f
        nop                             @ for backward compatibility

        .word   0x0                     @ REG0: RESUME_ADDR
        .word   0x0                     @ REG1: RESUME_FLAG
        .word   0x0                     @ REG2
        .word   0x0                     @ REG3
_switch_addr:
        .word   0x0                     @ REG4: SWITCH_ADDR
_hotplug_addr:
<------------------------------------------------------------- +0x1c
        .word   0x0                     @ REG5: CPU1_BOOT_REG
...
1: -> blah blah
        HYP entry ... blah blah

        adr     r0, _hotplug_addr
wait_for_addr:
        ldr     r1, [r0]
        cmp     r1, #0x0
        bxne    r1
        wfe
        b       wait_for_addr
...

As can be seen, the secondary CPUs check _hotplug_addr for a non zero
value on first being powered on, or after woken up from a wfe. This
_hotplug_addr happens to be at offset +0x1c from NS_RAM_BASE.
Linux mainline too has this hardcoded +0x1c.

>> +    static const struct dt_device_match exynos_dt_pmu_matches[] __initconst =
>> +    {
>> +        DT_MATCH_COMPATIBLE("samsung,exynos5250-pmu"),
>> +        DT_MATCH_COMPATIBLE("samsung,exynos5410-pmu"),
>> +        DT_MATCH_COMPATIBLE("samsung,exynos5420-pmu"),
>
> I suppose these are all similar enough for our purposes?
>
Yes, I could have just left it with the exynos5410-pmu and the
exynos5250-pmu, but then gathered the other compatible ones which
could be used for exynos5 generic boards in the future.

>> +        { /*sentinel*/ },
>> +    };
>> +
>> +    node = dt_find_matching_node(NULL, exynos_dt_pmu_matches);
>> +    if ( !node )
>> +    {
>> +        dprintk(XENLOG_ERR, "samsung,exynos5XXX-pmu missing in DT\n");
>
> We have some 4XXX in the list too. Make it exynosXXXX? (likewise the
> next message which was below but I've trimmed it by mistake).
>
I do not see any 4XXX in the pmu list that I have added. I think you
are referring the sysram compatible string which has exynos4 stuff in
it. And those dprintk the right string.

Same comment for the next message, but I see a typo where I have an
extra X in the exynos5XXXX instead of exynos5XXX

Please let me know if I should roll another patch with these comments
and the minor 'X' change.

Thanks!
> Ian.
>