[XEN/ARM PATCH v1 1/1] Unbreak Arndale XEN boot

[XEN/ARM PATCH v1 1/1] Unbreak Arndale XEN boot

[Suriyan Ramasami]

XEN/ARM: Unbreak Arndale XEN boot

   For the Arndale board the secondary CPU kick address is in secure
   sysram and at offset 0. The Linux code differentiates by the existence
   of "secure-firmware". Arndale does not have one, and boards which use
   secure sysram at offset 0 as smp init address should not have one either.

   For other boards which do have a "secure-firmware" node, use sysram-ns
   at offset +0x1c as the smp init address.

   Have tested this on the Odroid XU. I have also tested the other code path
   on the Odroid XU by removing "secure-firmware" from its DT. I could see
   that the other code path was exercised with correct smp init address
   values.

---

Changes between versions as follows:
   v1:
       Unbreak Arndale XEN boot.

       Remove BUG_ON for checking the address offsets that we poke with.
       Instead use the size field from the DT to make valid comparisons.

       This patch is based off of the staging area as suggested by Ian.

Signed-off-by: Suriyan Ramasami <suriyan.r@gmail.com>
---
 xen/arch/arm/platforms/exynos5.c | 113 ++++++++++++++++++++++++++++++---------
 1 file changed, 88 insertions(+), 25 deletions(-)

diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
index bc9ae15..334650c 100644
--- a/xen/arch/arm/platforms/exynos5.c
+++ b/xen/arch/arm/platforms/exynos5.c
@@ -28,6 +28,9 @@
 #include <asm/platform.h>
 #include <asm/io.h>
 
+/* This corresponds to CONFIG_NR_CPUS in linux config */
+#define EXYNOS_CONFIG_NR_CPUS       0x08
+
 #define EXYNOS_ARM_CORE0_CONFIG     0x2000
 #define EXYNOS_ARM_CORE_CONFIG(_nr) (0x80 * (_nr))
 #define EXYNOS_ARM_CORE_STATUS(_nr) (EXYNOS_ARM_CORE_CONFIG(_nr) + 0x4)
@@ -42,8 +45,6 @@ static int exynos5_init_time(void)
     u64 mct_base_addr;
     u64 size;
 
-    BUILD_BUG_ON(EXYNOS5_MCT_G_TCON >= PAGE_SIZE);
-
     node = dt_find_compatible_node(NULL, NULL, "samsung,exynos4210-mct");
     if ( !node )
     {
@@ -61,7 +62,14 @@ static int exynos5_init_time(void)
     dprintk(XENLOG_INFO, "mct_base_addr: %016llx size: %016llx\n",
             mct_base_addr, size);
 
-    mct = ioremap_attr(mct_base_addr, PAGE_SIZE, PAGE_HYPERVISOR_NOCACHE);
+    if ( size <= EXYNOS5_MCT_G_TCON )
+    {
+        dprintk(XENLOG_ERR, "EXYNOS5_MCT_G_TCON: %d is >= size\n",
+                EXYNOS5_MCT_G_TCON);
+        return -EINVAL;
+    }
+
+    mct = ioremap_attr(mct_base_addr, size, PAGE_HYPERVISOR_NOCACHE);
     if ( !mct )
     {
         dprintk(XENLOG_ERR, "Unable to map MCT\n");
@@ -91,14 +99,36 @@ static int exynos5250_specific_mapping(struct domain *d)
     return 0;
 }
 
-static int __init exynos5_smp_init(void)
+static int exynos_smp_init_getbaseandoffset(u64 *sysram_addr,
+                                            u64 *sysram_offset)
 {
-    void __iomem *sysram;
     struct dt_device_node *node;
-    u64 sysram_ns_base_addr;
     u64 size;
     int rc;
 
+    node = dt_find_compatible_node(NULL, NULL, "samsung,secure-firmware");
+    if ( !node )
+    {
+        /* Have to use sysram_base_addr + offset 0 for boot address */
+        node = dt_find_compatible_node(NULL, NULL, "samsung,exynos4210-sysram");
+        if ( !node )
+        {
+            dprintk(XENLOG_ERR, "samsung,exynos4210-sysram missing in DT\n");
+            return -ENXIO;
+        }
+
+        rc = dt_device_get_address(node, 0, sysram_addr, &size);
+        if ( rc )
+        {
+            dprintk(XENLOG_ERR, "Error in \"samsung,exynos4210-sysram\"\n");
+            return -ENXIO;
+        }
+
+        *sysram_offset = 0;
+        return 0;
+    }
+
+    /* Have to use sysram_ns_base_addr + 0x1c for boot address */
     node = dt_find_compatible_node(NULL, NULL, "samsung,exynos4210-sysram-ns");
     if ( !node )
     {
@@ -106,17 +136,38 @@ static int __init exynos5_smp_init(void)
         return -ENXIO;
     }
 
-    rc = dt_device_get_address(node, 0, &sysram_ns_base_addr, &size);
+    rc = dt_device_get_address(node, 0, sysram_addr, &size);
     if ( rc )
     {
         dprintk(XENLOG_ERR, "Error in \"samsung,exynos4210-sysram-ns\"\n");
         return -ENXIO;
     }
 
-    dprintk(XENLOG_INFO, "sysram_ns_base_addr: %016llx size: %016llx\n",
-            sysram_ns_base_addr, size);
+    *sysram_offset = 0x1c;
+    return 0;
+}
+
+static int __init exynos5_smp_init(void)
+{
+    void __iomem *sysram;
+    u64 sysram_addr;
+    u64 sysram_offset;
+    int rc;
+
+    rc = exynos_smp_init_getbaseandoffset(&sysram_addr, &sysram_offset);
+    if ( rc )
+        return rc;
 
-    sysram = ioremap_nocache(sysram_ns_base_addr, PAGE_SIZE);
+    dprintk(XENLOG_INFO, "sysram_addr: %016llx offset: %016llx\n",
+            sysram_addr, sysram_offset);
+
+    if ( sysram_offset >= PAGE_SIZE )
+    {
+        dprintk(XENLOG_ERR, "sysram_offset is >= PAGE_SIZE\n");
+        return -EINVAL;
+    }
+
+    sysram = ioremap_nocache(sysram_addr, PAGE_SIZE);
     if ( !sysram )
     {
         dprintk(XENLOG_ERR, "Unable to map exynos5 MMIO\n");
@@ -125,7 +176,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 + sysram_offset);
 
     iounmap(sysram);
 
@@ -134,14 +185,14 @@ static int __init exynos5_smp_init(void)
 
 static int exynos_cpu_power_state(void __iomem *power, int cpu)
 {
-    return __raw_readl(power + EXYNOS_ARM_CORE_STATUS(cpu)) &
-                       S5P_CORE_LOCAL_PWR_EN;
+    return __raw_readl(power + EXYNOS_ARM_CORE0_CONFIG +
+                       EXYNOS_ARM_CORE_STATUS(cpu)) & S5P_CORE_LOCAL_PWR_EN;
 }
 
 static void exynos_cpu_power_up(void __iomem *power, int cpu)
 {
     __raw_writel(S5P_CORE_LOCAL_PWR_EN,
-                 power + EXYNOS_ARM_CORE_CONFIG(cpu));
+                 power + EXYNOS_ARM_CORE0_CONFIG + EXYNOS_ARM_CORE_CONFIG(cpu));
 }
 
 static int exynos5_cpu_power_up(void __iomem *power, int cpu)
@@ -171,8 +222,7 @@ static int exynos5_cpu_power_up(void __iomem *power, int cpu)
     return 0;
 }
 
-static int exynos5_get_pmu_base_addr(u64 *power_base_addr) {
-    u64 size;
+static int exynos5_get_pmu_baseandsize(u64 *power_base_addr, u64 *size) {
     struct dt_device_node *node;
     int rc;
     static const struct dt_device_match exynos_dt_pmu_matches[] __initconst =
@@ -190,7 +240,7 @@ static int exynos5_get_pmu_base_addr(u64 *power_base_addr) {
         return -ENXIO;
     }
 
-    rc = dt_device_get_address(node, 0, power_base_addr, &size);
+    rc = dt_device_get_address(node, 0, power_base_addr, size);
     if ( rc )
     {
         dprintk(XENLOG_ERR, "Error in \"samsung,exynos5XXX-pmu\"\n");
@@ -198,7 +248,7 @@ static int exynos5_get_pmu_base_addr(u64 *power_base_addr) {
     }
 
     dprintk(XENLOG_DEBUG, "power_base_addr: %016llx size: %016llx\n",
-            *power_base_addr, size);
+            *power_base_addr, *size);
 
     return 0;
 }
@@ -206,15 +256,22 @@ static int exynos5_get_pmu_base_addr(u64 *power_base_addr) {
 static int exynos5_cpu_up(int cpu)
 {
     u64 power_base_addr;
+    u64 size;
     void __iomem *power;
     int rc;
 
-    rc = exynos5_get_pmu_base_addr(&power_base_addr);
+    rc = exynos5_get_pmu_baseandsize(&power_base_addr, &size);
     if ( rc )
         return rc;
 
-    power = ioremap_nocache(power_base_addr +
-                            EXYNOS_ARM_CORE0_CONFIG, PAGE_SIZE);
+    if ( size <= EXYNOS_ARM_CORE0_CONFIG +
+                 EXYNOS_ARM_CORE_STATUS(EXYNOS_CONFIG_NR_CPUS) )
+    {
+        dprintk(XENLOG_ERR, "CORE registers fall outside of pmu range.\n");
+        return -EINVAL;
+    }
+
+    power = ioremap_nocache(power_base_addr, size);
     if ( !power )
     {
         dprintk(XENLOG_ERR, "Unable to map power MMIO\n");
@@ -236,16 +293,22 @@ static int exynos5_cpu_up(int cpu)
 static void exynos5_reset(void)
 {
     u64 power_base_addr;
+    u64 size;
     void __iomem *pmu;
     int rc;
 
-    BUILD_BUG_ON(EXYNOS5_SWRESET >= PAGE_SIZE);
-
-    rc = exynos5_get_pmu_base_addr(&power_base_addr);
+    rc = exynos5_get_pmu_baseandsize(&power_base_addr, &size);
     if ( rc )
         return;
 
-    pmu = ioremap_nocache(power_base_addr, PAGE_SIZE);
+    if ( size <= EXYNOS5_SWRESET )
+    {
+        dprintk(XENLOG_ERR, "EXYNOS5_SWRESET: %d is >= size\n",
+                EXYNOS5_SWRESET);
+        return;
+    }
+
+    pmu = ioremap_nocache(power_base_addr, size);
     if ( !pmu )
     {
         dprintk(XENLOG_ERR, "Unable to map PMU\n");
-- 
1.9.1

Re: [XEN/ARM PATCH v1 1/1] Unbreak Arndale XEN boot

[Julien Grall]

Hello Suriyan,

My email address is @linaro.org not @linaro.com. I nearly miss this 
patch because of this.

Depending if Ian apply his patch (see the conversation on 
"Odroid-XU..."), I would update the title and the message.

On 11/09/14 10:25, Suriyan Ramasami wrote:
> diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
> index bc9ae15..334650c 100644
> --- a/xen/arch/arm/platforms/exynos5.c
> +++ b/xen/arch/arm/platforms/exynos5.c
> @@ -28,6 +28,9 @@
>   #include <asm/platform.h>
>   #include <asm/io.h>
>
> +/* This corresponds to CONFIG_NR_CPUS in linux config */
> +#define EXYNOS_CONFIG_NR_CPUS       0x08
> +
>   #define EXYNOS_ARM_CORE0_CONFIG     0x2000
>   #define EXYNOS_ARM_CORE_CONFIG(_nr) (0x80 * (_nr))
>   #define EXYNOS_ARM_CORE_STATUS(_nr) (EXYNOS_ARM_CORE_CONFIG(_nr) + 0x4)
> @@ -42,8 +45,6 @@ static int exynos5_init_time(void)
>       u64 mct_base_addr;
>       u64 size;
>
> -    BUILD_BUG_ON(EXYNOS5_MCT_G_TCON >= PAGE_SIZE);
> -
>       node = dt_find_compatible_node(NULL, NULL, "samsung,exynos4210-mct");
>       if ( !node )
>       {
> @@ -61,7 +62,14 @@ static int exynos5_init_time(void)
>       dprintk(XENLOG_INFO, "mct_base_addr: %016llx size: %016llx\n",
>               mct_base_addr, size);
>
> -    mct = ioremap_attr(mct_base_addr, PAGE_SIZE, PAGE_HYPERVISOR_NOCACHE);
> +    if ( size <= EXYNOS5_MCT_G_TCON )

Honestly, I don't think this check is very useful. The device tree 
should always contains valid size.

> +    {
> +        dprintk(XENLOG_ERR, "EXYNOS5_MCT_G_TCON: %d is >= size\n",
> +                EXYNOS5_MCT_G_TCON);
> +        return -EINVAL;
> +    }
> +
> +    mct = ioremap_attr(mct_base_addr, size, PAGE_HYPERVISOR_NOCACHE);
>       if ( !mct )
>       {
>           dprintk(XENLOG_ERR, "Unable to map MCT\n");
> @@ -91,14 +99,36 @@ static int exynos5250_specific_mapping(struct domain *d)
>       return 0;
>   }
>
> -static int __init exynos5_smp_init(void)
> +static int exynos_smp_init_getbaseandoffset(u64 *sysram_addr,
> +                                            u64 *sysram_offset)
>   {

This function contains nearly twice the same code. Except the compatible 
string and the offset which differs, everything is the same.

I would do smth like:

node = dt_find_compatible_node(NULL, NULL, "samsung,secure-firmare");
if ( !node )
{
    compatible = "samsung,exynos4210-sysram";
    *sysram_offset = 0;
}
else
{
    compatible "samsung,exynos4210-sysram-ns";
    *sysram_offset = 0x1c;
}

node = dt_find_compatible_node(NULL, NULL, compatible);
if ( !node )
....

[..]

> +static int __init exynos5_smp_init(void)
> +{
> +    void __iomem *sysram;
> +    u64 sysram_addr;
> +    u64 sysram_offset;
> +    int rc;
> +
> +    rc = exynos_smp_init_getbaseandoffset(&sysram_addr, &sysram_offset);
> +    if ( rc )
> +        return rc;
>
> -    sysram = ioremap_nocache(sysram_ns_base_addr, PAGE_SIZE);
> +    dprintk(XENLOG_INFO, "sysram_addr: %016llx offset: %016llx\n",
> +            sysram_addr, sysram_offset);
> +
> +    if ( sysram_offset >= PAGE_SIZE )
> +    {
> +        dprintk(XENLOG_ERR, "sysram_offset is >= PAGE_SIZE\n");
> +        return -EINVAL;
> +    }

As the previous check for the MCT. I don't think it's useful. Also why 
don't do get the size from the device tree?

> +
> +    sysram = ioremap_nocache(sysram_addr, PAGE_SIZE);
>       if ( !sysram )
>       {
>           dprintk(XENLOG_ERR, "Unable to map exynos5 MMIO\n");
> @@ -125,7 +176,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 + sysram_offset);
>
>       iounmap(sysram);
>
> @@ -134,14 +185,14 @@ static int __init exynos5_smp_init(void)
>
>   static int exynos_cpu_power_state(void __iomem *power, int cpu)
>   {
> -    return __raw_readl(power + EXYNOS_ARM_CORE_STATUS(cpu)) &
> -                       S5P_CORE_LOCAL_PWR_EN;
> +    return __raw_readl(power + EXYNOS_ARM_CORE0_CONFIG +
> +                       EXYNOS_ARM_CORE_STATUS(cpu)) & S5P_CORE_LOCAL_PWR_EN;

Linux has the EXYNOS_ARM_CORE0_CONFIG included in the macro 
EXYNOS_CORE_CONFIGURATION. I would do the same here.

[..]

> -    power = ioremap_nocache(power_base_addr +
> -                            EXYNOS_ARM_CORE0_CONFIG, PAGE_SIZE);
> +    if ( size <= EXYNOS_ARM_CORE0_CONFIG +
> +                 EXYNOS_ARM_CORE_STATUS(EXYNOS_CONFIG_NR_CPUS) )
> +    {
> +        dprintk(XENLOG_ERR, "CORE registers fall outside of pmu range.\n");
> +        return -EINVAL;
> +    }
> +

Same remark as before for the check.

[..]

> -    pmu = ioremap_nocache(power_base_addr, PAGE_SIZE);
> +    if ( size <= EXYNOS5_SWRESET )
> +    {
> +        dprintk(XENLOG_ERR, "EXYNOS5_SWRESET: %d is >= size\n",
> +                EXYNOS5_SWRESET);
> +        return;
> +    }
> +

Same here too.

Regards,

-- 
Julien Grall

Re: [XEN/ARM PATCH v1 1/1] Unbreak Arndale XEN boot

[Suriyan Ramasami]

On Thu, Sep 11, 2014 at 11:49 AM, Julien Grall <julien.grall@linaro.org> wrote:
> Hello Suriyan,
>
> My email address is @linaro.org not @linaro.com. I nearly miss this patch
> because of this.
>
Sorry about that. A slip on my part. Apologies.

> Depending if Ian apply his patch (see the conversation on "Odroid-XU..."), I
> would update the title and the message.
>
>
Would the title change back to the previous XU patch - "Add Odroid-XU
board ..etc" with patch version 7 and modified message?

> On 11/09/14 10:25, Suriyan Ramasami wrote:
>>
>> diff --git a/xen/arch/arm/platforms/exynos5.c
>> b/xen/arch/arm/platforms/exynos5.c
>> index bc9ae15..334650c 100644
>> --- a/xen/arch/arm/platforms/exynos5.c
>> +++ b/xen/arch/arm/platforms/exynos5.c
>> @@ -28,6 +28,9 @@
>>   #include <asm/platform.h>
>>   #include <asm/io.h>
>>
>> +/* This corresponds to CONFIG_NR_CPUS in linux config */
>> +#define EXYNOS_CONFIG_NR_CPUS       0x08
>> +
>>   #define EXYNOS_ARM_CORE0_CONFIG     0x2000
>>   #define EXYNOS_ARM_CORE_CONFIG(_nr) (0x80 * (_nr))
>>   #define EXYNOS_ARM_CORE_STATUS(_nr) (EXYNOS_ARM_CORE_CONFIG(_nr) + 0x4)
>> @@ -42,8 +45,6 @@ static int exynos5_init_time(void)
>>       u64 mct_base_addr;
>>       u64 size;
>>
>> -    BUILD_BUG_ON(EXYNOS5_MCT_G_TCON >= PAGE_SIZE);
>> -
>>       node = dt_find_compatible_node(NULL, NULL,
>> "samsung,exynos4210-mct");
>>       if ( !node )
>>       {
>> @@ -61,7 +62,14 @@ static int exynos5_init_time(void)
>>       dprintk(XENLOG_INFO, "mct_base_addr: %016llx size: %016llx\n",
>>               mct_base_addr, size);
>>
>> -    mct = ioremap_attr(mct_base_addr, PAGE_SIZE,
>> PAGE_HYPERVISOR_NOCACHE);
>> +    if ( size <= EXYNOS5_MCT_G_TCON )
>
>
> Honestly, I don't think this check is very useful. The device tree should
> always contains valid size.
>
No doubt. But, wouldn't one have a wrong value for EXYNOS5_MCT_G_TCON?
I thought we are trying to catch wrong values of the offsets that we
use, to poke at the registers.

>> +    {
>> +        dprintk(XENLOG_ERR, "EXYNOS5_MCT_G_TCON: %d is >= size\n",
>> +                EXYNOS5_MCT_G_TCON);
>> +        return -EINVAL;
>> +    }
>> +
>> +    mct = ioremap_attr(mct_base_addr, size, PAGE_HYPERVISOR_NOCACHE);
>>       if ( !mct )
>>       {
>>           dprintk(XENLOG_ERR, "Unable to map MCT\n");
>> @@ -91,14 +99,36 @@ static int exynos5250_specific_mapping(struct domain
>> *d)
>>       return 0;
>>   }
>>
>> -static int __init exynos5_smp_init(void)
>> +static int exynos_smp_init_getbaseandoffset(u64 *sysram_addr,
>> +                                            u64 *sysram_offset)
>>   {
>
>
> This function contains nearly twice the same code. Except the compatible
> string and the offset which differs, everything is the same.
>
> I would do smth like:
>
> node = dt_find_compatible_node(NULL, NULL, "samsung,secure-firmare");
> if ( !node )
> {
>    compatible = "samsung,exynos4210-sysram";
>    *sysram_offset = 0;
> }
> else
> {
>    compatible "samsung,exynos4210-sysram-ns";
>    *sysram_offset = 0x1c;
> }
>
> node = dt_find_compatible_node(NULL, NULL, compatible);
> if ( !node )
> ....
>
> [..]
>
I shall make that change.

>> +static int __init exynos5_smp_init(void)
>> +{
>> +    void __iomem *sysram;
>> +    u64 sysram_addr;
>> +    u64 sysram_offset;
>> +    int rc;
>> +
>> +    rc = exynos_smp_init_getbaseandoffset(&sysram_addr, &sysram_offset);
>> +    if ( rc )
>> +        return rc;
>>
>> -    sysram = ioremap_nocache(sysram_ns_base_addr, PAGE_SIZE);
>> +    dprintk(XENLOG_INFO, "sysram_addr: %016llx offset: %016llx\n",
>> +            sysram_addr, sysram_offset);
>> +
>> +    if ( sysram_offset >= PAGE_SIZE )
>> +    {
>> +        dprintk(XENLOG_ERR, "sysram_offset is >= PAGE_SIZE\n");
>> +        return -EINVAL;
>> +    }
>
>
> As the previous check for the MCT. I don't think it's useful. Also why don't
> do get the size from the device tree?
>
In this case as we are poking only offset 0 or 0x1c which is always
less than PAGE_SIZE, I didn't bother with the size. I could if you
insist.
I can get rid of this check as sysram_offset is always less that PAGE_SIZE.

>> +
>> +    sysram = ioremap_nocache(sysram_addr, PAGE_SIZE);
>>       if ( !sysram )
>>       {
>>           dprintk(XENLOG_ERR, "Unable to map exynos5 MMIO\n");
>> @@ -125,7 +176,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 + sysram_offset);
>>
>>       iounmap(sysram);
>>
>> @@ -134,14 +185,14 @@ static int __init exynos5_smp_init(void)
>>
>>   static int exynos_cpu_power_state(void __iomem *power, int cpu)
>>   {
>> -    return __raw_readl(power + EXYNOS_ARM_CORE_STATUS(cpu)) &
>> -                       S5P_CORE_LOCAL_PWR_EN;
>> +    return __raw_readl(power + EXYNOS_ARM_CORE0_CONFIG +
>> +                       EXYNOS_ARM_CORE_STATUS(cpu)) &
>> S5P_CORE_LOCAL_PWR_EN;
>
>
> Linux has the EXYNOS_ARM_CORE0_CONFIG included in the macro
> EXYNOS_CORE_CONFIGURATION. I would do the same here.
>
OK, I will make that change.

> [..]
>
>> -    power = ioremap_nocache(power_base_addr +
>> -                            EXYNOS_ARM_CORE0_CONFIG, PAGE_SIZE);
>> +    if ( size <= EXYNOS_ARM_CORE0_CONFIG +
>> +                 EXYNOS_ARM_CORE_STATUS(EXYNOS_CONFIG_NR_CPUS) )
>> +    {
>> +        dprintk(XENLOG_ERR, "CORE registers fall outside of pmu
>> range.\n");
>> +        return -EINVAL;
>> +    }
>> +
>
>
> Same remark as before for the check.
>
My same response, in the sense that the size from DT will be correct,
but don't we have to make sure that the offsets that we calculate with
the #defines, fall within size?

> [..]
>
>> -    pmu = ioremap_nocache(power_base_addr, PAGE_SIZE);
>> +    if ( size <= EXYNOS5_SWRESET )
>> +    {
>> +        dprintk(XENLOG_ERR, "EXYNOS5_SWRESET: %d is >= size\n",
>> +                EXYNOS5_SWRESET);
>> +        return;
>> +    }
>> +
>
>
> Same here too.
My same response as before, that the EXYNOS5_SWRESET offset might be
miscalculated or quoted for a newer platform which falls beyond size
offset.

Please let me know if these checks are needed or not. I believe they
are good to keep, but if you insist I could leave them out.

Also, the next version of this patch, do I still maintain the same
subject and comments, or should this come up as something else?

Thanks as always
- Suriyan

>
> Regards,
>
> --
> Julien Grall

Re: [XEN/ARM PATCH v1 1/1] Unbreak Arndale XEN boot

[Ian Campbell]

On Thu, 2014-09-11 at 14:01 -0700, Suriyan Ramasami wrote:
> On Thu, Sep 11, 2014 at 11:49 AM, Julien Grall <julien.grall@linaro.org> wrote:
> > Hello Suriyan,
> >
> > My email address is @linaro.org not @linaro.com. I nearly miss this patch
> > because of this.
> >
> Sorry about that. A slip on my part. Apologies.
> 
> > Depending if Ian apply his patch (see the conversation on "Odroid-XU..."), I
> > would update the title and the message.
> >
> >
> Would the title change back to the previous XU patch - "Add Odroid-XU
> board ..etc" with patch version 7 and modified message?
> 

I think something like "xen: arm: add support for exynos secure
firmware" or something like that.

> > On 11/09/14 10:25, Suriyan Ramasami wrote:
> >>
> >> diff --git a/xen/arch/arm/platforms/exynos5.c
> >> b/xen/arch/arm/platforms/exynos5.c
> >> index bc9ae15..334650c 100644
> >> --- a/xen/arch/arm/platforms/exynos5.c
> >> +++ b/xen/arch/arm/platforms/exynos5.c
> >> @@ -28,6 +28,9 @@
> >>   #include <asm/platform.h>
> >>   #include <asm/io.h>
> >>
> >> +/* This corresponds to CONFIG_NR_CPUS in linux config */
> >> +#define EXYNOS_CONFIG_NR_CPUS       0x08
> >> +
> >>   #define EXYNOS_ARM_CORE0_CONFIG     0x2000
> >>   #define EXYNOS_ARM_CORE_CONFIG(_nr) (0x80 * (_nr))
> >>   #define EXYNOS_ARM_CORE_STATUS(_nr) (EXYNOS_ARM_CORE_CONFIG(_nr) + 0x4)
> >> @@ -42,8 +45,6 @@ static int exynos5_init_time(void)
> >>       u64 mct_base_addr;
> >>       u64 size;
> >>
> >> -    BUILD_BUG_ON(EXYNOS5_MCT_G_TCON >= PAGE_SIZE);
> >> -
> >>       node = dt_find_compatible_node(NULL, NULL,
> >> "samsung,exynos4210-mct");
> >>       if ( !node )
> >>       {
> >> @@ -61,7 +62,14 @@ static int exynos5_init_time(void)
> >>       dprintk(XENLOG_INFO, "mct_base_addr: %016llx size: %016llx\n",
> >>               mct_base_addr, size);
> >>
> >> -    mct = ioremap_attr(mct_base_addr, PAGE_SIZE,
> >> PAGE_HYPERVISOR_NOCACHE);
> >> +    if ( size <= EXYNOS5_MCT_G_TCON )
> >
> >
> > Honestly, I don't think this check is very useful. The device tree should
> > always contains valid size.
> >
> No doubt. But, wouldn't one have a wrong value for EXYNOS5_MCT_G_TCON?
> I thought we are trying to catch wrong values of the offsets that we
> use, to poke at the registers.

It's basically to catch buggy device tree, or perhaps buggy use of
compatible variables which don't guarantee that the registers which we
use exist.

But I'm mostly ambivalent about the inclusion of the check. I'm not sure
if we globally prefer one way or the other but if others feel that such
checks don't make sense lets not bother.

> >> +static int __init exynos5_smp_init(void)
> >> +{
> >> +    void __iomem *sysram;
> >> +    u64 sysram_addr;
> >> +    u64 sysram_offset;
> >> +    int rc;
> >> +
> >> +    rc = exynos_smp_init_getbaseandoffset(&sysram_addr, &sysram_offset);
> >> +    if ( rc )
> >> +        return rc;
> >>
> >> -    sysram = ioremap_nocache(sysram_ns_base_addr, PAGE_SIZE);
> >> +    dprintk(XENLOG_INFO, "sysram_addr: %016llx offset: %016llx\n",
> >> +            sysram_addr, sysram_offset);
> >> +
> >> +    if ( sysram_offset >= PAGE_SIZE )
> >> +    {
> >> +        dprintk(XENLOG_ERR, "sysram_offset is >= PAGE_SIZE\n");
> >> +        return -EINVAL;
> >> +    }
> >
> >
> > As the previous check for the MCT. I don't think it's useful. Also why don't
> > do get the size from the device tree?
> >
> In this case as we are poking only offset 0 or 0x1c which is always
> less than PAGE_SIZE, I didn't bother with the size. I could if you
> insist.
> I can get rid of this check as sysram_offset is always less that PAGE_SIZE.

You should certainly map the size given from DT and not just PAGE_SIZE.
That might mean the getter function returning the size, or perhaps just
have it establish the mapping and return the virtual address of the
sysram region.

> Also, the next version of this patch, do I still maintain the same
> subject and comments, or should this come up as something else?

I think we've agreed that a new subject is in order, not sure but I
think the bulk of the commit log is still appropriate.

A few procedural comments:

Please use "git format-patch" (or even git send-email) and not "git
show" to export the patch for sending, this will remove the unnecessary
indent. 

The email's subject line will be included into the commit message, so no
need to repeat it in the body. "git format-patch" will do the right
thing.

"Changes between versions" stuff should come after your S-o-b and a
"---" (on a line of its own) marker, which means that they won't get
included in the commit message.

Ian.

Re: [XEN/ARM PATCH v1 1/1] Unbreak Arndale XEN boot

[Suriyan Ramasami]

On Fri, Sep 12, 2014 at 3:08 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Thu, 2014-09-11 at 14:01 -0700, Suriyan Ramasami wrote:
>> On Thu, Sep 11, 2014 at 11:49 AM, Julien Grall <julien.grall@linaro.org> wrote:
>> > Hello Suriyan,
>> >
>> > My email address is @linaro.org not @linaro.com. I nearly miss this patch
>> > because of this.
>> >
>> Sorry about that. A slip on my part. Apologies.
>>
>> > Depending if Ian apply his patch (see the conversation on "Odroid-XU..."), I
>> > would update the title and the message.
>> >
>> >
>> Would the title change back to the previous XU patch - "Add Odroid-XU
>> board ..etc" with patch version 7 and modified message?
>>
>
> I think something like "xen: arm: add support for exynos secure
> firmware" or something like that.
>
OK.

>> > On 11/09/14 10:25, Suriyan Ramasami wrote:
>> >>
>> >> diff --git a/xen/arch/arm/platforms/exynos5.c
>> >> b/xen/arch/arm/platforms/exynos5.c
>> >> index bc9ae15..334650c 100644
>> >> --- a/xen/arch/arm/platforms/exynos5.c
>> >> +++ b/xen/arch/arm/platforms/exynos5.c
>> >> @@ -28,6 +28,9 @@
>> >>   #include <asm/platform.h>
>> >>   #include <asm/io.h>
>> >>
>> >> +/* This corresponds to CONFIG_NR_CPUS in linux config */
>> >> +#define EXYNOS_CONFIG_NR_CPUS       0x08
>> >> +
>> >>   #define EXYNOS_ARM_CORE0_CONFIG     0x2000
>> >>   #define EXYNOS_ARM_CORE_CONFIG(_nr) (0x80 * (_nr))
>> >>   #define EXYNOS_ARM_CORE_STATUS(_nr) (EXYNOS_ARM_CORE_CONFIG(_nr) + 0x4)
>> >> @@ -42,8 +45,6 @@ static int exynos5_init_time(void)
>> >>       u64 mct_base_addr;
>> >>       u64 size;
>> >>
>> >> -    BUILD_BUG_ON(EXYNOS5_MCT_G_TCON >= PAGE_SIZE);
>> >> -
>> >>       node = dt_find_compatible_node(NULL, NULL,
>> >> "samsung,exynos4210-mct");
>> >>       if ( !node )
>> >>       {
>> >> @@ -61,7 +62,14 @@ static int exynos5_init_time(void)
>> >>       dprintk(XENLOG_INFO, "mct_base_addr: %016llx size: %016llx\n",
>> >>               mct_base_addr, size);
>> >>
>> >> -    mct = ioremap_attr(mct_base_addr, PAGE_SIZE,
>> >> PAGE_HYPERVISOR_NOCACHE);
>> >> +    if ( size <= EXYNOS5_MCT_G_TCON )
>> >
>> >
>> > Honestly, I don't think this check is very useful. The device tree should
>> > always contains valid size.
>> >
>> No doubt. But, wouldn't one have a wrong value for EXYNOS5_MCT_G_TCON?
>> I thought we are trying to catch wrong values of the offsets that we
>> use, to poke at the registers.
>
> It's basically to catch buggy device tree, or perhaps buggy use of
> compatible variables which don't guarantee that the registers which we
> use exist.
>
> But I'm mostly ambivalent about the inclusion of the check. I'm not sure
> if we globally prefer one way or the other but if others feel that such
> checks don't make sense lets not bother.
>
OK, I shall get rid of the checks.

>> >> +static int __init exynos5_smp_init(void)
>> >> +{
>> >> +    void __iomem *sysram;
>> >> +    u64 sysram_addr;
>> >> +    u64 sysram_offset;
>> >> +    int rc;
>> >> +
>> >> +    rc = exynos_smp_init_getbaseandoffset(&sysram_addr, &sysram_offset);
>> >> +    if ( rc )
>> >> +        return rc;
>> >>
>> >> -    sysram = ioremap_nocache(sysram_ns_base_addr, PAGE_SIZE);
>> >> +    dprintk(XENLOG_INFO, "sysram_addr: %016llx offset: %016llx\n",
>> >> +            sysram_addr, sysram_offset);
>> >> +
>> >> +    if ( sysram_offset >= PAGE_SIZE )
>> >> +    {
>> >> +        dprintk(XENLOG_ERR, "sysram_offset is >= PAGE_SIZE\n");
>> >> +        return -EINVAL;
>> >> +    }
>> >
>> >
>> > As the previous check for the MCT. I don't think it's useful. Also why don't
>> > do get the size from the device tree?
>> >
>> In this case as we are poking only offset 0 or 0x1c which is always
>> less than PAGE_SIZE, I didn't bother with the size. I could if you
>> insist.
>> I can get rid of this check as sysram_offset is always less that PAGE_SIZE.
>
> You should certainly map the size given from DT and not just PAGE_SIZE.
> That might mean the getter function returning the size, or perhaps just
> have it establish the mapping and return the virtual address of the
> sysram region.
>
OK, shall map the full size as specified in the DT, and not PAGE_SIZE.

>> Also, the next version of this patch, do I still maintain the same
>> subject and comments, or should this come up as something else?
>
> I think we've agreed that a new subject is in order, not sure but I
> think the bulk of the commit log is still appropriate.
>
> A few procedural comments:
>
> Please use "git format-patch" (or even git send-email) and not "git
> show" to export the patch for sending, this will remove the unnecessary
> indent.
>
I actually use git format-patch to generate the patch, and I do edit
some of the comments in that patch file, before I use git send-email.
Can you please give me an example of the  "unnecessary indent" that
you mention, so I can check what I am doing wrong.

> The email's subject line will be included into the commit message, so no
> need to repeat it in the body. "git format-patch" will do the right
> thing.
>
> "Changes between versions" stuff should come after your S-o-b and a
> "---" (on a line of its own) marker, which means that they won't get
> included in the commit message.
>
Sorry about that. I did know that part (has been repeatedly mentioned
before). It was a mistake on my part, while editing the output of
format-patch, I manually copy and paste the version changes that I am
doing, and this time around I pasted it before the S-o-b. Is that not
the right approach?

> Ian.
>

Re: [XEN/ARM PATCH v1 1/1] Unbreak Arndale XEN boot

[Julien Grall]

Hi Suriyan,

On 12/09/14 10:50, Suriyan Ramasami wrote:
>> A few procedural comments:
>>
>> Please use "git format-patch" (or even git send-email) and not "git
>> show" to export the patch for sending, this will remove the unnecessary
>> indent.
>>
> I actually use git format-patch to generate the patch, and I do edit
> some of the comments in that patch file, before I use git send-email.
> Can you please give me an example of the  "unnecessary indent" that
> you mention, so I can check what I am doing wrong.

Each beginning of your commit message lines start by an indent. git 
format-patch should drop it by default.

Current you have smth like:

	foo
	base

This should be:

foo
base

>
>> The email's subject line will be included into the commit message, so no
>> need to repeat it in the body. "git format-patch" will do the right
>> thing.
>>
>> "Changes between versions" stuff should come after your S-o-b and a
>> "---" (on a line of its own) marker, which means that they won't get
>> included in the commit message.
>>
> Sorry about that. I did know that part (has been repeatedly mentioned
> before). It was a mistake on my part, while editing the output of
> format-patch, I manually copy and paste the version changes that I am
> doing, and this time around I pasted it before the S-o-b. Is that not
> the right approach?

The S-o-b can be generated directly when you generate the commit (git 
commit -s ...).

For the changes you can also add in your commit message directly. So you 
should not need to modify the patch generated by git format-patch.

Regards,

-- 
Julien Grall

Re: [XEN/ARM PATCH v1 1/1] Unbreak Arndale XEN boot

[Suriyan Ramasami]

On Fri, Sep 12, 2014 at 11:39 AM, Julien Grall <julien.grall@linaro.org> wrote:
> Hi Suriyan,
>
> On 12/09/14 10:50, Suriyan Ramasami wrote:
>>>
>>> A few procedural comments:
>>>
>>> Please use "git format-patch" (or even git send-email) and not "git
>>> show" to export the patch for sending, this will remove the unnecessary
>>> indent.
>>>
>> I actually use git format-patch to generate the patch, and I do edit
>> some of the comments in that patch file, before I use git send-email.
>> Can you please give me an example of the  "unnecessary indent" that
>> you mention, so I can check what I am doing wrong.
>
>
> Each beginning of your commit message lines start by an indent. git
> format-patch should drop it by default.
>
> Current you have smth like:
>
>         foo
>         base
>
> This should be:
>
> foo
> base
>
Ah! OK, now I get it !

>>
>>> The email's subject line will be included into the commit message, so no
>>> need to repeat it in the body. "git format-patch" will do the right
>>> thing.
>>>
>>> "Changes between versions" stuff should come after your S-o-b and a
>>> "---" (on a line of its own) marker, which means that they won't get
>>> included in the commit message.
>>>
>> Sorry about that. I did know that part (has been repeatedly mentioned
>> before). It was a mistake on my part, while editing the output of
>> format-patch, I manually copy and paste the version changes that I am
>> doing, and this time around I pasted it before the S-o-b. Is that not
>> the right approach?
>
>
> The S-o-b can be generated directly when you generate the commit (git commit
> -s ...).
>
> For the changes you can also add in your commit message directly. So you
> should not need to modify the patch generated by git format-patch.
>
Thanks for this note. I shall follow this process from now on and stop
messing with manually editing the patch file!

> Regards,
>
> --
> Julien Grall

Re: [XEN/ARM PATCH v1 1/1] Unbreak Arndale XEN boot

[Julien Grall]

Hi Suriyan,

A couple of remarks about the Samsung secure firmware and your patch.

When secure firmware is present, the CPU bring up is done by SMC (I 
don't see this code in your patch).

Also, for now, the secure firmware MMIO range is mapped to DOM0. I'm not 
sure we should do that because it contains way to idle CPU...
I think we should blacklist it for now. And see how DOM0 behave without.

Regards,

On 11/09/14 14:01, Suriyan Ramasami wrote:
> On Thu, Sep 11, 2014 at 11:49 AM, Julien Grall <julien.grall@linaro.org> wrote:
>> Hello Suriyan,
>>
>> My email address is @linaro.org not @linaro.com. I nearly miss this patch
>> because of this.
>>
> Sorry about that. A slip on my part. Apologies.
>
>> Depending if Ian apply his patch (see the conversation on "Odroid-XU..."), I
>> would update the title and the message.
>>
>>
> Would the title change back to the previous XU patch - "Add Odroid-XU
> board ..etc" with patch version 7 and modified message?
>
>> On 11/09/14 10:25, Suriyan Ramasami wrote:
>>>
>>> diff --git a/xen/arch/arm/platforms/exynos5.c
>>> b/xen/arch/arm/platforms/exynos5.c
>>> index bc9ae15..334650c 100644
>>> --- a/xen/arch/arm/platforms/exynos5.c
>>> +++ b/xen/arch/arm/platforms/exynos5.c
>>> @@ -28,6 +28,9 @@
>>>    #include <asm/platform.h>
>>>    #include <asm/io.h>
>>>
>>> +/* This corresponds to CONFIG_NR_CPUS in linux config */
>>> +#define EXYNOS_CONFIG_NR_CPUS       0x08
>>> +
>>>    #define EXYNOS_ARM_CORE0_CONFIG     0x2000
>>>    #define EXYNOS_ARM_CORE_CONFIG(_nr) (0x80 * (_nr))
>>>    #define EXYNOS_ARM_CORE_STATUS(_nr) (EXYNOS_ARM_CORE_CONFIG(_nr) + 0x4)
>>> @@ -42,8 +45,6 @@ static int exynos5_init_time(void)
>>>        u64 mct_base_addr;
>>>        u64 size;
>>>
>>> -    BUILD_BUG_ON(EXYNOS5_MCT_G_TCON >= PAGE_SIZE);
>>> -
>>>        node = dt_find_compatible_node(NULL, NULL,
>>> "samsung,exynos4210-mct");
>>>        if ( !node )
>>>        {
>>> @@ -61,7 +62,14 @@ static int exynos5_init_time(void)
>>>        dprintk(XENLOG_INFO, "mct_base_addr: %016llx size: %016llx\n",
>>>                mct_base_addr, size);
>>>
>>> -    mct = ioremap_attr(mct_base_addr, PAGE_SIZE,
>>> PAGE_HYPERVISOR_NOCACHE);
>>> +    if ( size <= EXYNOS5_MCT_G_TCON )
>>
>>
>> Honestly, I don't think this check is very useful. The device tree should
>> always contains valid size.
>>
> No doubt. But, wouldn't one have a wrong value for EXYNOS5_MCT_G_TCON?
> I thought we are trying to catch wrong values of the offsets that we
> use, to poke at the registers.
>
>>> +    {
>>> +        dprintk(XENLOG_ERR, "EXYNOS5_MCT_G_TCON: %d is >= size\n",
>>> +                EXYNOS5_MCT_G_TCON);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    mct = ioremap_attr(mct_base_addr, size, PAGE_HYPERVISOR_NOCACHE);
>>>        if ( !mct )
>>>        {
>>>            dprintk(XENLOG_ERR, "Unable to map MCT\n");
>>> @@ -91,14 +99,36 @@ static int exynos5250_specific_mapping(struct domain
>>> *d)
>>>        return 0;
>>>    }
>>>
>>> -static int __init exynos5_smp_init(void)
>>> +static int exynos_smp_init_getbaseandoffset(u64 *sysram_addr,
>>> +                                            u64 *sysram_offset)
>>>    {
>>
>>
>> This function contains nearly twice the same code. Except the compatible
>> string and the offset which differs, everything is the same.
>>
>> I would do smth like:
>>
>> node = dt_find_compatible_node(NULL, NULL, "samsung,secure-firmare");
>> if ( !node )
>> {
>>     compatible = "samsung,exynos4210-sysram";
>>     *sysram_offset = 0;
>> }
>> else
>> {
>>     compatible "samsung,exynos4210-sysram-ns";
>>     *sysram_offset = 0x1c;
>> }
>>
>> node = dt_find_compatible_node(NULL, NULL, compatible);
>> if ( !node )
>> ....
>>
>> [..]
>>
> I shall make that change.
>
>>> +static int __init exynos5_smp_init(void)
>>> +{
>>> +    void __iomem *sysram;
>>> +    u64 sysram_addr;
>>> +    u64 sysram_offset;
>>> +    int rc;
>>> +
>>> +    rc = exynos_smp_init_getbaseandoffset(&sysram_addr, &sysram_offset);
>>> +    if ( rc )
>>> +        return rc;
>>>
>>> -    sysram = ioremap_nocache(sysram_ns_base_addr, PAGE_SIZE);
>>> +    dprintk(XENLOG_INFO, "sysram_addr: %016llx offset: %016llx\n",
>>> +            sysram_addr, sysram_offset);
>>> +
>>> +    if ( sysram_offset >= PAGE_SIZE )
>>> +    {
>>> +        dprintk(XENLOG_ERR, "sysram_offset is >= PAGE_SIZE\n");
>>> +        return -EINVAL;
>>> +    }
>>
>>
>> As the previous check for the MCT. I don't think it's useful. Also why don't
>> do get the size from the device tree?
>>
> In this case as we are poking only offset 0 or 0x1c which is always
> less than PAGE_SIZE, I didn't bother with the size. I could if you
> insist.
> I can get rid of this check as sysram_offset is always less that PAGE_SIZE.
>
>>> +
>>> +    sysram = ioremap_nocache(sysram_addr, PAGE_SIZE);
>>>        if ( !sysram )
>>>        {
>>>            dprintk(XENLOG_ERR, "Unable to map exynos5 MMIO\n");
>>> @@ -125,7 +176,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 + sysram_offset);
>>>
>>>        iounmap(sysram);
>>>
>>> @@ -134,14 +185,14 @@ static int __init exynos5_smp_init(void)
>>>
>>>    static int exynos_cpu_power_state(void __iomem *power, int cpu)
>>>    {
>>> -    return __raw_readl(power + EXYNOS_ARM_CORE_STATUS(cpu)) &
>>> -                       S5P_CORE_LOCAL_PWR_EN;
>>> +    return __raw_readl(power + EXYNOS_ARM_CORE0_CONFIG +
>>> +                       EXYNOS_ARM_CORE_STATUS(cpu)) &
>>> S5P_CORE_LOCAL_PWR_EN;
>>
>>
>> Linux has the EXYNOS_ARM_CORE0_CONFIG included in the macro
>> EXYNOS_CORE_CONFIGURATION. I would do the same here.
>>
> OK, I will make that change.
>
>> [..]
>>
>>> -    power = ioremap_nocache(power_base_addr +
>>> -                            EXYNOS_ARM_CORE0_CONFIG, PAGE_SIZE);
>>> +    if ( size <= EXYNOS_ARM_CORE0_CONFIG +
>>> +                 EXYNOS_ARM_CORE_STATUS(EXYNOS_CONFIG_NR_CPUS) )
>>> +    {
>>> +        dprintk(XENLOG_ERR, "CORE registers fall outside of pmu
>>> range.\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>
>>
>> Same remark as before for the check.
>>
> My same response, in the sense that the size from DT will be correct,
> but don't we have to make sure that the offsets that we calculate with
> the #defines, fall within size?
>
>> [..]
>>
>>> -    pmu = ioremap_nocache(power_base_addr, PAGE_SIZE);
>>> +    if ( size <= EXYNOS5_SWRESET )
>>> +    {
>>> +        dprintk(XENLOG_ERR, "EXYNOS5_SWRESET: %d is >= size\n",
>>> +                EXYNOS5_SWRESET);
>>> +        return;
>>> +    }
>>> +
>>
>>
>> Same here too.
> My same response as before, that the EXYNOS5_SWRESET offset might be
> miscalculated or quoted for a newer platform which falls beyond size
> offset.
>
> Please let me know if these checks are needed or not. I believe they
> are good to keep, but if you insist I could leave them out.
>
> Also, the next version of this patch, do I still maintain the same
> subject and comments, or should this come up as something else?
>
> Thanks as always
> - Suriyan
>
>>
>> Regards,
>>
>> --
>> Julien Grall

-- 
Julien Grall

Re: [XEN/ARM PATCH v1 1/1] Unbreak Arndale XEN boot

[Suriyan Ramasami]

On Fri, Sep 12, 2014 at 11:58 AM, Julien Grall <julien.grall@linaro.org> wrote:
> Hi Suriyan,
>
> A couple of remarks about the Samsung secure firmware and your patch.
>
> When secure firmware is present, the CPU bring up is done by SMC (I don't
> see this code in your patch).
>
Yes you are right. A call to call_firmware_op(cpu_boot, phys_cpu)
translating to exynos_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0) is missing
right before the call to waking up the  cpu.
This was intentional on my part as the trustzone blob that comes with
the odroid xu does not have code for that smc call and just returns
-1.

 I am guessing as we want this as generic as posisble, I should add
that part in.

> Also, for now, the secure firmware MMIO range is mapped to DOM0. I'm not
> sure we should do that because it contains way to idle CPU...
> I think we should blacklist it for now. And see how DOM0 behave without.
>
I shall try this on the Odroid XU.

Thanks!
 - Suriyan

> Regards,
>
>
> On 11/09/14 14:01, Suriyan Ramasami wrote:
>>
>> On Thu, Sep 11, 2014 at 11:49 AM, Julien Grall <julien.grall@linaro.org>
>> wrote:
>>>
>>> Hello Suriyan,
>>>
>>> My email address is @linaro.org not @linaro.com. I nearly miss this patch
>>> because of this.
>>>
>> Sorry about that. A slip on my part. Apologies.
>>
>>> Depending if Ian apply his patch (see the conversation on
>>> "Odroid-XU..."), I
>>> would update the title and the message.
>>>
>>>
>> Would the title change back to the previous XU patch - "Add Odroid-XU
>> board ..etc" with patch version 7 and modified message?
>>
>>> On 11/09/14 10:25, Suriyan Ramasami wrote:
>>>>
>>>>
>>>> diff --git a/xen/arch/arm/platforms/exynos5.c
>>>> b/xen/arch/arm/platforms/exynos5.c
>>>> index bc9ae15..334650c 100644
>>>> --- a/xen/arch/arm/platforms/exynos5.c
>>>> +++ b/xen/arch/arm/platforms/exynos5.c
>>>> @@ -28,6 +28,9 @@
>>>>    #include <asm/platform.h>
>>>>    #include <asm/io.h>
>>>>
>>>> +/* This corresponds to CONFIG_NR_CPUS in linux config */
>>>> +#define EXYNOS_CONFIG_NR_CPUS       0x08
>>>> +
>>>>    #define EXYNOS_ARM_CORE0_CONFIG     0x2000
>>>>    #define EXYNOS_ARM_CORE_CONFIG(_nr) (0x80 * (_nr))
>>>>    #define EXYNOS_ARM_CORE_STATUS(_nr) (EXYNOS_ARM_CORE_CONFIG(_nr) +
>>>> 0x4)
>>>> @@ -42,8 +45,6 @@ static int exynos5_init_time(void)
>>>>        u64 mct_base_addr;
>>>>        u64 size;
>>>>
>>>> -    BUILD_BUG_ON(EXYNOS5_MCT_G_TCON >= PAGE_SIZE);
>>>> -
>>>>        node = dt_find_compatible_node(NULL, NULL,
>>>> "samsung,exynos4210-mct");
>>>>        if ( !node )
>>>>        {
>>>> @@ -61,7 +62,14 @@ static int exynos5_init_time(void)
>>>>        dprintk(XENLOG_INFO, "mct_base_addr: %016llx size: %016llx\n",
>>>>                mct_base_addr, size);
>>>>
>>>> -    mct = ioremap_attr(mct_base_addr, PAGE_SIZE,
>>>> PAGE_HYPERVISOR_NOCACHE);
>>>> +    if ( size <= EXYNOS5_MCT_G_TCON )
>>>
>>>
>>>
>>> Honestly, I don't think this check is very useful. The device tree should
>>> always contains valid size.
>>>
>> No doubt. But, wouldn't one have a wrong value for EXYNOS5_MCT_G_TCON?
>> I thought we are trying to catch wrong values of the offsets that we
>> use, to poke at the registers.
>>
>>>> +    {
>>>> +        dprintk(XENLOG_ERR, "EXYNOS5_MCT_G_TCON: %d is >= size\n",
>>>> +                EXYNOS5_MCT_G_TCON);
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    mct = ioremap_attr(mct_base_addr, size, PAGE_HYPERVISOR_NOCACHE);
>>>>        if ( !mct )
>>>>        {
>>>>            dprintk(XENLOG_ERR, "Unable to map MCT\n");
>>>> @@ -91,14 +99,36 @@ static int exynos5250_specific_mapping(struct domain
>>>> *d)
>>>>        return 0;
>>>>    }
>>>>
>>>> -static int __init exynos5_smp_init(void)
>>>> +static int exynos_smp_init_getbaseandoffset(u64 *sysram_addr,
>>>> +                                            u64 *sysram_offset)
>>>>    {
>>>
>>>
>>>
>>> This function contains nearly twice the same code. Except the compatible
>>> string and the offset which differs, everything is the same.
>>>
>>> I would do smth like:
>>>
>>> node = dt_find_compatible_node(NULL, NULL, "samsung,secure-firmare");
>>> if ( !node )
>>> {
>>>     compatible = "samsung,exynos4210-sysram";
>>>     *sysram_offset = 0;
>>> }
>>> else
>>> {
>>>     compatible "samsung,exynos4210-sysram-ns";
>>>     *sysram_offset = 0x1c;
>>> }
>>>
>>> node = dt_find_compatible_node(NULL, NULL, compatible);
>>> if ( !node )
>>> ....
>>>
>>> [..]
>>>
>> I shall make that change.
>>
>>>> +static int __init exynos5_smp_init(void)
>>>> +{
>>>> +    void __iomem *sysram;
>>>> +    u64 sysram_addr;
>>>> +    u64 sysram_offset;
>>>> +    int rc;
>>>> +
>>>> +    rc = exynos_smp_init_getbaseandoffset(&sysram_addr,
>>>> &sysram_offset);
>>>> +    if ( rc )
>>>> +        return rc;
>>>>
>>>> -    sysram = ioremap_nocache(sysram_ns_base_addr, PAGE_SIZE);
>>>> +    dprintk(XENLOG_INFO, "sysram_addr: %016llx offset: %016llx\n",
>>>> +            sysram_addr, sysram_offset);
>>>> +
>>>> +    if ( sysram_offset >= PAGE_SIZE )
>>>> +    {
>>>> +        dprintk(XENLOG_ERR, "sysram_offset is >= PAGE_SIZE\n");
>>>> +        return -EINVAL;
>>>> +    }
>>>
>>>
>>>
>>> As the previous check for the MCT. I don't think it's useful. Also why
>>> don't
>>> do get the size from the device tree?
>>>
>> In this case as we are poking only offset 0 or 0x1c which is always
>> less than PAGE_SIZE, I didn't bother with the size. I could if you
>> insist.
>> I can get rid of this check as sysram_offset is always less that
>> PAGE_SIZE.
>>
>>>> +
>>>> +    sysram = ioremap_nocache(sysram_addr, PAGE_SIZE);
>>>>        if ( !sysram )
>>>>        {
>>>>            dprintk(XENLOG_ERR, "Unable to map exynos5 MMIO\n");
>>>> @@ -125,7 +176,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 + sysram_offset);
>>>>
>>>>        iounmap(sysram);
>>>>
>>>> @@ -134,14 +185,14 @@ static int __init exynos5_smp_init(void)
>>>>
>>>>    static int exynos_cpu_power_state(void __iomem *power, int cpu)
>>>>    {
>>>> -    return __raw_readl(power + EXYNOS_ARM_CORE_STATUS(cpu)) &
>>>> -                       S5P_CORE_LOCAL_PWR_EN;
>>>> +    return __raw_readl(power + EXYNOS_ARM_CORE0_CONFIG +
>>>> +                       EXYNOS_ARM_CORE_STATUS(cpu)) &
>>>> S5P_CORE_LOCAL_PWR_EN;
>>>
>>>
>>>
>>> Linux has the EXYNOS_ARM_CORE0_CONFIG included in the macro
>>> EXYNOS_CORE_CONFIGURATION. I would do the same here.
>>>
>> OK, I will make that change.
>>
>>> [..]
>>>
>>>> -    power = ioremap_nocache(power_base_addr +
>>>> -                            EXYNOS_ARM_CORE0_CONFIG, PAGE_SIZE);
>>>> +    if ( size <= EXYNOS_ARM_CORE0_CONFIG +
>>>> +                 EXYNOS_ARM_CORE_STATUS(EXYNOS_CONFIG_NR_CPUS) )
>>>> +    {
>>>> +        dprintk(XENLOG_ERR, "CORE registers fall outside of pmu
>>>> range.\n");
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>
>>>
>>>
>>> Same remark as before for the check.
>>>
>> My same response, in the sense that the size from DT will be correct,
>> but don't we have to make sure that the offsets that we calculate with
>> the #defines, fall within size?
>>
>>> [..]
>>>
>>>> -    pmu = ioremap_nocache(power_base_addr, PAGE_SIZE);
>>>> +    if ( size <= EXYNOS5_SWRESET )
>>>> +    {
>>>> +        dprintk(XENLOG_ERR, "EXYNOS5_SWRESET: %d is >= size\n",
>>>> +                EXYNOS5_SWRESET);
>>>> +        return;
>>>> +    }
>>>> +
>>>
>>>
>>>
>>> Same here too.
>>
>> My same response as before, that the EXYNOS5_SWRESET offset might be
>> miscalculated or quoted for a newer platform which falls beyond size
>> offset.
>>
>> Please let me know if these checks are needed or not. I believe they
>> are good to keep, but if you insist I could leave them out.
>>
>> Also, the next version of this patch, do I still maintain the same
>> subject and comments, or should this come up as something else?
>>
>> Thanks as always
>> - Suriyan
>>
>>>
>>> Regards,
>>>
>>> --
>>> Julien Grall
>
>
> --
> Julien Grall

Re: [XEN/ARM PATCH v1 1/1] Unbreak Arndale XEN boot

[Suriyan Ramasami]

On Fri, Sep 12, 2014 at 12:34 PM, Suriyan Ramasami <suriyan.r@gmail.com> wrote:
> On Fri, Sep 12, 2014 at 11:58 AM, Julien Grall <julien.grall@linaro.org> wrote:
>> Hi Suriyan,
>>
>> A couple of remarks about the Samsung secure firmware and your patch.
>>
>> When secure firmware is present, the CPU bring up is done by SMC (I don't
>> see this code in your patch).
>>
> Yes you are right. A call to call_firmware_op(cpu_boot, phys_cpu)
> translating to exynos_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0) is missing
> right before the call to waking up the  cpu.
> This was intentional on my part as the trustzone blob that comes with
> the odroid xu does not have code for that smc call and just returns
> -1.
>
>  I am guessing as we want this as generic as posisble, I should add
> that part in.
>
>> Also, for now, the secure firmware MMIO range is mapped to DOM0. I'm not
>> sure we should do that because it contains way to idle CPU...
>> I think we should blacklist it for now. And see how DOM0 behave without.
>>
> I shall try this on the Odroid XU.
>

Hello Julien,
   I blacklisted "samsung,secure-firmware" and I can't perceive any
change in DOM0's behavior in the Odroid XU. Do you recommend, I send
the next patch with this blacklisted?

- Suriyan

> Thanks!
>  - Suriyan
>
>> Regards,
>>
>>
>> On 11/09/14 14:01, Suriyan Ramasami wrote:
>>>
>>> On Thu, Sep 11, 2014 at 11:49 AM, Julien Grall <julien.grall@linaro.org>
>>> wrote:
>>>>
>>>> Hello Suriyan,
>>>>
>>>> My email address is @linaro.org not @linaro.com. I nearly miss this patch
>>>> because of this.
>>>>
>>> Sorry about that. A slip on my part. Apologies.
>>>
>>>> Depending if Ian apply his patch (see the conversation on
>>>> "Odroid-XU..."), I
>>>> would update the title and the message.
>>>>
>>>>
>>> Would the title change back to the previous XU patch - "Add Odroid-XU
>>> board ..etc" with patch version 7 and modified message?
>>>
>>>> On 11/09/14 10:25, Suriyan Ramasami wrote:
>>>>>
>>>>>
>>>>> diff --git a/xen/arch/arm/platforms/exynos5.c
>>>>> b/xen/arch/arm/platforms/exynos5.c
>>>>> index bc9ae15..334650c 100644
>>>>> --- a/xen/arch/arm/platforms/exynos5.c
>>>>> +++ b/xen/arch/arm/platforms/exynos5.c
>>>>> @@ -28,6 +28,9 @@
>>>>>    #include <asm/platform.h>
>>>>>    #include <asm/io.h>
>>>>>
>>>>> +/* This corresponds to CONFIG_NR_CPUS in linux config */
>>>>> +#define EXYNOS_CONFIG_NR_CPUS       0x08
>>>>> +
>>>>>    #define EXYNOS_ARM_CORE0_CONFIG     0x2000
>>>>>    #define EXYNOS_ARM_CORE_CONFIG(_nr) (0x80 * (_nr))
>>>>>    #define EXYNOS_ARM_CORE_STATUS(_nr) (EXYNOS_ARM_CORE_CONFIG(_nr) +
>>>>> 0x4)
>>>>> @@ -42,8 +45,6 @@ static int exynos5_init_time(void)
>>>>>        u64 mct_base_addr;
>>>>>        u64 size;
>>>>>
>>>>> -    BUILD_BUG_ON(EXYNOS5_MCT_G_TCON >= PAGE_SIZE);
>>>>> -
>>>>>        node = dt_find_compatible_node(NULL, NULL,
>>>>> "samsung,exynos4210-mct");
>>>>>        if ( !node )
>>>>>        {
>>>>> @@ -61,7 +62,14 @@ static int exynos5_init_time(void)
>>>>>        dprintk(XENLOG_INFO, "mct_base_addr: %016llx size: %016llx\n",
>>>>>                mct_base_addr, size);
>>>>>
>>>>> -    mct = ioremap_attr(mct_base_addr, PAGE_SIZE,
>>>>> PAGE_HYPERVISOR_NOCACHE);
>>>>> +    if ( size <= EXYNOS5_MCT_G_TCON )
>>>>
>>>>
>>>>
>>>> Honestly, I don't think this check is very useful. The device tree should
>>>> always contains valid size.
>>>>
>>> No doubt. But, wouldn't one have a wrong value for EXYNOS5_MCT_G_TCON?
>>> I thought we are trying to catch wrong values of the offsets that we
>>> use, to poke at the registers.
>>>
>>>>> +    {
>>>>> +        dprintk(XENLOG_ERR, "EXYNOS5_MCT_G_TCON: %d is >= size\n",
>>>>> +                EXYNOS5_MCT_G_TCON);
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +
>>>>> +    mct = ioremap_attr(mct_base_addr, size, PAGE_HYPERVISOR_NOCACHE);
>>>>>        if ( !mct )
>>>>>        {
>>>>>            dprintk(XENLOG_ERR, "Unable to map MCT\n");
>>>>> @@ -91,14 +99,36 @@ static int exynos5250_specific_mapping(struct domain
>>>>> *d)
>>>>>        return 0;
>>>>>    }
>>>>>
>>>>> -static int __init exynos5_smp_init(void)
>>>>> +static int exynos_smp_init_getbaseandoffset(u64 *sysram_addr,
>>>>> +                                            u64 *sysram_offset)
>>>>>    {
>>>>
>>>>
>>>>
>>>> This function contains nearly twice the same code. Except the compatible
>>>> string and the offset which differs, everything is the same.
>>>>
>>>> I would do smth like:
>>>>
>>>> node = dt_find_compatible_node(NULL, NULL, "samsung,secure-firmare");
>>>> if ( !node )
>>>> {
>>>>     compatible = "samsung,exynos4210-sysram";
>>>>     *sysram_offset = 0;
>>>> }
>>>> else
>>>> {
>>>>     compatible "samsung,exynos4210-sysram-ns";
>>>>     *sysram_offset = 0x1c;
>>>> }
>>>>
>>>> node = dt_find_compatible_node(NULL, NULL, compatible);
>>>> if ( !node )
>>>> ....
>>>>
>>>> [..]
>>>>
>>> I shall make that change.
>>>
>>>>> +static int __init exynos5_smp_init(void)
>>>>> +{
>>>>> +    void __iomem *sysram;
>>>>> +    u64 sysram_addr;
>>>>> +    u64 sysram_offset;
>>>>> +    int rc;
>>>>> +
>>>>> +    rc = exynos_smp_init_getbaseandoffset(&sysram_addr,
>>>>> &sysram_offset);
>>>>> +    if ( rc )
>>>>> +        return rc;
>>>>>
>>>>> -    sysram = ioremap_nocache(sysram_ns_base_addr, PAGE_SIZE);
>>>>> +    dprintk(XENLOG_INFO, "sysram_addr: %016llx offset: %016llx\n",
>>>>> +            sysram_addr, sysram_offset);
>>>>> +
>>>>> +    if ( sysram_offset >= PAGE_SIZE )
>>>>> +    {
>>>>> +        dprintk(XENLOG_ERR, "sysram_offset is >= PAGE_SIZE\n");
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>
>>>>
>>>>
>>>> As the previous check for the MCT. I don't think it's useful. Also why
>>>> don't
>>>> do get the size from the device tree?
>>>>
>>> In this case as we are poking only offset 0 or 0x1c which is always
>>> less than PAGE_SIZE, I didn't bother with the size. I could if you
>>> insist.
>>> I can get rid of this check as sysram_offset is always less that
>>> PAGE_SIZE.
>>>
>>>>> +
>>>>> +    sysram = ioremap_nocache(sysram_addr, PAGE_SIZE);
>>>>>        if ( !sysram )
>>>>>        {
>>>>>            dprintk(XENLOG_ERR, "Unable to map exynos5 MMIO\n");
>>>>> @@ -125,7 +176,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 + sysram_offset);
>>>>>
>>>>>        iounmap(sysram);
>>>>>
>>>>> @@ -134,14 +185,14 @@ static int __init exynos5_smp_init(void)
>>>>>
>>>>>    static int exynos_cpu_power_state(void __iomem *power, int cpu)
>>>>>    {
>>>>> -    return __raw_readl(power + EXYNOS_ARM_CORE_STATUS(cpu)) &
>>>>> -                       S5P_CORE_LOCAL_PWR_EN;
>>>>> +    return __raw_readl(power + EXYNOS_ARM_CORE0_CONFIG +
>>>>> +                       EXYNOS_ARM_CORE_STATUS(cpu)) &
>>>>> S5P_CORE_LOCAL_PWR_EN;
>>>>
>>>>
>>>>
>>>> Linux has the EXYNOS_ARM_CORE0_CONFIG included in the macro
>>>> EXYNOS_CORE_CONFIGURATION. I would do the same here.
>>>>
>>> OK, I will make that change.
>>>
>>>> [..]
>>>>
>>>>> -    power = ioremap_nocache(power_base_addr +
>>>>> -                            EXYNOS_ARM_CORE0_CONFIG, PAGE_SIZE);
>>>>> +    if ( size <= EXYNOS_ARM_CORE0_CONFIG +
>>>>> +                 EXYNOS_ARM_CORE_STATUS(EXYNOS_CONFIG_NR_CPUS) )
>>>>> +    {
>>>>> +        dprintk(XENLOG_ERR, "CORE registers fall outside of pmu
>>>>> range.\n");
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +
>>>>
>>>>
>>>>
>>>> Same remark as before for the check.
>>>>
>>> My same response, in the sense that the size from DT will be correct,
>>> but don't we have to make sure that the offsets that we calculate with
>>> the #defines, fall within size?
>>>
>>>> [..]
>>>>
>>>>> -    pmu = ioremap_nocache(power_base_addr, PAGE_SIZE);
>>>>> +    if ( size <= EXYNOS5_SWRESET )
>>>>> +    {
>>>>> +        dprintk(XENLOG_ERR, "EXYNOS5_SWRESET: %d is >= size\n",
>>>>> +                EXYNOS5_SWRESET);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>
>>>>
>>>>
>>>> Same here too.
>>>
>>> My same response as before, that the EXYNOS5_SWRESET offset might be
>>> miscalculated or quoted for a newer platform which falls beyond size
>>> offset.
>>>
>>> Please let me know if these checks are needed or not. I believe they
>>> are good to keep, but if you insist I could leave them out.
>>>
>>> Also, the next version of this patch, do I still maintain the same
>>> subject and comments, or should this come up as something else?
>>>
>>> Thanks as always
>>> - Suriyan
>>>
>>>>
>>>> Regards,
>>>>
>>>> --
>>>> Julien Grall
>>
>>
>> --
>> Julien Grall

Re: [XEN/ARM PATCH v1 1/1] Unbreak Arndale XEN boot

[Julien Grall]

On 12/09/14 14:03, Suriyan Ramasami wrote:
> On Fri, Sep 12, 2014 at 12:34 PM, Suriyan Ramasami <suriyan.r@gmail.com> wrote:
>> On Fri, Sep 12, 2014 at 11:58 AM, Julien Grall <julien.grall@linaro.org> wrote:
>>> Hi Suriyan,
>>>
>>> A couple of remarks about the Samsung secure firmware and your patch.
>>>
>>> When secure firmware is present, the CPU bring up is done by SMC (I don't
>>> see this code in your patch).
>>>
>> Yes you are right. A call to call_firmware_op(cpu_boot, phys_cpu)
>> translating to exynos_smc(SMC_CMD_CPU1BOOT, cpu, 0, 0) is missing
>> right before the call to waking up the  cpu.
>> This was intentional on my part as the trustzone blob that comes with
>> the odroid xu does not have code for that smc call and just returns
>> -1.
>>
>>   I am guessing as we want this as generic as posisble, I should add
>> that part in.
>>
>>> Also, for now, the secure firmware MMIO range is mapped to DOM0. I'm not
>>> sure we should do that because it contains way to idle CPU...
>>> I think we should blacklist it for now. And see how DOM0 behave without.
>>>
>> I shall try this on the Odroid XU.
>>
>
> Hello Julien,

Hello Suriyan,

>     I blacklisted "samsung,secure-firmware" and I can't perceive any
> change in DOM0's behavior in the Odroid XU. Do you recommend, I send
> the next patch with this blacklisted?

Yes please.

Regards,

-- 
Julien Grall