[PATCH v3 3/3] arm: mvebu: Initial ESPRESSOBin-Ultra board support
Luka Kovacic
luka.kovacic at sartura.hr
Mon Aug 16 10:37:16 CEST 2021
Hello Pali,
On Fri, Aug 13, 2021 at 12:59 PM Pali Rohár <pali at kernel.org> wrote:
>
> On Friday 13 August 2021 12:33:25 Luka Kovacic wrote:
> > On Fri, Aug 13, 2021 at 12:22 PM Pali Rohár <pali at kernel.org> 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ár <pali at kernel.org> wrote:
> > > > >
> > > > > On Friday 13 August 2021 01:39:38 Luka Kovacic wrote:
> > > > > > Add initial support for the ESPRESSOBin-Ultra board from Globalscale
> > > > > > 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 switch)
> > > > > > - 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 serial
> > > > > > number and MAC addresses.
> > > > > >
> > > > > > Signed-off-by: Luka Kovacic <luka.kovacic at sartura.hr>
> > > > > > Cc: Luka Perkov <luka.perkov at sartura.hr>
> > > > > > Cc: Robert Marko <robert.marko at sartura.hr>
> > > > > > ---
> > > > > > 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_defconfig
> > > > > ...
> > > > > > diff --git a/board/Marvell/mvebu_armada-37xx/board.c b/board/Marvell/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 <i2c.h>
> > > > > > #include <init.h>
> > > > > > #include <mmc.h>
> > > > > > +#include <miiphy.h>
> > > > > > #include <phy.h>
> > > > > > #include <asm/global_data.h>
> > > > > > #include <asm/io.h>
> > > > > > @@ -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 mii_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 = miiphy_get_current_dev();
> > > > > > + if (name) {
> > > > >
> > > > > It is possible that phy is not available? As you are calling code below
> > > > > 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, 0x12);
> > > > > > + /* In reg 20, write MODE[2:0] = 0x1 (SGMII to Copper) */
> > > > > > + miiphy_read(name, devaddr, MV88E1512_GENERAL_CTRL, ®);
> > > > > > + reg &= ~0x7;
> > > > > > + reg |= MV88E1512_MODE_SGMII;
> > > > > > + miiphy_write(name, devaddr, MV88E1512_GENERAL_CTRL, reg);
> > > > > > + /* PHY reset is necessary after changing MODE[2:0] */
> > > > > > + miiphy_read(name, devaddr, MV88E1512_GENERAL_CTRL, ®);
> > > > > > + reg |= 1 << MV88E1512_RESET_OFFS;
> > > > > > + miiphy_write(name, devaddr, MV88E1512_GENERAL_CTRL, reg);
> > > > > > + /* 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 = 0; i <= 5; i++) {
> > > > > > + mii_multi_chip_mode_write(bus, 3, MVEBU_PORT_CTRL_SMI_ADDR(i),
> > > > > > + MVEBU_SW_PORT_CTRL_REG,
> > > > > > + i == 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.
>
> For espressobin v5/v7 all ports are enabled, but forwarding is possible
> only in directions: "cpu to any non-cpu port" and "non-cpu port to cpu
> port". So between lan ports it is disabled and also between lan an wan.
>
> User may have custom settings in Linux where want to use e.g. two ports
> as wans (e.g. use one lan labeled port as wan2) and then during u-boot
> stage it would would break his network. That is why also forwarding
> between lan labeled ports is disabled.
I see, that makes sense.
I'll find a way to implement it like this.
>
> Or user may use custom vlan settings with authentication and then
> enabled forwarding in u-boot also can break network. This industrial
> Topaz switch has lot of HW functionality, so it is ideal for such kind
> of usage.
>
> > > 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 = 1; i <= 5; i++) {
> > > > > > + mii_multi_chip_mode_write(bus, 3, MVEBU_SW_G2_SMI_ADDR,
> > > > > > + MVEBU_G2_SMI_PHY_CMD_REG, 0x9400 +
> > > > > > + (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 mainline
> > > > > 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 ESPRESSOBin 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 has
> > > > > 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/tree/arch/arm64/boot/dts/marvell/armada-3720-espressobin-ultra.dts?h=v5.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?
>
> (This was about MTD partitions)
This won't be needed, thanks for pointing it out.
>
> > If the ports aren't correctly configured in U-Boot, Linux fails to configure the
> > Topaz switch.
>
> So this is a bug in Linux. Kernel really should not depend on specific
> network configuration done by bootloader.
>
> So DTS file for Ultra variant in Linux kernel is incomplete?
>
> I remember that DTS was merged into Linux kernel, but there were some
> issues and I do not know if they were fixed later after merging...
The DTS in the Linux kernel should be fine regarding networking, I think.
There was some issue with USB 3, so the node was disabled.
I'll check if it can be resolved.
>
> > In the current configuration traffic between LAN and WAN is blocked in U-Boot
> > and Linux correctly configures the Topaz switch.
> >
> > >
> > > > >
> > > > > >
> > > > > > spi_off = fdt_node_offset_by_compatible(blob, -1, "jedec,spi-nor");
> > > >
> > > > Kind regards,
> > > > Luka
> >
> > Kind regards,
> > Luka
Kind regards,
Luka
More information about the U-Boot
mailing list