| Message ID | 20081007234622.GF23200@xi.wantstofly.org |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 8 Oct 2008, Lennert Buytenhek wrote: > + > +static int reg_read(struct dsa_switch *ds, int addr, int reg) > +{ > + return mdiobus_read(ds->master_mii_bus, addr, reg); > +} Instead of all the code to add the mdio buses to the device tree and exporting mdiobus_read(), could you have just added the switch chip as a "phy" on the mdio bus? Then used phy_read/write() on it? I know it's not a phy, but you don't need to use the phy state machine and so on. Freescale gianfar MACs configure the serdes for SGMII using a virtual "TBI" device on the MDIO bus. Supposedly Andy has patch that adds them as devices on the MDIO bus, isn't of the gianfar specific MDIO back-door they currently use. > +#define REG_READ(addr, reg) \ > + ({ \ > + int __ret; \ > + \ > + __ret = reg_read(ds, addr, reg); \ > + if (__ret < 0) \ > + return __ret; \ > + __ret; \ > + }) Macro with a hidden use of a local ('ds') and a hidden return? The former is discouraged (but can sure save a lot of typing) but the latter seems like it's just begging to cause a missing unlock or free on an error path. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 07, 2008 at 05:45:41PM -0700, Trent Piepho wrote: > > +static int reg_read(struct dsa_switch *ds, int addr, int reg) > > +{ > > + return mdiobus_read(ds->master_mii_bus, addr, reg); > > +} > > Instead of all the code to add the mdio buses to the device tree and exporting > mdiobus_read(), could you have just added the switch chip as a "phy" on the > mdio bus? Then used phy_read/write() on it? I know it's not a phy, but you > don't need to use the phy state machine and so on. At least the switches that are supported now don't take up only one MII bus address, but use all 32, so I wouldn't be able to use phy_{read,write}() on them. And the first 5-10 of those addresses generally look like normal PHYs, but you wouldn't want to attach PHY drivers to them directly (since you need to wrap accesses to the PHYs on some switches to disable the internal PHY polling that the switch does first), which would complicate autoprobing quite a bit. > Freescale gianfar MACs configure the serdes for SGMII using a virtual > "TBI" device on the MDIO bus. Supposedly Andy has patch that adds > them as devices on the MDIO bus, isn't of the gianfar specific MDIO > back-door they currently use. I think with gianfar, the MDIO bus drivers are separate platform devices, and you can then get at the struct mii_bus * by taking the platform device private data pointer -- which is a bit of an ugly way of doing it, IMHO. > > +#define REG_READ(addr, reg) \ > > + ({ \ > > + int __ret; \ > > + \ > > + __ret = reg_read(ds, addr, reg); \ > > + if (__ret < 0) \ > > + return __ret; \ > > + __ret; \ > > + }) > > Macro with a hidden use of a local ('ds') and a hidden return? The > former is discouraged (but can sure save a lot of typing) but the > latter seems like it's just begging to cause a missing unlock or > free on an error path. Yeah, well, that was intentional (Nicolas Pitre suggested it, and I figured it made sense to do it). I think there are a couple more places in the tree that do this, and it very much increases readability because you no longer need to check the return value explicitly every single time you do an MII access. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 8, 2008 at 3:49 AM, Lennert Buytenhek <buytenh@wantstofly.org> wrote: > On Tue, Oct 07, 2008 at 05:45:41PM -0700, Trent Piepho wrote: > >> > +#define REG_READ(addr, reg) \ >> > + ({ \ >> > + int __ret; \ >> > + \ >> > + __ret = reg_read(ds, addr, reg); \ >> > + if (__ret < 0) \ >> > + return __ret; \ >> > + __ret; \ >> > + }) >> >> Macro with a hidden use of a local ('ds') and a hidden return? The >> former is discouraged (but can sure save a lot of typing) but the >> latter seems like it's just begging to cause a missing unlock or >> free on an error path. > > Yeah, well, that was intentional (Nicolas Pitre suggested it, and I > figured it made sense to do it). I think there are a couple more places > in the tree that do this, and it very much increases readability because > you no longer need to check the return value explicitly every single > time you do an MII access. I agree with Trent, it only increases readability in the sense that you have to read less when scanning over the code. To me, that is the least important sort of readability. This sort of macro just creates hidden traps. I know it's annoying to check that return value every time, but that's why we get paid the big bucks. ;) If, for some reason, the community feels this is the right way to go, at *least* rename the macro to make it clear that it returns on an error, and make ds an explicit parameter. Andy -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 08, 2008 at 03:20:18PM -0500, Andy Fleming wrote: > >> > +#define REG_READ(addr, reg) \ > >> > + ({ \ > >> > + int __ret; \ > >> > + \ > >> > + __ret = reg_read(ds, addr, reg); \ > >> > + if (__ret < 0) \ > >> > + return __ret; \ > >> > + __ret; \ > >> > + }) > >> > >> Macro with a hidden use of a local ('ds') and a hidden return? The > >> former is discouraged (but can sure save a lot of typing) but the > >> latter seems like it's just begging to cause a missing unlock or > >> free on an error path. > > > > Yeah, well, that was intentional (Nicolas Pitre suggested it, and I > > figured it made sense to do it). I think there are a couple more places > > in the tree that do this, and it very much increases readability because > > you no longer need to check the return value explicitly every single > > time you do an MII access. > > I agree with Trent, it only increases readability in the sense that > you have to read less when scanning over the code. To me, that is the > least important sort of readability. This sort of macro just creates > hidden traps. I know it's annoying to check that return value every > time, but that's why we get paid the big bucks. ;) Well, I think I picked this up from reading kernel sources. :) And some quick grepping shows that there's plenty of places that do this (implicit argument and/or implicit return): arch/powerpc/boot/libfdt/fdt_ro.c: #define CHECK_HEADER(fdt) \ { \ int err; \ if ((err = fdt_check_header(fdt)) != 0) \ return err; \ } drivers/net/e1000e/ethtool.c: #define REG_PATTERN_TEST_ARRAY(reg, offset, mask, write) \ do { \ if (reg_pattern_test(adapter, data, reg, offset, mask, write)) \ return 1; \ } while (0) drivers/net/wireless/ath5k/eeprom.h: define AR5K_ASSERT_ENTRY(_e, _s) do { \ if (_e >= _s) \ return (false); \ } while (0) I'd agree that there is some potential for lossage, but I wouldn't say that the implicit return thing is all that bad. I guess I just got used to the construct. > If, for some reason, the community feels this is the right way to go, > at *least* rename the macro to make it clear that it returns on an > error, and make ds an explicit parameter. Explicit parameter, fine with me. How about renaming REG_{READ,WRITE} to REG_{READ,WRITE}_RETURN_ON_ERR or somesuch? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig index 505aa14..3f2fd39 100644 --- a/net/dsa/Kconfig +++ b/net/dsa/Kconfig @@ -28,6 +28,13 @@ config NET_DSA_MV88E6XXX bool default n +config NET_DSA_MV88E6060 + bool "Marvell 88E6060 ethernet switch chip support" + select NET_DSA_TAG_TRAILER + ---help--- + This enables support for the Marvell 88E6060 ethernet switch + chip. + config NET_DSA_MV88E6XXX_NEED_PPU bool default n diff --git a/net/dsa/Makefile b/net/dsa/Makefile index 63d3c44..2374faf 100644 --- a/net/dsa/Makefile +++ b/net/dsa/Makefile @@ -5,6 +5,7 @@ obj-$(CONFIG_NET_DSA_TAG_TRAILER) += tag_trailer.o # switch drivers obj-$(CONFIG_NET_DSA_MV88E6XXX) += mv88e6xxx.o +obj-$(CONFIG_NET_DSA_MV88E6060) += mv88e6060.o obj-$(CONFIG_NET_DSA_MV88E6123_61_65) += mv88e6123_61_65.o obj-$(CONFIG_NET_DSA_MV88E6131) += mv88e6131.o diff --git a/net/dsa/mv88e6060.c b/net/dsa/mv88e6060.c new file mode 100644 index 0000000..54068ef --- /dev/null +++ b/net/dsa/mv88e6060.c @@ -0,0 +1,287 @@ +/* + * net/dsa/mv88e6060.c - Driver for Marvell 88e6060 switch chips + * Copyright (c) 2008 Marvell Semiconductor + * + * 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. + */ + +#include <linux/list.h> +#include <linux/netdevice.h> +#include <linux/phy.h> +#include "dsa_priv.h" + +#define REG_PORT(p) (8 + (p)) +#define REG_GLOBAL 0x0f + +static int reg_read(struct dsa_switch *ds, int addr, int reg) +{ + return mdiobus_read(ds->master_mii_bus, addr, reg); +} + +#define REG_READ(addr, reg) \ + ({ \ + int __ret; \ + \ + __ret = reg_read(ds, addr, reg); \ + if (__ret < 0) \ + return __ret; \ + __ret; \ + }) + + +static int reg_write(struct dsa_switch *ds, int addr, int reg, u16 val) +{ + return mdiobus_write(ds->master_mii_bus, addr, reg, val); +} + +#define REG_WRITE(addr, reg, val) \ + ({ \ + int __ret; \ + \ + __ret = reg_write(ds, addr, reg, val); \ + if (__ret < 0) \ + return __ret; \ + }) + +static char *mv88e6060_probe(struct mii_bus *bus, int sw_addr) +{ + int ret; + + ret = mdiobus_read(bus, REG_PORT(0), 0x03); + if (ret >= 0) { + ret &= 0xfff0; + if (ret == 0x0600) + return "Marvell 88E6060"; + } + + return NULL; +} + +static int mv88e6060_switch_reset(struct dsa_switch *ds) +{ + int i; + int ret; + + /* + * Set all ports to the disabled state. + */ + for (i = 0; i < 6; i++) { + ret = REG_READ(REG_PORT(i), 0x04); + REG_WRITE(REG_PORT(i), 0x04, ret & 0xfffc); + } + + /* + * Wait for transmit queues to drain. + */ + msleep(2); + + /* + * Reset the switch. + */ + REG_WRITE(REG_GLOBAL, 0x0A, 0xa130); + + /* + * Wait up to one second for reset to complete. + */ + for (i = 0; i < 1000; i++) { + ret = REG_READ(REG_GLOBAL, 0x00); + if ((ret & 0x8000) == 0x0000) + break; + + msleep(1); + } + if (i == 1000) + return -ETIMEDOUT; + + return 0; +} + +static int mv88e6060_setup_global(struct dsa_switch *ds) +{ + /* + * Disable discarding of frames with excessive collisions, + * set the maximum frame size to 1536 bytes, and mask all + * interrupt sources. + */ + REG_WRITE(REG_GLOBAL, 0x04, 0x0800); + + /* + * Enable automatic address learning, set the address + * database size to 1024 entries, and set the default aging + * time to 5 minutes. + */ + REG_WRITE(REG_GLOBAL, 0x0a, 0x2130); + + return 0; +} + +static int mv88e6060_setup_port(struct dsa_switch *ds, int p) +{ + int addr = REG_PORT(p); + + /* + * Do not force flow control, disable Ingress and Egress + * Header tagging, disable VLAN tunneling, and set the port + * state to Forwarding. Additionally, if this is the CPU + * port, enable Ingress and Egress Trailer tagging mode. + */ + REG_WRITE(addr, 0x04, (p == ds->cpu_port) ? 0x4103 : 0x0003); + + /* + * Port based VLAN map: give each port its own address + * database, allow the CPU port to talk to each of the 'real' + * ports, and allow each of the 'real' ports to only talk to + * the CPU port. + */ + REG_WRITE(addr, 0x06, + ((p & 0xf) << 12) | + ((p == ds->cpu_port) ? + ds->valid_port_mask : + (1 << ds->cpu_port))); + + /* + * Port Association Vector: when learning source addresses + * of packets, add the address to the address database using + * a port bitmap that has only the bit for this port set and + * the other bits clear. + */ + REG_WRITE(addr, 0x0b, 1 << p); + + return 0; +} + +static int mv88e6060_setup(struct dsa_switch *ds) +{ + int i; + int ret; + + ret = mv88e6060_switch_reset(ds); + if (ret < 0) + return ret; + + /* @@@ initialise atu */ + + ret = mv88e6060_setup_global(ds); + if (ret < 0) + return ret; + + for (i = 0; i < 6; i++) { + ret = mv88e6060_setup_port(ds, i); + if (ret < 0) + return ret; + } + + return 0; +} + +static int mv88e6060_set_addr(struct dsa_switch *ds, u8 *addr) +{ + REG_WRITE(REG_GLOBAL, 0x01, (addr[0] << 8) | addr[1]); + REG_WRITE(REG_GLOBAL, 0x02, (addr[2] << 8) | addr[3]); + REG_WRITE(REG_GLOBAL, 0x03, (addr[4] << 8) | addr[5]); + + return 0; +} + +static int mv88e6060_port_to_phy_addr(int port) +{ + if (port >= 0 && port <= 5) + return port; + return -1; +} + +static int mv88e6060_phy_read(struct dsa_switch *ds, int port, int regnum) +{ + int addr; + + addr = mv88e6060_port_to_phy_addr(port); + if (addr == -1) + return 0xffff; + + return reg_read(ds, addr, regnum); +} + +static int +mv88e6060_phy_write(struct dsa_switch *ds, int port, int regnum, u16 val) +{ + int addr; + + addr = mv88e6060_port_to_phy_addr(port); + if (addr == -1) + return 0xffff; + + return reg_write(ds, addr, regnum, val); +} + +static void mv88e6060_poll_link(struct dsa_switch *ds) +{ + int i; + + for (i = 0; i < DSA_MAX_PORTS; i++) { + struct net_device *dev; + int port_status; + int link; + int speed; + int duplex; + int fc; + + dev = ds->ports[i]; + if (dev == NULL) + continue; + + link = 0; + if (dev->flags & IFF_UP) { + port_status = reg_read(ds, REG_PORT(i), 0x00); + if (port_status < 0) + continue; + + link = !!(port_status & 0x1000); + } + + if (!link) { + if (netif_carrier_ok(dev)) { + printk(KERN_INFO "%s: link down\n", dev->name); + netif_carrier_off(dev); + } + continue; + } + + speed = (port_status & 0x0100) ? 100 : 10; + duplex = (port_status & 0x0200) ? 1 : 0; + fc = ((port_status & 0xc000) == 0xc000) ? 1 : 0; + + if (!netif_carrier_ok(dev)) { + printk(KERN_INFO "%s: link up, %d Mb/s, %s duplex, " + "flow control %sabled\n", dev->name, + speed, duplex ? "full" : "half", + fc ? "en" : "dis"); + netif_carrier_on(dev); + } + } +} + +static struct dsa_switch_driver mv88e6060_switch_driver = { + .tag_protocol = htons(ETH_P_TRAILER), + .probe = mv88e6060_probe, + .setup = mv88e6060_setup, + .set_addr = mv88e6060_set_addr, + .phy_read = mv88e6060_phy_read, + .phy_write = mv88e6060_phy_write, + .poll_link = mv88e6060_poll_link, +}; + +int __init mv88e6060_init(void) +{ + register_switch_driver(&mv88e6060_switch_driver); + return 0; +} +module_init(mv88e6060_init); + +void __exit mv88e6060_cleanup(void) +{ + unregister_switch_driver(&mv88e6060_switch_driver); +} +module_exit(mv88e6060_cleanup);