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@xxxxxxx> 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,
>>
>> 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
