From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id E4985C43334 for ; Mon, 20 Jun 2022 23:38:10 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 3C6E2832DA; Tue, 21 Jun 2022 01:38:08 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=gateworks.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=gateworks-com.20210112.gappssmtp.com header.i=@gateworks-com.20210112.gappssmtp.com header.b="YPM3OXZV"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 107018344E; Tue, 21 Jun 2022 01:38:07 +0200 (CEST) Received: from mail-pj1-x102c.google.com (mail-pj1-x102c.google.com [IPv6:2607:f8b0:4864:20::102c]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 282A282142 for ; Tue, 21 Jun 2022 01:38:01 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=gateworks.com Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=tharvey@gateworks.com Received: by mail-pj1-x102c.google.com with SMTP id k12-20020a17090a404c00b001eaabc1fe5dso12076768pjg.1 for ; Mon, 20 Jun 2022 16:38:01 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gateworks-com.20210112.gappssmtp.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=lz3pqWj1DhIJ5igbeSz7ApY1eIgbYYMLOdS48K2VJ2M=; b=YPM3OXZVqdDrRfEfUfsTc1GudDy49xDAR5GyCWubQiLXJdjFso3CuqYFAa6surtXn7 wAtNTzRuTXACtHsySs9jgHAIs0/A2nKzGDnrXPHc01FEdoSBYqYkzNGRf11t4BzHIbK1 AbdHJgkWGgLtkr1uf1DCLHyzyzYIaMMn/W4sESKNmkKi+fCfLbGoXCR4WQLjHu9VvtHr UHQVU3Nd5fd9y17Crfl746ms/bLuj5kcoguKpg3mkxok6qlFOaDhJHjbTuUodpNCyOfh u+5YnOePzBkw6vl+8cI+PPuzCexd+shmglKW5l7fc/1Vw5RfHdQXYQ8NFEv7KjawzNOs Qqpg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=lz3pqWj1DhIJ5igbeSz7ApY1eIgbYYMLOdS48K2VJ2M=; b=m40ycJBOig4jh0GLL+BVLLZf4+pficCxqZJbtxe2PvM56E2yXa9oV+WaNLOUod5Uvn yj8WLjt9uiJpwEScRIeU/oDNB7vr2OrkMZ0JVO4BqBP9ryHHYQ5deIpXFHP95xF9ow/a wPjpsY5E5JNoEervUOZWOV14mT5B9Y5tXGp2yRWkwiOw57KwjI4Y3c8mSJL9jaDWxOxn hC542b8ejRY5wO6+9KM36oNMy5qVaovTCpDpFZQ7h9z7FYk2CY2CmT83wFwIB87Gq/R2 739B2ykuFeuc6+m8SjXYx/7OdFUZBWtSOqVWeKIKL5U2PuS+ZAecdf45NPERJ/aNOaje B0nA== X-Gm-Message-State: AJIora9o0xu611XmXWZt5fMQx4abB0UfJHiHN0ngrdxnbvmc4qU3tGe2 dr1QWJs7z1c1K/d4Zu1DypE4GeuOP8MmP4P+3GW9LQ== X-Google-Smtp-Source: AGRyM1tytl9Uv033NzxUNFDhk0IPfqginKW21I0OnIx12beNI6e45VO8ok1wP/v+YetNvfb/rAvkf6cXtbDu3/NIzo0= X-Received: by 2002:a17:902:c2c7:b0:16a:3132:bc53 with SMTP id c7-20020a170902c2c700b0016a3132bc53mr3116120pla.90.1655768278971; Mon, 20 Jun 2022 16:37:58 -0700 (PDT) MIME-Version: 1.0 References: <20220523182549.30242-1-tharvey@gateworks.com> <20220523182549.30242-8-tharvey@gateworks.com> <20220620115847.nskmu4ucoyqde7wi@skbuf> In-Reply-To: <20220620115847.nskmu4ucoyqde7wi@skbuf> From: Tim Harvey Date: Mon, 20 Jun 2022 16:37:45 -0700 Message-ID: Subject: Re: [PATCH v3 7/8] net: add MV88E61xx DSA driver To: Vladimir Oltean Cc: Joe Hershberger , Ramon Fried , "u-boot@lists.denx.de" , Stefano Babic , Fabio Estevam , dl-uboot-imx , =?UTF-8?B?TWFyZWsgQmVoxILImW4=?= , Chris Packham , Anatolij Gustschin Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean t On Mon, Jun 20, 2022 at 4:58 AM Vladimir Oltean wrote: > > On Mon, May 23, 2022 at 11:25:48AM -0700, Tim Harvey wrote: > > Add a DSA driver for the MV88E61xx compatible GbE Ethernet switches. > > > > Cc: Marek Beh=C4=82=C8=99n > > Cc: Vladimir Oltean > > Signed-off-by: Tim Harvey > > Reviewed-by: Vladimir Oltean > > --- > > v3: > > - Added Vladimir's rb tag > > v2: > > - rebase on v2022.07-rc1 (use ofnode_get_phy_node) > > - remove unused commented out fields from struct > > - remove unused PORT_MASK macro > > - remove phy from priv struct name > > - refactor code from original drivers/net/phy/mv88e61xx with > > suggestions from review to consolidate some functions > > into mv88e61xx_dsa_port_enable > > - remove unecessary skiping of disabling of CPU port > > - remove unecessary dev_set_parent_priv > > - remove unnecessary static init flag > > - replace debug with a dev_warn if switch device-id unsupported > > - remove unnecessary xmit/recv functions as we rely on the fact that > > only a single port is active instead of mangling packets > > This looks good, but my opinion remains that we can rename mv88e61xx to > mv88e6xxx for consistency with Linux. Users will know that the drivers > are expected to support the same hardware models (even if the compatible > list is now incomplete and does not cover an actual 61xx device). > Ok, since you're asking for additional changes requiring a v4 I'll rename it to mvee86xxx as well. > > --- > > drivers/net/Kconfig | 7 + > > drivers/net/Makefile | 1 + > > drivers/net/mv88e61xx.c | 843 ++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 851 insertions(+) > > create mode 100644 drivers/net/mv88e61xx.c > > > > diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig > > index 7fe0e00649cf..edb1a0898986 100644 > > --- a/drivers/net/Kconfig > > +++ b/drivers/net/Kconfig > > @@ -433,6 +433,13 @@ config LPC32XX_ETH > > depends on ARCH_LPC32XX > > default y > > > > +config MV88E61XX > > + bool "Marvell MV88E61xx GbE switch DSA driver" > > + depends on DM_DSA && DM_MDIO > > + help > > + This driver implements a DSA switch driver for the MV88E61xx fa= mily > > + of GbE switches using the MDIO interface > > + > > config MVGBE > > bool "Marvell Orion5x/Kirkwood network interface support" > > depends on ARCH_KIRKWOOD || ARCH_ORION5X > > diff --git a/drivers/net/Makefile b/drivers/net/Makefile > > index 69fb3bbbf7cb..36b4c279430a 100644 > > --- a/drivers/net/Makefile > > +++ b/drivers/net/Makefile > > @@ -59,6 +59,7 @@ obj-$(CONFIG_MEDIATEK_ETH) +=3D mtk_eth.o > > obj-$(CONFIG_MPC8XX_FEC) +=3D mpc8xx_fec.o > > obj-$(CONFIG_MT7620_ETH) +=3D mt7620-eth.o > > obj-$(CONFIG_MT7628_ETH) +=3D mt7628-eth.o > > +obj-$(CONFIG_MV88E61XX) +=3D mv88e61xx.o > > obj-$(CONFIG_MVGBE) +=3D mvgbe.o > > obj-$(CONFIG_MVMDIO) +=3D mvmdio.o > > obj-$(CONFIG_MVNETA) +=3D mvneta.o > > diff --git a/drivers/net/mv88e61xx.c b/drivers/net/mv88e61xx.c > > new file mode 100644 > > index 000000000000..514835bf03b9 > > --- /dev/null > > +++ b/drivers/net/mv88e61xx.c > > @@ -0,0 +1,843 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * (C) Copyright 2022 > > + * Gateworks Corporation > > + * Tim Harvey > > + * > > + * (C) Copyright 2015 > > + * Elecsys Corporation > > + * Kevin Smith > > + * > > + * Original driver: > > + * (C) Copyright 2009 > > + * Marvell Semiconductor > > + * Prafulla Wadaskar > > + */ > > + > > +/* > > + * DSA driver for mv88e61xx ethernet switches. > > + * > > + * This driver configures the mv88e61xx for basic use as a DSA switch. > > + * > > + * This driver was adapted from drivers/net/phy/mv88e61xx and tested > > + * on the mv88e6176 via an SGMII interface. > > + */ > > + > > +#include > > +#include > > +#include > > Alphabetic ordering please. > ok > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +/* Device addresses */ > > +#define DEVADDR_PHY(p) (p) > > +#define DEVADDR_SERDES 0x0F > > + > > +/* SMI indirection registers for multichip addressing mode */ > > +#define SMI_CMD_REG 0x00 > > +#define SMI_DATA_REG 0x01 > > + > > +/* Global registers */ > > +#define GLOBAL1_STATUS 0x00 > > +#define GLOBAL1_CTRL 0x04 > > +#define GLOBAL1_MON_CTRL 0x1A > > + > > +/* Global 2 registers */ > > +#define GLOBAL2_REG_PHY_CMD 0x18 > > +#define GLOBAL2_REG_PHY_DATA 0x19 > > +#define GLOBAL2_REG_SCRATCH 0x1A > > + > > +/* Port registers */ > > +#define PORT_REG_STATUS 0x00 > > +#define PORT_REG_PHYS_CTRL 0x01 > > +#define PORT_REG_SWITCH_ID 0x03 > > +#define PORT_REG_CTRL 0x04 > > +#define PORT_REG_VLAN_MAP 0x06 > > +#define PORT_REG_VLAN_ID 0x07 > > +#define PORT_REG_LED_CTRL 0x16 > > + > > +/* Phy registers */ > > +#define PHY_REG_CTRL1 0x10 > > +#define PHY_REG_STATUS1 0x11 > > +#define PHY_REG_PAGE 0x16 > > + > > +/* Serdes registers */ > > +#define SERDES_REG_CTRL_1 0x10 > > + > > +/* Phy page numbers */ > > +#define PHY_PAGE_COPPER 0 > > +#define PHY_PAGE_SERDES 1 > > + > > +/* Register fields */ > > +#define GLOBAL1_CTRL_SWRESET BIT(15) > > + > > +#define GLOBAL1_MON_CTRL_CPUDEST_SHIFT 4 > > +#define GLOBAL1_MON_CTRL_CPUDEST_WIDTH 4 > > + > > +#define PORT_REG_STATUS_SPEED_SHIFT 8 > > +#define PORT_REG_STATUS_SPEED_10 0 > > +#define PORT_REG_STATUS_SPEED_100 1 > > +#define PORT_REG_STATUS_SPEED_1000 2 > > + > > +#define PORT_REG_STATUS_CMODE_MASK 0xF > > +#define PORT_REG_STATUS_CMODE_100BASE_X 0x8 > > +#define PORT_REG_STATUS_CMODE_1000BASE_X 0x9 > > +#define PORT_REG_STATUS_CMODE_SGMII 0xa > > + > > +#define PORT_REG_PHYS_CTRL_RGMII_DELAY_RXCLK BIT(15) > > +#define PORT_REG_PHYS_CTRL_RGMII_DELAY_TXCLK BIT(14) > > +#define PORT_REG_PHYS_CTRL_PCS_AN_EN BIT(10) > > +#define PORT_REG_PHYS_CTRL_PCS_AN_RST BIT(9) > > +#define PORT_REG_PHYS_CTRL_FC_VALUE BIT(7) > > +#define PORT_REG_PHYS_CTRL_FC_FORCE BIT(6) > > +#define PORT_REG_PHYS_CTRL_LINK_VALUE BIT(5) > > +#define PORT_REG_PHYS_CTRL_LINK_FORCE BIT(4) > > +#define PORT_REG_PHYS_CTRL_DUPLEX_VALUE BIT(3) > > +#define PORT_REG_PHYS_CTRL_DUPLEX_FORCE BIT(2) > > +#define PORT_REG_PHYS_CTRL_SPD1000 BIT(1) > > +#define PORT_REG_PHYS_CTRL_SPD100 BIT(0) > > +#define PORT_REG_PHYS_CTRL_SPD_MASK (BIT(1) | BIT(0)) > > + > > +#define PORT_REG_CTRL_PSTATE_SHIFT 0 > > +#define PORT_REG_CTRL_PSTATE_WIDTH 2 > > + > > +#define PORT_REG_VLAN_ID_DEF_VID_SHIFT 0 > > +#define PORT_REG_VLAN_ID_DEF_VID_WIDTH 12 > > + > > +#define PORT_REG_VLAN_MAP_TABLE_SHIFT 0 > > +#define PORT_REG_VLAN_MAP_TABLE_WIDTH 11 > > + > > +#define SERDES_REG_CTRL_1_FORCE_LINK BIT(10) > > + > > +/* Field values */ > > +#define PORT_REG_CTRL_PSTATE_DISABLED 0 > > +#define PORT_REG_CTRL_PSTATE_FORWARD 3 > > + > > +#define PHY_REG_CTRL1_ENERGY_DET_OFF 0 > > +#define PHY_REG_CTRL1_ENERGY_DET_SENSE_PULSE 1 > > +#define PHY_REG_CTRL1_ENERGY_DET_SENSE_ONLY 2 > > +#define PHY_REG_CTRL1_ENERGY_DET_SENSE_XMIT 3 > > + > > +/* 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 > > + > > +/* > > + * Macros for building commands for indirect addressing modes. These = are valid > > + * for both the indirect multichip addressing mode and the PHY indirec= tion > > + * required for the writes to any PHY register. > > + */ > > +#define SMI_BUSY BIT(15) > > +#define SMI_CMD_CLAUSE_22 BIT(12) > > +#define SMI_CMD_CLAUSE_22_OP_READ (2 << 10) > > +#define SMI_CMD_CLAUSE_22_OP_WRITE (1 << 10) > > + > > +#define SMI_CMD_READ (SMI_BUSY | SMI_CMD_CLAUSE_22 | \ > > + SMI_CMD_CLAUSE_22_OP_READ) > > +#define SMI_CMD_WRITE (SMI_BUSY | SMI_CMD_CLAUS= E_22 | \ > > + SMI_CMD_CLAUSE_22_OP_WRITE) > > + > > +#define SMI_CMD_ADDR_SHIFT 5 > > +#define SMI_CMD_ADDR_WIDTH 5 > > +#define SMI_CMD_REG_SHIFT 0 > > +#define SMI_CMD_REG_WIDTH 5 > > + > > +/* Defines for Scratch and Misc Registers */ > > +#define SCRATCH_UPDATE BIT(15) > > +#define SCRATCH_ADDR_SHIFT 8 > > +#define SCRATCH_ADDR_WIDTH 7 > > + > > +/* Scratch registers */ > > +#define SCRATCH_ADDR_GPIO0DIR 0x62 /* GPIO[7:0] directi= on 1=3Dinput */ > > +#define SCRATCH_ADDR_GPIO1DIR 0x63 /* GPIO[14:8] direct= ion 1=3Dinput */ > > +#define SCRATCH_ADDR_GPIO0DATA 0x64 /* GPIO[7:0] data */ > > +#define SCRATCH_ADDR_GPIO1DATA 0x65 /* GPIO[14:8] data *= / > > +#define SCRATCH_ADDR_GPIO0CTRL 0x68 /* GPIO[1:0] control= */ > > +#define SCRATCH_ADDR_GPIO1CTRL 0x69 /* GPIO[3:2] control= */ > > + > > +/* ID register values for different switch models */ > > +#define PORT_SWITCH_ID_6020 0x0200 > > +#define PORT_SWITCH_ID_6070 0x0700 > > +#define PORT_SWITCH_ID_6071 0x0710 > > +#define PORT_SWITCH_ID_6096 0x0980 > > +#define PORT_SWITCH_ID_6097 0x0990 > > +#define PORT_SWITCH_ID_6172 0x1720 > > +#define PORT_SWITCH_ID_6176 0x1760 > > +#define PORT_SWITCH_ID_6220 0x2200 > > +#define PORT_SWITCH_ID_6240 0x2400 > > +#define PORT_SWITCH_ID_6250 0x2500 > > +#define PORT_SWITCH_ID_6352 0x3520 > > + > > +struct mv88e61xx_priv { > > + int smi_addr; > > + int id; > > + int port_count; /* Number of switch ports */ > > + int port_reg_base; /* Base of the switch port registers */ > > + u8 global1; /* Offset of Switch Global 1 registers */ > > + u8 global2; /* Offset of Switch Global 2 registers */ > > + u8 phy_ctrl1_en_det_shift; /* 'EDet' bit field offset */ > > + u8 phy_ctrl1_en_det_width; /* Width of 'EDet' bit field */ > > + u8 phy_ctrl1_en_det_ctrl; /* 'EDet' control value */ > > +}; > > + > > +static inline int smi_cmd(int cmd, int addr, int reg) > > +{ > > + cmd =3D bitfield_replace(cmd, SMI_CMD_ADDR_SHIFT, SMI_CMD_ADDR_WI= DTH, > > + addr); > > + cmd =3D bitfield_replace(cmd, SMI_CMD_REG_SHIFT, SMI_CMD_REG_WIDT= H, reg); > > + return cmd; > > +} > > + > > +static inline int smi_cmd_read(int addr, int reg) > > +{ > > + return smi_cmd(SMI_CMD_READ, addr, reg); > > +} > > + > > +static inline int smi_cmd_write(int addr, int reg) > > +{ > > + return smi_cmd(SMI_CMD_WRITE, addr, reg); > > +} > > + > > +/* Wait for the current SMI indirect command to complete */ > > +static int mv88e61xx_smi_wait(struct udevice *dev, int smi_addr) > > +{ > > + int val; > > + u32 timeout =3D 100; > > + > > + do { > > + val =3D dm_mdio_read(dev->parent, smi_addr, MDIO_DEVAD_NO= NE, SMI_CMD_REG); > > + if (val >=3D 0 && (val & SMI_BUSY) =3D=3D 0) > > + return 0; > > + > > + mdelay(1); > > + } while (--timeout); > > + > > + dev_err(dev, "SMI busy timeout\n"); > > + return -ETIMEDOUT; > > +} > > + > > +/* > > + * The mv88e61xx has three types of addresses: the smi bus address, th= e device > > + * address, and the register address. The smi bus address distinguish= es it on > > + * the smi bus from other PHYs or switches. The device address determ= ines > > + * which on-chip register set you are reading/writing (the various PHY= s, their > > + * associated ports, or global configuration registers). The register= address > > + * is the offset of the register you are reading/writing. > > + * > > + * When the mv88e61xx is hardware configured to have address zero, it = behaves in > > + * single-chip addressing mode, where it responds to all SMI addresses= , using > > + * the smi address as its device address. This obviously only works w= hen this > > + * is the only chip on the SMI bus. This allows the driver to access = device > > + * registers without using indirection. When the chip is configured t= o a > > + * non-zero address, it only responds to that SMI address and requires= indirect > > + * writes to access the different device addresses. > > + */ > > +static int mv88e61xx_reg_read(struct udevice *dev, int addr, int reg) > > +{ > > + struct mv88e61xx_priv *priv =3D dev_get_priv(dev); > > + int smi_addr =3D priv->smi_addr; > > + int res; > > + > > + /* In single-chip mode, the device can be addressed directly */ > > + if (smi_addr =3D=3D 0) > > + return dm_mdio_read(dev->parent, addr, MDIO_DEVAD_NONE, r= eg); > > + > > + /* Wait for the bus to become free */ > > + res =3D mv88e61xx_smi_wait(dev, smi_addr); > > + if (res < 0) > > + return res; > > + > > + /* Issue the read command */ > > + res =3D dm_mdio_write(dev->parent, smi_addr, MDIO_DEVAD_NONE, SMI= _CMD_REG, > > + smi_cmd_read(addr, reg)); > > + if (res < 0) > > + return res; > > + > > + /* Wait for the read command to complete */ > > + res =3D mv88e61xx_smi_wait(dev, smi_addr); > > + if (res < 0) > > + return res; > > + > > + /* Read the data */ > > + res =3D dm_mdio_read(dev->parent, smi_addr, MDIO_DEVAD_NONE, SMI_= DATA_REG); > > + if (res < 0) > > + return res; > > + > > + return bitfield_extract(res, 0, 16); > > +} > > + > > +/* See the comment above mv88e61xx_reg_read */ > > +static int mv88e61xx_reg_write(struct udevice *dev, int addr, int reg,= u16 val) > > +{ > > + struct mv88e61xx_priv *priv =3D dev_get_priv(dev); > > + int smi_addr =3D priv->smi_addr; > > + int res; > > + > > + /* In single-chip mode, the device can be addressed directly */ > > + if (smi_addr =3D=3D 0) > > + return dm_mdio_write(dev->parent, addr, MDIO_DEVAD_NONE, = reg, val); > > + > > + /* Wait for the bus to become free */ > > + res =3D mv88e61xx_smi_wait(dev, smi_addr); > > + if (res < 0) > > + return res; > > + > > + /* Set the data to write */ > > + res =3D dm_mdio_write(dev->parent, smi_addr, MDIO_DEVAD_NONE, > > + SMI_DATA_REG, val); > > + if (res < 0) > > + return res; > > + > > + /* Issue the write command */ > > + res =3D dm_mdio_write(dev->parent, smi_addr, MDIO_DEVAD_NONE, SMI= _CMD_REG, > > + smi_cmd_write(addr, reg)); > > + if (res < 0) > > + return res; > > + > > + /* Wait for the write command to complete */ > > + res =3D mv88e61xx_smi_wait(dev, smi_addr); > > + if (res < 0) > > + return res; > > + > > + return 0; > > +} > > + > > +static int mv88e61xx_phy_wait(struct udevice *dev) > > +{ > > + struct mv88e61xx_priv *priv =3D dev_get_priv(dev); > > + int val; > > + u32 timeout =3D 100; > > + > > + do { > > + val =3D mv88e61xx_reg_read(dev, priv->global2, GLOBAL2_RE= G_PHY_CMD); > > + if (val >=3D 0 && (val & SMI_BUSY) =3D=3D 0) > > + return 0; > > + > > + mdelay(1); > > + } while (--timeout); > > + > > + return -ETIMEDOUT; > > +} > > + > > +static int mv88e61xx_phy_read_indirect(struct udevice *dev, int phyad,= int devad, int reg) > > +{ > > + struct mv88e61xx_priv *priv =3D dev_get_priv(dev); > > + int res; > > + > > + /* Issue command to read */ > > + res =3D mv88e61xx_reg_write(dev, priv->global2, > > + GLOBAL2_REG_PHY_CMD, > > + smi_cmd_read(phyad, reg)); > > + > > + /* Wait for data to be read */ > > + res =3D mv88e61xx_phy_wait(dev); > > + if (res < 0) > > + return res; > > + > > + /* Read retrieved data */ > > + return mv88e61xx_reg_read(dev, priv->global2, > > + GLOBAL2_REG_PHY_DATA); > > +} > > + > > +static int mv88e61xx_phy_write_indirect(struct udevice *dev, int phyad= , > > + int devad, int reg, u16 data) > > +{ > > + struct mv88e61xx_priv *priv =3D dev_get_priv(dev); > > + int res; > > + > > + /* Set the data to write */ > > + res =3D mv88e61xx_reg_write(dev, priv->global2, > > + GLOBAL2_REG_PHY_DATA, data); > > + if (res < 0) > > + return res; > > + /* Issue the write command */ > > + res =3D mv88e61xx_reg_write(dev, priv->global2, > > + GLOBAL2_REG_PHY_CMD, > > + smi_cmd_write(phyad, reg)); > > + if (res < 0) > > + return res; > > + > > + /* Wait for command to complete */ > > + return mv88e61xx_phy_wait(dev); > > +} > > + > > +/* Wrapper function to make calls to phy_read_indirect simpler */ > > +static int mv88e61xx_phy_read(struct udevice *dev, int phy, int reg) > > +{ > > + return mv88e61xx_phy_read_indirect(dev, DEVADDR_PHY(phy), > > + MDIO_DEVAD_NONE, reg); > > +} > > + > > +/* Wrapper function to make calls to phy_write_indirect simpler */ > > +static int mv88e61xx_phy_write(struct udevice *dev, int phy, int reg, = u16 val) > > +{ > > + return mv88e61xx_phy_write_indirect(dev, DEVADDR_PHY(phy), > > + MDIO_DEVAD_NONE, reg, val); > > +} > > + > > +static int mv88e61xx_port_read(struct udevice *dev, u8 port, u8 reg) > > +{ > > + struct mv88e61xx_priv *priv =3D dev_get_priv(dev); > > + > > + return mv88e61xx_reg_read(dev, priv->port_reg_base + port, reg); > > +} > > + > > +static int mv88e61xx_port_write(struct udevice *dev, u8 port, u8 reg, = u16 val) > > +{ > > + struct mv88e61xx_priv *priv =3D dev_get_priv(dev); > > + > > + return mv88e61xx_reg_write(dev, priv->port_reg_base + port, reg, = val); > > +} > > + > > +static int mv88e61xx_set_page(struct udevice *dev, u8 phy, u8 page) > > +{ > > + return mv88e61xx_phy_write(dev, phy, PHY_REG_PAGE, page); > > +} > > + > > +static int mv88e61xx_get_switch_id(struct udevice *dev) > > +{ > > + int res; > > + > > + res =3D mv88e61xx_port_read(dev, 0, PORT_REG_SWITCH_ID); > > + if (res < 0) > > + return res; > > + return res & 0xfff0; > > +} > > + > > +static bool mv88e61xx_6352_family(struct udevice *dev) > > +{ > > + struct mv88e61xx_priv *priv =3D dev_get_priv(dev); > > + > > + switch (priv->id) { > > + case PORT_SWITCH_ID_6172: > > + case PORT_SWITCH_ID_6176: > > + case PORT_SWITCH_ID_6240: > > + case PORT_SWITCH_ID_6352: > > + return true; > > + } > > + return false; > > +} > > + > > +static int mv88e61xx_get_cmode(struct udevice *dev, u8 port) > > +{ > > + int res; > > + > > + res =3D mv88e61xx_port_read(dev, port, PORT_REG_STATUS); > > + if (res < 0) > > + return res; > > + return res & PORT_REG_STATUS_CMODE_MASK; > > +} > > + > > +static int mv88e61xx_switch_reset(struct udevice *dev) > > +{ > > + struct mv88e61xx_priv *priv =3D dev_get_priv(dev); > > + int time; > > + int val; > > + u8 port; > > + > > + /* Disable all ports */ > > + for (port =3D 0; port < priv->port_count; port++) { > > + val =3D mv88e61xx_port_read(dev, port, PORT_REG_CTRL); > > + if (val < 0) > > + return val; > > + val =3D bitfield_replace(val, PORT_REG_CTRL_PSTATE_SHIFT, > > + PORT_REG_CTRL_PSTATE_WIDTH, > > + PORT_REG_CTRL_PSTATE_DISABLED); > > + val =3D mv88e61xx_port_write(dev, port, PORT_REG_CTRL, va= l); > > + if (val < 0) > > + return val; > > + } > > + > > + /* Wait 2 ms for queues to drain */ > > + udelay(2000); > > + > > + /* Reset switch */ > > + val =3D mv88e61xx_reg_read(dev, priv->global1, GLOBAL1_CTRL); > > + if (val < 0) > > + return val; > > + val |=3D GLOBAL1_CTRL_SWRESET; > > + val =3D mv88e61xx_reg_write(dev, priv->global1, GLOBAL1_CTRL, val= ); > > + if (val < 0) > > + return val; > > + > > + /* Wait up to 1 second for switch reset complete */ > > + for (time =3D 1000; time; time--) { > > + val =3D mv88e61xx_reg_read(dev, priv->global1, GLOBAL1_CT= RL); > > + if (val >=3D 0 && ((val & GLOBAL1_CTRL_SWRESET) =3D=3D 0)= ) > > + break; > > + udelay(1000); > > + } > > + if (!time) > > + return -ETIMEDOUT; > > + > > + return 0; > > +} > > + > > +static int mv88e61xx_serdes_init(struct udevice *dev) > > +{ > > + int val; > > + > > + val =3D mv88e61xx_set_page(dev, DEVADDR_SERDES, PHY_PAGE_SERDES); > > + if (val < 0) > > + return val; > > + > > + /* Power up serdes module */ > > + val =3D mv88e61xx_phy_read(dev, DEVADDR_SERDES, MII_BMCR); > > + if (val < 0) > > + return val; > > + val &=3D ~(BMCR_PDOWN); > > + val =3D mv88e61xx_phy_write(dev, DEVADDR_SERDES, MII_BMCR, val); > > + if (val < 0) > > + return val; > > + > > + return 0; > > +} > > + > > +static int mv88e61xx_fixed_port_setup(struct udevice *dev, u8 port) > > +{ > > + struct dsa_pdata *dsa_pdata =3D dev_get_uclass_plat(dev); > > + struct mv88e61xx_priv *priv =3D dev_get_priv(dev); > > + int val; > > + > > + val =3D mv88e61xx_port_read(dev, port, PORT_REG_PHYS_CTRL); > > + if (val < 0) > > + return val; > > + > > + val &=3D ~(PORT_REG_PHYS_CTRL_SPD_MASK | > > + PORT_REG_PHYS_CTRL_FC_VALUE | > > + PORT_REG_PHYS_CTRL_FC_FORCE); > > + val |=3D PORT_REG_PHYS_CTRL_FC_FORCE | > > + PORT_REG_PHYS_CTRL_DUPLEX_VALUE | > > + PORT_REG_PHYS_CTRL_DUPLEX_FORCE; > > + > > + if (priv->id =3D=3D PORT_SWITCH_ID_6071) { > > + val |=3D PORT_REG_PHYS_CTRL_SPD100; > > + } else { > > + val |=3D PORT_REG_PHYS_CTRL_PCS_AN_EN | > > + PORT_REG_PHYS_CTRL_PCS_AN_RST | > > + PORT_REG_PHYS_CTRL_SPD1000; > > + } > > + > > + if (port =3D=3D dsa_pdata->cpu_port) > > + val |=3D PORT_REG_PHYS_CTRL_LINK_VALUE | > > + PORT_REG_PHYS_CTRL_LINK_FORCE; > > + > > + return mv88e61xx_port_write(dev, port, PORT_REG_PHYS_CTRL, val); > > +} > > + > > +/* > > + * This function is used to pre-configure the required register > > + * offsets, so that the indirect register access to the PHY registers > > + * is possible. This is necessary to be able to read the PHY ID > > + * while driver probing or in get_phy_id(). The globalN register > > + * offsets must be initialized correctly for a detected switch, > > + * otherwise detection of the PHY ID won't work! > > + */ > > +static int mv88e61xx_priv_reg_offs_pre_init(struct udevice *dev) > > +{ > > + struct mv88e61xx_priv *priv =3D dev_get_priv(dev); > > + > > + /* > > + * Initial 'port_reg_base' value must be an offset of existing > > + * port register, then reading the ID should succeed. First, try > > + * to read via port registers with device address 0x10 (88E6096 > > + * and compatible switches). > > + */ > > + priv->port_reg_base =3D 0x10; > > + priv->id =3D mv88e61xx_get_switch_id(dev); > > + if (priv->id !=3D 0xfff0) { > > + priv->global1 =3D 0x1B; > > + priv->global2 =3D 0x1C; > > + return 0; > > + } > > + > > + /* > > + * Now try via port registers with device address 0x08 > > + * (88E6020 and compatible switches). > > + */ > > + priv->port_reg_base =3D 0x08; > > + priv->id =3D mv88e61xx_get_switch_id(dev); > > + if (priv->id !=3D 0xfff0) { > > + priv->global1 =3D 0x0F; > > + priv->global2 =3D 0x07; > > + return 0; > > + } > > + > > + dev_warn(dev, "%s Unknown ID 0x%x\n", __func__, priv->id); > > + > > + return -ENODEV; > > +} > > + > > +static int mv88e61xx_mdio_read(struct udevice *dev, int addr, int deva= d, int reg) > > +{ > > + return mv88e61xx_phy_read_indirect(dev->parent, DEVADDR_PHY(addr)= , > > + MDIO_DEVAD_NONE, reg); > > +} > > + > > +static int mv88e61xx_mdio_write(struct udevice *dev, int addr, int dev= ad, > > + int reg, u16 val) > > +{ > > + return mv88e61xx_phy_write_indirect(dev->parent, DEVADDR_PHY(addr= ), > > + MDIO_DEVAD_NONE, reg, val); > > +} > > + > > +static const struct mdio_ops mv88e61xx_mdio_ops =3D { > > + .read =3D mv88e61xx_mdio_read, > > + .write =3D mv88e61xx_mdio_write, > > +}; > > + > > +static int mv88e61xx_mdio_bind(struct udevice *dev) > > +{ > > + char name[32]; > > + static int num_devices; > > + > > + sprintf(name, "mv88e61xx-mdio-%d", num_devices++); > > + device_set_name(dev, name); > > + > > + return 0; > > +} > > + > > +U_BOOT_DRIVER(mv88e61xx_mdio) =3D { > > + .name =3D "mv88e61xx_mdio", > > + .id =3D UCLASS_MDIO, > > + .ops =3D &mv88e61xx_mdio_ops, > > + .bind =3D mv88e61xx_mdio_bind, > > + .priv_auto =3D sizeof(struct mv88e61xx_priv), > > + .plat_auto =3D sizeof(struct mdio_perdev_priv), > > +}; > > + > > +static int mv88e61xx_dsa_port_enable(struct udevice *dev, int port, st= ruct phy_device *phy) > > +{ > > + struct dsa_pdata *dsa_pdata =3D dev_get_uclass_plat(dev); > > + struct mv88e61xx_priv *priv =3D dev_get_priv(dev); > > + int val, ret; > > + > > + dev_dbg(dev, "%s P%d phy:0x%08x %s\n", __func__, port, > > + phy->phy_id, phy_string_for_interface(phy->interface)); > > + > > + /* > > + * Enable energy-detect sensing on PHY, used to determine when a = PHY > > + * port is physically connected > > + */ > > + val =3D mv88e61xx_phy_read(dev, port, PHY_REG_CTRL1); > > + if (val < 0) > > + return val; > > + val =3D bitfield_replace(val, priv->phy_ctrl1_en_det_shift, > > + priv->phy_ctrl1_en_det_width, > > + priv->phy_ctrl1_en_det_ctrl); > > + val =3D mv88e61xx_phy_write(dev, port, PHY_REG_CTRL1, val); > > + if (val < 0) > > + return val; > > + > > + /* enable port */ > > + val =3D mv88e61xx_port_read(dev, port, PORT_REG_CTRL); > > + if (val < 0) > > + return val; > > + val =3D bitfield_replace(val, PORT_REG_CTRL_PSTATE_SHIFT, > > + PORT_REG_CTRL_PSTATE_WIDTH, > > + PORT_REG_CTRL_PSTATE_FORWARD); > > + val =3D mv88e61xx_port_write(dev, port, PORT_REG_CTRL, val); > > + if (val < 0) > > + return val; > > + > > + /* configure RGMII delays for fixed link */ > > + if (phy->phy_id =3D=3D PHY_FIXED_ID) { > > + /* Physical Control register: Table 62 */ > > + val =3D mv88e61xx_port_read(dev, port, PORT_REG_PHYS_CTRL= ); > > + val &=3D ~(PORT_REG_PHYS_CTRL_RGMII_DELAY_RXCLK || > > + PORT_REG_PHYS_CTRL_RGMII_DELAY_TXCLK); > > + if (phy->interface =3D=3D PHY_INTERFACE_MODE_RGMII_ID || > > + phy->interface =3D=3D PHY_INTERFACE_MODE_RGMII_RXID) > > + val |=3D PORT_REG_PHYS_CTRL_RGMII_DELAY_RXCLK; > > + if (phy->interface =3D=3D PHY_INTERFACE_MODE_RGMII_ID || > > + phy->interface =3D=3D PHY_INTERFACE_MODE_RGMII_TXID) > > + val |=3D PORT_REG_PHYS_CTRL_RGMII_DELAY_TXCLK; > > + val |=3D BIT(5) | BIT(4); /* Force link */ > > + ret =3D mv88e61xx_port_write(dev, port, PORT_REG_PHYS_CTR= L, val); > > + if (ret < 0) > > + return ret; > > + > > + ret =3D mv88e61xx_fixed_port_setup(dev, port); > > + if (ret < 0) > > + return ret; > > + } > > + > > + if (port =3D=3D dsa_pdata->cpu_port) { > > + /* Set CPUDest for cpu port */ > > + val =3D mv88e61xx_reg_read(dev, priv->global1, GLOBAL1_MO= N_CTRL); > > + if (val < 0) > > + return val; > > + val =3D bitfield_replace(val, GLOBAL1_MON_CTRL_CPUDEST_SH= IFT, > > + GLOBAL1_MON_CTRL_CPUDEST_WIDTH, po= rt); > > + val =3D mv88e61xx_reg_write(dev, priv->global1, GLOBAL1_M= ON_CTRL, val); > > + if (val < 0) > > + return val; > > + > > + if (mv88e61xx_6352_family(dev)) { > > + /* initialize serdes */ > > + val =3D mv88e61xx_get_cmode(dev, dsa_pdata->cpu_p= ort); > > + if (val < 0) > > + return val; > > + if (val =3D=3D PORT_REG_STATUS_CMODE_100BASE_X || > > + val =3D=3D PORT_REG_STATUS_CMODE_1000BASE_X |= | > > + val =3D=3D PORT_REG_STATUS_CMODE_SGMII) { > > + val =3D mv88e61xx_serdes_init(dev); > > + if (val < 0) > > + return val; > > + } > > + } > > + } > > + > > + return 0; > > +} > > + > > +static void mv88e61xx_dsa_port_disable(struct udevice *dev, int port, = struct phy_device *phy) > > +{ > > + int val; > > + > > + dev_dbg(dev, "%s P%d phy:0x%x\n", __func__, port, phy->phy_id); > > + > > + val =3D mv88e61xx_port_read(dev, port, PORT_REG_CTRL); > > + if (val < 0) > > + return; > > + val =3D bitfield_replace(val, PORT_REG_CTRL_PSTATE_SHIFT, > > + PORT_REG_CTRL_PSTATE_WIDTH, > > + PORT_REG_CTRL_PSTATE_DISABLED); > > + val =3D mv88e61xx_port_write(dev, port, PORT_REG_CTRL, val); > > + if (val < 0) > > + return; > > +} > > + > > +static const struct dsa_ops mv88e61xx_dsa_ops =3D { > > + .port_enable =3D mv88e61xx_dsa_port_enable, > > + .port_disable =3D mv88e61xx_dsa_port_disable, > > You can drop "dsa" from these names so they become "mv88e6xxx_port_enable= " > and "mv88e6xxx_port_disable". In Marvell terminology, a DSA port is a > technical term for a cascade port, and this is not what this is about. > ok > > +}; > > + > > +/* bind and probe the switch mdios */ > > +static int mv88e61xx_dsa_probe_mdio(struct udevice *dev) > > And in general you can drop the "dsa" word from most function names, it > makes them longer with not a lot of benefit. > ok > > +{ > > + struct udevice *pdev; > > + ofnode node, mdios; > > + const char *name; > > + int ret; > > + > > + /* bind phy ports of mdios child node to mv88e61xx_mdio device */ > > + mdios =3D dev_read_subnode(dev, "mdios"); > > + if (ofnode_valid(mdios)) { > > + ofnode_for_each_subnode(node, mdios) { > > + name =3D ofnode_get_name(node); > > + ret =3D device_bind_driver_to_node(dev, > > + "mv88e61xx_mdio"= , > > + name, node, &pde= v); > > + if (ret) { > > + dev_err(dev, "failed to bind %s: %d\n", n= ame, ret); > > + continue; > > + } > > + > > + /* need to probe it as there is no compatible to = do so */ > > + ret =3D uclass_get_device_by_ofnode(UCLASS_MDIO, = node, &pdev); > > + if (ret) { > > + dev_err(dev, "failed to probe %s: %d\n", = name, ret); > > + continue; > > + } > > What do you do with this pdev once you get it? Are you missing a device_p= robe() call? > Also, why "pdev" and not "dev"? What does the "p" stand for? struct udevice *dev is passed into the function so I use pdev to iterate over the ports in the mdios node so 'pdev' means 'port' here. I do not need to do anything with pdev but I must use uclass_get_device_by_ofnode() to probe it and that function requires it. I don't need to call device_probe because uclass_get_device_by_ofnode does it for me > > > + } > > + } > > + > > + /* bind the mdios node to the parent mdio driver */ > > + name =3D ofnode_get_name(mdios); > > + ret =3D device_bind_driver_to_node(dev, > > + "mv88e61xx_mdio", > > + name, mdios, &pdev); > > + if (ret) > > + dev_err(dev, "failed to bind %s: %d\n", name, ret); > > + > > + /* need to probe it as there is no compatible to do so */ > > + ret =3D uclass_get_device_by_ofnode(UCLASS_MDIO, mdios, &pdev); > > + if (ret) > > + dev_err(dev, "failed to probe %s: %d\n", name, ret); > > + > > + return ret; > > I think this after the "if (ofnode_valid(mdios))" check is all stray > code that should be removed. There is no udevice that should be bound to > the "mdios" node, that is just a container. yes, your correct that code is not needed - will remove > > That, plus you should try to reduce the indentation by one level by > exiting early if (!ofnode_valid(mdio)) (and not with an error, it is > completely valid to not have an MDIO controller described in DT). > ok, good idea Thanks for the review! Tim > > +} > > + > > +static int mv88e61xx_dsa_probe(struct udevice *dev) > > +{ > > + struct mv88e61xx_priv *priv =3D dev_get_priv(dev); > > + struct udevice *master =3D dsa_get_master(dev); > > + int ret; > > + > > + if (!master) > > + return -ENODEV; > > + > > + dev_dbg(dev, "%s master:%s\n", __func__, master->name); > > + > > + /* probe internal mdio bus */ > > + ret =3D mv88e61xx_dsa_probe_mdio(dev); > > + if (ret) > > + return ret; > > + > > + ret =3D mv88e61xx_priv_reg_offs_pre_init(dev); > > + if (ret) > > + return ret; > > + > > + dev_dbg(dev, "ID=3D0x%x PORT_BASE=3D0x%02x GLOBAL1=3D0x%02x GLOBA= L2=3D0x%02x\n", > > + priv->id, priv->port_reg_base, priv->global1, priv->globa= l2); > > + switch (priv->id) { > > + case PORT_SWITCH_ID_6096: > > + case PORT_SWITCH_ID_6097: > > + case PORT_SWITCH_ID_6172: > > + case PORT_SWITCH_ID_6176: > > + case PORT_SWITCH_ID_6240: > > + case PORT_SWITCH_ID_6352: > > + priv->port_count =3D 11; > > + priv->phy_ctrl1_en_det_shift =3D 8; > > + priv->phy_ctrl1_en_det_width =3D 2; > > + priv->phy_ctrl1_en_det_ctrl =3D > > + PHY_REG_CTRL1_ENERGY_DET_SENSE_XMIT; > > + break; > > + case PORT_SWITCH_ID_6020: > > + case PORT_SWITCH_ID_6070: > > + case PORT_SWITCH_ID_6071: > > + case PORT_SWITCH_ID_6220: > > + case PORT_SWITCH_ID_6250: > > + priv->port_count =3D 7; > > + priv->phy_ctrl1_en_det_shift =3D 14; > > + priv->phy_ctrl1_en_det_width =3D 1; > > + priv->phy_ctrl1_en_det_ctrl =3D > > + PHY_REG_CTRL1_ENERGY_DET_SENSE_PULSE; > > + break; > > + default: > > + return -ENODEV; > > + } > > + > > + ret =3D mv88e61xx_switch_reset(dev); > > + if (ret < 0) > > + return ret; > > + > > + dsa_set_tagging(dev, 0, 0); > > + > > + return 0; > > +} > > + > > +static const struct udevice_id mv88e61xx_ids[] =3D { > > + { .compatible =3D "marvell,mv88e6085" }, > > + { } > > +}; > > + > > +U_BOOT_DRIVER(mv88e61xx) =3D { > > + .name =3D "mv88e61xx", > > + .id =3D UCLASS_DSA, > > + .of_match =3D mv88e61xx_ids, > > + .probe =3D mv88e61xx_dsa_probe, > > + .ops =3D &mv88e61xx_dsa_ops, > > + .priv_auto =3D sizeof(struct mv88e61xx_priv), > > +}; > > -- > > 2.17.1 > >