plat-orion problems with mv643xx_eth.c for mv78xx0

Orion maintainers,

config MV643XX_ETH can be selected for Discovery series chips (mv78xx0), but it has some problems.

1. Register PSC1 (0x44C) bit 4 is named port_reset. It defaults to 1 "port is reset". The documentation does not explain this bit, but the example code clears the bit to "port is not in reset". I needed to do the same thing and clear this bit on each port to get ethernet working:

===================================================================

@@ -2597,6 +2597,9 @@
         if (msp->base == NULL)
                 goto out_free;
 
+       // Take the part out of reset?
+       writel(readl(msp->base + 0x44C) & (~(1 << 4)), msp->base + 0x44C);
+
         /*
          * Set up and register SMI bus.
          */

2. This is confusing, but not an error. There's a variable port_number in mv643xx_eth_platform_data that MUST be zero for each port on a Discovery (MV78xx0) series processor. The printk will say "port 0" for all 4 ports, but it must be zero so the driver calculates a valid offset. This probably deserves a comment or a change in the printk.

3. This is confusing, but not an error. File mach/mv78xx0.h defines each ethernet port's base address like GE00_PHYS_BASE at the wrong address. Then orion_ge00_init() in plat-orion/common.c adds a magic 0x2000 constant which results in the correct address. This probably deserves a comment because the header defines incorrect addresses.

4. Each processor varies in support for MII, GMII, and RGMII interfaces. Function phy_init() calls phy_attach() which is hardcoded for PHY_INTERFACE_MODE_GMII. This depends on the board and can be different for each port, so it should probably be specified in the platform data instead of hardcoded.

-joey

Hello!

The original mv643xx chips (which this driver still supports) had one gigabit ethernet unit with two or three ports, while in subsequent chips, the gigabit ethernet unit has only one port, but there can be several copies of that whole unit in the chip, which causes all ports to be reported as port #0 (being port #0 in their respective unit) in those chips. I'll admit that this can be confusing.

Since the printk reports the device name anyway, perhaps we can just remove the "port %d" part entirely?

Each mbus target has 64KiB of target register address space. That the documented registers start at offset 0x2000 in that range doesn't change that (and there are some registers in the 0x0000 .. 0x1fff range, just not any documented ones). Given this, I wouldn't call the definition of GE00_PHYS_BASE incorrect.

As to the 0x2000 offset, the mv643xx_eth driver unfortunately expects that offset to have been included in the platform device mem resource address range (it did it this way before I took it over), and since this is encoded in various device trees, we can't easily change this -- or I would have advocated for passing in the 64 KiB aligned address.

This is indeed an issue -- and on some boards, this requires disabling certain PHY drivers (so that it will fall back to the generic PHY driver) to have working networking. If this affects your board, (tested :) patches would be appreciated.

thanks,
Lennert