[U-Boot] [PATCH v2] Marvell MV88E61XX Switch Driver support
Prafulla Wadaskar
prafulla at marvell.com
Thu Apr 9 12:10:39 CEST 2009
> -----Original Message-----
> From: Andrew Dyer [mailto:amdyer at gmail.com]
> Sent: Thursday, April 09, 2009 4:03 AM
> To: Prafulla Wadaskar
> Cc: u-boot at lists.denx.de; Ashish Karkare; Ronen Shitrit
> Subject: Re: [U-Boot] [PATCH v2] Marvell MV88E61XX Switch
> Driver support
>
> On Wed, Apr 8, 2009 at 1:30 PM, Prafulla Wadaskar
> <prafulla at marvell.com> wrote:
> > Chips supprted:-
> > 1. 88E6161 6 port gbe swtich with 5 integrated PHYs 2.
> 88E6165 6 port
> > gbe swtich with 5 integrated PHYs
> > Note: This driver is supported and tested against kirkwood egiga
> > interface, other interfaces can be added
>
> We are going to be using this chip in a project, so I'm happy
> to see this code come by :-) A few comments sprinkled around below.
Welcome...
>
> >
> > Contributors:
> > Yotam Admon <yotam at marvell.com>
> > Michael Blostein <michaelbl at marvell.com
> >
> > Reviewed by: Ronen Shitrit <rshitrit at marvell.com>
> > Signed-off-by: Prafulla Wadaskar <prafulla at marvell.com>
> > ---
> > Changelog:-
> > v2: updated as per review comments by Wolfgand Denk
>
> It's always good to spell the name of the guy with commit
> access right :-)
>
> > removed other two drivers form earlier patch debug_prints removed
> > driver moved to drivers/net/
> >
> > drivers/net/Makefile | 1 +
> > drivers/net/mv88e61xx.c | 291
> > +++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 292 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/net/mv88e61xx.c
> >
> > diff --git a/drivers/net/Makefile b/drivers/net/Makefile index
> > f0c5654..7482327 100644
> > --- a/drivers/net/Makefile
> > +++ b/drivers/net/Makefile
> > @@ -47,6 +47,7 @@ COBJS-$(CONFIG_MACB) += macb.o
> > COBJS-$(CONFIG_MCFFEC) += mcffec.o mcfmii.o
> > COBJS-$(CONFIG_MPC5xxx_FEC) += mpc5xxx_fec.o
> > COBJS-$(CONFIG_MPC512x_FEC) += mpc512x_fec.o
> > +COBJS-$(CONFIG_SWITCH_MV88E61XX) += mv88e61xx.o
> > COBJS-$(CONFIG_NATSEMI) += natsemi.o
> > COBJS-$(CONFIG_DRIVER_NE2000) += ne2000.o ne2000_base.o
> > COBJS-$(CONFIG_DRIVER_AX88796L) += ax88796.o ne2000_base.o
> diff --git
> > a/drivers/net/mv88e61xx.c b/drivers/net/mv88e61xx.c new file mode
> > 100644 index 0000000..8167919
> > --- /dev/null
> > +++ b/drivers/net/mv88e61xx.c
> > @@ -0,0 +1,291 @@
> > +/*
> > + * (C) Copyright 2009
> > + * Marvell Semiconductor <www.marvell.com>
> > + * Prafulla Wadaskar <prafulla at marvell.com>
> > + *
> > + * See file CREDITS for list of people who contributed to this
> > + * project.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation; either version 2 of
> > + * the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General
> Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
> > + * MA 02110-1301 USA
> > + */
> > +
> > +#include <common.h>
> > +
> > +#if defined (CONFIG_SWITCH_MV88E61XX)
> > +
> > +/* Enabled ports can be enabled in board header file */
> #if defined
> > +(CONFIG_MV88E61XX_ENABLED_PORTS) #define MV88E61XX_ENABLED_PORTS
> > +CONFIG_MV88E61XX_ENABLED_PORTS #else #define
> MV88E61XX_ENABLED_PORTS
> > +(BIT0 | BIT1 | BIT2 | \
> > + BIT3 | BIT4 | BIT5) #endif
> > +
> > +#if defined (CONFIG_88E6161)
> > +#define MV88E61XX_NAME "88E6161"
> > +#elif defined (CONFIG_88E6165)
> > +#define MV88E61XX_NAME "88E6165"
> > +#else
> > +#define MV88E61XX_NAME "88E61XX"
> > +#endif
> > +
> > +#define MV88E61XX_PHY_TIMEOUT 100000 #define
> > +MV88E61XX_MAX_PORTS_NUM 0x6
> > +/* CPU port can be configured in board header file */ #if defined
> > +(CONFIG_MV88E61XX_CPU_PORT) #define MV88E61XX_CPU_PORT
> > +CONFIG_MV88E61XX_CPU_PORT #else #define MV88E61XX_CPU_PORT
>
> > +0x5 #endif
> > +
> > +#define MV88E61XX_PRT_STS_REG 0x1 #define
> > +MV88E61XX_PRT_CTRL_REG 0x4 #define
> MV88E61XX_PRT_VMAP_REG
> > +0x6 #define MV88E61XX_PRT_VID_REG 0x7
> > +
> > +#define MV88E61XX_PRT_OFST 0x10 #define
> MV88E61XX_PHY_CMD
> > +0x18 #define MV88E61XX_PHY_DATA 0x19 #define
> > +MV88E61XX_GLOBAL2_REG_DEVADR 0x1C
> > +
> > +#ifdef CONFIG_KIRKWOOD_EGIGA
> > +#define smi_wr_reg eth_smi_reg_write #define smi_rd_reg
> > +eth_smi_reg_read #else /* add new interface above this */ #error
> > +Unsupported interface #endif
>
> As far as I understand it, "SMI" for the 88E6161 is really
> just the physical MII layer with some changes in the register
> definitions.
> Would it not make more sense to use the existing u-boot
> miiphy code in common/miiphyutil.c to define the access
> functions? This allows for platform code to register mii
> access functions and gets platform specific stuff like this
> out of the driver.
Thanks..miiphy is used for the same
>
> > +
> > +/* Chip Address mode
> > + * The Switch support two modes of operation
> > + * 1. single chip mode and
> > + * 2. Multi-chip mode
> > + * Refer chip documentation for more details
> > + *
> > + * By default single chip mode is configured
> > + * multichip mode operation can be configured in board header */
> > +#ifndef CONFIG_MV88E61XX_MULTICHIP_ADRMODE
> > +#define mv88e61xx_wr_phy smi_wr_reg
> > +#define mv88e61xx_rd_phy smi_rd_reg
> > +#else
> > +void mv88e61xx_wr_phy(u32 port, u32 phy_adr, u32 reg_ofs,
> u16 data) {
> > + u16 reg;
> > + u32 smi_dev_addr;
> > +
> > + smi_dev_addr = KW_REG_READ(KW_ETH_PHY_ADDR_REG(port));
>
> if CONFIG_MV88E61XX_MULTICHIP_ADRMODE is defined you're going
> to reference the KW_REG_READ and KW_ETH_PHY_ADDR_REG macros,
> which don't seem to be defined in this file. I'm assuming
> this is platform specific stuff.
It is more egiga driver specific,
you need to read phy address programmed for respective egga port on
Gbe controller to establish communication,
this settings is done in egiga driver, but there is no generic interface to read this
To make it more generic I have used miiphy api
i.e.
miiphy_read(name, 0xEE, 0xEE, &mii_dev_adr);
Values 0xEE are dummy to identify phy_address read.
Hope this will help,
we can define special interface in future if this becomes generic usecase
>
> > + do {
> > + smi_rd_reg(port, smi_dev_addr, 0x0, ®);
> > + } while ((reg & BIT15));
> > + /* Poll till SMIBusy bit is clear */
>
> How about a timeout here? I'm not a C style maven, but the
> extra () in the test could probably go away. Some constants
> for register names and bit functions would help as well.
> This code appears several times, so maybe could be factored
> out. Also the commenting style is a bit random through the
> file - sometimes the comment is before the action, other
> times afterwards.
Corrected....
Code factored too..
>
> > + smi_wr_reg(port, smi_dev_addr, 0x1, data);
> > + /* Write data to Switch indirect data register */
> > + smi_wr_reg(port, smi_dev_addr, 0x0,
> > + reg_ofs | (phy_adr << 5) | BIT10 | BIT12
> | BIT15);
> > + /* Write command to Switch indirect command
> register (write)
> > +*/ }
> > +
> > +void mv88e61xx_rd_phy(u32 port, u32 phy_adr, u32 reg_ofs,
> u16 * data)
> > +{
> > + u16 reg;
> > + u32 smi_dev_addr;
> > +
> > + smi_dev_addr = KW_REG_READ(KW_ETH_PHY_ADDR_REG(port));
>
> ditto above regarding the KW* macros
>
> > + do {
> > + smi_rd_reg(port, smi_dev_addr, 0x0, ®);
> > + } while ((reg & BIT15));
> > + /* Poll till SMIBusy bit is clear */
> > + smi_wr_reg(port, smi_dev_addr, 0x0,
> > + reg_ofs | (phy_adr << 5) | BIT10 | BIT12
> | BIT15);
> > + /* Write command to Switch indirect command
> register (read) */
> > + do {
> > + smi_rd_reg(port, smi_dev_addr, 0x0, ®);
> > + } while ((reg & BIT15));
> > + /* Poll till SMIBusy bit is clear */
> > + smi_rd_reg(port, smi_dev_addr, 0x1, (u16 *) & data);
> > + /* Read data from Switch indirect data register */
> } #endif /*
> > +CONFIG_MV88E61XX_MULTICHIP_ADRMODE */
> > +
> > +static void mv88e61xx_vlaninit(u32 port, u32 cpu_port, u32
> > +max_prtnum,
> > + u32 ports_ofs, u32 port_mask) {
> > + u32 prt;
> > + u16 reg;
> > +
> > + /* be sure all ports are disabled */
> > + for (prt = 0; prt < max_prtnum; prt++) {
> > + mv88e61xx_rd_phy(port, ports_ofs + prt,
> > + MV88E61XX_PRT_CTRL_REG,
> > + ®);
> > + reg &= ~0x3;
> > + mv88e61xx_wr_phy(port, ports_ofs + prt,
> > + MV88E61XX_PRT_CTRL_REG,
> > + reg);
> > + }
>
> Would this code (and similar code below) work on the 88E6123 which
> (IIRC) only has ports 0, 1, and 5?
It should.... I have added detection logic for 88E6123 too
>
> > + /* Set CPU port VID to 0x1 */
> > + mv88e61xx_rd_phy(port, (ports_ofs + cpu_port),
> > + MV88E61XX_PRT_VID_REG,
> > + ®);
> > + reg &= ~0xfff;
> > + reg |= 0x1;
> > + mv88e61xx_wr_phy(port, (ports_ofs + cpu_port),
> > + MV88E61XX_PRT_VID_REG,
> > + reg);
> > +
> > + /* Setting Port default priority for all ports to zero */
> > + for (prt = 0; prt < max_prtnum; prt++) {
> > + mv88e61xx_rd_phy(port, ports_ofs + prt,
> > + MV88E61XX_PRT_VID_REG,
> > + ®);
> > + reg &= ~0xc000;
> > + mv88e61xx_wr_phy(port, ports_ofs + prt,
> > + MV88E61XX_PRT_VID_REG,
> > + reg);
> > + }
> > + /* Setting VID and VID map for all ports except CPU port */
> > + for (prt = 0; prt < max_prtnum; prt++) {
> > + /* only for enabled ports */
> > + if ((1 << prt) & port_mask) {
> > + /* skip CPU port */
> > + if (prt == cpu_port)
> > + continue;
> > +
> > + /*
> > + * set Ports VLAN Mapping.
> > + * port prt <-->
> MV88E61XX_CPU_PORT VLAN #prt+1.
> > + */
> > +
> > + mv88e61xx_rd_phy(port, ports_ofs + prt,
> > +
> MV88E61XX_PRT_VID_REG, ®);
> > + reg &= ~0x0fff;
> > + reg |= (prt + 1);
> > + mv88e61xx_wr_phy(port, ports_ofs + prt,
> > +
> MV88E61XX_PRT_VID_REG, reg);
> > +
> > + /*
> > + * Set Vlan map table for all ports to send
> > + only to
> > + * MV88E61XX_CPU_PORT
> > + */
> > + mv88e61xx_rd_phy(port, ports_ofs + prt,
> > + MV88E61XX_PRT_VMAP_REG,
> > + ®);
> > + reg &= ~((1 << max_prtnum) - 1);
> > + reg |= (1 << cpu_port);
> > + mv88e61xx_wr_phy(port, ports_ofs + prt,
> > +
> MV88E61XX_PRT_VMAP_REG, reg);
> > + }
> > + }
> > + /* Set Vlan map table for MV88E61XX_CPU_PORT to see
> all ports
> > + */
> > + mv88e61xx_rd_phy(port, (ports_ofs + cpu_port),
> > + MV88E61XX_PRT_VMAP_REG,
> > + ®);
> > + reg &= ~((1 << max_prtnum) - 1);
> > + reg |= port_mask & ~(1 << cpu_port);
> > + mv88e61xx_wr_phy(port, (ports_ofs + cpu_port),
> > + MV88E61XX_PRT_VMAP_REG,
> > + reg);
> > +
> > + /*
> > + * enable only appropriate ports to forwarding mode
> > + * and disable the others
> > + */
> > + for (prt = 0; prt < max_prtnum; prt++) {
> > + if ((1 << prt) & port_mask) {
> > + mv88e61xx_rd_phy(port, ports_ofs + prt,
> > + MV88E61XX_PRT_CTRL_REG,
> > +®);
> > + reg |= 0x3;
> > + mv88e61xx_wr_phy(port, ports_ofs + prt,
> > +
> MV88E61XX_PRT_CTRL_REG, reg);
> > + } else {
> > + /* Disable port */
> > + mv88e61xx_rd_phy(port, ports_ofs + prt,
> > + MV88E61XX_PRT_CTRL_REG,
> > +®);
> > + reg &= ~0x3;
> > + mv88e61xx_wr_phy(port, ports_ofs + prt,
> > +
> MV88E61XX_PRT_CTRL_REG, reg);
> > + }
> > + }
> > +}
> > +
> > +/*
> > + * Marvell 88E61XX Switch initialization */ int
> > +mv_switch_88e61xx_init(u32 port) {
> > + u32 prt;
> > + u16 reg;
> > + volatile u32 timeout;
> > +
> > + /* Init vlan */
> > + mv88e61xx_vlaninit(port, MV88E61XX_CPU_PORT,
> > + MV88E61XX_MAX_PORTS_NUM,
> > + MV88E61XX_PRT_OFST,
> > + MV88E61XX_ENABLED_PORTS);
> > +
> > + /* Enable RGMII delay on Tx and Rx for CPU port */
> > + mv88e61xx_wr_phy(port, 0x14, 0x1a, 0x81e7);
> > + mv88e61xx_rd_phy(port, 0x15, 0x1a, ®);
> > + mv88e61xx_wr_phy(port, 0x15, 0x1a, 0x18);
> > + mv88e61xx_wr_phy(port, 0x14, 0x1a, 0xc1e7);
>
> ^ -- lots of magic numbers above and below here -- v
>
> In the middle of the above sequence you read the phy, but
> don't seem to do anything with 'reg'. Is this required?
I am sorry at this moment, I will replace them latter since I don't have enough data
>
> What if I don't want RGMII delay as I'm using GMII on my platform?
For GMII/MII above settings not required...
I have encapsulated them with CONFIG_SYS_RGMII_MODE
>
> > +
> > + for (prt = 0; prt < MV88E61XX_MAX_PORTS_NUM; prt++) {
> > + if (prt != MV88E61XX_CPU_PORT) {
> > + /*Enable Phy power up */
> > + mv88e61xx_wr_phy(port,
> > + MV88E61XX_GLOBAL2_REG_DEVADR,
> > +
> MV88E61XX_PHY_DATA, 0x3360);
> > + mv88e61xx_wr_phy(port,
> > + MV88E61XX_GLOBAL2_REG_DEVADR,
> > + MV88E61XX_PHY_CMD,
> > + (0x9410 | (prt << 5)));
> > +
> > + /*
> > + * Make sure SMIBusy bit cleared before
> > + another
> > + * SMI operation can take place
> > + */
> > + timeout = MV88E61XX_PHY_TIMEOUT;
> > + do {
> > + mv88e61xx_rd_phy(port,
> > +
> > + MV88E61XX_GLOBAL2_REG_DEVADR,
> > + MV88E61XX_PHY_CMD,
> > + ®);
> > + if (timeout-- == 0) {
> > + printf("SMI busy
> timeout\n");
> > + return -1;
> > + }
> > + } while (reg & BIT28); /* busy mask */
> > +
> > + mv88e61xx_wr_phy(port,
> > + MV88E61XX_GLOBAL2_REG_DEVADR,
> > +
> MV88E61XX_PHY_DATA, 0x1140);
> > + mv88e61xx_wr_phy(port,
> > + MV88E61XX_GLOBAL2_REG_DEVADR,
> > + MV88E61XX_PHY_CMD,
> > + (0x9400 | (prt << 5)));
> > +
> > + /*
> > + * Make sure SMIBusy bit cleared before
> > + another
> > + * SMI operation can take place
> > + */
> > + timeout = MV88E61XX_PHY_TIMEOUT;
> > + do {
> > + mv88e61xx_rd_phy(port,
> > +
> > + MV88E61XX_GLOBAL2_REG_DEVADR,
> > + MV88E61XX_PHY_CMD,
> > + ®);
> > + if (timeout-- == 0) {
> > + printf("SMI busy
> timeout\n");
> > + return -1;
> > + }
> > + } while (reg & BIT28); /* busy mask */
>
> replicated status polling code, might want to factor it out.
>
> > + }
> > +
> > + /*Enable port */
> > + mv88e61xx_wr_phy(port, MV88E61XX_PRT_OFST + prt, 4,
> > + 0x7f);
> > + }
> > + /*Force CPU port to RGMII FDX 1000Base */
> > + mv88e61xx_wr_phy(port, MV88E61XX_PRT_OFST +
> > + MV88E61XX_CPU_PORT, 1,
> > + 0x3e);
>
> This last write seems very platform specific, for example in
These lines removed, by default switch detects speed normaly
> our case we want GMII, not RGMII. On another project we
> might want ports 4 & 5 to use the copper phy.
In this case this driver many need updates, or we need to provide such interface.
Any was current objective of this driver it to power up Phy and configure switch in managed mode. Also there are several h/s settings that go on the board which configures switch on power on Reset.
Thanks for feedback, I have updated the driver, will post v3 soon
Regards..
Prafulla . .
>
> > +
> > + printf(MV88E61XX_NAME " Initialized\n");
> > + return 0;
> > +}
> > +
> > +#endif /* CONFIG_SWITCH_MV88E61XX */
> > --
> > 1.5.3.3
> >
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
> >
>
>
>
> --
> Hardware, n.:
> The parts of a computer system that can be kicked.
>
More information about the U-Boot
mailing list