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

From: Sean Anderson <seanga2@gmail.com>
To: Damien Le Moal <Damien.LeMoal@wdc.com>, Rob Herring <robh@kernel.org>
Cc: Atish Patra <Atish.Patra@wdc.com>,
	"linux-riscv@lists.infradead.org"
	<linux-riscv@lists.infradead.org>,
	Anup Patel <Anup.Patel@wdc.com>,
	"palmer@dabbelt.com" <palmer@dabbelt.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>
Subject: Re: [PATCH v16 09/16] riscv: Update Canaan Kendryte K210 device tree
Date: Mon, 8 Feb 2021 18:04:55 -0500	[thread overview]
Message-ID: <e451a093-a37f-6078-d158-965b7fce4ebf@gmail.com> (raw)
In-Reply-To: <BL0PR04MB6514747C7AD8FA7E56D61CB9E78F9@BL0PR04MB6514.namprd04.prod.outlook.com>

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

--Sean

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv