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

[Anup Patel <anup@brainfault.org>]

On Sat, Mar 7, 2020 at 5:48 AM Sean Anderson <seanga2@gmail.com> wrote:
>
> 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";

Yes, using "clock-names" to distinguish different clocks
of same device is the right way.

Regards,
Anup

> };
>
> --Sean
>