[ARM:PATCH v2 1/1] Pass the timer clock-frequency to DOM0

[ARM:PATCH v2 1/1] Pass the timer clock-frequency to DOM0

[Suriyan Ramasami]

    xen/arm: Pass the timer "clock-frequency" to DOM0 in make_timer_node

    If the DT representing the ARM generic timer mentions a clock-frequency,
    propragate it to the DT that is built for DOM0.

    This is necessary as a workaround for boards (Odroid-XU) where CNTFRQ is
    not set or returns a wrong value.

Signed-off-by: Suriyan Ramasami <suriyan.r@gmail.com>

---
Changed since v1:
    * Fix typo for quotes around clock-frequency.

---
 xen/arch/arm/domain_build.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index c9dda0e..645f9c6 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -622,6 +622,8 @@ static int make_timer_node(const struct domain *d, void *fdt,
     int res;
     const struct dt_irq *irq;
     gic_interrupt_t intrs[3];
+    u32 clock_frequency;
+    bool_t clock_valid;
 
     DPRINT("Create timer node\n");
 
@@ -639,6 +641,9 @@ static int make_timer_node(const struct domain *d, void *fdt,
         return -FDT_ERR_XEN(ENOENT);
     }
 
+    clock_valid = dt_property_read_u32(dev, "clock-frequency",
+                                       &clock_frequency);
+
     res = fdt_begin_node(fdt, "timer");
     if ( res )
         return res;
@@ -663,6 +668,13 @@ static int make_timer_node(const struct domain *d, void *fdt,
     if ( res )
         return res;
 
+    if ( clock_valid )
+    {
+        res = fdt_property_cell(fdt, "clock-frequency", clock_frequency);
+        if ( res )
+            return res;
+    }
+
     res = fdt_end_node(fdt);
 
     return res;
-- 
1.8.3.2

Re: [ARM:PATCH v2 1/1] Pass the timer clock-frequency to DOM0

[Julien Grall]

Hello Suriyan,

(Adding the other Xen ARM maintainers).

Thank you for the patch. Xen-devel starts to have a big traffic, we use 
to cc maintainers on patches. You can find them via 
script/get_maintainers.pl

On 27/03/14 03:54, Suriyan Ramasami wrote:
>      xen/arm: Pass the timer "clock-frequency" to DOM0 in make_timer_node

You guess you are copying git show in an email? If so, I advise you to 
read this website:
http://alblue.bandlem.com/2011/12/git-tip-of-week-patches-by-email.html

>      If the DT representing the ARM generic timer mentions a clock-frequency,
>      propragate it to the DT that is built for DOM0.
>
>      This is necessary as a workaround for boards (Odroid-XU) where CNTFRQ is
>      not set or returns a wrong value.

Do you have any plan to send a similar patch for domU?
>
> @@ -639,6 +641,9 @@ static int make_timer_node(const struct domain *d, void *fdt,
>           return -FDT_ERR_XEN(ENOENT);
>       }
>
> +    clock_valid = dt_property_read_u32(dev, "clock-frequency",
> +                                       &clock_frequency);
> +

Can you gather all your changes in one place? I don't see why you need 
to read the clock-frequency earlier.

Regards,

-- 
Julien Grall

Re: [ARM:PATCH v2 1/1] Pass the timer clock-frequency to DOM0

[Suriyan Ramasami]

On Thu, Mar 27, 2014 at 2:48 PM, Julien Grall <julien.grall@linaro.org> wrote:
> Hello Suriyan,
>
> (Adding the other Xen ARM maintainers).
>
> Thank you for the patch. Xen-devel starts to have a big traffic, we use to
> cc maintainers on patches. You can find them via script/get_maintainers.pl
>
Thank you for the comments Julien. Next time around, I shall make sure
I use get_maintainers.pl and get the list of the maintainers that I
need to cc.

>
> On 27/03/14 03:54, Suriyan Ramasami wrote:
>>
>>      xen/arm: Pass the timer "clock-frequency" to DOM0 in make_timer_node
>
>
> You guess you are copying git show in an email? If so, I advise you to read
> this website:
> http://alblue.bandlem.com/2011/12/git-tip-of-week-patches-by-email.html
>
I actually used git send-email as mentioned in the wiki.

>
>>      If the DT representing the ARM generic timer mentions a
>> clock-frequency,
>>      propragate it to the DT that is built for DOM0.
>>
>>      This is necessary as a workaround for boards (Odroid-XU) where CNTFRQ
>> is
>>      not set or returns a wrong value.
>
>
> Do you have any plan to send a similar patch for domU?
>
I am exploring other options for domU - possibly a parameter that can
be introduced in the cfg file for the domU. I am still exploring. Any
suggestions would be deeply appreciated!

I also am in parallel working on a patch for the odroid-xu (exynos5410).

>>
>> @@ -639,6 +641,9 @@ static int make_timer_node(const struct domain *d,
>> void *fdt,
>>           return -FDT_ERR_XEN(ENOENT);
>>       }
>>
>> +    clock_valid = dt_property_read_u32(dev, "clock-frequency",
>> +                                       &clock_frequency);
>> +
>
>
> Can you gather all your changes in one place? I don't see why you need to
> read the clock-frequency earlier.
>
I could gather them in one place, but I mimicked what make_cpus_node()
does, so that make_timer_node() too has similar code. Please let me
know if you would really like it in one place.

Regards
- Suriyan
> Regards
>
> --
> Julien Grall

Re: [ARM:PATCH v2 1/1] Pass the timer clock-frequency to DOM0

[Julien Grall]

Hello Suryian,

On 03/27/2014 10:16 PM, Suriyan Ramasami wrote:
> On Thu, Mar 27, 2014 at 2:48 PM, Julien Grall <julien.grall@linaro.org> wrote:
>> Hello Suriyan,
>> Thank you for the patch. Xen-devel starts to have a big traffic, we use to
>> cc maintainers on patches. You can find them via script/get_maintainers.pl
>>
> Thank you for the comments Julien. Next time around, I shall make sure
> I use get_maintainers.pl and get the list of the maintainers that I
> need to cc.
> 
>>
>> On 27/03/14 03:54, Suriyan Ramasami wrote:
>>>
>>>      xen/arm: Pass the timer "clock-frequency" to DOM0 in make_timer_node
>>
>>
>> You guess you are copying git show in an email? If so, I advise you to read
>> this website:
>> http://alblue.bandlem.com/2011/12/git-tip-of-week-patches-by-email.html
>>
> I actually used git send-email as mentioned in the wiki.

Do you also use git format-patch?

>>
>>>      If the DT representing the ARM generic timer mentions a
>>> clock-frequency,
>>>      propragate it to the DT that is built for DOM0.
>>>
>>>      This is necessary as a workaround for boards (Odroid-XU) where CNTFRQ
>>> is
>>>      not set or returns a wrong value.
>>
>>
>> Do you have any plan to send a similar patch for domU?
>>
> I am exploring other options for domU - possibly a parameter that can
> be introduced in the cfg file for the domU. I am still exploring. Any
> suggestions would be deeply appreciated!

I'm not in favor to add a new config option. The user may not be aware
that the clock frequency has to be set up in xl config file for his board.

That made me think that this is the only board with this issue. I would
definitely prefer to fix the clock frequency in U-boot. The bootloader
should respect the ARM ARM (see B.8.1.1):

"The CNTFRQ register is UNKNOWN at reset, and therefore the counter
frequency must written to CNTFRQ as part
of the system boot process."

> I also am in parallel working on a patch for the odroid-xu (exynos5410).

Great, thanks!

>>>
>>> @@ -639,6 +641,9 @@ static int make_timer_node(const struct domain *d,
>>> void *fdt,
>>>           return -FDT_ERR_XEN(ENOENT);
>>>       }
>>>
>>> +    clock_valid = dt_property_read_u32(dev, "clock-frequency",
>>> +                                       &clock_frequency);
>>> +
>>
>>
>> Can you gather all your changes in one place? I don't see why you need to
>> read the clock-frequency earlier.
>>
> I could gather them in one place, but I mimicked what make_cpus_node()
> does, so that make_timer_node() too has similar code. Please let me
> know if you would really like it in one place.

It has been done earlier because we also retrieve the compatible string
at the same time. The former is used to check if we can create the node
or not.

Regards,

-- 
Julien Grall

Re: [ARM:PATCH v2 1/1] Pass the timer clock-frequency to DOM0

[Ian Campbell]

On Fri, 2014-03-28 at 14:04 +0000, Julien Grall wrote:
> That made me think that this is the only board with this issue. I would
> definitely prefer to fix the clock frequency in U-boot. The bootloader
> should respect the ARM ARM (see B.8.1.1):
> 
> "The CNTFRQ register is UNKNOWN at reset, and therefore the counter
> frequency must written to CNTFRQ as part
> of the system boot process."

Yes, Suriyan, please exhaust this avenue of attack (fixing u-boot on the
platform) first.

Ian.

Re: [ARM:PATCH v2 1/1] Pass the timer clock-frequency to DOM0

[Suriyan Ramasami]

On Tue, Apr 1, 2014 at 4:12 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Fri, 2014-03-28 at 14:04 +0000, Julien Grall wrote:
>> That made me think that this is the only board with this issue. I would
>> definitely prefer to fix the clock frequency in U-boot. The bootloader
>> should respect the ARM ARM (see B.8.1.1):
>>
>> "The CNTFRQ register is UNKNOWN at reset, and therefore the counter
>> frequency must written to CNTFRQ as part
>> of the system boot process."
>
> Yes, Suriyan, please exhaust this avenue of attack (fixing u-boot on the
> platform) first.
>
Thank you gentlemen for your comments.
The problem with this board is that the uboot is not entered in secure
mode, and hence the mcr indstruction to set the ARCH timer frequency
cannot be executed in u-boot.
Nonetheless, I am pursuing a course of action where the tzsw blob
(samsung provided with no source) could be modified by taking over an
existing smc call and setting the ARCH timer frequency instead.
I shall keep you guys posted.
- Suriyan

> Ian.
>
>

Re: [ARM:PATCH v2 1/1] Pass the timer clock-frequency to DOM0

[Julien Grall]

Hello,

On 04/11/2014 05:05 PM, Suriyan Ramasami wrote:
> On Tue, Apr 1, 2014 at 4:12 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> On Fri, 2014-03-28 at 14:04 +0000, Julien Grall wrote:
>>> That made me think that this is the only board with this issue. I would
>>> definitely prefer to fix the clock frequency in U-boot. The bootloader
>>> should respect the ARM ARM (see B.8.1.1):
>>>
>>> "The CNTFRQ register is UNKNOWN at reset, and therefore the counter
>>> frequency must written to CNTFRQ as part
>>> of the system boot process."
>>
>> Yes, Suriyan, please exhaust this avenue of attack (fixing u-boot on the
>> platform) first.
>>
> Thank you gentlemen for your comments.
> The problem with this board is that the uboot is not entered in secure
> mode, and hence the mcr indstruction to set the ARCH timer frequency
> cannot be executed in u-boot.

If so, what is the state of the CPU when it jumps to U-boot?
The CPU has to be either in secure mode or hyp mode.
AFAIK, if it's NS you won't be able to go in hyp mode, unless crash the
CPU...

Regards,

-- 
Julien Grall

Re: [ARM:PATCH v2 1/1] Pass the timer clock-frequency to DOM0

[Suriyan Ramasami]

On Fri, Apr 11, 2014 at 9:32 AM, Julien Grall <julien.grall@linaro.org> wrote:
> Hello,
>
> On 04/11/2014 05:05 PM, Suriyan Ramasami wrote:
>> On Tue, Apr 1, 2014 at 4:12 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>>> On Fri, 2014-03-28 at 14:04 +0000, Julien Grall wrote:
>>>> That made me think that this is the only board with this issue. I would
>>>> definitely prefer to fix the clock frequency in U-boot. The bootloader
>>>> should respect the ARM ARM (see B.8.1.1):
>>>>
>>>> "The CNTFRQ register is UNKNOWN at reset, and therefore the counter
>>>> frequency must written to CNTFRQ as part
>>>> of the system boot process."
>>>
>>> Yes, Suriyan, please exhaust this avenue of attack (fixing u-boot on the
>>> platform) first.
>>>
>> Thank you gentlemen for your comments.
>> The problem with this board is that the uboot is not entered in secure
>> mode, and hence the mcr indstruction to set the ARCH timer frequency
>> cannot be executed in u-boot.
>
> If so, what is the state of the CPU when it jumps to U-boot?
> The CPU has to be either in secure mode or hyp mode.
> AFAIK, if it's NS you won't be able to go in hyp mode, unless crash the
> CPU...
>
The CPU is in HYP mode when it jumps to U-boot.
Thanks
- Suriyan

> Regards,
>
> --
> Julien Grall

Re: [ARM:PATCH v2 1/1] Pass the timer clock-frequency to DOM0

[Julien Grall]

Hello,

On 13/04/14 20:28, Suriyan Ramasami wrote:
> On Fri, Apr 11, 2014 at 9:32 AM, Julien Grall <julien.grall@linaro.org> wrote:
>> Hello,
>>
>> On 04/11/2014 05:05 PM, Suriyan Ramasami wrote:
>>> On Tue, Apr 1, 2014 at 4:12 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>>>> On Fri, 2014-03-28 at 14:04 +0000, Julien Grall wrote:
>>>>> That made me think that this is the only board with this issue. I would
>>>>> definitely prefer to fix the clock frequency in U-boot. The bootloader
>>>>> should respect the ARM ARM (see B.8.1.1):
>>>>>
>>>>> "The CNTFRQ register is UNKNOWN at reset, and therefore the counter
>>>>> frequency must written to CNTFRQ as part
>>>>> of the system boot process."
>>>>
>>>> Yes, Suriyan, please exhaust this avenue of attack (fixing u-boot on the
>>>> platform) first.
>>>>
>>> Thank you gentlemen for your comments.
>>> The problem with this board is that the uboot is not entered in secure
>>> mode, and hence the mcr indstruction to set the ARCH timer frequency
>>> cannot be executed in u-boot.
>>
>> If so, what is the state of the CPU when it jumps to U-boot?
>> The CPU has to be either in secure mode or hyp mode.
>> AFAIK, if it's NS you won't be able to go in hyp mode, unless crash the
>> CPU...
>>
> The CPU is in HYP mode when it jumps to U-boot.

I'm lost...

 From my understanding (i.e what I read on the web). For your case you 
use a modified U-boot/SPL, right?

If so, without this modification, in which state U-boot is booting? What 
are the CPU states along the different bl (1,2)?

Did you try this u-boot: 
https://github.com/medicalwei/u-boot-odroidxu-hyp? FYI I don't have any 
odroid XU. I'm trying to understand

Regards,

-- 
Julien Grall

Re: [ARM:PATCH v2 1/1] Pass the timer clock-frequency to DOM0

[Suriyan Ramasami]

On Sun, Apr 13, 2014 at 1:20 PM, Julien Grall <julien.grall@linaro.org> wrote:
> Hello,
>
>
> On 13/04/14 20:28, Suriyan Ramasami wrote:
>>
>> On Fri, Apr 11, 2014 at 9:32 AM, Julien Grall <julien.grall@linaro.org>
>> wrote:
>>>
>>> Hello,
>>>
>>> On 04/11/2014 05:05 PM, Suriyan Ramasami wrote:
>>>>
>>>> On Tue, Apr 1, 2014 at 4:12 AM, Ian Campbell <Ian.Campbell@citrix.com>
>>>> wrote:
>>>>>
>>>>> On Fri, 2014-03-28 at 14:04 +0000, Julien Grall wrote:
>>>>>>
>>>>>> That made me think that this is the only board with this issue. I
>>>>>> would
>>>>>> definitely prefer to fix the clock frequency in U-boot. The bootloader
>>>>>> should respect the ARM ARM (see B.8.1.1):
>>>>>>
>>>>>> "The CNTFRQ register is UNKNOWN at reset, and therefore the counter
>>>>>> frequency must written to CNTFRQ as part
>>>>>> of the system boot process."
>>>>>
>>>>>
>>>>> Yes, Suriyan, please exhaust this avenue of attack (fixing u-boot on
>>>>> the
>>>>> platform) first.
>>>>>
>>>> Thank you gentlemen for your comments.
>>>> The problem with this board is that the uboot is not entered in secure
>>>> mode, and hence the mcr indstruction to set the ARCH timer frequency
>>>> cannot be executed in u-boot.
>>>
>>>
>>> If so, what is the state of the CPU when it jumps to U-boot?
>>> The CPU has to be either in secure mode or hyp mode.
>>> AFAIK, if it's NS you won't be able to go in hyp mode, unless crash the
>>> CPU...
>>>
>> The CPU is in HYP mode when it jumps to U-boot.
>
>
> I'm lost...
>
> From my understanding (i.e what I read on the web). For your case you use a
> modified U-boot/SPL, right?
>
Correct - the one in hardlernel's git with a modification done by fanta.

> If so, without this modification, in which state U-boot is booting? What are
> the CPU states along the different bl (1,2)?
>
> Did you try this u-boot: https://github.com/medicalwei/u-boot-odroidxu-hyp?
> FYI I don't have any odroid XU. I'm trying to understand
Haven't tried this one.
>
Sorry Julien! Not my intent.
This post possibly has the details that you are looking for:
http://forum.odroid.com/viewtopic.php?f=64&t=2778&start=40#p31301

fanta's patch to get the CPU up in NS HYP mode is in hardkernel's git
repository ->
 https://github.com/hardkernel/u-boot/commit/56e5bdcb95d41f9236554de0578b0017a9f232a5

Hence, it would appear that the SPL BL2 code is entered in NS HYP mode.

Also, BL1 I believe is signed and encrypted - hence no access. BL2 can
be signed and that is how currently we are entering in NS HYP.
Apparently the tzsw can be signed too, and hence, I was thinking of
modifying a current smc call to have it set the CNTFRQ, which I
believe will be in monitor mode.

> Regards,
>
> --
> Julien Grall

Re: [ARM:PATCH v2 1/1] Pass the timer clock-frequency to DOM0

[Julien Grall]

On 13/04/14 21:48, Suriyan Ramasami wrote:
> On Sun, Apr 13, 2014 at 1:20 PM, Julien Grall <julien.grall@linaro.org> wrote:
> fanta's patch to get the CPU up in NS HYP mode is in hardkernel's git
> repository ->
>   https://github.com/hardkernel/u-boot/commit/56e5bdcb95d41f9236554de0578b0017a9f232a5
>
> Hence, it would appear that the SPL BL2 code is entered in NS HYP mode.
>
> Also, BL1 I believe is signed and encrypted - hence no access. BL2 can
> be signed and that is how currently we are entering in NS HYP.

Thanks for the explanation.

By any chance, do you know if the Arndale octa (exynos 5420) also bring 
CPU in HYP mode the same way?

> Apparently the tzsw can be signed too, and hence, I was thinking of
> modifying a current smc call to have it set the CNTFRQ, which I
> believe will be in monitor mode.

We had the same issue on the Arndale last year. Luckily u-boot were 
booting in Secure mode.

It would be great to have a firmware that correctly configure the Arch 
timer (according the ARM ARM). But I bet the Arndale octa will have the 
same issue. So we will have to handle this such configuration in Xen.

Can you send a new version with remarks make previously?

Regards,

-- 
Julien Grall

Re: [ARM:PATCH v2 1/1] Pass the timer clock-frequency to DOM0

[Suriyan Ramasami]

On Sun, Apr 13, 2014 at 2:40 PM, Julien Grall <julien.grall@linaro.org> wrote:
>
>
> On 13/04/14 21:48, Suriyan Ramasami wrote:
>>
>> On Sun, Apr 13, 2014 at 1:20 PM, Julien Grall <julien.grall@linaro.org>
>> wrote:
>> fanta's patch to get the CPU up in NS HYP mode is in hardkernel's git
>> repository ->
>>
>> https://github.com/hardkernel/u-boot/commit/56e5bdcb95d41f9236554de0578b0017a9f232a5
>>
>> Hence, it would appear that the SPL BL2 code is entered in NS HYP mode.
>>
>> Also, BL1 I believe is signed and encrypted - hence no access. BL2 can
>> be signed and that is how currently we are entering in NS HYP.
>
>
> Thanks for the explanation.
>
> By any chance, do you know if the Arndale octa (exynos 5420) also bring CPU
> in HYP mode the same way?
Unfortunately, I do not have the Arndale octa, and hence do not know.

>
>
>> Apparently the tzsw can be signed too, and hence, I was thinking of
>> modifying a current smc call to have it set the CNTFRQ, which I
>> believe will be in monitor mode.
>
>
> We had the same issue on the Arndale last year. Luckily u-boot were booting
> in Secure mode.
>
> It would be great to have a firmware that correctly configure the Arch timer
> (according the ARM ARM). But I bet the Arndale octa will have the same
> issue. So we will have to handle this such configuration in Xen.
>
> Can you send a new version with remarks make previously?
>
Yes, I surely will. BTW, if we do apply this patch, then we do have to
figure out how the domU's shall get the CNTFRQ value.

> Regards,
>
> --
> Julien Grall