[PATCH 1/1] clk: kendryte/pll.h: do not redefine nop()

[PATCH 1/1] clk: kendryte/pll.h: do not redefine nop()

[Heinrich Schuchardt]

The kendryte PLL code uses nop as barrier. The macro is not defined for
the sandbox on x86 but is defined on RISC-V.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/kendryte/pll.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/kendryte/pll.h b/include/kendryte/pll.h
index c8e3200799..55a40b9c97 100644
--- a/include/kendryte/pll.h
+++ b/include/kendryte/pll.h
@@ -7,6 +7,7 @@

 #include <clk.h>
 #include <test/export.h>
+#include <asm/io.h>

 #define K210_PLL_CLKR GENMASK(3, 0)
 #define K210_PLL_CLKF GENMASK(9, 4)
@@ -43,9 +44,13 @@ struct k210_pll_config {
 #ifdef CONFIG_UNIT_TEST
 TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in,
 				     struct k210_pll_config *best);
+
+#ifndef nop
 #define nop()
 #endif

+#endif
+
 extern const struct clk_ops k210_pll_ops;

 struct clk *k210_register_pll_struct(const char *name, const char *parent_name,
--
2.27.0

[PATCH 1/1] clk: kendryte/pll.h: do not redefine nop()

[Sean Anderson]

On 7/28/20 11:52 AM, Heinrich Schuchardt wrote:
> The kendryte PLL code uses nop as barrier. The macro is not defined for
> the sandbox on x86 but is defined on RISC-V.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  include/kendryte/pll.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/kendryte/pll.h b/include/kendryte/pll.h
> index c8e3200799..55a40b9c97 100644
> --- a/include/kendryte/pll.h
> +++ b/include/kendryte/pll.h
> @@ -7,6 +7,7 @@
> 
>  #include <clk.h>
>  #include <test/export.h>
> +#include <asm/io.h>
> 
>  #define K210_PLL_CLKR GENMASK(3, 0)
>  #define K210_PLL_CLKF GENMASK(9, 4)
> @@ -43,9 +44,13 @@ struct k210_pll_config {
>  #ifdef CONFIG_UNIT_TEST
>  TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in,
>  				     struct k210_pll_config *best);
> +
> +#ifndef nop
>  #define nop()
>  #endif
> 
> +#endif
> +
>  extern const struct clk_ops k210_pll_ops;
> 
>  struct clk *k210_register_pll_struct(const char *name, const char *parent_name,
> --
> 2.27.0
> 

Reviewed-by: Sean Anderson <seanga2@gmail.com>

[PATCH 1/1] clk: kendryte/pll.h: do not redefine nop()

[Bin Meng]

On Tue, Jul 28, 2020 at 11:52 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> The kendryte PLL code uses nop as barrier. The macro is not defined for
> the sandbox on x86 but is defined on RISC-V.

Is this kendryte PLL driver built for Sandbox?

>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  include/kendryte/pll.h | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/include/kendryte/pll.h b/include/kendryte/pll.h
> index c8e3200799..55a40b9c97 100644
> --- a/include/kendryte/pll.h
> +++ b/include/kendryte/pll.h
> @@ -7,6 +7,7 @@
>
>  #include <clk.h>
>  #include <test/export.h>
> +#include <asm/io.h>
>
>  #define K210_PLL_CLKR GENMASK(3, 0)
>  #define K210_PLL_CLKF GENMASK(9, 4)
> @@ -43,9 +44,13 @@ struct k210_pll_config {
>  #ifdef CONFIG_UNIT_TEST
>  TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in,
>                                      struct k210_pll_config *best);
> +
> +#ifndef nop
>  #define nop()
>  #endif

Maybe we can fix the kendryte PLL driver to use:

asm volatile ("nop");

??

Regards,
Bin

[PATCH 1/1] clk: kendryte/pll.h: do not redefine nop()

[Sean Anderson]

On 8/2/20 11:29 PM, Bin Meng wrote:
> On Tue, Jul 28, 2020 at 11:52 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> The kendryte PLL code uses nop as barrier. The macro is not defined for
>> the sandbox on x86 but is defined on RISC-V.
> 
> Is this kendryte PLL driver built for Sandbox?

Yes. I added a unit test for the parameter calculation function.

> 
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  include/kendryte/pll.h | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/include/kendryte/pll.h b/include/kendryte/pll.h
>> index c8e3200799..55a40b9c97 100644
>> --- a/include/kendryte/pll.h
>> +++ b/include/kendryte/pll.h
>> @@ -7,6 +7,7 @@
>>
>>  #include <clk.h>
>>  #include <test/export.h>
>> +#include <asm/io.h>
>>
>>  #define K210_PLL_CLKR GENMASK(3, 0)
>>  #define K210_PLL_CLKF GENMASK(9, 4)
>> @@ -43,9 +44,13 @@ struct k210_pll_config {
>>  #ifdef CONFIG_UNIT_TEST
>>  TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in,
>>                                      struct k210_pll_config *best);
>> +
>> +#ifndef nop
>>  #define nop()
>>  #endif
> 
> Maybe we can fix the kendryte PLL driver to use:
> 
> asm volatile ("nop");

Is that preferred over the nop macro?

--Sean

[PATCH 1/1] clk: kendryte/pll.h: do not redefine nop()

[Heinrich Schuchardt]

On 03.08.20 12:10, Sean Anderson wrote:
> On 8/2/20 11:29 PM, Bin Meng wrote:
>> On Tue, Jul 28, 2020 at 11:52 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>
>>> The kendryte PLL code uses nop as barrier. The macro is not defined for
>>> the sandbox on x86 but is defined on RISC-V.
>>
>> Is this kendryte PLL driver built for Sandbox?
>
> Yes. I added a unit test for the parameter calculation function.
>
>>
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> ---
>>>  include/kendryte/pll.h | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/include/kendryte/pll.h b/include/kendryte/pll.h
>>> index c8e3200799..55a40b9c97 100644
>>> --- a/include/kendryte/pll.h
>>> +++ b/include/kendryte/pll.h
>>> @@ -7,6 +7,7 @@
>>>
>>>  #include <clk.h>
>>>  #include <test/export.h>
>>> +#include <asm/io.h>
>>>
>>>  #define K210_PLL_CLKR GENMASK(3, 0)
>>>  #define K210_PLL_CLKF GENMASK(9, 4)
>>> @@ -43,9 +44,13 @@ struct k210_pll_config {
>>>  #ifdef CONFIG_UNIT_TEST
>>>  TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in,
>>>                                      struct k210_pll_config *best);
>>> +
>>> +#ifndef nop
>>>  #define nop()
>>>  #endif
>>
>> Maybe we can fix the kendryte PLL driver to use:
>>
>> asm volatile ("nop");
>
> Is that preferred over the nop macro?

On RISC-V nop is just a synonym for addi x0, x0, 0.

The only use of the nop statements is to guarantee a minimum time that
the reset signal is set in k210_pll_enable(). Would udelay(1); be a
valid replacement here?

Best regards

Heinrich

[PATCH 1/1] clk: kendryte/pll.h: do not redefine nop()

[Sean Anderson]

On 8/3/20 9:59 AM, Heinrich Schuchardt wrote:
> On 03.08.20 12:10, Sean Anderson wrote:
>> On 8/2/20 11:29 PM, Bin Meng wrote:
>>> On Tue, Jul 28, 2020 at 11:52 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>
>>>> The kendryte PLL code uses nop as barrier. The macro is not defined for
>>>> the sandbox on x86 but is defined on RISC-V.
>>>
>>> Is this kendryte PLL driver built for Sandbox?
>>
>> Yes. I added a unit test for the parameter calculation function.
>>
>>>
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>> ---
>>>>  include/kendryte/pll.h | 5 +++++
>>>>  1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/include/kendryte/pll.h b/include/kendryte/pll.h
>>>> index c8e3200799..55a40b9c97 100644
>>>> --- a/include/kendryte/pll.h
>>>> +++ b/include/kendryte/pll.h
>>>> @@ -7,6 +7,7 @@
>>>>
>>>>  #include <clk.h>
>>>>  #include <test/export.h>
>>>> +#include <asm/io.h>
>>>>
>>>>  #define K210_PLL_CLKR GENMASK(3, 0)
>>>>  #define K210_PLL_CLKF GENMASK(9, 4)
>>>> @@ -43,9 +44,13 @@ struct k210_pll_config {
>>>>  #ifdef CONFIG_UNIT_TEST
>>>>  TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in,
>>>>                                      struct k210_pll_config *best);
>>>> +
>>>> +#ifndef nop
>>>>  #define nop()
>>>>  #endif
>>>
>>> Maybe we can fix the kendryte PLL driver to use:
>>>
>>> asm volatile ("nop");
>>
>> Is that preferred over the nop macro?
> 
> On RISC-V nop is just a synonym for addi x0, x0, 0.
> 
> The only use of the nop statements is to guarantee a minimum time that
> the reset signal is set in k210_pll_enable(). Would udelay(1); be a
> valid replacement here?

Maybe. Because we are configuring the PLL, the CPU clock is temporarily
set to the in0 oscillator, so the timer would give an incorrect delay.
However, it would probably be fine even if incorrect.  The original
reason I used nops is because that is what the SDK does in its PLL
configuration function [1]. I believe I tried using udelay at one point,
but I don't remember whether I had any issues with it.

--Sean

[1] https://github.com/kendryte/kendryte-standalone-sdk/blob/develop/lib/drivers/sysctl.c#L844

[PATCH 1/1] clk: kendryte/pll.h: do not redefine nop()

[Heinrich Schuchardt]

On 03.08.20 16:08, Sean Anderson wrote:
> On 8/3/20 9:59 AM, Heinrich Schuchardt wrote:
>> On 03.08.20 12:10, Sean Anderson wrote:
>>> On 8/2/20 11:29 PM, Bin Meng wrote:
>>>> On Tue, Jul 28, 2020 at 11:52 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>
>>>>> The kendryte PLL code uses nop as barrier. The macro is not defined for
>>>>> the sandbox on x86 but is defined on RISC-V.
>>>>
>>>> Is this kendryte PLL driver built for Sandbox?
>>>
>>> Yes. I added a unit test for the parameter calculation function.
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>> ---
>>>>>  include/kendryte/pll.h | 5 +++++
>>>>>  1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/include/kendryte/pll.h b/include/kendryte/pll.h
>>>>> index c8e3200799..55a40b9c97 100644
>>>>> --- a/include/kendryte/pll.h
>>>>> +++ b/include/kendryte/pll.h
>>>>> @@ -7,6 +7,7 @@
>>>>>
>>>>>  #include <clk.h>
>>>>>  #include <test/export.h>
>>>>> +#include <asm/io.h>
>>>>>
>>>>>  #define K210_PLL_CLKR GENMASK(3, 0)
>>>>>  #define K210_PLL_CLKF GENMASK(9, 4)
>>>>> @@ -43,9 +44,13 @@ struct k210_pll_config {
>>>>>  #ifdef CONFIG_UNIT_TEST
>>>>>  TEST_STATIC int k210_pll_calc_config(u32 rate, u32 rate_in,
>>>>>                                      struct k210_pll_config *best);
>>>>> +
>>>>> +#ifndef nop
>>>>>  #define nop()
>>>>>  #endif
>>>>
>>>> Maybe we can fix the kendryte PLL driver to use:
>>>>
>>>> asm volatile ("nop");
>>>
>>> Is that preferred over the nop macro?
>>
>> On RISC-V nop is just a synonym for addi x0, x0, 0.
>>
>> The only use of the nop statements is to guarantee a minimum time that
>> the reset signal is set in k210_pll_enable(). Would udelay(1); be a
>> valid replacement here?
>
> Maybe. Because we are configuring the PLL, the CPU clock is temporarily
> set to the in0 oscillator, so the timer would give an incorrect delay.
> However, it would probably be fine even if incorrect.  The original
> reason I used nops is because that is what the SDK does in its PLL
> configuration function [1]. I believe I tried using udelay at one point,
> but I don't remember whether I had any issues with it.
>
> --Sean
>
> [1] https://github.com/kendryte/kendryte-standalone-sdk/blob/develop/lib/drivers/sysctl.c#L844
>

This is what udelay(1) gets compiled to:

000000000000004a <.LBE106>:
        udelay(1);
  4a:   4505                    li      a0,1

000000000000004c <.LVL21>:
  4c:   00000097                auipc   ra,0x0
  50:   000080e7                jalr    ra # 4c <.LVL21>

Somehow udelay() seems not to be fully implemented and the platform. So
I think we should stay with the nop() macro.

@Bin:
For sandbox testing it is ok to define nop() as nil here. On the real
hardware nop gives us a small delay.

Best regards

Heinrich

[PATCH 1/1] clk: kendryte/pll.h: do not redefine nop()

[Sean Anderson]

On 8/3/20 2:20 PM, Heinrich Schuchardt wrote:
> On 03.08.20 16:08, Sean Anderson wrote:
>> Maybe. Because we are configuring the PLL, the CPU clock is temporarily
>> set to the in0 oscillator, so the timer would give an incorrect delay.
>> However, it would probably be fine even if incorrect.  The original
>> reason I used nops is because that is what the SDK does in its PLL
>> configuration function [1]. I believe I tried using udelay at one point,
>> but I don't remember whether I had any issues with it.
>>
>> --Sean
>>
>> [1] https://github.com/kendryte/kendryte-standalone-sdk/blob/develop/lib/drivers/sysctl.c#L844
>>
> 
> This is what udelay(1) gets compiled to:
> 
> 000000000000004a <.LBE106>:
>         udelay(1);
>   4a:   4505                    li      a0,1
> 
> 000000000000004c <.LVL21>:
>   4c:   00000097                auipc   ra,0x0
>   50:   000080e7                jalr    ra # 4c <.LVL21>
> 
> Somehow udelay() seems not to be fully implemented and the platform. So
> I think we should stay with the nop() macro.

Hm, I don't see this generated code. What function is that a disassembly
of?

$ riscv64-linux-gnu-objdump -S --disassemble=__udelay u-boot

u-boot:     file format elf64-littleriscv


Disassembly of section .text:

Disassembly of section .text_rest:

00000000800197f8 <__udelay>:
	do_div(tick, 1000000);
	return tick;
}

void __weak __udelay(unsigned long usec)
{
    800197f8:	1101                	addi	sp,sp,-32
    800197fa:	ec06                	sd	ra,24(sp)
    800197fc:	e822                	sd	s0,16(sp)
    800197fe:	e426                	sd	s1,8(sp)
    80019800:	84aa                	mv	s1,a0
	uint64_t tmp;

	tmp = get_ticks() + usec_to_tick(usec);	/* get current timestamp */
    80019802:	f7bff0ef          	jal	ra,8001977c <get_ticks>
    80019806:	842a                	mv	s0,a0
    80019808:	8526                	mv	a0,s1
    8001980a:	fcbff0ef          	jal	ra,800197d4 <usec_to_tick>
    8001980e:	942a                	add	s0,s0,a0

	while (get_ticks() < tmp+1)	/* loop till event */
    80019810:	0405                	addi	s0,s0,1
    80019812:	f6bff0ef          	jal	ra,8001977c <get_ticks>
    80019816:	fe856ee3          	bltu	a0,s0,80019812 <__udelay+0x1a>
		 /*NOP*/;
}
    8001981a:	60e2                	ld	ra,24(sp)
    8001981c:	6442                	ld	s0,16(sp)
    8001981e:	64a2                	ld	s1,8(sp)
    80019820:	6105                	addi	sp,sp,32
    80019822:	8082                	ret

--Sean

[PATCH 1/1] clk: kendryte/pll.h: do not redefine nop()

[Heinrich Schuchardt]

On 05.08.20 13:45, Sean Anderson wrote:
> On 8/3/20 2:20 PM, Heinrich Schuchardt wrote:
>> On 03.08.20 16:08, Sean Anderson wrote:
>>> Maybe. Because we are configuring the PLL, the CPU clock is temporarily
>>> set to the in0 oscillator, so the timer would give an incorrect delay.
>>> However, it would probably be fine even if incorrect.  The original
>>> reason I used nops is because that is what the SDK does in its PLL
>>> configuration function [1]. I believe I tried using udelay at one point,
>>> but I don't remember whether I had any issues with it.
>>>
>>> --Sean
>>>
>>> [1] https://github.com/kendryte/kendryte-standalone-sdk/blob/develop/lib/drivers/sysctl.c#L844
>>>
>>
>> This is what udelay(1) gets compiled to:
>>
>> 000000000000004a <.LBE106>:
>>         udelay(1);
>>   4a:   4505                    li      a0,1
>>
>> 000000000000004c <.LVL21>:
>>   4c:   00000097                auipc   ra,0x0
>>   50:   000080e7                jalr    ra # 4c <.LVL21>
>>
>> Somehow udelay() seems not to be fully implemented and the platform. So
>> I think we should stay with the nop() macro.
>
> Hm, I don't see this generated code. What function is that a disassembly
> of?

I was disassembling

riscv64-linux-gnu-objdump -D -S drivers/clk/kendryte/pll.o

If I disassemble the file u-boot function k210_pll_enable seems to be
ok. nop()s replaced by udelay(1):

??????? writel(reg, pll->reg);
    80012524:???7518                ????ld??????a4,40(a0)
????????__iowmb();
    80012526:???0550000f          ??????fence???ow,ow
????????reg |= K210_PLL_RESET;
    8001252a:???003006b7          ??????lui?????a3,0x300
    8001252e:???8fd5                ????or??????a5,a5,a3
????????__arch_putl(val, addr);
    80012530:???c31c                ????sw??????a5,0(a4)
????????udelay(1);
    80012532:???4505                ????li??????a0,1
    80012534:???612200ef          ??????jal?????ra,80032b46 <udelay>
????????writel(reg, pll->reg);
    80012538:???741c                ????ld??????a5,40(s0)
????????__iowmb();
    8001253a:???0550000f          ??????fence???ow,ow

So it was simply the link step missing to show the call.

But anyway which way do you want the patch?

Best regards

Heinrich

>
> $ riscv64-linux-gnu-objdump -S --disassemble=__udelay u-boot
>
> u-boot:     file format elf64-littleriscv
>
>
> Disassembly of section .text:
>
> Disassembly of section .text_rest:
>
> 00000000800197f8 <__udelay>:
> 	do_div(tick, 1000000);
> 	return tick;
> }
>
> void __weak __udelay(unsigned long usec)
> {
>     800197f8:	1101                	addi	sp,sp,-32
>     800197fa:	ec06                	sd	ra,24(sp)
>     800197fc:	e822                	sd	s0,16(sp)
>     800197fe:	e426                	sd	s1,8(sp)
>     80019800:	84aa                	mv	s1,a0
> 	uint64_t tmp;
>
> 	tmp = get_ticks() + usec_to_tick(usec);	/* get current timestamp */
>     80019802:	f7bff0ef          	jal	ra,8001977c <get_ticks>
>     80019806:	842a                	mv	s0,a0
>     80019808:	8526                	mv	a0,s1
>     8001980a:	fcbff0ef          	jal	ra,800197d4 <usec_to_tick>
>     8001980e:	942a                	add	s0,s0,a0
>
> 	while (get_ticks() < tmp+1)	/* loop till event */
>     80019810:	0405                	addi	s0,s0,1
>     80019812:	f6bff0ef          	jal	ra,8001977c <get_ticks>
>     80019816:	fe856ee3          	bltu	a0,s0,80019812 <__udelay+0x1a>
> 		 /*NOP*/;
> }
>     8001981a:	60e2                	ld	ra,24(sp)
>     8001981c:	6442                	ld	s0,16(sp)
>     8001981e:	64a2                	ld	s1,8(sp)
>     80019820:	6105                	addi	sp,sp,32
>     80019822:	8082                	ret
>
> --Sean
>

[PATCH 1/1] clk: kendryte/pll.h: do not redefine nop()

[Heinrich Schuchardt]

On 8/5/20 2:19 PM, Heinrich Schuchardt wrote:
> On 05.08.20 13:45, Sean Anderson wrote:
>> On 8/3/20 2:20 PM, Heinrich Schuchardt wrote:
>>> On 03.08.20 16:08, Sean Anderson wrote:
>>>> Maybe. Because we are configuring the PLL, the CPU clock is temporarily
>>>> set to the in0 oscillator, so the timer would give an incorrect delay.
>>>> However, it would probably be fine even if incorrect.  The original
>>>> reason I used nops is because that is what the SDK does in its PLL
>>>> configuration function [1]. I believe I tried using udelay at one point,
>>>> but I don't remember whether I had any issues with it.
>>>>
>>>> --Sean
>>>>
>>>> [1] https://github.com/kendryte/kendryte-standalone-sdk/blob/develop/lib/drivers/sysctl.c#L844
>>>>
>>>
>>> This is what udelay(1) gets compiled to:
>>>
>>> 000000000000004a <.LBE106>:
>>>         udelay(1);
>>>   4a:   4505                    li      a0,1
>>>
>>> 000000000000004c <.LVL21>:
>>>   4c:   00000097                auipc   ra,0x0
>>>   50:   000080e7                jalr    ra # 4c <.LVL21>
>>>
>>> Somehow udelay() seems not to be fully implemented and the platform. So
>>> I think we should stay with the nop() macro.
>>
>> Hm, I don't see this generated code. What function is that a disassembly
>> of?
>
> I was disassembling
>
> riscv64-linux-gnu-objdump -D -S drivers/clk/kendryte/pll.o
>
> If I disassemble the file u-boot function k210_pll_enable seems to be
> ok. nop()s replaced by udelay(1):
>
> ??????? writel(reg, pll->reg);
>     80012524:???7518                ????ld??????a4,40(a0)
> ????????__iowmb();
>     80012526:???0550000f          ??????fence???ow,ow
> ????????reg |= K210_PLL_RESET;
>     8001252a:???003006b7          ??????lui?????a3,0x300
>     8001252e:???8fd5                ????or??????a5,a5,a3
> ????????__arch_putl(val, addr);
>     80012530:???c31c                ????sw??????a5,0(a4)
> ????????udelay(1);
>     80012532:???4505                ????li??????a0,1
>     80012534:???612200ef          ??????jal?????ra,80032b46 <udelay>
> ????????writel(reg, pll->reg);
>     80012538:???741c                ????ld??????a5,40(s0)
> ????????__iowmb();
>     8001253a:???0550000f          ??????fence???ow,ow
>
> So it was simply the link step missing to show the call.
>
> But anyway which way do you want the patch?

Hello Sean,

did I miss you reply?

Best regards

Heinrich

>
> Best regards
>
> Heinrich
>
>>
>> $ riscv64-linux-gnu-objdump -S --disassemble=__udelay u-boot
>>
>> u-boot:     file format elf64-littleriscv
>>
>>
>> Disassembly of section .text:
>>
>> Disassembly of section .text_rest:
>>
>> 00000000800197f8 <__udelay>:
>> 	do_div(tick, 1000000);
>> 	return tick;
>> }
>>
>> void __weak __udelay(unsigned long usec)
>> {
>>     800197f8:	1101                	addi	sp,sp,-32
>>     800197fa:	ec06                	sd	ra,24(sp)
>>     800197fc:	e822                	sd	s0,16(sp)
>>     800197fe:	e426                	sd	s1,8(sp)
>>     80019800:	84aa                	mv	s1,a0
>> 	uint64_t tmp;
>>
>> 	tmp = get_ticks() + usec_to_tick(usec);	/* get current timestamp */
>>     80019802:	f7bff0ef          	jal	ra,8001977c <get_ticks>
>>     80019806:	842a                	mv	s0,a0
>>     80019808:	8526                	mv	a0,s1
>>     8001980a:	fcbff0ef          	jal	ra,800197d4 <usec_to_tick>
>>     8001980e:	942a                	add	s0,s0,a0
>>
>> 	while (get_ticks() < tmp+1)	/* loop till event */
>>     80019810:	0405                	addi	s0,s0,1
>>     80019812:	f6bff0ef          	jal	ra,8001977c <get_ticks>
>>     80019816:	fe856ee3          	bltu	a0,s0,80019812 <__udelay+0x1a>
>> 		 /*NOP*/;
>> }
>>     8001981a:	60e2                	ld	ra,24(sp)
>>     8001981c:	6442                	ld	s0,16(sp)
>>     8001981e:	64a2                	ld	s1,8(sp)
>>     80019820:	6105                	addi	sp,sp,32
>>     80019822:	8082                	ret
>>
>> --Sean
>>
>

[PATCH 1/1] clk: kendryte/pll.h: do not redefine nop()

[Sean Anderson]

On 8/15/20 3:53 AM, Heinrich Schuchardt wrote:
> On 8/5/20 2:19 PM, Heinrich Schuchardt wrote:
>> On 05.08.20 13:45, Sean Anderson wrote:
>>> On 8/3/20 2:20 PM, Heinrich Schuchardt wrote:
>>>> On 03.08.20 16:08, Sean Anderson wrote:
>>>>> Maybe. Because we are configuring the PLL, the CPU clock is temporarily
>>>>> set to the in0 oscillator, so the timer would give an incorrect delay.
>>>>> However, it would probably be fine even if incorrect.  The original
>>>>> reason I used nops is because that is what the SDK does in its PLL
>>>>> configuration function [1]. I believe I tried using udelay at one point,
>>>>> but I don't remember whether I had any issues with it.
>>>>>
>>>>> --Sean
>>>>>
>>>>> [1] https://github.com/kendryte/kendryte-standalone-sdk/blob/develop/lib/drivers/sysctl.c#L844
>>>>>
>>>>
>>>> This is what udelay(1) gets compiled to:
>>>>
>>>> 000000000000004a <.LBE106>:
>>>>         udelay(1);
>>>>   4a:   4505                    li      a0,1
>>>>
>>>> 000000000000004c <.LVL21>:
>>>>   4c:   00000097                auipc   ra,0x0
>>>>   50:   000080e7                jalr    ra # 4c <.LVL21>
>>>>
>>>> Somehow udelay() seems not to be fully implemented and the platform. So
>>>> I think we should stay with the nop() macro.
>>>
>>> Hm, I don't see this generated code. What function is that a disassembly
>>> of?
>>
>> I was disassembling
>>
>> riscv64-linux-gnu-objdump -D -S drivers/clk/kendryte/pll.o
>>
>> If I disassemble the file u-boot function k210_pll_enable seems to be
>> ok. nop()s replaced by udelay(1):
>>
>> ??????? writel(reg, pll->reg);
>>     80012524:???7518                ????ld??????a4,40(a0)
>> ????????__iowmb();
>>     80012526:???0550000f          ??????fence???ow,ow
>> ????????reg |= K210_PLL_RESET;
>>     8001252a:???003006b7          ??????lui?????a3,0x300
>>     8001252e:???8fd5                ????or??????a5,a5,a3
>> ????????__arch_putl(val, addr);
>>     80012530:???c31c                ????sw??????a5,0(a4)
>> ????????udelay(1);
>>     80012532:???4505                ????li??????a0,1
>>     80012534:???612200ef          ??????jal?????ra,80032b46 <udelay>
>> ????????writel(reg, pll->reg);
>>     80012538:???741c                ????ld??????a5,40(s0)
>> ????????__iowmb();
>>     8001253a:???0550000f          ??????fence???ow,ow
>>
>> So it was simply the link step missing to show the call.
>>
>> But anyway which way do you want the patch?

I think the patch as-is is fine.

--Sean

> 
> Hello Sean,
> 
> did I miss you reply?> 
> Best regards
> 
> Heinrich
> 
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> $ riscv64-linux-gnu-objdump -S --disassemble=__udelay u-boot
>>>
>>> u-boot:     file format elf64-littleriscv
>>>
>>>
>>> Disassembly of section .text:
>>>
>>> Disassembly of section .text_rest:
>>>
>>> 00000000800197f8 <__udelay>:
>>> 	do_div(tick, 1000000);
>>> 	return tick;
>>> }
>>>
>>> void __weak __udelay(unsigned long usec)
>>> {
>>>     800197f8:	1101                	addi	sp,sp,-32
>>>     800197fa:	ec06                	sd	ra,24(sp)
>>>     800197fc:	e822                	sd	s0,16(sp)
>>>     800197fe:	e426                	sd	s1,8(sp)
>>>     80019800:	84aa                	mv	s1,a0
>>> 	uint64_t tmp;
>>>
>>> 	tmp = get_ticks() + usec_to_tick(usec);	/* get current timestamp */
>>>     80019802:	f7bff0ef          	jal	ra,8001977c <get_ticks>
>>>     80019806:	842a                	mv	s0,a0
>>>     80019808:	8526                	mv	a0,s1
>>>     8001980a:	fcbff0ef          	jal	ra,800197d4 <usec_to_tick>
>>>     8001980e:	942a                	add	s0,s0,a0
>>>
>>> 	while (get_ticks() < tmp+1)	/* loop till event */
>>>     80019810:	0405                	addi	s0,s0,1
>>>     80019812:	f6bff0ef          	jal	ra,8001977c <get_ticks>
>>>     80019816:	fe856ee3          	bltu	a0,s0,80019812 <__udelay+0x1a>
>>> 		 /*NOP*/;
>>> }
>>>     8001981a:	60e2                	ld	ra,24(sp)
>>>     8001981c:	6442                	ld	s0,16(sp)
>>>     8001981e:	64a2                	ld	s1,8(sp)
>>>     80019820:	6105                	addi	sp,sp,32
>>>     80019822:	8082                	ret
>>>
>>> --Sean
>>>
>>
> 

[PATCH 1/1] clk: kendryte/pll.h: do not redefine nop()

[Bin Meng]

On Tue, Jul 28, 2020 at 11:52 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> The kendryte PLL code uses nop as barrier. The macro is not defined for
> the sandbox on x86 but is defined on RISC-V.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  include/kendryte/pll.h | 5 +++++
>  1 file changed, 5 insertions(+)
>

Reviewed-by: Bin Meng <bin.meng@windriver.com>