| Message ID | 20230703124440.391970-3-eichest@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add a driver for the Marvell 88Q2110 PHY | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
> index 93b8efc792273..2913b145d5406 100644 > --- a/drivers/net/phy/Kconfig > +++ b/drivers/net/phy/Kconfig > @@ -223,6 +223,12 @@ config MARVELL_88X2222_PHY > Support for the Marvell 88X2222 Dual-port Multi-speed Ethernet > Transceiver. > > +config MARVELL_88Q2XXX_PHY > + tristate "Marvell 88Q2XXX PHY" > + help > + Support for the Marvell Automotive 88Q2XXX 100/1000BASE-T1 Ethernet > + PHYs. I suggest you follow the pattern of the 88X2222. Support for the Marvell 88Q2XXX 100/1000BASE-T1 Automotive Ethernet PHYs. These entries are sorted by these strings, so should be before the 88Q2XXX. > + > config MAXLINEAR_GPHY > tristate "Maxlinear Ethernet PHYs" > select POLYNOMIAL if HWMON > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile > index f289ab16a1dab..15d1908fd5cb7 100644 > --- a/drivers/net/phy/Makefile > +++ b/drivers/net/phy/Makefile > @@ -67,6 +67,7 @@ obj-$(CONFIG_LXT_PHY) += lxt.o > obj-$(CONFIG_MARVELL_10G_PHY) += marvell10g.o > obj-$(CONFIG_MARVELL_PHY) += marvell.o > obj-$(CONFIG_MARVELL_88X2222_PHY) += marvell-88x2222.o > +obj-$(CONFIG_MARVELL_88Q2XXX_PHY) += marvell-88q2xxx.o These are sorted by the CONFIG_*, so it should also be before the 88X2XXX > +static int mv88q2xxx_soft_reset(struct phy_device *phydev) > +{ > + phy_write_mmd(phydev, 3, 0x0900, 0x8000); Please use the #defines, e.g. MDIO_MMD_PCS. 0x900 is listed in IEEE 802.3 2022. It is a standard register. Please add a #define for it next to all the others. You can also add BIT() macros for PMA/PMD reset, and maybe Tx Disable, low power. > + > + return 0; > +} > + > +static void init_fast_ethernet(struct phy_device *phydev) > +{ > + u16 value = phy_read_mmd(phydev, 1, 0x0834); 2100 is also a standard register. > + > + value = value & 0xFFF0; > + > + phy_write_mmd(phydev, 1, 0x0834, value); > +} > + > +static void init_gbit_ethernet(struct phy_device *phydev) > +{ > + u16 value = phy_read_mmd(phydev, 1, 0x0834); > + > + value = (value & 0xFFF0) | 0x0001; > + > + phy_write_mmd(phydev, 1, 0x0834, value); > +} > + > +static int setup_master_slave(struct phy_device *phydev) > +{ > + u16 reg_data = phy_read_mmd(phydev, 1, 0x0834); > + > + switch (phydev->master_slave_set) { > + case MASTER_SLAVE_CFG_MASTER_FORCE: > + case MASTER_SLAVE_CFG_MASTER_PREFERRED: > + reg_data |= 0x4000; > + break; > + case MASTER_SLAVE_CFG_SLAVE_PREFERRED: > + case MASTER_SLAVE_CFG_SLAVE_FORCE: > + reg_data &= ~0x4000; > + break; > + case MASTER_SLAVE_CFG_UNKNOWN: > + case MASTER_SLAVE_CFG_UNSUPPORTED: > + return 0; > + default: > + phydev_warn(phydev, "Unsupported Master/Slave mode\n"); > + return -EOPNOTSUPP; > + } > + > + phy_write_mmd(phydev, 1, 0x0834, reg_data); > + > + return 0; > +} > + > +static int mv88q2xxx_config_aneg(struct phy_device *phydev) > +{ > + int ret; > + > + if (phydev->speed == SPEED_100) > + init_fast_ethernet(phydev); > + else if (phydev->speed == SPEED_1000) > + init_gbit_ethernet(phydev); > + > + ret = setup_master_slave(phydev); > + if (ret) > + return ret; > + > + mv88q2xxx_soft_reset(phydev); > + > + return 0; > +} > + > +static int mv88q2xxx_config_init(struct phy_device *phydev) > +{ > + return mv88q2xxx_config_aneg(phydev); > +} > + > +static int get_speed(struct phy_device *phydev) > +{ > + u16 value = 0; > + > + if (phydev->autoneg) > + value = (phy_read_mmd(phydev, 7, 0x801a) & 0x4000) >> 14; > + else > + value = (phy_read_mmd(phydev, 1, 0x0834) & 0x1); > + > + return value ? SPEED_1000 : SPEED_100; > +} > + > +static int check_link(struct phy_device *phydev) > +{ > + u16 ret1, ret2; > + > + if (phydev->speed == SPEED_1000) { > + ret1 = phy_read_mmd(phydev, 3, 0x0901); > + ret1 = phy_read_mmd(phydev, 3, 0x0901); > + ret2 = phy_read_mmd(phydev, 7, 0x8001); > + } else { > + ret1 = phy_read_mmd(phydev, 3, 0x8109); > + ret2 = phy_read_mmd(phydev, 3, 0x8108); > + } > + > + return (0x0 != (ret1 & 0x0004)) && (0x0 != (ret2 & 0x3000)) ? 1 : 0; > +} > + > +static int read_master_slave(struct phy_device *phydev) > +{ > + int reg; > + > + phydev->master_slave_get = MASTER_SLAVE_CFG_UNKNOWN; > + phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN; > + > + reg = phy_read_mmd(phydev, 7, 0x8001); > + if (reg & (1 << 14)) { > + phydev->master_slave_get = MASTER_SLAVE_CFG_MASTER_FORCE; > + phydev->master_slave_state = MASTER_SLAVE_STATE_MASTER; > + } else { > + phydev->master_slave_get = MASTER_SLAVE_CFG_SLAVE_FORCE; > + phydev->master_slave_state = MASTER_SLAVE_STATE_SLAVE; > + } > + > + return 0; > +} > + > +static int mv88q2xxx_read_status(struct phy_device *phydev) > +{ > + int ret; > + > + ret = genphy_update_link(phydev); > + if (ret) > + return ret; > + > + phydev->link = check_link(phydev); > + phydev->speed = get_speed(phydev); > + > + ret = read_master_slave(phydev); > + if (ret) > + return ret; > + > + return 0; > +} So all these functions can be placed in phy-c45.c, since they are generic to any 1000BaseT1 PHY. Please look at the other functions in there, and try the fit the patterns. Please also look at genphy_c45_pma_read_abilities() and extend it to detect the features of this PHY. > +static int mv88q2xxx_probe(struct phy_device *phydev) > +{ > + if (!phydev->is_c45) > + return -ENODEV; Why? > + > + return 0; > +} > + > +static int mv88q2xxxx_cable_test_start(struct phy_device *phydev) > +{ > + return 0; > +} > + > +static int mv88q2xxxx_cable_test_get_status(struct phy_device *phydev, > + bool *finished) > +{ > + ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A, > + ETHTOOL_A_CABLE_RESULT_CODE_OK); > + return 0; > +} There is no need to implement them if you don't have anything useful to return. > +static int mv88q2xxxx_get_sqi(struct phy_device *phydev) > +{ > + u16 value; > + > + if (phydev->speed == SPEED_100) > + value = (phy_read_mmd(phydev, 3, 0x8230) >> 12) & 0x0F; > + else > + value = phy_read_mmd(phydev, 3, 0xfc88) & 0x0F; This looks proprietary to this PHY. However the open alliance does have some standards in this area. Please check to see if they define registers. Andrew
On Mon, Jul 03, 2023 at 02:44:40PM +0200, Stefan Eichenberger wrote: > Add a driver for the Marvell 88Q2110. This driver is minimalistic, but > already allows to detect the link, switch between 100BASE-T1 and > 1000BASE-T1 and switch between master and slave mode. Autonegotiation > supported by the PHY is not yet implemented. > > Signed-off-by: Stefan Eichenberger <eichest@gmail.com> ... > +static struct mdio_device_id __maybe_unused mv88q2xxx_tbl[] = { > + { MARVELL_PHY_ID_88Q2110, MARVELL_PHY_ID_MASK }, > + { /*sentinel*/ }, No comma for a terminator entry. Francesco
On Mon, Jul 03, 2023 at 02:44:40PM +0200, Stefan Eichenberger wrote: > Add a driver for the Marvell 88Q2110. This driver is minimalistic, but > already allows to detect the link, switch between 100BASE-T1 and > 1000BASE-T1 and switch between master and slave mode. Autonegotiation > supported by the PHY is not yet implemented. > > Signed-off-by: Stefan Eichenberger <eichest@gmail.com> ... > --- /dev/null > +++ b/drivers/net/phy/marvell-88q2xxx.c > @@ -0,0 +1,217 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Marvell 88Q2XXX automotive 100BASE-T1/1000BASE-T1 PHY driver > + */ > + > +#include <linux/marvell_phy.h> > +#include <linux/phy.h> > +#include <linux/ethtool_netlink.h> sort? > +#define MARVELL_PHY_ID_88Q2110 0x002b0981 > + > +static int mv88q2xxx_soft_reset(struct phy_device *phydev) > +{ > + phy_write_mmd(phydev, 3, 0x0900, 0x8000); > + > + return 0; Should we `return phy_write_mmd()` to allow error propagation? This eventually applies to other functions. Francesco
On Mon, Jul 03, 2023 at 02:44:40PM +0200, Stefan Eichenberger wrote: > +static int check_link(struct phy_device *phydev) > +{ > + u16 ret1, ret2; > + > + if (phydev->speed == SPEED_1000) { > + ret1 = phy_read_mmd(phydev, 3, 0x0901); > + ret1 = phy_read_mmd(phydev, 3, 0x0901); This looks like some kind of BMSR register, where the link status latches on failure until the following read. By reading the register twice, you are discarding the information that the link _had_ failed since we last read it. You don't want to do that - it could come up with different parameters and because the link to phylib appears to remain up, it won't respond to the new link parameters. Always report the latched link status, not the current status.
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index 93b8efc792273..2913b145d5406 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -223,6 +223,12 @@ config MARVELL_88X2222_PHY Support for the Marvell 88X2222 Dual-port Multi-speed Ethernet Transceiver. +config MARVELL_88Q2XXX_PHY + tristate "Marvell 88Q2XXX PHY" + help + Support for the Marvell Automotive 88Q2XXX 100/1000BASE-T1 Ethernet + PHYs. + config MAXLINEAR_GPHY tristate "Maxlinear Ethernet PHYs" select POLYNOMIAL if HWMON diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile index f289ab16a1dab..15d1908fd5cb7 100644 --- a/drivers/net/phy/Makefile +++ b/drivers/net/phy/Makefile @@ -67,6 +67,7 @@ obj-$(CONFIG_LXT_PHY) += lxt.o obj-$(CONFIG_MARVELL_10G_PHY) += marvell10g.o obj-$(CONFIG_MARVELL_PHY) += marvell.o obj-$(CONFIG_MARVELL_88X2222_PHY) += marvell-88x2222.o +obj-$(CONFIG_MARVELL_88Q2XXX_PHY) += marvell-88q2xxx.o obj-$(CONFIG_MAXLINEAR_GPHY) += mxl-gpy.o obj-$(CONFIG_MEDIATEK_GE_PHY) += mediatek-ge.o obj-$(CONFIG_MESON_GXL_PHY) += meson-gxl.o diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c new file mode 100644 index 0000000000000..637697e9f19fa --- /dev/null +++ b/drivers/net/phy/marvell-88q2xxx.c @@ -0,0 +1,217 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Marvell 88Q2XXX automotive 100BASE-T1/1000BASE-T1 PHY driver + */ + +#include <linux/marvell_phy.h> +#include <linux/phy.h> +#include <linux/ethtool_netlink.h> + +#define MARVELL_PHY_ID_88Q2110 0x002b0981 + +static int mv88q2xxx_soft_reset(struct phy_device *phydev) +{ + phy_write_mmd(phydev, 3, 0x0900, 0x8000); + + return 0; +} + +static void init_fast_ethernet(struct phy_device *phydev) +{ + u16 value = phy_read_mmd(phydev, 1, 0x0834); + + value = value & 0xFFF0; + + phy_write_mmd(phydev, 1, 0x0834, value); +} + +static void init_gbit_ethernet(struct phy_device *phydev) +{ + u16 value = phy_read_mmd(phydev, 1, 0x0834); + + value = (value & 0xFFF0) | 0x0001; + + phy_write_mmd(phydev, 1, 0x0834, value); +} + +static int setup_master_slave(struct phy_device *phydev) +{ + u16 reg_data = phy_read_mmd(phydev, 1, 0x0834); + + switch (phydev->master_slave_set) { + case MASTER_SLAVE_CFG_MASTER_FORCE: + case MASTER_SLAVE_CFG_MASTER_PREFERRED: + reg_data |= 0x4000; + break; + case MASTER_SLAVE_CFG_SLAVE_PREFERRED: + case MASTER_SLAVE_CFG_SLAVE_FORCE: + reg_data &= ~0x4000; + break; + case MASTER_SLAVE_CFG_UNKNOWN: + case MASTER_SLAVE_CFG_UNSUPPORTED: + return 0; + default: + phydev_warn(phydev, "Unsupported Master/Slave mode\n"); + return -EOPNOTSUPP; + } + + phy_write_mmd(phydev, 1, 0x0834, reg_data); + + return 0; +} + +static int mv88q2xxx_config_aneg(struct phy_device *phydev) +{ + int ret; + + if (phydev->speed == SPEED_100) + init_fast_ethernet(phydev); + else if (phydev->speed == SPEED_1000) + init_gbit_ethernet(phydev); + + ret = setup_master_slave(phydev); + if (ret) + return ret; + + mv88q2xxx_soft_reset(phydev); + + return 0; +} + +static int mv88q2xxx_config_init(struct phy_device *phydev) +{ + return mv88q2xxx_config_aneg(phydev); +} + +static int get_speed(struct phy_device *phydev) +{ + u16 value = 0; + + if (phydev->autoneg) + value = (phy_read_mmd(phydev, 7, 0x801a) & 0x4000) >> 14; + else + value = (phy_read_mmd(phydev, 1, 0x0834) & 0x1); + + return value ? SPEED_1000 : SPEED_100; +} + +static int check_link(struct phy_device *phydev) +{ + u16 ret1, ret2; + + if (phydev->speed == SPEED_1000) { + ret1 = phy_read_mmd(phydev, 3, 0x0901); + ret1 = phy_read_mmd(phydev, 3, 0x0901); + ret2 = phy_read_mmd(phydev, 7, 0x8001); + } else { + ret1 = phy_read_mmd(phydev, 3, 0x8109); + ret2 = phy_read_mmd(phydev, 3, 0x8108); + } + + return (0x0 != (ret1 & 0x0004)) && (0x0 != (ret2 & 0x3000)) ? 1 : 0; +} + +static int read_master_slave(struct phy_device *phydev) +{ + int reg; + + phydev->master_slave_get = MASTER_SLAVE_CFG_UNKNOWN; + phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN; + + reg = phy_read_mmd(phydev, 7, 0x8001); + if (reg & (1 << 14)) { + phydev->master_slave_get = MASTER_SLAVE_CFG_MASTER_FORCE; + phydev->master_slave_state = MASTER_SLAVE_STATE_MASTER; + } else { + phydev->master_slave_get = MASTER_SLAVE_CFG_SLAVE_FORCE; + phydev->master_slave_state = MASTER_SLAVE_STATE_SLAVE; + } + + return 0; +} + +static int mv88q2xxx_read_status(struct phy_device *phydev) +{ + int ret; + + ret = genphy_update_link(phydev); + if (ret) + return ret; + + phydev->link = check_link(phydev); + phydev->speed = get_speed(phydev); + + ret = read_master_slave(phydev); + if (ret) + return ret; + + return 0; +} + +static int mv88q2xxx_probe(struct phy_device *phydev) +{ + if (!phydev->is_c45) + return -ENODEV; + + return 0; +} + +static int mv88q2xxxx_cable_test_start(struct phy_device *phydev) +{ + return 0; +} + +static int mv88q2xxxx_cable_test_get_status(struct phy_device *phydev, + bool *finished) +{ + ethnl_cable_test_result(phydev, ETHTOOL_A_CABLE_PAIR_A, + ETHTOOL_A_CABLE_RESULT_CODE_OK); + return 0; +} + +static int mv88q2xxxx_get_sqi(struct phy_device *phydev) +{ + u16 value; + + if (phydev->speed == SPEED_100) + value = (phy_read_mmd(phydev, 3, 0x8230) >> 12) & 0x0F; + else + value = phy_read_mmd(phydev, 3, 0xfc88) & 0x0F; + + return value; +} + +static int mv88q2xxxx_get_sqi_max(struct phy_device *phydev) +{ + return 15; +} + +static struct phy_driver mv88q2xxx_driver[] = { + { + .phy_id = MARVELL_PHY_ID_88Q2110, + .phy_id_mask = MARVELL_PHY_ID_MASK, + .features = PHY_GBIT_T1_FEATURES, + .name = "mv88q2110", + .probe = mv88q2xxx_probe, + .soft_reset = mv88q2xxx_soft_reset, + .config_init = mv88q2xxx_config_init, + .read_status = mv88q2xxx_read_status, + .config_aneg = mv88q2xxx_config_aneg, + .cable_test_start = mv88q2xxxx_cable_test_start, + .cable_test_get_status = mv88q2xxxx_cable_test_get_status, + .set_loopback = genphy_c45_loopback, + .get_sqi = mv88q2xxxx_get_sqi, + .get_sqi_max = mv88q2xxxx_get_sqi_max, + }, +}; + +module_phy_driver(mv88q2xxx_driver); + +static struct mdio_device_id __maybe_unused mv88q2xxx_tbl[] = { + { MARVELL_PHY_ID_88Q2110, MARVELL_PHY_ID_MASK }, + { /*sentinel*/ }, +}; +MODULE_DEVICE_TABLE(mdio, mv88q2xxx_tbl); + +MODULE_DESCRIPTION("Marvell Automotive 100BASE-T1/1000BASE-T1 Ethernet PHY driver (MV88Q2xxx)"); +MODULE_LICENSE("GPL");
Add a driver for the Marvell 88Q2110. This driver is minimalistic, but already allows to detect the link, switch between 100BASE-T1 and 1000BASE-T1 and switch between master and slave mode. Autonegotiation supported by the PHY is not yet implemented. Signed-off-by: Stefan Eichenberger <eichest@gmail.com> --- drivers/net/phy/Kconfig | 6 + drivers/net/phy/Makefile | 1 + drivers/net/phy/marvell-88q2xxx.c | 217 ++++++++++++++++++++++++++++++ 3 files changed, 224 insertions(+) create mode 100644 drivers/net/phy/marvell-88q2xxx.c