linux-kernel - Re: [PATCH 2/2] arm64: dts: marvell: add support for Methode eDPU

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CA+HBbNE1w5w6c8MwMuSwCFzjnyKOQ7Y0MV4bPijJW3rekWLo4w@mail.gmail.com>
Date:   Tue, 10 May 2022 13:41:22 +0200
From:   Robert Marko <robert.marko@...tura.hr>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc:     Rob Herring <robh+dt@...nel.org>,
        krzysztof.kozlowski+dt@...aro.org, Andrew Lunn <andrew@...n.ch>,
        gregory.clement@...tlin.com, sebastian.hesselbarth@...il.com,
        shawnguo@...nel.org, Linus Walleij <linus.walleij@...aro.org>,
        kostap@...vell.com, devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Pali Rohár <pali@...nel.org>,
        Marek Behún <marek.behun@....cz>
Subject: Re: [PATCH 2/2] arm64: dts: marvell: add support for Methode eDPU

On Tue, May 10, 2022 at 12:20 PM Krzysztof Kozlowski
<krzysztof.kozlowski@...aro.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@...tura.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"

>
> > +};
> > +> +  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?

>
> > +             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@...000 {
> > +                             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.

Regards,
Robert
>
> > +             compatible = "ti,tmp75c";
> > +             reg = <0x48>;
> > +     };
> > +
> > +     nct375@49 {
> > +             status = "okay";
> > +             compatible = "ti,tmp75c";
> > +             reg = <0x49>;
> > +     };
> > +};
> > +
> > +&eth0 {
> > +     status = "okay";
> > +     managed = "in-band-status";
> > +     phys = <&comphy1 0>;
> > +};
> > +
> > +&eth1 {
> > +     phy-mode = "sgmii";
> > +     status = "okay";
> > +     managed = "in-band-status";
> > +     phys = <&comphy0 1>;
> > +     sfp = <&sfp_eth1>;
> > +};
> > +
> > +&usb3 {
> > +     status = "okay";
> > +     phys = <&usb2_utmi_otg_phy>;
> > +     phy-names = "usb2-utmi-otg-phy";
> > +};
> > +
> > +&uart0 {
> > +     status = "okay";
> > +};
>
>
> Best regards,
> Krzysztof



-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@...tura.hr
Web: www.sartura.hr