Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <fde74400-34aa-df80-5af5-cb4ee89c8e6f@linaro.org>
Date:   Tue, 10 May 2022 13:46:38 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.1
Subject: Re: [PATCH 2/2] arm64: dts: marvell: add support for Methode eDPU
Content-Language: en-US
To:     Robert Marko <robert.marko@sartura.hr>
Cc:     Rob Herring <robh+dt@kernel.org>,
        krzysztof.kozlowski+dt@linaro.org, Andrew Lunn <andrew@lunn.ch>,
        gregory.clement@bootlin.com, sebastian.hesselbarth@gmail.com,
        shawnguo@kernel.org, Linus Walleij <linus.walleij@linaro.org>,
        kostap@marvell.com, devicetree <devicetree@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>,
        =?UTF-8?Q?Pali_Roh=c3=a1r?= <pali@kernel.org>,
        =?UTF-8?Q?Marek_Beh=c3=ban?= <marek.behun@nic.cz>
References: <20220509110028.144226-1-robert.marko@sartura.hr>
 <20220509110028.144226-2-robert.marko@sartura.hr>
 <8e22cbf7-eee1-0ec7-10f9-3839ec80dfbf@linaro.org>
 <CA+HBbNE1w5w6c8MwMuSwCFzjnyKOQ7Y0MV4bPijJW3rekWLo4w@mail.gmail.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <CA+HBbNE1w5w6c8MwMuSwCFzjnyKOQ7Y0MV4bPijJW3rekWLo4w@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 10/05/2022 13:41, Robert Marko wrote:
> On Tue, May 10, 2022 at 12:20 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 09/05/2022 13:00, Robert Marko wrote:
>>> Methode eDPU is an Armada 3720 powered board based on the Methode uDPU.
>>>
>>> They feature the same CPU, RAM, and storage as well as the form factor.
>>>
>>> However, eDPU only has one SFP slot plus a copper G.hn port.
>>>
>>> In order to reduce duplication, split the uDPU DTS into a common one.
>>>
>>> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
>>> ---
>>>  arch/arm64/boot/dts/marvell/Makefile          |   1 +
>>>  .../boot/dts/marvell/armada-3720-eDPU.dts     |  14 ++
>>>  .../boot/dts/marvell/armada-3720-uDPU.dts     | 148 +---------------
>>>  .../boot/dts/marvell/armada-3720-uDPU.dtsi    | 163 ++++++++++++++++++
>>>  4 files changed, 179 insertions(+), 147 deletions(-)
>>>  create mode 100644 arch/arm64/boot/dts/marvell/armada-3720-eDPU.dts
>>>  create mode 100644 arch/arm64/boot/dts/marvell/armada-3720-uDPU.dtsi
>>>
>>> diff --git a/arch/arm64/boot/dts/marvell/Makefile b/arch/arm64/boot/dts/marvell/Makefile
>>> index 1c794cdcb8e6..104d7d7e8215 100644
>>> --- a/arch/arm64/boot/dts/marvell/Makefile
>>> +++ b/arch/arm64/boot/dts/marvell/Makefile
>>> @@ -1,6 +1,7 @@
>>>  # SPDX-License-Identifier: GPL-2.0
>>>  # Mvebu SoC Family
>>>  dtb-$(CONFIG_ARCH_MVEBU) += armada-3720-db.dtb
>>> +dtb-$(CONFIG_ARCH_MVEBU) += armada-3720-eDPU.dtb
>>>  dtb-$(CONFIG_ARCH_MVEBU) += armada-3720-espressobin.dtb
>>>  dtb-$(CONFIG_ARCH_MVEBU) += armada-3720-espressobin-emmc.dtb
>>>  dtb-$(CONFIG_ARCH_MVEBU) += armada-3720-espressobin-ultra.dtb
>>> diff --git a/arch/arm64/boot/dts/marvell/armada-3720-eDPU.dts b/arch/arm64/boot/dts/marvell/armada-3720-eDPU.dts
>>> new file mode 100644
>>> index 000000000000..6b573a6854cc
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/marvell/armada-3720-eDPU.dts
>>> @@ -0,0 +1,14 @@
>>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
>>> +
>>> +/dts-v1/;
>>> +
>>> +#include "armada-3720-uDPU.dtsi"
>>> +
>>> +/ {
>>> +     model = "Methode eDPU Board";
>>> +     compatible = "methode,edpu", "marvell,armada3720";
>>
>> You need also bindings for the board compatible. Someone should convert
>> the Documentation/devicetree/bindings/arm/marvell/armada-37xx.txt to YAML.
> 
> Ok, I can convert the SoC compatibles at least for now.
> Any advice you can give me on how the handle the Espressobin boards
> having multiple board-specific compatibles?
> For example, Espressobin V7 has:
> "globalscale,espressobin-v7", "globalscale,espressobin"
> 

Documentation/devicetree/bindings/arm/fsl.yaml

>>
>>> +};
>>> +> +  sfp_eth1: sfp-eth1 {
>>
>> Generic node names, please.
> 
> Can you give me an example of what would be appropriate here because the SFP
> bindings example utilizes the same naming scheme as used here?

"sfp" if you have only one sfp node.

> 
>>
>>> +             compatible = "sff,sfp";
>>> +             i2c-bus = <&i2c1>;
>>> +             los-gpio = <&gpiosb 7 GPIO_ACTIVE_HIGH>;
>>> +             mod-def0-gpio = <&gpiosb 8 GPIO_ACTIVE_LOW>;
>>> +             tx-disable-gpio = <&gpiosb 9 GPIO_ACTIVE_HIGH>;
>>> +             tx-fault-gpio = <&gpiosb 10 GPIO_ACTIVE_HIGH>;
>>> +             maximum-power-milliwatt = <3000>;
>>> +     };
>>> +};
>>> +
>>> +&sdhci0 {
>>> +     status = "okay";
>>> +     bus-width = <8>;
>>> +     mmc-ddr-1_8v;
>>> +     mmc-hs400-1_8v;
>>> +     marvell,pad-type = "fixed-1-8v";
>>> +     non-removable;
>>> +     no-sd;
>>> +     no-sdio;
>>> +};
>>> +
>>> +&spi0 {
>>> +     status = "okay";
>>> +     pinctrl-names = "default";
>>> +     pinctrl-0 = <&spi_quad_pins>;
>>> +
>>> +     spi-flash@0 {
>>
>> Run dtbs_check and fix the errors.
> 
> Ok, will split the DTSI and eDPU commits and fixup nodes in between.
>>
>>> +             compatible = "jedec,spi-nor";
>>> +             reg = <0>;
>>> +             spi-max-frequency = <54000000>;
>>> +
>>> +             partitions {
>>> +                     compatible = "fixed-partitions";
>>> +                     #address-cells = <1>;
>>> +                     #size-cells = <1>;
>>> +                     /* only bootloader is located on the SPI */
>>> +                     partition@0 {
>>> +                             label = "firmware";
>>> +                             reg = <0x0 0x180000>;
>>> +                     };
>>> +
>>> +                     partition@180000 {
>>> +                             label = "u-boot-env";
>>> +                             reg = <0x180000 0x10000>;
>>> +                     };
>>> +             };
>>> +     };
>>> +};
>>> +
>>> +&pinctrl_nb {
>>> +     i2c2_recovery_pins: i2c2-recovery-pins {
>>> +             groups = "i2c2";
>>> +             function = "gpio";
>>> +     };
>>> +};
>>> +
>>> +&i2c1 {
>>> +     status = "okay";
>>> +     pinctrl-names = "default", "recovery";
>>> +     pinctrl-0 = <&i2c2_pins>;
>>> +     pinctrl-1 = <&i2c2_recovery_pins>;
>>> +     /delete-property/mrvl,i2c-fast-mode;
>>> +     scl-gpios = <&gpionb 2 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
>>> +     sda-gpios = <&gpionb 3 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
>>> +
>>> +     nct375@48 {
>>
>> Generic node names, representing class of a device.
> Ok, will rename in v2.
>>
>>> +             status = "okay";
>>
>> OK status is by default, why do you need it? Also, it goes as last property.
> 
> It's not needed, I have not changed any nodes, they are just
> copy/paste during the DTS split.
> Will drop it in v2.
> 

Hm, but the node names were different in original DTS, so this is not a
simple split. In such case better to correct coding styles in one patch
(node names, status etc) and then perform the split. The split should
create the same output DTB, which is not the case here.

Best regards,
Krzysztof