Re: [PATCH 08/10] riscv: Add Kendryte K210 device tree

[Sean Anderson <seanga2@gmail.com>]

On 3/2/20 12:08 AM, Damien Le Moal wrote:
> On 2020/03/02 14:05, Anup Patel wrote:
>> On Mon, Mar 2, 2020 at 10:21 AM Damien Le Moal <Damien.LeMoal@wdc.com> wrote:
>>>
>>> On 2020/03/02 13:22, Anup Patel wrote:
>>>> On Mon, Mar 2, 2020 at 9:46 AM Damien Le Moal <Damien.LeMoal@wdc.com> wrote:
>>>>>
>>>>> On 2020/03/02 13:07, Anup Patel wrote:
>>>>> [...]
>>>>>>> +       sram0: memory@80000000 {
>>>>>>> +               device_type = "memory";
>>>>>>> +               reg = <0x80000000 0x400000>;
>>>>>>> +       };
>>>>>>> +
>>>>>>> +       sram1: memory@80400000 {
>>>>>>> +               device_type = "memory";
>>>>>>> +               reg = <0x80400000 0x200000>;
>>>>>>> +       };
>>>>>>> +
>>>>>>> +       kpu_sram: memory@80600000 {
>>>>>>> +               device_type = "memory";
>>>>>>> +               reg = <0x80600000 0x200000>;
>>>>>>> +       };
>>>>>>
>>>>>> No need to have separate DT node for each RAM bank. This can be
>>>>>> express as single DT node as follows:
>>>>>>
>>>>>> sram: memory@80000000 {
>>>>>>         device_type = "memory";
>>>>>>         reg = <0x80000000 0x400000>,
>>>>>>                   <0x80400000 0x200000>,
>>>>>>                   <0x80600000 0x200000>;
>>>>>> };
>>>>>
>>>>> This is to match the U-boot device tree that Sean wrote. So I would rather keep
>>>>> it like this. And strictly speaking, if one wants to add a driver for the KPU,
>>>>> having the kpu memory segment for it separate makes it easy to reference it from
>>>>> a kpu device entry. But granted, the two sram segments can be declared with a
>>>>> single memory entry.

There is no clear documentation on how to do this, so I have been mostly
just trying things until they work. In U-Boot, separate memory device
nodes are treated as different "banks".

>>>>
>>>> But, that's not the preferred way of describing memory banks on the
>>>> same machine.
>>>> Usually, we create multiple memory DT nodes for NUMA systems.
>>>>
>>>> You can also refer various ARM/ARM64 DTS files.
>>>
>>> Oops... Sent an answer to this to the wrong email... Here it is again:
>>>
>>> Yes, I understand. But in the case of the K210, that last 2MB segment is really
>>> special as by default it is not usable as regular SRAM. I think it may be better
>>> to reflect that in the device tree. The K210 soc_early_init() call back can
>>> probe for that special entry too to see if it has to be turned on for use as
>>> regular memory or not (i.e. if a kpu driver will use it).
>>>
>>> Since booting Linux with 6MB of memory will be even more challenging than with
>>> 8, I agree that we may never see the case of a kpu driver being used. But I
>>> personally like making that special case clear in the device tree. No strong
>>> objection to your simplification though. So if you really object, I will go with it.
>>>
>>
>> I understand that it is helping you to distinguish last 2MB segment but this is
>> also possible using with single memory DT node as follows:
>>
>> sram: memory@80000000 {
>>         device_type = "memory";
>>         reg = <0x80000000 0x400000>,
>>                   <0x80400000 0x200000>,
>>                   <0x80600000 0x200000>;
>>         reg-names = "sram0", "sram1", "kpu_sram";
>> };
> 
> Nice trick. I did not know about it. Will use that then !
>>
>> The K210 soc_early_init() can do the following:
>> 1. Find memory DT node having device_type = "memory"
>> 2. Find bank number for "kpu_sram" based on "reg-names DT property
>> 3. Get based address of KPU SRAM from "reg" property based on bank
>> number found in step2 above.
>>
>> The reg-names is a standard DT property used to distinguish multiple
>> memory regions of device. Same can be used to distinguish multiple
>> banks of memory DT node.
>>
>> I am not adamant on having single memory DT node but just wanted
>> to let you know that this is not a preferred way for non-NUMA system.

Anup, do you have any suggestions on how to describe clocks for each
bank? I think the kpu sram may need some clock manipulation to work
properly. Perhaps something like

sram: memory@80000000 {
	device_type = "memory";
	reg = <0x80000000 0x400000>,
	      <0x80400000 0x200000>,
	      <0x80600000 0x200000>;
	reg-names = "sram0", "sram1", "kpu_sram";
	clocks = <&sysclk K210_CLK_SRAM0>,
		 <&sysclk K210_CLK_SRAM1>,
		 <&sysclk K210_CLK_PLL1>;
	clock-names = "sram0", "sram1", "kpu";
};

--Sean