Mvneta: help with a voltage patch

Hello,
It turns out that my LinkStation LS421DE has a bug on the ethernet port wich makes the connection to behave randomly unstable. After hours of testing I catched the problem.

It is a discrepancy between the ethernet voltage and the PHY voltage:

  • The ethernet at the Marvell SoC is configured by default to operate at 3.3 volts
  • The PHY only supports 1.8 volts.

The SoC can be configured to force the RGMII pins use the 1.8volts correct voltage by changing the value of a couple of bytes in the internal registers. Looks like the manufacturer wasn't aware of this, because it isn't configured by the bootloader nor the stock firmware.

In Openwrt of course the problem is still present. So I looked for a solution. There are several options to fix it:

  1. Setting the registers from userspace: easy, it can be done with devmem, but Openwrt comes with /dev/mem disabled. So it needs compiling an Openwrt image with /dev/mem enabled. Not easy for all users.
  2. Patching the bootloader: I already did it patching the stuff where the RAM is initialized, it apparently works fine. But again this is not an easy option, to flash a new bootloader which can cause a potential brick.
  3. Patching the kernel: this seems to be the best option, it won't require user intervention once Openwrt is installed. I also made a patch, see below.

This is the mvneta driver, where I made the patch:
https://elixir.bootlin.com/linux/v5.4.41/source/drivers/net/ethernet/marvell/mvneta.c

I noticed this is not the only one manufacturer which doesn't set the voltage correctly. Therefore other devices may benefit the patch.

However I'm not sure if patching mvneta is the best idea for fixing this issue. From my point of view it is ok, because it unconditionally will establish the correct voltage without requiring any manual intervention, and saving headaches because the bug isn't easy to detect.

--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c
@@ -459,6 +459,8 @@
 	bool neta_armada3700;
 	u16 rx_offset_correction;
 	const struct mbus_dram_target_info *dram_target_info;
+
+	phys_addr_t regaddr;
 };
 
 /* The mvneta_tx_desc and mvneta_rx_desc structures describe the
@@ -3653,6 +3655,63 @@
 	.mac_link_up = mvneta_mac_link_up,
 };
 
+#define MARVELL_PHY_ID_88E1318S	0x01410e90
+#define MARVELL_PHY_ID_88E1518	0x01410dd1
+#define VDDO_PADS_VOLTAGE_OFFS 	0x184E0
+
+/* Armada 370/XP RGMII pins voltage is 3v3 as default. It must be set
+ * to 1v8 with Marvell 88E1318S/88E1518 PHYs since they only support
+ * this voltage. The connection will be randomly unstable otherwise.
+ */
+static void mvneta_fix_voltage(struct mvneta_port *pp)
+{
+	void __iomem *vddo_addr;
+	bool is_port0;
+	u32 reg, phyid;
+
+	/* 0x70000 is the port0, 0x74000 is the port1 */
+	is_port0 = !(pp->regaddr & GENMASK(14, 12));
+
+	/* Get the PHY ID */
+	phyid = pp->dev->phydev->phy_id;
+
+	/* base address: 0xf1000000 or 0xd0000000 */
+	vddo_addr = ioremap((pp->regaddr & GENMASK(31,24)) +
+			    VDDO_PADS_VOLTAGE_OFFS, 4);
+
+	 /* Set the RGMII pins voltage at the SoC to 1v8 */
+	if (phyid == MARVELL_PHY_ID_88E1318S ||
+	    phyid == MARVELL_PHY_ID_88E1518) {
+		if (of_machine_is_compatible("marvell,armada370")){
+			if (is_port0) {
+				reg = readl(vddo_addr);
+				/* MPP pins 5 to 16 */
+				reg &= ~GENMASK(3,2);
+			} else {
+				reg = readl(vddo_addr);
+				/* MPP pins 17 to 31 */
+				reg &= ~GENMASK(5,4);
+			}
+		}
+		else if (of_machine_is_compatible("marvell,armadaxp")) {
+			if (is_port0) {
+				reg = readl(vddo_addr);
+				/* MPP pins 0 to 11 */
+				reg &= ~GENMASK(1,0);
+			} else {
+				reg = readl(vddo_addr);
+				/* MPP pins 12 to 23 */
+				reg &= ~GENMASK(3,2);
+			}
+		}
+		else
+			return;
+
+		writel(reg, vddo_addr);
+		netdev_info(pp->dev, "VDDO voltage asserted: 1v8\n");
+	}
+}
+
 static int mvneta_mdio_probe(struct mvneta_port *pp)
 {
 	struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
@@ -3661,6 +3720,9 @@
 	if (err)
 		netdev_err(pp->dev, "could not attach PHY: %d\n", err);
 
+	if (of_machine_is_compatible("marvell,armada-370-xp"))
+		mvneta_fix_voltage(pp);
+
 	phylink_ethtool_get_wol(pp->phylink, &wol);
 	device_set_wakeup_capable(&pp->dev->dev, !!wol.supported);
 
@@ -4553,6 +4615,7 @@
 	pp->comphy = comphy;
 	pp->phy_interface = phy_mode;
 	pp->dn = dn;
+	pp->regaddr = pdev->resource->start;
 
 	pp->rxq_def = rxq_def;
 	pp->indir[0] = rxq_def;

Another option could be to use device tree adding a property to the mvneta driver. But again I'm touching this driver. Or maybe the MVEBU pinctrl driver... I don't see how to articulate both drivers with the device tree to achieve the target.

Is it good this patch?, would be accepted by Openwrt or upstream?

Regards
Daniel

1 Like

The best way to approach this would be to add a device tree binding for this case that would set the correct voltage

Great work on this!

Could you provide detail on how you set this via /dev/mem?

I'm working with some folks who are experiencing the same issue from within the debian installer and would like to add a userspace script to address this at boot until your fix can be included upstream.

Hi 1000001101000, in my LinkStation LS421DE, it is as simple as executing this command:

devmem 0xd00184e0 32 0xa8a

But it depends on your specific device. Some Marvell Armadas remap their registers to 0xf1000000, in this case the command should be:

devmem 0xf10184e0 32 0xa8a

It also depends on where the PHY is connected, port1 or port2. For setting to 1.8V the pins, we need to clear the some particular bits associated with the port. as you can see in my previous patch. These bits also depends on the SoC, Armada 370 or XP.

This devmem utility is the one provided by busybox. Probably it won't be available in your debian system. You may prefer to use devmem2, therefore you'll need to adapt a bit the command.

Regards

Thanks!

I get segfaults when trying both, I assume this means I need to disable some kernel protections for it to work. I'll look into that at some point.

Before you responded I added some printk's to your patch to try to derive the address/value. It looks like there must be some sort of offset/remap that causes a slightly different value.

[   18.653752] phytest: Port1 Address E10B84E0 Value  AAA
[   18.658990] phytest: Port1 Address E10B84E0 Value  A8A

I've run the vanilla Debian kernel on 10+ LS400 devices over the past few years and have never encountered this issue but have now heard from ~5 other people who have had this problem. A few days ago I built a kernel package with the patch applied and had one of them test but they still had the issue:
https://github.com/1000001101000/Debian_on_Buffalo/blob/master/Tools/kernel_tools/armhf/linux-image-4.19.98_4.19.98-2_armhf.deb

Are you aware of any other prerequisites beyond applying the patch that are required for this to work?

Hi, the patch was fully tested under Linux v5.4.41, where I never got any segfault. AFAIK reading the SoC documentation there is no prerequisites to set these voltages.

You can also test the fix directly on the uBoot command line:

mw.l 0xd00184e0 0xa8a

If it works ok from uBoot, then it should also work with devmem, or by patching the kernel.

We've flashed your patched uboot to a few devices and are doing some testing now. So far things are looking good and I'm looking at ways to automate flashing it for users without a serial console/etc.

I tried it out on an LS441 and unsurprisingly it didn't work (the stock uboot images are different after all).

Would you be willing to provide the patch you created for uboot? I'm hoping to use it to create a working image for the LS441 (several people are seeing the same problem with that device).

Thanks again for all your help,

I've created a github repo with some utilities to modify the bootloader. Not really a patch but it should make the job.

With these tools you can split the patched header of the bootloader and insert it in the LS441 uboot.

You may also try to create an UART bootloader, and see if it works before flashing the new bootloader, to prevent a brick.

I don't provide the sourcecode because the tools were compiled under a barebox SDK. And the patched header were compiled under a Synology uboot SDK. Not clean enough to make a decent repo with them.

Regards

Sounds reasonable.

I made a first attempt on an ls410 which was previously working with your uboot image from the ls421de page but it didn't work. Can you see where I went wrong?

On my PC:
./kwbimage -x -i u-boot-ls400.buffalo.updated -o .
mv payload payload_ls400_orig
mv binary.0 header_ls400_orig
./kwbimage -x -i u-boot.buffalo-1.34_voodoo -o .
mv binary.0 header_voodoo
mv payload payload_voodoo
./mvebuimg -v 1 create -b header_voodoo -o ls400_test.bin spi payload_ls400_orig