diff mbox series

[12/13] riscv: icicle-kit: update microchip icicle kit device tree

Message ID 20211108150554.4457-13-conor.dooley@microchip.com
State New
Headers show
Series Update the icicle kit device tree | expand

Commit Message

Conor Dooley Nov. 8, 2021, 3:05 p.m. UTC 
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>
---
 .../dts/microchip/microchip-mpfs-fabric.dtsi  |  21 ++
 .../microchip/microchip-mpfs-icicle-kit.dts   | 159 +++++++--
 .../boot/dts/microchip/microchip-mpfs.dtsi    | 333 ++++++++++++++----
 3 files changed, 428 insertions(+), 85 deletions(-)
 create mode 100644 arch/riscv/boot/dts/microchip/microchip-mpfs-fabric.dtsi

Comments

Krzysztof Kozlowski Nov. 8, 2021, 9:40 p.m. UTC | #1 
On 08/11/2021 16:05, conor.dooley@microchip.com wrote:
> 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.

This should be multiple commits because you mix up refactoring (split)
and adding new features. The patch is really, really difficult to
review. I gave up in the middle.

> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  .../dts/microchip/microchip-mpfs-fabric.dtsi  |  21 ++
>  .../microchip/microchip-mpfs-icicle-kit.dts   | 159 +++++++--
>  .../boot/dts/microchip/microchip-mpfs.dtsi    | 333 ++++++++++++++----
>  3 files changed, 428 insertions(+), 85 deletions(-)
>  create mode 100644 arch/riscv/boot/dts/microchip/microchip-mpfs-fabric.dtsi
> 
> diff --git a/arch/riscv/boot/dts/microchip/microchip-mpfs-fabric.dtsi b/arch/riscv/boot/dts/microchip/microchip-mpfs-fabric.dtsi
> new file mode 100644
> index 000000000000..8fa3356494f1
> --- /dev/null
> +++ b/arch/riscv/boot/dts/microchip/microchip-mpfs-fabric.dtsi
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/* Copyright (c) 2020-2021 Microchip Technology Inc */
> +
> +/ {
> +	fpgadma: fpgadma@60020000 {
> +		compatible = "microchip,mpfs-fpga-dma-uio";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		reg = <0x0 0x60020000 0x0 0x1000>;
> +		interrupt-parent = <&plic>;
> +		interrupts = <PLIC_INT_FABRIC_F2H_2>;
> +		status = "okay";
> +	};
> +
> +	fpgalsram: fpga_lsram@61000000 {

Node names go with hyphen, but actually you should not need it, because
the name should be generic, e.g. "uio".

However there is no such compatible and checkpatch should complain about it.

> +		compatible = "generic-uio";
> +		reg = <0x0 0x61000000 0x0 0x0001000
> +			0x14 0x00000000 0x0 0x00010000>;
> +		status = "okay";
> +	};
> +};
> diff --git a/arch/riscv/boot/dts/microchip/microchip-mpfs-icicle-kit.dts b/arch/riscv/boot/dts/microchip/microchip-mpfs-icicle-kit.dts
> index fc1e5869df1b..4212129fcdf1 100644
> --- 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? Commit msg does not explain it.

>  	};
>  
>  	chosen {
> -		stdout-path = "serial0:115200n8";
> +		stdout-path = "mmuart1:115200n8";
>  	};
>  
>  	cpus {
>  		timebase-frequency = <RTCCLK_FREQ>;
>  	};
>  
> -	memory@80000000 {
> +	ddrc_cache_lo: memory@80000000 {
>  		device_type = "memory";
> -		reg = <0x0 0x80000000 0x0 0x40000000>;
> -		clocks = <&clkcfg 26>;
> +		reg = <0x0 0x80000000 0x0 0x2e000000>;
> +		clocks = <&clkcfg CLK_DDRC>;
> +		status = "okay";
> +	};
> +
> +	ddrc_cache_hi: memory@1000000000 {
> +		device_type = "memory";
> +		reg = <0x10 0x0 0x0 0x40000000>;
> +		clocks = <&clkcfg CLK_DDRC>;
> +		status = "okay";
>  	};
>  };
>  
> -&serial0 {
> +&mmuart1 {
>  	status = "okay";
>  };
>  
> -&serial1 {
> +&mmuart2 {
>  	status = "okay";
>  };
>  
> -&serial2 {
> +&mmuart3 {
>  	status = "okay";
>  };
>  
> -&serial3 {
> +&mmuart4 {
>  	status = "okay";
>  };
>  
>  &mmc {
>  	status = "okay";
> -
>  	bus-width = <4>;
>  	disable-wp;
>  	cap-sd-highspeed;
> +	cap-mmc-highspeed;
>  	card-detect-delay = <200>;
> +	mmc-ddr-1_8v;
> +	mmc-hs200-1_8v;
>  	sd-uhs-sdr12;
>  	sd-uhs-sdr25;
>  	sd-uhs-sdr50;
>  	sd-uhs-sdr104;
>  };
>  
> -&emac0 {
> +&spi0 {
> +	status = "okay";
> +	spidev@0 {
> +		compatible = "spidev";

1. There is no such compatible,
2. You should have big fat warning when booting, so such DT cannot be
accepted.

> +		reg = <0>; /* CS 0 */
> +		spi-max-frequency = <10000000>;
> +		status = "okay";
> +	};
> +};
> +
> +&spi1 {
> +	status = "okay";
> +};
> +
> +&qspi {
> +	status = "okay";
> +};
> +
> +&i2c0 {
> +	status = "okay";
> +};
> +
> +&i2c1 {
> +	status = "okay";
> +	pac193x: pac193x@10 {

Generic node name. Looks like compatible is not documented, so first
bindings.


> +		compatible = "microchip,pac1934";
> +		reg = <0x10>;
> +		samp-rate = <64>;
> +		status = "okay";
> +		ch0: channel0 {
> +			uohms-shunt-res = <10000>;
> +			rail-name = "VDDREG";
> +			channel_enabled;
> +		};
> +		ch1: channel1 {
> +			uohms-shunt-res = <10000>;
> +			rail-name = "VDDA25";
> +			channel_enabled;
> +		};
> +		ch2: channel2 {
> +			uohms-shunt-res = <10000>;
> +			rail-name = "VDD25";
> +			channel_enabled;
> +		};
> +		ch3: channel3 {
> +			uohms-shunt-res = <10000>;
> +			rail-name = "VDDA_REG";
> +			channel_enabled;
> +		};
> +	};
> +};
> +
> +&mac0 {
> +	status = "okay";
>  	phy-mode = "sgmii";
>  	phy-handle = <&phy0>;
> -	phy0: ethernet-phy@8 {
> -		reg = <8>;
> -		ti,fifo-depth = <0x01>;
> -	};
>  };
>  
> -&emac1 {
> +&mac1 {

I gave up here, it's not easy to find what is effect of refactoring,
what is a new node.

Best regards,
Krzysztof
Geert Uytterhoeven Nov. 9, 2021, 9:04 a.m. UTC | #2 
Hi Conor,

On Mon, Nov 8, 2021 at 4:07 PM <conor.dooley@microchip.com> wrote:
> 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>

Thanks for your patch!

Please split it into logical separated parts.

> --- /dev/null
> +++ b/arch/riscv/boot/dts/microchip/microchip-mpfs-fabric.dtsi
> @@ -0,0 +1,21 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/* Copyright (c) 2020-2021 Microchip Technology Inc */
> +
> +/ {
> +       fpgadma: fpgadma@60020000 {
> +               compatible = "microchip,mpfs-fpga-dma-uio";
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +               reg = <0x0 0x60020000 0x0 0x1000>;
> +               interrupt-parent = <&plic>;
> +               interrupts = <PLIC_INT_FABRIC_F2H_2>;
> +               status = "okay";
> +       };
> +
> +       fpgalsram: fpga_lsram@61000000 {
> +               compatible = "generic-uio";
> +               reg = <0x0 0x61000000 0x0 0x0001000
> +                       0x14 0x00000000 0x0 0x00010000>;

Please group by angular brackets, to increase readability, and support
automatic validation:

<0x0 0x61000000 0x0 0x0001000>, <0x14 0x00000000 0x0 0x00010000>

> +               status = "okay";
> +       };

Do we really want UIO nodes in upstream DT?

> +};
> diff --git a/arch/riscv/boot/dts/microchip/microchip-mpfs-icicle-kit.dts b/arch/riscv/boot/dts/microchip/microchip-mpfs-icicle-kit.dts
> index fc1e5869df1b..4212129fcdf1 100644
> --- 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.

>         };

>
> -&emac0 {
> +&spi0 {
> +       status = "okay";
> +       spidev@0 {
> +               compatible = "spidev";

Please don't use "spidev", but a proper compatible value describing
what is really connected.  If you want to use it with spidev (which
is software policy, not hardware description), add that compatible
value to drivers/spi/spidev.c:spidev_dt_ids[], or use driver_override
in sysfs at runtime.

> +               reg = <0>; /* CS 0 */
> +               spi-max-frequency = <10000000>;
> +               status = "okay";
> +       };
> +};
> +
> +&spi1 {
> +       status = "okay";

No slave devices specified?

> +};
> +
> +&qspi {
> +       status = "okay";
> +};
> +
> +&i2c0 {
> +       status = "okay";
> +};
> +
> +&i2c1 {
> +       status = "okay";
> +       pac193x: pac193x@10 {

adc@, I guess?

> +               compatible = "microchip,pac1934";
> +               reg = <0x10>;
> +               samp-rate = <64>;
> +               status = "okay";
> +               ch0: channel0 {
> +                       uohms-shunt-res = <10000>;
> +                       rail-name = "VDDREG";
> +                       channel_enabled;
> +               };
> +               ch1: channel1 {
> +                       uohms-shunt-res = <10000>;
> +                       rail-name = "VDDA25";
> +                       channel_enabled;
> +               };
> +               ch2: channel2 {
> +                       uohms-shunt-res = <10000>;
> +                       rail-name = "VDD25";
> +                       channel_enabled;
> +               };
> +               ch3: channel3 {
> +                       uohms-shunt-res = <10000>;
> +                       rail-name = "VDDA_REG";
> +                       channel_enabled;
> +               };
> +       };
> +};

> +&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?
Please group using angular brackets.

> --- a/arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi
> +++ b/arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi

> @@ -145,49 +149,55 @@ cpu4_intc: interrupt-controller {
>         soc {
>                 #address-cells = <2>;
>                 #size-cells = <2>;
> -               compatible = "simple-bus";
> +               compatible = "microchip,mpfs-soc", "simple-bus";
>                 ranges;
>
> -               cache-controller@2010000 {
> +               cctrllr: cache-controller@2010000 {
>                         compatible = "sifive,fu540-c000-ccache", "cache";
> +                       reg = <0x0 0x2010000 0x0 0x1000>;
> +                       interrupt-parent = <&plic>;
> +                       interrupts = <PLIC_INT_L2_METADATA_CORR
> +                               PLIC_INT_L2_METADAT_UNCORR
> +                               PLIC_INT_L2_DATA_CORR>;

Please group using angular brackets.

>                         cache-block-size = <64>;
>                         cache-level = <2>;
>                         cache-sets = <1024>;
>                         cache-size = <2097152>;
>                         cache-unified;
> -                       interrupt-parent = <&plic>;
> -                       interrupts = <1 2 3>;
> -                       reg = <0x0 0x2010000 0x0 0x1000>;
>                 };
>
> -               clint@2000000 {
> +               clint: clint@2000000 {
>                         compatible = "sifive,fu540-c000-clint", "sifive,clint0";
>                         reg = <0x0 0x2000000 0x0 0xC000>;
> -                       interrupts-extended = <&cpu0_intc 3 &cpu0_intc 7
> -                                               &cpu1_intc 3 &cpu1_intc 7
> -                                               &cpu2_intc 3 &cpu2_intc 7
> -                                               &cpu3_intc 3 &cpu3_intc 7
> -                                               &cpu4_intc 3 &cpu4_intc 7>;
> +                       interrupts-extended =
> +                                       <&cpu0_intc HART_INT_M_SOFT &cpu0_intc HART_INT_M_TIMER
> +                                        &cpu1_intc HART_INT_M_SOFT &cpu1_intc HART_INT_M_TIMER
> +                                        &cpu2_intc HART_INT_M_SOFT &cpu2_intc HART_INT_M_TIMER
> +                                        &cpu3_intc HART_INT_M_SOFT &cpu3_intc HART_INT_M_TIMER
> +                                        &cpu4_intc HART_INT_M_SOFT &cpu4_intc HART_INT_M_TIMER>;

Please group using angular brackets.

>                 };
>
>                 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?

>                         reg = <0x0 0xc000000 0x0 0x4000000>;
> +                       #interrupt-cells = <1>;
>                         riscv,ndev = <186>;
>                         interrupt-controller;
> -                       interrupts-extended = <&cpu0_intc 11
> -                                       &cpu1_intc 11 &cpu1_intc 9
> -                                       &cpu2_intc 11 &cpu2_intc 9
> -                                       &cpu3_intc 11 &cpu3_intc 9
> -                                       &cpu4_intc 11 &cpu4_intc 9>;
> +                       interrupts-extended = <&cpu0_intc HART_INT_M_EXT
> +                                       &cpu1_intc HART_INT_M_EXT &cpu1_intc HART_INT_S_EXT
> +                                       &cpu2_intc HART_INT_M_EXT &cpu2_intc HART_INT_S_EXT
> +                                       &cpu3_intc HART_INT_M_EXT &cpu3_intc HART_INT_S_EXT
> +                                       &cpu4_intc HART_INT_M_EXT &cpu4_intc HART_INT_S_EXT>;
>                 };
>
> -               dma@3000000 {
> -                       compatible = "sifive,fu540-c000-pdma";
> +               pdma: pdma@3000000 {
> +                       compatible = "microchip,mpfs-pdma-uio";
>                         reg = <0x0 0x3000000 0x0 0x8000>;
>                         interrupt-parent = <&plic>;
> -                       interrupts = <23 24 25 26 27 28 29 30>;
> +                       interrupts = <PLIC_INT_DMA_CH0_DONE PLIC_INT_DMA_CH0_ERR
> +                               PLIC_INT_DMA_CH1_DONE PLIC_INT_DMA_CH1_ERR
> +                               PLIC_INT_DMA_CH2_DONE PLIC_INT_DMA_CH2_ERR
> +                               PLIC_INT_DMA_CH3_DONE PLIC_INT_DMA_CH3_ERR>;

Please group using angular brackets.

>                         #dma-cells = <1>;
>                 };
>
> @@ -205,7 +215,7 @@ clkcfg: clkcfg@20002000 {
>                         clocks = <&refclk>;
>                         #clock-cells = <1>;
>                         clock-output-names = "cpu", "axi", "ahb", "envm",       /* 0-3   */
> -                                "mac0", "mac1", "mmc", "timer",                /* 4-7   */
> +                               "mac0", "mac1", "mmc", "timer",                 /* 4-7   */
>                                 "mmuart0", "mmuart1", "mmuart2", "mmuart3",     /* 8-11  */
>                                 "mmuart4", "spi0", "spi1", "i2c0",              /* 12-15 */
>                                 "i2c1", "can0", "can1", "usb",                  /* 16-19 */

No need for clock-output-names at all.

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

> +                       mac-address = [56 34 12 00 FC 01];

Please drop this.

>                         status = "disabled";
> +               };
>
> +               rtc: rtc@20124000 {
> +                       compatible = "microchip,mpfs-rtc";
>                         #address-cells = <1>;
>                         #size-cells = <0>;
> +                       reg = <0x0 0x20124000 0x0 0x1000>;
> +                       clocks = <&clkcfg CLK_RTC>;
> +                       clock-names = "rtc";
> +                       interrupt-parent = <&plic>;
> +                       interrupts = <PLIC_INT_RTC_WAKEUP PLIC_INT_RTC_MATCH>;

Please group using angular brackets.

> +                       status = "disabled";
>                 };
>
> +               usb: usb@20201000 {
> +                       compatible = "microchip,mpfs-usb-host";
> +                       reg = <0x0 0x20201000 0x0 0x1000>;
> +                       reg-names = "mc","control";
> +                       clocks = <&clkcfg CLK_USB>;
> +                       interrupt-parent = <&plic>;
> +                       interrupts = <PLIC_INT_USB_DMA PLIC_INT_USB_MC>;

Please group using angular brackets.

> +                       interrupt-names = "dma","mc";
> +                       dr_mode = "host";
> +                       status = "disabled";
> +               };
> +

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
Conor Dooley Nov. 10, 2021, 12:07 p.m. UTC | #3 
On 08/11/2021 21:40, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 08/11/2021 16:05, conor.dooley@microchip.com wrote:
>> 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.
> 
> This should be multiple commits because you mix up refactoring (split)
> and adding new features. The patch is really, really difficult to
> review. I gave up in the middle.
> 
>>
>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
>> ---
>>   .../dts/microchip/microchip-mpfs-fabric.dtsi  |  21 ++
>>   .../microchip/microchip-mpfs-icicle-kit.dts   | 159 +++++++--
>>   .../boot/dts/microchip/microchip-mpfs.dtsi    | 333 ++++++++++++++----
>>   3 files changed, 428 insertions(+), 85 deletions(-)
>>   create mode 100644 arch/riscv/boot/dts/microchip/microchip-mpfs-fabric.dtsi
>>
>> diff --git a/arch/riscv/boot/dts/microchip/microchip-mpfs-fabric.dtsi b/arch/riscv/boot/dts/microchip/microchip-mpfs-fabric.dtsi
>> new file mode 100644
>> index 000000000000..8fa3356494f1
>> --- /dev/null
>> +++ b/arch/riscv/boot/dts/microchip/microchip-mpfs-fabric.dtsi
>> @@ -0,0 +1,21 @@
>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
>> +/* Copyright (c) 2020-2021 Microchip Technology Inc */
>> +
>> +/ {
>> +     fpgadma: fpgadma@60020000 {
>> +             compatible = "microchip,mpfs-fpga-dma-uio";
>> +             #address-cells = <1>;
>> +             #size-cells = <0>;
>> +             reg = <0x0 0x60020000 0x0 0x1000>;
>> +             interrupt-parent = <&plic>;
>> +             interrupts = <PLIC_INT_FABRIC_F2H_2>;
>> +             status = "okay";
>> +     };
>> +
>> +     fpgalsram: fpga_lsram@61000000 {
> 
> Node names go with hyphen, but actually you should not need it, because
> the name should be generic, e.g. "uio".
sure, will change it to uio.
> 
> However there is no such compatible and checkpatch should complain about it.
yeah, this and the pac1934 i didnt send bindings for - erroneously 
thought i might not have to do so. ill get a binding sorted out for both 
of these.
> 
>> +             compatible = "generic-uio";
>> +             reg = <0x0 0x61000000 0x0 0x0001000
>> +                     0x14 0x00000000 0x0 0x00010000>;
>> +             status = "okay";
>> +     };
>> +};
>> diff --git a/arch/riscv/boot/dts/microchip/microchip-mpfs-icicle-kit.dts b/arch/riscv/boot/dts/microchip/microchip-mpfs-icicle-kit.dts
>> index fc1e5869df1b..4212129fcdf1 100644
>> --- 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? Commit msg does not explain it.
changed to match all of our documentation
> 
>>        };
>>
>>        chosen {
>> -             stdout-path = "serial0:115200n8";
>> +             stdout-path = "mmuart1:115200n8";
>>        };
>>
>>        cpus {
>>                timebase-frequency = <RTCCLK_FREQ>;
>>        };
>>
>> -     memory@80000000 {
>> +     ddrc_cache_lo: memory@80000000 {
>>                device_type = "memory";
>> -             reg = <0x0 0x80000000 0x0 0x40000000>;
>> -             clocks = <&clkcfg 26>;
>> +             reg = <0x0 0x80000000 0x0 0x2e000000>;
>> +             clocks = <&clkcfg CLK_DDRC>;
>> +             status = "okay";
>> +     };
>> +
>> +     ddrc_cache_hi: memory@1000000000 {
>> +             device_type = "memory";
>> +             reg = <0x10 0x0 0x0 0x40000000>;
>> +             clocks = <&clkcfg CLK_DDRC>;
>> +             status = "okay";
>>        };
>>   };
>>
>> -&serial0 {
>> +&mmuart1 {
>>        status = "okay";
>>   };
>>
>> -&serial1 {
>> +&mmuart2 {
>>        status = "okay";
>>   };
>>
>> -&serial2 {
>> +&mmuart3 {
>>        status = "okay";
>>   };
>>
>> -&serial3 {
>> +&mmuart4 {
>>        status = "okay";
>>   };
>>
>>   &mmc {
>>        status = "okay";
>> -
>>        bus-width = <4>;
>>        disable-wp;
>>        cap-sd-highspeed;
>> +     cap-mmc-highspeed;
>>        card-detect-delay = <200>;
>> +     mmc-ddr-1_8v;
>> +     mmc-hs200-1_8v;
>>        sd-uhs-sdr12;
>>        sd-uhs-sdr25;
>>        sd-uhs-sdr50;
>>        sd-uhs-sdr104;
>>   };
>>
>> -&emac0 {
>> +&spi0 {
>> +     status = "okay";
>> +     spidev@0 {
>> +             compatible = "spidev";
> 
> 1. There is no such compatible,
> 2. You should have big fat warning when booting, so such DT cannot be
> accepted.
this one was an oversight from me, that compatible has never been 
"spidev" on its own in our internal stuff and i mustve accidentally 
dropped a vendor string while making these patches.
> 
>> +             reg = <0>; /* CS 0 */
>> +             spi-max-frequency = <10000000>;
>> +             status = "okay";
>> +     };
>> +};
>> +
>> +&spi1 {
>> +     status = "okay";
>> +};
>> +
>> +&qspi {
>> +     status = "okay";
>> +};
>> +
>> +&i2c0 {
>> +     status = "okay";
>> +};
>> +
>> +&i2c1 {
>> +     status = "okay";
>> +     pac193x: pac193x@10 {
> 
> Generic node name. Looks like compatible is not documented, so first
> bindings.
> 
as above
> 
>> +             compatible = "microchip,pac1934";
>> +             reg = <0x10>;
>> +             samp-rate = <64>;
>> +             status = "okay";
>> +             ch0: channel0 {
>> +                     uohms-shunt-res = <10000>;
>> +                     rail-name = "VDDREG";
>> +                     channel_enabled;
>> +             };
>> +             ch1: channel1 {
>> +                     uohms-shunt-res = <10000>;
>> +                     rail-name = "VDDA25";
>> +                     channel_enabled;
>> +             };
>> +             ch2: channel2 {
>> +                     uohms-shunt-res = <10000>;
>> +                     rail-name = "VDD25";
>> +                     channel_enabled;
>> +             };
>> +             ch3: channel3 {
>> +                     uohms-shunt-res = <10000>;
>> +                     rail-name = "VDDA_REG";
>> +                     channel_enabled;
>> +             };
>> +     };
>> +};
>> +
>> +&mac0 {
>> +     status = "okay";
>>        phy-mode = "sgmii";
>>        phy-handle = <&phy0>;
>> -     phy0: ethernet-phy@8 {
>> -             reg = <8>;
>> -             ti,fifo-depth = <0x01>;
>> -     };
>>   };
>>
>> -&emac1 {
>> +&mac1 {
> 
> I gave up here, it's not easy to find what is effect of refactoring,
> what is a new node.
yeah, ill split it into several patches - probably one for the 
splitting, one for the new defines, one for the changes to existing 
nodes and one for node additions.
> 
> Best regards,
> Krzysztof
>
Conor Dooley Nov. 10, 2021, 2:19 p.m. UTC | #4 
On 09/11/2021 09:04, Geert Uytterhoeven wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Conor,
> 
> On Mon, Nov 8, 2021 at 4:07 PM <conor.dooley@microchip.com> wrote:
>> 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>
> 
> Thanks for your patch!
> 
> Please split it into logical separated parts.
yeah, ive split it into several patches - one for the splitting into 3, 
one for the new defines, one for the changes to existing nodes and one 
for node additions.

> 
>> --- /dev/null
>> +++ b/arch/riscv/boot/dts/microchip/microchip-mpfs-fabric.dtsi
>> @@ -0,0 +1,21 @@
>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
>> +/* Copyright (c) 2020-2021 Microchip Technology Inc */
>> +
>> +/ {
>> +       fpgadma: fpgadma@60020000 {
>> +               compatible = "microchip,mpfs-fpga-dma-uio";
>> +               #address-cells = <1>;
>> +               #size-cells = <0>;
>> +               reg = <0x0 0x60020000 0x0 0x1000>;
>> +               interrupt-parent = <&plic>;
>> +               interrupts = <PLIC_INT_FABRIC_F2H_2>;
>> +               status = "okay";
>> +       };
>> +
>> +       fpgalsram: fpga_lsram@61000000 {
>> +               compatible = "generic-uio";
>> +               reg = <0x0 0x61000000 0x0 0x0001000
>> +                       0x14 0x00000000 0x0 0x00010000>;
> 
> Please group by angular brackets, to increase readability, and support
> automatic validation:
> 
> <0x0 0x61000000 0x0 0x0001000>, <0x14 0x00000000 0x0 0x00010000>
> 
>> +               status = "okay";
>> +       };
> 
> Do we really want UIO nodes in upstream DT?
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.
> 
>> +};
>> diff --git a/arch/riscv/boot/dts/microchip/microchip-mpfs-icicle-kit.dts b/arch/riscv/boot/dts/microchip/microchip-mpfs-icicle-kit.dts
>> index fc1e5869df1b..4212129fcdf1 100644
>> --- 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. 
would it make more sense to call mmuart but alias it to serial?
ie serial0 = &mmuart0;
> 
>>          };
> 
>>
>> -&emac0 {
>> +&spi0 {
>> +       status = "okay";
>> +       spidev@0 {
>> +               compatible = "spidev";
> 
> Please don't use "spidev", but a proper compatible value describing
> what is really connected.  If you want to use it with spidev (which
> is software policy, not hardware description), add that compatible
> value to drivers/spi/spidev.c:spidev_dt_ids[], or use driver_override
> in sysfs at runtime.
as i said to Krzysztof - this one was an oversight from me, that 
compatible has never been "spidev"  on its own in our internal tree and 
i mustve accidentally omitted the vendor string while making these patches.

Either way, after talking some more we decided that this entry is not 
appropriate anyway and i will drop it.
> 
>> +               reg = <0>; /* CS 0 */
>> +               spi-max-frequency = <10000000>;
>> +               status = "okay";
>> +       };
>> +};
>> +
>> +&spi1 {
>> +       status = "okay";
> 
> No slave devices specified?
no, but its exposed
> 
>> +};
>> +
>> +&qspi {
>> +       status = "okay";
>> +};
>> +
>> +&i2c0 {
>> +       status = "okay";
>> +};
>> +
>> +&i2c1 {
>> +       status = "okay";
>> +       pac193x: pac193x@10 {
> 
> adc@, I guess?
sure
> 
>> +               compatible = "microchip,pac1934";
>> +               reg = <0x10>;
>> +               samp-rate = <64>;
>> +               status = "okay";
>> +               ch0: channel0 {
>> +                       uohms-shunt-res = <10000>;
>> +                       rail-name = "VDDREG";
>> +                       channel_enabled;
>> +               };
>> +               ch1: channel1 {
>> +                       uohms-shunt-res = <10000>;
>> +                       rail-name = "VDDA25";
>> +                       channel_enabled;
>> +               };
>> +               ch2: channel2 {
>> +                       uohms-shunt-res = <10000>;
>> +                       rail-name = "VDD25";
>> +                       channel_enabled;
>> +               };
>> +               ch3: channel3 {
>> +                       uohms-shunt-res = <10000>;
>> +                       rail-name = "VDDA_REG";
>> +                       channel_enabled;
>> +               };
>> +       };
>> +};
> 
>> +&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.

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.

> Please group using angular brackets.
> 
>> --- a/arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi
>> +++ b/arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi
> 
>> @@ -145,49 +149,55 @@ cpu4_intc: interrupt-controller {
>>          soc {
>>                  #address-cells = <2>;
>>                  #size-cells = <2>;
>> -               compatible = "simple-bus";
>> +               compatible = "microchip,mpfs-soc", "simple-bus";
>>                  ranges;
>>
>> -               cache-controller@2010000 {
>> +               cctrllr: cache-controller@2010000 {
>>                          compatible = "sifive,fu540-c000-ccache", "cache";
>> +                       reg = <0x0 0x2010000 0x0 0x1000>;
>> +                       interrupt-parent = <&plic>;
>> +                       interrupts = <PLIC_INT_L2_METADATA_CORR
>> +                               PLIC_INT_L2_METADAT_UNCORR
>> +                               PLIC_INT_L2_DATA_CORR>;
> 
> Please group using angular brackets.
> 
>>                          cache-block-size = <64>;
>>                          cache-level = <2>;
>>                          cache-sets = <1024>;
>>                          cache-size = <2097152>;
>>                          cache-unified;
>> -                       interrupt-parent = <&plic>;
>> -                       interrupts = <1 2 3>;
>> -                       reg = <0x0 0x2010000 0x0 0x1000>;
>>                  };
>>
>> -               clint@2000000 {
>> +               clint: clint@2000000 {
>>                          compatible = "sifive,fu540-c000-clint", "sifive,clint0";
>>                          reg = <0x0 0x2000000 0x0 0xC000>;
>> -                       interrupts-extended = <&cpu0_intc 3 &cpu0_intc 7
>> -                                               &cpu1_intc 3 &cpu1_intc 7
>> -                                               &cpu2_intc 3 &cpu2_intc 7
>> -                                               &cpu3_intc 3 &cpu3_intc 7
>> -                                               &cpu4_intc 3 &cpu4_intc 7>;
>> +                       interrupts-extended =
>> +                                       <&cpu0_intc HART_INT_M_SOFT &cpu0_intc HART_INT_M_TIMER
>> +                                        &cpu1_intc HART_INT_M_SOFT &cpu1_intc HART_INT_M_TIMER
>> +                                        &cpu2_intc HART_INT_M_SOFT &cpu2_intc HART_INT_M_TIMER
>> +                                        &cpu3_intc HART_INT_M_SOFT &cpu3_intc HART_INT_M_TIMER
>> +                                        &cpu4_intc HART_INT_M_SOFT &cpu4_intc HART_INT_M_TIMER>;
> 
> Please group using angular brackets.
> 
>>                  };
>>
>>                  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.
> 
>>                          reg = <0x0 0xc000000 0x0 0x4000000>;
>> +                       #interrupt-cells = <1>;
>>                          riscv,ndev = <186>;
>>                          interrupt-controller;
>> -                       interrupts-extended = <&cpu0_intc 11
>> -                                       &cpu1_intc 11 &cpu1_intc 9
>> -                                       &cpu2_intc 11 &cpu2_intc 9
>> -                                       &cpu3_intc 11 &cpu3_intc 9
>> -                                       &cpu4_intc 11 &cpu4_intc 9>;
>> +                       interrupts-extended = <&cpu0_intc HART_INT_M_EXT
>> +                                       &cpu1_intc HART_INT_M_EXT &cpu1_intc HART_INT_S_EXT
>> +                                       &cpu2_intc HART_INT_M_EXT &cpu2_intc HART_INT_S_EXT
>> +                                       &cpu3_intc HART_INT_M_EXT &cpu3_intc HART_INT_S_EXT
>> +                                       &cpu4_intc HART_INT_M_EXT &cpu4_intc HART_INT_S_EXT>;
>>                  };
>>
>> -               dma@3000000 {
>> -                       compatible = "sifive,fu540-c000-pdma";
>> +               pdma: pdma@3000000 {
>> +                       compatible = "microchip,mpfs-pdma-uio";
>>                          reg = <0x0 0x3000000 0x0 0x8000>;
>>                          interrupt-parent = <&plic>;
>> -                       interrupts = <23 24 25 26 27 28 29 30>;
>> +                       interrupts = <PLIC_INT_DMA_CH0_DONE PLIC_INT_DMA_CH0_ERR
>> +                               PLIC_INT_DMA_CH1_DONE PLIC_INT_DMA_CH1_ERR
>> +                               PLIC_INT_DMA_CH2_DONE PLIC_INT_DMA_CH2_ERR
>> +                               PLIC_INT_DMA_CH3_DONE PLIC_INT_DMA_CH3_ERR>;
> 
> Please group using angular brackets.
> 
>>                          #dma-cells = <1>;
>>                  };
>>
>> @@ -205,7 +215,7 @@ clkcfg: clkcfg@20002000 {
>>                          clocks = <&refclk>;
>>                          #clock-cells = <1>;
>>                          clock-output-names = "cpu", "axi", "ahb", "envm",       /* 0-3   */
>> -                                "mac0", "mac1", "mmc", "timer",                /* 4-7   */
>> +                               "mac0", "mac1", "mmc", "timer",                 /* 4-7   */
>>                                  "mmuart0", "mmuart1", "mmuart2", "mmuart3",     /* 8-11  */
>>                                  "mmuart4", "spi0", "spi1", "i2c0",              /* 12-15 */
>>                                  "i2c1", "can0", "can1", "usb",                  /* 16-19 */
> 
> No need for clock-output-names at all.
> 
>>
>> -               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.
> 
>> +                       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?
We set it in u-boot anyway, so I think dropping entirely is okay.
> 
>>                          status = "disabled";
>> +               };
>>
>> +               rtc: rtc@20124000 {
>> +                       compatible = "microchip,mpfs-rtc";
>>                          #address-cells = <1>;
>>                          #size-cells = <0>;
>> +                       reg = <0x0 0x20124000 0x0 0x1000>;
>> +                       clocks = <&clkcfg CLK_RTC>;
>> +                       clock-names = "rtc";
>> +                       interrupt-parent = <&plic>;
>> +                       interrupts = <PLIC_INT_RTC_WAKEUP PLIC_INT_RTC_MATCH>;
> 
> Please group using angular brackets.
> 
>> +                       status = "disabled";
>>                  };
>>
>> +               usb: usb@20201000 {
>> +                       compatible = "microchip,mpfs-usb-host";
>> +                       reg = <0x0 0x20201000 0x0 0x1000>;
>> +                       reg-names = "mc","control";
>> +                       clocks = <&clkcfg CLK_USB>;
>> +                       interrupt-parent = <&plic>;
>> +                       interrupts = <PLIC_INT_USB_DMA PLIC_INT_USB_MC>;
> 
> Please group using angular brackets.
> 
>> +                       interrupt-names = "dma","mc";
>> +                       dr_mode = "host";
>> +                       status = "disabled";
>> +               };
>> +
> 
> 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
>
Geert Uytterhoeven Nov. 10, 2021, 2:58 p.m. UTC | #5 
Hi Conor,

On Wed, Nov 10, 2021 at 3:20 PM <Conor.Dooley@microchip.com> wrote:
> On 09/11/2021 09:04, Geert Uytterhoeven wrote:
> > On Mon, Nov 8, 2021 at 4:07 PM <conor.dooley@microchip.com> wrote:
> >> 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 ;-)

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

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

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

> >>                  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?

> >> -               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.
> >
> >> +                       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
Conor Dooley Nov. 10, 2021, 3:07 p.m. UTC | #6 
On 10/11/2021 14:58, Geert Uytterhoeven wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Conor,
> 
>>>>                   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?
> 
yeah, ignore that. i confused this change (which is an accidental 
clobber) with another sifive compatible that was dropped for not being 
relevant. hardly makes sense to drop this one when i kept it for the 
clint and cache controller!

> 
> 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
>
Conor Dooley Nov. 15, 2021, 3:39 p.m. UTC | #7 
On 10/11/2021 14:58, Geert Uytterhoeven wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Conor,
> 
> On Wed, Nov 10, 2021 at 3:20 PM <Conor.Dooley@microchip.com> wrote:
>> On 09/11/2021 09:04, Geert Uytterhoeven wrote:
>>> On Mon, Nov 8, 2021 at 4:07 PM <conor.dooley@microchip.com> wrote:
>>>> From: Conor Dooley <conor.dooley@microchip.com>
>>>>
>>>> +&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.
The write (if was to be done) would be done by the bootloader, based on 
the bitstream written to the FPGA, before even u-boot is started. By 
application I meant the bootloader (or some other bare metal 
application), not a program running in userspace in case that's what you 
interpreted. Am I incorrect in thinking that if it is set up by the 
bootloader that Linux can take it for granted?
> 
>> 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.
> 
> 
> 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
>
Geert Uytterhoeven Nov. 15, 2021, 4:17 p.m. UTC | #8 
Hi Conor,

On Mon, Nov 15, 2021 at 4:39 PM <Conor.Dooley@microchip.com> wrote:
> On 10/11/2021 14:58, Geert Uytterhoeven wrote:
> > On Wed, Nov 10, 2021 at 3:20 PM <Conor.Dooley@microchip.com> wrote:
> >> On 09/11/2021 09:04, Geert Uytterhoeven wrote:
> >>> On Mon, Nov 8, 2021 at 4:07 PM <conor.dooley@microchip.com> wrote:
> >>>> From: Conor Dooley <conor.dooley@microchip.com>
> >>>>
> >>>> +&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.
>
> The write (if was to be done) would be done by the bootloader, based on
> the bitstream written to the FPGA, before even u-boot is started. By
> application I meant the bootloader (or some other bare metal
> application), not a program running in userspace in case that's what you
> interpreted. Am I incorrect in thinking that if it is set up by the
> bootloader that Linux can take it for granted?

If it is to be provided by the boot loader, the boot loader should fill
in the interrupts property, just like it already does (or should do, if it
doesn't) for /memory and chosen/bootargs.

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
Daire McNamara Nov. 17, 2021, 12:17 p.m. UTC | #9 
On Mon, 2021-11-15 at 17:17 +0100, Geert Uytterhoeven wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> Hi Conor,
> 
> On Mon, Nov 15, 2021 at 4:39 PM <Conor.Dooley@microchip.com> wrote:
> > On 10/11/2021 14:58, Geert Uytterhoeven wrote:
> > > On Wed, Nov 10, 2021 at 3:20 PM <Conor.Dooley@microchip.com>
> > > wrote:
> > > > On 09/11/2021 09:04, Geert Uytterhoeven wrote:
> > > > > On Mon, Nov 8, 2021 at 4:07 PM <conor.dooley@microchip.com>
> > > > > wrote:
> > > > > > From: Conor Dooley <conor.dooley@microchip.com>
> > > > > > 
> > > > > > +&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.
> > 
> > The write (if was to be done) would be done by the bootloader,
> > based on
> > the bitstream written to the FPGA, before even u-boot is started.
> > By
> > application I meant the bootloader (or some other bare metal
> > application), not a program running in userspace in case that's
> > what you
> > interpreted. Am I incorrect in thinking that if it is set up by the
> > bootloader that Linux can take it for granted?
> 
> If it is to be provided by the boot loader, the boot loader should
> fill
> in the interrupts property, just like it already does (or should do,
> if it
> doesn't) for /memory and chosen/bootargs.
Whether a given GPIO is routed via a bank where it has its own
interrupt line or via a bank where it shares an interrupt line is an
SoC capability.  A particular routing is instantiated by a particular
board (e.g. Icicle).  A custom bootloader feels like complete overkill
for this job.
> 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
diff mbox series

Patch

diff --git a/arch/riscv/boot/dts/microchip/microchip-mpfs-fabric.dtsi b/arch/riscv/boot/dts/microchip/microchip-mpfs-fabric.dtsi
new file mode 100644
index 000000000000..8fa3356494f1
--- /dev/null
+++ b/arch/riscv/boot/dts/microchip/microchip-mpfs-fabric.dtsi
@@ -0,0 +1,21 @@ 
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+/* Copyright (c) 2020-2021 Microchip Technology Inc */
+
+/ {
+	fpgadma: fpgadma@60020000 {
+		compatible = "microchip,mpfs-fpga-dma-uio";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <0x0 0x60020000 0x0 0x1000>;
+		interrupt-parent = <&plic>;
+		interrupts = <PLIC_INT_FABRIC_F2H_2>;
+		status = "okay";
+	};
+
+	fpgalsram: fpga_lsram@61000000 {
+		compatible = "generic-uio";
+		reg = <0x0 0x61000000 0x0 0x0001000
+			0x14 0x00000000 0x0 0x00010000>;
+		status = "okay";
+	};
+};
diff --git a/arch/riscv/boot/dts/microchip/microchip-mpfs-icicle-kit.dts b/arch/riscv/boot/dts/microchip/microchip-mpfs-icicle-kit.dts
index fc1e5869df1b..4212129fcdf1 100644
--- 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;
 	};
 
 	chosen {
-		stdout-path = "serial0:115200n8";
+		stdout-path = "mmuart1:115200n8";
 	};
 
 	cpus {
 		timebase-frequency = <RTCCLK_FREQ>;
 	};
 
-	memory@80000000 {
+	ddrc_cache_lo: memory@80000000 {
 		device_type = "memory";
-		reg = <0x0 0x80000000 0x0 0x40000000>;
-		clocks = <&clkcfg 26>;
+		reg = <0x0 0x80000000 0x0 0x2e000000>;
+		clocks = <&clkcfg CLK_DDRC>;
+		status = "okay";
+	};
+
+	ddrc_cache_hi: memory@1000000000 {
+		device_type = "memory";
+		reg = <0x10 0x0 0x0 0x40000000>;
+		clocks = <&clkcfg CLK_DDRC>;
+		status = "okay";
 	};
 };
 
-&serial0 {
+&mmuart1 {
 	status = "okay";
 };
 
-&serial1 {
+&mmuart2 {
 	status = "okay";
 };
 
-&serial2 {
+&mmuart3 {
 	status = "okay";
 };
 
-&serial3 {
+&mmuart4 {
 	status = "okay";
 };
 
 &mmc {
 	status = "okay";
-
 	bus-width = <4>;
 	disable-wp;
 	cap-sd-highspeed;
+	cap-mmc-highspeed;
 	card-detect-delay = <200>;
+	mmc-ddr-1_8v;
+	mmc-hs200-1_8v;
 	sd-uhs-sdr12;
 	sd-uhs-sdr25;
 	sd-uhs-sdr50;
 	sd-uhs-sdr104;
 };
 
-&emac0 {
+&spi0 {
+	status = "okay";
+	spidev@0 {
+		compatible = "spidev";
+		reg = <0>; /* CS 0 */
+		spi-max-frequency = <10000000>;
+		status = "okay";
+	};
+};
+
+&spi1 {
+	status = "okay";
+};
+
+&qspi {
+	status = "okay";
+};
+
+&i2c0 {
+	status = "okay";
+};
+
+&i2c1 {
+	status = "okay";
+	pac193x: pac193x@10 {
+		compatible = "microchip,pac1934";
+		reg = <0x10>;
+		samp-rate = <64>;
+		status = "okay";
+		ch0: channel0 {
+			uohms-shunt-res = <10000>;
+			rail-name = "VDDREG";
+			channel_enabled;
+		};
+		ch1: channel1 {
+			uohms-shunt-res = <10000>;
+			rail-name = "VDDA25";
+			channel_enabled;
+		};
+		ch2: channel2 {
+			uohms-shunt-res = <10000>;
+			rail-name = "VDD25";
+			channel_enabled;
+		};
+		ch3: channel3 {
+			uohms-shunt-res = <10000>;
+			rail-name = "VDDA_REG";
+			channel_enabled;
+		};
+	};
+};
+
+&mac0 {
+	status = "okay";
 	phy-mode = "sgmii";
 	phy-handle = <&phy0>;
-	phy0: ethernet-phy@8 {
-		reg = <8>;
-		ti,fifo-depth = <0x01>;
-	};
 };
 
-&emac1 {
+&mac1 {
 	status = "okay";
 	phy-mode = "sgmii";
 	phy-handle = <&phy1>;
 	phy1: ethernet-phy@9 {
 		reg = <9>;
-		ti,fifo-depth = <0x01>;
+		ti,fifo-depth = <0x1>;
+	};
+	phy0: ethernet-phy@8 {
+		reg = <8>;
+		ti,fifo-depth = <0x1>;
 	};
 };
+
+&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>;
+	status = "okay";
+};
+
+&rtc {
+	status = "okay";
+};
+
+&usb {
+	status = "okay";
+};
+
+&mbox {
+	status = "okay";
+};
+
+&pcie {
+	status = "okay";
+};
diff --git a/arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi b/arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi
index c9f6d205d2ba..805e07f0169e 100644
--- a/arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi
+++ b/arch/riscv/boot/dts/microchip/microchip-mpfs.dtsi
@@ -1,7 +1,10 @@ 
 // SPDX-License-Identifier: (GPL-2.0 OR MIT)
-/* Copyright (c) 2020 Microchip Technology Inc */
+/* Copyright (c) 2020-2021 Microchip Technology Inc */
 
-/dts-v1/;
+#include "dt-bindings/clock/microchip,mpfs-clock.h"
+#include "dt-bindings/interrupt-controller/microchip,mpfs-plic.h"
+#include "dt-bindings/interrupt-controller/riscv-hart.h"
+#include "microchip-mpfs-fabric.dtsi"
 
 / {
 	#address-cells = <2>;
@@ -16,8 +19,7 @@  cpus {
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-		cpu@0 {
-			clock-frequency = <0>;
+		cpu0: cpu@0 {
 			compatible = "sifive,e51", "sifive,rocket0", "riscv";
 			device_type = "cpu";
 			i-cache-block-size = <64>;
@@ -25,6 +27,7 @@  cpu@0 {
 			i-cache-size = <16384>;
 			reg = <0>;
 			riscv,isa = "rv64imac";
+			clocks = <&clkcfg CLK_CPU>;
 			status = "disabled";
 
 			cpu0_intc: interrupt-controller {
@@ -34,8 +37,7 @@  cpu0_intc: interrupt-controller {
 			};
 		};
 
-		cpu@1 {
-			clock-frequency = <0>;
+		cpu1: cpu@1 {
 			compatible = "sifive,u54-mc", "sifive,rocket0", "riscv";
 			d-cache-block-size = <64>;
 			d-cache-sets = <64>;
@@ -51,6 +53,7 @@  cpu@1 {
 			mmu-type = "riscv,sv39";
 			reg = <1>;
 			riscv,isa = "rv64imafdc";
+			clocks = <&clkcfg CLK_CPU>;
 			tlb-split;
 			status = "okay";
 
@@ -61,8 +64,7 @@  cpu1_intc: interrupt-controller {
 			};
 		};
 
-		cpu@2 {
-			clock-frequency = <0>;
+		cpu2: cpu@2 {
 			compatible = "sifive,u54-mc", "sifive,rocket0", "riscv";
 			d-cache-block-size = <64>;
 			d-cache-sets = <64>;
@@ -78,6 +80,7 @@  cpu@2 {
 			mmu-type = "riscv,sv39";
 			reg = <2>;
 			riscv,isa = "rv64imafdc";
+			clocks = <&clkcfg CLK_CPU>;
 			tlb-split;
 			status = "okay";
 
@@ -88,8 +91,7 @@  cpu2_intc: interrupt-controller {
 			};
 		};
 
-		cpu@3 {
-			clock-frequency = <0>;
+		cpu3: cpu@3 {
 			compatible = "sifive,u54-mc", "sifive,rocket0", "riscv";
 			d-cache-block-size = <64>;
 			d-cache-sets = <64>;
@@ -105,6 +107,7 @@  cpu@3 {
 			mmu-type = "riscv,sv39";
 			reg = <3>;
 			riscv,isa = "rv64imafdc";
+			clocks = <&clkcfg CLK_CPU>;
 			tlb-split;
 			status = "okay";
 
@@ -115,8 +118,7 @@  cpu3_intc: interrupt-controller {
 			};
 		};
 
-		cpu@4 {
-			clock-frequency = <0>;
+		cpu4: cpu@4 {
 			compatible = "sifive,u54-mc", "sifive,rocket0", "riscv";
 			d-cache-block-size = <64>;
 			d-cache-sets = <64>;
@@ -132,8 +134,10 @@  cpu@4 {
 			mmu-type = "riscv,sv39";
 			reg = <4>;
 			riscv,isa = "rv64imafdc";
+			clocks = <&clkcfg CLK_CPU>;
 			tlb-split;
 			status = "okay";
+
 			cpu4_intc: interrupt-controller {
 				#interrupt-cells = <1>;
 				compatible = "riscv,cpu-intc";
@@ -145,49 +149,55 @@  cpu4_intc: interrupt-controller {
 	soc {
 		#address-cells = <2>;
 		#size-cells = <2>;
-		compatible = "simple-bus";
+		compatible = "microchip,mpfs-soc", "simple-bus";
 		ranges;
 
-		cache-controller@2010000 {
+		cctrllr: cache-controller@2010000 {
 			compatible = "sifive,fu540-c000-ccache", "cache";
+			reg = <0x0 0x2010000 0x0 0x1000>;
+			interrupt-parent = <&plic>;
+			interrupts = <PLIC_INT_L2_METADATA_CORR
+				PLIC_INT_L2_METADAT_UNCORR
+				PLIC_INT_L2_DATA_CORR>;
 			cache-block-size = <64>;
 			cache-level = <2>;
 			cache-sets = <1024>;
 			cache-size = <2097152>;
 			cache-unified;
-			interrupt-parent = <&plic>;
-			interrupts = <1 2 3>;
-			reg = <0x0 0x2010000 0x0 0x1000>;
 		};
 
-		clint@2000000 {
+		clint: clint@2000000 {
 			compatible = "sifive,fu540-c000-clint", "sifive,clint0";
 			reg = <0x0 0x2000000 0x0 0xC000>;
-			interrupts-extended = <&cpu0_intc 3 &cpu0_intc 7
-						&cpu1_intc 3 &cpu1_intc 7
-						&cpu2_intc 3 &cpu2_intc 7
-						&cpu3_intc 3 &cpu3_intc 7
-						&cpu4_intc 3 &cpu4_intc 7>;
+			interrupts-extended =
+					<&cpu0_intc HART_INT_M_SOFT &cpu0_intc HART_INT_M_TIMER
+					 &cpu1_intc HART_INT_M_SOFT &cpu1_intc HART_INT_M_TIMER
+					 &cpu2_intc HART_INT_M_SOFT &cpu2_intc HART_INT_M_TIMER
+					 &cpu3_intc HART_INT_M_SOFT &cpu3_intc HART_INT_M_TIMER
+					 &cpu4_intc HART_INT_M_SOFT &cpu4_intc HART_INT_M_TIMER>;
 		};
 
 		plic: interrupt-controller@c000000 {
-			#interrupt-cells = <1>;
-			compatible = "sifive,fu540-c000-plic", "sifive,plic-1.0.0";
+			compatible = "sifive,plic-1.0.0";
 			reg = <0x0 0xc000000 0x0 0x4000000>;
+			#interrupt-cells = <1>;
 			riscv,ndev = <186>;
 			interrupt-controller;
-			interrupts-extended = <&cpu0_intc 11
-					&cpu1_intc 11 &cpu1_intc 9
-					&cpu2_intc 11 &cpu2_intc 9
-					&cpu3_intc 11 &cpu3_intc 9
-					&cpu4_intc 11 &cpu4_intc 9>;
+			interrupts-extended = <&cpu0_intc HART_INT_M_EXT
+					&cpu1_intc HART_INT_M_EXT &cpu1_intc HART_INT_S_EXT
+					&cpu2_intc HART_INT_M_EXT &cpu2_intc HART_INT_S_EXT
+					&cpu3_intc HART_INT_M_EXT &cpu3_intc HART_INT_S_EXT
+					&cpu4_intc HART_INT_M_EXT &cpu4_intc HART_INT_S_EXT>;
 		};
 
-		dma@3000000 {
-			compatible = "sifive,fu540-c000-pdma";
+		pdma: pdma@3000000 {
+			compatible = "microchip,mpfs-pdma-uio";
 			reg = <0x0 0x3000000 0x0 0x8000>;
 			interrupt-parent = <&plic>;
-			interrupts = <23 24 25 26 27 28 29 30>;
+			interrupts = <PLIC_INT_DMA_CH0_DONE PLIC_INT_DMA_CH0_ERR
+				PLIC_INT_DMA_CH1_DONE PLIC_INT_DMA_CH1_ERR
+				PLIC_INT_DMA_CH2_DONE PLIC_INT_DMA_CH2_ERR
+				PLIC_INT_DMA_CH3_DONE PLIC_INT_DMA_CH3_ERR>;
 			#dma-cells = <1>;
 		};
 
@@ -205,7 +215,7 @@  clkcfg: clkcfg@20002000 {
 			clocks = <&refclk>;
 			#clock-cells = <1>;
 			clock-output-names = "cpu", "axi", "ahb", "envm",	/* 0-3   */
-				 "mac0", "mac1", "mmc", "timer",		/* 4-7   */
+				"mac0", "mac1", "mmc", "timer",			/* 4-7   */
 				"mmuart0", "mmuart1", "mmuart2", "mmuart3",	/* 8-11  */
 				"mmuart4", "spi0", "spi1", "i2c0",		/* 12-15 */
 				"i2c1", "can0", "can1", "usb",			/* 16-19 */
@@ -214,90 +224,287 @@  clkcfg: clkcfg@20002000 {
 				"fic1", "fic2", "fic3", "athena", "cfm";	/* 28-32 */
 		};
 
-		serial0: serial@20000000 {
+		mmuart0: serial@20000000 {
 			compatible = "ns16550a";
 			reg = <0x0 0x20000000 0x0 0x400>;
 			reg-io-width = <4>;
 			reg-shift = <2>;
+			clocks = <&clkcfg CLK_MMUART0>;
 			interrupt-parent = <&plic>;
-			interrupts = <90>;
+			interrupts = <PLIC_INT_MMUART0>;
 			current-speed = <115200>;
-			clocks = <&clkcfg 8>;
-			status = "disabled";
+			status = "disabled"; /* Reserved for the HSS */
 		};
 
-		serial1: serial@20100000 {
+		mmuart1: serial@20100000 {
 			compatible = "ns16550a";
 			reg = <0x0 0x20100000 0x0 0x400>;
 			reg-io-width = <4>;
 			reg-shift = <2>;
+			clocks = <&clkcfg CLK_MMUART1>;
 			interrupt-parent = <&plic>;
-			interrupts = <91>;
+			interrupts = <PLIC_INT_MMUART1>;
 			current-speed = <115200>;
-			clocks = <&clkcfg 9>;
 			status = "disabled";
 		};
 
-		serial2: serial@20102000 {
+		mmuart2: serial@20102000 {
 			compatible = "ns16550a";
 			reg = <0x0 0x20102000 0x0 0x400>;
 			reg-io-width = <4>;
 			reg-shift = <2>;
+			clocks = <&clkcfg CLK_MMUART2>;
 			interrupt-parent = <&plic>;
-			interrupts = <92>;
+			interrupts = <PLIC_INT_MMUART2>;
 			current-speed = <115200>;
-			clocks = <&clkcfg 10>;
 			status = "disabled";
 		};
 
-		serial3: serial@20104000 {
+		mmuart3: serial@20104000 {
 			compatible = "ns16550a";
 			reg = <0x0 0x20104000 0x0 0x400>;
 			reg-io-width = <4>;
 			reg-shift = <2>;
+			clocks = <&clkcfg CLK_MMUART3>;
+			interrupt-parent = <&plic>;
+			interrupts = <PLIC_INT_MMUART3>;
+			current-speed = <115200>;
+			status = "disabled";
+		};
+
+		mmuart4: serial@20106000 {
+			compatible = "ns16550a";
+			reg = <0x0 0x20106000 0x0 0x400>;
+			reg-io-width = <4>;
+			reg-shift = <2>;
+			clocks = <&clkcfg CLK_MMUART4>;
 			interrupt-parent = <&plic>;
-			interrupts = <93>;
+			interrupts = <PLIC_INT_MMUART4>;
 			current-speed = <115200>;
-			clocks = <&clkcfg 11>;
 			status = "disabled";
 		};
 
-		/* Common node entry for emmc/sd */
-		mmc: mmc@20008000 {
+		mmc: mmc@20008000 { /* Common node entry for emmc/sd */
 			compatible = "microchip,mpfs-sd4hc", "cdns,sd4hc";
 			reg = <0x0 0x20008000 0x0 0x1000>;
+			clocks = <&clkcfg CLK_MMC>;
 			interrupt-parent = <&plic>;
-			interrupts = <88 89>;
-			clocks = <&clkcfg 6>;
+			interrupts = <PLIC_INT_MMC_MAIN PLIC_INT_MMC_WAKEUP>;
 			max-frequency = <200000000>;
 			status = "disabled";
 		};
 
-		emac0: ethernet@20110000 {
-			compatible = "cdns,macb";
-			reg = <0x0 0x20110000 0x0 0x2000>;
+		spi0: spi@20108000 {
+			compatible = "microchip,mpfs-spi";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0x0 0x20108000 0x0 0x1000>;
+			clocks = <&clkcfg CLK_SPI0>;
 			interrupt-parent = <&plic>;
-			interrupts = <64 65 66 67>;
-			local-mac-address = [00 00 00 00 00 00];
-			clocks = <&clkcfg 4>, <&clkcfg 2>;
-			clock-names = "pclk", "hclk";
+			interrupts = <PLIC_INT_SPI0>;
+			spi-max-frequency = <25000000>;
+			num-cs = <8>;
+			status = "disabled";
+		};
+
+		spi1: spi@20109000 {
+			compatible = "microchip,mpfs-spi";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0x0 0x20109000 0x0 0x1000>;
+			clocks = <&clkcfg CLK_SPI1>;
+			interrupt-parent = <&plic>;
+			interrupts = <PLIC_INT_SPI1>;
+			spi-max-frequency = <25000000>;
+			num-cs = <8>;
+			status = "disabled";
+		};
+
+		qspi: qspi@21000000 {
+			compatible = "microchip,mpfs-qspi";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0x0 0x21000000 0x0 0x1000>;
+			clocks = <&clkcfg CLK_QSPI>;
+			interrupt-parent = <&plic>;
+			interrupts = <PLIC_INT_QSPI>;
+			spi-max-frequency = <25000000>;
+			num-cs = <8>;
+			status = "disabled";
+		};
+
+		i2c0: i2c@2010a000 {
+			compatible = "microchip,mpfs-i2c";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0x0 0x2010a000 0x0 0x1000>;
+			clocks = <&clkcfg CLK_I2C0>;
+			interrupt-parent = <&plic>;
+			interrupts = <PLIC_INT_I2C0_MAIN>;
+			clock-frequency = <100000>;
 			status = "disabled";
+		};
+
+		i2c1: i2c@2010b000 {
+			compatible = "microchip,mpfs-i2c";
 			#address-cells = <1>;
 			#size-cells = <0>;
+			reg = <0x0 0x2010b000 0x0 0x1000>;
+			clocks = <&clkcfg CLK_I2C1>;
+			interrupt-parent = <&plic>;
+			interrupts = <PLIC_INT_I2C1_MAIN>;
+			clock-frequency = <100000>;
+			status = "disabled";
 		};
 
-		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>;
+			mac-address = [56 34 12 00 FC 01];
 			status = "disabled";
+		};
+
+		mac1: ethernet@20112000 {
+			compatible = "cdns,macb";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0x0 0x20112000 0x0 0x2000>;
+			clocks = <&clkcfg CLK_MAC1>, <&clkcfg CLK_AHB>;
 			clock-names = "pclk", "hclk";
+			interrupt-parent = <&plic>;
+			interrupts = <PLIC_INT_MAC1_INT
+				PLIC_INT_MAC1_QUEUE1
+				PLIC_INT_MAC1_QUEUE2
+				PLIC_INT_MAC1_QUEUE3
+				PLIC_INT_MAC1_EMAC
+				PLIC_INT_MAC1_MMSL>;
+			mac-address = [56 34 12 00 FC 02];
+			status = "disabled";
+		};
+
+		gpio0: gpio@20120000 {
+			compatible = "microchip,mpfs-gpio";
+			reg = <0x0 0x20120000 0x0 0x1000>;
+			clocks = <&clkcfg CLK_GPIO0>;
+			interrupt-parent = <&plic>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			status = "disabled";
+		};
+
+		gpio1: gpio@20121000 {
+			compatible = "microchip,mpfs-gpio";
+			reg = <000 0x20121000 0x0 0x1000>;
+			clocks = <&clkcfg CLK_GPIO1>;
+			interrupt-parent = <&plic>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			status = "disabled";
+		};
+
+		gpio2: gpio@20122000 {
+			compatible = "microchip,mpfs-gpio";
+			reg = <0x0 0x20122000 0x0 0x1000>;
+			clocks = <&clkcfg CLK_GPIO2>;
+			interrupt-parent = <&plic>;
+			gpio-controller;
+			#gpio-cells = <2>;
+			status = "disabled";
+		};
+
+		rtc: rtc@20124000 {
+			compatible = "microchip,mpfs-rtc";
 			#address-cells = <1>;
 			#size-cells = <0>;
+			reg = <0x0 0x20124000 0x0 0x1000>;
+			clocks = <&clkcfg CLK_RTC>;
+			clock-names = "rtc";
+			interrupt-parent = <&plic>;
+			interrupts = <PLIC_INT_RTC_WAKEUP PLIC_INT_RTC_MATCH>;
+			status = "disabled";
 		};
 
+		usb: usb@20201000 {
+			compatible = "microchip,mpfs-usb-host";
+			reg = <0x0 0x20201000 0x0 0x1000>;
+			reg-names = "mc","control";
+			clocks = <&clkcfg CLK_USB>;
+			interrupt-parent = <&plic>;
+			interrupts = <PLIC_INT_USB_DMA PLIC_INT_USB_MC>;
+			interrupt-names = "dma","mc";
+			dr_mode = "host";
+			status = "disabled";
+		};
+
+		mbox: mailbox@37020000 {
+			compatible = "microchip,mpfs-mailbox";
+			reg = <0x0 0x37020000 0x0 0x1000>, <0x0 0x2000318C 0x0 0x40>;
+			interrupt-parent = <&plic>;
+			interrupts = <PLIC_INT_G5C_MESSAGE>;
+			#mbox-cells = <1>;
+			status = "disabled";
+		};
+
+		pcie: pcie@2000000000 {
+			compatible = "microchip,pcie-host-1.0";
+			#address-cells = <0x3>;
+			#interrupt-cells = <0x1>;
+			#size-cells = <0x2>;
+			device_type = "pci";
+			reg = <0x20 0x0 0x0 0x8000000 0x0 0x43000000 0x0 0x10000>;
+			reg-names = "cfg", "apb";
+			clocks = <&clkcfg CLK_FIC0>, <&clkcfg CLK_FIC1>, <&clkcfg CLK_FIC3>;
+			clock-names = "fic0", "fic1", "fic3";
+			bus-range = <0x0 0x7f>;
+			interrupt-parent = <&plic>;
+			interrupts = <PLIC_INT_FABRIC_F2H_1>;
+			interrupt-map = <0 0 0 1 &pcie_intc 0>,
+					<0 0 0 2 &pcie_intc 1>,
+					<0 0 0 3 &pcie_intc 2>,
+					<0 0 0 4 &pcie_intc 3>;
+			interrupt-map-mask = <0 0 0 7>;
+			ranges = <0x3000000 0x0 0x8000000 0x20 0x8000000 0x0 0x80000000>;
+			msi-parent = <&pcie>;
+			msi-controller;
+			mchp,axi-m-atr0 = <0x10 0x0>;
+			status = "disabled";
+			pcie_intc: legacy-interrupt-controller {
+				#address-cells = <0>;
+				#interrupt-cells = <1>;
+				interrupt-controller;
+			};
+		};
+
+		syscontroller: syscontroller {
+			compatible = "microchip,mpfs-sys-controller";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			mboxes = <&mbox 0>;
+		};
+
+		hwrandom: hwrandom {
+			compatible = "microchip,mpfs-rng";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			syscontroller = <&syscontroller>;
+		};
+
+		sysserv: sysserv {
+			compatible = "microchip,mpfs-generic-service";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			syscontroller = <&syscontroller>;
+		};
 	};
 };