| Message ID | 20230710205900.52894-5-eichest@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add a driver for the Marvell 88Q2110 PHY | expand |
> +static int mv88q2xxx_soft_reset(struct phy_device *phydev) > +{ > + return phy_write_mmd(phydev, MDIO_MMD_PCS, > + MDIO_PCS_1000BT1_CTRL, MDIO_PCS_1000BT1_CTRL_RESET); > +} Does this bit clear on its own when the reset has completed? When performing a C22 soft reset, the code polls waiting for the bit to clear. Otherwise there is a danger you start writing other registers while it is still resetting. > +static int mv88q2xxx_read_link(struct phy_device *phydev) > +{ > + u16 ret1, ret2; > + > + /* The 88Q2XXX PHYs do not have the PMA/PMD status register available, > + * therefore we need to read the link status from the vendor specific > + * registers. > + */ > + if (phydev->speed == SPEED_1000) { > + /* Read twice to clear the latched status */ > + ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT); > + ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT); This is generally wrong. See for example genphy_update_link() and genphy_c45_read_link(). > +static int mv88q2xxx_probe(struct phy_device *phydev) > +{ > + return 0; > +} If it does nothing, it should not be needed. Andrew --- pw-bot: cr
On Mon, Jul 10, 2023 at 10:59:00PM +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 int mv88q2xxx_read_link(struct phy_device *phydev) > +{ > + u16 ret1, ret2; > + > + /* The 88Q2XXX PHYs do not have the PMA/PMD status register available, > + * therefore we need to read the link status from the vendor specific > + * registers. > + */ > + if (phydev->speed == SPEED_1000) { > + /* Read twice to clear the latched status */ > + ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT); > + ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT); > + /* Read vendor specific Auto-Negotiation status register to get > + * local and remote receiver status > + */ > + ret2 = phy_read_mmd(phydev, MDIO_MMD_AN, 0x8001); > + } else { > + /* Read vendor specific status registers, the registers are not > + * documented but they can be found in the Software > + * Initialization Guide > + */ > + ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, 0x8109); > + ret2 = phy_read_mmd(phydev, MDIO_MMD_PCS, 0x8108); > + } you are ignoring errors from phy_read_mmd() here > + > + /* Check the link status according to Software Initialization Guide */ > + return (0x0 != (ret1 & 0x0004)) && (0x0 != (ret2 & 0x3000)) ? 1 : 0; > +} > + > +static int mv88q2xxx_read_status(struct phy_device *phydev) > +{ > + int ret; > + > + phydev->link = mv88q2xxx_read_link(phydev); > + > + ret = genphy_c45_read_pma(phydev); > + if (ret) > + return ret; return genphy_c45_read_pma(phydev); > +static int mv88q2xxx_config_aneg(struct phy_device *phydev) > +{ > + int ret; > + > + ret = genphy_c45_config_aneg(phydev); > + if (ret) > + return ret; > + > + ret = mv88q2xxx_soft_reset(phydev); > + if (ret) > + return ret; return mv88q2xxx_soft_reset(phydev); > +static int mv88q2xxx_probe(struct phy_device *phydev) > +{ > + return 0; > +} not needed? just remove it, if nothing to be done > +static int mv88q2xxxx_get_sqi(struct phy_device *phydev) > +{ > + u16 value; > + > + if (phydev->speed == SPEED_100) { > + /* Read the SQI from the vendor specific receiver status > + * register > + */ > + value = (phy_read_mmd(phydev, MDIO_MMD_PCS, 0x8230) >> 12) & 0x0F; > + } else { > + /* Read from vendor specific registers, they are not documented > + * but can be found in the Software Initialization Guide. Only > + * revisions >= A0 are supported. > + */ > + phy_modify_mmd(phydev, MDIO_MMD_PCS, 0xFC5D, 0x00FF, 0x00AC); > + value = phy_read_mmd(phydev, MDIO_MMD_PCS, 0xfc88) & 0x0F; > + } errors from phy_modify_mmd/phy_read_mmd ignored?
> + if (phydev->speed == SPEED_1000) { > + /* Read twice to clear the latched status */ > + ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT); > + ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT); > + /* Read vendor specific Auto-Negotiation status register to get > + * local and remote receiver status > + */ > + ret2 = phy_read_mmd(phydev, MDIO_MMD_AN, 0x8001); > + } else { > + /* Read vendor specific status registers, the registers are not > + * documented but they can be found in the Software > + * Initialization Guide > + */ > + ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, 0x8109); > + ret2 = phy_read_mmd(phydev, MDIO_MMD_PCS, 0x8108); > + } > + > + /* Check the link status according to Software Initialization Guide */ > + return (0x0 != (ret1 & 0x0004)) && (0x0 != (ret2 & 0x3000)) ? 1 : 0; MDIO_PCS_1000BT1_STAT bit 2 is "Receive polarity" as defined in 802.3? Please add a #define for it. Please also add a #define for 0x3000 so we have some idea what is means. Andrew
On Mon, Jul 10, 2023 at 10:59:00PM +0200, Stefan Eichenberger wrote: > + /* The 88Q2XXX PHYs do not have the PMA/PMD status register available, > + * therefore we need to read the link status from the vendor specific > + * registers. > + */ > + if (phydev->speed == SPEED_1000) { > + /* Read twice to clear the latched status */ > + ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT); > + ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT); Please address my comment on this from v1, sent today. (Sorry it's late, but I've been on vacation since the start of July... which has now been disrupted. Presently "stuck" out on the English canals in beautiful countryside due to a lock failure, waiting for it to be repaired! :D Then I'll be taking the remainder of the vacation because I'll again be moving the boat!)
Hi Andrew, Thanks a lot for the review and all the hints, I have one short question below. > > +static int mv88q2xxx_read_link(struct phy_device *phydev) > > +{ > > + u16 ret1, ret2; > > + > > + /* The 88Q2XXX PHYs do not have the PMA/PMD status register available, > > + * therefore we need to read the link status from the vendor specific > > + * registers. > > + */ > > + if (phydev->speed == SPEED_1000) { > > + /* Read twice to clear the latched status */ > > + ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT); > > + ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT); > > This is generally wrong. See for example genphy_update_link() and > genphy_c45_read_link(). > Would something like this look fine to you? The issue is that I mix realtime data with latched data because the local and remote rx status is only available in realtime from what I can find in the datasheet. This would be for gbit, I split that up compared to the last version: static int mv88q2xxx_read_link_gbit(struct phy_device *phydev) { int ret1, ret2; /* The link state is latched low so that momentary link drops can be * detected. Do not double-read the status in polling mode to detect * such short link drops except the link was already down. In case we * are not polling, we always read the realtime status. */ if (!phy_polling_mode(phydev) || !phydev->link) { ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT); if (ret1 < 0) return ret1; } ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT); if (ret1 < 0) return ret1; /* Read vendor specific Auto-Negotiation status register to get local * and remote receiver status according to software initialization * guide. */ ret2 = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_MMD_AN_MV_STATUS); if (ret2 < 0) return ret2; /* Check if we have link and if the remote and local receiver are ok */ return (ret1 & MDIO_PCS_1000BT1_STAT_LINK) && (ret2 & MDIO_MMD_AN_MV_STATUS_LOCAL_RX) && (ret2 & MDIO_MMD_AN_MV_STATUS_REMOTE_RX); } With this we will detect link loss in polling mode and read the realtime status in non-polling mode. Compared to genphy_c45_read_link we will not immediately return "link up" in non polling mode but always do the second read to get the realtime link status. If we are only interested in the link status we could also skip the remote and local receiver check. However, as I understand the software initialization guide it could be that the receivers are not ready in that moment. Regards, Stefan
On Thu, Jul 13, 2023 at 11:42:12AM +0200, Stefan Eichenberger wrote: > Hi Andrew, > > Thanks a lot for the review and all the hints, I have one short question > below. > > > > +static int mv88q2xxx_read_link(struct phy_device *phydev) > > > +{ > > > + u16 ret1, ret2; > > > + > > > + /* The 88Q2XXX PHYs do not have the PMA/PMD status register available, > > > + * therefore we need to read the link status from the vendor specific > > > + * registers. > > > + */ > > > + if (phydev->speed == SPEED_1000) { > > > + /* Read twice to clear the latched status */ > > > + ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT); > > > + ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT); > > > > This is generally wrong. See for example genphy_update_link() and > > genphy_c45_read_link(). > > Would something like this look fine to you? The issue is that I mix > realtime data with latched data because the local and remote rx status > is only available in realtime from what I can find in the datasheet. > This would be for gbit, I split that up compared to the last version: I've never really understood this kind of reaction from people. A bit of example code gets suggested, and then the code gets sort of used as a half-hearted template... > static int mv88q2xxx_read_link_gbit(struct phy_device *phydev) > { > int ret1, ret2; > > /* The link state is latched low so that momentary link drops can be > * detected. Do not double-read the status in polling mode to detect > * such short link drops except the link was already down. In case we > * are not polling, we always read the realtime status. > */ > if (!phy_polling_mode(phydev) || !phydev->link) { > ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT); > if (ret1 < 0) > return ret1; Both genphy_update_link() and genphy_c45_read_link() check here whether the link is up, and if it is, it bypasses the second read (because there's no point re-reading it.) I've no idea why you dropped that optimisation. MDIO accesses aren't the cheapest thing... > With this we will detect link loss in polling mode and read the realtime > status in non-polling mode. Compared to genphy_c45_read_link we will not > immediately return "link up" in non polling mode but always do the > second read to get the realtime link status. Why do you think that's better? "Link" only latches low, and the entire point of that behaviour is so that management software can detect when the link has failed at some point since it last read the link status. There is only any point in re-reading the status register if on the first read it reports that the link has failed, and only then if we already knew that the link has failed. If we're using interrupt mode, then we need the current link status but that's only because of the way phylib has been written. > If we are only interested in the link status we could also skip the > remote and local receiver check. However, as I understand the software > initialization guide it could be that the receivers are not ready in > that moment. With copper PHYs, link up status means that the link is ready to pass data. Is this not the case with T1 PHYs?
Hi Russell, Thanks for your reply. On Thu, Jul 13, 2023 at 11:14:23AM +0100, Russell King (Oracle) wrote: > On Thu, Jul 13, 2023 at 11:42:12AM +0200, Stefan Eichenberger wrote: > > With this we will detect link loss in polling mode and read the realtime > > status in non-polling mode. Compared to genphy_c45_read_link we will not > > immediately return "link up" in non polling mode but always do the > > second read to get the realtime link status. > > Why do you think that's better? "Link" only latches low, and the > entire point of that behaviour is so that management software can > detect when the link has failed at some point since it last read > the link status. > > There is only any point in re-reading the status register if on the > first read it reports that the link has failed, and only then if we > already knew that the link has failed. > > If we're using interrupt mode, then we need the current link status > but that's only because of the way phylib has been written. > I agree with you. I missunderstood how the link status worked. I thought it will always show the status since the last read, independent of whether it went up or down. But it only latches link down. > > If we are only interested in the link status we could also skip the > > remote and local receiver check. However, as I understand the software > > initialization guide it could be that the receivers are not ready in > > that moment. > > With copper PHYs, link up status means that the link is ready to pass > data. Is this not the case with T1 PHYs? If I interpret their documentation correctly, they differentiate between PMA and PCS. The PMA link status could be up while the PCS link status is not. Maybe when the cable test is running this happens. Therefore, I keep the read for MDIO_MMD_AN_MV_STAT for now. Regards, Stefan
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index 78e6981650d94..87b8238587173 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -217,6 +217,12 @@ config MARVELL_10G_PHY help Support for the Marvell Alaska MV88X3310 and compatible PHYs. +config MARVELL_88Q2XXX_PHY + tristate "Marvell 88Q2XXX PHY" + help + Support for the Marvell 88Q2XXX 100/1000BASE-T1 Automotive Ethernet + PHYs. + config MARVELL_88X2222_PHY tristate "Marvell 88X2222 PHY" help diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile index 2fe51ea83babe..35142780fc9da 100644 --- a/drivers/net/phy/Makefile +++ b/drivers/net/phy/Makefile @@ -66,6 +66,7 @@ obj-$(CONFIG_LSI_ET1011C_PHY) += et1011c.o obj-$(CONFIG_LXT_PHY) += lxt.o obj-$(CONFIG_MARVELL_10G_PHY) += marvell10g.o obj-$(CONFIG_MARVELL_PHY) += marvell.o +obj-$(CONFIG_MARVELL_88Q2XXX_PHY) += marvell-88q2xxx.o obj-$(CONFIG_MARVELL_88X2222_PHY) += marvell-88x2222.o obj-$(CONFIG_MAXLINEAR_GPHY) += mxl-gpy.o obj-$(CONFIG_MEDIATEK_GE_PHY) += mediatek-ge.o diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c new file mode 100644 index 0000000000000..a073124c33f5e --- /dev/null +++ b/drivers/net/phy/marvell-88q2xxx.c @@ -0,0 +1,149 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Marvell 88Q2XXX automotive 100BASE-T1/1000BASE-T1 PHY driver + */ +#include <linux/ethtool_netlink.h> +#include <linux/marvell_phy.h> +#include <linux/phy.h> + +#define MARVELL_PHY_ID_88Q2110 0x002b0981 + +static int mv88q2xxx_soft_reset(struct phy_device *phydev) +{ + return phy_write_mmd(phydev, MDIO_MMD_PCS, + MDIO_PCS_1000BT1_CTRL, MDIO_PCS_1000BT1_CTRL_RESET); +} + +static int mv88q2xxx_read_link(struct phy_device *phydev) +{ + u16 ret1, ret2; + + /* The 88Q2XXX PHYs do not have the PMA/PMD status register available, + * therefore we need to read the link status from the vendor specific + * registers. + */ + if (phydev->speed == SPEED_1000) { + /* Read twice to clear the latched status */ + ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT); + ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, MDIO_PCS_1000BT1_STAT); + /* Read vendor specific Auto-Negotiation status register to get + * local and remote receiver status + */ + ret2 = phy_read_mmd(phydev, MDIO_MMD_AN, 0x8001); + } else { + /* Read vendor specific status registers, the registers are not + * documented but they can be found in the Software + * Initialization Guide + */ + ret1 = phy_read_mmd(phydev, MDIO_MMD_PCS, 0x8109); + ret2 = phy_read_mmd(phydev, MDIO_MMD_PCS, 0x8108); + } + + /* Check the link status according to Software Initialization Guide */ + return (0x0 != (ret1 & 0x0004)) && (0x0 != (ret2 & 0x3000)) ? 1 : 0; +} + +static int mv88q2xxx_read_status(struct phy_device *phydev) +{ + int ret; + + phydev->link = mv88q2xxx_read_link(phydev); + + ret = genphy_c45_read_pma(phydev); + if (ret) + return ret; + + return 0; +} + +static int mv88q2xxx_config_aneg(struct phy_device *phydev) +{ + int ret; + + ret = genphy_c45_config_aneg(phydev); + if (ret) + return ret; + + ret = mv88q2xxx_soft_reset(phydev); + if (ret) + return ret; + + return 0; +} + +static int mv88q2xxx_config_init(struct phy_device *phydev) +{ + int ret; + + /* The 88Q2XXX PHYs do have the extended ability register available, but + * register MDIO_PMA_EXTABLE where they should signalize it does not + * work according to specification. Therefore, we force it here. + */ + phydev->pma_extable = MDIO_PMA_EXTABLE_BT1; + + /* Read the current PHY configuration */ + ret = genphy_c45_read_pma(phydev); + if (ret) + return ret; + + return mv88q2xxx_config_aneg(phydev); +} + +static int mv88q2xxx_probe(struct phy_device *phydev) +{ + return 0; +} + +static int mv88q2xxxx_get_sqi(struct phy_device *phydev) +{ + u16 value; + + if (phydev->speed == SPEED_100) { + /* Read the SQI from the vendor specific receiver status + * register + */ + value = (phy_read_mmd(phydev, MDIO_MMD_PCS, 0x8230) >> 12) & 0x0F; + } else { + /* Read from vendor specific registers, they are not documented + * but can be found in the Software Initialization Guide. Only + * revisions >= A0 are supported. + */ + phy_modify_mmd(phydev, MDIO_MMD_PCS, 0xFC5D, 0x00FF, 0x00AC); + value = phy_read_mmd(phydev, MDIO_MMD_PCS, 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, + .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 88Q2XXX 100/1000BASE-T1 Automotive Ethernet PHY driver"); +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 | 149 ++++++++++++++++++++++++++++++ 3 files changed, 156 insertions(+) create mode 100644 drivers/net/phy/marvell-88q2xxx.c