From: Krzysztof Kozlowski <krzk@kernel.org>
To: wangjia@ultrarisc.com, Rob Herring <robh@kernel.org>,
Krzysztof Kozlowski <krzk+dt@kernel.org>,
Conor Dooley <conor+dt@kernel.org>,
Paul Walmsley <pjw@kernel.org>,
Palmer Dabbelt <palmer@dabbelt.com>,
Albert Ou <aou@eecs.berkeley.edu>,
Alexandre Ghiti <alex@ghiti.fr>,
Linus Walleij <linusw@kernel.org>,
Bartosz Golaszewski <brgl@kernel.org>,
Samuel Holland <samuel.holland@sifive.com>
Cc: Paul Walmsley <paul.walmsley@sifive.com>,
Palmer Dabbelt <palmer@sifive.com>,
Conor Dooley <conor@kernel.org>,
devicetree@vger.kernel.org, linux-riscv@lists.infradead.org,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org
Subject: Re: [PATCH 5/9] riscv: dts: ultrarisc: Add initial device tree for UltraRISC DP1000
Date: Thu, 21 May 2026 23:05:05 +0200 [thread overview]
Message-ID: <2d7b660c-97d4-4896-97df-7868b1d2fb50@kernel.org> (raw)
In-Reply-To: <20260515-ultrarisc-pinctrl-v1-5-bf559589ea8a@ultrarisc.com>
On 15/05/2026 03:18, Jia Wang via B4 Relay wrote:
> +
> + cluster0_l3: l3-cache0 {
> + /* L3 cache:
Use Linux coding style. In other places as well. Even netdev does not
use that style anymore!
> + * cache-unified, block-size 64B
> + * 16-way set associative, size 4MB
> + * per-cluster.
> + */
> + compatible = "cache";
> + cache-block-size = <64>;
> + cache-level = <3>;
> + cache-size = <0x400000>;
> + cache-sets = <0x1000>;
> + cache-unified;
> + next-level-cache = <&l4_cache>;
> + };
> +
> + cluster1_l3: l3-cache1 {
> + /* L3 cache:
> + * cache-unified, block-size 64B
> + * 16-way set associative, size 4MB
> + * per-cluster.
> + */
> + compatible = "cache";
> + cache-block-size = <64>;
> + cache-level = <3>;
> + cache-size = <0x400000>;
> + cache-sets = <0x1000>;
> + cache-unified;
> + next-level-cache = <&l4_cache>;
> + };
> + };
> +
> + clocks {
> + device_clk: device_clk {
You need to follow DTS coding style.
Anyway, something like "device clock" is completely uninformative or
even incorrect. I really doubt such thing as "device clock" exists...
Please use name for all fixed clocks which matches current format
recommendation: 'clock-<freq>' (see also the pattern in the binding for
any other options).
https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/clock/fixed-clock.yaml
> + compatible = "fixed-clock";
> + clock-frequency = <62500000>;
> + #clock-cells = <0>;
> + };
> +
> + timer_clk: timer_clk {
> + compatible = "fixed-clock";
> + clock-frequency = <50000000>;
> + #clock-cells = <0>;
> + };
> +
> + csr_clk: csr_clk {
> + compatible = "fixed-clock";
> + clock-frequency = <250000000>;
> + #clock-cells = <0>;
> + };
> + };
> +
> + l4_cache: l4-cache {
> + /* L4 cache:
> + * cache-unified, block-size 64B
> + * 16-way set associative, size 16MB
> + * shared by the SoC.
> + */
> + compatible = "cache";
> + cache-block-size = <64>;
> + cache-level = <4>;
> + cache-size = <0x1000000>;
> + cache-sets = <0x4000>;
> + cache-unified;
> + };
> +
> + memory@80000000 {
> + device_type = "memory";
> + reg = <0x00 0x80000000 0x4 0x00000000>;
> + };
> +
> + soc {
> + compatible = "simple-bus";
> + ranges;
> + #address-cells = <0x02>;
> + #size-cells = <0x02>;
<2> This is not hex and definitely does not need padding with 0.
> +
> + clint: clint@8000000 {
> + compatible = "sifive,clint0", "riscv,clint0";
> + reg = <0x00 0x8000000 0x00 0x100000>;
> + interrupts-extended = <&cpu0_intc 0x03>, <&cpu0_intc 0x07>,
> + <&cpu1_intc 0x03>, <&cpu1_intc 0x07>,
> + <&cpu2_intc 0x03>, <&cpu2_intc 0x07>,
> + <&cpu3_intc 0x03>, <&cpu3_intc 0x07>,
> + <&cpu4_intc 0x03>, <&cpu4_intc 0x07>,
> + <&cpu5_intc 0x03>, <&cpu5_intc 0x07>,
> + <&cpu6_intc 0x03>, <&cpu6_intc 0x07>,
> + <&cpu7_intc 0x03>, <&cpu7_intc 0x07>;
> + };
> +
> + plic: plic@9000000 {
> + compatible = "ultrarisc,dp1000-plic", "ultrarisc,cp100-plic";
> + reg = <0x00 0x9000000 0x00 0x4000000>;
> + #interrupt-cells = <1>;
> + #address-cells = <0>;
So hex or not hex? Please fix your DTS so it is consistent.
> + interrupt-controller;
> + interrupts-extended = <&cpu0_intc 0xb>, <&cpu0_intc 0x9>, <&cpu0_intc 0xa>,
> + <&cpu1_intc 0xb>, <&cpu1_intc 0x9>, <&cpu1_intc 0xa>,
> + <&cpu2_intc 0xb>, <&cpu2_intc 0x9>, <&cpu2_intc 0xa>,
> + <&cpu3_intc 0xb>, <&cpu3_intc 0x9>, <&cpu3_intc 0xa>,
> + <&cpu4_intc 0xb>, <&cpu4_intc 0x9>, <&cpu4_intc 0xa>,
> + <&cpu5_intc 0xb>, <&cpu5_intc 0x9>, <&cpu5_intc 0xa>,
> + <&cpu6_intc 0xb>, <&cpu6_intc 0x9>, <&cpu6_intc 0xa>,
> + <&cpu7_intc 0xb>, <&cpu7_intc 0x9>, <&cpu7_intc 0xa>;
> + riscv,ndev = <160>;
> + };
> +
> + pmx0: pinmux@11081000 {
> + compatible = "ultrarisc,dp1000-pinctrl";
> + reg = <0x0 0x11081000 0x0 0x1000>;
> + #pinctrl-cells = <2>;
> + };
> +
> + gpio: gpio@20200000 {
> + compatible = "snps,dw-apb-gpio";
> + reg = <0x0 0x20200000 0x0 0x1000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + clock-names = "bus", "db";
> + clocks = <&csr_clk>, <&device_clk>;
> +
> + gpio_a: gpio-port@0 {
> + compatible = "snps,dw-apb-gpio-port";
> + reg = <0>;
> + gpio-controller;
> + #gpio-cells = <2>;
> + snps,nr-gpios = <16>;
> + interrupt-controller;
> + #interrupt-cells = <2>;
> + interrupt-parent = <&plic>;
> + interrupts = <34>;
> + gpio-ranges = <&pmx0 0 0 16>;
> + };
> +
> + gpio_b: gpio-port@1 {
> + compatible = "snps,dw-apb-gpio-port";
> + reg = <1>;
> + gpio-controller;
> + #gpio-cells = <2>;
> + snps,nr-gpios = <8>;
> + gpio-ranges = <&pmx0 16 0 8>;
> + };
> +
> + gpio_c: gpio-port@2 {
> + compatible = "snps,dw-apb-gpio-port";
> + reg = <2>;
> + gpio-controller;
> + #gpio-cells = <2>;
> + snps,nr-gpios = <8>;
> + gpio-ranges = <&pmx0 24 0 8>;
> + };
> +
> + gpio_d: gpio-port@3 {
> + compatible = "snps,dw-apb-gpio-port";
> + reg = <3>;
> + gpio-controller;
> + #gpio-cells = <2>;
> + snps,nr-gpios = <8>;
> + gpio-ranges = <&pmx0 32 0 8>;
> + };
> + };
> +
> + uart0: serial@20300000 {
> + compatible = "ultrarisc,dp1000-uart", "snps,dw-apb-uart";
> + reg = <0x00 0x20300000 0x00 0x10000>;
> + interrupt-parent = <&plic>;
> + interrupts = <17>;
> + clock-frequency = <62500000>;
> + reg-io-width = <0x04>;
> + reg-shift = <0x02>;
> + };
> +
> + uart1: serial@20310000 {
> + compatible = "ultrarisc,dp1000-uart", "snps,dw-apb-uart";
> + reg = <0x00 0x20310000 0x00 0x10000>;
> + interrupt-parent = <&plic>;
> + interrupts = <18>;
> + clock-frequency = <62500000>;
> + reg-io-width = <0x04>;
> + reg-shift = <0x02>;
> + };
> +
> + uart2: serial@20400000 {
> + compatible = "ultrarisc,dp1000-uart", "snps,dw-apb-uart";
> + reg = <0x00 0x20400000 0x00 0x10000>;
> + interrupt-parent = <&plic>;
> + interrupts = <25>;
> + clock-frequency = <62500000>;
> + reg-io-width = <0x04>;
> + reg-shift = <0x02>;
> + };
> +
> + uart3: serial@20410000 {
> + compatible = "ultrarisc,dp1000-uart", "snps,dw-apb-uart";
> + reg = <0x00 0x20410000 0x00 0x10000>;
> + interrupt-parent = <&plic>;
> + interrupts = <26>;
> + clock-frequency = <62500000>;
> + reg-io-width = <0x04>;
> + reg-shift = <0x02>;
> + };
> +
> + spi0: spi@20320000 {
> + compatible = "snps,dw-apb-ssi";
> + reg = <0x0 0x20320000 0x0 0x1000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + clocks = <&device_clk>;
> + interrupt-parent = <&plic>;
> + interrupts = <19>;
> + num-cs = <3>;
> + };
> +
> + spi1: spi@20420000 {
> + compatible = "snps,dw-apb-ssi";
> + reg = <0x0 0x20420000 0x0 0x1000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + clocks = <&device_clk>;
> + interrupt-parent = <&plic>;
> + interrupts = <27>;
> + num-cs = <3>;
> + };
> +
> + i2c0: i2c@20330000 {
> + compatible = "snps,designware-i2c";
> + reg = <0x0 0x20330000 0x0 0x100>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + clock-frequency = <400000>;
> + clocks = <&device_clk>;
> + interrupt-parent = <&plic>;
> + interrupts = <20>;
> + };
> +
> + i2c1: i2c@20340000 {
> + compatible = "snps,designware-i2c";
> + reg = <0x0 0x20340000 0x0 0x100>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + clock-frequency = <400000>;
> + clocks = <&device_clk>;
> + interrupt-parent = <&plic>;
> + interrupts = <21>;
> + };
> +
> + i2c2: i2c@20430000 {
> + compatible = "snps,designware-i2c";
> + reg = <0x0 0x20430000 0x0 0x100>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + clock-frequency = <400000>;
> + clocks = <&device_clk>;
> + interrupt-parent = <&plic>;
> + interrupts = <28>;
> + };
> +
> + i2c3: i2c@20440000 {
> + compatible = "snps,designware-i2c";
> + reg = <0x0 0x20440000 0x0 0x100>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + clock-frequency = <400000>;
> + clocks = <&device_clk>;
> + interrupt-parent = <&plic>;
> + interrupts = <29>;
> + };
> +
> + pcie_x16: pcie@21000000 {
> + compatible = "ultrarisc,dp1000-pcie";
> + reg = <0x0 0x21000000 0x0 0x01000000>,
> + <0x0 0x4fff0000 0x0 0x00010000>;
> + reg-names = "dbi", "config";
> + ranges = <0x81000000 0x0 0x4fbf0000 0x0 0x4fbf0000 0x0 0x00400000>,
> + <0x82000000 0x0 0x40000000 0x0 0x40000000 0x0 0x0fbf0000>,
> + <0xc3000000 0x40 0x00000000 0x40 0x00000000 0xd 0x00000000>;
> + #address-cells = <3>;
> + #size-cells = <2>;
> + #interrupt-cells = <1>;
> + device_type = "pci";
> + dma-coherent;
> + bus-range = <0x0 0xff>;
> + num-lanes = <16>;
> + interrupt-parent = <&plic>;
> + interrupts = <43>, <44>, <45>, <46>, <47>;
> + interrupt-names = "msi", "inta", "intb", "intc", "intd";
> + interrupt-map-mask = <0x0 0x0 0x0 0x7>;
> + interrupt-map = <0x0 0x0 0x0 0x1 &plic 44>,
> + <0x0 0x0 0x0 0x2 &plic 45>,
> + <0x0 0x0 0x0 0x3 &plic 46>,
> + <0x0 0x0 0x0 0x4 &plic 47>;
Why PCIe without any devices is enabled? That's a bus.
Please look how other DTS are written because you created something
pretty different than entire kernel style.
> + };
> +
> + pcie_x4a: pcie@23000000 {
> + compatible = "ultrarisc,dp1000-pcie";
> + reg = <0x0 0x23000000 0x0 0x01000000>,
> + <0x0 0x6fff0000 0x0 0x00010000>;
> + reg-names = "dbi", "config";
> + ranges = <0x81000000 0x0 0x6fbf0000 0x0 0x6fbf0000 0x0 0x00400000>,
> + <0x82000000 0x0 0x60000000 0x0 0x60000000 0x0 0x0fbf0000>,
> + <0xc3000000 0x80 0x00000000 0x80 0x00000000 0xd 0x00000000>;
> + #address-cells = <3>;
> + #size-cells = <2>;
> + #interrupt-cells = <1>;
> + device_type = "pci";
> + dma-coherent;
> + bus-range = <0x0 0xff>;
> + num-lanes = <4>;
> + interrupt-parent = <&plic>;
> + interrupts = <63>, <64>, <65>, <66>, <67>;
> + interrupt-names = "msi", "inta", "intb", "intc", "intd";
> + interrupt-map-mask = <0x0 0x0 0x0 0x7>;
> + interrupt-map = <0x0 0x0 0x0 0x1 &plic 64>,
> + <0x0 0x0 0x0 0x2 &plic 65>,
> + <0x0 0x0 0x0 0x3 &plic 66>,
> + <0x0 0x0 0x0 0x4 &plic 67>;
> + };
> +
> + pcie_x4b: pcie@24000000 {
> + compatible = "ultrarisc,dp1000-pcie";
> + reg = <0x0 0x24000000 0x0 0x01000000>,
> + <0x0 0x7fff0000 0x0 0x00010000>;
> + reg-names = "dbi", "config";
> + ranges = <0x81000000 0x0 0x7fbf0000 0x0 0x7fbf0000 0x0 0x00400000>,
> + <0x82000000 0x0 0x70000000 0x0 0x70000000 0x0 0x0fbf0000>,
> + <0xc3000000 0xc0 0x00000000 0xc0 0x00000000 0xd 0x00000000>;
> + #address-cells = <3>;
> + #size-cells = <2>;
> + #interrupt-cells = <1>;
> + device_type = "pci";
> + dma-coherent;
> + bus-range = <0x0 0xff>;
> + num-lanes = <4>;
> + interrupt-parent = <&plic>;
> + interrupts = <73>, <74>, <75>, <76>, <77>;
> + interrupt-names = "msi", "inta", "intb", "intc", "intd";
> + interrupt-map-mask = <0x0 0x0 0x0 0x7>;
> + interrupt-map = <0x0 0x0 0x0 0x1 &plic 74>,
> + <0x0 0x0 0x0 0x2 &plic 75>,
> + <0x0 0x0 0x0 0x3 &plic 76>,
> + <0x0 0x0 0x0 0x4 &plic 77>;
> + };
> +
> + ethernet: ethernet@38000000 {
> + compatible = "snps,dwmac", "snps,dwmac-5.10a";
> + reg = <0x00 0x38000000 0x00 0x1000000>;
> + clocks = <&csr_clk>;
> + clock-names = "stmmaceth";
> + interrupt-parent = <&plic>;
> + interrupts = <84>;
> + interrupt-names = "macirq";
> + local-mac-address = [ff ff ff ff ff ff];
Drop. Not a property of the SoC.
> + max-speed = <1000>;
> + phy-mode = "rgmii-id";
> + snps,txpbl = <8>;
> + snps,rxpbl = <8>;
I doubt that Ethernet is complete on the SoC - without MAC and all other
resources. IOW, it is very weird that this is enabled here. Please explain.
> + };
> +
> + dmac: dma-controller@39000000 {
> + compatible = "snps,axi-dma-1.01a";
> + reg = <0x0 0x39000000 0x0 0x400>;
<0x0 here but why in other places is <0x00?
Write consistent code.
Best regards,
Krzysztof
next prev parent reply other threads:[~2026-05-21 21:05 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-05-15 1:17 [PATCH 0/9] riscv: ultrarisc: add DP1000 SoC DT and pinctrl support Jia Wang via B4 Relay
2026-05-15 1:17 ` [PATCH 1/9] dt-bindings: vendor-prefixes: add Rongda Jia Wang via B4 Relay
2026-05-15 1:20 ` sashiko-bot
2026-05-15 1:25 ` Jia Wang
2026-05-21 20:51 ` Krzysztof Kozlowski
2026-05-15 1:17 ` [PATCH 2/9] dt-bindings: riscv: cpus: Add UltraRISC CP100 compatible Jia Wang via B4 Relay
2026-05-15 10:06 ` Conor Dooley
2026-05-15 1:17 ` [PATCH 3/9] dt-bindings: riscv: Add UltraRISC DP1000 bindings Jia Wang via B4 Relay
2026-05-15 10:08 ` Conor Dooley
2026-05-18 3:06 ` Jia Wang
2026-05-15 1:18 ` [PATCH 4/9] dt-bindings: pinctrl: Add UltraRISC DP1000 pinctrl bindings Jia Wang via B4 Relay
2026-05-15 1:49 ` sashiko-bot
2026-05-15 8:43 ` Jia Wang
2026-05-15 10:12 ` Conor Dooley
2026-05-18 6:03 ` Jia Wang
2026-05-21 20:56 ` Krzysztof Kozlowski
2026-05-15 1:18 ` [PATCH 5/9] riscv: dts: ultrarisc: Add initial device tree for UltraRISC DP1000 Jia Wang via B4 Relay
2026-05-15 2:02 ` sashiko-bot
2026-05-19 6:55 ` Jia Wang
2026-05-15 10:26 ` Conor Dooley
2026-05-20 2:51 ` Jia Wang
2026-05-21 21:05 ` Krzysztof Kozlowski [this message]
2026-05-15 1:18 ` [PATCH 6/9] pinctrl: ultrarisc: Add UltraRISC DP1000 pinctrl driver Jia Wang via B4 Relay
2026-05-15 2:28 ` sashiko-bot
2026-05-20 7:15 ` Jia Wang
2026-05-21 21:09 ` Krzysztof Kozlowski
2026-05-15 1:18 ` [PATCH 7/9] riscv: dts: ultrarisc: add Rongda M0 board device tree Jia Wang via B4 Relay
2026-05-15 2:37 ` sashiko-bot
2026-05-20 8:26 ` Jia Wang
2026-05-15 10:28 ` Conor Dooley
2026-05-20 8:40 ` Jia Wang
2026-05-21 20:59 ` Krzysztof Kozlowski
2026-05-15 1:18 ` [PATCH 8/9] riscv: dts: ultrarisc: add Milk-V Titan " Jia Wang via B4 Relay
2026-05-15 2:50 ` sashiko-bot
2026-05-20 9:39 ` Jia Wang
2026-05-15 1:18 ` [PATCH 9/9] riscv: defconfig: enable ARCH_ULTRARISC Jia Wang via B4 Relay
2026-05-15 2:59 ` sashiko-bot
2026-05-20 9:49 ` Jia Wang
2026-05-21 20:57 ` Krzysztof Kozlowski
2026-05-15 10:05 ` [PATCH 0/9] riscv: ultrarisc: add DP1000 SoC DT and pinctrl support Conor Dooley
2026-05-21 9:52 ` Jia Wang
2026-05-21 10:23 ` Conor Dooley
2026-05-22 1:41 ` Jia Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2d7b660c-97d4-4896-97df-7868b1d2fb50@kernel.org \
--to=krzk@kernel.org \
--cc=alex@ghiti.fr \
--cc=aou@eecs.berkeley.edu \
--cc=brgl@kernel.org \
--cc=conor+dt@kernel.org \
--cc=conor@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=krzk+dt@kernel.org \
--cc=linusw@kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=palmer@dabbelt.com \
--cc=palmer@sifive.com \
--cc=paul.walmsley@sifive.com \
--cc=pjw@kernel.org \
--cc=robh@kernel.org \
--cc=samuel.holland@sifive.com \
--cc=wangjia@ultrarisc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox