[PATCH v16 09/16] riscv: Update Canaan Kendryte K210 device tree

[Damien Le Moal]

On 2021/02/09 8:05, Sean Anderson wrote:
> On 2/8/21 5:55 PM, Damien Le Moal wrote:
>> On 2021/02/09 7:53, Sean Anderson wrote:
>>> On 2/8/21 3:00 PM, Rob Herring wrote:
>>>> On Fri, Feb 5, 2021 at 6:13 PM Damien Le Moal <Damien.LeMoal at wdc.com> wrote:
>>>>>
>>>>> On Fri, 2021-02-05 at 14:25 -0600, Rob Herring wrote:
>>>>> [...]
>>>>>>> +                   otp0: nvmem at 50420000 {
>>>>>>> +                           #address-cells = <1>;
>>>>>>> +                           #size-cells = <1>;
>>>>>>> +                           compatible = "canaan,k210-otp";
>>>>>>> +                           reg = <0x50420000 0x100>,
>>>>>>> +                                 <0x88000000 0x20000>;
>>>>>>> +                           reg-names = "reg", "mem";
>>>>>>> +                           clocks = <&sysclk K210_CLK_ROM>;
>>>>>>> +                           resets = <&sysrst K210_RST_ROM>;
>>>>>>> +                           read-only;
>>>>>>> +                           status = "disabled";
>>>>>>
>>>>>> Your disabled nodes seem a bit excessive. A device should really only be
>>>>>> disabled if it's a board level decision to use or not. I'd assume the
>>>>>> OTP is always there and usable.
>>>>>
>>>>> Please see below.
>>>>>
>>>>>>
>>>>>>> +
>>>>>>> +                           /* Bootloader */
>>>>>>> +                           firmware at 00000 {
>>>>>>
>>>>>> Drop leading 0s.
>>>>>>
>>>>>> Is this memory mapped? If so, you are missing 'ranges' in the parent to
>>>>>> make it translateable.
>>>>>>
>>>>>>> +                                   reg = <0x00000 0xC200>;
>>>>>>> +                           };
>>>>>>> +
>>>>>>> +                           /*
>>>>>>> +                            * config string as described in RISC-V
>>>>>>> +                            * privileged spec 1.9
>>>>>>> +                            */
>>>>>>> +                           config-1-9 at 1c000 {
>>>>>>> +                                   reg = <0x1C000 0x1000>;
>>>>>>> +                           };
>>>>>>> +
>>>>>>> +                           /*
>>>>>>> +                            * Device tree containing only registers,
>>>>>>> +                            * interrupts, and cpus
>>>>>>> +                            */
>>>>>>> +                           fdt at 1d000 {
>>>>>>> +                                   reg = <0x1D000 0x2000>;
>>>>>>> +                           };
>>>>>>> +
>>>>>>> +                           /* CPU/ROM credits */
>>>>>>> +                           credits at 1f000 {
>>>>>>> +                                   reg = <0x1F000 0x1000>;
>>>>>>> +                           };
>>>>>>> +                   };
>>>>>>> +
>>>>>>> +                   dvp0: camera at 50430000 {
>>>>>>> +                           compatible = "canaan,k210-dvp";
>>>>>>
>>>>>> No documented. Seems to be several of them.
>>>>>
>>>>> There are no Linux drivers for these undocumented nodes. That is why I did not
>>>>> add any documentation.
>>>>
>>>> Documentation is required when dts files OR Linux drivers use them.
>>>>
>>>>> make dtbs_check does not complain about that as long as
>>>>> the nodes are marked disabled.
>>>>
>>>> 'disabled' should only turn off required properties missing checks.
>>>> Undocumented compatible strings checks are about to get turned on now
>>>> that I've made it work without false positives.
>>>>
>>>>> I kept these nodes to have the DTS in sync with
>>>>> U-Boot which has them.
>>>>
>>>> That's a worthwhile goal. Doesn't u-boot require documenting bindings?
>>>
>>> Generally, no. Usually if the bindings differ from the kernel they are
>>> documented, but usually the device trees are just imported straight from
>>> the kernel. This is a bit of an unusual case in that the device tree is
>>> being imported from U-Boot instead of the other way around.
>>>
>>>>
>>>>> Keeping them also creates documentation for the SoC
>>>>> since this device tree is more detailed than the SoC specsheet...
>>>>
>>>> It's already 'documented' in u-boot it seems...
>>>
>>> I would like to keep Kernel and U-Boot device trees in-sync. However, if
>>> there are significant divergences, that becomes more difficult.
>>
>> Sean,
>>
>> Are you OK with removing the nodes without a driver ? I think they are the same
>> for the kernel and U-Boot but I have not checked in detail.
> 
> I suppose. The 8285 uarts should be kept as iirc someone was using them.
> Same with i2c. WDT has a U-Boot driver, and probably has a Linux one too
> (I haven't checked). I believe the timers also have working drivers, but
> I haven't tested on Linux.

Yep, all of these are supported and work. I used i2c a lot for my biped robot
and robot arm control. The watchdog timers also work but I did not enable the
drivers by default in the kernel config.

Preparing v18 with the updated dtsi then. I do not think we need to change the
dts for the boards. Will check.

Thanks !

-- 
Damien Le Moal
Western Digital Research