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

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

On Thu, Sep 11, 2014 at 5:58 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2014-09-10 at 20:35 -0700, Suriyan Ramasami wrote:
>> On Wed, Sep 10, 2014 at 6:03 PM, Julien Grall <julien.grall@linaro.org> wrote:
>> > Hi Suriyan,
>> >
>> > On 10/09/14 17:51, Suriyan Ramasami wrote:
>> >>
>> >> On Wed, Sep 10, 2014 at 3:21 PM, Julien Grall <julien.grall@linaro.org>
>> >> wrote:
>> >>>
>> >>> On 08/09/14 10:26, Suriyan Ramasami wrote:
>> >>>>
>> >>>>
>> >>>> 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.
>> >>>
>> >>>
>> >>>
>> >>> You half read the Linux code... This offset is only add when there is a
>> >>> secure firmware (detected by "samsung,secure-firmware" node in the device
>> >>> tree).
>> >>>
>> >>> On the Arndale the node doesn't exist.
>> >>>
>> >> Thanks for digging there Julien. Previously, I must have gone through
>> >> the linux code with only exynos5410 in mind. Nonetheless, looks like I
>> >> have left out the code which handles the arndale and possibly 5420 and
>> >> the 5800 (if they do not have "samsung,secure-firmware" defined). But
>> >> on the other hand, I do see arndale-octa has the "secure-firmware"
>> >> entry which is a 5420 (so does the 5800). This adds to the confusion.
>> >> This is from looking at the linux-3.16.y source.
>> >>
>> >> Nonetheless, I think to handle arndale for now, I should add the
>> >> "samsung,secure-firmware" logic in the code which will then use
>> >> sysram_base_addr instead without any offset.
>> >>
>> >> So, how do I go about this? Should I roll out another patch with these
>> >> cumulative changes and also add the BUG_ON change?
>> >
>> >
>> > I think you could avoid to check the "samsung,secure-firmware" logic by only
>> > replicate arch/arm/mach-exynos/platsmp.c to Xen.
>> >
>>
>> Hi Julien,
>>    I think "samsung,secure-firmware" is what differentiates the code
>> path in platsmp.c, from what I can tell. Correct me if I am wrong,
>> please. This is from the smp_init's perspective in XEN.
>>
>> The code in linux is:
>>                 /*
>>                  * Try to set boot address using firmware first
>>                  * and fall back to boot register if it fails.
>>                  */
>>                 ret = call_firmware_op(set_cpu_boot_addr, phys_cpu, boot_addr);
>>                 if (ret && ret != -ENOSYS)
>>                         goto fail;
>>                 if (ret == -ENOSYS) {
>>                         void __iomem *boot_reg = cpu_boot_reg(phys_cpu);
>>
>>                         if (IS_ERR(boot_reg)) {
>>                                 ret = PTR_ERR(boot_reg);
>>                                 goto fail;
>>                         }
>>                         __raw_writel(boot_addr, cpu_boot_reg(phys_cpu));
>>                 }
>>
>> call_firmware_op(set_cpu_boot_addr, ...) will return -ENOSYS (which
>> then uses the alternative code path of using cpu_boot_reg(), which
>> uses sysram_base_addr), only if register_firmware_ops() is not called
>> with &exynos_firmware_ops, as per the code in mach-exynos/firmware.c.
>> Furthermore, it is not registered in exynos_firmware_init() only if
>> "samsung,secure-firmware" is not found in the DT.
>>
>> I am just wondering, if you had something else in mind which can be
>> achieved without depending on "samsung,secure-firmware". Please let me
>> know the alternative.
>
> I think what you suggest sounds plausible. For me the following fixes
> boot on my arndale (I mistakenly thought that it worked earlier but I
> was testing the wrong thing). If the presence of samsung,secure-firmware
> is the way to distinguish the two cases then lets go that way.
>

Hi Ian,
   A silly question ... The next patch that I send which possibly
handles Arndale and Odroid-XU, should that be based on master or
staging?

Thanks!
> Since arndale is used in automated tests I'm inclined towards applying
> this patch until a solution is found to detect the Odroid case.
>
> Ian.
>
> -----8<--------------
>
> From 24f74cbb981689b37d6bf83978c628a9fbabe246 Mon Sep 17 00:00:00 2001
> From: Ian Campbell <ian.campbell@citrix.com>
> Date: Thu, 11 Sep 2014 13:55:08 +0100
> Subject: [PATCH] xen: arm: fix boot on arndale.
>
> The differences between Arndale and the Odoid-XU are more interesting
> than first though, which results in 0bf8ddecb4df "xen/arm: Add
> support for the Odroid-XU board." breaking boot on arndale.
>
> Revert back to arndale compatible behaviour while we sort this out.
> Specifically we must (counterintuitively) use the regular (!ns)
> sysram and the correct offset is 0x0 and 0x1c.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/platforms/exynos5.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
> index bc9ae15..22a38f0 100644
> --- a/xen/arch/arm/platforms/exynos5.c
> +++ b/xen/arch/arm/platforms/exynos5.c
> @@ -99,7 +99,7 @@ static int __init exynos5_smp_init(void)
>      u64 size;
>      int rc;
>
> -    node = dt_find_compatible_node(NULL, NULL, "samsung,exynos4210-sysram-ns");
> +    node = dt_find_compatible_node(NULL, NULL, "samsung,exynos4210-sysram");
>      if ( !node )
>      {
>          dprintk(XENLOG_ERR, "samsung,exynos4210-sysram-ns missing in DT\n");
> @@ -125,7 +125,7 @@ static int __init exynos5_smp_init(void)
>
>      printk("Set SYSRAM to %"PRIpaddr" (%p)\n",
>             __pa(init_secondary), init_secondary);
> -    writel(__pa(init_secondary), sysram + 0x1c);
> +    writel(__pa(init_secondary), sysram);
>
>      iounmap(sysram);
>
> --
> 1.7.10.4
>
>
>