Re: [PATCH v2] net: phy: add Marvell 88X2222 transceiver support
> > +/* switch line-side interface between 10GBase-R and 1GBase-X
> > + * according to speed */
> > +static void mv_update_interface(struct phy_device *phydev)
> > +{
> > + struct mv_data *priv = phydev->priv;
> > +
> > + if (phydev->speed == SPEED_1 &&
> > + priv->line_interface == PHY_INTERFACE_MODE_1000BASEX) {
> > + priv->line_interface = PHY_INTERFACE_MODE_10GBASER;
> > +
> > + phy_write_mmd(phydev, MDIO_MMD_VEND2, MV_PCS_CONFIG,
> > + MV_PCS_HOST_XAUI | MV_PCS_LINE_10GBR);
> > + mv_soft_reset(phydev);
> > + }
> > +
> > + if (phydev->speed == SPEED_1000 &&
> > + priv->line_interface == PHY_INTERFACE_MODE_10GBASER) {
> > + priv->line_interface = PHY_INTERFACE_MODE_1000BASEX;
> > +
> > + phy_write_mmd(phydev, MDIO_MMD_VEND2, MV_PCS_CONFIG,
> > + MV_PCS_HOST_XAUI | MV_PCS_LINE_1GBX_AN);
> > + mv_soft_reset(phydev);
> > + }
>
> Wouldn't it be better to have a single function to set the line
> interface, used by both this function and your sfp_module_insert
> function? I'm thinking something like:
>
> static int mv_set_line_interface(struct phy_device *phydev,
>phy_interface_t line_interface)
> {
> ...
> }
>
> and calling that from both these locations to configure the PHY for
> 10GBASE-R, 1000BASE-X and SGMII modes.
Agreed. This got me confused, wondering where the SGMII handling was.
Andrew
Re: [PATCH v2] net: phy: add Marvell 88X2222 transceiver support
Hi Ivan
> +static int sfp_module_insert(void *_priv, const struct sfp_eeprom_id *id)
> +{
> + struct phy_device *phydev = _priv;
> + struct device *dev = >mdio.dev;
> + struct mv_data *priv = phydev->priv;
> + phy_interface_t sfp_interface;
Reverse Christmas tree please. Which means you will need to move some
of the assignments to the body of the function. Please also drop the _
from _priv.
> +
> + __ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_supported) = { 0, };
> +
> + sfp_parse_support(phydev->sfp_bus, id, sfp_supported);
> + sfp_interface = sfp_select_interface(phydev->sfp_bus, sfp_supported);
> +
> + dev_info(dev, "%s SFP module inserted", phy_modes(sfp_interface));
> +
> + switch (sfp_interface) {
> + case PHY_INTERFACE_MODE_10GBASER:
> + phydev->speed = SPEED_1;
> + phy_write_mmd(phydev, MDIO_MMD_VEND2, MV_PCS_CONFIG,
> + MV_PCS_HOST_XAUI | MV_PCS_LINE_10GBR);
> + break;
> + case PHY_INTERFACE_MODE_1000BASEX:
> + phydev->speed = SPEED_1000;
> + phy_write_mmd(phydev, MDIO_MMD_VEND2, MV_PCS_CONFIG,
> + MV_PCS_HOST_XAUI | MV_PCS_LINE_1GBX_AN);
> + break;
> + case PHY_INTERFACE_MODE_SGMII:
> + phydev->speed = SPEED_1000;
SPEED_UNKNOWN might be better here, until auto negotiation has
completed and you then know if it is 10/100/1G.
> + phy_write_mmd(phydev, MDIO_MMD_VEND2, MV_PCS_CONFIG,
> + MV_PCS_HOST_XAUI | MV_PCS_LINE_SGMII_AN);
> + phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_1GBX_CTRL,
> +BMCR_SPEED1000 | BMCR_SPEED100, BMCR_SPEED1000);
> + break;
> + default:
> + dev_err(dev, "Incompatible SFP module inserted\n");
> +
> + return -EINVAL;
> + }
> +
> + linkmode_and(phydev->supported, priv->supported, sfp_supported);
> + priv->line_interface = sfp_interface;
> +
> + return mv_soft_reset(phydev);
> +}
> +
> +static void sfp_module_remove(void *_priv)
> +{
> + struct phy_device *phydev = _priv;
> + struct mv_data *priv = phydev->priv;
More reverse christmass tree.
> +
> + priv->line_interface = PHY_INTERFACE_MODE_NA;
> + linkmode_copy(phydev->supported, priv->supported);
> +}
> +
> +static int mv_config_aneg(struct phy_device *phydev)
> +{
> + /* Try 10G first */
> + if (mv_is_10g_capable(phydev)) {
> + phydev->speed = SPEED_1;
> + mv_update_interface(phydev);
> +
> + ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_10GBR_STAT_RT);
> + if (ret < 0)
> + return ret;
> +
> + if (ret & MDIO_STAT1_LSTATUS) {
> + phydev->autoneg = AUTONEG_DISABLE;
> +
> + return mv_disable_aneg(phydev);
> + }
> +
> + /* 10G link was not established, switch back to 1G
> + * and proceed with true autonegotiation */
> + phydev->speed = SPEED_1000;
> + mv_update_interface(phydev);
So you are assuming the cable is plugged in and the peer is ready to
go? Try 10G once, and then fall back to 1G? This does not seem very
reliable, and likely to cause confusion. It works sometimes, not
others. I'm not sure this is a good idea.
> +static void mv_remove(struct phy_device *phydev)
> +{
> + struct device *dev = >mdio.dev;
> + struct mv_data *priv = phydev->priv;
> +
> + if (priv)
> + devm_kfree(dev, priv);
Why can devm_kfree(). The point of devm_ is that it frees itself.
Andrew
Re: [PATCH v2] net: phy: add Marvell 88X2222 transceiver support
On Sat, Feb 20, 2021 at 11:53:04AM +, Russell King - ARM Linux admin wrote:
> On Sat, Feb 20, 2021 at 12:46:23PM +0300, Ivan Bornyakov wrote:
> > +
> > + switch (sfp_interface) {
> > + case PHY_INTERFACE_MODE_10GBASER:
> > + phydev->speed = SPEED_1;
> > + phy_write_mmd(phydev, MDIO_MMD_VEND2, MV_PCS_CONFIG,
> > + MV_PCS_HOST_XAUI | MV_PCS_LINE_10GBR);
> > + break;
> > + case PHY_INTERFACE_MODE_1000BASEX:
> > + phydev->speed = SPEED_1000;
> > + phy_write_mmd(phydev, MDIO_MMD_VEND2, MV_PCS_CONFIG,
> > + MV_PCS_HOST_XAUI | MV_PCS_LINE_1GBX_AN);
> > + break;
> > + case PHY_INTERFACE_MODE_SGMII:
> > + phydev->speed = SPEED_1000;
> > + phy_write_mmd(phydev, MDIO_MMD_VEND2, MV_PCS_CONFIG,
> > + MV_PCS_HOST_XAUI | MV_PCS_LINE_SGMII_AN);
> > + phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_1GBX_CTRL,
> > + BMCR_SPEED1000 | BMCR_SPEED100, BMCR_SPEED1000);
>
> Isn't this forcing 1000Mbit, but SGMII relies on AN for the slower
> speeds.
>
It was intended as default, but you have a good point, there is no need
for this, I can just trigger config_aneg() instead.
> > + break;
> > + default:
> > + dev_err(dev, "Incompatible SFP module inserted\n");
> > +
> > + return -EINVAL;
> > + }
>
> I don't think you should set phydev->speed in this function - apart
> from the rtnl lock, there is no other locking here, so this is fragile.
>
> > + linkmode_and(phydev->supported, priv->supported, sfp_supported);
>
> I don't think this is a good idea; phylink does not expect the supported
> mask to change, and I suspect _no_ network device expects it to change.
> One of the things that network drivers and phylink does is to adjust the
> supported mask for a PHY according to the capabilities of the network
> device. For example, if they don't support pause modes, or something
> else. Overriding it in this way has the possibility to re-introduce
> modes that the network driver does not support.
>
OK, but how can I exclude modes unsupported by inserted SFP?
Or I shouldn't exclude any at all?
> > +/* switch line-side interface between 10GBase-R and 1GBase-X
> > + * according to speed */
> > +static void mv_update_interface(struct phy_device *phydev)
> > +{
> > + struct mv_data *priv = phydev->priv;
> > +
> > + if (phydev->speed == SPEED_1 &&
> > + priv->line_interface == PHY_INTERFACE_MODE_1000BASEX) {
> > + priv->line_interface = PHY_INTERFACE_MODE_10GBASER;
> > +
> > + phy_write_mmd(phydev, MDIO_MMD_VEND2, MV_PCS_CONFIG,
> > + MV_PCS_HOST_XAUI | MV_PCS_LINE_10GBR);
> > + mv_soft_reset(phydev);
> > + }
> > +
> > + if (phydev->speed == SPEED_1000 &&
> > + priv->line_interface == PHY_INTERFACE_MODE_10GBASER) {
> > + priv->line_interface = PHY_INTERFACE_MODE_1000BASEX;
> > +
> > + phy_write_mmd(phydev, MDIO_MMD_VEND2, MV_PCS_CONFIG,
> > + MV_PCS_HOST_XAUI | MV_PCS_LINE_1GBX_AN);
> > + mv_soft_reset(phydev);
> > + }
>
> Wouldn't it be better to have a single function to set the line
> interface, used by both this function and your sfp_module_insert
> function? I'm thinking something like:
>
> static int mv_set_line_interface(struct phy_device *phydev,
>phy_interface_t line_interface)
> {
> ...
> }
>
> and calling that from both these locations to configure the PHY for
> 10GBASE-R, 1000BASE-X and SGMII modes.
>
I'll think about it, thanks.
> > +
> > +static int mv_config_aneg(struct phy_device *phydev)
> > +{
> > + struct mv_data *priv = phydev->priv;
> > + int ret, adv;
> > +
> > + /* SFP is not present, do nothing */
> > + if (priv->line_interface == PHY_INTERFACE_MODE_NA)
> > + return 0;
> > +
> > + if (phydev->autoneg == AUTONEG_DISABLE ||
> > + phydev->speed == SPEED_1) {
> > + if (phydev->speed == SPEED_1 &&
> > + !mv_is_10g_capable(phydev))
> > + return -EINVAL;
> > +
> > + if (priv->line_interface == PHY_INTERFACE_MODE_SGMII) {
> > + ret = mv_set_sgmii_speed(phydev);
> > + if (ret < 0)
> > + return ret;
> > + } else {
> > + mv_update_interface(phydev);
> > + }
> > +
> > + return mv_disable_aneg(phydev);
> > + }
> > +
> > + /* Try 10G first */
> > + if (mv_is_10g_capable(phydev)) {
> > + phydev->speed = SPEED_1;
> > + mv_update_interface(phydev);
> > +
> > + ret = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_10GBR_STAT_RT);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (ret & MDIO_STAT1_LSTATUS) {
> > +
Re: [PATCH v2] net: phy: add Marvell 88X2222 transceiver support
On Sat, Feb 20, 2021 at 12:46:23PM +0300, Ivan Bornyakov wrote:
> Add basic support for the Marvell 88X multi-speed ethernet
> transceiver.
>
> This PHY provides data transmission over fiber-optic as well as Twinax
> copper links. The 88X supports 2 ports of 10GBase-R and 1000Base-X
> on the line-side interface. The host-side interface supports 4 ports of
> 10GBase-R, RXAUI, 1000Base-X and 2 ports of XAUI.
>
> This driver, however, supports only XAUI on the host-side and
> 1000Base-X/10GBase-R on the line-side, for now. The SGMII is also
> supported over 1000Base-X. Interrupts are not supported.
>
> Internal registers access compliant with the Clause 45 specification.
>
> Signed-off-by: Ivan Bornyakov
> ---
> drivers/net/phy/Kconfig | 6 +
> drivers/net/phy/Makefile | 1 +
> drivers/net/phy/marvell-88x.c | 510 ++
> include/linux/marvell_phy.h | 1 +
> 4 files changed, 518 insertions(+)
> create mode 100644 drivers/net/phy/marvell-88x.c
>
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 698bea312adc..a615b3660b05 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -201,6 +201,12 @@ config MARVELL_10G_PHY
> help
> Support for the Marvell Alaska MV88X3310 and compatible PHYs.
>
> +config MARVELL_88X_PHY
> + tristate "Marvell 88X PHY"
> + help
> + Support for the Marvell 88X Dual-port Multi-speed Ethernet
> + Transceiver.
> +
> config MICREL_PHY
> tristate "Micrel PHYs"
> help
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index a13e402074cf..de683e3abe63 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -63,6 +63,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_88X_PHY)+= marvell-88x.o
> obj-$(CONFIG_MESON_GXL_PHY) += meson-gxl.o
> obj-$(CONFIG_MICREL_KS8995MA)+= spi_ks8995.o
> obj-$(CONFIG_MICREL_PHY) += micrel.o
> diff --git a/drivers/net/phy/marvell-88x.c
> b/drivers/net/phy/marvell-88x.c
> new file mode 100644
> index ..5f1b6185e272
> --- /dev/null
> +++ b/drivers/net/phy/marvell-88x.c
> @@ -0,0 +1,510 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Marvell 88x dual-port multi-speed ethernet transceiver.
> + *
> + * Supports:
> + * XAUI on the host side.
> + * 1000Base-X or 10GBase-R on the line side.
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/* Port PCS Configuration */
> +#define MV_PCS_CONFIG 0xF002
> +#define MV_PCS_HOST_XAUI0x73
> +#define MV_PCS_LINE_10GBR (0x71 << 8)
> +#define MV_PCS_LINE_1GBX_AN (0x7B << 8)
> +#define MV_PCS_LINE_SGMII_AN(0x7F << 8)
> +
> +/* Port Reset and Power Down */
> +#define MV_PORT_RST 0xF003
> +#define MV_LINE_RST_SW BIT(15)
> +#define MV_HOST_RST_SW BIT(7)
> +#define MV_PORT_RST_SW (MV_LINE_RST_SW | MV_HOST_RST_SW)
> +
> +/* 10GBASE-R PCS Real Time Status Register */
> +#define MV_10GBR_STAT_RT0x8002
> +
> +/* 1000Base-X/SGMII Control Register */
> +#define MV_1GBX_CTRL(0x2000 + MII_BMCR)
> +
> +/* 1000BASE-X/SGMII Status Register */
> +#define MV_1GBX_STAT(0x2000 + MII_BMSR)
> +
> +/* 1000Base-X Auto-Negotiation Advertisement Register */
> +#define MV_1GBX_ADVERTISE (0x2000 + MII_ADVERTISE)
> +
> +/* 1000Base-X PHY Specific Status Register */
> +#define MV_1GBX_PHY_STAT0xA003
> +#define MV_1GBX_PHY_STAT_AN_RESOLVEDBIT(11)
> +#define MV_1GBX_PHY_STAT_DUPLEX BIT(13)
> +#define MV_1GBX_PHY_STAT_SPEED100 BIT(14)
> +#define MV_1GBX_PHY_STAT_SPEED1000 BIT(15)
> +
> +struct mv_data {
> + phy_interface_t line_interface;
> + __ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
> +};
> +
> +/* SFI PMA transmit enable */
> +static int mv_tx_enable(struct phy_device *phydev)
> +{
> + return phy_clear_bits_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_TXDIS,
> + MDIO_PMD_TXDIS_GLOBAL);
> +}
> +
> +/* SFI PMA transmit disable */
> +static int mv_tx_disable(struct phy_device *phydev)
> +{
> + return phy_set_bits_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_TXDIS,
> + MDIO_PMD_TXDIS_GLOBAL);
> +}
> +
> +static int mv_soft_reset(struct phy_device *phydev)
> +{
> + int val, ret;
> +
> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, MV_PORT_RST,
> + MV_PORT_RST_SW);
> + if (ret < 0)
> + return ret;
> +
> + return phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND2, MV_PORT_RST,
>
[PATCH v2] net: phy: add Marvell 88X2222 transceiver support
Add basic support for the Marvell 88X multi-speed ethernet
transceiver.
This PHY provides data transmission over fiber-optic as well as Twinax
copper links. The 88X supports 2 ports of 10GBase-R and 1000Base-X
on the line-side interface. The host-side interface supports 4 ports of
10GBase-R, RXAUI, 1000Base-X and 2 ports of XAUI.
This driver, however, supports only XAUI on the host-side and
1000Base-X/10GBase-R on the line-side, for now. The SGMII is also
supported over 1000Base-X. Interrupts are not supported.
Internal registers access compliant with the Clause 45 specification.
Signed-off-by: Ivan Bornyakov
---
drivers/net/phy/Kconfig | 6 +
drivers/net/phy/Makefile | 1 +
drivers/net/phy/marvell-88x.c | 510 ++
include/linux/marvell_phy.h | 1 +
4 files changed, 518 insertions(+)
create mode 100644 drivers/net/phy/marvell-88x.c
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 698bea312adc..a615b3660b05 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -201,6 +201,12 @@ config MARVELL_10G_PHY
help
Support for the Marvell Alaska MV88X3310 and compatible PHYs.
+config MARVELL_88X_PHY
+ tristate "Marvell 88X PHY"
+ help
+ Support for the Marvell 88X Dual-port Multi-speed Ethernet
+ Transceiver.
+
config MICREL_PHY
tristate "Micrel PHYs"
help
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index a13e402074cf..de683e3abe63 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -63,6 +63,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_88X_PHY) += marvell-88x.o
obj-$(CONFIG_MESON_GXL_PHY)+= meson-gxl.o
obj-$(CONFIG_MICREL_KS8995MA) += spi_ks8995.o
obj-$(CONFIG_MICREL_PHY) += micrel.o
diff --git a/drivers/net/phy/marvell-88x.c
b/drivers/net/phy/marvell-88x.c
new file mode 100644
index ..5f1b6185e272
--- /dev/null
+++ b/drivers/net/phy/marvell-88x.c
@@ -0,0 +1,510 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Marvell 88x dual-port multi-speed ethernet transceiver.
+ *
+ * Supports:
+ * XAUI on the host side.
+ * 1000Base-X or 10GBase-R on the line side.
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* Port PCS Configuration */
+#defineMV_PCS_CONFIG 0xF002
+#defineMV_PCS_HOST_XAUI0x73
+#defineMV_PCS_LINE_10GBR (0x71 << 8)
+#defineMV_PCS_LINE_1GBX_AN (0x7B << 8)
+#defineMV_PCS_LINE_SGMII_AN(0x7F << 8)
+
+/* Port Reset and Power Down */
+#defineMV_PORT_RST 0xF003
+#defineMV_LINE_RST_SW BIT(15)
+#defineMV_HOST_RST_SW BIT(7)
+#defineMV_PORT_RST_SW (MV_LINE_RST_SW | MV_HOST_RST_SW)
+
+/* 10GBASE-R PCS Real Time Status Register */
+#defineMV_10GBR_STAT_RT0x8002
+
+/* 1000Base-X/SGMII Control Register */
+#defineMV_1GBX_CTRL(0x2000 + MII_BMCR)
+
+/* 1000BASE-X/SGMII Status Register */
+#defineMV_1GBX_STAT(0x2000 + MII_BMSR)
+
+/* 1000Base-X Auto-Negotiation Advertisement Register */
+#defineMV_1GBX_ADVERTISE (0x2000 + MII_ADVERTISE)
+
+/* 1000Base-X PHY Specific Status Register */
+#defineMV_1GBX_PHY_STAT0xA003
+#defineMV_1GBX_PHY_STAT_AN_RESOLVEDBIT(11)
+#defineMV_1GBX_PHY_STAT_DUPLEX BIT(13)
+#defineMV_1GBX_PHY_STAT_SPEED100 BIT(14)
+#defineMV_1GBX_PHY_STAT_SPEED1000 BIT(15)
+
+struct mv_data {
+ phy_interface_t line_interface;
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
+};
+
+/* SFI PMA transmit enable */
+static int mv_tx_enable(struct phy_device *phydev)
+{
+ return phy_clear_bits_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_TXDIS,
+ MDIO_PMD_TXDIS_GLOBAL);
+}
+
+/* SFI PMA transmit disable */
+static int mv_tx_disable(struct phy_device *phydev)
+{
+ return phy_set_bits_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_PMA_TXDIS,
+ MDIO_PMD_TXDIS_GLOBAL);
+}
+
+static int mv_soft_reset(struct phy_device *phydev)
+{
+ int val, ret;
+
+ ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, MV_PORT_RST,
+ MV_PORT_RST_SW);
+ if (ret < 0)
+ return ret;
+
+ return phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND2, MV_PORT_RST,
+val, !(val & MV_PORT_RST_SW),
+5000, 100, true);
+}
+
+static int sfp_module_insert(void *_priv, const struct sfp_eeprom_id *id)
+{
+ struct phy_device *phydev = _priv;
+ struct device
