Re: [PATCH 1/3] dts: socfpga: Change Mercury+ AA1 devicetree to header

[Paweł Anikiel]

Thank you for the review, I'm thinking of splitting this commit into
several smaller ones:
- remove all status = "okay" things and i2c aliases
- remove sdmmc-ecc node (it's a part of the Arria 10 SoC, not the mercury)
- add atsha204a node
- add copyright header
- style fixes (phy reg, memory)
What do you think?

Please see my other comments below.

On Mon, May 30, 2022 at 8:55 PM Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> wrote:
>
> On 30/05/2022 15:08, Paweł Anikiel wrote:
> > The Mercury+ AA1 is not a standalone board, rather it's a module
> > with an Arria 10 SoC and some peripherals on it. Remove everything that
> > is not strictly related to the module.
>
> Subject has several issues:
> 1. Use prefix matching subsystem (git log --oneline)

Just to make sure, are you referring to the ARM: prefix?

> 2. You are not changing here anything to header. Header files have
> different format and end with .h. This is a DTSI file.

Yes, I meant dtsi.

>
> >
> > Signed-off-by: Paweł Anikiel <pan@xxxxxxxxxxxx>
>
> Thank you for your patch. There is something to discuss/improve.
>
> > ---
> >  arch/arm/boot/dts/Makefile                    |  1 -
> >  ...1.dts => socfpga_arria10_mercury_aa1.dtsi} | 68 ++++---------------
> >  2 files changed, 14 insertions(+), 55 deletions(-)
> >  rename arch/arm/boot/dts/{socfpga_arria10_mercury_aa1.dts => socfpga_arria10_mercury_aa1.dtsi} (58%)
> >
> > diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> > index edfbedaa6168..023c8b4ba45c 100644
> > --- a/arch/arm/boot/dts/Makefile
> > +++ b/arch/arm/boot/dts/Makefile
> > @@ -1146,7 +1146,6 @@ dtb-$(CONFIG_ARCH_S5PV210) += \
> >       s5pv210-torbreck.dtb
> >  dtb-$(CONFIG_ARCH_INTEL_SOCFPGA) += \
> >       socfpga_arria5_socdk.dtb \
> > -     socfpga_arria10_mercury_aa1.dtb \
> >       socfpga_arria10_socdk_nand.dtb \
> >       socfpga_arria10_socdk_qspi.dtb \
> >       socfpga_arria10_socdk_sdmmc.dtb \
> > diff --git a/arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dts b/arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dtsi
> > similarity index 58%
> > rename from arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dts
> > rename to arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dtsi
> > index a75c059b6727..fee1fc39bb2b 100644
> > --- a/arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dts
> > +++ b/arch/arm/boot/dts/socfpga_arria10_mercury_aa1.dtsi
> > @@ -1,57 +1,38 @@
> >  // SPDX-License-Identifier: GPL-2.0
> > -/dts-v1/;
> > -
> > +/*
> > + * Copyright 2022 Google LLC
> > + */
>
> How is this related to the patch?

I'll move this change to a seperate commit.

>
> >  #include "socfpga_arria10.dtsi"
> >
> >  / {
> > -
> > -     model = "Enclustra Mercury AA1";
> > -     compatible = "enclustra,mercury-aa1", "altr,socfpga-arria10", "altr,socfpga";
> > -
> >       aliases {
> >               ethernet0 = &gmac0;
> >               serial1 = &uart1;
> > -             i2c0 = &i2c0;
> > -             i2c1 = &i2c1;
> > -     };
> > -
> > -     memory@0 {
> > -             name = "memory";
> > -             device_type = "memory";
> > -             reg = <0x0 0x80000000>; /* 2GB */
> >       };
> >
> >       chosen {
> >               stdout-path = "serial1:115200n8";
> >       };
> > -};
> >
> > -&eccmgr {
> > -     sdmmca-ecc@ff8c2c00 {
> > -             compatible = "altr,socfpga-sdmmc-ecc";
> > -             reg = <0xff8c2c00 0x400>;
> > -             altr,ecc-parent = <&mmc>;
> > -             interrupts = <15 IRQ_TYPE_LEVEL_HIGH>,
> > -                          <47 IRQ_TYPE_LEVEL_HIGH>,
> > -                          <16 IRQ_TYPE_LEVEL_HIGH>,
> > -                          <48 IRQ_TYPE_LEVEL_HIGH>;
> > +     memory@0 {
> > +             name = "memory";
> > +             device_type = "memory";
> > +             reg = <0x0 0x80000000>; /* 2GB */
> >       };
> >  };
> >
> >  &gmac0 {
> >       phy-mode = "rgmii";
> > -     phy-addr = <0xffffffff>; /* probe for phy addr */
> > +     phy-handle = <&phy3>;
> >
> >       max-frame-size = <3800>;
> > -     status = "okay";
> > -
> > -     phy-handle = <&phy3>;
> >
> >       mdio {
> >               #address-cells = <1>;
> >               #size-cells = <0>;
> >               compatible = "snps,dwmac-mdio";
> >               phy3: ethernet-phy@3 {
> > +                     reg = <3>;
> >                       txd0-skew-ps = <0>; /* -420ps */
> >                       txd1-skew-ps = <0>; /* -420ps */
> >                       txd2-skew-ps = <0>; /* -420ps */
> > @@ -64,35 +45,23 @@ phy3: ethernet-phy@3 {
> >                       txc-skew-ps = <1860>; /* 960ps */
> >                       rxdv-skew-ps = <420>; /* 0ps */
> >                       rxc-skew-ps = <1680>; /* 780ps */
> > -                     reg = <3>;
>
> This and few other changes (like memory) are not related to the commit.
> Do not mix different cleanups together.

l'll put the cleanup changes into a seperate commit.

>
> >               };
> >       };
> >  };
> >
> > -&gpio0 {
> > -     status = "okay";
> > -};
> > -
> > -&gpio1 {
> > -     status = "okay";
> > -};
> > -
> > -&gpio2 {
> > -     status = "okay";
> > -};
>
> Why removing all these? Aren't they available on the SoM? The same
> question applies to several other pieces, for example UART and USB -
> aren't these part of SoM?

I'm removing them from here, but adding them to socfpga_arria10_chameleonv3.dts.
I think that enabling them should be done in the base board's dts, as
the connections
go to the base board. The Chameleon v3 has a USB port, but a different
base board might
not have one.

>
> > -
> >  &i2c1 {
> > -     status = "okay";
> > +     atsha204a: atsha204a@64 {
>
> How is this change related at all to what you described in commit? There
> was no atsha204a before.

As previously mentioned, I'll move this change to a seperate commit.

>
> > +             compatible = "atmel,atsha204a";
> > +             reg = <0x64>;
> > +     };
> > +
> >       isl12022: isl12022@6f {
> > -             status = "okay";
> >               compatible = "isil,isl12022";
> >               reg = <0x6f>;
> >       };
> >  };
> >
>
> Best regards,
> Krzysztof


Regards,
Paweł