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

[<Conor.Dooley@microchip.com>]

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
>