[PATCH v3 3/3] arm: mvebu: Initial ESPRESSOBin-Ultra board support
Pali Rohár
pali at kernel.org
Fri Aug 13 12:59:23 CEST 2021
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.
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)
> 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...
> 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
More information about the U-Boot
mailing list