From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-18.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A9191C4338F for ; Fri, 13 Aug 2021 10:33:48 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 30B6360EB5 for ; Fri, 13 Aug 2021 10:33:48 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 30B6360EB5 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=sartura.hr Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 87C0782D5B; Fri, 13 Aug 2021 12:33:45 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=sartura.hr Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=sartura-hr.20150623.gappssmtp.com header.i=@sartura-hr.20150623.gappssmtp.com header.b="Vyn8a6jO"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id C5AC082D91; Fri, 13 Aug 2021 12:33:42 +0200 (CEST) Received: from mail-io1-xd31.google.com (mail-io1-xd31.google.com [IPv6:2607:f8b0:4864:20::d31]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id DB0578201E for ; Fri, 13 Aug 2021 12:33:37 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=sartura.hr Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=luka.kovacic@sartura.hr Received: by mail-io1-xd31.google.com with SMTP id d22so12571286ioy.11 for ; Fri, 13 Aug 2021 03:33:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sartura-hr.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=0e9VmKhvsAG0AxJ9zKNMuNwDwt9Maiyvpzt7TK7WLhU=; b=Vyn8a6jOANSGUeR/r9d/WbpShKed70zPfvYpWF+G3xotFgTz5Ff8Lngj2Iv3ie21sj OxC+k5vaIKz5KWDbklSZqcAqiqdVc1+860Wk/o5qevdnq4eaM4bLKkIUHNxzh8Rh7sfV ez35PhkmsR8IUBUDcxrnyUfqyBBy08Fg28FOex0HjsFMnUYpgUIRqA53KcW1AB+4awVO CKr5+17M6CB1YIyAcVQqBNRQHttd64ntRtMDrbvohb4cJ/ZQ7bo8cKxjrP8+16IpUfKz izYHVJoTO9rMZzW7R4ydSvajVCHWzrHMy3OOg356/SZIK1HHqdUQqUTUcyehrzca1GkD 38Ow== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=0e9VmKhvsAG0AxJ9zKNMuNwDwt9Maiyvpzt7TK7WLhU=; b=SdVazQ0jirh+HmKtQPX6U7q6cmHo/BhqouIUujQ4AVQIGiDkC1ORq2tIItGFmI+jaU JmxAZ3ifpHaRjI6xt5ngLK0V0qx3XmdBTlDgbByFj82xZSbccV7jBq5Wf258+mhtjqPq JDVnZXHLnGa0dqcWnDS+s/L1pJYLL2IUuiZkbWYPohMgkxhnTS1wNkpX2lT7Ehia63l1 SX5UMzeXzUZPUkMSa7OP+RR4Q2+jw8OMbj6NRd8n14D3Lihhw7besFZj0lTacZWXeSJQ dBWgy7CnxHYRLiebSLqaZUKLJQMGd9z/oynJjQM4nvZp64eypw7jvrCxB4ujhm7F2+ir 0e6w== X-Gm-Message-State: AOAM532Lq+VamksRgatsv+eOgPtJQ9ZTq1CiKraaexUCmBa6TLBIfqMI UUslqrUs88QwmE0eYBBWVypqrrLJ56Q9n4DudL9nWA== X-Google-Smtp-Source: ABdhPJxCtwXGT9eMA15077nt760NF0gqxmmOb4bAMVBehqfyWecy5dMZlAOUbek7xmG3rMRxBSt6plnpKu/R8uTkKks= X-Received: by 2002:a05:6602:26d2:: with SMTP id g18mr1480441ioo.194.1628850816646; Fri, 13 Aug 2021 03:33:36 -0700 (PDT) MIME-Version: 1.0 References: <20210812233938.11633-1-luka.kovacic@sartura.hr> <20210812233938.11633-4-luka.kovacic@sartura.hr> <20210813092720.uty63b4ohgxnl3yw@pali> <20210813102220.wh5hzitaygjjjgmt@pali> In-Reply-To: <20210813102220.wh5hzitaygjjjgmt@pali> From: Luka Kovacic Date: Fri, 13 Aug 2021 12:33:25 +0200 Message-ID: Subject: Re: [PATCH v3 3/3] arm: mvebu: Initial ESPRESSOBin-Ultra board support To: =?UTF-8?Q?Pali_Roh=C3=A1r?= Cc: u-boot@lists.denx.de, Robert Marko , Luka Perkov , Marek Behun , Stefan Roese , sjg@chromium.org, patrick.delaunay@foss.st.com, xypron.glpk@gmx.de Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean On Fri, Aug 13, 2021 at 12:22 PM Pali Roh=C3=A1r wrote: > > On Friday 13 August 2021 12:03:57 Luka Kovacic wrote: > > Hello Pali, > > > > On Fri, Aug 13, 2021 at 11:27 AM Pali Roh=C3=A1r wrot= e: > > > > > > On Friday 13 August 2021 01:39:38 Luka Kovacic wrote: > > > > Add initial support for the ESPRESSOBin-Ultra board from Globalscal= e > > > > Technologies, Inc. > > > > > > > > The board is based on the 64-bit dual-core Marvell Armada 3720 SoC. > > > > Peripherals: > > > > - 5 Gigabit Ethernet ports (WAN has PoE, up to 30W, Topaz 6341 swi= tch) > > > > - RTC clock (PCF8563) > > > > - USB 3.0 port > > > > - USB 2.0 port > > > > - 4x LED > > > > - UART over Micro-USB > > > > - M.2 slot (2280) > > > > - Mini PCI-E slot > > > > > > > > Additionally, automatic import of the Marvell hw_info parameters is > > > > enabled via the recently added mac command for A37XX platforms. > > > > The parameters stored in Marvell hw_info are usually the board seri= al > > > > number and MAC addresses. > > > > > > > > Signed-off-by: Luka Kovacic > > > > Cc: Luka Perkov > > > > Cc: Robert Marko > > > > --- > > > > arch/arm/dts/Makefile | 1 + > > > > .../arm/dts/armada-3720-espressobin-ultra.dts | 114 ++++++++++ > > > > arch/arm/dts/armada-3720-espressobin.dts | 199 +-------------= --- > > > > arch/arm/dts/armada-3720-espressobin.dtsi | 210 ++++++++++++++= ++++ > > > > board/Marvell/mvebu_armada-37xx/MAINTAINERS | 8 + > > > > board/Marvell/mvebu_armada-37xx/board.c | 92 +++++++- > > > > .../mvebu_espressobin-ultra-88f3720_defconfig | 93 ++++++++ > > > > 7 files changed, 514 insertions(+), 203 deletions(-) > > > > create mode 100644 arch/arm/dts/armada-3720-espressobin-ultra.dts > > > > create mode 100644 arch/arm/dts/armada-3720-espressobin.dtsi > > > > create mode 100644 configs/mvebu_espressobin-ultra-88f3720_defconf= ig > > > ... > > > > diff --git a/board/Marvell/mvebu_armada-37xx/board.c b/board/Marvel= l/mvebu_armada-37xx/board.c > > > > index 2de9c2ac17..21c1eb7b22 100644 > > > > --- a/board/Marvell/mvebu_armada-37xx/board.c > > > > +++ b/board/Marvell/mvebu_armada-37xx/board.c > > > > @@ -11,6 +11,7 @@ > > > > #include > > > > #include > > > > #include > > > > +#include > > > > #include > > > > #include > > > > #include > > > > @@ -55,6 +56,15 @@ DECLARE_GLOBAL_DATA_PTR; > > > > #define MVEBU_G2_SMI_PHY_CMD_REG (24) > > > > #define MVEBU_G2_SMI_PHY_DATA_REG (25) > > > > > > > > +/* Marvell 88E1512 */ > > > > +#define MII_MARVELL_PHY_PAGE 22 > > > > + > > > > +#define MV88E1512_GENERAL_CTRL 20 > > > > +#define MV88E1512_MODE_SGMII 1 > > > > +#define MV88E1512_RESET_OFFS 15 > > > > + > > > > +#define ULTRA_MV88E1512_PHYADDR 0x1 > > > > + > > > > /* > > > > * Memory Controller Registers > > > > * > > > > @@ -282,12 +292,68 @@ static int mii_multi_chip_mode_write(struct m= ii_dev *bus, int dev_smi_addr, > > > > return 0; > > > > } > > > > > > > > -/* Bring-up board-specific network stuff */ > > > > -int board_network_enable(struct mii_dev *bus) > > > > +void force_phy_88e1512_sgmii_to_copper(u16 devaddr) > > > > { > > > > - if (!of_machine_is_compatible("globalscale,espressobin")) > > > > - return 0; > > > > + const char *name; > > > > + u16 reg; > > > > + > > > > + name =3D miiphy_get_current_dev(); > > > > + if (name) { > > > > > > It is possible that phy is not available? As you are calling code bel= ow > > > only in case name is not-NULL. > > > > Well, according to common/miiphyutil.c, it could also happen that it's = NULL. > > I see. But if it happens, is not it fatal error for this board? If name > is NULL then you cannot correctly configure board correctly, right? > > > > > > > > + /* SGMII-to-Copper mode initialization */ > > > > + > > > > + /* Select page 18 */ > > > > + miiphy_write(name, devaddr, MII_MARVELL_PHY_PAGE, 0x1= 2); > > > > + /* In reg 20, write MODE[2:0] =3D 0x1 (SGMII to Coppe= r) */ > > > > + miiphy_read(name, devaddr, MV88E1512_GENERAL_CTRL, &r= eg); > > > > + reg &=3D ~0x7; > > > > + reg |=3D MV88E1512_MODE_SGMII; > > > > + miiphy_write(name, devaddr, MV88E1512_GENERAL_CTRL, r= eg); > > > > + /* PHY reset is necessary after changing MODE[2:0] */ > > > > + miiphy_read(name, devaddr, MV88E1512_GENERAL_CTRL, &r= eg); > > > > + reg |=3D 1 << MV88E1512_RESET_OFFS; > > > > + miiphy_write(name, devaddr, MV88E1512_GENERAL_CTRL, r= eg); > > > > + /* Reset page selection */ > > > > + miiphy_write(name, devaddr, MII_MARVELL_PHY_PAGE, 0); > > > > + udelay(100); > > > > + } > > > > +} > > > > + > > > > +int board_network_enable_espressobin_ultra(struct mii_dev *bus) > > > > +{ > > > > + int i; > > > > + /* Setup 88E1512 SGMII-to-Copper mode */ > > > > + force_phy_88e1512_sgmii_to_copper(ULTRA_MV88E1512_PHYADDR); > > > > > > > > + /* > > > > + * FIXME: remove this code once Topaz driver gets available > > > > + * A3720 ESPRESSObin Ultra Board Only > > > > + * Configure Topaz switch (88E6341) > > > > + * Set port 1,2,3,4,5 to forwarding Mode (through Switch Port= registers) > > > > + */ > > > > + for (i =3D 0; i <=3D 5; i++) { > > > > + mii_multi_chip_mode_write(bus, 3, MVEBU_PORT_CTRL_SMI= _ADDR(i), > > > > + MVEBU_SW_PORT_CTRL_REG, > > > > + i =3D=3D 5 ? 0x7c : 0x7f); > > > > > > Why port 5 has different settings? > > > > It's to disable forwarding between the WAN port and LAN ports (I've > > tested this). > > I'm aware of this thread: > > https://github.com/MarvellEmbeddedProcessors/u-boot-marvell/issues/18 > > Ok. But your change seems to not be correct. I'm looking at > documentation and when low 2 bits in Port Control Register are zero then > specified port is completely disabled. > > I agree that for disabled port is obviously disabled also forwarding. > But I do not think it is what you want... Or, which behavior you want to > achieve? From above added comment (which seems to be also copy+paste) it > is not clear. > I think it would be completely fine if the WAN port is disabled in U-Boot. The current behaviour is that the LAN ports forward between each other and are all accessible from U-Boot, while the WAN port is in no way accessible. The link is up when testing the WAN port, but no traffic passes. > Look at that my change which only disables forwarding between ports. And > does not disable ports itself. It is already in newly renamed function > board_network_enable_espressobin(). That is why I do not like copy+paste > of code as it can be copied incorrectly, or differently without > describing commenting why it is differently. > > I see that board_network_enable_espressobin_ultra() and > board_network_enable_espressobin() are different. But they have some > common parts, e.g. this port policy configuration. So at least this code > path can be moved in some common function. I agree, these could be somehow unified. I will take a look. > > > > > > > > + } > > > > + > > > > + /* RGMII Delay on Port 0 (CPU port), force link to 1000Mbps *= / > > > > + mii_multi_chip_mode_write(bus, 3, MVEBU_PORT_CTRL_SMI_ADDR(0)= , > > > > + MVEBU_SW_LINK_CTRL_REG, 0xe002); > > > > + > > > > + /* Power up PHY 1, 2, 3, 4, 5 (through Global 2 registers) */ > > > > + mii_multi_chip_mode_write(bus, 3, MVEBU_SW_G2_SMI_ADDR, > > > > + MVEBU_G2_SMI_PHY_DATA_REG, 0x1140); > > > > + for (i =3D 1; i <=3D 5; i++) { > > > > + mii_multi_chip_mode_write(bus, 3, MVEBU_SW_G2_SMI_ADD= R, > > > > + MVEBU_G2_SMI_PHY_CMD_REG, 0= x9400 + > > > > + (MVEBU_PORT_CTRL_SMI_ADDR(i= ) << 5)); > > > > + } > > > > > > It looks like that by copying board_network_enable_espressobin_ultra(= ) > > > function from Marvell U-Boot instead of using code which is in mainli= ne > > > function board_network_enable() for Espressobin, you are introducing = a > > > security hole, which is in Marvell U-Boot and which was fixed in > > > mainline U-Boot for all supported Espressobin boards (see commit > > > 48f2c8a37f700859a7004dce5adb116597a45700). > > > > > > I would really suggest to not blindly copy old code from Marvell into > > > mainline U-Boot, as here we have fixed lot of issues. > > > > > > > + return 0; > > > > +} > > > > + > > > > +int board_network_enable_espressobin(struct mii_dev *bus) > > > > +{ > > > > /* > > > > * FIXME: remove this code once Topaz driver gets available > > > > * A3720 Community Board Only > > > > @@ -328,6 +394,16 @@ int board_network_enable(struct mii_dev *bus) > > > > return 0; > > > > } > > > > > > > > +/* Bring-up the board-specific networking */ > > > > +int board_network_enable(struct mii_dev *bus) > > > > +{ > > > > + if (of_machine_is_compatible("globalscale,espressobin")) > > > > + return board_network_enable_espressobin(bus); > > > > + if (of_machine_is_compatible("globalscale,espressobin-ultra")= ) > > > > + return board_network_enable_espressobin_ultra(bus); > > > > + return 0; > > > > +} > > > > + > > > > #if defined(CONFIG_OF_BOARD_SETUP) && defined(CONFIG_ENV_IS_IN_SPI= _FLASH) > > > > int ft_board_setup(void *blob, struct bd_info *bd) > > > > { > > > > @@ -336,8 +412,12 @@ int ft_board_setup(void *blob, struct bd_info = *bd) > > > > int parts_off; > > > > int part_off; > > > > > > > > - /* Fill SPI MTD partitions for Linux kernel on Espressobin */ > > > > - if (!of_machine_is_compatible("globalscale,espressobin")) > > > > + /* > > > > + * Fill SPI MTD partitions for the Linux kernel on ESPRESSOBi= n and > > > > + * ESPRESSOBin Ultra boards. > > > > + */ > > > > + if (!of_machine_is_compatible("globalscale,espressobin") && > > > > + !of_machine_is_compatible("globalscale,espressobin-ultra"= )) > > > > return 0; > > > > > > According to kernel DTS file, it looks like that Espressobin Ultra ha= s > > > different MTD partitions as other variants... Therefore Ultra needs > > > adjustments in this code. > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tr= ee/arch/arm64/boot/dts/marvell/armada-3720-espressobin-ultra.dts?h=3Dv5.13#= n78 > > > > That's true, I'll have to fix this. > > It is needed to call this function for ultra variant at all? Because as > I understand ultra variant has fixed layout as specified in kernel DTS > file and therefore it should not have any dynamic modification. > > Or is there any other issue? If the ports aren't correctly configured in U-Boot, Linux fails to configur= e the Topaz switch. In the current configuration traffic between LAN and WAN is blocked in U-Bo= ot and Linux correctly configures the Topaz switch. > > > > > > > > > > > > spi_off =3D fdt_node_offset_by_compatible(blob, -1, "jedec,sp= i-nor"); > > > > Kind regards, > > Luka Kind regards, Luka