Re: [PATCH] riscv: Fix Kendryte K210 device tree

[Damien Le Moal <Damien.LeMoal@wdc.com>]

On 2020/09/16 11:15, Sean Anderson wrote:
> On 9/15/20 8:55 AM, Damien Le Moal wrote:
>> The Kendryte K210 SoC CLINT is compatible with Sifive clint v0
>> (sifive,clint0). Fix the Kendryte K210 device tree clint entry to be
>> inline with the sifive timer definition documented in
>> Documentation/devicetree/bindings/timer/sifive,clint.yaml. Rename the
>> clint0 entry to "timer", fix the register mapping and fix the interrupt
>> extended entry.
>>
>> This fixes boot failures with Kendryte K210 SoC boards.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>> ---
>>  arch/riscv/boot/dts/kendryte/k210.dtsi | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/riscv/boot/dts/kendryte/k210.dtsi b/arch/riscv/boot/dts/kendryte/k210.dtsi
>> index c1df56ccb8d5..48d65fea45e9 100644
>> --- a/arch/riscv/boot/dts/kendryte/k210.dtsi
>> +++ b/arch/riscv/boot/dts/kendryte/k210.dtsi
>> @@ -95,10 +95,11 @@ sysctl: sysctl@50440000 {
>>  			#clock-cells = <1>;
>>  		};
>>  
>> -		clint0: interrupt-controller@2000000 {
>> +		timer@2000000 {
>>  			compatible = "riscv,clint0";
>> -			reg = <0x2000000 0xC000>;
>> -			interrupts-extended = <&cpu0_intc 3>,  <&cpu1_intc 3>;
>> +			reg = <0x2000000 0x10000>;
> 
> Why the change in size? The docs from the SDK [1] (which I think were copied
> from SiFive documentation around the time of their release) state that
> the CLINT is 0xC000 bytes. In addition, accessing memory past 0x0200C000
> yields a repeating memory pattern:

Oops. Copy-paste error. I will change that back to 0x0000C000.
> 
> 
> 0200c000: 69823183 02020120 6083c006 00008002    .1.i ......`....
> 0200c010: 69823183 02020120 6083c006 00008002    .1.i ......`....
> 0200c020: 69823183 02020120 6083c006 00008002    .1.i ......`....
> 0200c030: 69823183 02020120 6083c006 00008002    .1.i ......`....
> (continues until 0x02010000)
> 
> To me, it is clear that the CLINT is not 0x10000 in size, even though
> the legal address space extends to that size.
> 
> I am more curious how this fixes booting. What was the issue before?

There were 2 problems:
1) The old declaration of the entry as clint0: interrupt-controller@2000000 led
to a failure to find the driver.
2) The interrupt extended entry was wrong

> 
>> +			interrupts-extended =  <&cpu0_intc 3 &cpu0_intc 7
>> +						&cpu1_intc 3 &cpu1_intc 7>;
>>  			clocks = <&sysctl K210_CLK_ACLK>;
> 
> This clock is 50 times too fast(!) See [2] and [3] for a fix. I don't
> know if adding a "virtual" clock is the best solution for Linux. I would
> like to keep the device trees relatively in-sync, so please let me know
> if you think there is a better solution.

Huu... Let me check that. Once booted, using commands such as date or time does
not give me weird results, well at least, before this patch and clint changes. I
forgot to check yesterday when I tested (I only boot tested). Will check.

And yes, I will make effort to keep the DT in sync. Is the Kendryte support
upstream in U-Boot now ? I have not checked recently.

> 
>>  		};
>>  
>>
> 
> [1] https://github.com/kendryte/kendryte-standalone-sdk/blob/develop/lib/drivers/include/clint.h
> [2] https://patchwork.ozlabs.org/project/uboot/patch/20200915141724.503929-8-seanga2@gmail.com/
> [3] https://patchwork.ozlabs.org/project/uboot/patch/20200915141724.503929-9-seanga2@gmail.com/
> 


-- 
Damien Le Moal
Western Digital Research

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv