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@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?
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@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.
This sounds reasonable (unless such bus is still used internally in the
SoM).
Best regards,
Krzysztof
