Re: [PATCH 12/13] riscv: icicle-kit: update microchip icicle kit device tree
From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: 2021-11-10 14:58:49
Also in:
linux-crypto, linux-gpio, linux-i2c, linux-riscv, linux-rtc, linux-spi, linux-usb, lkml
Hi Conor, On Wed, Nov 10, 2021 at 3:20 PM [off-list ref] wrote:
On 09/11/2021 09:04, Geert Uytterhoeven wrote:quoted
On Mon, Nov 8, 2021 at 4:07 PM [off-list ref] wrote:quoted
From: Conor Dooley <conor.dooley@microchip.com> Update the device tree for the icicle kit by splitting it into a third part, which contains peripherals in the fpga fabric, add new peripherals (spi, qspi, gpio, rtc, pcie, system services, i2c), update parts of the memory map which have been changed. Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
As I said in the replies to another patch this is my first time doing any upstreaming of a device tree, i didnt realise that this would be a problem.
No problem, we're here to help you ;-)
quoted
quoted
--- a/arch/riscv/boot/dts/microchip/microchip-mpfs-icicle-kit.dts +++ b/arch/riscv/boot/dts/microchip/microchip-mpfs-icicle-kit.dts@@ -1,5 +1,5 @@ // SPDX-License-Identifier: (GPL-2.0 OR MIT) -/* Copyright (c) 2020 Microchip Technology Inc */ +/* Copyright (c) 2020-2021 Microchip Technology Inc */ /dts-v1/;@@ -13,72 +13,187 @@ / { compatible = "microchip,mpfs-icicle-kit", "microchip,mpfs"; aliases { - ethernet0 = &emac1; - serial0 = &serial0; - serial1 = &serial1; - serial2 = &serial2; - serial3 = &serial3; + mmuart0 = &mmuart0; + mmuart1 = &mmuart1; + mmuart2 = &mmuart2; + mmuart3 = &mmuart3; + mmuart4 = &mmuart4;Why? SerialN is the standard alias name.we changed the label to mmuart to match the microchip documentation.
The serialN aliases are standardized, so you cannot change them.
would it make more sense to call mmuart but alias it to serial? ie serial0 = &mmuart0;
You can change the labels, so that's OK.
quoted
quoted
+&spi1 { + status = "okay";No slave devices specified?no, but its exposed
But without specifying slave devices first you cannot use the controller anyway? While I2C supports instantiating slaves from userspace by writing to the new_device file in sysfs, SPI doesn't have that feature.
quoted
quoted
+&gpio2 { + interrupts = <PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT + PLIC_INT_GPIO2_NON_DIRECT>;Why override interrupts in the board .dts file? Doesn't this belong in the SoC .dtsi file?The interrupt setup for the gpio isnt fixed, there is an option to either connect the individual gpio interrupts to the plic *or* they can be connected to a per gpio controller common interrupt, and it is up to the driver to read a register to determine which interrupt triggered the common/NON_DIRECT interrupt. This decision is made by a write to a system register in application code, which to us didn't seem like it belonged in the soc .dtsi.
So it is software policy? Then it doesn't belong in the board DTS either.
Using the common interrupt for GPIO2 is the default on the polarfire-soc, there are only 38 per gpio line interrupts available of which 14 are connected to gpio0 and 24 to gpio1.
quoted
quoted
plic: interrupt-controller@c000000 { - #interrupt-cells = <1>; - compatible = "sifive,fu540-c000-plic", "sifive,plic-1.0.0"; + compatible = "sifive,plic-1.0.0";Why drop the first one again?we felt it didnt make sense to have something that specifically references the fu540 in the device tree for this board.
That would be a revert of commit 73d3c44115514616 ("riscv: dts:
microchip: add missing compatibles for clint and plic"), which you
supplied an R-b tag for?
Is this the same plic as in the FU540 SoC? Or do we need a new
microchip,mpfs-plic compatible value?
quoted
quoted
- emac1: ethernet@20112000 { + mac0: ethernet@20110000 { compatible = "cdns,macb"; - reg = <0x0 0x20112000 0x0 0x2000>; + #address-cells = <1>; + #size-cells = <0>; + reg = <0x0 0x20110000 0x0 0x2000>; + clocks = <&clkcfg CLK_MAC0>, <&clkcfg CLK_AHB>; + clock-names = "pclk", "hclk"; interrupt-parent = <&plic>; - interrupts = <70 71 72 73>; - local-mac-address = [00 00 00 00 00 00]; - clocks = <&clkcfg 5>, <&clkcfg 2>; + interrupts = <PLIC_INT_MAC0_INT + PLIC_INT_MAC0_QUEUE1 + PLIC_INT_MAC0_QUEUE2 + PLIC_INT_MAC0_QUEUE3 + PLIC_INT_MAC0_EMAC + PLIC_INT_MAC0_MMSL>;Please group using angular brackets.quoted
+ mac-address = [56 34 12 00 FC 01];Please drop this.Is the problem here having mac-address instead of local-, having either at all or that we have populated it rather than just filling with 0s?
MAC addresses are supposed to be unique.
We set it in u-boot anyway, so I think dropping entirely is okay.
Exactly.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds