Differential D19036 Diff 54632 sys/dev/etherswitch/e6000sw/e6000sw.c

Changeset View

Standalone View

sys/dev/etherswitch/e6000sw/e6000sw.c

Show All 39 Lines
#include <net/if.h>		#include <net/if.h>
#include <net/if_media.h>		#include <net/if_media.h>
#include <net/if_types.h>		#include <net/if_types.h>

#include <dev/etherswitch/etherswitch.h>		#include <dev/etherswitch/etherswitch.h>
#include <dev/mii/mii.h>		#include <dev/mii/mii.h>
#include <dev/mii/miivar.h>		#include <dev/mii/miivar.h>

#include <dev/fdt/fdt_common.h>
#include <dev/ofw/ofw_bus.h>		#include <dev/ofw/ofw_bus.h>
		#include <dev/ofw/ofw_bus_subr.h>

#include "e6000swreg.h"		#include "e6000swreg.h"
#include "etherswitch_if.h"		#include "etherswitch_if.h"
#include "miibus_if.h"		#include "miibus_if.h"
#include "mdio_if.h"		#include "mdio_if.h"

MALLOC_DECLARE(M_E6000SW);		MALLOC_DECLARE(M_E6000SW);
MALLOC_DEFINE(M_E6000SW, "e6000sw", "e6000sw switch");		MALLOC_DEFINE(M_E6000SW, "e6000sw", "e6000sw switch");
Show All 29 Lines	static etherswitch_info_t etherswitch_info = {
.es_nports = 0,		.es_nports = 0,
.es_nvlangroups = 0,		.es_nvlangroups = 0,
.es_vlan_caps = ETHERSWITCH_VLAN_PORT,		.es_vlan_caps = ETHERSWITCH_VLAN_PORT,
.es_name = "Marvell 6000 series switch"		.es_name = "Marvell 6000 series switch"
};		};

static void e6000sw_identify(driver_t *, device_t);		static void e6000sw_identify(driver_t *, device_t);
static int e6000sw_probe(device_t);		static int e6000sw_probe(device_t);
		static int e6000sw_parse_fixed_link(e6000sw_softc_t *, phandle_t, uint32_t);
		static int e6000sw_parse_ethernet(e6000sw_softc_t *, phandle_t, uint32_t);
static int e6000sw_attach(device_t);		static int e6000sw_attach(device_t);
static int e6000sw_detach(device_t);		static int e6000sw_detach(device_t);
static int e6000sw_readphy(device_t, int, int);		static int e6000sw_readphy(device_t, int, int);
static int e6000sw_writephy(device_t, int, int, int);		static int e6000sw_writephy(device_t, int, int, int);
static etherswitch_info_t* e6000sw_getinfo(device_t);		static etherswitch_info_t* e6000sw_getinfo(device_t);
static int e6000sw_getconf(device_t, etherswitch_conf_t *);		static int e6000sw_getconf(device_t, etherswitch_conf_t *);
static void e6000sw_lock(device_t);		static void e6000sw_lock(device_t);
static void e6000sw_unlock(device_t);		static void e6000sw_unlock(device_t);
▲ Show 20 Lines • Show All 79 Lines • ▼ Show 20 Lines
#define MDIO_READ(dev, addr, reg) \		#define MDIO_READ(dev, addr, reg) \
MDIO_READREG(device_get_parent(dev), (addr), (reg))		MDIO_READREG(device_get_parent(dev), (addr), (reg))
#define MDIO_WRITE(dev, addr, reg, val) \		#define MDIO_WRITE(dev, addr, reg, val) \
MDIO_WRITEREG(device_get_parent(dev), (addr), (reg), (val))		MDIO_WRITEREG(device_get_parent(dev), (addr), (reg), (val))

static void		static void
e6000sw_identify(driver_t *driver, device_t parent)		e6000sw_identify(driver_t *driver, device_t parent)
{		{

dteskeUnsubmitted Done Inline Actions Removing this blank line breaks style(9). To explain why the blank line was there: This function declares no variables and as such should have a blank line after the opening curly. dteske: Removing this blank line breaks style(9). To explain why the blank line was there: This…
if (device_find_child(parent, "e6000sw", -1) == NULL)		if (device_find_child(parent, "e6000sw", -1) == NULL)
BUS_ADD_CHILD(parent, 0, "e6000sw", -1);		BUS_ADD_CHILD(parent, 0, "e6000sw", -1);
}		}

static int		static int
e6000sw_probe(device_t dev)		e6000sw_probe(device_t dev)
{		{
e6000sw_softc_t *sc;		e6000sw_softc_t *sc;
const char *description;		const char *description;
phandle_t dsa_node, switch_node;		phandle_t switch_node;

dsa_node = fdt_find_compatible(OF_finddevice("/"),		switch_node = ofw_bus_find_compatible(OF_finddevice("/"),
"marvell,dsa", 0);		"marvell,mv88e6085");
switch_node = OF_child(dsa_node);

if (switch_node == 0)		if (switch_node == 0)
return (ENXIO);		return (ENXIO);

		if (bootverbose)
		device_printf(dev, "Found switch_node: 0x%x\n", switch_node);

sc = device_get_softc(dev);		sc = device_get_softc(dev);
sc->dev = dev;		sc->dev = dev;
sc->node = switch_node;		sc->node = switch_node;

if (OF_getencprop(sc->node, "reg", &sc->sw_addr,		if (OF_getencprop(sc->node, "reg", &sc->sw_addr,
sizeof(sc->sw_addr)) < 0)		sizeof(sc->sw_addr)) < 0)
return (ENXIO);		return (ENXIO);

if (!OF_hasprop(sc->node, "single-chip-addressing") &&		/*
(sc->sw_addr != 0 && (sc->sw_addr % 2) == 0))		* According to the Linux source code, all of the Switch IDs we support
		* are multi_chip capable, and should go into multi-chip mode if the
		* sw_addr != 0.
		*/
		if (!OF_hasprop(sc->node, "single-chip-addressing") && sc->sw_addr != 0)
sc->multi_chip = true;		sc->multi_chip = true;

		manuUnsubmitted Not Done Inline Actions I don't see any reference to this property in the binding docs or in the dts. Does that still exists ? Or is it again a Marvell specific property that only exists in their u-boot/linux fork ? manu: I don't see any reference to this property in the binding docs or in the dts. Does that still…
		mwUnsubmitted Not Done Inline Actions Hi Manu, Indeed this property is not documentet in Linux binding, however it is possible to configure board in a way that we have single chip addressing mode with non-zero mdio addressing. This is a workaround for a problem on the commercial, out-of-tree product (it uses FreeBSD only), therefore I'd prefer to keep it here. Best regards, Marcin mw: Hi Manu, Indeed this property is not documentet in Linux binding, however it is possible to…
		manuUnsubmitted Not Done Inline Actions Ok thanks for the clarification, maybe we should add a comment then. manu: Ok thanks for the clarification, maybe we should add a comment then.
		andrewUnsubmitted Not Done Inline Actions If it's a FreeBSD specific property we should prefix it with `freebsd,`. andrew: If it's a FreeBSD specific property we should prefix it with `freebsd,`.
		xistence_0x58.comAuthorUnsubmitted Done Inline Actions It looks like it was added in a commit by someone at Semihalf: https://github.com/freebsd/freebsd/commit/879cf7c79fa3a9a665515dc5f832a185f8927295 I haven't touched this part of it (even though I can't find a similar statement in any of the Linux tree), because I didn't want to remove something that was added with a purpose. xistence_0x58.com: It looks like it was added in a commit by someone at Semihalf: https://github.
/*		/*
* Create temporary lock, just to satisfy assertions,		* Create temporary lock, just to satisfy assertions,
* when obtaining the switch ID. Destroy immediately afterwards.		* when obtaining the switch ID. Destroy immediately afterwards.
*/		*/
sx_init(&sc->sx, "e6000sw_tmp");		sx_init(&sc->sx, "e6000sw_tmp");
E6000SW_LOCK(sc);		E6000SW_LOCK(sc);
sc->swid = e6000sw_readreg(sc, REG_PORT(0), SWITCH_ID) & 0xfff0;		sc->swid = e6000sw_readreg(sc, REG_PORT(0), SWITCH_ID) & 0xfff0;
E6000SW_UNLOCK(sc);		E6000SW_UNLOCK(sc);
Show All 28 Lines	e6000sw_probe(device_t dev)
}		}

device_set_desc(dev, description);		device_set_desc(dev, description);

return (BUS_PROBE_DEFAULT);		return (BUS_PROBE_DEFAULT);
}		}

static int		static int
e6000sw_parse_child_fdt(e6000sw_softc_t sc, phandle_t child, int pport)		e6000sw_parse_fixed_link(e6000sw_softc_t *sc, phandle_t fixed_link, uint32_t port)
{		{
char name, portlabel;
int speed;		int speed;
		if (fixed_link != 0) {
		sc->fixed_mask \|= (1 << port);

		if (OF_getencprop(fixed_link, "speed", &speed, sizeof(speed))> 0) {
		if (speed == 2500 &&
		(MVSWITCH(sc, MV88E6141) \|\|
		MVSWITCH(sc, MV88E6341))) {
		mwUnsubmitted Done Inline Actions no need to use {} with single line under 'if' mw: no need to use {} with single line under 'if'
		sc->fixed25_mask \|= (1 << port);
		}
		} else {
		device_printf(sc->dev,
		"Port %d has a fixed-link node without a speed "
		"property\n", port);

		return (ENXIO);
		}
		}

		return (0);
		}

		static int
		e6000sw_parse_ethernet(e6000sw_softc_t *sc, phandle_t port_handle, uint32_t port) {
		phandle_t fixed_link, switch_eth, switch_eth_handle;

		if (OF_getencprop(port_handle, "ethernet", (void*)&switch_eth_handle,
		sizeof(switch_eth_handle)) > 0) {
		if (switch_eth_handle > 0) {
		switch_eth = OF_node_from_xref(switch_eth_handle);

		fixed_link = ofw_bus_find_child(switch_eth,
		"fixed-link");
		andrewUnsubmitted Done Inline Actions Could this be moved into `e6000sw_parse_fixed_link` (and the same below)? andrew: Could this be moved into `e6000sw_parse_fixed_link` (and the same below)?

		if (e6000sw_parse_fixed_link(sc, fixed_link, port) != 0) {
		mwUnsubmitted Done Inline Actions drop {} mw: drop {}
		andrewUnsubmitted Done Inline Actions Or just make it `return (e6000sw_parse_fixed_link(...));` andrew: Or just make it `return (e6000sw_parse_fixed_link(...));`
		return (ENXIO);
		}

		} else
		device_printf(sc->dev,
		"Port %d has ethernet property but it points "
		"to an invalid location\n", port);
		}

		return (0);
		}

		static int
		e6000sw_parse_child_fdt(e6000sw_softc_t sc, phandle_t child, int pport)
		{
phandle_t fixed_link;		phandle_t fixed_link;
uint32_t port;		uint32_t port;

if (pport == NULL)		if (pport == NULL)
return (ENXIO);		return (ENXIO);

if (OF_getencprop(child, "reg", (void *)&port, sizeof(port)) < 0)		if (OF_getencprop(child, "reg", (void *)&port, sizeof(port)) < 0)
return (ENXIO);		return (ENXIO);
if (port >= sc->num_ports)		if (port >= sc->num_ports)
return (ENXIO);		return (ENXIO);
*pport = port;		*pport = port;

if (OF_getprop_alloc(child, "label", (void **)&portlabel) > 0) {		fixed_link = ofw_bus_find_child(child, "fixed-link");
if (strncmp(portlabel, "cpu", 3) == 0) {		if (e6000sw_parse_fixed_link(sc, fixed_link, port) != 0)
device_printf(sc->dev, "CPU port at %d\n", port);		return (ENXIO);
sc->cpuports_mask \|= (1 << port);
sc->fixed_mask \|= (1 << port);
}
free(portlabel, M_OFWPROP);
}

fixed_link = OF_child(child);		if (e6000sw_parse_ethernet(sc, child, port) != 0)
if (fixed_link != 0 &&		return (ENXIO);
		manuUnsubmitted Done Inline Actions I know that it's not part of the review but cpu port don't have a label property. So we will never set sc->cpuports_mask in this function. You should remove this block and set cpuports_mask/fixed_mask if the port node have a ethernet property (this seems to be the only way to detect a cpu port). manu: I know that it's not part of the review but cpu port don't have a label property. So we will…
		xistence_0x58.comAuthorUnsubmitted Done Inline Actions I can definitely update this as part of the review, even if it is not something I changed :-) xistence_0x58.com: I can definitely update this as part of the review, even if it is not something I changed :-)
OF_getprop_alloc(fixed_link, "name", (void **)&name) > 0) {
if (strncmp(name, "fixed-link", 10) == 0) {
/* Assume defaults: 1g - full-duplex. */
sc->fixed_mask \|= (1 << port);
if (OF_getencprop(fixed_link, "speed", &speed,
sizeof(speed)) > 0) {
if (speed == 2500 &&
(MVSWITCH(sc, MV88E6141) \|\|
MVSWITCH(sc, MV88E6341))) {
sc->fixed25_mask \|= (1 << port);
}
}
}
free(name, M_OFWPROP);
}
if ((sc->fixed_mask & (1 << port)) != 0)		if ((sc->fixed_mask & (1 << port)) != 0)
		manuUnsubmitted Done Inline Actions speed property is mandatory for fixed-link, you should print a message and return ENXIO if it's not present. manu: speed property is mandatory for fixed-link, you should print a message and return ENXIO if it's…
		mwUnsubmitted Done Inline Actions +1 mw: +1
device_printf(sc->dev, "fixed port at %d\n", port);		device_printf(sc->dev, "fixed port at %d\n", port);
else		else
		xistence_0x58.comAuthorUnsubmitted Done Inline Actions Extra `{ }` for readability on the else. xistence_0x58.com: Extra `{ }` for readability on the else.
device_printf(sc->dev, "PHY at port %d\n", port);		device_printf(sc->dev, "PHY at port %d\n", port);

return (0);		return (0);
}		}
		manuUnsubmitted Done Inline Actions fixed-link is a property of the port, not the node referenced by the ethernet phandle manu: fixed-link is a property of the port, not the node referenced by the ethernet phandle
		manuUnsubmitted Done Inline Actions Meh, bindings says that but the dts do not follow this ... Maybe we should check both ways then. manu: Meh, bindings says that but the dts do not follow this ... Maybe we should check both ways then.
		mwUnsubmitted Done Inline Actions In armada-388-clearfog.dtsi it is properly defined as a child node with props inside. However this patch introduces a functional change - it checks the fixed-link only for a port containing 'ethernet' property (i.e. the 'cpu' port). However it is possible to have it also on a 'lan' port, when it's connected to the external PHY (please refer to the clearfog DT as well). mw: In armada-388-clearfog.dtsi it is properly defined as a child node with props inside. However…
		xistence_0x58.comAuthorUnsubmitted Done Inline Actions I will update this code to do the check both ways so that we support either format for the DTS. xistence_0x58.com: I will update this code to do the check both ways so that we support either format for the DTS.

static int		static int
e6000sw_init_interface(e6000sw_softc_t *sc, int port)		e6000sw_init_interface(e6000sw_softc_t *sc, int port)
{		{
char name[IFNAMSIZ];		char name[IFNAMSIZ];

snprintf(name, IFNAMSIZ, "%sport", device_get_nameunit(sc->dev));		snprintf(name, IFNAMSIZ, "%sport", device_get_nameunit(sc->dev));

Show All 27 Lines	e6000sw_attach_miibus(e6000sw_softc_t *sc, int port)

return (0);		return (0);
}		}

static int		static int
e6000sw_attach(device_t dev)		e6000sw_attach(device_t dev)
{		{
e6000sw_softc_t *sc;		e6000sw_softc_t *sc;
phandle_t child;		phandle_t child, ports;
int err, port;		int err, port;
uint32_t reg;		uint32_t reg;

err = 0;		err = 0;
sc = device_get_softc(dev);		sc = device_get_softc(dev);

if (sc->multi_chip)		if (sc->multi_chip)
device_printf(dev, "multi-chip addressing mode\n");		device_printf(dev, "multi-chip addressing mode\n");
else		else
device_printf(dev, "single-chip addressing mode\n");		device_printf(dev, "single-chip addressing mode\n");

sx_init(&sc->sx, "e6000sw");		sx_init(&sc->sx, "e6000sw");

E6000SW_LOCK(sc);		E6000SW_LOCK(sc);
e6000sw_setup(dev, sc);		e6000sw_setup(dev, sc);

for (child = OF_child(sc->node); child != 0; child = OF_peer(child)) {		ports = ofw_bus_find_child(sc->node, "ports");

		manuUnsubmitted Done Inline Actions You need to check that ports != 0 manu: You need to check that ports != 0
		mwUnsubmitted Done Inline Actions +1 mw: +1
		xistence_0x58.comAuthorUnsubmitted Done Inline Actions If ports is 0, should I just return? A switch with no ports doesn't make much sense. xistence_0x58.com: If ports is 0, should I just return? A switch with no ports doesn't make much sense.
		if (ports == 0) {
		device_printf(dev, "failed to parse DTS: no ports found for "
		xistence_0x58.comAuthorUnsubmitted Done Inline Actions Style violation, will update indentation. xistence_0x58.com: Style violation, will update indentation.
		"switch\n");
		return (ENXIO);
		}

		for (child = OF_child(ports); child != 0; child = OF_peer(child)) {
err = e6000sw_parse_child_fdt(sc, child, &port);		err = e6000sw_parse_child_fdt(sc, child, &port);
if (err != 0) {		if (err != 0) {
device_printf(sc->dev, "failed to parse DTS\n");		device_printf(sc->dev, "failed to parse DTS\n");
goto out_fail;		goto out_fail;
}		}

/* Port is in use. */		/* Port is in use. */
sc->ports_mask \|= (1 << port);		sc->ports_mask \|= (1 << port);
▲ Show 20 Lines • Show All 920 Lines • Show Last 20 Lines