Re: [PATCH v11 03/10] riscv: Update Canaan Kendryte K210 device tree

[Sean Anderson <seanga2@gmail.com>]

On 1/14/21 8:03 PM, Damien Le Moal wrote:
> On 2021/01/15 9:35, Sean Anderson wrote:
>> On 1/14/21 7:06 PM, Sean Anderson wrote:
>>>
>>> On 1/14/21 7:01 PM, Sean Anderson wrote:
>>>>
>>>> On 1/14/21 6:32 PM, Palmer Dabbelt wrote:
>>>>> On Mon, 11 Jan 2021 16:58:41 PST (-0800), Damien Le Moal wrote:
>>>>>> Update the Canaan Kendryte K210 base device tree k210.dtsi to define
>>>>>> all peripherals of the SoC, their clocks and reset lines. The device
>>>>>> tree file k210.dts is renamed to k210_generic.dts and becomes the
>>>>>> default value selection of the SOC_CANAAN_K210_DTB_BUILTIN_SOURCE
>>>>>> configuration option. No device beside the serial console is defined by
>>>>>> this device tree. This makes this generic device tree suitable for use
>>>>>> with a builtin initramfs with all known K210 based boards.
>>>>>>
>>>>>> These changes result in the K210_CLK_ACLK clock ID to be unused and
>>>>>> removed from the dt-bindings k210-clk.h header file.
>>>>>>
>>>>>> Most updates to the k210.dtsi file come from Sean Anderson's work on
>>>>>> U-Boot support for the K210.
>>>>>>
>>>>>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>>>>>> Reviewed-by: Anup Patel <anup@brainfault.org>
>>>>>> ---
>>>>>>   arch/riscv/Kconfig.socs                     |   2 +-
>>>>>>   arch/riscv/boot/dts/canaan/k210.dts         |  23 -
>>>>>>   arch/riscv/boot/dts/canaan/k210.dtsi        | 551 +++++++++++++++++++-
>>>>>>   arch/riscv/boot/dts/canaan/k210_generic.dts |  46 ++
>>>>>>   include/dt-bindings/clock/k210-clk.h        |   1 -
>>>>>>   5 files changed, 573 insertions(+), 50 deletions(-)
>>>>>>   delete mode 100644 arch/riscv/boot/dts/canaan/k210.dts
>>>>>>   create mode 100644 arch/riscv/boot/dts/canaan/k210_generic.dts
>>>>>
>>>>> [Snipping this to the relevant bits, in case you missed it before.]
>>>>>
>>>>>> @@ -81,40 +107,515 @@ in0: oscillator {
>>>>>>       soc {
>>>>>>           #address-cells = <1>;
>>>>>>           #size-cells = <1>;
>>>>>> -        compatible = "kendryte,k210-soc", "simple-bus";
>>>>>> +        compatible = "canaan,k210-soc", "simple-bus";
>>>>>>           ranges;
>>>>>>           interrupt-parent = <&plic0>;
>>>>>>
>>>>>> -        sysctl: sysctl@50440000 {
>>>>>> -            compatible = "kendryte,k210-sysctl", "simple-mfd";
>>>>>> -            reg = <0x50440000 0x1000>;
>>>>>> -            #clock-cells = <1>;
>>>>>> +        debug0: debug@0 {
>>>>>> +            compatible = "canaan,k210-debug", "riscv,debug";
>>>>>
>>>>> I'm still getting lots of warnings about undocumented DT compatible strings
>>>>> from checpatch.  Some of them might be in flight, but I don't see many of them
>>>>> (including both of these debug ones) having been defined anywhere.  We went
>>>>> through a whole process to sort out the SiFive DT naming conventions, I don't
>>>>> want to just circumvent that for the Canaan stuff by merging it as-is.
>>>>
>>>> As far as I'm aware, it's recommended practice to add device-specific compatible
>>>>
>>>
>>> Here it's because "riscv,debug" doesn't exist. This is the "debug"
>>> device as described in the debug spec. AFAIK Linux never needs to
>>> configure this device. It could probably be removed.
>>>
>>> I am going to try and go through the list of nonexistant compatibles and
>>> see if there are any other devices like this (nothing else like it in
>>> Linux).
>>>
>>> --Sean
>>
>> Ok, here is the (abbreviated) output:
>>
>>
>>> cpu@0: compatible: ['canaan,k210', 'sifive,rocket0', 'riscv'] is not valid under any of the given schemas (Possible causes of the failure):
>>> cpu@0: compatible: ['canaan,k210', 'sifive,rocket0', 'riscv'] is too long
>>> cpu@0: compatible:0: 'canaan,k210' is not one of ['sifive,rocket0', 'sifive,bullet0', 'sifive,e5', 'sifive,e7', 'sifive,e51', 'sifive,e71', 'sifive,u54-mc', 'sifive,u74-mc', 'sifive,u54', 'sifive,u74', 'sifive,u5', 'sifive,u7']
>>
>> This is a device-specific compatible string as recommended by the
>> current DT docs.
>>
>>> cpu@0: mmu-type:0: 'none' is not one of ['riscv,sv32', 'riscv,sv39', 'riscv,sv48']
>>
>> This should be added (though it is technically incorrect).
> 
> will do.
> 
>>
>> (perhaps a version number should be added like 'riscv,sv39-1.10')
>>
>>> cpu@0: riscv,isa:0: 'rv64imafdgc' is not one of ['rv64imac', 'rv64imafdc']
>>
>> Should probably be fixed, as the g is redundant. However, this is
>> probably not a good warning going forward, as more exotic combinations
>> of extensions are implemented.
> 
> I had removed the "g" here. I guess I messed up a rebase and it is back. Will
> fix that.
> 
>>
>>> cpu@1: compatible: ['canaan,k210', 'sifive,rocket0', 'riscv'] is not valid under any of the given schemas (Possible causes of the failure):
>>> cpu@1: compatible: ['canaan,k210', 'sifive,rocket0', 'riscv'] is too long
>>> cpu@1: compatible:0: 'canaan,k210' is not one of ['sifive,rocket0', 'sifive,bullet0', 'sifive,e5', 'sifive,e7', 'sifive,e51', 'sifive,e71', 'sifive,u54-mc', 'sifive,u74-mc', 'sifive,u54', 'sifive,u74', 'sifive,u5', 'sifive,u7']
>>>
>>> cpu@1: mmu-type:0: 'none' is not one of ['riscv,sv32', 'riscv,sv39', 'riscv,sv48']
>>> cpu@1: riscv,isa:0: 'rv64imafdgc' is not one of ['rv64imac', 'rv64imafdc']
>>
>> see above
>>
>>> serial@38000000: compatible:0: 'canaan,k210-uarths' is not one of ['sifive,fu540-c000-uart', 'sifive,fu740-c000-uart']
>>
>> device-specific, and intentional
> 
> We can add this one to the sifive serial yaml.
> 
>>
>>> gpio-controller@38001000: compatible:0: 'canaan,k210-gpiohs' is not one of ['sifive,fu540-c000-gpio', 'sifive,fu740-c000-gpio']
>>
>> ditto
>>
>>> gpio-controller@38001000: 'ngpios' does not match any of the regexes: 'pinctrl-[0-9]+'
>>
>> known shortcoming with this dt property, but alas
> 
> Yes. The driver was fixed. But the yaml is behind I guess. Will send a patch for
> this one.
> 
>>
>>> gpio-controller@50200000: $nodename:0: 'gpio-controller@50200000' does not match '^gpio@[0-9a-f]+$'
>>
>> This matches devicetree/bindings/gpio/snps,dw-apb-gpio.yaml
> 
> This may be similar to the ngpios problem: the core expect something while the
> yaml defines something older... Will have a look.
> 
>>
>>> gpio-controller@50200000: compatible: ['canaan,k210-gpio', 'snps,dw-apb-gpio'] is too long
>>> gpio-controller@50200000: compatible: Additional items are not allowed ('snps,dw-apb-gpio' was unexpected)
> 
> I wonder if we can just remove 'canaan,k210-gpio' here ?
> 
>>> gpio-controller@50200000: 'gpio1@0' does not match any of the regexes: '^gpio-(port|controller)@[0-9a-f]+$', 'pinctrl-[0-9]+'
> 
> Not sure what to do about this one.
> 
>>> serial@50210000: compatible: ['canaan,k210-uart', 'snps,dw-apb-uart'] is not valid under any of the given schemas (Possible causes of the failure):
>>> serial@50210000: compatible: ['canaan,k210-uart', 'snps,dw-apb-uart'] is too long
>>> serial@50210000: compatible:0: 'canaan,k210-uart' is not one of ['renesas,r9a06g032-uart', 'renesas,r9a06g033-uart']
>>> serial@50210000: compatible:0: 'canaan,k210-uart' is not one of ['rockchip,px30-uart', 'rockchip,rk3036-uart', 'rockchip,rk3066-uart', 'rockchip,rk3188-uart', 'rockchip,rk3288-uart', 'rockchip,rk3308-uart', 'rockchip,rk3328-uart', 'rockchip,rk3368-uart', 'rockchip,rk3399-uart', 'rockchip,rv1108-uart']
>>> serial@50210000: compatible:0: 'canaan,k210-uart' is not one of ['brcm,bcm11351-dw-apb-uart', 'brcm,bcm21664-dw-apb-uart']
>>>
>>> serial@50220000: compatible: ['canaan,k210-uart', 'snps,dw-apb-uart'] is not valid under any of the given schemas (Possible causes of the failure):
>>> serial@50220000: compatible: ['canaan,k210-uart', 'snps,dw-apb-uart'] is too long
>>> serial@50220000: compatible:0: 'canaan,k210-uart' is not one of ['renesas,r9a06g032-uart', 'renesas,r9a06g033-uart']
>>> serial@50220000: compatible:0: 'canaan,k210-uart' is not one of ['rockchip,px30-uart', 'rockchip,rk3036-uart', 'rockchip,rk3066-uart', 'rockchip,rk3188-uart', 'rockchip,rk3288-uart', 'rockchip,rk3308-uart', 'rockchip,rk3328-uart', 'rockchip,rk3368-uart', 'rockchip,rk3399-uart', 'rockchip,rv1108-uart']
>>> serial@50220000: compatible:0: 'canaan,k210-uart' is not one of ['brcm,bcm11351-dw-apb-uart', 'brcm,bcm21664-dw-apb-uart']
>>>
>>> serial@50230000: compatible: ['canaan,k210-uart', 'snps,dw-apb-uart'] is not valid under any of the given schemas (Possible causes of the failure):
>>> serial@50230000: compatible: ['canaan,k210-uart', 'snps,dw-apb-uart'] is too long
>>> serial@50230000: compatible:0: 'canaan,k210-uart' is not one of ['renesas,r9a06g032-uart', 'renesas,r9a06g033-uart']
>>> serial@50230000: compatible:0: 'canaan,k210-uart' is not one of ['rockchip,px30-uart', 'rockchip,rk3036-uart', 'rockchip,rk3066-uart', 'rockchip,rk3188-uart', 'rockchip,rk3288-uart', 'rockchip,rk3308-uart', 'rockchip,rk3328-uart', 'rockchip,rk3368-uart', 'rockchip,rk3399-uart', 'rockchip,rv1108-uart']
>>> serial@50230000: compatible:0: 'canaan,k210-uart' is not one of ['brcm,bcm11351-dw-apb-uart', 'brcm,bcm21664-dw-apb-uart']
>>
>> More device-specific strings.
>>
>>> spi@50240000: compatible: ['canaan,k210-spi', 'snps,dw-apb-ssi-4.01', 'snps,dw-apb-ssi'] is not valid under any of the given schemas (Possible causes of the failure):
>>> spi@50240000: compatible: ['canaan,k210-spi', 'snps,dw-apb-ssi-4.01', 'snps,dw-apb-ssi'] is too long
>>> spi@50240000: compatible:0: 'canaan,k210-spi' is not one of ['snps,dw-apb-ssi', 'snps,dwc-ssi-1.01a']
>>> spi@50240000: compatible:0: 'canaan,k210-spi' is not one of ['mscc,ocelot-spi', 'mscc,jaguar2-spi']
>>
>> ditto
> 
> Actually a false warning from checkpatch. canaan,k210-spi was already added to
> the DW spi yaml.
> 
>>
>>> i2c@50280000: compatible: ['canaan,k210-i2c', 'snps,designware-i2c'] is not valid under any of the given schemas (Possible causes of the failure):
>>> i2c@50280000: compatible: ['canaan,k210-i2c', 'snps,designware-i2c'] is too long
>>>
>>> i2c@50290000: compatible: ['canaan,k210-i2c', 'snps,designware-i2c'] is not valid under any of the given schemas (Possible causes of the failure):
>>> i2c@50290000: compatible: ['canaan,k210-i2c', 'snps,designware-i2c'] is too long
>>>
>>> i2c@502A0000: compatible: ['canaan,k210-i2c', 'snps,designware-i2c'] is not valid under any of the given schemas (Possible causes of the failure):
>>> i2c@502A0000: compatible: ['canaan,k210-i2c', 'snps,designware-i2c'] is too long
>>
>> ditto
>>
>>> timer@502D0000: compatible: ['canaan,k210-timer', 'snps,dw-apb-timer'] is not valid under any of the given schemas (Possible causes of the failure):
>>> timer@502D0000: compatible: ['canaan,k210-timer', 'snps,dw-apb-timer'] is too long
>>> timer@502D0000: compatible:0: 'canaan,k210-timer' is not one of ['snps,dw-apb-timer-sp', 'snps,dw-apb-timer-osc']
>>>
>>> timer@502D0000: 'resets' does not match any of the regexes: 'pinctrl-[0-9]+'
>>
>> `resets` is the correct name for this property.
>>
>>> timer@502E0000: compatible: ['canaan,k210-timer', 'snps,dw-apb-timer'] is not valid under any of the given schemas (Possible causes of the failure):
>>> timer@502E0000: compatible: ['canaan,k210-timer', 'snps,dw-apb-timer'] is too long
>>> timer@502E0000: compatible:0: 'canaan,k210-timer' is not one of ['snps,dw-apb-timer-sp', 'snps,dw-apb-timer-osc']
>>
>> More device-specific.
>>
>>> timer@502E0000: 'resets' does not match any of the regexes: 'pinctrl-[0-9]+'
>>> timer@502F0000: compatible: ['canaan,k210-timer', 'snps,dw-apb-timer'] is not valid under any of the given schemas (Possible causes of the failure):
>>> timer@502F0000: compatible: ['canaan,k210-timer', 'snps,dw-apb-timer'] is too long
>>> timer@502F0000: compatible:0: 'canaan,k210-timer' is not one of ['snps,dw-apb-timer-sp', 'snps,dw-apb-timer-osc']
>>>
>>> timer@502F0000: 'resets' does not match any of the regexes: 'pinctrl-[0-9]+'
>>> watchdog@50400000: compatible: ['canaan,k210-wdt', 'snps,dw-wdt'] is not valid under any of the given schemas (Possible causes of the failure):
>>> watchdog@50400000: compatible: ['canaan,k210-wdt', 'snps,dw-wdt'] is too long
>>> watchdog@50400000: compatible:0: 'canaan,k210-wdt' is not one of ['rockchip,rk3066-wdt', 'rockchip,rk3188-wdt', 'rockchip,rk3288-wdt', 'rockchip,rk3368-wdt']
>>>
>>> watchdog@50410000: compatible: ['canaan,k210-wdt', 'snps,dw-wdt'] is not valid under any of the given schemas (Possible causes of the failure):
>>> watchdog@50410000: compatible: ['canaan,k210-wdt', 'snps,dw-wdt'] is too long
>>> watchdog@50410000: compatible:0: 'canaan,k210-wdt' is not one of ['rockchip,rk3066-wdt', 'rockchip,rk3188-wdt', 'rockchip,rk3288-wdt', 'rockchip,rk3368-wdt']
>>>
>>> spi@52000000: compatible: ['canaan,k210-spi', 'snps,dw-apb-ssi-4.01', 'snps,dw-apb-ssi'] is not valid under any of the given schemas (Possible causes of the failure):
>>> spi@52000000: compatible: ['canaan,k210-spi', 'snps,dw-apb-ssi-4.01', 'snps,dw-apb-ssi'] is too long
>>> spi@52000000: compatible:0: 'canaan,k210-spi' is not one of ['snps,dw-apb-ssi', 'snps,dwc-ssi-1.01a']
>>> spi@52000000: compatible:0: 'canaan,k210-spi' is not one of ['mscc,ocelot-spi', 'mscc,jaguar2-spi']
>>>
>>> spi@53000000: compatible: ['canaan,k210-spi', 'snps,dw-apb-ssi-4.01', 'snps,dw-apb-ssi'] is not valid under any of the given schemas (Possible causes of the failure):
>>> spi@53000000: compatible: ['canaan,k210-spi', 'snps,dw-apb-ssi-4.01', 'snps,dw-apb-ssi'] is too long
>>> spi@53000000: compatible:0: 'canaan,k210-spi' is not one of ['snps,dw-apb-ssi', 'snps,dwc-ssi-1.01a']
>>> spi@53000000: compatible:0: 'canaan,k210-spi' is not one of ['mscc,ocelot-spi', 'mscc,jaguar2-spi']
> 
> canaan,k210-spi is already added to the dw spi doc. Will check that again.
> canaan,k210-ssi is missing though but since there is no quirk associated with
> it, we should not need to mention it in the yaml.
> 
>>>
>>> spi@54000000: compatible: ['canaan,k210-ssi', 'snps,dwc-ssi-1.01a'] is not valid under any of the given schemas (Possible causes of the failure):
>>> spi@54000000: compatible: ['canaan,k210-ssi', 'snps,dwc-ssi-1.01a'] is too long
>>> spi@54000000: compatible:0: 'canaan,k210-ssi' is not one of ['snps,dw-apb-ssi', 'snps,dwc-ssi-1.01a']
>>> spi@54000000: compatible:0: 'canaan,k210-ssi' is not one of ['mscc,ocelot-spi', 'mscc,jaguar2-spi']
>>
>> Ok, so the vast majority of these warnings are from unknown compatible
>> strings. IMO this is not an issue, but the simple fix is to just add
>> these strings to the yaml files.
> 
> We could, but given that checkpatch spits out warnings for the ones already
> added, we should probably start with fixing checkpatch :)

Well, it could also be that I was on the wrong branch, or had some old
build artifacts.

> 
> And there are some nodes that have no drive in Linux, so no yaml. So a warning
> will remain for these but I do not want to remove the nodes.
> 
> Thansk for checking all this !

Just install dt-schema and you can check it too ;)

--Sean

> 
> 
>>
>> --Sean
>>
> 
> 


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