Hi Kevin,
On Mon, Dec 21, 2015 at 3:45 PM, Kevin Smith kevin.smith@elecsyscorp.com wrote:
The previous mv88e61xx driver was a driver for configuring the switch, but did not integrate with the PHY/networking system, so it could not be used as a PHY by U-boot. This is a complete rework to support this device as a PHY.
Signed-off-by: Kevin Smith kevin.smith@elecsyscorp.com Acked-by: Prafulla Wadaskar prafulla@marvell.com Cc: Joe Hershberger joe.hershberger@ni.com Cc: Stefan Roese sr@denx.de Cc: Marek Vasut marex@denx.de
drivers/net/phy/mv88e61xx.c | 664 ++++++++++++++++++++++++++++++++++++++++++++ drivers/net/phy/phy.c | 3 + include/phy.h | 1 + 3 files changed, 668 insertions(+) create mode 100644 drivers/net/phy/mv88e61xx.c
diff --git a/drivers/net/phy/mv88e61xx.c b/drivers/net/phy/mv88e61xx.c new file mode 100644 index 0000000..d24272e --- /dev/null +++ b/drivers/net/phy/mv88e61xx.c @@ -0,0 +1,664 @@ +/*
- (C) Copyright 2015
- Elecsys Corporation <www.elecsyscorp.com>
- Kevin Smith kevin.smith@elecsyscorp.com
- Original driver:
- (C) Copyright 2009
- Marvell Semiconductor <www.marvell.com>
- Prafulla Wadaskar prafulla@marvell.com
- SPDX-License-Identifier: GPL-2.0+
- */
+/*
- PHY driver for mv88e61xx ethernet switches.
- This driver configures the mv88e61xx for basic use as a PHY. The switch
- supports a VLAN configuration that determines how traffic will be routed
- between the ports. This driver uses a simple configuration that routes
- traffic from each PHY port only to the CPU port, and from the CPU port to
- any PHY port.
- The configuration determines which PHY ports to activate using the
- CONFIG_MV88E61XX_PHY_PORTS bitmask. Setting bit 0 will activate port 0, bit
- 1 activates port 1, etc. Do not set the bit for the port the CPU is
- connected to unless it is connected over a PHY interface (not MII).
- This driver was written for and tested on the mv88e6176 with an SGMII
- connection. Other configurations should be supported, but some additions or
- changes may be required.
- */
+#include <common.h> +#include <errno.h> +#include <miiphy.h> +#include <netdev.h>
+#define PHY_AUTONEGOTIATE_TIMEOUT 5000
+#define PORT_COUNT 7 +#define PORT_MASK ((1 << PORT_COUNT) - 1)
+/* Device addresses */ +#define DEVADDR_PHY(p) (p) +#define DEVADDR_PORT(p) (0x10 + (p)) +#define DEVADDR_SERDES 0x0F +#define DEVADDR_GLOBAL_1 0x1B +#define DEVADDR_GLOBAL_2 0x1C
+/* Global registers */ +#define GLOBAL1_STATUS 0x00 +#define GLOBAL1_CONTROL 0x04 +#define GLOBAL1_MONITOR_CONTROL 0x1A
+/* Global 2 registers */ +#define GLOBAL2_REG_PHY_CMD 0x18 +#define GLOBAL2_REG_PHY_DATA 0x19
+/* Port registers */ +#define PORT_REG_STATUS 0x00 +#define PORT_REG_PHYS_CONTROL 0x01 +#define PORT_REG_SWITCH_ID 0x03 +#define PORT_REG_CONTROL 0x04 +#define PORT_REG_VLAN_MAP 0x06 +#define PORT_REG_VLAN_ID 0x07
+/* Phy registers */ +#define PHY_REG_CONTROL1 0x10 +#define PHY_REG_STATUS1 0x11 +#define PHY_REG_PAGE 0x16
+/* Serdes registers */ +#define SERDES_REG_CONTROL_1 0x10
+/* Phy page numbers */ +#define PHY_PAGE_COPPER 0 +#define PHY_PAGE_SERDES 1
+#define PHY_WRITE_CMD 0x9400 +#define PHY_READ_CMD 0x9800
Please at least include a comment that these are commands written to the GLOBAL2_REG_PHY_CMD and are made selecting...
Enable Busy (start), Clause 22 frames, and write / read.
Even better would be to also use names that are a closer to the register they apply to, though that can get a bit long with these windowed interfaces. Maybe GLOBAL2_REG_PHY_WRITE_CMD and GLOBAL2_REG_PHY_READ_CMD?
Even better than that would be constants for the bitfield values that are ORed in these commands.
+/* PHY Status Register */ +#define PHY_REG_STATUS1_SPEED 0xc000 +#define PHY_REG_STATUS1_GBIT 0x8000 +#define PHY_REG_STATUS1_100 0x4000 +#define PHY_REG_STATUS1_DUPLEX 0x2000 +#define PHY_REG_STATUS1_SPDDONE 0x0800 +#define PHY_REG_STATUS1_LINK 0x0400 +#define PHY_REG_STATUS1_ENERGY 0x0010
+/* Check for required macros */ +#ifndef CONFIG_MV88E61XX_PHY_PORTS +#error Define CONFIG_MV88E61XX_PHY_PORTS to indicate which physical ports \
to activate+#endif +#ifndef CONFIG_MV88E61XX_CPU_PORT +#error Define CONFIG_MV88E61XX_CPU_PORT to the port the CPU is attached to +#endif
+enum {
SWITCH_MODEL_6176,/* Leave this last */SWITCH_MODEL_UNKNOWN+};
+static u16 switch_model_ids[] = {
[SWITCH_MODEL_6176] = 0x176,+};
+/* Wait for the current SMI PHY command to complete */ +static int mv88e61xx_smi_wait(struct mii_dev *bus) +{
int reg;u32 timeout = 100;do {reg = bus->read(bus, DEVADDR_GLOBAL_2, MDIO_DEVAD_NONE,GLOBAL2_REG_PHY_CMD);if (reg >= 0 && (reg & (1 << 15)) == 0)
Please add a constant for SMI_BUSY instead of "1 << 15".
return 0;mdelay(1);} while (--timeout);puts("SMI busy timeout\n");return -1;
-ETIMEDOUT?
+}
Generally avoid multiple consecutive blank lines unless it really helps to structure the code.
+/* Write a PHY register indirectly */
It would be good to make these "indirect" functions more clear.
+static int mv88e61xx_phy_write_bus(struct mii_dev *bus, int addr, int devad,
Maybe this should be called mv88e61xx_phy_write_external or mv88e61xx_phy_write_indirect.
int reg, u16 data)+{
struct mii_dev *phys_bus;
Why "phys_bus"? Isn't it an mdio_bus? Maybe that would be a better name.
/* Retrieve the actual MII bus device from private data */
This comment doesn't help. With a well named local var, that will be self-explanatory.
phys_bus = (struct mii_dev *)bus->priv;phys_bus->write(phys_bus, DEVADDR_GLOBAL_2, devad,GLOBAL2_REG_PHY_DATA, data);phys_bus->write(phys_bus, DEVADDR_GLOBAL_2, devad,GLOBAL2_REG_PHY_CMD, (PHY_WRITE_CMD | (addr << 5) | reg));
Reg and addr need to be masked to only include the valid bits (i.e. make sure reg < 32). This is done cleanly by building up the value with the functions in include/bitfield.h
return mv88e61xx_smi_wait(phys_bus);+}
Same comments for read...
+/* Read a PHY register indirectly */ +static int mv88e61xx_phy_read_bus(struct mii_dev *bus, int addr, int devad,
int reg)+{
struct mii_dev *phys_bus;int res;/* Retrieve the actual MII bus device from private data */phys_bus = (struct mii_dev *)bus->priv;phys_bus->write(phys_bus, DEVADDR_GLOBAL_2, devad, GLOBAL2_REG_PHY_CMD,(PHY_READ_CMD | (addr << 5) | reg));if (mv88e61xx_smi_wait(phys_bus))return -1;
Probably throughout, you should be passing error codes along instead of replacing them with -1.
res = phys_bus->read(phys_bus, DEVADDR_GLOBAL_2, devad,GLOBAL2_REG_PHY_DATA);return res;+}
+static int mv88e61xx_phy_read(struct phy_device *phydev, int phy,
int reg, u16 *val)+{
int res;res = mv88e61xx_phy_read_bus(phydev->bus, DEVADDR_PHY(phy),MDIO_DEVAD_NONE, reg);if (res < 0)return -1;*val = (u16)res;return 0;+}
+static int mv88e61xx_phy_write(struct phy_device *phydev, int phy,
int reg, u16 val)+{
return mv88e61xx_phy_write_bus(phydev->bus, DEVADDR_PHY(phy),MDIO_DEVAD_NONE, reg, val);+}
+static int mv88e61xx_switch_read(struct phy_device *phydev, u8 addr, u8 reg,
u16 *val)+{
struct mii_dev *bus;int res;bus = phydev->bus->priv;res = bus->read(bus, addr, MDIO_DEVAD_NONE, reg);if (res < 0)return -1;
-EIO? Or probably just pass whatever was returned.
*val = (u16)res;return 0;+}
+static int mv88e61xx_switch_write(struct phy_device *phydev, u8 addr, u8 reg,
u16 val)+{
struct mii_dev *bus;bus = phydev->bus->priv;
This is a bit ugly to read... bus = bus->priv. Maybe use mdio_bus for the local var like above?
return bus->write(bus, addr, MDIO_DEVAD_NONE, reg, val);+}
+static int mv88e61xx_port_read(struct phy_device *phydev, u8 port, u8 reg,
u16 *val)+{
return mv88e61xx_switch_read(phydev, DEVADDR_PORT(port), reg, val);+}
+static int mv88e61xx_port_write(struct phy_device *phydev, u8 port, u8 reg,
u16 val)+{
return mv88e61xx_switch_write(phydev, DEVADDR_PORT(port), reg, val);+}
+static int mv88e61xx_set_page(struct phy_device *phydev, u8 addr, u8 page) +{
return mv88e61xx_phy_write(phydev, addr, PHY_REG_PAGE, page);+}
+static u16 mv88e61xx_get_switch_model(struct phy_device *phydev) +{
u16 id;int i;if (mv88e61xx_port_read(phydev, 0, PORT_REG_SWITCH_ID, &id))return -1;id >>= 4;for (i = 0; i < ARRAY_SIZE(switch_model_ids); i++) {if (id == switch_model_ids[i])return i;}return SWITCH_MODEL_UNKNOWN;+}
+static int mv88e61xx_parse_status(struct phy_device *phydev) +{
unsigned int speed;unsigned int mii_reg;mii_reg = phy_read(phydev, MDIO_DEVAD_NONE, PHY_REG_STATUS1);if ((mii_reg & PHY_REG_STATUS1_LINK) &&!(mii_reg & PHY_REG_STATUS1_SPDDONE)) {int i = 0;puts("Waiting for PHY realtime link");while (!(mii_reg & PHY_REG_STATUS1_SPDDONE)) {/* Timeout reached ? */if (i > PHY_AUTONEGOTIATE_TIMEOUT) {puts(" TIMEOUT !\n");phydev->link = 0;break;}if ((i++ % 1000) == 0)putc('.');udelay(1000);mii_reg = phy_read(phydev, MDIO_DEVAD_NONE,PHY_REG_STATUS1);}puts(" done\n");udelay(500000); /* another 500 ms (results in faster booting) */} else {if (mii_reg & PHY_REG_STATUS1_LINK)phydev->link = 1;elsephydev->link = 0;}if (mii_reg & PHY_REG_STATUS1_DUPLEX)phydev->duplex = DUPLEX_FULL;elsephydev->duplex = DUPLEX_HALF;speed = mii_reg & PHY_REG_STATUS1_SPEED;switch (speed) {case PHY_REG_STATUS1_GBIT:phydev->speed = SPEED_1000;break;case PHY_REG_STATUS1_100:phydev->speed = SPEED_100;break;default:phydev->speed = SPEED_10;break;}return 0;+}
+static int mv88e61xx_switch_reset(struct phy_device *phydev) +{
int time;int res;u16 reg;u8 port;/* Disable all ports */for (port = 0; port < PORT_COUNT; port++) {if (mv88e61xx_port_read(phydev, port, PORT_REG_CONTROL, ®))return -1;reg &= ~0x3;if (mv88e61xx_port_write(phydev, port, PORT_REG_CONTROL, reg))return -1;}/* Wait 2 ms for queues to drain */udelay(2000);/* Reset switch */if (mv88e61xx_switch_read(phydev, DEVADDR_GLOBAL_1,GLOBAL1_CONTROL, ®))return -1;reg |= 0x8000;
For all bitwise operations like this you should be using the functions from include/bitfield.h
if (mv88e61xx_switch_write(phydev, DEVADDR_GLOBAL_1,GLOBAL1_CONTROL, reg))return -1;/* Wait up to 1 second for switch reset complete */for (time = 1000; time; time--) {res = mv88e61xx_switch_read(phydev, DEVADDR_GLOBAL_1,GLOBAL1_CONTROL, ®);if (res == 0 && ((reg & 0x8000) == 0))break;udelay(1000);}if (!time)return -1;
-ETIMEDOUT?
return 0;+}
+static int mv88e61xx_serdes_init(struct phy_device *phydev) +{
u16 val;/* Serdes registers are on page 1 */if (mv88e61xx_set_page(phydev, DEVADDR_SERDES, 1))return -1;/* Powerup and reset. Disable auto-negotiation, as two MACs (CPU and* switch) cannot negotiate with each other. */
Multi-line comment format: /* * */
if (mv88e61xx_phy_read(phydev, DEVADDR_SERDES, MII_BMCR, &val))return -1;val &= ~(BMCR_PDOWN | BMCR_ANENABLE);val |= BMCR_RESET;if (mv88e61xx_phy_write(phydev, DEVADDR_SERDES, MII_BMCR, val))return -1;/* 2 MACs cannot auto-negotiate, so we must force the link good */if (mv88e61xx_phy_read(phydev, DEVADDR_SERDES, SERDES_REG_CONTROL_1,&val))return -1;val |= 0x0400;if (mv88e61xx_phy_write(phydev, DEVADDR_SERDES, SERDES_REG_CONTROL_1,val))return -1;return 0;+}
+static int mv88e61xx_port_enable(struct phy_device *phydev, u8 port) +{
u16 val;if (mv88e61xx_port_read(phydev, port, PORT_REG_CONTROL, &val))return -1;val |= 0x03;if (mv88e61xx_port_write(phydev, port, PORT_REG_CONTROL, val))return -1;return 0;+}
+static int mv88e61xx_port_set_vlan(struct phy_device *phydev, u8 port,
u8 mask)+{
u16 val;/* Set VID to port number plus one */if (mv88e61xx_port_read(phydev, port, PORT_REG_VLAN_ID, &val))return -1;val &= ~((1 << 12) - 1);val |= port + 1;if (mv88e61xx_port_write(phydev, port, PORT_REG_VLAN_ID, val))return -1;/* Set VID mask */if (mv88e61xx_port_read(phydev, port, PORT_REG_VLAN_MAP, &val))return -1;val &= ~PORT_MASK;val |= (mask & PORT_MASK);if (mv88e61xx_port_write(phydev, port, PORT_REG_VLAN_MAP, val))return -1;return 0;+}
+static int mv88e61xx_set_cpu_port(struct phy_device *phydev) +{
u16 val;/* Set CPUDest */if (mv88e61xx_switch_read(phydev, DEVADDR_GLOBAL_1,GLOBAL1_MONITOR_CONTROL, &val))return -1;val &= ~(0xf << 4);val |= (CONFIG_MV88E61XX_CPU_PORT << 4);if (mv88e61xx_switch_write(phydev, DEVADDR_GLOBAL_1,GLOBAL1_MONITOR_CONTROL, val))return -1;/* Enable CPU port */if (mv88e61xx_port_enable(phydev, CONFIG_MV88E61XX_CPU_PORT))return -1;/* Allow CPU to route to any port */val = PORT_MASK & ~(1 << CONFIG_MV88E61XX_CPU_PORT);if (mv88e61xx_port_set_vlan(phydev, CONFIG_MV88E61XX_CPU_PORT, val))return -1;return 0;+}
+static int mv88e61xx_switch_init(struct phy_device *phydev) +{
static int init;if (init)return 0;if (mv88e61xx_switch_reset(phydev))return -1;if (mv88e61xx_set_cpu_port(phydev))return -1;/* Only the 6176 has the SERDES interface */if (mv88e61xx_get_switch_model(phydev) == SWITCH_MODEL_6176)if (mv88e61xx_serdes_init(phydev))return -1;init = 1;return 0;+}
+static int mv88e61xx_phy_enable(struct phy_device *phydev, u8 phy) +{
u16 val;if (mv88e61xx_phy_read(phydev, phy, MII_BMCR, &val))return -1;val &= ~(BMCR_PDOWN);if (mv88e61xx_phy_write(phydev, phy, MII_BMCR, val))return -1;return 0;+}
+static int mv88e61xx_phy_setup(struct phy_device *phydev, u8 phy) +{
u16 val;/* Enable energy-detect sensing on PHY, used to determine when a PHY* port is physically connected */if (mv88e61xx_phy_read(phydev, phy, PHY_REG_CONTROL1, &val))return -1;val |= (0x3 << 8);if (mv88e61xx_phy_write(phydev, phy, PHY_REG_CONTROL1, val))return -1;return 0;+}
+static int mv88e61xx_phy_config_port(struct phy_device *phydev, u8 phy) +{
if (mv88e61xx_port_enable(phydev, phy))return -1;if (mv88e61xx_port_set_vlan(phydev, phy,1 << CONFIG_MV88E61XX_CPU_PORT))return -1;return 0;+}
+static int mv88e61xx_probe(struct phy_device *phydev) +{
struct mii_dev *mii_dev;/* This device requires indirect reads/writes to the PHY registers* which the generic PHY code can't handle. Make a fake MII device to* handle reads/writes */mii_dev = mdio_alloc();if (!mii_dev)return -1;
-ENOMEM
/* Store the actual bus in the fake mii device */mii_dev->priv = phydev->bus;strncpy(mii_dev->name, "mv88e61xx_protocol", sizeof(mii_dev->name));mii_dev->read = mv88e61xx_phy_read_bus;mii_dev->write = mv88e61xx_phy_write_bus;/* Replace the bus with the fake device */
Fake how? This is a confusing comment to me as written.
phydev->bus = mii_dev;return 0;+}
+static int mv88e61xx_phy_config(struct phy_device *phydev) +{
int mac_addr;int i;if (mv88e61xx_switch_init(phydev))return -1;
Pass the returned value through.
mac_addr = phydev->addr;for (i = 0; i < PORT_COUNT; i++) {if ((1 << i) & CONFIG_MV88E61XX_PHY_PORTS) {phydev->addr = i;mv88e61xx_phy_enable(phydev, i);mv88e61xx_phy_setup(phydev, i);mv88e61xx_phy_config_port(phydev, i);
These all return status, but are ignored.
genphy_config_aneg(phydev);phy_reset(phydev);}}phydev->addr = mac_addr;return 0;+}
+static int mv88e61xx_phy_is_connected(struct phy_device *phydev) +{
u16 val;if (mv88e61xx_phy_read(phydev, phydev->addr, PHY_REG_STATUS1, &val))return 0;/* After reset, the energy detect signal remains high for a few seconds* regardless of whether a cable is connected. This function will* return false positives during this time. */
This is OK?
return (val & PHY_REG_STATUS1_ENERGY) == 0;+}
+static int mv88e61xx_phy_startup(struct phy_device *phydev) +{
int i;int mac_addr;int link = 0;mac_addr = phydev->addr;
This is a confusing name to use here. I assume the meaning it that it is the phy address of the port wired up to the mac on this host. However, the name overloads a common name for the L2 address of an Ethernet MAC.
Maybe use something like phy_addr_for_mac?
for (i = 0; i < PORT_COUNT; i++) {if ((1 << i) & CONFIG_MV88E61XX_PHY_PORTS) {phydev->addr = i;if (!mv88e61xx_phy_is_connected(phydev))continue;genphy_update_link(phydev);mv88e61xx_parse_status(phydev);link = (link || phydev->link);}}phydev->addr = mac_addr;phydev->link = link;if (mv88e61xx_get_switch_model(phydev) == SWITCH_MODEL_6176) {/* Configure MAC to the speed of the SGMII interface */phydev->speed = SPEED_1000;phydev->duplex = DUPLEX_FULL;}return 0;+}
+static struct phy_driver mv88e61xx_driver = {
.name = "Marvell MV88E61xx",.uid = 0x01410eb1,.mask = 0xfffffff0,.features = PHY_GBIT_FEATURES,.probe = mv88e61xx_probe,.config = mv88e61xx_phy_config,.startup = mv88e61xx_phy_startup,.shutdown = &genphy_shutdown,+};
+int phy_mv88e61xx_init(void) +{
phy_register(&mv88e61xx_driver);return 0;+}
+/* Overload weak get_phy_id definition since we need non-standard functions
- to read PHY registers */
Multi-line comment format.
+int get_phy_id(struct mii_dev *bus, int addr, int devad, u32 *phy_id) +{
struct mii_dev fake_bus;int phy_reg;fake_bus.priv = bus;phy_reg = mv88e61xx_phy_read_bus(&fake_bus, addr, devad, MII_PHYSID1);if (phy_reg < 0)return -EIO;*phy_id = phy_reg << 16;phy_reg = mv88e61xx_phy_read_bus(&fake_bus, addr, devad, MII_PHYSID2);if (phy_reg < 0)return -EIO;*phy_id |= (phy_reg & 0xffff);return 0;+} diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 51b5746..077c9f5 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -446,6 +446,9 @@ static LIST_HEAD(phy_drivers);
int phy_init(void) { +#ifdef CONFIG_MV88E61XX_SWITCH
phy_mv88e61xx_init();+#endif #ifdef CONFIG_PHY_AQUANTIA phy_aquantia_init(); #endif diff --git a/include/phy.h b/include/phy.h index 66cf61b..7cec9b8 100644 --- a/include/phy.h +++ b/include/phy.h @@ -238,6 +238,7 @@ int gen10g_startup(struct phy_device *phydev); int gen10g_shutdown(struct phy_device *phydev); int gen10g_discover_mmds(struct phy_device *phydev);
+int phy_mv88e61xx_init(void); int phy_aquantia_init(void); int phy_atheros_init(void); int phy_broadcom_init(void); -- 2.4.6 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot