| Message ID | 20220530130839.120710-2-pan@semihalf.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Chameleon v3 devicetree | expand |
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) 2. You are not changing here anything to header. Header files have different format and end with .h. This is a DTSI file. > > Signed-off-by: Paweł Anikiel <pan@semihalf.com> 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? > #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. > }; > }; > }; > > -&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? > - > &i2c1 { > - status = "okay"; > + atsha204a: atsha204a@64 { How is this change related at all to what you described in commit? There was no atsha204a before. > + compatible = "atmel,atsha204a"; > + reg = <0x64>; > + }; > + > isl12022: isl12022@6f { > - status = "okay"; > compatible = "isil,isl12022"; > reg = <0x6f>; > }; > }; > Best regards, Krzysztof
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@linaro.org> 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@semihalf.com> > > 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ł
On 31/05/2022 14:43, Paweł Anikiel wrote: > Thank you for the review, I'm thinking of splitting this commit into > several smaller ones: > - remove all status = "okay" things and i2c aliases This might have sense, might not. Depends whether the interface is actively used in the SoM or not. If the latter - the interface is only exposed to the carrier board - then this seems reasonable choice. > - remove sdmmc-ecc node (it's a part of the Arria 10 SoC, not the mercury) Sounds good, but maybe not remove but move? > - add atsha204a node > - add copyright header These can come together. Copyright by itself is not a meaningful change, but usually addon to actual copyrighted work. > - style fixes (phy reg, memory) Sure. > What do you think? > > Please see my other comments below. > > On Mon, May 30, 2022 at 8:55 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> 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? Yes, ARM: dts: socfpga: >> 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@semihalf.com> >> >> 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. This sounds reasonable (unless such bus is still used internally in the SoM). Best regards, Krzysztof
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 + */ #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>; }; }; }; -&gpio0 { - status = "okay"; -}; - -&gpio1 { - status = "okay"; -}; - -&gpio2 { - status = "okay"; -}; - &i2c1 { - status = "okay"; + atsha204a: atsha204a@64 { + compatible = "atmel,atsha204a"; + reg = <0x64>; + }; + isl12022: isl12022@6f { - status = "okay"; compatible = "isil,isl12022"; reg = <0x6f>; }; }; -/* Following mappings are taken from arria10 socdk dts */ &mmc { - status = "okay"; cap-sd-highspeed; broken-cd; bus-width = <4>; @@ -101,12 +70,3 @@ &mmc { &osc1 { clock-frequency = <33330000>; }; - -&uart1 { - status = "okay"; -}; - -&usb0 { - status = "okay"; - dr_mode = "host"; -};
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. Signed-off-by: Paweł Anikiel <pan@semihalf.com> --- 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%)