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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

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