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

[Damien Le Moal]

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.

> 
> --Sean
> 
>>
>> Rob
>>
> 
> 


-- 
Damien Le Moal
Western Digital Research