* [PATCH v3 0/8] Add MV88E61xx DSA driver and use on gwventana
@ 2022-05-23 18:25 Tim Harvey
2022-05-23 18:25 ` [PATCH v3 1/8] net: mdio-uclass: scan for dm mdio children on post-bind Tim Harvey
` (7 more replies)
0 siblings, 8 replies; 26+ messages in thread
From: Tim Harvey @ 2022-05-23 18:25 UTC (permalink / raw)
To: Joe Hershberger, Ramon Fried, Vladimir Oltean, u-boot,
Stefano Babic, Fabio Estevam, NXP i . MX U-Boot Team,
Marek Behún
Cc: Chris Packham, Anatolij Gustschin, Tim Harvey
This series adds a DSA driver for the MV88E61xx based on
drivers/net/phy/mv88e61xx and uses in on the gwventana_gw5904_defconfig.
The hope is that the other three boards that use the MV88E61xx driver
can move to this as well eventually so that we can remove the non-dm
driver and the 4 Kconfig options it requires.
The MV88E61xx has an MDIO interface thus DM_MDIO must be used so support
for a UCLASS_MDIO driver is added to the fec_mxc ethernet driver in a
way that allows a fallback to the previous non DM_MDIO case as there are
many boards out there using this driver that define DM_MDIO but do not
have the required dt props for a DM_MDIO driver which would cause a
regression.
Additionally some other patches are here suggested by Vladimir:
- ensure MDIO children are scanned on post-bind is needed
- sanity check DSA driver required ops are present
- allow DSA drivers to not require xmit/recv functions
- remove unecessary xmit/recv functions from ksz9477 driver
v3:
- fix mdios node in dt
- add Vladimir's rb tag's
v2:
- added Ramon's rb tag's to first two patches
- add patches for dsa-uclass to sanity check ops and make xmit/recv
optional
- fec: fix fallback for non conforming DM_MDIO dts
- mv88e61xx:
- rebase on v2022.07-rc2 (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
Tested on a Gateworks GW5904 which has a Marvell 88E6176 switch hanging
off the IMX6 FEC.
Best Regards,
Tim
Tim Harvey (8):
net: mdio-uclass: scan for dm mdio children on post-bind
net: dsa: move cpu port probe to dsa_post_probe
net: dsa: ensure dsa driver has proper ops
net: dsa: allow rcv() and xmit() to be optional
net: ksz9477: remove unnecessary xmit and recv functions
net: fec: add support for DM_MDIO
net: add MV88E61xx DSA driver
board: gw_ventana: enable MV88E61XX DSA support
arch/arm/dts/imx6qdl-gw5904.dtsi | 41 ++
board/gateworks/gw_ventana/gw_ventana.c | 50 +-
configs/gwventana_gw5904_defconfig | 7 +-
drivers/net/Kconfig | 7 +
drivers/net/Makefile | 1 +
drivers/net/fec_mxc.c | 90 ++-
drivers/net/ksz9477.c | 23 -
drivers/net/mv88e61xx.c | 843 ++++++++++++++++++++++++
net/dsa-uclass.c | 58 +-
net/mdio-uclass.c | 4 +
10 files changed, 1051 insertions(+), 73 deletions(-)
create mode 100644 drivers/net/mv88e61xx.c
--
2.17.1
^ permalink raw reply [flat|nested] 26+ messages in thread* [PATCH v3 1/8] net: mdio-uclass: scan for dm mdio children on post-bind 2022-05-23 18:25 [PATCH v3 0/8] Add MV88E61xx DSA driver and use on gwventana Tim Harvey @ 2022-05-23 18:25 ` Tim Harvey 2022-05-23 18:25 ` [PATCH v3 2/8] net: dsa: move cpu port probe to dsa_post_probe Tim Harvey ` (6 subsequent siblings) 7 siblings, 0 replies; 26+ messages in thread From: Tim Harvey @ 2022-05-23 18:25 UTC (permalink / raw) To: Joe Hershberger, Ramon Fried, Vladimir Oltean, u-boot, Stefano Babic, Fabio Estevam, NXP i . MX U-Boot Team, Marek Behún Cc: Chris Packham, Anatolij Gustschin, Tim Harvey If a DM_MDIO driver is used we need to scan the subnodes as well. Signed-off-by: Tim Harvey <tharvey@gateworks.com> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> Reviewed-by: Ramon Fried <rfried.dev@gmail.com> --- v2: - added Ramon's rb tag --- net/mdio-uclass.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/net/mdio-uclass.c b/net/mdio-uclass.c index 4401492ca015..d80037d0ac71 100644 --- a/net/mdio-uclass.c +++ b/net/mdio-uclass.c @@ -49,7 +49,11 @@ static int dm_mdio_post_bind(struct udevice *dev) return -EINVAL; } +#if CONFIG_IS_ENABLED(OF_REAL) + return dm_scan_fdt_dev(dev); +#else return 0; +#endif } int dm_mdio_read(struct udevice *mdio_dev, int addr, int devad, int reg) -- 2.17.1 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 2/8] net: dsa: move cpu port probe to dsa_post_probe 2022-05-23 18:25 [PATCH v3 0/8] Add MV88E61xx DSA driver and use on gwventana Tim Harvey 2022-05-23 18:25 ` [PATCH v3 1/8] net: mdio-uclass: scan for dm mdio children on post-bind Tim Harvey @ 2022-05-23 18:25 ` Tim Harvey 2022-05-23 18:25 ` [PATCH v3 3/8] net: dsa: ensure dsa driver has proper ops Tim Harvey ` (5 subsequent siblings) 7 siblings, 0 replies; 26+ messages in thread From: Tim Harvey @ 2022-05-23 18:25 UTC (permalink / raw) To: Joe Hershberger, Ramon Fried, Vladimir Oltean, u-boot, Stefano Babic, Fabio Estevam, NXP i . MX U-Boot Team, Marek Behún Cc: Chris Packham, Anatolij Gustschin, Tim Harvey In order to ensure that a DSA driver probe gets called before dsa_ops->port_probe move the port_probe of the cpu_port to a post-probe function. Signed-off-by: Tim Harvey <tharvey@gateworks.com> Reviewed-by: Ramon Fried <rfried.dev@gmail.com> Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- v3: - added Vladimir's rb tag v2: - added Ramon's rb tag --- net/dsa-uclass.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c index 9ff55a02fb23..07edc584daf3 100644 --- a/net/dsa-uclass.c +++ b/net/dsa-uclass.c @@ -466,8 +466,6 @@ static int dsa_pre_probe(struct udevice *dev) { struct dsa_pdata *pdata = dev_get_uclass_plat(dev); struct dsa_priv *priv = dev_get_uclass_priv(dev); - struct dsa_ops *ops = dsa_get_ops(dev); - int err; priv->num_ports = pdata->num_ports; priv->cpu_port = pdata->cpu_port; @@ -480,6 +478,15 @@ static int dsa_pre_probe(struct udevice *dev) uclass_find_device_by_ofnode(UCLASS_ETH, pdata->master_node, &priv->master_dev); + return 0; +} + +static int dsa_post_probe(struct udevice *dev) +{ + struct dsa_priv *priv = dev_get_uclass_priv(dev); + struct dsa_ops *ops = dsa_get_ops(dev); + int err; + /* Simulate a probing event for the CPU port */ if (ops->port_probe) { err = ops->port_probe(dev, priv->cpu_port, @@ -489,13 +496,14 @@ static int dsa_pre_probe(struct udevice *dev) } return 0; -} +}; UCLASS_DRIVER(dsa) = { .id = UCLASS_DSA, .name = "dsa", .post_bind = dsa_post_bind, .pre_probe = dsa_pre_probe, + .post_probe = dsa_post_probe, .per_device_auto = sizeof(struct dsa_priv), .per_device_plat_auto = sizeof(struct dsa_pdata), .per_child_plat_auto = sizeof(struct dsa_port_pdata), -- 2.17.1 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 3/8] net: dsa: ensure dsa driver has proper ops 2022-05-23 18:25 [PATCH v3 0/8] Add MV88E61xx DSA driver and use on gwventana Tim Harvey 2022-05-23 18:25 ` [PATCH v3 1/8] net: mdio-uclass: scan for dm mdio children on post-bind Tim Harvey 2022-05-23 18:25 ` [PATCH v3 2/8] net: dsa: move cpu port probe to dsa_post_probe Tim Harvey @ 2022-05-23 18:25 ` Tim Harvey 2022-05-23 18:25 ` [PATCH v3 4/8] net: dsa: allow rcv() and xmit() to be optional Tim Harvey ` (4 subsequent siblings) 7 siblings, 0 replies; 26+ messages in thread From: Tim Harvey @ 2022-05-23 18:25 UTC (permalink / raw) To: Joe Hershberger, Ramon Fried, Vladimir Oltean, u-boot, Stefano Babic, Fabio Estevam, NXP i . MX U-Boot Team, Marek Behún Cc: Chris Packham, Anatolij Gustschin, Tim Harvey Add a function to sanity check a dsa driver having proper ops. Suggested-by: Vladimir Oltean <vladimir.oltean@nxp.com> Signed-off-by: Tim Harvey <tharvey@gateworks.com> Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- v3: - added Vladimir's rb tag v2: new patch --- net/dsa-uclass.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c index 07edc584daf3..b3033c97aa63 100644 --- a/net/dsa-uclass.c +++ b/net/dsa-uclass.c @@ -342,6 +342,19 @@ U_BOOT_DRIVER(dsa_port) = { .plat_auto = sizeof(struct eth_pdata), }; +static int dsa_sanitize_ops(struct udevice *dev) +{ + struct dsa_ops *ops = dsa_get_ops(dev); + + if ((!ops->xmit || !ops->rcv) && + (!ops->port_enable && !ops->port_disable)) { + dev_err(dev, "Packets cannot be steered to ports\n"); + return -EINVAL; + } + + return 0; +} + /* * This function mostly deals with pulling information out of the device tree * into the pdata structure. @@ -358,6 +371,10 @@ static int dsa_post_bind(struct udevice *dev) if (!ofnode_valid(node)) return -ENODEV; + err = dsa_sanitize_ops(dev); + if (err) + return err; + pdata->master_node = ofnode_null(); node = ofnode_find_subnode(node, "ports"); -- 2.17.1 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 4/8] net: dsa: allow rcv() and xmit() to be optional 2022-05-23 18:25 [PATCH v3 0/8] Add MV88E61xx DSA driver and use on gwventana Tim Harvey ` (2 preceding siblings ...) 2022-05-23 18:25 ` [PATCH v3 3/8] net: dsa: ensure dsa driver has proper ops Tim Harvey @ 2022-05-23 18:25 ` Tim Harvey 2022-05-23 18:25 ` [PATCH v3 5/8] net: ksz9477: remove unnecessary xmit and recv functions Tim Harvey ` (3 subsequent siblings) 7 siblings, 0 replies; 26+ messages in thread From: Tim Harvey @ 2022-05-23 18:25 UTC (permalink / raw) To: Joe Hershberger, Ramon Fried, Vladimir Oltean, u-boot, Stefano Babic, Fabio Estevam, NXP i . MX U-Boot Team, Marek Behún Cc: Chris Packham, Anatolij Gustschin, Tim Harvey Allow rcv() and xmit() dsa driver ops to be optional in case a driver does not care to mangle a packet as in U-Boot only one network port is enabled at a time and thus no packet mangling is necessary. Suggested-by: Vladimir Oltean <vladimir.oltean@nxp.com> Signed-off-by: Tim Harvey <tharvey@gateworks.com> Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- v3: - added Vladimir's rb tag v2: new patch --- net/dsa-uclass.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c index b3033c97aa63..f82217e0d7a7 100644 --- a/net/dsa-uclass.c +++ b/net/dsa-uclass.c @@ -131,16 +131,14 @@ static void dsa_port_stop(struct udevice *pdev) * We copy the frame to a stack buffer where we have reserved headroom and * tailroom space. Headroom and tailroom are set to 0. */ -static int dsa_port_send(struct udevice *pdev, void *packet, int length) +static int dsa_port_mangle_packet(struct udevice *pdev, void *packet, int length) { + struct dsa_port_pdata *port_pdata = dev_get_parent_plat(pdev); struct udevice *dev = dev_get_parent(pdev); struct dsa_priv *priv = dev_get_uclass_priv(dev); int head = priv->headroom, tail = priv->tailroom; - struct udevice *master = dsa_get_master(dev); struct dsa_ops *ops = dsa_get_ops(dev); uchar dsa_packet_tmp[PKTSIZE_ALIGN]; - struct dsa_port_pdata *port_pdata; - int err; if (length + head + tail > PKTSIZE_ALIGN) return -EINVAL; @@ -152,10 +150,21 @@ static int dsa_port_send(struct udevice *pdev, void *packet, int length) /* copy back to preserve original buffer alignment */ memcpy(packet, dsa_packet_tmp, length); - port_pdata = dev_get_parent_plat(pdev); - err = ops->xmit(dev, port_pdata->index, packet, length); - if (err) - return err; + return ops->xmit(dev, port_pdata->index, packet, length); +} + +static int dsa_port_send(struct udevice *pdev, void *packet, int length) +{ + struct udevice *dev = dev_get_parent(pdev); + struct udevice *master = dsa_get_master(dev); + struct dsa_ops *ops = dsa_get_ops(dev); + int err; + + if (ops->xmit) { + err = dsa_port_mangle_packet(pdev, packet, length); + if (err) + return err; + } return eth_get_ops(master)->send(master, packet, length); } @@ -172,7 +181,7 @@ static int dsa_port_recv(struct udevice *pdev, int flags, uchar **packetp) int length, port_index, err; length = eth_get_ops(master)->recv(master, flags, packetp); - if (length <= 0) + if (length <= 0 || !ops->rcv) return length; /* -- 2.17.1 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 5/8] net: ksz9477: remove unnecessary xmit and recv functions 2022-05-23 18:25 [PATCH v3 0/8] Add MV88E61xx DSA driver and use on gwventana Tim Harvey ` (3 preceding siblings ...) 2022-05-23 18:25 ` [PATCH v3 4/8] net: dsa: allow rcv() and xmit() to be optional Tim Harvey @ 2022-05-23 18:25 ` Tim Harvey 2022-05-23 18:25 ` [PATCH v3 6/8] net: fec: add support for DM_MDIO Tim Harvey ` (2 subsequent siblings) 7 siblings, 0 replies; 26+ messages in thread From: Tim Harvey @ 2022-05-23 18:25 UTC (permalink / raw) To: Joe Hershberger, Ramon Fried, Vladimir Oltean, u-boot, Stefano Babic, Fabio Estevam, NXP i . MX U-Boot Team, Marek Behún Cc: Chris Packham, Anatolij Gustschin, Tim Harvey Remove the unnecessary xmit and recv functions. Signed-off-by: Tim Harvey <tharvey@gateworks.com> Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- v3: - added Vladimir's rb tag v2: new patch --- drivers/net/ksz9477.c | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/drivers/net/ksz9477.c b/drivers/net/ksz9477.c index ed8f1895cb12..fb5c76c600be 100644 --- a/drivers/net/ksz9477.c +++ b/drivers/net/ksz9477.c @@ -62,7 +62,6 @@ struct ksz_dsa_priv { struct udevice *dev; - int active_port; }; static inline int ksz_read8(struct udevice *dev, u32 reg, u8 *val) @@ -382,9 +381,6 @@ static int ksz_port_enable(struct udevice *dev, int port, struct phy_device *phy data8 |= SW_START; ksz_write8(priv->dev, REG_SW_OPERATION, data8); - /* keep track of current enabled non-cpu port */ - priv->active_port = port; - return 0; } @@ -413,28 +409,9 @@ static void ksz_port_disable(struct udevice *dev, int port, struct phy_device *p */ } -static int ksz_xmit(struct udevice *dev, int port, void *packet, int length) -{ - dev_dbg(dev, "%s P%d %d\n", __func__, port + 1, length); - - return 0; -} - -static int ksz_recv(struct udevice *dev, int *port, void *packet, int length) -{ - struct ksz_dsa_priv *priv = dev_get_priv(dev); - - dev_dbg(dev, "%s P%d %d\n", __func__, priv->active_port + 1, length); - *port = priv->active_port; - - return 0; -}; - static const struct dsa_ops ksz_dsa_ops = { .port_enable = ksz_port_enable, .port_disable = ksz_port_disable, - .xmit = ksz_xmit, - .rcv = ksz_recv, }; static int ksz_probe_mdio(struct udevice *dev) -- 2.17.1 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 6/8] net: fec: add support for DM_MDIO 2022-05-23 18:25 [PATCH v3 0/8] Add MV88E61xx DSA driver and use on gwventana Tim Harvey ` (4 preceding siblings ...) 2022-05-23 18:25 ` [PATCH v3 5/8] net: ksz9477: remove unnecessary xmit and recv functions Tim Harvey @ 2022-05-23 18:25 ` Tim Harvey 2022-05-23 18:25 ` [PATCH v3 7/8] net: add MV88E61xx DSA driver Tim Harvey 2022-05-23 18:25 ` [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support Tim Harvey 7 siblings, 0 replies; 26+ messages in thread From: Tim Harvey @ 2022-05-23 18:25 UTC (permalink / raw) To: Joe Hershberger, Ramon Fried, Vladimir Oltean, u-boot, Stefano Babic, Fabio Estevam, NXP i . MX U-Boot Team, Marek Behún Cc: Chris Packham, Anatolij Gustschin, Tim Harvey Add support for DM_MDIO by registering a UCLASS_MDIO driver and attempting to use it. This is necessary if wanting to use a DSA driver for example hanging off of the FEC MAC. Care is taken to fallback to non DM_MDIO mii bus as several boards define DM_MDIO without having the proper device-tree configuration necessary such as an mdio subnode, a phy-mode prop, and either a valid phy-handle prop or fixed-phy subnode which will cause dm_eth_phy_connect() to fail. Signed-off-by: Tim Harvey <tharvey@gateworks.com> --- v2: - fix fallback mechanism for legacy dt's that do not have phy-handle and mdio subnode --- drivers/net/fec_mxc.c | 90 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 88 insertions(+), 2 deletions(-) diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index a623a5c45e4d..9553699e3306 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -30,6 +30,8 @@ #include <asm/arch/imx-regs.h> #include <asm/mach-imx/sys_proto.h> #include <asm-generic/gpio.h> +#include <dm/device_compat.h> +#include <dm/lists.h> #include "fec_mxc.h" #include <eth_phy.h> @@ -1025,6 +1027,81 @@ struct mii_dev *fec_get_miibus(ulong base_addr, int dev_id) return bus; } +#ifdef CONFIG_DM_MDIO +struct dm_fec_mdio_priv { + struct ethernet_regs *regs; +}; + +static int dm_fec_mdio_read(struct udevice *dev, int addr, int devad, int reg) +{ + struct dm_fec_mdio_priv *priv = dev_get_priv(dev); + + return fec_mdio_read(priv->regs, addr, reg); +} + +static int dm_fec_mdio_write(struct udevice *dev, int addr, int devad, int reg, u16 data) +{ + struct dm_fec_mdio_priv *priv = dev_get_priv(dev); + + return fec_mdio_write(priv->regs, addr, reg, data); +} + +static const struct mdio_ops dm_fec_mdio_ops = { + .read = dm_fec_mdio_read, + .write = dm_fec_mdio_write, +}; + +static int dm_fec_mdio_probe(struct udevice *dev) +{ + struct dm_fec_mdio_priv *priv = dev_get_priv(dev); + + priv->regs = (struct ethernet_regs *)ofnode_get_addr(dev_ofnode(dev->parent)); + + return 0; +} + +U_BOOT_DRIVER(fec_mdio) = { + .name = "fec_mdio", + .id = UCLASS_MDIO, + .probe = dm_fec_mdio_probe, + .ops = &dm_fec_mdio_ops, + .priv_auto = sizeof(struct dm_fec_mdio_priv), +}; + +static int dm_fec_bind_mdio(struct udevice *dev) +{ + struct udevice *mdiodev; + const char *name; + ofnode mdio; + int ret = -ENODEV; + + /* for a UCLASS_MDIO driver we need to bind and probe manually + * for an internal MDIO bus that has no dt compatible of its own + */ + ofnode_for_each_subnode(mdio, dev_ofnode(dev)) { + name = ofnode_get_name(mdio); + + if (strcmp(name, "mdio")) + continue; + + ret = device_bind_driver_to_node(dev, "fec_mdio", + name, mdio, &mdiodev); + if (ret) { + printf("%s bind %s failed: %d\n", __func__, name, ret); + break; + } + + /* need to probe it as there is no compatible to do so */ + ret = uclass_get_device_by_ofnode(UCLASS_MDIO, mdio, &mdiodev); + if (!ret) + return 0; + printf("%s probe %s failed: %d\n", __func__, name, ret); + } + + return ret; +} +#endif + static int fecmxc_read_rom_hwaddr(struct udevice *dev) { struct fec_priv *priv = dev_get_priv(dev); @@ -1088,7 +1165,7 @@ static int device_get_phy_addr(struct fec_priv *priv, struct udevice *dev) static int fec_phy_init(struct fec_priv *priv, struct udevice *dev) { - struct phy_device *phydev; + struct phy_device *phydev = NULL; int addr; addr = device_get_phy_addr(priv, dev); @@ -1096,7 +1173,10 @@ static int fec_phy_init(struct fec_priv *priv, struct udevice *dev) addr = CONFIG_FEC_MXC_PHYADDR; #endif - phydev = phy_connect(priv->bus, addr, dev, priv->interface); + if (IS_ENABLED(CONFIG_DM_MDIO)) + phydev = dm_eth_phy_connect(dev); + if (!phydev) + phydev = phy_connect(priv->bus, addr, dev, priv->interface); if (!phydev) return -ENODEV; @@ -1227,6 +1307,12 @@ static int fecmxc_probe(struct udevice *dev) priv->dev_id = dev_seq(dev); + if (IS_ENABLED(CONFIG_DM_MDIO)) { + ret = dm_fec_bind_mdio(dev); + if (ret && ret != -ENODEV) + return ret; + } + #ifdef CONFIG_DM_ETH_PHY bus = eth_phy_get_mdio_bus(dev); #endif -- 2.17.1 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v3 7/8] net: add MV88E61xx DSA driver 2022-05-23 18:25 [PATCH v3 0/8] Add MV88E61xx DSA driver and use on gwventana Tim Harvey ` (5 preceding siblings ...) 2022-05-23 18:25 ` [PATCH v3 6/8] net: fec: add support for DM_MDIO Tim Harvey @ 2022-05-23 18:25 ` Tim Harvey 2022-06-20 11:58 ` Vladimir Oltean 2022-05-23 18:25 ` [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support Tim Harvey 7 siblings, 1 reply; 26+ messages in thread From: Tim Harvey @ 2022-05-23 18:25 UTC (permalink / raw) To: Joe Hershberger, Ramon Fried, Vladimir Oltean, u-boot, Stefano Babic, Fabio Estevam, NXP i . MX U-Boot Team, Marek Behún Cc: Chris Packham, Anatolij Gustschin, Tim Harvey Add a DSA driver for the MV88E61xx compatible GbE Ethernet switches. Cc: Marek Behún <marek.behun@nic.cz> Cc: Vladimir Oltean <vladimir.oltean@nxp.com> Signed-off-by: Tim Harvey <tharvey@gateworks.com> Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- 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 --- 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 family + 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) += mtk_eth.o obj-$(CONFIG_MPC8XX_FEC) += mpc8xx_fec.o obj-$(CONFIG_MT7620_ETH) += mt7620-eth.o obj-$(CONFIG_MT7628_ETH) += mt7628-eth.o +obj-$(CONFIG_MV88E61XX) += mv88e61xx.o obj-$(CONFIG_MVGBE) += mvgbe.o obj-$(CONFIG_MVMDIO) += mvmdio.o obj-$(CONFIG_MVNETA) += 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 <www.gateworks.com> + * Tim Harvey <tharvey@gateworks.com> + * + * (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> + */ + +/* + * 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 <bitfield.h> +#include <common.h> +#include <miiphy.h> +#include <dm/device.h> +#include <dm/device_compat.h> +#include <dm/device-internal.h> +#include <dm/lists.h> +#include <dm/of_extra.h> +#include <linux/delay.h> +#include <net/dsa.h> + +/* 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 indirection + * 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_CLAUSE_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] direction 1=input */ +#define SCRATCH_ADDR_GPIO1DIR 0x63 /* GPIO[14:8] direction 1=input */ +#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 = bitfield_replace(cmd, SMI_CMD_ADDR_SHIFT, SMI_CMD_ADDR_WIDTH, + addr); + cmd = bitfield_replace(cmd, SMI_CMD_REG_SHIFT, SMI_CMD_REG_WIDTH, 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 = 100; + + do { + val = dm_mdio_read(dev->parent, smi_addr, MDIO_DEVAD_NONE, SMI_CMD_REG); + if (val >= 0 && (val & SMI_BUSY) == 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, the device + * address, and the register address. The smi bus address distinguishes it on + * the smi bus from other PHYs or switches. The device address determines + * which on-chip register set you are reading/writing (the various PHYs, 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 when 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 to 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 = dev_get_priv(dev); + int smi_addr = priv->smi_addr; + int res; + + /* In single-chip mode, the device can be addressed directly */ + if (smi_addr == 0) + return dm_mdio_read(dev->parent, addr, MDIO_DEVAD_NONE, reg); + + /* Wait for the bus to become free */ + res = mv88e61xx_smi_wait(dev, smi_addr); + if (res < 0) + return res; + + /* Issue the read command */ + res = 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 = mv88e61xx_smi_wait(dev, smi_addr); + if (res < 0) + return res; + + /* Read the data */ + res = 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 = dev_get_priv(dev); + int smi_addr = priv->smi_addr; + int res; + + /* In single-chip mode, the device can be addressed directly */ + if (smi_addr == 0) + return dm_mdio_write(dev->parent, addr, MDIO_DEVAD_NONE, reg, val); + + /* Wait for the bus to become free */ + res = mv88e61xx_smi_wait(dev, smi_addr); + if (res < 0) + return res; + + /* Set the data to write */ + res = dm_mdio_write(dev->parent, smi_addr, MDIO_DEVAD_NONE, + SMI_DATA_REG, val); + if (res < 0) + return res; + + /* Issue the write command */ + res = 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 = 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 = dev_get_priv(dev); + int val; + u32 timeout = 100; + + do { + val = mv88e61xx_reg_read(dev, priv->global2, GLOBAL2_REG_PHY_CMD); + if (val >= 0 && (val & SMI_BUSY) == 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 = dev_get_priv(dev); + int res; + + /* Issue command to read */ + res = mv88e61xx_reg_write(dev, priv->global2, + GLOBAL2_REG_PHY_CMD, + smi_cmd_read(phyad, reg)); + + /* Wait for data to be read */ + res = 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 = dev_get_priv(dev); + int res; + + /* Set the data to write */ + res = mv88e61xx_reg_write(dev, priv->global2, + GLOBAL2_REG_PHY_DATA, data); + if (res < 0) + return res; + /* Issue the write command */ + res = 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 = 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 = 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 = 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 = 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 = 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 = dev_get_priv(dev); + int time; + int val; + u8 port; + + /* Disable all ports */ + for (port = 0; port < priv->port_count; port++) { + val = mv88e61xx_port_read(dev, port, PORT_REG_CTRL); + if (val < 0) + return val; + val = bitfield_replace(val, PORT_REG_CTRL_PSTATE_SHIFT, + PORT_REG_CTRL_PSTATE_WIDTH, + PORT_REG_CTRL_PSTATE_DISABLED); + val = mv88e61xx_port_write(dev, port, PORT_REG_CTRL, val); + if (val < 0) + return val; + } + + /* Wait 2 ms for queues to drain */ + udelay(2000); + + /* Reset switch */ + val = mv88e61xx_reg_read(dev, priv->global1, GLOBAL1_CTRL); + if (val < 0) + return val; + val |= GLOBAL1_CTRL_SWRESET; + val = 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 = 1000; time; time--) { + val = mv88e61xx_reg_read(dev, priv->global1, GLOBAL1_CTRL); + if (val >= 0 && ((val & GLOBAL1_CTRL_SWRESET) == 0)) + break; + udelay(1000); + } + if (!time) + return -ETIMEDOUT; + + return 0; +} + +static int mv88e61xx_serdes_init(struct udevice *dev) +{ + int val; + + val = mv88e61xx_set_page(dev, DEVADDR_SERDES, PHY_PAGE_SERDES); + if (val < 0) + return val; + + /* Power up serdes module */ + val = mv88e61xx_phy_read(dev, DEVADDR_SERDES, MII_BMCR); + if (val < 0) + return val; + val &= ~(BMCR_PDOWN); + val = 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 = dev_get_uclass_plat(dev); + struct mv88e61xx_priv *priv = dev_get_priv(dev); + int val; + + val = mv88e61xx_port_read(dev, port, PORT_REG_PHYS_CTRL); + if (val < 0) + return val; + + val &= ~(PORT_REG_PHYS_CTRL_SPD_MASK | + PORT_REG_PHYS_CTRL_FC_VALUE | + PORT_REG_PHYS_CTRL_FC_FORCE); + val |= PORT_REG_PHYS_CTRL_FC_FORCE | + PORT_REG_PHYS_CTRL_DUPLEX_VALUE | + PORT_REG_PHYS_CTRL_DUPLEX_FORCE; + + if (priv->id == PORT_SWITCH_ID_6071) { + val |= PORT_REG_PHYS_CTRL_SPD100; + } else { + val |= PORT_REG_PHYS_CTRL_PCS_AN_EN | + PORT_REG_PHYS_CTRL_PCS_AN_RST | + PORT_REG_PHYS_CTRL_SPD1000; + } + + if (port == dsa_pdata->cpu_port) + val |= 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 = 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 = 0x10; + priv->id = mv88e61xx_get_switch_id(dev); + if (priv->id != 0xfff0) { + priv->global1 = 0x1B; + priv->global2 = 0x1C; + return 0; + } + + /* + * Now try via port registers with device address 0x08 + * (88E6020 and compatible switches). + */ + priv->port_reg_base = 0x08; + priv->id = mv88e61xx_get_switch_id(dev); + if (priv->id != 0xfff0) { + priv->global1 = 0x0F; + priv->global2 = 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 devad, 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 devad, + 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 = { + .read = mv88e61xx_mdio_read, + .write = 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) = { + .name = "mv88e61xx_mdio", + .id = UCLASS_MDIO, + .ops = &mv88e61xx_mdio_ops, + .bind = mv88e61xx_mdio_bind, + .priv_auto = sizeof(struct mv88e61xx_priv), + .plat_auto = sizeof(struct mdio_perdev_priv), +}; + +static int mv88e61xx_dsa_port_enable(struct udevice *dev, int port, struct phy_device *phy) +{ + struct dsa_pdata *dsa_pdata = dev_get_uclass_plat(dev); + struct mv88e61xx_priv *priv = 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 = mv88e61xx_phy_read(dev, port, PHY_REG_CTRL1); + if (val < 0) + return val; + val = bitfield_replace(val, priv->phy_ctrl1_en_det_shift, + priv->phy_ctrl1_en_det_width, + priv->phy_ctrl1_en_det_ctrl); + val = mv88e61xx_phy_write(dev, port, PHY_REG_CTRL1, val); + if (val < 0) + return val; + + /* enable port */ + val = mv88e61xx_port_read(dev, port, PORT_REG_CTRL); + if (val < 0) + return val; + val = bitfield_replace(val, PORT_REG_CTRL_PSTATE_SHIFT, + PORT_REG_CTRL_PSTATE_WIDTH, + PORT_REG_CTRL_PSTATE_FORWARD); + val = mv88e61xx_port_write(dev, port, PORT_REG_CTRL, val); + if (val < 0) + return val; + + /* configure RGMII delays for fixed link */ + if (phy->phy_id == PHY_FIXED_ID) { + /* Physical Control register: Table 62 */ + val = mv88e61xx_port_read(dev, port, PORT_REG_PHYS_CTRL); + val &= ~(PORT_REG_PHYS_CTRL_RGMII_DELAY_RXCLK || + PORT_REG_PHYS_CTRL_RGMII_DELAY_TXCLK); + if (phy->interface == PHY_INTERFACE_MODE_RGMII_ID || + phy->interface == PHY_INTERFACE_MODE_RGMII_RXID) + val |= PORT_REG_PHYS_CTRL_RGMII_DELAY_RXCLK; + if (phy->interface == PHY_INTERFACE_MODE_RGMII_ID || + phy->interface == PHY_INTERFACE_MODE_RGMII_TXID) + val |= PORT_REG_PHYS_CTRL_RGMII_DELAY_TXCLK; + val |= BIT(5) | BIT(4); /* Force link */ + ret = mv88e61xx_port_write(dev, port, PORT_REG_PHYS_CTRL, val); + if (ret < 0) + return ret; + + ret = mv88e61xx_fixed_port_setup(dev, port); + if (ret < 0) + return ret; + } + + if (port == dsa_pdata->cpu_port) { + /* Set CPUDest for cpu port */ + val = mv88e61xx_reg_read(dev, priv->global1, GLOBAL1_MON_CTRL); + if (val < 0) + return val; + val = bitfield_replace(val, GLOBAL1_MON_CTRL_CPUDEST_SHIFT, + GLOBAL1_MON_CTRL_CPUDEST_WIDTH, port); + val = mv88e61xx_reg_write(dev, priv->global1, GLOBAL1_MON_CTRL, val); + if (val < 0) + return val; + + if (mv88e61xx_6352_family(dev)) { + /* initialize serdes */ + val = mv88e61xx_get_cmode(dev, dsa_pdata->cpu_port); + if (val < 0) + return val; + if (val == PORT_REG_STATUS_CMODE_100BASE_X || + val == PORT_REG_STATUS_CMODE_1000BASE_X || + val == PORT_REG_STATUS_CMODE_SGMII) { + val = 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 = mv88e61xx_port_read(dev, port, PORT_REG_CTRL); + if (val < 0) + return; + val = bitfield_replace(val, PORT_REG_CTRL_PSTATE_SHIFT, + PORT_REG_CTRL_PSTATE_WIDTH, + PORT_REG_CTRL_PSTATE_DISABLED); + val = mv88e61xx_port_write(dev, port, PORT_REG_CTRL, val); + if (val < 0) + return; +} + +static const struct dsa_ops mv88e61xx_dsa_ops = { + .port_enable = mv88e61xx_dsa_port_enable, + .port_disable = mv88e61xx_dsa_port_disable, +}; + +/* bind and probe the switch mdios */ +static int mv88e61xx_dsa_probe_mdio(struct udevice *dev) +{ + struct udevice *pdev; + ofnode node, mdios; + const char *name; + int ret; + + /* bind phy ports of mdios child node to mv88e61xx_mdio device */ + mdios = dev_read_subnode(dev, "mdios"); + if (ofnode_valid(mdios)) { + ofnode_for_each_subnode(node, mdios) { + name = ofnode_get_name(node); + ret = device_bind_driver_to_node(dev, + "mv88e61xx_mdio", + name, node, &pdev); + if (ret) { + dev_err(dev, "failed to bind %s: %d\n", name, ret); + continue; + } + + /* need to probe it as there is no compatible to do so */ + ret = uclass_get_device_by_ofnode(UCLASS_MDIO, node, &pdev); + if (ret) { + dev_err(dev, "failed to probe %s: %d\n", name, ret); + continue; + } + } + } + + /* bind the mdios node to the parent mdio driver */ + name = ofnode_get_name(mdios); + ret = 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 = uclass_get_device_by_ofnode(UCLASS_MDIO, mdios, &pdev); + if (ret) + dev_err(dev, "failed to probe %s: %d\n", name, ret); + + return ret; +} + +static int mv88e61xx_dsa_probe(struct udevice *dev) +{ + struct mv88e61xx_priv *priv = dev_get_priv(dev); + struct udevice *master = 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 = mv88e61xx_dsa_probe_mdio(dev); + if (ret) + return ret; + + ret = mv88e61xx_priv_reg_offs_pre_init(dev); + if (ret) + return ret; + + dev_dbg(dev, "ID=0x%x PORT_BASE=0x%02x GLOBAL1=0x%02x GLOBAL2=0x%02x\n", + priv->id, priv->port_reg_base, priv->global1, priv->global2); + 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 = 11; + priv->phy_ctrl1_en_det_shift = 8; + priv->phy_ctrl1_en_det_width = 2; + priv->phy_ctrl1_en_det_ctrl = + 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 = 7; + priv->phy_ctrl1_en_det_shift = 14; + priv->phy_ctrl1_en_det_width = 1; + priv->phy_ctrl1_en_det_ctrl = + PHY_REG_CTRL1_ENERGY_DET_SENSE_PULSE; + break; + default: + return -ENODEV; + } + + ret = mv88e61xx_switch_reset(dev); + if (ret < 0) + return ret; + + dsa_set_tagging(dev, 0, 0); + + return 0; +} + +static const struct udevice_id mv88e61xx_ids[] = { + { .compatible = "marvell,mv88e6085" }, + { } +}; + +U_BOOT_DRIVER(mv88e61xx) = { + .name = "mv88e61xx", + .id = UCLASS_DSA, + .of_match = mv88e61xx_ids, + .probe = mv88e61xx_dsa_probe, + .ops = &mv88e61xx_dsa_ops, + .priv_auto = sizeof(struct mv88e61xx_priv), +}; -- 2.17.1 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v3 7/8] net: add MV88E61xx DSA driver 2022-05-23 18:25 ` [PATCH v3 7/8] net: add MV88E61xx DSA driver Tim Harvey @ 2022-06-20 11:58 ` Vladimir Oltean 2022-06-20 23:37 ` Tim Harvey 0 siblings, 1 reply; 26+ messages in thread From: Vladimir Oltean @ 2022-06-20 11:58 UTC (permalink / raw) To: tharvey Cc: Joe Hershberger, Ramon Fried, u-boot, Stefano Babic, Fabio Estevam, dl-uboot-imx, Marek BehĂșn, Chris Packham, Anatolij Gustschin 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Ășn <marek.behun@nic.cz> > Cc: Vladimir Oltean <vladimir.oltean@nxp.com> > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > 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). > --- > 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 family > + 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) += mtk_eth.o > obj-$(CONFIG_MPC8XX_FEC) += mpc8xx_fec.o > obj-$(CONFIG_MT7620_ETH) += mt7620-eth.o > obj-$(CONFIG_MT7628_ETH) += mt7628-eth.o > +obj-$(CONFIG_MV88E61XX) += mv88e61xx.o > obj-$(CONFIG_MVGBE) += mvgbe.o > obj-$(CONFIG_MVMDIO) += mvmdio.o > obj-$(CONFIG_MVNETA) += 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 <http://www.gateworks.com/> > + * Tim Harvey <tharvey@gateworks.com> > + * > + * (C) Copyright 2015 > + * Elecsys Corporation <http://www.elecsyscorp.com/> > + * Kevin Smith <kevin.smith@elecsyscorp.com> > + * > + * Original driver: > + * (C) Copyright 2009 > + * Marvell Semiconductor <http://www.marvell.com/> > + * Prafulla Wadaskar <prafulla@marvell.com> > + */ > + > +/* > + * 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 <bitfield.h> > +#include <common.h> > +#include <miiphy.h> Alphabetic ordering please. > +#include <dm/device.h> > +#include <dm/device_compat.h> > +#include <dm/device-internal.h> > +#include <dm/lists.h> > +#include <dm/of_extra.h> > +#include <linux/delay.h> > +#include <net/dsa.h> > + > +/* 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 indirection > + * 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_CLAUSE_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] direction 1=input */ > +#define SCRATCH_ADDR_GPIO1DIR 0x63 /* GPIO[14:8] direction 1=input */ > +#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 = bitfield_replace(cmd, SMI_CMD_ADDR_SHIFT, SMI_CMD_ADDR_WIDTH, > + addr); > + cmd = bitfield_replace(cmd, SMI_CMD_REG_SHIFT, SMI_CMD_REG_WIDTH, 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 = 100; > + > + do { > + val = dm_mdio_read(dev->parent, smi_addr, MDIO_DEVAD_NONE, SMI_CMD_REG); > + if (val >= 0 && (val & SMI_BUSY) == 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, the device > + * address, and the register address. The smi bus address distinguishes it on > + * the smi bus from other PHYs or switches. The device address determines > + * which on-chip register set you are reading/writing (the various PHYs, 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 when 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 to 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 = dev_get_priv(dev); > + int smi_addr = priv->smi_addr; > + int res; > + > + /* In single-chip mode, the device can be addressed directly */ > + if (smi_addr == 0) > + return dm_mdio_read(dev->parent, addr, MDIO_DEVAD_NONE, reg); > + > + /* Wait for the bus to become free */ > + res = mv88e61xx_smi_wait(dev, smi_addr); > + if (res < 0) > + return res; > + > + /* Issue the read command */ > + res = 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 = mv88e61xx_smi_wait(dev, smi_addr); > + if (res < 0) > + return res; > + > + /* Read the data */ > + res = 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 = dev_get_priv(dev); > + int smi_addr = priv->smi_addr; > + int res; > + > + /* In single-chip mode, the device can be addressed directly */ > + if (smi_addr == 0) > + return dm_mdio_write(dev->parent, addr, MDIO_DEVAD_NONE, reg, val); > + > + /* Wait for the bus to become free */ > + res = mv88e61xx_smi_wait(dev, smi_addr); > + if (res < 0) > + return res; > + > + /* Set the data to write */ > + res = dm_mdio_write(dev->parent, smi_addr, MDIO_DEVAD_NONE, > + SMI_DATA_REG, val); > + if (res < 0) > + return res; > + > + /* Issue the write command */ > + res = 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 = 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 = dev_get_priv(dev); > + int val; > + u32 timeout = 100; > + > + do { > + val = mv88e61xx_reg_read(dev, priv->global2, GLOBAL2_REG_PHY_CMD); > + if (val >= 0 && (val & SMI_BUSY) == 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 = dev_get_priv(dev); > + int res; > + > + /* Issue command to read */ > + res = mv88e61xx_reg_write(dev, priv->global2, > + GLOBAL2_REG_PHY_CMD, > + smi_cmd_read(phyad, reg)); > + > + /* Wait for data to be read */ > + res = 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 = dev_get_priv(dev); > + int res; > + > + /* Set the data to write */ > + res = mv88e61xx_reg_write(dev, priv->global2, > + GLOBAL2_REG_PHY_DATA, data); > + if (res < 0) > + return res; > + /* Issue the write command */ > + res = 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 = 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 = 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 = 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 = 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 = 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 = dev_get_priv(dev); > + int time; > + int val; > + u8 port; > + > + /* Disable all ports */ > + for (port = 0; port < priv->port_count; port++) { > + val = mv88e61xx_port_read(dev, port, PORT_REG_CTRL); > + if (val < 0) > + return val; > + val = bitfield_replace(val, PORT_REG_CTRL_PSTATE_SHIFT, > + PORT_REG_CTRL_PSTATE_WIDTH, > + PORT_REG_CTRL_PSTATE_DISABLED); > + val = mv88e61xx_port_write(dev, port, PORT_REG_CTRL, val); > + if (val < 0) > + return val; > + } > + > + /* Wait 2 ms for queues to drain */ > + udelay(2000); > + > + /* Reset switch */ > + val = mv88e61xx_reg_read(dev, priv->global1, GLOBAL1_CTRL); > + if (val < 0) > + return val; > + val |= GLOBAL1_CTRL_SWRESET; > + val = 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 = 1000; time; time--) { > + val = mv88e61xx_reg_read(dev, priv->global1, GLOBAL1_CTRL); > + if (val >= 0 && ((val & GLOBAL1_CTRL_SWRESET) == 0)) > + break; > + udelay(1000); > + } > + if (!time) > + return -ETIMEDOUT; > + > + return 0; > +} > + > +static int mv88e61xx_serdes_init(struct udevice *dev) > +{ > + int val; > + > + val = mv88e61xx_set_page(dev, DEVADDR_SERDES, PHY_PAGE_SERDES); > + if (val < 0) > + return val; > + > + /* Power up serdes module */ > + val = mv88e61xx_phy_read(dev, DEVADDR_SERDES, MII_BMCR); > + if (val < 0) > + return val; > + val &= ~(BMCR_PDOWN); > + val = 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 = dev_get_uclass_plat(dev); > + struct mv88e61xx_priv *priv = dev_get_priv(dev); > + int val; > + > + val = mv88e61xx_port_read(dev, port, PORT_REG_PHYS_CTRL); > + if (val < 0) > + return val; > + > + val &= ~(PORT_REG_PHYS_CTRL_SPD_MASK | > + PORT_REG_PHYS_CTRL_FC_VALUE | > + PORT_REG_PHYS_CTRL_FC_FORCE); > + val |= PORT_REG_PHYS_CTRL_FC_FORCE | > + PORT_REG_PHYS_CTRL_DUPLEX_VALUE | > + PORT_REG_PHYS_CTRL_DUPLEX_FORCE; > + > + if (priv->id == PORT_SWITCH_ID_6071) { > + val |= PORT_REG_PHYS_CTRL_SPD100; > + } else { > + val |= PORT_REG_PHYS_CTRL_PCS_AN_EN | > + PORT_REG_PHYS_CTRL_PCS_AN_RST | > + PORT_REG_PHYS_CTRL_SPD1000; > + } > + > + if (port == dsa_pdata->cpu_port) > + val |= 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 = 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 = 0x10; > + priv->id = mv88e61xx_get_switch_id(dev); > + if (priv->id != 0xfff0) { > + priv->global1 = 0x1B; > + priv->global2 = 0x1C; > + return 0; > + } > + > + /* > + * Now try via port registers with device address 0x08 > + * (88E6020 and compatible switches). > + */ > + priv->port_reg_base = 0x08; > + priv->id = mv88e61xx_get_switch_id(dev); > + if (priv->id != 0xfff0) { > + priv->global1 = 0x0F; > + priv->global2 = 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 devad, 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 devad, > + 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 = { > + .read = mv88e61xx_mdio_read, > + .write = 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) = { > + .name = "mv88e61xx_mdio", > + .id = UCLASS_MDIO, > + .ops = &mv88e61xx_mdio_ops, > + .bind = mv88e61xx_mdio_bind, > + .priv_auto = sizeof(struct mv88e61xx_priv), > + .plat_auto = sizeof(struct mdio_perdev_priv), > +}; > + > +static int mv88e61xx_dsa_port_enable(struct udevice *dev, int port, struct phy_device *phy) > +{ > + struct dsa_pdata *dsa_pdata = dev_get_uclass_plat(dev); > + struct mv88e61xx_priv *priv = 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 = mv88e61xx_phy_read(dev, port, PHY_REG_CTRL1); > + if (val < 0) > + return val; > + val = bitfield_replace(val, priv->phy_ctrl1_en_det_shift, > + priv->phy_ctrl1_en_det_width, > + priv->phy_ctrl1_en_det_ctrl); > + val = mv88e61xx_phy_write(dev, port, PHY_REG_CTRL1, val); > + if (val < 0) > + return val; > + > + /* enable port */ > + val = mv88e61xx_port_read(dev, port, PORT_REG_CTRL); > + if (val < 0) > + return val; > + val = bitfield_replace(val, PORT_REG_CTRL_PSTATE_SHIFT, > + PORT_REG_CTRL_PSTATE_WIDTH, > + PORT_REG_CTRL_PSTATE_FORWARD); > + val = mv88e61xx_port_write(dev, port, PORT_REG_CTRL, val); > + if (val < 0) > + return val; > + > + /* configure RGMII delays for fixed link */ > + if (phy->phy_id == PHY_FIXED_ID) { > + /* Physical Control register: Table 62 */ > + val = mv88e61xx_port_read(dev, port, PORT_REG_PHYS_CTRL); > + val &= ~(PORT_REG_PHYS_CTRL_RGMII_DELAY_RXCLK || > + PORT_REG_PHYS_CTRL_RGMII_DELAY_TXCLK); > + if (phy->interface == PHY_INTERFACE_MODE_RGMII_ID || > + phy->interface == PHY_INTERFACE_MODE_RGMII_RXID) > + val |= PORT_REG_PHYS_CTRL_RGMII_DELAY_RXCLK; > + if (phy->interface == PHY_INTERFACE_MODE_RGMII_ID || > + phy->interface == PHY_INTERFACE_MODE_RGMII_TXID) > + val |= PORT_REG_PHYS_CTRL_RGMII_DELAY_TXCLK; > + val |= BIT(5) | BIT(4); /* Force link */ > + ret = mv88e61xx_port_write(dev, port, PORT_REG_PHYS_CTRL, val); > + if (ret < 0) > + return ret; > + > + ret = mv88e61xx_fixed_port_setup(dev, port); > + if (ret < 0) > + return ret; > + } > + > + if (port == dsa_pdata->cpu_port) { > + /* Set CPUDest for cpu port */ > + val = mv88e61xx_reg_read(dev, priv->global1, GLOBAL1_MON_CTRL); > + if (val < 0) > + return val; > + val = bitfield_replace(val, GLOBAL1_MON_CTRL_CPUDEST_SHIFT, > + GLOBAL1_MON_CTRL_CPUDEST_WIDTH, port); > + val = mv88e61xx_reg_write(dev, priv->global1, GLOBAL1_MON_CTRL, val); > + if (val < 0) > + return val; > + > + if (mv88e61xx_6352_family(dev)) { > + /* initialize serdes */ > + val = mv88e61xx_get_cmode(dev, dsa_pdata->cpu_port); > + if (val < 0) > + return val; > + if (val == PORT_REG_STATUS_CMODE_100BASE_X || > + val == PORT_REG_STATUS_CMODE_1000BASE_X || > + val == PORT_REG_STATUS_CMODE_SGMII) { > + val = 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 = mv88e61xx_port_read(dev, port, PORT_REG_CTRL); > + if (val < 0) > + return; > + val = bitfield_replace(val, PORT_REG_CTRL_PSTATE_SHIFT, > + PORT_REG_CTRL_PSTATE_WIDTH, > + PORT_REG_CTRL_PSTATE_DISABLED); > + val = mv88e61xx_port_write(dev, port, PORT_REG_CTRL, val); > + if (val < 0) > + return; > +} > + > +static const struct dsa_ops mv88e61xx_dsa_ops = { > + .port_enable = mv88e61xx_dsa_port_enable, > + .port_disable = 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. > +}; > + > +/* 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. > +{ > + struct udevice *pdev; > + ofnode node, mdios; > + const char *name; > + int ret; > + > + /* bind phy ports of mdios child node to mv88e61xx_mdio device */ > + mdios = dev_read_subnode(dev, "mdios"); > + if (ofnode_valid(mdios)) { > + ofnode_for_each_subnode(node, mdios) { > + name = ofnode_get_name(node); > + ret = device_bind_driver_to_node(dev, > + "mv88e61xx_mdio", > + name, node, &pdev); > + if (ret) { > + dev_err(dev, "failed to bind %s: %d\n", name, ret); > + continue; > + } > + > + /* need to probe it as there is no compatible to do so */ > + ret = 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_probe() call? Also, why "pdev" and not "dev"? What does the "p" stand for? > + } > + } > + > + /* bind the mdios node to the parent mdio driver */ > + name = ofnode_get_name(mdios); > + ret = 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 = 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. 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). > +} > + > +static int mv88e61xx_dsa_probe(struct udevice *dev) > +{ > + struct mv88e61xx_priv *priv = dev_get_priv(dev); > + struct udevice *master = 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 = mv88e61xx_dsa_probe_mdio(dev); > + if (ret) > + return ret; > + > + ret = mv88e61xx_priv_reg_offs_pre_init(dev); > + if (ret) > + return ret; > + > + dev_dbg(dev, "ID=0x%x PORT_BASE=0x%02x GLOBAL1=0x%02x GLOBAL2=0x%02x\n", > + priv->id, priv->port_reg_base, priv->global1, priv->global2); > + 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 = 11; > + priv->phy_ctrl1_en_det_shift = 8; > + priv->phy_ctrl1_en_det_width = 2; > + priv->phy_ctrl1_en_det_ctrl = > + 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 = 7; > + priv->phy_ctrl1_en_det_shift = 14; > + priv->phy_ctrl1_en_det_width = 1; > + priv->phy_ctrl1_en_det_ctrl = > + PHY_REG_CTRL1_ENERGY_DET_SENSE_PULSE; > + break; > + default: > + return -ENODEV; > + } > + > + ret = mv88e61xx_switch_reset(dev); > + if (ret < 0) > + return ret; > + > + dsa_set_tagging(dev, 0, 0); > + > + return 0; > +} > + > +static const struct udevice_id mv88e61xx_ids[] = { > + { .compatible = "marvell,mv88e6085" }, > + { } > +}; > + > +U_BOOT_DRIVER(mv88e61xx) = { > + .name = "mv88e61xx", > + .id = UCLASS_DSA, > + .of_match = mv88e61xx_ids, > + .probe = mv88e61xx_dsa_probe, > + .ops = &mv88e61xx_dsa_ops, > + .priv_auto = sizeof(struct mv88e61xx_priv), > +}; > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 7/8] net: add MV88E61xx DSA driver 2022-06-20 11:58 ` Vladimir Oltean @ 2022-06-20 23:37 ` Tim Harvey 2022-06-21 7:21 ` Vladimir Oltean 0 siblings, 1 reply; 26+ messages in thread From: Tim Harvey @ 2022-06-20 23:37 UTC (permalink / raw) To: Vladimir Oltean Cc: Joe Hershberger, Ramon Fried, u-boot, Stefano Babic, Fabio Estevam, dl-uboot-imx, Marek BehĂșn, Chris Packham, Anatolij Gustschin t On Mon, Jun 20, 2022 at 4:58 AM Vladimir Oltean <vladimir.oltean@nxp.com> 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Ășn <marek.behun@nic.cz> > > Cc: Vladimir Oltean <vladimir.oltean@nxp.com> > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com> > > --- > > 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 family > > + 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) += mtk_eth.o > > obj-$(CONFIG_MPC8XX_FEC) += mpc8xx_fec.o > > obj-$(CONFIG_MT7620_ETH) += mt7620-eth.o > > obj-$(CONFIG_MT7628_ETH) += mt7628-eth.o > > +obj-$(CONFIG_MV88E61XX) += mv88e61xx.o > > obj-$(CONFIG_MVGBE) += mvgbe.o > > obj-$(CONFIG_MVMDIO) += mvmdio.o > > obj-$(CONFIG_MVNETA) += 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 <http://www.gateworks.com/> > > + * Tim Harvey <tharvey@gateworks.com> > > + * > > + * (C) Copyright 2015 > > + * Elecsys Corporation <http://www.elecsyscorp.com/> > > + * Kevin Smith <kevin.smith@elecsyscorp.com> > > + * > > + * Original driver: > > + * (C) Copyright 2009 > > + * Marvell Semiconductor <http://www.marvell.com/> > > + * Prafulla Wadaskar <prafulla@marvell.com> > > + */ > > + > > +/* > > + * 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 <bitfield.h> > > +#include <common.h> > > +#include <miiphy.h> > > Alphabetic ordering please. > ok > > +#include <dm/device.h> > > +#include <dm/device_compat.h> > > +#include <dm/device-internal.h> > > +#include <dm/lists.h> > > +#include <dm/of_extra.h> > > +#include <linux/delay.h> > > +#include <net/dsa.h> > > + > > +/* 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 indirection > > + * 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_CLAUSE_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] direction 1=input */ > > +#define SCRATCH_ADDR_GPIO1DIR 0x63 /* GPIO[14:8] direction 1=input */ > > +#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 = bitfield_replace(cmd, SMI_CMD_ADDR_SHIFT, SMI_CMD_ADDR_WIDTH, > > + addr); > > + cmd = bitfield_replace(cmd, SMI_CMD_REG_SHIFT, SMI_CMD_REG_WIDTH, 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 = 100; > > + > > + do { > > + val = dm_mdio_read(dev->parent, smi_addr, MDIO_DEVAD_NONE, SMI_CMD_REG); > > + if (val >= 0 && (val & SMI_BUSY) == 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, the device > > + * address, and the register address. The smi bus address distinguishes it on > > + * the smi bus from other PHYs or switches. The device address determines > > + * which on-chip register set you are reading/writing (the various PHYs, 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 when 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 to 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 = dev_get_priv(dev); > > + int smi_addr = priv->smi_addr; > > + int res; > > + > > + /* In single-chip mode, the device can be addressed directly */ > > + if (smi_addr == 0) > > + return dm_mdio_read(dev->parent, addr, MDIO_DEVAD_NONE, reg); > > + > > + /* Wait for the bus to become free */ > > + res = mv88e61xx_smi_wait(dev, smi_addr); > > + if (res < 0) > > + return res; > > + > > + /* Issue the read command */ > > + res = 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 = mv88e61xx_smi_wait(dev, smi_addr); > > + if (res < 0) > > + return res; > > + > > + /* Read the data */ > > + res = 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 = dev_get_priv(dev); > > + int smi_addr = priv->smi_addr; > > + int res; > > + > > + /* In single-chip mode, the device can be addressed directly */ > > + if (smi_addr == 0) > > + return dm_mdio_write(dev->parent, addr, MDIO_DEVAD_NONE, reg, val); > > + > > + /* Wait for the bus to become free */ > > + res = mv88e61xx_smi_wait(dev, smi_addr); > > + if (res < 0) > > + return res; > > + > > + /* Set the data to write */ > > + res = dm_mdio_write(dev->parent, smi_addr, MDIO_DEVAD_NONE, > > + SMI_DATA_REG, val); > > + if (res < 0) > > + return res; > > + > > + /* Issue the write command */ > > + res = 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 = 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 = dev_get_priv(dev); > > + int val; > > + u32 timeout = 100; > > + > > + do { > > + val = mv88e61xx_reg_read(dev, priv->global2, GLOBAL2_REG_PHY_CMD); > > + if (val >= 0 && (val & SMI_BUSY) == 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 = dev_get_priv(dev); > > + int res; > > + > > + /* Issue command to read */ > > + res = mv88e61xx_reg_write(dev, priv->global2, > > + GLOBAL2_REG_PHY_CMD, > > + smi_cmd_read(phyad, reg)); > > + > > + /* Wait for data to be read */ > > + res = 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 = dev_get_priv(dev); > > + int res; > > + > > + /* Set the data to write */ > > + res = mv88e61xx_reg_write(dev, priv->global2, > > + GLOBAL2_REG_PHY_DATA, data); > > + if (res < 0) > > + return res; > > + /* Issue the write command */ > > + res = 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 = 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 = 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 = 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 = 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 = 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 = dev_get_priv(dev); > > + int time; > > + int val; > > + u8 port; > > + > > + /* Disable all ports */ > > + for (port = 0; port < priv->port_count; port++) { > > + val = mv88e61xx_port_read(dev, port, PORT_REG_CTRL); > > + if (val < 0) > > + return val; > > + val = bitfield_replace(val, PORT_REG_CTRL_PSTATE_SHIFT, > > + PORT_REG_CTRL_PSTATE_WIDTH, > > + PORT_REG_CTRL_PSTATE_DISABLED); > > + val = mv88e61xx_port_write(dev, port, PORT_REG_CTRL, val); > > + if (val < 0) > > + return val; > > + } > > + > > + /* Wait 2 ms for queues to drain */ > > + udelay(2000); > > + > > + /* Reset switch */ > > + val = mv88e61xx_reg_read(dev, priv->global1, GLOBAL1_CTRL); > > + if (val < 0) > > + return val; > > + val |= GLOBAL1_CTRL_SWRESET; > > + val = 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 = 1000; time; time--) { > > + val = mv88e61xx_reg_read(dev, priv->global1, GLOBAL1_CTRL); > > + if (val >= 0 && ((val & GLOBAL1_CTRL_SWRESET) == 0)) > > + break; > > + udelay(1000); > > + } > > + if (!time) > > + return -ETIMEDOUT; > > + > > + return 0; > > +} > > + > > +static int mv88e61xx_serdes_init(struct udevice *dev) > > +{ > > + int val; > > + > > + val = mv88e61xx_set_page(dev, DEVADDR_SERDES, PHY_PAGE_SERDES); > > + if (val < 0) > > + return val; > > + > > + /* Power up serdes module */ > > + val = mv88e61xx_phy_read(dev, DEVADDR_SERDES, MII_BMCR); > > + if (val < 0) > > + return val; > > + val &= ~(BMCR_PDOWN); > > + val = 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 = dev_get_uclass_plat(dev); > > + struct mv88e61xx_priv *priv = dev_get_priv(dev); > > + int val; > > + > > + val = mv88e61xx_port_read(dev, port, PORT_REG_PHYS_CTRL); > > + if (val < 0) > > + return val; > > + > > + val &= ~(PORT_REG_PHYS_CTRL_SPD_MASK | > > + PORT_REG_PHYS_CTRL_FC_VALUE | > > + PORT_REG_PHYS_CTRL_FC_FORCE); > > + val |= PORT_REG_PHYS_CTRL_FC_FORCE | > > + PORT_REG_PHYS_CTRL_DUPLEX_VALUE | > > + PORT_REG_PHYS_CTRL_DUPLEX_FORCE; > > + > > + if (priv->id == PORT_SWITCH_ID_6071) { > > + val |= PORT_REG_PHYS_CTRL_SPD100; > > + } else { > > + val |= PORT_REG_PHYS_CTRL_PCS_AN_EN | > > + PORT_REG_PHYS_CTRL_PCS_AN_RST | > > + PORT_REG_PHYS_CTRL_SPD1000; > > + } > > + > > + if (port == dsa_pdata->cpu_port) > > + val |= 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 = 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 = 0x10; > > + priv->id = mv88e61xx_get_switch_id(dev); > > + if (priv->id != 0xfff0) { > > + priv->global1 = 0x1B; > > + priv->global2 = 0x1C; > > + return 0; > > + } > > + > > + /* > > + * Now try via port registers with device address 0x08 > > + * (88E6020 and compatible switches). > > + */ > > + priv->port_reg_base = 0x08; > > + priv->id = mv88e61xx_get_switch_id(dev); > > + if (priv->id != 0xfff0) { > > + priv->global1 = 0x0F; > > + priv->global2 = 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 devad, 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 devad, > > + 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 = { > > + .read = mv88e61xx_mdio_read, > > + .write = 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) = { > > + .name = "mv88e61xx_mdio", > > + .id = UCLASS_MDIO, > > + .ops = &mv88e61xx_mdio_ops, > > + .bind = mv88e61xx_mdio_bind, > > + .priv_auto = sizeof(struct mv88e61xx_priv), > > + .plat_auto = sizeof(struct mdio_perdev_priv), > > +}; > > + > > +static int mv88e61xx_dsa_port_enable(struct udevice *dev, int port, struct phy_device *phy) > > +{ > > + struct dsa_pdata *dsa_pdata = dev_get_uclass_plat(dev); > > + struct mv88e61xx_priv *priv = 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 = mv88e61xx_phy_read(dev, port, PHY_REG_CTRL1); > > + if (val < 0) > > + return val; > > + val = bitfield_replace(val, priv->phy_ctrl1_en_det_shift, > > + priv->phy_ctrl1_en_det_width, > > + priv->phy_ctrl1_en_det_ctrl); > > + val = mv88e61xx_phy_write(dev, port, PHY_REG_CTRL1, val); > > + if (val < 0) > > + return val; > > + > > + /* enable port */ > > + val = mv88e61xx_port_read(dev, port, PORT_REG_CTRL); > > + if (val < 0) > > + return val; > > + val = bitfield_replace(val, PORT_REG_CTRL_PSTATE_SHIFT, > > + PORT_REG_CTRL_PSTATE_WIDTH, > > + PORT_REG_CTRL_PSTATE_FORWARD); > > + val = mv88e61xx_port_write(dev, port, PORT_REG_CTRL, val); > > + if (val < 0) > > + return val; > > + > > + /* configure RGMII delays for fixed link */ > > + if (phy->phy_id == PHY_FIXED_ID) { > > + /* Physical Control register: Table 62 */ > > + val = mv88e61xx_port_read(dev, port, PORT_REG_PHYS_CTRL); > > + val &= ~(PORT_REG_PHYS_CTRL_RGMII_DELAY_RXCLK || > > + PORT_REG_PHYS_CTRL_RGMII_DELAY_TXCLK); > > + if (phy->interface == PHY_INTERFACE_MODE_RGMII_ID || > > + phy->interface == PHY_INTERFACE_MODE_RGMII_RXID) > > + val |= PORT_REG_PHYS_CTRL_RGMII_DELAY_RXCLK; > > + if (phy->interface == PHY_INTERFACE_MODE_RGMII_ID || > > + phy->interface == PHY_INTERFACE_MODE_RGMII_TXID) > > + val |= PORT_REG_PHYS_CTRL_RGMII_DELAY_TXCLK; > > + val |= BIT(5) | BIT(4); /* Force link */ > > + ret = mv88e61xx_port_write(dev, port, PORT_REG_PHYS_CTRL, val); > > + if (ret < 0) > > + return ret; > > + > > + ret = mv88e61xx_fixed_port_setup(dev, port); > > + if (ret < 0) > > + return ret; > > + } > > + > > + if (port == dsa_pdata->cpu_port) { > > + /* Set CPUDest for cpu port */ > > + val = mv88e61xx_reg_read(dev, priv->global1, GLOBAL1_MON_CTRL); > > + if (val < 0) > > + return val; > > + val = bitfield_replace(val, GLOBAL1_MON_CTRL_CPUDEST_SHIFT, > > + GLOBAL1_MON_CTRL_CPUDEST_WIDTH, port); > > + val = mv88e61xx_reg_write(dev, priv->global1, GLOBAL1_MON_CTRL, val); > > + if (val < 0) > > + return val; > > + > > + if (mv88e61xx_6352_family(dev)) { > > + /* initialize serdes */ > > + val = mv88e61xx_get_cmode(dev, dsa_pdata->cpu_port); > > + if (val < 0) > > + return val; > > + if (val == PORT_REG_STATUS_CMODE_100BASE_X || > > + val == PORT_REG_STATUS_CMODE_1000BASE_X || > > + val == PORT_REG_STATUS_CMODE_SGMII) { > > + val = 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 = mv88e61xx_port_read(dev, port, PORT_REG_CTRL); > > + if (val < 0) > > + return; > > + val = bitfield_replace(val, PORT_REG_CTRL_PSTATE_SHIFT, > > + PORT_REG_CTRL_PSTATE_WIDTH, > > + PORT_REG_CTRL_PSTATE_DISABLED); > > + val = mv88e61xx_port_write(dev, port, PORT_REG_CTRL, val); > > + if (val < 0) > > + return; > > +} > > + > > +static const struct dsa_ops mv88e61xx_dsa_ops = { > > + .port_enable = mv88e61xx_dsa_port_enable, > > + .port_disable = 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 = dev_read_subnode(dev, "mdios"); > > + if (ofnode_valid(mdios)) { > > + ofnode_for_each_subnode(node, mdios) { > > + name = ofnode_get_name(node); > > + ret = device_bind_driver_to_node(dev, > > + "mv88e61xx_mdio", > > + name, node, &pdev); > > + if (ret) { > > + dev_err(dev, "failed to bind %s: %d\n", name, ret); > > + continue; > > + } > > + > > + /* need to probe it as there is no compatible to do so */ > > + ret = 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_probe() 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 = ofnode_get_name(mdios); > > + ret = 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 = 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 = dev_get_priv(dev); > > + struct udevice *master = 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 = mv88e61xx_dsa_probe_mdio(dev); > > + if (ret) > > + return ret; > > + > > + ret = mv88e61xx_priv_reg_offs_pre_init(dev); > > + if (ret) > > + return ret; > > + > > + dev_dbg(dev, "ID=0x%x PORT_BASE=0x%02x GLOBAL1=0x%02x GLOBAL2=0x%02x\n", > > + priv->id, priv->port_reg_base, priv->global1, priv->global2); > > + 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 = 11; > > + priv->phy_ctrl1_en_det_shift = 8; > > + priv->phy_ctrl1_en_det_width = 2; > > + priv->phy_ctrl1_en_det_ctrl = > > + 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 = 7; > > + priv->phy_ctrl1_en_det_shift = 14; > > + priv->phy_ctrl1_en_det_width = 1; > > + priv->phy_ctrl1_en_det_ctrl = > > + PHY_REG_CTRL1_ENERGY_DET_SENSE_PULSE; > > + break; > > + default: > > + return -ENODEV; > > + } > > + > > + ret = mv88e61xx_switch_reset(dev); > > + if (ret < 0) > > + return ret; > > + > > + dsa_set_tagging(dev, 0, 0); > > + > > + return 0; > > +} > > + > > +static const struct udevice_id mv88e61xx_ids[] = { > > + { .compatible = "marvell,mv88e6085" }, > > + { } > > +}; > > + > > +U_BOOT_DRIVER(mv88e61xx) = { > > + .name = "mv88e61xx", > > + .id = UCLASS_DSA, > > + .of_match = mv88e61xx_ids, > > + .probe = mv88e61xx_dsa_probe, > > + .ops = &mv88e61xx_dsa_ops, > > + .priv_auto = sizeof(struct mv88e61xx_priv), > > +}; > > -- > > 2.17.1 > > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 7/8] net: add MV88E61xx DSA driver 2022-06-20 23:37 ` Tim Harvey @ 2022-06-21 7:21 ` Vladimir Oltean 2022-06-21 15:11 ` Tim Harvey 0 siblings, 1 reply; 26+ messages in thread From: Vladimir Oltean @ 2022-06-21 7:21 UTC (permalink / raw) To: tharvey Cc: Joe Hershberger, Ramon Fried, u-boot, Stefano Babic, Fabio Estevam, dl-uboot-imx, Marek BehĂșn, Chris Packham, Anatolij Gustschin On Mon, Jun 20, 2022 at 04:37:45PM -0700, Tim Harvey wrote: > On Mon, Jun 20, 2022 at 4:58 AM Vladimir Oltean > <vladimir.oltean@nxp.com> wrote: > > > > On Mon, May 23, 2022 at 11:25:48AM -0700, Tim Harvey wrote: > > > +/* bind and probe the switch mdios */ > > > +static int mv88e61xx_dsa_probe_mdio(struct udevice *dev) > > > +{ > > > + struct udevice *pdev; > > > + ofnode node, mdios; > > > + const char *name; > > > + int ret; > > > + > > > + /* bind phy ports of mdios child node to mv88e61xx_mdio device */ > > > + mdios = dev_read_subnode(dev, "mdios"); > > > + if (ofnode_valid(mdios)) { > > > + ofnode_for_each_subnode(node, mdios) { > > > + name = ofnode_get_name(node); > > > + ret = device_bind_driver_to_node(dev, > > > + "mv88e61xx_mdio", > > > + name, node, &pdev); > > > + if (ret) { > > > + dev_err(dev, "failed to bind %s: %d\n", name, ret); > > > + continue; > > > + } > > > + > > > + /* need to probe it as there is no compatible to do so */ > > > + ret = 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_probe() 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. Yes, but those under the mdios node aren't ports, they're MDIO controllers, hence my comment. > 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 Ok, I didn't notice they all call uclass_get_device_tail() which calls device_probe(). ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 7/8] net: add MV88E61xx DSA driver 2022-06-21 7:21 ` Vladimir Oltean @ 2022-06-21 15:11 ` Tim Harvey 2022-06-23 12:43 ` Vladimir Oltean 0 siblings, 1 reply; 26+ messages in thread From: Tim Harvey @ 2022-06-21 15:11 UTC (permalink / raw) To: Vladimir Oltean Cc: Joe Hershberger, Ramon Fried, u-boot, Stefano Babic, Fabio Estevam, dl-uboot-imx, Marek BehĂșn, Chris Packham, Anatolij Gustschin On Tue, Jun 21, 2022 at 12:21 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > On Mon, Jun 20, 2022 at 04:37:45PM -0700, Tim Harvey wrote: > > On Mon, Jun 20, 2022 at 4:58 AM Vladimir Oltean > > <vladimir.oltean@nxp.com> wrote: > > > > > > On Mon, May 23, 2022 at 11:25:48AM -0700, Tim Harvey wrote: > > > > +/* bind and probe the switch mdios */ > > > > +static int mv88e61xx_dsa_probe_mdio(struct udevice *dev) > > > > +{ > > > > + struct udevice *pdev; > > > > + ofnode node, mdios; > > > > + const char *name; > > > > + int ret; > > > > + > > > > + /* bind phy ports of mdios child node to mv88e61xx_mdio device */ > > > > + mdios = dev_read_subnode(dev, "mdios"); > > > > + if (ofnode_valid(mdios)) { > > > > + ofnode_for_each_subnode(node, mdios) { > > > > + name = ofnode_get_name(node); > > > > + ret = device_bind_driver_to_node(dev, > > > > + "mv88e61xx_mdio", > > > > + name, node, &pdev); > > > > + if (ret) { > > > > + dev_err(dev, "failed to bind %s: %d\n", name, ret); > > > > + continue; > > > > + } > > > > + > > > > + /* need to probe it as there is no compatible to do so */ > > > > + ret = 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_probe() 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. > > Yes, but those under the mdios node aren't ports, they're MDIO > controllers, hence my comment. how about devp (dev pointer) or subdev or mdio? Tim > > > 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 > > Ok, I didn't notice they all call uclass_get_device_tail() which calls > device_probe(). ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 7/8] net: add MV88E61xx DSA driver 2022-06-21 15:11 ` Tim Harvey @ 2022-06-23 12:43 ` Vladimir Oltean 2022-08-07 0:07 ` Ramon Fried 0 siblings, 1 reply; 26+ messages in thread From: Vladimir Oltean @ 2022-06-23 12:43 UTC (permalink / raw) To: tharvey Cc: Joe Hershberger, Ramon Fried, u-boot, Stefano Babic, Fabio Estevam, dl-uboot-imx, Marek BehĂșn, Chris Packham, Anatolij Gustschin On Tue, Jun 21, 2022 at 08:11:06AM -0700, Tim Harvey wrote: > On Tue, Jun 21, 2022 at 12:21 AM Vladimir Oltean > <vladimir.oltean@nxp.com> wrote: > > > > On Mon, Jun 20, 2022 at 04:37:45PM -0700, Tim Harvey wrote: > > > On Mon, Jun 20, 2022 at 4:58 AM Vladimir Oltean > > > <vladimir.oltean@nxp.com> wrote: > > > > > > > > On Mon, May 23, 2022 at 11:25:48AM -0700, Tim Harvey wrote: > > > > > +/* bind and probe the switch mdios */ > > > > > +static int mv88e61xx_dsa_probe_mdio(struct udevice *dev) > > > > > +{ > > > > > + struct udevice *pdev; > > > > > + ofnode node, mdios; > > > > > + const char *name; > > > > > + int ret; > > > > > + > > > > > + /* bind phy ports of mdios child node to mv88e61xx_mdio device */ > > > > > + mdios = dev_read_subnode(dev, "mdios"); > > > > > + if (ofnode_valid(mdios)) { > > > > > + ofnode_for_each_subnode(node, mdios) { > > > > > + name = ofnode_get_name(node); > > > > > + ret = device_bind_driver_to_node(dev, > > > > > + "mv88e61xx_mdio", > > > > > + name, node, &pdev); > > > > > + if (ret) { > > > > > + dev_err(dev, "failed to bind %s: %d\n", name, ret); > > > > > + continue; > > > > > + } > > > > > + > > > > > + /* need to probe it as there is no compatible to do so */ > > > > > + ret = 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_probe() 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. > > > > Yes, but those under the mdios node aren't ports, they're MDIO > > controllers, hence my comment. > > how about devp (dev pointer) or subdev or mdio? The terminology problem I have with "mdio" is that it would be a struct udevice *, whereas the plural variable, "mdios", is an ofnode. How about mdev, for mdio device? ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 7/8] net: add MV88E61xx DSA driver 2022-06-23 12:43 ` Vladimir Oltean @ 2022-08-07 0:07 ` Ramon Fried 2022-08-08 15:21 ` Tim Harvey 0 siblings, 1 reply; 26+ messages in thread From: Ramon Fried @ 2022-08-07 0:07 UTC (permalink / raw) To: Vladimir Oltean Cc: tharvey, Joe Hershberger, u-boot, Stefano Babic, Fabio Estevam, dl-uboot-imx, Marek BehĂșn, Chris Packham, Anatolij Gustschin On Thu, Jun 23, 2022 at 3:43 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > On Tue, Jun 21, 2022 at 08:11:06AM -0700, Tim Harvey wrote: > > On Tue, Jun 21, 2022 at 12:21 AM Vladimir Oltean > > <vladimir.oltean@nxp.com> wrote: > > > > > > On Mon, Jun 20, 2022 at 04:37:45PM -0700, Tim Harvey wrote: > > > > On Mon, Jun 20, 2022 at 4:58 AM Vladimir Oltean > > > > <vladimir.oltean@nxp.com> wrote: > > > > > > > > > > On Mon, May 23, 2022 at 11:25:48AM -0700, Tim Harvey wrote: > > > > > > +/* bind and probe the switch mdios */ > > > > > > +static int mv88e61xx_dsa_probe_mdio(struct udevice *dev) > > > > > > +{ > > > > > > + struct udevice *pdev; > > > > > > + ofnode node, mdios; > > > > > > + const char *name; > > > > > > + int ret; > > > > > > + > > > > > > + /* bind phy ports of mdios child node to mv88e61xx_mdio device */ > > > > > > + mdios = dev_read_subnode(dev, "mdios"); > > > > > > + if (ofnode_valid(mdios)) { > > > > > > + ofnode_for_each_subnode(node, mdios) { > > > > > > + name = ofnode_get_name(node); > > > > > > + ret = device_bind_driver_to_node(dev, > > > > > > + "mv88e61xx_mdio", > > > > > > + name, node, &pdev); > > > > > > + if (ret) { > > > > > > + dev_err(dev, "failed to bind %s: %d\n", name, ret); > > > > > > + continue; > > > > > > + } > > > > > > + > > > > > > + /* need to probe it as there is no compatible to do so */ > > > > > > + ret = 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_probe() 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. > > > > > > Yes, but those under the mdios node aren't ports, they're MDIO > > > controllers, hence my comment. > > > > how about devp (dev pointer) or subdev or mdio? > > The terminology problem I have with "mdio" is that it would be a > struct udevice *, whereas the plural variable, "mdios", is an ofnode. > How about mdev, for mdio device? Tim ? ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 7/8] net: add MV88E61xx DSA driver 2022-08-07 0:07 ` Ramon Fried @ 2022-08-08 15:21 ` Tim Harvey 0 siblings, 0 replies; 26+ messages in thread From: Tim Harvey @ 2022-08-08 15:21 UTC (permalink / raw) To: Ramon Fried Cc: Vladimir Oltean, Joe Hershberger, u-boot, Stefano Babic, Fabio Estevam, dl-uboot-imx, Marek BehĂșn, Chris Packham, Anatolij Gustschin On Sat, Aug 6, 2022 at 5:07 PM Ramon Fried <rfried.dev@gmail.com> wrote: > > On Thu, Jun 23, 2022 at 3:43 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > > > On Tue, Jun 21, 2022 at 08:11:06AM -0700, Tim Harvey wrote: > > > On Tue, Jun 21, 2022 at 12:21 AM Vladimir Oltean > > > <vladimir.oltean@nxp.com> wrote: > > > > > > > > On Mon, Jun 20, 2022 at 04:37:45PM -0700, Tim Harvey wrote: > > > > > On Mon, Jun 20, 2022 at 4:58 AM Vladimir Oltean > > > > > <vladimir.oltean@nxp.com> wrote: > > > > > > > > > > > > On Mon, May 23, 2022 at 11:25:48AM -0700, Tim Harvey wrote: > > > > > > > +/* bind and probe the switch mdios */ > > > > > > > +static int mv88e61xx_dsa_probe_mdio(struct udevice *dev) > > > > > > > +{ > > > > > > > + struct udevice *pdev; > > > > > > > + ofnode node, mdios; > > > > > > > + const char *name; > > > > > > > + int ret; > > > > > > > + > > > > > > > + /* bind phy ports of mdios child node to mv88e61xx_mdio device */ > > > > > > > + mdios = dev_read_subnode(dev, "mdios"); > > > > > > > + if (ofnode_valid(mdios)) { > > > > > > > + ofnode_for_each_subnode(node, mdios) { > > > > > > > + name = ofnode_get_name(node); > > > > > > > + ret = device_bind_driver_to_node(dev, > > > > > > > + "mv88e61xx_mdio", > > > > > > > + name, node, &pdev); > > > > > > > + if (ret) { > > > > > > > + dev_err(dev, "failed to bind %s: %d\n", name, ret); > > > > > > > + continue; > > > > > > > + } > > > > > > > + > > > > > > > + /* need to probe it as there is no compatible to do so */ > > > > > > > + ret = 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_probe() 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. > > > > > > > > Yes, but those under the mdios node aren't ports, they're MDIO > > > > controllers, hence my comment. > > > > > > how about devp (dev pointer) or subdev or mdio? > > > > The terminology problem I have with "mdio" is that it would be a > > struct udevice *, whereas the plural variable, "mdios", is an ofnode. > > How about mdev, for mdio device? > Tim ? Hi Ramon, I haven't had time to get back to this yet. I hope to do so in the next few weeks. Best Regards, Tim ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support 2022-05-23 18:25 [PATCH v3 0/8] Add MV88E61xx DSA driver and use on gwventana Tim Harvey ` (6 preceding siblings ...) 2022-05-23 18:25 ` [PATCH v3 7/8] net: add MV88E61xx DSA driver Tim Harvey @ 2022-05-23 18:25 ` Tim Harvey 2022-06-14 17:00 ` Tim Harvey ` (2 more replies) 7 siblings, 3 replies; 26+ messages in thread From: Tim Harvey @ 2022-05-23 18:25 UTC (permalink / raw) To: Joe Hershberger, Ramon Fried, Vladimir Oltean, u-boot, Stefano Babic, Fabio Estevam, NXP i . MX U-Boot Team, Marek Behún Cc: Chris Packham, Anatolij Gustschin, Tim Harvey Add MV88E61XX DSA support: - update dt: U-Boot dsa driver requires different device-tree syntax than the linux driver in order to link the dsa ports to the mdio bus. - update defconfig - replace mv88e61xx_hw_reset weak override with board_phy_config support for mv88e61xx configuration that is outside the scope of the DSA driver Signed-off-by: Tim Harvey <tharvey@gateworks.com> --- v3: - move mdio's mdio@0 subnode v2: no changes --- arch/arm/dts/imx6qdl-gw5904.dtsi | 41 ++++++++++++++++++++ board/gateworks/gw_ventana/gw_ventana.c | 50 +++++++++---------------- configs/gwventana_gw5904_defconfig | 7 ++-- 3 files changed, 62 insertions(+), 36 deletions(-) diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi b/arch/arm/dts/imx6qdl-gw5904.dtsi index 286c7a9924c2..1b2f70d1ccb2 100644 --- a/arch/arm/dts/imx6qdl-gw5904.dtsi +++ b/arch/arm/dts/imx6qdl-gw5904.dtsi @@ -219,6 +219,33 @@ compatible = "marvell,mv88e6085"; reg = <0>; + mdios { + #address-cells = <1>; + #size-cells = <0>; + + mdio@0 { + reg = <0>; + #address-cells = <1>; + #size-cells = <0>; + + sw_phy0: ethernet-phy@0 { + reg = <0x0>; + }; + + sw_phy1: ethernet-phy@1 { + reg = <0x1>; + }; + + sw_phy2: ethernet-phy@2 { + reg = <0x2>; + }; + + sw_phy3: ethernet-phy@3 { + reg = <0x3>; + }; + }; + }; + ports { #address-cells = <1>; #size-cells = <0>; @@ -226,27 +253,41 @@ port@0 { reg = <0>; label = "lan4"; + phy-mode = "internal"; + phy-handle = <&sw_phy0>; }; port@1 { reg = <1>; label = "lan3"; + phy-mode = "internal"; + phy-handle = <&sw_phy1>; }; port@2 { reg = <2>; label = "lan2"; + phy-mode = "internal"; + phy-handle = <&sw_phy2>; }; port@3 { reg = <3>; label = "lan1"; + phy-mode = "internal"; + phy-handle = <&sw_phy3>; }; port@5 { reg = <5>; label = "cpu"; ethernet = <&fec>; + phy-mode = "rgmii-id"; + + fixed-link { + speed = <1000>; + full-duplex; + }; }; }; }; diff --git a/board/gateworks/gw_ventana/gw_ventana.c b/board/gateworks/gw_ventana/gw_ventana.c index c06630a66b66..bef3f7ef0d2b 100644 --- a/board/gateworks/gw_ventana/gw_ventana.c +++ b/board/gateworks/gw_ventana/gw_ventana.c @@ -68,44 +68,30 @@ int board_phy_config(struct phy_device *phydev) phy_write(phydev, MDIO_DEVAD_NONE, 14, val); } + /* Fixed PHY: for GW5904/GW5909 this is Marvell 88E6176 GbE Switch */ + else if (phydev->phy_id == 0xa5a55a5a && + ((board_type == GW5904) || (board_type == GW5909))) { + struct mii_dev *bus = miiphy_get_dev_by_name("mdio"); + + puts("MV88E61XX "); + /* GPIO[0] output CLK125 for RGMII_REFCLK */ + bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x62 << 8) | 0xfe); + bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x68 << 8) | 7); + + /* Port 0-3 LED configuration: Table 80/82 */ + /* LED configuration: 7:4-green (8=Activity) 3:0 amber (8=Link) */ + bus->write(bus, 0x10, 0, 0x16, 0x8088); + bus->write(bus, 0x11, 0, 0x16, 0x8088); + bus->write(bus, 0x12, 0, 0x16, 0x8088); + bus->write(bus, 0x13, 0, 0x16, 0x8088); + } + if (phydev->drv->config) phydev->drv->config(phydev); return 0; } -#ifdef CONFIG_MV88E61XX_SWITCH -int mv88e61xx_hw_reset(struct phy_device *phydev) -{ - struct mii_dev *bus = phydev->bus; - - /* GPIO[0] output, CLK125 */ - debug("enabling RGMII_REFCLK\n"); - bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0, - 0x1a /*MV_SCRATCH_MISC*/, - (1 << 15) | (0x62 /*MV_GPIO_DIR*/ << 8) | 0xfe); - bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0, - 0x1a /*MV_SCRATCH_MISC*/, - (1 << 15) | (0x68 /*MV_GPIO01_CNTL*/ << 8) | 7); - - /* RGMII delay - Physical Control register bit[15:14] */ - debug("setting port%d RGMII rx/tx delay\n", CONFIG_MV88E61XX_CPU_PORT); - /* forced 1000mbps full-duplex link */ - bus->write(bus, 0x10 + CONFIG_MV88E61XX_CPU_PORT, 0, 1, 0xc0fe); - phydev->autoneg = AUTONEG_DISABLE; - phydev->speed = SPEED_1000; - phydev->duplex = DUPLEX_FULL; - - /* LED configuration: 7:4-green (8=Activity) 3:0 amber (8=Link) */ - bus->write(bus, 0x10, 0, 0x16, 0x8088); - bus->write(bus, 0x11, 0, 0x16, 0x8088); - bus->write(bus, 0x12, 0, 0x16, 0x8088); - bus->write(bus, 0x13, 0, 0x16, 0x8088); - - return 0; -} -#endif // CONFIG_MV88E61XX_SWITCH - #if defined(CONFIG_VIDEO_IPUV3) static void enable_hdmi(struct display_info_t const *dev) { diff --git a/configs/gwventana_gw5904_defconfig b/configs/gwventana_gw5904_defconfig index d25a8324b1df..9809bc691508 100644 --- a/configs/gwventana_gw5904_defconfig +++ b/configs/gwventana_gw5904_defconfig @@ -103,12 +103,11 @@ CONFIG_SUPPORT_EMMC_BOOT=y CONFIG_FSL_USDHC=y CONFIG_MTD=y CONFIG_PHYLIB=y -CONFIG_MV88E61XX_SWITCH=y -CONFIG_MV88E61XX_CPU_PORT=5 -CONFIG_MV88E61XX_PHY_PORTS=0xf -CONFIG_MV88E61XX_FIXED_PORTS=0x0 +CONFIG_MV88E61XX=y +CONFIG_PHY_FIXED=y CONFIG_DM_ETH=y CONFIG_DM_MDIO=y +CONFIG_DM_DSA=y CONFIG_E1000=y CONFIG_FEC_MXC=y CONFIG_MII=y -- 2.17.1 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support 2022-05-23 18:25 ` [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support Tim Harvey @ 2022-06-14 17:00 ` Tim Harvey 2022-06-14 18:55 ` Vladimir Oltean 2022-06-20 11:42 ` Vladimir Oltean 2022-06-24 10:25 ` Vladimir Oltean 2 siblings, 1 reply; 26+ messages in thread From: Tim Harvey @ 2022-06-14 17:00 UTC (permalink / raw) To: Vladimir Oltean Cc: Chris Packham, Anatolij Gustschin, Ramon Fried, Joe Hershberger, u-boot, Stefano Babic, Fabio Estevam, NXP i . MX U-Boot Team, Marek Behún On Mon, May 23, 2022 at 11:26 AM Tim Harvey <tharvey@gateworks.com> wrote: > > Add MV88E61XX DSA support: > - update dt: U-Boot dsa driver requires different device-tree syntax > than the linux driver in order to link the dsa ports to the mdio bus. > - update defconfig > - replace mv88e61xx_hw_reset weak override with board_phy_config support > for mv88e61xx configuration that is outside the scope of the DSA driver > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > --- > v3: > - move mdio's mdio@0 subnode > v2: no changes > --- > arch/arm/dts/imx6qdl-gw5904.dtsi | 41 ++++++++++++++++++++ > board/gateworks/gw_ventana/gw_ventana.c | 50 +++++++++---------------- > configs/gwventana_gw5904_defconfig | 7 ++-- > 3 files changed, 62 insertions(+), 36 deletions(-) > > diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi b/arch/arm/dts/imx6qdl-gw5904.dtsi > index 286c7a9924c2..1b2f70d1ccb2 100644 > --- a/arch/arm/dts/imx6qdl-gw5904.dtsi > +++ b/arch/arm/dts/imx6qdl-gw5904.dtsi > @@ -219,6 +219,33 @@ > compatible = "marvell,mv88e6085"; > reg = <0>; > > + mdios { > + #address-cells = <1>; > + #size-cells = <0>; > + > + mdio@0 { > + reg = <0>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + sw_phy0: ethernet-phy@0 { > + reg = <0x0>; > + }; > + > + sw_phy1: ethernet-phy@1 { > + reg = <0x1>; > + }; > + > + sw_phy2: ethernet-phy@2 { > + reg = <0x2>; > + }; > + > + sw_phy3: ethernet-phy@3 { > + reg = <0x3>; > + }; > + }; > + }; > + Vladimir, I'm not sure if you've had a chance to look at this latest patch revision yet. I believe above is what you were describing as the correct way to do this (without modifying the Linux driver and improving bindings) Best Regards, Tim > ports { > #address-cells = <1>; > #size-cells = <0>; > @@ -226,27 +253,41 @@ > port@0 { > reg = <0>; > label = "lan4"; > + phy-mode = "internal"; > + phy-handle = <&sw_phy0>; > }; > > port@1 { > reg = <1>; > label = "lan3"; > + phy-mode = "internal"; > + phy-handle = <&sw_phy1>; > }; > > port@2 { > reg = <2>; > label = "lan2"; > + phy-mode = "internal"; > + phy-handle = <&sw_phy2>; > }; > > port@3 { > reg = <3>; > label = "lan1"; > + phy-mode = "internal"; > + phy-handle = <&sw_phy3>; > }; > > port@5 { > reg = <5>; > label = "cpu"; > ethernet = <&fec>; > + phy-mode = "rgmii-id"; > + > + fixed-link { > + speed = <1000>; > + full-duplex; > + }; > }; > }; > }; > diff --git a/board/gateworks/gw_ventana/gw_ventana.c b/board/gateworks/gw_ventana/gw_ventana.c > index c06630a66b66..bef3f7ef0d2b 100644 > --- a/board/gateworks/gw_ventana/gw_ventana.c > +++ b/board/gateworks/gw_ventana/gw_ventana.c > @@ -68,44 +68,30 @@ int board_phy_config(struct phy_device *phydev) > phy_write(phydev, MDIO_DEVAD_NONE, 14, val); > } > > + /* Fixed PHY: for GW5904/GW5909 this is Marvell 88E6176 GbE Switch */ > + else if (phydev->phy_id == 0xa5a55a5a && > + ((board_type == GW5904) || (board_type == GW5909))) { > + struct mii_dev *bus = miiphy_get_dev_by_name("mdio"); > + > + puts("MV88E61XX "); > + /* GPIO[0] output CLK125 for RGMII_REFCLK */ > + bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x62 << 8) | 0xfe); > + bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x68 << 8) | 7); > + > + /* Port 0-3 LED configuration: Table 80/82 */ > + /* LED configuration: 7:4-green (8=Activity) 3:0 amber (8=Link) */ > + bus->write(bus, 0x10, 0, 0x16, 0x8088); > + bus->write(bus, 0x11, 0, 0x16, 0x8088); > + bus->write(bus, 0x12, 0, 0x16, 0x8088); > + bus->write(bus, 0x13, 0, 0x16, 0x8088); > + } > + > if (phydev->drv->config) > phydev->drv->config(phydev); > > return 0; > } > > -#ifdef CONFIG_MV88E61XX_SWITCH > -int mv88e61xx_hw_reset(struct phy_device *phydev) > -{ > - struct mii_dev *bus = phydev->bus; > - > - /* GPIO[0] output, CLK125 */ > - debug("enabling RGMII_REFCLK\n"); > - bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0, > - 0x1a /*MV_SCRATCH_MISC*/, > - (1 << 15) | (0x62 /*MV_GPIO_DIR*/ << 8) | 0xfe); > - bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0, > - 0x1a /*MV_SCRATCH_MISC*/, > - (1 << 15) | (0x68 /*MV_GPIO01_CNTL*/ << 8) | 7); > - > - /* RGMII delay - Physical Control register bit[15:14] */ > - debug("setting port%d RGMII rx/tx delay\n", CONFIG_MV88E61XX_CPU_PORT); > - /* forced 1000mbps full-duplex link */ > - bus->write(bus, 0x10 + CONFIG_MV88E61XX_CPU_PORT, 0, 1, 0xc0fe); > - phydev->autoneg = AUTONEG_DISABLE; > - phydev->speed = SPEED_1000; > - phydev->duplex = DUPLEX_FULL; > - > - /* LED configuration: 7:4-green (8=Activity) 3:0 amber (8=Link) */ > - bus->write(bus, 0x10, 0, 0x16, 0x8088); > - bus->write(bus, 0x11, 0, 0x16, 0x8088); > - bus->write(bus, 0x12, 0, 0x16, 0x8088); > - bus->write(bus, 0x13, 0, 0x16, 0x8088); > - > - return 0; > -} > -#endif // CONFIG_MV88E61XX_SWITCH > - > #if defined(CONFIG_VIDEO_IPUV3) > static void enable_hdmi(struct display_info_t const *dev) > { > diff --git a/configs/gwventana_gw5904_defconfig b/configs/gwventana_gw5904_defconfig > index d25a8324b1df..9809bc691508 100644 > --- a/configs/gwventana_gw5904_defconfig > +++ b/configs/gwventana_gw5904_defconfig > @@ -103,12 +103,11 @@ CONFIG_SUPPORT_EMMC_BOOT=y > CONFIG_FSL_USDHC=y > CONFIG_MTD=y > CONFIG_PHYLIB=y > -CONFIG_MV88E61XX_SWITCH=y > -CONFIG_MV88E61XX_CPU_PORT=5 > -CONFIG_MV88E61XX_PHY_PORTS=0xf > -CONFIG_MV88E61XX_FIXED_PORTS=0x0 > +CONFIG_MV88E61XX=y > +CONFIG_PHY_FIXED=y > CONFIG_DM_ETH=y > CONFIG_DM_MDIO=y > +CONFIG_DM_DSA=y > CONFIG_E1000=y > CONFIG_FEC_MXC=y > CONFIG_MII=y > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support 2022-06-14 17:00 ` Tim Harvey @ 2022-06-14 18:55 ` Vladimir Oltean 0 siblings, 0 replies; 26+ messages in thread From: Vladimir Oltean @ 2022-06-14 18:55 UTC (permalink / raw) To: tharvey Cc: Chris Packham, Anatolij Gustschin, Ramon Fried, Joe Hershberger, u-boot, Stefano Babic, Fabio Estevam, dl-uboot-imx, Marek BehĂșn Hi Tim, On Tue, Jun 14, 2022 at 10:00:57AM -0700, Tim Harvey wrote: > Vladimir, > > I'm not sure if you've had a chance to look at this latest patch > revision yet. I believe above is what you were describing as the > correct way to do this (without modifying the Linux driver and > improving bindings) > > Best Regards, > > Tim I'm currently on vacation but I'm slowly working through my inbox. This is certainly one of the reasonable ways of doing things, but it will require modifying the mv88e6xxx Linux driver to support an "mdios" subnode (something which nobody should object against, I believe). I'll follow up with more comments when I get to these patches. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support 2022-05-23 18:25 ` [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support Tim Harvey 2022-06-14 17:00 ` Tim Harvey @ 2022-06-20 11:42 ` Vladimir Oltean 2022-06-21 16:57 ` Tim Harvey 2022-06-24 10:25 ` Vladimir Oltean 2 siblings, 1 reply; 26+ messages in thread From: Vladimir Oltean @ 2022-06-20 11:42 UTC (permalink / raw) To: tharvey Cc: Joe Hershberger, Ramon Fried, u-boot, Stefano Babic, Fabio Estevam, dl-uboot-imx, Marek BehĂșn, Chris Packham, Anatolij Gustschin On Mon, May 23, 2022 at 11:25:49AM -0700, Tim Harvey wrote: > Add MV88E61XX DSA support: > - update dt: U-Boot dsa driver requires different device-tree syntax > than the linux driver in order to link the dsa ports to the mdio bus. > - update defconfig > - replace mv88e61xx_hw_reset weak override with board_phy_config support > for mv88e61xx configuration that is outside the scope of the DSA driver > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > --- > v3: > - move mdio's mdio@0 subnode > v2: no changes > --- > arch/arm/dts/imx6qdl-gw5904.dtsi | 41 ++++++++++++++++++++ > board/gateworks/gw_ventana/gw_ventana.c | 50 +++++++++---------------- > configs/gwventana_gw5904_defconfig | 7 ++-- > 3 files changed, 62 insertions(+), 36 deletions(-) > > diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi b/arch/arm/dts/imx6qdl-gw5904.dtsi > index 286c7a9924c2..1b2f70d1ccb2 100644 > --- a/arch/arm/dts/imx6qdl-gw5904.dtsi > +++ b/arch/arm/dts/imx6qdl-gw5904.dtsi > @@ -219,6 +219,33 @@ > compatible = "marvell,mv88e6085"; > reg = <0>; > > + mdios { > + #address-cells = <1>; > + #size-cells = <0>; > + > + mdio@0 { > + reg = <0>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + sw_phy0: ethernet-phy@0 { > + reg = <0x0>; > + }; > + > + sw_phy1: ethernet-phy@1 { > + reg = <0x1>; > + }; > + > + sw_phy2: ethernet-phy@2 { > + reg = <0x2>; > + }; > + > + sw_phy3: ethernet-phy@3 { > + reg = <0x3>; > + }; > + }; > + }; > + > ports { > #address-cells = <1>; > #size-cells = <0>; > @@ -226,27 +253,41 @@ > port@0 { > reg = <0>; > label = "lan4"; > + phy-mode = "internal"; > + phy-handle = <&sw_phy0>; > }; > > port@1 { > reg = <1>; > label = "lan3"; > + phy-mode = "internal"; > + phy-handle = <&sw_phy1>; > }; > > port@2 { > reg = <2>; > label = "lan2"; > + phy-mode = "internal"; > + phy-handle = <&sw_phy2>; > }; > > port@3 { > reg = <3>; > label = "lan1"; > + phy-mode = "internal"; > + phy-handle = <&sw_phy3>; > }; > > port@5 { > reg = <5>; > label = "cpu"; > ethernet = <&fec>; > + phy-mode = "rgmii-id"; > + > + fixed-link { > + speed = <1000>; > + full-duplex; > + }; > }; > }; > }; > diff --git a/board/gateworks/gw_ventana/gw_ventana.c b/board/gateworks/gw_ventana/gw_ventana.c > index c06630a66b66..bef3f7ef0d2b 100644 > --- a/board/gateworks/gw_ventana/gw_ventana.c > +++ b/board/gateworks/gw_ventana/gw_ventana.c > @@ -68,44 +68,30 @@ int board_phy_config(struct phy_device *phydev) > phy_write(phydev, MDIO_DEVAD_NONE, 14, val); > } > > + /* Fixed PHY: for GW5904/GW5909 this is Marvell 88E6176 GbE Switch */ > + else if (phydev->phy_id == 0xa5a55a5a && PHY_FIXED_ID, but see below. > + ((board_type == GW5904) || (board_type == GW5909))) { > + struct mii_dev *bus = miiphy_get_dev_by_name("mdio"); > + > + puts("MV88E61XX "); > + /* GPIO[0] output CLK125 for RGMII_REFCLK */ > + bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x62 << 8) | 0xfe); > + bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x68 << 8) | 7); > + > + /* Port 0-3 LED configuration: Table 80/82 */ > + /* LED configuration: 7:4-green (8=Activity) 3:0 amber (8=Link) */ > + bus->write(bus, 0x10, 0, 0x16, 0x8088); > + bus->write(bus, 0x11, 0, 0x16, 0x8088); > + bus->write(bus, 0x12, 0, 0x16, 0x8088); > + bus->write(bus, 0x13, 0, 0x16, 0x8088); > + } > + There's nothing too board specific about this configuration, why do you feel it does not belong to the DSA driver? Some macros hiding away magic register addresses and values would be good either way. > if (phydev->drv->config) > phydev->drv->config(phydev); > > return 0; > } > > -#ifdef CONFIG_MV88E61XX_SWITCH > -int mv88e61xx_hw_reset(struct phy_device *phydev) > -{ > - struct mii_dev *bus = phydev->bus; > - > - /* GPIO[0] output, CLK125 */ > - debug("enabling RGMII_REFCLK\n"); > - bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0, > - 0x1a /*MV_SCRATCH_MISC*/, > - (1 << 15) | (0x62 /*MV_GPIO_DIR*/ << 8) | 0xfe); > - bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0, > - 0x1a /*MV_SCRATCH_MISC*/, > - (1 << 15) | (0x68 /*MV_GPIO01_CNTL*/ << 8) | 7); > - > - /* RGMII delay - Physical Control register bit[15:14] */ > - debug("setting port%d RGMII rx/tx delay\n", CONFIG_MV88E61XX_CPU_PORT); > - /* forced 1000mbps full-duplex link */ > - bus->write(bus, 0x10 + CONFIG_MV88E61XX_CPU_PORT, 0, 1, 0xc0fe); > - phydev->autoneg = AUTONEG_DISABLE; > - phydev->speed = SPEED_1000; > - phydev->duplex = DUPLEX_FULL; > - > - /* LED configuration: 7:4-green (8=Activity) 3:0 amber (8=Link) */ > - bus->write(bus, 0x10, 0, 0x16, 0x8088); > - bus->write(bus, 0x11, 0, 0x16, 0x8088); > - bus->write(bus, 0x12, 0, 0x16, 0x8088); > - bus->write(bus, 0x13, 0, 0x16, 0x8088); > - > - return 0; > -} > -#endif // CONFIG_MV88E61XX_SWITCH > - > #if defined(CONFIG_VIDEO_IPUV3) > static void enable_hdmi(struct display_info_t const *dev) > { > diff --git a/configs/gwventana_gw5904_defconfig b/configs/gwventana_gw5904_defconfig > index d25a8324b1df..9809bc691508 100644 > --- a/configs/gwventana_gw5904_defconfig > +++ b/configs/gwventana_gw5904_defconfig > @@ -103,12 +103,11 @@ CONFIG_SUPPORT_EMMC_BOOT=y > CONFIG_FSL_USDHC=y > CONFIG_MTD=y > CONFIG_PHYLIB=y > -CONFIG_MV88E61XX_SWITCH=y > -CONFIG_MV88E61XX_CPU_PORT=5 > -CONFIG_MV88E61XX_PHY_PORTS=0xf > -CONFIG_MV88E61XX_FIXED_PORTS=0x0 > +CONFIG_MV88E61XX=y > +CONFIG_PHY_FIXED=y > CONFIG_DM_ETH=y > CONFIG_DM_MDIO=y > +CONFIG_DM_DSA=y > CONFIG_E1000=y > CONFIG_FEC_MXC=y > CONFIG_MII=y > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support 2022-06-20 11:42 ` Vladimir Oltean @ 2022-06-21 16:57 ` Tim Harvey 2022-06-23 12:42 ` Vladimir Oltean 0 siblings, 1 reply; 26+ messages in thread From: Tim Harvey @ 2022-06-21 16:57 UTC (permalink / raw) To: Vladimir Oltean Cc: Joe Hershberger, Ramon Fried, u-boot, Stefano Babic, Fabio Estevam, dl-uboot-imx, Marek BehĂșn, Chris Packham, Anatolij Gustschin On Mon, Jun 20, 2022 at 4:42 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > On Mon, May 23, 2022 at 11:25:49AM -0700, Tim Harvey wrote: > > Add MV88E61XX DSA support: > > - update dt: U-Boot dsa driver requires different device-tree syntax > > than the linux driver in order to link the dsa ports to the mdio bus. > > - update defconfig > > - replace mv88e61xx_hw_reset weak override with board_phy_config support > > for mv88e61xx configuration that is outside the scope of the DSA driver > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > --- > > v3: > > - move mdio's mdio@0 subnode > > v2: no changes > > --- > > arch/arm/dts/imx6qdl-gw5904.dtsi | 41 ++++++++++++++++++++ > > board/gateworks/gw_ventana/gw_ventana.c | 50 +++++++++---------------- > > configs/gwventana_gw5904_defconfig | 7 ++-- > > 3 files changed, 62 insertions(+), 36 deletions(-) > > > > diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi b/arch/arm/dts/imx6qdl-gw5904.dtsi > > index 286c7a9924c2..1b2f70d1ccb2 100644 > > --- a/arch/arm/dts/imx6qdl-gw5904.dtsi > > +++ b/arch/arm/dts/imx6qdl-gw5904.dtsi > > @@ -219,6 +219,33 @@ > > compatible = "marvell,mv88e6085"; > > reg = <0>; > > > > + mdios { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + mdio@0 { > > + reg = <0>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + sw_phy0: ethernet-phy@0 { > > + reg = <0x0>; > > + }; > > + > > + sw_phy1: ethernet-phy@1 { > > + reg = <0x1>; > > + }; > > + > > + sw_phy2: ethernet-phy@2 { > > + reg = <0x2>; > > + }; > > + > > + sw_phy3: ethernet-phy@3 { > > + reg = <0x3>; > > + }; > > + }; > > + }; > > + > > ports { > > #address-cells = <1>; > > #size-cells = <0>; > > @@ -226,27 +253,41 @@ > > port@0 { > > reg = <0>; > > label = "lan4"; > > + phy-mode = "internal"; > > + phy-handle = <&sw_phy0>; > > }; > > > > port@1 { > > reg = <1>; > > label = "lan3"; > > + phy-mode = "internal"; > > + phy-handle = <&sw_phy1>; > > }; > > > > port@2 { > > reg = <2>; > > label = "lan2"; > > + phy-mode = "internal"; > > + phy-handle = <&sw_phy2>; > > }; > > > > port@3 { > > reg = <3>; > > label = "lan1"; > > + phy-mode = "internal"; > > + phy-handle = <&sw_phy3>; > > }; > > > > port@5 { > > reg = <5>; > > label = "cpu"; > > ethernet = <&fec>; > > + phy-mode = "rgmii-id"; > > + > > + fixed-link { > > + speed = <1000>; > > + full-duplex; > > + }; > > }; > > }; > > }; > > diff --git a/board/gateworks/gw_ventana/gw_ventana.c b/board/gateworks/gw_ventana/gw_ventana.c > > index c06630a66b66..bef3f7ef0d2b 100644 > > --- a/board/gateworks/gw_ventana/gw_ventana.c > > +++ b/board/gateworks/gw_ventana/gw_ventana.c > > @@ -68,44 +68,30 @@ int board_phy_config(struct phy_device *phydev) > > phy_write(phydev, MDIO_DEVAD_NONE, 14, val); > > } > > > > + /* Fixed PHY: for GW5904/GW5909 this is Marvell 88E6176 GbE Switch */ > > + else if (phydev->phy_id == 0xa5a55a5a && > > PHY_FIXED_ID, but see below. > > > + ((board_type == GW5904) || (board_type == GW5909))) { > > + struct mii_dev *bus = miiphy_get_dev_by_name("mdio"); > > + > > + puts("MV88E61XX "); > > + /* GPIO[0] output CLK125 for RGMII_REFCLK */ > > + bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x62 << 8) | 0xfe); > > + bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x68 << 8) | 7); > > + > > + /* Port 0-3 LED configuration: Table 80/82 */ > > + /* LED configuration: 7:4-green (8=Activity) 3:0 amber (8=Link) */ > > + bus->write(bus, 0x10, 0, 0x16, 0x8088); > > + bus->write(bus, 0x11, 0, 0x16, 0x8088); > > + bus->write(bus, 0x12, 0, 0x16, 0x8088); > > + bus->write(bus, 0x13, 0, 0x16, 0x8088); > > + } > > + > > There's nothing too board specific about this configuration, why do you > feel it does not belong to the DSA driver? > > Some macros hiding away magic register addresses and values would be > good either way. > I don't much care for MAC/PHY drivers configuring GPIO's and LED's due to the lack of consistent dt bindings for such a thing and I wasn't planning on trying to enhance the capabilities of the mv88e6xxx driver. There are 7 functions each of the 15 GPIO's can be set to: 0 - GPIO 1 - PTP_TRIG - Precise Timing Protocol Trigger Generate Output 2 - PTP_EVREQ - Precise Timing Protocol Event Request Input 3 - PTP_EXTCLK - Precise Timing Protocol ExternalClock Input 4 - SE_RCLK0 - SyncE Receive Clock 0 Output 5 - SE_RCLK1 - SyncE Receive Clock 1 Output 6 - Reserved 7 - CLK125 - Free running 125 MHz Clock Output There are two LED's per port each of which can be set to 16 different functions also and these functions take a lot of words to describe thus probably wouldn't lend well to #define names. Are there dt bindings that you would suggest I look at for per-port LED config on a switch, or GPIO config on a switch? If I add dt-bindings I'll have to update the kernel driver as well which again, was not my goal here. Maybe moving these into the mv88e6xxx driver per dt bindings could be a 'todo'. This patch isn't making what is already in the board file more obscure, it is just updating it to work with the new dsa driver. The following was what this patch replaced: -#ifdef CONFIG_MV88E61XX_SWITCH -int mv88e61xx_hw_reset(struct phy_device *phydev) -{ - struct mii_dev *bus = phydev->bus; - - /* GPIO[0] output, CLK125 */ - debug("enabling RGMII_REFCLK\n"); - bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0, - 0x1a /*MV_SCRATCH_MISC*/, - (1 << 15) | (0x62 /*MV_GPIO_DIR*/ << 8) | 0xfe); - bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0, - 0x1a /*MV_SCRATCH_MISC*/, - (1 << 15) | (0x68 /*MV_GPIO01_CNTL*/ << 8) | 7); - - /* RGMII delay - Physical Control register bit[15:14] */ - debug("setting port%d RGMII rx/tx delay\n", CONFIG_MV88E61XX_CPU_PORT); - /* forced 1000mbps full-duplex link */ - bus->write(bus, 0x10 + CONFIG_MV88E61XX_CPU_PORT, 0, 1, 0xc0fe); - phydev->autoneg = AUTONEG_DISABLE; - phydev->speed = SPEED_1000; - phydev->duplex = DUPLEX_FULL; - - /* LED configuration: 7:4-green (8=Activity) 3:0 amber (8=Link) */ - bus->write(bus, 0x10, 0, 0x16, 0x8088); - bus->write(bus, 0x11, 0, 0x16, 0x8088); - bus->write(bus, 0x12, 0, 0x16, 0x8088); - bus->write(bus, 0x13, 0, 0x16, 0x8088); - - return 0; -} -#endif // CONFIG_MV88E61XX_SWITCH Best Regards, Tim > > if (phydev->drv->config) > > phydev->drv->config(phydev); > > > > return 0; > > } > > > > -#ifdef CONFIG_MV88E61XX_SWITCH > > -int mv88e61xx_hw_reset(struct phy_device *phydev) > > -{ > > - struct mii_dev *bus = phydev->bus; > > - > > - /* GPIO[0] output, CLK125 */ > > - debug("enabling RGMII_REFCLK\n"); > > - bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0, > > - 0x1a /*MV_SCRATCH_MISC*/, > > - (1 << 15) | (0x62 /*MV_GPIO_DIR*/ << 8) | 0xfe); > > - bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0, > > - 0x1a /*MV_SCRATCH_MISC*/, > > - (1 << 15) | (0x68 /*MV_GPIO01_CNTL*/ << 8) | 7); > > - > > - /* RGMII delay - Physical Control register bit[15:14] */ > > - debug("setting port%d RGMII rx/tx delay\n", CONFIG_MV88E61XX_CPU_PORT); > > - /* forced 1000mbps full-duplex link */ > > - bus->write(bus, 0x10 + CONFIG_MV88E61XX_CPU_PORT, 0, 1, 0xc0fe); > > - phydev->autoneg = AUTONEG_DISABLE; > > - phydev->speed = SPEED_1000; > > - phydev->duplex = DUPLEX_FULL; > > - > > - /* LED configuration: 7:4-green (8=Activity) 3:0 amber (8=Link) */ > > - bus->write(bus, 0x10, 0, 0x16, 0x8088); > > - bus->write(bus, 0x11, 0, 0x16, 0x8088); > > - bus->write(bus, 0x12, 0, 0x16, 0x8088); > > - bus->write(bus, 0x13, 0, 0x16, 0x8088); > > - > > - return 0; > > -} > > -#endif // CONFIG_MV88E61XX_SWITCH > > - > > #if defined(CONFIG_VIDEO_IPUV3) > > static void enable_hdmi(struct display_info_t const *dev) > > { > > diff --git a/configs/gwventana_gw5904_defconfig b/configs/gwventana_gw5904_defconfig > > index d25a8324b1df..9809bc691508 100644 > > --- a/configs/gwventana_gw5904_defconfig > > +++ b/configs/gwventana_gw5904_defconfig > > @@ -103,12 +103,11 @@ CONFIG_SUPPORT_EMMC_BOOT=y > > CONFIG_FSL_USDHC=y > > CONFIG_MTD=y > > CONFIG_PHYLIB=y > > -CONFIG_MV88E61XX_SWITCH=y > > -CONFIG_MV88E61XX_CPU_PORT=5 > > -CONFIG_MV88E61XX_PHY_PORTS=0xf > > -CONFIG_MV88E61XX_FIXED_PORTS=0x0 > > +CONFIG_MV88E61XX=y > > +CONFIG_PHY_FIXED=y > > CONFIG_DM_ETH=y > > CONFIG_DM_MDIO=y > > +CONFIG_DM_DSA=y > > CONFIG_E1000=y > > CONFIG_FEC_MXC=y > > CONFIG_MII=y > > -- > > 2.17.1 > > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support 2022-06-21 16:57 ` Tim Harvey @ 2022-06-23 12:42 ` Vladimir Oltean 2022-06-23 16:07 ` Tim Harvey 0 siblings, 1 reply; 26+ messages in thread From: Vladimir Oltean @ 2022-06-23 12:42 UTC (permalink / raw) To: tharvey Cc: Joe Hershberger, Ramon Fried, u-boot, Stefano Babic, Fabio Estevam, dl-uboot-imx, Marek BehĂșn, Chris Packham, Anatolij Gustschin On Tue, Jun 21, 2022 at 09:57:35AM -0700, Tim Harvey wrote: > > > diff --git a/board/gateworks/gw_ventana/gw_ventana.c b/board/gateworks/gw_ventana/gw_ventana.c > > > index c06630a66b66..bef3f7ef0d2b 100644 > > > --- a/board/gateworks/gw_ventana/gw_ventana.c > > > +++ b/board/gateworks/gw_ventana/gw_ventana.c > > > @@ -68,44 +68,30 @@ int board_phy_config(struct phy_device *phydev) > > > phy_write(phydev, MDIO_DEVAD_NONE, 14, val); > > > } > > > > > > + /* Fixed PHY: for GW5904/GW5909 this is Marvell 88E6176 GbE Switch */ > > > + else if (phydev->phy_id == 0xa5a55a5a && > > > + ((board_type == GW5904) || (board_type == GW5909))) { > > > + struct mii_dev *bus = miiphy_get_dev_by_name("mdio"); > > > + > > > + puts("MV88E61XX "); > > > + /* GPIO[0] output CLK125 for RGMII_REFCLK */ > > > + bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x62 << 8) | 0xfe); > > > + bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x68 << 8) | 7); > > > + > > > + /* Port 0-3 LED configuration: Table 80/82 */ > > > + /* LED configuration: 7:4-green (8=Activity) 3:0 amber (8=Link) */ > > > + bus->write(bus, 0x10, 0, 0x16, 0x8088); > > > + bus->write(bus, 0x11, 0, 0x16, 0x8088); > > > + bus->write(bus, 0x12, 0, 0x16, 0x8088); > > > + bus->write(bus, 0x13, 0, 0x16, 0x8088); > > > + } > > > + > > > > There's nothing too board specific about this configuration, why do you > > feel it does not belong to the DSA driver? > > > > Some macros hiding away magic register addresses and values would be > > good either way. > > > > I don't much care for MAC/PHY drivers configuring GPIO's and LED's due > to the lack of consistent dt bindings for such a thing and I wasn't > planning on trying to enhance the capabilities of the mv88e6xxx > driver. > > There are 7 functions each of the 15 GPIO's can be set to: > 0 - GPIO > 1 - PTP_TRIG - Precise Timing Protocol Trigger Generate Output > 2 - PTP_EVREQ - Precise Timing Protocol Event Request Input > 3 - PTP_EXTCLK - Precise Timing Protocol ExternalClock Input > 4 - SE_RCLK0 - SyncE Receive Clock 0 Output > 5 - SE_RCLK1 - SyncE Receive Clock 1 Output > 6 - Reserved > 7 - CLK125 - Free running 125 MHz Clock Output > > There are two LED's per port each of which can be set to 16 different > functions also and these functions take a lot of words to describe > thus probably wouldn't lend well to #define names. > > Are there dt bindings that you would suggest I look at for per-port > LED config on a switch, or GPIO config on a switch? If I add > dt-bindings I'll have to update the kernel driver as well which again, > was not my goal here. Maybe moving these into the mv88e6xxx driver per > dt bindings could be a 'todo'. > > This patch isn't making what is already in the board file more > obscure, it is just updating it to work with the new dsa driver. The > following was what this patch replaced: > -#ifdef CONFIG_MV88E61XX_SWITCH > -int mv88e61xx_hw_reset(struct phy_device *phydev) > -{ > - struct mii_dev *bus = phydev->bus; > - > - /* GPIO[0] output, CLK125 */ > - debug("enabling RGMII_REFCLK\n"); > - bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0, > - 0x1a /*MV_SCRATCH_MISC*/, > - (1 << 15) | (0x62 /*MV_GPIO_DIR*/ << 8) | 0xfe); > - bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0, > - 0x1a /*MV_SCRATCH_MISC*/, > - (1 << 15) | (0x68 /*MV_GPIO01_CNTL*/ << 8) | 7); > - > - /* RGMII delay - Physical Control register bit[15:14] */ > - debug("setting port%d RGMII rx/tx delay\n", CONFIG_MV88E61XX_CPU_PORT); > - /* forced 1000mbps full-duplex link */ > - bus->write(bus, 0x10 + CONFIG_MV88E61XX_CPU_PORT, 0, 1, 0xc0fe); > - phydev->autoneg = AUTONEG_DISABLE; > - phydev->speed = SPEED_1000; > - phydev->duplex = DUPLEX_FULL; > - > - /* LED configuration: 7:4-green (8=Activity) 3:0 amber (8=Link) */ > - bus->write(bus, 0x10, 0, 0x16, 0x8088); > - bus->write(bus, 0x11, 0, 0x16, 0x8088); > - bus->write(bus, 0x12, 0, 0x16, 0x8088); > - bus->write(bus, 0x13, 0, 0x16, 0x8088); > - > - return 0; > -} > -#endif // CONFIG_MV88E61XX_SWITCH > > Best Regards, > > Tim Ok, I was thinking PHY LED configuration could be hardcoded in the driver itself (no DT bindings) and nobody would probably even notice. For pin configuration as RGMII 125 MHz clock or GPIO or otherwise, there would probably need to be a "pinctrl" DT subnode with a specific pinctrl driver attached. It's best if the development for that would be done in concert with the Linux community, perhaps even in Linux first. In any case, from my side it's ok if you leave a pinctrl sub-driver as TODO. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support 2022-06-23 12:42 ` Vladimir Oltean @ 2022-06-23 16:07 ` Tim Harvey 0 siblings, 0 replies; 26+ messages in thread From: Tim Harvey @ 2022-06-23 16:07 UTC (permalink / raw) To: Vladimir Oltean Cc: Joe Hershberger, Ramon Fried, u-boot, Stefano Babic, Fabio Estevam, dl-uboot-imx, Marek BehĂșn, Chris Packham, Anatolij Gustschin On Thu, Jun 23, 2022 at 5:42 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > On Tue, Jun 21, 2022 at 09:57:35AM -0700, Tim Harvey wrote: > > > > diff --git a/board/gateworks/gw_ventana/gw_ventana.c b/board/gateworks/gw_ventana/gw_ventana.c > > > > index c06630a66b66..bef3f7ef0d2b 100644 > > > > --- a/board/gateworks/gw_ventana/gw_ventana.c > > > > +++ b/board/gateworks/gw_ventana/gw_ventana.c > > > > @@ -68,44 +68,30 @@ int board_phy_config(struct phy_device *phydev) > > > > phy_write(phydev, MDIO_DEVAD_NONE, 14, val); > > > > } > > > > > > > > + /* Fixed PHY: for GW5904/GW5909 this is Marvell 88E6176 GbE Switch */ > > > > + else if (phydev->phy_id == 0xa5a55a5a && > > > > + ((board_type == GW5904) || (board_type == GW5909))) { > > > > + struct mii_dev *bus = miiphy_get_dev_by_name("mdio"); > > > > + > > > > + puts("MV88E61XX "); > > > > + /* GPIO[0] output CLK125 for RGMII_REFCLK */ > > > > + bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x62 << 8) | 0xfe); > > > > + bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x68 << 8) | 7); > > > > + > > > > + /* Port 0-3 LED configuration: Table 80/82 */ > > > > + /* LED configuration: 7:4-green (8=Activity) 3:0 amber (8=Link) */ > > > > + bus->write(bus, 0x10, 0, 0x16, 0x8088); > > > > + bus->write(bus, 0x11, 0, 0x16, 0x8088); > > > > + bus->write(bus, 0x12, 0, 0x16, 0x8088); > > > > + bus->write(bus, 0x13, 0, 0x16, 0x8088); > > > > + } > > > > + > > > > > > There's nothing too board specific about this configuration, why do you > > > feel it does not belong to the DSA driver? > > > > > > Some macros hiding away magic register addresses and values would be > > > good either way. > > > > > > > I don't much care for MAC/PHY drivers configuring GPIO's and LED's due > > to the lack of consistent dt bindings for such a thing and I wasn't > > planning on trying to enhance the capabilities of the mv88e6xxx > > driver. > > > > There are 7 functions each of the 15 GPIO's can be set to: > > 0 - GPIO > > 1 - PTP_TRIG - Precise Timing Protocol Trigger Generate Output > > 2 - PTP_EVREQ - Precise Timing Protocol Event Request Input > > 3 - PTP_EXTCLK - Precise Timing Protocol ExternalClock Input > > 4 - SE_RCLK0 - SyncE Receive Clock 0 Output > > 5 - SE_RCLK1 - SyncE Receive Clock 1 Output > > 6 - Reserved > > 7 - CLK125 - Free running 125 MHz Clock Output > > > > There are two LED's per port each of which can be set to 16 different > > functions also and these functions take a lot of words to describe > > thus probably wouldn't lend well to #define names. > > > > Are there dt bindings that you would suggest I look at for per-port > > LED config on a switch, or GPIO config on a switch? If I add > > dt-bindings I'll have to update the kernel driver as well which again, > > was not my goal here. Maybe moving these into the mv88e6xxx driver per > > dt bindings could be a 'todo'. > > > > This patch isn't making what is already in the board file more > > obscure, it is just updating it to work with the new dsa driver. The > > following was what this patch replaced: > > -#ifdef CONFIG_MV88E61XX_SWITCH > > -int mv88e61xx_hw_reset(struct phy_device *phydev) > > -{ > > - struct mii_dev *bus = phydev->bus; > > - > > - /* GPIO[0] output, CLK125 */ > > - debug("enabling RGMII_REFCLK\n"); > > - bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0, > > - 0x1a /*MV_SCRATCH_MISC*/, > > - (1 << 15) | (0x62 /*MV_GPIO_DIR*/ << 8) | 0xfe); > > - bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0, > > - 0x1a /*MV_SCRATCH_MISC*/, > > - (1 << 15) | (0x68 /*MV_GPIO01_CNTL*/ << 8) | 7); > > - > > - /* RGMII delay - Physical Control register bit[15:14] */ > > - debug("setting port%d RGMII rx/tx delay\n", CONFIG_MV88E61XX_CPU_PORT); > > - /* forced 1000mbps full-duplex link */ > > - bus->write(bus, 0x10 + CONFIG_MV88E61XX_CPU_PORT, 0, 1, 0xc0fe); > > - phydev->autoneg = AUTONEG_DISABLE; > > - phydev->speed = SPEED_1000; > > - phydev->duplex = DUPLEX_FULL; > > - > > - /* LED configuration: 7:4-green (8=Activity) 3:0 amber (8=Link) */ > > - bus->write(bus, 0x10, 0, 0x16, 0x8088); > > - bus->write(bus, 0x11, 0, 0x16, 0x8088); > > - bus->write(bus, 0x12, 0, 0x16, 0x8088); > > - bus->write(bus, 0x13, 0, 0x16, 0x8088); > > - > > - return 0; > > -} > > -#endif // CONFIG_MV88E61XX_SWITCH > > > > Best Regards, > > > > Tim > > Ok, I was thinking PHY LED configuration could be hardcoded in the > driver itself (no DT bindings) and nobody would probably even notice. No, it is always a bad idea to hard code a specific configuration in a driver. Most modern PHY's have 4 LEDs with 16 configurations and board vendors vary on what 2 LED's they use and how. I have seen this done in PHY drivers and it makes no sense to inflict your LED policy on other users without doing it in a configurable dt way and avoiding changes if a configuration is not found for backwards compatibility. > For pin configuration as RGMII 125 MHz clock or GPIO or otherwise, > there would probably need to be a "pinctrl" DT subnode with a specific > pinctrl driver attached. It's best if the development for that would be > done in concert with the Linux community, perhaps even in Linux first. > > In any case, from my side it's ok if you leave a pinctrl sub-driver as TODO. Can I have your Reviewed-By on this patch or do you want me to update this with a 'TODO' comment? Best Regards, Tim ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support 2022-05-23 18:25 ` [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support Tim Harvey 2022-06-14 17:00 ` Tim Harvey 2022-06-20 11:42 ` Vladimir Oltean @ 2022-06-24 10:25 ` Vladimir Oltean 2022-06-24 23:16 ` Tim Harvey 2 siblings, 1 reply; 26+ messages in thread From: Vladimir Oltean @ 2022-06-24 10:25 UTC (permalink / raw) To: tharvey Cc: Joe Hershberger, Ramon Fried, u-boot, Stefano Babic, Fabio Estevam, dl-uboot-imx, Marek BehĂșn, Chris Packham, Anatolij Gustschin On Mon, May 23, 2022 at 11:25:49AM -0700, Tim Harvey wrote: > Add MV88E61XX DSA support: > - update dt: U-Boot dsa driver requires different device-tree syntax > than the linux driver in order to link the dsa ports to the mdio bus. > - update defconfig > - replace mv88e61xx_hw_reset weak override with board_phy_config support > for mv88e61xx configuration that is outside the scope of the DSA driver > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > --- > v3: > - move mdio's mdio@0 subnode > v2: no changes > --- > arch/arm/dts/imx6qdl-gw5904.dtsi | 41 ++++++++++++++++++++ > board/gateworks/gw_ventana/gw_ventana.c | 50 +++++++++---------------- > configs/gwventana_gw5904_defconfig | 7 ++-- > 3 files changed, 62 insertions(+), 36 deletions(-) > > diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi b/arch/arm/dts/imx6qdl-gw5904.dtsi > index 286c7a9924c2..1b2f70d1ccb2 100644 > --- a/arch/arm/dts/imx6qdl-gw5904.dtsi > +++ b/arch/arm/dts/imx6qdl-gw5904.dtsi > @@ -219,6 +219,33 @@ > compatible = "marvell,mv88e6085"; > reg = <0>; > > + mdios { > + #address-cells = <1>; > + #size-cells = <0>; > + > + mdio@0 { If you are going to follow this new model with a dedicated "mdios" subnode, I've consulted with Andrew Lunn and Florian Fainelli and there shouldn't be a problem to later make Linux accept this alternate binding format. But in that case, please match this OF node by a dedicated compatible string, like "marvell,mv88e6xxx-mdio-internal", so that there will be a way to differentiate this from the existing "marvell,mv88e6xxx-mdio-external" when support for that is added in U-Boot. Alternatively, to repeat myself, you can always follow the de-facto bindings for Linux mv88e6xxx which have: switch0: switch0@0 { compatible = "marvell,mv88e6190"; ports { #address-cells = <1>; #size-cells = <0>; ... }; mdio { // internal #address-cells = <1>; #size-cells = <0>; ... }; mdio1 { compatible = "marvell,mv88e6xxx-mdio-external"; #address-cells = <1>; #size-cells = <0>; ... }; }; and the associated parsing code: static int mv88e6xxx_mdios_register(struct mv88e6xxx_chip *chip, struct device_node *np) { struct device_node *child; int err; /* Always register one mdio bus for the internal/default mdio * bus. This maybe represented in the device tree, but is * optional. */ child = of_get_child_by_name(np, "mdio"); err = mv88e6xxx_mdio_register(chip, child, false); of_node_put(child); if (err) return err; /* Walk the device tree, and see if there are any other nodes * which say they are compatible with the external mdio * bus. */ for_each_available_child_of_node(np, child) { if (of_device_is_compatible( child, "marvell,mv88e6xxx-mdio-external")) { err = mv88e6xxx_mdio_register(chip, child, true); if (err) { mv88e6xxx_mdios_unregister(chip); of_node_put(child); return err; } } } return 0; } Personally I believe that if you have limited amount of time to spend on U-Boot DM_DSA and DT bindings in general, you should just follow the format already accepted by Linux ("mdio" node is for internal MDIO, doesn't have compatible string, is placed directly under "switch" node", while external MDIO is matched by compatible string and its node can have any name). What we should try to accomplish is that the DT blob that U-Boot creates for itself here to be coherent enough that Linux is able to understand and use it, in case we decide to pass it to the operating system. With your approach you'd have work to do in Linux as well to accept the "mdios" subnode and parse controllers by compatible string inside, and I'm not exactly sure you're willing to do that. > + reg = <0>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + sw_phy0: ethernet-phy@0 { > + reg = <0x0>; > + }; > + > + sw_phy1: ethernet-phy@1 { > + reg = <0x1>; > + }; > + > + sw_phy2: ethernet-phy@2 { > + reg = <0x2>; > + }; > + > + sw_phy3: ethernet-phy@3 { > + reg = <0x3>; > + }; > + }; > + }; > + > ports { > #address-cells = <1>; > #size-cells = <0>; > @@ -226,27 +253,41 @@ > port@0 { > reg = <0>; > label = "lan4"; > + phy-mode = "internal"; > + phy-handle = <&sw_phy0>; > }; > > port@1 { > reg = <1>; > label = "lan3"; > + phy-mode = "internal"; > + phy-handle = <&sw_phy1>; > }; > > port@2 { > reg = <2>; > label = "lan2"; > + phy-mode = "internal"; > + phy-handle = <&sw_phy2>; > }; > > port@3 { > reg = <3>; > label = "lan1"; > + phy-mode = "internal"; > + phy-handle = <&sw_phy3>; > }; > > port@5 { > reg = <5>; > label = "cpu"; > ethernet = <&fec>; > + phy-mode = "rgmii-id"; > + > + fixed-link { > + speed = <1000>; > + full-duplex; > + }; > }; > }; > }; > diff --git a/board/gateworks/gw_ventana/gw_ventana.c b/board/gateworks/gw_ventana/gw_ventana.c > index c06630a66b66..bef3f7ef0d2b 100644 > --- a/board/gateworks/gw_ventana/gw_ventana.c > +++ b/board/gateworks/gw_ventana/gw_ventana.c > @@ -68,44 +68,30 @@ int board_phy_config(struct phy_device *phydev) > phy_write(phydev, MDIO_DEVAD_NONE, 14, val); > } > > + /* Fixed PHY: for GW5904/GW5909 this is Marvell 88E6176 GbE Switch */ > + else if (phydev->phy_id == 0xa5a55a5a && > + ((board_type == GW5904) || (board_type == GW5909))) { > + struct mii_dev *bus = miiphy_get_dev_by_name("mdio"); > + > + puts("MV88E61XX "); > + /* GPIO[0] output CLK125 for RGMII_REFCLK */ > + bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x62 << 8) | 0xfe); > + bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x68 << 8) | 7); > + > + /* Port 0-3 LED configuration: Table 80/82 */ > + /* LED configuration: 7:4-green (8=Activity) 3:0 amber (8=Link) */ > + bus->write(bus, 0x10, 0, 0x16, 0x8088); > + bus->write(bus, 0x11, 0, 0x16, 0x8088); > + bus->write(bus, 0x12, 0, 0x16, 0x8088); > + bus->write(bus, 0x13, 0, 0x16, 0x8088); > + } > + > if (phydev->drv->config) > phydev->drv->config(phydev); > > return 0; > } > > -#ifdef CONFIG_MV88E61XX_SWITCH > -int mv88e61xx_hw_reset(struct phy_device *phydev) > -{ > - struct mii_dev *bus = phydev->bus; > - > - /* GPIO[0] output, CLK125 */ > - debug("enabling RGMII_REFCLK\n"); > - bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0, > - 0x1a /*MV_SCRATCH_MISC*/, > - (1 << 15) | (0x62 /*MV_GPIO_DIR*/ << 8) | 0xfe); > - bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0, > - 0x1a /*MV_SCRATCH_MISC*/, > - (1 << 15) | (0x68 /*MV_GPIO01_CNTL*/ << 8) | 7); > - > - /* RGMII delay - Physical Control register bit[15:14] */ > - debug("setting port%d RGMII rx/tx delay\n", CONFIG_MV88E61XX_CPU_PORT); > - /* forced 1000mbps full-duplex link */ > - bus->write(bus, 0x10 + CONFIG_MV88E61XX_CPU_PORT, 0, 1, 0xc0fe); > - phydev->autoneg = AUTONEG_DISABLE; > - phydev->speed = SPEED_1000; > - phydev->duplex = DUPLEX_FULL; > - > - /* LED configuration: 7:4-green (8=Activity) 3:0 amber (8=Link) */ > - bus->write(bus, 0x10, 0, 0x16, 0x8088); > - bus->write(bus, 0x11, 0, 0x16, 0x8088); > - bus->write(bus, 0x12, 0, 0x16, 0x8088); > - bus->write(bus, 0x13, 0, 0x16, 0x8088); > - > - return 0; > -} > -#endif // CONFIG_MV88E61XX_SWITCH > - > #if defined(CONFIG_VIDEO_IPUV3) > static void enable_hdmi(struct display_info_t const *dev) > { > diff --git a/configs/gwventana_gw5904_defconfig b/configs/gwventana_gw5904_defconfig > index d25a8324b1df..9809bc691508 100644 > --- a/configs/gwventana_gw5904_defconfig > +++ b/configs/gwventana_gw5904_defconfig > @@ -103,12 +103,11 @@ CONFIG_SUPPORT_EMMC_BOOT=y > CONFIG_FSL_USDHC=y > CONFIG_MTD=y > CONFIG_PHYLIB=y > -CONFIG_MV88E61XX_SWITCH=y > -CONFIG_MV88E61XX_CPU_PORT=5 > -CONFIG_MV88E61XX_PHY_PORTS=0xf > -CONFIG_MV88E61XX_FIXED_PORTS=0x0 > +CONFIG_MV88E61XX=y > +CONFIG_PHY_FIXED=y > CONFIG_DM_ETH=y > CONFIG_DM_MDIO=y > +CONFIG_DM_DSA=y > CONFIG_E1000=y > CONFIG_FEC_MXC=y > CONFIG_MII=y > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support 2022-06-24 10:25 ` Vladimir Oltean @ 2022-06-24 23:16 ` Tim Harvey 2022-06-25 0:13 ` Vladimir Oltean 0 siblings, 1 reply; 26+ messages in thread From: Tim Harvey @ 2022-06-24 23:16 UTC (permalink / raw) To: Vladimir Oltean Cc: Joe Hershberger, Ramon Fried, u-boot, Stefano Babic, Fabio Estevam, dl-uboot-imx, Marek BehĂșn, Chris Packham, Anatolij Gustschin On Fri, Jun 24, 2022 at 3:25 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > On Mon, May 23, 2022 at 11:25:49AM -0700, Tim Harvey wrote: > > Add MV88E61XX DSA support: > > - update dt: U-Boot dsa driver requires different device-tree syntax > > than the linux driver in order to link the dsa ports to the mdio bus. > > - update defconfig > > - replace mv88e61xx_hw_reset weak override with board_phy_config support > > for mv88e61xx configuration that is outside the scope of the DSA driver > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > --- > > v3: > > - move mdio's mdio@0 subnode > > v2: no changes > > --- > > arch/arm/dts/imx6qdl-gw5904.dtsi | 41 ++++++++++++++++++++ > > board/gateworks/gw_ventana/gw_ventana.c | 50 +++++++++---------------- > > configs/gwventana_gw5904_defconfig | 7 ++-- > > 3 files changed, 62 insertions(+), 36 deletions(-) > > > > diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi b/arch/arm/dts/imx6qdl-gw5904.dtsi > > index 286c7a9924c2..1b2f70d1ccb2 100644 > > --- a/arch/arm/dts/imx6qdl-gw5904.dtsi > > +++ b/arch/arm/dts/imx6qdl-gw5904.dtsi > > @@ -219,6 +219,33 @@ > > compatible = "marvell,mv88e6085"; > > reg = <0>; > > > > + mdios { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + mdio@0 { > > If you are going to follow this new model with a dedicated "mdios" subnode, > I've consulted with Andrew Lunn and Florian Fainelli and there shouldn't > be a problem to later make Linux accept this alternate binding format. > But in that case, please match this OF node by a dedicated compatible > string, like "marvell,mv88e6xxx-mdio-internal", so that there will be a > way to differentiate this from the existing "marvell,mv88e6xxx-mdio-external" > when support for that is added in U-Boot. > > Alternatively, to repeat myself, you can always follow the de-facto > bindings for Linux mv88e6xxx which have: > > switch0: switch0@0 { > compatible = "marvell,mv88e6190"; > > ports { > #address-cells = <1>; > #size-cells = <0>; > > ... > }; > > mdio { // internal > #address-cells = <1>; > #size-cells = <0>; > > ... > }; > > mdio1 { > compatible = "marvell,mv88e6xxx-mdio-external"; > #address-cells = <1>; > #size-cells = <0>; > > ... > }; > }; > Documentation/devicetree/bindings/net/dsa/marvell.txt shows en example with just one child node under the internal mdio node: mdio { #address-cells = <1>; #size-cells = <0>; switch1phy0: switch1phy0@0 { reg = <0>; interrupt-parent = <&switch0>; interrupts = <0 IRQ_TYPE_LEVEL_HIGH>; }; }; Am I to assume I can add additional nodes there for the other ports such as the following? mdio { #address-cells = <1>; #size-cells = <0>; switch1phy0: switch1phy0@0 { reg = <0>; }; switch1phy1: switch1phy1@1 { reg = <1>; }; switch1phy2: switch1phy2@2 { reg = <2>; }; switch1phy3: switch1phy3@3 { reg = <3>; }; ... }; Best Regards, Tim > and the associated parsing code: > > static int mv88e6xxx_mdios_register(struct mv88e6xxx_chip *chip, > struct device_node *np) > { > struct device_node *child; > int err; > > /* Always register one mdio bus for the internal/default mdio > * bus. This maybe represented in the device tree, but is > * optional. > */ > child = of_get_child_by_name(np, "mdio"); > err = mv88e6xxx_mdio_register(chip, child, false); > of_node_put(child); > if (err) > return err; > > /* Walk the device tree, and see if there are any other nodes > * which say they are compatible with the external mdio > * bus. > */ > for_each_available_child_of_node(np, child) { > if (of_device_is_compatible( > child, "marvell,mv88e6xxx-mdio-external")) { > err = mv88e6xxx_mdio_register(chip, child, true); > if (err) { > mv88e6xxx_mdios_unregister(chip); > of_node_put(child); > return err; > } > } > } > > return 0; > } > > Personally I believe that if you have limited amount of time to spend on > U-Boot DM_DSA and DT bindings in general, you should just follow the > format already accepted by Linux ("mdio" node is for internal MDIO, > doesn't have compatible string, is placed directly under "switch" node", > while external MDIO is matched by compatible string and its node can > have any name). > > What we should try to accomplish is that the DT blob that U-Boot creates > for itself here to be coherent enough that Linux is able to understand > and use it, in case we decide to pass it to the operating system. With > your approach you'd have work to do in Linux as well to accept the > "mdios" subnode and parse controllers by compatible string inside, and > I'm not exactly sure you're willing to do that. > > > + reg = <0>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + sw_phy0: ethernet-phy@0 { > > + reg = <0x0>; > > + }; > > + > > + sw_phy1: ethernet-phy@1 { > > + reg = <0x1>; > > + }; > > + > > + sw_phy2: ethernet-phy@2 { > > + reg = <0x2>; > > + }; > > + > > + sw_phy3: ethernet-phy@3 { > > + reg = <0x3>; > > + }; > > + }; > > + }; > > + > > ports { > > #address-cells = <1>; > > #size-cells = <0>; > > @@ -226,27 +253,41 @@ > > port@0 { > > reg = <0>; > > label = "lan4"; > > + phy-mode = "internal"; > > + phy-handle = <&sw_phy0>; > > }; > > > > port@1 { > > reg = <1>; > > label = "lan3"; > > + phy-mode = "internal"; > > + phy-handle = <&sw_phy1>; > > }; > > > > port@2 { > > reg = <2>; > > label = "lan2"; > > + phy-mode = "internal"; > > + phy-handle = <&sw_phy2>; > > }; > > > > port@3 { > > reg = <3>; > > label = "lan1"; > > + phy-mode = "internal"; > > + phy-handle = <&sw_phy3>; > > }; > > > > port@5 { > > reg = <5>; > > label = "cpu"; > > ethernet = <&fec>; > > + phy-mode = "rgmii-id"; > > + > > + fixed-link { > > + speed = <1000>; > > + full-duplex; > > + }; > > }; > > }; > > }; > > diff --git a/board/gateworks/gw_ventana/gw_ventana.c b/board/gateworks/gw_ventana/gw_ventana.c > > index c06630a66b66..bef3f7ef0d2b 100644 > > --- a/board/gateworks/gw_ventana/gw_ventana.c > > +++ b/board/gateworks/gw_ventana/gw_ventana.c > > @@ -68,44 +68,30 @@ int board_phy_config(struct phy_device *phydev) > > phy_write(phydev, MDIO_DEVAD_NONE, 14, val); > > } > > > > + /* Fixed PHY: for GW5904/GW5909 this is Marvell 88E6176 GbE Switch */ > > + else if (phydev->phy_id == 0xa5a55a5a && > > + ((board_type == GW5904) || (board_type == GW5909))) { > > + struct mii_dev *bus = miiphy_get_dev_by_name("mdio"); > > + > > + puts("MV88E61XX "); > > + /* GPIO[0] output CLK125 for RGMII_REFCLK */ > > + bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x62 << 8) | 0xfe); > > + bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x68 << 8) | 7); > > + > > + /* Port 0-3 LED configuration: Table 80/82 */ > > + /* LED configuration: 7:4-green (8=Activity) 3:0 amber (8=Link) */ > > + bus->write(bus, 0x10, 0, 0x16, 0x8088); > > + bus->write(bus, 0x11, 0, 0x16, 0x8088); > > + bus->write(bus, 0x12, 0, 0x16, 0x8088); > > + bus->write(bus, 0x13, 0, 0x16, 0x8088); > > + } > > + > > if (phydev->drv->config) > > phydev->drv->config(phydev); > > > > return 0; > > } > > > > -#ifdef CONFIG_MV88E61XX_SWITCH > > -int mv88e61xx_hw_reset(struct phy_device *phydev) > > -{ > > - struct mii_dev *bus = phydev->bus; > > - > > - /* GPIO[0] output, CLK125 */ > > - debug("enabling RGMII_REFCLK\n"); > > - bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0, > > - 0x1a /*MV_SCRATCH_MISC*/, > > - (1 << 15) | (0x62 /*MV_GPIO_DIR*/ << 8) | 0xfe); > > - bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0, > > - 0x1a /*MV_SCRATCH_MISC*/, > > - (1 << 15) | (0x68 /*MV_GPIO01_CNTL*/ << 8) | 7); > > - > > - /* RGMII delay - Physical Control register bit[15:14] */ > > - debug("setting port%d RGMII rx/tx delay\n", CONFIG_MV88E61XX_CPU_PORT); > > - /* forced 1000mbps full-duplex link */ > > - bus->write(bus, 0x10 + CONFIG_MV88E61XX_CPU_PORT, 0, 1, 0xc0fe); > > - phydev->autoneg = AUTONEG_DISABLE; > > - phydev->speed = SPEED_1000; > > - phydev->duplex = DUPLEX_FULL; > > - > > - /* LED configuration: 7:4-green (8=Activity) 3:0 amber (8=Link) */ > > - bus->write(bus, 0x10, 0, 0x16, 0x8088); > > - bus->write(bus, 0x11, 0, 0x16, 0x8088); > > - bus->write(bus, 0x12, 0, 0x16, 0x8088); > > - bus->write(bus, 0x13, 0, 0x16, 0x8088); > > - > > - return 0; > > -} > > -#endif // CONFIG_MV88E61XX_SWITCH > > - > > #if defined(CONFIG_VIDEO_IPUV3) > > static void enable_hdmi(struct display_info_t const *dev) > > { > > diff --git a/configs/gwventana_gw5904_defconfig b/configs/gwventana_gw5904_defconfig > > index d25a8324b1df..9809bc691508 100644 > > --- a/configs/gwventana_gw5904_defconfig > > +++ b/configs/gwventana_gw5904_defconfig > > @@ -103,12 +103,11 @@ CONFIG_SUPPORT_EMMC_BOOT=y > > CONFIG_FSL_USDHC=y > > CONFIG_MTD=y > > CONFIG_PHYLIB=y > > -CONFIG_MV88E61XX_SWITCH=y > > -CONFIG_MV88E61XX_CPU_PORT=5 > > -CONFIG_MV88E61XX_PHY_PORTS=0xf > > -CONFIG_MV88E61XX_FIXED_PORTS=0x0 > > +CONFIG_MV88E61XX=y > > +CONFIG_PHY_FIXED=y > > CONFIG_DM_ETH=y > > CONFIG_DM_MDIO=y > > +CONFIG_DM_DSA=y > > CONFIG_E1000=y > > CONFIG_FEC_MXC=y > > CONFIG_MII=y > > -- > > 2.17.1 > > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support 2022-06-24 23:16 ` Tim Harvey @ 2022-06-25 0:13 ` Vladimir Oltean 2022-06-25 0:25 ` Tim Harvey 0 siblings, 1 reply; 26+ messages in thread From: Vladimir Oltean @ 2022-06-25 0:13 UTC (permalink / raw) To: tharvey Cc: Joe Hershberger, Ramon Fried, u-boot, Stefano Babic, Fabio Estevam, dl-uboot-imx, Marek BehĂșn, Chris Packham, Anatolij Gustschin On Fri, Jun 24, 2022 at 04:16:34PM -0700, Tim Harvey wrote: > On Fri, Jun 24, 2022 at 3:25 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > > > On Mon, May 23, 2022 at 11:25:49AM -0700, Tim Harvey wrote: > > > Add MV88E61XX DSA support: > > > - update dt: U-Boot dsa driver requires different device-tree syntax > > > than the linux driver in order to link the dsa ports to the mdio bus. > > > - update defconfig > > > - replace mv88e61xx_hw_reset weak override with board_phy_config support > > > for mv88e61xx configuration that is outside the scope of the DSA driver > > > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > > --- > > > v3: > > > - move mdio's mdio@0 subnode > > > v2: no changes > > > --- > > > arch/arm/dts/imx6qdl-gw5904.dtsi | 41 ++++++++++++++++++++ > > > board/gateworks/gw_ventana/gw_ventana.c | 50 +++++++++---------------- > > > configs/gwventana_gw5904_defconfig | 7 ++-- > > > 3 files changed, 62 insertions(+), 36 deletions(-) > > > > > > diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi b/arch/arm/dts/imx6qdl-gw5904.dtsi > > > index 286c7a9924c2..1b2f70d1ccb2 100644 > > > --- a/arch/arm/dts/imx6qdl-gw5904.dtsi > > > +++ b/arch/arm/dts/imx6qdl-gw5904.dtsi > > > @@ -219,6 +219,33 @@ > > > compatible = "marvell,mv88e6085"; > > > reg = <0>; > > > > > > + mdios { > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + mdio@0 { > > > > If you are going to follow this new model with a dedicated "mdios" subnode, > > I've consulted with Andrew Lunn and Florian Fainelli and there shouldn't > > be a problem to later make Linux accept this alternate binding format. > > But in that case, please match this OF node by a dedicated compatible > > string, like "marvell,mv88e6xxx-mdio-internal", so that there will be a > > way to differentiate this from the existing "marvell,mv88e6xxx-mdio-external" > > when support for that is added in U-Boot. > > > > Alternatively, to repeat myself, you can always follow the de-facto > > bindings for Linux mv88e6xxx which have: > > > > switch0: switch0@0 { > > compatible = "marvell,mv88e6190"; > > > > ports { > > #address-cells = <1>; > > #size-cells = <0>; > > > > ... > > }; > > > > mdio { // internal > > #address-cells = <1>; > > #size-cells = <0>; > > > > ... > > }; > > > > mdio1 { > > compatible = "marvell,mv88e6xxx-mdio-external"; > > #address-cells = <1>; > > #size-cells = <0>; > > > > ... > > }; > > }; > > > > Documentation/devicetree/bindings/net/dsa/marvell.txt shows en example > with just one child node under the internal mdio node: > > mdio { > #address-cells = <1>; > #size-cells = <0>; > switch1phy0: switch1phy0@0 { > reg = <0>; > interrupt-parent = <&switch0>; > interrupts = <0 IRQ_TYPE_LEVEL_HIGH>; > }; > }; > > Am I to assume I can add additional nodes there for the other ports > such as the following? > > mdio { > #address-cells = <1>; > #size-cells = <0>; > > switch1phy0: switch1phy0@0 { > reg = <0>; > }; > > switch1phy1: switch1phy1@1 { > reg = <1>; > }; > > switch1phy2: switch1phy2@2 { > reg = <2>; > }; > > switch1phy3: switch1phy3@3 { > reg = <3>; > }; > > ... > }; Sure, but name those PHY nodes "ethernet-phy@N" rather than "switchMphyN", as Documentation/devicetree/bindings/net/ethernet-phy.yaml requires. Many mistakes were made in writing mv88e6xxx device trees, we don't need to follow each and every one of them, only the important ones. ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support 2022-06-25 0:13 ` Vladimir Oltean @ 2022-06-25 0:25 ` Tim Harvey 0 siblings, 0 replies; 26+ messages in thread From: Tim Harvey @ 2022-06-25 0:25 UTC (permalink / raw) To: Vladimir Oltean Cc: Joe Hershberger, Ramon Fried, u-boot, Stefano Babic, Fabio Estevam, dl-uboot-imx, Marek BehĂșn, Chris Packham, Anatolij Gustschin On Fri, Jun 24, 2022 at 5:13 PM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > On Fri, Jun 24, 2022 at 04:16:34PM -0700, Tim Harvey wrote: > > On Fri, Jun 24, 2022 at 3:25 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote: > > > > > > On Mon, May 23, 2022 at 11:25:49AM -0700, Tim Harvey wrote: > > > > Add MV88E61XX DSA support: > > > > - update dt: U-Boot dsa driver requires different device-tree syntax > > > > than the linux driver in order to link the dsa ports to the mdio bus. > > > > - update defconfig > > > > - replace mv88e61xx_hw_reset weak override with board_phy_config support > > > > for mv88e61xx configuration that is outside the scope of the DSA driver > > > > > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > > > --- > > > > v3: > > > > - move mdio's mdio@0 subnode > > > > v2: no changes > > > > --- > > > > arch/arm/dts/imx6qdl-gw5904.dtsi | 41 ++++++++++++++++++++ > > > > board/gateworks/gw_ventana/gw_ventana.c | 50 +++++++++---------------- > > > > configs/gwventana_gw5904_defconfig | 7 ++-- > > > > 3 files changed, 62 insertions(+), 36 deletions(-) > > > > > > > > diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi b/arch/arm/dts/imx6qdl-gw5904.dtsi > > > > index 286c7a9924c2..1b2f70d1ccb2 100644 > > > > --- a/arch/arm/dts/imx6qdl-gw5904.dtsi > > > > +++ b/arch/arm/dts/imx6qdl-gw5904.dtsi > > > > @@ -219,6 +219,33 @@ > > > > compatible = "marvell,mv88e6085"; > > > > reg = <0>; > > > > > > > > + mdios { > > > > + #address-cells = <1>; > > > > + #size-cells = <0>; > > > > + > > > > + mdio@0 { > > > > > > If you are going to follow this new model with a dedicated "mdios" subnode, > > > I've consulted with Andrew Lunn and Florian Fainelli and there shouldn't > > > be a problem to later make Linux accept this alternate binding format. > > > But in that case, please match this OF node by a dedicated compatible > > > string, like "marvell,mv88e6xxx-mdio-internal", so that there will be a > > > way to differentiate this from the existing "marvell,mv88e6xxx-mdio-external" > > > when support for that is added in U-Boot. > > > > > > Alternatively, to repeat myself, you can always follow the de-facto > > > bindings for Linux mv88e6xxx which have: > > > > > > switch0: switch0@0 { > > > compatible = "marvell,mv88e6190"; > > > > > > ports { > > > #address-cells = <1>; > > > #size-cells = <0>; > > > > > > ... > > > }; > > > > > > mdio { // internal > > > #address-cells = <1>; > > > #size-cells = <0>; > > > > > > ... > > > }; > > > > > > mdio1 { > > > compatible = "marvell,mv88e6xxx-mdio-external"; > > > #address-cells = <1>; > > > #size-cells = <0>; > > > > > > ... > > > }; > > > }; > > > > > > > Documentation/devicetree/bindings/net/dsa/marvell.txt shows en example > > with just one child node under the internal mdio node: > > > > mdio { > > #address-cells = <1>; > > #size-cells = <0>; > > switch1phy0: switch1phy0@0 { > > reg = <0>; > > interrupt-parent = <&switch0>; > > interrupts = <0 IRQ_TYPE_LEVEL_HIGH>; > > }; > > }; > > > > Am I to assume I can add additional nodes there for the other ports > > such as the following? > > > > mdio { > > #address-cells = <1>; > > #size-cells = <0>; > > > > switch1phy0: switch1phy0@0 { > > reg = <0>; > > }; > > > > switch1phy1: switch1phy1@1 { > > reg = <1>; > > }; > > > > switch1phy2: switch1phy2@2 { > > reg = <2>; > > }; > > > > switch1phy3: switch1phy3@3 { > > reg = <3>; > > }; > > > > ... > > }; > > Sure, but name those PHY nodes "ethernet-phy@N" rather than "switchMphyN", > as Documentation/devicetree/bindings/net/ethernet-phy.yaml requires. > Many mistakes were made in writing mv88e6xxx device trees, we don't need > to follow each and every one of them, only the important ones. Ok, I'll attempt to go that route. I'm out of the office next week so expect a v4 sometime after that. Best Regards, Tim ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2022-08-08 15:22 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-23 18:25 [PATCH v3 0/8] Add MV88E61xx DSA driver and use on gwventana Tim Harvey 2022-05-23 18:25 ` [PATCH v3 1/8] net: mdio-uclass: scan for dm mdio children on post-bind Tim Harvey 2022-05-23 18:25 ` [PATCH v3 2/8] net: dsa: move cpu port probe to dsa_post_probe Tim Harvey 2022-05-23 18:25 ` [PATCH v3 3/8] net: dsa: ensure dsa driver has proper ops Tim Harvey 2022-05-23 18:25 ` [PATCH v3 4/8] net: dsa: allow rcv() and xmit() to be optional Tim Harvey 2022-05-23 18:25 ` [PATCH v3 5/8] net: ksz9477: remove unnecessary xmit and recv functions Tim Harvey 2022-05-23 18:25 ` [PATCH v3 6/8] net: fec: add support for DM_MDIO Tim Harvey 2022-05-23 18:25 ` [PATCH v3 7/8] net: add MV88E61xx DSA driver Tim Harvey 2022-06-20 11:58 ` Vladimir Oltean 2022-06-20 23:37 ` Tim Harvey 2022-06-21 7:21 ` Vladimir Oltean 2022-06-21 15:11 ` Tim Harvey 2022-06-23 12:43 ` Vladimir Oltean 2022-08-07 0:07 ` Ramon Fried 2022-08-08 15:21 ` Tim Harvey 2022-05-23 18:25 ` [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support Tim Harvey 2022-06-14 17:00 ` Tim Harvey 2022-06-14 18:55 ` Vladimir Oltean 2022-06-20 11:42 ` Vladimir Oltean 2022-06-21 16:57 ` Tim Harvey 2022-06-23 12:42 ` Vladimir Oltean 2022-06-23 16:07 ` Tim Harvey 2022-06-24 10:25 ` Vladimir Oltean 2022-06-24 23:16 ` Tim Harvey 2022-06-25 0:13 ` Vladimir Oltean 2022-06-25 0:25 ` Tim Harvey
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.