diff mbox

[Xen-devel,XEN/ARM,v6,1/1] Add OdroidXU board (Exynos 5410)

Message ID 1410440292.25388.19.camel@kazak.uk.xensource.com
State New
Headers show

Commit Message

Ian Campbell Sept. 11, 2014, 12:58 p.m. UTC
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.

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(-)

Comments

Suriyan Ramasami Sept. 11, 2014, 1:30 p.m. UTC | #1
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
>
>
>
Ian Campbell Sept. 11, 2014, 1:35 p.m. UTC | #2
On Thu, 2014-09-11 at 06:30 -0700, Suriyan Ramasami wrote:
> 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?

staging please, plus this patch I think.

(normally master is fine but in this case there are relevant changes in
the the master..staging range).
Julien Grall Sept. 11, 2014, 6:36 p.m. UTC | #3
Hi Ian,

On 11/09/14 05:58, Ian Campbell wrote:
> 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.
>
> Since arndale is used in automated tests I'm inclined towards applying
> this patch until a solution is found to detect the Odroid case.
> -----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>
Reviewed-by: Julien Grall <julien.grall@linaro.org>

Also I will give a look to the fix up patch ("Unbreak Arndale XEN Boot") 
today.

Regards,
Ian Campbell Sept. 12, 2014, 10:19 a.m. UTC | #4
On Thu, 2014-09-11 at 11:36 -0700, Julien Grall wrote:
> Hi Ian,
> 
> On 11/09/14 05:58, Ian Campbell wrote:
> > 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.
> >
> > Since arndale is used in automated tests I'm inclined towards applying
> > this patch until a solution is found to detect the Odroid case.
> > -----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>
> Reviewed-by: Julien Grall <julien.grall@linaro.org>
> 
> Also I will give a look to the fix up patch ("Unbreak Arndale XEN Boot") 
> today.

Thanks. In the meantime I have applied the fix to make arndale work
again.

Ian.
diff mbox

Patch

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);