All of lore.kernel.org
 help / color / mirror / Atom feed
From: Joe Hershberger <joe.hershberger@gmail.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH v2 2/2] net: phy: Add PHY driver for mv88e61xx switches
Date: Tue, 26 Jan 2016 18:11:43 -0600	[thread overview]
Message-ID: <CANr=Z=afa_UXY9rF4aVwO+nKt7b4kY-mXXwT-KeZatAHirqkRA@mail.gmail.com> (raw)
In-Reply-To: <1450734319-9515-4-git-send-email-kevin.smith@elecsyscorp.com>

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;
> +               else
> +                       phydev->link = 0;
> +       }
> +
> +       if (mii_reg & PHY_REG_STATUS1_DUPLEX)
> +               phydev->duplex = DUPLEX_FULL;
> +       else
> +               phydev->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, &reg))
> +                       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, &reg))
> +               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, &reg);
> +               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 at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot

  reply	other threads:[~2016-01-27  0:11 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-12-21 21:45 [U-Boot] [PATCH 0/2] net: phy: mv88e61xx: Revise as a PHY driver Kevin Smith
2015-12-21 21:45 ` [U-Boot] [PATCH v2 " Kevin Smith
2016-01-26 16:09   ` Albert ARIBAUD
2016-01-26 16:13     ` Joe Hershberger
2016-01-26 16:56       ` Kevin Smith
2016-01-26 22:13         ` Albert ARIBAUD
2015-12-21 21:45 ` [U-Boot] [PATCH v2 1/2] net: Remove unused mv88e61xx switch driver Kevin Smith
2016-01-26 15:08   ` Joe Hershberger
2015-12-21 21:45 ` [U-Boot] [PATCH v2 2/2] net: phy: Add PHY driver for mv88e61xx switches Kevin Smith
2016-01-27  0:11   ` Joe Hershberger [this message]
2016-01-27 16:29     ` Kevin Smith
2016-01-27 17:28       ` Albert ARIBAUD
2016-01-27 20:11         ` Joe Hershberger
2016-03-31 19:33 ` [U-Boot] [PATCH v3 0/2] net: phy: mv88e61xx: Revise as a PHY driver Kevin Smith
2016-03-31 19:33   ` [U-Boot] [PATCH v3 2/2] net: phy: Add PHY driver for mv88e61xx switches Kevin Smith
2016-04-25 22:14     ` Joe Hershberger
2016-05-03 20:17     ` [U-Boot] " Joe Hershberger
2016-03-31 19:33   ` [U-Boot] [PATCH v3 1/2] net: Remove unused mv88e61xx switch driver Kevin Smith
2016-05-03 20:17     ` [U-Boot] " Joe Hershberger

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CANr=Z=afa_UXY9rF4aVwO+nKt7b4kY-mXXwT-KeZatAHirqkRA@mail.gmail.com' \
    --to=joe.hershberger@gmail.com \
    --cc=u-boot@lists.denx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.