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ł
