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

[Geert Uytterhoeven <geert@linux-m68k.org>]

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