[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 at 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 at 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