| Message ID | 1410440292.25388.19.camel@kazak.uk.xensource.com |
|---|---|
| State | New |
| Headers | show |
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 > > >
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).
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,
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 --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);