Hi Conor,
On Wed, Nov 10, 2021 at 3:20 PM <[email protected]> wrote:
> On 09/11/2021 09:04, Geert Uytterhoeven wrote:
> > On Mon, Nov 8, 2021 at 4:07 PM <[email protected]> wrote:
> >> From: Conor Dooley <[email protected]>
> >>
> >> 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 <[email protected]>
> 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 -- [email protected]
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
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 -- [email protected]
>
> 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
>
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 <[email protected]> wrote:
>> On 09/11/2021 09:04, Geert Uytterhoeven wrote:
>>> On Mon, Nov 8, 2021 at 4:07 PM <[email protected]> wrote:
>>>> From: Conor Dooley <[email protected]>
>>>>
>>>> +&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 -- [email protected]
>
> 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
>
Hi Conor,
On Mon, Nov 15, 2021 at 4:39 PM <[email protected]> wrote:
> On 10/11/2021 14:58, Geert Uytterhoeven wrote:
> > On Wed, Nov 10, 2021 at 3:20 PM <[email protected]> wrote:
> >> On 09/11/2021 09:04, Geert Uytterhoeven wrote:
> >>> On Mon, Nov 8, 2021 at 4:07 PM <[email protected]> wrote:
> >>>> From: Conor Dooley <[email protected]>
> >>>>
> >>>> +&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 -- [email protected]
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
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 <[email protected]> wrote:
> > On 10/11/2021 14:58, Geert Uytterhoeven wrote:
> > > On Wed, Nov 10, 2021 at 3:20 PM <[email protected]>
> > > wrote:
> > > > On 09/11/2021 09:04, Geert Uytterhoeven wrote:
> > > > > On Mon, Nov 8, 2021 at 4:07 PM <[email protected]>
> > > > > wrote:
> > > > > > From: Conor Dooley <[email protected]>
> > > > > >
> > > > > > +&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 --
> [email protected]
>
> 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