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

[Sean Anderson <seanga2@gmail.com>]

On 2/8/21 3:00 PM, Rob Herring wrote:
> On Fri, Feb 5, 2021 at 6:13 PM Damien Le Moal <Damien.LeMoal@wdc.com> wrote:
>>
>> On Fri, 2021-02-05 at 14:25 -0600, Rob Herring wrote:
>> [...]
>>>> +                   otp0: nvmem@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@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@1c000 {
>>>> +                                   reg = <0x1C000 0x1000>;
>>>> +                           };
>>>> +
>>>> +                           /*
>>>> +                            * Device tree containing only registers,
>>>> +                            * interrupts, and cpus
>>>> +                            */
>>>> +                           fdt@1d000 {
>>>> +                                   reg = <0x1D000 0x2000>;
>>>> +                           };
>>>> +
>>>> +                           /* CPU/ROM credits */
>>>> +                           credits@1f000 {
>>>> +                                   reg = <0x1F000 0x1000>;
>>>> +                           };
>>>> +                   };
>>>> +
>>>> +                   dvp0: camera@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

> 
> Rob
>