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

On 2021/02/09 8:05, Sean Anderson wrote:

On 2/8/21 5:55 PM, Damien Le Moal wrote:

quoted

On 2021/02/09 7:53, Sean Anderson wrote:

quoted

On 2/8/21 3:00 PM, Rob Herring wrote:

quoted

On Fri, Feb 5, 2021 at 6:13 PM Damien Le Moal [off-list ref] wrote:

quoted

On Fri, 2021-02-05 at 14:25 -0600, Rob Herring wrote:
[...]

quoted

quoted

+                   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.

quoted

quoted

+
+                           /* Bootloader */
+                           firmware@00000 {

Drop leading 0s.

Is this memory mapped? If so, you are missing 'ranges' in the parent to
make it translateable.

quoted

+                                   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.

quoted

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.

quoted

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.

quoted

quoted

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